diff --git a/CHANGELOG.md b/CHANGELOG.md index 9451664efb..7b60853ec6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -192,6 +192,7 @@ This resolves [#548](https://github.com/JarryShaw/PyPCAPKit/issues/548), which r - `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)). +- `httpv1.HTTP.from_data` could not rebuild an HTTP/1 message whose body is empty: `HTTP.from_data(info)` for `b'GET / HTTP/1.1\r\nHost: a\r\n\r\n'` raised `TypeError: can't concat NoneType to bytes`. `read` records an empty body as `None`, `_make_data` passed it through, and `make` concatenated it. `_make_data` now hands `make` `b''` for a `None` body; `info.body` is still `None`, so readers see no change. `httpv2` is unaffected: its empty-payload frames already round-trip ([#1050](https://github.com/JarryShaw/PyPCAPKit/issues/1050)). ### pcapkit.toolkit diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index bc74b4f8c7..a5df9636dc 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -2187,6 +2187,13 @@ Fixed 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`). +* ``httpv1.HTTP.from_data`` could not rebuild an HTTP/1 message whose body is + empty: ``HTTP.from_data(info)`` for ``b'GET / HTTP/1.1\r\nHost: a\r\n\r\n'`` + raised ``TypeError: can't concat NoneType to bytes``. ``read`` records an empty + body as ``None``, ``_make_data`` passed it through, and ``make`` concatenated it. + ``_make_data`` now hands ``make`` ``b''`` for a ``None`` body; ``info.body`` is + still ``None``, so readers see no change. ``httpv2`` is unaffected: its + empty-payload frames already round-trip (:issue:`1050`). pcapkit.toolkit --------------- diff --git a/docs/source/contributing/conventions/process.rst b/docs/source/contributing/conventions/process.rst index 8a2efc7d30..eb34273a4e 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 -166 entries, and no entry carries an inline kind label:: +167 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 f776cc5ef5..47c256f6e2 100644 --- a/pcapkit/protocols/application/httpv1.py +++ b/pcapkit/protocols/application/httpv1.py @@ -345,6 +345,9 @@ def _make_data(cls, data: 'Data_HTTP') -> 'dict[str, Any]': # type: ignore[over Key-value pairs for protocol construction. """ + # NOTE: ``read`` records an empty body as :obj:`None`, which is the + # public ``info.body`` value; ``make`` takes ``bytes`` and concatenates + # it, so the read-side ``None`` is translated back here (#1050). return { 'http_version': data.receipt.version, # type: ignore[attr-defined] 'method': getattr(data.receipt, 'method', None), @@ -352,7 +355,7 @@ def _make_data(cls, data: 'Data_HTTP') -> 'dict[str, Any]': # type: ignore[over 'status': getattr(data.receipt, 'status', None), 'message': getattr(data.receipt, 'message', None), 'headers': data.header, - 'body': data.body, + 'body': b'' if data.body is None else data.body, } def _read_http_header(self, header: 'bytes') -> 'tuple[Data_Header, OrderedMultiDict[str, str]]': diff --git a/tests/protocols/application/test_http_unit.py b/tests/protocols/application/test_http_unit.py index 9e8792da86..e8d671b632 100644 --- a/tests/protocols/application/test_http_unit.py +++ b/tests/protocols/application/test_http_unit.py @@ -1093,9 +1093,9 @@ def test_httpv1_make_read_round_trips_a_message_with_no_header_fields(self) -> N 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. + same bytes. The empty-bodied request's ``from_data`` round trip is + :meth:`test_httpv1_from_data_round_trips_a_message_with_an_empty_body` + (#1050). """ import io @@ -1118,6 +1118,35 @@ def test_httpv1_make_read_round_trips_a_message_with_no_header_fields(self) -> N parsed = HTTPv1(io.BytesIO(wire), len(wire)) self.assertEqual(HTTPv1(**HTTPv1._make_data(parsed.info)).data, wire) + def test_httpv1_from_data_round_trips_a_message_with_an_empty_body(self) -> None: + """``from_data(info)`` rebuilds a message whose body is empty (#1050). + + ``read`` records an empty body as :obj:`None`, and that stays the public + ``info.body`` value; ``_make_data`` used to pass it straight to ``make``, + which concatenated it onto the header bytes and raised ``TypeError``. + The response with a body is a regression guard: it round-tripped before + the fix and must keep doing so. + + """ + import io + + from pcapkit.protocols.application.httpv1 import HTTP as HTTPv1 + + cases = { + 'request with headers': (b'GET / HTTP/1.1\r\nHost: a\r\n\r\n', True), + 'headerless request': (b'GET / HTTP/1.1\r\n\r\n', True), + 'response with headers': (b'HTTP/1.1 204 No Content\r\nServer: x\r\n\r\n', True), + 'response with a body': (b'HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nhi', False), + } + for name, (raw, empty) in cases.items(): + with self.subTest(name): + parsed = HTTPv1(io.BytesIO(raw), len(raw)) + if empty: + self.assertIsNone(parsed.info.body) + rebuilt = HTTPv1.from_data(parsed.info) + self.assertEqual(rebuilt.data, raw) + self.assertEqual(rebuilt.info.body, parsed.info.body) + 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).