diff --git a/CHANGES/12931.bugfix.rst b/CHANGES/12931.bugfix.rst new file mode 100644 index 00000000000..6b62e87dac7 --- /dev/null +++ b/CHANGES/12931.bugfix.rst @@ -0,0 +1 @@ +Fixed some inconsistent case sensitivity on request methods -- by :user:`Dreamsorcerer`. diff --git a/CHANGES/12976.bugfix.rst b/CHANGES/12976.bugfix.rst new file mode 100644 index 00000000000..008095a3351 --- /dev/null +++ b/CHANGES/12976.bugfix.rst @@ -0,0 +1 @@ +Fixed the client decompressing frames when ``permessage-deflate`` was not negotiated -- by :user:`Dreamsorcerer`. diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md index 2834202427f..4b5af95cbd4 100644 --- a/THREAT_MODEL.md +++ b/THREAT_MODEL.md @@ -527,7 +527,7 @@ client-side, the writer adds masks to outgoing frames. | :--- | :--- | :--- | :--- | | 3.1 | Unmasked client frames accepted | None — the reader is direction-agnostic; `web_ws.py` does not enforce client-mask either. | **Recommended hardening: Enforce RFC 6455 §5.1 mask direction in strict mode only (gated on `DEBUG`, mirroring the HTTP parser's lenient-default / strict-DEBUG asymmetry): server reader rejects frames with `has_mask == 0`, client reader rejects masked server frames, both with a `PROTOCOL_ERROR`-style close. Production default stays lenient for interop.** | | 3.2 | Non-cryptographic mask RNG | `partial(random.getrandbits, 32)` per writer instance. | Documented design decision: WebSocket masking exists for cache-poisoning resistance against intermediaries, not as a confidentiality primitive. The mask needs to be performant — called once per outbound frame on a hot path — and does not need to be cryptographically unpredictable. `random.getrandbits(32)` is the deliberate choice. | -| 3.3 | RSV bits | `reader_py.py:WebSocketReader._feed_data` ties RSV1 acceptance to the PMCE-negotiated `_compress` flag; RSV2/3 always rejected. | None. | +| 3.3 | RSV bits | `reader_py.py:WebSocketReader._feed_data` gates RSV1 on the PMCE-negotiated `_compress` flag; RSV2/3 always rejected. | None. | | 3.4 | Unknown opcode | Rejected. | None. | | 3.5–3.7 | Control-frame and fragmentation rules | All enforced at reader. | None. | | 3.8 | Fragment memory bound | `max_msg_size` enforced pre-FIN and at assembly. Default 4 MiB. | **User**: set a smaller `max_msg_size` for protocols where messages are bounded (e.g. chat); the 4 MiB default suits arbitrary payloads. | @@ -546,6 +546,12 @@ client-side, the writer adds masks to outgoing frames. `max_msg_size + 1` and rejects with `MESSAGE_TOO_BIG` (1009) on overflow. This is the primary mitigation for zip-bomb-style attacks against WebSocket peers. +- **PR #12976** — the client created its `WebSocketReader` + without passing `compress`, so the reader defaulted to `compress=True` and + decompressed RSV1 frames even when PMCE was never negotiated (threat 3.3; + RFC 6455 §5.2 requires failing such frames). Fixed by passing + `compress=bool(compress)` in `client.py:_ws_connect` and removing the + `compress` / `decode_text` defaults on `WebSocketReader.__init__`. - No formal CVE has been published against the WebSocket framing layer to date. diff --git a/aiohttp/_websocket/reader_py.py b/aiohttp/_websocket/reader_py.py index 1e6f8bd5295..82b5b65af60 100644 --- a/aiohttp/_websocket/reader_py.py +++ b/aiohttp/_websocket/reader_py.py @@ -143,8 +143,8 @@ def __init__( self, queue: WebSocketDataQueue, max_msg_size: int, - compress: bool = True, - decode_text: bool = True, + compress: bool, + decode_text: bool, ) -> None: self.queue = queue self._max_msg_size = max_msg_size diff --git a/aiohttp/client.py b/aiohttp/client.py index 79b3bb044cd..809fa63ed24 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -490,6 +490,8 @@ async def _request( if self.closed: raise RuntimeError("Session is closed") + method = method.upper() + if not isinstance(ssl, SSL_ALLOWED_TYPES): raise TypeError( "ssl should be SSLContext, Fingerprint, or bool, " @@ -1182,7 +1184,12 @@ async def _ws_connect( compress=compress, client_notakeover=notakeover, ) - parser = WebSocketReader(reader, max_msg_size, decode_text=decode_text) + parser = WebSocketReader( + reader, + max_msg_size, + compress=bool(compress), + decode_text=decode_text, + ) cb = None if heartbeat is None else ws_resp._on_data_received conn_proto.set_parser(parser, reader, data_received_cb=cb) return ws_resp diff --git a/aiohttp/hdrs.py b/aiohttp/hdrs.py index b64b62ee7f2..1082da3f4cc 100644 --- a/aiohttp/hdrs.py +++ b/aiohttp/hdrs.py @@ -108,14 +108,8 @@ X_FORWARDED_HOST: Final[istr] = istr("X-Forwarded-Host") X_FORWARDED_PROTO: Final[istr] = istr("X-Forwarded-Proto") -# These are the upper/lower case variants of the headers/methods -# Example: {'hOst', 'host', 'HoST', 'HOSt', 'hOsT', 'HosT', 'hoSt', ...} -METH_HEAD_ALL: Final = frozenset( - map("".join, itertools.product(*zip(METH_HEAD.upper(), METH_HEAD.lower()))) -) -METH_CONNECT_ALL: Final = frozenset( - map("".join, itertools.product(*zip(METH_CONNECT.upper(), METH_CONNECT.lower()))) -) +# Case permutations of the Host header — for callers that match against +# raw header tokens before istr/CIMultiDict folding. HOST_ALL: Final = frozenset( map("".join, itertools.product(*zip(HOST.upper(), HOST.lower()))) ) diff --git a/aiohttp/helpers.py b/aiohttp/helpers.py index 2fa8a19b6a7..1a45a060ff1 100644 --- a/aiohttp/helpers.py +++ b/aiohttp/helpers.py @@ -112,7 +112,7 @@ EMPTY_BODY_STATUS_CODES = frozenset((204, 304, *range(100, 200))) # https://datatracker.ietf.org/doc/html/rfc9112#section-6.3-2.1 # https://datatracker.ietf.org/doc/html/rfc9112#section-6.3-2.2 -EMPTY_BODY_METHODS = hdrs.METH_HEAD_ALL +EMPTY_BODY_METHODS = frozenset({hdrs.METH_HEAD}) DEBUG = sys.flags.dev_mode or ( not sys.flags.ignore_environment and bool(os.environ.get("PYTHONASYNCIODEBUG")) @@ -1164,7 +1164,7 @@ def must_be_empty_body(method: str, code: int) -> bool: return ( code in EMPTY_BODY_STATUS_CODES or method in EMPTY_BODY_METHODS - or (200 <= code < 300 and method in hdrs.METH_CONNECT_ALL) + or (200 <= code < 300 and method == hdrs.METH_CONNECT) ) @@ -1176,5 +1176,5 @@ def should_remove_content_length(method: str, code: int) -> bool: # https://www.rfc-editor.org/rfc/rfc9110.html#section-8.6-8 # https://www.rfc-editor.org/rfc/rfc9110.html#section-15.4.5-4 return code in EMPTY_BODY_STATUS_CODES or ( - 200 <= code < 300 and method in hdrs.METH_CONNECT_ALL + 200 <= code < 300 and method == hdrs.METH_CONNECT ) diff --git a/aiohttp/http_parser.py b/aiohttp/http_parser.py index 8d48b877c0b..6abbe04feae 100644 --- a/aiohttp/http_parser.py +++ b/aiohttp/http_parser.py @@ -644,6 +644,7 @@ def parse_message(self, lines: list[bytes]) -> RawRequestMessage: # method if not TOKENRE.fullmatch(method): raise BadHttpMethod(method) + method = method.upper() # version match = VERSRE.fullmatch(version) diff --git a/aiohttp/test_utils.py b/aiohttp/test_utils.py index 8cef43de753..a550b369288 100644 --- a/aiohttp/test_utils.py +++ b/aiohttp/test_utils.py @@ -611,7 +611,7 @@ def make_mocked_request( ) message = RawRequestMessage( - method, + method.upper(), path, version, headers, diff --git a/aiohttp/web_response.py b/aiohttp/web_response.py index 441d7f02900..c9fa02f2524 100644 --- a/aiohttp/web_response.py +++ b/aiohttp/web_response.py @@ -708,7 +708,7 @@ async def _start(self, request: "BaseRequest") -> AbstractStreamWriter: body_len = len(self._body) if self._body else "0" # https://www.rfc-editor.org/rfc/rfc9110.html#section-8.6-7 if body_len != "0" or ( - self.status != 304 and request.method not in hdrs.METH_HEAD_ALL + self.status != 304 and request.method != hdrs.METH_HEAD ): self._headers[hdrs.CONTENT_LENGTH] = str(body_len) diff --git a/tests/autobahn/test_autobahn.py b/tests/autobahn/test_autobahn.py index 1a0015e6755..1920b8f2c95 100644 --- a/tests/autobahn/test_autobahn.py +++ b/tests/autobahn/test_autobahn.py @@ -102,7 +102,6 @@ def test_client(report_dir: Path, request: pytest.FixtureRequest) -> None: results = get_test_results(report_dir / "clients", "aiohttp") xfail = { - "3.4": "Actual events match at least one expected.", "7.9.5": "The close code should have been 1002 or empty", "9.1.4": "Did not receive message within 100 seconds.", "9.1.5": "Did not receive message within 100 seconds.", diff --git a/tests/test_benchmarks_http_websocket.py b/tests/test_benchmarks_http_websocket.py index 762d365158e..1ccbca316cb 100644 --- a/tests/test_benchmarks_http_websocket.py +++ b/tests/test_benchmarks_http_websocket.py @@ -25,7 +25,9 @@ def test_read_large_binary_websocket_messages( queue = WebSocketDataQueue( BaseProtocol(event_loop), DEFAULT_CHUNK_SIZE, loop=event_loop ) - reader = WebSocketReader(queue, max_msg_size=DEFAULT_CHUNK_SIZE) + reader = WebSocketReader( + queue, max_msg_size=DEFAULT_CHUNK_SIZE, compress=True, decode_text=True + ) # PACK3 has a minimum message length of 2**16 bytes. message = b"x" * ((2**16) + 1) @@ -48,7 +50,9 @@ def test_read_one_hundred_websocket_text_messages( queue = WebSocketDataQueue( BaseProtocol(event_loop), DEFAULT_CHUNK_SIZE, loop=event_loop ) - reader = WebSocketReader(queue, max_msg_size=DEFAULT_CHUNK_SIZE) + reader = WebSocketReader( + queue, max_msg_size=DEFAULT_CHUNK_SIZE, compress=True, decode_text=True + ) raw_message = ( b'\x81~\x01!{"id":1,"src":"shellyplugus-c049ef8c30e4","dst":"aios-1453812500' b'8","result":{"name":null,"id":"shellyplugus-c049ef8c30e4","mac":"C049EF8C30E' diff --git a/tests/test_client_ws_functional.py b/tests/test_client_ws_functional.py index 411f310d5ed..ef69bdaffb6 100644 --- a/tests/test_client_ws_functional.py +++ b/tests/test_client_ws_functional.py @@ -2,6 +2,7 @@ import json import struct import sys +import zlib from contextlib import suppress from typing import Literal, NoReturn from unittest import mock @@ -18,10 +19,10 @@ hdrs, web, ) -from aiohttp._websocket.models import WSMessageBinary +from aiohttp._websocket.models import WS_DEFLATE_TRAILING, WSMessageBinary from aiohttp._websocket.reader import WebSocketDataQueue from aiohttp.client_ws import ClientWSTimeout -from aiohttp.http import WSCloseCode +from aiohttp.http import WebSocketError, WSCloseCode if sys.version_info >= (3, 11): import asyncio as async_timeout @@ -1618,3 +1619,51 @@ async def handler(request: web.Request) -> web.WebSocketResponse: # receive_json() with orjson-style loads should work with bytes data = await ws.receive_json(loads=orjson_style_loads) assert data == {"value": 42} + + +async def test_client_rejects_compressed_frame_without_negotiation( + aiohttp_client: AiohttpClient, +) -> None: + """A client that never negotiated permessage-deflate must reject RSV1 frames. + + Per RFC 6455 section 5.2, a non-zero reserved bit with no negotiated + extension defining it MUST fail the connection. The client used to build its + WebSocketReader without passing ``compress``, so the reader defaulted to + ``compress=True`` and silently decompressed server frames with compression + off. + """ + payload = b"this frame should never be decompressed by the client" + + async def handler(request: web.Request) -> web.WebSocketResponse: + ws = web.WebSocketResponse() + await ws.prepare(request) + transport = request.transport + assert transport is not None + # Compress the payload the permessage-deflate way (raw DEFLATE minus the + # trailing 00 00 ff ff) and frame it with FIN + RSV1 set, even though the + # handshake never negotiated permessage-deflate. + compressor = zlib.compressobj( + zlib.Z_DEFAULT_COMPRESSION, zlib.DEFLATED, -zlib.MAX_WBITS + ) + compressed = compressor.compress(payload) + compressed += compressor.flush(zlib.Z_SYNC_FLUSH) + compressed = compressed.removesuffix(WS_DEFLATE_TRAILING) + first_byte = 0x80 | 0x40 | WSMsgType.TEXT.value # FIN + RSV1 + TEXT + frame = struct.pack("!BB", first_byte, len(compressed)) + compressed + transport.write(frame) + # Keep the connection open until the client tears it down. + await ws.receive() + return ws + + app = web.Application() + app.router.add_get("/", handler) + client = await aiohttp_client(app) + + async with client.ws_connect("/") as ws: + # Default compress=0: no permessage-deflate offered or negotiated. + assert ws.compress == 0 + msg = await ws.receive() + + assert msg.type is WSMsgType.ERROR, msg + assert isinstance(msg.data, WebSocketError) + assert msg.data.code == WSCloseCode.PROTOCOL_ERROR diff --git a/tests/test_helpers.py b/tests/test_helpers.py index a499746607a..4d6b9e34e1d 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -1163,6 +1163,8 @@ def test_method_must_be_empty_body() -> None: assert "HEAD" in EMPTY_BODY_METHODS # CONNECT is only empty on a successful response assert "CONNECT" not in EMPTY_BODY_METHODS + # Callers are expected to pass already-normalised (uppercase) methods. + assert "head" not in EMPTY_BODY_METHODS def test_should_remove_content_length_is_subset_of_must_be_empty_body() -> None: diff --git a/tests/test_http_parser.py b/tests/test_http_parser.py index dd28b194650..e61af88cc16 100644 --- a/tests/test_http_parser.py +++ b/tests/test_http_parser.py @@ -630,11 +630,34 @@ def test_parse_unusual_request_line(parser: HttpRequestParser) -> None: msg, _ = messages[0] assert msg.compression is None assert not msg.upgrade - assert msg.method == "#smol" + assert msg.method == "#SMOL" assert msg.path == "//a" assert msg.version == (1, 3) +def test_py_parser_normalises_method_to_uppercase( + event_loop: asyncio.AbstractEventLoop, server: Server[Request] +) -> None: + """Test Python parser canonicalises method tokens. + + llhttp rejects lowercase upstream, so this only applies to the Python parser. + """ + protocol = RequestHandler(server, loop=event_loop) + parser = HttpRequestParserPy( + protocol, + event_loop, + 2**16, + max_line_size=8190, + max_field_size=8190, + ) + protocol._parser = parser + text = b"get /test HTTP/1.1\r\nHost: a\r\n\r\n" + messages, _upgrade, _tail = parser.feed_data(text) + assert len(messages) == 1 + msg, _ = messages[0] + assert msg.method == "GET" + + def test_parse(parser: HttpRequestParser) -> None: text = b"GET /test HTTP/1.1\r\nHost: a\r\n\r\n" messages, upgrade, tail = parser.feed_data(text) diff --git a/tests/test_test_utils.py b/tests/test_test_utils.py index ef9acf9af22..0d13b062bf0 100644 --- a/tests/test_test_utils.py +++ b/tests/test_test_utils.py @@ -333,6 +333,29 @@ async def handler(request: web.Request) -> web.Response: assert num_requests == 1 +async def test_retry_persistent_connection_lowercase_method( + aiohttp_client: AiohttpClient, +) -> None: + """Lowercase idempotent methods must still trigger retry.""" + num_requests = 0 + + async def handler(request: web.Request) -> web.Response: + nonlocal num_requests + num_requests += 1 + if num_requests == 1: + request.protocol.force_close() + return web.Response() + + app = web.Application() + app.router.add_get("/", handler) + client = await aiohttp_client(app) + client.session._retry_connection = True + async with client.request("get", "/") as resp: + assert resp.status == 200 + + assert num_requests == 2 + + async def test_server_context_manager(app: web.Application) -> None: async with TestServer(app) as server: async with aiohttp.ClientSession() as client: diff --git a/tests/test_urldispatch.py b/tests/test_urldispatch.py index 92796075f69..c0bd809f3a3 100644 --- a/tests/test_urldispatch.py +++ b/tests/test_urldispatch.py @@ -204,6 +204,17 @@ async def test_add_route_with_add_head_shortcut(router: web.UrlDispatcher) -> No assert info.route.name is None +async def test_dispatch_lowercase_method(router: web.UrlDispatcher) -> None: + """Lowercase wire methods normalise on the request and dispatch correctly.""" + handler = make_handler() + router.add_get("/", handler) + req = make_mocked_request("get", "/") + assert req.method == "GET" + info = await router.resolve(req) + assert info is not None + assert handler is info.handler + + async def test_add_with_name(router: web.UrlDispatcher) -> None: handler = make_handler() router.add_route("GET", "/handler/to/path", handler, name="name") diff --git a/tests/test_websocket_parser.py b/tests/test_websocket_parser.py index c8ae28d27e7..b703232fd71 100644 --- a/tests/test_websocket_parser.py +++ b/tests/test_websocket_parser.py @@ -136,12 +136,16 @@ def out_low_limit( def parser_low_limit( out_low_limit: WebSocketDataQueue, ) -> PatchableWebSocketReader: - return PatchableWebSocketReader(out_low_limit, 4 * 1024 * 1024) + return PatchableWebSocketReader( + out_low_limit, 4 * 1024 * 1024, compress=True, decode_text=True + ) @pytest.fixture() def parser(out: WebSocketDataQueue) -> PatchableWebSocketReader: - return PatchableWebSocketReader(out, 4 * 1024 * 1024) + return PatchableWebSocketReader( + out, 4 * 1024 * 1024, compress=True, decode_text=True + ) def test_feed_data_remembers_exception(parser: WebSocketReader) -> None: @@ -601,7 +605,9 @@ def test_parse_compress_error_frame(parser: PatchableWebSocketReader) -> None: def test_parse_no_compress_frame_single(out: WebSocketDataQueue) -> None: - parser_no_compress = PatchableWebSocketReader(out, 0, compress=False) + parser_no_compress = PatchableWebSocketReader( + out, 0, compress=False, decode_text=True + ) with pytest.raises(WebSocketError) as ctx: parser_no_compress.parse_frame(struct.pack("!BB", 0b11000001, 0b00000001)) @@ -609,7 +615,7 @@ def test_parse_no_compress_frame_single(out: WebSocketDataQueue) -> None: def test_msg_too_large(out: WebSocketDataQueue) -> None: - parser = WebSocketReader(out, 256, compress=False) + parser = WebSocketReader(out, 256, compress=False, decode_text=True) data = build_frame(b"text" * 256, WSMsgType.TEXT) with pytest.raises(WebSocketError) as ctx: parser._feed_data(data) @@ -617,7 +623,7 @@ def test_msg_too_large(out: WebSocketDataQueue) -> None: def test_msg_too_large_not_fin(out: WebSocketDataQueue) -> None: - parser = WebSocketReader(out, 256, compress=False) + parser = WebSocketReader(out, 256, compress=False, decode_text=True) data = build_frame(b"text" * 256, WSMsgType.TEXT, is_fin=False) with pytest.raises(WebSocketError) as ctx: parser._feed_data(data) @@ -626,7 +632,7 @@ def test_msg_too_large_not_fin(out: WebSocketDataQueue) -> None: @pytest.mark.usefixtures("parametrize_zlib_backend") def test_compressed_msg_too_large(out: WebSocketDataQueue) -> None: - parser = WebSocketReader(out, 256, compress=True) + parser = WebSocketReader(out, 256, compress=True, decode_text=True) data = build_frame(b"aaa" * 256, WSMsgType.TEXT, ZLibBackend=ZLibBackend) with pytest.raises(WebSocketError) as ctx: parser._feed_data(data) @@ -636,7 +642,7 @@ def test_compressed_msg_too_large(out: WebSocketDataQueue) -> None: @pytest.mark.parametrize("fin", (0x80, 0x00), ids=("fin", "non-fin")) def test_msg_too_large_at_header(out: WebSocketDataQueue, fin: int) -> None: max_msg_size = 256 - parser = WebSocketReader(out, max_msg_size, compress=False) + parser = WebSocketReader(out, max_msg_size, compress=False, decode_text=True) # Header alone: TEXT, 64-bit length, declares 1 MiB of payload. header = PACK_LEN3(fin | WSMsgType.TEXT, 127, 1024 * 1024) @@ -650,7 +656,7 @@ def test_msg_too_large_at_header(out: WebSocketDataQueue, fin: int) -> None: def test_msg_too_large_across_fragments(out: WebSocketDataQueue) -> None: # Individual fragments fit under max_msg_size but accumulate past it. max_msg_size = 256 - parser = WebSocketReader(out, max_msg_size, compress=False) + parser = WebSocketReader(out, max_msg_size, compress=False, decode_text=True) first = build_frame(b"a" * 100, WSMsgType.TEXT, is_fin=False) parser._feed_data(first) @@ -670,7 +676,7 @@ def test_msg_too_large_text_after_non_fin_text(out: WebSocketDataQueue) -> None: # Protocol-violating sequence: a fresh TEXT arrives while a fragmented # message is still open. max_msg_size = 256 - parser = WebSocketReader(out, max_msg_size, compress=False) + parser = WebSocketReader(out, max_msg_size, compress=False, decode_text=True) first = build_frame(b"a" * 200, WSMsgType.TEXT, is_fin=False) parser._feed_data(first) @@ -693,7 +699,7 @@ def test_reserved_opcode_rejected_at_header( out: WebSocketDataQueue, opcode: int ) -> None: # RFC 6455 reserves opcodes 0x3-0x7 (non-control) and 0xB-0xF (control). - parser = WebSocketReader(out, max_msg_size=256, compress=False) + parser = WebSocketReader(out, max_msg_size=256, compress=False, decode_text=True) header = PACK_LEN3(0x80 | opcode, 127, 1024 * 1024) with pytest.raises(WebSocketError, match=rf"^Unexpected opcode={opcode}$") as ctx: diff --git a/tests/test_websocket_writer.py b/tests/test_websocket_writer.py index e9d930bc54e..497160b78c9 100644 --- a/tests/test_websocket_writer.py +++ b/tests/test_websocket_writer.py @@ -162,7 +162,7 @@ async def test_send_compress_cancelled( queue = WebSocketDataQueue( mock.Mock(_reading_paused=False), DEFAULT_CHUNK_SIZE, loop=loop ) - reader = WebSocketReader(queue, 50000) + reader = WebSocketReader(queue, 50000, compress=True, decode_text=True) # Replace executor with slow one to make race condition reproducible writer._compressobj = writer._get_compressor(None) @@ -217,7 +217,7 @@ async def test_send_compress_multiple_cancelled( writer = WebSocketWriter(protocol, transport, compress=15) loop = asyncio.get_running_loop() queue = WebSocketDataQueue(mock.Mock(_reading_paused=False), 2**16, loop=loop) - reader = WebSocketReader(queue, 50000) + reader = WebSocketReader(queue, 50000, compress=True, decode_text=True) # Replace executor with slow one writer._compressobj = writer._get_compressor(None) @@ -311,7 +311,7 @@ async def test_concurrent_messages( queue = WebSocketDataQueue( mock.Mock(_reading_paused=False), DEFAULT_CHUNK_SIZE, loop=loop ) - reader = WebSocketReader(queue, 50000) + reader = WebSocketReader(queue, 50000, compress=True, decode_text=True) writers = [] payloads = [] for count in range(1, 64 + 1):