fix(http): check the buffer's length, not just the declared one, in HTTP/2's frame guard

[JarryShaw]

Please follow the guide below

• Searched for similar pull requests
• Followed the coding style (make pylint, make mypy, make isort)
• make test passes, and a test case covers the change
• Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md — 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 #799.

httpv2.py's guard tested only the declared 24-bit length off the wire, never
the buffer. HTTP.unpack now rejects a non-empty buffer under nine octets
before the schema layer runs (an implicit-length, genuinely exhausted stream
still raises StreamEOFError, unchanged), and read's guard now also
requires schema.length <= length — a nine-octet buffer declaring 16777215
no longer parses and reports that length as fact.

A buffer that clears nine octets can still carry a frame whose own
fixed-width fields exceed what's left (GOAWAY at 9-16 octets, PUSH_PROMISE
at 9-12, padded DATA/HEADERS/PUSH_PROMISE), crashing the schema layer
with a bare struct.error. An earlier revision of this PR narrowed
_guess_version's struct.error suppression on the strength of a sweep that
never hit this class; a cross-review measured it (1136 escapes) and it's
reverted — the root cause is generic Schema.unpack/FieldBase.length
machinery shared by ten schema modules, filed as #805 rather than widened
here.

Two pre-#799 tests relied on this exact leniency to label garbage/the HTTP/2
preface as version='2' — updated to reflect that #799 correctly refuses
both now.

[JarryShaw]

Cross-review verdict: NEEDS CHANGES (haiku; author was sonnet). The unpack guard is sound and well-tested — it is the _guess_version narrowing that regresses. I verified both blocking findings myself rather than relay them.

1. The narrowed suppress(ProtocolError) lets a bare struct.error escape HTTP(). One 16-octet GOAWAY, b'\x00\x00\x15\x07\x00\x00\x00\x00\x00' + b'\xff'*7:

BASE  HTTP(data, 16)            -> ProtocolError: unknown HTTP version   [ProtocolError -> BaseError -> ValueError]
HEAD  HTTP(data, 16)            -> error: bad char in struct format      [error -> Exception -> BaseException]
      HTTP(data, 16, version=2) -> ProtocolError: HTTP/2: invalid format    (both trees)

The MRO is the damning part: at head the escape is not a ValueError and cannot be caught as a protocol error — the exact contract #789 was raised to repair, and which http.py:120-127 argues for in its own comment. The guard only protects the outer 9-octet header; frames whose inner payload has a fixed width wider than the remainder (GOAWAY at buflen 9–16, PUSH_PROMISE 9–12, any over-padded DATA/HEADERS) still drive pkt['__length__'] negative into struct.calcsize('-Ns'). These are ordinary snaplen-truncation shapes. The review's grid puts it at 1136 new HTTP()-route escapes.

Why the original sweep measured zero: its nine byte patterns land on frame types 0x00/0xff/0x03, none of which has a short fixed-width payload field. Reproducing that shape gives 0 escapes; swapping in three patterns with a real type octet at offset 3 gives 88. Fifth instance this session of a probe that could not detect what it looked for.

2. #799's headline harm survives, unchanged. Verified on both trees:

buflen=9 declared=16777215 -> PARSED, reports length=16777215     (identical base and head)

Also true at buflens 10, 12, 16, 24 — overstated by 16,777,206 octets. The guard compares each quantity against the constant 9, never against the other. So Closes #799 overstates what lands.

Required: fix the root cause — a negative field length should raise ProtocolError in Schema.unpack/FieldBase.length, which closes both classes and makes the narrowing true — or drop the narrowing for now. Plus: correct the NOTE and the Raises: docstring (both now false), add a real-bytes regression test (both new arm tests use mock.patch, so they cannot see this), and state the residual rather than closing #799 on it.

Mitigation worth noting: Protocol.analyze catches bare Exception → Raw, so end-to-end extraction is unaffected on both trees. The blast radius is the direct HTTP() API.

[JarryShaw]

Revised to 3d85e56f6. Both blocking findings verified closed by me, on a git archive export of the new head:

PROVENANCE: /tmp/w802n/pcapkit/__init__.py
-- 1: the 16-octet GOAWAY --
  HTTP(data,16)            -> ProtocolError: unknown HTTP version    [ProtocolError -> BaseError -> ValueError]
  HTTP(data,16,version=2)  -> ProtocolError: HTTP/2: invalid format
-- 2: declared >> buffer --
  buflen=9,10,12,16,24  declared=16777215 -> ProtocolError x5   (all five PARSED before, reporting 16777215)
-- boundary --
  declared=9  buflen=9 -> PARSED        declared=10 buflen=9 -> ProtocolError
-- EOF signal --
  HTTPv2(b"") -> StreamEOFError         [StreamEOFError -> BaseError -> EOFError]

Blocker 1 was resolved by dropping the narrowing rather than by widening the guard, which is the right call: arm 2 goes back to suppress(ProtocolError, struct.error), and the root cause is filed as #805. The author checked the blast radius first and found ten schema modules keying a field length off pkt['__length__'] the same way — ftp, httpv1, httpv2, ngap, hip, ipv6_route, mh, ethernet, pcapng, sctp — which is too broad for this PR and is exactly why #805 exists. schema.py is also held by #788 right now, so touching it here would have collided.

