Summary: | CORS preflight with a non-200 response should be a preflight failure | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Boris Zbarsky <bzbarsky> | ||||||||||
Component: | DOM | Assignee: | 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
Boris Zbarsky
2013-02-27 13:51:11 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.
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. 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. What is the story with 3xx redirects? Unfortunately, I lost track of changes to redirect behavior some time ago. 3xx handling for preflights is already specially defined in the spec; I believe it's a preflight failure.... 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. Created attachment 281355 [details]
Patch
Created attachment 281359 [details]
Fixing w3c test
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? 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. Created attachment 281554 [details]
Patch for landing
Comment on attachment 281554 [details] Patch for landing Clearing flags on attachment: 281554 Committed r202162: <http://trac.webkit.org/changeset/202162> All reviewed patches have been landed. Closing bug. |