From 99e537859a813edbcbf4ce94eae69f33f6361e22 Mon Sep 17 00:00:00 2001
From: Tom Hughes <tom@compton.nu>
Date: Fri, 13 Nov 2020 15:13:37 +0000
Subject: [PATCH] Fix HTML escaping issues with user role icons

---
 .rubocop_todo.yml                      |  6 ------
 app/helpers/user_roles_helper.rb       |  4 +---
 app/views/users/show.html.erb          |  2 +-
 test/helpers/user_roles_helper_test.rb | 12 ++++++------
 4 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 406a13265..c1060cbe2 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -172,9 +172,3 @@ Style/FrozenStringLiteralComment:
 # Configuration parameters: Strict.
 Style/NumericLiterals:
   MinDigits: 11
-
-# Offense count: 19
-# Cop supports --auto-correct.
-Style/StringConcatenation:
-  Exclude:
-    - 'test/helpers/user_roles_helper_test.rb'
diff --git a/app/helpers/user_roles_helper.rb b/app/helpers/user_roles_helper.rb
index 384fb7280..79e7cc012 100644
--- a/app/helpers/user_roles_helper.rb
+++ b/app/helpers/user_roles_helper.rb
@@ -1,8 +1,6 @@
 module UserRolesHelper
   def role_icons(user)
-    UserRole::ALL_ROLES.reduce("".html_safe) do |acc, elem|
-      "#{acc} #{role_icon(user, elem)}"
-    end
+    safe_join(UserRole::ALL_ROLES.collect { |role| role_icon(user, role) }.compact, " ")
   end
 
   def role_icon(user, role)
diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb
index 440d68874..694f561b2 100644
--- a/app/views/users/show.html.erb
+++ b/app/views/users/show.html.erb
@@ -2,7 +2,7 @@
   <div id='userinformation'>
     <%= user_image @user %>
     <div class='userinformation-inner'>
-      <h1><%= @user.display_name %><%= role_icons(@user) %></h1>
+      <h1><%= @user.display_name %> <%= role_icons(@user) %></h1>
       <% if current_user and @user.id == current_user.id %>
         <!-- Displaying user's own profile page to themself -->
         <ul class='secondary-actions clearfix'>
diff --git a/test/helpers/user_roles_helper_test.rb b/test/helpers/user_roles_helper_test.rb
index 26c303cfa..dfd790a0b 100644
--- a/test/helpers/user_roles_helper_test.rb
+++ b/test/helpers/user_roles_helper_test.rb
@@ -51,10 +51,10 @@ class UserRolesHelperTest < ActionView::TestCase
     self.current_user = create(:user)
 
     icons = role_icons(current_user)
-    assert_dom_equal "  ", icons
+    assert_dom_equal "", icons
 
     icons = role_icons(create(:moderator_user))
-    expected = "  " + <<~HTML.delete("\n")
+    expected = <<~HTML.delete("\n")
       <picture>
       <source srcset="/images/roles/moderator.svg" type="image/svg+xml" />
       <img srcset="/images/roles/moderator.svg" border="0" alt="This user is a moderator" title="This user is a moderator" src="/images/roles/moderator.png" width="20" height="20" />
@@ -63,7 +63,7 @@ class UserRolesHelperTest < ActionView::TestCase
     assert_dom_equal expected, icons
 
     icons = role_icons(create(:super_user))
-    expected = " " + <<~HTML.delete("\n")
+    expected = <<~HTML.delete("\n")
       <picture>
       <source srcset="/images/roles/administrator.svg" type="image/svg+xml" />
       <img srcset="/images/roles/administrator.svg" border="0" alt="This user is an administrator" title="This user is an administrator" src="/images/roles/administrator.png" width="20" height="20" />
@@ -81,7 +81,7 @@ class UserRolesHelperTest < ActionView::TestCase
 
     user = create(:user)
     icons = role_icons(user)
-    expected = " " + <<~HTML.delete("\n")
+    expected = <<~HTML.delete("\n")
       <a confirm="Are you sure you want to grant the role `administrator&#39; to the user `#{user.display_name}&#39;?" rel="nofollow" data-method="post" href="/user/#{ERB::Util.u(user.display_name)}/role/administrator/grant">
       <picture>
       <source srcset="/images/roles/blank_administrator.svg" type="image/svg+xml" />
@@ -99,7 +99,7 @@ class UserRolesHelperTest < ActionView::TestCase
 
     moderator_user = create(:moderator_user)
     icons = role_icons(moderator_user)
-    expected = " " + <<~HTML.delete("\n")
+    expected = <<~HTML.delete("\n")
       <a confirm="Are you sure you want to grant the role `administrator&#39; to the user `#{moderator_user.display_name}&#39;?" rel="nofollow" data-method="post" href="/user/#{ERB::Util.u(moderator_user.display_name)}/role/administrator/grant">
       <picture>
       <source srcset="/images/roles/blank_administrator.svg" type="image/svg+xml" />
@@ -117,7 +117,7 @@ class UserRolesHelperTest < ActionView::TestCase
 
     super_user = create(:super_user)
     icons = role_icons(super_user)
-    expected = " " + <<~HTML.delete("\n")
+    expected = <<~HTML.delete("\n")
       <a confirm="Are you sure you want to revoke the role `administrator&#39; from the user `#{super_user.display_name}&#39;?" rel="nofollow" data-method="post" href="/user/#{ERB::Util.u(super_user.display_name)}/role/administrator/revoke">
       <picture>
       <source srcset="/images/roles/administrator.svg" type="image/svg+xml" />
-- 
2.39.5