Bug 111255 – FormData.append should prefer application-specified filename to filename in a File

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Victor Costan]

Description snippet from Chrome's bug http://crbug.com/165095

What steps will reproduce the problem?
1. Load the following JSbin -- http://jsbin.com/irefov/1/edit
2. Open the Developer Tools.
3. Click the 'Choose file' button in the <input type="file">, select a (preferably small text) file.
4. Click the XHR to zn-testbed.herokuapp.com' in the Network pane. Click the 'Request' tab, and look at the 'Request payload'
5. Look at the name after "filename=" in the form data.

What is the expected result?
The file name should be "file.name.from.js", as supplied in JavaScript.

What happens instead?
The file name is the name of the file selected in the <input type="file"> UI.

Step 6 in the FormData.append algorithm in the spec says that the 3rd argument should be used as the file name, when given.
http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#dom-formdata-append

I have a failing test for this bug and I'm working on a patch. I will attach the patch as soon as I get it working, hopefully today.

[Victor Costan]

Created attachment 191114 [details]
Patch

[Build Bot]

Comment on attachment 191114 [details]
Patch

Attachment 191114 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16822421

New failing tests:
http/tests/local/formdata/send-form-data-with-filename.html

[Victor Costan]

Comment on attachment 191114 [details]
Patch

FWIW, the patch passes send-form-data-with-filename (the test I added) on my machine. I did a clean WebKit rebuild and re-ran all tests, and it still passes.

[Alexey Proskuryakov]

Comment on attachment 191114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191114&action=review

I don't have the time to look at code changes today, but wanted to make some comments about the test nonetheless. Please address these.

It is expected that the test fails on WebKit2. Please add it to expectations file next to other tests that fail because of beginDragWithFiles being unimplemented.

> LayoutTests/ChangeLog:12
> +        * http/tests/local/formdata/script-tests/send-form-data-with-filename.js: Added.
> +        (runTest):
> +        * http/tests/local/formdata/send-form-data-with-filename-expected.txt: Added.
> +        * http/tests/local/formdata/send-form-data-with-filename.html: Added.

Please don't split the test into two parts. We unfortunately still have many examples of this, but there really aren't any upsides, and many downsides to such splitting.

[Victor Costan]

@Alexey Proskuryakov: thank you for the very quick feedback!

Just to confirm, should I inline the JavaScript files (resources/send-form-data-common.js, script-tests/send-form-data-with-filename.js, fast/js/resources/js-test-post.js) into the HTML and remove unused code?

[Alexey Proskuryakov]

> script-tests/send-form-data-with-filename.js

Just this one. Others are shared between multiple tests, so there is a benefit in keeping them as separate files.

[Victor Costan]

Created attachment 191128 [details]
Patch

[Victor Costan]

@Alexey Proskuryakov: thank you very much for your directions!

I have uploaded a new patch that addresses your feedback. When you have some time, can you please take another look?

[Alexey Proskuryakov]

Comment on attachment 191128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191128&action=review

> Source/WebCore/platform/network/FormData.cpp:239
> +                        // Let the application specify a filename if it's going to generate a replacement file for the upload.

This part is questionable enough for me to say r-, although I could be convinced with a good argument.

The purpose of this code is to let a client create a ZIP archive of a bundle when selected for form upload (such as a .pages document or an application bundle on Mac). The archive is created in a temporary folder, and deleted when not needed any more.

With this change, we will no longer ask the client whether it wants to generate an archive in the case when the name is specified. So, we'll try to upload a folder, and fail while sending the request.

> LayoutTests/http/tests/local/formdata/send-form-data-with-filename.html:11
> +description("Test for sending FormData via XMLHttpRequest.");

It would be better to explain the purpose of the test in more detail (e.g. "Test that filename passed to FormData.append() takes precedence over name attribute of File").

> LayoutTests/http/tests/local/formdata/send-form-data-with-filename.html:17
> +        { 'type': 'file', 'name': 'file1', 'value': '../resources/file-for-drag-to-send.txt', 'filename': 'custom-name.txt' }

This is not directly related to the purpose of this patch, but now that we are looking at this, could you please check what happens if filename contains dangerous characters, like line feeds or quote marks? Anything in HTTP request headers that is client controllable is a huge attack target.

[Victor Costan]

Created attachment 191290 [details]
Patch

[Victor Costan]

@Alexey Proskuryakov: Thank you very much for explaining! Sorry, I didn't realize what's behind shouldReplaceWithGeneratedFileForUpload :(

How about this new approach? It's a bit of duplicated code, but there's no wasted work. I can remove the duplicated code if you're ok with wastefully assigning "blob" to the name String every once in a while. I think that the duplicated code is OK though, because it's not that much, and it's tested.

Regarding having special characters in the filename, should I work that into a separate patch, or let this patch grow?

Once again, thank you very much for your patience and guidance!

[Alexey Proskuryakov]

Comment on attachment 191290 [details]
Patch

> Regarding having special characters in the filename, should I work that into a separate patch, or let this patch grow?

It would be helpful if you could check what's going on there, but it's certainly not part of this patch.

[WebKit Review Bot]

Comment on attachment 191290 [details]
Patch

Clearing flags on attachment: 191290

Committed r144677: <http://trac.webkit.org/changeset/144677>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Victor Costan]

@Alexey Proskuryakov: It seems like special characters in the filename are handled properly. I added a test case to make sure this will always be the case.

https://bugs.webkit.org/show_bug.cgi?id=111380