Repository navigation
http: _guess_version trial-parses instead of identifying, so it answers HTTP/2 for garbage text and for the preface by accident #800
Description
Activity
- addedbugIssues reporting a defect (set by the bug report template; a default, not an assessment)Issues reporting a defect (set by the bug report template; a default, not an assessment)fixPull requests that fix a defect (fix: subject prefix)Pull requests that fix a defect (fix: subject prefix)
on Sep 25, 2026 Labelling
blockedso it does not read as unheld work.Checkable blocker: #799 merged. Both changes edit
pcapkit/protocols/application/http.py's_guess_version, and #799 is in flight on it now — it narrows the arm-2 suppression back toProtocolErroronce the sub-9 class is uniform. Adding a preface test on top of a_guess_versionthat is about to change means writing against a moving target, then re-deriving.gh pr list -R JarryShaw/PyPCAPKit --search "799 in:body" --state all --json number,state,mergedAtAnd the order is load-bearing rather than tidy. #799 makes the sub-9 class uniformly
ProtocolError; only then does a preface test have a clean fall-through to sit in front of. Doing it the other way round means the preface test would be papering over an inconsistent arm 2 instead of complementing a clean one.Unblocks the moment #799 lands, and #682's repoint is blocked on this one — that dependency is written on #682.
- addedblockedDeferred pending another issue or decision; see the last comment for what unblocks itDeferred pending another issue or decision; see the last comment for what unblocks it
on Sep 25, 2026 Unblocked — #799 merged via #802 at 21:44:26Z as
f046b38f8. Itshttpv2guard now tests the buffer as well as the declared length, so the sub-9 class is uniform and_guess_versionhas a clean fall-through to sit in front of.Dispatching a worker. Two things #802 changed that this issue's body predates:
- Arm 2 is back to
suppress(ProtocolError, struct.error)— fix(http): check the buffer's length, not just the declared one, in HTTP/2's frame guard #802 dropped the narrowing rather than widening the guard, because the root cause needsSchema.unpackand is tracked as corekit: a negative field length only warns in Schema.unpack, instead of raising ProtocolError #805. So do not assume the narrowed form when writing the preface test. read()now rejectsschema.length > length, which is what made two pre-existing http: _guess_version's HTTP/2 arm is unreachable, and PayloadField.protocol does no case folding #787 tests fail — they had been passing only becauseb'not http at all'was read as declared length 7,237,492 and the preface'sb'PRI'as 5,265,993. Both are now self-consistent fixtures. That second number is exactly why this issue exists: the preface "worked" by being misparsed as a frame header.
The rest of the plan stands as written — prefix-compare the first 24 octets against
b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'before any parse attempt, leaveUpgrade: h2cexplicitly out of scope, and answerRawrather than guessing on a mid-stream segment. #682's repoint stays blocked on this.- Arm 2 is back to
- addedwipWork in flight - a covering PR is open or an agent is actively on itWork in flight - a covering PR is open or an agent is actively on itand removedblockedDeferred pending another issue or decision; see the last comment for what unblocks itDeferred pending another issue or decision; see the last comment for what unblocks it
on Sep 25, 2026 - added a commit that references this issue
on Sep 25, 2026 - removedwipWork in flight - a covering PR is open or an agent is actively on itWork in flight - a covering PR is open or an agent is actively on it
on Sep 25, 2026 - added 9 commits that reference this issue
on Sep 26, 2026
Metadata
Metadata
Assignees
Labels
Projects
- StatusShow more project fieldsDone
Describe the bug
_guess_versiondecides the HTTP version by trial-parsing: tryhttpv1, and if that declines, tryhttpv2. That answers "did a parser accept this?" when the question is "what is this?" — and it gets both directions wrong. Measured onmain:It reaches the right answer on the preface for the wrong reason, and the wrong answer on text that is not HTTP at all. The maintainer's requirement, verbatim from #682:
Expected behavior
A positive identification before any parse attempt: compare the first 24 octets against
b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n'. RFC 9113 §3.4 designed that sequence for exactly this purpose — it is deliberately a well-formed HTTP/1.1 request line whose methodPRIis reserved and unregistered, so an HTTP/1 parser rejects it and an HTTP/2 detector recognises it. A prefix compare cannot false-positive on valid HTTP/1 and needs no parse.Then fall through to the existing HTTP/1 start-line regexes, which are correctly anchored (
httpv1.py:60,:62,:72), and only then to a parse attempt.What this does not fix, and should not pretend to
The
Upgrade: h2cform (RFC 7540 §3.2, deprecated but not removed by RFC 9113 §3.1) is stateful and not expressible in this shape at all. On the wire the upgrade request is HTTP/1.1 and parses correctly today; the switch takes effect after the101, so deciding that later segments on the same 4-tuple are HTTP/2 needs per-connection state._guess_versionreceives one payload with no flow context. RecognisingUpgrade: h2cin a request is possible; acting on it is not.Likewise a mid-stream segment — a bare HTTP/2 frame header with no preface, or an opaque HTTP/1 body chunk — is genuinely undecidable from one payload. The honest outcome there is
Raw, not a coin-flip. Any heuristic on the 9-byte frame header ("type ≤ 9, reserved bit clear") will misfire on binary HTTP/1 bodies, which is precisely howb'foo bar baz…'is classified HTTP/2 today.Additional context
Sequencing — this is step 2 of 3, from the maintainer's ruling on #682 ("TCP:80 should use the proxy I think, since both HTTP/1 and HTTP/2 bind on them"):
httpv2's guard tests the declared length, not the buffer, so the sub-9 class behaves inconsistently. In flight.httpv1to the proxy.The order matters and is not cosmetic.
_guess_versionis entered 0 times across all 1604 frames of the fixture corpus today, because every HTTP frame arrives over TCP andtcp.py:330bindshttpv1directly. Repointing first would put 231 real HTTP/1 frames through a guess path that is known-wrong on non-HTTP input, trading an honestRawfor a confident wrongHTTP/2on the main traffic path.Also note the corpus cannot test this. No fixture uses UDP 80/8080 and none carries HTTP/2, so a preface test needs its own synthetic coverage — a real preface, a preface plus SETTINGS, garbage text, a mid-stream frame header, and an
Upgrade: h2cexchange asserted to stay HTTP/1.1.Related: #682, #799, #787, #789.