Closed Bug 1454325 Opened 2 years ago Closed 1 year ago

Adjust XMLHttpRequest Content-Type handling

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox64 --- disabled
firefox68 --- fixed

People

(Reporter: annevk, Assigned: twisniewski)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Priority: -- → P3
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.
Flags: needinfo?(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.
Flags: needinfo?(annevk)
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?
Flags: needinfo?(hsivonen)
> Yeah, I think that should be okay, given what other browsers ship.

Does Chrome normalize utf-8 to UTF-8 here?
Flags: needinfo?(hsivonen) → needinfo?(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.
Flags: needinfo?(annevk)
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.
have XHRs adjust content type of uploads per spec using the MIME Sniffing standard
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
Review feedback addressed. Requesting check-in.
Keywords: checkin-needed
Wait, I jumped the shark. I don't think this patch was actually given an r+ yet. Cancelling check-in for now.
Keywords: checkin-needed
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.
Attachment #9009417 - Flags: review+
Thanks for the review while you're away, hsivonen!

I've rebased the patch just in case. Requesting check-in.
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca06d80acf9b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee: nobody → twisniewski
Depends on: 1496754
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?
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>
Anne, what do you think? Should we disable this for now using the about:config flag, or keep it nightly-only?
Flags: needinfo?(annevk)
Depends on: 1499136
Anne said yes on Slack, so clearing the ni.
Flags: needinfo?(annevk)
> 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.
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.
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?
Flags: needinfo?(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).
Flags: needinfo?(twisniewski)
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...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> 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.
(updating the flag as per Bug 1499136)
Keywords: site-compat
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.
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!
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.
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.
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.
OK, makes sense, thanks.

update XHR upload content-type handling to match the spec

Component: DOM → DOM: Core & HTML
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

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

Flags: needinfo?(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

Flags: needinfo?(twisniewski)
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
Status: REOPENED → RESOLVED
Closed: 2 years ago1 year ago
Resolution: --- → FIXED
Target Milestone: mozilla64 → mozilla68
You need to log in before you can comment on or make changes to this bug.