From 0e6e5f3cfe12ea716d9356ee809f989b10f7d127 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 5 Oct 2026 01:52:51 -0400 Subject: [PATCH] fix(protocols): let Application carry an undissected remainder (#719) `Application` forbade payload dispatch outright, which is stricter than the invariant it means to encode. Ruled on #719. * `_decode_next_layer` and `_import_next_layer` raised `UnsupportedCall` unconditionally. They now accept the `-1` sentinel -- the rest of the packet, undissected -- and delegate to `ProtocolBase`, which resolves it to `Raw`, or to `NoPayload` when nothing remains. Any other `proto` is still refused, and the message now names the protocol number rather than claiming the attribute does not exist. * `__post_init__` unconditionally overwrote `_next` with `NoPayload()` and rebuilt `_protos` after `read()`, discarding whatever a dispatch had just set. It now fills them only when `read` left `_next` unset. * The class docstring said "transport layer protocol family" and said nothing about the dispatch contract. Both corrected. The invariant is "no further *protocol* layer above this one". Undissected trailer bytes are not a protocol layer, so refusing `-1` was refusing something the invariant permits -- and it is what blocked a functionally application-layer protocol that still has a trailer from naming `Application` as its base at all. Breaking in two ways a third party can observe. A subclass calling either method with `-1` changes from raising to succeeding; and a subclass whose `read` assigns `_next` itself used to have it overwritten with `NoPayload` after `read()` returned, and now keeps it. No subclass in the package does either -- the five are `FTP`, `HTTP`, `HTTPv1`, `HTTPv2` and `NGAP`, none of which overrides the two dispatch methods or `__post_init__` -- so nothing in the tree changes behaviour. `_import_next_layer` keeps its `# type: ignore[override]` and its `super()` call gains `[call-arg,misc]`. The base is wrapped in `@beholder`, which mypy sees as a no-argument method, so both are load-bearing rather than decorative; `protocol.py:598` carries the same pair for the same reason. Dropping the override ignore drew five new errors in this file. `_decode_next_layer` needs none: widening `proto` to `Optional[int]` is LSP-legal. Measured -- mypy on this file reports zero errors on both this change and main, against 305 elsewhere either way. New test pins both halves. Three of its five cases fail on main -- the two sentinel dispatches and the `_import_next_layer` acceptance -- and the other two are regression guards on what must not change: a real protocol number is still refused, and an application protocol that never dispatches still ends in `NoPayload`. tests/protocols/application + tests/project: 399 passed, 1 skipped, 1305 subtests passed. --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 9 ++ .../contributing/conventions/process.rst | 2 +- pcapkit/protocols/application/application.py | 66 ++++++++-- .../test_application_dispatch_unit.py | 123 ++++++++++++++++++ 5 files changed, 188 insertions(+), 13 deletions(-) create mode 100644 tests/protocols/application/test_application_dispatch_unit.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 96bcc9c472..6a8e43c59a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -122,6 +122,7 @@ Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) an - renames with no compatibility alias left behind: PCAP-NG `Option` subclasses spell the namespace class keyword `ns=` instead of `namespace=` ([#439](https://github.com/JarryShaw/PyPCAPKit/issues/439)). - `tests/protocols/transport/test_tcp_udp_unit.py` now reaches the MP_JOIN dispatchers through `TCP()` itself, instead of assigning a Python `set` to `_flags` on a bare `TCP.__new__(TCP)`. A `set` answers the membership tests `_make_mptcp_join` and `_read_mptcp_join` use, so every flag branch ran and both TCP modules read 100% coverage, while the attribute had neither the `aenum.IntFlag` type production assigns nor the ordering that governs when it exists -- how [#587](https://github.com/JarryShaw/PyPCAPKit/issues/587) stayed invisible behind that number, and the `cast('Enum_Flags', 0)` no-op behind it. Reverting [#587](https://github.com/JarryShaw/PyPCAPKit/issues/587)'s hoist now fails two of the file's 17 tests with `AttributeError: 'TCP' object has no attribute '_flags'`, and restoring the `cast` fails two with `TypeError: argument of type 'int' is not a container or iterable`; all 17 passed before. The library is unchanged and coverage does not move -- the point is what the same numbers are now worth ([#603](https://github.com/JarryShaw/PyPCAPKit/issues/603)). - building a protocol through its constructor with a keyword that names nothing now raises `UnsupportedCall` instead of discarding it. **This is a behaviour change to a public API**: every `make` in the tree ends its signature with `**kwargs` and reads nothing out of it, so a misspelled keyword was accepted, dropped, and the field it named kept its default -- wrong octets, nothing said. That is what [#602](https://github.com/JarryShaw/PyPCAPKit/issues/602) cost: `examples/generators/options.py` asked for `seq=1` where `TCP.make` spells it `seq_no`, and 25 generated fixture frames carried sequence number `0`. [#541](https://github.com/JarryShaw/PyPCAPKit/issues/541) and [#556](https://github.com/JarryShaw/PyPCAPKit/issues/556) were the same silence. The schema layer was never so permissive (`Schema.__update__` warns `UnknownFieldWarning`), and that asymmetry is what this closes. The check sits in `ProtocolBase.__init__` rather than `make`, because `__post_init__` passes one `**kwargs` to the construction *and* to the parse of what it has just constructed, so a keyword declared only by `read` legitimately travels through `make` (`HIP.read` declares `extension`, which `HIP.make` does not, and the option generator depends on it). The accepted set is the union of every keyword-taking parameter of `make`, `read`, `pack`, `unpack`, `__post_init__` and `__init__` across the MRO, computed once per class from `inspect.signature`. Parsing is deliberately untouched: there the keywords are whatever the engines and the four `_import_next_layer` implementations forward, a protocol cannot know which its parent passed on, and a dropped parse keyword changes how a packet is read, not what its octets say. Two opt-in escapes exist per class through a new `__keywords__`: a set, for a keyword read out of `**kwargs` by name (`ESP.read` with `packet`); and `None`, for a dispatcher whose real signature belongs to a class chosen at call time (only `HTTP.make` forwarding to `HTTPv1`/`HTTPv2`). The message names the near neighbour (`seq` reports *did you mean 'seq_no'?*). `from_data` warns `UnknownFieldWarning` rather than raising, because its keywords are whatever `_make_data` returned, so the defect is a key of that mapping disagreeing with the signature it is spread into, and the person who meets it cannot fix it. This surfaced four latent bugs in this repository, all residue of [#602](https://github.com/JarryShaw/PyPCAPKit/issues/602) and fixed here: the `_TCP_BASE` of `examples/generators/dispatch.py` and three stale copies under `tests/protocols/transport/`, each building segments with sequence number `0`. Three more are reported, not fixed, being a defect per protocol: `Frame._make_data` returns `ts_src` where `make` declares `ts_sec`, `L2TPv2._make_data` returns `prio` where it declares `priority`, and `Header._make_data` returns a `magic_number` that `Header.make` does not take, so `from_data` has been dropping a frame's timestamp, an L2TPv2 priority bit and a capture's byte order, and now says so. One limitation: a *direct* `SomeProtocol.make(...)` call is not checked and still discards silently, since the check sits where every producer's keywords converge rather than inside each of the 30 `make` implementations; `object.__new__(cls).make(**kwargs)`, the idiom `HTTP.make` uses, reaches it ([#617](https://github.com/JarryShaw/PyPCAPKit/issues/617)). +- **a breaking change to** `Application`: it no longer forbids payload dispatch outright. It encodes "no further protocol layer above this one", which undissected trailer bytes are not, so `_decode_next_layer` and `_import_next_layer` accept the `-1` sentinel -- the rest of the packet, resolving to `Raw`, or to `NoPayload` when nothing remains -- and `__post_init__` keeps a payload that `read` set instead of overwriting it with `NoPayload`. A real protocol number (any `proto` other than `-1`) is still refused with `UnsupportedCall`. A subclass that calls either method with `-1` changes from raising to succeeding; no subclass in the package does ([#719](https://github.com/JarryShaw/PyPCAPKit/issues/719)). #### Fixed diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 9b625a04ba..7f0a387c1f 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -1187,6 +1187,15 @@ Changed converge rather than inside each of the 30 ``make`` implementations; ``object.__new__(cls).make(**kwargs)``, the idiom ``HTTP.make`` uses, reaches it (:issue:`617`). +* **a breaking change to** ``Application``: it no longer forbids payload dispatch + outright. It encodes "no further protocol layer above this one", which undissected + trailer bytes are not, so ``_decode_next_layer`` and ``_import_next_layer`` accept + the ``-1`` sentinel -- the rest of the packet, resolving to ``Raw``, or to + ``NoPayload`` when nothing remains -- and ``__post_init__`` keeps a payload that + ``read`` set instead of overwriting it with ``NoPayload``. A real protocol number + (any ``proto`` other than ``-1``) is still refused with ``UnsupportedCall``. A + subclass that calls either method with ``-1`` changes from raising to succeeding; + no subclass in the package does (:issue:`719`). Fixed ~~~~~ diff --git a/docs/source/contributing/conventions/process.rst b/docs/source/contributing/conventions/process.rst index 32eb9444a5..83a1eff846 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 -158 entries, and no entry carries an inline kind label:: +159 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/application.py b/pcapkit/protocols/application/application.py index ac55744a09..b90cadfd50 100644 --- a/pcapkit/protocols/application/application.py +++ b/pcapkit/protocols/application/application.py @@ -28,7 +28,17 @@ class Application(ProtocolBase[_PT, _ST], Generic[_PT, _ST]): # pylint: disable=abstract-method - """Abstract base class for transport layer protocol family.""" + """Abstract base class for application layer protocol family. + + An application layer protocol has no further *protocol* layer above it, so + :meth:`_decode_next_layer` and :meth:`_import_next_layer` refuse to dispatch + on a protocol number. What they do permit is the ``-1`` sentinel, which asks + for the rest of the packet undissected and resolves to + :class:`~pcapkit.protocols.misc.raw.Raw` -- trailing bytes are not a further + protocol layer -- or to :class:`~pcapkit.protocols.misc.null.NoPayload` when + nothing remains. + + """ ########################################################################## # Defaults. @@ -73,10 +83,14 @@ def __post_init__(self, file: 'Optional[IO[bytes] | bytes]' = None, # call super post-init super().__post_init__(file, length, **kwargs) # type: ignore[arg-type] - #: pcapkit.protocols.null.NoPayload: Payload of current instance. - self._next = NoPayload() - #: pcapkit.corekit.protochain.ProtoChain: Protocol chain of current instance. - self._protos = ProtoChain(self.__class__, self.alias) + # ``read`` may have dispatched the undissected remainder through + # ``_decode_next_layer``, which already set the payload and the chain + # (basis included); only a protocol that did not gets the empty default + if getattr(self, '_next', None) is None: + #: pcapkit.protocols.null.NoPayload: Payload of current instance. + self._next = NoPayload() + #: pcapkit.corekit.protochain.ProtoChain: Protocol chain of current instance. + self._protos = ProtoChain(self.__class__, self.alias) @classmethod def __index__(cls) -> 'NoReturn': # pylint: disable=invalid-index-returned @@ -93,21 +107,49 @@ def __index__(cls) -> 'NoReturn': # pylint: disable=invalid-index-returned ########################################################################## def _decode_next_layer(self, dict_: '_PT', proto: 'Optional[int]' = None, length: 'Optional[int]' = None, *, - packet: 'Optional[dict[str, Any]]' = None) -> 'NoReturn': - """Decode next layer protocol. + packet: 'Optional[dict[str, Any]]' = None) -> '_PT': + r"""Decode next layer protocol. + + Arguments: + dict\_: info buffer + proto: next layer protocol index; only the ``-1`` sentinel + (the rest, undissected) is accepted + length: valid (*non-padding*) length + packet: packet info (passed from :meth:`self.unpack `) + + Returns: + Current protocol with the undissected remainder as payload. Raises: - UnsupportedCall: This protocol doesn't support :meth:`_decode_next_layer`. + UnsupportedCall: ``proto`` is anything but ``-1``, i.e. a real + protocol dispatch, which this protocol doesn't support. """ - raise UnsupportedCall(f"'{self.__class__.__name__}' object has no attribute '_decode_next_layer'") + if proto != -1: + raise UnsupportedCall(f"'{self.__class__.__name__}' object cannot dispatch protocol {proto!r}; " + "only the undissected remainder (-1) is supported") + return super()._decode_next_layer(dict_, -1, length, packet=packet) def _import_next_layer(self, proto: 'int', length: 'Optional[int]' = None, *, # type: ignore[override] - packet: 'Optional[dict[str, Any]]' = None) -> 'NoReturn': + packet: 'Optional[dict[str, Any]]' = None) -> 'ProtocolBase': """Import next layer extractor. + Arguments: + proto: next layer protocol index; only the ``-1`` sentinel + (the rest, undissected) is accepted + length: valid (*non-padding*) length + packet: packet info (passed from :meth:`self.unpack `) + + Returns: + Instance of :class:`~pcapkit.protocols.misc.raw.Raw`, or of + :class:`~pcapkit.protocols.misc.null.NoPayload` when nothing remains. + Raises: - UnsupportedCall: This protocol doesn't support :meth:`_import_next_layer`. + UnsupportedCall: ``proto`` is anything but ``-1``, i.e. a real + protocol dispatch, which this protocol doesn't support. """ - raise UnsupportedCall(f"'{self.__class__.__name__}' object has no attribute '_import_next_layer'") + if proto != -1: + raise UnsupportedCall(f"'{self.__class__.__name__}' object cannot dispatch protocol {proto!r}; " + "only the undissected remainder (-1) is supported") + return super()._import_next_layer(-1, length, packet=packet) # type: ignore[call-arg,misc] diff --git a/tests/protocols/application/test_application_dispatch_unit.py b/tests/protocols/application/test_application_dispatch_unit.py new file mode 100644 index 0000000000..5331a9c1bf --- /dev/null +++ b/tests/protocols/application/test_application_dispatch_unit.py @@ -0,0 +1,123 @@ +"""Unit tests for payload dispatch on :class:`~pcapkit.protocols.application.application.Application`. + +:class:`Application` encodes "no further *protocol* layer above this one". That +forbids dispatching on a protocol number, but not handing the undissected rest +of the packet to :class:`~pcapkit.protocols.misc.raw.Raw`, which is what the +``-1`` sentinel asks for. The tests pin both halves: the sentinel is accepted +and resolves to ``Raw``, and a real protocol number is still refused. + +""" + +from __future__ import annotations + +import importlib.util +import unittest + +from tests._support import purge_modules + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class ApplicationDispatchUnitTests(unittest.TestCase): + def setUp(self) -> None: + purge_modules(['pcapkit']) + + @staticmethod + def _protocol_class(proto: int | None): + """An :class:`Application` whose ``read`` dispatches the rest on ``proto``, or not at all.""" + from pcapkit.corekit.fields.misc import PayloadField + from pcapkit.corekit.fields.strings import BytesField + from pcapkit.corekit.infoclass import info_final + from pcapkit.protocols.application.application import Application + from pcapkit.protocols.data.data import Data + from pcapkit.protocols.schema.schema import Schema, schema_final + + @info_final + class DummyData(Data): + value: int = 0 + + @schema_final + class DummySchema(Schema): + head: bytes = BytesField(length=2, default=b'') + payload: bytes = PayloadField(length=lambda packet: packet['__length__'] - 2, default=b'') + + class DummyApplication(Application[DummyData, DummySchema], + schema=DummySchema, data=DummyData): + @property + def name(self) -> str: + return 'Dummy Application' + + @property + def length(self) -> int: + return 2 + + def read(self, length: int | None = None, **kwargs: object) -> DummyData: + data = DummyData(value=1) + if proto is None: + return data + return self._decode_next_layer(data, proto, len(self._data) - 2) + + def make(self, packet: bytes = b'ab', **kwargs: object) -> DummySchema: + return DummySchema(head=packet[:2], payload=packet[2:]) + + return DummyApplication + + def test_sentinel_dispatches_remainder_to_raw(self) -> None: + from pcapkit.protocols.misc.raw import Raw + + proto = self._protocol_class(-1)(packet=b'abtrailer') + + self.assertEqual(proto.layer, 'Application') + self.assertIsInstance(proto.payload, Raw) + self.assertEqual(bytes(proto.payload), b'trailer') + self.assertIs(proto.info.__next_type__, Raw) + self.assertEqual(proto.info.__next_name__, 'raw') + # the chain keeps the dispatched layer rather than being reset + self.assertIn('Raw', str(proto.protochain)) + + def test_sentinel_with_nothing_left_is_no_payload(self) -> None: + from pcapkit.protocols.misc.null import NoPayload + + proto = self._protocol_class(-1)(packet=b'ab') + + self.assertIsInstance(proto.payload, NoPayload) + + def test_undispatched_application_still_has_no_payload(self) -> None: + from pcapkit.protocols.misc.null import NoPayload + + proto = self._protocol_class(None)(packet=b'abtrailer') + + self.assertIsInstance(proto.payload, NoPayload) + self.assertEqual(str(proto.protochain), 'DummyApplication') + + def test_real_protocol_number_is_still_refused(self) -> None: + from pcapkit.utilities.exceptions import UnsupportedCall + + for number in (0, 6, 17, 0x0800): + with self.subTest(proto=number): + with self.assertRaises(UnsupportedCall): + self._protocol_class(number)(packet=b'abtrailer') + + def test_import_next_layer_accepts_only_the_sentinel(self) -> None: + from pcapkit.protocols.misc.raw import Raw + from pcapkit.utilities.exceptions import UnsupportedCall + + proto = object.__new__(self._protocol_class(-1)) + proto._data = b'abtrailer' + proto._sigterm = False + proto._exlayer = proto._exproto = proto._exctx = None + proto._get_payload = lambda: b'trailer' # type: ignore[method-assign] + + self.assertIsInstance(proto._import_next_layer(-1, 7), Raw) + for number in (0, 6, 17, 0x0800): + with self.subTest(proto=number): + with self.assertRaises(UnsupportedCall): + proto._import_next_layer(number, 7) + with self.assertRaises(UnsupportedCall): + proto._decode_next_layer(None, number, 7) + + +if __name__ == '__main__': + unittest.main()