Bug 1598216 - Change file extensions with invalid characters to valid file extensions on save. r=Gijs
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 09 Mar 2020 16:11:13 +0000
changeset 517761 45f786f133e9d35d2a4f429af23e5565bc55b42c
parent 517760 99d6961f2d92932a5f48d5f12489a0fda60d0e73
child 517762 bb07f7e084a1b8066ab1c5700dae3e8f1d7c1494
push id109601
push usermak77@bonardo.net
push dateTue, 10 Mar 2020 08:51:23 +0000
treeherderautoland@45f786f133e9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1598216
milestone76.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1598216 - Change file extensions with invalid characters to valid file extensions on save. r=Gijs Differential Revision: https://phabricator.services.mozilla.com/D65823
toolkit/components/downloads/DownloadPaths.jsm
toolkit/components/downloads/test/unit/test_DownloadPaths.js
uriloader/exthandler/nsExternalHelperAppService.cpp
uriloader/exthandler/nsExternalHelperAppService.h
uriloader/exthandler/tests/mochitest/HelperAppLauncherDialog_chromeScript.js
uriloader/exthandler/tests/mochitest/invalidCharFileExtension.sjs
uriloader/exthandler/tests/mochitest/mochitest.ini
uriloader/exthandler/tests/mochitest/test_invalidCharFileExtension.xhtml
uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml
uriloader/exthandler/tests/mochitest/unsafeBidi_chromeScript.js
--- a/toolkit/components/downloads/DownloadPaths.jsm
+++ b/toolkit/components/downloads/DownloadPaths.jsm
@@ -74,16 +74,17 @@ var DownloadPaths = {
         .replace(/</g, "(")
         .replace(/>/g, ")")
         .replace(/"/g, "'");
     }
     return leafName
       .replace(/[\\/]+/g, "_")
       .replace(/[\u200e\u200f\u202a-\u202e]/g, "")
       .replace(gConvertToSpaceRegExp, " ")
+      .replace(/\s{2,}/g, " ")
       .replace(/^[\s\u180e.]+|[\s\u180e.]+$/g, "");
   },
 
   /**
    * Creates a uniquely-named file starting from the name of the provided file.
    * If a file with the provided name already exists, the function attempts to
    * create nice alternatives, like "base(1).ext" (instead of "base-1.ext").
    *
--- a/toolkit/components/downloads/test/unit/test_DownloadPaths.js
+++ b/toolkit/components/downloads/test/unit/test_DownloadPaths.js
@@ -32,36 +32,36 @@ function testCreateNiceUniqueFile(aTempF
 
 add_task(async function test_sanitize() {
   // Platform-dependent conversion of special characters to spaces.
   const kSpecialChars = 'A:*?|""<<>>;,+=[]B][=+,;>><<""|?*:C';
   if (AppConstants.platform == "android") {
     testSanitize(kSpecialChars, "A B C");
     testSanitize(" :: Website :: ", "Website");
     testSanitize("* Website!", "Website!");
-    testSanitize("Website | Page!", "Website   Page!");
-    testSanitize("Directory Listing: /a/b/", "Directory Listing  _a_b_");
+    testSanitize("Website | Page!", "Website Page!");
+    testSanitize("Directory Listing: /a/b/", "Directory Listing _a_b_");
   } else if (AppConstants.platform == "win") {
     testSanitize(kSpecialChars, "A ''(());,+=[]B][=+,;))(('' C");
     testSanitize(" :: Website :: ", "Website");
     testSanitize("* Website!", "Website!");
-    testSanitize("Website | Page!", "Website   Page!");
-    testSanitize("Directory Listing: /a/b/", "Directory Listing  _a_b_");
+    testSanitize("Website | Page!", "Website Page!");
+    testSanitize("Directory Listing: /a/b/", "Directory Listing _a_b_");
   } else if (AppConstants.platform == "macosx") {
     testSanitize(kSpecialChars, 'A *?|""<<>>;,+=[]B][=+,;>><<""|?* C');
     testSanitize(" :: Website :: ", "Website");
     testSanitize("* Website!", "* Website!");
     testSanitize("Website | Page!", "Website | Page!");
-    testSanitize("Directory Listing: /a/b/", "Directory Listing  _a_b_");
+    testSanitize("Directory Listing: /a/b/", "Directory Listing _a_b_");
   } else {
     testSanitize(kSpecialChars, kSpecialChars.replace(/[:]/g, " "));
     testSanitize(" :: Website :: ", "Website");
     testSanitize("* Website!", "* Website!");
     testSanitize("Website | Page!", "Website | Page!");
-    testSanitize("Directory Listing: /a/b/", "Directory Listing  _a_b_");
+    testSanitize("Directory Listing: /a/b/", "Directory Listing _a_b_");
   }
 
   // Conversion of consecutive runs of slashes and backslashes to underscores.
   testSanitize("\\ \\\\Website\\/Page// /", "_ _Website_Page_ _");
 
   // Removal of leading and trailing whitespace and dots after conversion.
   testSanitize("  Website  ", "Website");
   testSanitize(". . Website . Page . .", "Website . Page");
--- a/uriloader/exthandler/nsExternalHelperAppService.cpp
+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
@@ -1173,22 +1173,32 @@ nsExternalAppHandler::nsExternalAppHandl
       mTransfer(nullptr),
       mRequest(nullptr),
       mExtProtSvc(aExtProtSvc) {
   // make sure the extention includes the '.'
   if (!aTempFileExtension.IsEmpty() && aTempFileExtension.First() != '.')
     mTempFileExtension = char16_t('.');
   AppendUTF8toUTF16(aTempFileExtension, mTempFileExtension);
 
+  // Get mSuggestedFileName's current file extension.
+  nsAutoString originalFileExt;
+  int32_t pos = mSuggestedFileName.RFindChar('.');
+  if (pos != kNotFound) {
+    mSuggestedFileName.Right(originalFileExt,
+                             mSuggestedFileName.Length() - pos);
+  }
+
   // replace platform specific path separator and illegal characters to avoid
-  // any confusion
-  mSuggestedFileName.ReplaceChar(KNOWN_PATH_SEPARATORS FILE_ILLEGAL_CHARACTERS,
-                                 '_');
-  mTempFileExtension.ReplaceChar(KNOWN_PATH_SEPARATORS FILE_ILLEGAL_CHARACTERS,
-                                 '_');
+  // any confusion.
+  // Try to keep the use of spaces or underscores in sync with the Downloads
+  // code sanitization in DownloadPaths.jsm
+  mSuggestedFileName.ReplaceChar(KNOWN_PATH_SEPARATORS, '_');
+  mSuggestedFileName.ReplaceChar(FILE_ILLEGAL_CHARACTERS, ' ');
+  mTempFileExtension.ReplaceChar(KNOWN_PATH_SEPARATORS, '_');
+  mTempFileExtension.ReplaceChar(FILE_ILLEGAL_CHARACTERS, ' ');
 
   // Remove unsafe bidi characters which might have spoofing implications (bug
   // 511521).
   const char16_t unsafeBidiCharacters[] = {
       char16_t(0x061c),  // Arabic Letter Mark
       char16_t(0x200e),  // Left-to-Right Mark
       char16_t(0x200f),  // Right-to-Left Mark
       char16_t(0x202a),  // Left-to-Right Embedding
@@ -1199,18 +1209,33 @@ nsExternalAppHandler::nsExternalAppHandl
       char16_t(0x2066),  // Left-to-Right Isolate
       char16_t(0x2067),  // Right-to-Left Isolate
       char16_t(0x2068),  // First Strong Isolate
       char16_t(0x2069),  // Pop Directional Isolate
       char16_t(0)};
   mSuggestedFileName.ReplaceChar(unsafeBidiCharacters, '_');
   mTempFileExtension.ReplaceChar(unsafeBidiCharacters, '_');
 
-  // Make sure extension is correct.
-  EnsureSuggestedFileName();
+  // Remove trailing or leading spaces that we may have generated while
+  // sanitizing.
+  mSuggestedFileName.CompressWhitespace();
+  mTempFileExtension.CompressWhitespace();
+
+  // Append after removing trailing whitespaces from the name.
+  if (originalFileExt.FindCharInSet(
+          KNOWN_PATH_SEPARATORS FILE_ILLEGAL_CHARACTERS) != kNotFound) {
+    // The file extension contains invalid characters and using it would
+    // generate an unusable file, thus use mTempFileExtension instead.
+    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");
 }
 
@@ -1290,30 +1315,24 @@ void nsExternalAppHandler::RetargetLoadN
  * 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::EnsureSuggestedFileName() {
+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) {
-    // Get mSuggestedFileName's current extension.
-    nsAutoString fileExt;
-    int32_t pos = mSuggestedFileName.RFindChar('.');
-    if (pos != kNotFound)
-      mSuggestedFileName.Right(fileExt, mSuggestedFileName.Length() - pos);
-
     // Now, compare fileExt to mTempFileExtension.
-    if (fileExt.Equals(mTempFileExtension,
-                       nsCaseInsensitiveStringComparator())) {
+    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
--- a/uriloader/exthandler/nsExternalHelperAppService.h
+++ b/uriloader/exthandler/nsExternalHelperAppService.h
@@ -424,21 +424,20 @@ class nsExternalAppHandler final : publi
   void NotifyTransfer(nsresult aStatus);
 
   /**
    * Helper routine that searches a pref string for a given mime type
    */
   bool GetNeverAskFlagFromPref(const char* prefName, const char* aContentType);
 
   /**
-   * Helper routine to ensure mSuggestedFileName is "correct";
-   * this ensures that mTempFileExtension only contains an extension when it
-   * is different from mSuggestedFileName's extension.
+   * Helper routine to ensure that mTempFileExtension only contains an extension
+   * when it is different from mSuggestedFileName's extension.
    */
-  void EnsureSuggestedFileName();
+  void EnsureTempFileExtension(const nsString& aFileExt);
 
   typedef enum { kReadError, kWriteError, kLaunchError } ErrorType;
   /**
    * Utility function to send proper error notification to web progress listener
    */
   void SendStatusChange(ErrorType type, nsresult aStatus, nsIRequest* aRequest,
                         const nsString& path);
 
rename from uriloader/exthandler/tests/mochitest/unsafeBidi_chromeScript.js
rename to uriloader/exthandler/tests/mochitest/HelperAppLauncherDialog_chromeScript.js
new file mode 100644
--- /dev/null
+++ b/uriloader/exthandler/tests/mochitest/invalidCharFileExtension.sjs
@@ -0,0 +1,14 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+function handleRequest(request, response) {
+  response.setStatusLine(request.httpVersion, 200, "OK");
+
+  if (!request.queryString.match(/^name=/))
+    return;
+  var name = decodeURIComponent(request.queryString.substring(5));
+
+  response.setHeader("Content-Type", "image/png; name=\"" + name + "\"");
+  response.setHeader("Content-Disposition", "attachment; filename=\"" + name + "\"");
+}
--- a/uriloader/exthandler/tests/mochitest/mochitest.ini
+++ b/uriloader/exthandler/tests/mochitest/mochitest.ini
@@ -1,14 +1,20 @@
 [DEFAULT]
 support-files =
   handlerApp.xhtml
   handlerApps.js
