Adjust XMLHttpRequest Content-Type handling

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
8 days ago

People

(Reporter: annevk, Assigned: twisniewski)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 disabled, firefox68 fixed)

Details

Attachments

(2 attachments)

Reporter

Description

Last year
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.
Reporter

Updated

Last year
Priority: -- → P3
Assignee

Comment 1

9 months 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.
Flags: needinfo?(annevk)
Reporter

Comment 2

9 months 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.
Flags: needinfo?(annevk)
Assignee

Comment 3

9 months 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?
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)
Reporter

Comment 5

9 months 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.
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.
Assignee

Comment 7

9 months ago
have XHRs adjust content type of uploads per spec using the MIME Sniffing standard
Assignee

Comment 8

9 months 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
Assignee

Comment 9

9 months ago
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

Comment 13

9 months 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
Keywords: checkin-needed

Comment 14

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ca06d80acf9b
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee: nobody → twisniewski

Updated

9 months ago
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)
Assignee

Updated

8 months ago
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.
Reporter

Comment 20

8 months 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.
Reporter

Comment 21

8 months 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?
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)
Reporter

Comment 23

8 months 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...
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.
Reporter

Comment 26

8 months 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.
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!
Reporter

Comment 28

8 months 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.
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.
Reporter

Comment 30

8 months 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.
OK, makes sense, thanks.

update XHR upload content-type handling to match the spec

Component: DOM → DOM: Core & HTML

Comment 34

3 months 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

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)

Comment 37

3 months 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

Comment 38

3 months ago
bugherder
Status: REOPENED → RESOLVED
Closed: 9 months ago3 months ago
Resolution: --- → FIXED
Target Milestone: mozilla64 → mozilla68
You need to log in before you can comment on or make changes to this bug.