1744779 - Failing to download file due to missing filename sanitation

Status: VERIFIED FIXED

(Firefox :: Screenshots, defect, P2)

Description

[Andreas Wagner [:TheOne] [use NI]]

When trying to download a screenshot from a page whose title has certain non-allowed characters, saving on disk fails without any visual feedback.

STR:

1. Go to https://addons.mozilla.org/he/firefox/addon/multi-account-containers/ .
2. Use the Screenshots feature to a screenshot.
3. Click "Download".

Expected result:
The downloaded screenshot should be on the disk.

Actual result:
The screenshot file isn't on the disk.

Additional information:

• For RTL locales on addons.mozilla.org, the first character of the page title (which is used as part of the screenshot filename) is U+200F.
• The downloads API at https://searchfox.org/mozilla-central/source/toolkit/components/extensions/parent/ext-downloads.js#671 (which is called by https://searchfox.org/mozilla-central/source/browser/extensions/screenshots/background/main.js#207-212) rejects the promise because the sanitizer (https://searchfox.org/mozilla-central/source/toolkit/components/downloads/DownloadPaths.jsm#75-90) detected illegal characters, in this case U+200F.
• Screenshots doesn't check for the result of the promise and never notices that it got rejected.

Comment 1

[Sam Foster [:sfoster] (he/him)]

Thanks for filing this. I didn't find a dupe but I'm a little surprised we're just now finding this.

Comment 3

[Sam Foster [:sfoster] (he/him)]

I wonder if it would make sense to expose DownloadPaths.sanitize on the downloads extension API. Then this extension - and all the others - wouldn't have to guess what was ok and what was not.

Comment 4

[Sam Foster [:sfoster] (he/him)]

(In reply to Sam Foster [:sfoster] (he/him) from comment #3)

I wonder if it would make sense to expose DownloadPaths.sanitize on the downloads extension API. Then this extension - and all the others - wouldn't have to guess what was ok and what was not.

We are working towards porting Screenshots to a browser component, where this question becomes moot, but in the meantime - and for other extensions - I'm interested in options and opinions here. Yes, I can just copy/paste the relevant bits into the Screenshots code for now and that might be most expedient for here and now.

Comment 5

[Sam Foster [:sfoster] (he/him)]

(In reply to Sam Foster [:sfoster] (he/him) from comment #3)

I wonder if it would make sense to expose DownloadPaths.sanitize on the downloads extension API. Then this extension - and all the others - wouldn't have to guess what was ok and what was not.

Has there been any discussion of something like this, do you have your own thoughts :robwu?

Comment 6

[Rob Wu [:robwu]]

Throwing an error for invalid file names is intentional, and also consistent with what Chrome does (Chrome is even stricter on the permitted file names (source)).

I don't see value in offering a new extension method to sanitize file paths, because there are hardly any extension APIs to operate on files / actual file names. If anything, a new optional option to automatically offer sanitization of file names would make more sense. But unless this is adopted by other browsers, extensions would still need to do error handling and include a fallback for sanitizing the file name.

Comment 7

[Sam Foster [:sfoster] (he/him)]

(In reply to Rob Wu [:robwu] from comment #6)

I don't see value in offering a new extension method to sanitize file paths, because there are hardly any extension APIs to operate on files / actual file names. If anything, a new optional option to automatically offer sanitization of file names would make more sense. But unless this is adopted by other browsers, extensions would still need to do error handling and include a fallback for sanitizing the file name.

Ok that's useful context. For Screenshots, I'll just snag the important pieces from DownloadPaths.sanitize so we can generate filenames that we can be reasonably sure will work on the current OS, and pass the DownloadPaths.sanitize test.

Comment 8

[Neil Deakin]

Attachment: Bug 1744779, improve filename sanitization when selecting a filename for a screenshot, and change the filename length cropping to account for 255-byte filename limit on linux, r=sfoster

Comment 9

[Neil Deakin]

Sam, the issue with bug 1748784 is a different issue. It is caused because the filename is too long for a linux filesystem which is limited to 255 bytes for a filename.

But in that bug, you suggested that you had a fix for the filename length. Is that a fix we could include here?

Comment 10

[Sam Foster [:sfoster] (he/him)]

I think this was the important bit:

diff --git a/browser/extensions/screenshots/build/shot.js b/browser/extensions/screenshots/build/shot.js
--- a/browser/extensions/screenshots/build/shot.js
+++ b/browser/extensions/screenshots/build/shot.js
@@ -383,7 +383,7 @@ this.shot = (function() {
       const filenameDate = currentDateTime.substring(0, 10);
       const filenameTime = currentDateTime.substring(11, 19).replace(/:/g, "-");
       let clipFilename = `Screenshot ${filenameDate} at ${filenameTime} ${filenameTitle}`;
-      const clipFilenameBytesSize = clipFilename.length * 2; // JS STrings are UTF-16
+      const clipFilenameBytesSize = new Blob([clipFilename]).size; // Max size needs to be bytes not chars
       if (clipFilenameBytesSize > 251) {
         // 255 bytes (Usual filesystems max) - 4 for the ".png" file extension string
         const excedingchars = (clipFilenameBytesSize - 246) / 2; // 251 - 5 for ellipsis "[...]"

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/9ace613c8e1b

Comment 12

[Alice Varro [avarro], Ecosystem QA]

I have verified this issue using the latest Firefox Nightly 110.0a1 (Build ID: 20230103094636) on Linux Mint 20.2 x64 using the steps provided in the Description and I can confirm the following:

• The downloaded screenshot is successfully saved on the disk.