Bug 1885860. r=Gijs

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

dom/tests/browser/browser.toml

@@ -137,6 +137,13 @@ support-files = [
 ["browser_persist_cross_origin_iframe.js"]
 support-files = ["image.html"]
 
+["browser_persist_filename.js"]
+support-files = [
+  "file_browserPersist_filename.html",
+  "file_browserPersist_filename.css",
+  "file_browserPersist_a_very_long_filename_used_for_truncation_test.css",
+]
+
 ["browser_persist_image_accept.js"]
 
 ["browser_persist_mixed_content_image.js"]

dom/tests/browser/browser_persist_cross_origin_iframe.js

@@ -49,6 +49,12 @@ function canonicalizeExtension(str) {
   return str.replace(/\.htm$/, ".html");
 }
 
+// Strips the _XXXX seed (4 base64url chars) that nsWebBrowserPersist inserts
+// before the file extension of saved subresources.
+function stripSeedFromComponent(component) {
+  return component.replace(/_[A-Za-z0-9_-]{4}(_data|\.[^.]+)$/, "$1");
+}
+
 function checkContents(dir, expected, str) {
   let stack = [dir];
   let files = [];
@@ -58,7 +64,10 @@ function checkContents(dir, expected, str) {
         stack.push(file);
       }
 
-      let path = canonicalizeExtension(file.getRelativePath(dir));
+      let path = canonicalizeExtension(file.getRelativePath(dir))
+        .split("/")
+        .map(stripSeedFromComponent)
+        .join("/");
       files.push(path);
     }
   }

dom/tests/browser/browser_persist_filename.js

