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

Access control is unnecessary #65

Closed
davidbarratt opened this issue Apr 4, 2020 · 4 comments · Fixed by #70
Closed

Access control is unnecessary #65

davidbarratt opened this issue Apr 4, 2020 · 4 comments · Fixed by #70

Comments

@davidbarratt
Copy link
Contributor

davidbarratt commented Apr 4, 2020

Problem
This library attempts to control access to ensure that the request meets the allowable criteria under the config. However, this isn't necessary because the browser already does this. The most restrictive thing you can do, is doing nothing. CORS is about allowing things, not restricting them.

The preflight cannot even be cached because it has to varied by Access-Control-Request-Headers. However this isn't necessary, the config is the same regardless of what is requested.

Proposed Solution
Remove access checking which is unnecessary and breaks caching by forcing the request to be varied by the Origin.

@barryvdh
Copy link
Collaborator

barryvdh commented Apr 5, 2020

The only case that access needs to be check if you're using both allowCredentials and a wilcard origin. Because this is not supported by the CORS spec, we return that exact origin. But that is probably the only case in which we actually need to check the header (and set Vary: origin)

@barryvdh
Copy link
Collaborator

barryvdh commented Apr 5, 2020

Can you check out #70 ?
I think that solves it while taking into account dynamic Origins for supportsCredentials and patterns.

@davidbarratt
Copy link
Contributor Author

The only case that access needs to be check if you're using both allowCredentials and a wilcard origin. Because this is not supported by the CORS spec, we return that exact origin. But that is probably the only case in which we actually need to check the header (and set Vary: origin)

This case would always need the origin varried, but it doesn't need to return a failing response. If the preflight doesn't include Access-Control-Allow-Credentials it wont make the request, if it's a simple request, then the response becomes opaque.

Though if the user is specifying a wildcard for the origins, then they wouldn't be able to get to the failed response in the first place becuase they are allowing it for all origins (regardless if the browser supports a response like that).

@barryvdh
Copy link
Collaborator

barryvdh commented Apr 5, 2020

Yeah #70 always adds the Vary header for that case, and checks the origin before setting header. In the other case, no Vary is set and no check is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment