Publify - Stored Cross-Site Scripting (XSS) in Editor

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

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

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

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

@@ -1,4 +1,4 @@
 <% cache article do %>
-  <%= raw article.html(:body) %>
-  <%= raw article.html(:extended) %>
+  <%= article.html(:body) %>
+  <%= article.html(:extended) %>
 <% end %>

@@ -1,3 +1,3 @@
 <div id="viewpage">
-  <%= raw html @page %>
+  <%= html @page %>
 </div>

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

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

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

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

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

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

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

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

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

@@ -1,2 +1,2 @@
 <h1 class='page-header'><%= link_to_permalink(@page, @page.title) %></h1>
-<%= raw @page.html %>
+<%= @page.html %>

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