Bug 1706871 - Fix HandleInternally + Insecure Downloads r=mak

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

dom/security/test/mixedcontentblocker/browser_test_mixed_content_download.js

@@ -41,8 +41,15 @@ async function task_openPanel() {
   await promise;
 }
 
-function shouldPromptDownload() {
-  // Waits until the download Prompt is shown
+/**
+ * Waits until the download Prompt is shown, selects
+ * the choosen <action>, then accepts the dialog
+ * @param [action] Which action to select, either:
+ *        "handleInternally", "save" or "open".
+ * @returns {Promise} Resolved once done.
+ */
+
+function shouldPromptDownload(action = "save") {
   return new Promise((resolve, reject) => {
     Services.wm.addListener({
       onOpenWindow(xulWin) {
@@ -55,8 +62,8 @@ function shouldPromptDownload() {
           ) {
             let dialog = win.document.getElementById("unknownContentType");
             let button = dialog.getButton("accept");
-            let saveRadio = win.document.getElementById("save");
-            saveRadio.click();
+            let actionRadio = win.document.getElementById(action);
+            actionRadio.click();
             button.disabled = false;
             dialog.acceptDialog();
             resolve();
@@ -99,14 +106,22 @@ async function shouldNotifyDownloadUI() {
   // Waits until a Blocked download was added to the Download List
   // -> returns the blocked Download
   let list = await Downloads.getList(Downloads.ALL);
-  return new Promise(res => {
+  return new Promise((res, err) => {
     const view = {
-      onDownloadAdded: aDownload => {
+      onDownloadAdded: async aDownload => {
         let { error } = aDownload;
         if (
           error.becauseBlockedByReputationCheck &&
           error.reputationCheckVerdict == Downloads.Error.BLOCK_VERDICT_INSECURE
         ) {
+          // It's an insecure Download, now Check that it has been cleaned up properly
+          if ((await IOUtils.stat(aDownload.target.path)).size != 0) {
+            throw new Error(`Download target is not empty!`);
+          }
+          if ((await IOUtils.stat(aDownload.target.path)).size != 0) {
+            throw new Error(`Download partFile was not cleaned up properly`);
+          }
+
           res(aDownload);
           list.removeView(view);
         }
@@ -116,7 +131,7 @@ async function shouldNotifyDownloadUI() {
   });
 }
 
-async function runTest(url, link, checkFunction, decscription) {
+async function runTest(url, link, checkFunction, description) {
   await SpecialPowers.pushPrefEnv({
     set: [["dom.block_download_insecure", true]],
   });
@@ -128,7 +143,7 @@ async function runTest(url, link, checkFunction, decscription) {
   let browser = gBrowser.getBrowserForTab(tab);
   await BrowserTestUtils.browserLoaded(browser);
 
-  info("Checking: " + decscription);
+  info("Checking: " + description);
 
   let checkPromise = checkFunction();
   // Click the Link to trigger the download
@@ -138,7 +153,7 @@ async function runTest(url, link, checkFunction, decscription) {
 
   await checkPromise;
 
-  ok(true, decscription);
+  ok(true, description);
   BrowserTestUtils.removeTab(tab);
 
   await SpecialPowers.popPrefEnv();
@@ -213,3 +228,28 @@ add_task(async function() {
     "A Blocked Download Should open the Download Panel"
   );
 });
+
+// Test Download an insecure pdf and choose "Open with Firefox"
+add_task(async function download_open_insecure_pdf() {
+  await promiseFocus();
+  await runTest(
+    SECURE_BASE_URL,
+    "insecurePDF",
+    async () => {
+      info("awaiting that the Download Prompt is shown");
+      await shouldPromptDownload("handleInternally");
+      let download = await shouldNotifyDownloadUI();
+      let newTabPromise = BrowserTestUtils.waitForNewTab(gBrowser);
+      await download.unblock();
+      ok(download.error == null, "There should be no error after unblocking");
+      let tab = await newTabPromise;
+      ok(
+        tab.linkedBrowser._documentURI.filePath == download.target.path,
+        "The download target was opened"
+      );
+      BrowserTestUtils.removeTab(tab);
+      ok(true, "The Content was opened in a new tab");
+    },
+    "A Blocked PDF can be opened internally"
+  );
+});

dom/security/test/mixedcontentblocker/download_page.html

@@ -19,7 +19,6 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=676619
       secureLink.href=`https://${host}/${path}`;
       secureLink.download="true";
       secureLink.textContent="Secure Link";
-      
       secureLink.id="secure";
 
       const insecureLink = document.createElement("a");        
@@ -28,6 +27,13 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=676619
       insecureLink.id="insecure";
       insecureLink.textContent="Not secure Link";
 
+      const insecurePDF = document.createElement("a");        
+      insecurePDF.href=`http://${host}/${path}?type=application/pdf&name=example.pdf`;
+      insecurePDF.download="true";
+      insecurePDF.id="insecurePDF";
+      insecurePDF.textContent="Not secure Link to PDF";
+
+      document.body.append(insecurePDF);
       document.body.append(secureLink);
       document.body.append(insecureLink);
   </script>

dom/security/test/mixedcontentblocker/download_server.sjs

@@ -2,8 +2,20 @@
 
 function handleRequest(request, response)
 {
+  let type = "image/png";
+  let filename = "hello.png";
+  request.queryString.split('&').forEach((val) => {
+    var [key, value] = val.split('=');
+    if (key == "type"){
+      type= value;
+    }
+    if (key == "name"){
+      filename = value;
+    }
+  });
+
   response.setHeader("Cache-Control", "no-cache", false);
-  response.setHeader("Content-Disposition", "attachment");
-  response.setHeader("Content-Type", "image/png");
+  response.setHeader("Content-Disposition", `attachment; filename=${filename}`);
+  response.setHeader("Content-Type", type);
   response.write('🙈🙊🐵🙊');
 }

toolkit/components/downloads/DownloadCore.jsm

@@ -29,6 +29,7 @@ const { XPCOMUtils } = ChromeUtils.import(
 XPCOMUtils.defineLazyModuleGetters(this, {
   AppConstants: "resource://gre/modules/AppConstants.jsm",
   DownloadHistory: "resource://gre/modules/DownloadHistory.jsm",
+  DownloadPaths: "resource://gre/modules/DownloadPaths.jsm",
   E10SUtils: "resource://gre/modules/E10SUtils.jsm",
   FileUtils: "resource://gre/modules/FileUtils.jsm",
   NetUtil: "resource://gre/modules/NetUtil.jsm",
@@ -2470,7 +2471,21 @@ DownloadCopySaver.prototype = {
     }
 
     if (partFilePath) {
-      await IOUtils.move(partFilePath, targetPath);
+      try {
+        await IOUtils.move(partFilePath, targetPath);
+      } catch (e) {
+        if (e.name === "NotAllowedError") {
+          // In case we cannot write to the target file
+          // retry with a new unique name
+          let uniquePath = DownloadPaths.createNiceUniqueFile(
+            new FileUtils.File(targetPath)
+          ).path;
+          await IOUtils.move(partFilePath, uniquePath);
+          this.download.target.path = uniquePath;
+        } else {
+          throw e;
+        }
+      }
     }
   },
 

uriloader/exthandler/nsExternalHelperAppService.cpp

@@ -1704,6 +1704,11 @@ NS_IMETHODIMP nsExternalAppHandler::OnStartRequest(nsIRequest* request) {
     request->Cancel(NS_ERROR_ABORT);
     return NS_OK;
   }
+  if (mDownloadClassification != nsITransfer::DOWNLOAD_ACCEPTABLE) {
+    // in any other case, we need to suspend the request so
+    // no bytes are transferred into the tempFile
+    request->Suspend();
+  }
 
   nsCOMPtr<nsIFileChannel> fileChan(do_QueryInterface(request));
   mIsFileChannel = fileChan != nullptr;
@@ -2291,6 +2296,10 @@ nsresult nsExternalAppHandler::CreateTransfer() {
   if (mDownloadClassification != nsITransfer::DOWNLOAD_ACCEPTABLE) {
     mCanceled = true;
     mRequest->Cancel(NS_ERROR_ABORT);
+    if (mSaver) {
+      mSaver->Finish(NS_ERROR_ABORT);
+      mSaver = nullptr;
+    }
     return CreateFailedTransfer();
   }
   nsresult rv;
@@ -2393,11 +2402,11 @@ nsresult nsExternalAppHandler::CreateFailedTransfer() {
   if (mBrowsingContext) {
     rv = transfer->InitWithBrowsingContext(
         mSourceUrl, pseudoTarget, u""_ns, mMimeInfo, mTimeDownloadStarted,
-        nullptr, this, channel && NS_UsePrivateBrowsing(channel),
+        mTempFile, this, channel && NS_UsePrivateBrowsing(channel),
         mDownloadClassification, mBrowsingContext, mHandleInternally);
   } else {
     rv = transfer->Init(mSourceUrl, pseudoTarget, u""_ns, mMimeInfo,
-                        mTimeDownloadStarted, nullptr, this,
+                        mTimeDownloadStarted, mTempFile, this,
                         channel && NS_UsePrivateBrowsing(channel),
                         mDownloadClassification);
   }