Skip to content

fix(httpv2)!: frame Length counts the 9-octet header, so RFC 9113 frames are rejected - #1138

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1121-httpv2-frame-length
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1121-httpv2-frame-length

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (pylint, mypy, isort on the three changed pcapkit/ files — clean apart from pre-existing warnings on untouched lines)
  • make test passes, and a test case covers the change — ran test_httpv2_frame_length_1121_unit, test_http_unit, test_httpv2_payload_length_unit, test_httpv2_negative_length_unit, test_tcp_http_dispatch_unit, test_option_roundtrip_unit, test_option_coverage_runtime, tests/project
  • Added a changelog entry — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1121.

make() wrote Length as _make_http_length(...) + 9 and every per-frame check was stated as payload + 9, while _read_http_priority checked 9 — so pcapkit rejected even its own PRIORITY frame. Length is now the payload alone, the 9-octet header excluded (RFC 9113 §4.1): PRIORITY 5, RST_STREAM 4, PING 8, WINDOW_UPDATE 4, PUSH_PROMISE ≥ 4, SETTINGS a multiple of 6 and 0 under ACK, and read requires the buffer to back nine octets plus the declared payload.

Breaking. make() now emits 000005 02 00 00000003 00000001 0f for a PRIORITY frame where it previously emitted 00000e …; captures written to the old header-inclusive convention no longer parse, and RFC-conformant ones do.

Probe, nine frames packed by scapy's independent scapy.contrib.http2, parsed and rebuilt:

before: PRIORITY/RST_STREAM/SETTINGS/SETTINGS-ACK/PING/WINDOW_UPDATE/DATA/PUSH_PROMISE
        -> ProtocolError; GOAWAY parsed but make() != wire
after:  all nine parse, report the payload length, and make() == the wire octets

Test evidence — new module tests/protocols/application/test_httpv2_frame_length_1121_unit.py, fixtures byte-identical to scapy's output. With pcapkit/protocols/application/httpv2.py reverted: 29 failed, 5 passed, 2 subtests passed. With the fix: 5 passed, 31 subtests passed.

Existing tests that pinned the old convention and were updated: test_http_unit, test_httpv2_payload_length_unit, test_httpv2_negative_length_unit (their http2_frame_bytes helpers and the per-frame length literals) and test_tcp_http_dispatch_unit (its options-transport.pcap fixture octets). EXPECTED_FAILURES['httpv2-frame/PRIORITY'] now passes and is removed.

…mes are rejected

``HTTP.make`` wrote Length as ``_make_http_length(...) + 9`` and ``read``
compared the declared value against the whole buffer, so every per-frame
check was stated as payload + 9 -- and ``_read_http_priority`` checked 9,
rejecting even the 14 ``make`` wrote for it.

Length is now the frame payload alone, the header excluded (RFC 9113 4.1):
PRIORITY 5, RST_STREAM 4, PING 8, WINDOW_UPDATE 4, PUSH_PROMISE >= 4,
SETTINGS a multiple of 6 and 0 under ACK, and ``read`` requires the buffer
to back nine octets plus the declared payload.

Breaking: ``make`` emits different Length octets, and a capture written to
the old convention no longer parses.

Closes #1121
@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on e1cce76cc: GOOD TO GO (ran on Sonnet; author Opus)

  • RFC 9113: every constant matches the fetched text. §4.1 excludes the 9 header octets from Length; PRIORITY is 5, RST_STREAM 4, PING 8, WINDOW_UPDATE 4; SETTINGS must be a multiple of 6 (0 with ACK); PUSH_PROMISE needs at least 4.
  • Scapy oracle: 16 frames built with scapy.contrib.http2, including padded DATA, HEADERS and PUSH_PROMISE, plus CONTINUATION and GOAWAY with debug data. All parse with info.length equal to the wire Length, and make() rebuilds each byte for byte.
  • Read guard: boundaries probed:
    • an empty payload in exactly 9 octets is accepted, and so is an exact fit;
    • a buffer one byte short is rejected;
    • every fixed-size frame at its length ±1 raises ProtocolError.
  • Tests: with httpv2.py reverted, the new module fails 29. Reverting only the guard's + 9 fails two tests. Every converted assertion in the four edited modules moved by exactly −9 and still tests the same rule.

Side effect, to note under breaking: declared lengths 0–8 are now valid, so the HTTP/1-vs-/2 fall-through classifies more arbitrary binary as HTTP/2 DATA. For example, 9 or more zero octets now parse as HTTP/2 type=0 instead of unknown HTTP version. In a 5000-buffer fuzz: 149 HTTP/2 labels on this PR against 39 on main. This follows from the RFC, since an empty DATA frame is legal, but it is a behaviour change.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.74% (unit tier, Python 3.14, e1cce76cc, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1035 2342 846 90.23%
pcapkit/corekit 1874 91 578 22 94.33%
pcapkit/dumpkit 136 0 40 0 100.00%
pcapkit/foundation 2422 143 842 34 92.62%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15653 188 3942 163 98.18%
pcapkit/toolkit 487 71 144 3 84.15%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit dbb30c4 into main Oct 6, 2026
42 checks passed
@JarryShaw
JarryShaw deleted the fix/1121-httpv2-frame-length branch October 6, 2026 20:40
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 2026
JarryShaw added a commit that referenced this pull request Oct 6, 2026
- 22 new 1.5.0 entries for the 30 PRs merged since b10e795
  (#1106-#1158), with related fixes grouped per the #918 ruling;
  three marked breaking to match their labels (#1116, #1138, #1145)
- regenerate CHANGELOG.md with util/changelog_md.py
- process.rst: entry-count pin re-measured, 167 -> 189
- pep.rst: note the plist round-trip test skipped on the upstream
  dictdumper fractional-second <date> defect (DictDumper#122)

tests/project: conventions_doc_claims, changelog_md and
documentation_claims pass; changelog_md.py --check is in step.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(httpv2)!: frame Length counts the 9-octet header, so RFC 9113 frames are rejected

1 participant