1789500 - Emoji at the end of a filename or a directory name is treated as invalid character when downloading

Status: VERIFIED FIXED

(Firefox :: File Handling, defect)

Description

[:ytoku]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:104.0) Gecko/20100101 Firefox/104.0

Steps to reproduce:

In a WebExtensions Add-on,
download a file by downloads.download() with a filename which ends with emoji such as 😀.

browser.downloads.download({
...
filename: 'path/to/a😀',
})

A directory name which ends with emoji also cause the problem.

browser.downloads.download({
...
filename: 'path/a😀/a.txt',
})

The probrem does not occur with a filename with a suffix such as a😀.txt.

Actual results:

downloads.download() rejects trailing emoji characters and throws an exception:
Error: filename must not contain illegal characters

Expected results:

downloads.download() accepts the specified filename and saves the file as the specified filename.

Comment 1

[:ytoku]

I already reported the problem in bug 1778880 but the previous issue has marked as duplicated of bug 1778429 and partially fixed.
So I have reported again the remaining problem.

Comment 2

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

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

Comment 3

[:ytoku]

I confirmed that the problem is introduced in Firefox 102. (https://bugzilla.mozilla.org/show_bug.cgi?id=1778880#c5)

For your information, a procedure by accessing to the page and downloading like bug 1778429 also causes the problem.

1. Prepare a webserver provides a file whose filename ends with emoji such as 😀, for example a😀.
2. Access to the file
3. Ctrl-S

Actual results:

Downloaded filename becomes a

Actual results:

Downloaded filename becomes a😀

Comment 4

[Marco Bonardo [:mak]]

the downloads.download() API uses DownloadPaths.sanitize() for each component of the path, that goes through validateFileNameForSaving and finally in nsExternalHelperAppService::SanitizeFileName. So it's likely a regression from bug 1746052.
If I call from the console:

Cc["@mozilla.org/mime;1"].getService(Ci.nsIMIMEService).validateFileNameForSaving('a😀', "", Ci.nsIMIMEService.VALIDATE_SANITIZE_ONLY);

I get back "a\ud83d", while the emoji should be \ud83d\ude00 and the comparison will fail.
It sounds like some unexpected trimming?

Comment 5

[Marco Bonardo [:mak]]

I can try to debug this.

Comment 6

[Marco Bonardo [:mak]]

lastNonTrimmable/Truncate calculation seems to be off, not considering surrogate pairs

Comment 7

[Marco Bonardo [:mak]]

Attachment: Bug 1789500 - filename sanitization may cut surrogate pairs when truncating. r=neildeakin

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5cd1b8fabca7
filename sanitization may cut surrogate pairs when truncating. r=NeilDeakin

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 9294972 [details]
Bug 1789500 - filename sanitization may cut surrogate pairs when truncating. r=neildeakin

Beta/Release Uplift Approval Request

• User impact if declined: Certain intl file names may fail to sanitize and the WebExt download api will fail too in that case.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Steps are in the bug
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): fixing a simple utf16/ucs4 char size mistake
• String changes made/needed: no
• Is Android affected?: Unknown

Comment 10

[Marian-Vasile Laza]

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

Comment 11

[:ytoku]

Thanks!
Would you backport the patch to esr102?

Comment 12

[Alex Cornestean]

Hello,

Verified as fixed on the latest Nightly (106.0a1/20220918214503) under Windows 10 x64 and Ubuntu 16.04 LTS.

To verify the issue, a web server running on localhost has been set up to provide the example “a😀” file. The fix has been verified via both simply clicking on the file to be downloaded and using a download manager extension (DownThemAll!).

In both cases, the file is properly downloaded as “a😀” and no errors are logged to console, confirming the fix. For more details see the attached screenshot.

Comment 13

[Alex Cornestean]

Attachment: 2022-09-19_11h48_40.png

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9294972 [details]
Bug 1789500 - filename sanitization may cut surrogate pairs when truncating. r=neildeakin

Morphing into a mozilla-release uplift request as this landed in the 106 cycle and mozilla-beta is now 106.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 9294972 [details]
Bug 1789500 - filename sanitization may cut surrogate pairs when truncating. r=neildeakin

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Downloading addons may be affected and replace valid chars in filenames with invalid ones
• User impact if declined: downloads with wrong name
• Fix Landed on Version: 106
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fixing an UCS4 char size mistake, simple fix.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9294972 [details]
Bug 1789500 - filename sanitization may cut surrogate pairs when truncating. r=neildeakin

Approved for 102.4esr.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/43921fc5e6d3

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9294972 [details]
Bug 1789500 - filename sanitization may cut surrogate pairs when truncating. r=neildeakin

This feels pretty edge-case to me. I think we can let this ride 106 versus taking it in a dot release.

Comment 19

[Alex Cornestean]

Verified as Fixed on the latest ESR (102.4.0esr/20221010114559) under Windows 10 x64 and Ubuntu 16.04 LTS.

As per the same steps performed in Comment 12, the file is served from a web server running on localhost and then downloaded both via simply clicking on it and a download manager extension. In both cases, the file is properly downloaded as “a😀” and no errors are logged to console, confirming the fix.

For further details, see the attached screenshot.

Comment 20

[Alex Cornestean]

Attachment: 2022-10-11_10h24_10.png