Use full URL for secure attachments

* 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
* add additional specs

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

@@ -41,8 +41,7 @@ export function resetCache() {
 
 function retrieveCachedUrl($upload, dataAttribute, callback) {
   const cachedUpload = lookupCachedUploadUrl($upload.data(dataAttribute));
-  const url =
-    dataAttribute === "orig-href" ? cachedUpload.short_path : cachedUpload.url;
+  const url = getAttributeBasedUrl(dataAttribute, cachedUpload);
 
   if (url) {
     $upload.removeAttr(`data-${dataAttribute}`);
@@ -52,6 +51,24 @@ function retrieveCachedUrl($upload, dataAttribute, callback) {
   }
 }
 
+function getAttributeBasedUrl(dataAttribute, cachedUpload) {
+  // 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 (
+    Discourse.SiteSettings.secure_media &&
+    cachedUpload.url.indexOf("secure-media-uploads") > -1
+  ) {
+    return cachedUpload.url;
+  }
+
+  return cachedUpload.short_path;
+}
+
 function _loadCachedShortUrls($uploads) {
   $uploads.each((_idx, upload) => {
     const $upload = $(upload);

lib/pretty_text/helpers.rb

@@ -68,7 +68,7 @@ def lookup_upload_urls(urls)
           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] = {

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: "/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,11 +26,41 @@ 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()
       .trim(),
     '<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="/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

@@ -52,8 +52,8 @@ QUnit.module("lib:pretty-text/upload-short-url", {
     });
 
     fixture().html(
-      imageSrcs.map(src => `<img data-orig-src="${src.url}">`).join("") +
-        attachmentSrcs.map(src => `<a data-orig-href="${src.url}">`).join("")
+      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("")
     );
   },
 
@@ -105,3 +105,23 @@ QUnit.test("resolveAllShortUrls", async assert => {
     short_path: "/uploads/short-url/e.mp3"
   });
 });
+
+QUnit.test("resolveAllShortUrls - href + src replaced correctly", async assert => {
+  await resolveAllShortUrls(ajax);
+
+  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 => {
+  Discourse.SiteSettings.secure_media = true;
+  await resolveAllShortUrls(ajax);
+
+  let link = fixture().find('a');
+  assert.equal(link.attr('href'), "/uploads/default/original/3X/c/b/3.pdf");
+});