CORS preflight with a non-200 response should be a preflight failure

[Boris Zbarsky]

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.

[Alexey Proskuryakov]

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.

[Boris Zbarsky]

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.

[Boris Zbarsky]

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.

[Alexey Proskuryakov]

What is the story with 3xx redirects? Unfortunately, I lost track of changes to redirect behavior some time ago.

[Boris Zbarsky]

3xx handling for preflights is already specially defined in the spec; I believe it's a preflight failure....

[youenn fablet]

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.

[youenn fablet]

Created attachment 281355 [details]
Patch

[youenn fablet]

Created attachment 281359 [details]
Fixing w3c test

[Darin Adler]

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?

[youenn fablet]

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.

[youenn fablet]

Created attachment 281554 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 281554 [details]
Patch for landing

Clearing flags on attachment: 281554

Committed r202162: <http://trac.webkit.org/changeset/202162>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.