From: Andy Allan <git@gravitystorm.co.uk>
Date: Wed, 23 Jun 2021 15:26:50 +0000 (+0100)
Subject: Use hash-based flash objects to render complex flash messages
X-Git-Tag: live~2100^2~5
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/24f6aeda6a1b657d68a4e66f3a34d14ef408d652?ds=sidebyside

Use hash-based flash objects to render complex flash messages

Since flash objects can only be String, Hash or Array (notably excluding SafeBuffers), then this approach is necessary to render complex html in a safe manner.

Each local can be treated as an (unsafe) string, and therefore escaped normally when rendered into the template. The template (and translation strings) can
contain html since they are no longer stored in the flash as a plain string.

Fixes #3215
---

diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb
index f9de54df0..372ac2a70 100644
--- a/app/controllers/confirmations_controller.rb
+++ b/app/controllers/confirmations_controller.rb
@@ -66,7 +66,7 @@ class ConfirmationsController < ApplicationController
       flash[:error] = t "confirmations.confirm_resend.failure", :name => params[:display_name]
     else
       UserMailer.signup_confirm(user, user.tokens.create).deliver_later
-      flash[:notice] = t "confirmations.confirm_resend.success_html", :email => user.email, :sender => Settings.email_from
+      flash[:notice] = { :partial => "confirmations/resend_success_flash", :locals => { :email => user.email, :sender => Settings.email_from } }
     end
 
     redirect_to login_path
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index ae90d980e..faf538fa9 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -68,4 +68,14 @@ module ApplicationHelper
 
     data
   end
+
+  # If the flash is a hash, then it will be a partial with a hash of locals, so we can call `render` on that
+  # This allows us to render html into a flash message in a safe manner.
+  def render_flash(flash)
+    if flash.is_a?(Hash)
+      render flash.with_indifferent_access
+    else
+      flash
+    end
+  end
 end
diff --git a/app/views/confirmations/_resend_success_flash.html.erb b/app/views/confirmations/_resend_success_flash.html.erb
new file mode 100644
index 000000000..1895de761
--- /dev/null
+++ b/app/views/confirmations/_resend_success_flash.html.erb
@@ -0,0 +1 @@
+<%= t "confirmations.confirm_resend.success_html", :email => email, :sender => sender %>
diff --git a/app/views/layouts/_flash.html.erb b/app/views/layouts/_flash.html.erb
index 0f39c4a47..f8a89d6df 100644
--- a/app/views/layouts/_flash.html.erb
+++ b/app/views/layouts/_flash.html.erb
@@ -4,7 +4,7 @@
       <source srcset="<%= image_path "notice.svg" %>" type="image/svg+xml" />
       <%= image_tag("notice.png", :srcset => image_path("notice.svg"), :class => "small_icon", :border => 0) %>
     </picture>
-    <div class="message"><%= flash[:error] %></div>
+    <div class="message"><%= render_flash(flash[:error]) %></div>
   </div>
 <% end %>
 
@@ -14,7 +14,7 @@
       <source srcset="<%= image_path "notice.svg" %>" type="image/svg+xml"></source>
       <%= image_tag("notice.png", :srcset => image_path("notice.svg"), :class => "small_icon", :border => 0) %>
     </picture>
-    <div class="message"><%= flash[:warning] %></div>
+    <div class="message"><%= render_flash(flash[:warning]) %></div>
   </div>
 <% end %>
 
@@ -24,6 +24,6 @@
       <source srcset="<%= image_path "notice.svg" %>" type="image/svg+xml"></source>
       <%= image_tag("notice.png", :srcset => image_path("notice.svg"), :class => "small_icon", :border => 0) %>
     </picture>
-    <div class="message"><%= flash[:notice] %></div>
+    <div class="message"><%= render_flash(flash[:notice]) %></div>
   </div>
 <% end %>
diff --git a/test/controllers/confirmations_controller_test.rb b/test/controllers/confirmations_controller_test.rb
index bae0158cd..f583e5e39 100644
--- a/test/controllers/confirmations_controller_test.rb
+++ b/test/controllers/confirmations_controller_test.rb
@@ -211,7 +211,8 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
 
     assert_response :redirect
     assert_redirected_to login_path
-    assert_match(/sent a new confirmation/, flash[:notice])
+    assert_equal("confirmations/resend_success_flash", flash[:notice][:partial])
+    assert_equal({ :email => user.email, :sender => Settings.email_from }, flash[:notice][:locals])
 
     email = ActionMailer::Base.deliveries.last
 
diff --git a/test/system/confirmation_resend.rb b/test/system/confirmation_resend.rb
new file mode 100644
index 000000000..69103e67c
--- /dev/null
+++ b/test/system/confirmation_resend.rb
@@ -0,0 +1,26 @@
+require "application_system_test_case"
+
+class ConfirmationResendSystemTest < ApplicationSystemTestCase
+  def setup
+    @user = build(:user)
+    visit user_new_path
+
+    fill_in "Email", :with => @user.email
+    fill_in "Email Confirmation", :with => @user.email
+    fill_in "Display Name", :with => @user.display_name
+    fill_in "Password", :with => "testtest"
+    fill_in "Confirm Password", :with => "testtest"
+    click_button "Sign Up"
+
+    check "I have read and agree to the above contributor terms"
+    check "I have read and agree to the Terms of Use"
+    click_button "Continue"
+  end
+
+  test "flash message should not contain raw html" do
+    visit user_confirm_resend_path(@user)
+
+    assert page.has_content?("sent a new confirmation")
+    assert_not page.has_content?("<br />")
+  end
+end