From ae2576020ed9fd27914b83239bfff2e8f92c9e88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 6 Oct 2019 03:35:54 +0200 Subject: [PATCH 1/4] Extract method to use WYSIWYGSanitizer in views This is similar to methods we use like `sanitize` or `markdown`. --- app/helpers/application_helper.rb | 4 ++++ app/views/budgets/_phases.html.erb | 2 +- app/views/dashboard/_proposed_action.html.erb | 4 ++-- app/views/dashboard/actions/new_request.html.erb | 2 +- .../mailer/new_actions_notification_rake_created.html.erb | 2 +- .../mailer/new_actions_notification_rake_published.html.erb | 2 +- app/views/legislation/processes/milestones.html.erb | 2 +- app/views/mailer/newsletter.html.erb | 2 +- app/views/polls/index.html.erb | 2 +- 9 files changed, 13 insertions(+), 9 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 9785c46cf..cb4fb879c 100644 --- a/app/helpers/application_helper.rb +++ b/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 diff --git a/app/views/budgets/_phases.html.erb b/app/views/budgets/_phases.html.erb index f9791a43e..9a05b8ae7 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? %> -

<%= auto_link_already_sanitized_html(WYSIWYGSanitizer.new.sanitize(phase.summary)) %>

+

<%= auto_link_already_sanitized_html(wysiwyg(phase.summary)) %>

<% end %> diff --git a/app/views/dashboard/_proposed_action.html.erb b/app/views/dashboard/_proposed_action.html.erb index 48e69c21a..83536298a 100644 --- a/app/views/dashboard/_proposed_action.html.erb +++ b/app/views/dashboard/_proposed_action.html.erb @@ -38,10 +38,10 @@ <%= t("dashboard.recommended_actions.show_description") %>
- <%= WYSIWYGSanitizer.new.sanitize(proposed_action.description) %> + <%= wysiwyg(proposed_action.description) %>
<% else %> - <%= WYSIWYGSanitizer.new.sanitize(proposed_action.description) %> + <%= wysiwyg(proposed_action.description) %> <% end %> <% end %> diff --git a/app/views/dashboard/actions/new_request.html.erb b/app/views/dashboard/actions/new_request.html.erb index ba6486ce5..4359f17c8 100644 --- a/app/views/dashboard/actions/new_request.html.erb +++ b/app/views/dashboard/actions/new_request.html.erb @@ -2,7 +2,7 @@
- <%= WYSIWYGSanitizer.new.sanitize(dashboard_action.description) %> + <%= wysiwyg(dashboard_action.description) %> <%= render "dashboard/form" %>
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 7f4d74cb1..f43c1600a 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 @@
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 f4b00ba40..f120ab1bc 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 @@
diff --git a/app/views/legislation/processes/milestones.html.erb b/app/views/legislation/processes/milestones.html.erb index 8ea3b6c9b..be66be7b2 100644 --- a/app/views/legislation/processes/milestones.html.erb +++ b/app/views/legislation/processes/milestones.html.erb @@ -6,7 +6,7 @@
- <%= WYSIWYGSanitizer.new.sanitize(@process.milestones_summary) %> + <%= wysiwyg(@process.milestones_summary) %>
diff --git a/app/views/mailer/newsletter.html.erb b/app/views/mailer/newsletter.html.erb index e77eaa1f0..3b45f6105 100644 --- a/app/views/mailer/newsletter.html.erb +++ b/app/views/mailer/newsletter.html.erb @@ -1,5 +1,5 @@

- <%= auto_link_already_sanitized_html WYSIWYGSanitizer.new.sanitize(@newsletter.body) %> + <%= auto_link_already_sanitized_html wysiwyg(@newsletter.body) %>

diff --git a/app/views/polls/index.html.erb b/app/views/polls/index.html.erb index c4c722435..251e8493f 100644 --- a/app/views/polls/index.html.erb +++ b/app/views/polls/index.html.erb @@ -14,7 +14,7 @@
<% if show_polls_description? %>
- <%= auto_link_already_sanitized_html WYSIWYGSanitizer.new.sanitize(@active_poll.description) %> + <%= auto_link_already_sanitized_html wysiwyg(@active_poll.description) %>
<% end %> From 7bf4e4d6113a6c2bf3cfc619d6e756f5f01c68bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 6 Oct 2019 03:41:53 +0200 Subject: [PATCH 2/4] Sanitize descriptions in the views Sanitizing descriptions before saving a record has a few drawbacks: 1. It makes the application rely on data being safe in the database. If somehow dangerous data enters the database, the application will be vulnerable to XSS attacks 2. It makes the code complicated 3. It isn't backwards compatible; if we decide to disallow a certain HTML tag in the future, we'd need to sanitize existing data. On the other hand, sanitizing the data in the view means we don't need to triple-check dangerous HTML has already been stripped when we see the method `auto_link_already_sanitized_html`, since now every time we use it we sanitize the text in the same line we call this method. We could also sanitize the data twice, both when saving to the database and when displaying values in the view. However, doing so wouldn't make the application safer, since we sanitize text introduced through textarea fields but we don't sanitize text introduced through input fields. Finally, we could also overwrite the `description` method so it sanitizes the text. But we're already introducing Globalize which overwrites that method, and overwriting it again is a bit too confusing in my humble opinion. It can also lead to hard-to-debug behaviour. --- app/models/budget.rb | 12 +----- app/models/budget/phase.rb | 11 ------ app/models/concerns/globalizable.rb | 4 -- app/models/concerns/sanitizable.rb | 37 ------------------- app/models/poll/question/answer.rb | 4 -- app/views/admin/debates/show.html.erb | 2 +- .../hidden_budget_investments/index.html.erb | 2 +- app/views/admin/hidden_debates/index.html.erb | 2 +- .../admin/hidden_proposals/index.html.erb | 2 +- .../poll/questions/answers/show.html.erb | 2 +- app/views/admin/poll/questions/show.html.erb | 2 +- app/views/budgets/index.html.erb | 2 +- .../budgets/investments/_investment.html.erb | 2 +- .../investments/_investment_detail.erb | 2 +- app/views/budgets/show.html.erb | 2 +- app/views/debates/_debate.html.erb | 2 +- app/views/debates/show.html.erb | 2 +- app/views/legislation/proposals/show.html.erb | 2 +- .../budgets/investments/index.html.erb | 2 +- app/views/moderation/debates/index.html.erb | 2 +- app/views/moderation/proposals/index.html.erb | 2 +- app/views/polls/show.html.erb | 2 +- app/views/proposals/_info.html.erb | 2 +- .../tracking/budget_investments/edit.html.erb | 2 +- .../tracking/budget_investments/show.html.erb | 2 +- .../welcome/_recommended_carousel.html.erb | 2 +- spec/features/xss_spec.rb | 25 +++++++++++++ spec/models/budget/phase_spec.rb | 8 ---- spec/models/budget_spec.rb | 1 - spec/shared/models/sanitizable.rb | 32 ---------------- 30 files changed, 47 insertions(+), 129 deletions(-) diff --git a/app/models/budget.rb b/app/models/budget.rb index 3ca9d2147..8d6023fcb 100644 --- a/app/models/budget.rb +++ b/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( diff --git a/app/models/budget/phase.rb b/app/models/budget/phase.rb index ccaf2f78d..859beb51f 100644 --- a/app/models/budget/phase.rb +++ b/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 diff --git a/app/models/concerns/globalizable.rb b/app/models/concerns/globalizable.rb index 9051188b5..7fb00d887 100644 --- a/app/models/concerns/globalizable.rb +++ b/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 diff --git a/app/models/concerns/sanitizable.rb b/app/models/concerns/sanitizable.rb index 387d089ff..b9222bc56 100644 --- a/app/models/concerns/sanitizable.rb +++ b/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 diff --git a/app/models/poll/question/answer.rb b/app/models/poll/question/answer.rb index 5ac37c10a..5f245cb4e 100644 --- a/app/models/poll/question/answer.rb +++ b/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)) diff --git a/app/views/admin/debates/show.html.erb b/app/views/admin/debates/show.html.erb index 2d70c4596..c0f70cf73 100644 --- a/app/views/admin/debates/show.html.erb +++ b/app/views/admin/debates/show.html.erb @@ -25,7 +25,7 @@
- <%= auto_link_already_sanitized_html @debate.description %> + <%= auto_link_already_sanitized_html wysiwyg(@debate.description) %>

<%= t("votes.supports") %>

diff --git a/app/views/admin/hidden_budget_investments/index.html.erb b/app/views/admin/hidden_budget_investments/index.html.erb index 39cc71710..ed89d5cdc 100644 --- a/app/views/admin/hidden_budget_investments/index.html.erb +++ b/app/views/admin/hidden_budget_investments/index.html.erb @@ -20,7 +20,7 @@
- <%= investment.description %> + <%= wysiwyg(investment.description) %>
diff --git a/app/views/admin/hidden_debates/index.html.erb b/app/views/admin/hidden_debates/index.html.erb index 25cfd7102..7392eb3e6 100644 --- a/app/views/admin/hidden_debates/index.html.erb +++ b/app/views/admin/hidden_debates/index.html.erb @@ -20,7 +20,7 @@
- <%= debate.description %> + <%= wysiwyg(debate.description) %>
diff --git a/app/views/admin/hidden_proposals/index.html.erb b/app/views/admin/hidden_proposals/index.html.erb index 5cbadf84d..bf8a2ff6a 100644 --- a/app/views/admin/hidden_proposals/index.html.erb +++ b/app/views/admin/hidden_proposals/index.html.erb @@ -21,7 +21,7 @@

