fix(http): reach _guess_version's HTTP/2 arm, and resolve PayloadField names

pcapkit/corekit/fields/misc.py

@@ -6,6 +6,7 @@
 
 from pcapkit.corekit.fields.field import FieldBase, NoValue
 from pcapkit.utilities.exceptions import FieldError, NoDefaultValue
+from pcapkit.utilities.warnings import RegistryWarning, warn
 
 __all__ = [
     'ConditionalField', 'PayloadField',
@@ -225,7 +226,9 @@ class PayloadField(FieldBase[_TP]):
         length: Field size (in bytes); if a callable is given, it should return
             an integer value and accept the current packet as its only argument.
         default: Field default value.
-        protocol: Payload protocol.
+        protocol: Payload protocol, as a class or as a registered protocol name;
+            see :meth:`the property setter <PayloadField.protocol>`, through
+            which this argument is resolved.
         callback: Callback function to be called upon
             :meth:`self.__call__ <pcapkit.corekit.fields.field.FieldBase.__call__>`.
 
@@ -259,21 +262,67 @@ def protocol(self, protocol: 'Type[_TP] | str') -> 'None':
         """Set payload protocol.
 
         Arguments:
-            protocol: Payload protocol.
+            protocol: Payload protocol. A :obj:`str` is resolved against the
+                :data:`pcapkit.protocols.__proto__` registry, case-insensitively,
+                and an unresolved name leaves the payload as
+                :class:`~pcapkit.protocols.misc.raw.Raw`.
+
+        Warns:
+            pcapkit.utilities.warnings.RegistryWarning: If ``protocol`` names a
+                protocol the registry does not hold.
 
         """
         if isinstance(protocol, str):
             from pcapkit.protocols import __proto__  # pylint: disable=import-outside-top-level
-            protocol = cast('Type[_TP]', __proto__.get(protocol))
+
+            # NOTE: The registry is keyed on the upper-cased class name, both
+            # when it is seeded (``pcapkit/protocols/__init__.py:75``) and when
+            # ``pcapkit.foundation.registry.protocols.register_protocol`` adds to
+            # it, so a name given in any other case missed every time -- and a
+            # miss leaves ``_protocol`` as :obj:`None`, which the property above
+            # resolves to :class:`~pcapkit.protocols.misc.raw.Raw`. So
+            # ``PayloadField(protocol='http')`` yielded a raw payload instead of
+            # HTTP, with nothing to say so (#787).
+            resolved = __proto__.get(protocol.upper())
+
+            # NOTE: Warned rather than left silent, and warned rather than
+            # raised. Unlike the registry lookups that dispatch on a code read
+            # off the wire -- where a miss is ordinary traffic and
+            # :class:`~pcapkit.protocols.misc.raw.Raw` is the right answer --
+            # this branch is reached only from a caller that named a protocol in
+            # source, so a miss is a mistake in that name rather than a property
+            # of the captured packet, and it is otherwise indistinguishable from
+            # an unparsed payload. It stays a warning because the :obj:`None`
+            # fallback is itself legitimate (a ``PayloadField`` with no protocol
+            # at all is the common case), so refusing the assignment outright
+            # would reject a lenient spelling the field has always accepted.
+            if resolved is None:
+                warn(f'unregistered payload protocol: {protocol!r}', RegistryWarning)
+
+            protocol = cast('Type[_TP]', resolved)
         self._protocol = protocol
 
     def __init__(self, length: 'int | Callable[[dict[str, Any]], int]' = lambda _: -1,
                  default: '_TP | NoValueType | bytes' = NoValue,
-                 protocol: 'Optional[Type[_TP]]' = None,
+                 protocol: 'Optional[Type[_TP] | str]' = None,
                  callback: 'Callable[[Self, dict[str, Any]], None]' = lambda *_: None) -> 'None':
         #self._name = '<payload>'
         self._default = default  # type: ignore[assignment]
-        self._protocol = protocol  # type: ignore[assignment]
+
+        # NOTE: Through the property rather than straight to ``_protocol``, so a
+        # name given here is resolved exactly as one assigned later is. Writing
+        # the attribute directly stored the :obj:`str` verbatim and the getter
+        # handed that same string back, so ``PayloadField(protocol='http')``
+        # yielded neither the protocol nor
+        # :class:`~pcapkit.protocols.misc.raw.Raw` but ``'http'`` itself -- and
+        # ``protocol='HTTP'`` was no better, since the case was never what this
+        # path went wrong on (#787). The lookup the setter performs stays inside
+        # its ``isinstance(protocol, str)`` branch, so a field declared in a
+        # schema class body -- every in-library use, none of which names a
+        # protocol -- still does not import :mod:`pcapkit.protocols` while that
+        # package may itself be mid-import.
+        self.protocol = protocol  # type: ignore[assignment]
+
         self._callback = callback
 
         self._length_callback = None

pcapkit/protocols/application/http.py

@@ -15,6 +15,7 @@
 
 """
 import contextlib
+import struct
 from typing import TYPE_CHECKING, Generic
 
 from pcapkit.protocols.application.application import Application
@@ -116,7 +117,15 @@ def read(self, length: 'Optional[int]' = None, *,
                 http = protocol(self._data, length, **kwargs)
             except ProtocolError:
                 raise
-            except ValueError as error:
+            # NOTE: :exc:`struct.error` alongside :exc:`ValueError` because the
+            # two are disjoint -- it derives straight from :exc:`Exception` --
+            # and a payload too short for a versioned parser's fixed header
+            # raises the former from deep inside the schema machinery
+            # (``FieldBase.length`` calls :func:`struct.calcsize` on a template
+            # built from a negative length). A caller of this method cannot
+            # catch that as a protocol error, which is the whole point of the
+            # conversion the next line performs, so it is converted too.
+            except (ValueError, struct.error) as error:
                 raise ProtocolError(f'HTTP/{version}: invalid format') from error
 
         self._version = http.version
@@ -197,13 +206,66 @@ def _guess_version(self, length: 'int', **kwargs: 'Any') -> 'HTTP':
         Returns:
             Parsed packet data.
 
+        Raises:
+            ProtocolError: If no version in the family accepts the payload. This
+                is the *only* exception this method raises for an unparseable
+                payload -- a candidate failing in any other way is a fall-through
+                and not an answer.
+
         """
+        # NOTE: The two arms suppress different sets, and the asymmetry is
+        # deliberate. Only the *last* arm additionally suppresses
+        # :exc:`struct.error`, because a payload too short to hold HTTP/2's
+        # nine-octet frame header usually fails inside the schema machinery with
+        # that stdlib exception rather than with a protocol error --
+        # ``FieldBase.length`` calls :func:`struct.calcsize` on a template built
+        # from a negative length -- and it is neither a ``ProtocolError`` nor a
+        # :exc:`ValueError`, so it used to leave this method uncatchable by any
+        # caller: ``HTTP(io.BytesIO(b'\x00' * 8), 8)`` raised a bare
+        # :exc:`struct.error` where the closing ``raise`` below is the documented
+        # answer. ("Usually" because ``httpv2.HTTP``'s own guard tests the
+        # *declared* length, not the buffer's, so a four-octet payload declaring
+        # fifteen parses instead of failing.) Suppressing it on the last arm is
+        # safe in the sense that matters here -- no arm follows, so nothing can
+        # answer in place of the error.
+        #
+        # The residual, which is real and is not zero: a genuine ``httpv2``
+        # schema defect that raised :exc:`struct.error` on well-formed HTTP/2
+        # bytes would now be reported as ``unknown HTTP version`` rather than
+        # crashing loudly, so a bug in that schema is quieter than it was. That is
+        # accepted because the alternative is a dispatcher no caller can catch,
+        # and it is bounded: ``httpv2.HTTP`` stays reachable directly, where
+        # nothing is suppressed, and that is the documented route for a caller who
+        # wants the unwrapped failure.
+        #
+        # This is keyed on being *last*, not on being the HTTP/2 arm. Inserting an
+        # arm after this one would silently make the reasoning false, and the
+        # regression test guards arm 1 specifically, so it would not catch that:
+        # the new arm must take the :exc:`struct.error` suppression and this one
+        # must give it up.
+        #
+        # Widening the *first* arm the same way was measured and reverted, as it
+        # buys nothing and costs a great deal. Nothing reaches a
+        # :exc:`struct.error` through ``httpv1.HTTP``: nine byte patterns over
+        # lengths 0-24, on this route and on ``read(version=1)``, answered
+        # ``ProtocolError`` 450 times out of 450. And
+        # :class:`~pcapkit.utilities.exceptions.StructError` *subclasses*
+        # :exc:`struct.error`, so suppressing it on a non-final arm swallows
+        # pcapkit's own signal and hands the payload to the arm below -- which
+        # accepts anything of at least nine octets. With a fault injected at arm
+        # 1, a *valid* HTTP/1.1 request came back ``version='2'``, and over UDP
+        # port 80 its ``protochain`` read ``UDP:HTTP/2``. A confident HTTP/2
+        # mislabel of HTTP/1 traffic is the exact failure #787 exists to stop, and
+        # it is worse than letting the error escape to ``beholder``, which turns
+        # it into ``Raw``; it would also erase ``StructError.eof``, which
+        # ``NoPayload`` handling reads. ``unknown HTTP version`` is only the best
+        # case, needing arm 2 to decline as well.
         from pcapkit.protocols.application.httpv1 import HTTP as HTTPv1  # isort: skip # pylint: disable=line-too-long,import-outside-toplevel
         with contextlib.suppress(ProtocolError):
             return HTTPv1(self._data, length, **kwargs)
 
         from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2  # isort: skip # pylint: disable=line-too-long,import-outside-toplevel
-        with contextlib.suppress(ProtocolError):
+        with contextlib.suppress(ProtocolError, struct.error):
             return HTTPv2(self._data, length, **kwargs)
 
         raise ProtocolError("unknown HTTP version")

pcapkit/protocols/application/httpv1.py

@@ -139,7 +139,20 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_HTTP':
         schema = self.__header__
 
         packet = schema.data
-        header, body = packet.split(b'\r\n\r\n', maxsplit=1)
+
+        # NOTE: A payload carrying no header/body separator at all unpacks short
+        # here, and the bare ``ValueError`` that used to escape is what made
+        # ``HTTP._guess_version``'s HTTP/2 arm unreachable: that dispatcher falls
+        # through on ``ProtocolError`` alone, so an HTTP/1 attempt on HTTP/2 wire
+        # bytes aborted the guess rather than failing it, and the HTTP/2 attempt
+        # never ran (#787). ``ProtocolError`` is what the ``Raises:`` section
+        # above already promises for a malformed packet, and the same conversion
+        # the explicit ``version=`` path performs at ``http.py:119``; chained, so
+        # the underlying unpacking error stays reachable as ``__cause__``.
+        try:
+            header, body = packet.split(b'\r\n\r\n', maxsplit=1)
+        except ValueError as error:
+            raise ProtocolError('HTTP: invalid format') from error
 
         header_line, header_unpacked = self._read_http_header(header)
         body_unpacked = self._read_http_body(body, headers=header_unpacked) or None
@@ -285,10 +298,63 @@ def _read_http_header(self, header: 'bytes') -> 'tuple[Data_Header, OrderedMulti
             ProtocolError: If the packet is malformed.
 
         """
-        startline, headerfield = header.split(b'\r\n', 1)
-        para1, para2, para3 = re.split(rb'\s+', startline, 2)
-        fields = headerfield.split(b'\r\n')
-        lists = (re.split(rb'\s*:\s*', field, 1) for field in fields)
+        # NOTE: Both unpackings are short for input that is not an HTTP/1
+        # message: a header of one line with no CRLF -- the HTTP/2 connection
+        # preface, ``PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n``, splits to exactly that
+        # -- and a start line of fewer than three whitespace-separated tokens.
+        # Raised as ``ProtocolError`` for the reason ``read`` gives above, and
+        # to the same message this method already uses below for a start line it
+        # cannot recognise (#787).
+        try:
+            startline, headerfield = header.split(b'\r\n', 1)
+            para1, para2, para3 = re.split(rb'\s+', startline, maxsplit=2)
+        except ValueError as error:
+            raise ProtocolError('HTTP: invalid format') from error
+
+        # NOTE: A field line beginning with SP or HTAB is an ``obs-fold``
+        # continuation of the line before it (:rfc:`9112#section-5.2`), and is
+        # unfolded here -- the RFC's own remedy -- rather than treated as a field
+        # line of its own. Deprecated, but present in real captures, and the two
+        # ways it used to come out were both wrong: a continuation carrying no
+        # colon left the split below one element long and ``item[1]`` raised
+        # :exc:`IndexError`, which is neither a :exc:`ValueError` nor a
+        # ``ProtocolError`` and so escaped ``HTTP._guess_version``'s suppression
+        # exactly as the bare :exc:`ValueError` of #787 did; a continuation that
+        # happened to contain one was worse, parsing silently into a spurious
+        # extra field (``X-Long: a`` plus ``b: c``, for a folded ``X-Long: a b``)
+        # with nothing raised at all. Unfolded, a folded message parses to the
+        # field it actually carries, so this input class stops reaching the
+        # HTTP/2 arm by accident instead of merely failing more politely.
+        fields = []  # type: list[bytes]
+        for line in headerfield.split(b'\r\n'):
+            if line.startswith((b' ', b'\t')):
+                # A continuation with nothing to continue -- the first field line
+                # folded -- is malformed rather than unfoldable.
+                if not fields:
+                    raise ProtocolError('HTTP: invalid format')
+                # NOTE: The accumulator is right-stripped as well as the
+                # continuation, because the production is ``obs-fold = OWS CRLF
+                # RWS`` and it is the *whole* obs-fold that is replaced by a
+                # single space -- the OWS before the CRLF belongs to the fold,
+                # not to the value. Stripping only the continuation left that OWS
+                # in place, so ``X: a \t\r\n\tb`` unfolded to ``'a \t  b'``
+                # rather than ``'a b'``: four of five folded/literal pairs
+                # disagreed, and a HTAB survived where the RFC prescribes SP.
+                fields[-1] = fields[-1].rstrip() + b' ' + line.strip()
+                continue
+            fields.append(line)
+
+        # NOTE: Checked rather than left to ``item[1]``, and refused rather than
+        # skipped: a field line with no colon is not a header field, and dropping
+        # it would hand back a message whose fields are quietly not the ones on
+        # the wire. ``ProtocolError`` for the reason the start-line split above
+        # gives, and to the same message.
+        lists = []  # type: list[list[bytes]]
+        for field in fields:
+            item = re.split(rb'\s*:\s*', field, maxsplit=1)
+            if len(item) != 2:
+                raise ProtocolError('HTTP: invalid format')
+            lists.append(item)
 
         if TYPE_CHECKING:
             header_line: 'Data_Header'