1463487 - webextensions download api filename is not decoded

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[robbendebiene]

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)

Comment 1

[Arshad Kazmi [:arshadkazmi42]]

I am interested to take this up

Comment 2

[Rob Wu [:robwu]]

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

Comment 3

[Rob Wu [:robwu]]

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

Comment 4

[Arshad Kazmi [:arshadkazmi42]]

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?

Comment 5

[Rob Wu [:robwu]]

Add it to the existing file.

Comment 6

[Arshad Kazmi [:arshadkazmi42]]

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?

Comment 7

[Rob Wu [:robwu]]

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

Comment 8

[Arshad Kazmi [:arshadkazmi42]]

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?

Comment 9

[Arshad Kazmi [:arshadkazmi42]]

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#[e-mail]"

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

Comment 10

[Rob Wu [:robwu]]

(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.

Comment 11

[Arshad Kazmi [:arshadkazmi42]]

So I removed all the changes I did and added below code

Defined GLobally: 

const FILE_NAME_ENCODED = "file#[e-mail]";
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#[e-mail] 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.

Comment 12

[Rob Wu [:robwu]]

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.

Comment 13

[Arshad Kazmi [:arshadkazmi42]]

Attachment: Bug 1463487 - Use urldecoded filename by default in downloads.download

Comment 14

[Arshad Kazmi [:arshadkazmi42]]

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?

Comment 15

[Arshad Kazmi [:arshadkazmi42]]

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

Comment 16

[Arshad Kazmi [:arshadkazmi42]]

need info

Comment 17

[Rob Wu [:robwu]]

(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.

Comment 18

[Arshad Kazmi [:arshadkazmi42]]

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.

Comment 19

[Rob Wu [:robwu]]

Comment on attachment 9008135 [details]
Bug 1463487 - Use urldecoded filename by default in downloads.download

Rob Wu [:robwu] has approved the revision.

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 9008135 [details]
Bug 1463487 - Use urldecoded filename by default in downloads.download

Andrew Swan [:aswan] has approved the revision.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa2dfb61abb2
Use urldecoded filename by default in downloads.download r=robwu,aswan

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/aa2dfb61abb2

Comment 23

[CosminB]

Attachment: 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.

Comment 24

[Chris Mills [:cmills]]

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.

Comment 25

[Irene Smith]

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?

Comment 26

[Rob Wu [:robwu]]

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.

Comment 27

[Irene Smith]

I also removed the information from the release notes.