1454325 - Adjust XMLHttpRequest Content-Type handling

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Anne (:annevk)]

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.

Comment 1

[Thomas Wisniewski [:twisniewski]]

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.

Comment 2

[Anne (:annevk)]

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.

Comment 3

[Thomas Wisniewski [:twisniewski]]

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?

Comment 4

[Henri Sivonen (:hsivonen)]

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

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

Comment 5

[Anne (:annevk)]

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.

Comment 6

[Henri Sivonen (:hsivonen)]

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.

Comment 7

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1454325 - have XHRs adjust content type of uploads per spec using the MIME Sniffing standard; r?hsivonen

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

Comment 8

[Thomas Wisniewski [:twisniewski]]

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

Comment 9

[Thomas Wisniewski [:twisniewski]]

Review feedback addressed. Requesting check-in.

Comment 10

[Thomas Wisniewski [:twisniewski]]

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

Comment 11

[Henri Sivonen (:hsivonen)]

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.

Comment 12

[Thomas Wisniewski [:twisniewski]]

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

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca06d80acf9b
have XHRs adjust content type of uploads per spec using the MIME Sniffing standard; r=hsivonen

Comment 14

[Eliza Balazs [:ebalazs_]]

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

Comment 15

[Ben Bucksch (:BenB)]

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?

Comment 16

[Ben Bucksch (:BenB)]

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>

Comment 17

[Thomas Wisniewski [:twisniewski]]

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

Comment 18

[Thomas Wisniewski [:twisniewski]]

Anne said yes on Slack, so clearing the ni.

Comment 19

[Ben Bucksch (:BenB)]

> 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.

Comment 20

[Anne (:annevk)]

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.

Comment 21

[Anne (:annevk)]

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?

Comment 22

[Thomas Wisniewski [:twisniewski]]

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).

Comment 23

[Anne (:annevk)]

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...

Comment 24

[Ben Bucksch (:BenB)]

> 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.

Comment 25

[Kohei Yoshino [:kohei]]

(updating the flag as per Bug 1499136)

Comment 26

[Anne (:annevk)]

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.

Comment 27

[Ben Bucksch (:BenB)]

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!

Comment 28

[Anne (:annevk)]

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.

Comment 29

[Ben Bucksch (:BenB)]

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.

Comment 30

[Anne (:annevk)]

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.

Comment 31

[Ben Bucksch (:BenB)]

OK, makes sense, thanks.

Comment 32

[Thomas Wisniewski [:twisniewski]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c6280da6ce35f6a5ddbfe616368ba8a2d946104

Comment 33

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1454325 - update XHR upload content-type handling to match the spec. r?baku

update XHR upload content-type handling to match the spec

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f525479fabd6
update XHR upload content-type handling to match the spec. r=baku

Comment 35

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset f525479fabd6 (Bug 1454325) for xpcshell/test_Http.js failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=os%2Cx%2C10.10%2Copt%2Cxpcshell%2Ctests%2Ctest-macosx64%2Fopt-xpcshell%2Cx%28x%29&tochange=dd235c6dd881e45afb01a1320d4c7567a7dd1067&fromchange=711d5fdae7b254489d47f3172e50c7738c4e77dd&selectedJob=234698563

Backout link: https://hg.mozilla.org/integration/autoland/rev/dd235c6dd881e45afb01a1320d4c7567a7dd1067

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=234698563&repo=autoland&lineNumber=7757

02:21:19 INFO - TEST-START | toolkit/modules/tests/xpcshell/test_Http.js
02:21:19 WARNING - TEST-UNEXPECTED-FAIL | toolkit/modules/tests/xpcshell/test_Http.js | xpcshell return code: 0
02:21:19 INFO - TEST-INFO took 281ms
02:21:19 INFO - >>>>>>>
02:21:19 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
02:21:19 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
02:21:19 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
02:21:19 INFO - running event loop
02:21:19 INFO - toolkit/modules/tests/xpcshell/test_Http.js | Starting test_successCallback
02:21:19 INFO - (xpcshell/head.js) | test test_successCallback pending (2)
02:21:19 INFO - (xpcshell/head.js) | test pending (3)
02:21:19 INFO - (xpcshell/head.js) | test run_next_test 0 finished (3)
02:21:19 INFO - TEST-PASS | toolkit/modules/tests/xpcshell/test_Http.js | test_successCallback - [test_successCallback : 72] "Success!" == "Success!"
02:21:19 INFO - (xpcshell/head.js) | test finished (2)
02:21:19 INFO - (xpcshell/head.js) | test run_next_test 1 pending (2)
02:21:19 INFO - (xpcshell/head.js) | test test_successCallback finished (2)
02:21:19 INFO - toolkit/modules/tests/xpcshell/test_Http.js | Starting test_errorCallback
02:21:19 INFO - (xpcshell/head.js) | test test_errorCallback pending (2)
02:21:19 INFO - (xpcshell/head.js) | test pending (3)
02:21:19 INFO - (xpcshell/head.js) | test run_next_test 1 finished (3)
02:21:19 INFO - TEST-PASS | toolkit/modules/tests/xpcshell/test_Http.js | test_errorCallback - [test_errorCallback : 94] "404 - Not Found" == "404 - Not Found"
02:21:19 INFO - (xpcshell/head.js) | test finished (2)
02:21:19 INFO - (xpcshell/head.js) | test run_next_test 2 pending (2)
02:21:19 INFO - (xpcshell/head.js) | test test_errorCallback finished (2)
02:21:19 INFO - toolkit/modules/tests/xpcshell/test_Http.js | Starting test_PostData
02:21:19 INFO - (xpcshell/head.js) | test test_PostData pending (2)
02:21:19 INFO - (xpcshell/head.js) | test pending (3)
02:21:19 INFO - (xpcshell/head.js) | test run_next_test 2 finished (3)
02:21:19 INFO - TEST-PASS | toolkit/modules/tests/xpcshell/test_Http.js | test_PostData - [test_PostData : 50] "POST" == "POST"
02:21:19 WARNING - TEST-UNEXPECTED-FAIL | toolkit/modules/tests/xpcshell/test_Http.js | test_PostData - [test_PostData : 55] "application/x-www-form-urlencoded; charset=utf-8" == "application/x-www-form-urlencoded;charset=UTF-8"
02:21:19 INFO - /Users/cltbld/tasks/task_1552960875/build/tests/xpcshell/tests/toolkit/modules/tests/xpcshell/test_Http.js:getDataChecker/<:55
02:21:19 INFO - resource://testing-common/httpd.js:handleResponse:2172
02:21:19 INFO - resource://testing-common/httpd.js:process:1147
02:21:19 INFO - resource://testing-common/httpd.js:_handleResponse:1555
02:21:19 INFO - resource://testing-common/httpd.js:_processBody:1418
02:21:19 INFO - resource://testing-common/httpd.js:onInputStreamReady:1306
02:21:19 INFO - /Users/cltbld/tasks/task_1552960875/build/tests/xpcshell/head.js:_do_main:224
02:21:19 INFO - /Users/cltbld/tasks/task_1552960875/build/tests/xpcshell/head.js:_execute_test:526
02:21:19 INFO - -e:null:1
02:21:19 INFO - exiting test
02:21:19 WARNING - TEST-UNEXPECTED-FAIL | toolkit/modules/tests/xpcshell/test_Http.js | test_PostData - [test_PostData : 111] false == true
02:21:19 INFO - /Users/cltbld/tasks/task_1552960875/build/tests/xpcshell/tests/toolkit/modules/tests/xpcshell/test_Http.js:onError:111
02:21:19 INFO - resource://gre/modules/Http.jsm:httpRequest/xhr.onload:71
02:21:19 INFO - /Users/cltbld/tasks/task_1552960875/build/tests/xpcshell/head.js:_do_main:226
02:21:19 INFO - /Users/cltbld/tasks/task_1552960875/build/tests/xpcshell/head.js:_execute_test:526
02:21:19 INFO - -e:null:1
02:21:19 INFO - exiting test
02:21:19 INFO - PID 6295 | JavaScript error: /Users/cltbld/tasks/task_1552960875/build/tests/xpcshell/head.js, line 739: NS_ERROR_ABORT:
02:21:19 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "NS_ERROR_ABORT: " {file: "/Users/cltbld/tasks/task_1552960875/build/tests/xpcshell/head.js" line: 739}]"
02:21:19 INFO - <<<<<<<
02:21:19 INFO - INFO | Result summary:
02:21:19 INFO - INFO | Passed: 3171
02:21:19 WARNING - INFO | Failed: 1
02:21:19 WARNING - One or more unittests failed.
02:21:19 INFO - INFO | Todo: 4

Comment 36

[Thomas Wisniewski [:twisniewski]]

Hmm, not sure what happened there (the test should have been updated).

Try run for trivial fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0318b30d9d86418d1639a0d46ec349968e324e8e

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d12bb5576d0f
update XHR upload content-type handling to match the spec. r=baku

Comment 38

[Oana Pop-Rus]

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