protocols: honour layer=/protocol= limits, and forward the packet context

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.')

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 <pcapkit.protocols.protocol.Protocol._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 <pcapkit.protocols.protocol.Protocol._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 <ProtocolBase._import_next_layer>`.
+                While parsing, it is republished as ``__packet__`` so that
+                :meth:`self.unpack <Protocol.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 <pcapkit.protocols.protocol.Protocol._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 <ProtocolBase._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 <ProtocolBase._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]
 

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

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)

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)