Skip to content

fix(http): tighten HTTP/2 version guessing so arbitrary binary is not taken for a DATA frame - #1153

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1141-http2-guess-tightening
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1141-http2-guess-tightening

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

  • Searched for similar pull requests
  • Followed the coding style (pylint/mypy with the Makefile flags on http.py, no new messages; isort --check-only on all four files)
  • make test passes, and a test case covers the change (per module only, listed below)
  • 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 #1141 (option 2). The fall-through in HTTP._guess_version now runs its HTTP/2 trial only if _test_http2_frame passes. That check requires an RFC 9113 frame type, no undefined flag, the reserved bit clear, and the stream that type is sent on (DATA, HEADERS, PRIORITY, RST_STREAM, PUSH_PROMISE and CONTINUATION on a non-zero stream; SETTINGS, PING and GOAWAY on stream 0; WINDOW_UPDATE on either). Explicit version=2 parsing is unchanged.

Probe: nine zero octets went from HTTP/2 type=0 to ProtocolError: unknown HTTP version. A seeded zero-heavy fuzz (5000 buffers) went from 566 HTTP/2 labels to 101, and every one left is DATA on a non-zero stream, which is a legal frame. All 18 legal frames built with Scapy's contrib.http2 (all ten types, plus ACK, padded and priority variants) are still guessed as HTTP/2.

Tests: the new test_http_guess_http2_1141_unit.py passes 5 of 5. With http.py reverted it fails 11 (9 rejected-header subtests, the nine-zero case and the fuzz). Scapy's frames are embedded as hex, because the test leg does not install Scapy. These modules pass: test_http_unit 65, test_tcp_http_dispatch_unit 3, test_http_runtime 5, the three test_httpv2_*_unit modules, test_option_coverage_runtime 3, test_option_roundtrip_unit 6, and tests/project 385 (1 skipped).

Existing tests edited: the test_tcp_http_dispatch_unit fixture was a stream-0 DATA frame, which is exactly the case this PR now rejects; it is now 000000000000000001. Two test_http_unit cases drove the HTTP/2 arm with non-frame bytes; they now use frame headers that pass the check, so they test the same thing as before.

… taken for a DATA frame

Since #1138 any nine octets with a small enough Length are a self-consistent
HTTP/2 frame, so the fall-through in HTTP._guess_version labelled zero-heavy
binary as HTTP/2: nine zero octets parsed as DATA on stream 0.

- Gate the HTTP/2 trial on _test_http2_frame: an RFC 9113 frame type, no
  undefined flag, reserved bit clear, and the stream that type is sent on.
- Explicit version=2 parsing is unchanged.
- test_tcp_http_dispatch_unit's fixture was a stream-0 DATA frame; it is now
  an empty DATA frame on stream 1. Two test_http_unit cases that drove the
  HTTP/2 arm with non-frame bytes now use frame headers that pass the gate.

Closes #1141
@JarryShaw JarryShaw added 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 a6a8d2b53: GOOD TO GO (ran on Sonnet; author Opus)

  • RFC 9113: every row of _HTTP2_FRAME_RULES matches §4.1 and §6, for each type's defined-flag mask and its stream rule.
  • Legal frames: 64 hand-built frames, covering every type, every legal flag subset, padded and unpadded, and the legal streams, all pass the gate and parse as httpv2. Explicit version=2 is unchanged.
  • Captures: across all 23 captures, only two payloads change. Both are random bytes in many_interfaces.pcapng that used to decode as unassigned frame types. No capture carries ALTSVC, ORIGIN or PRIORITY_UPDATE.
  • Tests: with http.py reverted, 11 tests fail. The fuzz is deterministic, 566 against 101, and its per-label subtests carry the real check. The fixture and test conversions are legitimate.

Inaccurate PR claim: options-transport.pcap frames 35–43 were already Raw before this PR. The generator still writes header-inclusive HTTP/2 Lengths, which #1138 rejects. The capture file is byte-identical either way. Added to #1156.

@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
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.76% (unit tier, Python 3.14, a6a8d2b53, 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 1873 91 578 22 94.33%
pcapkit/dumpkit 136 0 40 0 100.00%
pcapkit/foundation 2425 145 842 34 92.56%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15676 188 3956 163 98.18%
pcapkit/toolkit 487 65 144 1 85.42%
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 ad7e193 into main Oct 6, 2026
41 checks passed
@JarryShaw
JarryShaw deleted the fix/1141-http2-guess-tightening branch October 6, 2026 22:38
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

design(httpv2): should HTTP/2 version guessing be tightened after #1138?

1 participant