diff --git a/Gemfile b/Gemfile index 5b4bb8f6ac..d2de5aea7b 100644 --- a/Gemfile +++ b/Gemfile @@ -15,6 +15,8 @@ gem 'turbolinks', '~> 5.0.0' gem 'dropzonejs-rails' gem 'rails_autolink' +gem 'sanitize' + gem 'pg' gem 'pghero' diff --git a/Gemfile.lock b/Gemfile.lock index 73ffc2c31f..5bc05d076a 100644 --- a/Gemfile.lock +++ b/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 diff --git a/app/controllers/replies_controller.rb b/app/controllers/replies_controller.rb index 2f35a3a535..4a8a3712c9 100644 --- a/app/controllers/replies_controller.rb +++ b/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 diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 4d97e3bd47..5805eea67b 100644 --- a/app/controllers/topics_controller.rb +++ b/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 diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index c862483a2c..f9aedc22e7 100644 --- a/app/helpers/application_helper.rb +++ b/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 diff --git a/app/models/comment.rb b/app/models/comment.rb index 05181c89dd..ca0a4525d4 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -1,6 +1,5 @@ class Comment < ApplicationRecord include MarkdownBody - belongs_to :commentable, polymorphic: true belongs_to :user diff --git a/app/models/concerns/markdown_body.rb b/app/models/concerns/markdown_body.rb index cd7d37875e..beacaf3735 100644 --- a/app/models/concerns/markdown_body.rb +++ b/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 diff --git a/app/models/page_version.rb b/app/models/page_version.rb index 04a54c3fa5..7593f187f4 100644 --- a/app/models/page_version.rb +++ b/app/models/page_version.rb @@ -1,4 +1,6 @@ class PageVersion < ApplicationRecord + include MarkdownBody + belongs_to :user belongs_to :page end diff --git a/app/models/reply.rb b/app/models/reply.rb index f95c4b3556..2870f8d613 100644 --- a/app/models/reply.rb +++ b/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? } diff --git a/app/models/topic.rb b/app/models/topic.rb index 7d9ca88c72..20cc213b16 100644 --- a/app/models/topic.rb +++ b/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 diff --git a/app/serializers/reply_serializer.rb b/app/serializers/reply_serializer.rb index 7456be64e8..9845ac17e7 100644 --- a/app/serializers/reply_serializer.rb +++ b/app/serializers/reply_serializer.rb @@ -2,7 +2,6 @@ # # == attributes # - *id* [Integer] 编号 -# - *body_html* [String] 以转换成 HTML 的正文 # - *topic_id* [Integer] 话题编号 # - *deleted* [Boolean] 是否已删除 # - *likes_count* [Integer] 赞数量 diff --git a/app/views/admin/page_versions/show.html.erb b/app/views/admin/page_versions/show.html.erb index ac63dc0b91..782700b6c7 100644 --- a/app/views/admin/page_versions/show.html.erb +++ b/app/views/admin/page_versions/show.html.erb @@ -17,4 +17,4 @@ Title: <%= @page_version.title %> -<%= markdown @page_version.body %> \ No newline at end of file +<%= @page_version.body_html %> \ No newline at end of file diff --git a/app/views/comments/_comment.html.erb b/app/views/comments/_comment.html.erb index 721da0ec10..cbf2c5afe2 100644 --- a/app/views/comments/_comment.html.erb +++ b/app/views/comments/_comment.html.erb @@ -5,7 +5,7 @@ <%= user_name_tag(item.user) %> <%= t("common.published_at") %> <%= timeago(item.created_at) %>
- <%= sanitize item.body_html %> + <%= item.body_html %>
diff --git a/app/views/notifications/_mention.html.erb b/app/views/notifications/_mention.html.erb index 1d66bf53e8..45c0b088f4 100644 --- a/app/views/notifications/_mention.html.erb +++ b/app/views/notifications/_mention.html.erb @@ -11,7 +11,7 @@ <% if reply.present? %>
- <%= raw reply.body_html %> + <%= reply.body_html %>
<% end %> <% when Topic %> @@ -26,7 +26,7 @@ 在 <%= topic_title_tag(topic) %> 提及你:
- <%= raw topic.body_html %> + <%= topic.body_html %>
<% end %> <% end %> diff --git a/app/views/notifications/_topic_reply.erb b/app/views/notifications/_topic_reply.erb index dd73b9e34e..f9e18cb792 100644 --- a/app/views/notifications/_topic_reply.erb +++ b/app/views/notifications/_topic_reply.erb @@ -12,7 +12,7 @@ <%- if reply.present? -%>
- <%= raw reply.body_html %> + <%= reply.body_html %>
<%- end -%> <% end %> diff --git a/app/views/pages/show.html.erb b/app/views/pages/show.html.erb index f32287d0c8..82c1973c3c 100644 --- a/app/views/pages/show.html.erb +++ b/app/views/pages/show.html.erb @@ -24,7 +24,7 @@
- <%= raw @page.body_html %> + <%= @page.body_html %>
diff --git a/app/views/replies/_reply.html.erb b/app/views/replies/_reply.html.erb index 20badb60ac..82d9ae5dd2 100644 --- a/app/views/replies/_reply.html.erb +++ b/app/views/replies/_reply.html.erb @@ -35,7 +35,7 @@
- <%= raw reply.body_html %> + <%= reply.body_html %>
<% end %> diff --git a/app/views/shared/_comment.html.erb b/app/views/shared/_comment.html.erb index 743efb9d5f..cf43506327 100644 --- a/app/views/shared/_comment.html.erb +++ b/app/views/shared/_comment.html.erb @@ -7,7 +7,7 @@ <%= user_name_tag(comment.user) %> 发表于 <%= timeago(comment.created_at) %>
- <%= raw comment.body_html %> + <%= comment.body_html %>
diff --git a/app/views/topic_mailer/new_reply.html.erb b/app/views/topic_mailer/new_reply.html.erb index a2253dde54..d90ed3b51a 100644 --- a/app/views/topic_mailer/new_reply.html.erb +++ b/app/views/topic_mailer/new_reply.html.erb @@ -3,7 +3,7 @@ <%= user_name_tag(@reply.user) %> 回复了 《<%= topic_title_tag(@topic)%>》:

