From e470ea1cc16b23221e31a5c495a1b27666df01ef Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Mon, 11 Nov 2019 00:46:44 +0100
Subject: [PATCH] Avoid JavaScript execution in banner URLs

Using `sanitize` we make sure the `href` attribute does not execute any
dangerous code. The possibility of a banner pointing to a dangerous URL
was very reduced, though, since only administrators can edit this
attribute.
---
 app/helpers/banners_helper.rb     |  7 +++++++
 app/views/shared/_banner.html.erb |  5 +----
 spec/features/xss_spec.rb         | 10 ++++++++++
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/app/helpers/banners_helper.rb b/app/helpers/banners_helper.rb
index 06dcb67fb..6a62c275b 100644
--- a/app/helpers/banners_helper.rb
+++ b/app/helpers/banners_helper.rb
@@ -18,4 +18,11 @@ module BannersHelper
   def banner_font_color_or_default
     @banner.font_color.presence || banner_default_font_color
   end
+
+  def banner_target_link(banner)
+    link_to banner.target_url do
+      content_tag(:h2, banner.title, style: "color:#{banner.font_color}") +
+        content_tag(:h3, banner.description, style: "color:#{banner.font_color}")
+    end
+  end
 end
diff --git a/app/views/shared/_banner.html.erb b/app/views/shared/_banner.html.erb
index 48f315a94..9da5bfa6d 100644
--- a/app/views/shared/_banner.html.erb
+++ b/app/views/shared/_banner.html.erb
@@ -1,7 +1,4 @@
 <% banner ||= @banners.sample %>
 <div class="banner" style="background-color:<%= banner.background_color %>;">
-  <%= link_to banner.target_url do %>
-    <h2 style="color:<%= banner.font_color %>;"><%= banner.title %></h2>
-    <h3 style="color:<%= banner.font_color %>;"><%= banner.description %></h3>
-  <% end %>
+  <%= sanitize banner_target_link(banner), attributes: %w[href style] %>
 </div>
diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb
index 46a7eb777..172044797 100644
--- a/spec/features/xss_spec.rb
+++ b/spec/features/xss_spec.rb
@@ -25,6 +25,16 @@ describe "Cross-Site Scripting protection", :js do
     expect(page.text).not_to be_empty
   end
 
+  scenario "banner URL" do
+    banner = create(:banner, title: "Banned!", target_url: "javascript:document.body.remove()")
+
+    login_as(create(:administrator).user)
+    visit edit_admin_banner_path(banner)
+    find(:css, "a", text: "Banned!").click
+
+    expect(page.text).not_to be_empty
+  end
+
   scenario "document title" do
     process = create(:legislation_process)
     create(:document, documentable: process, title: attack_code)