Merge pull request #6642 from NHSDigital/fix-vulnerabilities

Fix a couple of security vulnerabilities

Author: thomasleese

app/components/app_flash_message_component.html.erb

@@ -5,11 +5,11 @@
      success: success?,
    ) do |notification_banner| %>
   <% notification_banner.with_heading(
-       text: heading.html_safe,
+       text: heading,
        link_text: heading_link_text,
        link_href: heading_link_href,
      ) if heading.present? %>
   <% if body.present? %>
-    <p><%= body.html_safe %></p>
+    <p><%= body %></p>
   <% end %>
 <% end %>

app/controllers/batches_controller.rb

@@ -26,8 +26,7 @@ def create
     if expiry_validator.date_params_valid? && @form.save
       redirect_to vaccines_path,
                   flash: {
-                    success:
-                      "Batch <span class=\"nhsuk-u-text-break-word\">#{batch.number}</span> added".html_safe
+                    success: "Batch #{batch.number} added"
                   }
     else
       @form.expiry = expiry_validator.date_params_as_struct

app/controllers/concerns/authentication_concern.rb

@@ -9,7 +9,7 @@ module AuthenticationConcern
     # only return true if the given URL is part of this domain (relative or absolute)
     # or part of the mavis reporting app, or localhost (to support local dev)
     def is_valid_redirect?(url)
-      url.start_with?("/") || url.start_with?(request.base_url) ||
+      !url_from(url).nil? ||
         url.start_with?(
           Settings.reporting_api.client_app.root_url || "http://localhost"
         )

spec/components/app_flash_message_component_spec.rb

@@ -89,6 +89,36 @@
         include("Success body")
       )
     end
+
+    context "when heading contains HTML but is not marked as safe" do
+      before { flash[:success][:heading] = "<p>HTML</p>" }
+
+      it "doesn't render the heading as HTML" do
+        expect(
+          rendered.css(".nhsuk-notification-banner__content").inner_html
+        ).to(include("&lt;p&gt;HTML&lt;/p&gt;"))
+      end
+    end
+
+    context "when body contains HTML that is marked as safe" do
+      before { flash[:success][:body] = "<p>HTML</p>".html_safe }
+
+      it "doesn't render the body as HTML" do
+        expect(
+          rendered.css(".nhsuk-notification-banner__content").inner_html
+        ).to(include("<p>HTML</p>"))
+      end
+    end
+
+    context "when body contains HTML but is not marked as safe" do
+      before { flash[:success][:body] = "<p>HTML</p>" }
+
+      it "doesn't render the body as HTML" do
+        expect(
+          rendered.css(".nhsuk-notification-banner__content").inner_html
+        ).to(include("&lt;p&gt;HTML&lt;/p&gt;"))
+      end
+    end
   end
 
   describe "the role attribute" do

spec/controllers/concerns/authentication_concern_spec.rb

@@ -29,7 +29,7 @@ def current_user
       .new(request: mock_request)
   end
 
-  describe "set_user_cis2_info" do
+  describe "#set_user_cis2_info" do
     let(:user) { build(:user, cis2_info: nil) }
 
     context "when cis2 is disabled" do
@@ -53,9 +53,6 @@ def current_user
     end
   end
 
-  # The commented out code block you provided is a pending RSpec example group for a method called
-  # `#add_auth_code_to`. This method is expected to find or generate a `OneTimeToken` for a given user with
-  # the `cis2_info` from the current session. The example group contains two contexts:
   describe "#add_auth_code_to" do
     let(:user) { create(:user) }
     let(:token) do
@@ -100,7 +97,7 @@ def current_user
     end
   end
 
-  describe "reporting_app_redirect_uri_with_auth_code_for" do
+  describe "#reporting_app_redirect_uri_with_auth_code_for" do
     let(:session_cis2_info) { { "team_workgroup" => "r1l" } }
     let(:token) do
       build(:reporting_api_one_time_token, user: user, token: "mytoken")

spec/features/user_cis2_authentication_from_redirect_spec.rb

@@ -22,14 +22,22 @@
     and_the_return_url_has_a_token_param_added_to_it
   end
 
-  scenario "someone has supplied their own external redirect url" do
+  scenario "someone has supplied their own absolute redirect url" do
     given_a_test_team_is_setup_in_mavis_and_cis2
     when_i_go_to_the_start_page_with_a_redirect_uri_param_that_does_not_match_the_reporting_app
 
     when_i_click_the_cis2_login_button
     then_i_see_the_dashboard
   end
 
+  scenario "someone has supplied their own schema-relative redirect url" do
+    given_a_test_team_is_setup_in_mavis_and_cis2
+    when_i_go_to_the_start_page_with_a_redirect_uri_param_that_is_schema_relative
+
+    when_i_click_the_cis2_login_button
+    then_i_see_the_dashboard
+  end
+
   def given_a_test_team_is_setup_in_mavis_and_cis2
     @user = create(:user, uid: "123")
     @team = create(:team, users: [@user])
@@ -61,12 +69,14 @@ def when_i_go_to_the_start_page_with_a_redirect_uri_param_that_matches_the_repor
     visit [start_path, "redirect_uri=#{uri}"].join("?")
   end
 
-  def redirect_elsewhere_url
-    "https://some.example.com/redirect/elsewhere"
+  def when_i_go_to_the_start_page_with_a_redirect_uri_param_that_is_schema_relative
+    uri =
+      URI.encode_uri_component("https://some.example.com/redirect/elsewhere")
+    visit [start_path, "redirect_uri=#{uri}"].join("?")
   end
 
   def when_i_go_to_the_start_page_with_a_redirect_uri_param_that_does_not_match_the_reporting_app
-    uri = URI.encode_uri_component(redirect_elsewhere_url)
+    uri = URI.encode_uri_component("//some.example.com/redirect/elsewhere")
     visit [start_path, "redirect_uri=#{uri}"].join("?")
   end