fix: cors middleware mirrors origin in case no initial cookie is present#2675
Conversation
|
Please create a discussion. |
|
Done that #2684 |
tests/middleware/test_cors.py
Outdated
| 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 |
There was a problem hiding this comment.
Well, I don't think this is correct. Your own StackOverflow link on the discussion shows that Access-Control-Allow-Credentials should be True.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
I'm not sure this check should be here. See comment on the tests.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
@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...
|
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 |
SebastianBr
force-pushed
the
fix-cors-middleware-origin-mirroring-without-initial-cookie
branch
from
8c15a6a to
de0e1dd
Compare
SebastianBr
force-pushed
the
fix-cors-middleware-origin-mirroring-without-initial-cookie
branch
from
de0e1dd to
4a49a51
Compare
|
Since I've merged #3137, I don't think this is relevant anymore. But thanks for the PR! |
2 participants
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