@@ -0,0 +1,142 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+const TEST_PATH = getRootDirectory(gTestPath).replace(
+  "chrome://mochitests/content",
+  "https://example.com"
+);
+
+// Matches the _XXXX seed (4 base64url chars) that nsWebBrowserPersist appends
+// to subresource filenames, immediately before any file extension.
+const SEED_RE = /_[A-Za-z0-9_-]{4}(?=\.[^.]+$)/;
+
+async function savePageToDir(url, name) {
+  return BrowserTestUtils.withNewTab(url, async function (browser) {
+    let doc = await new Promise(resolve => {
+      browser.frameLoader.startPersistence(null, {
+        onDocumentReady: resolve,
+        onError(e) {
+          ok(false, "startPersistence failed: " + e);
+        },
+      });
+    });
+
+    let browserPersist = Cc[
+      "@mozilla.org/embedding/browser/nsWebBrowserPersist;1"
+    ].createInstance(Ci.nsIWebBrowserPersist);
+
+    // saveDocument() requires nsIFile; derive paths from the nsIFile objects.
+    let tmp = Services.dirsvc.get("TmpD", Ci.nsIFile);
+    let savedFile = tmp.clone();
+    savedFile.append(name + ".html");
+    let savedDir = tmp.clone();
+    savedDir.append(name + "_files");
+
+    registerCleanupFunction(async () => {
+      await IOUtils.remove(savedFile.path, { ignoreAbsent: true });
+      await IOUtils.remove(savedDir.path, {
+        ignoreAbsent: true,
+        recursive: true,
+      });
+    });
+
+    await new Promise(resolve => {
+      browserPersist.progressListener = {
+        onProgressChange() {},
+        onLocationChange() {},
+        onStatusChange() {},
+        onSecurityChange() {},
+        onContentBlockingEvent() {},
+        onStateChange(_persist, _req, state) {
+          const done =
+            Ci.nsIWebProgressListener.STATE_STOP |
+            Ci.nsIWebProgressListener.STATE_IS_NETWORK;
+          if ((state & done) === done) {
+            resolve();
+          }
+        },
+      };
+      browserPersist.saveDocument(doc, savedFile, savedDir, null, 0, 0);
+    });
+
+    let children = await IOUtils.getChildren(savedDir.path, {
+      ignoreAbsent: true,
+    });
+    return children.map(p => PathUtils.filename(p));
+  });
+}
+
+// All subresources saved by one nsWebBrowserPersist instance must have a seed
+// of the same value (it is generated once in the constructor).
+add_task(async function test_seed_format_and_consistency() {
+  let files = await savePageToDir(
+    TEST_PATH + "file_browserPersist_filename.html",
+    "browserPersist_seed_consistency"
+  );
+
+  Assert.greaterOrEqual(
+    files.length,
+    2,
+    "At least two subresources were saved"
+  );
+
+  for (let name of files) {
+    Assert.ok(SEED_RE.test(name), `${name} has a seed suffix`);
+  }
+
+  let seeds = new Set(files.map(name => (name.match(SEED_RE) || [""])[0]));
+  Assert.equal(
+    seeds.size,
+    1,
+    `All saved subresources share the same seed, got: ${Array.from(seeds)}`
+  );
+});
+
+// When the filename exceeds kDefaultMaxFilenameLength the base is truncated,
+// but the seed must still be present in the saved filename.
+add_task(async function test_long_filename_truncated_with_seed() {
+  let files = await savePageToDir(
+    TEST_PATH + "file_browserPersist_filename.html",
+    "browserPersist_truncation"
+  );
+
+  let longFile = files.find(name =>
+    name.startsWith("file_browserPersist_a_very_long")
+  );
+  Assert.ok(longFile, "Long-named CSS file was saved");
+  Assert.lessOrEqual(
+    longFile.length,
+    64,
+    `Saved filename length ${longFile.length} is at most 64 chars`
+  );
+  Assert.ok(
+    SEED_RE.test(longFile),
+    `Truncated filename still has a seed: ${longFile}`
+  );
+});
+
+// When two subresource URLs produce the same base filename, the second one gets
+// a uniqueness counter (_002, _003, …) inserted before the seed.
+add_task(async function test_uniqueness_counter_with_seed() {
+  let files = await savePageToDir(
+    TEST_PATH + "file_browserPersist_filename.html",
+    "browserPersist_uniqueness"
+  );
+
+  let dummyFiles = files.filter(name => name.startsWith("dummy"));
+  Assert.equal(
+    dummyFiles.length,
+    2,
+    "Both dummy.png references were saved separately"
+  );
+
+  let seeds = dummyFiles.map(name => (name.match(SEED_RE) || [""])[0]);
+  Assert.equal(seeds[0], seeds[1], "Both files share the same seed");
+
+  Assert.ok(
+    dummyFiles.some(name => /_\d{3}_[A-Za-z0-9_-]{4}\./.test(name)),
+    "One dummy file has a uniqueness counter before the seed"
+  );
+});

dom/tests/browser/file_browserPersist_a_very_long_filename_used_for_truncation_test.css

@@ -0,0 +1,3 @@
+body {
+  margin: 0;
+}

dom/tests/browser/file_browserPersist_filename.css

@@ -0,0 +1,3 @@
+body {
+  color: black;
+}

dom/tests/browser/file_browserPersist_filename.html

@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <link rel="stylesheet" href="file_browserPersist_filename.css">
+  <link rel="stylesheet" href="file_browserPersist_a_very_long_filename_used_for_truncation_test.css">
+</head>
+<body>
+  <!-- Two img tags with different query strings produce the same base filename,
+       exercising the uniqueness counter logic in CalculateUniqueFilename. -->
+  <img src="dummy.png">
+  <img src="dummy.png?v=2">
+</body>
+</html>

dom/webbrowserpersist/nsWebBrowserPersist.cpp

@@ -8,8 +8,10 @@
 
 #include "ReferrerInfo.h"
 #include "WebBrowserPersistLocalDocument.h"
+#include "mozilla/Base64.h"
 #include "mozilla/Mutex.h"
 #include "mozilla/Printf.h"
+#include "mozilla/RandomNum.h"
 #include "mozilla/TextUtils.h"
 #include "mozilla/WebBrowserPersistDocumentParent.h"
 #include "mozilla/dom/BrowserParent.h"
@@ -278,6 +280,36 @@ const uint32_t kDefaultPersistFlags =
 const char* kWebBrowserPersistStringBundle =
     "chrome://global/locale/nsWebBrowserPersist.properties";
 
