Anne (:annevk) Reporter
	Description • 6 years ago

See https://github.com/whatwg/xhr/pull/176 and https://github.com/w3c/web-platform-tests/pull/8422.

In particular, XMLHttpRequest Content-Type manipulation is now to be done in terms of the MIME type parser defined in MIME Sniffing.

	Thomas Wisniewski [:twisniewski] Assignee
	Comment 1 • 6 years ago

Anne, fixing this will cause a bunch of test failures because of the following edge cases:
1. tests which assert that the charset will be left as-is if it's a case-insensitive match for UTF-8 (possibly including single quotes), rather than being normalized.
2. tests which assert that there is a space before the semicolon ("x; y=z" instead of "x;y=z", which is how the MIME Sniffing serializing works).

This will break the fixes for the old bug 397234 and bug 393968. I assume we're alright with that? If so, I'll try to find the time to fix all of the tests to match the spec.

	Anne (:annevk) Reporter
	Comment 2 • 6 years ago

Yeah, I think that should be okay, given what other browsers ship. If we can implement it in such a way that it's easy to revert that might be beneficial though since apparently it's somewhat sensitive.

	Thomas Wisniewski [:twisniewski] Assignee
	Comment 3 • 6 years ago

Yes, the patch is functionally just adding an if-clause, so it's easy to revert. As a result I could also allow it to be disabled using an about:config pref, which might be wise in case there is any surprise webcompat fallout. hsivonen, what do you think?

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 4 • 6 years ago

> Yeah, I think that should be okay, given what other browsers ship.

Does Chrome normalize utf-8 to UTF-8 here?

	Anne (:annevk) Reporter
	Comment 5 • 6 years ago

Yeah, see http://w3c-test.org/xhr/send-content-type-charset.htm. E.g., Firefox fails "Correct text/plain MIME with charset" which Chrome passes.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 6 • 6 years ago

OK. In that case it seems safe to proceed. If it's easy to have a pref for undoing the change quickly, let's have the pref and a follow-up bug for removing the pref. If a pref turns out to be trickier than expected, let's proceed without a pref.

	Thomas Wisniewski [:twisniewski] Assignee
	Comment 7 • 6 years ago

have XHRs adjust content type of uploads per spec using the MIME Sniffing standard

	Thomas Wisniewski [:twisniewski] Assignee
	Comment 8 • 6 years ago

Ok, I've added a pref, dom.xhr.standard_content_type_normalization (which defaults to true), and updated the few tests that needed to change.

A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e001930b94c246c9bdab97e1650c175f3278a07

	Thomas Wisniewski [:twisniewski] Assignee
	Comment 9 • 6 years ago

Review feedback addressed. Requesting check-in.

	Thomas Wisniewski [:twisniewski] Assignee
	Comment 10 • 6 years ago

Wait, I jumped the shark. I don't think this patch was actually given an r+ yet. Cancelling check-in for now.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11)
	Comment 11 • 6 years ago

Comment on attachment 9009417 [details]
Bug 1454325 - have XHRs adjust content type of uploads per spec using the MIME Sniffing standard; r?hsivonen

Henri Sivonen (:hsivonen) (away until 2018-09-21) has approved the revision.

	Thomas Wisniewski [:twisniewski] Assignee
	Comment 12 • 6 years ago

Thanks for the review while you're away, hsivonen!

I've rebased the patch just in case. Requesting check-in.

	Pulsebot
	Comment 13 • 6 years ago

Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ca06d80acf9b
have XHRs adjust content type of uploads per spec using the MIME Sniffing standard; r=hsivonen

	Eliza Balazs [:ebalazs_]
	Comment 14 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ca06d80acf9b

	Ben Bucksch (:BenB)
	Comment 15 • 6 years ago

