Bug 1383262 - Better download() error message and default filename r?aswan draft
authorTomislav Jovanovic <tomica@gmail.com>
Sun, 30 Jul 2017 02:36:08 +0200
changeset 618449 2fab2b62d3345e2e36a482d1a54698538f8ad2af
parent 618383 26516ba270816a6cc90f5c42a9b66701369a551f
child 640072 f3d7167e0271a05364037311100c837dde385e52
push id71335
push userbmo:tomica@gmail.com
push dateMon, 31 Jul 2017 14:13:14 +0000
reviewersaswan
bugs1383262
milestone56.0a1
Bug 1383262 - Better download() error message and default filename r?aswan MozReview-Commit-ID: IbPb2sj3AQ9
toolkit/components/extensions/ext-downloads.js
toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js
--- a/toolkit/components/extensions/ext-downloads.js
+++ b/toolkit/components/extensions/ext-downloads.js
@@ -412,16 +412,20 @@ this.downloads = class extends Extension
             let path = OS.Path.split(filename);
             if (path.absolute) {
               return Promise.reject({message: "filename must not be an absolute path"});
             }
 
             if (path.components.some(component => component == "..")) {
               return Promise.reject({message: "filename must not contain back-references (..)"});
             }
+
+            if (AppConstants.platform === "win" && /[|"*?:<>]/.test(filename)) {
+              return Promise.reject({message: "filename must not contain illegal characters"});
+            }
           }
 
           if (options.conflictAction == "prompt") {
             // TODO
             return Promise.reject({message: "conflictAction prompt not yet implemented"});
           }
 
           if (options.headers) {
@@ -458,21 +462,21 @@ this.downloads = class extends Extension
 
           async function createTarget(downloadsDir) {
             let target;
             if (filename) {
               target = OS.Path.join(downloadsDir, filename);
             } else {
               let uri = NetUtil.newURI(options.url);
 
-              let remote = "download";
+              let remote;
               if (uri instanceof Ci.nsIURL) {
                 remote = uri.fileName;
               }
-              target = OS.Path.join(downloadsDir, remote);
+              target = OS.Path.join(downloadsDir, remote || "download");
             }
 
             // Create any needed subdirectories if required by filename.
             const dir = OS.Path.dirname(target);
             await OS.File.makeDir(dir, {from: downloadsDir});
 
             if (await OS.File.exists(target)) {
               // This has a race, something else could come along and create
--- a/toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js
@@ -1,25 +1,25 @@
 /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
 /* vim: set sts=2 sw=2 et tw=80: */
 "use strict";
 
-/* global OS */
-
 Cu.import("resource://gre/modules/osfile.jsm");
 Cu.import("resource://gre/modules/Downloads.jsm");
 
 const gServer = createHttpServer();
 gServer.registerDirectory("/data/", do_get_file("data"));
 
+gServer.registerPathHandler("/dir/", (_, res) => res.write("length=8"));
+
 const WINDOWS = AppConstants.platform == "win";
 
-const BASE = `http://localhost:${gServer.identity.primaryPort}/data`;
+const BASE = `http://localhost:${gServer.identity.primaryPort}/`;
 const FILE_NAME = "file_download.txt";
-const FILE_URL = BASE + "/" + FILE_NAME;
+const FILE_URL = BASE + "data/" + FILE_NAME;
 const FILE_NAME_UNIQUE = "file_download(1).txt";
 const FILE_LEN = 46;
 
 let downloadDir;
 
 function setup() {
   downloadDir = FileUtils.getDir("TmpD", ["downloads"]);
   downloadDir.createUnique(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
@@ -231,30 +231,46 @@ add_task(async function test_downloads()
   await download({
     url: FILE_URL,
     filename: OS.Path.join("foo", "..", "..", "file_download.txt"),
   }).then(msg => {
     equal(msg.status, "error", "downloads.download() fails with back-references");
     equal(msg.errmsg, "filename must not contain back-references (..)", "error message for back-references is correct");
   });
 
+  // Test illegal characters on Windows.
+  if (WINDOWS) {
+    await download({
+      url: FILE_URL,
+      filename: "like:this",
+    }).then(msg => {
+      equal(msg.status, "error", "downloads.download() fails with illegal chars");
+      equal(msg.errmsg, "filename must not contain illegal characters", "error message correct");
+    });
+  }
+
   // Try to download a blob url
   const BLOB_STRING = "Hello, world";
   await testDownload({
     blobme: [BLOB_STRING],
     filename: FILE_NAME,
   }, FILE_NAME, BLOB_STRING.length, "blob url");
   extension.sendMessage("killTheBlob");
 
   // Try to download a blob url without a given filename
   await testDownload({
     blobme: [BLOB_STRING],
   }, "download", BLOB_STRING.length, "blob url with no filename");
   extension.sendMessage("killTheBlob");
 
+  // Download a normal URL with an empty filename part.
+  await testDownload({
+    url: BASE + "dir/",
+  }, "download", 8, "normal url with empty filename");
+
   await extension.unload();
 });
 
 add_task(async function test_download_post() {
   const server = createHttpServer();
   const url = `http://localhost:${server.identity.primaryPort}/post-log`;
 
   let received;