From 26f9bce347afde585d9786b62c5d3e0940e90a88 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 28 Oct 2026 00:06:52 -0400 Subject: [PATCH] fix(protocols): four more maker guards name the rejected argument Follow-up to #2405, which left these out of #2296's checklist. #2406. - hopopt.py / ipv6_opts.py: _make_opt_pad's Pad1-length warning, matching MH._make_opt_pad's text from #2405 (alias swapped). - httpv2.py: HTTP.make's unsupported-frame-type guard. - pcapng.py: PCAPNG.make's unsupported-block-type guard (the dispatcher, not _make_block_dsb, which #2405 already fixed). Added the missing Raises: docstring section on the two make() guards, matching #2405's MH.make/_make_block_dsb docstrings. Test: tests/protocols/test_maker_invalid_format_messages_2406_unit.py (new, 9 tests/18 subtests), plus a ReadersKeepInvalidFormat pin that _read_opt_pad on both protocols still says "invalid format" -- the exact twin of the maker guard changed here. Confirmed fail-without-fix: 12 failures against unpatched pcapkit, each a message-text mismatch. tests/protocols/ in full: 2020 passed/51388 subtests/0 failed. mypy and pylint findings on the 4 touched files are unchanged from main. --- pcapkit/protocols/application/httpv2.py | 9 +- pcapkit/protocols/internet/hopopt.py | 3 +- pcapkit/protocols/internet/ipv6_opts.py | 3 +- pcapkit/protocols/misc/pcapng.py | 9 +- ...maker_invalid_format_messages_2406_unit.py | 247 ++++++++++++++++++ 5 files changed, 267 insertions(+), 4 deletions(-) create mode 100644 tests/protocols/test_maker_invalid_format_messages_2406_unit.py diff --git a/pcapkit/protocols/application/httpv2.py b/pcapkit/protocols/application/httpv2.py index d978dff39..e2575d5e1 100644 --- a/pcapkit/protocols/application/httpv2.py +++ b/pcapkit/protocols/application/httpv2.py @@ -359,6 +359,12 @@ def make(self, # type: ignore[override] Returns: Constructed packet data. + Raises: + ProtocolError: If ``frame`` is none of the forms this maker reads -- + raw :obj:`bytes`, a :obj:`dict` of frame arguments, an + :class:`~pcapkit.protocols.data.application.httpv2.HTTP` data + model or a :class:`~pcapkit.protocols.schema.schema.Schema`. + .. versionchanged:: 1.5.0 * ``reserved`` is a new parameter before ``sid``, so a 1.4.x @@ -400,7 +406,8 @@ def make(self, # type: ignore[override] elif isinstance(frame, Schema): frame_val = frame else: - raise ProtocolError(f'HTTP/2: [Type {type_val}] invalid format') + raise ProtocolError(f'HTTP/2: [Type {type_val}] invalid frame: must be bytes, a dict, ' + f'an HTTP data model or a Schema, not {frame!r}') flags_val = {} # type: dict[str, int] for bit in range(8): diff --git a/pcapkit/protocols/internet/hopopt.py b/pcapkit/protocols/internet/hopopt.py index 70d73ced7..6681df50c 100644 --- a/pcapkit/protocols/internet/hopopt.py +++ b/pcapkit/protocols/internet/hopopt.py @@ -1536,7 +1536,8 @@ def _make_opt_pad(self, code: 'Enum_Option', opt: 'Optional[Data_PadOption]' = N pad = getattr(opt, 'pad', b'') if code == Enum_Option.Pad1 and length != 0: - warn(f'{self.alias}: [OptNo {code}] invalid format', ProtocolWarning) + warn(f'{self.alias}: [OptNo {code}] invalid length: must be 0 for a Pad1 option, ' + f'not {length!r}; making a PadN', ProtocolWarning) code = Enum_Option.PadN # type: ignore[assignment] return Schema_PadOption( diff --git a/pcapkit/protocols/internet/ipv6_opts.py b/pcapkit/protocols/internet/ipv6_opts.py index 0c51639ed..dca1af49a 100644 --- a/pcapkit/protocols/internet/ipv6_opts.py +++ b/pcapkit/protocols/internet/ipv6_opts.py @@ -1524,7 +1524,8 @@ def _make_opt_pad(self, code: 'Enum_Option', opt: 'Optional[Data_PadOption]' = N pad = getattr(opt, 'pad', b'') if code == Enum_Option.Pad1 and length != 0: - warn(f'{self.alias}: [OptNo {code}] invalid format', ProtocolWarning) + warn(f'{self.alias}: [OptNo {code}] invalid length: must be 0 for a Pad1 option, ' + f'not {length!r}; making a PadN', ProtocolWarning) code = Enum_Option.PadN # type: ignore[assignment] return Schema_PadOption( diff --git a/pcapkit/protocols/misc/pcapng.py b/pcapkit/protocols/misc/pcapng.py index 07acb5b0e..634c558e3 100644 --- a/pcapkit/protocols/misc/pcapng.py +++ b/pcapkit/protocols/misc/pcapng.py @@ -1490,6 +1490,12 @@ def make(self, Returns: Constructed packet data. + Raises: + ProtocolError: If ``block`` is none of the forms this maker reads -- + raw :obj:`bytes`, a :obj:`dict` of block arguments, a + :class:`~pcapkit.protocols.data.misc.pcapng.PCAPNG` data model + or a :class:`~pcapkit.protocols.schema.schema.Schema`. + """ type_val = cast('Enum_BlockType', self._make_index(type, type_default, namespace=type_namespace, @@ -1535,7 +1541,8 @@ def make(self, elif isinstance(block, Schema): block_val = block else: - raise ProtocolError(f'PCAP-NG: [Type {type_val}] invalid format') + raise ProtocolError(f'PCAP-NG: [Type {type_val}] invalid block: must be bytes, a dict, ' + f'a PCAPNG data model or a Schema, not {block!r}') return Schema_PCAPNG( type=type_val, diff --git a/tests/protocols/test_maker_invalid_format_messages_2406_unit.py b/tests/protocols/test_maker_invalid_format_messages_2406_unit.py new file mode 100644 index 000000000..aada9efb8 --- /dev/null +++ b/tests/protocols/test_maker_invalid_format_messages_2406_unit.py @@ -0,0 +1,247 @@ +# -*- coding: utf-8 -*- +"""GitHub issue #2406: the makers #2296 did not list name their argument. + +#2296 swept ``_make_*`` guards off the *reader's* text -- +``: [... N] invalid format`` -- and onto the house form established by +#2232 and #2257, ``: : must be X, not {value!r}``. +The maker-side sites below were outside its checklist and kept the generic +wording: + +* :meth:`HOPOPT._make_opt_pad + ` and + :meth:`IPv6_Opts._make_opt_pad + `, the twins of + the ``Pad1`` length warning #2296 fixed on :class:`~pcapkit.protocols.internet.mh.MH`; +* :meth:`HTTP.make ` and + :meth:`PCAPNG.make `, the + dispatchers whose unsupported-``frame``/``block`` guard is the same shape as + ``MH.make``'s. + +The parse side is deliberately untouched, as in #2296: ``invalid format`` is the +right thing for a reader to say. :class:`ReadersKeepInvalidFormat` below pins +that division on the one pair where the reader and the maker guard the very same +thing -- ``_read_opt_pad`` and ``_make_opt_pad`` both reject a ``Pad1`` carrying +a non-zero length. + +Every message is pinned whole, and every maker's valid input is exercised in the +same class, so a guard that starts refusing good input regresses here too. + +:mod:`pcapkit` is imported inside each test, after +:func:`~tests._support.reimport_once_per_class`. + +""" + +from __future__ import annotations + +import unittest +import warnings +from typing import TYPE_CHECKING + +from tests._support import reimport_once_per_class + +if TYPE_CHECKING: + from typing import Any + + +class MakerMessageTestCase(unittest.TestCase): + """Re-import :mod:`pcapkit` once per class, as the suite's convention asks.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def protocol(self, cls: 'Any') -> 'Any': + """An uninitialised protocol instance, for calling a maker directly. + + The makers under test read nothing off ``self`` but ``alias``, so there + is no packet to construct first -- and constructing one would route the + bad argument through a schema that raises its own error instead. + + """ + return object.__new__(cls) + + +class IPv6ExtensionPadOptionWarnings(MakerMessageTestCase): + """``_make_opt_pad`` warns about ``length``, not about a format. + + HOPOPT and IPv6-Opts carry byte-identical copies of the option machinery, + down to the ``Pad1`` guard, so both are driven from one table: the only + thing that differs in the message is the alias each protocol reports. + + """ + + def protocols(self) -> 'list[tuple[Any, str]]': + """Each padding-option maker, paired with the alias it names itself by.""" + from pcapkit.protocols.internet.hopopt import HOPOPT + from pcapkit.protocols.internet.ipv6_opts import IPv6_Opts + + return [(HOPOPT, 'HOPOPT'), (IPv6_Opts, 'IPv6-Opts')] + + def test_pad1_with_a_length_warns_by_name(self) -> None: + from pcapkit.const.ipv6.option import Option + from pcapkit.utilities.warnings import ProtocolWarning + + code = Option.Pad1 + for cls, alias in self.protocols(): + protocol = self.protocol(cls) + for length in (1, 3): + with self.subTest(protocol=alias, length=length): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + schema = protocol._make_opt_pad(code, length=length) + messages = [str(w.message) for w in caught + if issubclass(w.category, ProtocolWarning)] + self.assertEqual(len(messages), 1, messages) + self.assertEqual( + messages[0], + f'{alias}: [OptNo {code}] invalid length: ' + f'must be 0 for a Pad1 option, not {length!r}; making a PadN') + self.assertEqual(schema.type, Option.PadN) + + def test_pad1_with_no_length_does_not_warn(self) -> None: + from pcapkit.const.ipv6.option import Option + from pcapkit.utilities.warnings import ProtocolWarning + + for cls, alias in self.protocols(): + protocol = self.protocol(cls) + with self.subTest(protocol=alias): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + schema = protocol._make_opt_pad(Option.Pad1, length=0) + self.assertEqual([str(w.message) for w in caught + if issubclass(w.category, ProtocolWarning)], []) + self.assertEqual(schema.type, Option.Pad1) + + def test_padn_with_a_length_still_builds(self) -> None: + from pcapkit.const.ipv6.option import Option + from pcapkit.utilities.warnings import ProtocolWarning + + for cls, alias in self.protocols(): + protocol = self.protocol(cls) + with self.subTest(protocol=alias): + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + schema = protocol._make_opt_pad(Option.PadN, length=2, pad=b'\x00\x00') + self.assertEqual([str(w.message) for w in caught + if issubclass(w.category, ProtocolWarning)], []) + self.assertEqual(schema.type, Option.PadN) + self.assertEqual(schema.len, 2) + self.assertEqual(schema.pad, b'\x00\x00') + + +class HTTPFrameMessage(MakerMessageTestCase): + """``HTTP.make`` names ``frame`` and the forms it reads.""" + + def test_unreadable_frame_names_the_argument(self) -> None: + from pcapkit.const.http.frame import Frame + from pcapkit.protocols.application.httpv2 import HTTP + from pcapkit.utilities.exceptions import ProtocolError + + http = self.protocol(HTTP) + code = Frame.DATA + for frame in (5, 1.5, 'frame', []): + with self.subTest(frame=frame): + with self.assertRaises(ProtocolError) as context: + http.make(type=code, frame=frame) + self.assertEqual( + str(context.exception), + f'HTTP/2: [Type {code}] invalid frame: must be bytes, a dict, ' + f'an HTTP data model or a Schema, not {frame!r}') + + def test_raw_frame_still_builds(self) -> None: + from pcapkit.const.http.frame import Frame + from pcapkit.protocols.application.httpv2 import HTTP + + http = self.protocol(HTTP) + schema = http.make(type=Frame.DATA, frame=b'\x01\x02') + self.assertEqual(schema.frame, b'\x01\x02') + self.assertEqual(schema.length, 2) + + def test_an_omitted_frame_is_not_rejected(self) -> None: + from pcapkit.const.http.frame import Frame + from pcapkit.protocols.application.httpv2 import HTTP + + # ``frame=None`` dispatches to the frame type's own constructor (#2274) + # rather than reaching the guard, so the guard has not widened to it. + http = self.protocol(HTTP) + self.assertEqual(http.make(type=Frame.DATA, frame=None).type, Frame.DATA) + + +class PCAPNGBlockMessage(MakerMessageTestCase): + """``PCAPNG.make`` names ``block`` and the forms it reads.""" + + def protocol(self, cls: 'Any') -> 'Any': + """As the base helper, but with the section context the dispatcher reads. + + ``PCAPNG.make`` settles the byte order before handing the block over, + and reads :attr:`PCAPNG._ctx` to do it. :data:`None` is what an + out-of-capture construction carries, and sends it to the default + ``__packet__`` byte order. + + """ + protocol = super().protocol(cls) + protocol._ctx = None + return protocol + + def test_unreadable_block_names_the_argument(self) -> None: + from pcapkit.const.pcapng.block_type import BlockType + from pcapkit.protocols.misc.pcapng import PCAPNG + from pcapkit.utilities.exceptions import ProtocolError + + pcapng = self.protocol(PCAPNG) + code = BlockType.Enhanced_Packet_Block + for block in (5, None, 1.5, 'block'): + with self.subTest(block=block): + with self.assertRaises(ProtocolError) as context: + pcapng.make(type=code, block=block) + self.assertEqual( + str(context.exception), + f'PCAP-NG: [Type {code}] invalid block: must be bytes, a dict, ' + f'a PCAPNG data model or a Schema, not {block!r}') + + def test_raw_block_still_builds(self) -> None: + from pcapkit.const.pcapng.block_type import BlockType + from pcapkit.protocols.misc.pcapng import PCAPNG + + pcapng = self.protocol(PCAPNG) + schema = pcapng.make(type=BlockType.Enhanced_Packet_Block, block=b'\x00\x01') + self.assertEqual(schema.type, BlockType.Enhanced_Packet_Block) + self.assertEqual(schema.block, b'\x00\x01') + + +class ReadersKeepInvalidFormat(MakerMessageTestCase): + """The parse side still says ``invalid format``, as #2296 asks. + + ``_read_opt_pad`` and ``_make_opt_pad`` reject the same thing -- a ``Pad1`` + option carrying a non-zero length -- so the pair is where a sweep over the + makers is most likely to catch a reader by accident. + + """ + + def readers(self) -> 'list[tuple[Any, Any, str]]': + """Each padding-option reader, its schema class and its alias.""" + from pcapkit.protocols.internet.hopopt import HOPOPT + from pcapkit.protocols.internet.ipv6_opts import IPv6_Opts + from pcapkit.protocols.schema.internet.hopopt import PadOption as Schema_HOPOPTPad + from pcapkit.protocols.schema.internet.ipv6_opts import PadOption as Schema_IPv6OptsPad + + return [(HOPOPT, Schema_HOPOPTPad, 'HOPOPT'), + (IPv6_Opts, Schema_IPv6OptsPad, 'IPv6-Opts')] + + def test_the_reader_still_reports_an_invalid_format(self) -> None: + from pcapkit.const.ipv6.option import Option + from pcapkit.corekit.multidict import OrderedMultiDict + from pcapkit.utilities.exceptions import ProtocolError + + code = Option.Pad1 + for cls, schema_cls, alias in self.readers(): + protocol = self.protocol(cls) + with self.subTest(protocol=alias): + schema = schema_cls(type=code, len=3, pad=b'') + with self.assertRaises(ProtocolError) as context: + protocol._read_opt_pad(schema, options=OrderedMultiDict()) + self.assertEqual(str(context.exception), + f'{alias}: [OptNo {code}] invalid format') + + +if __name__ == '__main__': + unittest.main()