Bug 960749 - Improve error messages when < and > are used in filenames. r=bsmedberg, a=1.4+
authorGhislain 'Aus' Lacroix <glacroix@mozilla.com>
Thu, 20 Mar 2014 14:05:40 -0700
changeset 192258 203ea09d5bdbf0946aeaf7fe7db25db96bb67f62
parent 192257 175ec8a02237e3be8728d0305b7d422bc72d6027
child 192259 a96a82ca7f0219d71010fe3631bb7643fa8d365e
push id474
push userasasaki@mozilla.com
push dateMon, 02 Jun 2014 21:01:02 +0000
treeherdermozilla-release@967f4cf1b31c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg, 1
bugs960749
milestone30.0a2
Bug 960749 - Improve error messages when < and > are used in filenames. r=bsmedberg, a=1.4+ * Unify OS_FILE_ILLEGAL_CHARACTERS * Provide KNOWN_PATH_SEPARATORS in addition to FILE_PATH_SEPARATOR. * Update nsExternalAppHandler to use KNOWN_PATH_SEPARATORS instead of FILE_PATH_SEPARATOR when removing path separators. * Fix DownloadsIPC.jsm when compiling with MOZ_DEBUG (would throw on undefined download.id). * Add test for stripping out illegal characters when downloading.
dom/downloads/src/DownloadsIPC.jsm
dom/downloads/tests/mochitest.ini
dom/downloads/tests/test_downloads_bad_file.html
uriloader/exthandler/nsExternalHelperAppService.cpp
xpcom/glue/nsCRTGlue.h
--- a/dom/downloads/src/DownloadsIPC.jsm
+++ b/dom/downloads/src/DownloadsIPC.jsm
@@ -76,17 +76,17 @@ this.DownloadsIPC = {
     // We actually have an array of downloads.
     aDownloads.forEach((aDownload) => {
       this.downloads[aDownload.id] = aDownload;
     });
   },
 
   receiveMessage: function(aMessage) {
     let download = aMessage.data;
-    debug("message: " + aMessage.name + " " + download.id);
+    debug("message: " + aMessage.name);
     switch(aMessage.name) {
       case "Downloads:GetList:Return":
         this._updateDownloadsArray(download);
 
         if (!this.ready) {
           this.getListPromises.forEach(aPromise =>
                                        aPromise.resolve(this.downloads));
           this.getListPromises.length = 0;
--- a/dom/downloads/tests/mochitest.ini
+++ b/dom/downloads/tests/mochitest.ini
@@ -1,11 +1,12 @@
 [DEFAULT]
 skip-if = buildapp == 'b2g' # bug 979446, frequent failures
 support-files =
   serve_file.sjs
 
 [test_downloads_navigator_object.html]
 [test_downloads_basic.html]
 [test_downloads_large.html]
+[test_downloads_bad_file.html]
 [test_downloads_pause_remove.html]
 [test_downloads_pause_resume.html]
 skip-if = toolkit=='gonk' # b2g(bug 947167) b2g-debug(bug 947167)
new file mode 100644
--- /dev/null
+++ b/dom/downloads/tests/test_downloads_bad_file.html
@@ -0,0 +1,93 @@
+<!DOCTYPE html>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=960749
+-->
+<head>
+  <title>Test for Bug 960749 Downloads API</title>
+  <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=960749">Mozilla Bug 960749</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+</div>
+<a href="serve_file.sjs?contentType=application/octet-stream&size=1024" download=".&lt;.EVIL.&gt;\ / : * ? &quot; |file.bin" id="download1">Download #1</a>
+<pre id="test">
+<script class="testbody" type="text/javascript;version=1.7">
+
+// Testing a simple download, waiting for it to be done.
+
+SimpleTest.waitForExplicitFinish();
+
+var index = -1;
+var expected = "_.EVIL.__ _ _ _ _ _ _file.bin";
+
+function next() {
+  index += 1;
+  if (index >= steps.length) {
+    ok(false, "Shouldn't get here!");
+    return;
+  }
+  try {
+    steps[index]();
+  } catch(ex) {
+    ok(false, "Caught exception", ex);
+  }
+}
+
+function checkTargetFilename(download) {
+  ok(download.path.endsWith(expected),
+     "Download path leaf name '" + download.path +
+     "' should match '" + expected + "' filename.");
+
+  SimpleTest.finish();
+}
+
+function downloadChange(evt) {
+  var download = evt.download;
+
+  if (download.state === "succeeded") {
+    checkTargetFilename(download);
+  }
+}
+
+function downloadStart(evt) {
+  var download = evt.download;
+  download.onstatechange = downloadChange;
+}
+
+var steps = [
+  // Start by setting the pref to true.
+  function() {
+    SpecialPowers.pushPrefEnv({
+      set: [["dom.mozDownloads.enabled", true]]
+    }, next);
+  },
+
+  // Setup the event listeners.
+  function() {
+    SpecialPowers.pushPermissions([
+      {type: "downloads", allow: true, context: document}
+    ], function() {
+      navigator.mozDownloadManager.ondownloadstart = downloadStart;
+      next();
+    });
+  },
+
+  // Click on the <a download> to start the download.
+  function() {
+    document.getElementById("download1").click();
+  }
+];
+
+next();
+
+</script>
+</pre>
+</body>
+</html>
+
--- a/uriloader/exthandler/nsExternalHelperAppService.cpp
+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
@@ -1233,18 +1233,18 @@ nsExternalAppHandler::nsExternalAppHandl
 {
 
   // make sure the extention includes the '.'
   if (!aTempFileExtension.IsEmpty() && aTempFileExtension.First() != '.')
     mTempFileExtension = char16_t('.');
   AppendUTF8toUTF16(aTempFileExtension, mTempFileExtension);
 
   // replace platform specific path separator and illegal characters to avoid any confusion
-  mSuggestedFileName.ReplaceChar(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS, '_');
-  mTempFileExtension.ReplaceChar(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS, '_');
+  mSuggestedFileName.ReplaceChar(KNOWN_PATH_SEPARATORS FILE_ILLEGAL_CHARACTERS, '_');
+  mTempFileExtension.ReplaceChar(KNOWN_PATH_SEPARATORS 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
     char16_t(0x202b), // Right-to-Left Embedding
@@ -1431,23 +1431,23 @@ nsresult nsExternalAppHandler::SetUpTemp
   NS_Free(buffer);
   buffer = nullptr;
   NS_ENSURE_SUCCESS(rv, rv);
 
   tempLeafName.Truncate(wantedFileNameLength);
 
   // Base64 characters are alphanumeric (a-zA-Z0-9) and '+' and '/', so we need
   // to replace illegal characters -- notably '/'
-  tempLeafName.ReplaceChar(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS, '_');
+  tempLeafName.ReplaceChar(KNOWN_PATH_SEPARATORS FILE_ILLEGAL_CHARACTERS, '_');
 
   // now append our extension.
   nsAutoCString ext;
   mMimeInfo->GetPrimaryExtension(ext);
   if (!ext.IsEmpty()) {
-    ext.ReplaceChar(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS, '_');
+    ext.ReplaceChar(KNOWN_PATH_SEPARATORS FILE_ILLEGAL_CHARACTERS, '_');
     if (ext.First() != '.')
       tempLeafName.Append('.');
     tempLeafName.Append(ext);
   }
 
   // We need to temporarily create a dummy file with the correct
   // file extension to determine the executable-ness, so do this before adding
   // the extra .part extension.
--- a/xpcom/glue/nsCRTGlue.h
+++ b/xpcom/glue/nsCRTGlue.h
@@ -105,27 +105,32 @@ NS_COM_GLUE void NS_MakeRandomString(cha
 
 #define FF '\f'
 #define TAB '\t'
 
 #define CRSTR "\015"
 #define LFSTR "\012"
 #define CRLF "\015\012"     /* A CR LF equivalent string */
 
+// We use the most restrictive filesystem as our default set of illegal filename
+// characters. This is currently Windows.
+#define OS_FILE_ILLEGAL_CHARACTERS "/:*?\"<>|"
+// We also provide a list of all known file path separators for all filesystems.
+// This can be used in replacement of FILE_PATH_SEPARATOR when you need to
+// identify or replace all known path separators.
+#define KNOWN_PATH_SEPARATORS "\\/"
+
 #if defined(XP_MACOSX)
   #define FILE_PATH_SEPARATOR        "/"
-  #define OS_FILE_ILLEGAL_CHARACTERS ":"
 #elif defined(XP_WIN)
   #define FILE_PATH_SEPARATOR        "\\"
-  #define OS_FILE_ILLEGAL_CHARACTERS "/:*?\"<>|"
 #elif defined(XP_UNIX)
   #define FILE_PATH_SEPARATOR        "/"
-  #define OS_FILE_ILLEGAL_CHARACTERS ""
 #else
-  #error need_to_define_your_file_path_separator_and_illegal_characters
+  #error need_to_define_your_file_path_separator_and_maybe_illegal_characters
 #endif
 
 // Not all these control characters are illegal in all OSs, but we don't really
 // want them appearing in filenames
 #define CONTROL_CHARACTERS     "\001\002\003\004\005\006\007" \
                            "\010\011\012\013\014\015\016\017" \
                            "\020\021\022\023\024\025\026\027" \
                            "\030\031\032\033\034\035\036\037"