From f0072dd8978f7eb3dc0b1138a11f66ff310b87e9 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 3 Mar 2020 15:44:01 +1100 Subject: [PATCH] FIX: Stop infinite lookup-urls issue for video/audio on page (#9096) Meta report: https://meta.discourse.org/t/excessive-requests-to-uploads-lookup-urls-leading-to-429-response/143119 * The data-orig-src attribute was not being removed from cooked video and audio so the composer was infinitely trying to get the URLs for them, which would never resolve to anything * Also the code that retrieved the short URL was unscoped, and was getting everything on the page. if running from the composer we now scope to the preview window * Also fixed a minor issue where the element href for the video and audio tags was not being set when the short URL was found --- .../components/composer-editor.js.es6 | 2 +- .../engines/discourse-markdown-it.js.es6 | 6 ++- .../pretty-text/upload-short-url.js.es6 | 29 ++++++++--- test/javascripts/lib/pretty-text-test.js.es6 | 50 +++++++++++++++++++ .../lib/upload-short-url-test.js.es6 | 28 ++++++++++- 5 files changed, 103 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6 index 24c71dd8fb..5a47546093 100644 --- a/app/assets/javascripts/discourse/components/composer-editor.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-editor.js.es6 @@ -1014,7 +1014,7 @@ export default Component.extend({ ); // Short upload urls need resolution - resolveAllShortUrls(ajax); + resolveAllShortUrls(ajax, ".d-editor-preview-wrapper"); if (this._enableAdvancedEditorPreviewSync()) { this._syncScroll( diff --git a/app/assets/javascripts/pretty-text/engines/discourse-markdown-it.js.es6 b/app/assets/javascripts/pretty-text/engines/discourse-markdown-it.js.es6 index 6e6392d774..213aef39d9 100644 --- a/app/assets/javascripts/pretty-text/engines/discourse-markdown-it.js.es6 +++ b/app/assets/javascripts/pretty-text/engines/discourse-markdown-it.js.es6 @@ -145,9 +145,10 @@ function videoHTML(token, opts) { const src = token.attrGet("src"); const origSrc = token.attrGet("data-orig-src"); const preloadType = opts.secureMedia ? "none" : "metadata"; + const dataOrigSrcAttr = origSrc !== null ? `data-orig-src="${origSrc}"` : ""; return `
`; @@ -157,8 +158,9 @@ function audioHTML(token, opts) { const src = token.attrGet("src"); const origSrc = token.attrGet("data-orig-src"); const preloadType = opts.secureMedia ? "none" : "metadata"; + const dataOrigSrcAttr = origSrc !== null ? `data-orig-src="${origSrc}"` : ""; return ``; } diff --git a/app/assets/javascripts/pretty-text/upload-short-url.js.es6 b/app/assets/javascripts/pretty-text/upload-short-url.js.es6 index b82e524173..294bae54de 100644 --- a/app/assets/javascripts/pretty-text/upload-short-url.js.es6 +++ b/app/assets/javascripts/pretty-text/upload-short-url.js.es6 @@ -9,6 +9,11 @@ export function lookupCachedUploadUrl(shortUrl) { const MISSING = "missing"; export function lookupUncachedUploadUrls(urls, ajax) { + urls = _.compact(urls); + if (urls.length === 0) { + return; + } + return ajax("/uploads/lookup-urls", { method: "POST", data: { short_urls: urls } @@ -77,7 +82,7 @@ function _loadCachedShortUrls($uploads) { }); break; - case "SOURCE": // video tag > source tag + case "SOURCE": // video/audio tag > source tag retrieveCachedUrl($upload, "orig-src", url => { $upload.attr("src", url); @@ -85,11 +90,19 @@ function _loadCachedShortUrls($uploads) { let hostRegex = new RegExp("//" + window.location.host, "g"); url = url.replace(hostRegex, ""); } - $upload.attr("src", window.location.origin + url); + let fullUrl = window.location.origin + url; + $upload.attr("src", fullUrl); // this is necessary, otherwise because of the src change the - // video just doesn't bother loading! - $upload.parent()[0].load(); + // video/audio just doesn't bother loading! + let $parent = $upload.parent(); + $parent[0].load(); + + // set the url and text for the tag within the
+ /secure-media-uploads/original/3X/c/b/test.mp4 + +

`, + "It returns the correct video HTML when the URL is mapped with secure media, removing data-orig-src" + ); +}); + QUnit.test("audio", assert => { assert.cooked( "![young americans|audio](upload://eyPnj7UzkU0AkGkx2dx8G4YM1Jx.mp3)", @@ -1023,6 +1049,30 @@ QUnit.test("audio", assert => { ); }); +QUnit.test("audio - mapped url - secure media enabled", assert => { + function lookupUploadUrls() { + let cache = {}; + cache["upload://eyPnj7UzkU0AkGkx2dx8G4YM1Jx.mp3"] = { + short_url: "upload://eyPnj7UzkU0AkGkx2dx8G4YM1Jx.mp3", + url: "/secure-media-uploads/original/3X/c/b/test.mp3", + short_path: "/uploads/short-url/blah" + }; + return cache; + } + assert.cookedOptions( + "![baby shark|audio](upload://eyPnj7UzkU0AkGkx2dx8G4YM1Jx.mp3)", + { + siteSettings: { secure_media: true }, + lookupUploadUrls: lookupUploadUrls + }, + `

`, + "It returns the correct audio HTML when the URL is mapped with secure media, removing data-orig-src" + ); +}); + QUnit.test("censoring", assert => { assert.cookedOptions( "Pleased to meet you, but pleeeease call me later, xyz123", diff --git a/test/javascripts/lib/upload-short-url-test.js.es6 b/test/javascripts/lib/upload-short-url-test.js.es6 index f969e64878..4e7422c874 100644 --- a/test/javascripts/lib/upload-short-url-test.js.es6 +++ b/test/javascripts/lib/upload-short-url-test.js.es6 @@ -22,6 +22,11 @@ QUnit.module("lib:pretty-text/upload-short-url", { short_url: "upload://b.jpeg", url: "/uploads/default/original/3X/c/b/2.jpeg", short_path: "/uploads/short-url/b.jpeg" + }, + { + short_url: "upload://z.jpeg", + url: "/uploads/default/original/3X/c/b/9.jpeg", + short_path: "/uploads/short-url/z.jpeg" } ]; @@ -53,7 +58,8 @@ QUnit.module("lib:pretty-text/upload-short-url", { fixture().html( imageSrcs.map(src => ``).join("") + - attachmentSrcs.map(src => ``).join("") + attachmentSrcs.map(src => ``).join("") + + `
` ); }, @@ -105,3 +111,23 @@ QUnit.test("resolveAllShortUrls", async assert => { short_path: "/uploads/short-url/e.mp3" }); }); + +QUnit.test("resolveAllShortUrls - scoped", async assert => { + let lookup; + await resolveAllShortUrls(ajax, ".scoped-area"); + + lookup = lookupCachedUploadUrl("upload://z.jpeg"); + + assert.deepEqual(lookup, { + url: "/uploads/default/original/3X/c/b/9.jpeg", + short_path: "/uploads/short-url/z.jpeg" + }); + + // do this because the pretender caches ALL the urls, not + // just the ones being looked up (like the normal behaviour) + resetCache(); + await resolveAllShortUrls(ajax, ".scoped-area"); + + lookup = lookupCachedUploadUrl("upload://a.jpeg"); + assert.deepEqual(lookup, {}); +});