1779128 - Bookmark shortcut file creation broken with long page titles

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P2)

Description

[tettee.green]

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

Steps to reproduce:

(I am aware of https://bugzilla.mozilla.org/show_bug.cgi?id=1774683 , but this is a niche situation not fixed by that fix, so I am filing a separate bug for this)

OS: windows 7
1. Went to this twitter post (light nsfw) https://twitter.com/wahiko94/status/991253919534755840
1b. or alternately change the current page title via the console with:

top.document.title = "わひこ@C100土曜西さ13a on Twitter: \"GHost9Solidのノウハウを生かした次回作案②「血〇〇法」、発覚されていない状態で敵に近づき特定のボタンを押すと血〇〇法のモーションが発動、首筋に柔らかい感触を感じ敵は××されてしまう https://t.co/gM552Fqmza\" / Twitter"

or alternately

top.document.title = " \"GHd触を感じ敵は××されてしまう https://t.co/gza\" / Tr----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------"

I have tried editing the title to see what works and what doesn't and it seems to be related to title length. If the title is shorter, then the fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1774683 kicks in and it works fine.

2. Click and drag padlock in URL bar to file explorer, release mouse button.

Actual results:

It made an empty folder called
わひこ@C100土曜西さ13a on Twitter GHost9Solidのノウハウを生かした次回作案②「血〇〇法」、発覚されていない状態で敵に近づき特定のボタンを押すと血〇〇法のモーション.co
or with the alternate title I tried it made an empty folder called

GHd触を感じ敵は××.co

In addition windows pops up an error message:

"[Path]\[name of created folder]\[extra bit that also came from the title] is not accessible.
The filename, directory name, or volume label syntax is incorrect."```


Expected results:

A normal bookmark shortcut should have been created.

Comment 1

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

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

Comment 2

[Alice0775 White]

I can reproduce the issue on Nightly104.0a1, 103.0b7 and 102.0.1 Windows10.

Comment 3

[:Gijs (he/him)]

Thanks for filing this with these clear steps!

Confirmed this is also a regression from bug 1746052.

Comment 4

[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 5

[Neil Deakin]

The issue here is that the page title contains a period so the part after that is being treated as an extension. That's not a big issue but the extension also contains a slash that ends up not getting sanitized because the page title is too long and is being truncated.

This also uses VALIDATE_SANITIZE_ONLY as there is no known content type so the extension is never 'fixed'.

So there could be several parts that could be fixed here:

• if a page title is being used to form the filename, strip out any periods beforehand.
• get the extension from the filename after sanitizing it, then truncate the filename, then append the sanitized extension again.

Comment 6

[Monica Chiorean]

I was able to reproduce the issue on Win7 using Beta 102.0b1 (20220530132252) and using steps from description -> 1b alternately.
Issue still reproduces on Win7 using Beta 103.0b8 (20220212185923) and Nightly 104.0a1(20220713215226).

Comment 7

[Neil Deakin]

Attachment: Bug 1779128, rework filename sanitization to ensure that the extension is properly validated, r=gijs

The existing code doesn't use the sanitized extension part of the filename, when it replaces the filename when it is too long but instead used a version passed to SanitizeFileName. This newer version always gets the extension from the filename after it has been validated.

Some tests have slightly different results, because the file is now cropped slightly differently when the character count and byte count of long filenames don't match, resulting in some filenames being cropped a few extra characters more than needed.

Comment 8

[Neil Deakin]

Attachment: Bug 1779128, rework filename sanitization to ensure that the extension is properly validation, r=gijs

The existing code doesn't use the sanitized extension part of the filename, when it replaces the filename when it is too long but instead used a version passed to SanitizeFileName. This newer version always gets the extension from the filename after it has been validated.

Some tests have slightly different results, because the file is now cropped slightly differently when the character count and byte count of long filenames don't match, resulting in some filenames being cropped a few extra characters more than needed.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93cfe80ec8a9
rework filename sanitization to ensure that the extension is properly validated, r=Gijs

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/93cfe80ec8a9

Comment 11

[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 12

[Neil Deakin]

Comment on attachment 9285482 [details]
Bug 1779128, rework filename sanitization to ensure that the extension is properly validated, r=gijs

Beta/Release Uplift Approval Request

• User impact if declined: Any page with a title containing a period and slash ends up not being saved correctly when dragged to the file system.
• 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:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Reworks quite a bit of the filename sanitization code, but is covered by extensive automated tests
• String changes made/needed: None
• Is Android affected?: No

Comment 13

[Dianna Smith [:diannaS]]

Comment on attachment 9285482 [details]
Bug 1779128, rework filename sanitization to ensure that the extension is properly validated, r=gijs

Approved for 104.0b3

Comment 14

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2e9419d45bcd

Comment 15

[Alexandru Trif, Desktop QA [:atrif]]

Reproduced the issue using STR from comment 0 with Firefox 104.0a1 (20220711215641) on Windows 10x64 and Windows 7x64. The bookmark shortcut is not created after being dragged to file explorer.
The issue is verified fixed with Firefox 104.0b3 (20220728185815) and 105.0a1 (20220728215301) on Windows 10x64 and Windows 7x64. The bookmark shortcut is successfully created while being dragged to explorer after following STR from comment 0.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Neil Deakin]

Comment on attachment 9285482 [details]
Bug 1779128, rework filename sanitization to ensure that the extension is properly validated, r=gijs

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Any page with a title containing a period and slash ends up not being saved correctly when dragged to the file system.
• User impact if declined:
• Fix Landed on Version: 104
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Reworks quite a bit of the filename sanitization code, but is covered by extensive automated tests

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9285482 [details]
Bug 1779128, rework filename sanitization to ensure that the extension is properly validated, r=gijs

Approved for 102.2esr.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/5b5eec241e1f

Comment 20

[Alexandru Trif, Desktop QA [:atrif]]

Verified fixed with Firefox 102.2.0esr (20220815170108) on Windows 10x64 and Windows 7x64. The bookmark shortcut is successfully created while being dragged to explorer after following STR from comment 0.