fix(http): identify the HTTP version before parsing, not by trial and error (#800)

[JarryShaw]

Please follow the guide below

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort) — all three run on the changed modules, no new findings
• make test passes, and a test case covers the change — tests/protocols/application/ scope; the full suite OOMs on this machine
• Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A — changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

• fix — corrects a defect
• feat — adds a feature
• perf — changes performance, not behaviour
• refactor — changes neither behaviour nor performance
• test — tests only
• docs — documentation only
• ci — workflows or build tooling
• chore — anything else

---

Description of your pull request and other information

Closes #800.

_guess_version now identifies before it parses. Measured on 4530424df vs this branch:

payload	before	after
preface + SETTINGS	ProtocolError: unknown HTTP version	version='2', SETTINGS parsed
preface alone (24 octets)	ProtocolError: unknown HTTP version	ProtocolError: HTTP/2: connection preface with no frame
b'foo bar baz\r\nX: y\r\n\r\n'	ProtocolError: unknown HTTP version	unchanged — and now by identification, not by #802's length guard
bare SETTINGS (mid-stream)	version='2'	unchanged, via the fall-through
Upgrade: h2c request / 101	version='1.1'	unchanged
malformed HTTP/1 (no-colon field)	unknown HTTP version	HTTP: invalid format, from the version identified

The preface is skipped rather than fed to httpv2, and counts as header, so info is byte-identical to reading that frame alone — without which the injected packet=self.packet.payload sliced from octet 9 of a buffer whose frame starts at 24 and reported preface remnants as payload.

Out of scope, deliberately: Upgrade: h2c needs per-connection state (one payload here, no flow context), and a mid-stream frame is undecidable — no type <= 9 heuristic, since that is what misfires on binary HTTP/1 bodies. Both are documented in _guess_version and pinned by tests.

Protochain over all 23 sample captures (1604 frames, 231 HTTP-bearing) is byte-identical to 4530424df; pytest tests/protocols/application/ 118 passed / 420 subtests, cross-checked at 59 tests OK under unittest; coverage 100% on http.py and httpv1.py.

[JarryShaw]

Two corrections from the author, both verified by me, and the second changes what this PR should be credited with.

1. My brief named the wrong base. I wrote f046b38f8; origin/main was already 4530424df (#803 landed in between). The author based on and measured against 4530424df, which is correct.

2. Garbage text does not read HTTP/2 on stock — #802 already closed that half. Measured by me on the real 4530424df:

PROVENANCE: /tmp/m4530/pcapkit/__init__.py
garbage text b'foo bar baz\r\nX: y\r\n\r\n'  -> ProtocolError: unknown HTTP version
real preface + SETTINGS                       -> ProtocolError: unknown HTTP version

So #800's headline had two halves and only one was live. The version=2-on-garbage figure in the issue was measured pre-#802, and #802's schema.length > length check already fixed it — any text payload's first three ASCII octets declare ≥ 2,105,376. The live defect was the false negative: a real HTTP/2 connection opening read as not-HTTP at all, which this PR fixes. The author said so unprompted rather than claiming both halves, and labelled its own garbage-text test a regression guard rather than a fix demonstration. That is the right call and I am recording it so #800 is not closed on an overstated basis.

Two defects its own probes caught that would otherwise have shipped, both worth noting because each would have been invisible in review:

• The start-line predicate claimed the preface. The preface is deliberately a well-formed HTTP/1.1 request line, so testing only the first line returned True for it — mirroring read()'s \r\n\r\n split first is what fixes it, since the preface's header is PRI * HTTP/2.0 with no CRLF left. Only arm ordering was hiding it; the agreement test found it.
• info.packet leaked the preface tail. ProtocolBase.__init__ injects packet=self.packet.payload sliced at self.length, so with the frame at octet 24 and length 9, twenty-four octets of preface remnant came back as payload. Fixed with _preface_length so info is byte-identical to reading the frame alone.

Coverage discipline worth crediting: the first pass after the fix dropped http.py to 98% (lines 318-319, the struct.error normalisation), and rather than leave it the author added the preface + 16-octet GOAWAY case — the #805 residual — to reach 100%/100% again.

Cross-review running. Upgrade: h2c and mid-stream both correctly left out of scope, with a passing regression test pinning h2c as HTTP/1.1 so a later "h2c support" attempt has to change a test that explains why not.

[JarryShaw]

Cross-review verdict: GOOD TO GO (sonnet; author was opus). All seven load-bearing claims independently measured against both trees, nothing left unverified in budget.

The preface is now positively identified, and the diagnosis is honest. preface + SETTINGS before → ProtocolError: unknown HTTP version; after → version='2', info.packet == b'', info.length == 9 — not 5265993. preface alone after → ProtocolError: HTTP/2: connection preface with no frame, a distinct answer rather than a generic one. The 24-byte compare runs before any import of httpv1/httpv2, so the trial parse is strictly downstream.

The false-positive attack went further than I asked and the fix holds. Beyond POST/PUT/PATCH/PROPFIND, the reviewer tried PRIVATE — a method literally starting with PRI — and b'PRI / HTTP/1.1\r\nHost: x\r\n\r\n', the reserved method one octet from a false positive. Both correctly return version='1.1', because the compare is an exact 24-byte match rather than a method-prefix heuristic.

Corpus regression clean, and it instrumented the corollary rather than inferring it. 23 captures, total_frames=1604 http_frames=231 on both trees, full per-frame protochain diff byte-for-byte empty. Then it instrumented _guess_version directly and confirmed it is entered 0 times across the whole corpus — tcp.py:330-331 binds httpv1.HTTP for TCP:80/8080, and no fixture uses UDP:80/8080. So all real coverage of this fix is synthetic, and the corpus-identical result is a regression guard with zero coverage of the new path. Worth stating in the PR body, which currently only implies it.

Scope discipline confirmed: no frame-header heuristic anywhere, httpv2.py untouched, and the trial-parse fall-through moved down but otherwise byte-identical. httpv1.py's +65 lines are one pure function in a single hunk, not called from within httpv1.py at all, so read()/_read_http_header() are untouched.

Fail-before is 6 of 8, with the two exceptions named and legitimate. test_guess_version_reaches_http2_on_the_connection_preface (the explicit version=2 path, untouched by this fix) and test_guess_version_keeps_an_upgrade_h2c_exchange_on_http1 (h2c out of scope) both pass on stock — each self-declared in its own docstring as a regression pin rather than a fix demonstration. Honest intentional exception, not a hidden gap. Full file on head: 59/59 under unittest.

One discrepancy, non-load-bearing: the body claims 420 pytest subtests, measured 388. unittest's 59/59 matches the body and is the authority here.

The reviewer caught its own probe artefact and said so — its first synthetic frame helper used payload-only length instead of this library's whole-frame convention, producing a false "preface+SETTINGS still fails" that contradicted the PR's table. It fixed it by diffing against the test file's real http2_frame_bytes helper rather than trusting its own construction. Twelfth artefact this session, and the fourth caught by the agent that made it.

Flipping to review: good-to-go.