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

Set explicit Origin in CORS preflight response if allow_credentials is True and allow_origins is wildcard #1113

Merged
merged 9 commits into from Apr 14, 2021

Conversation

jcwilson
Copy link
Contributor

@jcwilson jcwilson commented Dec 11, 2020

When a non-simple request will include credentials, the browser will likely send a CORS preflight request.
The browser will fail the request unless the preflight response explicitly allows the request's Origin in the
Access-Control-Allow-Origin response header rather than *.

This can be resolved by updating the CORS middleware to respond to preflight requests with the explicit
origin from the request rather than the literal * wildcard value when allow_credentials=True.

The current workaround is to provide an equivalently permissive allow_origin_regex pattern like ".*",
but this would appear 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-preflight
requests:

# If request includes any cookie headers, then we must respond
# with the specific origin instead of '*'.
if self.allow_all_origins and has_cookie:
    headers["Access-Control-Allow-Origin"] = origin

This PR just updates the preflight logic to match the simple request logic. The difference is that preflight
requests won't contain cookies so we can't perform the same has_cookie check. That's why we'll rely
on the allow_credentials setting.

See the relevant MDN CORS docs here:

When used as part of a response to a preflight request, this indicates whether or not the actual request can
be made using credentials. Note that simple GET requests are not preflighted, and so if a request is made for
a resource with credentials, if this header is not returned with the resource, the response is ignored by the
browser and not returned to web content.

Note: This could be considered an interface-breaking change and demand a stronger version bump if starlette
follows a strict semver policy.
Servers that currently specify allow_origins="*" instruct the browser to
block credentialed CORS calls that require 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?

Josh Wilson added 2 commits December 11, 2020 02:21
…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.
@JayH5 JayH5 added the cors Cross-Origin Resource Sharing label Feb 4, 2021
@jcwilson
Copy link
Contributor Author

jcwilson commented Apr 7, 2021

This is the error one currently gets when attempting to make a CORS request with credentials and allow_origins=["*"]

image

@jcwilson
Copy link
Contributor Author

jcwilson commented Apr 7, 2021

@JayH5 thanks for taking care of the other CORS PRs. Let me know if there's anything I can do to help out with this one. I know CORS is such a headache to context switch to.

@jcwilson

This comment has been minimized.

Josh Wilson added 2 commits April 6, 2021 18:38
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.
starlette/middleware/cors.py Outdated Show resolved Hide resolved
Josh Wilson added 2 commits April 6, 2021 23:14
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.
preflight_headers.update(
{
"Access-Control-Allow-Methods": ", ".join(allow_methods),
"Access-Control-Max-Age": str(max_age),
}
)
allow_headers = sorted(SAFELISTED_HEADERS | set(allow_headers))
if allow_headers and "*" not in allow_headers:
if allow_headers and not allow_all_headers:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't allow_headers redundant here since it's never an empty list? This could be reduced to this, right?:

if not allow_all_headers:
    ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it looks unused.

Also a bit concerned about headers with mixed case in the Access-Control-Allow-Headers header, but if it is an issue it's a separate one.

Copy link
Member

@JayH5 JayH5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcwilson been meaning to say: this looks good to me, but I don't feel that I have enough experience with CORS to say what the best practices are, especially w.r.t. allow_origins="*". If another member of @encode/maintainers is happy to sign-off on this then so am I. Alternatively, if you can find precedence for this behaviour in any commonly used libraries, especially in the Flask or Django space, that would help.

preflight_headers.update(
{
"Access-Control-Allow-Methods": ", ".join(allow_methods),
"Access-Control-Max-Age": str(max_age),
}
)
allow_headers = sorted(SAFELISTED_HEADERS | set(allow_headers))
if allow_headers and "*" not in allow_headers:
if allow_headers and not allow_all_headers:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it looks unused.

Also a bit concerned about headers with mixed case in the Access-Control-Allow-Headers header, but if it is an issue it's a separate one.

@jcwilson
Copy link
Contributor Author

jcwilson commented Apr 10, 2021

Alternatively, if you can find precedence for this behaviour in any commonly used libraries, especially in the Flask or Django space, that would help.

Sure thing. Let me get back to you on that. I've been working more closely with the envoy-proxy implementation lately, but I'll see what else I can dig up.

@jcwilson
Copy link
Contributor Author

jcwilson commented Apr 11, 2021

I don't believe envoy-proxy will ever send back * on preflight requests, regardless of the allow-credentials setting.

Django via django-cors-headers performs nearly identical logic to what this PR contains.

Flask-CORS seems to over-complicate things a bit, but you'd have to specifically force it to try to send the literal * value and even then it checks for that and prevents that misconfiguration. The actual behavior when configured with origins="*" doesn't even look at whether credentials are allowed or not, it will just reflect back the request's origin. (If one were to set send_wildcard to attempt to get it to send *, it would trigger the above error before getting to that point.)

Copy link
Member

@JayH5 JayH5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I'm happy with this, especially with the django-cors-headers precedent.

@JayH5 JayH5 merged commit 995d70c into encode:master Apr 14, 2021
@JayH5
Copy link
Member

JayH5 commented Apr 14, 2021

Thank you!

@jcwilson
Copy link
Contributor Author

Thank you, too. And @euri10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cors Cross-Origin Resource Sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants