1903780 - downloads.download throws if the folder name contains a dot and a space

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[eight04]

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

Steps to reproduce:

1. Download and install the extension: https://github.com/eight04/webextension-test/archive/refs/heads/illegal-filename-2.zip
2. Click the extension button to trigger browserAction.
3. Inspect the background page and open the console.

Actual results:

Two errors are thrown:

Error: filename must not contain illegal characters

Expected results:

Two files are downloaded correctly with FF126 and Edge.

Reported by https://github.com/eight04/image-picka/issues/347#issuecomment-2173368486

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'WebExtensions::Untriaged' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[:Gijs (out for now; he/him)]

I guess this is because we're intending to replace the space with _ in the file extension after bug 1891234. Neil, does that sound right? Can we refine the replacement so it doesn't affect the case reported here? (looks like it's a filename that contains . followed by somewhere, before the final .jpg)

Comment 3

[vcarciu]

I am not able to see any errors in consoles in FF 127 and 129, but also the files expected to be downloaded are not present.

Can you please attach a video or a screenshot with the errors?

Comment 4

[eight04]

Attachment: 2024-06-25 21-55-34.mkv

I started the browser via web-ext. Does it affect logging?

Comment 5

[Alex Cornestean]

Hello,

I reproduced the issue on the latest Nightly (129.0a1/20240625213230), Beta (128.0b7/20240624113619) and Release (127.0.2/20240624183754) under Windows 10 x64 and Ubuntu 22.04 LTS.

Clicking the extension button will throw two errors as mentioned in Comment 0 and the files are not downloaded. See the attached screenshots for more details.

Comment 6

[Alex Cornestean]

Attachment: 2024-06-26_09h37_57.png

Comment 8

[Donal Meehan [:dmeehan]]

:robwu do you plan on making a similar fix as in Bug 1898498?
:zombie could this be triaged for severity?

Comment 9

[Rob Wu [:robwu]]

I'm not planning to do the same fix as bug 1898498, because I don't think that we should disallow directories in the format blabla<dot><space>something.

I think that we need to use a different mechanism to verify whether an input is an acceptable directory name, because DownloadPaths.sanitize is now too strict.

• https://searchfox.org/mozilla-central/rev/b368ed8b48c0ea8ed2f1948e4776a6fbb5976dff/toolkit/components/extensions/parent/ext-downloads.js#704-709
• https://searchfox.org/mozilla-central/rev/b368ed8b48c0ea8ed2f1948e4776a6fbb5976dff/mobile/shared/components/extensions/ext-downloads.js#205-207

Comment 10

[Tomislav Jovanovic :zombie]

(In reply to :Gijs (he/him) from comment #2)

I guess this is because we're intending to replace the space with _ in the file extension after bug 1891234. Neil, does that sound right? Can we refine the replacement so it doesn't affect the case reported here? (looks like it's a filename that contains . followed by somewhere, before the final .jpg)

Are you thinking of this change for all downloads, not just for webextension specific ones? In other words, this change could be instead of Rob's suggestions?

Comment 11

[:Gijs (out for now; he/him)]

Actually, I'm a bit confused re-reading this bug and particularly comment #9. How would a user download a directory? :-\

(it was not clear to me from comment 0 that there was an issue with a directory, I assumed there was an issue with files and their names. I'm not aware of any way a website can download a directory directly - only files (which may be archives, which when un-archived can result in directories... but that's not really the same thing.)

I don't know that the existing checks are set up to check directory names (where extensions aren't meaningful). And if there is some non-download mechanism that allows an extension to create a folder then I don't know why that is passing through the download filename sanitizer...

Comment 13

[Tomislav Jovanovic :zombie]

I guess you're right, I was thinking that the directory was not the relevant part, that it would drop the space even if example was 1.1 A.1.jpg, but that's not the issue. It is indeed how we use the sanitizer, splitting up directory names and sanitizing each separately, which makes 1.1 A as not a valid directory name.

Comment 14

[ptidamus]

Hi

I was the user who found the specific problem with the addon I'm using.

Let me explain.

I use 2 addons. Both use the pagetitle to name things.
As I download content from an app site, page title is usually as follow : "appname - version xx.xx.xx from xxxxx"
So as you can see, I have spaces, dots and dashes.

The first addon creates a text file, named "pagetitle.txt". I have no problems with that one.
The second one downloads images and put them in a directory, of course the directory doesn't exist at the time. So let's say if I have 3 images, I will have these :
"pagetitle/1.jpg"
"pagetitle/2.jpg"
"pagetitle/3.jpg"
That allows me to have the images of the apps, in the same directory named "pagetitle"

Comment 15

[Umair Ahmed]

I have been facing this bug through an extension, Image Picka, with the following use case:

The extension allows batch downloading of images sourced from a single tab or multiple tabs.
It puts the files inside a folder. It can be multi-level.
The folder is named by the title of the tab from where the operation was initiated.

A directory is not being downloaded but files are being downloaded to a directory created by the extension at the configured Firefox download location.

Comment 16

[Rob Wu [:robwu]]

(In reply to :Gijs (he/him) from comment #11)

Actually, I'm a bit confused re-reading this bug and particularly comment #9. How would a user download a directory? :-\

The downloads.download extension API accepts a path separator in the filename option, which instructs the browser to save to that directory (within the default downloads directory). In the extension API implementation, we are currently relying on DownloadPaths.sanitize to not only verify that the leaf (file name) is valid, but using the same for the directory name: https://searchfox.org/mozilla-central/rev/b368ed8b48c0ea8ed2f1948e4776a6fbb5976dff/toolkit/components/extensions/parent/ext-downloads.js#674-677,691-713

It looks like the DownloadPaths.sanitize method has diverged from the original implementation, to the point that it is overcautious and sanitizes file names that are not unsafe.

Would it be feasible to change the underlying implementation of DownloadPaths.sanitize to NOT strip whitespace when compressWhitespaces: false is used? Alternatively, a new option to stop stripping whitespace after a dot.

Comment 17

[:Gijs (out for now; he/him)]

(In reply to Rob Wu [:robwu] from comment #16)

(In reply to :Gijs (he/him) from comment #11)

Actually, I'm a bit confused re-reading this bug and particularly comment #9. How would a user download a directory? :-\

The downloads.download extension API accepts a path separator in the filename option, which instructs the browser to save to that directory (within the default downloads directory). In the extension API implementation, we are currently relying on DownloadPaths.sanitize to not only verify that the leaf (file name) is valid, but using the same for the directory name: https://searchfox.org/mozilla-central/rev/b368ed8b48c0ea8ed2f1948e4776a6fbb5976dff/toolkit/components/extensions/parent/ext-downloads.js#674-677,691-713

OK. Yeah, the intent of that function was never to validate directory names - it's just a wrapper around the mime service's functions to validate filenames for saving, which have explicit flags to even modify the extensions based on the mimetype, which make it (IMO) clear that it wasn't intended for use on directories.

It looks like the DownloadPaths.sanitize method has diverged from the original implementation, to the point that it is overcautious and sanitizes file names that are not unsafe.

I've CC'd you on the security bug. Off-hand, "overcautious" seems incorrect, but if you think there is some middle ground in the original implementation that would fix that security bug without causing this issue then please elaborate.

Would it be feasible to change the underlying implementation of DownloadPaths.sanitize to NOT strip whitespace when compressWhitespaces: false is used? Alternatively, a new option to stop stripping whitespace after a dot.

It sounds like the webextensions implementation needs this API to have an option that ignores all the file extension logic and only validates the directory name - although "what's left" at that point, I'm not sure. If there is some logic left then yes, I expect adding a flag to the mime service and calling that would be OK.

Comment 18

[Rob Wu [:robwu]]

Needinfo-ing myself - I need to figure out a better validation mechanism for directories that is not as strict as the file name validator. If needed I'll set up a meeting with Gijs, Neil and :pbz to discuss this.

Comment 19

[Neil Deakin]

I think for directories you just want to use sanitizeFileName() but with a flag to not do the extension truncation that happens mostly at the end of that function.

Comment 20

[Rob Wu [:robwu]]

I'm going to add a patch. I'm thinking of a generic "allow directory names" function. For now I'm focusing on solving the regression, but I can imagine the flag being expanded in the future to cover more cases.

Comment 21

[Rob Wu [:robwu]]

Attachment: Bug 1903780 - Support directory names again in downloads.download

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d1c243062c9
Support directory names again in downloads.download r=geckoview-reviewers,NeilDeakin,m_kato

Comment 23

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/7d1c243062c9

Comment 24

[Rob Wu [:robwu]]

Attachment: Bug 1903780 - Support directory names again in downloads.download

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

Comment 25

[Rob Wu [:robwu]]

Attachment: Bug 1903780 - Support directory names again in downloads.download

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

Comment 26

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: Extensions fail to save files to directories with spaces after the last dot. This was a regression in 128 (uplifted to 127).
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: QA not needed, but if desired STR are in the report.
• Risk associated with taking this patch: Low
• Explanation of risk level: The patch adds an option to opt in to exactly the desired behavior and uses that option in extension code only. There is also extensive test coverage for this option.
• String changes made/needed: No
• Is Android affected?: no

Comment 27

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: Extensions fail to save files to directories with spaces after the last dot. This was a regression on the ESR115 due to an uplift.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: QA not needed, but if desired STR are in the report.
• Risk associated with taking this patch: Low
• Explanation of risk level: The patch adds an option to opt in to exactly the desired behavior and uses that option in extension code only. There is also extensive test coverage for this option.
• String changes made/needed: No
• Is Android affected?: no

Comment 28

[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-firefox130 to wontfix.

For more information, please visit BugBot documentation.

Comment 29

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/f3bf1705bfa0

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/79e9d3eb5b1a

Comment 31

[Rob Wu [:robwu]]

Attachment: Bug 1903780 - Support directory names again in downloads.download

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

Comment 32

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Extensions fail to save files to directories with spaces after the last dot. This was a regression in 128 (uplifted to 127).
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: QA not needed, but if desired STR are in the report.
• Risk associated with taking this patch: Low
• Explanation of risk level: The patch adds an option to opt in to exactly the desired behavior and uses that option in extension code only. There is also extensive test coverage for this option.
• String changes made/needed: None
• Is Android affected?: no

Comment 33

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/05050104ec0c

Comment 34

[Alex Cornestean]

Verified as Fixed. Tested on the latest Nightly (131.0a1/20240821160637), Beta (130.0b9/20240821170443 from https://treeherder.mozilla.org/jobs?repo=mozilla-beta&revision=05050104ec0cb90fa4bebc6fdb01ee6bd7075b72), ESR 115 (115.15.0esr/20240821131223 from https://treeherder.mozilla.org/jobs?repo=mozilla-esr115&revision=79e9d3eb5b1a9ac9562c4f13f5194598f285e3c7) and ESR 128 (128.2.0esr/20240821131032 from https://treeherder.mozilla.org/jobs?repo=mozilla-esr128&revision=f3bf1705bfa0f923e4541e6cc9f9aada80b733d9) under Windows 10 x64 and Ubuntu 22.04 LTS.

Using the example extension linked in Comment 0, clicking the extension button will download two html files in two folders - 1. foo and 1.foo bar.
The Error: filename must not contain illegal characters error is no longer thrown.

Considering the above, the issue is fixed.