CVE-2021-25974: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: In Publify, versions v8.0 to v9.2.4 are vulnerable to stored XSS. A user with a “publisher” role is able to inject and execute arbitrary JavaScript code while creating a page/article.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 5.4
Published: 2021-11-10T11:15Z
Repository: https://github.com/publify/publify
Language: Ruby
Files changed: publify_core/app/helpers/base_helper.rb, publify_core/app/models/content_base.rb, publify_core/app/models/feedback.rb, publify_core/app/views/articles/_article_excerpt.html.erb, publify_core/app/views/articles/_full_article_content.html.erb, publify_core/app/views/articles/view_page.html.erb, publify_core/app/views/comments/_comment.html.erb, publify_core/app/views/notes/_note.html.erb, publify_core/app/views/notes/index.html.erb, publify_core/spec/helpers/base_helper_spec.rb, publify_core/spec/models/article_spec.rb, publify_core/spec/models/comment_spec.rb, publify_core/spec/models/content_spec.rb, publify_core/spec/models/note_spec.rb, publify_core/spec/models/page_spec.rb, themes/bootstrap-2/views/articles/view_page.html.erb, themes/bootstrap-2/views/comments/_comment.html.erb
Commit date: 2021-10-10 19:37:47 +0200
Commit message: 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

Diff: diff --git a/publify_core/app/helpers/base_helper.rb b/publify_core/app/helpers/base_helper.rb
index b7ba0a01c..53c972948 100644
--- a/publify_core/app/helpers/base_helper.rb
+++ b/publify_core/app/helpers/base_helper.rb
@@ -238,14 +238,19 @@ module BaseHelper
       html(item, :all)
     end
   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
 
   def nofollowified_link_to(text, url)
     if this_blog.dofollowify
diff --git a/publify_core/app/models/content_base.rb b/publify_core/app/models/content_base.rb
index 764091830..5349c44d7 100644
--- a/publify_core/app/models/content_base.rb
+++ b/publify_core/app/models/content_base.rb
@@ -3,10 +3,16 @@
 module ContentBase
   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
 
   def really_send_notifications
     interested_users.each do |value|
@@ -37,14 +43,14 @@ module ContentBase
     prehtml = html_preprocess(field, text).to_s
     html = (text_filter || default_text_filter).filter_text(prehtml) || prehtml
     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)
     html
   end
diff --git a/publify_core/app/models/feedback.rb b/publify_core/app/models/feedback.rb
index 5c0b88ae4..984477c7a 100644
--- a/publify_core/app/models/feedback.rb
+++ b/publify_core/app/models/feedback.rb
@@ -9,16 +9,10 @@ class Feedback < ApplicationRecord
   belongs_to :article, touch: true
 
   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
 
   before_save :correct_url, :classify_content
   before_create :create_guid, :article_allows_this_feedback
diff --git a/publify_core/app/views/articles/_article_excerpt.html.erb b/publify_core/app/views/articles/_article_excerpt.html.erb
index ca51d07af..afea7619e 100644
--- a/publify_core/app/views/articles/_article_excerpt.html.erb
+++ b/publify_core/app/views/articles/_article_excerpt.html.erb
@@ -3,11 +3,11 @@
     <%= article.excerpt %>
     <div class="extended">
       <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>
       </div>
     <% end %>
diff --git a/publify_core/app/views/articles/_full_article_content.html.erb b/publify_core/app/views/articles/_full_article_content.html.erb
index fc1169bc0..7a881fa7a 100644
--- a/publify_core/app/views/articles/_full_article_content.html.erb
+++ b/publify_core/app/views/articles/_full_article_content.html.erb
@@ -1,4 +1,4 @@
 <% cache article do %>
-  <%= raw article.html(:body) %>
-  <%= raw article.html(:extended) %>
+  <%= article.html(:body) %>
+  <%= article.html(:extended) %>
 <% end %>
diff --git a/publify_core/app/views/articles/view_page.html.erb b/publify_core/app/views/articles/view_page.html.erb
index 9657e9404..cc5b85eff 100644
--- a/publify_core/app/views/articles/view_page.html.erb
+++ b/publify_core/app/views/articles/view_page.html.erb
@@ -1,3 +1,3 @@
 <div id="viewpage">
-  <%= raw html @page %>
+  <%= html @page %>
 </div>
diff --git a/publify_core/app/views/comments/_comment.html.erb b/publify_core/app/views/comments/_comment.html.erb
index a9a8b705d..7f01dcc85 100644
--- a/publify_core/app/views/comments/_comment.html.erb
+++ b/publify_core/app/views/comments/_comment.html.erb
@@ -4,11 +4,11 @@
     <%= avatar_tag(email: comment.email, url: comment.url) %>
     <cite><strong><%= link_to_unless(comment.url.blank?, h(comment.author), comment.url) %></strong></cite>
     <%= 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') %>
         </div>
       <% end %>
diff --git a/publify_core/app/views/notes/_note.html.erb b/publify_core/app/views/notes/_note.html.erb
index a1c28a9fb..389d96c82 100644
--- a/publify_core/app/views/notes/_note.html.erb
+++ b/publify_core/app/views/notes/_note.html.erb
@@ -1,9 +1,9 @@
 <% 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)) %> |
         <%= link_to note.redirect.from_url, note.redirect.from_url %> |
         <%= author_link note %>
diff --git a/publify_core/app/views/notes/index.html.erb b/publify_core/app/views/notes/index.html.erb
index 9d7f385d5..7ff3a9db1 100644
--- a/publify_core/app/views/notes/index.html.erb
+++ b/publify_core/app/views/notes/index.html.erb
@@ -1,10 +1,10 @@
 <div class="hfeed">
   <% 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>
     </article>
         <hr />
diff --git a/publify_core/spec/helpers/base_helper_spec.rb b/publify_core/spec/helpers/base_helper_spec.rb
index 829bd3b7e..3e0a8408c 100644
--- a/publify_core/spec/helpers/base_helper_spec.rb
+++ b/publify_core/spec/helpers/base_helper_spec.rb
@@ -158,28 +158,42 @@ describe BaseHelper, type: :helper do
         to eq '<a href="http://myblog.net">my blog</a>'
     end
   end
 
   describe "#nofollowify_links" do
+    let(:original_html) { '<a href="http://myblog.net">my blog</a>'.html_safe }
+
     before do
       @blog = create :blog
     end
 
     it "with dofollowify disabled, links should be nofollowed" do
       @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
 
   describe "#render_sidebars" do
     let(:blog) { create :blog }
diff --git a/publify_core/spec/models/article_spec.rb b/publify_core/spec/models/article_spec.rb
index 2798258e2..aeacc71bc 100644
--- a/publify_core/spec/models/article_spec.rb
+++ b/publify_core/spec/models/article_spec.rb
@@ -396,10 +396,18 @@ describe Article, type: :model do
       expect(@article.body).to eq("foo")
       expect(@article.extended).to eq("bar")
     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)
       expect(article.comment_url).
         to eq("#{blog.root_path}/comments?article_id=#{article.id}")
diff --git a/publify_core/spec/models/comment_spec.rb b/publify_core/spec/models/comment_spec.rb
index dcf967f16..138d037fe 100644
--- a/publify_core/spec/models/comment_spec.rb
+++ b/publify_core/spec/models/comment_spec.rb
@@ -255,12 +255,47 @@ describe Comment, type: :model do
       expect(described_class.last_published).to eq([comment_6, comment_5, comment_4,
                                                     comment_3, comment_2])
     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
diff --git a/publify_core/spec/models/content_spec.rb b/publify_core/spec/models/content_spec.rb
index 5cb59d625..35865c0c4 100644
--- a/publify_core/spec/models/content_spec.rb
+++ b/publify_core/spec/models/content_spec.rb
@@ -142,21 +142,6 @@ describe Content, type: :model do
       let(:author) { build(:user, name: "") }
 
       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
