From 336ea4aa1694976259137b1d839e7cb810357639 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 26 Oct 2026 17:00:36 -0400 Subject: [PATCH] fix(http): give HTTP's version, data, start-line and settings errors the HTTP: prefix (#2298) Problem: http.HTTP's version, data and version-guess errors, httpv1.HTTP._make_start_line's errors, and httpv2.HTTP._make_http_settings's rejection all broke the house ": : ... {value!r}" form -- missing or malformed prefixes, capitalised with a full stop, and some without the rejected values. Fix: reword them as "HTTP: invalid version: 3", "HTTP: invalid data: ...", "HTTP: unknown version", "HTTP: invalid start line: ..." (naming the method and status it got), and "HTTP/2: [Type 4] invalid settings: 42" (dropping the stray space before the colon and adding the rejected value). Test: test_http_message_prefix_2298_unit pins each message, including the settings one; the existing pins in test_http_unit, test_http_guess_http2_1141_unit and test_http_from_data_1154_unit follow the new text. --- pcapkit/protocols/application/http.py | 11 +- pcapkit/protocols/application/httpv1.py | 5 +- pcapkit/protocols/application/httpv2.py | 3 +- .../test_http_from_data_1154_unit.py | 2 +- .../test_http_guess_http2_1141_unit.py | 4 +- .../test_http_message_prefix_2298_unit.py | 124 ++++++++++++++++++ tests/protocols/application/test_http_unit.py | 16 +-- 7 files changed, 146 insertions(+), 19 deletions(-) create mode 100644 tests/protocols/application/test_http_message_prefix_2298_unit.py diff --git a/pcapkit/protocols/application/http.py b/pcapkit/protocols/application/http.py index c82061f9fd..5db536591a 100644 --- a/pcapkit/protocols/application/http.py +++ b/pcapkit/protocols/application/http.py @@ -298,7 +298,7 @@ def read(self, length: 'Optional[int]' = None, *, elif version == 2: from pcapkit.protocols.application.httpv2 import HTTP as protocol # type: ignore[assignment] # isort: skip # pylint: disable=line-too-long,import-outside-toplevel else: - raise ProtocolError(f"invalid HTTP version: {version}") + raise ProtocolError(f'HTTP: invalid version: {version!r}') try: http = protocol(self._data, length, **kwargs) @@ -358,7 +358,7 @@ def make(self, elif version == 2: from pcapkit.protocols.application.httpv2 import HTTP as protocol # type: ignore[assignment] # isort: skip # pylint: disable=line-too-long,import-outside-toplevel else: - raise ProtocolError(f"invalid HTTP version: {version}") + raise ProtocolError(f'HTTP: invalid version: {version!r}') # NOTE: ``protocol.make`` is an ordinary instance method (the abstract # declaration at ``ProtocolBase.make`` takes ``self``, and the @@ -449,7 +449,8 @@ def _make_data(cls, data: '_PT') -> 'dict[str, Any]': # type: ignore[override] return {'version': 1, **HTTPv1._make_data(data)} if isinstance(data, Data_HTTPv2): return {'version': 2, 'preface': bool(data.get('preface')), **HTTPv2._make_data(data)} - raise ProtocolError(f"invalid HTTP data: {type(data).__name__}") + raise ProtocolError(f'HTTP: invalid data: must be HTTP/1.* or HTTP/2 data, ' + f'not {type(data).__name__}') def _test_preface(self, length: 'int') -> 'bool': """Test whether the first ``length`` octets open with the HTTP/2 connection preface. @@ -632,10 +633,10 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP': # test rejects only headers that no conforming sender writes, and it # gates this guess alone: an explicit ``version=2`` is not affected. if not _test_http2_frame(self._data[:length]): - raise ProtocolError("unknown HTTP version") + raise ProtocolError('HTTP: unknown version') from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2 # isort: skip # pylint: disable=line-too-long,import-outside-toplevel with contextlib.suppress(ProtocolError, struct.error): return HTTPv2(self._data, length, **kwargs) - raise ProtocolError("unknown HTTP version") + raise ProtocolError('HTTP: unknown version') diff --git a/pcapkit/protocols/application/httpv1.py b/pcapkit/protocols/application/httpv1.py index fdb673b2ae..3af432d0fe 100644 --- a/pcapkit/protocols/application/httpv1.py +++ b/pcapkit/protocols/application/httpv1.py @@ -334,7 +334,7 @@ def _make_start_line(self, http_version: 'str | bytes', *, version = http_version.encode() if isinstance(http_version, str) else http_version if method is not None and status is None: if uri is None: - raise ProtocolError('HTTP request must have URI.') + raise ProtocolError('HTTP: invalid start line: a request needs a URI') if isinstance(method, Enum_Method): meth = method.value.encode() @@ -362,7 +362,8 @@ def _make_start_line(self, http_version: 'str | bytes', *, # status-code is 3DIGIT (RFC 9112, section 4), so keep a leading zero header_line = b'HTTP/%s %03d %s\r\n' % (version, status_code_val, msg) else: - raise ProtocolError('HTTP packet must be either request or response.') + raise ProtocolError('HTTP: invalid start line: must have either a method or a status, ' + f'not method={method!r} and status={status!r}') return header_line @classmethod diff --git a/pcapkit/protocols/application/httpv2.py b/pcapkit/protocols/application/httpv2.py index 4e70f7f687..f22c033179 100644 --- a/pcapkit/protocols/application/httpv2.py +++ b/pcapkit/protocols/application/httpv2.py @@ -1342,7 +1342,8 @@ def _make_http_settings(self, frame: 'Optional[Data_SettingsFrame]' = None, *, ) settings_val.append(temp) else: - raise ProtocolError(f'HTTP/2 : [Type {Enum_Frame.SETTINGS}] invalid settings') + raise ProtocolError(f'HTTP/2: [Type {Enum_Frame.SETTINGS}] invalid settings: ' + f'{settings!r}') return Schema_SettingsFrame( settings=settings_val, diff --git a/tests/protocols/application/test_http_from_data_1154_unit.py b/tests/protocols/application/test_http_from_data_1154_unit.py index 9bcab4666f..8b4b8cf6c9 100644 --- a/tests/protocols/application/test_http_from_data_1154_unit.py +++ b/tests/protocols/application/test_http_from_data_1154_unit.py @@ -106,7 +106,7 @@ def test_dispatcher_rejects_data_of_no_http_version(self) -> None: from pcapkit.protocols.application.http import HTTP from pcapkit.utilities.exceptions import ProtocolError - with self.assertRaisesRegex(ProtocolError, 'invalid HTTP data'): + with self.assertRaisesRegex(ProtocolError, '^HTTP: invalid data: '): HTTP._make_data({'version': 1}) # type: ignore[arg-type] diff --git a/tests/protocols/application/test_http_guess_http2_1141_unit.py b/tests/protocols/application/test_http_guess_http2_1141_unit.py index 1d7add3b83..65d7dda20c 100644 --- a/tests/protocols/application/test_http_guess_http2_1141_unit.py +++ b/tests/protocols/application/test_http_guess_http2_1141_unit.py @@ -108,7 +108,7 @@ def test_nine_zero_octets_are_not_guessed_as_http2(self) -> None: """The #1141 reproduction: a stream-0 ``DATA`` frame is not HTTP/2 by guess.""" from pcapkit.utilities.exceptions import ProtocolError - with self.assertRaisesRegex(ProtocolError, 'unknown HTTP version'): + with self.assertRaisesRegex(ProtocolError, 'HTTP: unknown version'): self._guess(bytes(9)) def test_explicit_version_still_parses_nine_zero_octets(self) -> None: @@ -125,7 +125,7 @@ def test_headers_no_sender_writes_are_not_guessed(self) -> None: for name, octets in REJECTED_FRAMES: with self.subTest(frame=name): - with self.assertRaisesRegex(ProtocolError, 'unknown HTTP version'): + with self.assertRaisesRegex(ProtocolError, 'HTTP: unknown version'): self._guess(bytes.fromhex(octets)) def test_every_legal_frame_type_is_still_guessed_as_http2(self) -> None: diff --git a/tests/protocols/application/test_http_message_prefix_2298_unit.py b/tests/protocols/application/test_http_message_prefix_2298_unit.py new file mode 100644 index 0000000000..1a7cff877c --- /dev/null +++ b/tests/protocols/application/test_http_message_prefix_2298_unit.py @@ -0,0 +1,124 @@ +# -*- coding: utf-8 -*- +"""HTTP's version, data, start-line and ``SETTINGS`` errors carry the ``HTTP:`` prefix. C.f. #2298. + +The rest of HTTP's messages read ``HTTP: invalid format``, the house +``: : … {value!r}`` form (#2232, #2257). Before +the fix these read ``invalid HTTP version: 3``, ``invalid HTTP data: dict`` and +``unknown HTTP version`` from :class:`~pcapkit.protocols.application.http.HTTP`, +and ``HTTP request must have URI.`` and +``HTTP packet must be either request or response.`` from +:meth:`httpv1.HTTP._make_start_line +`, capitalised, with +a closing full stop and without the values that were rejected. The +``SETTINGS`` one read ``HTTP/2 : [Type 4] invalid settings`` from +:meth:`httpv2.HTTP._make_http_settings +`, a space before +the colon and no rejected value either. + +Every case builds its own octets in memory and reads no capture. Classes are +imported inside each test, after +:func:`~tests._support.reimport_once_per_class`, so they match the live +:mod:`pcapkit` import. + +""" + +import io +import unittest + +from tests._support import reimport_once_per_class + + +class TestHTTPDispatcherMessages(unittest.TestCase): + """Pin the dispatcher's version and data messages.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def test_make_names_the_rejected_version(self) -> None: + from pcapkit.protocols.application.http import HTTP + from pcapkit.utilities.exceptions import ProtocolError + + with self.assertRaises(ProtocolError) as ctx: + HTTP.make(HTTP.__new__(HTTP), version=3) # type: ignore[arg-type] + self.assertEqual(str(ctx.exception), 'HTTP: invalid version: 3') + + def test_read_names_the_rejected_version(self) -> None: + from pcapkit.protocols.application.http import HTTP + from pcapkit.utilities.exceptions import ProtocolError + + raw = b'GET / HTTP/1.1\r\n\r\n' + with self.assertRaises(ProtocolError) as ctx: + HTTP(io.BytesIO(raw), len(raw), version='3') + self.assertEqual(str(ctx.exception), "HTTP: invalid version: '3'") + + def test_make_data_names_the_rejected_type(self) -> None: + from pcapkit.protocols.application.http import HTTP + from pcapkit.utilities.exceptions import ProtocolError + + with self.assertRaises(ProtocolError) as ctx: + HTTP._make_data({'version': 1}) # type: ignore[arg-type] + self.assertEqual(str(ctx.exception), + 'HTTP: invalid data: must be HTTP/1.* or HTTP/2 data, not dict') + + def test_guess_version_has_the_prefix(self) -> None: + from pcapkit.protocols.application.http import HTTP + from pcapkit.utilities.exceptions import ProtocolError + + raw = b'\x00' * 3 + with self.assertRaises(ProtocolError) as ctx: + HTTP(io.BytesIO(raw), len(raw)) + self.assertEqual(str(ctx.exception), 'HTTP: unknown version') + + +class TestHTTPv1StartLineMessages(unittest.TestCase): + """Pin ``httpv1.HTTP._make_start_line``'s messages.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def test_request_without_uri(self) -> None: + from pcapkit.protocols.application.httpv1 import HTTP + from pcapkit.utilities.exceptions import ProtocolError + + with self.assertRaises(ProtocolError) as ctx: + HTTP(method='GET', http_version='1.1') + self.assertEqual(str(ctx.exception), 'HTTP: invalid start line: a request needs a URI') + + def test_neither_or_both_of_method_and_status(self) -> None: + from pcapkit.protocols.application.httpv1 import HTTP + from pcapkit.utilities.exceptions import ProtocolError + + cases = ( + ({}, 'not method=None and status=None'), + ({'method': 'GET', 'status': 200}, "not method='GET' and status=200"), + ) + for kwargs, tail in cases: + with self.subTest(**kwargs): + with self.assertRaises(ProtocolError) as ctx: + HTTP.__new__(HTTP)._make_start_line(http_version='1.1', **kwargs) + self.assertEqual(str(ctx.exception), + 'HTTP: invalid start line: must have either a method or a ' + f'status, {tail}') + + +class TestHTTPv2SettingsMessage(unittest.TestCase): + """Pin ``httpv2.HTTP._make_http_settings``'s rejection message.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def test_settings_names_the_rejected_value(self) -> None: + from pcapkit.const.http.frame import Frame + from pcapkit.protocols.application.httpv2 import HTTP + from pcapkit.utilities.exceptions import ProtocolError + + for settings, tail in ((42, '42'), (None, 'None'), ('x', "'x'"), (4.5, '4.5')): + with self.subTest(settings=settings): + with self.assertRaises(ProtocolError) as ctx: + bytes(HTTP(type=Frame.SETTINGS, frame={'settings': settings})) + self.assertEqual(str(ctx.exception), + f'HTTP/2: [Type 4] invalid settings: {tail}') + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/application/test_http_unit.py b/tests/protocols/application/test_http_unit.py index 992b1b0d43..33cc3564ff 100644 --- a/tests/protocols/application/test_http_unit.py +++ b/tests/protocols/application/test_http_unit.py @@ -509,7 +509,7 @@ def test_guess_version_identifies_the_http2_connection_preface(self) -> None: preface's own ASCII as a frame header, ``b'PRI'`` becoming a declared length of 5,263,945, which the consistency check of #799/#802 refuses -- so a real HTTP/2 connection opening, bare preface or - preface-plus-SETTINGS, would raise ``unknown HTTP version``. + preface-plus-SETTINGS, raises ``HTTP: unknown version``. Three things are asserted, because "it answers 2" alone would also be true of a parse that consumed the preface as framing: @@ -683,7 +683,7 @@ def test_guess_version_commits_to_http1_once_the_start_line_says_so(self) -> Non Asserted on the message, because the exception *type* is ``ProtocolError`` either way: a payload both arms declined comes back - ``unknown HTTP version``, while these come back ``HTTP: invalid format`` + ``HTTP: unknown version``, while these come back ``HTTP: invalid format`` from the version that was actually identified. """ @@ -871,7 +871,7 @@ def test_guess_version_still_prefers_http1_for_http1_bytes(self) -> None: self.assertEqual(guessed.info, explicit.info) def test_guess_version_still_reports_an_unknown_http_version(self) -> None: - """Bytes neither arm accepts must raise ``unknown HTTP version``. + """Bytes neither arm accepts must raise ``HTTP: unknown version``. ``_guess_version``'s closing ``raise`` is only reachable when *both* arms fail with :class:`~pcapkit.utilities.exceptions.ProtocolError`, so @@ -899,7 +899,7 @@ def test_guess_version_still_reports_an_unknown_http_version(self) -> None: with self.assertRaises(ProtocolError) as ctx: HTTP(io.BytesIO(raw), len(raw)) - self.assertEqual(str(ctx.exception), 'unknown HTTP version') + self.assertEqual(str(ctx.exception), 'HTTP: unknown version') def test_guess_version_reports_unknown_version_for_a_short_payload(self) -> None: """A payload too short for an HTTP/2 frame header must not leak :exc:`struct.error`. @@ -1004,7 +1004,7 @@ def test_httpv1_reads_a_request_with_no_header_fields(self) -> None: legal. The header block is then the start line alone, with no CRLF in it, which ``_read_http_header`` must not refuse as malformed. Asserted through the dispatcher too: were ``test_start_line`` to mirror such a - refusal, ``HTTP`` would answer ``unknown HTTP version`` for the same + refusal, ``HTTP`` would answer ``HTTP: unknown version`` for the same bytes. """ @@ -1129,8 +1129,8 @@ def test_httpv1_still_refuses_the_http2_connection_preface(self) -> None: 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'), + ('truncated preface', b'PRI * HTTP/2.0\r\n\r\n', 'HTTP: unknown version'), + ('preface, corrupt tail', b'PRI * HTTP/2.0\r\n\r\nXX\r\n\r\n', 'HTTP: unknown version'), ) for label, raw, dispatched in cases: @@ -1398,7 +1398,7 @@ def test_guess_version_does_not_suppress_struct_error_on_the_http1_arm(self) -> and the HTTP/2 arm accepts any payload of at least nine octets -- so a swallowed error on arm 1 becomes a confident ``version='2'`` for a message that is plainly HTTP/1, which is the mislabel #787 exists to - stop rather than a tidier failure. ``unknown HTTP version`` is only the + stop rather than a tidier failure. ``HTTP: unknown version`` is only the best case, needing arm 2 to decline as well. The *outer*-header slice of the class this suppression exists for is