diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6 index 5a47546093..52f44e78f5 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, ".d-editor-preview-wrapper"); + resolveAllShortUrls(ajax, this.siteSettings, ".d-editor-preview-wrapper"); if (this._enableAdvancedEditorPreviewSync()) { this._syncScroll( diff --git a/app/assets/javascripts/discourse/components/cook-text.js.es6 b/app/assets/javascripts/discourse/components/cook-text.js.es6 index 256636a3c5..f3ad48f549 100644 --- a/app/assets/javascripts/discourse/components/cook-text.js.es6 +++ b/app/assets/javascripts/discourse/components/cook-text.js.es6 @@ -16,7 +16,7 @@ const CookText = Component.extend({ next(() => window .requireModule("pretty-text/upload-short-url") - .resolveAllShortUrls(ajax) + .resolveAllShortUrls(ajax, this.siteSettings) ); }); } diff --git a/app/assets/javascripts/pretty-text/engines/discourse-markdown/upload-protocol.js.es6 b/app/assets/javascripts/pretty-text/engines/discourse-markdown/upload-protocol.js.es6 index 3e075b13ac..174e603212 100644 --- a/app/assets/javascripts/pretty-text/engines/discourse-markdown/upload-protocol.js.es6 +++ b/app/assets/javascripts/pretty-text/engines/discourse-markdown/upload-protocol.js.es6 @@ -60,7 +60,16 @@ function rule(state) { break; case "a": if (mapped) { - token.attrs[srcIndex][1] = mapped.short_path; + // when secure media is enabled we want the full /secure-media-uploads/ + // url to take advantage of access control security + if ( + state.md.options.discourse.limitedSiteSettings.secureMedia && + mapped.url.indexOf("secure-media-uploads") > -1 + ) { + token.attrs[srcIndex][1] = mapped.url; + } else { + token.attrs[srcIndex][1] = mapped.short_path; + } } else { token.attrs[srcIndex][1] = state.md.options.discourse.getURL( "/404" 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 294bae54de..401d73dff0 100644 --- a/app/assets/javascripts/pretty-text/upload-short-url.js.es6 +++ b/app/assets/javascripts/pretty-text/upload-short-url.js.es6 @@ -44,10 +44,9 @@ export function resetCache() { _cache = {}; } -function retrieveCachedUrl($upload, dataAttribute, callback) { +function retrieveCachedUrl($upload, siteSettings, dataAttribute, callback) { const cachedUpload = lookupCachedUploadUrl($upload.data(dataAttribute)); - const url = - dataAttribute === "orig-href" ? cachedUpload.short_path : cachedUpload.url; + const url = getAttributeBasedUrl(dataAttribute, cachedUpload, siteSettings); if (url) { $upload.removeAttr(`data-${dataAttribute}`); @@ -57,12 +56,34 @@ function retrieveCachedUrl($upload, dataAttribute, callback) { } } -function _loadCachedShortUrls($uploads) { +function getAttributeBasedUrl(dataAttribute, cachedUpload, siteSettings) { + if (!cachedUpload.url) { + return; + } + + // non-attachments always use the full URL + if (dataAttribute !== "orig-href") { + return cachedUpload.url; + } + + // attachments should use the full /secure-media-uploads/ URL + // in this case for permission checks + if ( + siteSettings.secure_media && + cachedUpload.url.indexOf("secure-media-uploads") > -1 + ) { + return cachedUpload.url; + } + + return cachedUpload.short_path; +} + +function _loadCachedShortUrls($uploads, siteSettings) { $uploads.each((_idx, upload) => { const $upload = $(upload); switch (upload.tagName) { case "A": - retrieveCachedUrl($upload, "orig-href", url => { + retrieveCachedUrl($upload, siteSettings, "orig-href", url => { $upload.attr("href", url); // Replace "|attachment" with class='attachment' @@ -77,13 +98,13 @@ function _loadCachedShortUrls($uploads) { break; case "IMG": - retrieveCachedUrl($upload, "orig-src", url => { + retrieveCachedUrl($upload, siteSettings, "orig-src", url => { $upload.attr("src", url); }); break; case "SOURCE": // video/audio tag > source tag - retrieveCachedUrl($upload, "orig-src", url => { + retrieveCachedUrl($upload, siteSettings, "orig-src", url => { $upload.attr("src", url); if (url.startsWith(`//${window.location.host}`)) { @@ -110,31 +131,31 @@ function _loadCachedShortUrls($uploads) { }); } -function _loadShortUrls($uploads, ajax) { +function _loadShortUrls($uploads, ajax, siteSettings) { let urls = $uploads.toArray().map(upload => { const $upload = $(upload); return $upload.data("orig-src") || $upload.data("orig-href"); }); return lookupUncachedUploadUrls(urls, ajax).then(() => - _loadCachedShortUrls($uploads) + _loadCachedShortUrls($uploads, siteSettings) ); } -export function resolveAllShortUrls(ajax, scope = null) { +export function resolveAllShortUrls(ajax, siteSettings, scope = null) { const attributes = "img[data-orig-src], a[data-orig-href], source[data-orig-src]"; let $shortUploadUrls = $(scope || document).find(attributes); if ($shortUploadUrls.length > 0) { - _loadCachedShortUrls($shortUploadUrls); + _loadCachedShortUrls($shortUploadUrls, siteSettings); $shortUploadUrls = $(scope || document).find(attributes); if ($shortUploadUrls.length > 0) { // this is carefully batched so we can do a leading debounce (trigger right away) return debounce( null, - () => _loadShortUrls($shortUploadUrls, ajax), + () => _loadShortUrls($shortUploadUrls, ajax, siteSettings), 450, true ); diff --git a/lib/pretty_text/helpers.rb b/lib/pretty_text/helpers.rb index 0de6000fea..d207e01eed 100644 --- a/lib/pretty_text/helpers.rb +++ b/lib/pretty_text/helpers.rb @@ -68,7 +68,7 @@ module PrettyText sha1, url, extension, original_filename, secure = row if short_urls = reverse_map[sha1] - secure_media = FileHelper.is_supported_media?(original_filename) && SiteSetting.secure_media? && secure + secure_media = SiteSetting.secure_media? && secure short_urls.each do |short_url| result[short_url] = { diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb index e27a4d4671..2f3333cacd 100644 --- a/spec/requests/uploads_controller_spec.rb +++ b/spec/requests/uploads_controller_spec.rb @@ -578,7 +578,7 @@ describe UploadsController do expect(result[0]["short_path"]).to eq(upload.short_path) end - it 'does not return secure urls for non-media uploads' do + it 'returns secure urls for non-media uploads' do upload.update!(original_filename: "not-an-image.pdf", extension: "pdf") sign_in(user) @@ -586,7 +586,7 @@ describe UploadsController do expect(response.status).to eq(200) result = JSON.parse(response.body) - expect(result[0]["url"]).not_to match("/secure-media-uploads") + expect(result[0]["url"]).to match("/secure-media-uploads") expect(result[0]["short_path"]).to eq(upload.short_path) end end diff --git a/test/javascripts/acceptance/composer-attachment-test.js.es6 b/test/javascripts/acceptance/composer-attachment-test.js.es6 index edf6bc4374..0333936f4b 100644 --- a/test/javascripts/acceptance/composer-attachment-test.js.es6 +++ b/test/javascripts/acceptance/composer-attachment-test.js.es6 @@ -1,21 +1,18 @@ import { acceptance } from "helpers/qunit-helpers"; -acceptance("Composer Attachment", { - loggedIn: true, - pretend(server, helper) { - server.post("/uploads/lookup-urls", () => { - return helper.response([ - { - short_url: "upload://asdsad.png", - url: "/uploads/default/3X/1/asjdiasjdiasida.png", - short_path: "/uploads/short-url/asdsad.png" - } - ]); - }); - } -}); +function setupPretender(server, helper) { + server.post("/uploads/lookup-urls", () => { + return helper.response([ + { + short_url: "upload://asdsad.png", + url: "/secure-media-uploads/default/3X/1/asjdiasjdiasida.png", + short_path: "/uploads/short-url/asdsad.png" + } + ]); + }); +} -QUnit.test("attachments are cooked properly", async assert => { +async function writeInComposer(assert) { await visit("/t/internationalization-localization/280"); await click("#topic-footer-buttons .btn.create"); @@ -29,7 +26,17 @@ QUnit.test("attachments are cooked properly", async assert => { ); await fillIn(".d-editor-input", "[test|attachment](upload://asdsad.png)"); +} +acceptance("Composer Attachment", { + loggedIn: true, + pretend(server, helper) { + setupPretender(server, helper); + } +}); + +QUnit.test("attachments are cooked properly", async assert => { + await writeInComposer(assert); assert.equal( find(".d-editor-preview:visible") .html() @@ -37,3 +44,26 @@ QUnit.test("attachments are cooked properly", async assert => { '

test

' ); }); + +acceptance("Composer Attachment - Secure Media Enabled", { + loggedIn: true, + settings: { + secure_media: true + }, + pretend(server, helper) { + setupPretender(server, helper); + } +}); + +QUnit.test( + "attachments are cooked properly when secure media is enabled", + async assert => { + await writeInComposer(assert); + assert.equal( + find(".d-editor-preview:visible") + .html() + .trim(), + '

test

' + ); + } +); diff --git a/test/javascripts/helpers/site-settings.js b/test/javascripts/helpers/site-settings.js index 96807b7a7c..621aa3d6ac 100644 --- a/test/javascripts/helpers/site-settings.js +++ b/test/javascripts/helpers/site-settings.js @@ -99,7 +99,8 @@ Discourse.SiteSettingsOriginal = { desktop_category_page_style: "categories_and_latest_topics", enable_mentions: true, enable_personal_messages: true, - unicode_usernames: false + unicode_usernames: false, + secure_media: false }; Discourse.SiteSettings = jQuery.extend( true, diff --git a/test/javascripts/lib/pretty-text-test.js.es6 b/test/javascripts/lib/pretty-text-test.js.es6 index 23708ddacb..7c75034529 100644 --- a/test/javascripts/lib/pretty-text-test.js.es6 +++ b/test/javascripts/lib/pretty-text-test.js.es6 @@ -973,6 +973,58 @@ QUnit.test("images", assert => { ); }); +QUnit.test("attachment", assert => { + assert.cooked( + "[test.pdf|attachment](upload://o8iobpLcW3WSFvVH7YQmyGlKmGM.pdf)", + `

test.pdf

`, + "It returns the correct attachment link HTML" + ); +}); + +QUnit.test("attachment - mapped url - secure media disabled", assert => { + function lookupUploadUrls() { + let cache = {}; + cache["upload://o8iobpLcW3WSFvVH7YQmyGlKmGM.pdf"] = { + short_url: "upload://o8iobpLcW3WSFvVH7YQmyGlKmGM.pdf", + url: + "/secure-media-uploads/original/3X/c/b/o8iobpLcW3WSFvVH7YQmyGlKmGM.pdf", + short_path: "/uploads/short-url/blah" + }; + return cache; + } + assert.cookedOptions( + "[test.pdf|attachment](upload://o8iobpLcW3WSFvVH7YQmyGlKmGM.pdf)", + { + siteSettings: { secure_media: false }, + lookupUploadUrls: lookupUploadUrls + }, + `

test.pdf

`, + "It returns the correct attachment link HTML when the URL is mapped without secure media" + ); +}); + +QUnit.test("attachment - mapped url - secure media enabled", assert => { + function lookupUploadUrls() { + let cache = {}; + cache["upload://o8iobpLcW3WSFvVH7YQmyGlKmGM.pdf"] = { + short_url: "upload://o8iobpLcW3WSFvVH7YQmyGlKmGM.pdf", + url: + "/secure-media-uploads/original/3X/c/b/o8iobpLcW3WSFvVH7YQmyGlKmGM.pdf", + short_path: "/uploads/short-url/blah" + }; + return cache; + } + assert.cookedOptions( + "[test.pdf|attachment](upload://o8iobpLcW3WSFvVH7YQmyGlKmGM.pdf)", + { + siteSettings: { secure_media: true }, + lookupUploadUrls: lookupUploadUrls + }, + `

test.pdf

`, + "It returns the correct attachment link HTML when the URL is mapped with secure media" + ); +}); + QUnit.test("video - secure media enabled", assert => { assert.cookedOptions( "![baby shark|video](upload://eyPnj7UzkU0AkGkx2dx8G4YM1Jx.mp4)", diff --git a/test/javascripts/lib/upload-short-url-test.js.es6 b/test/javascripts/lib/upload-short-url-test.js.es6 index f0521a174b..849b2d7602 100644 --- a/test/javascripts/lib/upload-short-url-test.js.es6 +++ b/test/javascripts/lib/upload-short-url-test.js.es6 @@ -7,13 +7,12 @@ import { ajax } from "discourse/lib/ajax"; import { fixture } from "helpers/qunit-helpers"; import pretender from "helpers/create-pretender"; -QUnit.module("lib:pretty-text/upload-short-url", { - beforeEach() { - const response = object => { - return [200, { "Content-Type": "application/json" }, object]; - }; - - const imageSrcs = [ +function stubUrls(imageSrcs, attachmentSrcs, otherMediaSrcs) { + const response = object => { + return [200, { "Content-Type": "application/json" }, object]; + }; + if (!imageSrcs) { + imageSrcs = [ { short_url: "upload://a.jpeg", url: "/uploads/default/original/3X/c/b/1.jpeg", @@ -30,16 +29,20 @@ QUnit.module("lib:pretty-text/upload-short-url", { short_path: "/uploads/short-url/z.jpeg" } ]; + } - const attachmentSrcs = [ + if (!attachmentSrcs) { + attachmentSrcs = [ { short_url: "upload://c.pdf", url: "/uploads/default/original/3X/c/b/3.pdf", short_path: "/uploads/short-url/c.pdf" } ]; + } - const otherMediaSrcs = [ + if (!otherMediaSrcs) { + otherMediaSrcs = [ { short_url: "upload://d.mp4", url: "/uploads/default/original/3X/c/b/4.mp4", @@ -51,30 +54,37 @@ QUnit.module("lib:pretty-text/upload-short-url", { short_path: "/uploads/short-url/e.mp3" } ]; + } + // prettier-ignore + pretender.post("/uploads/lookup-urls", () => { //eslint-disable-line + return response(imageSrcs.concat(attachmentSrcs.concat(otherMediaSrcs))); + }); - pretender.post("/uploads/lookup-urls", () => { - return response(imageSrcs.concat(attachmentSrcs.concat(otherMediaSrcs))); - }); - - fixture().html( - imageSrcs.map(src => ``).join("") + - attachmentSrcs.map(src => ``).join("") + - `
` - ); - }, - + fixture().html( + imageSrcs.map(src => ``).join("") + + attachmentSrcs + .map( + src => + `big enterprise contract.pdf` + ) + .join("") + + `
` + ); +} +QUnit.module("lib:pretty-text/upload-short-url", { afterEach() { resetCache(); } }); QUnit.test("resolveAllShortUrls", async assert => { + stubUrls(); let lookup; lookup = lookupCachedUploadUrl("upload://a.jpeg"); assert.deepEqual(lookup, {}); - await resolveAllShortUrls(ajax); + await resolveAllShortUrls(ajax, { secure_media: false }); lookup = lookupCachedUploadUrl("upload://a.jpeg"); @@ -112,7 +122,52 @@ QUnit.test("resolveAllShortUrls", async assert => { }); }); +QUnit.test( + "resolveAllShortUrls - href + src replaced correctly", + async assert => { + stubUrls(); + await resolveAllShortUrls(ajax, { secure_media: false }); + + let image1 = fixture() + .find("img") + .eq(0); + let image2 = fixture() + .find("img") + .eq(1); + let link = fixture().find("a"); + + assert.equal(image1.attr("src"), "/uploads/default/original/3X/c/b/1.jpeg"); + assert.equal(image2.attr("src"), "/uploads/default/original/3X/c/b/2.jpeg"); + assert.equal(link.attr("href"), "/uploads/short-url/c.pdf"); + } +); + +QUnit.test( + "resolveAllShortUrls - when secure media is enabled use the attachment full URL", + async assert => { + stubUrls( + null, + [ + { + short_url: "upload://c.pdf", + url: "/secure-media-uploads/default/original/3X/c/b/3.pdf", + short_path: "/uploads/short-url/c.pdf" + } + ], + null + ); + await resolveAllShortUrls(ajax, { secure_media: true }); + + let link = fixture().find("a"); + assert.equal( + link.attr("href"), + "/secure-media-uploads/default/original/3X/c/b/3.pdf" + ); + } +); + QUnit.test("resolveAllShortUrls - scoped", async assert => { + stubUrls(); let lookup; await resolveAllShortUrls(ajax, ".scoped-area");