Closed Bug 1779128 Opened 2 years ago Closed 2 years ago

Bookmark shortcut file creation broken with long page titles

Categories

(Firefox :: File Handling, defect, P2)

Firefox 102
Desktop
All
defect

Tracking

()

VERIFIED FIXED
105 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox-esr102 --- verified
firefox102 --- wontfix
firefox103 --- wontfix
firefox104 --- verified
firefox105 --- verified

People

(Reporter: tettee.green, Assigned: enndeakin)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.

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.

Component: Untriaged → Disability Access APIs
Product: Firefox → Core
Component: Disability Access APIs → File Handling
Product: Core → Firefox

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

Status: UNCONFIRMED → NEW
Ever confirmed: true

Thanks for filing this with these clear steps!

Confirmed this is also a regression from bug 1746052.

Blocks: 1778322
Keywords: regression
OS: Unspecified → All
Regressed by: 1746052
Hardware: Unspecified → Desktop

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

Flags: needinfo?(enndeakin)

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.
Flags: needinfo?(enndeakin)

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).

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.

Assignee: nobody → enndeakin
Status: NEW → ASSIGNED

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.

Attachment #9285711 - Attachment is obsolete: true
Severity: -- → S3
Priority: -- → P2
Attachment #9285482 - Attachment description: Bug 1779128, rework filename sanitization to ensure that the extension is properly validation, r=gijs → Bug 1779128, rework filename sanitization to ensure that the extension is properly validated, r=gijs
Pushed by neil@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/93cfe80ec8a9 rework filename sanitization to ensure that the extension is properly validated, r=Gijs
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch

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.

Flags: needinfo?(enndeakin)
Blocks: 1778429

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
Flags: needinfo?(enndeakin)
Attachment #9285482 - Flags: approval-mozilla-beta?

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

Approved for 104.0b3

Attachment #9285482 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

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

Flags: needinfo?(enndeakin)

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
Flags: needinfo?(enndeakin)
Attachment #9285482 - Flags: approval-mozilla-esr102?

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

Approved for 102.2esr.

Attachment #9285482 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+

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.

QA Whiteboard: [qa-triaged]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: