Merge pull request #3749 from consul/sanitize_description

Sanitize descriptions in the views

app/helpers/application_helper.rb

@@ -36,6 +36,10 @@ module ApplicationHelper
     sanitize(Redcarpet::Markdown.new(renderer, extensions).render(text))
   end
 
+  def wysiwyg(text)
+    WYSIWYGSanitizer.new.sanitize(text)
+  end
+
   def author_of?(authorable, user)
     return false if authorable.blank? || user.blank?
     authorable.author_id == user.id

app/models/budget.rb

@@ -42,8 +42,6 @@ class Budget < ApplicationRecord
 
   has_one :poll
 
-  before_validation :sanitize_descriptions
-
   after_create :generate_phases
 
   scope :drafting, -> { where(phase: "drafting") }
@@ -79,7 +77,7 @@ class Budget < ApplicationRecord
     if phases.exists? && phases.send(phase).description.present?
       phases.send(phase).description
     else
-      send("description_#{phase}")&.html_safe
+      send("description_#{phase}")
     end
   end
 
@@ -205,14 +203,6 @@ class Budget < ApplicationRecord
 
   private
 
-    def sanitize_descriptions
-      s = WYSIWYGSanitizer.new
-      Budget::Phase::PHASE_KINDS.each do |phase|
-        sanitized = s.sanitize(send("description_#{phase}"))
-        send("description_#{phase}=", sanitized)
-      end
-    end
-
     def generate_phases
       Budget::Phase::PHASE_KINDS.each do |phase|
         Budget::Phase.create(

app/models/budget/phase.rb

@@ -9,17 +9,6 @@ class Budget
     translates :summary, touch: true
     translates :description, touch: true
     include Globalizable
-
-    class Translation
-      before_validation :sanitize_description
-
-      private
-
-        def sanitize_description
-          self.description = WYSIWYGSanitizer.new.sanitize(description)
-        end
-    end
-
     include Sanitizable
 
     belongs_to :budget

app/models/concerns/globalizable.rb

@@ -9,10 +9,6 @@ module Globalizable
     validate :check_translations_number, on: :update, if: :translations_required?
     after_validation :copy_error_to_current_translation, on: :update
 
-    def description
-      self.read_attribute(:description)&.html_safe
-    end
-
     def locales_not_marked_for_destruction
       translations.reject(&:marked_for_destruction?).map(&:locale)
     end

app/models/concerns/sanitizable.rb

@@ -2,49 +2,12 @@ module Sanitizable
   extend ActiveSupport::Concern
 
   included do
-    before_validation :sanitize_description
     before_validation :sanitize_tag_list
-
-    unless included_modules.include? Globalizable
-      def description
-        super&.html_safe
-      end
-    end
   end
 
   protected
 
-    def sanitize_description
-      if translatable_description?
-        sanitize_description_translations
-      else
-        self.description = WYSIWYGSanitizer.new.sanitize(description)
-      end
-    end
-
     def sanitize_tag_list
       self.tag_list = TagSanitizer.new.sanitize_tag_list(tag_list) if self.class.taggable?
     end
-
-    def translatable_description?
-      self.class.included_modules.include?(Globalizable) &&
-      self.class.translated_attribute_names.include?(:description)
-    end
-
-    def sanitize_description_translations
-      # Sanitize description when using attribute accessor in place of nested translations.
-      # This is because Globalize gem create translations on after save callback
-      # https://github.com/globalize/globalize/blob/e37c471775d196cd4318e61954572c300c015467/lib/globalize/active_record/act_macro.rb#L105
-      if translations.empty?
-        Globalize.with_locale(I18n.locale) do
-          self.description = WYSIWYGSanitizer.new.sanitize(description)
-        end
-      end
-
-      translations.reject(&:_destroy).each do |translation|
-        Globalize.with_locale(translation.locale) do
-          self.description = WYSIWYGSanitizer.new.sanitize(description)
-        end
-      end
-    end
 end

app/models/legislation/draft_version.rb

@@ -7,8 +7,6 @@ class Legislation::DraftVersion < ApplicationRecord
   translates :title,     touch: true
   translates :changelog, touch: true
   translates :body,      touch: true
-  translates :body_html, touch: true
-  translates :toc_html,  touch: true
   include Globalizable
 
   belongs_to :process, class_name: "Legislation::Process", foreign_key: "legislation_process_id"
@@ -20,23 +18,16 @@ class Legislation::DraftVersion < ApplicationRecord
 
   scope :published, -> { where(status: "published").order("id DESC") }
 
-  before_save :render_html
-
-  def render_html
+  def body_html
     renderer = Redcarpet::Render::HTML.new(with_toc_data: true)
-    toc_renderer = Redcarpet::Render::HTML_TOC.new(with_toc_data: true)
 
-    if body_changed?
-      self.body_html = Redcarpet::Markdown.new(renderer).render(body)
-      self.toc_html = Redcarpet::Markdown.new(toc_renderer).render(body)
+    Redcarpet::Markdown.new(renderer).render(body)
   end
 
-    translations.each do |translation|
-      if translation.body_changed?
-        translation.body_html = Redcarpet::Markdown.new(renderer).render(translation.body)
-        translation.toc_html = Redcarpet::Markdown.new(toc_renderer).render(translation.body)
-      end
-    end
+  def toc_html
+    renderer = Redcarpet::Render::HTML_TOC.new(with_toc_data: true)
+
+    Redcarpet::Markdown.new(renderer).render(body)
   end
 
   def display_title

app/models/poll/question/answer.rb

@@ -19,10 +19,6 @@ class Poll::Question::Answer < ApplicationRecord
 
   scope :visibles, -> { where(hidden: false) }
 
-  def description
-    self[:description]&.html_safe
-  end
-
   def self.order_answers(ordered_array)
     ordered_array.each_with_index do |answer_id, order|
       find(answer_id).update_attribute(:given_order, (order + 1))

app/views/admin/debates/show.html.erb

@@ -25,7 +25,7 @@
           </span>
   </div>
 
-  <%= auto_link_already_sanitized_html @debate.description %>
+  <%= auto_link_already_sanitized_html wysiwyg(@debate.description) %>
 
   <h3><%= t("votes.supports") %></h3>
 

app/views/admin/hidden_budget_investments/index.html.erb

@@ -20,7 +20,7 @@
         </td>
         <td>
           <div class="moderation-description">
-            <%= investment.description %>
+            <%= wysiwyg(investment.description) %>
           </div>
         </td>
         <td class="align-top">

app/views/admin/hidden_debates/index.html.erb

@@ -20,7 +20,7 @@
         </td>
         <td>
           <div class="moderation-description">
-            <%= debate.description %>
+            <%= wysiwyg(debate.description) %>
           </div>
         </td>
         <td class="align-top">

app/views/admin/hidden_proposals/index.html.erb

@@ -21,7 +21,7 @@
         <td>
           <div class="moderation-description">
             <p><small><%= proposal.summary %></small></p>
-            <%= proposal.description %>
+            <%= wysiwyg(proposal.description) %>
             <% if proposal.video_url.present? %>
               <p><%= sanitize_and_auto_link proposal.video_url %></p>
             <% end %>

app/views/admin/poll/questions/answers/show.html.erb

@@ -21,7 +21,7 @@
   <tbody>
     <tr>
       <td><%= @answer.title %></td>
-      <td><%= @answer.description %></td>
+      <td><%= wysiwyg(@answer.description) %></td>
       <td class="text-center">
         (<%= @answer.images.count %>)<br>
         <%= link_to t("admin.answers.show.images_list"), admin_answer_images_path(@answer) %>

app/views/admin/poll/questions/show.html.erb

@@ -82,7 +82,7 @@
     <% @question.question_answers.each do |answer| %>
       <tr id="<%= dom_id(answer) %>" class="poll_question_answer" data-answer-id="<%= answer.id %>">
         <td class="align-top"><%= link_to answer.title, admin_answer_path(answer) %></td>
-        <td class="align-top break"><%= answer.description %></td>
+        <td class="align-top break"><%= wysiwyg(answer.description) %></td>
         <td class="align-top text-center">
           (<%= answer.images.count %>)
           <br>

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><%= auto_link_already_sanitized_html(WYSIWYGSanitizer.new.sanitize(phase.summary)) %></p>
+      <p><%= auto_link_already_sanitized_html(wysiwyg(phase.summary)) %></p>
     </li>
   <% end %>
 </ul>

app/views/budgets/index.html.erb

@@ -15,7 +15,7 @@
 
           <h1><%= current_budget.name %></h1>
           <div class="description">
-            <%= auto_link_already_sanitized_html(current_budget.description) %>
+            <%= auto_link_already_sanitized_html wysiwyg(current_budget.description) %>
           </div>
           <p>
             <%= link_to t("budgets.index.section_header.help"), "#section_help" %>

app/views/budgets/investments/_investment.html.erb

@@ -46,7 +46,7 @@
               <%= investment.heading.name %>
             </p>
             <div class="investment-project-description">
-              <%= investment.description %>
+              <%= wysiwyg(investment.description) %>
               <div class="truncate"></div>
             </div>
             <%= render "shared/tags", taggable: investment, limit: 5 %>

app/views/budgets/investments/_investment_detail.erb

@@ -22,7 +22,7 @@
   <%= sanitize(t("budgets.investments.show.code", code: investment.id)) %>
 </p>
 
-<%= auto_link_already_sanitized_html investment.description %>
+<%= auto_link_already_sanitized_html wysiwyg(investment.description) %>
 
 <% if feature?(:map) && map_location_available?(@investment.map_location) %>
   <div class="margin">

app/views/budgets/show.html.erb

@@ -9,7 +9,7 @@
 
       <h1><%= @budget.name %></h1>
 
-      <%= auto_link_already_sanitized_html(@budget.description) %>
+      <%= auto_link_already_sanitized_html wysiwyg(@budget.description) %>
     </div>
     <div class="small-12 medium-3 column info padding" data-equalizer-watch>
       <p>

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">
-            <%= WYSIWYGSanitizer.new.sanitize(proposed_action.description) %>
+            <%= wysiwyg(proposed_action.description) %>
           </div>
         <% else %>
-          <%= WYSIWYGSanitizer.new.sanitize(proposed_action.description) %>
+          <%= wysiwyg(proposed_action.description) %>
         <% end %>
       <% end %>
 

app/views/dashboard/actions/new_request.html.erb

@@ -2,7 +2,7 @@
 
 <div class="row expanded">
   <div class="small-12 medium-8 column">
-    <%= WYSIWYGSanitizer.new.sanitize(dashboard_action.description) %>
+    <%= wysiwyg(dashboard_action.description) %>
     <%= render "dashboard/form" %>
   </div>
 

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><%= WYSIWYGSanitizer.new.sanitize(first_proposed_action.description) %></p>
+        <p><%= wysiwyg(first_proposed_action.description) %></p>
       <% end %>
     </ul>
     <br>

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><%= WYSIWYGSanitizer.new.sanitize(first_proposed_action.description) %></p>
+        <p><%= wysiwyg(first_proposed_action.description) %></p>
       <% end %>
     </ul>
     <br>

app/views/debates/_debate.html.erb

@@ -39,7 +39,7 @@
 
             </p>
             <div class="debate-description">
-              <%= debate.description %>
+              <%= wysiwyg(debate.description) %>
               <div class="truncate"></div>
             </div>
             <%= render "shared/tags", taggable: debate, limit: 5 %>

app/views/debates/show.html.erb

@@ -30,7 +30,7 @@
           </span>
         </div>
 
-        <%= auto_link_already_sanitized_html @debate.description %>
+        <%= auto_link_already_sanitized_html wysiwyg(@debate.description) %>
 
         <%= render "shared/tags", taggable: @debate %>
 

app/views/legislation/draft_versions/show.html.erb

@@ -66,7 +66,7 @@
                    data-legislation-annotatable-base-url="<%= legislation_process_draft_version_path(@process, @draft_version) %>"
                    data-legislation-open-phase="<%= @process.allegations_phase.open? %>">
           <% end %>
-            <%= sanitize(@draft_version.body_html) %>
+            <%= sanitize(@draft_version.body_html, { attributes: ["id"] }) %>
           </section>
         </div>
       </div>

app/views/legislation/processes/milestones.html.erb

@@ -6,7 +6,7 @@
 
 <div class="row">
   <div class="small-12 column">
-    <%= WYSIWYGSanitizer.new.sanitize(@process.milestones_summary) %>
+    <%= wysiwyg(@process.milestones_summary) %>
   </div>
 </div>
 

app/views/legislation/proposals/show.html.erb

@@ -68,7 +68,7 @@
           </div>
         <% end %>
 
-        <%= auto_link_already_sanitized_html @proposal.description %>
+        <%= auto_link_already_sanitized_html wysiwyg(@proposal.description) %>
 
         <% if @proposal.video_url.present? %>
           <div class="video-link">

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;">
-    <%= auto_link_already_sanitized_html WYSIWYGSanitizer.new.sanitize(@newsletter.body) %>
+    <%= auto_link_already_sanitized_html wysiwyg(@newsletter.body) %>
   </p>
 </td>

app/views/moderation/budgets/investments/index.html.erb

@@ -40,7 +40,7 @@
             <%= investment.author.username %>
             <br>
             <div class="moderation-description">
-              <%= investment.description %>
+              <%= wysiwyg(investment.description) %>
             </div>
           </td>
           <td class="text-center">

app/views/moderation/debates/index.html.erb

@@ -37,7 +37,7 @@
             <%= debate.author.username %>
             <br>
             <div class="moderation-description">
-              <%= debate.description %>
+              <%= wysiwyg(debate.description) %>
             </div>
           </td>
           <td class="text-center">

app/views/moderation/proposals/index.html.erb

@@ -37,7 +37,7 @@
             <%= proposal.author.username %>
             <br>
             <div class="moderation-description">
-              <%= proposal.description %>
+              <%= wysiwyg(proposal.description) %>
             </div>
           </td>
           <td class="text-center">

app/views/polls/index.html.erb

@@ -14,7 +14,7 @@
   <div class="small-12 column">
     <% if show_polls_description? %>
       <div class="polls-description">
-        <%= auto_link_already_sanitized_html WYSIWYGSanitizer.new.sanitize(@active_poll.description) %>
+        <%= auto_link_already_sanitized_html wysiwyg(@active_poll.description) %>
       </div>
     <% end %>
 

app/views/polls/show.html.erb

@@ -67,7 +67,7 @@
                     <div id="answer_description_<%= answer.id %>"
                          class="answer-description short answer-left-divider" data-toggler="short">
                         <% if answer.description.present? %>
-                          <%= answer.description %>
+                          <%= wysiwyg(answer.description) %>
                         <% end %>
 
                         <% if answer.images.any? %>

app/views/proposals/_info.html.erb

@@ -39,7 +39,7 @@
   </div>
 <% end %>
 
-<%= auto_link_already_sanitized_html @proposal.description %>
+<%= auto_link_already_sanitized_html wysiwyg(@proposal.description) %>
 
 <% if feature?(:map) && map_location_available?(@proposal.map_location) %>
   <div class="margin">

app/views/tracking/budget_investments/edit.html.erb

@@ -7,6 +7,6 @@
 
 <h1><%= @investment.title %></h1>
 
-<%= auto_link_already_sanitized_html @investment.description %>
+<%= auto_link_already_sanitized_html wysiwyg(@investment.description) %>
 
 <%= render "tracking/milestones/milestones", milestoneable: @investment %>

app/views/tracking/budget_investments/show.html.erb

@@ -3,7 +3,7 @@
 <h2><%= t("tracking.budget_investments.show.title") %> <%= @investment.id %> </h2>
 <h1><%= @investment.title %></h1>
 
-<%= auto_link_already_sanitized_html @investment.description %>
+<%= auto_link_already_sanitized_html wysiwyg(@investment.description) %>
 
 <% if @investment.external_url.present? %>
   <p><%= sanitize_and_auto_link @investment.external_url %></p>

app/views/welcome/_recommended_carousel.html.erb

@@ -16,7 +16,7 @@
                   <%= link_to recommended_path(recommended) do %>
                     <h4 class="truncate-horizontal-text"><%= recommended.title %></h4>
                   <% end %>
-                  <%= recommended.description %>
+                  <%= wysiwyg(recommended.description) %>
                 </div>
               </div>
             </li>

db/migrate/20180801140800_add_collaborative_legislation_translations.rb

@@ -20,12 +20,14 @@ class AddCollaborativeLegislationTranslations < ActiveRecord::Migration[4.2]
       {
         title:     :string,
         changelog: :text,
-        body:      :text,
-        body_html: :text,
-        toc_html:  :text
+        body:      :text
       },
       { migrate_data: true }
     )
