-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix cookies with duplicate names being lost when updating cookie jar #11106
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #11106 +/- ##
========================================
Coverage 98.82% 98.83%
========================================
Files 129 129
Lines 41505 41726 +221
Branches 2234 2241 +7
========================================
+ Hits 41019 41239 +220
Misses 337 337
- Partials 149 150 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This will be bit slower... but correct. We could go faster by doing something like... but I'm too afraid to do it because there might be subtle differences between it and SimpleCookie and we need bug for bug compatibility. We could also reduce this to use It would be nice to make the parser more flexible because than we can solve #2683 as well. diff --git a/aiohttp/abc.py b/aiohttp/abc.py
index 0b584eba4..b468caa8d 100644
--- a/aiohttp/abc.py
+++ b/aiohttp/abc.py
@@ -1,4 +1,5 @@
import logging
+import re
import socket
from abc import ABC, abstractmethod
from collections.abc import Sized
@@ -169,6 +170,75 @@ else:
ClearCookiePredicate = Callable[["Morsel[str]"], bool]
+# Cookie parsing optimization constants
+_COOKIE_NAME_RE = re.compile(r"^[!#$%&\'*+\-.0-9A-Z^_`a-z|~]+$")
+_COOKIE_SPLIT_RE = re.compile(r";\s*")
+_KNOWN_ATTRS = frozenset(
+ ["path", "domain", "max-age", "expires", "secure", "httponly", "samesite"]
+)
+
+
+def _parse_cookie_header(header: str) -> Optional[Tuple[str, Morsel[str]]]:
+ """Fast cookie header parser that creates a Morsel directly."""
+ if not header:
+ return None
+
+ # Split by semicolon to get parts
+ parts = _COOKIE_SPLIT_RE.split(header)
+ if not parts:
+ return None
+
+ # First part must be name=value
+ first_part = parts[0]
+ eq_idx = first_part.find("=")
+ if eq_idx <= 0: # No = or empty name
+ return None
+
+ name = first_part[:eq_idx].strip()
+ value = first_part[eq_idx + 1 :].strip()
+
+ # Validate cookie name - raise CookieError for invalid names
+ if not name:
+ return None
+ if not _COOKIE_NAME_RE.match(name):
+ raise CookieError(f"Illegal cookie name {name!r}")
+
+ # Remove quotes from value if present
+ if len(value) >= 2 and value[0] == '"' and value[-1] == '"':
+ value = value[1:-1]
+
+ # Create Morsel
+ morsel = Morsel()
+ morsel.set(name, value, value)
+
+ # Parse attributes
+ for part in parts[1:]:
+ part = part.strip()
+ if not part:
+ continue
+
+ eq_idx = part.find("=")
+ if eq_idx > 0:
+ # Attribute with value
+ attr_name = part[:eq_idx].strip().lower()
+
+ # Only process known attributes
+ if attr_name in _KNOWN_ATTRS:
+ attr_value = part[eq_idx + 1 :].strip()
+ morsel[attr_name] = attr_value
+ else:
+ # Boolean attribute
+ attr_name = part.lower()
+ if attr_name == "secure":
+ morsel["secure"] = True
+ elif attr_name == "httponly":
+ morsel["httponly"] = True
+ elif attr_name not in _KNOWN_ATTRS:
+ # Unknown boolean attribute - reject for SimpleCookie compatibility
+ return None
+
+ return name, morsel
+
class AbstractCookieJar(Sized, IterableBase):
"""Abstract Cookie Jar."""
@@ -196,18 +266,16 @@ class AbstractCookieJar(Sized, IterableBase):
"""
Update cookies from raw Set-Cookie headers.
- Default implementation parses each header separately to preserve
- cookies with same name but different domain/path.
+ Optimized implementation that parses cookies 40-57% faster than
+ SimpleCookie by using a direct parser that creates Morsel objects.
"""
- # Default implementation for backward compatibility
cookies_to_update: List[Tuple[str, Morsel[str]]] = []
+
for cookie_header in headers:
- tmp_cookie = SimpleCookie()
try:
- tmp_cookie.load(cookie_header)
- # Collect all cookies as tuples (name, morsel)
- for name, morsel in tmp_cookie.items():
- cookies_to_update.append((name, morsel))
+ result = _parse_cookie_header(cookie_header)
+ if result:
+ cookies_to_update.append(result)
except CookieError as exc:
client_logger.warning("Can not load response cookies: %s", exc)
|
Backport to 3.12: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply cfb9931 on top of patchback/backports/3.12/cfb99316e1fd89943b1f6c3793d286629b7acc3d/pr-11106 Backporting merged PR #11106 into master
🤖 @patchback |
Backport to 3.13: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply cfb9931 on top of patchback/backports/3.13/cfb99316e1fd89943b1f6c3793d286629b7acc3d/pr-11106 Backporting merged PR #11106 into master
🤖 @patchback |
Thanks for the review |
…eing lost when updating cookie jar (#11109)
…eing lost when updating cookie jar (#11108)
I think we did suggest somewhere that in v4 or beyond we could consider replacing http.cookies completely, as it does seem to have quite a few issues and is not properly designed to do what we're trying to achieve here.. |
Summary
This PR fixes an issue where cookies with duplicate names but different domains or paths were being lost when updating the cookie jar. The root cause was that
SimpleCookie
uses only the cookie name as its key, causing later cookies with the same name to overwrite earlier ones.Changes
ClientResponse
to store raw Set-Cookie headersupdate_cookies_from_headers
method toAbstractCookieJar
that processes each Set-Cookie header individually to preserve duplicate cookie namesRelated Issues
Technical Details
The fix works by processing each Set-Cookie header separately and collecting all cookies before updating the jar, rather than relying on SimpleCookie's dictionary-like behavior which loses duplicates.