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

CORSMiddleware does not provide explicit origin although Authorization header is present #1832

Open
Kludex opened this issue Aug 29, 2022 Discussed in #1823 · 6 comments
Open
Labels
cors Cross-Origin Resource Sharing good first issue Good for beginners help wanted Feel free to help
Milestone

Comments

@Kludex
Copy link
Sponsor Member

Kludex commented Aug 29, 2022

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.

if self.allow_all_origins and has_cookie:
self.allow_explicit_origin(headers, origin)

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.
Fund with Polar
@Kludex Kludex added the cors Cross-Origin Resource Sharing label Aug 29, 2022
@Kludex
Copy link
Sponsor Member Author

Kludex commented Aug 29, 2022

@Kludex
Copy link
Sponsor Member Author

Kludex commented Sep 1, 2022

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
Copy link

gyusang commented Sep 7, 2022

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
Copy link
Sponsor Member Author

Kludex commented Sep 18, 2022

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

@Kludex Kludex added help wanted Feel free to help good first issue Good for beginners labels Feb 15, 2023
@Kludex Kludex added this to the Version 1.x milestone Feb 15, 2023
@gnat
Copy link

gnat commented Apr 20, 2023

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
Copy link

ShreySinha02 commented Feb 12, 2024

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)```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cors Cross-Origin Resource Sharing good first issue Good for beginners help wanted Feel free to help
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants