From f028ffaf29d871c54d3a50e25cae077e0ed7c591 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 14 Feb 2018 10:39:44 +1100 Subject: [PATCH] SECURITY: correct local onebox category checks Also removes ugly "source_topic_id" from cooked posts Patch was authored by @zogstrip Signed-off-by: Sam --- .../components/composer-editor.js.es6 | 2 +- .../components/composer-title.js.es6 | 8 +- .../engines/discourse-markdown/quotes.js.es6 | 1 - .../javascripts/pretty-text/oneboxer.js.es6 | 15 +- app/controllers/onebox_controller.rb | 8 +- app/models/post_analyzer.rb | 2 +- lib/cooked_post_processor.rb | 14 +- lib/discourse.rb | 8 +- lib/onebox/engine/discourse_local_onebox.rb | 129 -------------- lib/oneboxer.rb | 136 +++++++++++---- spec/components/cooked_post_processor_spec.rb | 3 +- .../engine/discourse_local_onebox_spec.rb | 161 ------------------ spec/components/oneboxer_spec.rb | 71 ++++++++ spec/controllers/onebox_controller_spec.rb | 54 ++++-- 14 files changed, 251 insertions(+), 361 deletions(-) delete mode 100644 lib/onebox/engine/discourse_local_onebox.rb delete mode 100644 spec/components/onebox/engine/discourse_local_onebox_spec.rb diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6 index 6b3ca5b4001..b0623158e4c 100644 --- a/app/assets/javascripts/discourse/components/composer-editor.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-editor.js.es6 @@ -372,7 +372,7 @@ export default Ember.Component.extend({ post.set('refreshedPost', true); } - $oneboxes.each((_, o) => load(o, refresh, ajax, this.currentUser.id)); + $oneboxes.each((_, o) => load({ elem: o, refresh, ajax, categoryId: this.get('composer.category.id') })); }, _warnMentionedGroups($preview) { diff --git a/app/assets/javascripts/discourse/components/composer-title.js.es6 b/app/assets/javascripts/discourse/components/composer-title.js.es6 index 2a828a9483a..86acf45c773 100644 --- a/app/assets/javascripts/discourse/components/composer-title.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-title.js.es6 @@ -80,7 +80,13 @@ export default Ember.Component.extend({ const link = document.createElement('a'); link.href = this.get('composer.title'); - let loadOnebox = load(link, false, ajax, this.currentUser.id, true); + const loadOnebox = load({ + elem: link, + refresh: false, + ajax, + synchronous: true, + categoryId: this.get('composer.category.id'), + }); if (loadOnebox && loadOnebox.then) { loadOnebox.then( () => { diff --git a/app/assets/javascripts/pretty-text/engines/discourse-markdown/quotes.js.es6 b/app/assets/javascripts/pretty-text/engines/discourse-markdown/quotes.js.es6 index 3a0523bef66..27873269ee1 100644 --- a/app/assets/javascripts/pretty-text/engines/discourse-markdown/quotes.js.es6 +++ b/app/assets/javascripts/pretty-text/engines/discourse-markdown/quotes.js.es6 @@ -105,7 +105,6 @@ const rule = { let title = topicInfo.title; - if (options.enableEmoji) { title = performEmojiUnescape(topicInfo.title, { getURL: options.getURL, emojiSet: options.emojiSet diff --git a/app/assets/javascripts/pretty-text/oneboxer.js.es6 b/app/assets/javascripts/pretty-text/oneboxer.js.es6 index 245c87cbca1..90538994b7f 100644 --- a/app/assets/javascripts/pretty-text/oneboxer.js.es6 +++ b/app/assets/javascripts/pretty-text/oneboxer.js.es6 @@ -43,16 +43,15 @@ function loadNext(ajax) { let timeoutMs = 150; let removeLoading = true; - const { url, refresh, $elem, userId } = loadingQueue.shift(); + const { url, refresh, $elem, categoryId } = loadingQueue.shift(); // Retrieve the onebox return ajax("/onebox", { dataType: 'html', - data: { url, refresh }, + data: { url, refresh, category_id: categoryId }, cache: true }).then(html => { let $html = $(html); - localCache[normalize(url)] = $html; $elem.replaceWith($html); applySquareGenericOnebox($html, normalize(url)); @@ -60,7 +59,7 @@ function loadNext(ajax) { if (result && result.jqXHR && result.jqXHR.status === 429) { timeoutMs = 2000; removeLoading = false; - loadingQueue.unshift({ url, refresh, $elem, userId }); + loadingQueue.unshift({ url, refresh, $elem, categoryId }); } else { failedCache[normalize(url)] = true; } @@ -75,14 +74,14 @@ function loadNext(ajax) { // Perform a lookup of a onebox based an anchor $element. // It will insert a loading indicator and remove it when the loading is complete or fails. -export function load(e, refresh, ajax, userId, synchronous) { - const $elem = $(e); +export function load({ elem , refresh = true, ajax, synchronous = false, categoryId }) { + const $elem = $(elem); // If the onebox has loaded or is loading, return if ($elem.data('onebox-loaded')) return; if ($elem.hasClass('loading-onebox')) return; - const url = e.href; + const url = elem.href; // Unless we're forcing a refresh... if (!refresh) { @@ -99,7 +98,7 @@ export function load(e, refresh, ajax, userId, synchronous) { $elem.addClass('loading-onebox'); // Add to the loading queue - loadingQueue.push({ url, refresh, $elem, userId }); + loadingQueue.push({ url, refresh, $elem, categoryId }); // Load next url in queue if (synchronous) { diff --git a/app/controllers/onebox_controller.rb b/app/controllers/onebox_controller.rb index 7c0d16af322..dea29ce5368 100644 --- a/app/controllers/onebox_controller.rb +++ b/app/controllers/onebox_controller.rb @@ -14,13 +14,19 @@ class OneboxController < ApplicationController return render(body: nil, status: 429) if Oneboxer.is_previewing?(current_user.id) user_id = current_user.id + category_id = params[:category_id].to_i invalidate = params[:refresh] == 'true' url = params[:url] hijack do Oneboxer.preview_onebox!(user_id) - preview = Oneboxer.preview(url, invalidate_oneboxes: invalidate) + preview = Oneboxer.preview(url, + invalidate_oneboxes: invalidate, + user_id: user_id, + category_id: category_id + ) + preview.strip! if preview.present? Oneboxer.onebox_previewed!(user_id) diff --git a/app/models/post_analyzer.rb b/app/models/post_analyzer.rb index 919b7d42155..ddcfa2fd0c0 100644 --- a/app/models/post_analyzer.rb +++ b/app/models/post_analyzer.rb @@ -31,7 +31,7 @@ class PostAnalyzer cooked = PrettyText.cook(raw, opts) end - result = Oneboxer.apply(cooked, topic_id: @topic_id) do |url, _| + result = Oneboxer.apply(cooked) do |url| @found_oneboxes = true Oneboxer.invalidate(url) if opts[:invalidate_oneboxes] Oneboxer.cached_onebox(url) diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index e9637755f09..263d139e729 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -368,15 +368,13 @@ class CookedPostProcessor end def post_process_oneboxes - args = { - post_id: @post.id, - invalidate_oneboxes: !!@opts[:invalidate_oneboxes], - } - - # apply oneboxes - Oneboxer.apply(@doc, topic_id: @post.topic_id) do |url| + Oneboxer.apply(@doc) do |url| @has_oneboxes = true - Oneboxer.onebox(url, args) + Oneboxer.onebox(url, + invalidate_oneboxes: !!@opts[:invalidate_oneboxes], + user_id: @post&.user_id, + category_id: @post&.topic&.category_id + ) end oneboxed_images.each do |img| diff --git a/lib/discourse.rb b/lib/discourse.rb index 036836c6057..45b6416c6b1 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -236,15 +236,11 @@ module Discourse end def self.route_for(uri) - - uri = URI(uri) rescue nil unless (uri.is_a?(URI)) + uri = URI(uri) rescue nil unless uri.is_a?(URI) return unless uri path = uri.path || "" - if (uri.host == Discourse.current_hostname && - path.start_with?(Discourse.base_uri)) || - !uri.host - + if !uri.host || (uri.host == Discourse.current_hostname && path.start_with?(Discourse.base_uri)) path.slice!(Discourse.base_uri) return Rails.application.routes.recognize_path(path) end diff --git a/lib/onebox/engine/discourse_local_onebox.rb b/lib/onebox/engine/discourse_local_onebox.rb deleted file mode 100644 index b4add001ac0..00000000000 --- a/lib/onebox/engine/discourse_local_onebox.rb +++ /dev/null @@ -1,129 +0,0 @@ -module Onebox - module Engine - class DiscourseLocalOnebox - include Engine - - # Use this onebox before others - def self.priority - 1 - end - - def self.===(other) - url = other.to_s - return false unless url[Discourse.base_url] - - route = Discourse.route_for(url) - - !!(route[:controller] =~ /topics|uploads|users/) - rescue ActionController::RoutingError - false - end - - def to_html - uri = URI(@url) - path = uri.path || "" - route = Discourse.route_for(uri) - - case route[:controller] - when "uploads" then upload_html(path) - when "topics" then topic_html(route) - when "users" then user_html(route) - end - end - - private - - def upload_html(path) - case File.extname(path) - when /^\.(mov|mp4|webm|ogv)$/i - "" - when /^\.(mp3|ogg|wav|m4a)$/i - "" - end - end - - def topic_html(route) - link = "#{@url}" - source_topic_id = @url[/[&?]source_topic_id=(\d+)/, 1].to_i - source_topic = Topic.find_by(id: source_topic_id) if source_topic_id > 0 - - if route[:post_number].present? && route[:post_number].to_i > 1 - post = Post.find_by(topic_id: route[:topic_id], post_number: route[:post_number]) - return link unless can_see_post?(post, source_topic) - - topic = post.topic - slug = Slug.for(topic.title) - excerpt = post.excerpt(SiteSetting.post_onebox_maxlength) - excerpt.gsub!(/[\r\n]+/, " ") - excerpt.gsub!("[/quote]", "[quote]") # don't break my quote - - quote = "[quote=\"#{post.user.username}, topic:#{topic.id}, slug:#{slug}, post:#{post.post_number}\"]\n#{excerpt}\n[/quote]" - - args = {} - args[:topic_id] = source_topic_id if source_topic_id > 0 - - PrettyText.cook(quote, args) - else - topic = Topic.find_by(id: route[:topic_id]) - return link unless can_see_topic?(topic, source_topic) - - first_post = topic.ordered_posts.first - - args = { - topic_id: topic.id, - avatar: PrettyText.avatar_img(topic.user.avatar_template, "tiny"), - original_url: @url, - title: PrettyText.unescape_emoji(CGI::escapeHTML(topic.title)), - category_html: CategoryBadge.html_for(topic.category), - quote: first_post.excerpt(SiteSetting.post_onebox_maxlength), - } - - template = File.read("#{Rails.root}/lib/onebox/templates/discourse_topic_onebox.hbs") - Mustache.render(template, args) - end - end - - def user_html(route) - link = "#{@url}" - username = route[:username] || '' - user = User.find_by(username_lower: username.downcase) - - if user - args = { - user_id: user.id, - username: user.username, - avatar: PrettyText.avatar_img(user.avatar_template, "extra_large"), - name: user.name, - bio: user.user_profile.bio_excerpt(230), - location: user.user_profile.location, - joined: I18n.t('joined'), - created_at: user.created_at.strftime(I18n.t('datetime_formats.formats.date_only')), - website: user.user_profile.website, - website_name: UserSerializer.new(user).website_name, - original_url: @url - } - - template = File.read("#{Rails.root}/lib/onebox/templates/discourse_user_onebox.hbs") - Mustache.render(template, args) - else - return link - end - end - - def can_see_post?(post, source_topic) - return false if post.nil? || post.hidden || post.trashed? || post.topic.nil? - Guardian.new.can_see_post?(post) || same_category?(post.topic.category, source_topic) - end - - def can_see_topic?(topic, source_topic) - return false if topic.nil? || topic.trashed? || topic.private_message? - Guardian.new.can_see_topic?(topic) || same_category?(topic.category, source_topic) - end - - def same_category?(category, source_topic) - source_topic.try(:category_id) == category.try(:id) - end - - end - end -end diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index 121984f680d..64e1ee8b3c6 100644 --- a/lib/oneboxer.rb +++ b/lib/oneboxer.rb @@ -28,13 +28,13 @@ module Oneboxer def self.preview(url, options = nil) options ||= {} invalidate(url) if options[:invalidate_oneboxes] - onebox_raw(url)[:preview] + onebox_raw(url, options)[:preview] end def self.onebox(url, options = nil) options ||= {} invalidate(url) if options[:invalidate_oneboxes] - onebox_raw(url)[:onebox] + onebox_raw(url, options)[:onebox] end def self.cached_onebox(url) @@ -76,41 +76,22 @@ module Oneboxer doc end - def self.append_source_topic_id(url, topic_id) - # hack urls to create proper expansions - if url =~ Regexp.new("^#{Discourse.base_url.gsub(".", "\\.")}.*$", true) - uri = URI.parse(url) rescue nil - if uri && uri.path - route = Rails.application.routes.recognize_path(uri.path) rescue nil - if route && route[:controller] == 'topics' - url += (url =~ /\?/ ? "&" : "?") + "source_topic_id=#{topic_id}" - end - end - end - url - end - def self.apply(string_or_doc, args = nil) doc = string_or_doc doc = Nokogiri::HTML::fragment(doc) if doc.is_a?(String) changed = false each_onebox_link(doc) do |url, element| - if args && args[:topic_id] - url = append_source_topic_id(url, args[:topic_id]) - end - onebox, _preview = yield(url, element) + onebox, _ = yield(url, element) if onebox parsed_onebox = Nokogiri::HTML::fragment(onebox) next unless parsed_onebox.children.count > 0 # special logic to strip empty p elements - if element.parent && - element.parent.node_name && - element.parent.node_name.downcase == "p" && - element.parent.children.count == 1 + if element&.parent&.node_name&.downcase == "p" && element&.parent&.children&.count == 1 element = element.parent end + changed = true element.swap parsed_onebox.to_html end @@ -149,7 +130,104 @@ module Oneboxer "onebox__#{url}" end - def self.onebox_raw(url) + def self.onebox_raw(url, opts = {}) + local_onebox(url, opts) || external_onebox(url) + rescue => e + # no point warning here, just cause we have an issue oneboxing a url + # we can later hunt for failed oneboxes by searching logs if needed + Rails.logger.info("Failed to onebox #{url} #{e} #{e.backtrace}") + # return a blank hash, so rest of the code works + blank_onebox + end + + def self.local_onebox(url, opts = {}) + return unless route = Discourse.route_for(url) + + html = + case route[:controller] + when "uploads" then local_upload_html(url) + when "topics" then local_topic_html(url, route, opts) + when "users" then local_user_html(url, route) + end + + html = html.presence || "#{url}" + { onebox: html, preview: html } + end + + def self.local_upload_html(url) + case File.extname(URI(url).path || "") + when /^\.(mov|mp4|webm|ogv)$/i + "" + when /^\.(mp3|ogg|wav|m4a)$/i + "" + end + end + + def self.local_topic_html(url, route, opts) + return unless current_user = User.find_by(id: opts[:user_id]) + return unless current_category = Category.find_by(id: opts[:category_id]) + return unless Guardian.new(current_user).can_see_category?(current_category) + + if route[:post_number].to_i > 1 + post = Post.find_by(topic_id: route[:topic_id], post_number: route[:post_number]) + + return unless post.present? && !post.hidden + return unless current_category.id == post.topic.category_id || Guardian.new.can_see_post?(post) + + topic = post.topic + excerpt = post.excerpt(SiteSetting.post_onebox_maxlength) + excerpt.gsub!(/[\r\n]+/, " ") + excerpt.gsub!("[/quote]", "[quote]") # don't break my quote + + quote = "[quote=\"#{post.user.username}, topic:#{topic.id}, post:#{post.post_number}\"]\n#{excerpt}\n[/quote]" + + PrettyText.cook(quote) + else + return unless topic = Topic.find_by(id: route[:topic_id]) + return unless current_category.id == topic.category_id || Guardian.new.can_see_topic?(topic) + + first_post = topic.ordered_posts.first + + args = { + topic_id: topic.id, + avatar: PrettyText.avatar_img(topic.user.avatar_template, "tiny"), + original_url: url, + title: PrettyText.unescape_emoji(CGI::escapeHTML(topic.title)), + category_html: CategoryBadge.html_for(topic.category), + quote: first_post.excerpt(SiteSetting.post_onebox_maxlength), + } + + template = File.read("#{Rails.root}/lib/onebox/templates/discourse_topic_onebox.hbs") + Mustache.render(template, args) + end + end + + def self.local_user_html(url, route) + username = route[:username] || "" + + if user = User.find_by(username_lower: username.downcase) + args = { + user_id: user.id, + username: user.username, + avatar: PrettyText.avatar_img(user.avatar_template, "extra_large"), + name: user.name, + bio: user.user_profile.bio_excerpt(230), + location: user.user_profile.location, + joined: I18n.t('joined'), + created_at: user.created_at.strftime(I18n.t('datetime_formats.formats.date_only')), + website: user.user_profile.website, + website_name: UserSerializer.new(user).website_name, + original_url: url + } + + template = File.read("#{Rails.root}/lib/onebox/templates/discourse_user_onebox.hbs") + Mustache.render(template, args) + else + nil + end + end + + def self.external_onebox(url) Rails.cache.fetch(onebox_cache_key(url), expires_in: 1.day) do fd = FinalDestination.new(url, ignore_redirects: ignore_redirects, force_get_hosts: force_get_hosts) uri = fd.resolve @@ -169,14 +247,8 @@ module Oneboxer r = Onebox.preview(uri.to_s, options) - { onebox: r.to_s, preview: r.try(:placeholder_html).to_s } + { onebox: r.to_s, preview: r&.placeholder_html.to_s } end - rescue => e - # no point warning here, just cause we have an issue oneboxing a url - # we can later hunt for failed oneboxes by searching logs if needed - Rails.logger.info("Failed to onebox #{url} #{e} #{e.backtrace}") - # return a blank hash, so rest of the code works - blank_onebox end end diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb index d2b90f5e4d7..618b69a349d 100644 --- a/spec/components/cooked_post_processor_spec.rb +++ b/spec/components/cooked_post_processor_spec.rb @@ -479,10 +479,11 @@ describe CookedPostProcessor do before do Oneboxer.expects(:onebox) - .with("http://www.youtube.com/watch?v=9bZkp7q19f0", post_id: 123, invalidate_oneboxes: true) + .with("http://www.youtube.com/watch?v=9bZkp7q19f0", invalidate_oneboxes: true, user_id: nil, category_id: post.topic.category_id) .returns("
GANGNAM STYLE
") cpp.post_process_oneboxes end + it "inserts the onebox without wrapping p" do expect(cpp).to be_dirty expect(cpp.html).to match_html "
GANGNAM STYLE
" diff --git a/spec/components/onebox/engine/discourse_local_onebox_spec.rb b/spec/components/onebox/engine/discourse_local_onebox_spec.rb deleted file mode 100644 index efc60b97501..00000000000 --- a/spec/components/onebox/engine/discourse_local_onebox_spec.rb +++ /dev/null @@ -1,161 +0,0 @@ -require 'rails_helper' - -describe Onebox::Engine::DiscourseLocalOnebox do - - before { SiteSetting.external_system_avatars_enabled = false } - - def build_link(url) - %|#{url}| - end - - context "for a link to a post" do - let(:post) { Fabricate(:post) } - let(:post2) { Fabricate(:post, topic: post.topic, post_number: 2) } - - it "returns a link if post isn't found" do - url = "#{Discourse.base_url}/t/not-exist/3/2" - expect(Onebox.preview(url).to_s).to eq(build_link(url)) - end - - it "returns a link if not allowed to see the post" do - url = "#{Discourse.base_url}#{post2.url}" - Guardian.any_instance.expects(:can_see_post?).returns(false) - expect(Onebox.preview(url).to_s).to eq(build_link(url)) - end - - it "returns a link if post is hidden" do - hidden_post = Fabricate(:post, topic: post.topic, post_number: 2, hidden: true, hidden_reason_id: Post.hidden_reasons[:flag_threshold_reached]) - url = "#{Discourse.base_url}#{hidden_post.url}" - expect(Onebox.preview(url).to_s).to eq(build_link(url)) - end - - it "returns some onebox goodness if post exists and can be seen" do - url = "#{Discourse.base_url}#{post2.url}?source_topic_id=#{post2.topic_id + 1}" - html = Onebox.preview(url).to_s - expect(html).to include(post2.excerpt) - expect(html).to include(post2.topic.title) - - url = "#{Discourse.base_url}#{post2.url}/?source_topic_id=#{post2.topic_id + 1}" - html = Onebox.preview(url).to_s - expect(html).to include(post2.excerpt) - expect(html).to include(post2.topic.title) - - html = Onebox.preview("#{Discourse.base_url}#{post2.url}").to_s - expect(html).to include(post2.user.username) - expect(html).to include(post2.excerpt) - end - end - - context "for a link to a topic" do - let(:post) { Fabricate(:post) } - let(:topic) { post.topic } - - it "returns a link if topic isn't found" do - url = "#{Discourse.base_url}/t/not-found/123" - expect(Onebox.preview(url).to_s).to eq(build_link(url)) - end - - it "returns a link if not allowed to see the topic" do - url = topic.url - Guardian.any_instance.expects(:can_see_topic?).returns(false) - expect(Onebox.preview(url).to_s).to eq(build_link(url)) - end - - it "replaces emoji in the title" do - topic.update_column(:title, "Who wants to eat a :hamburger:") - expect(Onebox.preview(topic.url).to_s).to match(/hamburger\.png/) - end - - it "returns some onebox goodness if topic exists and can be seen" do - html = Onebox.preview(topic.url).to_s - expect(html).to include(topic.ordered_posts.first.user.username) - expect(html).to include("
") - - html = Onebox.preview("#{topic.url}/?u=codinghorror").to_s - expect(html).to include(topic.ordered_posts.first.user.username) - expect(html).to include("
") - end - end - - context "for a link to a user profile" do - let(:user) { Fabricate(:user) } - - it "returns a link if user isn't found" do - url = "#{Discourse.base_url}/u/none" - expect(Onebox.preview(url).to_s).to eq(build_link(url)) - end - - it "returns some onebox goodness if user exists" do - html = Onebox.preview("#{Discourse.base_url}/u/#{user.username}").to_s - expect(html).to include(user.username) - expect(html).to include(user.name) - expect(html).to include(user.created_at.strftime("%B %-d, %Y")) - expect(html).to include('