1778429 - Half-width kana filenames are corrupted when saving

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P2)

Description

[note6673]

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

Steps to reproduce:

1. Access to the test file with half-width katakana in the filename https://pastebin.com/QJSFnU4Z on Firefox 102
2. Click the "download"

Alternative flow:

1. Access to the test file with half-width katakana in the filename https://pastebin.com/QJSFnU4Z on Firefox 102
2. Ctrl + S

Actual results:

Downloaded filenames are "test_ ｽ _TEST_テスト.txt".

Expected results:

Downloaded filenames are "test_ﾃｽﾄ_TEST_テスト.txt".

This bug is not reproduced in firefox 101.0.1.
I believe this is due to bug 1746052.

Comment 1

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

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

Comment 2

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

:enndeakin, since you are the author of the regressor, bug 1746052, could you take a look?
For more information, please visit auto_nag documentation.

Comment 3

[Alice0775 White]

[Tracking Requested - why for this release]:Half-width kana filenames are corrupted when saving

I can reproduce the issue on Nightly104.0a1 , Firefox 103.0b5, 102.0.1, 102.0esr Windows10.

Comment 4

[Neil Deakin]

This is because FILE_ILLEGAL_CHARACTERS is a single-byte string but is being compared using a utf16 string.

Comment 6

[Neil Deakin]

It is likely that 1772006 will fix this.

Comment 7

[:ytoku]

Please consider escalating severity because Bug 1778880 has no workaround.
In WebExtension, an add-on controls the filename then a user cannot fix the filename and just get a error that the file cannot be saved.

Comment 8

[mcdamo]

Note this validation bug is not limited to half-width kana, some other examples being rejected by downloads.download() are U+00A0 (no-break space) and 🏴‍☠️ (Pirate flag)

Comment 9

[:Gijs (he/him)]

Nika, are the patches in bug 1772006 likely to reland soon?

Neil, is there a more short-term solution we could employ here?

Comment 11

[Neil Deakin]

A short-term fix is to make utf16 copies of FILE_ILLEGAL_CHARACTERS and other constants that are used.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

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

Nika, are the patches in bug 1772006 likely to reland soon?

I haven't had the time to properly rebase & then fix any bitrot issues on them yet, so I don't know how much work that's going to be to get them 100% ready to land. I can try to up the priority a bit though - it's been at the back of my todo list for a while

That being said, I don't entirely know how my patches from that bug will fix any issues here, as I don't remember changing the semantics of any methods around FILE_ILLEGAL_CHARACTERS in my patches (as the existing code does utf-16 character-by-character replacements, even when passed narrow strings IIRC).

Comment 13

[Neil Deakin]

Attachment: Bug 1778429, use utf-16 character literals when calling ReplaceChar in SanitizeFileName, r=gijs

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0b4c0e343ef
use utf-16 character literals when calling ReplaceChar in SanitizeFileName, r=Gijs

Comment 15

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/e0b4c0e343ef

Comment 16

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

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

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

For more information, please visit auto_nag documentation.

Comment 17

[:Gijs (he/him)]

Comment on attachment 9286919 [details]
Bug 1778429, use utf-16 character literals when calling ReplaceChar in SanitizeFileName, r=gijs

Beta/Release Uplift Approval Request

• User impact if declined: Broken filename sanitization
• 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: See comment 0 and the dupe.
• List of other uplifts needed: bug 1779128 (or resolve conflicts)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): 2-line change with automated test coverage
• String changes made/needed: No
• Is Android affected?: Unknown

Comment 18

[Neil Deakin]

Note that the patch as is won't apply without 1779128.

Comment 19

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #18)

Note that the patch as is won't apply without 1779128.

Sorry! Updated the beta approval request.

Comment 20

[Dianna Smith [:diannaS]]

Comment on attachment 9286919 [details]
Bug 1778429, use utf-16 character literals when calling ReplaceChar in SanitizeFileName, r=gijs

Approved for 104.0b3

Comment 21

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/728103b792c6

Comment 22

[Bogdan Maris, Desktop Test Engineering]

Reproduced the issue using an old Nightly from 2022-07-06, verified that this issue is fixed using the steps from comment 0 and using Download All add-on (no error saying that the characters are invalid) on Latest Nightly 105.0a1 and Firefox 104.0b3 across platforms (Windows 10, macOS 11.6 and Ubuntu 18.04). Leaving the status to RESOLVED until the patch reaches ESR as well.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR102 approval when you're comfortable doing so. It grafts cleanly.

Comment 24

[Neil Deakin]

Comment on attachment 9286919 [details]
Bug 1778429, use utf-16 character literals when calling ReplaceChar in SanitizeFileName, r=gijs

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Some pages containing certain characters (typically japanese characters) aren't sanitized properly when saving/dragging to desktop
• User impact if declined: File not saved correctly
• Fix Landed on Version: 104
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minor fix to handle UTF16 characters correctly.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9286919 [details]
Bug 1778429, use utf-16 character literals when calling ReplaceChar in SanitizeFileName, r=gijs

Approved for 102.2esr.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/f2bffe3964ae

Comment 27

[Bogdan Maris, Desktop Test Engineering]

Also verified this is fixed using latest esr102 build from treeherder (https://treeherder.mozilla.org/jobs?repo=mozilla-esr102&tochange=18fa1f585ac2f5d0c1d620581814df29d3267db8&fromchange=f95a172405adace8207bd40df4643fab779255f3) across platforms (Windows 10 64bit, macOS 11.6 and Ubuntu 18.04).