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] 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)