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)