FIX: Use full URL for secure attachments when secure media enabled (#9037)

When secure media is enabled and an attachment is marked as secure we want to use the full url instead of the short-url so we get the same access control post protections as secure media uploads.

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(

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)
       );
     });
   }

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"

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

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] = {

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

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 => {
     '<p><a class="attachment" href="/uploads/short-url/asdsad.png">test</a></p>'
   );
 });
+
+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(),
+      '<p><a class="attachment" href="/secure-media-uploads/default/3X/1/asjdiasjdiasida.png">test</a></p>'
+    );
+  }
+);

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,

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)",
+    `<p><a class="attachment" href="/404" data-orig-href="upload://o8iobpLcW3WSFvVH7YQmyGlKmGM.pdf">test.pdf</a></p>`,
+    "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
+    },
+    `<p><a class="attachment" href="/uploads/short-url/blah">test.pdf</a></p>`,
+    "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
+    },
+    `<p><a class="attachment" href="/secure-media-uploads/original/3X/c/b/o8iobpLcW3WSFvVH7YQmyGlKmGM.pdf">test.pdf</a></p>`,
+    "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)",

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 => `<img data-orig-src="${src.url}">`).join("") +
-        attachmentSrcs.map(src => `<a data-orig-href="${src.url}">`).join("") +
-        `<div class="scoped-area"><img data-orig-src="${imageSrcs[2].url}"></div>`
-    );
-  },
-
+  fixture().html(
+    imageSrcs.map(src => `<img data-orig-src="${src.short_url}"/>`).join("") +
+      attachmentSrcs
+        .map(
+          src =>
+            `<a data-orig-href="${src.short_url}">big enterprise contract.pdf</a>`
+        )
+        .join("") +
+      `<div class="scoped-area"><img data-orig-src="${imageSrcs[2].url}"></div>`
+  );
+}
+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");