diff --git a/pcapkit/protocols/application/httpv2.py b/pcapkit/protocols/application/httpv2.py index 5066786a2..d978dff39 100644 --- a/pcapkit/protocols/application/httpv2.py +++ b/pcapkit/protocols/application/httpv2.py @@ -335,7 +335,7 @@ def make(self, # type: ignore[override] flags: 'Flags' = 0, # type: ignore[assignment] reserved: 'int' = 0, sid: 'int' = 0, - frame: 'bytes | Data_HTTP | Schema_FrameType | dict[str, Any]' = b'', + frame: 'Optional[bytes | Data_HTTP | Schema_FrameType | dict[str, Any]]' = None, payload: 'bytes | ProtocolBase | Schema' = b'', **kwargs: 'Any') -> 'Schema_HTTP': """Make (construct) packet data. @@ -349,7 +349,10 @@ def make(self, # type: ignore[override] constructor derives from ``frame``. reserved: Reserved bit of the frame header. sid: Stream ID of HTTP/2 frame. - frame: Frame data of HTTP/2 frame. + frame: Frame data of HTTP/2 frame. Omitting it, i.e. :data:`None`, + builds the frame type's own default payload, exactly as ``{}`` + does. An explicit :obj:`bytes` value, ``b''`` included, is + packed as raw octets instead. payload: Octets after the frame, e.g. the next frame. **kwargs: Arbitrary keyword arguments. @@ -358,15 +361,26 @@ def make(self, # type: ignore[override] .. versionchanged:: 1.5.0 - ``reserved`` is a new parameter before ``sid``, so a 1.4.x - positional argument meant for ``sid`` or ``frame`` lands one - parameter early (:issue:`1246`). + * ``reserved`` is a new parameter before ``sid``, so a 1.4.x + positional argument meant for ``sid`` or ``frame`` lands one + parameter early (:issue:`1246`). + * ``frame`` defaults to :data:`None` and dispatches to the frame + type's constructor, since the former ``b''`` default is shorter + than the payload a frame type of fixed or minimum size needs, so + such a frame could not be built at all (:issue:`2274`). """ type_val = cast('Enum_Frame', self._make_index(type, type_default, namespace=type_namespace, reversed=type_reversed, pack=False)) + if frame is None: + # A frame type whose payload is of fixed or minimum size cannot be + # built from empty octets, so an omitted frame dispatches to that + # type's own constructor, as ``{}`` does (#2274). An explicit + # ``b''`` stays raw. + frame = {} + if isinstance(frame, bytes): frame_val = frame # type: bytes | Schema_FrameType elif isinstance(frame, (dict, Data_HTTP)): @@ -1298,15 +1312,22 @@ def _make_http_settings(self, frame: 'Optional[Data_SettingsFrame]' = None, *, settings: Settings, as raw octets, an :class:`~pcapkit.corekit.multidict.OrderedMultiDict`, a plain :obj:`dict` of identifier to value or a list of - ``(identifier, value)`` pairs. + ``(identifier, value)`` pairs. :data:`None`, the default, + carries no parameters at all. **kwargs: Arbitrary keyword arguments. Returns: Constructed frame schema and updated flags. Raises: - ProtocolError: If ``settings`` is not :obj:`bytes`, a :obj:`dict` - or a :obj:`list`, :data:`None` (the default) included. + ProtocolError: If ``settings`` is not :obj:`bytes`, a :obj:`dict`, + a :obj:`list` or :data:`None`. + + .. versionchanged:: 1.5.0 + + ``settings`` of :data:`None`, the default, builds a ``SETTINGS`` + frame with no parameters, which :rfc:`9113#section-6.5` permits, + rather than being rejected (:issue:`2274`). """ if frame is not None: @@ -1317,8 +1338,13 @@ def _make_http_settings(self, frame: 'Optional[Data_SettingsFrame]' = None, *, if ack: flags |= Schema_SettingsFrame.Flags.ACK - if isinstance(settings, bytes): - settings_val = settings # type: bytes | list[Schema_SettingPair] + if settings is None: + # A SETTINGS frame carrying no parameters is valid (RFC 9113 + # ยง6.5), so the default builds one rather than being rejected + # (#2274). + settings_val = [] # type: bytes | list[Schema_SettingPair] + elif isinstance(settings, bytes): + settings_val = settings elif isinstance(settings, dict): settings_val = [] # A plain ``dict`` has no ``multi`` keyword (#2223). diff --git a/tests/protocols/application/test_http_message_prefix_2298_unit.py b/tests/protocols/application/test_http_message_prefix_2298_unit.py index 1a7cff877..186fb5205 100644 --- a/tests/protocols/application/test_http_message_prefix_2298_unit.py +++ b/tests/protocols/application/test_http_message_prefix_2298_unit.py @@ -112,7 +112,9 @@ def test_settings_names_the_rejected_value(self) -> None: from pcapkit.protocols.application.httpv2 import HTTP from pcapkit.utilities.exceptions import ProtocolError - for settings, tail in ((42, '42'), (None, 'None'), ('x', "'x'"), (4.5, '4.5')): + # ``None`` is no longer rejected: it builds a parameterless ``SETTINGS`` + # frame (#2274), pinned in test_httpv2_frame_default_2274_unit.py. + for settings, tail in ((42, '42'), ('x', "'x'"), (4.5, '4.5')): with self.subTest(settings=settings): with self.assertRaises(ProtocolError) as ctx: bytes(HTTP(type=Frame.SETTINGS, frame={'settings': settings})) diff --git a/tests/protocols/application/test_http_unit.py b/tests/protocols/application/test_http_unit.py index 33cc3564f..0df1fe6ac 100644 --- a/tests/protocols/application/test_http_unit.py +++ b/tests/protocols/application/test_http_unit.py @@ -2316,8 +2316,10 @@ def test_httpv2_frame_constructors_cover_all_frame_types_and_branches(self) -> N settings=OrderedMultiDict([(Setting.MAX_FRAME_SIZE, 16384)])), ) self.assertEqual(settings_dict_schema.settings[0].id, Setting.MAX_FRAME_SIZE) + settings_none_schema, _ = proto._make_http_settings(settings=None) + self.assertEqual(list(settings_none_schema.settings), []) with self.assertRaises(ProtocolError): - proto._make_http_settings(settings=None) + proto._make_http_settings(settings=42) push_schema, push_flags = proto._make_http_push_promise( end_headers=True, diff --git a/tests/protocols/application/test_httpv2_error_default_2311_unit.py b/tests/protocols/application/test_httpv2_error_default_2311_unit.py index 2240e942b..c3b51044b 100644 --- a/tests/protocols/application/test_httpv2_error_default_2311_unit.py +++ b/tests/protocols/application/test_httpv2_error_default_2311_unit.py @@ -8,10 +8,11 @@ :rfc:`9113#section-7` gives a condition that is not the result of an error. An explicit ``error`` is still honoured. -Each frame is built with ``frame={}``, because the default ``frame=b''`` does -not build these fixed-payload frames at all (#2274). Every case builds its own -octets in memory and reads no capture. Classes are imported inside each test, -after :func:`~tests._support.reimport_once_per_class`. +Each frame is built with ``frame={}``, which dispatches to the frame type's own +constructor; since #2274 the default ``frame=None`` takes that dispatch too, so +it builds the same octets. Every case builds its own octets in memory and reads +no capture. Classes are imported inside each test, after +:func:`~tests._support.reimport_once_per_class`. """ diff --git a/tests/protocols/application/test_httpv2_frame_default_2274_unit.py b/tests/protocols/application/test_httpv2_frame_default_2274_unit.py new file mode 100644 index 000000000..687d81537 --- /dev/null +++ b/tests/protocols/application/test_httpv2_frame_default_2274_unit.py @@ -0,0 +1,174 @@ +# -*- coding: utf-8 -*- +"""``HTTP.make`` builds every HTTP/2 frame type without a ``frame`` argument. + +GitHub issue #2274: ``make`` defaulted ``frame`` to ``b''``, which is packed as +raw octets with Length 0. A frame type whose payload is of fixed or minimum size +is longer than that, so ``HTTP(type=, sid=...)`` could not build one at all: +``PRIORITY``, ``RST_STREAM``, ``PING`` and ``WINDOW_UPDATE`` raised +``HTTP/2: [Type ] invalid format`` and ``PUSH_PROMISE`` and ``GOAWAY`` raised +``Field resolved to a negative length``, each after a ``SchemaWarning: +packet length < 0``. ``frame={}``, which dispatches to the frame type's own +constructor, built them. + +``frame`` now defaults to :data:`None` and takes that dispatch, so the default +call and ``frame={}`` give the same octets for every frame type. An explicit +:obj:`bytes` value, ``b''`` included, is still packed raw. ``SETTINGS`` needed +the other half of the fix: ``_make_http_settings`` rejected its own +``settings=None`` default, so ``frame={}`` raised there instead, and +:data:`None` now builds the parameterless ``SETTINGS`` frame +:rfc:`9113#section-6.5` permits. + +Every case builds its own octets in memory and reads no capture. Classes are +imported inside each test, after :func:`~tests._support.reimport_once_per_class`. + +""" + +import unittest +from typing import Any + +from tests._support import reimport_once_per_class + +#: Octets ``HTTP(type=, sid=)`` builds, by frame type name and Stream +#: Identifier. Every one of them is also what ``frame={}`` builds. +DEFAULT_FRAMES = { + 'DATA': (1, bytes.fromhex('000000000000000001')), + 'HEADERS': (1, bytes.fromhex('000000010000000001')), + 'PRIORITY': (1, bytes.fromhex('000005020000000001' '00000000' '00')), + 'RST_STREAM': (1, bytes.fromhex('000004030000000001' '00000000')), + 'SETTINGS': (0, bytes.fromhex('000000040000000000')), + 'PUSH_PROMISE': (1, bytes.fromhex('000004050000000001' '00000000')), + 'PING': (0, bytes.fromhex('000008060000000000' '0000000000000000')), + 'GOAWAY': (0, bytes.fromhex('000008070000000000' '00000000' '00000000')), + 'WINDOW_UPDATE': (1, bytes.fromhex('000004080000000001' '00000000')), + 'CONTINUATION': (1, bytes.fromhex('000000090000000001')), +} + +#: An unassigned frame type, whose payload has no fixed size, so the default and +#: ``frame={}`` agreed on it before the fix as well. +UNASSIGNED_TYPE = 0xfa + + +class TestHTTPv2DefaultFrame(unittest.TestCase): + """Pin what ``make`` builds when ``frame`` is left out.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def build(self, type: 'Any', sid: 'int', **kwargs: 'Any') -> 'Any': + """Build the HTTP/2 frame of ``type`` on stream ``sid``.""" + from pcapkit.protocols.application.httpv2 import HTTP + + return HTTP(type=type, sid=sid, **kwargs) + + def test_default_builds_every_frame_type(self) -> None: + from pcapkit.const.http.frame import Frame + + for name, (sid, wire) in DEFAULT_FRAMES.items(): + with self.subTest(frame=name): + self.assertEqual(bytes(self.build(Frame[name], sid)).hex(), wire.hex()) + + def test_default_agrees_with_an_empty_mapping(self) -> None: + from pcapkit.const.http.frame import Frame + + for name, (sid, _) in DEFAULT_FRAMES.items(): + with self.subTest(frame=name): + self.assertEqual(bytes(self.build(Frame[name], sid)).hex(), + bytes(self.build(Frame[name], sid, frame={})).hex()) + + def test_default_frames_round_trip(self) -> None: + from pcapkit.const.http.frame import Frame + from pcapkit.protocols.application.httpv2 import HTTP + + for name, (sid, wire) in DEFAULT_FRAMES.items(): + with self.subTest(frame=name): + made = bytes(self.build(Frame[name], sid)) + parsed = HTTP(made, len(made)) + self.assertEqual(parsed.info.type, Frame[name]) + self.assertEqual(parsed.info.sid, sid) + self.assertEqual(bytes(HTTP.from_data(parsed.info)).hex(), wire.hex()) + + def test_an_unassigned_type_is_unchanged(self) -> None: + wire = bytes.fromhex('000000fa0000000001') + for label, kwargs in (('default', {}), ('mapping', {'frame': {}}), ('bytes', {'frame': b''})): + with self.subTest(frame=label): + self.assertEqual(bytes(self.build(UNASSIGNED_TYPE, 1, **kwargs)).hex(), wire.hex()) + + def test_explicit_bytes_are_still_packed_raw(self) -> None: + from pcapkit.const.http.frame import Frame + + # Raw octets are packed as given, rather than through the frame type's + # constructor, so they need not be the payload that type would build. + made = bytes(self.build(Frame.RST_STREAM, 1, frame=b'\x00\x00\x00\x08')) + self.assertEqual(made.hex(), '000004030000000001' '00000008') + self.assertEqual(bytes(self.build(Frame.DATA, 1, frame=b'abc')).hex(), + '000003000000000001' '616263') + + def test_empty_bytes_still_build_a_zero_length_frame(self) -> None: + from pcapkit.const.http.frame import Frame + + # ``b''`` is an explicit request for no payload, which is what a frame + # type of no fixed size builds anyway. + for name in ('DATA', 'HEADERS', 'SETTINGS', 'CONTINUATION'): + with self.subTest(frame=name): + sid, wire = DEFAULT_FRAMES[name] + self.assertEqual(bytes(self.build(Frame[name], sid, frame=b'')).hex(), wire.hex()) + + def test_explicit_empty_bytes_stay_raw_on_a_fixed_size_type(self) -> None: + from pcapkit.const.http.frame import Frame + from pcapkit.protocols.application.httpv2 import HTTP + + # An explicit ``b''`` is still packed raw, even on a frame type whose + # default (``None``) dispatches to a constructor with a non-empty + # payload -- it must not be swept into that dispatch. Built via + # ``make`` directly, bypassing the round-trip parse that a length-0 + # PING would legitimately fail (PING's payload is a fixed 8 octets). + schema = HTTP.__new__(HTTP).make(type=Frame.PING, frame=b'') + self.assertEqual(int(schema.length), 0) + self.assertEqual(schema.frame, b'') + + +class TestHTTPv2EmptySettings(unittest.TestCase): + """Pin the ``SETTINGS`` frame ``_make_http_settings`` builds from ``None``.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def test_none_builds_a_parameterless_settings_frame(self) -> None: + from pcapkit.const.http.frame import Frame + from pcapkit.protocols.application.httpv2 import HTTP + + for label, frame in (('default', {}), ('explicit', {'settings': None})): + with self.subTest(settings=label): + made = bytes(HTTP(type=Frame.SETTINGS, sid=0, frame=frame)) + self.assertEqual(made.hex(), DEFAULT_FRAMES['SETTINGS'][1].hex()) + self.assertEqual(HTTP(made, len(made)).info.length, 0) + + def test_the_schema_carries_no_entries(self) -> None: + from pcapkit.protocols.application.httpv2 import HTTP + + schema, flags = HTTP.__new__(HTTP)._make_http_settings() + self.assertEqual(list(schema.settings), []) + self.assertEqual(int(flags), 0) + + def test_an_empty_settings_frame_still_takes_the_ack_flag(self) -> None: + from pcapkit.const.http.frame import Frame + from pcapkit.protocols.application.httpv2 import HTTP + + made = bytes(HTTP(type=Frame.SETTINGS, sid=0, frame={'ack': True})) + self.assertEqual(made.hex(), '000000040100000000') + + def test_other_settings_types_are_still_rejected(self) -> None: + from pcapkit.const.http.frame import Frame + from pcapkit.protocols.application.httpv2 import HTTP + from pcapkit.utilities.exceptions import ProtocolError + + for settings, tail in ((42, '42'), ('x', "'x'"), (4.5, '4.5')): + with self.subTest(settings=settings): + with self.assertRaises(ProtocolError) as ctx: + bytes(HTTP(type=Frame.SETTINGS, frame={'settings': settings})) + self.assertEqual(str(ctx.exception), + f'HTTP/2: [Type 4] invalid settings: {tail}') + + +if __name__ == '__main__': + unittest.main()