Refactor markdown sanitize (#824)

* 重构 Markdown 的 HTML 结果处理方式，去掉 body_html，改为读取的时候转换 + cache.
用 sanitize gem 来处理 HTML 安全的问题，同时修复 YouTube iframe 的支持。

Author: huacnlee

Gemfile

@@ -15,6 +15,8 @@ gem 'turbolinks', '~> 5.0.0'
 gem 'dropzonejs-rails'
 gem 'rails_autolink'
 
+gem 'sanitize'
+
 gem 'pg'
 gem 'pghero'
 

Gemfile.lock

@@ -1,6 +1,6 @@
 GIT
   remote: https://github.com/elasticsearch/elasticsearch-rails
-  revision: 15761247f3e99654bda946a178e50b5365414b59
+  revision: b6d485748c71a07d064ea2a46a6da82d64a04cd7
   specs:
     elasticsearch-model (0.1.9)
       activesupport (> 3)
@@ -111,6 +111,7 @@ GEM
     coffee-script-source (1.11.1)
     concurrent-ruby (1.0.2)
     connection_pool (2.2.1)
+    crass (1.0.2)
     dalli (2.7.6)
     database_cleaner (1.5.3)
     debug_inspector (0.0.2)
@@ -160,7 +161,7 @@ GEM
     faraday (0.9.2)
       multipart-post (>= 1.2, < 3)
     ffi (1.9.14)
-    font-awesome-rails (4.7.0.0)
+    font-awesome-rails (4.7.0.1)
       railties (>= 3.2, < 5.1)
     get_process_mem (0.2.1)
     globalid (0.3.7)
@@ -195,7 +196,7 @@ GEM
       rails-dom-testing (>= 1, < 3)
       railties (>= 4.2.0)
       thor (>= 0.14, < 2.0)
-    json (2.0.2)
+    json (1.8.3)
     jsonapi (0.1.1.beta2)
       json (~> 1.8)
     jwt (1.5.6)
@@ -229,6 +230,8 @@ GEM
     nio4r (1.2.1)
     nokogiri (1.6.8.1)
       mini_portile2 (~> 2.1.0)
+    nokogumbo (1.4.10)
+      nokogiri
     notifications (0.2.0)
       rails (>= 4.2.0, < 5.1)
       will_paginate (>= 3.0.0)
@@ -363,6 +366,10 @@ GEM
     ruby_dep (1.5.0)
     rucaptcha (1.2.0)
       railties (>= 3.2)
+    sanitize (4.4.0)
+      crass (~> 1.0.2)
+      nokogiri (>= 1.4.4)
+      nokogumbo (~> 1.4.1)
     sass (3.4.22)
     sass-rails (5.0.6)
       railties (>= 4.0.0, < 6)
@@ -414,7 +421,7 @@ GEM
     turbolinks (5.0.1)
       turbolinks-source (~> 5)
     turbolinks-source (5.0.0)
-    twemoji (3.0.2)
+    twemoji (3.1.0)
       nokogiri (~> 1.6.2)
     tzinfo (1.2.2)
       thread_safe (~> 0.1)
@@ -501,6 +508,7 @@ DEPENDENCIES
   rubocop (~> 0.39.0)
   ruby-push-notifications
   rucaptcha
+  sanitize
   sass-rails
   second_level_cache
   sidekiq

app/controllers/replies_controller.rb

@@ -24,7 +24,7 @@ def index
       return
     end
 
-    @replies = Reply.unscoped.where('topic_id = ? and id > ?', @topic.id, last_id).without_body.order(:id).all
+    @replies = Reply.unscoped.where('topic_id = ? and id > ?', @topic.id, last_id).order(:id).all
     if current_user
       current_user.read_topic(@topic, replies_ids: @replies.collect(&:id))
     end

app/controllers/topics_controller.rb

@@ -32,7 +32,7 @@ def index
   end
 
   def feed
-    @topics = Topic.without_hide_nodes.recent.without_body.limit(20).includes(:node, :user, :last_reply_user)
+    @topics = Topic.without_hide_nodes.recent.limit(20).includes(:node, :user, :last_reply_user)
     render layout: false if stale?(@topics)
   end
 
@@ -49,7 +49,7 @@ def node
 
   def node_feed
     @node = Node.find(params[:id])
-    @topics = @node.topics.recent.without_body.limit(20)
+    @topics = @node.topics.recent.limit(20)
     render layout: false if stale?([@node, @topics])
   end
 
@@ -93,7 +93,7 @@ def show
     @show_raw = params[:raw] == '1'
     @can_reply = can? :create, Reply
 
-    @replies = Reply.unscoped.where(topic_id: @topic.id).without_body.order(:id).all
+    @replies = Reply.unscoped.where(topic_id: @topic.id).order(:id).all
 
     check_current_user_liked_replies
     check_current_user_status_for_topic

app/helpers/application_helper.rb

@@ -1,16 +1,14 @@
 module ApplicationHelper
-  ALLOW_TAGS = %w(p br img h1 h2 h3 h4 h5 h6 blockquote pre code b i
-                  strong em table tr td tbody th strike del u a ul ol li span hr)
-  ALLOW_ATTRIBUTES = %w(href src class width height id title alt target rel data-floor frameborder allowfullscreen)
   EMPTY_STRING = ''.freeze
 
   def markdown(text)
-    sanitize_markdown(Homeland::Markdown.call(text))
+    Rails.cache.fetch(['markdown', Digest::MD5.hexdigest(text)]) do
+      raw sanitize_markdown(Homeland::Markdown.call(text))
+    end
   end
 
-  def sanitize_markdown(body)
-    # TODO: This method slow, 3.5ms per call in topic body
-    sanitize(body, tags: ALLOW_TAGS, attributes: ALLOW_ATTRIBUTES)
+  def sanitize_markdown(html)
+    Sanitize.fragment(html, Homeland::Sanitize::DEFAULT)
   end
 
   def notice_message

app/models/comment.rb

@@ -1,6 +1,5 @@
 class Comment < ApplicationRecord
   include MarkdownBody
-
   belongs_to :commentable, polymorphic: true
   belongs_to :user
 

app/models/concerns/markdown_body.rb

@@ -1,18 +1,9 @@
 module MarkdownBody
   extend ActiveSupport::Concern
-  include ActionView::Helpers::SanitizeHelper
+  include ActionView::Helpers::OutputSafetyHelper
   include ApplicationHelper
 
-  included do
-    before_save :markdown_body
-    scope :without_body, -> { without(:body) }
-  end
-
-  private
-
-  def markdown_body
-    if self.body_changed?
-      self.body_html = sanitize_markdown(Homeland::Markdown.call(body))
-    end
+  def body_html
+    markdown(body)
   end
 end

app/models/page_version.rb

@@ -1,4 +1,6 @@
 class PageVersion < ApplicationRecord
+  include MarkdownBody
+
   belongs_to :user
   belongs_to :page
 end

app/models/reply.rb

@@ -1,7 +1,7 @@
 require 'digest/md5'
 class Reply < ApplicationRecord
-  include SoftDelete
   include MarkdownBody
+  include SoftDelete
   include Likeable
   include Mentionable
   include MentionTopic
@@ -16,8 +16,7 @@ class Reply < ApplicationRecord
   delegate :login, to: :user, prefix: true, allow_nil: true
 
   scope :without_system, -> { where(action: nil) }
-  scope :fields_for_list, -> { select(:topic_id, :id, :body_html, :updated_at, :created_at) }
-  scope :without_body, -> { select(column_names - ['body']) }
+  scope :fields_for_list, -> { select(:topic_id, :id, :body, :updated_at, :created_at) }
 
   validates :body, presence: true, unless: -> { system_event? }
   validates :body, uniqueness: { scope: [:topic_id, :user_id], message: '不能重复提交。' }, unless: -> { system_event? }

app/models/topic.rb

@@ -10,8 +10,8 @@
 ]
 
 class Topic < ApplicationRecord