+
+    add_column :legislation_draft_version_translations, :body_html, :text
+    add_column :legislation_draft_version_translations, :toc_html, :text
+
     Legislation::QuestionOption.create_translation_table!(
       { value: :string },
       { migrate_data: true }

db/migrate/20191006114603_remove_html_fields_form_legislation_draft_version.rb

@@ -0,0 +1,8 @@
+class RemoveHtmlFieldsFormLegislationDraftVersion < ActiveRecord::Migration[5.0]
+  def change
+    remove_column :legislation_draft_versions, :body_html
+    remove_column :legislation_draft_version_translations, :body_html
+    remove_column :legislation_draft_versions, :toc_html
+    remove_column :legislation_draft_version_translations, :toc_html
+  end
+end

db/schema.rb

@@ -759,8 +759,6 @@ ActiveRecord::Schema.define(version: 20191020153455) do
     t.string   "title"
     t.text     "changelog"
     t.text     "body"
-    t.text     "body_html"
-    t.text     "toc_html"
     t.datetime "hidden_at"
     t.index ["hidden_at"], name: "index_legislation_draft_version_translations_on_hidden_at", using: :btree
     t.index ["legislation_draft_version_id"], name: "index_900e5ba94457606e69e89193db426e8ddff809bc", using: :btree
@@ -777,8 +775,6 @@ ActiveRecord::Schema.define(version: 20191020153455) do
     t.datetime "hidden_at"
     t.datetime "created_at",                               null: false
     t.datetime "updated_at",                               null: false
-    t.text     "body_html"
-    t.text     "toc_html"
     t.index ["hidden_at"], name: "index_legislation_draft_versions_on_hidden_at", using: :btree
     t.index ["legislation_process_id"], name: "index_legislation_draft_versions_on_legislation_process_id", using: :btree
     t.index ["status"], name: "index_legislation_draft_versions_on_status", using: :btree

lib/consul_form_builder.rb

@@ -21,7 +21,7 @@ class ConsulFormBuilder < FoundationRailsHelper::FormBuilder
 
   def check_box(attribute, options = {})
     if options[:label] != false
-      label = content_tag(:span, sanitize(label_text(object, attribute, options[:label]), class: "checkbox"))
+      label = content_tag(:span, sanitize(label_text(object, attribute, options[:label])), class: "checkbox")
 
       super(attribute, options.merge(label: label, label_options: label_options_for(options)))
     else
@@ -76,7 +76,7 @@ class ConsulFormBuilder < FoundationRailsHelper::FormBuilder
 
     def help_text_id(attribute, options)
       if options[:hint]
-        "#{custom_label(attribute, nil, nil).match(/for=\"(.+)\"/)[1]}-help-text"
+        "#{custom_label(attribute, nil, nil).match(/for="([^"]+)"/)[1]}-help-text"
       end
     end
 end

spec/features/xss_spec.rb

@@ -121,6 +121,31 @@ describe "Cross-Site Scripting protection", :js do
     expect(page.text).not_to be_empty
   end
 
+  scenario "proposal description" do
+    proposal = create(:proposal, description: attack_code)
+
+    visit proposal_path(proposal)
+
+    expect(page.text).not_to be_empty
+  end
+
+  scenario "investment description" do
+    investment = create(:budget_investment, description: attack_code)
+
+    visit budget_investment_path(investment.budget, investment)
+
+    expect(page.text).not_to be_empty
+  end
+
+  scenario "budget phase description" do
+    budget = create(:budget)
+    budget.current_phase.update(description: attack_code)
+
+    visit budget_path(budget)
+
+    expect(page.text).not_to be_empty
+  end
+
   scenario "markdown conversion" do
     process = create(:legislation_process, description: attack_code)
 
@@ -128,4 +153,13 @@ describe "Cross-Site Scripting protection", :js do
 
     expect(page.text).not_to be_empty
   end
+
+  scenario "legislation version body filters script tags but not header IDs" do
+    version = create(:legislation_draft_version, :published, body: "# Title 1\n#{attack_code}")
+
+    visit legislation_process_draft_version_path(version.process, version)
+
+    expect(page.text).not_to be_empty
+    expect(page).to have_css "h1#title-1", text: "Title 1"
+  end
 end

spec/models/budget/phase_spec.rb

@@ -205,12 +205,4 @@ describe Budget::Phase do
       end
     end
   end
-
-  describe "#sanitize_description" do
-    it "removes not allowed html entities from the description" do
-      expect do
-        first_phase.update_attributes(description: '<p><a href="/"><b>a</b></a></p> <script>javascript</script>')
-      end.to change { first_phase.description }.to('<p><a href="/">a</a></p> javascript')
-    end
-  end
 end

spec/models/budget_spec.rb

@@ -37,7 +37,6 @@ describe Budget do
         Budget::Phase::PHASE_KINDS.each do |phase_kind|
           budget.phase = phase_kind
           expect(budget.description).to eq(budget.send("description_#{phase_kind}"))
-          expect(budget.description).to be_html_safe
         end
       end
     end

spec/models/legislation/draft_version_spec.rb

@@ -10,7 +10,7 @@ describe Legislation::DraftVersion do
     expect(legislation_draft_version).to be_valid
   end
 
-  it "renders and saves the html from the markdown body field" do
+  it "renders the html from the markdown body field" do
     legislation_draft_version.body = body_markdown
 
     legislation_draft_version.save!
@@ -19,16 +19,6 @@ describe Legislation::DraftVersion do
     expect(legislation_draft_version.toc_html).to eq(toc_html)
   end
 
-  it "renders and saves the html from the markdown body field with alternative translation" do
-    legislation_draft_version.title_fr = "Français"
-    legislation_draft_version.body_fr = body_markdown
-
-    legislation_draft_version.save!
-
-    expect(legislation_draft_version.body_html_fr).to eq(body_html)
-    expect(legislation_draft_version.toc_html_fr).to eq(toc_html)
-  end
-
   def body_markdown
     <<-BODY_MARKDOWN
 # Title 1

spec/shared/models/sanitizable.rb

@@ -1,38 +1,6 @@
 shared_examples "sanitizable" do
   let(:sanitizable)        { build(model_name(described_class)) }
 
-  it "is sanitized" do
-    sanitizable.description = "<script>alert('danger');</script>"
-
-    sanitizable.valid?
-
-    expect(sanitizable.description).to eq("alert('danger');")
-  end
-
-  it "is html_safe" do
-    sanitizable.description = "<script>alert('danger');</script>"
-
-    sanitizable.valid?
-
-    expect(sanitizable.description).to be_html_safe
-  end
-
-  it "is sanitized using globalize accessors" do
-    sanitizable.description_en = "<script>alert('danger');</script>"
-
-    sanitizable.valid?
-
-    expect(sanitizable.description_en).to eq("alert('danger');")
-  end
-
-  it "is html_safe using globalize accessors" do
-    sanitizable.description_en = "<script>alert('danger');</script>"
-
-    sanitizable.valid?
-
-    expect(sanitizable.description_en).to be_html_safe
-  end
-
   describe "#tag_list" do
     before do
       unless described_class.included_modules.include?(Taggable)