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

fix: cors middleware mirrors origin in case no initial cookie is present #2675

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SebastianBr
Copy link

Summary

Whenever a CORS request doesn't contain a cookie in the header, but we try to set one (set-cookie in the response header), the origin of the request is not mirrored in the response, leading to CORS errors.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@Kludex
Copy link
Member

Kludex commented Sep 1, 2024

Please create a discussion.

@Kludex Kludex closed this Sep 1, 2024
@SebastianBr
Copy link
Author

Done that #2684

@Kludex Kludex reopened this Sep 2, 2024
@Kludex Kludex closed this Sep 2, 2024
@Kludex Kludex reopened this Sep 2, 2024
assert response.status_code == 200
assert response.text == "Homepage"
assert response.headers["access-control-allow-origin"] == "https://example.org"
assert "access-control-allow-credentials" not in response.headers
Copy link
Member

Choose a reason for hiding this comment

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

Well, I don't think this is correct. Your own StackOverflow link on the discussion shows that Access-Control-Allow-Credentials should be True.

Copy link
Author

Choose a reason for hiding this comment

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

Allow-Credetails is true in the response the middleware is created accordingly:

CORSMiddleware(allow_credentials=True, ...)

It needs to be True so that the browser accepts the cookie. I updated the test accordingly.

The open question for me is under what conditions we need to mirror the origin. Is set-cookie sufficient even if allow_credentials is False or do you require both? I believe setting allow_credentials to False and setting cookies is user error. In my personal code I would log this user error.

@@ -158,9 +158,9 @@ async def send(
headers = MutableHeaders(scope=message)
headers.update(self.simple_headers)
origin = request_headers["Origin"]
has_cookie = "cookie" in request_headers
has_cookie = "cookie" in request_headers or "set-cookie" in headers
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this check should be here. See comment on the tests.

Copy link
Author

Choose a reason for hiding this comment

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

I also tried checking the django implementation (although I am not expert on these things). It looks like they basically never check the wether a cookie was there or shall be set:

if conf.CORS_ALLOW_ALL_ORIGINS and not conf.CORS_ALLOW_CREDENTIALS:

From what I read, I would suggest removing line 161 completely and repalce 165 with

if self.allow_all_origins and self.allow_credentails:

What do you think @Kludex ?

Copy link
Author

Choose a reason for hiding this comment

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

@Kludex
I checked some documentation and stack overflow again and I think the mirroring of the origin is not working properly in this middleware.

Mirroring the origin is required for various purposes if allow credentials is true, not only cookies (https://stackoverflow.com/questions/25064158/how-to-make-get-cors-request-with-authorization-header & https://stackoverflow.com/questions/40663641/cors-with-client-https-certificates)

Basically, whenever credentails are used the the origin has to match the host of the client call otherwise the browser will block all things that require credentials such as cookies, authorization headers, certificates and so.

Django also solves it this way to basically always mirror the origin when allow credentials is true (https://github.com/adamchainz/django-cors-headers/blob/0c34907bca897da9934bfd7da40b38ae89da44d5/src/corsheaders/middleware.py#L115) since it otherwise will never work.

Unfortunately many tests will fail...

@Kludex
Copy link
Member

Kludex commented Sep 2, 2024

We can continue the discussion here.

I've checked the https://github.com/adamchainz/django-cors-headers implementation, and it doesn't look like they handle this case either.

Also, the Set-Cookie page on https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie says in a warning the behavior I mention on the credentials.

@SebastianBr SebastianBr force-pushed the fix-cors-middleware-origin-mirroring-without-initial-cookie branch from 8c15a6a to de0e1dd Compare September 2, 2024 13:06
@SebastianBr SebastianBr force-pushed the fix-cors-middleware-origin-mirroring-without-initial-cookie branch from de0e1dd to 4a49a51 Compare September 2, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants