From 52cd9972bb832a32a51c014ad6096a1bd01ecdcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Tue, 20 Dec 2016 00:31:10 +0100 Subject: [PATCH] FIX: prevent DDoS with lots of _oneboxable_ links FIX: ensure the onebox route is only allowed to logged in users FIX: only allow 1 outgoing onebox preview per user FIX: client should only do 1 preview at a time --- .../components/composer-editor.js.es6 | 2 +- .../components/composer-title.js.es6 | 3 +- .../javascripts/pretty-text/oneboxer.js.es6 | 66 ++++++++++----- app/controllers/onebox_controller.rb | 26 ++++-- lib/oneboxer.rb | 22 ++++- spec/controllers/onebox_controller_spec.rb | 84 ++++++++++++++----- 6 files changed, 151 insertions(+), 52 deletions(-) diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6 index 7473ff08d8c..a851ae88609 100644 --- a/app/assets/javascripts/discourse/components/composer-editor.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-editor.js.es6 @@ -167,7 +167,7 @@ export default Ember.Component.extend({ post.set('refreshedPost', true); } - $oneboxes.each((_, o) => load(o, refresh, ajax)); + $oneboxes.each((_, o) => load(o, refresh, ajax, this.currentUser.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 8db46e574bf..6d1e06dc2c7 100644 --- a/app/assets/javascripts/discourse/components/composer-title.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-title.js.es6 @@ -55,13 +55,12 @@ export default Ember.Component.extend({ if (this.get('isAbsoluteUrl') && (this.get('composer.reply')||"").length === 0) { // Try to onebox. If success, update post body and title. - this.set('composer.loading', true); const link = document.createElement('a'); link.href = this.get('composer.title'); - let loadOnebox = load(link, false, ajax); + let loadOnebox = load(link, false, ajax, this.currentUser.id); if (loadOnebox && loadOnebox.then) { loadOnebox.then( () => { diff --git a/app/assets/javascripts/pretty-text/oneboxer.js.es6 b/app/assets/javascripts/pretty-text/oneboxer.js.es6 index 90be7612c0e..6d068658638 100644 --- a/app/assets/javascripts/pretty-text/oneboxer.js.es6 +++ b/app/assets/javascripts/pretty-text/oneboxer.js.es6 @@ -1,14 +1,51 @@ +let timeout; +const loadingQueue = []; const localCache = {}; const failedCache = {}; +function loadNext(ajax) { + if (loadingQueue.length === 0) { + timeout = null; + return; + } + + let timeoutMs = 150; + let removeLoading = true; + const { url, refresh, elem, userId } = loadingQueue.shift(); + + // Retrieve the onebox + return ajax("/onebox", { + dataType: 'html', + data: { url, refresh, user_id: userId }, + cache: true + }).then(html => { + localCache[url] = html; + elem.replaceWith(html); + }, result => { + if (result && result.jqXHR && result.jqXHR.status === 429) { + timeoutMs = 2000; + removeLoading = false; + loadingQueue.unshift({ url, refresh, elem, userId }); + } else { + failedCache[url] = true; + } + }).finally(() => { + timeout = setTimeout(() => loadNext(ajax), timeoutMs); + if (removeLoading) { + elem.removeClass('loading-onebox'); + elem.data('onebox-loaded'); + } + }); +} + // 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) { - const $elem = $(e); +export function load(e, refresh, ajax, userId) { + const elem = $(e); // If the onebox has loaded or is loading, return - if ($elem.data('onebox-loaded')) return; - if ($elem.hasClass('loading-onebox')) return; + if (elem.data('onebox-loaded')) return; + if (elem.hasClass('loading-onebox')) return; const url = e.href; @@ -24,22 +61,13 @@ export function load(e, refresh, ajax) { } // Add the loading CSS class - $elem.addClass('loading-onebox'); + elem.addClass('loading-onebox'); - // Retrieve the onebox - return ajax("/onebox", { - dataType: 'html', - data: { url, refresh }, - cache: true - }).then(html => { - localCache[url] = html; - $elem.replaceWith(html); - }, () => { - failedCache[url] = true; - }).finally(() => { - $elem.removeClass('loading-onebox'); - $elem.data('onebox-loaded'); - }); + // Add to the loading queue + loadingQueue.push({ url, refresh, elem, userId }); + + // Load next url in queue + timeout = timeout || setTimeout(() => loadNext(ajax), 150); } export function lookupCache(url) { diff --git a/app/controllers/onebox_controller.rb b/app/controllers/onebox_controller.rb index 474feb5fd73..9b09a0f3b87 100644 --- a/app/controllers/onebox_controller.rb +++ b/app/controllers/onebox_controller.rb @@ -1,16 +1,32 @@ require_dependency 'oneboxer' class OneboxController < ApplicationController + before_filter :ensure_logged_in def show - result = Oneboxer.preview(params[:url], invalidate_oneboxes: params[:refresh] == 'true') - result.strip! if result.present? + params.require(:user_id) - # If there is no result, return a 404 - if result.blank? + preview = Oneboxer.cached_preview(params[:url]) + preview.strip! if preview.present? + + return render(text: preview) if preview.present? + + # only 1 outgoing preview per user + return render(nothing: true, status: 429) if Oneboxer.is_previewing?(params[:user_id]) + + Oneboxer.preview_onebox!(params[:user_id]) + + preview = Oneboxer.preview(params[:url], invalidate_oneboxes: params[:refresh] == 'true') + preview.strip! if preview.present? + + Scheduler::Defer.later("Onebox previewed") { + Oneboxer.onebox_previewed!(params[:user_id]) + } + + if preview.blank? render nothing: true, status: 404 else - render text: result + render text: preview end end diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index 13d81522025..5c5c99f89f4 100644 --- a/lib/oneboxer.rb +++ b/lib/oneboxer.rb @@ -16,13 +16,13 @@ module Oneboxer def self.preview(url, options=nil) options ||= {} - Oneboxer.invalidate(url) if options[:invalidate_oneboxes] + invalidate(url) if options[:invalidate_oneboxes] onebox_raw(url)[:preview] end def self.onebox(url, options=nil) options ||= {} - Oneboxer.invalidate(url) if options[:invalidate_oneboxes] + invalidate(url) if options[:invalidate_oneboxes] onebox_raw(url)[:onebox] end @@ -88,7 +88,7 @@ module Oneboxer doc = Nokogiri::HTML::fragment(doc) if doc.is_a?(String) changed = false - Oneboxer.each_onebox_link(doc) do |url, element| + each_onebox_link(doc) do |url, element| if args && args[:topic_id] url = append_source_topic_id(url, args[:topic_id]) end @@ -112,8 +112,24 @@ module Oneboxer Result.new(doc, changed) end + def self.is_previewing?(user_id) + $redis.get(preview_key(user_id)) == "1" + end + + def self.preview_onebox!(user_id) + $redis.setex(preview_key(user_id), 1.minute, "1") + end + + def self.onebox_previewed!(user_id) + $redis.del(preview_key(user_id)) + end + private + def self.preview_key(user_id) + "PREVIEWING_ONEBOX_#{user_id}" + end + def self.blank_onebox { preview: "", onebox: "" } end diff --git a/spec/controllers/onebox_controller_spec.rb b/spec/controllers/onebox_controller_spec.rb index a2f5c381740..adec21d4f8a 100644 --- a/spec/controllers/onebox_controller_spec.rb +++ b/spec/controllers/onebox_controller_spec.rb @@ -4,42 +4,82 @@ describe OneboxController do let(:url) { "http://google.com" } - it 'invalidates the cache if refresh is passed' do - Oneboxer.expects(:preview).with(url, invalidate_oneboxes: true) - xhr :get, :show, url: url, refresh: 'true' + it "requires the user to be logged in" do + expect { xhr :get, :show, url: url }.to raise_error(Discourse::NotLoggedIn) end - describe "found onebox" do + describe "logged in" do - let(:body) { "this is the onebox body"} + before { @user = log_in(:admin) } - before do - Oneboxer.expects(:preview).with(url, invalidate_oneboxes: false).returns(body) - xhr :get, :show, url: url + it 'invalidates the cache if refresh is passed' do + Oneboxer.expects(:preview).with(url, invalidate_oneboxes: true) + xhr :get, :show, url: url, refresh: 'true', user_id: @user.id end - it 'returns success' do - expect(response).to be_success + describe "cached onebox" do + + let(:body) { "This is a cached onebox body" } + + before do + Oneboxer.expects(:cached_preview).with(url).returns(body) + Oneboxer.expects(:preview).never + xhr :get, :show, url: url, user_id: @user.id + end + + it "returns success" do + expect(response).to be_success + end + + it "returns the cached onebox response in the body" do + expect(response.body).to eq(body) + end + end - it 'returns the onebox response in the body' do - expect(response.body).to eq(body) + describe "only 1 outgoing preview per user" do + + it "returns 429" do + Oneboxer.expects(:is_previewing?).returns(true) + xhr :get, :show, url: url, user_id: @user.id + expect(response.status).to eq(429) + end + end - end + describe "found onebox" do - describe "missing onebox" do + let(:body) { "this is the onebox body"} + + before do + Oneboxer.expects(:preview).with(url, invalidate_oneboxes: false).returns(body) + xhr :get, :show, url: url, user_id: @user.id + end + + it 'returns success' do + expect(response).to be_success + end + + it 'returns the onebox response in the body' do + expect(response.body).to eq(body) + end - it "returns 404 if the onebox is nil" do - Oneboxer.expects(:preview).with(url, invalidate_oneboxes: false).returns(nil) - xhr :get, :show, url: url - expect(response.response_code).to eq(404) end - it "returns 404 if the onebox is an empty string" do - Oneboxer.expects(:preview).with(url, invalidate_oneboxes: false).returns(" \t ") - xhr :get, :show, url: url - expect(response.response_code).to eq(404) + describe "missing onebox" do + + it "returns 404 if the onebox is nil" do + Oneboxer.expects(:preview).with(url, invalidate_oneboxes: false).returns(nil) + xhr :get, :show, url: url, user_id: @user.id + expect(response.response_code).to eq(404) + end + + it "returns 404 if the onebox is an empty string" do + Oneboxer.expects(:preview).with(url, invalidate_oneboxes: false).returns(" \t ") + xhr :get, :show, url: url, user_id: @user.id + expect(response.response_code).to eq(404) + end + end end