This is breaking me. I am trying to do autodiscover with outlook.com, and use XmlHttpRequest(), and set the Content-Type to "text/xml; charset=utf-8". Due to this bug, outlook.com server (<https://outlook.com/autodiscover/autodiscover.xml> with POST of XML) tells me:
Cannot process the message because the content type 'text/xml;charset=UTF-8' was not the expected type 'text/xml; charset=utf-8'.

Now, I agree that they are wrong and the space is optional and charsets are case insensitive. The spec is clear on that and outlook.com is misbehaving. Yet, this is preventing me from contacting the service, and I can't do anything (other than changing the pref - which I cannot easily do for all my users).

Could this maybe an optional feature that can be disabled using an API? Or even the change backed out?

	Ben Bucksch (:BenB)
	Comment 16 • 6 years ago

While you are strictly correct according to standards, this bug might still not be a good idea according to Postel's law:

"Jon Postel... wrote in an early specification of TCP:
TCP implementations should follow a general principle of robustness: be conservative in what you do, be liberal in what you accept from others."
<https://en.wikipedia.org/wiki/Robustness_principle>

	Thomas Wisniewski [:twisniewski] Assignee
	Comment 17 • 6 years ago

Anne, what do you think? Should we disable this for now using the about:config flag, or keep it nightly-only?

	Thomas Wisniewski [:twisniewski] Assignee
	Comment 18 • 6 years ago

Anne said yes on Slack, so clearing the ni.

	Ben Bucksch (:BenB)
	Comment 19 • 6 years ago

> if (parsed && parsed->HasParameter(kLiteralString_charset)) {
>   parsed->SetParameterValue(kLiteralString_charset, kLiteralString_UTF_8);
>   parsed->Serialize(uploadContentType);

Re code, this looks strange to me. I am not sure what you're intending here, but this replaces *any* charset always with UTF-8. For example, no matter whether I set UTF-16 or ISO-5589-1 (Latin1) or ISO-5589-15 (Euro sign) or whatever, it will always be replaced with UTF-8.

Given that this affects XmlHttpRequest, and affects all websites, I think that's extremely dangerous and will break a lot of websites.

	Anne (:annevk) Reporter
	Comment 20 • 6 years ago

Ben, we've been replacing charsets with UTF-8 for forever. The only recent change is how we go about that. We now parse and serialize. Before we scanned and replaced. We need to adjust the strategy slightly and will go back to scan and replace given the amount of literal matching going on, but there's nothing fundamentally wrong here.

	Anne (:annevk) Reporter
	Comment 21 • 6 years ago

I created an alternative strategy here:

* https://github.com/whatwg/xhr/pull/224
* https://github.com/whatwg/mimesniff/pull/86

Tests at: https://github.com/web-platform-tests/wpt/pull/13544.

Thomas, I guess we should have a new bug on implementing that instead?

	Thomas Wisniewski [:twisniewski] Assignee
	Comment 22 • 6 years ago

I'm not sure what others think, but I'm alright with keeping track of the overall work here, in meta-bug fashion (we should at least create dependent bugs for each sub-fix).

	Anne (:annevk) Reporter
	Comment 23 • 6 years ago

Okay, I think it's somewhat unusual practice to reopen a fixed bug and use it in such a manner, but as long as the work gets done it's probably okay...

	Ben Bucksch (:BenB)
	Comment 24 • 6 years ago

> Ben, we've been replacing charsets with UTF-8 for forever.

Forgive my ignorance, but that's surprising me. And I cannot find the (previous) code that did replace charsets in upload mimetypes with UTF-8.

I see a few mentions of JSON parsing of responses, being supported only with UTF-8, which makes kind of sense. But this here is upload (not response parsing), and applies to all mime types (not just JSON), if I am reading this correctly.

	Kohei Yoshino
	Comment 25 • 6 years ago

(updating the flag as per Bug 1499136)

	Anne (:annevk) Reporter
	Comment 26 • 6 years ago

FYI: https://github.com/whatwg/xhr/pull/224 and https://github.com/web-platform-tests/wpt/pull/13779 describe how I think we should address this.

	Ben Bucksch (:BenB)
	Comment 27 • 6 years ago

So, now you're speccing that if there is a space, we should not remove it, and if the charset is "UTF-8", we should not change it, is that right? Thank you!

	Anne (:annevk) Reporter
	Comment 28 • 6 years ago

Not exactly, it basically modifies the original input only if after parsing the charset parameter value is not a case-insensitive match for UTF-8, rather than always modifying it if there's a charset parameter. E.g., text/html;garbage;charset=utf-8 would also remain unmodified and not normalized, as would text/html;charset=utf-8;charset=somethingelse.

	Ben Bucksch (:BenB)
	Comment 29 • 6 years ago

Thanks for addressing this case.

For the record, a "Content-Type: text/html; charset=ISO-5589-1" (or other valid charsets) should not be modified, either. It is completely legit for a webapp to upload a Latin1 HTML file.

	Anne (:annevk) Reporter
	Comment 30 • 6 years ago

The manipulation only happens for strings and Document objects, neither of which web developers have any control over. I recommend reading the XMLHttpRequest standard for further details.

	Ben Bucksch (:BenB)
	Comment 31 • 6 years ago

OK, makes sense, thanks.

	Pulsebot
	Comment 34 • 5 years ago

Pushed by twisniewski@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f525479fabd6
update XHR upload content-type handling to match the spec. r=baku

	Pulsebot
	Comment 37 • 5 years ago

Pushed by twisniewski@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d12bb5576d0f
update XHR upload content-type handling to match the spec. r=baku