Closed Bug 1463487 Opened 6 years ago Closed 6 years ago

webextensions download api filename is not decoded

Categories

(WebExtensions :: General, defect, P3)

60 Branch
defect

Tracking

(firefox64 verified)

VERIFIED FIXED
mozilla64
Tracking Status
firefox64 --- verified

People

(Reporter: robbendebiene, Assigned: arshadkazmi42, Mentored)

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180516032328

Steps to reproduce:

Save an image whose filename contains special characters with the downloads API and omit the filename so the default image name is taken.

Example:

const queryDownload = browser.downloads.download({
 url: "http://img0.joyreactor.cc/pics/post/KatRaccoon-%D0%9A%D0%BE%D0%BC%D0%B8%D0%BA%D1%81%D1%8B-%D0%BA%D1%83%D1%80%D1%8B-4442270.jpeg"
});


Actual results:

The filename of the image is not decoded (decodeURI) and looks like this: KatRaccoon-%D0%9A%D0%BE%D0%BC%D0%B8%D0%BA%D1%81%D1%8B-%D0%BA%D1%83%D1%80%D1%8B-4442270.jpeg


Expected results:

The image name should be decoded to: KatRaccoon-Комиксы-куры-4442270.jpeg

If an image or file is saved via the context menu the filename is properly decoded, therefore I would expect the same behaviour by using the downloads API.

