-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix CookieError crash with control characters on Python 3.13 #12395
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
base: master
Are you sure you want to change the base?
Changes from 11 commits
17db306
e33ff01
5b9a07c
eee0ad7
12274d0
5ad7288
9c556cf
659bc5a
78d8173
d6c7ad8
cece051
9317e23
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,6 @@ | ||||||
| Fixed a crash (``CookieError``) in the cookie parser when receiving cookies | ||||||
| containing ASCII control characters on CPython builds with the CVE-2026-3644 | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sphinx comes with a built-in role for this
Suggested change
|
||||||
| patch. The parser now gracefully falls back to storing the raw, still-escaped | ||||||
| ``coded_value`` when the decoded value contains control characters, and skips | ||||||
| cookies whose raw header contains literal control characters that cannot be | ||||||
| safely stored -- by :user:`rodrigobnogueira`. | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
|
|
||
| import re | ||
| from collections.abc import Sequence | ||
| from http.cookies import Morsel | ||
| from http.cookies import CookieError, Morsel | ||
| from typing import cast | ||
|
|
||
| from .log import internal_logger | ||
|
|
@@ -82,6 +82,48 @@ | |
| ) | ||
|
|
||
|
|
||
| def _safe_set_morsel_state( | ||
| morsel: Morsel[str], | ||
| key: str, | ||
| value: str, | ||
| coded_value: str, | ||
| ) -> bool: | ||
| r"""Set morsel state, handling control-character rejection after CVE-2026-3644. | ||
|
|
||
| CPython builds that include the CVE-2026-3644 patch added validation in | ||
| ``Morsel.__setstate__`` that rejects values containing ASCII control | ||
| characters. When ``_unquote`` decodes octal escape sequences | ||
| (e.g. ``\012`` → ``\n``) the resulting value may contain such characters. | ||
|
|
||
| When that happens we fall back to storing the *raw* (still-escaped) | ||
| ``coded_value`` as both ``value`` and ``coded_value`` so the cookie | ||
| is preserved without crashing. | ||
|
|
||
| If the ``coded_value`` itself contains literal control characters | ||
| (e.g. a raw ``\x07`` in the header), the cookie is unsalvageable and | ||
| the function returns ``False`` so the caller can skip it. | ||
|
|
||
| Returns: | ||
| True if the morsel state was set successfully, False if the | ||
| cookie should be skipped. | ||
| """ | ||
| try: | ||
| morsel.__setstate__( # type: ignore[attr-defined] | ||
| {"key": key, "value": value, "coded_value": coded_value} | ||
| ) | ||
| except CookieError: | ||
| # The decoded value contains control characters rejected after | ||
| # CVE-2026-3644. Fall back to keeping the raw coded_value. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still unclear to implications of allowing the coded_value here, rather than just rejecting the cookie entirely.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fallback to coded_value preserves the cookie with its original escaped representation (e.g. "\012newline\012") instead of dropping it entirely. The coded_value at this point contains only printable ASCII (backslash-escaped octals), so there are no injection or smuggling concerns. If the application later echoes this cookie back via If you'd prefer to reject the cookie entirely instead, I can simplify the function to a single try/except that returns False on any
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still leaning towards rejection. See what @bdraco thinks.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! Code is simpler now. I like it.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. Reject seems to be the way to go. Someone will complain if we really need to allow this |
||
| try: | ||
| morsel.__setstate__( # type: ignore[attr-defined] | ||
| {"key": key, "value": coded_value, "coded_value": coded_value} | ||
| ) | ||
| except CookieError: | ||
| # coded_value itself has literal control chars — unsalvageable. | ||
| return False | ||
| return True | ||
|
|
||
|
|
||
| def preserve_morsel_with_coded_value(cookie: Morsel[str]) -> Morsel[str]: | ||
| """ | ||
| Preserve a Morsel's coded_value exactly as received from the server. | ||
|
|
@@ -102,13 +144,10 @@ def preserve_morsel_with_coded_value(cookie: Morsel[str]) -> Morsel[str]: | |
|
|
||
| """ | ||
| mrsl_val = cast("Morsel[str]", cookie.get(cookie.key, Morsel())) | ||
| # 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 | ||
| # setting protected attributes directly and unlikely to change since it would | ||
| # break pickling. | ||
| mrsl_val.__setstate__( # type: ignore[attr-defined] | ||
| {"key": cookie.key, "value": cookie.value, "coded_value": cookie.coded_value} | ||
| ) | ||
| if not _safe_set_morsel_state( | ||
| mrsl_val, cookie.key, cookie.value, cookie.coded_value | ||
| ): | ||
| return cookie | ||
| return mrsl_val | ||
|
|
||
|
|
||
|
|
@@ -206,10 +245,8 @@ def parse_cookie_header(header: str) -> list[tuple[str, Morsel[str]]]: | |
| invalid_names.append(key) | ||
| else: | ||
| morsel = Morsel() | ||
| morsel.__setstate__( # type: ignore[attr-defined] | ||
| {"key": key, "value": _unquote(value), "coded_value": value} | ||
| ) | ||
| cookies.append((key, morsel)) | ||
| if _safe_set_morsel_state(morsel, key, _unquote(value), value): | ||
| cookies.append((key, morsel)) | ||
|
|
||
| # Move to next cookie or end | ||
| i = next_semi + 1 if next_semi != -1 else n | ||
|
|
@@ -227,13 +264,8 @@ def parse_cookie_header(header: str) -> list[tuple[str, Morsel[str]]]: | |
| # Create new 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 | ||
| # setting protected attributes directly and unlikely to change since it would | ||
| # break pickling. | ||
| morsel.__setstate__( # type: ignore[attr-defined] | ||
| {"key": key, "value": _unquote(value), "coded_value": value} | ||
| ) | ||
| if not _safe_set_morsel_state(morsel, key, _unquote(value), value): | ||
| continue | ||
|
|
||
| cookies.append((key, morsel)) | ||
|
|
||
|
|
@@ -323,15 +355,13 @@ def parse_set_cookie_headers(headers: Sequence[str]) -> list[tuple[str, Morsel[s | |
| # Create new morsel | ||
| current_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 | ||
| # setting protected attributes directly and unlikely to change since it would | ||
| # break pickling. | ||
| current_morsel.__setstate__( # type: ignore[attr-defined] | ||
| {"key": key, "value": _unquote(value), "coded_value": value} | ||
| ) | ||
| parsed_cookies.append((key, current_morsel)) | ||
| morsel_seen = True | ||
| if _safe_set_morsel_state( | ||
| current_morsel, key, _unquote(value), value | ||
| ): | ||
| parsed_cookies.append((key, current_morsel)) | ||
| morsel_seen = True | ||
| else: | ||
| current_morsel = None | ||
| else: | ||
| # Invalid cookie string - no value for non-attribute | ||
| break | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,12 +3,14 @@ | |||||||
| import logging | ||||||||
| import sys | ||||||||
| import time | ||||||||
| import typing | ||||||||
| from http.cookies import ( | ||||||||
| CookieError, | ||||||||
| Morsel, | ||||||||
| SimpleCookie, | ||||||||
| _unquote as simplecookie_unquote, | ||||||||
| ) | ||||||||
| from unittest.mock import patch | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of calling the low-level stdlib interface, a natively integrated way would be using
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've never really understood the point of the mocker fixture... |
||||||||
|
|
||||||||
| import pytest | ||||||||
|
|
||||||||
|
|
@@ -1134,6 +1136,57 @@ def test_parse_set_cookie_headers_uses_unquote_with_octal( | |||||||
| assert morsel.coded_value == expected_coded | ||||||||
|
|
||||||||
|
|
||||||||
| @pytest.mark.parametrize( | ||||||||
| ("header", "expected_name", "expected_coded"), | ||||||||
| [ | ||||||||
| (r'name="\012newline\012"', "name", r'"\012newline\012"'), | ||||||||
| (r'tab="\011separated\011values"', "tab", r'"\011separated\011values"'), | ||||||||
| ], | ||||||||
| ) | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add IDs here as a separate arg or right in the params? |
||||||||
| def test_parse_set_cookie_headers_ctl_chars_from_octal( | ||||||||
| header: str, expected_name: str, expected_coded: str | ||||||||
| ) -> None: | ||||||||
| """Ensure octal escapes that decode to control characters don't crash the parser. | ||||||||
|
|
||||||||
| CPython builds with the CVE-2026-3644 patch reject control characters in | ||||||||
| cookies. When octal unquoting produces a control character, the parser | ||||||||
| should fall back to the raw coded_value instead of raising CookieError. | ||||||||
| """ | ||||||||
| result = parse_set_cookie_headers([header]) | ||||||||
|
|
||||||||
| assert len(result) == 1 | ||||||||
| name, morsel = result[0] | ||||||||
|
|
||||||||
| assert name == expected_name | ||||||||
| assert morsel.coded_value == expected_coded | ||||||||
| # Depending on CPython build, morsel.value will either be the decoded string | ||||||||
| # (pre CVE-2026-3644 patch) or the raw coded_value (post patch). | ||||||||
| # We just ensure it doesn't crash and the coded_value is preserved. | ||||||||
|
|
||||||||
|
|
||||||||
| def test_parse_set_cookie_headers_literal_ctl_chars() -> None: | ||||||||
| r"""Ensure literal control characters in a cookie value don't crash the parser. | ||||||||
|
|
||||||||
| If the raw header itself contains a control character (e.g. BEL \\x07), | ||||||||
| both the decoded value and coded_value are unsalvageable. The parser | ||||||||
| should gracefully skip the cookie instead of raising CookieError. | ||||||||
| """ | ||||||||
| result = parse_set_cookie_headers(['name="a\x07b"']) | ||||||||
| # On CPython with CVE-2026-3644 patch the cookie is skipped; | ||||||||
| # on older builds it may be accepted. Either way, no crash. | ||||||||
| if result: | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's turn this into a parametrized test with one of the params having a skip mark based on the know CPython range so the runtime is known explicitly.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — turned this into a parametrized test with two cases (bel-in-value and bel-with-attribute). |
||||||||
| assert result[0][0] == "name" | ||||||||
|
|
||||||||
|
|
||||||||
| def test_parse_set_cookie_headers_literal_ctl_chars_preserves_others() -> None: | ||||||||
| """Ensure a cookie with literal control chars doesn't break subsequent cookies.""" | ||||||||
| result = parse_set_cookie_headers(['bad="a\x07b"; good=value', "another=cookie"]) | ||||||||
| # "good" is an attribute of "bad" (same header), so it's not a separate cookie. | ||||||||
| # "another" is in a separate header and must always be preserved. | ||||||||
| names = [name for name, _ in result] | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this work with a generator expression?
Suggested change
|
||||||||
| assert "another" in names | ||||||||
|
|
||||||||
|
|
||||||||
| # Tests for parse_cookie_header (RFC 6265 compliant Cookie header parser) | ||||||||
|
|
||||||||
|
|
||||||||
|
|
@@ -1597,8 +1650,18 @@ def test_parse_cookie_header_empty_key_in_fallback( | |||||||
| assert name2 == "another" | ||||||||
| assert morsel2.value == "test" | ||||||||
|
|
||||||||
| assert "Cannot load cookie. Illegal cookie name" in caplog.text | ||||||||
| assert "''" in caplog.text | ||||||||
|
|
||||||||
| def test_parse_cookie_header_literal_ctl_chars() -> None: | ||||||||
| r"""Ensure literal control characters in a cookie value don't crash the parser. | ||||||||
|
|
||||||||
| If the raw header itself contains a control character (e.g. BEL \\x07), | ||||||||
| the cookie is unsalvageable. The parser should gracefully skip it. | ||||||||
| """ | ||||||||
| result = parse_cookie_header('name="a\x07b"; good=cookie') | ||||||||
| # On CPython with CVE-2026-3644 patch the bad cookie is skipped; | ||||||||
| # on older builds it may be accepted. Either way, no crash. | ||||||||
| names = [name for name, _ in result] | ||||||||
| assert "good" in names | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Structures like this tend to read better semantically, plus there's no throw-away list creation and the iteration stops early:
Suggested change
|
||||||||
|
|
||||||||
|
|
||||||||
| @pytest.mark.parametrize( | ||||||||
|
|
@@ -1789,3 +1852,37 @@ def test_unquote_compatibility_with_simplecookie(test_value: str) -> None: | |||||||
| f"our={_unquote(test_value)!r}, " | ||||||||
| f"SimpleCookie={simplecookie_unquote(test_value)!r}" | ||||||||
| ) | ||||||||
|
|
||||||||
|
|
||||||||
| @pytest.fixture | ||||||||
| def mock_strict_morsel() -> typing.Iterator[None]: | ||||||||
| original_setstate = Morsel.__setstate__ # type: ignore[attr-defined] | ||||||||
|
|
||||||||
| def _mock_setstate(self: Morsel[str], state: dict[str, str]) -> None: | ||||||||
| if any(ord(c) < 32 for c in state.get("value", "")): | ||||||||
| raise CookieError() | ||||||||
| original_setstate(self, state) | ||||||||
|
|
||||||||
| with patch( | ||||||||
| "aiohttp._cookie_helpers.Morsel.__setstate__", | ||||||||
| autospec=True, | ||||||||
| side_effect=_mock_setstate, | ||||||||
| ): | ||||||||
| yield | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you migrate to the builtin |
||||||||
|
|
||||||||
|
|
||||||||
| def test_cookie_helpers_cve_fallback(mock_strict_morsel: None) -> None: | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a fixture does not return a usable object, there's no need to inject it as a test function argument only to discard an unused local var. Instead, wire it through a mark as follows:
Suggested change
|
||||||||
| m: Morsel[str] = Morsel() | ||||||||
| assert helpers._safe_set_morsel_state(m, "k", "v\n", "v\\012") is True | ||||||||
| assert m.value == "v\\012" | ||||||||
|
|
||||||||
| assert helpers._safe_set_morsel_state(Morsel(), "k", "v\n", "v\n") is False | ||||||||
|
|
||||||||
| cookie: Morsel[str] = Morsel() | ||||||||
| cookie._key, cookie._value, cookie._coded_value = "k", "v\n", "v\n" # type: ignore[attr-defined] | ||||||||
| assert preserve_morsel_with_coded_value(cookie) is cookie | ||||||||
|
|
||||||||
| assert parse_cookie_header("f=b\x07r;") == [] | ||||||||
| assert parse_cookie_header("f=b\x07r") == [] | ||||||||
| assert parse_cookie_header('f="b\x07r";') == [] | ||||||||
| assert parse_set_cookie_headers(['f="b\x07r";']) == [] | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work?