Skip to content
Permalink
Browse files Browse the repository at this point in the history
Ensure all Content html is sanitized
- Require all content to be html_safe before displaying
- Sanitize all content html, not just Comments
- Make nofollowify_links take and return html_safe strings
  • Loading branch information
mvz committed Oct 10, 2021
1 parent dd72fb7 commit fefd5f7
Show file tree
Hide file tree
Showing 17 changed files with 102 additions and 41 deletions.
7 changes: 6 additions & 1 deletion publify_core/app/helpers/base_helper.rb
Expand Up @@ -240,10 +240,15 @@ def fetch_html_content_for_feeds(item, this_blog)
end

def nofollowify_links(string)
raise ArgumentError, "string", "must be html_safe" unless string.html_safe?

if this_blog.dofollowify
string
else
string.gsub(/<a(.*?)>/i, '<a\1 rel="nofollow">')
followify_scrubber = Loofah::Scrubber.new do |node|
node.set_attribute "rel", "nofollow" if node.name == "a"
end
sanitize h(string), scrubber: followify_scrubber
end
end

Expand Down
12 changes: 9 additions & 3 deletions publify_core/app/models/content_base.rb
Expand Up @@ -5,6 +5,12 @@ def self.included(base)
base.extend ClassMethods
end

class ContentTextHelpers
include ActionView::Helpers::UrlHelper
include ActionView::Helpers::TextHelper
include ActionView::Helpers::SanitizeHelper
end

attr_accessor :just_changed_published_status
alias just_changed_published_status? just_changed_published_status

Expand Down Expand Up @@ -39,10 +45,10 @@ def generate_html(field, text = nil)
html_postprocess(field, html).to_s
end

# Post-process the HTML. This is a noop by default, but Comment overrides it
# to enforce HTML sanity.
# Post-process the HTML
def html_postprocess(_field, html)
html
helper = ContentTextHelpers.new
helper.sanitize html
end

def html_preprocess(_field, html)
Expand Down
6 changes: 0 additions & 6 deletions publify_core/app/models/feedback.rb
Expand Up @@ -11,12 +11,6 @@ class Feedback < ApplicationRecord
include PublifyGuid
include ContentBase

class ContentTextHelpers
include ActionView::Helpers::UrlHelper
include ActionView::Helpers::TextHelper
include ActionView::Helpers::SanitizeHelper
end

validate :feedback_not_closed, on: :create
validates :article, presence: true

Expand Down
2 changes: 1 addition & 1 deletion publify_core/app/views/articles/_article_excerpt.html.erb
Expand Up @@ -5,7 +5,7 @@
<p><%= link_to_permalink article, t('.continue_reading') %></p>
</div>
<% else %>
<%= raw article.html(:body) %>
<%= article.html(:body) %>
<% if article.extended? %>
<div class="extended">
<p><%= link_to_permalink article, t('.continue_reading') %></p>
Expand Down
@@ -1,4 +1,4 @@
<% cache article do %>
<%= raw article.html(:body) %>
<%= raw article.html(:extended) %>
<%= article.html(:body) %>
<%= article.html(:extended) %>
<% end %>
2 changes: 1 addition & 1 deletion publify_core/app/views/articles/view_page.html.erb
@@ -1,3 +1,3 @@
<div id="viewpage">
<%= raw html @page %>
<%= html @page %>
</div>
2 changes: 1 addition & 1 deletion publify_core/app/views/comments/_comment.html.erb
Expand Up @@ -6,7 +6,7 @@
<%= t('.said') %> <%= display_date_and_time comment.created_at %>:
</p>
<div class="content">
<%= raw nofollowify_links comment.generate_html(:body) %>
<%= nofollowify_links comment.generate_html(:body) %>
<% unless comment.published? %>
<div class="spamwarning">
<%= t('.this_comment_has_been_flagged_for_moderator_approval') %>
Expand Down
2 changes: 1 addition & 1 deletion publify_core/app/views/notes/_note.html.erb
@@ -1,7 +1,7 @@
<% cache [note, note.user] do %>
<article class='status'>
<%= author_picture note %>
<div class='p-name entry-title e-content entry-content article'><%= raw note.html(:body) %></div>
<div class='p-name entry-title e-content entry-content article'><%= note.html(:body) %></div>
<footer>
<small>
<%= link_to_permalink(note, display_date_and_time(note.published_at)) %> |
Expand Down
2 changes: 1 addition & 1 deletion publify_core/app/views/notes/index.html.erb
Expand Up @@ -2,7 +2,7 @@
<% for note in @notes %>
<div class='h-entry hentry h-as-note'>
<article>
<p class='p-name entry-title e-content entry-content article'><%= raw note.html(:body) %></p>
<p class='p-name entry-title e-content entry-content article'><%= note.html(:body) %></p>
<footer>
<small><%= link_to_permalink(note, display_date_and_time(note.published_at)) %></small>
</footer>
Expand Down
24 changes: 19 additions & 5 deletions publify_core/spec/helpers/base_helper_spec.rb
Expand Up @@ -160,6 +160,8 @@ def parse_request(_contents, _request_params)
end

describe "#nofollowify_links" do
let(:original_html) { '<a href="http://myblog.net">my blog</a>'.html_safe }

