Open Bug 1906455 Opened 3 months ago Updated 28 days ago

Dragging a URL to windows file system can create files with too long filenames

Categories

(Firefox :: File Handling, defect, P3)

Firefox 127
Desktop
Windows
defect

Tracking

()

Tracking Status
firefox-esr115 --- wontfix
firefox-esr128 --- affected
firefox128 --- wontfix
firefox129 --- wontfix
firefox130 --- wontfix
firefox131 --- fix-optional

People

(Reporter: tore, Unassigned, NeedInfo)

References

(Regression)

Details

(Keywords: parity-chrome, regression)

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

Steps to reproduce:

When I visit https://github.com/paolosalvatori/servicebusexplorer

I want to save a copy of the URL into a local Windows 10 folder. I drag the URL and a shortcut is created. Great! That works!

Actual results:

But the file name is too long! Due to the length, If I try to double click on the URL to open the page in firefox, then Windows complains that can't/won't open it.

when trying to add the file in Git, also fails

Z:\GitHubTN\azure\Azure for .NET developers\Module 16 - Event and Message driven applications\30 - Azure Service Bus>git add .
error: open("Azure for .NET developers/Module 16 - Event and Message driven applications/30 - Azure Service Bus/paolosalvatori_ServiceBusExplorer_ The Service Bus Explorer allows users to connect to a Service Bus namespace and administer messaging entities in an easy manner. The tool provides advanced features like import_export functionality or the ability to.url"): Filename too long
error: unable to index file 'Azure for .NET developers/Module 16 - Event and Message driven applications/30 - Azure Service Bus/paolosalvatori_ServiceBusExplorer_ The Service Bus Explorer allows users to connect to a Service Bus namespace and administer messaging entities in an easy manner. The tool provides advanced features like import_export functionality or the ability to.url'
fatal: adding files failed

Expected results:

I would expect that FireFox would truncate the file name (based on the page title), so that the total length including the path is less than the max path/file length in Windows.

Google Chrome handles this much better , it limit the file name to

GitHub - paolosalvatori-ServiceBusExplorer- The Service Bus Explorer allows users to connect to a Service Bus namespace and a.url

While Firefox creates this URL:

paolosalvatori_ServiceBusExplorer_ The Service Bus Explorer allows users to connect to a Service Bus namespace and administer messaging entities in an easy manner. The tool provides advanced features like import_export functionality or the ability to.url

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

Component: Untriaged → Widget: Win32
Product: Firefox → Core

We actually do truncate the alleged filename to MAX_PATH characters [0]... but that won't help when the file is in any directory, and especially not a directory whose path is itself 99 characters long. Chrome appears to truncate to approximately MAX_PATH/2 characters, which is probably a reasonable approach.

Note that, as a workaround, the file can be renamed from File Explorer by selecting it and pressing F2, or equivalently by right-clicking it and selecting "Rename".


[0] See: [a], [b], [c].

Status: UNCONFIRMED → NEW
Component: Widget: Win32 → File Handling
Ever confirmed: true
Keywords: parity-chrome
Product: Core → Firefox

Could it not adjust the filename based on the current path so that its total length and the path are within the limits?

When the file name gets too long, things start to break. You can't click on it, and you can't rename it in some cases, in some cases it can't be deleted.

so it's pretty hard to get rid of when the file name is too long.

All I want is to avoid any surprises, regardless of the current path depth :-)

Could it not adjust the filename based on the current path so that its total length and the path are within the limits?

Unfortunately, I don't believe it can. Windows doesn't tell us what the target path is; it just tells us that whatever we're dropping the object onto wants a file.