A consequence worth noting: the Raises: docstring needed no separate fix — reverting the narrowing made it true again on its own.

Blocker 2 closed by adding schema.length > length to read()'s guard, which is the check #799 actually asks for rather than the length-floor it had.

Tightening it broke two pre-existing #787 tests, and the reason is the interesting part: both had been passing only because of the bug #799 fixes. test_http_guess_version_falls_through_to_the_http2_arm read b'not http at all''s first three bytes as declared length 7,237,492 against 15 real octets, and test_guess_version_reaches_http2_on_the_connection_preface read the preface's b'PRI' as 5,265,993. Both were asserting reachability via a frame header that nothing backed. Now fixed to self-consistent fixtures, with explicit assertions that the old inputs correctly raise.

The defeated structural test was deleted rather than patched, since its premise (both arms suppress only ProtocolError) no longer holds — right call over keeping a weak check. A real-bytes regression test replaces the mock.patch-only pair, and both changed files now reach 100% line+branch with test_http_unit.py alone, so the earlier directory-wide caveat is gone.

Flipping review: needs-changes → review: pending: the old verdict was pinned to the dead 958e3edae. Delta re-check dispatched to the same reviewer, which still holds its 18,144-case grid.

[JarryShaw]

Cross-review verdict at 3d85e56f6: GOOD TO GO (haiku delta re-check; author sonnet). Delivered inside the 25-minute budget with all five priorities done. I re-derived the decisive claim with my own self-testing grid, across the three patterns the original sweep could not reach:

                                  OLD 958e3edae      NEW 3d85e56f6
GOAWAY 0x07            HTTPv2     SSSSSSSSPPPPPPPP   SSSSSSSSPPPPPPPP
GOAWAY 0x07            HTTP()     SSSSSSSSPPPPPPPP   ........PPPPPPPP
PUSH_PROMISE 0x05      HTTPv2     SSSSPPPPPPPPPPPP   SSSSPPPPPPPPPPPP
PUSH_PROMISE 0x05      HTTP()     SSSSPPPPPPPPPPPP   ....PPPPPPPPPPPP
PADDED DATA 0x00/0x08  HTTPv2     SSSSSSSSSSSSSSSS   SSSSSSSSSSSSSSSS
PADDED DATA 0x00/0x08  HTTP()     SSSSSSSSSSSSSSSS   ................
struct.error escapes:             HTTPv2=28 HTTP()=28   HTTPv2=28 HTTP()=0

S = bare struct.error, . = ProtocolError, P = parsed, columns buflen 9…24. The HTTPv2-direct row staying non-zero is the self-test — a blind probe would read 0 on both rows, so the 0 on the HTTP() route is load-bearing rather than an artefact. The reviewer's full grid agrees at scale: base 1136 escapes / 0 via HTTP(), old head 2272 / 1136, new head 1136 / 0 — exactly base.

It also re-ran the 18,144-case sub-9 grid despite the unpack restructure into 0 < len(self) < 9 / elif length < 9: 18,144/18,144 uniform ProtocolError, and the same script still detects 7,272 struct.error + 296 parses on base.

Both rewritten #787 tests fail against true base, so they assert something real rather than merely passing. The first keeps the load-bearing entered == ['httpv1', 'httpv2'] assertion and swaps only the outcome clause; the second asserts in both directions and its docstring points the remaining gap at #800 instead of claiming a fix. No other fixture relies on an unbacked declared length — seven HTTP/2-touching files enumerated and run, all green, and test_httpv2_payload_length_unit.py's builder is (len(payload) + 9).to_bytes(3, 'big'), self-consistent by construction.

The deleted structural test lost nothing. Its replacement uses real wire bytes on the un-mocked guess path — the exact gap the old one had — and is load-bearing: run against the old-head library it errors with struct.error, i.e. it catches the original regression. Its assertions include assertNotIsInstance(struct.error), which is what makes the narrowing unrepeatable. Arm 1's "must not suppress" pin survives separately, so both arms still have a guard.

HTTPv2(b'') → StreamEOFError restored, verified across three trees so the check has a known positive and negative: old head gave ProtocolError there, base and new head give StreamEOFError. Explicit length=0 deliberately stays ProtocolError (base raised struct.error, so still an improvement) and the asymmetry is argued in code.

Coverage caveat gone: http.py 68/0/16/0 100% and httpv2.py 320/0/128/0 100% standalone with test_http_unit.py alone — the previously-uncovered line 224 is now reached. 52 passed / 369 subtests at head; against true base, 9 distinct methods fail.

Residual, stated not hidden: 1136 struct.error escapes remain via directly-constructed HTTPv2 — unchanged from base, correctly attributed to the negative-field-length root, documented in both the unpack docstring and the _guess_version NOTE with the 16-octet GOAWAY spelled out, and filed as #805. One scoping note for #805: read()'s new schema.length > length clause cannot prevent these, because the crash happens inside Schema.unpack during unpack() and read() never runs — so #805 must not be scoped as a read() fix.

UNVERIFIED and accepted: pylint, mypy and protochain identity were not re-run. The diff adds comments plus one comparison clause and one if/elif; the first pass established an identical message set, and no sample capture reaches httpv2 at all.

Flipping to review: good-to-go.