Bug 51366 – REGRESSION (r66452): Form data no longer contains 'Content-Type' header for files with unrecognized extensions

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Andy Estes]

Prior to r66452, WebKit would include 'Content-Type: application/octet-stream' for files with no extension or with extensions that can't be resolved to a more appropriate MIME type. r66452 changed this behavior to exclude the 'Content-Type' header in these cases. This change violates RFC 1867, which says in section 3.3:

"Each part should be labelled with an appropriate content-type if the media type is known (e.g., inferred from the file extension or operating system typing information) or as application/octet-stream."

[Andy Estes]

<rdar://problem/8545265>

[Andy Estes]

Created attachment 77058 [details]
Test

[Andy Estes]

(In reply to comment #2)
> Created an attachment (id=77058) [details]
> Test

To run the test, apply this patch and run it with run-webkit-tests.

[Andy Estes]

I'm actually just about to upload a patch for this.

[Jian Li]

Created attachment 77061 [details]
Proposed Patch

[Andy Estes]

Comment on attachment 77061 [details]
Proposed Patch

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

> WebCore/platform/network/FormData.cpp:231
> +                    contentType = "application/octet-stream";

I think the more appropriate way to fix this is to change createBlobDataForFile() to call MIMETypeRegistry::getMIMETypeForPath() rather than MIMETypeRegistry::getMIMETypeForExtension(). getMIMETypeForPath() already has the logic to default to 'application/octet-stream'. If we claim that 'application/octet-stream' is the appropriate default MIME type, we should consolidate that logic in one place.

[Andy Estes]

Created attachment 77071 [details]
Patch

[Andy Estes]

I didn't mean to step on top of your patch, Jian. We were just both working on this at the same time.

[Andy Estes]

Side note: Darin (Adler) pointed out that it's inconsistent to have MIMETypeRegistry::getMIMETypeForPath() return 'application/octet-stream' by default but MIMETypeRegistry::getMIMETypeForExtension() return the empty string. There should be a separate bug to clean this up and have them both return the same thing.

[Jian Li]

(In reply to comment #9)
> Side note: Darin (Adler) pointed out that it's inconsistent to have MIMETypeRegistry::getMIMETypeForPath() return 'application/octet-stream' by default but MIMETypeRegistry::getMIMETypeForExtension() return the empty string. There should be a separate bug to clean this up and have them both return the same thing.

I did not realize that you're also working the patch. I thought you just filed a bug and produced a layout test for testing.

Per the File API spec (http://www.w3.org/TR/FileAPI/#dfn-type):
type
The ASCII-encoded string in lower case representing the media type of the Blob, expressed as an RFC2046 MIME type [RFC2046]. User agents SHOULD return the MIME type of the Blob, if it is known. If implementations cannot determine the media type of the Blob, they MUST return the empty string.

So this is the reason I called getMIMETypeForExtension() on purpose in File.cpp. I think we need to unify getMIMETypeForExtension and getMIMETypeForPath. But we need to allow passing the default MIME type as an argument if the valid one cannot be found. How do you think?

[Andy Estes]

Comment on attachment 77071 [details]
Patch

r- based on Jian's comments.

[Darin Adler]

Comment on attachment 77061 [details]
Proposed Patch

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

Things I’d like to see after landing this:

    1) Functions in MIME type registry made consistent about how the express no known MIME type. Null string seems a good standard; maybe we can move to that everywhere.

    2) The "application/octet-stream" MIME type string in one place rather than repeated multiple places. The model should be how we use the blankURL() function for the "about:blank" URL.

    3) Test coverage for the Blob case we would have broken with Andy’s patch.

>> WebCore/platform/network/FormData.cpp:231
>> +                    contentType = "application/octet-stream";
> 
> I think the more appropriate way to fix this is to change createBlobDataForFile() to call MIMETypeRegistry::getMIMETypeForPath() rather than MIMETypeRegistry::getMIMETypeForExtension(). getMIMETypeForPath() already has the logic to default to 'application/octet-stream'. If we claim that 'application/octet-stream' is the appropriate default MIME type, we should consolidate that logic in one place.

As we discussed, it’s fine to have the fix here, but things are a mess, with multiple layers that do not agree on how to say “no known MIME type”. Lets get this fixed, and then do a bit of cleanup.

[Andy Estes]

I filed https://bugs.webkit.org/show_bug.cgi?id=51416 for the MIMETypeRegistry issue.

[Andy Estes]

I'll land Jian's patch now that Darin has reviewed it.

[Andy Estes]

Committed r74427: <http://trac.webkit.org/changeset/74427>

[WebKit Review Bot]

http://trac.webkit.org/changeset/74427 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
http/tests/local/formdata/send-form-data-with-sliced-file.html

[Andy Estes]

(In reply to comment #16)
> http://trac.webkit.org/changeset/74427 might have broken SnowLeopard Intel Release (Tests)
> The following tests are not passing:
> http/tests/local/formdata/send-form-data-with-sliced-file.html

I believe the new result for this test os correct; I updated it in http://trac.webkit.org/changeset/74435.

From reading the FileAPI spec, it sounds like a sliced blob should not have a content type unless one is specified explicitly as the third argument to Blob.slice() (as opposed to it inheriting content type from its parent blob). When this is the case, Blob.type should return the empty string and the Content-Type in the HTTP POST should be 'application/octet-stream' according to RFC 1867. The new test results are showing that the latter is indeed the case for sliced files.

Jian, let me know if you disagree with this assessment.