<%= proposal.summary %>

- <%= proposal.description %> + <%= wysiwyg(proposal.description) %> <% if proposal.video_url.present? %>

<%= sanitize_and_auto_link proposal.video_url %>

<% end %> diff --git a/app/views/admin/poll/questions/answers/show.html.erb b/app/views/admin/poll/questions/answers/show.html.erb index 83249f606..8244687b2 100644 --- a/app/views/admin/poll/questions/answers/show.html.erb +++ b/app/views/admin/poll/questions/answers/show.html.erb @@ -21,7 +21,7 @@ <%= @answer.title %> - <%= @answer.description %> + <%= wysiwyg(@answer.description) %> (<%= @answer.images.count %>)
<%= link_to t("admin.answers.show.images_list"), admin_answer_images_path(@answer) %> diff --git a/app/views/admin/poll/questions/show.html.erb b/app/views/admin/poll/questions/show.html.erb index edf693bf7..b550149ed 100644 --- a/app/views/admin/poll/questions/show.html.erb +++ b/app/views/admin/poll/questions/show.html.erb @@ -82,7 +82,7 @@ <% @question.question_answers.each do |answer| %> <%= link_to answer.title, admin_answer_path(answer) %> - <%= answer.description %> + <%= wysiwyg(answer.description) %> (<%= answer.images.count %>)
diff --git a/app/views/budgets/index.html.erb b/app/views/budgets/index.html.erb index 3ad23eec3..e8d58d08a 100644 --- a/app/views/budgets/index.html.erb +++ b/app/views/budgets/index.html.erb @@ -15,7 +15,7 @@

<%= current_budget.name %>

- <%= auto_link_already_sanitized_html(current_budget.description) %> + <%= auto_link_already_sanitized_html wysiwyg(current_budget.description) %>

<%= link_to t("budgets.index.section_header.help"), "#section_help" %> diff --git a/app/views/budgets/investments/_investment.html.erb b/app/views/budgets/investments/_investment.html.erb index 0a2bcea84..c3863f9ce 100644 --- a/app/views/budgets/investments/_investment.html.erb +++ b/app/views/budgets/investments/_investment.html.erb @@ -46,7 +46,7 @@ <%= investment.heading.name %>

- <%= investment.description %> + <%= wysiwyg(investment.description) %>
<%= render "shared/tags", taggable: investment, limit: 5 %> diff --git a/app/views/budgets/investments/_investment_detail.erb b/app/views/budgets/investments/_investment_detail.erb index 90838e9d2..80e5dd5e5 100644 --- a/app/views/budgets/investments/_investment_detail.erb +++ b/app/views/budgets/investments/_investment_detail.erb @@ -22,7 +22,7 @@ <%= sanitize(t("budgets.investments.show.code", code: investment.id)) %>

-<%= auto_link_already_sanitized_html investment.description %> +<%= auto_link_already_sanitized_html wysiwyg(investment.description) %> <% if feature?(:map) && map_location_available?(@investment.map_location) %>
diff --git a/app/views/budgets/show.html.erb b/app/views/budgets/show.html.erb index 826752248..3949208be 100644 --- a/app/views/budgets/show.html.erb +++ b/app/views/budgets/show.html.erb @@ -9,7 +9,7 @@

<%= @budget.name %>

- <%= auto_link_already_sanitized_html(@budget.description) %> + <%= auto_link_already_sanitized_html wysiwyg(@budget.description) %>

diff --git a/app/views/debates/_debate.html.erb b/app/views/debates/_debate.html.erb index 57adbd11f..72bc717ac 100644 --- a/app/views/debates/_debate.html.erb +++ b/app/views/debates/_debate.html.erb @@ -39,7 +39,7 @@

