Skip to content

Commit

Permalink
Improve the content presneters to display rich content properly
Browse files Browse the repository at this point in the history
- The content presenters previously allowed some unwanted tags
  (links, images), this is now fixed
- Allow specific tags in the content areas (bold and italics) for
  rich text capabilities
  • Loading branch information
ahukkanen committed Mar 7, 2019
1 parent e9a19f7 commit 50a3521
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 18 deletions.
11 changes: 6 additions & 5 deletions app/commands/decidim/plans/admin/export_plans_to_budgets.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ module Admin
# A command with all the business logic when an admin exports plans to a
# single budget component.
class ExportPlansToBudgets < Rectify::Command
include ActionView::Helpers::TextHelper
include Decidim::Plans::RichPresenter

# Public: Initializes the command.
#
Expand Down Expand Up @@ -85,13 +85,14 @@ def project_description(original_plan)
original_plan.sections.each do |section|
content = original_plan.contents.find_by(section: section)
content.body.each do |locale, body_text|
title = sanitize(section.body[locale])
title = plain_content(section.body[locale])
pr_desc[locale] ||= ""
pr_desc[locale] += "<h3>#{title}</h3>\n"

# Wrap non-HTML strings within a <p> tag and replace newlines with
# <br>. This also takes care of sanitization.
pr_desc[locale] += simple_format(body_text)
# <br>. This also takes care of sanitization and specific styling
# of the text, such as bold, italics, etc.
pr_desc[locale] += rich_content(body_text)
end
end

Expand All @@ -103,7 +104,7 @@ def project_description(original_plan)

def sanitize_localized(hash)
hash.each do |locale, value|
hash[locale] = sanitize(value)
hash[locale] = plain_content(value)
end
end
end
Expand Down
12 changes: 11 additions & 1 deletion app/presenters/concerns/decidim/plans/rich_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,18 @@ module RichPresenter
extend ActiveSupport::Concern
include ActionView::Helpers::TextHelper

def plain_content(content)
sanitize(content, tags: [])
end

def rich_content(content)
simple_format(content, wrapper_tag: nil)
simple_format(sanitize(content, tags: allowed_rich_tags), wrapper_tag: nil)
end

protected

def allowed_rich_tags
["strong", "em", "b", "i"]
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/presenters/decidim/plans/content_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ def content
end

def title
sanitize(translated_attribute(content.title))
plain_content(translated_attribute(content.title))
end

def body
Expand Down
6 changes: 3 additions & 3 deletions app/presenters/decidim/plans/plan_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,16 @@ def plan_path
end

def title
sanitize(translated_attribute(plan.title))
plain_content(translated_attribute(plan.title))
end

def body
fields = plan.sections.map do |section|
content = plan.contents.find_by(section: section)
next if content.nil?

section_title = sanitize(translated_attribute(content.title))
section_body = sanitize(translated_attribute(content.body))
section_title = plain_content(translated_attribute(content.title))
section_body = plain_content(translated_attribute(content.body))
"<dt>#{section_title}</dt> <dd>#{section_body}</dd>"
end

Expand Down
19 changes: 15 additions & 4 deletions spec/presenters/decidim/plans/content_presenter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,21 @@ module Plans
let(:plan) { create(:plan) }
let(:content) { create(:content, plan: plan) }

let(:malicious_content_array) do
[
"<script>alert('XSS');</script>",
"<img src='https://www.decidim.org'>",
"<a href='http://www.decidim.org'>Link</a>"
]
end
let(:malicious_content) { malicious_content_array.join("\n") }

describe "#title" do
it "returns title in current locale" do
expect(subject.title).to eq(content.section.body["en"])
end

context "when title contains malicious HTML" do
let(:malicious_content) { "<script>alert('XSS');</script>" }
let(:section) do
create(
:section,
Expand All @@ -25,7 +33,9 @@ module Plans
let(:content) { create(:content, section: section, plan: plan) }

it "sanitizes the HTML" do
expect(subject.title).not_to include(malicious_content)
malicious_content_array.each do |mc|
expect(subject.title).not_to include(mc)
end
end
end
end
Expand All @@ -36,7 +46,6 @@ module Plans
end

context "when body contains malicious HTML" do
let(:malicious_content) { "<script>alert('XSS');</script>" }
let(:content) do
create(
:content,
Expand All @@ -46,7 +55,9 @@ module Plans
end

it "sanitizes the HTML" do
expect(subject.body).not_to include(malicious_content)
malicious_content_array.each do |mc|
expect(subject.body).not_to include(mc)
end
end
end
end
Expand Down
19 changes: 15 additions & 4 deletions spec/presenters/decidim/plans/plan_presenter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ module Plans
let(:component_id) { plan.component.id }
let(:process_slug) { plan.component.participatory_space.slug }

let(:malicious_content_array) do
[
"<script>alert('XSS');</script>",
"<img src='https://www.decidim.org'>",
"<a href='http://www.decidim.org'>Link</a>"
]
end
let(:malicious_content) { malicious_content_array.join("\n") }

describe "#author" do
it "returns Decidim::UserPresenter" do
expect(subject.author).to be_a(Decidim::UserPresenter)
Expand All @@ -31,7 +40,6 @@ module Plans
end

context "when title contains malicious HTML" do
let(:malicious_content) { "<script>alert('XSS');</script>" }
let(:plan) do
create(
:plan,
Expand All @@ -40,7 +48,9 @@ module Plans
end

it "sanitizes the HTML" do
expect(subject.title).not_to include(malicious_content)
malicious_content_array.each do |mc|
expect(subject.title).not_to include(mc)
end
end
end
end
Expand All @@ -60,7 +70,6 @@ module Plans
end

context "when body contains malicious HTML" do
let(:malicious_content) { "<script>alert('XSS');</script>" }
let(:section) do
create(
:section,
Expand All @@ -79,7 +88,9 @@ module Plans
end

it "sanitizes the HTML" do
expect(subject.body).not_to include(malicious_content)
malicious_content_array.each do |mc|
expect(subject.title).not_to include(mc)
end
end
end
end
Expand Down

0 comments on commit 50a3521

Please sign in to comment.