diff --git a/CHANGES/13671.bugfix.rst b/CHANGES/13671.bugfix.rst new file mode 100644 index 00000000000..e6fbc7000c7 --- /dev/null +++ b/CHANGES/13671.bugfix.rst @@ -0,0 +1 @@ +Fixed pure-Python request parser not reading a body in a ``HEAD`` request -- by :user:`Dreamsorcerer`. diff --git a/CHANGES/13674.bugfix.rst b/CHANGES/13674.bugfix.rst new file mode 100644 index 00000000000..5152bceae62 --- /dev/null +++ b/CHANGES/13674.bugfix.rst @@ -0,0 +1 @@ +Fixed host-only cookie state being lost on expiration -- by :user:`Dreamsorcerer`. diff --git a/CHANGES/13677.bugfix.rst b/CHANGES/13677.bugfix.rst new file mode 100644 index 00000000000..bea67cba799 --- /dev/null +++ b/CHANGES/13677.bugfix.rst @@ -0,0 +1 @@ +Fixed a possible ``OverflowError`` on cookies and a connection not being closed properly -- by :user:`Dreamsorcerer`. diff --git a/aiohttp/abc.py b/aiohttp/abc.py index c90f9f459ee..b9c7199a20c 100644 --- a/aiohttp/abc.py +++ b/aiohttp/abc.py @@ -171,7 +171,7 @@ def cookies(self) -> MappingProxyType[tuple[str, str], SimpleCookie]: @property @abstractmethod - def host_only_cookies(self) -> frozenset[tuple[str, str]]: + def host_only_cookies(self) -> frozenset[tuple[str, str, str]]: """Return the host-only cookies stored in this jar.""" @abstractmethod diff --git a/aiohttp/client.py b/aiohttp/client.py index f91b81e5712..b5f563d3f3f 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -602,6 +602,7 @@ async def _request( timer = tm.timer() req: ClientRequest | None = None + resp: ClientResponse | None = None try: with timer: # https://www.rfc-editor.org/rfc/rfc9112.html#name-retrying-requests @@ -892,6 +893,10 @@ async def _request( handle.cancel() handle = None + if resp is not None: + # A failure occurred after the response was received. + resp.close() + if req is not None and req._body is not None: await req._body.close() diff --git a/aiohttp/cookiejar.py b/aiohttp/cookiejar.py index a5a1052970d..913e4a51ec4 100644 --- a/aiohttp/cookiejar.py +++ b/aiohttp/cookiejar.py @@ -92,7 +92,8 @@ def __init__( self._morsel_cache: defaultdict[tuple[str, str], dict[str, Morsel[str]]] = ( defaultdict(dict) ) - self._host_only_cookies: set[tuple[str, str]] = set() + # Cookie identity is (domain, path, name). + self._host_only_cookies: set[tuple[str, str, str]] = set() self._unsafe = unsafe self._quote_cookie = quote_cookie if treat_as_secure_origin is None: @@ -127,7 +128,7 @@ def cookies(self) -> MappingProxyType[tuple[str, str], SimpleCookie]: return MappingProxyType(self._cookies) @property - def host_only_cookies(self) -> frozenset[tuple[str, str]]: + def host_only_cookies(self) -> frozenset[tuple[str, str, str]]: """Return the host-only cookies stored in this jar.""" return frozenset(self._host_only_cookies) @@ -156,7 +157,7 @@ def save(self, file_path: PathLike) -> None: if attr_val: morsel_data[attr] = attr_val # Persist or it reloads as a domain cookie and leaks to subdomains. - if (domain, name) in self._host_only_cookies: + if (domain, path, name) in self._host_only_cookies: morsel_data["host_only"] = True if (exp := self._expirations.get((domain, path, name))) is not None: morsel_data["expires_timestamp"] = exp @@ -309,7 +310,7 @@ def _do_expiration(self) -> None: def _delete_cookies(self, to_del: list[tuple[str, str, str]]) -> None: for domain, path, name in to_del: - self._host_only_cookies.discard((domain, name)) + self._host_only_cookies.discard((domain, path, name)) self._cookies[(domain, path)].pop(name, None) self._morsel_cache[(domain, path)].pop(name, None) self._expirations.pop((domain, path, name), None) @@ -363,18 +364,12 @@ def _update_cookies( domain = "" del cookie["domain"] - if not domain and hostname is not None: - # Set the cookie's domain to the response hostname - # and set its host-only-flag - self._host_only_cookies.add((hostname, name)) - domain = cookie["domain"] = hostname - if domain and domain[0] == ".": # Remove leading dot domain = domain[1:] cookie["domain"] = domain - if hostname and not self._is_domain_match(domain, hostname): + if domain and hostname and not self._is_domain_match(domain, hostname): # Setting cookies for different domains is not allowed continue @@ -390,10 +385,26 @@ def _update_cookies( cookie["path"] = path path = path.rstrip("/") + if not domain and hostname is not None: + self._host_only_cookies.add((hostname, path, name)) + domain = cookie["domain"] = hostname + else: + # A cookie with an explicit Domain attribute replaces any + # host-only cookie with the same (domain, path, name) identity. + self._host_only_cookies.discard((domain, path, name)) + if max_age := cookie["max-age"]: try: delta_seconds = int(max_age) - max_age_expiration = min(time.time() + delta_seconds, self.MAX_TIME) + # https://datatracker.ietf.org/doc/html/rfc6265#section-5.2.2 + if delta_seconds <= 0: + max_age_expiration = 0.0 + else: + # Cap first to protect against OverflowError on next line. + delta_seconds = min(delta_seconds, self.MAX_TIME) + max_age_expiration = min( + time.time() + delta_seconds, self.MAX_TIME + ) self._expire_cookie(max_age_expiration, domain, path, name) except ValueError: cookie["max-age"] = "" @@ -477,7 +488,7 @@ def filter_cookies(self, request_url: URL) -> "BaseCookie[str]": for name, cookie in self._cookies[p].items(): domain = cookie["domain"] - if (domain, name) in self._host_only_cookies and domain != hostname: + if domain != hostname and p + (name,) in self._host_only_cookies: continue # Skip edge case when the cookie has a trailing slash but request doesn't. @@ -622,7 +633,7 @@ def cookies(self) -> MappingProxyType[tuple[str, str], SimpleCookie]: return MappingProxyType({}) @property - def host_only_cookies(self) -> frozenset[tuple[str, str]]: + def host_only_cookies(self) -> frozenset[tuple[str, str, str]]: """Return an empty frozenset.""" return frozenset() diff --git a/aiohttp/http_parser.py b/aiohttp/http_parser.py index e5ba3d129f2..2484f62489a 100644 --- a/aiohttp/http_parser.py +++ b/aiohttp/http_parser.py @@ -423,8 +423,12 @@ def get_content_length() -> int | None: assert self.protocol is not None # calculate payload + # https://www.rfc-editor.org/info/rfc9112/#name-message-body-length + # https://www.rfc-editor.org/info/rfc9110/#section-9.3.1-6 + # EMPTY_BODY_METHODS should only apply to responses. + # self.method is None on request parser. empty_body = code in EMPTY_BODY_STATUS_CODES or bool( - method and method in EMPTY_BODY_METHODS + self.method and self.method in EMPTY_BODY_METHODS ) if not empty_body and ( (length is not None and length > 0) or msg.chunked diff --git a/docs/client_reference.rst b/docs/client_reference.rst index 8b5ec33e0ac..5ec5f204b11 100644 --- a/docs/client_reference.rst +++ b/docs/client_reference.rst @@ -2556,11 +2556,17 @@ Utilities .. attribute:: host_only_cookies - A :class:`frozenset` of ``(domain, name)`` tuples indicating which - cookies are host-only (not sent to subdomains). + A :class:`frozenset` of ``(domain, path, name)`` tuples indicating + which cookies are host-only (not sent to subdomains). .. versionadded:: 3.14 + .. versionchanged:: 3.14.4 + + The tuples gained the *path* element; host-only state is tracked + per ``(domain, path, name)`` cookie identity so that same-named + cookies on other paths cannot affect it. + .. class:: DummyCookieJar(*, loop=None) :canonical: aiohttp.cookiejar.DummyCookieJar diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index f0b37facbf8..7e6de06936d 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -15,7 +15,7 @@ import time import zipfile import zlib -from collections.abc import AsyncIterator, Awaitable, Callable +from collections.abc import AsyncIterator, Awaitable, Callable, Sequence from contextlib import suppress from typing import TYPE_CHECKING, Any, NoReturn from unittest import mock @@ -3019,6 +3019,50 @@ async def handler(request: web.Request) -> web.Response: assert int(cookie["max-age"]) == int(overflow) +async def test_connection_released_when_cookie_processing_fails( + aiohttp_client: AiohttpClient, +) -> None: + class EvilJar(aiohttp.CookieJar): + def update_cookies_from_headers( + self, headers: Sequence[str], response_url: URL + ) -> None: + raise RuntimeError("boom") + + hold = asyncio.Event() + + async def hostile(request: web.Request) -> web.StreamResponse: + ret = web.StreamResponse() + ret.content_length = 2 + ret.set_cookie("sid", "x") + await ret.prepare(request) + await ret.write(b"x") + await hold.wait() + assert False + + async def clean(request: web.Request) -> web.Response: + return web.Response() + + app = web.Application() + app.router.add_get("/hostile", hostile) + app.router.add_get("/clean", clean) + connector = aiohttp.TCPConnector(limit=1) + client = await aiohttp_client(app, connector=connector, cookie_jar=EvilJar()) + + try: + for _ in range(2): + with pytest.raises(RuntimeError, match="boom") as excinfo: + await client.get("/hostile") + assert not connector._acquired + del excinfo + + # The single connector slot is free again: an unaffected request + # succeeds instead of waiting forever for a connection. + async with client.get("/clean") as resp: + assert resp.status == 200 + finally: + hold.set() + + async def test_request_conn_error() -> None: async with aiohttp.ClientSession() as client: with pytest.raises(aiohttp.ClientConnectionError): diff --git a/tests/test_client_session.py b/tests/test_client_session.py index e24e07b9a6e..3851416320d 100644 --- a/tests/test_client_session.py +++ b/tests/test_client_session.py @@ -792,7 +792,7 @@ def cookies(self) -> MappingProxyType[tuple[str, str], SimpleCookie]: return MappingProxyType({}) @property - def host_only_cookies(self) -> frozenset[tuple[str, str]]: + def host_only_cookies(self) -> frozenset[tuple[str, str, str]]: return frozenset() def clear(self, predicate: abc.ClearCookiePredicate | None = None) -> None: diff --git a/tests/test_cookiejar.py b/tests/test_cookiejar.py index d020305f3d3..72d9b28c1d5 100644 --- a/tests/test_cookiejar.py +++ b/tests/test_cookiejar.py @@ -825,7 +825,45 @@ async def test_cookie_jar_host_only_cookies_property() -> None: host_only = jar.host_only_cookies assert isinstance(host_only, frozenset) - assert ("example.com", "hostonly") in host_only + assert ("example.com", "", "hostonly") in host_only + + +def test_host_only_marker_survives_same_name_expiry_on_other_path() -> None: + """Expiring a same-name cookie on another path must not clear host-only state.""" + jar = CookieJar() + origin = URL("http://auth.example.com/") + subdomain = URL("http://evil.auth.example.com/") + + jar.update_cookies_from_headers(["sid=secret; Path=/"], origin) + assert "sid" not in jar.filter_cookies(subdomain) + + # Attacker-controlled descendant expires a same-name cookie on its own path. + jar.update_cookies_from_headers( + ["sid=gone; Domain=auth.example.com; Path=/attacker; Max-Age=0"], + subdomain, + ) + + assert ("auth.example.com", "", "sid") in jar.host_only_cookies + assert "sid" not in jar.filter_cookies(subdomain) + assert jar.filter_cookies(origin)["sid"].value == "secret" + + +def test_explicit_domain_replacement_clears_host_only_marker() -> None: + """A replacing cookie with an explicit Domain is a domain cookie.""" + jar = CookieJar() + origin = URL("http://example.com/") + subdomain = URL("http://sub.example.com/") + + jar.update_cookies_from_headers(["sid=hostonly; Path=/"], origin) + assert ("example.com", "", "sid") in jar.host_only_cookies + assert "sid" not in jar.filter_cookies(subdomain) + + jar.update_cookies_from_headers( + ["sid=domainwide; Domain=example.com; Path=/"], origin + ) + + assert jar.host_only_cookies == frozenset() + assert jar.filter_cookies(subdomain)["sid"].value == "domainwide" async def test_cookie_jar_cookies_property_immutable() -> None: @@ -1686,7 +1724,7 @@ def test_save_load_json_preserves_host_only_scope(tmp_path: Path) -> None: jar_load = CookieJar() jar_load.load(file_path=file_path) - assert jar_load.host_only_cookies == frozenset({("auth.example.com", "sid")}) + assert jar_load.host_only_cookies == frozenset({("auth.example.com", "", "sid")}) assert "sid" not in jar_load.filter_cookies(subdomain) assert "sid" in jar_load.filter_cookies(issuer) @@ -1711,6 +1749,28 @@ def test_save_load_json_domain_cookie_still_matches_subdomain( assert "sid" in jar_load.filter_cookies(subdomain) +def test_save_load_json_host_only_per_path(tmp_path: Path) -> None: + """Verify save/load keeps host-only state per (domain, path, name).""" + file_path = tmp_path / "per_path.json" + origin = URL("https://example.com/") + subdomain = URL("https://sub.example.com/") + + jar_save = CookieJar() + jar_save.update_cookies_from_headers( + ["sid=hostonly; Path=/", "sid=domainwide; Domain=example.com; Path=/api"], + origin, + ) + jar_save.save(file_path=file_path) + + jar_load = CookieJar() + jar_load.load(file_path=file_path) + + assert jar_load.host_only_cookies == frozenset({("example.com", "", "sid")}) + assert "sid" not in jar_load.filter_cookies(subdomain) + filtered = jar_load.filter_cookies(URL("https://sub.example.com/api/x")) + assert filtered["sid"].value == "domainwide" + + def test_save_load_json_preserves_max_age_deadline(tmp_path: Path) -> None: """Verify save/load restores the absolute deadline without resetting it.""" file_path = tmp_path / "max_age.json" @@ -1898,3 +1958,25 @@ async def test_cookie_jar_unsafe_property() -> None: jar_unsafe = CookieJar(unsafe=True) assert jar_unsafe.unsafe is True + + +def test_update_cookies_max_age_beyond_float_range_is_clamped() -> None: + """A hostile Max-Age larger than float max must clamp, not raise OverflowError.""" + url = URL("https://example.com/") + jar = CookieJar() + + jar.update_cookies_from_headers([f"sid=x; Max-Age={'9' * 309}"], url) + + assert "sid" in jar.filter_cookies(url) + assert jar._expirations[("example.com", "", "sid")] == CookieJar.MAX_TIME + + +def test_update_cookies_negative_max_age_beyond_float_range_expires() -> None: + """A negative Max-Age below float min must expire the cookie, not raise.""" + url = URL("https://example.com/") + jar = CookieJar() + + jar.update_cookies_from_headers([f"sid=x; Max-Age=-{'9' * 309}"], url) + + assert "sid" not in jar.filter_cookies(url) + assert len(jar) == 0 diff --git a/tests/test_http_parser.py b/tests/test_http_parser.py index 846f8392759..a04e5ea1e9a 100644 --- a/tests/test_http_parser.py +++ b/tests/test_http_parser.py @@ -2504,6 +2504,31 @@ def test_http_request_chunked_payload_and_next_message( assert not payload2.is_eof() +def test_http_request_parser_head_with_content_length_payload( + parser: HttpRequestParser, +) -> None: + smuggled = b"GET /smuggled HTTP/1.1\r\nHost: a\r\n\r\n" + text = ( + b"HEAD /test HTTP/1.1\r\nHost: a\r\nContent-Length: %d\r\n\r\n" % len(smuggled) + + smuggled + + b"POST /next HTTP/1.1\r\nHost: a\r\nContent-Length: 0\r\n\r\n" + ) + messages, upgraded, tail = parser.feed_data(text) + + assert len(messages) == 2 + msg, payload = messages[0] + assert msg.method == "HEAD" + assert b"".join(payload._buffer) == smuggled + assert payload.is_eof() + + msg2, payload2 = messages[1] + assert msg2.method == "POST" + assert msg2.path == "/next" + assert payload2.is_eof() + assert not upgraded + assert not tail + + def test_http_request_chunked_payload_chunks(parser: HttpRequestParser) -> None: text = b"GET /test HTTP/1.1\r\nHost: a\r\ntransfer-encoding: chunked\r\n\r\n" msg, payload = parser.feed_data(text)[0][0]