From db1ccb18c7bd48fdd5ab63d6ad0259974a0a557a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Tue, 1 Oct 2019 18:50:17 +0200
Subject: [PATCH] Use `safe_join` instead of `html_safe`

The name `html_safe` is very confusing, and many developers (including
me a few years ago) think what that method does is convert the HTML
contents to safe content. It's actually quite the opposite: it marks the
string as safe, so the HTML inside it isn't stripped out by Rails.

In some cases we were marking strings as safe because we wanted to add
some HTML. However, it meant the whole string was considered safe, and
not just the contents which were under our control.

In particular, some translations added by admins to the database or
through crowding were marked as safe, when it wasn't necessarily the
case.

Although AFAIK crowdin checks for potential cross-site scripting
attacks, it's a good practice to sanitize parts of a string potentially
out of our control before marking the string as HTML safe.
---
 app/helpers/budget_investments_helper.rb |  2 +-
 app/helpers/documents_helper.rb          | 11 ++++++-----
 app/helpers/translatable_form_helper.rb  |  4 ++--
 spec/features/xss_spec.rb                | 18 ++++++++++++++++++
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/app/helpers/budget_investments_helper.rb b/app/helpers/budget_investments_helper.rb
index bcfdf0f49..1e6d8405e 100644
--- a/app/helpers/budget_investments_helper.rb
+++ b/app/helpers/budget_investments_helper.rb
@@ -10,7 +10,7 @@ module BudgetInvestmentsHelper
     translation = t("admin.budget_investments.index.list.#{column}")
 
     link_to(
-      "#{translation} <span class='icon-sortable #{icon}'></span>".html_safe,
+      safe_join([translation, content_tag(:span, "", class: "icon-sortable #{icon}")]),
       admin_budget_budget_investments_path(sort_by: column, direction: direction)
     )
   end
diff --git a/app/helpers/documents_helper.rb b/app/helpers/documents_helper.rb
index a096f7346..1386796bb 100644
--- a/app/helpers/documents_helper.rb
+++ b/app/helpers/documents_helper.rb
@@ -50,10 +50,11 @@ module DocumentsHelper
   end
 
   def document_item_link(document)
-    link_to "#{document.title} <small>(#{document.humanized_content_type} | \
-             #{number_to_human_size(document.attachment_file_size)}</small>)".html_safe,
-             document.attachment.url,
-             target: "_blank",
-             title: t("shared.target_blank")
+    info_text = "#{document.humanized_content_type} | #{number_to_human_size(document.attachment_file_size)}"
+
+    link_to safe_join([document.title, content_tag(:small, "(#{info_text})")], " "),
+            document.attachment.url,
+            target: "_blank",
+            title: t("shared.target_blank")
   end
 end
diff --git a/app/helpers/translatable_form_helper.rb b/app/helpers/translatable_form_helper.rb
index 92eff3d33..78ce9566d 100644
--- a/app/helpers/translatable_form_helper.rb
+++ b/app/helpers/translatable_form_helper.rb
@@ -26,9 +26,9 @@ module TranslatableFormHelper
       visible_locales.map do |locale|
         @translations[locale] = translation_for(locale)
       end
-      visible_locales.map do |locale|
+      safe_join(visible_locales.map do |locale|
         Globalize.with_locale(locale) { fields_for_locale(locale, &block) }
-      end.join.html_safe
+      end)
     end
 
     private
diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb
index c99fc6c76..9519c4766 100644
--- a/spec/features/xss_spec.rb
+++ b/spec/features/xss_spec.rb
@@ -12,4 +12,22 @@ describe "Cross-Site Scripting protection", :js do
 
     expect(page.text).not_to be_empty
   end
+
+  scenario "document title" do
+    process = create(:legislation_process)
+    create(:document, documentable: process, title: attack_code)
+
+    visit legislation_process_path(process)
+
+    expect(page.text).not_to be_empty
+  end
+
+  scenario "hacked translations" do
+    I18nContent.create(key: "admin.budget_investments.index.list.title", value: attack_code)
+
+    login_as(create(:administrator).user)
+    visit admin_budget_budget_investments_path(create(:budget_investment).budget)
+
+    expect(page.text).not_to be_empty
+  end
 end