Bug 1685737 - Older blocked mixed-content downloads get removed from download-list on restart. r=Gijs

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

toolkit/components/downloads/DownloadStore.jsm

@@ -28,6 +28,10 @@
 
 "use strict";
 
+// Time after which insecure downloads that have not been dealt with on shutdown
+// get removed (5 minutes).
+const MAX_INSECURE_DOWNLOAD_AGE_MS = 5 * 60 * 1000;
+
 var EXPORTED_SYMBOLS = ["DownloadStore"];
 
 const { XPCOMUtils } = ChromeUtils.import(
@@ -99,12 +103,30 @@ DownloadStore.prototype = {
         return;
       }
 
+      // Set this to true when we make changes to the download list that should
+      // be reflected in the file again.
+      let storeChanges = false;
+      let removePromises = [];
       let storeData = JSON.parse(gTextDecoder.decode(bytes));
 
       // Create live downloads based on the static snapshot.
       for (let downloadData of storeData.list) {
         try {
           let download = await Downloads.createDownload(downloadData);
+
+          // Insecure downloads that have not been dealt with on shutdown should
+          // get cleaned up and removed from the download list on restart unless
+          // they are very new
+          if (
+            download.error?.becauseBlockedByReputationCheck &&
+            download.error.reputationCheckVerdict == "Insecure" &&
+            Date.now() - download.startTime > MAX_INSECURE_DOWNLOAD_AGE_MS
+          ) {
+            removePromises.push(download.removePartialData());
+            storeChanges = true;
+            continue;
+          }
+
           try {
             if (!download.succeeded && !download.canceled && !download.error) {
               // Try to restart the download if it was in progress during the
@@ -126,6 +148,15 @@ DownloadStore.prototype = {
           Cu.reportError(ex);
         }
       }
+
+      if (storeChanges) {
+        try {
+          await Promise.all(removePromises);
+          await this.save();
+        } catch (ex) {
+          Cu.reportError(ex);
+        }
+      }
     })();
   },
 

toolkit/components/downloads/test/unit/test_DownloadStore.js

@@ -16,6 +16,11 @@ ChromeUtils.defineModuleGetter(
   "DownloadStore",
   "resource://gre/modules/DownloadStore.jsm"
 );
+ChromeUtils.defineModuleGetter(
+  this,
+  "DownloadError",
+  "resource://gre/modules/DownloadCore.jsm"
+);
 
 /**
  * Returns a new DownloadList object with an associated DownloadStore.
@@ -388,3 +393,72 @@ add_task(async function test_save_reload_unknownProperties() {
     "download3saver2"
   );
 });
+
+/**
+ * Saves insecure downloads to a file, then reloads the file and checks if they
+ * are still there.
+ */
+add_task(async function test_insecure_download_deletion() {
+  let [listForSave, storeForSave] = await promiseNewListAndStore();
+  let [listForLoad, storeForLoad] = await promiseNewListAndStore(
+    storeForSave.path
+  );
+  let referrerInfo = new ReferrerInfo(
+    Ci.nsIReferrerInfo.EMPTY,
+    true,
+    NetUtil.newURI(TEST_REFERRER_URL)
+  );
+
+  const createTestDownload = async startTime => {
+    // Create a valid test download and start it so it creates a file
+    let targetFile = getTempFile(TEST_TARGET_FILE_NAME);
+    let download = await Downloads.createDownload({
+      source: { url: httpUrl("empty.txt"), referrerInfo },
+      target: targetFile.path,
+      startTime: new Date().toISOString(),
+      contentType: "application/zip",
+    });
+    await download.start();
+
+    // Add an "Insecure Download" error and overwrite the start time for
+    // serialization
+    download.hasBlockedData = true;
+    download.error = DownloadError.fromSerializable({
+      becauseBlockedByReputationCheck: true,
+      reputationCheckVerdict: "Insecure",
+    });
+    download.startTime = startTime;
+
+    let targetPath = download.target.path;
+
+    // Add download to store, save, load and retrieve deserialized download list
+    listForSave.add(download);
+    await storeForSave.save();
+    await storeForLoad.load();
+    let loadedDownloadList = await listForLoad.getAll();
+
+    return [loadedDownloadList, targetPath];
+  };
+
+  // Insecure downloads that are older than 5 minutes should get removed from
+  // the download-store and the file should get deleted. (360000 = 6 minutes)
+  let [loadedDownloadList1, targetPath1] = await createTestDownload(
+    new Date(Date.now() - 360000)
+  );
+
+  Assert.equal(loadedDownloadList1.length, 0, "Download should be removed");
+  Assert.ok(
+    !(await OS.File.exists(targetPath1)),
+    "The file should have been deleted."
+  );
+
+  // Insecure downloads that are newer than 5 minutes should stay in the
+  // download store and the file should remain.
+  let [loadedDownloadList2, targetPath2] = await createTestDownload(new Date());
+
+  Assert.equal(loadedDownloadList2.length, 1, "Download should be kept");
+  Assert.ok(
+    await OS.File.exists(targetPath2),
+    "The file should have not been deleted."
+  );
+});

uriloader/exthandler/nsExternalHelperAppService.cpp

@@ -2388,6 +2388,10 @@ nsresult nsExternalAppHandler::CreateFailedTransfer() {
   // We won't pass the temp file to the transfer, so if we have one it needs to
   // get deleted now.
   if (mTempFile) {
+    if (mSaver) {
+      mSaver->Finish(NS_BINDING_ABORTED);
+      mSaver = nullptr;
+    }
     mTempFile->Remove(false);
   }