From 31c2379a4ef3978330d122b8296ca718a089b099 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Fri, 11 Oct 2019 13:45:41 +0200
Subject: [PATCH 1/3] Don't sanitize `<span>` tags in HTML attributes

Doing so will cause the `<span>` tag to be rendered in the document,
instead of being rendered as a data attribute.
---
 app/views/shared/_common_globalize_locales.html.erb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/views/shared/_common_globalize_locales.html.erb b/app/views/shared/_common_globalize_locales.html.erb
index e48aef1e5..7eed43d55 100644
--- a/app/views/shared/_common_globalize_locales.html.erb
+++ b/app/views/shared/_common_globalize_locales.html.erb
@@ -1,7 +1,7 @@
 <div class="row globalize-languages column padding-top <%= highlight_translation_html_class %>"
-     data-zero-languages-description="<%= sanitize(t("shared.translations.languages_in_use", count: 0)) %>"
-     data-one-languages-description="<%= sanitize(t("shared.translations.languages_in_use", count: 1)) %>"
-     data-other-languages-description="<%= sanitize(t("shared.translations.languages_in_use", count: 2)) %>">
+     data-zero-languages-description="<%= t("shared.translations.languages_in_use", count: 0) %>"
+     data-one-languages-description="<%= t("shared.translations.languages_in_use", count: 1) %>"
+     data-other-languages-description="<%= t("shared.translations.languages_in_use", count: 2) %>">
   <div class="small-6 large-3 column">
     <span class="small">
       <strong class="js-languages-description"><%= selected_languages_description(resource) %></strong>

From d61e8cb6a6f59b00f0c6a47dfd4f2b5317c7057e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Fri, 18 Oct 2019 22:00:27 +0200
Subject: [PATCH 2/3] Use text() instead of html()

Using html() makes it possible to insert <script> tags in the DOM, and
in this case we aren't supposed to be inserting any HTML.