+namespace {
+
+nsCString GenerateRandomSeed() {
+  constexpr size_t SEED_LEN = 3;
+  uint8_t buffer[SEED_LEN];
+  if (!mozilla::GenerateRandomBytesFromOS(buffer, SEED_LEN)) {
+    for (uint8_t& entry : buffer) {
+      Maybe<uint64_t> maybeSeed = mozilla::RandomUint64();
+      if (maybeSeed.isNothing()) {
+        return EmptyCString();
+      }
+      entry = static_cast<uint8_t>(maybeSeed.value());
+    }
+  }
+
+  nsAutoCString seed;
+  nsresult rv = Base64URLEncode(SEED_LEN, buffer,
+                                Base64URLEncodePaddingPolicy::Omit, seed);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return EmptyCString();
+  }
+
+  // 3 bytes produce exactly 4 base64url chars (no padding needed); keep all
+  // 4 so the seed is distinguishable from the 3-digit uniqueness counter.
+  seed.Insert('_', 0);
+  return seed;
+}
+
+}  // namespace
+
 nsWebBrowserPersist::nsWebBrowserPersist()
     : mCurrentDataPathIsRelative(false),
       mCurrentThingsToPersist(0),
@@ -296,7 +328,12 @@ nsWebBrowserPersist::nsWebBrowserPersist()
       mTotalCurrentProgress(0),
       mTotalMaxProgress(0),
       mWrapColumn(72),
-      mEncodingFlags(0) {}
+      mEncodingFlags(0),
+      mFilenameRandomSeed(GenerateRandomSeed()) {
+  MOZ_ASSERT(
+      !mFilenameRandomSeed.IsEmpty(),
+      "Failed to generate random seed; saved filenames will be predictable");
+}
 
 nsWebBrowserPersist::~nsWebBrowserPersist() { Cleanup(); }
 