before do
@blog = create :blog
end
Expand All @@ -168,16 +170,28 @@ def parse_request(_contents, _request_params)
@blog.dofollowify = false
@blog.save

expect(nofollowify_links('<a href="http://myblog.net">my blog</a>')).
to eq('<a href="http://myblog.net" rel="nofollow">my blog</a>')
result = nofollowify_links(original_html)

aggregate_failures do
expect(result).to eq('<a href="http://myblog.net" rel="nofollow">my blog</a>')
expect(result).to be_html_safe
end
end

it "with dofollowify enabled, links should be nofollowed" do
it "with dofollowify enabled, links should be not nofollowed" do
@blog.dofollowify = true
@blog.save

expect(nofollowify_links('<a href="http://myblog.net">my blog</a>')).
to eq('<a href="http://myblog.net">my blog</a>')
result = nofollowify_links(original_html)

aggregate_failures do
expect(result).to eq('<a href="http://myblog.net">my blog</a>')
expect(result).to be_html_safe
end
end

it "does not accept unsafe html" do
expect { nofollowify_links("just an unsafe string") }.to raise_error ArgumentError
end
end

Expand Down
8 changes: 8 additions & 0 deletions publify_core/spec/models/article_spec.rb
Expand Up @@ -398,6 +398,14 @@
end
end

describe "#html" do
let(:article) { build_stubbed :article }

it "returns an html_safe string" do
expect(article.html).to be_html_safe
end
end

describe "#comment_url" do
it "renders complete url of comment" do
article = build_stubbed(:article, id: 123)
Expand Down
39 changes: 37 additions & 2 deletions publify_core/spec/models/comment_spec.rb
Expand Up @@ -257,10 +257,45 @@ def valid_comment(options = {})
end
end

describe "#generate_html" do
describe "#html" do
it "renders email addresses in the body" do
comment = build_stubbed(:comment, body: "foo@example.com")
expect(comment.generate_html(:body)).to match(/mailto:/)
expect(comment.html).to match(/mailto:/)
end

it "returns an html_safe string" do
comment = build_stubbed(:comment, body: "Just a comment")
expect(comment.html).to be_html_safe
end

context "with an evil comment" do
let(:comment) { build_stubbed :comment, body: "Test foo <script>do_evil();</script>" }
let(:blog) { comment.article.blog }

["", "textile", "markdown", "smartypants", "markdown smartypants"].each do |filter|
it "rejects xss attempt with filter '#{filter}'" do
blog.comment_text_filter = filter

ActiveSupport::Deprecation.silence do
assert comment.html(:body) !~ /<script>/
end
end
end
end

context "with a markdown comment with italic and bold" do
let(:comment) { build(:comment, body: "Comment body _italic_ **bold**") }
let(:blog) { comment.article.blog }

it "converts the comment markup to html" do
blog.comment_text_filter = "markdown"
result = comment.html

aggregate_failures do
expect(result).to match(%r{<em>italic</em>})
expect(result).to match(%r{<strong>bold</strong>})
end
end
end
end
end
15 changes: 0 additions & 15 deletions publify_core/spec/models/content_spec.rb
Expand Up @@ -144,19 +144,4 @@
it { expect(content.author_name).to eq(author.login) }
end
end

describe "#generate_html" do
context "with a blog with markdown filter" do
let!(:blog) { create(:blog, comment_text_filter: "markdown") }

context "comment with italic and bold" do
let(:comment) { build(:comment, body: "Comment body _italic_ **bold**") }

it "converts the comment markup to HTML" do
expect(comment.generate_html(:body)).to match(%r{<em>italic</em>})
expect(comment.generate_html(:body)).to match(%r{<strong>bold</strong>})
end
end
end
end
end
7 changes: 7 additions & 0 deletions publify_core/spec/models/note_spec.rb
Expand Up @@ -255,6 +255,13 @@
it { expect(note.twitter_message.length).to eq(140) }
end
end

describe "#html" do
it "returns an html_safe string" do
note = build(:note, body: "A test tweet with a #hashtag")
expect(note.html).to be_html_safe
end
end
end

context "with a dofollowify blog" do
Expand Down
7 changes: 7 additions & 0 deletions publify_core/spec/models/page_spec.rb
Expand Up @@ -89,4 +89,11 @@
it { expect(page.redirect).to be_blank }
end
end

describe "#html" do
it "returns an html_safe string" do
page = build(:page)
expect(page.html).to be_html_safe
end
end
end
2 changes: 1 addition & 1 deletion themes/bootstrap-2/views/articles/view_page.html.erb
@@ -1,2 +1,2 @@
<h1 class='page-header'><%= link_to_permalink(@page, @page.title) %></h1>
<%= raw @page.html %>
<%= @page.html %>
2 changes: 1 addition & 1 deletion themes/bootstrap-2/views/comments/_comment.html.erb
Expand Up @@ -5,7 +5,7 @@
<%= t('.by') %> <%= comment.url.blank? ? h(comment.author) : nofollowified_link_to(h(comment.author), comment.url) %>
<%= display_date_and_time comment.created_at %>
</h4>
<%= raw comment.html %>
<%= comment.html %>
<%- unless comment.published? %>
<div class="spamwarning"><%= t('.this_comment_has_been_flagged_for_moderator_approval') %></div>
<%- end %>
Expand Down

0 comments on commit fefd5f7

Please sign in to comment.