I haven't found a way to focus on a field with Capybara, then add a
character, and focus on another field. So I've manually triggered the
change event in the test.
---
 app/assets/javascripts/banners.js         |  4 ++--
 app/assets/javascripts/markdown_editor.js |  6 +++---
 spec/features/xss_spec.rb                 | 12 ++++++++++++
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/app/assets/javascripts/banners.js b/app/assets/javascripts/banners.js
index d69066c92..9ae732ee4 100644
--- a/app/assets/javascripts/banners.js
+++ b/app/assets/javascripts/banners.js
@@ -4,12 +4,12 @@
     initialize: function() {
       $("[data-js-banner-title]").on({
         change: function() {
-          $("#js-banner-title").html($(this).val());
+          $("#js-banner-title").text($(this).val());
         }
       });
       $("[data-js-banner-description]").on({
         change: function() {
-          $("#js-banner-description").html($(this).val());
+          $("#js-banner-description").text($(this).val());
         }
       });
       $("[name='banner[background_color]']").on({
diff --git a/app/assets/javascripts/markdown_editor.js b/app/assets/javascripts/markdown_editor.js
index c0a587e31..7369c356f 100644
--- a/app/assets/javascripts/markdown_editor.js
+++ b/app/assets/javascripts/markdown_editor.js
@@ -33,10 +33,10 @@
           editor.toggleClass("fullscreen");
           $(".fullscreen-container").toggleClass("medium-8", "medium-12");
           span = $(this).find("span");
-          if (span.html() === span.data("open-text")) {
-            span.html(span.data("closed-text"));
+          if (span.text() === span.data("open-text")) {
+            span.text(span.data("closed-text"));
           } else {
-            span.html(span.data("open-text"));
+            span.text(span.data("open-text"));
           }
           if (editor.hasClass("fullscreen")) {
             App.MarkdownEditor.find_textarea(editor).height($(window).height() - 100);
diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb
index 71447c976..4852e3396 100644
--- a/spec/features/xss_spec.rb
+++ b/spec/features/xss_spec.rb
@@ -13,6 +13,18 @@ describe "Cross-Site Scripting protection", :js do
     expect(page.text).not_to be_empty
   end
 
+  scenario "edit banner" do
+    banner = create(:banner, title: attack_code)
+
+    login_as(create(:administrator).user)
+    visit edit_admin_banner_path(banner)
+
+    title_id = find_field("Title")[:id]
+    execute_script "document.getElementById('#{title_id}').dispatchEvent(new Event('change'))"
+
+    expect(page.text).not_to be_empty
+  end
+
   scenario "document title" do
     process = create(:legislation_process)
     create(:document, documentable: process, title: attack_code)

From 7f1bfc6bd7aba0e9804c0b65d10b5ec46df4e592 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sat, 19 Oct 2019 02:20:59 +0200
Subject: [PATCH 3/3] Avoid using html() to set languages description

The jQuery html() function does not filter <script> tags, so if somehow
an attacker introduced a <script> in the translation, we would be
vulnerable to a XSS attack.

Note using $.parseHTML wouldn't solve the problem, since it doesn't
filter attributes in image tags.

Since changing the text of the part which doesn't have the count wasn't
very clean, I've added another <span> tag for the part with the
description, and so we can use jQuery's text() function to replace it.
---
 app/assets/javascripts/globalize.js |  7 ++++---
 config/locales/en/general.yml       |  6 +++---
 config/locales/es/general.yml       |  6 +++---
 spec/features/xss_spec.rb           | 10 ++++++++++
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/app/assets/javascripts/globalize.js b/app/assets/javascripts/globalize.js
index a7757f520..c653bf627 100644
--- a/app/assets/javascripts/globalize.js
+++ b/app/assets/javascripts/globalize.js
@@ -77,9 +77,10 @@
     update_description: function() {
       var count, description;
       count = App.Globalize.enabled_locales().length;
-      description = App.Globalize.language_description(count);
-      $(".js-languages-description").html(description);
-      $(".js-languages-count").text(count);
+      description = $(App.Globalize.language_description(count)).filter(".description").text();
+
+      $(".js-languages-description .description").text(description);
+      $(".js-languages-description .count").text(count);
     },
     language_description: function(count) {
       switch (count) {
diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml
index b08677ab4..44b7c9358 100644
--- a/config/locales/en/general.yml
+++ b/config/locales/en/general.yml
@@ -806,9 +806,9 @@ en:
       remove_language: Remove language
       add_language: Add language
       languages_in_use:
-        zero: "<span class='js-languages-count'>0</span> languages in use"
-        one: "<span class='js-languages-count'>1</span> language in use"
-        other: "<span class='js-languages-count'>%{count}</span> languages in use"
+        zero: "<span class='count'>0</span> <span class='description'>languages in use</span>"
+        one: "<span class='count'>1</span> <span class='description'>language in use</span>"
+        other: "<span class='count'>%{count}</span> <span class='description'>languages in use</span>"
   social:
     facebook: "%{org} Facebook"
     twitter: "%{org} Twitter"
diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml
index 4b2391528..791d98cb1 100644
--- a/config/locales/es/general.yml
+++ b/config/locales/es/general.yml
@@ -803,9 +803,9 @@ es:
       remove_language: Eliminar idioma
       add_language: Añadir idioma
       languages_in_use:
-        zero: "<span class='js-languages-count'>0</span> idiomas en uso"
-        one: "<span class='js-languages-count'>1</span> idioma en uso"
-        other: "<span class='js-languages-count'>%{count}</span> idiomas en uso"
+        zero: "<span class='count'>0</span> <span class='description'>idiomas en uso</span>"
+        one: "<span class='count'>1</span> <span class='description'> idioma en uso</span>"
+        other: "<span class='count'>%{count}</span> <span class='description'>idiomas en uso</span>"
   social:
     facebook: "Facebook de %{org}"
     twitter: "Twitter de %{org}"
diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb
index 4852e3396..32114b89c 100644
--- a/spec/features/xss_spec.rb
+++ b/spec/features/xss_spec.rb
@@ -61,6 +61,16 @@ describe "Cross-Site Scripting protection", :js do
     expect(page.text).not_to be_empty
   end
 
+  scenario "languages in use" do
+    I18nContent.create(key: "shared.translations.languages_in_use", value: attack_code)
+
+    login_as(create(:administrator).user)
+    visit edit_admin_budget_path(create(:budget))
+    click_link "Remove language"
+
+    expect(page.text).not_to be_empty
+  end
+
   scenario "proposal actions in dashboard" do
     proposal = create(:proposal)