consul/nairobi
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Sanitize descriptions in the views

Browse Source 
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.
This commit is contained in:
Javi Martín
2019-10-06 03:41:53 +02:00
parent ae2576020e
commit 7bf4e4d611
30 changed files with 47 additions and 129 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
app/models/budget.rb
View File

@@ -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(

11
app/models/budget/phase.rb
View File

@@ -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

4
app/models/concerns/globalizable.rb
View File

@@ -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

37
app/models/concerns/sanitizable.rb
View File

@@ -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

4
app/models/poll/question/answer.rb
View File

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

2
app/views/admin/debates/show.html.erb
View File

@@ -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>

2
app/views/admin/hidden_budget_investments/index.html.erb
View File

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

2
app/views/admin/hidden_debates/index.html.erb
View File

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

2
app/views/admin/hidden_proposals/index.html.erb
View File

@@ -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 %>

2
app/views/admin/poll/questions/answers/show.html.erb
View File

@@ -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) %>

2
app/views/admin/poll/questions/show.html.erb
View File

@@ -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>

2
app/views/budgets/index.html.erb
View File

@@ -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" %>

2
app/views/budgets/investments/_investment.html.erb
View File

@@ -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 %>

2
app/views/budgets/investments/_investment_detail.erb
View File

@@ -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">

2
app/views/budgets/show.html.erb
View File

@@ -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>

2
app/views/debates/_debate.html.erb
View File

@@ -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 %>

2
app/views/debates/show.html.erb
View File

@@ -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 %>

2
app/views/legislation/proposals/show.html.erb
View File

@@ -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">

2
app/views/moderation/budgets/investments/index.html.erb
View File

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

2
app/views/moderation/debates/index.html.erb
View File

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

2
app/views/moderation/proposals/index.html.erb
View File

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

2
app/views/polls/show.html.erb
View File

@@ -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? %>

2
app/views/proposals/_info.html.erb
View File

@@ -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">

2
app/views/tracking/budget_investments/edit.html.erb
View File

@@ -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 %>

2
app/views/tracking/budget_investments/show.html.erb
View File

@@ -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>

2
app/views/welcome/_recommended_carousel.html.erb
View File

@@ -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>

25
spec/features/xss_spec.rb
View File

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

8
spec/models/budget/phase_spec.rb
View File

@@ -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

1
spec/models/budget_spec.rb
View File

@@ -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

32
spec/shared/models/sanitizable.rb
View File

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