forked from encode/starlette
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Set explicit Origin in CORS preflight response if allow_credentials is True and allow_origins is wildcard #2
Open
jcwilson
wants to merge
9
commits into
master
Choose a base branch
from
cors-preflight-allow-credentials
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
14fae82
to
4b1fc4e
Compare
ece46e3
to
f3ab6a6
Compare
…s True and allow_origins is wildcard When making a preflight request, the browser makes no indication as to whether the actual subsequent request will pass up credentials. However, unless the preflight response explicitly allows the request's `Origin` in the `Access-Control-Response-Header`, the browser will fail the CORS check and prevent the actual follow-up CORS request. This means that responding with the `*` wildcard is not sufficient to allow preflighted credentialed requests. The current workaround is to provide an equivalently permissive `allow_origin_regex` pattern. The `simple_response()` code already performs similar logic which currently only applies to non-preflighted requests since the browser would never make a preflighted request that hits this code due to this issue: ``` if self.allow_all_origins and has_cookie: headers["Access-Control-Allow-Origin"] = origin ``` This just bring the two halves inline with each other.
f3ab6a6
to
1a28cce
Compare
This simplifies the code slightly by using this recently added method. It has some trade-offs, though. We now construct a `MutableHeaders` instead of a simple `dict` when copying the pre-computed preflight headers, and we move the `Vary` header construction out of the pre-computation and into the call handler. I think it makes the code more maintainable and the added per-call computation is minimal.
This also names and caches some of the boolean tests in __init__() which we use in later if-blocks. This follows the existing pattern in order to better self-document the code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When making a preflight request, the browser provides no indication as to whether the actual subsequent
CORS request will pass up credentials (read: cookies). However, unless the preflight response explicitly allows
the request's
Origin
in theAccess-Control-Response-Header
, the browser will fail the CORS check andprevent the actual follow-up CORS request. This means that responding to preflight requests with the
*
wildcard when
allow_credentials=True
is not sufficient to allow preflighted credentialed requests.The current workaround is to provide an equivalently permissive
allow_origin_regex
pattern, but this wouldappear to be a POLA violation. If one wishes to allow calls from all origins, regardless of the
allow_credentials
setting, then
allow_origins=["*"]
should be sufficient to express that intent.The
simple_response()
code already performs similar logic which currently only applies to non-preflightedrequests since the browser would never make a preflighted request that hits said code due to this issue:
This PR just brings the two halves inline with each other.
See the relevant MDN CORS docs here:
Note: This could be considered an interface-breaking change and demand a stronger version bump if starlette
follows a strict semver policy. Servers that that currently specify
allow_origins="*"
instruct the browser toblock credentialed CORS calls that required preflight authorization. Adopting this behavior would allow these
calls through and could result in an unexpected relaxation of their current effective security rules. However, it
looks like starlette is still pre-1.0.0, so maybe this just means a minor version bump?