When uploading a directory, include file paths relative to the directory (in the 'name' field in the MIME headers)

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Depends on: 1 bug)

Trunk
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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+
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 793148 [details] [diff] [review]
patch to add a 'path' argument to FormData::append

(that patch, for the record)
(Assignee)

Comment 4

5 years ago
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
(Assignee)

Comment 5

5 years ago
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?
Flags: needinfo?(jwatt)
(Assignee)

Comment 7

5 years ago
No. They will send a path relative to the directory that the user picked using the system file picker.
Flags: needinfo?(jwatt)
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?
(Assignee)

Comment 9

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/068c13777f4d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(Assignee)

Comment 13

5 years ago
(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.

Updated

5 years ago
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)

Comment 14

5 years ago
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.

Updated

5 years ago
Depends on: 923435
You need to log in before you can comment on or make changes to this bug.