@@ -1921,17 +1958,34 @@ nsresult nsWebBrowserPersist::CalculateUniqueFilename(
   nsCOMPtr<nsIURL> url(do_QueryInterface(aURI));
   NS_ENSURE_TRUE(url, NS_ERROR_FAILURE);
 
-  bool nameHasChanged = false;
-  nsresult rv;
-
   // Get the old filename
   nsAutoCString filename;
-  rv = url->GetFileName(filename);
+  nsresult rv = url->GetFileName(filename);
   NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
   nsAutoCString directory;
   rv = url->GetDirectory(directory);
   NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
 
+  // URL-decode and sanitize the filename to prevent dangerous characters
+  // (path separators, control characters, bidi marks, etc.).
+  NS_UnescapeURL(filename);
+  if (!mMIMEService) {
+    mMIMEService = do_GetService(NS_MIMESERVICE_CONTRACTID, &rv);
+  }
+  if (mMIMEService) {
+    nsAutoString filenameU16;
+    CopyUTF8toUTF16(filename, filenameU16);
+    nsAutoString sanitized;
+    if (NS_SUCCEEDED(mMIMEService->ValidateFileNameForSaving(
+            filenameU16, EmptyCString(),
+            nsIMIMEService::VALIDATE_SANITIZE_ONLY |
+                nsIMIMEService::VALIDATE_DONT_TRUNCATE |
+                nsIMIMEService::VALIDATE_NO_DEFAULT_FILENAME,
+            sanitized))) {
+      CopyUTF16toUTF8(sanitized, filename);
+    }
+  }
+
   // Split the filename into a base and an extension.
   // e.g. "foo.html" becomes "foo" & ".html"
   //
@@ -1949,6 +2003,11 @@ nsresult nsWebBrowserPersist::CalculateUniqueFilename(
     base = filename;
   }
 
+  // Add random seed suffix to file names.
+  filename.Assign(base);
+  filename.Append(mFilenameRandomSeed);
+  filename.Append(ext);
+
   // Test if the filename is longer than allowed by the OS
   int32_t needToChop = filename.Length() - kDefaultMaxFilenameLength;
   if (needToChop > 0) {
@@ -1969,8 +2028,8 @@ nsresult nsWebBrowserPersist::CalculateUniqueFilename(
     }
 
     filename.Assign(base);
+    filename.Append(mFilenameRandomSeed);
     filename.Append(ext);
-    nameHasChanged = true;
   }
 
   // Ensure the filename is unique
@@ -1989,6 +2048,9 @@ nsresult nsWebBrowserPersist::CalculateUniqueFilename(
       if (base.IsEmpty() || duplicateCounter > 1) {
         SmprintfPointer tmp = mozilla::Smprintf("_%03d", duplicateCounter);
         NS_ENSURE_TRUE(tmp, NS_ERROR_OUT_OF_MEMORY);
+        // filename already includes the 5-char seed, so the threshold
+        // kDefaultMaxFilenameLength - 4 correctly leaves room for the
+        // 4-char counter suffix (_001) without exceeding the limit.
         if (filename.Length() < kDefaultMaxFilenameLength - 4) {
           tmpBase = base;
         } else {
@@ -2001,14 +2063,15 @@ nsresult nsWebBrowserPersist::CalculateUniqueFilename(
 
       tmpPath.Assign(directory);
       tmpPath.Append(tmpBase);
+      tmpPath.Append(mFilenameRandomSeed);
       tmpPath.Append(ext);
 
       // Test if the name is a duplicate
       if (!mFilenameList.Contains(tmpPath)) {
         if (!base.Equals(tmpBase)) {
           filename.Assign(tmpBase);
+          filename.Append(mFilenameRandomSeed);
           filename.Append(ext);
-          nameHasChanged = true;
         }
         break;
       }
@@ -2021,37 +2084,31 @@ nsresult nsWebBrowserPersist::CalculateUniqueFilename(
   newFilepath.Append(filename);
   mFilenameList.AppendElement(newFilepath);
 
-  // Update the uri accordingly if the filename actually changed
-  if (nameHasChanged) {
-    // Final sanity test
-    if (filename.Length() > kDefaultMaxFilenameLength) {
-      NS_WARNING(
-          "Filename wasn't truncated less than the max file length - how can "
-          "that be?");
-      return NS_ERROR_FAILURE;
-    }
-
-    nsCOMPtr<nsIFile> localFile;
-    GetLocalFileFromURI(aURI, getter_AddRefs(localFile));
-
-    if (localFile) {
-      nsAutoString filenameAsUnichar;
-      CopyASCIItoUTF16(filename, filenameAsUnichar);
-      localFile->SetLeafName(filenameAsUnichar);
-
-      // Resync the URI with the file after the extension has been appended
-      return NS_MutateURI(aURI)
-          .Apply(&nsIFileURLMutator::SetFile, localFile)
-          .Finalize(aOutURI);
-    }
-    return NS_MutateURI(url)
-        .Apply(&nsIURLMutator::SetFileName, filename, nullptr)
-        .Finalize(aOutURI);
+  // Update the uri accordingly since the filename changed.
+  // Final sanity test
+  if (filename.Length() > kDefaultMaxFilenameLength) {
+    NS_WARNING(
+        "Filename wasn't truncated less than the max file length - how can "
+        "that be?");
+    return NS_ERROR_FAILURE;
   }
 
-  // TODO (:valentin) This method should always clone aURI
-  aOutURI = aURI;
-  return NS_OK;
+  nsCOMPtr<nsIFile> localFile;
+  GetLocalFileFromURI(aURI, getter_AddRefs(localFile));
+
+  if (localFile) {
+    nsAutoString filenameAsUnichar;
+    CopyUTF8toUTF16(filename, filenameAsUnichar);
+    localFile->SetLeafName(filenameAsUnichar);
+
+    // Resync the URI with the file after the extension has been appended
+    return NS_MutateURI(aURI)
+        .Apply(&nsIFileURLMutator::SetFile, localFile)
+        .Finalize(aOutURI);
+  }
+  return NS_MutateURI(url)
+      .Apply(&nsIURLMutator::SetFileName, filename, nullptr)
+      .Finalize(aOutURI);
 }
 
 nsresult nsWebBrowserPersist::MakeFilenameFromURI(nsIURI* aURI,
@@ -2285,6 +2342,72 @@ void nsWebBrowserPersist::EndDownloadInternal(nsresult aResult) {
     CleanupLocalFiles();
   }
 
+  // Since filenames are randomized per save session, re-saving a page to the
+  // same location leaves behind files from the previous save that now have
+  // different names. Remove any entry in the data directory that was not
+  // written in this session.
+  if (NS_SUCCEEDED(aResult) && mCurrentDataPath) {
+    nsCOMPtr<nsIFile> localDataPath;
+    if (NS_SUCCEEDED(GetLocalFileFromURI(mCurrentDataPath,
+                                         getter_AddRefs(localDataPath)))) {
+      bool exists = false;
+      localDataPath->Exists(&exists);
+      nsCOMPtr<nsIURL> dataPathURL(do_QueryInterface(mCurrentDataPath));
+      nsAutoCString dataDir;
+      if (exists && dataPathURL &&
+          NS_SUCCEEDED(dataPathURL->GetFilePath(dataDir))) {
+        if (!dataDir.IsEmpty() && dataDir.Last() != '/') {
+          dataDir.Append('/');
+        }
+        nsCOMPtr<nsIDirectoryEnumerator> entries;
+        if (NS_SUCCEEDED(
+                localDataPath->GetDirectoryEntries(getter_AddRefs(entries)))) {
+          // Collect stale entries synchronously, then delete asynchronously to
+          // avoid blocking the main thread on I/O. Enumeration must be
+          // synchronous so that files created by a concurrent new save (after
+          // this point) are not included in the deletion list.
+          nsTArray<std::pair<nsString, bool>> toDelete;
+          nsCOMPtr<nsIFile> entry;
+          while (NS_SUCCEEDED(entries->GetNextFile(getter_AddRefs(entry))) &&
+                 entry) {
+            nsAutoString leafNameU16;
+            entry->GetLeafName(leafNameU16);
+            nsAutoCString leafName;
+            CopyUTF16toUTF8(leafNameU16, leafName);
+            // entryPath is built from GetFilePath() (URL path, forward
+            // slashes) + nsIFile leaf name (UTF-8), matching the format used
+            // by CalculateUniqueFilename and SaveSubframeContent when they
+            // insert entries into mFilenameList.
+            nsAutoCString entryPath(dataDir);
+            entryPath.Append(leafName);
+            if (!mFilenameList.Contains(entryPath)) {
+              bool isDir = false;
+              entry->IsDirectory(&isDir);
+              nsAutoString path;
+              entry->GetPath(path);
+              toDelete.AppendElement(std::pair{nsString(path), isDir});
+            }
+          }
+          if (!toDelete.IsEmpty()) {
+            NS_DispatchBackgroundTask(
+                NS_NewRunnableFunction(
+                    "nsWebBrowserPersist::CleanupStaleFiles",
+                    [paths = std::move(toDelete)]() {
+                      for (const auto& [path, isDir] : paths) {
+                        nsCOMPtr<nsIFile> file;
+                        if (NS_SUCCEEDED(
+                                NS_NewLocalFile(path, getter_AddRefs(file)))) {
+                          file->Remove(isDir);
+                        }
+                      }
+                    }),
+                NS_DISPATCH_EVENT_MAY_BLOCK);
+          }
+        }
+      }
+    }
+  }
+
   // Cleanup the channels
   Cleanup();
 
@@ -2532,7 +2655,9 @@ nsresult nsWebBrowserPersist::SaveSubframeContent(
   rv = AppendPathToURI(frameURI, filenameWithExt, frameURI);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // Work out the path for the subframe data
+  // Work out the path for the subframe data. The _data directory is a local
+  // organizational folder never fetched from the server, and the files inside
+  // it will be randomized individually, so its name does not need seeding.
   nsCOMPtr<nsIURI> frameDataURI = mCurrentDataPath;
   nsAutoString newFrameDataPath(aData->mFilename);
 
@@ -2541,16 +2666,25 @@ nsresult nsWebBrowserPersist::SaveSubframeContent(
   rv = AppendPathToURI(frameDataURI, newFrameDataPath, frameDataURI);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // Make frame document & data path conformant and unique
+  // Register the data directory in mFilenameList so the stale-file cleanup
+  // in EndDownloadInternal does not remove it after a successful save.
+  nsCOMPtr<nsIURL> dataURL(do_QueryInterface(frameDataURI));
+  if (dataURL) {
+    nsAutoCString directory, filename;
+    if (NS_SUCCEEDED(dataURL->GetDirectory(directory)) &&
+        NS_SUCCEEDED(dataURL->GetFileName(filename))) {
+      NS_UnescapeURL(filename);
+      directory.Append(filename);
+      mFilenameList.AppendElement(directory);
+    }
+  }
+
+  // Make frame document path conformant and unique
   nsCOMPtr<nsIURI> out;
   rv = CalculateUniqueFilename(frameURI, out);
   NS_ENSURE_SUCCESS(rv, rv);
   frameURI = out;
 
-  rv = CalculateUniqueFilename(frameDataURI, out);
-  NS_ENSURE_SUCCESS(rv, rv);
-  frameDataURI = out;
-
   mCurrentThingsToPersist++;
 
   // We shouldn't use SaveDocumentInternal for the contents

dom/webbrowserpersist/nsWebBrowserPersist.h

@@ -182,6 +182,8 @@ class nsWebBrowserPersist final : public nsIInterfaceRequestor,
   int16_t mWrapColumn;
   uint32_t mEncodingFlags;
   nsString mContentType;
+  // Random suffix added to downloaded filenames to make them less predictable.
+  nsCString mFilenameRandomSeed;
 };
 
 #endif

toolkit/components/windowcreator/test/test_bug449141.html

@@ -43,8 +43,14 @@ var progressListener = {
       var videoFile = getTempDir();
       videoFile.append(this.dirName);
       dirExists = videoFile.exists();
-      videoFile.append("320x240.webm");
-      videoExists = videoFile.exists();
+      var entries = videoFile.directoryEntries;
+      while (entries.hasMoreElements()) {
+        var entry = entries.nextFile;
+        if (entry.leafName.startsWith("320x240") && entry.leafName.endsWith(".webm")) {
+          videoExists = true;
+          break;
+        }
+      }
       this.folder.remove(true);
       this.file.remove(false);
       ok(dirExists, "Directory containing video file should be created");

uriloader/exthandler/tests/mochitest/browser_save_filenames.js

@@ -232,6 +232,12 @@ function getDirectoryEntries(dir) {
   return files;
 }
 
+// Strips the random seed (_XXXX, 4 base64url chars) that nsWebBrowserPersist
+// inserts before the file extension of saved subresources.
+function stripSeedFromFilename(filename) {
+  return filename.replace(/_[A-Za-z0-9_-]{4}(?=\.[^.]+$|$)/, "");
+}
+
 // This test saves the document as a complete web page and verifies
 // that the resources are saved with the correct filename.
 add_task(async function save_document() {
@@ -256,20 +262,19 @@ add_task(async function save_document() {
   saveBrowser(browser);
   await savePromise;
 
-  let filesSaved = getDirectoryEntries(tmpDir);
+  let filesSaved = getDirectoryEntries(tmpDir).map(stripSeedFromFilename);
 
   for (let idx = 0; idx < expectedItems.length; idx++) {
     let filename = expectedItems[idx].filename;
 
-    let file = tmpDir.clone();
-    file.append(filename);
-
     let fileIdx = -1;
     // Use checkShortenedFilename to check long filenames.
     if (filename.length > 240) {
       for (let t = 0; t < filesSaved.length; t++) {
+        // After seed stripping a 64-char truncated filename is 59 chars;
+        // use 55 as the lower bound to also tolerate CreateUnique shortening.
         if (
-          filesSaved[t].length > 60 &&
+          filesSaved[t].length > 55 &&
           checkShortenedFilename(filesSaved[t], filename)
         ) {
           fileIdx = t;