-

<%= raw @reply.body_html %>

+

<%= @reply.body_html %>

<%= topic_title_tag(@topic) %>. diff --git a/app/views/topics/feed.builder b/app/views/topics/feed.builder index cac080173d..9c9e8dbf73 100644 --- a/app/views/topics/feed.builder +++ b/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 diff --git a/app/views/topics/node_feed.builder b/app/views/topics/node_feed.builder index 4500aa45b8..2a5f54c340 100644 --- a/app/views/topics/node_feed.builder +++ b/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) diff --git a/app/views/topics/show.html.erb b/app/views/topics/show.html.erb index 9d57eb83b9..282eaf5e59 100644 --- a/app/views/topics/show.html.erb +++ b/app/views/topics/show.html.erb @@ -36,7 +36,7 @@ <%= raw Setting.before_topic_html %>

- <%= raw @topic.body_html %> + <%= @topic.body_html %>
diff --git a/app/views/users/_replies.html.erb b/app/views/users/_replies.html.erb index b593f3dc36..854957092a 100644 --- a/app/views/users/_replies.html.erb +++ b/app/views/users/_replies.html.erb @@ -8,7 +8,7 @@ at <%= timeago(reply.created_at) %>
- <%= raw reply.body_html %> + <%= reply.body_html %>
<% end %> diff --git a/db/migrate/20161215014636_remove_body_html_field.rb b/db/migrate/20161215014636_remove_body_html_field.rb new file mode 100644 index 0000000000..3fa9238f6f --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 2818f5097b..f68d54658d 100644 --- a/db/schema.rb +++ b/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 diff --git a/lib/homeland/sanitize.rb b/lib/homeland/sanitize.rb new file mode 100644 index 0000000000..1c7453ec03 --- /dev/null +++ b/lib/homeland/sanitize.rb @@ -0,0 +1,54 @@ +module Homeland + module Sanitize + # https://github.com/rgrove/sanitize#example-transformer-to-whitelist-youtube-video-embeds + YOUTUBE_TRANSFORMER = lambda do |env| + node = env[:node] + node_name = env[:node_name] + + # Don't continue if this node is already whitelisted or is not an element. + return if env[:is_whitelisted] || !node.element? + + # Don't continue unless the node is an iframe. + return unless node_name == 'iframe' + + # Verify that the video URL is actually a valid YouTube video URL. + return unless node['src'] =~ %r|\A(?:https?:)?//(?:www\.)?youtube(?:-nocookie)?\.com/| + + # We're now certain that this is a YouTube embed, but we still need to run + # it through a special Sanitize step to ensure that no unwanted elements or + # attributes that don't belong in a YouTube embed can sneak in. + ::Sanitize.node!(node, { + :elements => %w[iframe], + + :attributes => { + 'iframe' => %w[allowfullscreen class frameborder height src width] + } + }) + + # Now that we're sure that this is a valid YouTube embed and that there are + # no unwanted elements or attributes hidden inside it, we can tell Sanitize + # to whitelist the current node. + {:node_whitelist => [node]} + end + + DEFAULT = ::Sanitize::Config.freeze_config( + elements: %w[ + p br img h1 h2 h3 h4 h5 h6 blockquote pre code b i del + strong em table tr td tbody th strike del u a ul ol li span hr + ], + + attributes: ::Sanitize::Config.merge({}, + # 这里要确保是 :all, 而不是 'all' + :all => %w[class id lang style tabindex title translate], + 'a' => %w[href rel data-floor target], + 'img' => %w[alt src width height] + ), + + protocols: { + 'a' => { 'href' => ['http', 'https', 'mailto', :relative] } + }, + + transformers: [YOUTUBE_TRANSFORMER], + ) + end +end \ No newline at end of file diff --git a/spec/api/replies_spec.rb b/spec/api/replies_spec.rb index 3be5c976fe..fa77116a2a 100644 --- a/spec/api/replies_spec.rb +++ b/spec/api/replies_spec.rb @@ -52,9 +52,7 @@ expect(response.status).to eq(200) reply.reload expect(json['reply']['body']).to eq 'bar dar' - expect(json['reply']['body_html']).to eq reply.body_html expect(reply.body).to eq 'bar dar' - expect(reply.body_html).to eq '

