1791530 - Webextensions cannot sanitize filenames before calling downloads.download which causes downloads to fail unpredictably

Status: NEW

(WebExtensions :: General, defect, P3)

Description

[Marco Castelluccio [:marco]]

STR:

1. Open a PDF such as https://bug1782738.bmoattachments.org/attachment.cgi?id=9288118
2. Right click, select "Take screenshot" from the context menu
3. Select an area
4. Click on "Download"

Expected results:
An image should be downloaded with the contents of the selected area.

Actual results:
Nothing happens.

Comment 1

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

:enndeakin, since you are the author of the regressor, bug 1746052, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 2

[Marco Bonardo [:mak]]

multiprocess console shows

filename must not contain illegal characters main.js:202:17
    <anonima> moz-extension://8dbb8cc2-45ea-4ece-a911-66634460ecce/background/main.js:202

It uses the downloads.download API that compares the provided filename with the sanitized version and fails if they differ...

The name of the file is
Screenshot 2022-09-20 at 11-54-11 Anlage 2 zum Vertrag über die Versorgung der Versicherten mit zum Verbrauch be­stimm­ten Pflegehilfsmitteln gem. § 78 Absatz 1 i. V. m. § 40 Absatz 2 SGB XI - testPhm.pdf.png
Sanitized version is
Screenshot 2022-09-20 at 11-54-11 Anlage 2 zum Vertrag über die Versorgung der Versicherten mit zum Verbrauch be_stimm_ten Pflegehilfsmitteln gem. § 78 Absatz 1 i. V. m. § 40 Absatz 2 SGB XI - testPhm.pdf.png

The difference is in bestimmten.
Not sure what's best approach, the add-on cannot use sanitize in its context, downloads.download maybe should always sanitize and return the sanitized name to the caller instead of rejecting the name? Otherwise we could sanitize in sshots, but risk more cases in the future.

Comment 3

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

Set release status flags based on info from the regressing bug 1746052

Comment 4

[:Gijs (he/him)]

I think this should be fixed in webextensions code. Screenshots is moving away from extensions so the specific issue here is going to be fixed that way, but this is likely to affect other add-ons.

Comment 5

[Tomislav Jovanovic :zombie]

Quoting Rob Wu from bug 1744779 comment #6

Throwing an error for invalid file names is intentional, and also consistent with what Chrome does (Chrome is even stricter on the permitted file names (source)).

I don't see value in offering a new extension method to sanitize file paths, because there are hardly any extension APIs to operate on files / actual file names. If anything, a new optional option to automatically offer sanitization of file names would make more sense. But unless this is adopted by other browsers, extensions would still need to do error handling and include a fallback for sanitizing the file name.

Comment 6

[Donal Meehan [:dmeehan]]

Looks like it's too late for 107.
Leaving 108 status clear, unless anyone thinks we should be following this for 108?

Comment 7

[Rob Wu [:robwu]]

I see a potential resolution here that does not require API changes nor changes in extensions.

The downloads.download API has the conflictAction parameter that is documented have the following values:

• "uniquify" - The browser will modify the filename to make it unique.
• "overwrite" - The browser will overwrite the old file with the newly-downloaded file.
• "prompt" - The browser will prompt the user, asking them to choose whether to uniquify or overwrite. (not implemented in Firefox)

The default conflictAction is "uniquify", which could be interpreted as the extension not caring about the specific name, as long as it is somewhat reasonable.

We could continue to strictly enforce the validation when conflictAction is "prompt" but try to more liberally interpret "uniquify" as approval to mangle the filename to something else.

P.S. For comparison, Chrome's default behavior is already to mangle file names (independently of the conflictAction parameter).

Comment 8

[Rob Wu [:robwu]]

We discussed this in last week's office hours and approved of automatically fixing up filenames, e.g. as described in comment 7.