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

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) {

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( () => {

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) {
+export function load(e, refresh, ajax, userId) {
-  const $elem = $(e);
+  const elem = $(e);
 
   // If the onebox has loaded or is loading, return
-  if ($elem.data('onebox-loaded')) return;
+  if (elem.data('onebox-loaded')) return;
-  if ($elem.hasClass('loading-onebox')) 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
+  // Add to the loading queue
-  return ajax("/onebox", {
+  loadingQueue.push({ url, refresh, elem, userId });
-    dataType: 'html',
+
-    data: { url, refresh },
+  // Load next url in queue
-    cache: true
+  timeout = timeout || setTimeout(() => loadNext(ajax), 150);
-  }).then(html => {
-    localCache[url] = html;
-    $elem.replaceWith(html);
-  }, () => {
-    failedCache[url] = true;
-  }).finally(() => {
-    $elem.removeClass('loading-onebox');
-    $elem.data('onebox-loaded');
-  });
 }
 
 export function lookupCache(url) {

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')
+    params.require(:user_id)
-    result.strip! if result.present?
 
-    # If there is no result, return a 404
+    preview = Oneboxer.cached_preview(params[:url])
-    if result.blank?
+    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
 

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

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
+  it "requires the user to be logged in" do
-    Oneboxer.expects(:preview).with(url, invalidate_oneboxes: true)
+    expect { xhr :get, :show, url: url }.to raise_error(Discourse::NotLoggedIn)
-    xhr :get, :show, url: url, refresh: 'true'
   end
 
-  describe "found onebox" do
+  describe "logged in" do
 
-    let(:body) { "this is the onebox body"}
+    before { @user = log_in(:admin) }
 
-    before do
+    it 'invalidates the cache if refresh is passed' do
-      Oneboxer.expects(:preview).with(url, invalidate_oneboxes: false).returns(body)
+      Oneboxer.expects(:preview).with(url, invalidate_oneboxes: true)
-      xhr :get, :show, url: url
+      xhr :get, :show, url: url, refresh: 'true', user_id: @user.id
     end
 
-    it 'returns success' do
+    describe "cached onebox" do
-      expect(response).to be_success
+
+      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
+    describe "only 1 outgoing preview per user" do
-      expect(response.body).to eq(body)
+
+      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
+    describe "missing onebox" do
-      Oneboxer.expects(:preview).with(url, invalidate_oneboxes: false).returns(" \t ")
+
-      xhr :get, :show, url: url
+      it "returns 404 if the onebox is nil" do
-      expect(response.response_code).to eq(404)
+        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