bar dar

' end it 'should work by admin' do diff --git a/spec/factories/pages.rb b/spec/factories/pages.rb index 0f737de632..74363e4d1c 100644 --- a/spec/factories/pages.rb +++ b/spec/factories/pages.rb @@ -3,6 +3,5 @@ sequence(:slug) { |n| "slug#{n}" } sequence(:title) { |n| "title#{n}" } sequence(:body) { |n| "body#{n}" } - sequence(:body_html) { |n| "body_html#{n}" } end end diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 16d4d7a280..3408349b17 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -1,10 +1,54 @@ require 'rails_helper' describe ApplicationHelper, type: :helper do - it 'sanitize_markdown' do - expect(helper.sanitize_markdown('link')).to eq('link') + describe '.sanitize_markdown' do + describe '' do + it 'should block javascript' do + expect(helper.sanitize_markdown('link')).to eq('link') + end + end + + describe '')).to eq('alert();') + end + end + + describe '')).to eq('.body{}') + end + end + + describe '')).to eq('') + end + + it 'should allow youtube iframe' do + html = ' + + ' + expect(helper.sanitize_markdown(html)).to eq(html) + end + end + + describe 'img' do + it 'should work' do + html = '' + expect(helper.sanitize_markdown(html)).to eq(html) + end + end + + describe 'a' do + it 'should work' do + html = '111' + expect(helper.sanitize_markdown(html)).to eq(html) + end + end end + describe 'markdown' do context 'bad html' do it 'filter script' do diff --git a/spec/models/reply_spec.rb b/spec/models/reply_spec.rb index b055c4ab38..cecf713a23 100644 --- a/spec/models/reply_spec.rb +++ b/spec/models/reply_spec.rb @@ -110,38 +110,6 @@ end end - describe 'format body' do - it 'should covert body with Markdown on create' do - r = create(:reply, body: '*foo*') - expect(r.body_html).to eq('

foo

') - end - - it 'should covert body on save' do - r = create(:reply, body: '*foo*') - old_html = r.body_html - r.body = '*bar*' - r.save - expect(r.body_html).not_to eq(old_html) - end - - it 'should not store body_html when it not changed' do - r = create(:reply, body: '*foo*') - r.body = '*fooaa*' - allow(r).to receive(:body_changed?).and_return(false) - old_html = r.body_html - r.save - expect(r.body_html).to eq(old_html) - end - - context '#link_mention_user' do - it 'should add link to mention users' do - body = '@foo' - reply = create(:reply, body: body) - expect(reply.body_html).to eq('

@foo

') - end - end - end - describe 'ban words for Reply body' do let(:topic) { create(:topic) } it 'should work' do diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 098fc46a5d..a518991d06 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -72,11 +72,6 @@ expect(topic.last_active_mark).to eq(old_last_active_mark) end - it 'should covert body with Markdown on create' do - t = create(:topic, body: '*foo*') - expect(t.body_html).to eq('

foo

') - end - it 'should get page and floor by reply' do replies = [] 5.times do @@ -86,23 +81,6 @@ expect(topic.floor_of_reply(replies[3])).to eq(4) end - it 'should covert body on save' do - t = create(:topic, body: '*foo*') - old_html = t.body_html - t.body = '*bar*' - t.save - expect(t.body_html).not_to eq(old_html) - end - - it 'should not store body_html when it not changed' do - t = create(:topic, body: '*foo*') - t.body = '*fooaa*' - allow(t).to receive(:body_changed?).and_return(false) - old_html = t.body_html - t.save - expect(t.body_html).to eq(old_html) - end - it 'should log deleted user name when use destroy_by' do t = create(:topic) t.destroy_by(user)