-
Notifications
You must be signed in to change notification settings - Fork 463
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
[Feature request] A more powerful custom origin calculation method depending on other headers #288
Comments
I don't think that is feasable with CORS. Have you verified? Because either the |
@dougwilson Am I understanding you correctly - you're skeptical cause not all headers will be available for checking in that newly proposed origin calculation callback? If so, I still think it's good enough, cause you can at least check some headers. The most important ones are the authorization & cookie headers, so as long as those are there, that's already very helpful. |
I'm saying that AFAIK neither of those exact headers would be available, so your solution wouldn't work no matter if this was added or not. I'm asking if you verified that or not. |
@dougwilson I haven't verified this, no, it was my assumption that you'd get these headers like you would with a normal request. With this new information coming to light, the feature request might not make any sense anymore |
So the way CORS itself works (unrelated to this module) is that it's a "double-opt-in" system. The idea is that the client has no idea if the web server supports CORS and so it needs a way to probe that. What they did was create a "pre-flight" request whenever the original request was not a "safe request" -- that is, if the request is not one of the "simple verbs" and/or it contains auth-like headers like Cookie, Authortization, etc. So when that is the case, the web browser will first make that "pre-flight request" which is the veb OPTIONS and includes none of the body content and headers from the original request, and that request is where your decision needs to happen on what the Access-Control-Allow-Origin and etc. headers contain. If it doesn't contain the Origin then the web browser will fail and never make the original request. Your example wants to decide between two cases that are both "non-safe" and thus they would both use a pre-flight, at which point, it would not be possible to tell which is which case but you need to supply a Access-Control-Allow-Origin to get the original request to be made, of course. I hope that makes sense. |
It does make sense, thank you |
Currently this middleware only allows you to dynamically resolve the CORS header value depending on the origin of the incoming request, but sometimes this isn't enough and more information about the request is necessary to make the decision.
Something like this would be nice:
Here's a use case:
An app has header-based-authentication (a token is passed along with each request) and an
Access-Control-Allow-Origin: *
value, cause CSRF attacks are kind of impossible - there's no cookies to be forwarded and an attacker would have to somehow steal the token from some other origin's localStorage or something (which is impossible AFAIK) to be able to use it with an API call.But then there's a plan to also enable session-based-authentication for the same app. This changes security constraints, however, because now a
Access-Control-Allow-Origin: *
value is def going to allow CSRF attacks to happen, cause even if the API call is invoked fromattacker.com
the cookie is going to be forwarded alongside the request and will allow the attacker to use the API with the browser user's cookie.One solution to that problem (the best one IMO) is to calculate a
Access-Control-Allow-Origin
value differently depending on which authentication method is used. If there's anAuthorization
header, thenAccess-Control-Allow-Origin: *
is fine, but if there's a session cookie header instead, thenAccess-Control-Allow-Origin
will have to be more limited and only allow specific approved origins.If needed I can do a PR for this, just let me know if you accept the premise and will merge it once it's done
The text was updated successfully, but these errors were encountered: