From 416ec83ae57924d721e6e374f4cda78bd77a4599 Mon Sep 17 00:00:00 2001 From: OsamaSayegh Date: Tue, 26 Nov 2024 23:04:39 +0300 Subject: [PATCH] SECURITY: Limit /inline-onebox to 10 URLs at a time --- .../lib/pretty-text/inline-oneboxer-test.js | 56 ++++++++++ .../pretty-text/addon/inline-oneboxer.js | 49 +++++---- app/controllers/inline_onebox_controller.rb | 22 ++++ config/locales/server.en.yml | 2 + lib/inline_oneboxer.rb | 16 +++ .../requests/inline_onebox_controller_spec.rb | 104 +++++++++++++++++- 6 files changed, 228 insertions(+), 21 deletions(-) create mode 100644 app/assets/javascripts/discourse/tests/unit/lib/pretty-text/inline-oneboxer-test.js diff --git a/app/assets/javascripts/discourse/tests/unit/lib/pretty-text/inline-oneboxer-test.js b/app/assets/javascripts/discourse/tests/unit/lib/pretty-text/inline-oneboxer-test.js new file mode 100644 index 00000000000..2573b7def87 --- /dev/null +++ b/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", + ]); + }); +}); diff --git a/app/assets/javascripts/pretty-text/addon/inline-oneboxer.js b/app/assets/javascripts/pretty-text/addon/inline-oneboxer.js index b427ce6141b..8dbdb578895 100644 --- a/app/assets/javascripts/pretty-text/addon/inline-oneboxer.js +++ b/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,26 +14,35 @@ export function applyInlineOneboxes(inline, ajax, opts) { return; } - ajax("/inline-onebox", { - data: { - urls, - category_id: opts.categoryId, - topic_id: opts.topicId, - }, - }).then((result) => { - result["inline-oneboxes"].forEach((onebox) => { - if (onebox.title) { - _cache[onebox.url] = onebox; + const batchSize = 10; + for (let i = 0; i < urls.length; i += batchSize) { + const batch = urls.slice(i, i + batchSize); - let links = inline[onebox.url] || []; - links.forEach((link) => { - link.innerText = onebox.title; - link.classList.add("inline-onebox"); - link.classList.remove("inline-onebox-loading"); - }); - } - }); - }); + try { + const result = await ajax("/inline-onebox", { + data: { + urls: batch, + category_id: opts.categoryId, + topic_id: opts.topicId, + }, + }); + result["inline-oneboxes"].forEach((onebox) => { + if (onebox.title) { + _cache[onebox.url] = onebox; + + let links = inline[onebox.url] || []; + links.forEach((link) => { + link.innerText = onebox.title; + link.classList.add("inline-onebox"); + link.classList.remove("inline-onebox-loading"); + }); + } + }); + } catch (err) { + // eslint-disable-next-line no-console + console.error("Inline onebox request failed", err, batch); + } + } } export function cachedInlineOnebox(url) { diff --git a/app/controllers/inline_onebox_controller.rb b/app/controllers/inline_onebox_controller.rb index 3b3d61c55c2..b8e4240a5c4 100644 --- a/app/controllers/inline_onebox_controller.rb +++ b/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 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 1b178612d99..87d64afcd1d 100644 --- a/config/locales/server.en.yml +++ b/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" diff --git a/lib/inline_oneboxer.rb b/lib/inline_oneboxer.rb index 477cbd16ba0..9f3d0377c59 100644 --- a/lib/inline_oneboxer.rb +++ b/lib/inline_oneboxer.rb @@ -89,6 +89,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) @@ -129,4 +141,8 @@ class InlineOneboxer author.username end end + + def self.preview_key(user_id) + "inline-onebox:preview:#{user_id}" + end end diff --git a/spec/requests/inline_onebox_controller_spec.rb b/spec/requests/inline_onebox_controller_spec.rb index c7110f3e6a1..5d6d2a2ab58 100644 --- a/spec/requests/inline_onebox_controller_spec.rb +++ b/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 } + + + + Concurrent inline-oneboxing test + + + + + 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 } + + + + Concurrent inline-oneboxing test + + + + + HTML + end + + stub_request(:get, "http://example.com/url-2").to_return do |request| + { status: 200, body: <<~HTML } + + + + Concurrent inline-oneboxing test 2 + + + + + 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)