diff --git a/CHANGES/13855.bugfix.rst b/CHANGES/13855.bugfix.rst new file mode 100644 index 00000000000..bd894fc960e --- /dev/null +++ b/CHANGES/13855.bugfix.rst @@ -0,0 +1,5 @@ +Resolved a redirect ``Location`` with the scheme of the current URL but +without ``//``, such as ``http:/path`` or ``http:path``, against the +current URL, as browsers do, instead of treating it as an absolute URL +without a host +-- by :user:`asvetlov`. diff --git a/CHANGES/13858.bugfix.rst b/CHANGES/13858.bugfix.rst new file mode 100644 index 00000000000..9bd2d110184 --- /dev/null +++ b/CHANGES/13858.bugfix.rst @@ -0,0 +1,7 @@ +Rejected absolute-form request targets without ``//`` or with an empty +host, such as ``http:/example.com/`` or ``http:///example.com/``, before +parsing them with yarl, which reads a host from them in its WHATWG mode; +RFC 9110 requires a host for ``http`` and ``https``. The invalid URL test +data no longer uses ``http:///example.com``, which such a yarl version +parses as ``http://example.com/``, as browsers do +-- by :user:`asvetlov`. diff --git a/CHANGES/13861.contrib.rst b/CHANGES/13861.contrib.rst new file mode 100644 index 00000000000..8ce6e9a811a --- /dev/null +++ b/CHANGES/13861.contrib.rst @@ -0,0 +1,4 @@ +Changed the long host in the ``Host`` header tests to one that is not made +only of digits, since yarl now parses such a host as an IP address in its +default mode and rejects this one as out of range +-- by :user:`asvetlov`. diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md index 86ea1bb3dab..898dfe27520 100644 --- a/THREAT_MODEL.md +++ b/THREAT_MODEL.md @@ -268,7 +268,7 @@ into `StreamReader`) is then handed to `web_protocol.RequestHandler` and | 1.9 | Chunk-size DoS | The parser doesn't cap chunk size, but **server-side body length is bounded by `client_max_size` (default `1 MiB`)** in `web_request.py:BaseRequest.read`. Client-side responses are bounded by user-supplied `max_body_size` / streaming reads. | None. If a cap is ever needed at the parser level, plumb it through `HttpPayloadParser`. | | 1.10 | Chunk-extension DoS | Chunk-extension content is bounded by the same wire-level size constraints (it shares the chunk-size line with `max_line_size`). | **Add an explicit test that chunk-extension flooding cannot blow past `max_line_size`.** | | 1.11 | Parser error reflection | `http_parser.py` truncates to `[:100]` only for `LineTooLong`; `BadStatusLine` / `InvalidHeader` / `TransferEncodingError` carry the offending line up to `max_line_size` / `max_field_size`. `_http_parser.pyx` bounds its snippet to 50 bytes either side of the error position, so input with no CRLF to delimit the offending line is not quoted in full (`test_c_parser_error_message_bounded_for_crlf_free_input`). | **Audit any aiohttp path where `BadHttpMessage` content is reflected to the client unsanitised.** **User**: Review custom `web_log` configurations and any middleware that reflects parser exception messages back to the peer. | -| 1.12 | Cython ⇄ pure-Python divergence | `tests/test_http_parser.py` parameterises tests over `REQUEST_PARSERS` / `RESPONSE_PARSERS` (pure-Python always; Cython when the extension imports). The high-leverage attack vectors are already covered under both backends: CL+TE (`test_content_length_transfer_encoding`), CL×N (`test_duplicate_singleton_header_rejected`), obs-fold (`test_reject_obsolete_line_folding`, `test_http_response_parser_obs_line_folding*`), CR/LF/NUL (`test_bad_headers`, `test_http_response_parser_null_byte_in_header_value`, `test_http_response_parser_bad_crlf`), version regex (`test_http_request_parser_bad_version*`, `test_http_response_parser_bad_version*`), bare-LF line endings (`test_reject_bare_lf_no_cross_request_leak`), control characters in the request target (`test_http_request_parser_ctl_in_request_target`). | None. When new attack vectors emerge, add them to the parameterised tests. | +| 1.12 | Cython ⇄ pure-Python divergence | `tests/test_http_parser.py` parameterises tests over `REQUEST_PARSERS` / `RESPONSE_PARSERS` (pure-Python always; Cython when the extension imports). The high-leverage attack vectors are already covered under both backends: CL+TE (`test_content_length_transfer_encoding`), CL×N (`test_duplicate_singleton_header_rejected`), obs-fold (`test_reject_obsolete_line_folding`, `test_http_response_parser_obs_line_folding*`), CR/LF/NUL (`test_bad_headers`, `test_http_response_parser_null_byte_in_header_value`, `test_http_response_parser_bad_crlf`), version regex (`test_http_request_parser_bad_version*`, `test_http_response_parser_bad_version*`), bare-LF line endings (`test_reject_bare_lf_no_cross_request_leak`), control characters in the request target (`test_http_request_parser_ctl_in_request_target`), absolute-form targets without `//` or with an empty host (`test_url_absolute_form_empty_host_rejected`). | None. When new attack vectors emerge, add them to the parameterised tests. | | 1.13 | llhttp version drift | Manual upgrade via `make generate-llhttp`; vendor pinned in `vendor/llhttp/package.json`. | Track upstream releases (e.g. via Dependabot rule for `vendor/llhttp/package.json`), bump on every llhttp release, regenerate in CI. | | 1.14 | npm-side compromise of `llhttp` | The vendored output is checked into git, so a compromise during a future regen would be detectable in PR review. See [§5.19](#519-build--release-supply-chain). | **Make the llhttp build reproducible: pin Node.js version, commit the npm lockfile, and on every bump verify the regenerated C against upstream's release tarballs before committing.** | diff --git a/aiohttp/_http_parser.pyx b/aiohttp/_http_parser.pyx index b656aa741ed..5ee5a905b91 100644 --- a/aiohttp/_http_parser.pyx +++ b/aiohttp/_http_parser.pyx @@ -31,7 +31,7 @@ from .http_exceptions import ( PayloadEncodingError, TransferEncodingError, ) -from .http_parser import DeflateBuffer as _DeflateBuffer +from .http_parser import DeflateBuffer as _DeflateBuffer, _has_authority from .http_writer import ( HttpVersion as _HttpVersion, HttpVersion10 as _HttpVersion10, @@ -793,11 +793,13 @@ cdef class HttpRequestParser(HttpParser): else: # absolute-form for proxy maybe, # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.2 - try: - self._url = URL(self._path, encoded=True) - host = self._url.raw_host - except ValueError: - host = None + host = None + if _has_authority(self._path): + try: + self._url = URL(self._path, encoded=True) + host = self._url.raw_host + except ValueError: + pass # https://www.rfc-editor.org/rfc/rfc9110#section-4.2.1-4 if host is None: raise InvalidURLError( diff --git a/aiohttp/client.py b/aiohttp/client.py index 9e2112791c1..0bcffd81c5e 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -168,6 +168,22 @@ from typing import Unpack +# URL parsers strip leading and trailing C0 control characters and spaces and +# drop tabs and newlines before splitting a URL. +_C0_CONTROL_OR_SPACE = "".join(map(chr, range(0x21))) +_REMOVE_TAB_OR_NEWLINE = str.maketrans("", "", "\t\n\r") + + +def _has_no_authority(location: str, scheme: str) -> bool: + """Tell if a URL with a scheme has no "//" after the scheme. + + Browsers read a backslash like a slash there for http and https. + """ + location = location.strip(_C0_CONTROL_OR_SPACE).translate(_REMOVE_TAB_OR_NEWLINE) + rest = location[len(scheme) + 1 : len(scheme) + 3] + return rest[:1] not in ("/", "\\") or rest[1:] not in ("/", "\\") + + class _RequestOptions(TypedDict, total=False): params: Query data: Any @@ -834,7 +850,11 @@ async def _request( await req._body.close() resp.close() raise NonHttpUrlRedirectClientError(r_url) - elif not scheme: + elif not scheme or ( + scheme == url.scheme and _has_no_authority(r_url, scheme) + ): + # "http:/path" or "http:path" is a reference to + # the current URL, as browsers resolve it. parsed_redirect_url = url.join(parsed_redirect_url) try: diff --git a/aiohttp/http_parser.py b/aiohttp/http_parser.py index 7e2376ac1a1..878cbf76f73 100644 --- a/aiohttp/http_parser.py +++ b/aiohttp/http_parser.py @@ -91,6 +91,18 @@ DIGITS: Final[Pattern[str]] = re.compile(r"\d+", re.ASCII) HEXDIGITS: Final[Pattern[bytes]] = re.compile(rb"[0-9a-fA-F]+") + +def _has_authority(target: str) -> bool: + """Tell if an absolute-form request target has "//" and a non-empty authority. + + RFC 9110 section 4.2 requires a host for http and https. yarl's default + WHATWG mode reads a host from "http:host/p" or "http:///host/p", so the + target is checked before it is parsed. + """ + rest = target.partition(":")[2] + return rest[:2] == "//" and rest[2:3] not in ("", "/", "?", "#") + + # RFC 9110 singleton headers — duplicates are rejected in strict mode. # In lax mode (response parser default), the check is skipped entirely # since real-world servers (e.g. Google APIs, Werkzeug) commonly send @@ -719,6 +731,10 @@ def parse_message(self, lines: list[bytes]) -> RawRequestMessage: else: # absolute-form for proxy maybe, # https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.2 + if not _has_authority(path): + raise InvalidURLError( + path.encode(errors="surrogateescape").decode("latin1") + ) try: url = URL(path, encoded=True) host = url.raw_host diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index fedb42d2390..705a10f2534 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -1006,6 +1006,38 @@ async def handler_ok(request: web.Request) -> web.Response: assert resp.url.path == "/ok" +@pytest.mark.parametrize( + ("location", "path", "query"), + ( + ("http:/ok", "/ok", {}), + ("http:ok", "/ok", {}), + ("http:/ok?a=b", "/ok", {"a": "b"}), + ("http:/", "/", {}), + ), +) +async def test_redirect_same_scheme_without_authority( + aiohttp_client: AiohttpClient, location: str, path: str, query: dict[str, str] +) -> None: + async def handler_redirect(request: web.Request) -> web.Response: + return web.Response(status=301, headers={"Location": location}) + + async def handler_ok(request: web.Request) -> web.Response: + assert dict(request.query) == query + return web.Response(status=200) + + app = web.Application() + app.router.add_route("GET", path, handler_ok) + app.router.add_route("GET", "/redirect", handler_redirect) + client = await aiohttp_client(app) + + async with client.get("/redirect") as resp: + assert resp.status == 200 + assert resp.url.host == "127.0.0.1" + assert resp.url.path == path + assert dict(resp.url.query) == query + assert len(resp.history) == 1 + + async def test_history(aiohttp_client: AiohttpClient) -> None: async def handler_redirect(request: web.Request) -> web.Response: return web.Response(status=301, headers={"Location": "/ok"}) @@ -3210,12 +3242,9 @@ async def handler_redirect(request: web.Request) -> web.Response: ("http://example.org:non_int_port/", "http://example.org:non_int_port/"), ) -INVALID_URL_WITH_ERROR_MESSAGE_YARL_ORIGIN = ( - # # yarl.URL.origin raises ValueError - ("http:/", "http:///"), - ("http:/example.com", "http:///example.com"), - ("http:///example.com", "http:///example.com"), -) +# yarl.URL.origin raises ValueError. A redirect to "http:/" resolves against +# the current URL instead, see test_redirect_same_scheme_without_authority(). +INVALID_URL_WITH_ERROR_MESSAGE_YARL_ORIGIN = (("http:/", "http:///"),) NON_HTTP_URL_WITH_ERROR_MESSAGE = ( ("call:+380123456789", r"call:\+380123456789"), @@ -3259,8 +3288,7 @@ async def test_invalid_and_non_http_url( ( *( (url, message, InvalidUrlRedirectClientError) - for (url, message) in INVALID_URL_WITH_ERROR_MESSAGE_YARL_ORIGIN - + INVALID_URL_WITH_ERROR_MESSAGE_YARL_NEW + for (url, message) in INVALID_URL_WITH_ERROR_MESSAGE_YARL_NEW ), *( (url, message, NonHttpUrlRedirectClientError) @@ -3294,8 +3322,7 @@ async def generate_redirecting_response(request: web.Request) -> web.Response: ( *( (url, message, InvalidUrlRedirectClientError) - for (url, message) in INVALID_URL_WITH_ERROR_MESSAGE_YARL_ORIGIN - + INVALID_URL_WITH_ERROR_MESSAGE_YARL_NEW + for (url, message) in INVALID_URL_WITH_ERROR_MESSAGE_YARL_NEW ), *( (url, message, NonHttpUrlRedirectClientError) @@ -5558,8 +5585,8 @@ async def test_invalid_redirect_origin_closes_payload( async def redirect_handler(request: web.Request) -> web.Response: # Read the payload to simulate server processing await request.read() - # Return a URL that will fail origin() check - using a relative URL without host - return web.Response(status=307, headers={hdrs.LOCATION: "http:///path"}) + # Return a URL that will fail origin() check - using a URL without host + return web.Response(status=307, headers={hdrs.LOCATION: "http://"}) app = web.Application() app.router.add_post("/redirect", redirect_handler) @@ -5632,7 +5659,7 @@ async def stall(reader: asyncio.StreamReader, writer: asyncio.StreamWriter) -> N async def test_request_error_before_body_created_does_not_mask() -> None: async with aiohttp.ClientSession() as session: with pytest.raises(InvalidUrlClientError): - await session.get("http:///path") + await session.get("http://") async def test_amazon_like_cookie_scenario(aiohttp_client: AiohttpClient) -> None: diff --git a/tests/test_client_request.py b/tests/test_client_request.py index 224b7fd570c..e1cd4629e82 100644 --- a/tests/test_client_request.py +++ b/tests/test_client_request.py @@ -631,29 +631,19 @@ async def test_params_empty_path_and_url(make_client_request: _RequestMaker) -> assert str(req_none.url) == "http://python.org" +# A long host that is not all digits: WHATWG parses a host made only of +# numbers, like this one without ".test", as an IPv4 address. +LONG_HOST = "12345678901234567890123456789012345678901234567890.test" + + async def test_gen_netloc_all(make_client_request: _RequestMaker) -> None: - req = make_client_request( - "get", - URL( - "https://aiohttp:pwpwpw@12345678901234567890123456789012345678901234567890:8080" - ), - ) - assert ( - req.headers["HOST"] - == "12345678901234567890123456789" + "012345678901234567890:8080" - ) + req = make_client_request("get", URL(f"https://aiohttp:pwpwpw@{LONG_HOST}:8080")) + assert req.headers["HOST"] == f"{LONG_HOST}:8080" async def test_gen_netloc_no_port(make_client_request: _RequestMaker) -> None: - req = make_client_request( - "get", - URL( - "https://aiohttp:pwpwpw@12345678901234567890123456789012345678901234567890/" - ), - ) - assert ( - req.headers["HOST"] == "12345678901234567890123456789" + "012345678901234567890" - ) + req = make_client_request("get", URL(f"https://aiohttp:pwpwpw@{LONG_HOST}/")) + assert req.headers["HOST"] == LONG_HOST async def test_cookie_coded_value_preserved(make_client_request: _RequestMaker) -> None: diff --git a/tests/test_client_session.py b/tests/test_client_session.py index 3851416320d..e37373472e3 100644 --- a/tests/test_client_session.py +++ b/tests/test_client_session.py @@ -1729,3 +1729,25 @@ async def test_netrc_auth_host_not_in_netrc(auth_server: TestServer) -> None: text = await resp.text() # Should not have auth since the host is not in netrc assert text == "no_auth" + + +@pytest.mark.parametrize( + ("location", "expected"), + ( + ("http:/ok", True), + ("http:ok", True), + ("http:", True), + ("http:/", True), + ("http://example.com/", False), + ("http:///example.com", False), + (" http:///example.com", False), + ("\x00http:///example.com", False), + ("http:/\t/example.com", False), + ("http:/\n/example.com", False), + ("http:\\\\example.com", False), + ("http:\\/example.com", False), + ("http:/\\example.com", False), + ), +) +def test_has_no_authority(location: str, expected: bool) -> None: + assert client._has_no_authority(location, "http") is expected diff --git a/tests/test_http_parser.py b/tests/test_http_parser.py index 90ac8f3e1ed..11b7df69933 100644 --- a/tests/test_http_parser.py +++ b/tests/test_http_parser.py @@ -1188,8 +1188,23 @@ def test_url_authority_form_only_connect(parser: HttpRequestParser) -> None: b"https:////protected", b"https://:80/protected", b"https://user@/protected", + b"https://", + b"https://?q", + b"http:protected/x", + b"http:/protected/x", + b"http:\\\\protected/x", + ), + ids=( + "empty-host", + "empty-host-extra-slash", + "port-only", + "userinfo-only", + "no-authority", + "empty-host-query", + "no-slashes", + "one-slash", + "backslashes", ), - ids=("empty-host", "empty-host-extra-slash", "port-only", "userinfo-only"), ) def test_url_absolute_form_empty_host_rejected( parser: HttpRequestParser, target: bytes