fix(http): reach _guess_version's HTTP/2 arm, and resolve PayloadField names - #789
Conversation
|
Cross-review at 1. The HTTP/2 arm is still dead for a whole input class, 30 lines below the fix. Pre-existing, but the obs-fold form is real wire input, and the PR's own test docstring claims it covers "both of the unpackings that a non-HTTP/1 payload lands short on". There are three, and the third is not a 2. A regression the PR introduces: stdlib It is the bare stdlib 3. The blast-radius reasoning names TCP and omits UDP — where it actually lands. So The core fix is right and belongs in Two smaller items to fold in: |
779d4f7 to
b9cf62f
Compare
|
Revised to So the bare Its two design choices, both justified by measurement rather than preference:
Finding 4 had a side effect worth having: the positional- Finding 5: both numbers were wrong, mine included. Neither 91 nor 96 reproduces against any selection — that directory collects 95, the touched files 68. Replaced with figures that name their selection: 340 tests / 2080 subtests across the three mandated paths (was 335/2063), 73 / 157 for the touched files, agreeing exactly under pytest and unittest. My diagnosis of why 91 was wrong is confirmed exactly: with It also merged current Label back to |
|
Delta re-check at The strongest evidence it produced was unprompted: full Counts reproduced exactly: 340/2080 vs 335/2063, 73/157 agreeing under pytest and unittest, Three items it raised as non-blocking, and I am routing all three — two are real rather than cosmetic:
Unprompted find, out of scope for #787 but a real bug: six sites in Holding |
b9cf62f to
714b864
Compare
|
Revised to Item 1, verified myself on the new head: I relayed the reviewer's two disagreeing pairs; pre-fix it was 4 of 5, with HTAB surviving in two. The new test asserts folded-vs-literal equivalence over a table rather than against hand-written values — which is the property RFC 9112 §5.2's remedy is actually defined by, and a better test than the one I asked for. Item 2, verified myself — a fault in arm 1 now propagates instead of becoming HTTP/2: So the narrowing removed the mislabel without unfixing finding 2. The worker also rewrote the comment around the real principle — an arm that is not last hands the payload onward when it swallows, so only the last arm may widen — which is a better rule than "narrow arm 1". It checked arm 2 unprompted, since my Item 3: four classes, not three. My count conflated the field-less request and response. Table in the body now, numeral dropped, each cause attributed. And it volunteered a bound on its own strongest evidence, which I want on the record: the 23-capture protochain diff is Counts: 342/2093 on the mandated selection, 75/170 on the touched files agreeing exactly under pytest and unittest, both changed modules at 100% line and branch, mypy 0 in either file, pylint 8.99 with no new finding. Re-review dispatched on a different model; holding |
|
Delta re-check at So the A false positive it fell into and corrected, worth recording. Its first instrumentation patched the proxy's It also caught that its own first pass used Two corrections to numbers I relayed:
Both mutation checks bit exactly: reverting the Two items I am routing rather than shipping — details on the thread. |
…d names - httpv1.HTTP raised a bare exception for any payload that is not an HTTP/1 message, at four sites: the header/body split in read, the header's CRLF split and the start-line split in _read_http_header, and item[1] on a field line with no colon. HTTP._guess_version falls through on ProtocolError alone, so the HTTP/1 attempt aborted the guess rather than failing it and the HTTP/2 arm below was dead code -- for every one of those four input classes, not just the first. All four now raise ProtocolError, which is what each method's Raises: section already documented; the three unpacking sites chain their cause. - Those four sites are the whole of it, and a test pins the invariant rather than the list: over a battery spanning both methods, nothing escapes httpv1.HTTP that is not a ProtocolError. A fifth bare-raising site would fail there, which is what keeps the set of affected payloads derivable. - An obs-fold continuation line (RFC 9112 5.2) is legal HTTP/1 and is now unfolded, the RFC's own remedy, rather than read as a field of its own: with no colon it raised that IndexError, and with one it parsed in silence into a spurious extra field. The production is OWS CRLF RWS and the whole of it is replaced by one space, so the accumulator is right-stripped too -- otherwise the OWS before the CRLF survived into the value and a folded message disagreed with its literal equivalent, HTAB included. - A payload under nine octets reached httpv2.HTTP for the first time once the arm was reachable, and usually fails inside the schema with a bare struct.error -- neither a ValueError nor a ProtocolError nor pcapkit's StructError, so no caller could catch it. read converts it, and _guess_version suppresses it on the last arm only: an arm that is not last hands the payload onward when it swallows, so widening the HTTP/1 arm turned an injected fault on valid HTTP/1.1 bytes into a confident version='2'. Nothing reaches a struct.error through httpv1 anyway, 450 payload/route pairs tried. The residual is stated in the comment: a real httpv2 schema defect now reports as unknown HTTP version rather than crashing. That defect is httpv2.HTTP's and predates this change -- HTTPv2 built directly still raises it, and is left for its own review. - re.split's maxsplit is passed by keyword at both sites: positionally it is a DeprecationWarning on 3.13+ and is documented to become a TypeError, which would revive #787 from a third exception type. - PayloadField.protocol looked a str up in __proto__ as given, while the registry is keyed on the upper-cased class name, so a name not already upper-case resolved to None and silently yielded Raw. It now folds case, and warns with RegistryWarning for a name that is genuinely unregistered. - PayloadField.__init__ wrote _protocol directly, bypassing that setter, so a name given to the constructor was stored verbatim and handed back as a str. It now assigns through the property. - Four existing tests pinned the old behaviour and are updated rather than silenced: two asserted 'HTTP/1: invalid format', the message HTTP.read produced when it re-labelled a ValueError it no longer sees; one asserted that malformed HTTP/1 bytes leave _guess_version as a ValueError; and one pinned the answer the reachable arm gave for garbage rather than that the arm was reached, which would have made "garbage reports HTTP/2" a contract. Refs #787. 343 tests and 2117 subtests green across tests/protocols/application/, tests/corekit/ and tests/protocols/test_construction_keyword_check_unit.py with captures generated; the three files this touches are 76 tests and 194 subtests, agreeing exactly under pytest and unittest. http.py and httpv1.py at 100% line and branch. Protochains over all 23 sample captures, 1604 frames, are byte identical to the base revision's.
714b864 to
11bb769
Compare
|
Revised to I told it the arm-2 sweep was 81, not 76, and that "81 is forced: 9 patterns × 9 lengths (0–8)". That arithmetic assumed every sub-9 cell raises So 81 − 5 = 76, its original number. And the reason is a real sub-finding worth having: It also corrected my diagnosis of the pylint number. I said the 8.99 was pylint's cached previous-run score. Wrong — the footer reads Item 1 landed as a derivation, and the answer is 4 rows, not 5. It enumerated every raise-capable operation in base Two things the site-keyed view exposed that the example-keyed one hid: my "field-less request" and "field-less response" rows were the same site, and that site's members include the HTTP/2 preface where And a test now bounds it instead of a list. Both routed comment items are in, including the arm-3 clause. Counts: 343/2117 on the mandated selection, 76/194 on the touched files agreeing under both runners, both changed modules 100% line and branch, protochain still |
|
Delta re-check at It re-enumerated the sites itself before reading the author's list and landed on the same four, then attributed every payload by traceback frame on base. Two things that settles:
And it exhaustively verified the The 76-vs-81 question is settled, and both numbers were right about different things. Its nine patterns × nine sub-9 lengths = 81 cells that all raise — of which 76 are pylint settled too: 9.47 vs 9.51 is purely file scope — 9.47 for the two changed modules, 9.51 with One thing it flagged as derived rather than observed, which I want on the record: it did not re-run the 1604-frame protochain extraction, because the source delta has no non-comment line and Counts: 343/2117 mandated, 76/194 touched files agreeing under both runners, Setting |
…k included `main` moved from 73f09ae to 4530424 while this PR sat open, and the 1.5.0 section cited none of the 25 commits in between. Ten new bullets cover thirteen of them, appended in merge order, with the file's own `**a breaking change**` lead sentence on the three that are breaking: - #754 -- AppType split into per-transport registries; the 1,004 portless and 704 transportless rows stop being members. Breaking. - #764 -- an out-of-range port in `AppType.get` is refused, not minted, so `TCP.make(srcport=99999)` raises; per-transport `_missing_` spans. Breaking. - #778 -- `@final` enforced at runtime on `Info`/`Schema`: bare `@final` raises `InfoError`/`SchemaError` at first construction, deriving from a finalised class raises, and `SchemaError` is a `ValueError` where a caller may have been catching `TypeError`. Breaking. - #772 (with #790's docstring reword), #766, #759, #787, #794 (with #791's citation repoint), #792/#798 and #802 -- the remaining seven. Also re-ran the citation sweep against `origin/main` rather than the checkout. One stale line number fixed: the `httpv2.py` `header.length != 9` guard the `#692` entry calls out moved from `:562` to `:650` under #789 and #802. The preamble's "reaching #726" becomes #805, the new maximum reference. Verified unmoved on 4530424: `protocol.py:1411`, `schema/internet/ipv4.py:336`, `traceflow.py` 146/149/162/424, the four `:type:` fields in `engine.rst`, `reassembly.rst` and `traceflow.rst`, and `EXPECTED_FAILURES` at 43 entries. Carries the previous round's #651/#646 corrections unchanged. Two literals were reflowed so no ``literal`` wraps a line, which the generator's residual guard refuses. `changelog_md.py --check` exit 0; `test_changelog_md.py` 47 passed, 37 subtests.
Please follow the guide below
make pylint,make mypy,make isort)make testpasses, and a test case covers the changedocs/source/changelog/and regeneratedCHANGELOG.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) #657What is the purpose of your pull request?
fix— corrects a defectfeat— adds a featureperf— changes performance, not behaviourrefactor— changes neither behaviour nor performancetest— tests onlydocs— documentation onlyci— workflows or build toolingchore— anything elseDescription of your pull request and other information
Both defects in #787, plus three more the cross-review found in the same code.
Defect 1 — chose normalising at the source (
httpv1.HTTPraisesProtocolError), not suppressingValueErrorin_guess_version. A bareValueErrorout of that constructor is never meaningful: bothreadand_read_http_headeralready documentRaises: ProtocolError, and #583's_RE_STATUSfix already treated a bareValueErrorfrom this method as the defect. No test or caller depends on the type —beholderand_import_next_layerboth catchException, so the Raw fallback is unaffected. Ruled out suppressingValueError:ProtocolErroris aValueErrorsubclass, so that widens the net to every stray stdlibValueErrorand would hide real bugs as "unknown HTTP version" — and it would fix only the proxy, leaving the bare error ontcp.py's ports 80/8080, which dispatch straight toHTTPv1and never reach_guess_version. No port binding touched.Two further exception types were keeping the same arm dead, and are fixed here rather than left as caveats. A field line with no colon left
re.splitone element long, soitem[1]raisedIndexError— not aValueError, so neither the new guard norsuppress(ProtocolError)caught it. Anobs-foldcontinuation line (RFC 9112 §5.2) is exactly such a line and is legal HTTP/1 that real captures carry, so it is unfolded — the RFC's own remedy — and only a genuinely colon-less line is refused; refusing the folded form instead would have left a legal request falling through to the HTTP/2 arm, which is the mislabel this PR exists to stop. Measured on the previous head,b'GET / HTTP/1.1\r\nX-Long: a\r\n b\r\nHost: e\r\n\r\n'raisedIndexError: list index out of rangeboth directly and through the proxy, and a continuation that did carry a colon was worse still — it parsed in silence into a spurious extra field (X-Long: aplusb: c, for a foldedX-Long: a b), with nothing raised at all. The unfold replaces the wholeobs-fold = OWS CRLF RWS, so the accumulator is right-stripped as well as the continuation: stripping only the continuation left the OWS before the CRLF inside the value, and four of five folded/literal pairs disagreed (X: a \tover\tbgave'a \t b'for a literal'a b'), with a HTAB surviving where the RFC prescribes SP.Separately, a payload under nine octets reaches
httpv2.HTTPonce the arm is live, and failed there with a barestruct.error— neither aValueError, aProtocolError, nor pcapkit's ownStructError, soexcept ValueError,except ProtocolErrorandexcept BaseErrorall missed it.readconverts it, and_guess_versionsuppresses it besideProtocolErroron the HTTP/2 arm only. The asymmetry is load-bearing: that arm is last, so nothing follows to answer in its place, whereas suppressing on the HTTP/1 arm hands the payload onward to an arm that accepts anything of ≥9 octets. An earlier revision of this PR widened both, and it was measured to be actively harmful —StructErrorsubclassesstruct.error, so with a fault injected at arm 1 a valid HTTP/1.1 request came backversion='2', and over UDP/80 itsprotochainreadUDP:HTTP/2;unknown HTTP versionis only the best case, needing arm 2 to decline too. It also buys nothing: nine byte patterns × lengths 0–24 × both HTTP/1 routes gaveProtocolError450 times out of 450, and a sweep of arm 2 found itsstruct.errorhits are all stdlib-only and all at sizes 0–8, so noStructError.eofsignal is erased there either. Precisely: of the 81 sub-9 cells (9 patterns × lengths 0–8), 76 raisestruct.errorand 5 parse successfully —httpv2.HTTP's ownschema.length < 9guard tests the declared length, not the buffer's, sob'\x00\x00\x0f\x04'(4 octets declaring 15) parses. The comment in_guess_versionsays "usually" for that reason. Zero of the 81 raise pcapkit'sStructError. The residual is stated there too, and is real: a genuinehttpv2schema defect now reports asunknown HTTP versionrather than crashing, which is accepted becausehttpv2.HTTPstays reachable directly where nothing is suppressed. The underlying defect ishttpv2.HTTP's and predates this PR —HTTPv2built directly still raises it, andHTTP(..., version=2)did too, on the base revision as much as on this one — so it is fixed at this dispatcher's boundary, where both routes into a versioned parser already normalise their failures; a length guard in_guess_versionwas rejected because it would have left the explicitversion=2route leaking identically and would have put RFC 7540's 9-octet frame size in the dispatcher that exists not to know it. Finally,re.split'smaxsplitis passed by keyword at both sites: positionally it is aDeprecationWarningon 3.13+ and is documented to become aTypeError, a third exception type that would revive #787 verbatim. It was also firing on every HTTP parse — warnings across the test selection below drop from 8824 to 41.Defect 2 — case-folds, and does warn. A miss here is reached only from a caller that named a protocol in source, so it is a mistake in that name, not a property of the packet, and
Rawis otherwise indistinguishable from an unparsed payload. It stays a warning (RegistryWarning) rather than a raise becauseNone→Rawis legitimate for a field with no protocol. No in-library call site passes a string, so this adds no noise. Also: the issue's own reproduction goes through__init__, which assigned_protocoldirectly and bypassed the setter —PayloadField(protocol='http').protocolwas'http'itself, and'HTTP'was no better, so case was never what that path went wrong on.__init__now assigns through the property._guess_versionis on the real extraction path, so the remaining consequence is aprotochainmislabel inpcapkit.extract()output — not an API-only curiosity. An earlier revision of this description argued the blast radius was narrow because TCP ports 80 and 8080 dispatch straight toHTTPv1. That is true but incomplete: UDP ports 80 and 8080 dispatch to this proxy, not toHTTPv1—UDP.__proto__maps both topcapkit.protocols.application.http— so_guess_versionruns during ordinary extraction. The affected set is derived, not enumerated. Earlier revisions of this description counted it by example and it grew 1 → 3 → 4 on successive reviews, each round finding a class the last had missed — which is a bad way to establish a blast radius. It is derivable instead:So the set is indexed by the bare-exception raise sites, and those are exactly the four this PR converts — verified by walking every raise-capable operation in
readand_read_http_headeron the base revision and attributing each by traceback frame. One row per site makes the table complete by construction:read:packet.split(b'\r\n\r\n')— no separatorb'not http at all'HTTP/2_read_http_header:header.split(b'\r\n')— header with no CRLFHTTP/2_read_http_header:re.split(rb'\s+', startline, maxsplit=2)— start line under 3 tokensb'GET /\r\nHost: e\r\n\r\n',b'PRI\r\nHost: e\r\n\r\n'HTTP/2_read_http_header:item[1]— field line with no colonb'GET / HTTP/1.1\r\nNoColonHere\r\n\r\n'HTTP/2Site 3 is the one the enumerations kept missing: it is not "field-less" (it has a field section), not colon-less, and not fairly called "non-HTTP bytes". Note the site keys the row, but whether
HTTP/2is wrong depends on the payload — for the preface in site 2 it is the correct answer and the whole point of #787; for a field-less HTTP/1 message in the same site it is a mislabel. Over UDP/80 the mislabelling members readUDP:HTTP/2where base readUDP:Raw.Causes, per site: sites 2's HTTP/1 members are a pre-existing
_read_http_headerdefect and a candidate follow-up, deliberately not fixed here because the CRLF split that refuses them is load-bearing — relaxing it makes the prefacePRI * HTTP/2.0parse as a valid HTTP/1 request (_RE_METHODmatchesPRI,_RE_VERSIONmatchesHTTP/2.0) and revives this very bug. Site 1 is inherent to a reachable second arm, sincehttpv2.HTTPaccepts any payload of ≥9 octets with an unassigned frame type being expected traffic per RFC 9113 — any fix for #787 has it. Sites 3 and 4 are the narrow price of refusing a malformed message rather than dropping part of it.The candidate set partitions exhaustively and disjointly into three outcomes, measured base-vs-head over a 16-payload battery: 8 →
HTTP/2(the table), 1 →HTTP/1.1(the obs-folded request, which this PR now parses correctly — an improvement, not a mislabel), and 1 →ProtocolError(sub-9-octet payloads, which changed only in which exception they raise and are now catchable). The four classes that already raisedProtocolErroron base — an unrecognised 3-token start line, a non-numeric status, a lowercase method, a bad version token — are correctly excluded: they already answeredHTTP/2before this PR and are unchanged by it.Rather than pin that table, a test pins the invariant that bounds it:
test_httpv1_never_lets_a_bare_exception_escapeasserts that over a battery spanning both methods, nothing escapeshttpv1.HTTPthat is not aProtocolError. A fifth bare-raising site fails there, so a sixth class cannot appear unnoticed. Against the base library that test reports 13 subtest failures.End-to-end check. Protochains for every frame of all 23 sample captures — 1604 frames, of which 231 are HTTP — are byte-identical across the PR base, the previous revision and this one. That bounds the changes above to input classes the sample captures do not contain; note the captures reach HTTP over TCP, which dispatches straight to
HTTPv1, so they do not exercise the proxy path itself.Existing HTTP tests, before → after. Baseline at
0419c1c97(captures generated): 59 passed. Four tests observed the old behaviour and are updated rather than silenced, each with the reason in its docstring: two asserted'HTTP/1: invalid format', the messageHTTP.readproduced when it re-labelled aValueErrorit no longer sees; one asserted that malformed HTTP/1 bytes leave_guess_versionas aValueError, the defect stated as the contract; and one pinnedversion == '2'forb'not http at all', which makes "garbage must report HTTP/2" a contract and would block a laterhttpv2tightening — it now asserts that the second arm was entered, which is the property #787 is about, with the answer for real HTTP/2 bytes pinned bytest_guess_version_reaches_http2_on_the_connection_prefacewhere it belongs.Counts, with the selection named so they can be reproduced. The
91 tests / 152 subtestsquoted earlier was wrong and should not have been reported as green: it was a run in which five capture-dependent tests intest_http_runtime.pywere failing for want of generated captures (verified — exactly those five fail whenexamples/captures/is absent). With captures generated: 343 tests / 2117 subtests pass acrosstests/protocols/application/,tests/corekit/andtests/protocols/test_construction_keyword_check_unit.py, against 335 / 2063 before this PR's revisions; the three files this PR touches are 76 tests / 194 subtests, agreeing exactly underpytestandunittest(76 = 76, checked both ways, sincepytest-subtestscan report a parent node as passed when only its subTests fail).http.pyandhttpv1.pyare both at 100% line and branch, new statements and branches included.misc.pymeasures 72% on that selection and is unchanged by this revision — the earlier66% → 69%was against a narrower one.mypyreports 0 findings in either changed file (95 tree-wide, unchanged).pylintunder the project's ownPYLINT_FLAGS— which disabledesign, so noR09xxappears — gives 9.47/10 against a 9.43 baseline at the first revision of this PR, with the finding set byte-identical (10 findings, all pre-existing kinds:import-outside-toplevel,protected-access,attribute-defined-outside-init,arguments-renamed,unused-argument,cyclic-import). An earlier revision of this description quoted 8.99 and atoo-many-localsmovement; that came from a barepylintrun using defaults rather than the project's flags, under whichtoo-many-localsis not enabled at all.isort -l100 -ppcapkitis clean.