1413074 - download filename not sanitized with Content-Disposition: inline

Status: RESOLVED FIXED

(Firefox :: File Handling, defect, P1)

Description

[l.mai]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20171009144901

Steps to reproduce:

I was playing around with HTTP headers and Firefox (on Linux).

1. I visited a URL that responded with 'Content-Disposition: attachment; filename="../wat.txt"'
2. I visited a URL that responded with 'Content-Disposition: inline; filename="../wat.txt"'
3. I pressed Ctrl-S to save the page

4. I visited a URL that responded with 'Content-Disposition: attachment; filename="/wat2.txt"'
5. I visited a URL that responded with 'Content-Disposition: inline; filename="/wat2.txt"'
6. I pressed Ctrl-S to save the page


Actual results:

1. Firefox popped up a dialog: "You have chosen to open: " with an empty filename. Saving the file produced a file called ~/Downloads/YVQeN4_Y.asc.
2. Firefox displayed the response inline.
3. Firefox popped up a "Save as" dialog with the filename prefilled as "../wat.txt". Clicking the "Save" button actually created a file outside of the visible directory (i.e. the dialog displayed ~/Downloads, but saving created ~/Downloads/../wat.txt, which is ~/wat.txt).

4. Firefox popped up a dialog: "You have chosen to open: _wat2.txt".
5. Firefox displayed the response inline.
6. Firefox popped up a "Save as" dialog with the filename prefilled as "/wat2.txt". Clicking "Save" starts a download, but it immediately shows up as "Failed" (presumably because I don't have write permissions on /).


Expected results:

1. Honestly, I'm not sure. I would have expected it to mangle the name to "___wat.txt" or ".._wat.txt". However, the "You have chosen to open" dialog should not display an empty filename field; if nothing else, it should show the final generated name (YVQeN4_Y.asc).
2. -
3. Firefox should probably not use / characters as-is in the suggested filename. In particular, it should not silently create a file in a different directory than what the save dialog shows. It should use the same name-mangling algorithm that "Content-Disposition: attachment" uses.

4. -
5. -
6. Firefox should probably not use / characters as-is in the suggested filename. In particular, it should not silently create a file in a different directory than what the save dialog shows. It should use the same name-mangling algorithm that "Content-Disposition: attachment" uses.

Comment 1

[:Paolo Amadini]

Thanks for reporting! The issue of files being saved in a parent directory affects Linux only, because on other platforms the file picker sanitizes the file name itself, but it makes sense for the browser to prepare a valid file name consistently on all platforms.

Comment 2

[:Paolo Amadini]

Attachment: Bug 1413074 - Sanitize the file names of downloads before opening the file picker.

Review commit: https://reviewboard.mozilla.org/r/195404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195404/

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8924143 [details]
Bug 1413074 - Sanitize the file names of downloads before opening the file picker.

https://reviewboard.mozilla.org/r/195404/#review201070

::: toolkit/mozapps/downloads/nsHelperAppDlg.js:287
(Diff revision 1)
>        // Use file picker to show dialog.
>        var nsIFilePicker = Components.interfaces.nsIFilePicker;
>        var picker = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
>        var windowTitle = bundle.GetStringFromName("saveDialogTitle");
>        picker.init(parent, windowTitle, nsIFilePicker.modeSave);
> -      picker.defaultString = aDefaultFile;
> +      picker.defaultString = this.getFinalLeafName(aDefaultFile);

Could you please rename the aDefaultFile argument of this method to aDefaultFileName (like it is in the idl)? I found it quite confusing since it sounds like a file object but it's passed around to all the methods expecting a filename...

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dfb5111a3a8
Sanitize the file names of downloads before opening the file picker. r=mak

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser-chrome's browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3dc7f1d15d2794494bdb801551cd111b4c7fa3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7dfb5111a3a8bc78cabdfaca96c4d949e33bafdb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141936948&repo=mozilla-inbound

[task 2017-11-03T12:10:47.081Z] 12:10:47     INFO - TEST-PASS | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js | LastDir should be the expected display dir - 
[task 2017-11-03T12:10:47.082Z] 12:10:47     INFO - TEST-PASS | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js | gDownloadLastDir should be the expected display dir - 
[task 2017-11-03T12:10:47.084Z] 12:10:47     INFO - Console message: [JavaScript Error: "leafName is null" {file: "resource://gre/modules/DownloadPaths.jsm" line: 76}]
[task 2017-11-03T12:10:47.086Z] 12:10:47     INFO - sanitize@resource://gre/modules/DownloadPaths.jsm:76:5
[task 2017-11-03T12:10:47.087Z] 12:10:47     INFO - getFinalLeafName@jar:file:///builds/worker/workspace/build/application/firefox/omni.ja!/components/nsHelperAppDlg.js:375:12
[task 2017-11-03T12:10:47.088Z] 12:10:47     INFO - promptForSaveToFileAsync/<@jar:file:///builds/worker/workspace/build/application/firefox/omni.ja!/components/nsHelperAppDlg.js:289:30
[task 2017-11-03T12:10:47.089Z] 12:10:47     INFO - async*promptForSaveToFileAsync@jar:file:///builds/worker/workspace/build/application/firefox/omni.ja!/components/nsHelperAppDlg.js:252:6
[task 2017-11-03T12:10:47.091Z] 12:10:47     INFO - testDownloadDir@chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js:79:5
[task 2017-11-03T12:10:47.092Z] 12:10:47     INFO - test/<@chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js:83:5
[task 2017-11-03T12:10:47.095Z] 12:10:47     INFO - test/testOnWindow/<@chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js:44:7
[task 2017-11-03T12:10:47.096Z] 12:10:47     INFO - whenNewWindowLoaded/</<@chrome://mochitests/content/browser/browser/components/privatebrowsing/test/browser/head.js:19:40
[task 2017-11-03T12:10:47.097Z] 12:10:47     INFO - run@chrome://mochikit/content/browser-test.js:1039:9
[task 2017-11-03T12:10:47.098Z] 12:10:47     INFO - 
[task 2017-11-03T12:10:47.099Z] 12:10:47     INFO - Buffered messages finished
[task 2017-11-03T12:10:47.102Z] 12:10:47     INFO - TEST-UNEXPECTED-FAIL | browser/components/privatebrowsing/test/browser/browser_privatebrowsing_downloadLastDir.js | Test timed out -

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd4bc797100
Sanitize the file names of downloads before opening the file picker. r=mak

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/8bd4bc797100