diff --git a/publify_core/spec/models/note_spec.rb b/publify_core/spec/models/note_spec.rb
index c7eb50226..88fc793ab 100644
--- a/publify_core/spec/models/note_spec.rb
+++ b/publify_core/spec/models/note_spec.rb
@@ -253,10 +253,17 @@ describe Note, type: :model do
 
         it { expect(note.twitter_message).to eq(expected_tweet) }
         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
     let!(:blog) { create(:blog, dofollowify: true) }
 
diff --git a/publify_core/spec/models/page_spec.rb b/publify_core/spec/models/page_spec.rb
index 0768cd437..500549d5d 100644
--- a/publify_core/spec/models/page_spec.rb
+++ b/publify_core/spec/models/page_spec.rb
@@ -87,6 +87,13 @@ describe Page, type: :model do
       let(:page) { create(:page, state: "draft") }
 
       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
diff --git a/themes/bootstrap-2/views/articles/view_page.html.erb b/themes/bootstrap-2/views/articles/view_page.html.erb
index ed2df7272..7daed45d8 100644
--- a/themes/bootstrap-2/views/articles/view_page.html.erb
+++ b/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 %>
diff --git a/themes/bootstrap-2/views/comments/_comment.html.erb b/themes/bootstrap-2/views/comments/_comment.html.erb
index 1abe22a25..85bdd11d7 100644
--- a/themes/bootstrap-2/views/comments/_comment.html.erb
+++ b/themes/bootstrap-2/views/comments/_comment.html.erb
@@ -3,11 +3,11 @@
   <div class='content'>
     <h4>
       <%= 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 %>
     <hr />
   </div>

Vulnerable code:       string.gsub(/<a(.*?)>/i, '<a\1 rel="nofollow">')
  # Post-process the HTML.  This is a noop by default, but Comment overrides it
  # to enforce HTML sanity.
    html
  class ContentTextHelpers
    include ActionView::Helpers::UrlHelper
    include ActionView::Helpers::TextHelper
    include ActionView::Helpers::SanitizeHelper
  end

    <%= raw article.html(:body) %>
  <%= raw article.html(:body) %>
  <%= raw article.html(:extended) %>
  <%= raw html @page %>
      <%= raw nofollowify_links comment.generate_html(:body) %>
    <div class='p-name entry-title e-content entry-content article'><%= raw note.html(:body) %></div>
      <p class='p-name entry-title e-content entry-content article'><%= raw note.html(:body) %></p>
      expect(nofollowify_links('<a href="http://myblog.net">my blog</a>')).
        to eq('<a href="http://myblog.net" rel="nofollow">my blog</a>')
    it "with dofollowify enabled, links should be nofollowed" do
      expect(nofollowify_links('<a href="http://myblog.net">my blog</a>')).
        to eq('<a href="http://myblog.net">my blog</a>')
  describe "#generate_html" do
      expect(comment.generate_html(:body)).to match(/mailto:/)

  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
<%= raw @page.html %>
    <%= raw comment.html %>
Fixed code:     raise ArgumentError, "string", "must be html_safe" unless string.html_safe?

      followify_scrubber = Loofah::Scrubber.new do |node|
        node.set_attribute "rel", "nofollow" if node.name == "a"
      end
      sanitize h(string), scrubber: followify_scrubber
  class ContentTextHelpers
    include ActionView::Helpers::UrlHelper
    include ActionView::Helpers::TextHelper
    include ActionView::Helpers::SanitizeHelper
  end

  # Post-process the HTML
    helper = ContentTextHelpers.new
    helper.sanitize html
    <%= article.html(:body) %>
  <%= article.html(:body) %>
  <%= article.html(:extended) %>
  <%= html @page %>
      <%= nofollowify_links comment.generate_html(:body) %>
    <div class='p-name entry-title e-content entry-content article'><%= note.html(:body) %></div>
      <p class='p-name entry-title e-content entry-content article'><%= note.html(:body) %></p>
    let(:original_html) { '<a href="http://myblog.net">my blog</a>'.html_safe }

      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
    it "with dofollowify enabled, links should be not nofollowed" do
      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
  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 "#html" do
      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

    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

  describe "#html" do
    it "returns an html_safe string" do
      page = build(:page)
      expect(page.html).to be_html_safe
    end
  end
<%= @page.html %>
    <%= comment.html %>