SECURITY: Limit /inline-onebox to 10 URLs at a time

app/assets/javascripts/discourse/tests/unit/lib/pretty-text/inline-oneboxer-test.js

@@ -0,0 +1,56 @@
+import { settled } from "@ember/test-helpers";
+import { setupTest } from "ember-qunit";
+import { applyInlineOneboxes } from "pretty-text/inline-oneboxer";
+import { module, test } from "qunit";
+import { ajax } from "discourse/lib/ajax";
+import pretender, { response } from "discourse/tests/helpers/create-pretender";
+
+module("Unit | Pretty Text | Inline Oneboxer", function (hooks) {
+  setupTest(hooks);
+
+  let links;
+  hooks.beforeEach(function () {
+    links = {};
+    for (let i = 0; i < 11; i++) {
+      const url = `http://example.com/url-${i}`;
+      links[url] = document.createElement("DIV");
+    }
+  });
+
+  hooks.afterEach(function () {
+    links = {};
+  });
+
+  test("batches requests when oneboxing more than 10 urls", async function (assert) {
+    const requestedUrls = [];
+    let requestCount = 0;
+
+    pretender.get("/inline-onebox", async (request) => {
+      requestCount++;
+      requestedUrls.push(...request.queryParams.urls);
+      return response(200, { "inline-oneboxes": [] });
+    });
+
+    applyInlineOneboxes(links, ajax);
+    await settled();
+
+    assert.strictEqual(
+      requestCount,
+      2,
+      "it splits the 11 urls into 2 requests"
+    );
+    assert.deepEqual(requestedUrls, [
+      "http://example.com/url-0",
+      "http://example.com/url-1",
+      "http://example.com/url-2",
+      "http://example.com/url-3",
+      "http://example.com/url-4",
+      "http://example.com/url-5",
+      "http://example.com/url-6",
+      "http://example.com/url-7",
+      "http://example.com/url-8",
+      "http://example.com/url-9",
+      "http://example.com/url-10",
+    ]);
+  });
+});

app/assets/javascripts/pretty-text/addon/inline-oneboxer.js

@@ -1,6 +1,6 @@
 const _cache = {};
 
