From 0f485308b73193afd3d9a10f2fca3f39d04f5cc3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Wed, 2 Oct 2019 13:42:48 +0200
Subject: [PATCH] Sanitize CKEditor content before displaying it

It's possible to create a newsletter or a proposed action with
<script> tags by filling in the body using a textarea instead of a
CKEditor. While we trust our administrators not to do so, it's better to
completely eliminate that possibility.
---
 app/helpers/proposals_dashboard_helper.rb             |  2 +-
 app/views/budgets/_phases.html.erb                    |  2 +-
 app/views/dashboard/_proposed_action.html.erb         |  4 ++--
 .../new_actions_notification_rake_created.html.erb    |  2 +-
 .../new_actions_notification_rake_published.html.erb  |  2 +-
 app/views/mailer/newsletter.html.erb                  |  2 +-
 spec/features/xss_spec.rb                             | 11 +++++++++++
 7 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/app/helpers/proposals_dashboard_helper.rb b/app/helpers/proposals_dashboard_helper.rb
index 16efad83a..1151a7d0d 100644
--- a/app/helpers/proposals_dashboard_helper.rb
+++ b/app/helpers/proposals_dashboard_helper.rb
@@ -97,7 +97,7 @@ module ProposalsDashboardHelper
   end
 
   def proposed_action_description(proposed_action)
-    raw proposed_action.description.truncate(200)
+    sanitize proposed_action.description.truncate(200)
   end
 
   def proposed_action_long_description?(proposed_action)
diff --git a/app/views/budgets/_phases.html.erb b/app/views/budgets/_phases.html.erb
index 61dd1904c..26df99837 100644
--- a/app/views/budgets/_phases.html.erb
+++ b/app/views/budgets/_phases.html.erb
@@ -7,7 +7,7 @@
         -
         <%= l(phase.ends_at.to_date - 1.day, format: :long) if phase.ends_at.present? %>
       </span>
-      <p><%= safe_html_with_links(phase.summary) %></p>
+      <p><%= safe_html_with_links(WYSIWYGSanitizer.new.sanitize(phase.summary)) %></p>
     </li>
   <% end %>
 </ul>
diff --git a/app/views/dashboard/_proposed_action.html.erb b/app/views/dashboard/_proposed_action.html.erb
index ac26e0394..48e69c21a 100644
--- a/app/views/dashboard/_proposed_action.html.erb
+++ b/app/views/dashboard/_proposed_action.html.erb
@@ -38,10 +38,10 @@
             <small><%= t("dashboard.recommended_actions.show_description") %></small>
           </a>
           <div id="proposed_action_description_<%= dom_id(proposed_action) %>" class="hide" data-toggler=".hide">
-            <%= proposed_action.description.html_safe %>
+            <%= WYSIWYGSanitizer.new.sanitize(proposed_action.description) %>
           </div>
         <% else %>
-          <%= proposed_action.description.html_safe %>
+          <%= WYSIWYGSanitizer.new.sanitize(proposed_action.description) %>
         <% end %>
       <% end %>
 
diff --git a/app/views/dashboard/mailer/new_actions_notification_rake_created.html.erb b/app/views/dashboard/mailer/new_actions_notification_rake_created.html.erb
index ff6f3d817..479003d2e 100644
--- a/app/views/dashboard/mailer/new_actions_notification_rake_created.html.erb
+++ b/app/views/dashboard/mailer/new_actions_notification_rake_created.html.erb
@@ -35,7 +35,7 @@
     <ul>
       <li><%= first_proposed_action.title %></li>
       <% if first_proposed_action.description.present? %>
-        <p><%= first_proposed_action.description.html_safe %></p>
+        <p><%= WYSIWYGSanitizer.new.sanitize(first_proposed_action.description) %></p>
       <% end %>
     </ul>
     <br>
diff --git a/app/views/dashboard/mailer/new_actions_notification_rake_published.html.erb b/app/views/dashboard/mailer/new_actions_notification_rake_published.html.erb
index f8d7b0d7d..f4b00ba40 100644
--- a/app/views/dashboard/mailer/new_actions_notification_rake_published.html.erb
+++ b/app/views/dashboard/mailer/new_actions_notification_rake_published.html.erb
@@ -36,7 +36,7 @@
     <ul>
       <li><%= first_proposed_action.title %></li>
       <% if first_proposed_action.description.present? %>
-        <p><%= first_proposed_action.description.html_safe %></p>
+        <p><%= WYSIWYGSanitizer.new.sanitize(first_proposed_action.description) %></p>
       <% end %>
     </ul>
     <br>
diff --git a/app/views/mailer/newsletter.html.erb b/app/views/mailer/newsletter.html.erb
index c310f2596..afc394f10 100644
--- a/app/views/mailer/newsletter.html.erb
+++ b/app/views/mailer/newsletter.html.erb
@@ -1,5 +1,5 @@
 <td style="padding-bottom: 20px; padding-left: 10px;">
   <p style="font-family: 'Open Sans','Helvetica Neue',arial,sans-serif;font-size: 14px;line-height: 24px;">
-    <%= safe_html_with_links @newsletter.body.html_safe %>
+    <%= safe_html_with_links WYSIWYGSanitizer.new.sanitize(@newsletter.body) %>
   </p>
 </td>
diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb
index 9519c4766..d7b46b8d0 100644
--- a/spec/features/xss_spec.rb
+++ b/spec/features/xss_spec.rb
@@ -30,4 +30,15 @@ describe "Cross-Site Scripting protection", :js do
 
     expect(page.text).not_to be_empty
   end
+
+  scenario "proposal actions in dashboard" do
+    proposal = create(:proposal)
+
+    create(:dashboard_action, description: attack_code)
+
+    login_as(proposal.author)
+    visit recommended_actions_proposal_dashboard_path(proposal)
+
+    expect(page.text).not_to be_empty
+  end
 end