14.73 KB, patch
|Details | Diff | Splinter Review|
1.14 KB, patch
|Details | Diff | Splinter Review|
Created attachment 793129 [details] [diff] [review] patch When a user picks a directory using <input type=file> and submits the form to a server, the files in the multipart form submission need to include their path relative to the picked directory if the server is to be able to reconstruct the directory tree. We should include the file path in the 'name' field in the MIME headers for files in multipart form submissions to enable this when the path is not the empty string.
Attachment #793129 - Flags: review?(jonas)
Comment on attachment 793129 [details] [diff] [review] patch Review of attachment 793129 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that fixed ::: content/html/content/src/nsFormSubmission.cpp @@ +475,5 @@ > rv = EncodeVal(filename16, filename, true); > NS_ENSURE_SUCCESS(rv, rv); > } > > + // Get and encode the filepath Can you do all of this inside of the if-statement above? That way you can avoid calling EncodeVal an additional time and doing the QI an additional time.
Attachment #793129 - Flags: review?(jonas) → review+
I have a follow-up patch to add an aPathname argument (its value coming from FormData::append). That's why I was breaking it out, but I guess if we allow FormData::append's 'name' argument to include a path component then there's no need for that. I'll move the code up as you request.
Created attachment 793148 [details] [diff] [review] patch to add a 'path' argument to FormData::append (that patch, for the record)
Created attachment 793160 [details] [diff] [review] patch to include the path in the 'filename' field [r=sicking] patch updated for review comments
Attachment #793129 - Attachment is obsolete: true
If we change File.path to include a trailing slash this can (must) be simplified to not add it in.
I hope I'm misreading the patch, but I thought I'd better ask: do these patches (or the ones in bug 894840) going to send the full (absolute) path associated with the submitted files to the server?
No. They will send a path relative to the directory that the user picked using the system file picker.
OK, thanks. So the problem I'm concerned about is revealing the user name through /home/user/ and such. So I guess that can only happen in edge cases like picking /tmp/file1 and /home/user/file2. But perhaps such combinations are not even possible to pick?
There is nothing preventing such directories from being picked right now, but we could add a list of checks before flipping the pref to turn this stuff on. I've filed bug 907707 to cover that.
Comment on attachment 793160 [details] [diff] [review] patch to include the path in the 'filename' field [r=sicking] I thought we decided to include the trailing / in the path?
The user would literally have to pick his own home directory in order for the account name to be included. And even then it wouldn't be obvious that the directory name is actually the account name. So the user would have to pick the / or /user directory in order for there to be any leakage here. attaching files from multiple separate directories isn't a problem. We don't do anything like finding common ancestor directories and include their names. We literally only include the stuff that the user picks.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(In reply to Jonas Sicking (:sicking) (vacation until 9/9. In norway until 9/16) from comment #10) > I thought we decided to include the trailing / in the path? Ah, right. I'll push that change. I filed bug 912385 to give the changeset a bug.
Summary: Include the file path in the 'name' field in the MIME headers for files in multipart form submissions → When uploading a directory, include file paths relative to the directory (in the 'name' field in the MIME headers)
This could use tests to ensure we're sending the correct amount of path information and the correct slashes on all platforms.
Yes, sorry, I missed that in my review. It should be possible to do an automated test for this since we are able to override the filepicker and return whatever file/directory we want using just JS. Please reopen this bug or file a new one.
You need to log in before you can comment on or make changes to this bug.