- <%= debate.description %> + <%= wysiwyg(debate.description) %>
<%= render "shared/tags", taggable: debate, limit: 5 %> diff --git a/app/views/debates/show.html.erb b/app/views/debates/show.html.erb index 113041e63..4b7211fdb 100644 --- a/app/views/debates/show.html.erb +++ b/app/views/debates/show.html.erb @@ -30,7 +30,7 @@
- <%= auto_link_already_sanitized_html @debate.description %> + <%= auto_link_already_sanitized_html wysiwyg(@debate.description) %> <%= render "shared/tags", taggable: @debate %> diff --git a/app/views/legislation/proposals/show.html.erb b/app/views/legislation/proposals/show.html.erb index 7f21c1506..c581567c5 100644 --- a/app/views/legislation/proposals/show.html.erb +++ b/app/views/legislation/proposals/show.html.erb @@ -68,7 +68,7 @@
<% end %> - <%= auto_link_already_sanitized_html @proposal.description %> + <%= auto_link_already_sanitized_html wysiwyg(@proposal.description) %> <% if @proposal.video_url.present? %>
diff --git a/app/views/moderation/budgets/investments/index.html.erb b/app/views/moderation/budgets/investments/index.html.erb index d9c636d63..a2f08582a 100644 --- a/app/views/moderation/budgets/investments/index.html.erb +++ b/app/views/moderation/budgets/investments/index.html.erb @@ -40,7 +40,7 @@ <%= investment.author.username %>
- <%= investment.description %> + <%= wysiwyg(investment.description) %>
diff --git a/app/views/moderation/debates/index.html.erb b/app/views/moderation/debates/index.html.erb index 637d475bc..9e0d02301 100644 --- a/app/views/moderation/debates/index.html.erb +++ b/app/views/moderation/debates/index.html.erb @@ -37,7 +37,7 @@ <%= debate.author.username %>
- <%= debate.description %> + <%= wysiwyg(debate.description) %>
diff --git a/app/views/moderation/proposals/index.html.erb b/app/views/moderation/proposals/index.html.erb index 8ecbaddf5..1d4f0f2cf 100644 --- a/app/views/moderation/proposals/index.html.erb +++ b/app/views/moderation/proposals/index.html.erb @@ -37,7 +37,7 @@ <%= proposal.author.username %>
- <%= proposal.description %> + <%= wysiwyg(proposal.description) %>
diff --git a/app/views/polls/show.html.erb b/app/views/polls/show.html.erb index f639376fc..04ceb286f 100644 --- a/app/views/polls/show.html.erb +++ b/app/views/polls/show.html.erb @@ -67,7 +67,7 @@
<% if answer.description.present? %> - <%= answer.description %> + <%= wysiwyg(answer.description) %> <% end %> <% if answer.images.any? %> diff --git a/app/views/proposals/_info.html.erb b/app/views/proposals/_info.html.erb index 69fedefcd..f6c5e07b3 100644 --- a/app/views/proposals/_info.html.erb +++ b/app/views/proposals/_info.html.erb @@ -39,7 +39,7 @@
<% 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) %>
diff --git a/app/views/tracking/budget_investments/edit.html.erb b/app/views/tracking/budget_investments/edit.html.erb index 9a0cf01fd..09a4d30fa 100644 --- a/app/views/tracking/budget_investments/edit.html.erb +++ b/app/views/tracking/budget_investments/edit.html.erb @@ -7,6 +7,6 @@

<%= @investment.title %>

-<%= auto_link_already_sanitized_html @investment.description %> +<%= auto_link_already_sanitized_html wysiwyg(@investment.description) %> <%= render "tracking/milestones/milestones", milestoneable: @investment %> diff --git a/app/views/tracking/budget_investments/show.html.erb b/app/views/tracking/budget_investments/show.html.erb index 78c118604..1bf3b740e 100644 --- a/app/views/tracking/budget_investments/show.html.erb +++ b/app/views/tracking/budget_investments/show.html.erb @@ -3,7 +3,7 @@

<%= t("tracking.budget_investments.show.title") %> <%= @investment.id %>

<%= @investment.title %>

-<%= auto_link_already_sanitized_html @investment.description %> +<%= auto_link_already_sanitized_html wysiwyg(@investment.description) %> <% if @investment.external_url.present? %>

<%= sanitize_and_auto_link @investment.external_url %>

diff --git a/app/views/welcome/_recommended_carousel.html.erb b/app/views/welcome/_recommended_carousel.html.erb index c78c1c32d..15d445593 100644 --- a/app/views/welcome/_recommended_carousel.html.erb +++ b/app/views/welcome/_recommended_carousel.html.erb @@ -16,7 +16,7 @@ <%= link_to recommended_path(recommended) do %>

<%= recommended.title %>

