Bug 1652520 - do not overwrite extensions for all filetypes, r=mak

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

browser/base/content/test/general/browser_bug676619.js

@@ -48,7 +48,7 @@ async function testLink(link, name) {
   is(
     win.document.getElementById("location").value,
     name,
-    "file name should match"
+    `file name should match (${link})`
   );
 
   await BrowserTestUtils.closeWindow(win);
@@ -91,8 +91,6 @@ async function runTest(url) {
   // htm vs html) on different OSes.
   let oggExtension = getMIMEInfoForType("application/ogg").primaryExtension;
   await testLink("link13", "no file extension." + oggExtension);
-  let htmlExtension = getMIMEInfoForType("text/html").primaryExtension;
-  await testLink("link14", "javascript." + htmlExtension);
 
   BrowserTestUtils.removeTab(tab);
 }

browser/base/content/test/general/download_page.html

@@ -35,8 +35,6 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=676619
                 download="example.com" id="link12" target="_blank">Download "example.com"</a></li>
         <li><a href="video.ogg"
                 download="no file extension" id="link13">Download "force extension"</a></li>
-        <li><a href="javascript:(1+2)+'force extension'"
-            download="javascript.txt" id="link14">Download "javascript.txt" (will correct extension due to mimetype)</a></li>
     </ul>
     <div id="unload-flag">Okay</div>
 

dom/security/test/general/browser_test_data_text_csv.js

@@ -38,12 +38,6 @@ add_task(async function() {
   let win = await windowPromise;
 
   let expectedValue = "text/csv;foo,bar,foobar";
-  let mimeInfo = getMIMEInfoForType("text/csv");
-  try {
-    expectedValue = "." + mimeInfo.primaryExtension;
-  } catch (ex) {
-    /* fails on Windows, bug 1671930 */
-  }
   is(
     win.document.getElementById("location").value,
     expectedValue,

uriloader/exthandler/nsExternalHelperAppService.cpp

@@ -568,6 +568,38 @@ static const nsDefaultMimeTypeEntry nonDecodableExtensions[] = {
     {APPLICATION_COMPRESS, "z"},
     {APPLICATION_GZIP, "svgz"}};
 
+/**
+ * Mimetypes for which we enforce using a known extension.
+ *
+ * In addition to this list, we do this for all audio/, video/ and
+ * image/ mimetypes.
+ */
+static const char* forcedExtensionMimetypes[] = {
+    // OpenDocument formats
+    "application/vnd.oasis.opendocument.text",
+    "application/vnd.oasis.opendocument.presentation",
+    "application/vnd.oasis.opendocument.spreadsheet",
+    "application/vnd.oasis.opendocument.graphics",
+
+    // Legacy Microsoft Office
+    "application/msword", "application/vnd.ms-powerpoint",
+    "application/vnd.ms-excel",
+
+    // Office Open XML
+    "application/vnd.openxmlformats-officedocument.wordprocessingml.document",
+    "application/vnd.openxmlformats-officedocument.presentationml.presentation",
+    "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet",
+
+    APPLICATION_PDF,
+
+    APPLICATION_OGG,
+
+    APPLICATION_ZIP,
+
+    APPLICATION_JSON, APPLICATION_WASM,
+
+    TEXT_CALENDAR, TEXT_CSS, TEXT_VCARD, TEXT_XML};
+
 /**
  * Primary extensions of types whose descriptions should be overwritten.
  * This extension is concatenated with "ExtHandlerDescription" to look up the
@@ -1275,25 +1307,68 @@ nsExternalAppHandler::nsExternalAppHandler(
   mSuggestedFileName.CompressWhitespace();
   mTempFileExtension.CompressWhitespace();
 
-  // After removing trailing whitespaces from the name, if we have a
-  // temp file extension, replace the file extension with it if:
-  // - there is no extant file extension (or it only consists of ".")
-  // - the extant file extension contains invalid characters, or
-  // - the extant file extension is not known by the mimetype.
+  EnsureCorrectExtension(originalFileExt);
+
+  mBufferSize = Preferences::GetUint("network.buffer.cache.size", 4096);
+}
+
+nsExternalAppHandler::~nsExternalAppHandler() {
+  MOZ_ASSERT(!mSaver, "Saver should hold a reference to us until deleted");
+}
+
+bool nsExternalAppHandler::ShouldForceExtension(const nsString& aFileExt) {
+  nsAutoCString MIMEType;
+  if (!mMimeInfo || NS_FAILED(mMimeInfo->GetMIMEType(MIMEType))) {
+    return false;
+  }
+
+  bool canForce = StringBeginsWith(MIMEType, "image/"_ns) ||
+                  StringBeginsWith(MIMEType, "audio/"_ns) ||
+                  StringBeginsWith(MIMEType, "video/"_ns);
+
+  if (!canForce) {
+    for (const char* mime : forcedExtensionMimetypes) {
+      if (MIMEType.Equals(mime)) {
+        canForce = true;
+        break;
+      }
+    }
+    if (!canForce) {
+      return false;
+    }
+  }
+  // If we get here, we know for sure the mimetype allows us to overwrite the
+  // existing extension, if it's wrong. Return whether the extension is wrong:
+
   bool knownExtension = false;
-  // Note that originalFileExt is either empty or consists of an
-  // extension *including the dot* which we need to remove:
-  bool haveBogusExtension =
-      mMimeInfo && !originalFileExt.IsEmpty() &&
-      NS_SUCCEEDED(mMimeInfo->ExtensionExists(
-          Substring(NS_ConvertUTF16toUTF8(originalFileExt), 1),
-          &knownExtension)) &&
-      !knownExtension;
-  if (!mTempFileExtension.IsEmpty() &&
-      (originalFileExt.Length() == 0 || originalFileExt.EqualsLiteral(".") ||
-       originalFileExt.FindCharInSet(
-           KNOWN_PATH_SEPARATORS FILE_ILLEGAL_CHARACTERS) != kNotFound ||
-       haveBogusExtension)) {
+  // Note that aFileExt is either empty or consists of an extension
+  // *including the dot* which we remove for ExtensionExists().
+  return (
+      aFileExt.IsEmpty() || aFileExt.EqualsLiteral(".") ||
+      (NS_SUCCEEDED(mMimeInfo->ExtensionExists(
+           Substring(NS_ConvertUTF16toUTF8(aFileExt), 1), &knownExtension)) &&
+       !knownExtension));
+}
+
+void nsExternalAppHandler::EnsureCorrectExtension(const nsString& aFileExt) {
+  // If we don't have an extension (which will include the .),
+  // just short-circuit.
+  if (mTempFileExtension.Length() <= 1) {
+    return;
+  }
+
+  // After removing trailing whitespaces from the name, if we have a
+  // temp file extension, there are broadly 2 cases where we want to
+  // replace the extension.
+  // First, if the file extension contains invalid characters.
+  // Second, for document type mimetypes, if the extension is either
+  // missing or not valid for this mimetype.
+  bool replaceExtension =
+      (aFileExt.FindCharInSet(KNOWN_PATH_SEPARATORS FILE_ILLEGAL_CHARACTERS) !=
+       kNotFound) ||
+      ShouldForceExtension(aFileExt);
+
+  if (replaceExtension) {
     int32_t pos = mSuggestedFileName.RFindChar('.');
     if (pos != kNotFound) {
       mSuggestedFileName =
@@ -1301,18 +1376,16 @@ nsExternalAppHandler::nsExternalAppHandler(
     } else {
       mSuggestedFileName.Append(mTempFileExtension);
     }
-    originalFileExt = mTempFileExtension;
   }
 
-  // Make sure later we won't append mTempFileExtension if it's identical to
-  // the currently used file extension.
-  EnsureTempFileExtension(originalFileExt);
-
-  mBufferSize = Preferences::GetUint("network.buffer.cache.size", 4096);
-}
-
-nsExternalAppHandler::~nsExternalAppHandler() {
-  MOZ_ASSERT(!mSaver, "Saver should hold a reference to us until deleted");
+  /*
+   * Ensure we don't double-append the file extension if it matches:
+   */
+  if (replaceExtension ||
+      aFileExt.Equals(mTempFileExtension, nsCaseInsensitiveStringComparator)) {
+    // Matches -> mTempFileExtension can be empty
+    mTempFileExtension.Truncate();
+  }
 }
 
 void nsExternalAppHandler::DidDivertRequest(nsIRequest* request) {
@@ -1389,33 +1462,6 @@ void nsExternalAppHandler::RetargetLoadNotifications(nsIRequest* request) {
   }
 }
 
-/**
- * Make mTempFileExtension contain an extension exactly when its previous value
- * is different from mSuggestedFileName's extension, so that it can be appended
- * to mSuggestedFileName and form a valid, useful leaf name.
- * This is required so that the (renamed) temporary file has the correct
- * extension after downloading to make sure the OS will launch the application
- * corresponding to the MIME type (which was used to calculate
- * mTempFileExtension).  This prevents a cgi-script named foobar.exe that
- * returns application/zip from being named foobar.exe and executed as an
- * executable file. It also blocks content that a web site might provide with a
- * content-disposition header indicating filename="foobar.exe" from being
- * downloaded to a file with extension .exe and executed.
- */
-void nsExternalAppHandler::EnsureTempFileExtension(const nsString& aFileExt) {
-  // Make sure there is a mTempFileExtension (not "" or ".").
-  // Remember that mTempFileExtension will always have the leading "."
-  // (the check for empty is just to be safe).
-  if (mTempFileExtension.Length() > 1) {
-    // Now, compare fileExt to mTempFileExtension.
-    if (aFileExt.Equals(mTempFileExtension,
-                        nsCaseInsensitiveStringComparator)) {
-      // Matches -> mTempFileExtension can be empty
-      mTempFileExtension.Truncate();
-    }
-  }
-}
-
 nsresult nsExternalAppHandler::SetUpTempFile(nsIChannel* aChannel) {
   // First we need to try to get the destination directory for the temporary
   // file.

uriloader/exthandler/nsExternalHelperAppService.h

@@ -461,10 +461,22 @@ class nsExternalAppHandler final : public nsIStreamListener,
   bool GetNeverAskFlagFromPref(const char* prefName, const char* aContentType);
 
   /**
-   * Helper routine to ensure that mTempFileExtension only contains an extension
+   * Helper routine that checks whether we should enforce an extension
+   * for this file.
+   */
+  bool ShouldForceExtension(const nsString& aFileExt);
+
+  /**
+   * Helper routine to ensure that mSuggestedFileName ends in the correct
+   * extension, in case the original extension contains invalid characters
+   * or if this download is for a mimetype where we enforce using a specific
+   * extension (image/, video/, and audio/ based mimetypes, and a few specific
+   * document types).
+   *
+   * It also ensure that mTempFileExtension only contains an extension
    * when it is different from mSuggestedFileName's extension.
    */
-  void EnsureTempFileExtension(const nsString& aFileExt);
+  void EnsureCorrectExtension(const nsString& aFileExt);
 
   typedef enum { kReadError, kWriteError, kLaunchError } ErrorType;
   /**

uriloader/exthandler/tests/mochitest/browser.ini

@@ -31,6 +31,7 @@ support-files =
   file_with@@funny_name.png^headers^
   file_with[funny_name.webm
   file_with[funny_name.webm^headers^
+[browser_extension_correction.js]
 [browser_open_internal_choice_persistence.js]
 support-files =
   file_pdf_application_pdf.pdf

uriloader/exthandler/tests/mochitest/browser_extension_correction.js

@@ -0,0 +1,51 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+async function testLinkWithoutExtension(type, expectExtension) {
+  info("Checking " + type);
+
+  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();
+  });
+
+  let win = await winPromise;
+
+  let actualName = win.document.getElementById("location").value;
+  let expectedName = "somefile";
+  if (expectExtension) {
+    expectedName += "." + getMIMEInfoForType(type).primaryExtension;
+    is(actualName, expectedName, `${type} should get an extension`);
+  } else {
+    is(actualName, expectedName, `${type} should not get an extension`);
+  }
+
+  await BrowserTestUtils.closeWindow(win);
+}
+
+/**
+ * Check that for document types, images, videos and audio files,
+ * we enforce a useful extension.
+ */
+add_task(async function test_enforce_useful_extension() {
+  await BrowserTestUtils.withNewTab("data:text/html,", async browser => {
+    await testLinkWithoutExtension("image/png", true);
+    await testLinkWithoutExtension("audio/ogg", true);
+    await testLinkWithoutExtension("video/webm", true);
+    await testLinkWithoutExtension("application/msword", true);
+    await testLinkWithoutExtension("application/pdf", true);
+
+    await testLinkWithoutExtension("application/x-gobbledygook", false);
+    await testLinkWithoutExtension("application/octet-stream", false);
+    await testLinkWithoutExtension("binary/octet-stream", false);
+    await testLinkWithoutExtension("application/x-msdownload", false);
+  });
+});