XMLHttpRequest: Content-Type can require a preflight #5070

annevk · 2017-03-08T11:12:01Z

Supersedes #4801.

wpt-pr-bot · 2017-03-08T11:12:03Z

Notifying @Manishearth, @caitp, @emilio, @hallvors, @ibelem, @jdm, @jungkees, @kangxu, @mathiasbynens, @ronkorving, and @wisniewskit. (Learn how reviewing works.)

annevk · 2017-03-08T11:17:27Z

@bzbarsky you may want to review this since you last modified this test in https://bugzilla.mozilla.org/show_bug.cgi?id=1328761. I also think you may have introduced a same-origin policy violation in Firefox since you can't just preserve Content-Type when its value is not safelisted as per https://fetch.spec.whatwg.org/#cors-safelisted-request-header. I'm happy to file a bug by the way.

(As a side effect Chrome now passes all these tests and Safari only fails two.)

ghost · 2017-03-08T11:19:01Z

View the complete job log.

Firefox (nightly channel)

Testing web-platform-tests at revision d94eee3f3d94c3bfa544ad41b53fa209a985f0f7
Using browser at version BuildID 20170306110339; SourceStamp 966464a68a2cb3ca1125808e34abb5c1d34e3797
Starting 10 test iterations

ghost · 2017-03-08T11:19:02Z

View the complete job log.

Chrome (unstable channel)

Testing web-platform-tests at revision d94eee3f3d94c3bfa544ad41b53fa209a985f0f7

bzbarsky · 2017-03-08T22:31:55Z

I also think you may have introduced a same-origin policy violation in Firefox since you can't just preserve Content-Type when its value is not safelisted

My change linked to above just preserved the "Content-Type not set" state across redirects, instead of converting it to "Content-Type set to empty string", which is what Firefox used to do before my change. It didn't change the behavior of any cases in which the header was set.

Looking at Firefox code, we use the concept at https://fetch.spec.whatwg.org/#cors-safelisted-request-header to decide whether to preflight or not. I'm not sure whether we use it for anything else. Bug report is probably a good idea. That said, I see nothing in the spec about special handling of these on redirect per se, just in terms of checking what the preflight response looks like, right?

bzbarsky · 2017-03-08T22:32:02Z

XMLHttpRequest/send-redirect-to-cors.htm

-        }
-
-        if (typeof body == "string") {
+        } else if (typeof body == "string") {


Why the else after return here and below?

bzbarsky · 2017-03-08T22:32:19Z

XMLHttpRequest/send-redirect-to-cors.htm

          var arr = Array.prototype.slice.call(body);
          return { body: String.fromCharCode.apply(null, arr), type: "NO" }
+        } else {
+          return { body: "EXTRACT NOT IMPLEMENTED", type: "EXTRACT NOT IMPLEMENTED" }


And this long line instead of the more readable two lines?

We usually use 100 columns.

I guess I can revert these changes though, they are editorial only.

Fwiw, the wrapping of the body/type bits wasn't because it was too wide; it was just because it's more readable (for me at least) to have the different props on different lines.... I don't feel super-strongly about it, though.

bzbarsky · 2017-03-08T22:32:50Z

XMLHttpRequest/send-redirect-to-cors.htm

-              if (client.readyState == 4) {
+          client.onreadystatechange = t.step_func(() => {
+            if (client.readyState == 4) {
+              if(!setExplicitType) {


Space after "if".

bzbarsky · 2017-03-08T22:35:02Z

XMLHttpRequest/send-redirect-to-cors.htm

+                assert_equals(client.getResponseHeader("x-request-content-type"), expectedType);
+                assert_equals(client.getResponseHeader("x-request-data"), expectedBody);
+              } else {
+                // Content-Type is not safelisted by corsenabled.py -> network error


We should probably have a test for the case in which it is safelisted.

We should also have a test for the case in which it's not safelisted but one of the safe values was set.

The latter is already covered by the text/plain example.

We have an example that makes an explicit setRequestHeader with text/plain as the value?

No we don't. Per the standard they're equivalent, but I suppose it doesn't hurt to have both.

annevk · 2017-03-09T08:11:45Z

That said, I see nothing in the spec about special handling of these on redirect per se, just in terms of checking what the preflight response looks like, right?

Yeah, the problem is that Firefox doesn't appear to do a preflight or maybe it does a preflight, but does not require Content-Type to be listed in the response.

bzbarsky · 2017-03-09T08:22:49Z

Hrm. Looking at the code some more, it really looks like this should work in Firefox; we're using one place both to decide that this header needs a preflight and that it's unsafe and needs whitelisting Please do get that bug filed!

bzbarsky · 2017-03-09T09:07:12Z

Actually, I filed it. https://bugzilla.mozilla.org/show_bug.cgi?id=1345788

annevk · 2017-03-09T09:08:38Z

Thanks, I was planning on getting there, still working through all my GitHub backlog.

bzbarsky · 2017-03-09T09:09:15Z

Yeah, I got sucked into debugging it, so figured I might as well file with the info I had. ;)

Supersedes #4801.

annevk · 2017-03-09T11:56:04Z

I think I covered all bases now. I'll file a bug on WebKit once you approve this.

bzbarsky · 2017-03-09T17:22:16Z

XMLHttpRequest/send-redirect-to-cors.htm

          }
          client.send(body)
-        })
+        }, document.title + " (" + name + ")")
      }
      redirect("301")


