webextensions download api filename is not decoded

VERIFIED FIXED in Firefox 64

Status

P3
normal
VERIFIED FIXED
10 months ago
4 months ago

People

(Reporter: robbendebiene, Assigned: arshadkazmi42, Mentored)

Tracking

({dev-doc-complete, good-first-bug})

60 Branch
mozilla64
dev-doc-complete, good-first-bug

Firefox Tracking Flags

(firefox64 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

10 months ago
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)

Updated

9 months ago
Component: Downloads API → General
Product: Toolkit → WebExtensions
Mentor: rob
Keywords: good-first-bug
Priority: -- → P3
(Assignee)

Comment 1

7 months ago
I am interested to take this up
Flags: needinfo?(rob)

Comment 2

7 months ago
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)

Comment 3

7 months ago
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
(Assignee)

Comment 4

7 months ago
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)

Comment 5

7 months ago
Add it to the existing file.
Flags: needinfo?(rob)
(Assignee)

Comment 6

7 months ago
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)

Comment 7

7 months ago
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)
(Assignee)

Comment 8

7 months ago
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)
(Assignee)

Comment 9

7 months ago
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

Comment 10

7 months ago
(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)
(Assignee)

Comment 11

7 months ago
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)

Comment 12

7 months ago
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)
(Assignee)

Comment 14

7 months ago
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)
(Assignee)

Comment 15

7 months ago
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)
(Assignee)

Comment 16

7 months ago
need info
Flags: needinfo?(rob)

Comment 17

7 months ago
(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)
(Assignee)

Comment 18

7 months ago
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 19

6 months ago
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+

Updated

6 months ago
Flags: needinfo?(rob)

Updated

6 months ago
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+
(Assignee)

Updated

6 months ago
Keywords: checkin-needed

Comment 21

6 months ago
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

Comment 22

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aa2dfb61abb2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Comment 23

6 months ago
Posted 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.

Updated

6 months ago
Status: RESOLVED → VERIFIED
status-firefox64: fixed → 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.

Comment 25

4 months ago
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?
Keywords: dev-doc-needed → dev-doc-complete

Comment 26

4 months ago
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

4 months ago
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.