845881 - HTTP status 204 is considered a preflight failure in FX 20 beta 1

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Stephane "Twidi" Angel]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130223010342

Steps to reproduce:

I tried to do a XHR requests with CORS, passing a header normally authorized by "Access-Control-Allow-Headers" (here, "Content-Type")

I wrote a short html test case: http://twidi.com/test-cors-fx20.html


Actual results:

The XHR returned an empty error in Firefox 20, but it was ok in previous versions.




Expected results:

The XHR should call the GET method to get the data, not fire an error.

Note that if i use a header not listed in ""Access-Control-Allow-Headers", i have an XHR error in version 20 and others, as attended.

Comment 1

[Stephane "Twidi" Angel]

I forgot something:

The "Access-Control-Allow-Headers" is found in the response headers of the OPTIONS call, called  only because I added a header (automatically added as "Access-Control-Request-Header" in the OPTIONS call) in the request

Without the additional header, I have a correct GET call with the wanted data.

Comment 2

[Mathieu Pillard [:mat]]

Some additional information about what's going on when using the testcase provided in the URL:

With FF19:
- If Include header is not checked, FF does the GET request directly
- If Include header is checked, FF does an OPTIONS request first, resulting in a 204 No Content with the Access-Control-Allow-Headers set correctly, FF then does the GET request

(The OPTIONS part matches the CORS documentation, since it happens when add a custom header to the XHR call. See https://developer.mozilla.org/en-US/docs/HTTP/Access_control_CORS#Preflighted_requests )

With FF20 Beta 1:
- If Include header is not checked, FF does the GET request directly
- If Include header is checked, FF does an OPTIONS request first, resulting in a 204 No Content with the Access-Control-Allow-Headers set correctly, but then does *not* do the GET request.

Comment 3

[Anthony Ricaud (:rik)]

Anne: See comment 2, what should be the right behaviour?

Comment 4

[Boris Zbarsky [:bzbarsky]]

The behavior changed in this checkin range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20ec9014f220&tochange=d8e4f06198dc

The relevant change in there is bug 814093.  Preflight requests must get a 200 response, while this server is sending 204.  We used to allow that, but per spec that should fail.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I should note that this patch also fails in Opera, though not in WebKit-based browsers.  I filed https://bugs.webkit.org/show_bug.cgi?id=111008

Anne, we should probably make sure there are tests for this in the CORS test suite....  Alternately, perhaps the spec should be changed to allow 204 here.

Comment 6

[Anne (:annevk)]

Odin does the test suite.

I would be fine with updating the specification, but then we should probably test if other response codes result in the same behavior.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So the one thing is, we need to decide what we're doing before we ship 20.  Shipping the behavior change and then undoing it is silly.  I have to admit I had naively assumed the spec was based on actual testing of UAs... <sigh>.

At first glance I don't see any checks at all for HTTP status in the CORS/preflight specific code in WebKit, though it's possible they happen in some lower level somehow.

I tried IE9, but the test page here doesn't seem to work in it even when I force it into IE9 standards mode....

Comment 8

[Boris Zbarsky [:bzbarsky]]

Adam, do you happen to know what WebKit is actually doing here offhand?

Comment 9

[Stephane "Twidi" Angel]

Thanks for taking this problem into consideration (and to made it more precise)

About IE9, i wrote the test case for FX only, the test must use "XDomainRequest" for IE8&9.

Do you want me to update the page to be able to run in IE ? (but I won't be able to test it)

PS: you can check with http://client.cors-api.appspot.com/client too.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Do you want me to update the page to be able to run in IE ? 

Let's not worry about that for the moment; if it needs a different codepath it's not that relevant for compat purposes.

IE10 does support XHR with CORS, and allows non-200 status codes.

Given that I just noticed the broken server here is github, I think the spec just needs to change.  I'll be backing out the patch for bug 814093 until the spec gets sorted out.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Go back to allowing any 2xx response code to a preflight request, since web sites do that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Need to get this on all branches....

Comment 13

[Odin Hørthe Omdal (odinho, Opera)]

Yes, I seem to remember these.

WebKit behaviour is just very strange. :-) 204 is not really that strange, but it accepts other things as far as I've seen.
http://odinho.html5.org/CORS/testsuite-report.html

There is a test for this in the testsuite, and I guess you fixed all of the fails there for 20:
http://test.s0.no/w3c-tests/webappsec/tests/cors/submitted/opera/staging/status-async.htm

(aside: I got some very good feedback on the testsuite from you guys, but I haven't had any free time to do the updates. And anyway the w3c-test server is broken so preflights doesn't work because OPTIONS doesn't get sent down to PHP.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

I'm not sure what to make of that report...  It claims that we fail all the "status after preflight" tests in 20... was it using an old build of some sort?

In any case, we're going to go back to accepting any 2xx here for now, until the spec is sorted out.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

204 definitely seems like it should work per the http spec. The preflight response doesn't contain an entity-body and so 204 makes a lot of sense for the server to emit.

I'm fine with either accepting all 2xx responses, or only accepting 200 and 204.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd say it's up to Anne :)

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 719462 [details] [diff] [review]
Go back to allowing any 2xx response code to a preflight request, since web sites do that.

Review of attachment 719462 [details] [diff] [review]:
-----------------------------------------------------------------

r=me though if this is the way we want to go.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I think this is the way we want to go.  Certainly for branches.

http://hg.mozilla.org/integration/mozilla-inbound/rev/b8a59825d718

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 719462 [details] [diff] [review]
Go back to allowing any 2xx response code to a preflight request, since web sites do that.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 814093 
User impact if declined: Some XHR API calls may not work right (in particular, to
   GitHub's API).
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): Low risk: just backs out
   bug 814093
String or UUID changes made by this patch: None.

Comment 20

[Odin Hørthe Omdal (odinho, Opera)]

> I'm not sure what to make of that report...  It claims that we fail all the "status after preflight" tests in
> 20... was it using an old build of some sort?

Oh, yes, sorry, I should've said on the page what "real" versions they were. I were testing nightlies for everyone. Or as close as I could come. So that was obviously a bogus number, -- aurorafirefox 20 + the date would obviously be better. Anyway, now you know why ;-)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[bhavana bajaj [:bajaj]]

Comment on attachment 719462 [details] [diff] [review]
Go back to allowing any 2xx response code to a preflight request, since web sites do that.

low risk, backout patch. Approving for uplift.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc030949013c
https://hg.mozilla.org/releases/mozilla-beta/rev/de9292e91f8b

Comment 24

[Virgil Dicu [:virgil] [QA]]

Verified the error in Firefox 20.0 beta 1 (20130220104816) by launching the testcase from comment 0.

The testcase is successfully for Firefox 20.0 beta 3 (20130305164032).

Verified for Windows 7 and Mac OS X 10.8. 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0

Comment 25

[ukaniow]

The same problem exist in actual version of FF when you make POST request with json data. When server response 204 Firfox log CORS error.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Please file a bug with specific steps to reproduce (and in particular, the exact way the POST request is made, the exact HTTP headers returned, and the exact error you are seeing are needed) and cc me on that bug.