Skip to content

Fix cookie parsing issues#11112

Merged
bdraco merged 112 commits intomasterfrom
cookie_fix
Jun 2, 2025
Merged

Fix cookie parsing issues#11112
bdraco merged 112 commits intomasterfrom
cookie_fix

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented Jun 1, 2025 •
edited
Loading

What do these changes do?

Summary

This PR fixes multiple long-standing cookie parsing issues by implementing a more lenient cookie parser that handles real-world cookies while maintaining compatibility with the HTTP cookie specification.

Issues Fixed

Key Changes

1. Use parse_cookie_headers for parsing cookie headers

Replaced Python's strict SimpleCookie with the more lenient parse_cookie_headers function in multiple places:

Server-side (aiohttp/web_request.py):

  • Modified cookie parsing in incoming requests to accept cookies with special characters (fixes CookieError on invalid cookie names #2683)
  • Prevents 500 errors when encountering cookies with characters like {}[]() that are commonly used by real-world websites

Client-side:

  • aiohttp/client_reqrep.py: Modified cookie parsing in two places:
    • Response cookies: Uses parse_cookie_headers to handle malformed cookies gracefully
      • Maintains backward compatibility by still exposing response.cookies as a SimpleCookie object
      • Uses parse_cookie_headers for the actual parsing, then updates the SimpleCookie with the results
    • Request cookies: Updated update_cookies() method to use parse_cookie_headers when parsing existing Cookie headers
      • Prevents errors when updating cookies if the existing Cookie header contains special characters
  • aiohttp/abc.py: Simplified update_cookies_from_headers() in AbstractCookieJar
    • Removed try/except blocks since parse_cookie_headers handles errors internally without raising exceptions
    • Used by all cookie jar implementations (CookieJar, DummyCookieJar) for consistent cookie handling

2. Updated logger usage

  • Changed client_logger to internal_logger in parse_cookie_headers since the function is now used by both client and server code

3. Fixed inconsistent cookie quoting behavior in CookieJar

  • Shared cookies (domain="", path="") were not respecting the quote_cookie setting
  • Non-shared cookies were properly respecting quote_cookie, but shared cookies were always sent unquoted
  • Fixed by using the same _build_morsel() method for both shared and non-shared cookies
  • This ensures all cookies respect the self._quote_cookie setting consistently

4. Refactored cookie morsel building in CookieJar

  • Extracted cookie morsel creation into a dedicated _build_morsel() method
  • Uses the new helper functions from _cookie_helpers module:
    • make_non_quoted_morsel() when quote_cookie=False
    • preserve_morsel_with_coded_value() when cookie is already properly quoted
    • make_quoted_morsel() when cookie needs quoting
  • Simplified the code by always using BaseCookie for the filtered result to ensure our quoting is always respected.

5. Comprehensive test coverage

Added extensive tests to ensure:

  • Cookies with special characters are accepted
  • Real-world cookie formats work correctly
  • Quoted cookie values are handled consistently
  • Edge cases are handled properly

Technical Details

  • More tolerant parsing of cookie names
  • Consistent unquoting of cookie values
  • Maintains backward compatibility
  • Gracefully handles malformed cookies
  • Ensures consistent behavior for all cookies regardless of domain/path

The implementation aims to make aiohttp more robust when handling cookies from various web services and frameworks.

Are there changes in behavior for the user?

More lenient parser.

Is it a substantial burden for the maintainers to support this?

We have to maintain a cookie parser, however its not as bad as I was expecting since our use case is more narrow than everything SimpleCookie does

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Jun 2, 2025

Still happy with this after sleeping on it. Just landing in LHR now to make the OHF summit in Dublin. While I've done everything I can think of to write tests for it, I'm thinking it would be best to do a 3.12.7rc0 with this and run it through Home Assistant CI and a few other projects before pushing 3.12.7.

@bdraco bdraco merged commit 8edec63 into master Jun 2, 2025
40 checks passed
@bdraco bdraco deleted the cookie_fix branch June 2, 2025 08:18
@patchback
Copy link
Copy Markdown
Contributor

patchback bot commented Jun 2, 2025 •
edited
Loading

Backport to 3.12: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 8edec63 on top of patchback/backports/3.12/8edec635b65035ad819cd98abc7bfeb192f788a3/pr-11112

Backporting merged PR #11112 into master

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it: 
    $ git remote add upstream https://github.com/aio-libs/aiohttp.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code: 
    $ git fetch upstream
$ git checkout -b patchback/backports/3.12/8edec635b65035ad819cd98abc7bfeb192f788a3/pr-11112 upstream/3.12
  4. Now, cherry-pick PR Fix cookie parsing issues #11112 contents into that branch: 
    $ git cherry-pick -x 8edec635b65035ad819cd98abc7bfeb192f788a3
    If it'll yell at you with something like fatal: Commit 8edec635b65035ad819cd98abc7bfeb192f788a3 is a merge but no -m option was given., add -m 1 as follows instead: 
    $ git cherry-pick -m1 -x 8edec635b65035ad819cd98abc7bfeb192f788a3
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Fix cookie parsing issues #11112 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub: 
    $ git push origin patchback/backports/3.12/8edec635b65035ad819cd98abc7bfeb192f788a3/pr-11112
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback
Copy link
Copy Markdown
Contributor

patchback bot commented Jun 2, 2025 •
edited
Loading

Backport to 3.13: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 8edec63 on top of patchback/backports/3.13/8edec635b65035ad819cd98abc7bfeb192f788a3/pr-11112

Backporting merged PR #11112 into master

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it: 
    $ git remote add upstream https://github.com/aio-libs/aiohttp.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code: 
    $ git fetch upstream
$ git checkout -b patchback/backports/3.13/8edec635b65035ad819cd98abc7bfeb192f788a3/pr-11112 upstream/3.13
  4. Now, cherry-pick PR Fix cookie parsing issues #11112 contents into that branch: 
    $ git cherry-pick -x 8edec635b65035ad819cd98abc7bfeb192f788a3
    If it'll yell at you with something like fatal: Commit 8edec635b65035ad819cd98abc7bfeb192f788a3 is a merge but no -m option was given., add -m 1 as follows instead: 
    $ git cherry-pick -m1 -x 8edec635b65035ad819cd98abc7bfeb192f788a3
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Fix cookie parsing issues #11112 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub: 
    $ git push origin patchback/backports/3.13/8edec635b65035ad819cd98abc7bfeb192f788a3/pr-11112
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

bdraco added a commit that referenced this pull request Jun 2, 2025
@bdraco
Fix cookie parsing issues (#11112)
0e414c4 
(cherry picked from commit 8edec63)
bdraco added a commit that referenced this pull request Jun 2, 2025
@bdraco
Fix cookie parsing issues (#11112)
d70cdf5 
(cherry picked from commit 8edec63)
bdraco added a commit that referenced this pull request Jun 2, 2025
@bdraco
PR #11112/8edec63 backport][3.13] Fix cookie parsing issues (#11118)
24e030b
bdraco added a commit that referenced this pull request Jun 2, 2025
@bdraco
PR #11112/8edec63 backport][3.12] Fix cookie parsing issues (#11117)
741cb61
@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Jun 2, 2025

I've done all the production testing I can do with the RC. All looks good

mdellweg added a commit to mdellweg/pulpcore that referenced this pull request Jun 4, 2025
@mdellweg
Remove invalid cookie test
8ac955e 
A change in aiohttp now accepts basically all invalid cookies and
additionally stops failing on the ones left. The content-app will
never fail the request based on cookies anymore.

aio-libs/aiohttp#11112
@mdellweg mdellweg mentioned this pull request Jun 4, 2025
Merged
mdellweg added a commit to pulp/pulpcore that referenced this pull request Jun 4, 2025
@mdellweg
Remove invalid cookie test
375bf76 
A change in aiohttp now accepts basically all invalid cookies and
additionally stops failing on the ones left. The content-app will
never fail the request based on cookies anymore.

aio-libs/aiohttp#11112
patchback bot pushed a commit to pulp/pulpcore that referenced this pull request Jun 4, 2025
@mdellweg @patchback
Remove invalid cookie test
dcc7957 
A change in aiohttp now accepts basically all invalid cookies and
additionally stops failing on the ones left. The content-app will
never fail the request based on cookies anymore.

aio-libs/aiohttp#11112
(cherry picked from commit 375bf76)
@patchback patchback bot mentioned this pull request Jun 4, 2025
Merged
patchback bot pushed a commit to pulp/pulpcore that referenced this pull request Jun 4, 2025
@mdellweg @patchback
Remove invalid cookie test
674d377 
A change in aiohttp now accepts basically all invalid cookies and
additionally stops failing on the ones left. The content-app will
never fail the request based on cookies anymore.

aio-libs/aiohttp#11112
(cherry picked from commit 375bf76)
@patchback patchback bot mentioned this pull request Jun 4, 2025
Merged
mdellweg added a commit to pulp/pulpcore that referenced this pull request Jun 4, 2025
@mdellweg
Remove invalid cookie test
1535d40 
A change in aiohttp now accepts basically all invalid cookies and
additionally stops failing on the ones left. The content-app will
never fail the request based on cookies anymore.

aio-libs/aiohttp#11112
(cherry picked from commit 375bf76)
mdellweg added a commit to pulp/pulpcore that referenced this pull request Jun 4, 2025
@mdellweg
Remove invalid cookie test
d676642 
A change in aiohttp now accepts basically all invalid cookies and
additionally stops failing on the ones left. The content-app will
never fail the request based on cookies anymore.

aio-libs/aiohttp#11112
(cherry picked from commit 375bf76)
@bdraco bdraco mentioned this pull request Jun 10, 2025
Merged
5 tasks
This was referenced Jun 10, 2025
Merged
Merged
@Dreamsorcerer
Copy link
Copy Markdown
Member

Dreamsorcerer commented Aug 8, 2025

@bdraco We might be able to revert these changes in 3.15?
python/cpython#113663

@bdraco
Copy link
Copy Markdown
Member Author

bdraco commented Aug 8, 2025

Nice. I guess we can go back to the standard one in ~5.5 years 😄

@Dreamsorcerer Dreamsorcerer mentioned this pull request Aug 9, 2025
Open
@Cycloctane Cycloctane mentioned this pull request Sep 18, 2025
Closed
@thehesiod thehesiod mentioned this pull request Oct 10, 2025
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Dreamsorcerer Dreamsorcerer Dreamsorcerer left review comments

@webknjaz webknjaz Awaiting requested review from webknjaz webknjaz is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees

No one assigned

Labels

backport-3.13 Trigger automatic backporting to the 3.13 release branch by Patchback robot bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Milestone

No milestone

3 participants

@bdraco @Dreamsorcerer @github-advanced-security