Skip to content

Commit fb66c60

Browse files
authored
fix(httpv2): give make() a usable default for frame types with a fixed-size payload (#2395)
HTTP.make defaulted frame to b'', packed verbatim with Length 0, so PRIORITY, RST_STREAM, PUSH_PROMISE, PING, GOAWAY and WINDOW_UPDATE could not be read back and HTTP(type=<T>, sid=...) raised. HTTP._make_http_settings also raised on settings=None, though RFC 9113 section 6.5 permits a SETTINGS frame carrying no parameters. frame now defaults to None, dispatching to the frame type's own _make_http_* constructor -- the same as passing frame={} -- while an explicit b'' still packs raw, unchanged. _make_http_settings treats settings=None as no parameters rather than raising. Chosen over raising a clearer error: every other make default already produces a valid packet, so a well-worded exception on the default call would still have been an API wart dressed up as a diagnostic. Dispatching makes the default and frame={} byte-identical for every frame type, at no extra cost since each constructor already has working per-field defaults. Tests: test_httpv2_frame_default_2274_unit.py pins the octets all ten frame types emit by default, that they match frame={} and round-trip, that explicit bytes stay raw, and both empty and populated SETTINGS frames. test_http_unit.py's constructor sweep now pins settings=None building rather than raising, and test_http_message_prefix_2298_unit.py and test_httpv2_error_default_2311_unit.py are updated for prose and cases that pinned the old behaviour.
1 parent 00fd65b commit fb66c60

5 files changed

Lines changed: 221 additions & 16 deletions

File tree

‎pcapkit/protocols/application/httpv2.py‎

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,7 @@ def make(self, # type: ignore[override]
335335
flags: 'Flags' = 0, # type: ignore[assignment]
336336
reserved: 'int' = 0,
337337
sid: 'int' = 0,
338-
frame: 'bytes | Data_HTTP | Schema_FrameType | dict[str, Any]' = b'',
338+
frame: 'Optional[bytes | Data_HTTP | Schema_FrameType | dict[str, Any]]' = None,
339339
payload: 'bytes | ProtocolBase | Schema' = b'',
340340
**kwargs: 'Any') -> 'Schema_HTTP':
341341
"""Make (construct) packet data.
@@ -349,7 +349,10 @@ def make(self, # type: ignore[override]
349349
constructor derives from ``frame``.
350350
reserved: Reserved bit of the frame header.
351351
sid: Stream ID of HTTP/2 frame.
352-
frame: Frame data of HTTP/2 frame.
352+
frame: Frame data of HTTP/2 frame. Omitting it, i.e. :data:`None`,
353+
builds the frame type's own default payload, exactly as ``{}``
354+
does. An explicit :obj:`bytes` value, ``b''`` included, is
355+
packed as raw octets instead.
353356
payload: Octets after the frame, e.g. the next frame.
354357
**kwargs: Arbitrary keyword arguments.
355358
@@ -358,15 +361,26 @@ def make(self, # type: ignore[override]
358361
359362
.. versionchanged:: 1.5.0
360363
361-
``reserved`` is a new parameter before ``sid``, so a 1.4.x
362-
positional argument meant for ``sid`` or ``frame`` lands one
363-
parameter early (:issue:`1246`).
364+
* ``reserved`` is a new parameter before ``sid``, so a 1.4.x
365+
positional argument meant for ``sid`` or ``frame`` lands one
366+
parameter early (:issue:`1246`).
367+
* ``frame`` defaults to :data:`None` and dispatches to the frame
368+
type's constructor, since the former ``b''`` default is shorter
369+
than the payload a frame type of fixed or minimum size needs, so
370+
such a frame could not be built at all (:issue:`2274`).
364371
365372
"""
366373
type_val = cast('Enum_Frame',
367374
self._make_index(type, type_default, namespace=type_namespace,
368375
reversed=type_reversed, pack=False))
369376

377+
if frame is None:
378+
# A frame type whose payload is of fixed or minimum size cannot be
379+
# built from empty octets, so an omitted frame dispatches to that
380+
# type's own constructor, as ``{}`` does (#2274). An explicit
381+
# ``b''`` stays raw.
382+
frame = {}
383+
370384
if isinstance(frame, bytes):
371385
frame_val = frame # type: bytes | Schema_FrameType
372386
elif isinstance(frame, (dict, Data_HTTP)):
@@ -1298,15 +1312,22 @@ def _make_http_settings(self, frame: 'Optional[Data_SettingsFrame]' = None, *,
12981312
settings: Settings, as raw octets, an
12991313
:class:`~pcapkit.corekit.multidict.OrderedMultiDict`, a plain
13001314
:obj:`dict` of identifier to value or a list of
1301-
``(identifier, value)`` pairs.
1315+
``(identifier, value)`` pairs. :data:`None`, the default,
1316+
carries no parameters at all.
13021317
**kwargs: Arbitrary keyword arguments.
13031318
13041319
Returns:
13051320
Constructed frame schema and updated flags.
13061321
13071322
Raises:
1308-
ProtocolError: If ``settings`` is not :obj:`bytes`, a :obj:`dict`
1309-
or a :obj:`list`, :data:`None` (the default) included.
1323+
ProtocolError: If ``settings`` is not :obj:`bytes`, a :obj:`dict`,
1324+
a :obj:`list` or :data:`None`.
1325+
1326+
.. versionchanged:: 1.5.0
1327+
1328+
``settings`` of :data:`None`, the default, builds a ``SETTINGS``
1329+
frame with no parameters, which :rfc:`9113#section-6.5` permits,
1330+
rather than being rejected (:issue:`2274`).
13101331
13111332
"""
13121333
if frame is not None:
@@ -1317,8 +1338,13 @@ def _make_http_settings(self, frame: 'Optional[Data_SettingsFrame]' = None, *,
13171338
if ack:
13181339
flags |= Schema_SettingsFrame.Flags.ACK
13191340

1320-
if isinstance(settings, bytes):
1321-
settings_val = settings # type: bytes | list[Schema_SettingPair]
1341+
if settings is None:
1342+
# A SETTINGS frame carrying no parameters is valid (RFC 9113
1343+
# §6.5), so the default builds one rather than being rejected
1344+
# (#2274).
1345+
settings_val = [] # type: bytes | list[Schema_SettingPair]
1346+
elif isinstance(settings, bytes):
1347+
settings_val = settings
13221348
elif isinstance(settings, dict):
13231349
settings_val = []
13241350
# A plain ``dict`` has no ``multi`` keyword (#2223).

‎tests/protocols/application/test_http_message_prefix_2298_unit.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,9 @@ def test_settings_names_the_rejected_value(self) -> None:
112112
from pcapkit.protocols.application.httpv2 import HTTP
113113
from pcapkit.utilities.exceptions import ProtocolError
114114

115-
for settings, tail in ((42, '42'), (None, 'None'), ('x', "'x'"), (4.5, '4.5')):
115+
# ``None`` is no longer rejected: it builds a parameterless ``SETTINGS``
116+
# frame (#2274), pinned in test_httpv2_frame_default_2274_unit.py.
117+
for settings, tail in ((42, '42'), ('x', "'x'"), (4.5, '4.5')):
116118
with self.subTest(settings=settings):
117119
with self.assertRaises(ProtocolError) as ctx:
118120
bytes(HTTP(type=Frame.SETTINGS, frame={'settings': settings}))

‎tests/protocols/application/test_http_unit.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2316,8 +2316,10 @@ def test_httpv2_frame_constructors_cover_all_frame_types_and_branches(self) -> N
23162316
settings=OrderedMultiDict([(Setting.MAX_FRAME_SIZE, 16384)])),
23172317
)
23182318
self.assertEqual(settings_dict_schema.settings[0].id, Setting.MAX_FRAME_SIZE)
2319+
settings_none_schema, _ = proto._make_http_settings(settings=None)
2320+
self.assertEqual(list(settings_none_schema.settings), [])
23192321
with self.assertRaises(ProtocolError):
2320-
proto._make_http_settings(settings=None)
2322+
proto._make_http_settings(settings=42)
23212323

23222324
push_schema, push_flags = proto._make_http_push_promise(
23232325
end_headers=True,

‎tests/protocols/application/test_httpv2_error_default_2311_unit.py‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,11 @@
88
:rfc:`9113#section-7` gives a condition that is not the result of an error.
99
An explicit ``error`` is still honoured.
1010
11-
Each frame is built with ``frame={}``, because the default ``frame=b''`` does
12-
not build these fixed-payload frames at all (#2274). Every case builds its own
13-
octets in memory and reads no capture. Classes are imported inside each test,
14-
after :func:`~tests._support.reimport_once_per_class`.
11+
Each frame is built with ``frame={}``, which dispatches to the frame type's own
12+
constructor; since #2274 the default ``frame=None`` takes that dispatch too, so
13+
it builds the same octets. Every case builds its own octets in memory and reads
14+
no capture. Classes are imported inside each test, after
15+
:func:`~tests._support.reimport_once_per_class`.
1516
1617
"""
1718

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
# -*- coding: utf-8 -*-
2+
"""``HTTP.make`` builds every HTTP/2 frame type without a ``frame`` argument.
3+
4+
GitHub issue #2274: ``make`` defaulted ``frame`` to ``b''``, which is packed as
5+
raw octets with Length 0. A frame type whose payload is of fixed or minimum size
6+
is longer than that, so ``HTTP(type=<T>, sid=...)`` could not build one at all:
7+
``PRIORITY``, ``RST_STREAM``, ``PING`` and ``WINDOW_UPDATE`` raised
8+
``HTTP/2: [Type <T>] invalid format`` and ``PUSH_PROMISE`` and ``GOAWAY`` raised
9+
``Field <f> resolved to a negative length``, each after a ``SchemaWarning:
10+
packet length < 0``. ``frame={}``, which dispatches to the frame type's own
11+
constructor, built them.
12+
13+
``frame`` now defaults to :data:`None` and takes that dispatch, so the default
14+
call and ``frame={}`` give the same octets for every frame type. An explicit
15+
:obj:`bytes` value, ``b''`` included, is still packed raw. ``SETTINGS`` needed
16+
the other half of the fix: ``_make_http_settings`` rejected its own
17+
``settings=None`` default, so ``frame={}`` raised there instead, and
18+
:data:`None` now builds the parameterless ``SETTINGS`` frame
19+
:rfc:`9113#section-6.5` permits.
20+
21+
Every case builds its own octets in memory and reads no capture. Classes are
22+
imported inside each test, after :func:`~tests._support.reimport_once_per_class`.
23+
24+
"""
25+
26+
import unittest
27+
from typing import Any
28+
29+
from tests._support import reimport_once_per_class
30+
31+
#: Octets ``HTTP(type=<name>, sid=<sid>)`` builds, by frame type name and Stream
32+
#: Identifier. Every one of them is also what ``frame={}`` builds.
33+
DEFAULT_FRAMES = {
34+
'DATA': (1, bytes.fromhex('000000000000000001')),
35+
'HEADERS': (1, bytes.fromhex('000000010000000001')),
36+
'PRIORITY': (1, bytes.fromhex('000005020000000001' '00000000' '00')),
37+
'RST_STREAM': (1, bytes.fromhex('000004030000000001' '00000000')),
38+
'SETTINGS': (0, bytes.fromhex('000000040000000000')),
39+
'PUSH_PROMISE': (1, bytes.fromhex('000004050000000001' '00000000')),
40+
'PING': (0, bytes.fromhex('000008060000000000' '0000000000000000')),
41+
'GOAWAY': (0, bytes.fromhex('000008070000000000' '00000000' '00000000')),
42+
'WINDOW_UPDATE': (1, bytes.fromhex('000004080000000001' '00000000')),
43+
'CONTINUATION': (1, bytes.fromhex('000000090000000001')),
44+
}
45+
46+
#: An unassigned frame type, whose payload has no fixed size, so the default and
47+
#: ``frame={}`` agreed on it before the fix as well.
48+
UNASSIGNED_TYPE = 0xfa
49+
50+
51+
class TestHTTPv2DefaultFrame(unittest.TestCase):
52+
"""Pin what ``make`` builds when ``frame`` is left out."""
53+
54+
def setUp(self) -> None:
55+
reimport_once_per_class(self)
56+
57+
def build(self, type: 'Any', sid: 'int', **kwargs: 'Any') -> 'Any':
58+
"""Build the HTTP/2 frame of ``type`` on stream ``sid``."""
59+
from pcapkit.protocols.application.httpv2 import HTTP
60+
61+
return HTTP(type=type, sid=sid, **kwargs)
62+
63+
def test_default_builds_every_frame_type(self) -> None:
64+
from pcapkit.const.http.frame import Frame
65+
66+
for name, (sid, wire) in DEFAULT_FRAMES.items():
67+
with self.subTest(frame=name):
68+
self.assertEqual(bytes(self.build(Frame[name], sid)).hex(), wire.hex())
69+
70+
def test_default_agrees_with_an_empty_mapping(self) -> None:
71+
from pcapkit.const.http.frame import Frame
72+
73+
for name, (sid, _) in DEFAULT_FRAMES.items():
74+
with self.subTest(frame=name):
75+
self.assertEqual(bytes(self.build(Frame[name], sid)).hex(),
76+
bytes(self.build(Frame[name], sid, frame={})).hex())
77+
78+
def test_default_frames_round_trip(self) -> None:
79+
from pcapkit.const.http.frame import Frame
80+
from pcapkit.protocols.application.httpv2 import HTTP
81+
82+
for name, (sid, wire) in DEFAULT_FRAMES.items():
83+
with self.subTest(frame=name):
84+
made = bytes(self.build(Frame[name], sid))
85+
parsed = HTTP(made, len(made))
86+
self.assertEqual(parsed.info.type, Frame[name])
87+
self.assertEqual(parsed.info.sid, sid)
88+
self.assertEqual(bytes(HTTP.from_data(parsed.info)).hex(), wire.hex())
89+
90+
def test_an_unassigned_type_is_unchanged(self) -> None:
91+
wire = bytes.fromhex('000000fa0000000001')
92+
for label, kwargs in (('default', {}), ('mapping', {'frame': {}}), ('bytes', {'frame': b''})):
93+
with self.subTest(frame=label):
94+
self.assertEqual(bytes(self.build(UNASSIGNED_TYPE, 1, **kwargs)).hex(), wire.hex())
95+
96+
def test_explicit_bytes_are_still_packed_raw(self) -> None:
97+
from pcapkit.const.http.frame import Frame
98+
99+
# Raw octets are packed as given, rather than through the frame type's
100+
# constructor, so they need not be the payload that type would build.
101+
made = bytes(self.build(Frame.RST_STREAM, 1, frame=b'\x00\x00\x00\x08'))
102+
self.assertEqual(made.hex(), '000004030000000001' '00000008')
103+
self.assertEqual(bytes(self.build(Frame.DATA, 1, frame=b'abc')).hex(),
104+
'000003000000000001' '616263')
105+
106+
def test_empty_bytes_still_build_a_zero_length_frame(self) -> None:
107+
from pcapkit.const.http.frame import Frame
108+
109+
# ``b''`` is an explicit request for no payload, which is what a frame
110+
# type of no fixed size builds anyway.
111+
for name in ('DATA', 'HEADERS', 'SETTINGS', 'CONTINUATION'):
112+
with self.subTest(frame=name):
113+
sid, wire = DEFAULT_FRAMES[name]
114+
self.assertEqual(bytes(self.build(Frame[name], sid, frame=b'')).hex(), wire.hex())
115+
116+
def test_explicit_empty_bytes_stay_raw_on_a_fixed_size_type(self) -> None:
117+
from pcapkit.const.http.frame import Frame
118+
from pcapkit.protocols.application.httpv2 import HTTP
119+
120+
# An explicit ``b''`` is still packed raw, even on a frame type whose
121+
# default (``None``) dispatches to a constructor with a non-empty
122+
# payload -- it must not be swept into that dispatch. Built via
123+
# ``make`` directly, bypassing the round-trip parse that a length-0
124+
# PING would legitimately fail (PING's payload is a fixed 8 octets).
125+
schema = HTTP.__new__(HTTP).make(type=Frame.PING, frame=b'')
126+
self.assertEqual(int(schema.length), 0)
127+
self.assertEqual(schema.frame, b'')
128+
129+
130+
class TestHTTPv2EmptySettings(unittest.TestCase):
131+
"""Pin the ``SETTINGS`` frame ``_make_http_settings`` builds from ``None``."""
132+
133+
def setUp(self) -> None:
134+
reimport_once_per_class(self)
135+
136+
def test_none_builds_a_parameterless_settings_frame(self) -> None:
137+
from pcapkit.const.http.frame import Frame
138+
from pcapkit.protocols.application.httpv2 import HTTP
139+
140+
for label, frame in (('default', {}), ('explicit', {'settings': None})):
141+
with self.subTest(settings=label):
142+
made = bytes(HTTP(type=Frame.SETTINGS, sid=0, frame=frame))
143+
self.assertEqual(made.hex(), DEFAULT_FRAMES['SETTINGS'][1].hex())
144+
self.assertEqual(HTTP(made, len(made)).info.length, 0)
145+
146+
def test_the_schema_carries_no_entries(self) -> None:
147+
from pcapkit.protocols.application.httpv2 import HTTP
148+
149+
schema, flags = HTTP.__new__(HTTP)._make_http_settings()
150+
self.assertEqual(list(schema.settings), [])
151+
self.assertEqual(int(flags), 0)
152+
153+
def test_an_empty_settings_frame_still_takes_the_ack_flag(self) -> None:
154+
from pcapkit.const.http.frame import Frame
155+
from pcapkit.protocols.application.httpv2 import HTTP
156+
157+
made = bytes(HTTP(type=Frame.SETTINGS, sid=0, frame={'ack': True}))
158+
self.assertEqual(made.hex(), '000000040100000000')
159+
160+
def test_other_settings_types_are_still_rejected(self) -> None:
161+
from pcapkit.const.http.frame import Frame
162+
from pcapkit.protocols.application.httpv2 import HTTP
163+
from pcapkit.utilities.exceptions import ProtocolError
164+
165+
for settings, tail in ((42, '42'), ('x', "'x'"), (4.5, '4.5')):
166+
with self.subTest(settings=settings):
167+
with self.assertRaises(ProtocolError) as ctx:
168+
bytes(HTTP(type=Frame.SETTINGS, frame={'settings': settings}))
169+
self.assertEqual(str(ctx.exception),
170+
f'HTTP/2: [Type 4] invalid settings: {tail}')
171+
172+
173+
if __name__ == '__main__':
174+
unittest.main()

0 commit comments

Comments
 (0)