CORSMiddleware does not provide explicit origin although Authorization header is present

[Kludex]

Discussed in #1823

^{Originally posted by gyusang August 26, 2022}
When sending a CORS request with credentials, wildcard origin is rejected by the standard.
The CORS middleware handles this case when cookies are included, but is missing the case when Authorization header is present.
https://github.com/encode/starlette/blob/31164e346b9bd1ce17d968e1301c3bb2c23bb418/starlette/middleware/cors.py#L164-L165
Since Token authentication is also widely used these days, I believe explicit header should be returned when Authorization header is present.

Important

• We're using Polar.sh so you can upvote and help fund this issue.
• We receive the funding once the issue is completed & confirmed by you.
• Thank you in advance for helping prioritize & fund our backlog.

[Kludex]

Reference: https://fetch.spec.whatwg.org/#cors-protocol-and-credentials

[Kludex]

More reference: a rust package that I've found raises an error when allow_credentials is True, and the server tries to send * on allow_origins.

https://github.com/lawliet89/rocket_cors/blob/54fae0701dffbe5df686465780218644ee3fae5f/src/lib.rs#L1131-L1138

This is not an argument to raise an error, it's just saying that we are not complying with W3C.

[gyusang]

Yes, I agree that this option is not compliant with W3C.
Allowing credentials to all origins is unsafe, as it cannot prevent phishing websites from gathering credentials from users then requesting CORS with credentials.
Still, "allowing all origins all credentials" is an attractive option especially in the development phase, where multiple hosts (vercel, localhost, multiple subdomains) that needs credential-allowed CORS are created and destroyed frequently.
Then, we have to choose one of the following policy.

1. Be strict, raise an error when allowing credentials to all origin
2. Be cautious, throw a warning when allowing credentials to all origin
3. Be quiet, respect the configured option and allow credentials to all origin

If we choose policy 2 or 3 and the configuration allows credentials to all origins, we have to choose and implement one of the following mechanism to ensure that all requests including credentials are allowed.
A. If the configuration allows credentials to all origins, always respond with explicit Access-Control-Allow-Origin headers
B. If the configuration allows credentials to all origins, and the request includes credentials(either using the cookie or the Authorization header), respond with explicit Access-Control-Allow-Origin headers
C. Regardless of the configuration, if the request includes credentials(either using the cookie or the Authorization header), respond with explicit Access-Control-Allow-Origin headers

@Kludex pointed out that there are implementations using mechanism A, but no implementation that depends on the request header(e.g. B, C). Since the original code was implementing C partially, I added the Authorization header part to complete the functionality.

Since mechanism B and C cannot cover the mTLS credential case described in #1823, mechanism A may be better than the others.
Still, I think policy 2 or 3 are better than policy 1 since there may be some use cases that need to allow credentials to all origins.

[Kludex]

The question here is... Are there implementations not using mechanism A? If not, can't we just change our logic to be like that?

[gnat]

We could just raise an exception if Authorization headers are sent while Access-Control-Allow-Origin is *. But frankly, we do not have to do anything.

The browser will not allow it, and will warn you if you do the wrong thing. No need to make it complicated.

Outside of a browser, bad actors can just fake the origin.

[ShreySinha02]

I am checking for credential if credentials is allowed, then explicit origin function is called if header include any of the authorization or cookies

if self.allow_all_origins:
            if self.allow_credentials and has_authorization or has_cookie:
                self.allow_explicit_origin(headers, origin)```

[fcollonval]

Thanks @ShreySinha02

@Kludex would you mind providing an update on this issue and the proposed fix? Is there anyway the community could help moving forward on this?

[Pravindgholap]

Hi, I’d like to work on this issue.
I checked the latest code in main and see that the Authorization header case is still missing.
Can you confirm it’s still open for contribution?