diff --git a/CHANGES/11632.bugfix.rst b/CHANGES/11632.bugfix.rst new file mode 100644 index 00000000000..c07bfb2b1f7 --- /dev/null +++ b/CHANGES/11632.bugfix.rst @@ -0,0 +1 @@ +Fixed cookie parser to continue parsing subsequent cookies when encountering a malformed cookie that fails regex validation, such as Google's ``g_state`` cookie with unescaped quotes -- by :user:`bdraco`. diff --git a/CHANGES/11713.bugfix.rst b/CHANGES/11713.bugfix.rst new file mode 100644 index 00000000000..dbb45a5254f --- /dev/null +++ b/CHANGES/11713.bugfix.rst @@ -0,0 +1 @@ +Fixed loading netrc credentials from the default :file:`~/.netrc` (:file:`~/_netrc` on Windows) location when the :envvar:`NETRC` environment variable is not set -- by :user:`bdraco`. diff --git a/CHANGES/11714.bugfix.rst b/CHANGES/11714.bugfix.rst new file mode 120000 index 00000000000..5a506f1ded3 --- /dev/null +++ b/CHANGES/11714.bugfix.rst @@ -0,0 +1 @@ +11713.bugfix.rst \ No newline at end of file diff --git a/aiohttp/_cookie_helpers.py b/aiohttp/_cookie_helpers.py index 7fe8f43d12b..20a278b0d5b 100644 --- a/aiohttp/_cookie_helpers.py +++ b/aiohttp/_cookie_helpers.py @@ -166,7 +166,10 @@ def parse_cookie_header(header: str) -> list[tuple[str, Morsel[str]]]: attribute names (like 'path' or 'secure') should be treated as cookies. This parser uses the same regex-based approach as parse_set_cookie_headers - to properly handle quoted values that may contain semicolons. + to properly handle quoted values that may contain semicolons. When the + regex fails to match a malformed cookie, it falls back to simple parsing + to ensure subsequent cookies are not lost + https://github.com/aio-libs/aiohttp/issues/11632 Args: header: The Cookie header value to parse @@ -177,6 +180,7 @@ def parse_cookie_header(header: str) -> list[tuple[str, Morsel[str]]]: if not header: return [] + morsel: Morsel[str] cookies: list[tuple[str, Morsel[str]]] = [] i = 0 n = len(header) @@ -185,7 +189,32 @@ def parse_cookie_header(header: str) -> list[tuple[str, Morsel[str]]]: # Use the same pattern as parse_set_cookie_headers to find cookies match = _COOKIE_PATTERN.match(header, i) if not match: - break + # Fallback for malformed cookies https://github.com/aio-libs/aiohttp/issues/11632 + # Find next semicolon to skip or attempt simple key=value parsing + next_semi = header.find(";", i) + eq_pos = header.find("=", i) + + # Try to extract key=value if '=' comes before ';' + if eq_pos != -1 and (next_semi == -1 or eq_pos < next_semi): + end_pos = next_semi if next_semi != -1 else n + key = header[i:eq_pos].strip() + value = header[eq_pos + 1 : end_pos].strip() + + # Validate the name (same as regex path) + if not _COOKIE_NAME_RE.match(key): + internal_logger.warning( + "Can not load cookie: Illegal cookie name %r", key + ) + else: + morsel = Morsel() + morsel.__setstate__( # type: ignore[attr-defined] + {"key": key, "value": _unquote(value), "coded_value": value} + ) + cookies.append((key, morsel)) + + # Move to next cookie or end + i = next_semi + 1 if next_semi != -1 else n + continue key = match.group("key") value = match.group("val") or "" @@ -197,7 +226,7 @@ def parse_cookie_header(header: str) -> list[tuple[str, Morsel[str]]]: continue # Create new morsel - morsel: Morsel[str] = Morsel() + morsel = Morsel() # Preserve the original value as coded_value (with quotes if present) # We use __setstate__ instead of the public set() API because it allows us to # bypass validation and set already validated state. This is more stable than diff --git a/aiohttp/client.py b/aiohttp/client.py index 059f1adc401..026006023ce 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -590,14 +590,7 @@ async def _request( auth = self._default_auth # Try netrc if auth is still None and trust_env is enabled. - # Only check if NETRC environment variable is set to avoid - # creating an expensive executor job unnecessarily. - if ( - auth is None - and self._trust_env - and url.host is not None - and os.environ.get("NETRC") - ): + if auth is None and self._trust_env and url.host is not None: auth = await self._loop.run_in_executor( None, self._get_netrc_auth, url.host ) diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index 347ec198e24..b1aa569b569 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -341,8 +341,9 @@ tuples UI un unawaited -undercounting unclosed +undercounting +unescaped unhandled unicode unittest diff --git a/tests/conftest.py b/tests/conftest.py index e5dc79cad4d..0dd0eb050ab 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,6 +3,7 @@ import asyncio import base64 import os +import platform import socket import ssl import sys @@ -329,6 +330,23 @@ def netrc_other_host(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> Path: return netrc_file +@pytest.fixture +def netrc_home_directory(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> Path: + """Create a netrc file in a mocked home directory without setting NETRC env var.""" + home_dir = tmp_path / "home" + home_dir.mkdir() + netrc_filename = "_netrc" if platform.system() == "Windows" else ".netrc" + netrc_file = home_dir / netrc_filename + netrc_file.write_text("default login netrc_user password netrc_pass\n") + + home_env_var = "USERPROFILE" if platform.system() == "Windows" else "HOME" + monkeypatch.setenv(home_env_var, str(home_dir)) + # Ensure NETRC env var is not set + monkeypatch.delenv("NETRC", raising=False) + + return netrc_file + + @pytest.fixture def start_connection() -> Iterator[mock.Mock]: with mock.patch( diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index 473427278f8..95b40cce9bb 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -3748,12 +3748,12 @@ async def test_netrc_auth_from_env( # type: ignore[misc] @pytest.mark.usefixtures("no_netrc") -async def test_netrc_auth_skipped_without_env_var( # type: ignore[misc] +async def test_netrc_auth_skipped_without_netrc_file( # type: ignore[misc] headers_echo_client: Callable[ ..., Awaitable[TestClient[web.Request, web.Application]] ], ) -> None: - """Test that netrc authentication is skipped when NETRC env var is not set.""" + """Test that netrc authentication is skipped when no netrc file exists.""" client = await headers_echo_client(trust_env=True) async with client.get("/") as r: assert r.status == 200 @@ -3762,6 +3762,20 @@ async def test_netrc_auth_skipped_without_env_var( # type: ignore[misc] assert "Authorization" not in content["headers"] +@pytest.mark.usefixtures("netrc_home_directory") +async def test_netrc_auth_from_home_directory( # type: ignore[misc] + headers_echo_client: Callable[ + ..., Awaitable[TestClient[web.Request, web.Application]] + ], +) -> None: + """Test that netrc authentication works from default ~/.netrc without NETRC env var.""" + client = await headers_echo_client(trust_env=True) + async with client.get("/") as r: + assert r.status == 200 + content = await r.json() + assert content["headers"]["Authorization"] == "Basic bmV0cmNfdXNlcjpuZXRyY19wYXNz" + + @pytest.mark.usefixtures("netrc_default_contents") async def test_netrc_auth_overridden_by_explicit_auth( # type: ignore[misc] headers_echo_client: Callable[ diff --git a/tests/test_client_session.py b/tests/test_client_session.py index 21057d3fbb5..e9106c3443d 100644 --- a/tests/test_client_session.py +++ b/tests/test_client_session.py @@ -1368,8 +1368,8 @@ async def test_netrc_auth_skipped_without_trust_env(auth_server: TestServer) -> @pytest.mark.usefixtures("no_netrc") -async def test_netrc_auth_skipped_without_netrc_env(auth_server: TestServer) -> None: - """Test that netrc authentication is skipped when NETRC env var is not set.""" +async def test_netrc_auth_skipped_without_netrc_file(auth_server: TestServer) -> None: + """Test that netrc authentication is skipped when no netrc file exists.""" async with ( ClientSession(trust_env=True) as session, session.get(auth_server.make_url("/")) as resp, @@ -1378,6 +1378,17 @@ async def test_netrc_auth_skipped_without_netrc_env(auth_server: TestServer) -> assert text == "no_auth" +@pytest.mark.usefixtures("netrc_home_directory") +async def test_netrc_auth_from_home_directory(auth_server: TestServer) -> None: + """Test that netrc authentication works from default ~/.netrc location without NETRC env var.""" + async with ( + ClientSession(trust_env=True) as session, + session.get(auth_server.make_url("/")) as resp, + ): + text = await resp.text() + assert text == "auth:Basic bmV0cmNfdXNlcjpuZXRyY19wYXNz" + + @pytest.mark.usefixtures("netrc_default_contents") async def test_netrc_auth_overridden_by_explicit_auth(auth_server: TestServer) -> None: """Test that explicit auth parameter overrides netrc authentication.""" diff --git a/tests/test_cookie_helpers.py b/tests/test_cookie_helpers.py index 575bbe54d01..577e3156560 100644 --- a/tests/test_cookie_helpers.py +++ b/tests/test_cookie_helpers.py @@ -1137,7 +1137,6 @@ def test_parse_cookie_header_empty() -> None: assert parse_cookie_header(" ") == [] -@pytest.mark.xfail(reason="https://github.com/aio-libs/aiohttp/issues/11632") def test_parse_cookie_gstate_header() -> None: header = ( "_ga=ga; " @@ -1444,6 +1443,142 @@ def test_parse_cookie_header_illegal_names(caplog: pytest.LogCaptureFixture) -> assert "Can not load cookie: Illegal cookie name 'invalid,cookie'" in caplog.text +def test_parse_cookie_header_large_value() -> None: + """Test that large cookie values don't cause DoS.""" + large_value = "A" * 8192 + header = f"normal=value; large={large_value}; after=cookie" + + result = parse_cookie_header(header) + cookie_names = [name for name, _ in result] + + assert len(result) == 3 + assert "normal" in cookie_names + assert "large" in cookie_names + assert "after" in cookie_names + + large_cookie = next(morsel for name, morsel in result if name == "large") + assert len(large_cookie.value) == 8192 + + +def test_parse_cookie_header_multiple_equals() -> None: + """Test handling of multiple equals signs in cookie values.""" + header = "session=abc123; data=key1=val1&key2=val2; token=xyz" + + result = parse_cookie_header(header) + + assert len(result) == 3 + + name1, morsel1 = result[0] + assert name1 == "session" + assert morsel1.value == "abc123" + + name2, morsel2 = result[1] + assert name2 == "data" + assert morsel2.value == "key1=val1&key2=val2" + + name3, morsel3 = result[2] + assert name3 == "token" + assert morsel3.value == "xyz" + + +def test_parse_cookie_header_fallback_preserves_subsequent_cookies() -> None: + """Test that fallback parser doesn't lose subsequent cookies.""" + header = 'normal=value; malformed={"json":"value"}; after1=cookie1; after2=cookie2' + + result = parse_cookie_header(header) + cookie_names = [name for name, _ in result] + + assert len(result) == 4 + assert cookie_names == ["normal", "malformed", "after1", "after2"] + + name1, morsel1 = result[0] + assert morsel1.value == "value" + + name2, morsel2 = result[1] + assert morsel2.value == '{"json":"value"}' + + name3, morsel3 = result[2] + assert morsel3.value == "cookie1" + + name4, morsel4 = result[3] + assert morsel4.value == "cookie2" + + +def test_parse_cookie_header_whitespace_in_fallback() -> None: + """Test that fallback parser handles whitespace correctly.""" + header = "a=1; b = 2 ; c= 3; d =4" + + result = parse_cookie_header(header) + + assert len(result) == 4 + for name, morsel in result: + assert name in ("a", "b", "c", "d") + assert morsel.value in ("1", "2", "3", "4") + + +def test_parse_cookie_header_empty_value_in_fallback() -> None: + """Test that fallback handles empty values correctly.""" + header = "normal=value; empty=; another=test" + + result = parse_cookie_header(header) + + assert len(result) == 3 + + name1, morsel1 = result[0] + assert name1 == "normal" + assert morsel1.value == "value" + + name2, morsel2 = result[1] + assert name2 == "empty" + assert morsel2.value == "" + + name3, morsel3 = result[2] + assert name3 == "another" + assert morsel3.value == "test" + + +def test_parse_cookie_header_invalid_name_in_fallback( + caplog: pytest.LogCaptureFixture, +) -> None: + """Test that fallback parser rejects cookies with invalid names.""" + header = 'normal=value; invalid,name={"x":"y"}; another=test' + + result = parse_cookie_header(header) + + assert len(result) == 2 + + name1, morsel1 = result[0] + assert name1 == "normal" + assert morsel1.value == "value" + + name2, morsel2 = result[1] + assert name2 == "another" + assert morsel2.value == "test" + + assert "Can not load cookie: Illegal cookie name 'invalid,name'" in caplog.text + + +def test_parse_cookie_header_empty_key_in_fallback( + caplog: pytest.LogCaptureFixture, +) -> None: + """Test that fallback parser logs warning for empty cookie names.""" + header = 'normal=value; ={"malformed":"json"}; another=test' + + result = parse_cookie_header(header) + + assert len(result) == 2 + + name1, morsel1 = result[0] + assert name1 == "normal" + assert morsel1.value == "value" + + name2, morsel2 = result[1] + assert name2 == "another" + assert morsel2.value == "test" + + assert "Can not load cookie: Illegal cookie name ''" in caplog.text + + @pytest.mark.parametrize( ("input_str", "expected"), [