Bug 91702 – Files from drag and file <input> should use getMIMETypeForExtension to determine content type.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Daniel Cheng]

In r117432, kinuko merged some code together. One path was using getMIMETypeForExtension and another was using getWellKnownMIMETypeForExtension. Using the latter causes sites like youtube.com, which filter on the content-type of the dragged file, to break since the .type attribute would be empty.

(Unrelated: wouldn't it be nice if the header file for MIMETypeRegistry described the difference between the two and what a 'well known' MIME type is?)

[Kinuko Yasuda]

Thanks for catching this!
Recalling the history, we intentionally use getWellKnownMIMETypeForExtension for files from FileSystem API (c.f. http://code.google.com/p/chromium/issues/detail?id=86108).  So probably a quick conservative fix would be to revert the merging part but have two slightly different getMIMEType code around.

[Daniel Cheng]

Created attachment 153402 [details]
Patch

Rough patch that sketches out what we need.

[Jian Li]

(In reply to comment #2)
> Created an attachment (id=153402) [details]
> Patch
> 
> Rough patch that sketches out what we need.

The patch looks OK in terms of just fixing the youtube upload bug. However, I am wondering what should be the right fix in order to take all scenarios into account.

We choose to use the well known MIME type for files from FileSystem. For all other types of files, do we also have the same concern and want to use the well known MIME type too? If not, I think we should let FileSystem caller pass the wellknow type as another argument to File constructor. Darin, could you please comment on this?

[Daniel Cheng]

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=153402) [details] [details]
> > Patch
> > 
> > Rough patch that sketches out what we need.
> 
> The patch looks OK in terms of just fixing the youtube upload bug. However, I am wondering what should be the right fix in order to take all scenarios into account.
> 
> We choose to use the well known MIME type for files from FileSystem. For all other types of files, do we also have the same concern and want to use the well known MIME type too? If not, I think we should let FileSystem caller pass the wellknow type as another argument to File constructor. Darin, could you please comment on this?

Kinuko mentioned that the file input element should probably be covered as well. I am currently updating my patch to reflect this, as well as updating the platform/* implementations.

Kinuko is also working on this bug from the opposite direction--she is modifying the filesystem calls to use a special File constructor/argument.

I think it's slightly safer to limit File to 'well-known' MIME types by default and require an explicit call to allow WebKit to use the OS rather than the other way around, but I'm not sure if this is a strong enough argument to prefer my patch over Kinuko's.

[Kinuko Yasuda]

Created attachment 153415 [details]
Patch (adds special ctor for FileSystem)

[Kinuko Yasuda]

(In reply to comment #5)
> Created an attachment (id=153415) [details]
> Patch (adds special ctor for FileSystem)

The patch that does the other way around.
My take is: if we want to call getMIMEType by default and only handle FileSystem files specifically we should just handle add a special ctor foro them. Otherwise Daniel's way might work better.

(I'll dig to see if we can add a test for this patch)

[Jian Li]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=153415) [details] [details]
> > Patch (adds special ctor for FileSystem)
> 
> The patch that does the other way around.
> My take is: if we want to call getMIMEType by default and only handle FileSystem files specifically we should just handle add a special ctor foro them. Otherwise Daniel's way might work better.
> 
> (I'll dig to see if we can add a test for this patch)

Yes, we need to find out which case(s) we want to guard against.

[Daniel Cheng]

Created attachment 153588 [details]
Special ctor for drag/file input

[Daniel Cheng]

I talked to Darin and he said that the usual approach is to 'default to safe'... but my patch is definitely bigger than Kinuko's patch and touches more places, so it may be harder to merge into Chrome 21. I'm adding some security folks as well to see if they have any thoughts.

[Daniel Cheng]

Created attachment 153856 [details]
Patch

[Jian Li]

Comment on attachment 153856 [details]
Patch

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

> Source/WebCore/fileapi/File.h:53
> +    static PassRefPtr<File> createWithRelativePathFromUserAction(const String& path, const String& relativePath);

The new naming scheme seems to be a bit confusing. How about we make enum public and force the caller to select the right creator to call for most cases, sth like:
    enum FileOrigin {
        // The file is selected as a result of an user action, such as ...
        FromUserAction,
        // The file is created ...
        FromSystem
    };

    static PassRefPtr<File> create(FileOrigin, const String& path)

    // Create a file with a name exposed to the author ...
    static PassRefPtr<File> createWithName(FileOrigin, const String& path, const String& name)

    // For deserialization.
    static PassRefPtr<File> create(const String& path, const KURL& srcURL, const String& type)

#if ENABLE(DIRECTORY_UPLOAD)
    // FromUserAction is assumed.
    static PassRefPtr<File> createWithRelativePath(const String& path, const String& relativePath)
#endif

#if ENABLE(FILE_SYSTEM)
    // FromSystem is assumed.
    static PassRefPtr<File> createForFileSystemFile(const String& name, const FileMetadata& metadata)
#endif

[Daniel Cheng]

Created attachment 153918 [details]
Patch

[Jian Li]

Comment on attachment 153918 [details]
Patch

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

> Source/WebCore/fileapi/File.h:47
> +    static PassRefPtr<File> create(const String& path, ContentTypeLookupPolicy policy = WellKnownContentTypes)

nit: empty line before the enum definition and the creator definition .

[Daniel Cheng]

Committed r123495: <http://trac.webkit.org/changeset/123495>

[Lucas Forschler]

Mass move bugs into the DOM component.