Bookmark shortcut file creation broken with long page titles
Categories
(Firefox :: File Handling, defect, P2)
Tracking
()
People
(Reporter: tettee.green, Assigned: enndeakin)
References
(Blocks 1 open bug, Regression)
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
Bug 1779128, rework filename sanitization to ensure that the extension is properly validated, r=gijs
(deleted),
text/x-phabricator-request
|
diannaS
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr102+
|
Details |
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•2 years ago
|
||
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.
Updated•2 years ago
|
Comment 2•2 years ago
|
||
I can reproduce the issue on Nightly104.0a1, 103.0b7 and 102.0.1 Windows10.
Comment 3•2 years ago
|
||
Thanks for filing this with these clear steps!
Confirmed this is also a regression from bug 1746052.
Comment 4•2 years ago
|
||
:enndeakin, since you are the author of the regressor, bug 1746052, could you take a look?
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 5•2 years ago
|
||
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•2 years ago
|
||
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).
Assignee | ||
Comment 7•2 years ago
|
||
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.
Updated•2 years ago
|
Assignee | ||
Comment 8•2 years ago
|
||
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.
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 10•2 years ago
|
||
bugherder |
Comment 11•2 years ago
|
||
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
towontfix
.
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 12•2 years ago
|
||
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•2 years ago
|
||
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•2 years ago
|
||
bugherder uplift |
Updated•2 years ago
|
Updated•2 years ago
|
Comment 15•2 years ago
|
||
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•2 years ago
|
||
Please nominate this for ESR102 approval when you're comfortable doing so. It grafts cleanly.
Assignee | ||
Comment 17•2 years ago
|
||
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•2 years ago
|
||
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•2 years ago
|
||
bugherder uplift |
Comment 20•2 years ago
|
||
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.
Description
•