Bug 1676345 - honour extensions from URLs with query parameters for useless mimetypes to avoid taking the extension from bogus local data, r=mak

This restores extension discovery logic to using the URL for requests where
there's no filename in the Content-Disposition header information, and where
the mimetype gives no indication of the 'correct' extension.

This is a short-term workaround for the larger issue that we should not have
bad local information for these mimetypes - fixing that is bug 1659008, but
requires using the extension to get a better mimetype, which this patch will
also help with.

Differential Revision: https://phabricator.services.mozilla.com/D97824

uriloader/exthandler/nsExternalHelperAppService.cpp

@@ -244,8 +244,23 @@ static bool GetFilenameAndExtensionFromChannel(nsIChannel* aChannel,
     // Windows ignores terminating dots. So we have to as well, so
     // that our security checks do "the right thing"
     aFileName.Trim(".", false);
+    // We can get an extension if the filename is from a header, or if getting
+    // it from the URL was allowed.
+    bool canGetExtensionFromFilename =
+      !gotFileNameFromURI ||
+      aAllowURLExtension;
+    // ... , or if the mimetype is meaningless and we have nothing to go on:
+    if (!canGetExtensionFromFilename) {
+      nsAutoCString contentType;
+      if (NS_SUCCEEDED(aChannel->GetContentType(contentType))) {
+          canGetExtensionFromFilename =
+            contentType.EqualsIgnoreCase(APPLICATION_OCTET_STREAM) ||
+            contentType.EqualsIgnoreCase("binary/octet-stream") ||
+            contentType.EqualsIgnoreCase("application/x-msdownload");
+      }
+    }
 
-    if (!gotFileNameFromURI || aAllowURLExtension) {
+    if (canGetExtensionFromFilename) {
       // XXX RFindCharInReadable!!
       nsAutoString fileNameStr(aFileName);
       int32_t idx = fileNameStr.RFindChar(char16_t('.'));

uriloader/exthandler/tests/mochitest/browser.ini

@@ -32,6 +32,9 @@ support-files =
   file_with[funny_name.webm
   file_with[funny_name.webm^headers^
 [browser_extension_correction.js]
+support-files =
+  file_as.exe
+  file_as.exe^headers^
 [browser_open_internal_choice_persistence.js]
 support-files =
   file_pdf_application_pdf.pdf

uriloader/exthandler/tests/mochitest/browser_extension_correction.js

@@ -3,32 +3,96 @@
 
 "use strict";
 
-async function testLinkWithoutExtension(type, expectExtension) {
+const TEST_PATH = getRootDirectory(gTestPath).replace(
+  "chrome://mochitests/content",
+  "https://example.com"
+);
+
+add_task(async function setup() {
+  await SpecialPowers.pushPrefEnv({
+    set: [["browser.download.useDownloadDir", true]],
+  });
+  registerCleanupFunction(async () => {
+    let publicList = await Downloads.getList(Downloads.PUBLIC);
+    let downloads = await publicList.getAll();
+    for (let download of downloads) {
+      if (download.target.exists) {
+        try {
+          info("removing " + download.target.path);
+          await IOUtils.remove(download.target.path);
+        } catch (ex) {
+          /* ignore */
+        }
+      }
+    }
+    await publicList.removeFinished();
+  });
+});
+
+async function testLinkWithoutExtension(type, shouldHaveExtension) {
   info("Checking " + type);
 
+  let task = function() {
+    return SpecialPowers.spawn(gBrowser.selectedBrowser, [type], mimetype => {
+      let link = content.document.createElement("a");
+      link.textContent = "Click me";
+      link.href = "data:" + mimetype + ",hello";
+      link.download = "somefile";
+      content.document.body.appendChild(link);
+      link.click();
+    });
+  };
+  await checkDownloadWithExtensionState(task, { type, shouldHaveExtension });
+}
+
+async function checkDownloadWithExtensionState(
+  task,
+  { type, shouldHaveExtension, expectedName = null }
+) {
   let winPromise = BrowserTestUtils.domWindowOpenedAndLoaded();
 
-  await SpecialPowers.spawn(gBrowser.selectedBrowser, [type], mimetype => {
-    let link = content.document.createElement("a");
-    link.textContent = "Click me";
-    link.href = "data:" + mimetype + ",hello";
-    link.download = "somefile";
-    content.document.body.appendChild(link);
-    link.click();
-  });
+  await task();
 
+  info("Waiting for dialog.");
   let win = await winPromise;
 
   let actualName = win.document.getElementById("location").value;
-  let expectedName = "somefile";
-  if (expectExtension) {
-    expectedName += "." + getMIMEInfoForType(type).primaryExtension;
+  if (shouldHaveExtension) {
+    expectedName ??= "somefile." + getMIMEInfoForType(type).primaryExtension;
     is(actualName, expectedName, `${type} should get an extension`);
   } else {
+    expectedName ??= "somefile";
     is(actualName, expectedName, `${type} should not get an extension`);
   }
 
-  await BrowserTestUtils.closeWindow(win);
+  let closedPromise = BrowserTestUtils.windowClosed(win);
+
+  if (shouldHaveExtension) {
+    // Wait for the download.
+    let publicList = await Downloads.getList(Downloads.PUBLIC);
+    let downloadFinishedPromise = promiseDownloadFinished(publicList);
+
+    // Then pick "save" in the dialog.
+    let dialog = win.document.getElementById("unknownContentType");
+    win.document.getElementById("save").click();
+    let button = dialog.getButton("accept");
+    button.disabled = false;
+    dialog.acceptDialog();
+
+    // Wait for the download to finish and check the extension is correct.
+    let download = await downloadFinishedPromise;
+    is(
+      PathUtils.filename(download.target.path),
+      expectedName,
+      `Downloaded file should also match ${expectedName}`
+    );
+    await IOUtils.remove(download.target.path);
+  } else {
+    // We just cancel out for files that would end up without a path, as we'd
+    // prompt for a filename.
+    win.close();
+  }
+  return closedPromise;
 }
 
 /**
@@ -49,3 +113,34 @@ add_task(async function test_enforce_useful_extension() {
     await testLinkWithoutExtension("application/x-msdownload", false);
   });
 });
+
+/**
+ * Check that we still use URL extension info when we don't have anything else,
+ * despite bogus local info.
+ */
+add_task(async function test_broken_saved_handlerinfo_and_useless_mimetypes() {
+  let bogusType = getMIMEInfoForType("binary/octet-stream");
+  bogusType.setFileExtensions(["jpg"]);
+  let handlerSvc = Cc["@mozilla.org/uriloader/handler-service;1"].getService(
+    Ci.nsIHandlerService
+  );
+  handlerSvc.store(bogusType);
+  let tabToClean = null;
+  let task = function() {
+    return BrowserTestUtils.openNewForegroundTab(
+      gBrowser,
+      TEST_PATH + "file_as.exe?foo=bar"
+    ).then(tab => {
+      return (tabToClean = tab);
+    });
+  };
+  await checkDownloadWithExtensionState(task, {
+    type: "binary/octet-stream",
+    shouldHaveExtension: true,
+    expectedName: "file_as.exe",
+  });
+  // Downloads should really close their tabs...
+  if (tabToClean?.isConnected) {
+    BrowserTestUtils.removeTab(tabToClean);
+  }
+});

uriloader/exthandler/tests/mochitest/file_as.exe

@@ -0,0 +1 @@
+Not actually an executable... but let's pretend!

uriloader/exthandler/tests/mochitest/file_as.exe^headers^

@@ -0,0 +1,2 @@
+Content-Type: binary/octet-stream
+Content-Disposition: attachment