<% end %> - <%= recommended.description %> + <%= wysiwyg(recommended.description) %>
diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb index 32114b89c..a545b7e78 100644 --- a/spec/features/xss_spec.rb +++ b/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) diff --git a/spec/models/budget/phase_spec.rb b/spec/models/budget/phase_spec.rb index a54634724..921d9eca1 100644 --- a/spec/models/budget/phase_spec.rb +++ b/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: '

a

') - end.to change { first_phase.description }.to('

a

javascript') - end - end end diff --git a/spec/models/budget_spec.rb b/spec/models/budget_spec.rb index 4859621f5..043709823 100644 --- a/spec/models/budget_spec.rb +++ b/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 diff --git a/spec/shared/models/sanitizable.rb b/spec/shared/models/sanitizable.rb index 542bc8126..2e4faa05b 100644 --- a/spec/shared/models/sanitizable.rb +++ b/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 = "" - - sanitizable.valid? - - expect(sanitizable.description).to eq("alert('danger');") - end - - it "is html_safe" do - sanitizable.description = "" - - sanitizable.valid? - - expect(sanitizable.description).to be_html_safe - end - - it "is sanitized using globalize accessors" do - sanitizable.description_en = "" - - sanitizable.valid? - - expect(sanitizable.description_en).to eq("alert('danger');") - end - - it "is html_safe using globalize accessors" do - sanitizable.description_en = "" - - sanitizable.valid? - - expect(sanitizable.description_en).to be_html_safe - end - describe "#tag_list" do before do unless described_class.included_modules.include?(Taggable) From 68ca29fa8bc970ee516c27bff23591f10412edd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 21 Oct 2019 21:29:10 +0200 Subject: [PATCH 3/4] Convert markdown to HTML on demand We were converting markdown to HTML every time we saved a record, which has the same problems as sanitizing HTML before saving it to the database, particularly because the body of a legislation draft is stored in a translations table. Performance-wise this isn't a problem: converting a text with more than 200_000 characters takes about a milisecond on my machine. Note we need to modify a migration generated by globalize, since the method `create_translation_table!` would fail now that we don't define `translates :body_html` in the model. --- app/models/legislation/draft_version.rb | 23 ++++++------------- .../legislation/draft_versions/show.html.erb | 2 +- ..._collaborative_legislation_translations.rb | 8 ++++--- ...l_fields_form_legislation_draft_version.rb | 8 +++++++ db/schema.rb | 4 ---- spec/features/xss_spec.rb | 9 ++++++++ spec/models/legislation/draft_version_spec.rb | 12 +--------- 7 files changed, 31 insertions(+), 35 deletions(-) create mode 100644 db/migrate/20191006114603_remove_html_fields_form_legislation_draft_version.rb diff --git a/app/models/legislation/draft_version.rb b/app/models/legislation/draft_version.rb index 56a42fd07..e9da06546 100644 --- a/app/models/legislation/draft_version.rb +++ b/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) - end + 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 diff --git a/app/views/legislation/draft_versions/show.html.erb b/app/views/legislation/draft_versions/show.html.erb index 8259c74fa..3429c3e03 100644 --- a/app/views/legislation/draft_versions/show.html.erb +++ b/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"] }) %>
diff --git a/db/migrate/20180801140800_add_collaborative_legislation_translations.rb b/db/migrate/20180801140800_add_collaborative_legislation_translations.rb index 80d9d1f5b..607801492 100644 --- a/db/migrate/20180801140800_add_collaborative_legislation_translations.rb +++ b/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 } diff --git a/db/migrate/20191006114603_remove_html_fields_form_legislation_draft_version.rb b/db/migrate/20191006114603_remove_html_fields_form_legislation_draft_version.rb new file mode 100644 index 000000000..4beaf22b9 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 692ec3f82..7e762e9a8 100644 --- a/db/schema.rb +++ b/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 diff --git a/spec/features/xss_spec.rb b/spec/features/xss_spec.rb index a545b7e78..8e9032fe5 100644 --- a/spec/features/xss_spec.rb +++ b/spec/features/xss_spec.rb @@ -153,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 diff --git a/spec/models/legislation/draft_version_spec.rb b/spec/models/legislation/draft_version_spec.rb index 4052b40b3..33eadb98d 100644 --- a/spec/models/legislation/draft_version_spec.rb +++ b/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 From 17ae9c96dc100fccefb68fc1a12f26b2ff569a15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2019 17:11:20 +0200 Subject: [PATCH 4/4] Fix typos sanitizing checkbox labels --- lib/consul_form_builder.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/consul_form_builder.rb b/lib/consul_form_builder.rb index e5b77f69d..86c84589f 100644 --- a/lib/consul_form_builder.rb +++ b/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