-  unsafeBidi_chromeScript.js
-  unsafeBidiFileName.sjs
 
 [test_handlerApps.xhtml]
 skip-if = (toolkit == 'android' || os == 'mac') || e10s # OS X: bug 786938
 scheme = https
+[test_invalidCharFileExtension.xhtml]
+skip-if = toolkit == 'android' && !is_fennec # Bug 1525959
+support-files =
+  HelperAppLauncherDialog_chromeScript.js
+  invalidCharFileExtension.sjs
 [test_unknown_ext_protocol_handlers.html]
 [test_unsafeBidiChars.xhtml]
 skip-if = toolkit == 'android' && !is_fennec # Bug 1525959
+support-files =
+  HelperAppLauncherDialog_chromeScript.js
+  unsafeBidiFileName.sjs
 [test_web_protocol_handlers.html]
new file mode 100644
--- /dev/null
+++ b/uriloader/exthandler/tests/mochitest/test_invalidCharFileExtension.xhtml
@@ -0,0 +1,56 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head>
+  <title>Test for Handling of unsafe bidi chars</title>
+  <script src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<p id="display"></p>
+<iframe id="test"></iframe>
+<script type="text/javascript">
+<![CDATA[
+
+var tests = [
+  ["test.png:large", "test.png large.png"],
+  ["test.png/large", "test.png_large.png"],
+  [":test.png::large:", "test.png large.png"],
+];
+
+const INSECURE_REGISTER_PREF = "dom.registerProtocolHandler.insecure.enabled";
+
+add_task(async function() {
+  SpecialPowers.setBoolPref(INSECURE_REGISTER_PREF, false);
+  let url = SimpleTest.getTestFileURL("HelperAppLauncherDialog_chromeScript.js");
+  let chromeScript = SpecialPowers.loadChromeScript(url);
+
+  for (let [name, expected] of tests) {
+    let promiseName = new Promise(function(resolve) {
+      chromeScript.addMessageListener("suggestedFileName",
+                                      function listener(data) {
+        chromeScript.removeMessageListener("suggestedFileName", listener);
+        resolve(data);
+      });
+    });
+    document.getElementById("test").src =
+      "invalidCharFileExtension.sjs?name=" + encodeURIComponent(name);
+    is((await promiseName), expected, "got the expected sanitized name");
+  }
+
+  let promise = new Promise(function(resolve) {
+    chromeScript.addMessageListener("unregistered", function listener() {
+      chromeScript.removeMessageListener("unregistered", listener);
+      resolve();
+    });
+  });
+  chromeScript.sendAsyncMessage("unregister");
+  await promise;
+
+  SpecialPowers.clearUserPref(INSECURE_REGISTER_PREF);
+
+  chromeScript.destroy();
+});
+
+]]>
+</script>
+</body>
+</html>
--- a/uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml
+++ b/uriloader/exthandler/tests/mochitest/test_unsafeBidiChars.xhtml
@@ -36,17 +36,17 @@ function replace(name, x) {
 function sanitize(name) {
   return replace(name, "_");
 }
 
 const INSECURE_REGISTER_PREF = "dom.registerProtocolHandler.insecure.enabled";
 
 add_task(async function() {
   SpecialPowers.setBoolPref(INSECURE_REGISTER_PREF, false);
-  let url = SimpleTest.getTestFileURL("unsafeBidi_chromeScript.js");
+  let url = SimpleTest.getTestFileURL("HelperAppLauncherDialog_chromeScript.js");
   let chromeScript = SpecialPowers.loadChromeScript(url);
 
   for (let test of tests) {
     for (let char of unsafeBidiChars) {
       let promiseName = new Promise(function(resolve) {
         chromeScript.addMessageListener("suggestedFileName",
                                         function listener(data) {
           chromeScript.removeMessageListener("suggestedFileName", listener);