Bug 61155 - [fileapi] Add a File::createWithName method to avoid obfuscated filename leakage from FileEntry.file() method
Summary: [fileapi] Add a File::createWithName method to avoid obfuscated filename leak...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-19 16:27 PDT by Adam Klein
Modified: 2011-05-23 13:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.61 KB, patch)
2011-05-19 16:32 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (8.56 KB, patch)
2011-05-19 17:35 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Update comment (8.59 KB, patch)
2011-05-23 10:28 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (8.59 KB, patch)
2011-05-23 11:00 PDT, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-05-19 16:27:46 PDT
[fileapi] Add a File::createWithName method to avoid obfuscated filename leakage from FileEntry.file() method
Comment 1 Adam Klein 2011-05-19 16:32:39 PDT
Created attachment 94143 [details]
Patch
Comment 2 Adam Klein 2011-05-19 16:34:58 PDT
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
Comment 3 Jian Li 2011-05-19 16:44:13 PDT
(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.
Comment 4 Adam Klein 2011-05-19 16:44:39 PDT
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).
Comment 5 Adam Klein 2011-05-19 16:47:53 PDT
(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?
Comment 6 Adam Klein 2011-05-19 16:56:42 PDT
(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.
Comment 7 Eric U. 2011-05-19 16:58:56 PDT
(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].
Comment 8 Jian Li 2011-05-19 17:06:24 PDT
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|.
Comment 9 Adam Klein 2011-05-19 17:35:26 PDT
Created attachment 94150 [details]
Patch
Comment 10 Adam Klein 2011-05-19 17:35:33 PDT
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.
Comment 11 Jian Li 2011-05-19 18:52:56 PDT
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.
Comment 12 Adam Klein 2011-05-23 10:28:32 PDT
Created attachment 94438 [details]
Update comment
Comment 13 Adam Klein 2011-05-23 10:31:00 PDT
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)
Comment 14 Jian Li 2011-05-23 10:43:14 PDT
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.
Comment 15 Adam Klein 2011-05-23 11:00:55 PDT
Created attachment 94445 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2011-05-23 13:47:17 PDT
Comment on attachment 94445 [details]
Patch for landing

Clearing flags on attachment: 94445

Committed r87095: <http://trac.webkit.org/changeset/87095>
Comment 17 WebKit Commit Bot 2011-05-23 13:47:24 PDT
All reviewed patches have been landed.  Closing bug.