(The issue was first reported here: https://github.com/Robbendebiene/Gesturefy/issues/292)
Component: Downloads API → General
Product: Toolkit → WebExtensions
Mentor: rob
Keywords: good-first-bug
Priority: -- → P3
I am interested to take this up
Flags: needinfo?(rob)
In this case, the "filename" parameter is not specified in the downloads.download call, so the filename is derived from the URL:

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/toolkit/components/extensions/parent/ext-downloads.js#469

You should use decodeURIComponent on the filename before passing it to DownloadPaths.sanitize.

In the bug report, decodeURI was suggested, but this would result in partially decoded URLs in some cases, such as when %20 (space) is used.

In your previous contributions (contextMenu API), browser-chrome tests were used because that API modified the browser UI.
The downloads API is independent of browser UI, so you should use xpcshell test instead, for example by adding a few good test cases here:
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js#263
The documentation of decodeURComponent provides some good examples:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURIComponent
Assignee: nobody → arshadkazmi42
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(rob)
In my previous comment I forgot to use a permalink. Here is the permalink that stays the same even if the code changes.

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js#263
I was writing the test case.
Should I create a new test case file or should I add my test cases in the file mentioned in Comment 2?
Flags: needinfo?(rob)
Add it to the existing file.
Flags: needinfo?(rob)
I am done with the script

add_task(async function test_encoded_url_downloads() {
  function background() {
    browser.test.onMessage.addListener(async (msg, options) => {
      if (msg === "download.request") {
        try {
          let id = await browser.downloads.download(options);
          browser.test.sendMessage("download.done", {status: "success", id: id});
        } catch (error) {
          browser.test.sendMessage("download.done", {status: "error", errmsg: error.message});
        }
      } else if (msg === "search.start") {
        let downloadedItem = await browser.downloads.search(options);
        browser.test.sendMessage("search.done", downloadedItem);
      }
    });
  }

  const manifest = {permissions: ["downloads"]};
  const extension = ExtensionTestUtils.loadExtension({background, manifest});
  await extension.startup();


  function download(options) {
    extension.sendMessage("download.request", options);
    return extension.awaitMessage("download.done");
  }

  function search(options) {
    extension.sendMessage("search.start", options);
    return extension.awaitMessage("search.done");
  }

  async function testEncodedDownload(options) {
    let msg = await download(options);
    equal(msg.status, "success", `downloads works with encoded url`);

    const downloadedItem = await search({
      id: msg.id
    });
    const downloadedFileName = downloadedItem[0].filename;
    const lastIndex = downloadedItem[0].filename.lastIndexOf("/");
    equal(downloadedFileName.substr(lastIndex + 1), FILE_NAME_ENCODED, "Downloaded filename should match with the input filename");
  }

  await testEncodedDownload({url: FILE_ENCODED_URL});
  remove(FILE_NAME_ENCODED);
  await extension.unload();
});


Getting one issue, after running the script I am getting this error in logs


FAIL xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js - xpcshell return code: 0
  FAIL test_encoded_url_downloads - [test_encoded_url_downloads : 40] Leftover file /Users/arshad/Arshad/workspace/opensource/mozilla-central/obj-x86_64-apple-darwin17.7.0/temp/xpc-profile-oHEfo6/temp/downloads/file-download-encoded.txt in download directory - false == true


It looks like its not able to remove the file after test completion. Any suggestions, what could be missing here?
Flags: needinfo?(rob)
It is not necessary to add a whole new task. Simple re-use the test_downloads task and add the new tests after this:
https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js#264-268
Flags: needinfo?(rob)
After adding my download code here, 

https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/toolkit/components/extensions/test/xpcshell/test_ext_downloads_download.js#264-268


still I need to verify the filename, right?
Where I need to verify the name of the file?

Should I add code to fetch downloaded file and match the filename below that code?
Flags: needinfo?(rob)
I have also created a new file here

/Users/arshad/Arshad/workspace/opensource/mozilla-central/toolkit/components/extensions/test/xpcshell/data/

with this name "fill#download@encoded.txt"

Then I am formatting the url like this

[0]const FILE_ENCODED_URL = BASE + "data/" + encodeURIComponent(FILE_NAME_ENCODED);

After that I am downloading file using this [0] url
(In reply to Arshad Kazmi from comment #8)
> still I need to verify the filename, right?
> Where I need to verify the name of the file?
>
> Should I add code to fetch downloaded file and match the filename below that code?

I highlighted lines 264-268 as a hint. I suggest that you look up the definition of that function (elsewhere in the test file), as that will probably answer your questions.


(In reply to Arshad Kazmi from comment #9)
> I have also created a new file here

There is no need for an actual file. You can define another HTTP resource in the test file itself, similar to how the handler for "/dir/" is defined.
Flags: needinfo?(rob)
So I removed all the changes I did and added below code

Defined GLobally: 

const FILE_NAME_ENCODED = "file#download@encoded.txt";
const FILE_ENCODED_URL = BASE + "data/" + encodeURIComponent(FILE_NAME_ENCODED);


Added below code at the mentioned line number in Comment 7.

// Download a normal URL with an empty filename part.
  await download({
    url: FILE_ENCODED_URL
  })


After running still getting this error

Leftover file ../mozilla-central/obj-x86_64-apple-darwin17.7.0/temp/xpc-profile-IZuwYW/temp/downloads/file#download@encoded.txt in download directory - false == true


I tried adding 

remove(FILE_NAME_ENCODED)

but still getting same error.

If even a single new file gets created, the test fails.
Flags: needinfo?(rob)
Why are you calling "download"? Please glance over the whole test file and try to understand what the test is doing before writing code.

The test_download task consists of roughly two distinct parts. The first part tests API calls that are expected to fail (hence just download without file removal); the second part consists of tests where the download is expected to succeed.
Flags: needinfo?(rob)
I tried with this filename "file%Xencode%2A"

and it throws error saying 

Error: "malformed URI sequence"

So malformed URI needs to be rejected with an error? Or it needs to be parsed in some other way?
Flags: needinfo?(rob)
I explored a bit.

Found "unescape" function. Its working for invalid escapes.

But I am confused on the usage of it.

I should just unescape the filename inplace of decodeURIComponent?
Or Should I add try catch block to handle malformed error and then use unescape
Flags: needinfo?(rob)
need info
Flags: needinfo?(rob)
(In reply to Arshad Kazmi from comment #15)
> I explored a bit.
> 
> Found "unescape" function. Its working for invalid escapes.

Well done!

> But I am confused on the usage of it.
> 
> I should just unescape the filename inplace of decodeURIComponent?
> Or Should I add try catch block to handle malformed error and then use
> unescape

You should also add a test case where the filename contains an URL-encoded multi-byte UTF-8 character.
From that you can tell which of your two options should be used.
Flags: needinfo?(rob)
I tried with these two file names

const FILE_NAME_ENCODED_1 = "file%Xencode%2A.txt";
const FILE_NAME_ENCODED_2 = "file's%2Bencoded'%20download_test.txt";

And test case is passing for both file only with try, catch method


try {
 filename = DownloadPaths.sanitize(decodeURIComponent(uri.fileName));
} catch (e) {
 filename = DownloadPaths.sanitize(unescape(uri.fileName));
}


I think, I will go with try catch method.
Flags: needinfo?(rob)
Attachment #9008135 - Attachment description: Decoding downloaded api filename → Bug 1463487 - Webextension download api filename decoding
Attachment #9008135 - Attachment description: Bug 1463487 - Webextension download api filename decoding → Bug 1463487 - Use urldecoded filename by default in downloads.download
Comment on attachment 9008135 [details]
Bug 1463487 - Use urldecoded filename by default in downloads.download

Rob Wu [:robwu] has approved the revision.
Attachment #9008135 - Flags: review+
Flags: needinfo?(rob)
Keywords: dev-doc-needed
Comment on attachment 9008135 [details]
Bug 1463487 - Use urldecoded filename by default in downloads.download

Andrew Swan [:aswan] has approved the revision.
Attachment #9008135 - Flags: review+
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa2dfb61abb2
Use urldecoded filename by default in downloads.download r=robwu,aswan
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aa2dfb61abb2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Attached image Bug1463487.png
I was able to reproduce this issue on Firefox 63.0b9(20180924202103) under Win 7 64-bit and  Mac OS X 10.13.3.

This issue is verified as fixed on Firefox 64.0a1(20180925100100) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached screenshot.
Status: RESOLVED → VERIFIED
Note to docs team:

I have added a note to cover this change in the Fx 64 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#API_changes

The docs for this will involve a simple change and a compat data entry to represent this change.
Added a note to the page. The BCD says that download filename is supported from Firefox 47. Do we need to add a note to the compatibility data?
Thanks Irene.

No further action is needed.
This does not need to be added to BCD.

The location of the notes at the "options" parameter and "url" property seemed out of place, because this bug is about the default file name, not the URL.
I considered moving these notes to the "filename" property, so that the behavior is not explicitly documented. By leaving the behavior unspecified, we can change the implementation without breaking extensions that would try to rely on documented behavior.

The filename should ideally be based on the Content-Disposition header and fall back to the URL.
In Chrome, this is the case.
In Firefox, this happens when a URL is opened in the browser, but NOT when downloaded through the extension API (this might be addressed if we can fix bug 1245652).

Since the value is different across browsers, and the behavior is likely to change, I decided to remove the notes for now.
I also removed the information from the release notes.
You need to log in before you can comment on or make changes to this bug.