diff --git a/CHANGES/11105.bugfix.rst b/CHANGES/11105.bugfix.rst new file mode 100644 index 00000000000..33578aa7a95 --- /dev/null +++ b/CHANGES/11105.bugfix.rst @@ -0,0 +1,10 @@ +Fixed an issue where cookies with duplicate names but different domains or paths +were lost when updating the cookie jar. The :class:`~aiohttp.ClientSession` +cookie jar now correctly stores all cookies even if they have the same name but +different domain or path, following the :rfc:`6265#section-5.3` storage model -- by :user:`bdraco`. + +Note that :attr:`ClientResponse.cookies ` returns +a :class:`~http.cookies.SimpleCookie` which uses the cookie name as a key, so +only the last cookie with each name is accessible via this interface. All cookies +can be accessed via :meth:`ClientResponse.headers.getall('Set-Cookie') +` if needed. diff --git a/CHANGES/11106.bugfix.rst b/CHANGES/11106.bugfix.rst new file mode 120000 index 00000000000..3e5efb0f3f3 --- /dev/null +++ b/CHANGES/11106.bugfix.rst @@ -0,0 +1 @@ +11105.bugfix.rst \ No newline at end of file diff --git a/CHANGES/11107.misc.rst b/CHANGES/11107.misc.rst new file mode 100644 index 00000000000..37ac4622bd9 --- /dev/null +++ b/CHANGES/11107.misc.rst @@ -0,0 +1 @@ +Avoided creating closed futures in ``ResponseHandler`` that will never be awaited -- by :user:`bdraco`. diff --git a/CHANGES/4486.bugfix.rst b/CHANGES/4486.bugfix.rst new file mode 120000 index 00000000000..3e5efb0f3f3 --- /dev/null +++ b/CHANGES/4486.bugfix.rst @@ -0,0 +1 @@ +11105.bugfix.rst \ No newline at end of file diff --git a/aiohttp/abc.py b/aiohttp/abc.py index ce414012ea6..0b584eba4d3 100644 --- a/aiohttp/abc.py +++ b/aiohttp/abc.py @@ -2,7 +2,7 @@ import socket from abc import ABC, abstractmethod from collections.abc import Sized -from http.cookies import BaseCookie, Morsel +from http.cookies import BaseCookie, CookieError, Morsel, SimpleCookie from typing import ( TYPE_CHECKING, Any, @@ -13,6 +13,7 @@ Iterable, List, Optional, + Sequence, Tuple, TypedDict, Union, @@ -21,6 +22,7 @@ from multidict import CIMultiDict from yarl import URL +from .log import client_logger from .typedefs import LooseCookies if TYPE_CHECKING: @@ -188,6 +190,31 @@ def clear_domain(self, domain: str) -> None: def update_cookies(self, cookies: LooseCookies, response_url: URL = URL()) -> None: """Update cookies.""" + def update_cookies_from_headers( + self, headers: Sequence[str], response_url: URL + ) -> None: + """ + Update cookies from raw Set-Cookie headers. + + Default implementation parses each header separately to preserve + cookies with same name but different domain/path. + """ + # 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)) + except CookieError as exc: + client_logger.warning("Can not load response cookies: %s", exc) + + # Update all cookies at once for efficiency + if cookies_to_update: + self.update_cookies(cookies_to_update, response_url) + @abstractmethod def filter_cookies(self, request_url: URL) -> "BaseCookie[str]": """Return the jar's cookies filtered by their attributes.""" diff --git a/aiohttp/client.py b/aiohttp/client.py index b9dde9df6d0..d4800a49ac6 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -722,8 +722,11 @@ async def _connect_and_send_request( raise raise ClientOSError(*exc.args) from exc - if cookies := resp._cookies: - self._cookie_jar.update_cookies(cookies, resp.url) + # Update cookies from raw headers to preserve duplicates + if resp._raw_cookie_headers: + self._cookie_jar.update_cookies_from_headers( + resp._raw_cookie_headers, resp.url + ) # redirects if resp.status in (301, 302, 303, 307, 308) and allow_redirects: diff --git a/aiohttp/client_proto.py b/aiohttp/client_proto.py index be1e7672efe..c1c0869f059 100644 --- a/aiohttp/client_proto.py +++ b/aiohttp/client_proto.py @@ -46,7 +46,26 @@ def __init__(self, loop: asyncio.AbstractEventLoop) -> None: self._timeout_ceil_threshold: Optional[float] = 5 - self.closed: asyncio.Future[None] = self._loop.create_future() + self._closed: Union[None, asyncio.Future[None]] = None + self._connection_lost_called = False + + @property + def closed(self) -> Union[None, asyncio.Future[None]]: + """Future that is set when the connection is closed. + + This property returns a Future that will be completed when the connection + is closed. The Future is created lazily on first access to avoid creating + futures that will never be awaited. + + Returns: + - A Future[None] if the connection is still open or was closed after + this property was accessed + - None if connection_lost() was already called before this property + was ever accessed (indicating no one is waiting for the closure) + """ + if self._closed is None and not self._connection_lost_called: + self._closed = self._loop.create_future() + return self._closed @property def upgraded(self) -> bool: @@ -80,6 +99,7 @@ def is_connected(self) -> bool: return self.transport is not None and not self.transport.is_closing() def connection_lost(self, exc: Optional[BaseException]) -> None: + self._connection_lost_called = True self._drop_timeout() original_connection_error = exc @@ -87,23 +107,23 @@ def connection_lost(self, exc: Optional[BaseException]) -> None: connection_closed_cleanly = original_connection_error is None - if connection_closed_cleanly: - set_result(self.closed, None) - else: - assert original_connection_error is not None - set_exception( - self.closed, - ClientConnectionError( - f"Connection lost: {original_connection_error !s}", - ), - original_connection_error, - ) - # Mark the exception as retrieved to prevent - # "Future exception was never retrieved" warnings - # The exception is always passed on through - # other means, so this is safe - with suppress(Exception): - self.closed.exception() + if self._closed is not None: + # If someone is waiting for the closed future, + # we should set it to None or an exception. If + # self._closed is None, it means that + # connection_lost() was called already + # or nobody is waiting for it. + if connection_closed_cleanly: + set_result(self._closed, None) + else: + assert original_connection_error is not None + set_exception( + self._closed, + ClientConnectionError( + f"Connection lost: {original_connection_error !s}", + ), + original_connection_error, + ) if self._payload_parser is not None: with suppress(Exception): # FIXME: log this somehow? diff --git a/aiohttp/client_reqrep.py b/aiohttp/client_reqrep.py index e4e0b8a9903..957d8c25562 100644 --- a/aiohttp/client_reqrep.py +++ b/aiohttp/client_reqrep.py @@ -228,6 +228,7 @@ class ClientResponse(HeadersMixin): _connection: Optional["Connection"] = None # current connection _cookies: Optional[SimpleCookie] = None + _raw_cookie_headers: Optional[Tuple[str, ...]] = None _continue: Optional["asyncio.Future[bool]"] = None _source_traceback: Optional[traceback.StackSummary] = None _session: Optional["ClientSession"] = None @@ -309,12 +310,29 @@ def _writer(self, writer: Optional["asyncio.Task[None]"]) -> None: @property def cookies(self) -> SimpleCookie: if self._cookies is None: - self._cookies = SimpleCookie() + if self._raw_cookie_headers is not None: + # Parse cookies for response.cookies (SimpleCookie for backward compatibility) + cookies = SimpleCookie() + for hdr in self._raw_cookie_headers: + try: + cookies.load(hdr) + except CookieError as exc: + client_logger.warning("Can not load response cookies: %s", exc) + self._cookies = cookies + else: + self._cookies = SimpleCookie() return self._cookies @cookies.setter def cookies(self, cookies: SimpleCookie) -> None: self._cookies = cookies + # Generate raw cookie headers from the SimpleCookie + if cookies: + self._raw_cookie_headers = tuple( + morsel.OutputString() for morsel in cookies.values() + ) + else: + self._raw_cookie_headers = None @reify def url(self) -> URL: @@ -474,13 +492,8 @@ async def start(self, connection: "Connection") -> "ClientResponse": # cookies if cookie_hdrs := self.headers.getall(hdrs.SET_COOKIE, ()): - cookies = SimpleCookie() - for hdr in cookie_hdrs: - try: - cookies.load(hdr) - except CookieError as exc: - client_logger.warning("Can not load response cookies: %s", exc) - self._cookies = cookies + # Store raw cookie headers for CookieJar + self._raw_cookie_headers = tuple(cookie_hdrs) return self def _response_eof(self) -> None: diff --git a/aiohttp/connector.py b/aiohttp/connector.py index 4f3f22369a5..55cfb185f7a 100644 --- a/aiohttp/connector.py +++ b/aiohttp/connector.py @@ -462,13 +462,15 @@ def _close_immediately(self) -> List[Awaitable[object]]: self._cleanup_closed_handle.cancel() for data in self._conns.values(): - for proto, t0 in data: + for proto, _ in data: proto.close() - waiters.append(proto.closed) + if closed := proto.closed: + waiters.append(closed) for proto in self._acquired: proto.close() - waiters.append(proto.closed) + if closed := proto.closed: + waiters.append(closed) # TODO (A.Yushovskiy, 24-May-2019) collect transp. closing futures for transport in self._cleanup_closed_transports: diff --git a/docs/client_reference.rst b/docs/client_reference.rst index f9806fdf985..8b9068a2761 100644 --- a/docs/client_reference.rst +++ b/docs/client_reference.rst @@ -1483,6 +1483,19 @@ Response object HTTP cookies of response (*Set-Cookie* HTTP header, :class:`~http.cookies.SimpleCookie`). + .. note:: + + Since :class:`~http.cookies.SimpleCookie` uses cookie name as the + key, cookies with the same name but different domains or paths will + be overwritten. Only the last cookie with a given name will be + accessible via this attribute. + + To access all cookies, including duplicates with the same name, + use :meth:`response.headers.getall('Set-Cookie') `. + + The session's cookie jar will correctly store all cookies, even if + they are not accessible via this attribute. + .. attribute:: headers A case-insensitive multidict proxy with HTTP headers of diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index ea288657912..d5087ec7c52 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -2712,6 +2712,7 @@ async def handler(request: web.Request) -> web.Response: async with client.get("/") as resp: assert 200 == resp.status cookie_names = {c.key for c in client.session.cookie_jar} + _ = resp.cookies assert cookie_names == {"c1", "c2"} m_log.warning.assert_called_with("Can not load response cookies: %s", mock.ANY) @@ -5182,3 +5183,148 @@ async def redirect_handler(request: web.Request) -> web.Response: assert ( payload.close_called ), "Payload.close() was not called when InvalidUrlRedirectClientError (invalid origin) was raised" + + +async def test_amazon_like_cookie_scenario(aiohttp_client: AiohttpClient) -> None: + """Test real-world cookie scenario similar to Amazon.""" + + class FakeResolver(AbstractResolver): + def __init__(self, port: int): + self._port = port + + async def resolve( + self, host: str, port: int = 0, family: int = 0 + ) -> List[ResolveResult]: + if host in ("amazon.it", "www.amazon.it"): + return [ + { + "hostname": host, + "host": "127.0.0.1", + "port": self._port, + "family": socket.AF_INET, + "proto": 0, + "flags": 0, + } + ] + assert False, f"Unexpected host: {host}" + + async def close(self) -> None: + """Close the resolver if needed.""" + + async def handler(request: web.Request) -> web.Response: + response = web.Response(text="Login successful") + + # Simulate Amazon-like cookies from the issue + cookies = [ + "session-id=146-7423990-7621939; Domain=.amazon.it; " + "Expires=Mon, 31-May-2027 10:00:00 GMT; Path=/; " + "Secure; HttpOnly", + "session-id=147-8529641-8642103; Domain=.www.amazon.it; " + "Expires=Mon, 31-May-2027 10:00:00 GMT; Path=/; HttpOnly", + "session-id-time=2082758401l; Domain=.amazon.it; " + "Expires=Mon, 31-May-2027 10:00:00 GMT; Path=/; Secure", + "session-id-time=2082758402l; Domain=.www.amazon.it; " + "Expires=Mon, 31-May-2027 10:00:00 GMT; Path=/", + "ubid-acbit=257-7531983-5395266; Domain=.amazon.it; " + "Expires=Mon, 31-May-2027 10:00:00 GMT; Path=/; Secure", + 'x-acbit="KdvJzu8W@Fx6Jj3EuNFLuP0N7OtkuCfs"; Version=1; ' + "Domain=.amazon.it; Path=/; Secure; HttpOnly", + "at-acbit=Atza|IwEBIM-gLr8; Domain=.amazon.it; " + "Expires=Mon, 31-May-2027 10:00:00 GMT; Path=/; " + "Secure; HttpOnly", + 'sess-at-acbit="4+6VzSJPHIFD/OqO264hFxIng8Y="; ' + "Domain=.amazon.it; Expires=Mon, 31-May-2027 10:00:00 GMT; " + "Path=/; Secure; HttpOnly", + "lc-acbit=it_IT; Domain=.amazon.it; " + "Expires=Mon, 31-May-2027 10:00:00 GMT; Path=/", + "i18n-prefs=EUR; Domain=.amazon.it; " + "Expires=Mon, 31-May-2027 10:00:00 GMT; Path=/", + "av-profile=null; Domain=.amazon.it; " + "Expires=Mon, 31-May-2027 10:00:00 GMT; Path=/; Secure", + 'user-pref-token="Am81ywsJ69xObBnuJ2FbilVH0mg="; ' + "Domain=.amazon.it; Path=/; Secure", + ] + + for cookie in cookies: + response.headers.add("Set-Cookie", cookie) + + return response + + app = web.Application() + app.router.add_get("/", handler) + + # Get the test server + server = await aiohttp_client(app) + port = server.port + + # Create a new client session with our fake resolver + resolver = FakeResolver(port) + + async with ( + aiohttp.TCPConnector(resolver=resolver, force_close=True) as connector, + aiohttp.ClientSession(connector=connector) as session, + ): + # Make request to www.amazon.it which will resolve to + # 127.0.0.1:port. This allows cookies for both .amazon.it + # and .www.amazon.it domains + resp = await session.get(f"http://www.amazon.it:{port}/") + + # Check headers + cookie_headers = resp.headers.getall("Set-Cookie") + assert ( + len(cookie_headers) == 12 + ), f"Expected 12 headers, got {len(cookie_headers)}" + + # Check parsed cookies - SimpleCookie only keeps the last + # cookie with each name. So we expect 10 unique cookie names + # (not 12) + expected_cookie_names = { + "session-id", # Will only have one + "session-id-time", # Will only have one + "ubid-acbit", + "x-acbit", + "at-acbit", + "sess-at-acbit", + "lc-acbit", + "i18n-prefs", + "av-profile", + "user-pref-token", + } + assert set(resp.cookies.keys()) == expected_cookie_names + assert ( + len(resp.cookies) == 10 + ), f"Expected 10 cookies in SimpleCookie, got {len(resp.cookies)}" + + # The important part: verify the session's cookie jar has + # all cookies. The cookie jar should have all 12 cookies, + # not just 10 + jar_cookies = list(session.cookie_jar) + assert ( + len(jar_cookies) == 12 + ), f"Expected 12 cookies in jar, got {len(jar_cookies)}" + + # Verify we have both session-id cookies with different domains + session_ids = [c for c in jar_cookies if c.key == "session-id"] + assert ( + len(session_ids) == 2 + ), f"Expected 2 session-id cookies, got {len(session_ids)}" + + # Verify the domains are different + session_id_domains = {c["domain"] for c in session_ids} + assert session_id_domains == { + "amazon.it", + "www.amazon.it", + }, f"Got domains: {session_id_domains}" + + # Verify we have both session-id-time cookies with different + # domains + session_id_times = [c for c in jar_cookies if c.key == "session-id-time"] + assert ( + len(session_id_times) == 2 + ), f"Expected 2 session-id-time cookies, got {len(session_id_times)}" + + # Now test that the raw headers were properly preserved + assert resp._raw_cookie_headers is not None + assert ( + len(resp._raw_cookie_headers) == 12 + ), "All raw headers should be preserved" diff --git a/tests/test_client_proto.py b/tests/test_client_proto.py index fa39b38d45c..e8749aaeb9f 100644 --- a/tests/test_client_proto.py +++ b/tests/test_client_proto.py @@ -261,13 +261,50 @@ async def test_connection_lost_exception_is_marked_retrieved( proto = ResponseHandler(loop=loop) proto.connection_made(mock.Mock()) + # Access closed property before connection_lost to ensure future is created + closed_future = proto.closed + assert closed_future is not None + # Simulate an SSL shutdown timeout error ssl_error = TimeoutError("SSL shutdown timed out") proto.connection_lost(ssl_error) # Verify the exception was set on the closed future - assert proto.closed.done() - exc = proto.closed.exception() + assert closed_future.done() + exc = closed_future.exception() assert exc is not None assert "Connection lost: SSL shutdown timed out" in str(exc) assert exc.__cause__ is ssl_error + + +async def test_closed_property_lazy_creation( + loop: asyncio.AbstractEventLoop, +) -> None: + """Test that closed future is created lazily.""" + proto = ResponseHandler(loop=loop) + + # Initially, the closed future should not be created + assert proto._closed is None + + # Accessing the property should create the future + closed_future = proto.closed + assert closed_future is not None + assert isinstance(closed_future, asyncio.Future) + assert not closed_future.done() + + # Subsequent access should return the same future + assert proto.closed is closed_future + + +async def test_closed_property_after_connection_lost( + loop: asyncio.AbstractEventLoop, +) -> None: + """Test that closed property returns None after connection_lost if never accessed.""" + proto = ResponseHandler(loop=loop) + proto.connection_made(mock.Mock()) + + # Don't access proto.closed before connection_lost + proto.connection_lost(None) + + # After connection_lost, closed should return None if it was never accessed + assert proto.closed is None diff --git a/tests/test_client_response.py b/tests/test_client_response.py index b3811973630..2c3666d94bf 100644 --- a/tests/test_client_response.py +++ b/tests/test_client_response.py @@ -3,6 +3,7 @@ import asyncio import gc import sys +from http.cookies import SimpleCookie from json import JSONDecodeError from unittest import mock @@ -12,7 +13,7 @@ from yarl import URL import aiohttp -from aiohttp import ClientSession, http +from aiohttp import ClientSession, hdrs, http from aiohttp.client_reqrep import ClientResponse, RequestInfo from aiohttp.connector import Connection from aiohttp.helpers import TimerNoop @@ -1377,3 +1378,144 @@ def test_response_not_closed_after_get_ok(mocker: MockerFixture) -> None: assert not response.ok assert not response.closed assert spy.call_count == 0 + + +def test_response_duplicate_cookie_names( + loop: asyncio.AbstractEventLoop, session: ClientSession +) -> None: + """ + Test that response.cookies handles duplicate cookie names correctly. + + Note: This behavior (losing cookies with same name but different domains/paths) + is arguably undesirable, but we promise to return a SimpleCookie object, and + SimpleCookie uses cookie name as the key. This is documented behavior. + + To access all cookies including duplicates, users should use: + - response.headers.getall('Set-Cookie') for raw headers + - The session's cookie jar correctly stores all cookies + """ + response = ClientResponse( + "get", + URL("http://example.com"), + request_info=mock.Mock(), + writer=WriterMock(), + continue100=None, + timer=TimerNoop(), + traces=[], + loop=loop, + session=session, + ) + + # Set headers with duplicate cookie names but different domains + headers = CIMultiDict( + [ + ( + "Set-Cookie", + "session-id=123-4567890; Domain=.example.com; Path=/; Secure", + ), + ("Set-Cookie", "session-id=098-7654321; Domain=.www.example.com; Path=/"), + ("Set-Cookie", "user-pref=dark; Domain=.example.com; Path=/"), + ("Set-Cookie", "user-pref=light; Domain=api.example.com; Path=/"), + ] + ) + response._headers = CIMultiDictProxy(headers) + # Set raw cookie headers as done in ClientResponse.start() + response._raw_cookie_headers = tuple(headers.getall("Set-Cookie", [])) + + # SimpleCookie only keeps the last cookie with each name + # This is expected behavior since SimpleCookie uses name as the key + assert len(response.cookies) == 2 # Only 'session-id' and 'user-pref' + assert response.cookies["session-id"].value == "098-7654321" # Last one wins + assert response.cookies["user-pref"].value == "light" # Last one wins + + +def test_response_raw_cookie_headers_preserved( + loop: asyncio.AbstractEventLoop, session: ClientSession +) -> None: + """Test that raw Set-Cookie headers are preserved in _raw_cookie_headers.""" + response = ClientResponse( + "get", + URL("http://example.com"), + request_info=mock.Mock(), + writer=WriterMock(), + continue100=None, + timer=TimerNoop(), + traces=[], + loop=loop, + session=session, + ) + + # Set headers with multiple cookies + cookie_headers = [ + "session-id=123; Domain=.example.com; Path=/; Secure", + "session-id=456; Domain=.www.example.com; Path=/", + "tracking=xyz; Domain=.example.com; Path=/; HttpOnly", + ] + + headers: CIMultiDict[str] = CIMultiDict() + for cookie_hdr in cookie_headers: + headers.add("Set-Cookie", cookie_hdr) + + response._headers = CIMultiDictProxy(headers) + + # Set raw cookie headers as done in ClientResponse.start() + response._raw_cookie_headers = tuple(response.headers.getall(hdrs.SET_COOKIE, [])) + + # Verify raw headers are preserved + assert response._raw_cookie_headers == tuple(cookie_headers) + assert len(response._raw_cookie_headers) == 3 + + # But SimpleCookie only has unique names + assert len(response.cookies) == 2 # 'session-id' and 'tracking' + + +def test_response_cookies_setter_updates_raw_headers( + loop: asyncio.AbstractEventLoop, session: ClientSession +) -> None: + """Test that setting cookies property updates _raw_cookie_headers.""" + response = ClientResponse( + "get", + URL("http://example.com"), + request_info=mock.Mock(), + writer=WriterMock(), + continue100=None, + timer=TimerNoop(), + traces=[], + loop=loop, + session=session, + ) + + # Create a SimpleCookie with some cookies + cookies = SimpleCookie() + cookies["session-id"] = "123456" + cookies["session-id"]["domain"] = ".example.com" + cookies["session-id"]["path"] = "/" + cookies["session-id"]["secure"] = True + + cookies["tracking"] = "xyz789" + cookies["tracking"]["domain"] = ".example.com" + cookies["tracking"]["httponly"] = True + + # Set the cookies property + response.cookies = cookies + + # Verify _raw_cookie_headers was updated + assert response._raw_cookie_headers is not None + assert len(response._raw_cookie_headers) == 2 + assert isinstance(response._raw_cookie_headers, tuple) + + # Check the raw headers contain the expected cookie strings + raw_headers = list(response._raw_cookie_headers) + assert any("session-id=123456" in h for h in raw_headers) + assert any("tracking=xyz789" in h for h in raw_headers) + assert any("Secure" in h for h in raw_headers) + assert any("HttpOnly" in h for h in raw_headers) + + # Verify cookies property returns the same object + assert response.cookies is cookies + + # Test setting empty cookies + empty_cookies = SimpleCookie() + response.cookies = empty_cookies + # Should not set _raw_cookie_headers for empty cookies + assert response._raw_cookie_headers is None diff --git a/tests/test_client_session.py b/tests/test_client_session.py index 0f16a0c7735..63d6e98165e 100644 --- a/tests/test_client_session.py +++ b/tests/test_client_session.py @@ -4,7 +4,7 @@ import io import json from collections import deque -from http.cookies import SimpleCookie +from http.cookies import BaseCookie, SimpleCookie from typing import ( Any, Awaitable, @@ -13,8 +13,10 @@ Iterator, List, NoReturn, + Optional, TypedDict, Union, + cast, ) from unittest import mock from uuid import uuid4 @@ -25,7 +27,7 @@ from yarl import URL import aiohttp -from aiohttp import client, hdrs, tracing, web +from aiohttp import abc, client, hdrs, tracing, web from aiohttp.client import ClientSession from aiohttp.client_proto import ResponseHandler from aiohttp.client_reqrep import ClientRequest, ConnectionKey @@ -692,8 +694,43 @@ async def test_cookie_jar_usage( ) -> None: req_url = None - jar = mock.Mock() - jar.filter_cookies.return_value = None + class MockCookieJar(abc.AbstractCookieJar): + def __init__(self) -> None: + self._update_cookies_mock = mock.Mock() + self._filter_cookies_mock = mock.Mock(return_value=BaseCookie()) + self._clear_mock = mock.Mock() + self._clear_domain_mock = mock.Mock() + self._items: List[Any] = [] + + @property + def quote_cookie(self) -> bool: + return True + + def clear(self, predicate: Optional[abc.ClearCookiePredicate] = None) -> None: + self._clear_mock(predicate) + + def clear_domain(self, domain: str) -> None: + self._clear_domain_mock(domain) + + def update_cookies(self, cookies: Any, response_url: URL = URL()) -> None: + self._update_cookies_mock(cookies, response_url) + + def filter_cookies(self, request_url: URL) -> BaseCookie[str]: + return cast(BaseCookie[str], self._filter_cookies_mock(request_url)) + + def __len__(self) -> int: + return len(self._items) + + def __iter__(self) -> Iterator[Any]: + return iter(self._items) + + jar = MockCookieJar() + + assert jar.quote_cookie is True + assert len(jar) == 0 + assert list(jar) == [] + jar.clear() + jar.clear_domain("example.com") async def handler(request: web.Request) -> web.Response: nonlocal req_url @@ -710,23 +747,25 @@ async def handler(request: web.Request) -> web.Response: ) # Updating the cookie jar with initial user defined cookies - jar.update_cookies.assert_called_with({"request": "req_value"}) + jar._update_cookies_mock.assert_called_with({"request": "req_value"}, URL()) - jar.update_cookies.reset_mock() + jar._update_cookies_mock.reset_mock() resp = await session.get("/") resp.release() assert req_url is not None # Filtering the cookie jar before sending the request, # getting the request URL as only parameter - jar.filter_cookies.assert_called_with(URL(req_url)) + jar._filter_cookies_mock.assert_called_with(URL(req_url)) # Updating the cookie jar with the response cookies - assert jar.update_cookies.called - resp_cookies = jar.update_cookies.call_args[0][0] - assert isinstance(resp_cookies, SimpleCookie) - assert "response" in resp_cookies - assert resp_cookies["response"].value == "resp_value" + assert jar._update_cookies_mock.called + resp_cookies = jar._update_cookies_mock.call_args[0][0] + # Now update_cookies is called with a list of tuples + assert isinstance(resp_cookies, list) + assert len(resp_cookies) == 1 + assert resp_cookies[0][0] == "response" + assert resp_cookies[0][1].value == "resp_value" async def test_cookies_with_not_quoted_cookie_jar( diff --git a/tests/test_connector.py b/tests/test_connector.py index ab854545882..4ae73ce9b9c 100644 --- a/tests/test_connector.py +++ b/tests/test_connector.py @@ -315,6 +315,30 @@ async def test_close(key: ConnectionKey) -> None: assert conn.closed +async def test_close_with_proto_closed_none(key: ConnectionKey) -> None: + """Test close when protocol.closed is None.""" + # Create protocols where closed property returns None + proto1 = mock.create_autospec(ResponseHandler, instance=True) + proto1.closed = None + proto1.close = mock.Mock() + + proto2 = mock.create_autospec(ResponseHandler, instance=True) + proto2.closed = None + proto2.close = mock.Mock() + + conn = aiohttp.BaseConnector() + conn._conns[key] = deque([(proto1, 0)]) + conn._acquired.add(proto2) + + # Close the connector - this should handle the case where proto.closed is None + await conn.close() + + # Verify close was called on both protocols + assert proto1.close.called + assert proto2.close.called + assert conn.closed + + async def test_get(loop: asyncio.AbstractEventLoop, key: ConnectionKey) -> None: conn = aiohttp.BaseConnector() try: diff --git a/tests/test_cookiejar.py b/tests/test_cookiejar.py index 364b8a701c5..3d3fed31f13 100644 --- a/tests/test_cookiejar.py +++ b/tests/test_cookiejar.py @@ -2,6 +2,7 @@ import datetime import heapq import itertools +import logging import pickle import unittest from http.cookies import BaseCookie, Morsel, SimpleCookie @@ -1205,3 +1206,218 @@ async def test_filter_cookies_does_not_leak_memory() -> None: for key, morsels in jar._morsel_cache.items(): assert key in jar._cookies, f"Orphaned morsel cache entry for {key}" assert len(morsels) > 0, f"Empty morsel cache entry found for {key}" + + +def test_update_cookies_from_headers() -> None: + """Test update_cookies_from_headers method.""" + jar: CookieJar = CookieJar() + url: URL = URL("http://example.com/path") + + # Test with simple cookies + headers = [ + "session-id=123456; Path=/", + "user-pref=dark-mode; Domain=.example.com", + "tracking=xyz789; Secure; HttpOnly", + ] + + jar.update_cookies_from_headers(headers, url) + + # Verify all cookies were added to the jar + assert len(jar) == 3 + + # Check cookies available for HTTP URL (secure cookie should be filtered out) + filtered_http: BaseCookie[str] = jar.filter_cookies(url) + assert len(filtered_http) == 2 + assert "session-id" in filtered_http + assert filtered_http["session-id"].value == "123456" + assert "user-pref" in filtered_http + assert filtered_http["user-pref"].value == "dark-mode" + assert "tracking" not in filtered_http # Secure cookie not available on HTTP + + # Check cookies available for HTTPS URL (all cookies should be available) + url_https: URL = URL("https://example.com/path") + filtered_https: BaseCookie[str] = jar.filter_cookies(url_https) + assert len(filtered_https) == 3 + assert "tracking" in filtered_https + assert filtered_https["tracking"].value == "xyz789" + + +def test_update_cookies_from_headers_duplicate_names() -> None: + """Test that duplicate cookie names with different domains are preserved.""" + jar: CookieJar = CookieJar() + url: URL = URL("http://www.example.com/") + + # Headers with duplicate names but different domains + headers: List[str] = [ + "session-id=123456; Domain=.example.com; Path=/", + "session-id=789012; Domain=.www.example.com; Path=/", + "user-pref=light; Domain=.example.com", + "user-pref=dark; Domain=sub.example.com", + ] + + jar.update_cookies_from_headers(headers, url) + + # Should have 3 cookies (user-pref=dark for sub.example.com is rejected) + assert len(jar) == 3 + + # Verify we have both session-id cookies + all_cookies: List[Morsel[str]] = list(jar) + session_ids: List[Morsel[str]] = [c for c in all_cookies if c.key == "session-id"] + assert len(session_ids) == 2 + + # Check their domains are different + domains: Set[str] = {c["domain"] for c in session_ids} + assert domains == {"example.com", "www.example.com"} + + +def test_update_cookies_from_headers_invalid_cookies( + caplog: pytest.LogCaptureFixture, +) -> None: + """Test that invalid cookies are logged and skipped.""" + jar: CookieJar = CookieJar() + url: URL = URL("http://example.com/") + + # Mix of valid and invalid cookies + headers: List[str] = [ + "valid-cookie=value123", + "ISAWPLB{A7F52349-3531-4DA9-8776-F74BC6F4F1BB}=" + "{925EC0B8-CB17-4BEB-8A35-1033813B0523}; " + "HttpOnly; Path=/", # This cookie with curly braces causes CookieError + "another-valid=value456", + ] + + # Enable logging for the client logger + with caplog.at_level(logging.WARNING, logger="aiohttp.client"): + jar.update_cookies_from_headers(headers, url) + + # Check that we logged warnings for invalid cookies + assert "Can not load response cookies" in caplog.text + + # Valid cookies should still be added + assert len(jar) >= 2 # At least the two clearly valid cookies + filtered: BaseCookie[str] = jar.filter_cookies(url) + assert "valid-cookie" in filtered + assert "another-valid" in filtered + + +def test_update_cookies_from_headers_empty_list() -> None: + """Test that empty header list is handled gracefully.""" + jar: CookieJar = CookieJar() + url: URL = URL("http://example.com/") + + # Should not raise any errors + jar.update_cookies_from_headers([], url) + + assert len(jar) == 0 + + +def test_update_cookies_from_headers_with_attributes() -> None: + """Test cookies with various attributes are handled correctly.""" + jar: CookieJar = CookieJar() + url: URL = URL("https://secure.example.com/app/page") + + headers: List[str] = [ + "secure-cookie=value1; Secure; HttpOnly; SameSite=Strict", + "expiring-cookie=value2; Max-Age=3600; Path=/app", + "domain-cookie=value3; Domain=.example.com; Path=/", + "dated-cookie=value4; Expires=Wed, 09 Jun 2030 10:18:14 GMT", + ] + + jar.update_cookies_from_headers(headers, url) + + # All cookies should be stored + assert len(jar) == 4 + + # Verify secure cookie (should work on HTTPS subdomain) + # Note: cookies without explicit path get path from URL (/app) + filtered_https_root: BaseCookie[str] = jar.filter_cookies( + URL("https://secure.example.com/") + ) + assert len(filtered_https_root) == 1 # Only domain-cookie has Path=/ + assert "domain-cookie" in filtered_https_root + + # Check app path + filtered_https_app: BaseCookie[str] = jar.filter_cookies( + URL("https://secure.example.com/app/") + ) + assert len(filtered_https_app) == 4 # All cookies match + assert "secure-cookie" in filtered_https_app + assert "expiring-cookie" in filtered_https_app + assert "domain-cookie" in filtered_https_app + assert "dated-cookie" in filtered_https_app + + # Secure cookie should not be available on HTTP + filtered_http_app: BaseCookie[str] = jar.filter_cookies( + URL("http://secure.example.com/app/") + ) + assert "secure-cookie" not in filtered_http_app + assert "expiring-cookie" in filtered_http_app # Non-secure cookies still available + assert "domain-cookie" in filtered_http_app + assert "dated-cookie" in filtered_http_app + + +def test_update_cookies_from_headers_preserves_existing() -> None: + """Test that update_cookies_from_headers preserves existing cookies.""" + jar: CookieJar = CookieJar() + url: URL = URL("http://example.com/") + + # Add some initial cookies + jar.update_cookies( + { + "existing1": "value1", + "existing2": "value2", + }, + url, + ) + + # Add more cookies via headers + headers: List[str] = [ + "new-cookie1=value3", + "new-cookie2=value4", + ] + + jar.update_cookies_from_headers(headers, url) + + # Should have all 4 cookies + assert len(jar) == 4 + filtered: BaseCookie[str] = jar.filter_cookies(url) + assert "existing1" in filtered + assert "existing2" in filtered + assert "new-cookie1" in filtered + assert "new-cookie2" in filtered + + +def test_update_cookies_from_headers_overwrites_same_cookie() -> None: + """Test that cookies with same name/domain/path are overwritten.""" + jar: CookieJar = CookieJar() + url: URL = URL("http://example.com/") + + # Add initial cookie + jar.update_cookies({"session": "old-value"}, url) + + # Update with new value via headers + headers: List[str] = ["session=new-value"] + jar.update_cookies_from_headers(headers, url) + + # Should still have just 1 cookie with updated value + assert len(jar) == 1 + filtered: BaseCookie[str] = jar.filter_cookies(url) + assert filtered["session"].value == "new-value" + + +def test_dummy_cookie_jar_update_cookies_from_headers() -> None: + """Test that DummyCookieJar ignores update_cookies_from_headers.""" + jar: DummyCookieJar = DummyCookieJar() + url: URL = URL("http://example.com/") + + headers: List[str] = [ + "cookie1=value1", + "cookie2=value2", + ] + + # Should not raise and should not store anything + jar.update_cookies_from_headers(headers, url) + + assert len(jar) == 0 + filtered: BaseCookie[str] = jar.filter_cookies(url) + assert len(filtered) == 0