Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CO Redirect in the face of CORs may not be correct in specs #32

Closed
toddreifsteck opened this issue Jun 27, 2016 · 10 comments
Closed

CO Redirect in the face of CORs may not be correct in specs #32

toddreifsteck opened this issue Jun 27, 2016 · 10 comments
Assignees
Milestone

Comments

@toddreifsteck
Copy link
Member

Ilya created a very useful test at:
http://output.jsbin.com/suxagi/latest/quiet

It demonstrates a Cross Origin Redirect encountering CORs in Firefox/Microsoft Edge which is correct per Fetch spec at time of this issue being created. HTTP Fetch 5.3, point 5.4 which instructs a network error to occur per the current Beacon/Fetch specs.

Chrome does not follow the current specs and allows the redirect.

There are some large web properties evaluating a move to sendBeacon and this issue was encountered by them in private communication with Ilya and he raised it with each browser vendor. As it seems to require a spec update, I've moved the issue to the spec issues list.

The question for the specs is this:
Is the intent of the ACAO header to protect only the body? (Which seems to require a Beacon/Fetch spec update)
OR is it intended to protect redirects as well? (As written)

@igrigorik
Copy link
Member

Related FF bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1280692. As noted by Cristoph in that thread, we have some confusion between various specs:

https://www.w3.org/TR/cors/#cors-api-specification-redirect

Since browsers are based on a same origin security model and the policy outlined in this specification is intended for APIs used in browsers, it is expected that APIs that will utilize this policy will have to handle a same origin request that results in a redirect that is cross-origin in a special way.

For APIs that transparently handle redirects CORS API specifications are encouraged to handle this scenario transparently as well by "catching" the redirect and invoking the cross-origin request algorithm on the (cross-origin) redirect URL.

https://fetch.spec.whatwg.org/#http-fetch

5.4 implies an error should be returned
6.5 does the http-redirect-fetch


Stepping back, in term of actual functional requirements for sendBeacon: we set credentials mode to "include", mode to "cors", and redirect mode is set to "follow" by default. The intent behind these flags is to allow sites to shift away from using image pings.. which provide same semantics, but require that sites block the browser from unloading and cancelling such requests.

In practice, the redirects in particular appear to be a fairly common case: the request is bounced through different analytics services, etc. The reason this came up as an issue is that after running some trials the telemetry showed that browsers that block redirects have a lower delivery rate / worse performance than image pings.


Is the intent of the ACAO header to protect only the body? (Which seems to require a Beacon/Fetch spec update) OR is it intended to protect redirects as well? (As written)

There is also the question of whether the Set-Cookie's are processed. If we want to match the existing image-beacon use case (which, I think we should), we need to follow redirects and allow set-cookies to be processed. We don't care for the response; we don't allow access to the response body.

/cc @annevk

@toddreifsteck
Copy link
Member Author

Upon reading and re-reading both Beacon and Fetch, this could be solvable within the Beacon spec (assuming we believe this is correct beahvior for sendBeacon). We could use 'cors' for Blob and 'no-cors' for other payloads.

Otherwise, if the payload is a Blob and the resulting Content-Type is not a simple header, then a CORS preflight is made and the server needs to first allow such requests by returning the appropriate set of CORS headers (Access-Control-Allow-Credentials, Access-Control-Allow-Origin, Access-Control-Allow-Headers); same as XHR/fetch.

@igrigorik
Copy link
Member

~related: #10 (comment)

We could use 'cors' for Blob and 'no-cors' for other payloads.

As a sanity check, how would this interact with credentials mode? We set it to "include" for Beacon and if we combine that with "no-cors" for non-Blob payloads... Set-Cookie's are processed?

@annevk
Copy link
Member

annevk commented Jun 28, 2016

Yes, they would be, but using "no-cors" needs to be carefully evaluated as it's extremely easy to violate the same-origin policy as sendBeacon() apparently has already done multiple times now. 😒

@igrigorik
Copy link
Member

@annevk any tips on checklist of edge cases to consider? :)

re, set-cookies: what's confusing me is the last example in 4.2.6, which shows a fetch request with credentials set to include and (default) mode of no-cors. If I'm reading that correctly, in order for Set-Cookie to be processed, the origin would have to return both the ACAO and the ACAC headers? If so, that's basically a non-starter for replacing the current image beacons.

As a separate question: can we clarify the behavior for mode=cors and redirects? Does each redirect need to provide the various ACA* headers, or only the destination?

@annevk
Copy link
Member

annevk commented Jun 28, 2016

None of the examples there use "no-cors". The default for fetch() is "cors".

@annevk
Copy link
Member

annevk commented Jun 28, 2016

Each redirect needs to opt-in, otherwise you have information leakage.

@igrigorik
Copy link
Member

None of the examples there use "no-cors". The default for fetch() is "cors".

A request has an associated mode, which is "same-origin", "cors", "no-cors", "navigate", or "websocket". Unless stated otherwise, it is "no-cors".

From: https://fetch.spec.whatwg.org/#concept-request-mode.. I'm probably missing something obvious?

@annevk
Copy link
Member

annevk commented Jun 28, 2016

Yeah, fetch() overrides that. You're looking at the global default for all request-creating entities.

igrigorik added a commit that referenced this issue Jun 29, 2016
@igrigorik igrigorik added this to the Level 1 milestone Jun 29, 2016
@igrigorik igrigorik self-assigned this Jun 29, 2016
@igrigorik
Copy link
Member

Closing, this was resolved in #33. Also, see #34 for followup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants