1898498 - downloads.download throws if the filename containing "%"

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[eight04]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:128.0) Gecko/20100101 Firefox/128.0

Steps to reproduce:

1. Visit https://github.com/eight04/webextension-test/archive/refs/heads/illegal-filename.zip, download the extension, and unzip.
2. Visit about:debugging#/runtime/this-firefox and load it as a temporary addon.
3. Click the extension button to trigger browserAction.
4. Inspect the background page and open the console.

Actual results:

An error is thrown:

Error: filename must not contain illegal characters

Expected results:

In Firefox 126, a file named %.html is downloaded.

Comment 1

[:Gijs (he/him)]

There was a security bug that we fixed for normal (non-webextension-initiated) downloads in 127 around % in filenames. But that normally leads to the filename being sanitized (ie the invalid characters get replaced with valid ones).

I don't know why the webextension API complains instead of doing that - over to webextensions.

Comment 2

[Alex Cornestean]

Hello,

I reproduced the issue on the latest Nightly (128.0a1/20240530213713) and Beta (127.0b8/20240529091551) under Windows 10 x64 and Ubuntu 22.04 LTS. Also checked Firefox Release (126.0.1/20240526221752) but this version is not affected.

The issue occurs as described in Comment 0.

Ran a mozregression as well with the following results:

2024-05-31T09:21:05.985000: DEBUG : Found commit message:
Bug 1891234, additional filename filter checks, r=Gijs,extension-reviewers

Differential Revision: https://phabricator.services.mozilla.com/D208659

Corresponding pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=822a570ebc0a96218ce637d79b5af9e0e7c90030&tochange=91862a7f62978e0a2e8cf8f72ac03666c7076890

Comment 4

[Neil Deakin]

I think this is just a version of 1791530.

The webextension apis throw an error on an invalid filename rather than accepting the sanitized filename.

Comment 5

[Rob Wu [:robwu]]

We should fix up the filename (bug 1791530) or document more clearly that invalid characters can cause errors. The current list is quite detailed but does not cover enough: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/downloads/download#filename

Chrome converts % to _: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/extensions/api/downloads/downloads_api.cc;l=1126-1128;drc=ac83a5a2d3c04763d86ce16d92f3904cc9566d3a
We should do the same, because a % is quite common (e.g. if someone were to derive the filename from the URL, odds are that it will contain a %).

Comment 6

[eight04]

Having an illegal character list would help. Currently I'm using this table:
https://github.com/eight04/image-picka/blob/39f4b4e686a6726383489cd6763dd783f18f0983/src/lib/escape.js#L3

Comment 7

[Rob Wu [:robwu]]

Would you like to have full control over the filename (i.e. explicit errors when the chosen filename is not supported), or would you prefer the browser to automatically fix the incorrect character, e.g. consistent with native browser UI?

Bug 1791530 is relevant.

Comment 8

[eight04]

Would you like to have full control over the filename (i.e. explicit errors when the chosen filename is not supported), or would you prefer the browser to automatically fix the incorrect character, e.g. consistent with native browser UI?

One benefit of automatic fixing is that we won't have a breaking change when adding a new illegal character (e.g. % in this case.)
https://addons.mozilla.org/en-US/firefox/addon/image-picka/reviews/2082846/
And extension developers don't have to update their escape table individually.
But then we should tell them to query the download ID if they need the real filename.

Personally, I'm fine with both cases as long as those characters are documented. Otherwise I have to dig the source or write a test suite to find out which characters are invalid.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:willdurand, could you have a look please?

For more information, please visit BugBot documentation.

Comment 10

[Rob Wu [:robwu]]

Because % worked before and now does not, I'm going to submit a patch that converts % to _ like Chrome. Otherwise extensions that used to work before will break.

Comment 11

[Rob Wu [:robwu]]

Attachment: Bug 1898498 - downloads.download - transform % to _

Comment 12

[eight04]

I wrote a test to check illegal characters (from 0 to 127) and here is the result:
https://github.com/eight04/webext-illegal-filename/blob/master/report.md

Seems that there are only two characters behave differently from chromium:

• 0 - In edge, you can actually put \0 in the filename. Not sure what will actually happen though.
• 37 - %
  • FF 126 - valid.
  • FF 129 - invalid.
  • Edge 126 - valid. Will be replaced with _.

I also tried testing firefox-android. However the promise returned by downloads.download never resolves.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51a412de4d68
downloads.download - transform % to _ r=willdurand,geckoview-reviewers,m_kato

Comment 14

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/51a412de4d68

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:robwu, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox128 to wontfix.

For more information, please visit BugBot documentation.

Comment 16

[Rob Wu [:robwu]]

Attachment: Bug 1898498 - downloads.download - transform % to _

Original Revision: https://phabricator.services.mozilla.com/D213846

Comment 17

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Fixes regression in 127+128: Extensions cannot save files where the suggested filename contains a "%"
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: See STR from bug report.
• Risk associated with taking this patch: none
• Explanation of risk level: Change limited to the part of the extension API implementation that is affected.
• String changes made/needed: none
• Is Android affected?: no

Comment 18

[Rob Wu [:robwu]]

Attachment: Bug 1898498 - downloads.download - transform % to _

Original Revision: https://phabricator.services.mozilla.com/D213846

Comment 19

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: Fixes regression in ESR115 (127): Extensions cannot save files where the suggested filename contains a "%"
• Code covered by automated testing: yes
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: See STR from bug report.
• Risk associated with taking this patch: none
• Explanation of risk level: Change limited to the part of the extension API implementation that is affected. Covered by unit tests.
• String changes made/needed: none
• Is Android affected?: no

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/d25dbcf9694f

Comment 21

[Alex Cornestean]

Verified as Fixed. Tested on the latest Nightly (129.0a1/20240618214224) and Beta (128.0b5/20240618143848, from https://treeherder.mozilla.org/jobs?repo=mozilla-beta&revision=d25dbcf9694ffef374d61fdaa933c071315ba0c1) under Windows 10 x64 and Ubuntu 22.04 LTS.

The % is replaced by _ in the downloaded file name and the error is no longer thrown, confirming the fix.

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/4ce591c56fd8

Comment 23

[eight04]

A user reported a similar case with a different pattern. It's seems that it is not just about "illegal characters":
https://bugzilla.mozilla.org/show_bug.cgi?id=1903780

Comment 24

[Alex Cornestean]

Verified as Fixed. Tested on ESR (115.13.0esr/20240620161327 from https://treeherder.mozilla.org/jobs?repo=mozilla-esr115&revision=4ce591c56fd88ca3fcf7ad3a46b9cb7d0ecb8257) under Windows 10 x64 and Ubuntu 22.04 LTS.

The % is replaced by _ in the downloaded file name and the error is no longer thrown, confirming the fix.

Comment 25

[:Gijs (he/him)]

(In reply to eight04 from comment #23)

A user reported a similar case with a different pattern. It's seems that it is not just about "illegal characters":
https://bugzilla.mozilla.org/show_bug.cgi?id=1903780

There was a separate change to filename sanitization that can replace spaces with _ in some situations. I'll follow up in bug 1903780.