1746052 - Unify ways to select and validate filenames when saving

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P3)

Description

[Neil Deakin]

There are at least five different ways in which one can save a file such as an image. However they all use different codepaths that determine the filename differently, and apply different validation rules to the filename.

1. Save As

• does use the filename from the content-disposition, but also uses the 'name' if it is not present.
• uses the document title if available
• replaces some invalid extensions but for other invalid extensions, just appends to the existing one (reallyapng.jpeg becomes reallyapng.png but image.pang becomes image.pang.png)
• strips out many illegal characters (more so than drag and copy)
• does not truncate long filenames (such that on Mac, the save button is just disabled and it is unclear why)

2. Subfiles saved from save document, complete:

• doesn't use the filename from the content-disposition
• replaces invalid extensions
• truncates long filenames to a maximum of 64 characters
• strips out almost all non-alphanumeric characters
• if the file already exists, adds a number such as '_001', '_002' etc. to the filename

3. Drag image to file system:

• doesn't use the filename from the content-disposition
• replaces invalid extensions
• strips out some illegal characters
• does not truncate long filenames
• Mac uses different but identical code for this

4. Copy/Paste image to file system

• doesn't use the filename from the content-disposition
• replaces invalid extensions
• strips out some illegal characters
• does not truncate long filenames

5. Content-disposition: attachment or download attribute

• filename from the content-disposition overrides the download attribute (this is the correct behaviour)
• doesn't replace invalid extensions
• strips out some illegal characters (a yet still different set than the other types)
• truncates long filenames to a maximum to 138(?) characters

There are other various minor differences. It would be good to unify all of these so that they same rules apply in each case.

Comment 1

[Neil Deakin]

Attachment: Bug 1746052, add a means to get the filename that should be used for saving an image to disk, r=aosmond

Comment 2

[Neil Deakin]

Attachment: Bug 1746052, add methods to the mime service that compute and validate a filename for a given content type, r=gijs

Depends on D135950

Comment 3

[Neil Deakin]

Attachment: Bug 1746052, use ValidateFileNameForSaving to compute the filename for dragging images to the file system, r=haik

Depends on D135951

Comment 4

[Neil Deakin]

Attachment: Bug 1746052, use ValidateFileNameForSaving to determine a valid filename for pasting a file to the filesystem, r=smaug

Depends on D135952

Comment 5

[Neil Deakin]

Attachment: Bug 1746052, use GetValidFileName to determine the filename from the channel when saving a resource, r=gijs

Depends on D135953

Comment 6

[Neil Deakin]

Attachment: Bug 1746052, don't modify the extension for text and binary types, as this is the behaviour that save as had, r=gijs

Depends on D135954

Comment 7

[Neil Deakin]

Attachment: Bug 1746052, further restrict the characters that are allowed in filenames, as save as doesn't allow these, r=mccr8

Depends on D135955

Comment 8

[Neil Deakin]

Attachment: Bug 1746052, select a default filename for saving if the filename ends up being empty, r=gijs

Depends on D135956

Comment 9

[Neil Deakin]

Attachment: Bug 1746052, use validateFileNameForSaving to get and validate the default filename when a document or image is chosen to be saved, r=gijs

Depends on D135957

Comment 10

[Neil Deakin]

Attachment: Bug 1746052, add tests for filename validation when dragging images, copy/paste of images, saving a document or image, and when content disposition: attachment is used, and add a unit test that verifies nsIMIMEService.validateFileNameForSaving, r=mhowell

Depends on D135958

Comment 11

[Neil Deakin]

No existing tests were modified by these changes, however some behaviour changes with regards
to filenames that currently have no automated tests is described below. Some of the following
might need further investigation to determine whether they are problems or not:

1. There are differences between the way that Save Page chooses whether the append the valid extension
   to an existing invalid extension versus replacing the invalid extension. Some types -- images and
   those listed in forcedExtensionMimetypes -- replace existing invalid extensions, whereas other
   types append to any existing invalid extension.