Ray, you moved this to file handling - where does the code that provides a filename to Windows for the drag/drop actually live? Although we could change the URL bar code to e.g. produce a shorter title in the x-moz-url format, (a) that would not be great for other drags (that don't result in url files) and (b) that would not fix other drag sources.

I would have thought this stuff lives in widget's nsDragService? I can totally believe it's somewhere else - but I'm not sure how to find out, and it sounds like you already dug into this a bit.

Component: File Handling → Address Bar
Flags: needinfo?(rkraesig)

Actually, Marco found https://searchfox.org/mozilla-central/rev/91f6127b6f591da4037821791c345147d9a575da/widget/windows/nsDataObj.cpp#1203,1244 and that lives in widget. So I'm tentatively moving this back to widget but will keep the needinfo.

Component: Address Bar → Widget: Win32
Product: Firefox → Core

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

Ray, you moved this to file handling - where does the code that provides a filename to Windows for the drag/drop actually live?

The code you linked is the code that provides the filename to Windows, yes.

The code that provides the filename to that code, however, is highlighted in my previous comment...

[0] See: [a], [b], [c].

... and exists in uriloader/exthandler/nsExternalHelperAppService.cpp.

The functions Marco noted both call ValidateFilename(), which invokes nsIMIMEService::ValidateFileNameForSaving, which is implemented by nsExternalHelperAppService by invoking a different overload of ValidateFileNameForSaving, which invokes SanitizeFilename, which is the function I linked as [a], above.

I have no reason to believe that the calls in nsDataObj.cpp are the only invocations of ValidateFileNameForSaving which could result in this issue.

Flags: needinfo?(rkraesig) → needinfo?(gijskruitbosch+bugs)

Neil is probably best-placed to suggest what should happen here, then. validateFilenameForSaving also does not take a target directory so it still cannot really reliably do better in terms of figuring out MAX_PATH constraints. I would anticipate that if that helper ended up overwriting user-chosen filenames to make them fit in MAX_PATH / 2, that would upset people.

What I suspect may need to happen is (a) allow consumers to pass an nsIFile or similar for the containing path, if available (which I recognize it's not in this case) and (b) whether or not the filename was user-chosen (similar to how we handle aAfterFilePicker). Then non-user-chosen filenames can be compromised in length based on the target location (if available, and MAX_PATH / 2 if not), and we can leave others alone.

I also confess that I am confused about what exactly Windows' restrictions are here. Why does it allow saving files that it then cannot deal with? Are the issues in comment 0 just other pieces of software that do not deal with extended filenames, given Windows Explorer is fine?

Component: Widget: Win32 → File Handling
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(enndeakin)
Product: Core → Firefox

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

I also confess that I am confused about what exactly Windows' restrictions are here. Why does it allow saving files that it then cannot deal with? Are the issues in comment 0 just other pieces of software that do not deal with extended filenames, given Windows Explorer is fine?

Mostly, yes. MAX_PATH is not a hard limit; I know of at least three ways around it. Some relevant documentation can be found here, here, and here.

(In reply to Ray Kraesig [:rkraesig] from comment #9)

I know of at least three ways around it.

It does not help unless File Explorer uses them. Quoting from the bug description:

If I try to double click on the URL to open the page in firefox, then Windows complains that can't/won't open it.

Also we can't force users to use the registry hack. It may be possible that we limit the filename length only if the registry setting is disabled.

We used to truncate the filename to 128 characters (see bug 250392). This commit effectively removed this limit.

Keywords: regression
Regressed by: CVE-2022-46874

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

The simplest would be to just set the maximum filename length to something shorter on Windows, but passing a nsIFile target directory would also be an option.

Flags: needinfo?(enndeakin)

but passing a nsIFile target directory would also be an option.

...except that it is impossible for Drag and Drop case. See comment #4.

:mkmelin, since you are the author of the regressor, bug 1746139, could you take a look?

For more information, please visit BugBot documentation.

Flags: needinfo?(mkmelin+mozilla)

(Not really my patch).
For Thunderbird we set the max length to 1/3 of maximum, https://phabricator.services.mozilla.com/D158825, and I haven't heard any complaints.

Flags: needinfo?(mkmelin+mozilla)

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

The severity field is not set for this bug.
:Gijs, could you have a look please?

For more information, please visit BugBot documentation.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Ray Kraesig [:rkraesig] from comment #7)

I have no reason to believe that the calls in nsDataObj.cpp are the only invocations of ValidateFileNameForSaving which could result in this issue.

I expect that in a lot of cases (normal downloads / "save page as", etc.) the filename will already have passed through a Windows file picker dialog and that (based on the earlier discussion) likely guarantees this can't be an issue as the picker presumably does not let you choose files that Windows Explorer doesn't know how to deal with.

Hardcoding a limit of 128 for all files (inside ValidateFileNameForSaving) would probably break some user workflows, so I don't think we should do only that. So I see 3 options:

  1. add the 128 limit back to the drag/drop code specifically
  2. add a 128 limit to ValidateFileNameForSaving with a flag to "escape" it, then audit all the callpaths and add this flag to any consumers where the leafname is already known to be below the MAX_PATH when combined with the destination (ie the filepicker case outlined above)
  3. add a 128 limit to ValidateFileNameForSaving and an ability to pass an nsIFile or full path, and only use the 128 limit if no full path / nsIFile is present. If more info is present, use that to limit the file name.

The second 2 options are probably "better", but also significantly more work.

Neil or Ray, I don't suppose you have cycles to work on this? If not, I can probably write a patch for (1) but don't have cycles to pursue 2 or 3 at the moment.

Severity: -- → S3
Flags: needinfo?(rkraesig)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(enndeakin)
OS: Unspecified → Windows
Priority: -- → P3
Hardware: Unspecified → Desktop

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

I expect that in a lot of cases (normal downloads / "save page as", etc.) the filename will already have passed through a Windows file picker dialog and that (based on the earlier discussion) likely guarantees this can't be an issue as the picker presumably does not let you choose files that Windows Explorer doesn't know how to deal with.

I'm not sure why you're presuming that, because the picker will absolutely let you do that. It has a couple of checks but they don't cover the whole possibility space.

Neil or Ray, I don't suppose you have cycles to work on this

I don't, sadly. Even if I did, I'd probably just go with 1) — I don't think 2) or 3) are worth the effort, especially given comment #16.

Flags: needinfo?(rkraesig)

(In reply to Ray Kraesig [:rkraesig] from comment #21)

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

I expect that in a lot of cases (normal downloads / "save page as", etc.) the filename will already have passed through a Windows file picker dialog and that (based on the earlier discussion) likely guarantees this can't be an issue as the picker presumably does not let you choose files that Windows Explorer doesn't know how to deal with.

I'm not sure why you're presuming that, because the picker will absolutely let you do that. It has a couple of checks but they don't cover the whole possibility space.

Windows' own filepicker doesn't enforce MAX_PATH and then produces unusable files? 😭

(The code I linked in comment 20 would still enforce stuff for temp files for downloads; I haven't looked to see if we have similar restrictions around the final file though I would naively expect us to... basically, by the law of numbers, and how many people drag-to-save vs download files, if this was broken for downloads in general, all the time, I'd have expected us to hear about it well before now. Ditto for "save page as".)

I don't, sadly. Even if I did, I'd probably just go with 1) — I don't think 2) or 3) are worth the effort, especially given comment #16.

OK, that's helpful input, thank you!

You need to log in before you can comment on or make changes to this bug.