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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: 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.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 2

[Jonathan Watt [:jwatt]]

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.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch to add a 'path' argument to FormData::append

(that patch, for the record)

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch to include the path in the 'filename' field [r=sicking]

patch updated for review comments

Comment 5

[Jonathan Watt [:jwatt]]

If we change File.path to include a trailing slash this can (must) be simplified to not add it in.

Comment 6

[Mats Palmgren (inactive)]

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?

Comment 7

[Jonathan Watt [:jwatt]]

No. They will send a path relative to the directory that the user picked using the system file picker.

Comment 8

[Mats Palmgren (inactive)]

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?

Comment 9

[Jonathan Watt [:jwatt]]

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 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/068c13777f4d

Comment 13

[Jonathan Watt [:jwatt]]

(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.

Comment 14

[Jesse Ruderman]

This could use tests to ensure we're sending the correct amount of path information and the correct slashes on all platforms.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.