-  include Likeable
   include MarkdownBody
+  include Likeable
   include SoftDelete
   include Mentionable
   include Closeable
@@ -45,7 +45,6 @@ class Topic < ApplicationRecord
   scope :popular,            -> { where('likes_count > 5') }
   scope :excellent,          -> { where('excellent >= 1') }
   scope :without_hide_nodes, -> { exclude_column_ids('node_id', Topic.topic_index_hide_node_ids) }
-  scope :without_body,       -> { select(column_names - ['body']) }
   scope :without_node_ids,   -> (ids) { exclude_column_ids('node_id', ids) }
   scope :exclude_column_ids, lambda { |column, ids|
     if ids.empty?
@@ -99,7 +98,7 @@ def related_topics(size = 5)
   end
 
   def self.fields_for_list
-    columns = %w(body body_html who_deleted follower_ids)
+    columns = %w(body who_deleted follower_ids)
     select(column_names - columns.map(&:to_s))
   end
 

app/serializers/reply_serializer.rb

@@ -2,7 +2,6 @@
 #
 # == attributes
 # - *id* [Integer] 编号
-# - *body_html* [String] 以转换成 HTML 的正文
 # - *topic_id* [Integer] 话题编号
 # - *deleted* [Boolean] 是否已删除
 # - *likes_count* [Integer] 赞数量

app/views/admin/page_versions/show.html.erb

@@ -17,4 +17,4 @@
   Title: <%= @page_version.title %>
 </div>
 
-<%= markdown @page_version.body %>
+<%= @page_version.body_html %>

app/views/comments/_comment.html.erb

@@ -5,7 +5,7 @@
         <%= user_name_tag(item.user) %> <%= t("common.published_at") %> <%= timeago(item.created_at) %>
       </div>
       <div class="body markdown">
-        <%= sanitize item.body_html %>
+        <%= item.body_html %>
       </div>
     </div>
 </div>

app/views/notifications/_mention.html.erb

@@ -11,7 +11,7 @@
   </div>
   <% if reply.present? %>
     <div class="media-content summary markdown">
-      <%= raw reply.body_html %>
+      <%= reply.body_html %>
     </div>
   <% end %>
 <% when Topic %>
@@ -26,7 +26,7 @@
       在 <%= topic_title_tag(topic) %> 提及你：
     </div>
     <div class="media-content summary markdown">
-      <%= raw topic.body_html %>
+      <%= topic.body_html %>
     </div>
   <% end %>
 <% end %>

app/views/notifications/_topic_reply.erb

@@ -12,7 +12,7 @@
   </div>
   <%- if reply.present? -%>
     <div class="media-content summary markdown">
-      <%= raw reply.body_html %>
+      <%= reply.body_html %>
     </div>
   <%- end -%>
 <% end %>

app/views/pages/show.html.erb

@@ -24,7 +24,7 @@
 
       <div class="panel-body markdown">
         <article>
-          <%= raw @page.body_html %>
+          <%= @page.body_html %>
         </article>
       </div>
 

app/views/replies/_reply.html.erb

@@ -35,7 +35,7 @@
         </span>
       </div>
       <div class="markdown<%= ' deleted' if reply.deleted? %>">
-        <%= raw reply.body_html %>
+        <%= reply.body_html %>
       </div>
     </div>
   <% end %>

app/views/shared/_comment.html.erb

@@ -7,7 +7,7 @@
       <span class="name"><%= user_name_tag(comment.user) %></span> 发表于 <%= timeago(comment.created_at) %>
     </div>
     <div class="body markdown">
-      <%= raw comment.body_html %>
+      <%= comment.body_html %>
     </div>
   </div>
 </div>

app/views/topic_mailer/new_reply.html.erb

@@ -3,7 +3,7 @@
    <%= user_name_tag(@reply.user) %>  回复了 《<%= topic_title_tag(@topic)%>》:</label> <br />
 </p>
 <blockquote>
-  <p><%= raw @reply.body_html %><br/></p>
+  <p><%= @reply.body_html %><br/></p>
 </blockquote>
 <p>
  <label> 帖子地址:</label> <%= topic_title_tag(@topic) %>.

app/views/topics/feed.builder

@@ -8,7 +8,7 @@ xml.rss(version: "2.0"){
       for topic in @topics
         xml.item do
           xml.title topic.title
-          xml.description raw(topic.body_html)
+          xml.description markdown(topic.body)
           xml.author topic.user.login
           xml.pubDate(topic.created_at.strftime("%a, %d %b %Y %H:%M:%S %z"))
           xml.link topic_url topic

app/views/topics/node_feed.builder

@@ -7,7 +7,7 @@ xml.rss version: "2.0" do
     for topic in @topics
       xml.item do
         xml.title topic.title
-        xml.description raw(topic.body_html)
+        xml.description markdown(topic.body)
         xml.author topic.user.login
         xml.pubDate topic.created_at.strftime("%a, %d %b %Y %H:%M:%S %z")
         xml.link topic_url(topic)

app/views/topics/show.html.erb

@@ -36,7 +36,7 @@
         <%= raw Setting.before_topic_html %>
 
         <article>
-          <%= raw @topic.body_html %>
+          <%= @topic.body_html %>
         </article>
 
 

app/views/users/_replies.html.erb

@@ -8,7 +8,7 @@
           <span class="info">at <%= timeago(reply.created_at) %></span>
         </div>
         <div class="body markdown">
-          <%= raw reply.body_html %>
+          <%= reply.body_html %>
         </div>
       </li>
     <% end %>

db/migrate/20161215014636_remove_body_html_field.rb

@@ -0,0 +1,8 @@
+class RemoveBodyHtmlField < ActiveRecord::Migration[5.0]
+  def change
+    remove_column :topics, :body_html
+    remove_column :replies, :body_html
+    remove_column :pages, :body_html
+    remove_column :comments, :body_html
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160912124102) do
+ActiveRecord::Schema.define(version: 20161215014636) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -26,7 +26,6 @@
 
   create_table "comments", force: :cascade do |t|
     t.text     "body",             null: false
-    t.text     "body_html"
     t.integer  "user_id",          null: false
     t.string   "commentable_type"
     t.integer  "commentable_id"
@@ -164,7 +163,6 @@
     t.string   "slug",                           null: false
     t.string   "title",                          null: false
     t.text     "body",                           null: false
-    t.text     "body_html"
     t.boolean  "locked",         default: false
     t.integer  "version",        default: 0,     null: false
     t.integer  "editor_ids",     default: [],    null: false, array: true
@@ -189,7 +187,6 @@
     t.integer  "user_id",                         null: false
     t.integer  "topic_id",                        null: false
     t.text     "body",                            null: false
-    t.text     "body_html"
     t.integer  "state",              default: 1,  null: false
     t.integer  "liked_user_ids",     default: [],              array: true
     t.integer  "likes_count",        default: 0
@@ -264,7 +261,6 @@
     t.integer  "node_id",                               null: false
     t.string   "title",                                 null: false
     t.text     "body",                                  null: false
-    t.text     "body_html"
     t.integer  "last_reply_id"
     t.integer  "last_reply_user_id"
     t.string   "last_reply_user_login"
@@ -348,5 +344,4 @@
     t.index ["login"], name: "index_users_on_login", using: :btree
     t.index ["unlock_token"], name: "index_users_on_unlock_token", unique: true, using: :btree
   end
-
 end