Bug 111008

Summary: CORS preflight with a non-200 response should be a preflight failure
Product: WebKit Reporter: Boris Zbarsky <bzbarsky>
Component: DOMAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, achristensen, ap, cdumez, commit-queue, darin, japhet, mkwst, s.angel, youennf
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=158425
https://bugs.webkit.org/show_bug.cgi?id=158984
Attachments:
Description Flags
incomplete patch
none
Patch
none
Fixing w3c test
none
Patch for landing none

Description Boris Zbarsky 2013-02-27 13:51:11 PST
http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0 says:

  If the response has an HTTP status code that is not 200

    Apply the network error steps.

Neither Chrome nor Safari seem to do this, based on the testcase at http://twidi.com/test-cors-fx20.html (which returns a 204 instead).

This works correctly in Opera and in the Firefox 20 betas.
Comment 1 Alexey Proskuryakov 2013-03-01 13:54:23 PST
Created attachment 191030 [details]
incomplete patch

Interesting idea to try, despite being incompatible with all browsers. Do I infer it correctly that this breaks github?

The attached patch makes this change and adds a test case. However multiple existing tests start to fail, and will need to be carefully updated to ensure that they keep testing what they used to.
Comment 2 Boris Zbarsky 2013-03-01 14:51:13 PST
Note that I think it's not worth doing the 200-only thing at this point.  I think WebKit _should_ stop accepting 5xx and 4xx preflights, however: there are no legitimate uses and it's additional attack surface.
Comment 3 Boris Zbarsky 2013-03-01 14:51:52 PST
So basically I propose limiting to either 2xx or "200 or 204".  Which of those to do is still up in the air; if you have opinions please post to public-webappsec.
Comment 4 Alexey Proskuryakov 2013-03-01 15:03:27 PST
What is the story with 3xx redirects? Unfortunately, I lost track of changes to redirect behavior some time ago.
Comment 5 Boris Zbarsky 2013-03-01 15:05:14 PST
3xx handling for preflights is already specially defined in the spec; I believe it's a preflight failure....
Comment 6 youenn fablet 2016-06-07 00:02:04 PDT
Step 5.7 of https://fetch.spec.whatwg.org/#cors-preflight-fetch requires the preflight response to be a ok response (2XX code).
That makes sense to me.
Comment 7 youenn fablet 2016-06-15 06:29:10 PDT
Created attachment 281355 [details]
Patch
Comment 8 youenn fablet 2016-06-15 07:23:26 PDT
Created attachment 281359 [details]
Fixing w3c test
Comment 9 Darin Adler 2016-06-16 08:51:48 PDT
Comment on attachment 281359 [details]
Fixing w3c test

View in context: https://bugs.webkit.org/attachment.cgi?id=281359&action=review

> Source/WebCore/platform/network/ResourceResponseBase.h:74
> +    bool isSuccessful() const { return httpStatusCode() >=200 && httpStatusCode() < 300; }

Missing space after "=".

Since httpStatusCode() is not an inline function, we should put its result in a local variable rather than calling it twice.

What does this function need to do, to do the right thing for non-HTTP requests?
Comment 10 youenn fablet 2016-06-17 03:00:27 PDT
Thanks for the review.

(In reply to comment #9)
> Comment on attachment 281359 [details]
> Fixing w3c test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281359&action=review
> 
> > Source/WebCore/platform/network/ResourceResponseBase.h:74
> > +    bool isSuccessful() const { return httpStatusCode() >=200 && httpStatusCode() < 300; }
> 
> Missing space after "=".

OK

> Since httpStatusCode() is not an inline function, we should put its result
> in a local variable rather than calling it twice.

I will put isSuccessful in the cpp file then.

> What does this function need to do, to do the right thing for non-HTTP
> requests?

"successful" is defined by the HTTP specification (https://tools.ietf.org/html/rfc7231#section-6.3), so it is probably http specific.

"ok" is defined by the fetch spec (https://fetch.spec.whatwg.org/#statuses).
Maybe we should reuse that term. I recall that alex prefered successful, hence why I chose it.

I am wondering whether there are cases where "0" http status code means success as well.
Comment 11 youenn fablet 2016-06-17 03:04:17 PDT
Created attachment 281554 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2016-06-17 03:34:04 PDT
Comment on attachment 281554 [details]
Patch for landing

Clearing flags on attachment: 281554

Committed r202162: <http://trac.webkit.org/changeset/202162>
Comment 13 WebKit Commit Bot 2016-06-17 03:34:09 PDT
All reviewed patches have been landed.  Closing bug.