-export function applyInlineOneboxes(inline, ajax, opts) {
+export async function applyInlineOneboxes(inline, ajax, opts) {
   opts = opts || {};
 
   const urls = Object.keys(inline).filter((url) => !_cache[url]);
@@ -14,13 +14,18 @@ export function applyInlineOneboxes(inline, ajax, opts) {
     return;
   }
 
-  ajax("/inline-onebox", {
+  const batchSize = 10;
+  for (let i = 0; i < urls.length; i += batchSize) {
+    const batch = urls.slice(i, i + batchSize);
+
+    try {
+      const result = await ajax("/inline-onebox", {
         data: {
-      urls,
+          urls: batch,
           category_id: opts.categoryId,
           topic_id: opts.topicId,
         },
-  }).then((result) => {
+      });
       result["inline-oneboxes"].forEach((onebox) => {
         if (onebox.title) {
           _cache[onebox.url] = onebox;
@@ -33,7 +38,11 @@ export function applyInlineOneboxes(inline, ajax, opts) {
           });
         }
       });
-  });
+    } catch (err) {
+      // eslint-disable-next-line no-console
+      console.error("Inline onebox request failed", err, batch);
+    }
+  }
 }
 
 export function cachedInlineOnebox(url) {

app/controllers/inline_onebox_controller.rb

@@ -1,10 +1,30 @@
 # frozen_string_literal: true
 
 class InlineOneboxController < ApplicationController
+  MAX_URLS_LIMIT = 10
+
   requires_login
 
   def show
+    urls = params[:urls] || []
+
+    if urls.size > MAX_URLS_LIMIT
+      render json: failed_json.merge(errors: [I18n.t("inline_oneboxer.too_many_urls")]), status: 413
+      return
+    end
+
+    current_user_id = current_user.id
+
+    if InlineOneboxer.is_previewing?(current_user_id)
+      response.headers["Retry-After"] = "60"
+      render json: failed_json.merge(errors: [I18n.t("inline_oneboxer.concurrency_not_allowed")]),
+             status: 429
+      return
+    end
+
     hijack do
+      InlineOneboxer.preview!(current_user_id)
+
       oneboxes =
         InlineOneboxer.new(
           params[:urls] || [],
@@ -12,6 +32,8 @@ class InlineOneboxController < ApplicationController
           category_id: params[:category_id].to_i,
           topic_id: params[:topic_id].to_i,
         ).process
+
+      InlineOneboxer.finish_preview!(current_user_id)
       render json: { "inline-oneboxes" => oneboxes }
     end
   end

config/locales/server.en.yml

@@ -70,6 +70,8 @@ en:
   inline_oneboxer:
     topic_page_title_post_number: "#%{post_number}"
     topic_page_title_post_number_by_user: "#%{post_number} by %{username}"
+    too_many_urls: "Can't inline onebox more than 10 URLs in a single request."
+    concurrency_not_allowed: "Concurrent inline-oneboxing requests are not allowed. Please send one request at a time."
   components:
     enabled_filter: "Enabled"
     disabled_filter: "Disabled"

lib/inline_oneboxer.rb

@@ -86,6 +86,18 @@ class InlineOneboxer
     nil
   end
 
+  def self.is_previewing?(user_id)
+    Discourse.redis.get(preview_key(user_id)) == "1"
+  end
+
+  def self.preview!(user_id)
+    Discourse.redis.setex(preview_key(user_id), 1.minute, "1")
+  end
+
+  def self.finish_preview!(user_id)
+    Discourse.redis.del(preview_key(user_id))
+  end
+
   private
 
   def self.onebox_for(url, title, opts)
@@ -126,4 +138,8 @@ class InlineOneboxer
       author.username
     end
   end
+
+  def self.preview_key(user_id)
+    "inline-onebox:preview:#{user_id}"
+  end
 end

spec/requests/inline_onebox_controller_spec.rb

@@ -7,7 +7,8 @@ RSpec.describe InlineOneboxController do
   end
 
   context "when logged in" do
-    let!(:user) { sign_in(Fabricate(:user)) }
+    fab!(:user)
+    before { sign_in(user) }
 
     it "returns empty JSON for empty input" do
       get "/inline-onebox.json", params: { urls: [] }
@@ -16,6 +17,107 @@ RSpec.describe InlineOneboxController do
       expect(json["inline-oneboxes"]).to eq([])
     end
 
+    it "returns a 413 error if more than 10 urls are sent" do
+      get "/inline-onebox.json", params: { urls: ("a".."k").to_a }
+      expect(response.status).to eq(413)
+      json = response.parsed_body
+      expect(json["errors"]).to include(I18n.t("inline_oneboxer.too_many_urls"))
+    end
+
+    it "returns a 429 error for concurrent requests from the same user" do
+      blocked = true
+      reached = false
+
+      stub_request(:get, "http://example.com/url-1").to_return do |request|
+        reached = true
+        sleep 0.001 while blocked
+        { status: 200, body: <<~HTML }
+          <html>
+            <head>
+              <title>
+                Concurrent inline-oneboxing test
+              </title>
+            </head>
+            <body></body>
+          </html>
+        HTML
+      end
+
+      t1 = Thread.new { get "/inline-onebox.json", params: { urls: ["http://example.com/url-1"] } }
+
+      sleep 0.001 while !reached
+
+      get "/inline-onebox.json", params: { urls: ["http://example.com/url-2"] }
+      expect(response.status).to eq(429)
+      expect(response.parsed_body["errors"]).to include(
+        I18n.t("inline_oneboxer.concurrency_not_allowed"),
+      )
+
+      blocked = false
+      t1.join
+      expect(response.status).to eq(200)
+      json = response.parsed_body
+      expect(json["inline-oneboxes"].size).to eq(1)
+      expect(json["inline-oneboxes"][0]["title"]).to eq("Concurrent inline-oneboxing test")
+    end
+
+    it "allows concurrent requests from different users" do
+      another_user = Fabricate(:user)
+
+      blocked = true
+      reached = false
+
+      stub_request(:get, "http://example.com/url-1").to_return do |request|
+        reached = true
+        sleep 0.001 while blocked
+        { status: 200, body: <<~HTML }
+          <html>
+            <head>
+              <title>
+                Concurrent inline-oneboxing test
+              </title>
+            </head>
+            <body></body>
+          </html>
+        HTML
+      end
+
+      stub_request(:get, "http://example.com/url-2").to_return do |request|
+        { status: 200, body: <<~HTML }
+          <html>
+            <head>
+              <title>
+                Concurrent inline-oneboxing test 2
+              </title>
+            </head>
+            <body></body>
+          </html>
+        HTML
+      end
+
+      t1 =
+        Thread.new do
+          sign_in(user)
+          get "/inline-onebox.json", params: { urls: ["http://example.com/url-1"] }
+        end
+
+      sleep 0.001 while !reached
+
+      sign_in(another_user)
+      get "/inline-onebox.json", params: { urls: ["http://example.com/url-2"] }
+      expect(response.status).to eq(200)
+      json = response.parsed_body
+      expect(json["inline-oneboxes"].size).to eq(1)
+      expect(json["inline-oneboxes"][0]["title"]).to eq("Concurrent inline-oneboxing test 2")
+
+      blocked = false
+      t1.join
+      expect(response.status).to eq(200)
+      json = response.parsed_body
+      expect(json["inline-oneboxes"].size).to eq(1)
+      expect(json["inline-oneboxes"][0]["title"]).to eq("Concurrent inline-oneboxing test")
+    end
+
     context "with topic link" do
       fab!(:topic)