Closed Bug 845881 Opened 7 years ago Closed 7 years ago

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

Categories

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

20 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox20 + verified
firefox21 + fixed
firefox22 --- fixed

People

(Reporter: s.angel, Assigned: bzbarsky)

References

()

Details

Attachments

(1 file)

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.
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.
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.
Anne: See comment 2, what should be the right behaviour?
Component: Untriaged → DOM
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
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.
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.
Flags: needinfo?(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.
Flags: needinfo?(annevk)
Flags: needinfo?(odinho)
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....
Flags: needinfo?(jonas)
Flags: needinfo?(annevk)
Summary: "Access-Control-Allow-Headers" not respected in FX 20 beta 1 → HTTP status 204 is considered a preflight failure in FX 20 beta 1
Adam, do you happen to know what WebKit is actually doing here offhand?
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.
> 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.
Assignee: nobody → bzbarsky
Blocks: 814093
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(odinho)
Flags: needinfo?(jonas)
Flags: needinfo?(annevk)
Need to get this on all branches....
Whiteboard: [need review]
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.)
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.
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 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.
Attachment #719462 - Flags: review?(jonas) → review+
I think this is the way we want to go.  Certainly for branches.

http://hg.mozilla.org/integration/mozilla-inbound/rev/b8a59825d718
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla22
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.
Attachment #719462 - Flags: approval-mozilla-beta?
Attachment #719462 - Flags: approval-mozilla-aurora?
> 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 ;-)
https://hg.mozilla.org/mozilla-central/rev/b8a59825d718
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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.
Attachment #719462 - Flags: approval-mozilla-beta?
Attachment #719462 - Flags: approval-mozilla-beta+
Attachment #719462 - Flags: approval-mozilla-aurora?
Attachment #719462 - Flags: approval-mozilla-aurora+
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

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.

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.

Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.