Refactor cookie handling in CookiesMiddleware #5946
Conversation
emarondan
force-pushed
the
cookie-setting-5841
branch
from
80aaa7c
to
67bf7c5
Compare
emarondan
force-pushed
the
cookie-setting-5841
branch
from
67bf7c5
to
be0a1d2
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #5946 +/- ##
===========================================
- Coverage 88.85% 42.47% -46.39%
===========================================
Files 162 163 +1
Lines 11057 11550 +493
Branches 1801 1880 +79
===========================================
- Hits 9825 4906 -4919
- Misses 954 6269 +5315
- Partials 278 375 +97
|
emarondan
changed the title
emarondan
force-pushed
the
cookie-setting-5841
branch
2 times, most recently
from
7fa0e79
to
69bb49d
Compare
| cookie.domain = request_domain | ||
|
|
||
| jar.set_cookie_if_ok(cookie, request) | ||
| cookie.domain = request_domain |
There was a problem hiding this comment.
@wRAR With this modification the cookie is always set on the jar. The cookie domain must be the same as the request one in order to be set by jar.set_cookie_if_ok()
There was a problem hiding this comment.
I'm trying to understand why the logic is inverted by this change.
| jar.set_cookie_if_ok(cookie, request) | ||
| else: | ||
| print(f'seting cookie {cookie.__dict__}') | ||
| jar.set_cookie(cookie) |
There was a problem hiding this comment.
If the domain is not public, the cookie should be set anyway.
| @@ -646,7 +646,7 @@ def test_user_set_cookie_domain_suffix_public_period(self): | |||
| "https://foo.co.uk", | |||
| "https://bar.co.uk", | |||
| "co.uk", | |||
| cookies1=False, | |||
| cookies1=True, | |||
There was a problem hiding this comment.
With the modification made on the handler, I think this test should change since the cookie will be present on the first request.
There was a problem hiding this comment.
Can you please explain why should the cookies be present on the first request and absent on the second one, as both requests are similar? Does https://datatracker.ietf.org/doc/html/rfc6265#section-5.3 say the cookie should be modified to be for foo.co.uk? I'm not sure yet.
| @@ -655,7 +655,7 @@ def test_user_set_cookie_domain_suffix_public_private(self): | |||
| "https://foo.blogspot.com", | |||
| "https://bar.blogspot.com", | |||
| "blogspot.com", | |||
| cookies1=False, | |||
| cookies1=True, | |||
There was a problem hiding this comment.
Same as above test.
emarondan
force-pushed
the
cookie-setting-5841
branch
from
69bb49d
to
7d32fb4
Compare
|
I don’t think the changes to existing tests are correct. I think we need to change the implementation instead to make sure they continue to pass as they were. Also, the cookie processing method affects both requests and responses, so it would be good to make sure we have a test to make sure that, if we get a Set-Cookie header from a request with a cookie set for an unrelated domain, that the cookie is not added to the cookiejar. We trust users, not servers. |
- adding test to make sure we don't add a cookie with an unrelated domain from a response
emarondan
force-pushed
the
cookie-setting-5841
branch
from
7d32fb4
to
c8b19d8
Compare
| response.headers["Set-Cookie"] = "asd=fgh; domain=d.example" | ||
| self.mw.process_response(request, response, spider=None) | ||
| jar = self.mw.jars[None] | ||
| assert not jar._cookies.get("c.example", {}).get("/", {}).get("asd") |
There was a problem hiding this comment.
The problem with the new approach is, I think, that it also applies to server-side cookies.
I think this test is not right, and fixing it as follows:
| response.headers["Set-Cookie"] = "asd=fgh; domain=d.example" | |
| self.mw.process_response(request, response, spider=None) | |
| jar = self.mw.jars[None] | |
| assert not jar._cookies.get("c.example", {}).get("/", {}).get("asd") | |
| response.headers["Set-Cookie"] = "asd=fgh; domain=c.example" | |
| self.mw.process_response(request, response, spider=None) | |
| jar = self.mw.jars[None] | |
| assert not jar._cookies.get("c.example", {}).get("/", {}).get("asd") # I wonder if we can just assert that c.example is not in the jar to simplify the test. |
Will probably make it fail with the new approach.
It would not be very clean, but I wonder if it would be possible to change the policy right before setting user-defined cookies (and only user-defined cookies, not Set-Cookie headers from servers), and restore it right after.
If not that way, we need to think of something else.
…dd a cookie or not is made on process_response - Update test case for CookiesMiddleware. Now it test if the domain is present on the cookie jar
|
@emarondan I have made a small change to Please, see if you can find a way, however unclean, to get |
Can close #5841