diff --git a/pcapkit/__main__.py b/pcapkit/__main__.py index cca9b09d6c..7ed084d554 100644 --- a/pcapkit/__main__.py +++ b/pcapkit/__main__.py @@ -77,9 +77,22 @@ def get_parser() -> 'ArgumentParser': help=('Indicate extraction engine. Note that except ' 'default or pcapkit engine, all other engines ' 'need support of corresponding packages.')) - parser.add_argument('-P', '--protocol', action='store', dest='protocol', default='null', metavar='PROTOCOL', + parser.add_argument('-P', '--protocol', action='store', dest='protocol', default=None, metavar='PROTOCOL', help='Indicate extraction stops after which protocol.') - parser.add_argument('-L', '--layer', action='store', dest='layer', default='None', metavar='LAYER', + # NOTE: ``choices`` rather than a free-form string, because the layer names + # are a closed set (``pcapkit.foundation.extraction.Layers``) and a name + # outside it is not rejected anywhere downstream -- it simply never matches a + # protocol's ``__layer__`` and the parse silently runs to the top of the + # stack, which is the failure mode GH-356 was about. ``type=str.lower`` keeps + # ``-L Internet`` working, as it did before the choices were declared, since + # ``Extractor.__init__`` lowercases the value anyway. + # + # The defaults are :data:`None` and not the ``'None'``/``'null'`` strings they + # used to be: ``Extractor.__init__`` substitutes its own sentinels for an + # omitted value, so passing the strings only worked because it happened to + # lowercase ``'None'`` into the sentinel it wanted. + parser.add_argument('-L', '--layer', action='store', dest='layer', default=None, metavar='LAYER', + type=str.lower, choices=['link', 'internet', 'transport', 'application', 'none'], help='Indicate extract frames until which layer.') parser.add_argument('-B', '--buffer-save', action='store_true', default=False, help='Indicate if store buffer to file when reading from stdin.') diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index 47deb1b146..bf20f34742 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -526,8 +526,18 @@ def __init__(self, file: 'Optional[IO[bytes] | bytes]' = None, length: 'Optional length: Length of packet data. _layer (str): Parse packet until ``_layer`` (:attr:`self._exlayer `). + While parsing, the un-prefixed ``layer`` is accepted as well -- + see the note below. _protocol (Union[str, Protocol, Type[Protocol]]): Parse packet until ``_protocol`` (:attr:`self._exproto `). + While parsing, the un-prefixed ``protocol`` is accepted as well -- + see the note below. + packet (dict[str, Any]): Packet context of the enclosing layer, as + handed over by + :meth:`self._import_next_layer `. + While parsing, it is republished as ``__packet__`` so that + :meth:`self.unpack ` -- and through it the + schema -- can see it; see the note below. __context__ (Union[ContextRegistry, ProtocolContext, Mapping[str, ProtocolContext], Iterable[ProtocolContext]]): Caller supplied parsing context (:attr:`self._exctx `), c.f. :mod:`pcapkit.corekit.context`. It is consumed here rather @@ -536,15 +546,69 @@ def __init__(self, file: 'Optional[IO[bytes] | bytes]' = None, length: 'Optional :meth:`self._import_next_layer `. **kwargs: Arbitrary keyword arguments. + Note: + Three of the keywords above are *out-of-band*: they configure the + parse rather than describing the packet, and every one of them is + consumed here, at the one point each of a protocol's producers passes + through. That is deliberate, and it is what the normalisation below + relies on -- fixing a spelling here fixes it for the engines, for all + four :meth:`_import_next_layer ` + implementations, and for any third party protocol that copied their + shape, rather than one call site at a time. + """ #logger.debug('%s(file, %s, **%s)', type(self).__name__, length, kwargs) + # Whether this instantiation parses an existing packet, as opposed to + # constructing a new one. ``file`` is the discriminator the rest of this + # method already turns on: ``__post_init__`` reads the stream when there + # is one and calls ``self.pack(**kwargs)`` when there is not. It matters + # below because ``layer``, ``protocol`` and ``packet`` are out-of-band + # only while parsing -- on the construction path they are ordinary + # ``make()`` arguments, and consuming them there would silently drop the + # value being constructed. + parsing = file is not None + #: int: File pointer. self._seekset = io.SEEK_SET # type: int #: str: Parse packet until such layer. self._exlayer = kwargs.pop('_layer', None) # type: Optional[str] #: str: Parse packet until such protocol. self._exproto = kwargs.pop('_protocol', None) # type: Optional[str | ProtocolBase | Type[ProtocolBase]] + + # NOTE: The parse limits are documented here as ``_layer`` and + # ``_protocol``, but no producer in the tree spells them that way. The + # engines build the outermost protocol with ``layer=``/``protocol=`` + # (``pcapkit.foundation.engines.pcap.PCAP.read_frame`` and + # ``pcapkit.foundation.engines.pcapng.PCAPNG.read_frame``), every + # ``_import_next_layer`` recurses into the next one the same way, and the + # un-prefixed pair is also the public spelling that + # ``pcapkit.extract(layer=..., protocol=...)`` and the CLI's ``-L``/``-P`` + # use. Both were therefore dropped into ``**kwargs`` and ignored, so + # neither option did anything at all; see GH-356. Accepting both + # spellings is what makes them work, and the prefixed one still wins so + # that a caller which reads this docstring is not overridden by a limit + # its parent happened to be forwarding. + if parsing: + layer = kwargs.pop('layer', None) + protocol = kwargs.pop('protocol', None) + if self._exlayer is None: + self._exlayer = layer + if self._exproto is None: + self._exproto = protocol + + # NOTE: ``Extractor.__init__`` substitutes the strings ``'none'`` and + # ``'null'`` for an omitted ``layer``/``protocol`` + # (``pcapkit.foundation.extraction.Extractor.__init__``), and + # ``pcapkit.interface.core.extract`` does the same for ``layer``. They are + # sentinels meaning "no limit", so recognise them as such instead of + # carrying them into ``_check_term_threshold`` on every protocol of every + # packet, where they would be compared against real protocol names. + if isinstance(self._exlayer, str) and self._exlayer.lower() == 'none': + self._exlayer = None + if isinstance(self._exproto, str) and self._exproto.lower() == 'null': + self._exproto = None + #: pcapkit.corekit.context.ContextRegistry: Caller supplied parsing context. # NOTE: Every nested layer normalises the context it was handed, so an # already-normalised registry is adopted as-is: ``make()`` copies, and @@ -556,6 +620,28 @@ def __init__(self, file: 'Optional[IO[bytes] | bytes]' = None, length: 'Optional #: bool: If terminate parsing next layer of protocol. self._sigterm = self._check_term_threshold() + # NOTE: The enclosing layer's packet context arrives as ``packet=`` -- the + # spelling ``_import_next_layer`` uses -- but the schema layer reads it + # from ``__packet__`` (``self.unpack`` below, and the ``pack``/``unpack`` + # overrides of ``Frame`` and ``PCAPNG``). Nothing bridged the two, so a + # schema's ``unpack``/``post_process`` always saw an empty dict however + # much the outer layer had put in it: an ``IPv6`` source address never + # reached the HOPOPT MPL option that RFC 7731 elides from the wire, and a + # destination address never reached the RPL source route header that + # RFC 6554 needs it to decompress. Republish it here, for the same reason + # the limits above are normalised here. See GH-382. + # + # A copy rather than the dict itself: ``Schema.unpack`` writes every field + # it reads into the context it is given, plus its own ``__length__`` and + # ``__option_padding__`` bookkeeping, and the IPv6 extension header walk + # hands one dict to each header in turn. Sharing it would leave one + # header's fields visible to the next, where a ``ConditionalField`` test + # or a length callback could read a sibling's stale value instead of + # failing. ``Schema.unpack`` already isolates its own per-field contexts + # the same way. + if parsing and '__packet__' not in kwargs and isinstance(kwargs.get('packet'), dict): + kwargs['__packet__'] = dict(kwargs['packet']) + # post-init customisations self.__post_init__(file, length, **kwargs) # type: ignore[arg-type] diff --git a/pcapkit/utilities/decorators.py b/pcapkit/utilities/decorators.py index 62a6f9d23d..f170fb732b 100644 --- a/pcapkit/utilities/decorators.py +++ b/pcapkit/utilities/decorators.py @@ -198,5 +198,34 @@ def unpack(*args: 'P.args', **kwargs: 'P.kwargs') -> 'R_prepare': schema = func(cls, data, length, packet) ret = schema.post_process(packet) + # NOTE: ``Schema.unpack`` clears ``__updated__`` before returning, but + # ``post_process`` runs after it and assigns fields -- and every field + # assignment sets the flag again (``Schema.__setattr__``). The schema is + # then left marked as needing a re-pack even though its ``__buffer__`` + # already holds the octets just read off the wire, so the next + # ``bytes(schema)`` or ``len(schema)`` silently re-packs it. + # + # That re-pack is not merely wasted work: ``Schema.pack`` calls + # ``post_process`` a *second* time, with a packet context rebuilt from + # the schema's own fields and therefore holding none of the enclosing + # layer's, so it overwrites exactly the values ``post_process`` derived + # from that context. It is what discarded the IPv6 source address that + # ``pcapkit.protocols.schema.internet.hopopt.MPLOption.post_process`` + # had just resolved: ``OptionField.unpack`` measures each parsed option + # with ``len(data)``, which triggered the re-pack one option later. + # + # ``Schema.pack`` already orders the two the other way round -- clear the + # flag *after* ``post_process``, not before -- so match it here and the + # revision made while unpacking survives. + # + # Guarded rather than assigned outright because ``post_process`` may hand + # back something other than a schema: an implementation is free to return + # a nested one instead of ``self``, as + # ``pcapkit.protocols.schema.internet.hopopt._SMFDPDOption`` does, and the + # decorator is also applied to stand-ins in the test suite that return + # the packet mapping. Only a real schema carries the flag. + if hasattr(ret, '__updated__'): + ret.__updated__ = False + return cast('R_prepare', ret) return unpack diff --git a/tests/cli/test_main.py b/tests/cli/test_main.py index 2160db7403..8d673a17fb 100644 --- a/tests/cli/test_main.py +++ b/tests/cli/test_main.py @@ -87,6 +87,68 @@ def test_get_parser_parses_expected_arguments(self) -> None: self.assertTrue(args.json) self.assertTrue(args.files) + def test_layer_and_protocol_reach_the_extractor(self) -> None: + """``-L``/``-P`` are forwarded under the names ``Extractor`` reads. + + The CLI half of GH-356. The forwarding was always correct -- it is the + core that dropped the limits -- so this pins the contract that made it + correct: ``Extractor`` takes ``layer=`` and ``protocol=``, and the CLI + must not invent its own spelling for either. + + """ + emoji = types.SimpleNamespace(emojize=lambda text: text) + module, extractor_cls, _ = self._load_cli_module(emoji_module=emoji) + + with mock.patch.object(sys, 'argv', ['pcapkit-cli', 'capture.pcap', + '-L', 'internet', '-P', 'TCP']): + self.assertEqual(module.main(), 0) + + created = extractor_cls.created[-1] + self.assertEqual(created.kwargs['layer'], 'internet') + self.assertEqual(created.kwargs['protocol'], 'TCP') + + def test_layer_defaults_to_none_and_is_case_insensitive(self) -> None: + """An omitted ``-L``/``-P`` forwards :data:`None`, not a sentinel string. + + ``Extractor.__init__`` substitutes its own ``'none'``/``'null'`` for an + omitted value, so the CLI has nothing to substitute. It used to pass the + strings ``'None'`` and ``'null'``, which only worked because + ``Extractor`` happened to lowercase the former into the sentinel it + wanted. + + """ + emoji = types.SimpleNamespace(emojize=lambda text: text) + module, _, _ = self._load_cli_module(emoji_module=emoji) + + parser = module.get_parser() + + bare = parser.parse_args(['input.pcap']) + self.assertIsNone(bare.layer) + self.assertIsNone(bare.protocol) + + # ``-L Internet`` still works: the value is lowercased before it is + # matched against the layer names. + self.assertEqual(parser.parse_args(['input.pcap', '-L', 'Internet']).layer, 'internet') + self.assertEqual(parser.parse_args(['input.pcap', '--layer', 'LINK']).layer, 'link') + + def test_layer_rejects_a_name_that_is_not_a_layer(self) -> None: + """A misspelled ``-L`` fails loudly rather than being ignored. + + The layer names are a closed set and nothing downstream validates them: + an unknown name simply never matches a protocol's ``__layer__``, so the + parse runs to the top of the stack and the user is given a full report + they did not ask for -- silently, which is the failure GH-356 was about. + + """ + emoji = types.SimpleNamespace(emojize=lambda text: text) + module, _, _ = self._load_cli_module(emoji_module=emoji) + + parser = module.get_parser() + + with mock.patch('sys.stderr', io.StringIO()): + with self.assertRaises(SystemExit): + parser.parse_args(['input.pcap', '-L', 'nonsense']) + def test_main_uses_json_format_and_stdin_when_requested(self) -> None: emoji = types.SimpleNamespace(emojize=lambda text: text) module, extractor_cls, _ = self._load_cli_module(emoji_module=emoji) diff --git a/tests/integration/test_cli_subprocess.py b/tests/integration/test_cli_subprocess.py index acc22bfdec..6178e902b8 100644 --- a/tests/integration/test_cli_subprocess.py +++ b/tests/integration/test_cli_subprocess.py @@ -14,22 +14,48 @@ from __future__ import annotations import json +import os import pathlib import subprocess # nosec: B404 import sys import unittest from tests._support import sample_path +from tests._tiers import ROOT from tests.integration._helpers import HAS_EMOJI, HAS_RUNTIME, EndToEndTestCase #: How long a single CLI run is allowed to take. The captures used here are two -#: to six frames, so this is a hang guard rather than a budget. +#: to twenty-six frames, so this is a hang guard rather than a budget. TIMEOUT = 120 #: Path of the installed console script, if it is on this interpreter's path. CLI_SCRIPT = pathlib.Path(sys.executable).with_name('pcapkit-cli') +def cli_environment() -> 'dict[str, str]': + """This process's environment, with the checkout ahead of :envvar:`PYTHONPATH`. + + Every run below happens in a temporary working directory, so the checkout is + *not* on the subprocess's :data:`sys.path` and ``python -m pcapkit`` would + import whichever :mod:`pcapkit` is installed instead. With an editable + install of this very directory the two are the same file and nothing shows; + from a git worktree, or against any other tree than the installed one, they + are different files and the subprocess silently tests the wrong one. + + That is a blind spot rather than an inconvenience: a change to + :mod:`pcapkit.__main__` can be asserted here and pass on code it never ran. + So the tree the tests were collected from is put first, which is what the + in-process tiers already do by virtue of :program:`pytest`'s ``rootdir``. + + """ + environment = dict(os.environ) + existing = environment.get('PYTHONPATH') + environment['PYTHONPATH'] = ( + str(ROOT) if not existing else os.pathsep.join((str(ROOT), existing)) + ) + return environment + + @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') @unittest.skipUnless(HAS_EMOJI, "the cli extra's 'emoji' dependency is not installed") class CommandLineTests(EndToEndTestCase): @@ -40,7 +66,7 @@ def run_cli(self, *args: 'str', expect: 'int | None' = 0) -> 'subprocess.Complet completed = subprocess.run( # nosec: B603 [sys.executable, '-m', 'pcapkit', *args], cwd=str(self.tmp_path), capture_output=True, text=True, - timeout=TIMEOUT, check=False, + timeout=TIMEOUT, check=False, env=cli_environment(), ) if expect is not None: self.assertEqual(completed.returncode, expect, @@ -107,6 +133,41 @@ def test_engine_option_selects_an_alternative_engine(self) -> None: self.assertTrue((self.tmp_path / 'report.txt').is_file()) + def test_layer_option_stops_the_printed_chain_where_it_names(self) -> None: + """``-L`` reaches the parse, not just the argument namespace (GH-356). + + Frame 4 of :file:`http6.cap` is the shortest unambiguous witness: it + parses to all four layers unlimited, so each limit shortens the chain the + verbose output prints. Every one of these printed the full chain while the + limits were inert. + + """ + capture = sample_path('http6.cap') + unlimited = self.run_cli(capture, '-v') + link = self.run_cli(capture, '-v', '-L', 'link') + internet = self.run_cli(capture, '-v', '-L', 'internet') + + self.assertIn('Frame 4: Ethernet:IPv6:TCP:HTTP/1.1', unlimited.stdout) + self.assertIn('Frame 4: Ethernet:Internet_Protocol_version_6', link.stdout) + self.assertIn('Frame 4: Ethernet:IPv6:TCP', internet.stdout) + for completed in (link, internet): + self.assertNotIn('HTTP/1.1', completed.stdout) + + def test_protocol_option_stops_the_printed_chain_where_it_names(self) -> None: + """``-P`` likewise, taking a protocol name rather than a layer.""" + capture = sample_path('http6.cap') + stopped = self.run_cli(capture, '-v', '-P', 'TCP') + + self.assertIn('Frame 4: Ethernet:IPv6:TCP:Raw', stopped.stdout) + self.assertNotIn('HTTP/1.1', stopped.stdout) + + def test_layer_option_rejects_a_name_that_is_not_a_layer(self) -> None: + """A misspelled ``-L`` exits with the usage message instead of being ignored.""" + completed = self.run_cli(sample_path('in.pcap'), '-L', 'nonsense', expect=2) + + self.assertIn('usage: pcapkit-cli', completed.stderr) + self.assertIn('--layer', completed.stderr) + def test_missing_capture_fails_and_names_the_path(self) -> None: missing = str(self.tmp_path / 'absent.pcap') completed = self.run_cli(missing, '-o', 'report', '-j', expect=None) @@ -137,7 +198,7 @@ def test_console_script_writes_the_same_report(self) -> None: completed = subprocess.run( # nosec: B603 [str(CLI_SCRIPT), sample_path('arp.pcap'), '-o', 'report', '-j', '-a'], cwd=str(self.tmp_path), capture_output=True, text=True, - timeout=TIMEOUT, check=False, + timeout=TIMEOUT, check=False, env=cli_environment(), ) self.assertEqual(completed.returncode, 0, completed.stderr) diff --git a/tests/integration/test_frame_iteration.py b/tests/integration/test_frame_iteration.py index 3d5312300f..325ed764ac 100644 --- a/tests/integration/test_frame_iteration.py +++ b/tests/integration/test_frame_iteration.py @@ -9,17 +9,21 @@ The extraction limits -- ``layer`` and ``protocol``, which the command line tool exposes as ``-L`` and ``-P`` -- belong to the same spectrum: they are what stops -that walk short of the application layer. They do not work; see -:class:`ExtractionLimitTests`. +that walk short of the application layer. See :class:`ExtractionLimitTests`, +which asserts where each of them stops it. """ from __future__ import annotations import unittest +from typing import TYPE_CHECKING from tests._support import sample_path from tests.integration._helpers import HAS_RUNTIME, EndToEndTestCase +if TYPE_CHECKING: + from typing import Any + #: Frames of :file:`http6.cap` that carry an HTTP header block: the request and #: the response of each of the two connections. HTTP_FRAMES = (4, 6, 19, 21) @@ -76,42 +80,63 @@ def test_call_form_walks_the_same_frames_as_the_iterator(self) -> None: extractor() +def stack(frame: 'Any') -> 'list[Any]': + """The protocol objects of ``frame``, outermost first. + + Walks ``payload`` rather than reading ``protochain``, because the chain is a + string of *names* and a name does not say what the object is: a + :class:`~pcapkit.protocols.misc.raw.Raw` standing in for a payload the parse + declined to enter is labelled with the enumeration it was handed, so + ``Ethernet:IPv6:TCP`` is what both a fully parsed TCP header and a stopped + parse holding TCP's octets verbatim look like. Only the objects tell them + apart, which is what :class:`ExtractionLimitTests` has to assert on. + + """ + from pcapkit.protocols.misc.null import NoPayload + + walked = [] # type: list[Any] + current = frame.payload + while current is not None and not isinstance(current, NoPayload): + walked.append(current) + current = getattr(current, 'payload', None) + return walked + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class ExtractionLimitTests(EndToEndTestCase): - """``layer`` and ``protocol``, which stop parsing part way up the stack.""" + """``layer`` and ``protocol``, which stop parsing part way up the stack. + + Both were inert until GH-356. ``ProtocolBase.__init__`` read the limits from + ``_layer``/``_protocol``, while every producer -- the two engines building + the outermost protocol, all four ``_import_next_layer`` implementations + recursing into the next one, and the public + ``pcapkit.extract(layer=..., protocol=...)`` that the CLI's ``-L``/``-P`` + feed -- passed them without the underscore. They landed in ``**kwargs``, were + dropped, ``_sigterm`` stayed :data:`False` the whole way up, and the parse + always ran to the top of the stack whatever the caller asked for. + + So these tests assert on *where the parse stopped*, not on the option being + stored: a test that only checked ``extractor._exlyr`` would have passed + throughout the bug's lifetime. + + """ + + #: Frame 4 of :file:`http6.cap`, whose unlimited chain is the full four + #: layers, and what each limit should leave of it. The last entry of each + #: tuple is the class the parse is expected to stop *at*; everything beyond + #: it should be one :class:`~pcapkit.protocols.misc.raw.Raw`. + STOPS = { + 'link': ('Ethernet',), + 'internet': ('Ethernet', 'IPv6'), + 'transport': ('Ethernet', 'IPv6', 'TCP'), + } - @unittest.skip('blocked on the parse limit never reaching the next layer: ' - 'pcapkit/protocols/protocol.py:1157 passes it as layer=/protocol= while ' - 'pcapkit/protocols/protocol.py:514 reads _layer/_protocol') def test_layer_and_protocol_limits_stop_the_parse(self) -> None: - """``layer`` and ``protocol`` should stop parsing where they name. - - Neither does anything at all. ``ProtocolBase.__init__`` reads the limits - from ``kwargs.pop('_layer')`` and ``kwargs.pop('_protocol')`` - (``pcapkit/protocols/protocol.py:514`` and ``:516``), but every caller - passes them under the un-prefixed names: - ``pcapkit/protocols/protocol.py:1157`` recurses with - ``layer=self._exlayer, protocol=self._exproto``, and - ``pcapkit/foundation/engines/pcap.py:146`` builds the frame with - ``layer=ext._exlyr, protocol=ext._exptl``. The keywords therefore land in - ``**kwargs`` and are dropped, ``_sigterm`` stays :data:`False` all the - way up, and the parse always runs to the top of the stack. - - Measured on frame 4 of :file:`http6.cap`, whose full chain is - ``Ethernet:IPv6:TCP:HTTP/1.1``: ``layer='internet'``, - ``layer='transport'``, ``layer='link'``, ``protocol='TCP'`` and - ``protocol='IPv6'`` every one of them yield that same full chain. - - That the mismatch is the whole story can be shown without the extractor, - by handing one protocol object each spelling:: - - >>> from pcapkit.protocols.link.ethernet import Ethernet - >>> str(Ethernet(io.BytesIO(raw), len(raw), _layer='Link').protochain) - 'Ethernet:Internet_Protocol_version_6' - >>> str(Ethernet(io.BytesIO(raw), len(raw), layer='Link').protochain) - 'Ethernet:IPv6:TCP:HTTP/1.1' - - The command line tool passes ``-L`` and ``-P`` straight through to the - same place, so ``pcapkit-cli -L internet`` is equally inert. + """``layer`` and ``protocol`` stop parsing where they name. + + The assertion the issue was filed against: frame 4 of :file:`http6.cap` + parses as ``Ethernet:IPv6:TCP:HTTP/1.1`` unlimited, and none of these + limits may leave HTTP in it. """ for limit in ({'layer': 'internet'}, {'layer': 'transport'}, {'protocol': 'TCP'}): @@ -123,6 +148,148 @@ def test_layer_and_protocol_limits_stop_the_parse(self) -> None: self.assertTrue(chain.startswith('Ethernet:IPv6')) self.assertNotIn('HTTP', chain) + def test_no_limit_parses_to_the_top_of_the_stack(self) -> None: + """The control: without a limit, frame 4 reaches the application layer. + + Also covers the sentinels ``Extractor.__init__`` substitutes for an + omitted argument -- ``layer='none'`` and ``protocol='null'`` -- which + must mean "no limit" rather than naming a layer or a protocol to stop at. + + """ + import pcapkit + + for limit in ({}, {'layer': 'none'}, {'protocol': 'null'}, + {'layer': None, 'protocol': None}): + with self.subTest(**limit): + extractor = self.extract(fin=sample_path('http6.cap'), nofile=True, + store=True, **limit) + frame = extractor.frame[3] + + self.assertEqual(str(frame.protochain), 'Ethernet:IPv6:TCP:HTTP/1.1') + self.assertIn(pcapkit.HTTP, frame) + self.assertEqual( + [type(protocol).__name__ for protocol in stack(frame)], + ['Ethernet', 'IPv6', 'TCP', 'HTTP'], + ) + + def test_layer_limit_stops_at_the_named_layer_and_leaves_raw_above_it(self) -> None: + """Each ``layer`` value stops the walk at the protocol of that layer. + + What is above the stop is a single :class:`~pcapkit.protocols.misc.raw.Raw` + holding the octets the parse declined to enter, and its ``error`` is + :data:`None` -- the stop is deliberate, not a parse failure that happens + to look like one. + + """ + from pcapkit.protocols.misc.raw import Raw + + for layer, expected in self.STOPS.items(): + with self.subTest(layer=layer): + extractor = self.extract(fin=sample_path('http6.cap'), nofile=True, + store=True, layer=layer) + walked = stack(extractor.frame[3]) + + self.assertEqual([type(protocol).__name__ for protocol in walked[:-1]], + list(expected)) + self.assertIsInstance(walked[-1], Raw) + self.assertIsNone(walked[-1].info.error) + self.assertEqual(len(walked), len(expected) + 1) + + def test_layer_limit_keeps_the_unparsed_payload_verbatim(self) -> None: + """Stopping loses nothing: the ``Raw`` holds what the next layer would have. + + ``layer='internet'`` stops above IPv6, so the octets the TCP header and + everything after it would have been parsed from are still there, byte for + byte, and they are the same octets the unlimited parse consumed. + + """ + full = self.extract(fin=sample_path('http6.cap'), nofile=True, store=True) + stopped = self.extract(fin=sample_path('http6.cap'), nofile=True, store=True, + layer='internet') + + tcp_onwards = bytes(stack(full.frame[3])[2]) + raw = stack(stopped.frame[3])[-1] + + self.assertEqual(bytes(raw), tcp_onwards) + self.assertGreater(len(tcp_onwards), 0) + + def test_protocol_limit_accepts_a_name_or_a_protocol_class(self) -> None: + """``protocol`` takes a name, a class, or an instance of one. + + ``Protocol.expand_comp`` is documented to accept all three, and the + limit is compared through it, so the three spellings have to agree. + + """ + import pcapkit + from pcapkit.protocols.misc.raw import Raw + + for protocol in ('TCP', 'tcp', pcapkit.TCP): + with self.subTest(protocol=protocol): + extractor = self.extract(fin=sample_path('http6.cap'), nofile=True, + store=True, protocol=protocol) + walked = stack(extractor.frame[3]) + + self.assertEqual([type(item).__name__ for item in walked], + ['Ethernet', 'IPv6', 'TCP', 'Raw']) + self.assertIsInstance(walked[-1], Raw) + self.assertNotIn(pcapkit.HTTP, extractor.frame[3]) + + def test_protocol_limit_stops_below_the_transport_layer(self) -> None: + """``protocol='IPv6'`` stops at IPv6, i.e. TCP is never parsed either.""" + import pcapkit + from pcapkit.protocols.misc.raw import Raw + + extractor = self.extract(fin=sample_path('http6.cap'), nofile=True, store=True, + protocol='IPv6') + walked = stack(extractor.frame[3]) + + self.assertEqual([type(item).__name__ for item in walked], + ['Ethernet', 'IPv6', 'Raw']) + self.assertNotIn(pcapkit.TCP, extractor.frame[3]) + self.assertIsInstance(walked[-1], Raw) + + def test_limits_apply_to_every_frame_not_only_the_first(self) -> None: + """The limit is honoured for the whole capture. + + The engines build one protocol per frame, so a limit that reached only + the first frame would still be a bug -- and the four HTTP-carrying frames + of :file:`http6.cap` are what would show it. + + """ + import pcapkit + + extractor = self.extract(fin=sample_path('http6.cap'), nofile=True, store=True, + layer='internet') + + self.assertEqual(len(extractor.frame), 26) + self.assertFalse(any(pcapkit.HTTP in frame for frame in extractor.frame)) + self.assertFalse(any(pcapkit.TCP in frame for frame in extractor.frame)) + + def test_pcapng_engine_honours_the_limits_too(self) -> None: + """The PCAP-NG engine is a second, independent producer of the limits. + + It builds its outermost protocol in + ``pcapkit.foundation.engines.pcapng.PCAPNG.read_frame`` rather than + through ``PCAP.read_frame``, so it has to be checked separately: the + protochains for :file:`test.pcapng` were byte-identical to the unlimited + run while the limits were inert. + + """ + chains = {} + for limit in ({}, {'layer': 'link'}, {'layer': 'internet'}): + extractor = self.extract(fin=sample_path('test.pcapng'), nofile=True, + store=True, **limit) + chains[tuple(sorted(limit.items()))] = [ + str(frame.protochain) for frame in extractor.frame + ] + + unlimited = chains[()] + self.assertNotEqual(chains[(('layer', 'link'),)], unlimited) + self.assertNotEqual(chains[(('layer', 'internet'),)], unlimited) + # Anything the unlimited run took past the link layer must be shorter now. + self.assertTrue(any(len(short.split(':')) < len(long.split(':')) + for short, long in zip(chains[(('layer', 'link'),)], unlimited))) + if __name__ == '__main__': unittest.main() diff --git a/tests/protocols/test_protocol_base_unit.py b/tests/protocols/test_protocol_base_unit.py index df1abefcc4..298414e929 100644 --- a/tests/protocols/test_protocol_base_unit.py +++ b/tests/protocols/test_protocol_base_unit.py @@ -4,6 +4,7 @@ import enum import importlib.util import io +import ipaddress import unittest from unittest import mock @@ -354,6 +355,186 @@ def test_make_payload_branches(self) -> None: self.assertIsInstance(payload, DummyProtocol) self.assertEqual(payload.info.to_dict()['value'], 12) + def test_parse_limits_accept_the_spelling_every_producer_uses(self) -> None: + """``layer=``/``protocol=`` are honoured while parsing, ignored while making. + + The limits are documented on ``ProtocolBase.__init__`` as ``_layer`` and + ``_protocol``, but nothing in the tree spells them that way: the engines + and every ``_import_next_layer`` pass them without the underscore, so + both were silently dropped (GH-356). Both spellings therefore have to + work, and the prefixed one has to win when the two disagree. + + The un-prefixed pair may only be consumed while *parsing*, though. + ``protocol`` is a real ``make()`` argument -- ``IPv4.make`` takes one, and + ``Data_IPv6.to_dict`` carries one straight into ``from_data`` -- so + swallowing it on the construction path would silently drop the value + being constructed. + + """ + DummyProtocol, _, _ = self._make_protocol_class() + + for keywords, layer, proto in ( + ({'_layer': 'Internet'}, 'Internet', None), + ({'layer': 'Internet'}, 'Internet', None), + ({'_protocol': 'dummyprotocol'}, None, 'dummyprotocol'), + ({'protocol': 'dummyprotocol'}, None, 'dummyprotocol'), + # the prefixed spelling wins over the un-prefixed one + ({'_layer': 'Internet', 'layer': 'Transport'}, 'Internet', None), + ): + with self.subTest(**keywords): + parsed = DummyProtocol(io.BytesIO(b'abpayload'), 9, **keywords) + + self.assertEqual(parsed._exlayer, layer) + self.assertEqual(parsed._exproto, proto) + self.assertTrue(parsed._sigterm) + + # ... and the un-prefixed pair reaches ``make()`` untouched when there is + # no source stream, i.e. nothing is being parsed. + made = DummyProtocol(packet=b'abpayload', protocol='dummyprotocol', layer='Internet') + self.assertIsNone(made._exlayer) + self.assertIsNone(made._exproto) + self.assertFalse(made._sigterm) + + def test_no_limit_sentinels_are_not_treated_as_a_limit(self) -> None: + """``layer='none'`` and ``protocol='null'`` mean "no limit", not a name. + + They are what ``Extractor.__init__`` substitutes for an omitted argument, + so they reach every protocol of every packet and must not be compared + against real protocol names. + + """ + DummyProtocol, _, _ = self._make_protocol_class() + + for keywords in ({'layer': 'none'}, {'layer': 'NONE'}, {'protocol': 'null'}, + {'_layer': 'none'}, {'_protocol': 'null'}, + {'layer': 'none', 'protocol': 'null'}): + with self.subTest(**keywords): + parsed = DummyProtocol(io.BytesIO(b'abpayload'), 9, **keywords) + + self.assertIsNone(parsed._exlayer) + self.assertIsNone(parsed._exproto) + self.assertFalse(parsed._sigterm) + + def test_packet_context_is_republished_as_dunder_packet(self) -> None: + """The enclosing layer's ``packet=`` reaches ``unpack`` as ``__packet__``. + + ``_import_next_layer`` hands the next protocol its packet context as + ``packet=``, but the schema layer reads it from ``__packet__`` + (``Protocol.unpack``, and the ``pack``/``unpack`` overrides of ``Frame`` + and ``PCAPNG``). Nothing bridged the two, so a schema always saw an empty + dict (GH-382). + + The republished dict is a *copy*: ``Schema.unpack`` writes every field it + reads into the context it is handed, and the IPv6 extension header walk + gives one dict to each header in turn, so sharing it would leak one + header's fields into the next one's context. + + """ + DummyProtocol, _, _ = self._make_protocol_class() + + seen = {} # type: dict[str, object] + original = DummyProtocol.unpack + + def capture(self, length=None, **kwargs): + # Snapshot before delegating: ``Schema.unpack`` writes its own + # ``__length__`` and every field it reads into the context, so what + # arrived is only observable ahead of the call. + seen['snapshot'] = dict(kwargs.get('__packet__') or {}) + seen['identity'] = kwargs.get('__packet__') + seen['packet'] = kwargs.get('packet') + return original(self, length, **kwargs) + + DummyProtocol.unpack = capture # type: ignore[method-assign] + try: + outer = {'src': 'the-source', 'dst': 'the-destination'} + DummyProtocol(io.BytesIO(b'abpayload'), 9, packet=outer) + finally: + DummyProtocol.unpack = original # type: ignore[method-assign] + + self.assertEqual(seen['snapshot'], {'src': 'the-source', 'dst': 'the-destination'}) + # A copy, and the enclosing layer's own dict is left exactly as it was. + self.assertIsNot(seen['identity'], outer) + self.assertEqual(outer, {'src': 'the-source', 'dst': 'the-destination'}) + # ``packet=`` is left in place as well, since it is the documented + # ``_import_next_layer`` spelling and some callers still read it. + self.assertIs(seen['packet'], outer) + + def test_explicit_dunder_packet_is_not_overridden(self) -> None: + """A caller that already supplies ``__packet__`` keeps its own dict. + + The PCAP-NG engine does exactly that -- it passes the section's snapshot + length as ``__packet__`` -- so the bridge must only fill the keyword in + when it is absent. + + """ + DummyProtocol, _, _ = self._make_protocol_class() + + seen = {} # type: dict[str, object] + original = DummyProtocol.unpack + + def capture(self, length=None, **kwargs): + seen['identity'] = kwargs.get('__packet__') + return original(self, length, **kwargs) + + DummyProtocol.unpack = capture # type: ignore[method-assign] + try: + chosen = {'snaplen': 262144} + DummyProtocol(io.BytesIO(b'abpayload'), 9, + packet={'src': 'ignored'}, __packet__=chosen) + finally: + DummyProtocol.unpack = original # type: ignore[method-assign] + + self.assertIs(seen['identity'], chosen) + + def test_outer_address_reaches_a_schema_that_needs_it(self) -> None: + """A real consumer of the packet context gets its value (GH-382). + + RFC 7731 lets an MPL option elide its Seed-ID from the wire when the + Seed-ID type is ``IPV6_SOURCE_ADDRESS``, in which case the seed *is* the + enclosing IPv6 source address. + ``pcapkit.protocols.schema.internet.hopopt.MPLOption.post_process`` + implements that by reading ``packet['src']`` -- a value only the outer + layer knows -- and ``IPv6.read`` does put it in the dict it hands down. + The dict never arrived as ``__packet__``, so the seed silently came back + as :data:`None` for every such option. + + The capture is built here rather than read from + :file:`examples/captures/`: this is a unit-tier module, and none of the + committed captures carries an MPL option. + + """ + from pcapkit.const.ipv6.seed_id import SeedID + from pcapkit.protocols.internet.ipv6 import IPv6 + + source = ipaddress.IPv6Address('2001:db8::1') + destination = ipaddress.IPv6Address('ff02::1') + + #: One HOPOPT extension header, eight octets: an MPL option whose + #: Seed-ID is elided, then two ``Pad1`` octets to fill the header out. + hopopt = bytes([ + 59, # next header: IPv6-NoNxt + 0, # hdr ext len: 0, i.e. 8 octets in total + 0x6D, # option type: MPL_Option + 0x02, # opt data len: 2 -- flags and sequence only, seed elided + 0x00, # flags: S=0b00 (IPV6_SOURCE_ADDRESS), M=0, V=0 + 0x2A, # sequence + 0x00, # option type: Pad1 + 0x00, # option type: Pad1 + ]) + packet = (bytes([0x60, 0x00, 0x00, 0x00]) # version, traffic class, flow label + + len(hopopt).to_bytes(2, 'big') # payload length + + bytes([0, 64]) # next header: HOPOPT; hop limit + + source.packed + destination.packed + + hopopt) + + parsed = IPv6(io.BytesIO(packet), len(packet)) + option = list(parsed.info.hopopt.options.values())[0] + + self.assertEqual(str(parsed.protochain), 'IPv6:HOPOPT') + self.assertEqual(parsed.info.src, source) + self.assertEqual(option.seed_type, SeedID.IPV6_SOURCE_ADDRESS) + self.assertEqual(option.seed_id, source) + if __name__ == '__main__': unittest.main() diff --git a/tests/utilities/test_decorators.py b/tests/utilities/test_decorators.py index 5d857f60f4..f600615379 100644 --- a/tests/utilities/test_decorators.py +++ b/tests/utilities/test_decorators.py @@ -103,6 +103,70 @@ def unpack(cls, data, length=None, packet=None): with self.assertRaises(EOFError): DemoSchema.unpack(b'', None, None) + def test_prepare_leaves_the_schema_clean_after_post_process(self) -> None: + """``post_process``'s revisions must not mark the schema as needing a re-pack. + + ``Schema.unpack`` clears ``__updated__`` before returning, but + ``post_process`` runs after it and every field it assigns sets the flag + again. The schema was therefore left dirty, and the next + ``bytes(schema)`` re-packed it -- which calls ``post_process`` a second + time with a packet context rebuilt from the schema's own fields, so any + value ``post_process`` had derived from the *enclosing* layer's context + was overwritten with the fallback. ``Schema.pack`` clears the flag after + ``post_process``, and this asserts the unpacking path now matches it. + + """ + class DemoSchema: + def __init__(self) -> None: + self.__updated__ = False + + @classmethod + def pre_unpack(cls, packet) -> None: + return None + + def post_process(self, packet): + # stand-in for a field assignment, which is what sets the flag + self.value = packet.get('src', 'fallback') + self.__updated__ = True + return self + + @classmethod + @self.decorators.prepare + def unpack(cls, data, length=None, packet=None): + return cls() + + schema = DemoSchema.unpack(b'payload', None, {'src': 'outer-source'}) + + self.assertEqual(schema.value, 'outer-source') + self.assertFalse(schema.__updated__) + + def test_prepare_tolerates_a_post_process_that_returns_no_schema(self) -> None: + """``post_process`` need not return a schema, so the flag reset is guarded. + + An implementation may hand back a nested schema instead of ``self`` -- + ``pcapkit.protocols.schema.internet.hopopt._SMFDPDOption`` returns + ``self.data`` -- and the stand-ins above return the packet mapping. + Neither may raise from the reset. + + """ + class DemoSchema: + @classmethod + def pre_unpack(cls, packet) -> None: + packet['prepped'] = True + + def post_process(self, packet): + return packet + + @classmethod + @self.decorators.prepare + def unpack(cls, data, length=None, packet=None): + return cls() + + returned = DemoSchema.unpack(b'payload', None, None) + + self.assertIsInstance(returned, dict) + self.assertTrue(returned['prepped']) + def test_beholder_wraps_struct_eof_with_no_payload(self) -> None: exceptions = self.exceptions