diff --git a/pcapkit/protocols/application/http.py b/pcapkit/protocols/application/http.py index db985e69bf..592315b9b2 100644 --- a/pcapkit/protocols/application/http.py +++ b/pcapkit/protocols/application/http.py @@ -131,7 +131,23 @@ def make(self, from pcapkit.protocols.application.httpv2 import HTTP as protocol # type: ignore[assignment] # isort: skip # pylint: disable=line-too-long,import-outside-toplevel else: raise ProtocolError(f"invalid HTTP version: {version}") - return protocol.make(**kwargs) # type: ignore[return-value] + + # NOTE: ``protocol.make`` is an ordinary instance method (the abstract + # declaration at ``ProtocolBase.make`` takes ``self``, and the + # versioned overrides use instance-bound helpers such as + # ``self._make_index``/``self.__frame__``), so calling it on the + # class itself -- as this used to -- left ``self`` unfilled and raised + # ``TypeError`` for every real call; see GH-452. There is no ``file`` + # or ``length`` to construct with here, since building a packet from + # keyword arguments is the inverse of parsing one, so a bare instance + # via ``protocol.__new__`` -- bypassing ``__init__``'s parse/pack + # machinery entirely -- is what the versioned ``make`` needs to be + # called on. This is safe only as long as the versioned ``make`` never + # reads state that ``__init__``/``__post_init__`` would otherwise have + # established (neither ``HTTPv1.make`` nor ``HTTPv2.make`` does today); + # a future ``make`` override that reaches for such state would need a + # different dispatch here. + return protocol.__new__(protocol).make(**kwargs) # type: ignore[return-value] ########################################################################## # Utilities. diff --git a/pcapkit/protocols/schema/application/httpv2.py b/pcapkit/protocols/schema/application/httpv2.py index ea6cd80070..d02cdb9f63 100644 --- a/pcapkit/protocols/schema/application/httpv2.py +++ b/pcapkit/protocols/schema/application/httpv2.py @@ -282,7 +282,7 @@ class Flags(FrameType.Flags): #: Settings. settings: 'list[SettingPair]' = ListField( length=lambda pkt: pkt['__length__'], - item_type=SettingPair, # type: ignore[arg-type] + item_type=SchemaField(schema=SettingPair), ) if TYPE_CHECKING: diff --git a/tests/protocols/application/test_http_unit.py b/tests/protocols/application/test_http_unit.py index 99d3431ec0..31e508c4ab 100644 --- a/tests/protocols/application/test_http_unit.py +++ b/tests/protocols/application/test_http_unit.py @@ -144,6 +144,74 @@ def make(**kwargs): with self.assertRaises(ProtocolError): HTTP._guess_version(http, 9) + def test_http_make_dispatches_to_real_versioned_classes(self) -> None: + """Regression test for GH-452. + + ``HTTP.make`` used to dispatch with ``protocol.make(**kwargs)``, where + ``protocol`` is the imported *class* -- an unbound call, since neither + ``HTTPv1.make`` nor ``HTTPv2.make`` is declared ``staticmethod`` or + ``classmethod`` on the real class (both are plain ``def make(self, + ...)``, matching the abstract ``ProtocolBase.make``). Every real call + therefore raised ``TypeError: make() missing 1 required positional + argument: 'self'``. + + This deliberately exercises the **real** ``HTTPv1``/``HTTPv2`` + classes rather than a fake with a ``staticmethod`` ``make`` -- + ``test_http_read_make_and_guess_version_delegation_paths`` above uses + exactly such a fake, which is what let the original defect through: + a ``staticmethod`` absorbs an unbound call the same as a bound one, + so a fake-based assertion passes whether or not ``self`` is actually + threaded through. + """ + from pcapkit.protocols.application.http import HTTP + from pcapkit.protocols.schema.application.httpv1 import HTTP as Schema_HTTPv1 + from pcapkit.protocols.schema.application.httpv2 import HTTP as Schema_HTTPv2 + + http = object.__new__(HTTP) + + schema_v1 = http.make(version=1, method='GET', uri='/index.html') + self.assertIsInstance(schema_v1, Schema_HTTPv1) + self.assertIn(b'GET /index.html HTTP/1.1\r\n', schema_v1.data) + + schema_v2 = http.make(version=2, sid=1, frame=b'payload') + self.assertIsInstance(schema_v2, Schema_HTTPv2) + self.assertEqual(schema_v2.stream['sid'], 1) + self.assertEqual(schema_v2.frame, b'payload') + + def test_http_construction_reaches_the_versioned_make_callee(self) -> None: + """Regression test for GH-452, using the corrected reproduction. + + The issue's original reproduction called ``HTTP.make(version=1, + ...)`` directly on the class -- but the *outer* ``HTTP.make`` is + itself an ordinary instance method, so that call fails at the outer + method and never demonstrates anything about the inner + ``protocol.make(**kwargs)`` dispatch this issue is actually about. + + The real, supported entry point is construction: + :meth:`ProtocolBase.__init__` (``protocol.py:519``, the ``**kwargs``- + only overload at ``:517``) calls ``self.pack(**kwargs)`` when built + with no ``file``, and :meth:`ProtocolBase.pack` (``protocol.py:284``) + is ``self.__header__ = self.make(**kwargs)`` -- a *bound* call on the + outer ``HTTP`` instance, which is what actually reaches the inner, + previously-unbound ``protocol.make(**kwargs)``. So the reachable + reproduction is ``HTTP(version=1, ...)``, not ``HTTP.make(...)``: + + - on the unfixed code, this raises ``TypeError: HTTP.make() missing + 1 required positional argument: 'self'`` -- the *same* exception + text as the wrong reproduction, but reached legitimately this time. + - on the fixed code, deliberately minimal keyword arguments (no + ``method``/``status``) reach ``HTTPv1.make`` and are rejected + *there*, as ``ProtocolError: HTTP/1: invalid format`` -- proving + the callee was actually reached, which "no ``TypeError``" alone + would not. + """ + from pcapkit.protocols.application.http import HTTP + from pcapkit.utilities.exceptions import ProtocolError + + with self.assertRaises(ProtocolError) as ctx: + HTTP(version=1, http_version='1.1', method='GET', uri='/') + self.assertEqual(str(ctx.exception), 'HTTP/1: invalid format') + def test_http_read_explicit_version_uses_same_buffer_as_guess(self) -> None: """Regression test for GH-447. @@ -762,6 +830,61 @@ def test_httpv2_frame_constructors_cover_all_frame_types_and_branches(self) -> N with self.assertRaises(ProtocolError): proto.make(type=Frame.DATA, frame=object()) + def test_settings_frame_settings_field_wraps_item_schema(self) -> None: + """Regression test for GH-459. + + ``SettingsFrame.settings`` used to pass the bare ``SettingPair`` + *class* as ``ListField``'s ``item_type``, where every sibling + (``tcp.py``'s ``SACK.sack``, ``hip.py``, ``mh.py``'s + ``CGAParametersOption.parameters``, ``sctp.py``'s + ``gap_blocks``/``dup_tsn``) wraps its schema item in a + :class:`~pcapkit.corekit.fields.misc.SchemaField`. A bare + ``SchemaMeta`` is not a field instance, so ``ListField.unpack``'s + schema branch -- ``field = self._item_type(packet)`` -- constructed a + ``SettingPair`` from the packet *dict* instead of configuring a + per-item field, and the ``isinstance(self._item_type, SchemaField)`` + check that picks the schema branch was ``False`` for a bare class in + the first place, so it fell through to the plain-field branch and + failed there instead: ``field.length`` does not exist on a + ``SettingPair`` instance. + + This is a unit-level check on the field wiring, not an end-to-end + ``HTTPv2`` round trip: the SETTINGS frame's pack path still dies + earlier on ``KeyError: 'flags'``, raised by ``FrameType.post_process`` + at ``schema/application/httpv2.py:144`` where it reaches the enclosing + header's ``flags`` field through a nested packet context that cannot + see it (GH-445, fixed by the still-open PR #457), so a real + ``SettingsFrame.pack()``/``HTTPv2(...).make()`` round trip through this + field remains unreachable until that lands. The + ``httpv2-frame/SETTINGS`` entry in ``EXPECTED_FAILURES`` records that + same ``KeyError: 'flags'``. + """ + from pcapkit.corekit.fields.misc import SchemaField + from pcapkit.protocols.schema.application.httpv2 import SettingPair, SettingsFrame + + field = SettingsFrame.__fields__['settings'] + + # The type-check the ``# type: ignore[arg-type]`` used to silence: + # ``item_type`` must be a field instance, not the schema class itself. + self.assertIsInstance(field._item_type, SchemaField) + self.assertIs(field._item_type.schema, SettingPair) + + # Two SETTINGS pairs, 6 octets each: HEADER_TABLE_SIZE=4096, + # ENABLE_PUSH=0. + raw = (1).to_bytes(2, 'big') + (4096).to_bytes(4, 'big') \ + + (2).to_bytes(2, 'big') + (0).to_bytes(4, 'big') + packet = {'__length__': len(raw)} + + settings = field(packet).unpack(raw, packet) + + self.assertEqual(len(settings), 2) + self.assertIsInstance(settings[0], SettingPair) + self.assertIsInstance(settings[1], SettingPair) + self.assertEqual(settings[0].id, 1) + self.assertEqual(settings[0].value, 4096) + self.assertEqual(settings[1].id, 2) + self.assertEqual(settings[1].value, 0) + def test_httpv2_callable_frame_registry_paths(self) -> None: from pcapkit.const.http.frame import Frame from pcapkit.protocols.application.httpv2 import HTTP as HTTPv2