scrapy icon indicating copy to clipboard operation
scrapy copied to clipboard

Refactor cookie handling in CookiesMiddleware

Open emarondan opened this issue 2 years ago • 3 comments

  • Refactoring cookie handling in CookiesMiddleware
  • Adding test case for off-domain jar storage

Can close #5841

emarondan avatar Jun 09 '23 17:06 emarondan

Codecov Report

Merging #5946 (c1a5e15) into master (4dacad0) will decrease coverage by 46.39%. Report is 366 commits behind head on master. The diff coverage is 65.84%.

:exclamation: Current head c1a5e15 differs from pull request most recent head 4d58842. Consider uploading reports for the commit 4d58842 to get more accurate results

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     
Files Changed Coverage Δ
scrapy/__init__.py 84.21% <0.00%> (ø)
scrapy/cmdline.py 0.00% <0.00%> (-67.97%) :arrow_down:
scrapy/commands/shell.py 0.00% <0.00%> (-92.86%) :arrow_down:
scrapy/downloadermiddlewares/cookies.py 37.50% <0.00%> (-60.40%) :arrow_down:
scrapy/downloadermiddlewares/decompression.py 32.75% <0.00%> (-67.25%) :arrow_down:
scrapy/downloadermiddlewares/httpcache.py 33.73% <ø> (-60.25%) :arrow_down:
scrapy/linkextractors/__init__.py 75.00% <ø> (-25.00%) :arrow_down:
scrapy/mail.py 38.88% <0.00%> (-39.77%) :arrow_down:
scrapy/squeues.py 54.32% <ø> (-45.68%) :arrow_down:
scrapy/contracts/__init__.py 18.51% <20.00%> (-65.08%) :arrow_down:
... and 74 more

... and 64 files with indirect coverage changes

:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=scrapy).

codecov[bot] avatar Jun 22 '23 12:06 codecov[bot]

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.

Gallaecio avatar Jul 27 '23 14:07 Gallaecio

@emarondan I have made a small change to test_process_response_unrelated_domain, to cover the issue I fear we might introduce here. When receiving a cookie in a response for a different domain, the cookie should be ignored altogether; we should not add it to the cookie jar, not for the actual response domain, not for the cookie domain.

Please, see if you can find a way, however unclean, to get test_off_domain_jar_storage to pass without breaking the new test_process_response_unrelated_domain in the process.

Gallaecio avatar Sep 08 '23 08:09 Gallaecio