Didn't you change behavior for this call? It used to set the Content-Type header, but now no longer does. Is that purposeful?

Yeah, because that made them all fail, but I'll add some more tests.

bzbarsky · 2017-03-09T17:25:01Z

And in particular, we no longer have GET tests with explicit content-type in there now, right?

annevk · 2017-03-09T18:05:34Z

Okay, I covered some more ground.

bzbarsky · 2017-03-09T18:05:55Z

Looks great, thanks!

annevk · 2017-03-09T18:11:05Z

Filed https://bugs.webkit.org/show_bug.cgi?id=169420 on the WebKit failures.

wpt-pr-bot added the xhr label Mar 8, 2017

bzbarsky reviewed Mar 8, 2017

View reviewed changes

XMLHttpRequest: Content-Type can require a preflight

3d29fdd

Supersedes #4801.

annevk force-pushed the annevk/xhr-content-type-cross-origin branch from e654de1 to 3d29fdd Compare March 9, 2017 11:26

annevk added 3 commits March 9, 2017 12:29

address nits

c109a5c

test setRequestHeader() with safe listed values

b6d435b

Also add tests where Content-Type is safelisted by the server

f993716

bzbarsky reviewed Mar 9, 2017

View reviewed changes

add some more tests

9e74601

annevk merged commit 98d5840 into master Mar 9, 2017

annevk deleted the annevk/xhr-content-type-cross-origin branch March 9, 2017 18:11

jdm mentioned this pull request Apr 21, 2020

XHR and fetch tests for redirect-to-cors behaviour don't match #23156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XMLHttpRequest: Content-Type can require a preflight #5070

XMLHttpRequest: Content-Type can require a preflight #5070

annevk commented Mar 8, 2017

wpt-pr-bot commented Mar 8, 2017

annevk commented Mar 8, 2017

ghost commented Mar 8, 2017 •

edited by ghost

Loading

ghost commented Mar 8, 2017 •

edited by ghost

Loading

bzbarsky commented Mar 8, 2017 •

edited

Loading

bzbarsky Mar 8, 2017

bzbarsky Mar 8, 2017

annevk Mar 9, 2017

annevk Mar 9, 2017

bzbarsky Mar 9, 2017

bzbarsky Mar 8, 2017

bzbarsky Mar 8, 2017

annevk Mar 9, 2017

bzbarsky Mar 9, 2017

annevk Mar 9, 2017

annevk commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

annevk commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

annevk commented Mar 9, 2017

bzbarsky Mar 9, 2017

annevk Mar 9, 2017

bzbarsky commented Mar 9, 2017

annevk commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

annevk commented Mar 9, 2017

XMLHttpRequest: Content-Type can require a preflight #5070

XMLHttpRequest: Content-Type can require a preflight #5070

Conversation

annevk commented Mar 8, 2017

wpt-pr-bot commented Mar 8, 2017

annevk commented Mar 8, 2017

ghost commented Mar 8, 2017 • edited by ghost Loading

Firefox (nightly channel)

ghost commented Mar 8, 2017 • edited by ghost Loading

Chrome (unstable channel)

bzbarsky commented Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

annevk commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

annevk commented Mar 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky commented Mar 9, 2017

annevk commented Mar 9, 2017

bzbarsky commented Mar 9, 2017

annevk commented Mar 9, 2017

ghost commented Mar 8, 2017 •

edited by ghost

Loading

ghost commented Mar 8, 2017 •

edited by ghost

Loading

bzbarsky commented Mar 8, 2017 •

edited

Loading