2. Some empty filenames resulted in filenames like '.jpg' or 'exe'. The behaviour here
   uses the default filename of 'index' instead, such that, for example, 'index.jpg' would be used.
   This has been applied to all cases, but probably needs some modification.
3. Save Page can use the web page title when saving a document. This should work but has not been
   tested much yet.
4. Invalid characters are now handled more consistently for all save types, so some variations
   of results will apply. For example, '*' is now replaced with a space. Some of the checks that
   DownLoadPaths.sanitize does are not yet added. In particular, string.CollapseWhitespace() only
   collapses ascii whitespace, but save as also collapses a wide variety of other whitespace
   characters.
5. Long filenames are always cropped at 64 characters. This should ideally be fixed to be
   platform-specific. (But save web page, complete currently used 64 characters on all platforms,
   other save types don't have any restriction at all.)
6. May wish to expose the sanitize function (SanitizeFilename) somehow so that DownloadPaths.sanitize
   can use the same method.

These differences can be checked by running the new test browser_save_filenames.js without any of
the patches applied.

Oddities (current behaviour and new):

• Plain text and binary types do not get their extensions checked for validity.
• When calling GetFromTypeAndExtension, the extension of image types is ignored. On Mac,
  at least, the extension is validated by checking if the specified extension would be
  handled by the same application as the primary extension. For me, this is Preview.app
  for any image extension. So this means that jpeg, for example, is treated as a valid
  extension for a png image.
• The test browser_test_data_text_csv.js has a case where there is no filename, so it defaults
  to the temporary file that was created when downloading.
• On Windows, when saving a file with no content type and an .exe extension, .txt is
  appended to the filename. This is done at a later step in validateLeafName in HelperAppDlg.jsm.
  This is special-cased (case 54) in the testcase.
• Windows does not have a application/x-javascript mime type handler specified by default so
  the mime code (in uriloader/exthandler/win) creates a mime info object where the primary
  extension is the extension you specified. When the extraMimeEntries gets checked, it adds
  the .js extension to the mime info object but at a lower priority. This means that the
  .js extension is never applied. This affects a number of testcases, for example, 23 and 24.
• Sniffing is done too generically in toolkit/components/mediasniffer/nsMediaSniffer.cpp and
  a file web_video1.ogv will be sniffed as application/ogg even though the extension implies
  it should be video/ogg. This doesn't cause any effect with the current patches however but
  was noticed as a issue to investigate further.
• Mac seems to use the extension .dms to refer to unknown binary files. This does happen in some
  situations currently, and possibly could still happen when saving some files, but this is
  reduced and doesn't appear to show up in any cases with these patches applied.
• The name field Content-Disposition is used by contentAreaUtils.js as well as the filename. I'm
  not clear of the background behind this, but this special case is kept in just when using this
  codepath.
• nsWebBrowserPersist::CalculateAndAppendFileExt used to be supplied an original url that was
  checked as an alternate, but I can't find a case where that url differs from the regular url
  that was supplied. It seems inconsequential though so I just removed this.

Comment 12

[Neil Deakin]

Attachment: Bug 1746052, improve file sanitization by filtering and collpasing more whitespace characters and truncating files to 255 bytes, r=gijs

Depends on D135951

Comment 13

[Neil Deakin]

Attachment: Bug 1746052, don't allow Windows reserved filenames when sanitizing filenames. Move MangleTextToValidFileName to nsLocalFileWin and rename it to CheckForReservedFileName, r=gijs

Depends on D135957

Comment 14

[Neil Deakin]

Attachment: Bug 1746052, use validateFileNameForSaving in DownloadPaths.sanitize with flags to emulate something similar to the existing behaviour, r=mak

Depends on D135959

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7665f02c114e
add a means to get the filename that should be used for saving an image to disk, r=aosmond
https://hg.mozilla.org/integration/autoland/rev/c91230a8ea90
add methods to the mime service that compute and validate a filename for a given content type, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/1a389759585a
improve file sanitization by filtering and collpasing more whitespace characters and truncating files to 255 bytes, r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/94c95f004221
use ValidateFileNameForSaving to compute the filename for dragging images to the file system, r=haik
https://hg.mozilla.org/integration/autoland/rev/73d17535d8d1
use ValidateFileNameForSaving to determine a valid filename for pasting a file to the filesystem, r=smaug
https://hg.mozilla.org/integration/autoland/rev/a698068d078f
use GetValidFileName to determine the filename from the channel when saving a resource, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/5b76d8d76b2b
don't modify the extension for text and binary types, as this is the behaviour that save as had, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/daccb796a093
further restrict the characters that are allowed in filenames, as save as doesn't allow these, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/ebc6720fdab3
select a default filename for saving if the filename ends up being empty, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/30de4b77f242
don't allow Windows reserved filenames when sanitizing filenames. Move MangleTextToValidFileName to nsLocalFileWin and rename it to CheckForReservedFileName, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/b0ef7c68abcf
use validateFileNameForSaving to get and validate the default filename when a document or image is chosen to be saved, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/229edc158a2b
add tests for filename validation when dragging images, copy/paste of images, saving a document or image, and when content disposition: attachment is used, and add a unit test that verifies nsIMIMEService.validateFileNameForSaving, r=mhowell
https://hg.mozilla.org/integration/autoland/rev/d19cc58e3cab
use validateFileNameForSaving in DownloadPaths.sanitize with flags to emulate something similar to the existing behaviour, r=mak

Comment 16

[Natalia Csoregi [:nataliaCs]]

Backed out for causing failures on uriloader/exthandler/tests/

• backout: https://hg.mozilla.org/integration/autoland/rev/15cebc1bbdefe8a635edef28b8503b89446105b3
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=d19cc58e3cab262ccf4f7e908d59f8b08903a67e
• failure logs:
  • TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/mochitest/browser_save_filenames.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. -
  • TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/mochitest/browser_auto_close_window.js | Test timed out -

Comment 17

[Neil Deakin]

Attachment: Bug 1746052, on Mac, set the always ask flag for the macbinary type as well, r=gijs

This test involves clicking on a link to a .bin file. In the Mac test environment, this is given the macbinary content type. This test assumes that it will be octet-stream (another valid content type for the bin extension) and sets the alwaysAskBeforeHandling flag for that, so the dialog never shows up. However, another subtest needs octet-stream to prompt as well, so set the always ask for both.

Depends on D138738

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41fe77c5f4b2
add a means to get the filename that should be used for saving an image to disk, r=aosmond
https://hg.mozilla.org/integration/autoland/rev/1ee59c52578a
add methods to the mime service that compute and validate a filename for a given content type, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/108e14122e53
improve file sanitization by filtering and collpasing more whitespace characters and truncating files to 255 bytes, r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/4c1ab30de0bd
use ValidateFileNameForSaving to compute the filename for dragging images to the file system, r=haik
https://hg.mozilla.org/integration/autoland/rev/9891f3b0229b
use ValidateFileNameForSaving to determine a valid filename for pasting a file to the filesystem, r=smaug
https://hg.mozilla.org/integration/autoland/rev/73e4689120f0
use GetValidFileName to determine the filename from the channel when saving a resource, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/561a24801d4d
don't modify the extension for text and binary types, as this is the behaviour that save as had, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/1f979899c843
further restrict the characters that are allowed in filenames, as save as doesn't allow these, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/1398fc0669d2
select a default filename for saving if the filename ends up being empty, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/2055ec1e9a57
don't allow Windows reserved filenames when sanitizing filenames. Move MangleTextToValidFileName to nsLocalFileWin and rename it to CheckForReservedFileName, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/5cf2378f6eb4
use validateFileNameForSaving to get and validate the default filename when a document or image is chosen to be saved, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d4796eeeaf64
add tests for filename validation when dragging images, copy/paste of images, saving a document or image, and when content disposition: attachment is used, and add a unit test that verifies nsIMIMEService.validateFileNameForSaving, r=mhowell
https://hg.mozilla.org/integration/autoland/rev/684b2aca10bb
use validateFileNameForSaving in DownloadPaths.sanitize with flags to emulate something similar to the existing behaviour, r=mak
https://hg.mozilla.org/integration/autoland/rev/bf46b0add531
on Mac, set the always ask flag for the macbinary type as well, r=Gijs

Comment 19

[Noemi Erli[:noemi_erli]]

Backed out 14 changesets (Bug 1746052) for causing build bustages in nsExternalHelperAppService.cpp CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=M7Cz89JCRQW1oToRzx0B1w.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=bf46b0add531503bb1f37b51c3cb80552d6adba2

Log: https://treeherder.mozilla.org/logviewer?job_id=377016321&repo=autoland&lineNumber=31367
https://treeherder.mozilla.org/logviewer?job_id=377017991&repo=autoland&lineNumber=6396

Backout: https://hg.mozilla.org/integration/autoland/rev/53107941679965745bfe12311b44f411aaccb5a7

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0dfb58ea1a60
add a means to get the filename that should be used for saving an image to disk, r=aosmond
https://hg.mozilla.org/integration/autoland/rev/e8245a15c3c9
add methods to the mime service that compute and validate a filename for a given content type, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d60765ec0794
improve file sanitization by filtering and collpasing more whitespace characters and truncating files to 255 bytes, r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/6861a1edb0f7
use ValidateFileNameForSaving to compute the filename for dragging images to the file system, r=haik
https://hg.mozilla.org/integration/autoland/rev/36e041cca337
use ValidateFileNameForSaving to determine a valid filename for pasting a file to the filesystem, r=smaug
https://hg.mozilla.org/integration/autoland/rev/f95c284ca227
use GetValidFileName to determine the filename from the channel when saving a resource, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/f47ddc358e1a
don't modify the extension for text and binary types, as this is the behaviour that save as had, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/a8ab5e1f863c
further restrict the characters that are allowed in filenames, as save as doesn't allow these, r=mccr8
https://hg.mozilla.org/integration/autoland/rev/ad582bc5019f
select a default filename for saving if the filename ends up being empty, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/45b3ecacb364
don't allow Windows reserved filenames when sanitizing filenames. Move MangleTextToValidFileName to nsLocalFileWin and rename it to CheckForReservedFileName, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/c5206784e1a6
use validateFileNameForSaving to get and validate the default filename when a document or image is chosen to be saved, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/fa3172404fd7
add tests for filename validation when dragging images, copy/paste of images, saving a document or image, and when content disposition: attachment is used, and add a unit test that verifies nsIMIMEService.validateFileNameForSaving, r=mhowell
https://hg.mozilla.org/integration/autoland/rev/98d24e564a25
use validateFileNameForSaving in DownloadPaths.sanitize with flags to emulate something similar to the existing behaviour, r=mak
https://hg.mozilla.org/integration/autoland/rev/b91422aa4ba1
on Mac, set the always ask flag for the macbinary type as well, r=Gijs

Comment 21

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0dfb58ea1a60
https://hg.mozilla.org/mozilla-central/rev/e8245a15c3c9
https://hg.mozilla.org/mozilla-central/rev/d60765ec0794
https://hg.mozilla.org/mozilla-central/rev/6861a1edb0f7
https://hg.mozilla.org/mozilla-central/rev/36e041cca337
https://hg.mozilla.org/mozilla-central/rev/f95c284ca227
https://hg.mozilla.org/mozilla-central/rev/f47ddc358e1a
https://hg.mozilla.org/mozilla-central/rev/a8ab5e1f863c
https://hg.mozilla.org/mozilla-central/rev/ad582bc5019f
https://hg.mozilla.org/mozilla-central/rev/45b3ecacb364
https://hg.mozilla.org/mozilla-central/rev/c5206784e1a6
https://hg.mozilla.org/mozilla-central/rev/fa3172404fd7
https://hg.mozilla.org/mozilla-central/rev/98d24e564a25
https://hg.mozilla.org/mozilla-central/rev/b91422aa4ba1

Comment 22

[At Close Range]

since version 101.2 until 103.0b4, firefox no longer saves winrar/zip extensions when downloading those files