Skip to content

Commit 6216eaa

Browse files
committed
Fix XSS vulnerability in flash banner
We were making the body of any flash message as HTML safe, but that may not always be the case. There's only one place in the code where we relied on this functionality, and it was to format the batch number in a particular way. For now, it's much easier to remove that formatting. We can't know if the input is HTML safe as it's stored in the session and we lose that information (even if `html_safe` is called). Jira-Issue: MAV-6743
1 parent ee336d2 commit 6216eaa

3 files changed

Lines changed: 33 additions & 4 deletions

File tree

app/components/app_flash_message_component.html.erb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,11 @@
55
success: success?,
66
) do |notification_banner| %>
77
<% notification_banner.with_heading(
8-
text: heading.html_safe,
8+
text: heading,
99
link_text: heading_link_text,
1010
link_href: heading_link_href,
1111
) if heading.present? %>
1212
<% if body.present? %>
13-
<p><%= body.html_safe %></p>
13+
<p><%= body %></p>
1414
<% end %>
1515
<% end %>

app/controllers/batches_controller.rb

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,7 @@ def create
2626
if expiry_validator.date_params_valid? && @form.save
2727
redirect_to vaccines_path,
2828
flash: {
29-
success:
30-
"Batch <span class=\"nhsuk-u-text-break-word\">#{batch.number}</span> added".html_safe
29+
success: "Batch #{batch.number} added"
3130
}
3231
else
3332
@form.expiry = expiry_validator.date_params_as_struct

spec/components/app_flash_message_component_spec.rb

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,36 @@
8989
include("Success body")
9090
)
9191
end
92+
93+
context "when heading contains HTML but is not marked as safe" do
94+
before { flash[:success][:heading] = "<p>HTML</p>" }
95+
96+
it "doesn't render the heading as HTML" do
97+
expect(
98+
rendered.css(".nhsuk-notification-banner__content").inner_html
99+
).to(include("&lt;p&gt;HTML&lt;/p&gt;"))
100+
end
101+
end
102+
103+
context "when body contains HTML that is marked as safe" do
104+
before { flash[:success][:body] = "<p>HTML</p>".html_safe }
105+
106+
it "doesn't render the body as HTML" do
107+
expect(
108+
rendered.css(".nhsuk-notification-banner__content").inner_html
109+
).to(include("<p>HTML</p>"))
110+
end
111+
end
112+
113+
context "when body contains HTML but is not marked as safe" do
114+
before { flash[:success][:body] = "<p>HTML</p>" }
115+
116+
it "doesn't render the body as HTML" do
117+
expect(
118+
rendered.css(".nhsuk-notification-banner__content").inner_html
119+
).to(include("&lt;p&gt;HTML&lt;/p&gt;"))
120+
end
121+
end
92122
end
93123

94124
describe "the role attribute" do

0 commit comments

Comments
 (0)