1637973 - browser.downloads.download file name with more than one space and illegal characters error

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[hurzon]

Attachment: sampleAddOn package it contains manifest.json and background.js

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

Steps to reproduce:

call browser.downloads.download() with filename with more than one space character (consecutive). For example { filename: "0 a/image.png" }: there are two spaces between "0" and "a" folder name.

I attached an simple addon package.

1. Install it, you can see "SAVE IMG FILE" context menu, and "SAVE" sub-menu on a image.
2. Click this sub-menu "SAVE", and it will try to download an image file (Google logo image) as "0 a/image.png", but it fails.

Actual results:

It says "Error: filename must not contain illegal characters" on javascript console.

Expected results:

An image file should be downloaded into the specified directory and file name.
At least, until ver. 75.0, it worked with two or more consecutive space characters in the file name.
By the way, if there is one space character (e.g. "0 a/image.png"), the image file is saved without any error.

Comment 1

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

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=99d6961f2d92932a5f48d5f12489a0fda60d0e73&tochange=45f786f133e9d35d2a4f429af23e5565bc55b42c

Comment 4

[:Gijs (he/him)]

This seems to be a result of the code at https://searchfox.org/mozilla-central/rev/3ce874dc2703831af3e5ef3a1d216ffd08057fa5/toolkit/components/extensions/parent/ext-downloads.js#581-584 rejecting any sanitized paths. I'm not sure why it does that. Presumably the webextension folks woul dknow.

The double space sanitization got added in bug 1598216 so that if we get sent a path with foo :bar in it, and we sub a space for :, we normalize the two consecutive spaces back to 1 space...

Comment 5

[Tomislav Jovanovic :zombie]

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

This seems to be a result of the code at https://searchfox.org/mozilla-central/rev/3ce874dc2703831af3e5ef3a1d216ffd08057fa5/toolkit/components/extensions/parent/ext-downloads.js#581-584 rejecting any sanitized paths. I'm not sure why it does that. Presumably the webextension folks woul dknow.

The idea is that if extension used a filename with invalid characters, we would throw. Since this is an actual api with explicitly passed filename, we prefer throwing an error instead of silently failing or normalizing it. We detect errors by checking if sanitized filename differs from the passed argument. This has mostly worked as described until bug 1598216.

The double space sanitization got added in bug 1598216 so that if we get sent a path with foo :bar in it, and we sub a space for :, we normalize the two consecutive spaces back to 1 space...

That sounds like good reasoning for the case when the original filename is invalid, but it doesn't looks like consecutive spaces normalization was intentional for already valid filenames. This is definitely not what we want for an explicit api like extensions, but I would also argue Firefox shouldn't be dropping consecutive spaces from otherwise valid filenames, they might be meaningful in some cases (just an anecdote, but my partner used to regularly download reports from a local government website which used leading whitespace for dates and case numbers, for proper sorting purposes).

Marco, what do you think?

Comment 6

[Marco Bonardo [:mak]]

I'm not sure how we'd distinguish a space generated from replacing an invalid char, from a normal space (without ugly double replacements). Even if we'd first check if there are invalid chars, there could still be multiple spaces too. So the question is pretty much whether we should compress whitespaces at all.
I think the most common case is making the file name more readable and usable to the user, rather than supporting exotic use-cases, like the example you pointed out, and compressing whitespaces goes that direction. For example posting a filename with multiple spaces on the Web compresses them into a single one (note how comment 0 itself had to specify the name contains 2 spaces); or typing into a shell, it's hard to distinguish the number of consecutive spaces to type.

I must note that to fix this specific case we could add an options argument to sanitize, like { compressWhitespaces: false } and WebExt could use that, while the rest of the code will keep compressing.
I don't have such strong feelings anyway, this is my opinion but if others think differently I'm fine to revert.

Comment 7

[hurzon]

Maybe I should not disturb the discussion here now, as I am not familiar with the API development policy on FF. But I would like to share my feeling as a user and reporter of this issue.

I understand the necessity of sanitizing the file name, and agree that it helps to generate human readable one for users. Besides, I also think that having multiple space characters in file name is a little bit strange.
What I originally want to do with this WebExt is to save a file in a directory which is in a few layers down of the directory hierarchy from the download root. Actually, the file name itself doesn't matter if the multiple spaces are compressed or not. But I care the intermediate directory names. I rarely see those directory names in console window. I usually see them through file manger software (e.g. Explorer), and adding extra spaces makes it easier for me to view / organize them on the window.

By the way, when I use file download dialog, there is no problem to save files in those directory with multiple space characters in their name. That is why I had a feeling that the WebExt might have additional file name policy, and it narrows the valid file name definition which is permitted on a OS. And if that is the case, I thought it might be good to report this situation to know how current developers think.

Anyway, if the option to suppress space compression becomes available, it will be great. But if it generate any discrepancy / difficulty for your future development, I will revise my directory names in order to use my extension on FF.

Comment 8

[Marco Bonardo [:mak]]

(In reply to hurzon from comment #7)

Anyway, if the option to suppress space compression becomes available, it will be great.

Just to clarify, my suggestion would be an internal option for our code, for your extension everything would return to work as before, you'd not need to provide any additional options.

Comment 9

[hurzon]

Thank you very much for your clarification. I imagined by myself that the option might be available through "about:config" tab.

Comment 10

[Tomislav Jovanovic :zombie]

(In reply to Marco Bonardo [:mak] from comment #6)

I'm not sure how we'd distinguish a space generated from replacing an invalid char, from a normal space (without ugly double replacements). Even if we'd first check if there are invalid chars, there could still be multiple spaces too.

I was thinking this was in JS, so something like replacing \s? \X ([\s \X]* \X)? \s? with a single space. It would be harder in C++ without regular expressions, but could be done.

Comment 11

[Marco Bonardo [:mak]]

It's a lot more complex, for a tiny benefit.

Comment 12

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

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

For more information, please visit auto_nag documentation.

Comment 13

[Tomislav Jovanovic :zombie]

(In reply to Marco Bonardo [:mak] from comment #11)

It's a lot more complex, for a tiny benefit.

Nod, that's a question of cost/benefit. I'll leave the more general decision about user downloads to you.

For the concrete case of extension compatibility, are you able to take this bug Marco?

Comment 15

[Tim Spurway [:tspurway]]

S1 or S2 bugs need an assignee - could you find someone for this bug?

Comment 16

[Marco Bonardo [:mak]]

I can try.

Comment 17

[Marco Bonardo [:mak]]

Attachment: Bug 1637973 - browser.downloads.download throws error with filenames having multiple spaces. r=zombie

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c88e66a73e6
browser.downloads.download throws error with filenames having multiple spaces. r=zombie

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5c88e66a73e6

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta78?

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 9154680 [details]
Bug 1637973 - browser.downloads.download throws error with filenames having multiple spaces. r=zombie

Beta/Release Uplift Approval Request

• User impact if declined: WebExtension downloads api doesn't act properly, some valid downloads may fail
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: The automated test is enough.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very simple patch with a test
• String changes made/needed:

Comment 22

[Julien Cristau [:jcristau]]

Comment on attachment 9154680 [details]
Bug 1637973 - browser.downloads.download throws error with filenames having multiple spaces. r=zombie

approved for 78.0b6

Comment 23

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1309c5e2c8c6