From 82ec132a8f22c7b31e1e09a6b4d60ab754e8fe46 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 5 Oct 2026 16:26:52 -0400 Subject: [PATCH] fix(httpv1): accept HTTP/1 messages with no header fields (#1041) RFC 9112 allows zero field lines, but `_read_http_header` unpacked the header's CRLF split two ways and refused a header that was only the start line, so `GET / HTTP/1.0\r\n\r\n` raised `ProtocolError` and `HTTPv1(method='GET', uri='/', http_version='1.1')` could not read back the bytes its own `make` built. `_test_start_line` mirrored the check, so the dispatcher answered `unknown HTTP version` for the same bytes. A CRLF-less header is now a start line with zero fields in both. That check was also what refused the HTTP/2 connection preface, whose header is exactly `PRI * HTTP/2.0`; both now refuse it by name (`_HTTP2_PREFACE_HEADER`), to the same `HTTP: invalid format`. --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 11 ++ .../contributing/conventions/process.rst | 2 +- pcapkit/protocols/application/httpv1.py | 48 ++++-- tests/protocols/application/test_http_unit.py | 152 ++++++++++++++++-- .../test_construction_keyword_check_unit.py | 31 ++-- 6 files changed, 192 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e880756b2d..56e313a65d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -190,6 +190,7 @@ This resolves [#548](https://github.com/JarryShaw/PyPCAPKit/issues/548), which r - **a breaking change to** 8 more bespoke registries: step 2 of [#860](https://github.com/JarryShaw/PyPCAPKit/issues/860), PR 1 of 2 (`AppType` is PR 2, [#874](https://github.com/JarryShaw/PyPCAPKit/pull/874) below), bringing `StatusCode`, `ReturnCode`, `ResponseKind`, `GroupingInformation`, `OptionType`, `FEATCode`, `Command` and `Method` onto `EnumRegistry` and applying [#775](https://github.com/JarryShaw/PyPCAPKit/issues/775)'s mint/unmint ruling to their `get()` as well as `_missing_`. The first five convert 12 unambiguous placeholder branches (`Unassigned`, `Unknown`, `opt_unknown`) to `_unregistered_member`; the three with a custom `__new__` (`StatusCode`, `ReturnCode`, `OptionType`) get an override reconstructing the attributes the base's generic helper would leave unset, and `StatusCode`/`ReturnCode`'s hand-written `get()`, still on the retired `default == -1` convention, is replaced by the base's (no caller relied on the old form). `OptionType` keeps its own `get()` for its multi-namespace dispatch, but a round-2 review found it still minted on both its int/namespace and `str` paths, and the live pcapng parse path (`PCAPNG._make_pcapng_options`) calls it with wire bytes, so parsing an undeclared option code still registered a permanent member; both paths now build an unregistered member. `FEATCode`, `Command` and `Method` mint the literal wire value as its own name rather than any placeholder; per the owner's ruling (*"get will not have sufficient information to create new ones"*: `Command` needs `feat`/`desc`/`type`/`conf` and `Method` needs `safe`/`idempotent`, which a bare wire string lacks), both `_missing_` and each class's own `get()` (a second, independent mint site) now build an unregistered member. `FEATCode`'s crawler now declares all 15 real FEAT-code values from the live IANA table instead of minting 10 of them as a side effect of evaluating `Command`'s rows at import time, the import-time-mutation shape [#861](https://github.com/JarryShaw/PyPCAPKit/pull/861) removed from `FilterType`; pinned count-agnostically so a future IANA update cannot fail a correct regeneration. Untouched, per rulings: `CommandType` stays `IntFlag` (real `A|P` composites in the generated data), and `TransportProtocol`'s `auto()` renumbering and `AppType` are PR 2's. `pcapkit/protocols/schema/misc/pcapng.py`'s `OptionEnumField.post_process` docstring is corrected: it still bypasses `OptionType.get()` directly, now for `pickle` round-trip safety ([#860](https://github.com/JarryShaw/PyPCAPKit/issues/860) already stops the minting that used to be the reason), since neither of `OptionType`'s two paths gets both pickling and correct rendering right at once ([#869](https://github.com/JarryShaw/PyPCAPKit/pull/869)). - `register` on `ProtocolBase`, `Frame`, `PCAPNG`, `SCTP`, `Link`, `Internet` and the concrete `Transport` subclasses (`TCP`, `UDP`) now raises `RegistryError` for a non-class argument, where it raised a bare `TypeError` ("issubclass() arg 1 must be a class") from inside `abc`. Each guard gains an explicit `isinstance(protocol, type)` test ahead of its `issubclass`, after the `ModuleDescriptor` is unwrapped, so a descriptor naming a non-class attribute is rejected too. All seven `Raises:` clauses already promised `RegistryError` if `protocol` "is not a ... subclass", which a non-class satisfied in spirit but not in fact; they now read "is not a class, or not a ... subclass". No guard's target class changes, and a wrong class still raises `RegistryError` as before. `Transport.register` on `Transport` itself still raises `UnsupportedCall` first; only its subclasses reach the guard. Not breaking for exception handling: `RegistryError` subclasses `TypeError` through `BaseError`, so `except TypeError` still catches it, and the type and message text are the only things a handler can key on that change. Two observable side effects do come with it, because `BaseError` is loud by default: the non-class path now emits pcapkit's one `CRITICAL` log record, and outside devmode it installs `sys.excepthook` and `threading.excepthook`, neither of which the bare `TypeError` from `abc` did. That is the wrong-class path's existing behaviour, so the fix makes the two consistent rather than introducing something new. Follows the same fix for the foundation registrars ([#1021](https://github.com/JarryShaw/PyPCAPKit/issues/1021)) ([#1026](https://github.com/JarryShaw/PyPCAPKit/issues/1026)). - `PCAPNG`'s `ns_dnsname`, `ns_dnsIP4addr` and `ns_dnsIP6addr` scope guards, on both the read and the make side, now say the option "must be in Name Resolution Block", which is the block they check; they used to name the systemd(1) Journal Export Block, with a literal `:manpage:` role in the text. `register_record`'s `RegistryWarning` likewise now says "name resolution record already registered". The make-side `ns_dnsIP4addr`/`ns_dnsIP6addr` messages also drop the lowercase `ns_dnsip4addr`/`ns_dnsip6addr` spelling for the `OptionType` member name. Two more guards named the wrong option: `_make_option_if_rxspeed`'s scope and duplicate guards now say `[if_rxspeed]` rather than `[if_txspeed]`, and `_read_option_epb_queue`'s length guard says `[epb_queue]` rather than `[epb_packetid]`. Only message text changes; the exception and warning types are unchanged ([#1038](https://github.com/JarryShaw/PyPCAPKit/issues/1038)). +- `httpv1.HTTP` refused every HTTP/1 message with no header fields, which [[RFC 9112 Section 2.1](https://datatracker.ietf.org/doc/html/rfc9112#section-2.1)] allows (`*( field-line CRLF )`): `b'GET / HTTP/1.0\r\n\r\n'` raised `ProtocolError: HTTP: invalid format`, because `_read_http_header` treated a header with no CRLF as malformed. So `HTTPv1(method='GET', uri='/', http_version='1.1')` failed to read back the bytes its own `make` built, and `HTTP._guess_version` answered `unknown HTTP version`, since `_test_start_line` mirrored the same check. A CRLF-less header is now a start line with zero fields, in the parser and the predicate alike. That check was also what refused the HTTP/2 connection preface, whose header is exactly `PRI * HTTP/2.0`; it is now refused by name instead, to the same message ([#1041](https://github.com/JarryShaw/PyPCAPKit/issues/1041)). ### pcapkit.toolkit diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index e4b1a232a7..e79aa52418 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -2163,6 +2163,17 @@ Fixed ``_read_option_epb_queue``'s length guard says ``[epb_queue]`` rather than ``[epb_packetid]``. Only message text changes; the exception and warning types are unchanged (:issue:`1038`). +* ``httpv1.HTTP`` refused every HTTP/1 message with no header fields, which + [:rfc:`9112#section-2.1`] allows (``*( field-line CRLF )``): + ``b'GET / HTTP/1.0\r\n\r\n'`` raised ``ProtocolError: HTTP: invalid format``, + because ``_read_http_header`` treated a header with no CRLF as malformed. So + ``HTTPv1(method='GET', uri='/', http_version='1.1')`` failed to read back the bytes + its own ``make`` built, and ``HTTP._guess_version`` answered + ``unknown HTTP version``, since ``_test_start_line`` mirrored the same check. A + CRLF-less header is now a start line with zero fields, in the parser and the + predicate alike. That check was also what refused the HTTP/2 connection preface, + whose header is exactly ``PRI * HTTP/2.0``; it is now refused by name instead, to + the same message (:issue:`1041`). pcapkit.toolkit --------------- diff --git a/docs/source/contributing/conventions/process.rst b/docs/source/contributing/conventions/process.rst index 1168a152a6..fee1a7871d 100644 --- a/docs/source/contributing/conventions/process.rst +++ b/docs/source/contributing/conventions/process.rst @@ -101,7 +101,7 @@ commands down instead of a figure that will be stale by the next merge: The grouping scheme was settled on :issue:`918`: **a section per top-level module, with** ``Added``/``Changed``/``Fixed`` **nested inside each** -- module granularity, not per-file and not per-subpackage. The file carries **9** module-level sections holding -164 entries, and no entry carries an inline kind label:: +165 entries, and no entry carries an inline kind label:: $ grep -cE '^\* \*\*(Added|Changed|Fixed)\*\*' docs/source/changelog/1.5.0.rst 0 diff --git a/pcapkit/protocols/application/httpv1.py b/pcapkit/protocols/application/httpv1.py index e18b2eace9..f776cc5ef5 100644 --- a/pcapkit/protocols/application/httpv1.py +++ b/pcapkit/protocols/application/httpv1.py @@ -34,6 +34,7 @@ from pcapkit.const.http.status_code import StatusCode as Enum_StatusCode from pcapkit.corekit.enum import EnumLookup from pcapkit.corekit.multidict import OrderedMultiDict +from pcapkit.protocols.application.http import _HTTP2_PREFACE from pcapkit.protocols.application.http import HTTP as HTTPBase from pcapkit.protocols.data.application.httpv1 import HTTP as Data_HTTP from pcapkit.protocols.data.application.httpv1 import RequestHeader as Data_RequestHeader @@ -72,6 +73,12 @@ # the syntax. _RE_STATUS = re.compile(rb'\d{3}\Z') +# The HTTP/2 connection preface's header block, as ``HTTP.read`` splits it off at +# the first CRLFCRLF: ``PRI * HTTP/2.0``. A start line with zero field lines, so +# the patterns above accept it; it is refused by name in ``_read_http_header`` +# and ``_test_start_line`` so that the preface never parses as HTTP/1. +_HTTP2_PREFACE_HEADER = _HTTP2_PREFACE.split(b'\r\n\r\n', maxsplit=1)[0] + def _test_start_line(data: 'bytes') -> 'bool': """Whether ``data`` opens with an HTTP/1.* start line. @@ -102,17 +109,18 @@ def _test_start_line(data: 'bytes') -> 'bool': ``test_start_line_predicate_agrees_with_the_httpv1_parser`` pins that agreement. - Both of the unpackings the parser performs *before* those patterns are - mirrored too, and this is not pedantry -- the second of them is the whole - reason the HTTP/2 connection preface is not claimed here. ``PRI * + The checks the parser performs *before* those patterns are mirrored too, + and this is not pedantry -- they are the whole reason the HTTP/2 + connection preface is not claimed here. ``PRI * HTTP/2.0\\r\\n\\r\\nSM\\r\\n\\r\\n`` is deliberately a well-formed HTTP/1.1 *request line* (:rfc:`9113#section-3.4`), so a predicate that tested only the first line would answer :data:`True` for it. Split at the header/body separator first, as :meth:`HTTP.read ` does, and the preface's - header is ``PRI * HTTP/2.0`` with no CRLF left in it -- which is why the - parser refuses it, and why this does. Without the separator split this - returns :data:`True` for the preface. + header is exactly ``PRI * HTTP/2.0`` (``_HTTP2_PREFACE_HEADER``), which + the parser refuses by name and so does this. Any *other* header with no + CRLF in it is a start line with zero field lines, which + :rfc:`9112#section-2.1` allows and both accept (:issue:`1041`). An HTTP/0.9 request line carries only two tokens and so is not recognised here either, matching the parser, which raises on fewer than three. @@ -122,10 +130,10 @@ def _test_start_line(data: 'bytes') -> 'bool': if header == data: # no header/body separator -- ``read`` raises return False - startline = header.split(b'\r\n', maxsplit=1)[0] - if startline == header: # header holds no CRLF -- ``_read_http_header`` raises + if header == _HTTP2_PREFACE_HEADER: # the preface -- ``_read_http_header`` raises return False + startline = header.split(b'\r\n', maxsplit=1)[0] try: para1, para2, para3 = re.split(rb'\s+', startline, maxsplit=2) except ValueError: @@ -369,15 +377,21 @@ def _read_http_header(self, header: 'bytes') -> 'tuple[Data_Header, OrderedMulti ProtocolError: If the packet is malformed. """ - # NOTE: Both unpackings are short for input that is not an HTTP/1 - # message: a header of one line with no CRLF -- the HTTP/2 connection - # preface, ``PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n``, splits to exactly that - # -- and a start line of fewer than three whitespace-separated tokens. - # Raised as ``ProtocolError`` for the reason ``read`` gives above, with - # the same message this method uses below for a start line it cannot - # recognise. + # NOTE: A header with no CRLF is a start line with zero field lines, + # which :rfc:`9112#section-2.1` allows (``*( field-line CRLF )``). The + # one such header refused by name is the HTTP/2 connection preface's: + # ``PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n`` splits in ``read`` to exactly + # ``PRI * HTTP/2.0``, which ``_RE_METHOD`` and ``_RE_VERSION`` would + # otherwise accept as an HTTP/1 request (see ``_HTTP2_PREFACE_HEADER``). + if header == _HTTP2_PREFACE_HEADER: + raise ProtocolError('HTTP: invalid format') + startline, _, headerfield = header.partition(b'\r\n') + + # NOTE: Short for a start line of fewer than three whitespace-separated + # tokens. Raised as ``ProtocolError`` for the reason ``read`` gives + # above, with the same message this method uses below for a start line + # it cannot recognise. try: - startline, headerfield = header.split(b'\r\n', 1) para1, para2, para3 = re.split(rb'\s+', startline, maxsplit=2) except ValueError as error: raise ProtocolError('HTTP: invalid format') from error @@ -394,7 +408,7 @@ def _read_http_header(self, header: 'bytes') -> 'tuple[Data_Header, OrderedMulti # extra field (``X-Long: a`` plus ``b: c``, for a folded ``X-Long: a b``). # Unfolded, a folded message parses to the field it actually carries. fields = [] # type: list[bytes] - for line in headerfield.split(b'\r\n'): + for line in headerfield.split(b'\r\n') if headerfield else (): if line.startswith((b' ', b'\t')): # A continuation with nothing to continue -- the first field line # folded -- is malformed rather than unfoldable. diff --git a/tests/protocols/application/test_http_unit.py b/tests/protocols/application/test_http_unit.py index ffc403d13a..9e8792da86 100644 --- a/tests/protocols/application/test_http_unit.py +++ b/tests/protocols/application/test_http_unit.py @@ -234,20 +234,16 @@ def test_http_construction_reaches_the_versioned_make_callee(self) -> None: recognise, so #787 makes one message of two rather than introducing a new one. - Worth recording, since the docstring above used to claim otherwise: the - rejection is not ``HTTPv1.make`` refusing these keywords. ``make`` - accepts them and builds ``b'GET / HTTP/1.1\\r\\n\\r\\n'``; it is the - read-back of that packet which fails, because ``_read_http_header`` - requires a header field section and a field-less request has none. That - a valid minimal HTTP/1 request cannot be re-read is a defect in its own - right, and not one #787 set out to fix. + Since #1041 the read-back succeeds, so the callee is proven reached by + its output instead: ``make`` builds ``b'GET / HTTP/1.1\\r\\n\\r\\n'``, + a field-less request, and ``_read_http_header`` now accepts zero field + lines (:rfc:`9112#section-2.1`) where it used to reject them. """ from pcapkit.protocols.application.http import HTTP - from pcapkit.utilities.exceptions import ProtocolError - with self.assertRaises(ProtocolError) as ctx: - HTTP(version=1, http_version='1.1', method='GET', uri='/') - self.assertEqual(str(ctx.exception), 'HTTP: invalid format') + proto = HTTP(version=1, http_version='1.1', method='GET', uri='/') + self.assertEqual(proto.data, b'GET / HTTP/1.1\r\n\r\n') + self.assertEqual(proto.version, '1.1') def test_http_read_explicit_version_uses_same_buffer_as_guess(self) -> None: """Regression test for GH-447. @@ -771,13 +767,16 @@ def test_start_line_predicate_agrees_with_the_httpv1_parser(self) -> None: ('response', b'HTTP/1.1 200 OK\r\nServer: example\r\n\r\nbody'), ('response, no message', b'HTTP/1.0 404 -\r\nServer: example\r\n\r\n'), ('hyphenated method', b'M-SEARCH * HTTP/1.1\r\nHost: example.com\r\n\r\n'), + # Zero field lines (#1041). + ('field-less request', b'GET / HTTP/1.0\r\n\r\n'), + ('field-less response', b'HTTP/1.0 200 OK\r\n\r\nbody'), ) refused = ( # Rejected by the anchored patterns themselves. ('lowercase method', b'Get / HTTP/1.1\r\nHost: example.com\r\n\r\n'), ('four-digit status', b'HTTP/1.1 2000 OK\r\nServer: example\r\n\r\n'), ('no version token', b'GET / FTP/1.1\r\nHost: example.com\r\n\r\n'), - # Rejected by the unpackings ``_read_http_header`` performs first. + # Rejected by the checks ``read`` and ``_read_http_header`` perform first. ('two-token start line', b'GET /\r\nHost: example.com\r\n\r\n'), ('no CRLF at all', b'GET / HTTP/1.1'), ('the HTTP/2 preface', b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'), @@ -1021,9 +1020,8 @@ def test_httpv1_read_raises_protocol_error_for_a_malformed_message(self) -> None cases = ( # No header/body separator at all -- ``read``'s own unpacking. ('no separator', b'not http at all'), - # A header with no CRLF in it -- the HTTP/2 preface is exactly this - # once the separator has been split off. - ('preface start line', b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'), + # The HTTP/2 preface is no longer refused by an unpacking (#1041) -- + # ``test_httpv1_still_refuses_the_http2_connection_preface`` covers it. # A start line of fewer than three whitespace-separated tokens. ('two-token start line', b'GET /\r\nHost: example.com\r\n\r\n'), ('one-token start line', b'PRI\r\nHost: example.com\r\n\r\n'), @@ -1037,6 +1035,125 @@ def test_httpv1_read_raises_protocol_error_for_a_malformed_message(self) -> None self.assertIsInstance(ctx.exception.__cause__, ValueError) self.assertNotIsInstance(ctx.exception.__cause__, ProtocolError) + def test_httpv1_reads_a_request_with_no_header_fields(self) -> None: + """A request line followed straight by the empty line parses (#1041). + + :rfc:`9112#section-2.1` gives ``*( field-line CRLF )``, so zero fields is + legal. The header block is then the start line alone, with no CRLF in + it, which ``_read_http_header`` used to refuse as malformed. Asserted + through the dispatcher too: ``_test_start_line`` mirrored that refusal, + so ``HTTP`` answered ``unknown HTTP version`` for the same bytes. + + """ + import io + + from pcapkit.protocols.application.http import HTTP + from pcapkit.protocols.application.httpv1 import HTTP as HTTPv1 + from pcapkit.protocols.application.httpv1 import Type, _test_start_line + + raw = b'GET / HTTP/1.0\r\n\r\n' + proto = HTTPv1(io.BytesIO(raw), len(raw)) + + self.assertEqual(proto.info.receipt.type, Type.REQUEST) + self.assertEqual(proto.info.receipt.method, 'GET') + self.assertEqual(proto.info.receipt.uri, '/') + self.assertEqual(proto.version, '1.0') + self.assertEqual(len(proto.info.header), 0) + self.assertIsNone(proto.info.body) + self.assertEqual(proto.length, len(b'GET / HTTP/1.0')) + + self.assertTrue(_test_start_line(raw)) + self.assertEqual(HTTP(io.BytesIO(raw), len(raw)).version, '1.0') + + def test_httpv1_reads_a_response_with_no_header_fields_and_a_body(self) -> None: + """A status line, the empty line, then a body parses (#1041).""" + import io + + from pcapkit.protocols.application.http import HTTP + from pcapkit.protocols.application.httpv1 import HTTP as HTTPv1 + from pcapkit.protocols.application.httpv1 import Type, _test_start_line + + raw = b'HTTP/1.0 200 OK\r\n\r\nbody' + proto = HTTPv1(io.BytesIO(raw), len(raw)) + + self.assertEqual(proto.info.receipt.type, Type.RESPONSE) + self.assertEqual(proto.info.receipt.status, 200) + self.assertEqual(proto.info.receipt.message, 'OK') + self.assertEqual(proto.version, '1.0') + self.assertEqual(len(proto.info.header), 0) + self.assertEqual(proto.info.body, b'body') + + self.assertTrue(_test_start_line(raw)) + self.assertEqual(HTTP(io.BytesIO(raw), len(raw)).version, '1.0') + + def test_httpv1_make_read_round_trips_a_message_with_no_header_fields(self) -> None: + """``make`` emits zero fields as ``start-line CRLF CRLF``; ``read`` takes it back (#1041). + + ``make`` already produced those bytes when given no ``headers``, so + constructing ``HTTPv1(method='GET', uri='/', http_version='1.1')`` raised + from its own ``read``. The response is also round-tripped through + ``_make_data``, so an empty parsed header dict is shown to rebuild the + same bytes; the request is not, because ``_make_data`` hands an empty + body back as :obj:`None`, which ``make`` cannot concatenate -- with or + without header fields, so a separate defect from this one. + + """ + import io + + from pcapkit.protocols.application.httpv1 import HTTP as HTTPv1 + + request = HTTPv1(method='GET', uri='/', http_version='1.1') + self.assertEqual(request.data, b'GET / HTTP/1.1\r\n\r\n') + self.assertEqual(request.info.receipt.method, 'GET') + self.assertEqual(request.info.receipt.uri, '/') + self.assertEqual(request.version, '1.1') + self.assertEqual(len(request.info.header), 0) + + wire = b'HTTP/1.0 200 OK\r\n\r\nbody' + response = HTTPv1(status=200, message='OK', http_version='1.0', body=b'body') + self.assertEqual(response.data, wire) + self.assertEqual(response.info.receipt.status, 200) + self.assertEqual(len(response.info.header), 0) + + parsed = HTTPv1(io.BytesIO(wire), len(wire)) + self.assertEqual(HTTPv1(**HTTPv1._make_data(parsed.info)).data, wire) + + def test_httpv1_still_refuses_the_http2_connection_preface(self) -> None: + """Accepting zero fields must not let the HTTP/2 preface parse as HTTP/1 (#1041). + + The preface's header block, once split at the first CRLFCRLF, is + ``PRI * HTTP/2.0`` -- a start line with zero fields, which ``_RE_METHOD`` + and ``_RE_VERSION`` both accept. Refusing every CRLF-less header used to + be what turned it away; it is now refused by name. A regression guard: + this held before the fix and must hold after it, to the same message, in + the parser and the predicate alike, and the dispatcher's answers for a + complete and a truncated preface are unchanged. + + """ + import io + + from pcapkit.protocols.application.http import HTTP + from pcapkit.protocols.application.httpv1 import HTTP as HTTPv1 + from pcapkit.protocols.application.httpv1 import _test_start_line + from pcapkit.utilities.exceptions import ProtocolError + + cases = ( + ('preface', b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n', 'HTTP/2: connection preface with no frame'), + ('truncated preface', b'PRI * HTTP/2.0\r\n\r\n', 'unknown HTTP version'), + ('preface, corrupt tail', b'PRI * HTTP/2.0\r\n\r\nXX\r\n\r\n', 'unknown HTTP version'), + ) + + for label, raw, dispatched in cases: + with self.subTest(case=label): + with self.assertRaises(ProtocolError) as ctx: + HTTPv1(io.BytesIO(raw), len(raw)) + self.assertEqual(str(ctx.exception), 'HTTP: invalid format') + self.assertFalse(_test_start_line(raw)) + + with self.assertRaises(ProtocolError) as ctx: + HTTP(io.BytesIO(raw), len(raw)) + self.assertEqual(str(ctx.exception), dispatched) + def test_httpv1_refuses_a_field_line_with_no_colon(self) -> None: """A colon-less field line must raise ``ProtocolError``, not :exc:`IndexError`. @@ -1225,8 +1342,6 @@ def test_httpv1_never_lets_a_bare_exception_escape(self) -> None: ('read: no CRLFCRLF', b'not http at all'), ('read: no CRLFCRLF, long', b'abcdefghijklmnopqrstuvwxyz0123456789'), ('header: no CRLF (preface)', b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'), - ('header: no CRLF (field-less request)', b'GET / HTTP/1.0\r\n\r\n'), - ('header: no CRLF (field-less response)', b'HTTP/1.1 200 OK\r\n\r\n'), ('start line: two tokens', b'GET /\r\nHost: e\r\n\r\n'), ('start line: one token', b'PRI\r\nHost: e\r\n\r\n'), ('field line: no colon', b'GET / HTTP/1.1\r\nNoColonHere\r\n\r\n'), @@ -1245,6 +1360,9 @@ def test_httpv1_never_lets_a_bare_exception_escape(self) -> None: ('valid request', b'GET / HTTP/1.1\r\nHost: e\r\n\r\n'), ('valid response', b'HTTP/1.1 200 OK\r\nServer: s\r\n\r\n'), ('valid folded request', b'GET / HTTP/1.1\r\nX: a\r\n b\r\nHost: e\r\n\r\n'), + # Field-less, refused as a CRLF-less header until #1041. + ('valid field-less request', b'GET / HTTP/1.0\r\n\r\n'), + ('valid field-less response', b'HTTP/1.1 200 OK\r\n\r\n'), ) for label, raw in payloads: diff --git a/tests/protocols/test_construction_keyword_check_unit.py b/tests/protocols/test_construction_keyword_check_unit.py index 4a15a174a7..516caff19d 100644 --- a/tests/protocols/test_construction_keyword_check_unit.py +++ b/tests/protocols/test_construction_keyword_check_unit.py @@ -550,21 +550,17 @@ def test_a_dispatcher_may_decline_the_check(self) -> None: UnsupportedCall: HTTP: unexpected keyword(s): 'http_version', 'method', 'uri' Asserted through the real class rather than a dummy, because the point is - that this one protocol declines and the rest do not. The construction still - fails, with :exc:`~pcapkit.utilities.exceptions.ProtocolError` from - :class:`HTTPv1 ` -- which is the - outcome + that this one protocol declines and the rest do not. The construction + succeeds, and the packet it builds -- ``b'GET / HTTP/1.1\\r\\n\\r\\n'``, + which only :class:`HTTPv1 `'s + ``make`` produces from these keywords -- is the proof that they reached + the delegate rather than being refused on the way. + + Until #1041 that packet failed to read back, as ``ProtocolError: HTTP: + invalid format``, because ``httpv1.HTTP`` refused a message with no header + fields; this test asserted that error as its proof of delegation. :meth:`HTTPUnitTests.test_http_construction_reaches_the_versioned_make_callee` - already pins, and is the proof that the keywords reached the delegate - rather than being refused on the way. - - The asserted message was ``'HTTP/1: invalid format'`` until #787, and the - attribution above said ``HTTPv1.make``. Neither the exception type nor what - this test is *for* moved: ``HTTPv1.make`` accepts these keywords and builds - the packet, and it is reading that packet back that fails -- now as a - ``ProtocolError`` raised by ``httpv1.HTTP`` itself, which ``HTTP.read`` - re-raises unchanged rather than re-labelling with the dispatched version. - See that method's docstring for the whole of it. + pins the same outcome from the HTTP side. """ from pcapkit.protocols.application.http import HTTP @@ -572,7 +568,6 @@ def test_a_dispatcher_may_decline_the_check(self) -> None: from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 from pcapkit.protocols.protocol import _declared_keywords from pcapkit.protocols.transport.tcp import TCP - from pcapkit.utilities.exceptions import ProtocolError self.assertIsNone(_declared_keywords(HTTP)) self.assertIsNotNone(_declared_keywords(TCP)) @@ -588,9 +583,9 @@ def test_a_dispatcher_may_decline_the_check(self) -> None: self.assertIsNotNone(accepted) self.assertIn(keyword, accepted) - with self.assertRaises(ProtocolError) as context: - HTTP(version=1, http_version='1.1', method='GET', uri='/') - self.assertEqual(str(context.exception), 'HTTP: invalid format') + proto = HTTP(version=1, http_version='1.1', method='GET', uri='/') + self.assertEqual(proto.data, b'GET / HTTP/1.1\r\n\r\n') + self.assertEqual(proto.version, '1.1') @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')