Bug 61155 – [fileapi] Add a File::createWithName method to avoid obfuscated filename leakage from FileEntry.file() method

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Klein]

[fileapi] Add a File::createWithName method to avoid obfuscated filename leakage from FileEntry.file() method

[Adam Klein]

Created attachment 94143 [details]
Patch

[Adam Klein]

This patch also happens to ensure the MIME type of the File is chosen based on FileEntry.name instead of the obfuscated name (covered by the same tests).

I tested this locally in chrome with --use-obfuscated-file-system --allow-file-access-from-files --unlimited-quota-for-files

[Jian Li]

(In reply to comment #2)
> This patch also happens to ensure the MIME type of the File is chosen based on FileEntry.name instead of the obfuscated name (covered by the same tests).
> 
> I tested this locally in chrome with --use-obfuscated-file-system --allow-file-access-from-files --unlimited-quota-for-files

Can you add a layout test for this?

Also, for the new File constructor:
   File(const String& path, const String& name);
it seems that you allow name to be empty. This seems to be unnatural.

[Adam Klein]

Comment on attachment 94143 [details]
Patch

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

> Source/WebCore/fileapi/File.cpp:39
> +    const String& nameForMIMEType = !name.isEmpty() ? name : path;

I thought about making name a required argument and always using it to figure out the MIME type but was a little worried that this could change behavior on some platform where pathGetFileName() did something surprising (basically, anything other than pulling out the last path component).

[Adam Klein]

(In reply to comment #3)
> (In reply to comment #2)
> > This patch also happens to ensure the MIME type of the File is chosen based on FileEntry.name instead of the obfuscated name (covered by the same tests).
> > 
> > I tested this locally in chrome with --use-obfuscated-file-system --allow-file-access-from-files --unlimited-quota-for-files
> 
> Can you add a layout test for this?

You mean with the Chromium flag set?  I'll defer to Eric about whether that's a good idea at this point.  Is there some standard way to say "run this test with the following Chromium flag set?"  I'm not aware of one.

> Also, for the new File constructor:
>    File(const String& path, const String& name);
> it seems that you allow name to be empty. This seems to be unnatural.

Not sure what you mean by "allow".  The existing code doesn't care of m_path is empty, nor if m_name ends up empty.  What did you think the code should do if name is empty?

[Adam Klein]

(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > This patch also happens to ensure the MIME type of the File is chosen based on FileEntry.name instead of the obfuscated name (covered by the same tests).
> > > 
> > > I tested this locally in chrome with --use-obfuscated-file-system --allow-file-access-from-files --unlimited-quota-for-files
> > 
> > Can you add a layout test for this?
> 
> You mean with the Chromium flag set?  I'll defer to Eric about whether that's a good idea at this point.  Is there some standard way to say "run this test with the following Chromium flag set?"  I'm not aware of one.

Also, I should say that --use-obfuscated-file-system will be the default very soon (by M13), at which point the existing tests will provide this coverage.

[Eric U.]

(In reply to comment #3)
> (In reply to comment #2)
> > This patch also happens to ensure the MIME type of the File is chosen based on FileEntry.name instead of the obfuscated name (covered by the same tests).
> > 
> > I tested this locally in chrome with --use-obfuscated-file-system --allow-file-access-from-files --unlimited-quota-for-files
> 
> Can you add a layout test for this?

There's no way to test this via layout test yet, as this constructor isn't exposed to javascript.  When Chromium switches to using obfuscated filesystem paths [next week], the existing layout test fast/filesystem/file-from-file-entry.html will test this code [in fact, it's why we're adding this before making the Chromium change].

[Jian Li]

Comment on attachment 94143 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        Added an optional name argument to ensure MIME type lookup uses the

I think you can move your comment "Added an ..." to be immediately following ": " of the previous line.

> Source/WebCore/ChangeLog:24
> +        as it now needs to mutate the created File before returning it.

Not sure I understand your comment about "out-of-lined" and "mutate".  Probably just the first half sentence seems to be enough.

> Source/WebCore/fileapi/File.h:56
> +    static PassRefPtr<File> createWithName(const String& path, const String& name)

Please add the comment saying that |name| is the filename that is observed by the user, that might be different from the real filename embedded in |path|.

Or maybe even better, consider rename |name| to something like |observedName|.

[Adam Klein]

Created attachment 94150 [details]
Patch

[Adam Klein]

Comment on attachment 94143 [details]
Patch

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

>> Source/WebCore/ChangeLog:20
>> +        Added an optional name argument to ensure MIME type lookup uses the
> 
> I think you can move your comment "Added an ..." to be immediately following ": " of the previous line.

Done, and same elsewhere.

>> Source/WebCore/ChangeLog:24
>> +        as it now needs to mutate the created File before returning it.
> 
> Not sure I understand your comment about "out-of-lined" and "mutate".  Probably just the first half sentence seems to be enough.

Done.

>> Source/WebCore/fileapi/File.h:56
>> +    static PassRefPtr<File> createWithName(const String& path, const String& name)
> 
> Please add the comment saying that |name| is the filename that is observed by the user, that might be different from the real filename embedded in |path|.
> 
> Or maybe even better, consider rename |name| to something like |observedName|.

I'd rather the comment in this case, actually, otherwise the comment is specifying how it will be called, which is really up to the callers.  See if you like my comment.

[Jian Li]

Comment on attachment 94150 [details]
Patch

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

> Source/WebCore/fileapi/File.cpp:39
> +    const String& nameForMIMEType = !name.isEmpty() ? name : path;

Simpler to say:
  static PassOwnPtr<BlobData> createBlobDataForFile(const String& path, const String& nameForMIMEType = String())
  {
      ...
      if (nameForMIMEType.isEmpty())
          nameForMIMEType = path;

> Source/WebCore/fileapi/File.h:56
> +    // Create a file whose name, for purposes of MIME type and access through |File.name|, does not match its path.

How about something like:
  // Create a file with different name exposed to the user (File.type and File.name in DOM), that is different from the one provided in the path.

[Adam Klein]

Created attachment 94438 [details]
Update comment

[Adam Klein]

Comment on attachment 94150 [details]
Patch

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

>> Source/WebCore/fileapi/File.cpp:39
>> +    const String& nameForMIMEType = !name.isEmpty() ? name : path;
> 
> Simpler to say:
>   static PassOwnPtr<BlobData> createBlobDataForFile(const String& path, const String& nameForMIMEType = String())
>   {
>       ...
>       if (nameForMIMEType.isEmpty())
>           nameForMIMEType = path;

No can do: references can't be re-bound.  I _could_ do

static PassOwnPtr<BlobData> createBlobDataForFile(const String& path, String nameForMIMEType = String())
{
    ...
    if (nameForMIMEType.isEmpty())
          nameForMIMEType = path;
}

But that incurs unnecessary copies.

>> Source/WebCore/fileapi/File.h:56
>> +    // Create a file whose name, for purposes of MIME type and access through |File.name|, does not match its path.
> 
> How about something like:
>   // Create a file with different name exposed to the user (File.type and File.name in DOM), that is different from the one provided in the path.

Done.  Now reads: 

// Create a file with a name exposed to the author (via File.name and associated DOM properties) that differs from the one provided in the path.

(switched "user" for "author", avoided direct mention of MIME type)

[Jian Li]

Comment on attachment 94438 [details]
Update comment

Looks good except one minor comment. Please fix it before you land the patch.

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

> Source/WebCore/ChangeLog:13
> +        (WebCore::DOMFileSystem::createFile): Updated to call createWithName()

Please end the sentence with period and do the same thing for other comments.

[Adam Klein]

Created attachment 94445 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 94445 [details]
Patch for landing

Clearing flags on attachment: 94445

Committed r87095: <http://trac.webkit.org/changeset/87095>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.