Skip to content

fix(tcp): repoint TCP:80/8080 at the HTTP version proxy (#682) - #821

Merged
JarryShaw merged 1 commit into
mainfrom
fix/682-repoint-tcp80-http-proxy
Sep 26, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/682-repoint-tcp80-http-proxy

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #682.

TCP.__proto__ bound httpv1.HTTP directly for ports 80 and 8080 (tcp.py:330-331 on 3cbdf8999), so a segment on either port was HTTP/1 by assertion of the port number. Both versions share those ports on the wire, so the port cannot decide the version and the payload has to. Repointed to pcapkit.protocols.application.http.HTTP, whose identification became positive in #800/#814 — which is why this waited.

udp.py already bound the proxy for both ports, so this removes the asymmetry its docstring and docs/source/pep.rst documented as an open request; both are updated.

Protochain over all 23 captures / 1604 frames is not byte-identical, and that is the fix. All 231 HTTP/1.1 frames keep their chain (0 lost an HTTP layer). Nine frames in options-transport.pcap change Ethernet:IPv4:TCP:Raw -> Ethernet:IPv4:TCP:HTTP/2: genuine HTTP/2 frames (self-consistent 9-octet headers, types 0-9, sid=0) built by this library's own httpv2.HTTP.make, previously refused by the HTTP/1 parser.

Instrumented _guess_version entry count over the corpus: 0 -> 252 (231 HTTP/1.1 + 9 HTTP/2 + 12 fall-throughs that stay Raw).

examples/generators/dispatch.py's PINNED_TARGETS follows the repoint, which is what tests/protocols/test_dispatch_registry_unit.py checks against.

One pre-existing failure, unrelated and out of scope: test_http_unit.py::test_guess_version_reports_a_preface_with_no_frame_as_such fails identically on stock 3cbdf8999.

@JarryShaw JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix) test Pull requests that add or correct tests (test: subject prefix) docs Pull requests that change documentation only (docs: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 26, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

These 10 red legs are not this PR's fault — they are main's, and I have filed them as #822.

Verified rather than assumed, because "the same test fails on the baseline" is exactly the claim a broken PR would also make:

  • This PR's only change to pcapkit/protocols/application/http.py is inside a # comment block — git diff origin/main..refs/pull/821/head -- pcapkit/protocols/application/http.py is 5 lines of comment prose, no statements. It does not touch tests/protocols/application/test_http_unit.py at all.
  • main at 3cbdf8999 fails the same 10 legs on the same test in its own run, 36201817798: test_guess_version_reports_a_preface_with_no_frame_as_such, "Field debug resolved to a negative length; template='-1s'" != 'HTTP/2: invalid format'.

#821 is simply the first PR whose branch was cut after #814 merged, which is why #817 and #820 — both based on pre-#814 main — are green on 3.13/3.14 while this is not. #822 has the mechanism and a worker is on it.

One correction to my own earlier note here: I flagged the udp.py change as unrequested code. It is prose only — the 80:/8080: rows appear as unchanged context in the diff, because UDP already bound the proxy on 3cbdf8999. And the _guess_version count is 0 → 252 (231 HTTP/1.1 + 9 HTTP/2 + 12 fall-throughs), not the 231 I quoted.

Cross-review is running; verdict to follow.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict: GOOD TO GO (sonnet; author was opus). All seven load-bearing claims confirmed by independent derivation, not by restating the author — including one probe the reviewer caught failing on itself (grep -P "\tHTTP" returned 0 on both trees; switched to grep -c "HTTP", validated against a known-negative control string).

Highlights it re-derived rather than accepted: the 9 changed frames are 35-43 1-indexed (the author's "34-42" is 0-indexed, same frames); 23 captures / 1604 frames enumerated with os.scandir so http6.cap is included; exactly 18 differing protochain lines across 1604×2, all of them those 9 frames; _guess_version instrumented at runtime to {'count': 252, 'v1': 231, 'v2': 9, 'raised': 12} against a baseline of {'count': 0, ...}; the PINNED_TARGETS revert reproducing exactly 4 FAIL: lines; 8 FAIL: lines on baseline counted with plain unittest, not pytest-subtests.

The nuance worth your attention, which I verified myself on origin/main. None of those 9 frames carries the RFC 9113 preface, so they do not reach HTTP/2 by positive identification — they land on _guess_version's fall-through arm. That is pre-existing and deliberate: the docstring at pcapkit/protocols/application/http.py:268-275 says a self-consistent bare frame "is still parsed as HTTP/2, but by the fall-through below -- on the parser's own length/type consistency rules -- rather than by a guess dressed up as identification", and explicitly refuses a stronger frame-header heuristic because "that misfires on binary HTTP/1 bodies".

So the one behavioural consequence of this PR: TCP:80/8080 traffic that is neither valid HTTP/1 nor preface-carrying is now exposed to that fall-through, where httpv1 alone would have left it Raw. A non-HTTP binary protocol producing a self-consistent 9+-octet pseudo-frame would come back HTTP/2. UDP has had this since before #682; TCP has not. Not a defect in this change and not something to fix here — but it is the real cost of the repoint, and it should be on the record before you merge.

The 10 red legs are #822's, inherited from main — see my note above.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 26, 2026
`TCP.__proto__` bound `httpv1.HTTP` directly for ports 80 and 8080, so a
segment on either port was HTTP/1 by assertion of the port number: an HTTP/2
payload there was refused by the HTTP/1 parser and reported as `Raw`. Both
versions share those ports on the wire, so the port cannot decide the version
and the payload has to.

* `pcapkit/protocols/transport/tcp.py` — ports 80 and 8080 now resolve to
  `pcapkit.protocols.application.http.HTTP`, which identifies the version
  before parsing (#800, landed via #814) rather than trial-parsing.
* `examples/generators/dispatch.py` — `PINNED_TARGETS` for `tcp/80` and
  `tcp/8080` follow the repoint.
* `pcapkit/protocols/transport/udp.py`, `docs/source/pep.rst` — the prose
  documenting the TCP/UDP asymmetry is now stale; both tables agree.
* `tests/protocols/transport/test_tcp_http_dispatch_unit.py` — new; pins the
  descriptor, the TCP/UDP parity, and that HTTP/2 on port 80/8080 decodes as
  HTTP/2 while HTTP/1.1 still decodes as HTTP/1.1.

All 231 HTTP/1.1 frames in the 23-capture corpus keep their chain; 9 frames in
`options-transport.pcap` change `TCP:Raw` -> `TCP:HTTP/2`, which is the defect
being fixed. `_guess_version` entry count over the corpus goes 0 -> 252.
@JarryShaw
JarryShaw force-pushed the fix/682-repoint-tcp80-http-proxy branch from 67b88a4 to 555c4fc Compare September 26, 2026 01:35
@JarryShaw

Copy link
Copy Markdown
Owner Author

Rebased onto main (21e9588af) and force-pushed: 67b88a4e9 → 555c4fcd2.

main was red on two defects when the verdict was granted here; both are now fixed on it (#820 as 319da66bb, #822 as 21e9588af), so the 10 red legs on this PR should clear on the new run.

The review: good-to-go carries over because the rebase is content-identical, not because a verdict existed: diff of git diff 3cbdf8999...67b88a4e9 against git diff origin/main...555c4fcd2 is empty. One commit, same six files.

Heads-up on a latent collision: this PR and #826 both touch pcapkit/protocols/application/http.py — this one a comment tense line, #826 a larger comment block. Whichever merges second will want a rebase.

@JarryShaw

Copy link
Copy Markdown
Owner Author

CI is complete and clean — ok=27 fail=0 inc=0 on 555c4fcd2 — so this is ready to merge.

The 10 red legs here were #822's, inherited from main; that fix landed as 21e9588af and I rebased this branch onto it. The rebase was content-identical (diff of the pre- and post-rebase diffs is empty), which is why the cross-review's GOOD TO GO carries rather than needing a fresh pass. main itself is green now too — 18 jobs success, 0 failed.

Ordering note, because it decides which of two PRs needs a rebase: this PR and #826 both touch pcapkit/protocols/application/http.py — here it is a single comment-tense line, there a rewritten comment block. Whichever merges second will need a rebase. #817 touches neither and is independent.

The behavioural caveat from the cross-review still stands and is on the record above: TCP:80/8080 traffic that is neither valid HTTP/1 nor preface-carrying is now exposed to _guess_version's fall-through arm, where httpv1 left it Raw. That is the documented cost of the repoint, not a defect in it.

@JarryShaw
JarryShaw merged commit c287152 into main Sep 26, 2026
31 checks passed
@JarryShaw
JarryShaw deleted the fix/682-repoint-tcp80-http-proxy branch September 26, 2026 02:45
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 26, 2026
JarryShaw added a commit that referenced this pull request Sep 26, 2026
…s in #817 and #821

Two bullets, both non-breaking, appended after the #800 entry in merge
order. Bullet count 128 to 130 (`grep -cE '^\* \*\*'`).

- #804 (PR #817) -- the three `__repr__` methods #798 left `%`-formatted
  are f-strings now, dropping `consider-using-f-string` from both const
  modules and both vendor templates; the other bespoke templates in
  `{const,vendor}/{ftp,http}/` still carry the disable, so #804's claim
  holds for this pair only, not for those directories.

- #682 (PR #821) -- `TCP.__proto__` no longer binds `httpv1.HTTP` directly
  for ports 80/8080; both repoint to the generic HTTP proxy `_guess_version`
  identifies through, which only became reliable once #800/#814 landed.
  `udp.py` already pointed there, so that side of the PR is prose-only
  (its port rows and docstring), not a code change, and the entry says so.
  Protochain over the 23 sample captures is *not* byte-identical: 9 frames
  in `options-transport.pcap` go `Raw` to `HTTP/2`, all 231 HTTP/1.1 frames
  are unaffected, and `_guess_version`'s entry count goes 0 to 252.

  Not marked `**a breaking change to**`: PR #821's own labels are
  `bug,fix,docs,test`, no `breaking`, unlike #759/#783 and #805/#811 last
  round, whose crediting PRs did carry it. The entry does say what a
  `breaking`-blind reader would still want to know -- TCP:80/8080 traffic
  that is neither valid HTTP/1 nor preface-carrying now reaches
  `_guess_version`'s fall-through arm instead of the direct `httpv1` bind's
  unconditional `Raw`, which is where the 12 (of 252) fall-throughs the PR
  measured come from.

`util/changelog_md.py` regenerated `CHANGELOG.md`, first pass, no
line-spanning literal this round; `--check` exit 0.
`test_changelog_md.py` 47 passed.
JarryShaw added a commit that referenced this pull request Sep 28, 2026
…s in #817 and #821

Two bullets, both non-breaking, appended after the #800 entry in merge
order. Bullet count 128 to 130 (`grep -cE '^\* \*\*'`).

- #804 (PR #817) -- the three `__repr__` methods #798 left `%`-formatted
  are f-strings now, dropping `consider-using-f-string` from both const
  modules and both vendor templates; the other bespoke templates in
  `{const,vendor}/{ftp,http}/` still carry the disable, so #804's claim
  holds for this pair only, not for those directories.

- #682 (PR #821) -- `TCP.__proto__` no longer binds `httpv1.HTTP` directly
  for ports 80/8080; both repoint to the generic HTTP proxy `_guess_version`
  identifies through, which only became reliable once #800/#814 landed.
  `udp.py` already pointed there, so that side of the PR is prose-only
  (its port rows and docstring), not a code change, and the entry says so.
  Protochain over the 23 sample captures is *not* byte-identical: 9 frames
  in `options-transport.pcap` go `Raw` to `HTTP/2`, all 231 HTTP/1.1 frames
  are unaffected, and `_guess_version`'s entry count goes 0 to 252.

  Not marked `**a breaking change to**`: PR #821's own labels are
  `bug,fix,docs,test`, no `breaking`, unlike #759/#783 and #805/#811 last
  round, whose crediting PRs did carry it. The entry does say what a
  `breaking`-blind reader would still want to know -- TCP:80/8080 traffic
  that is neither valid HTTP/1 nor preface-carrying now reaches
  `_guess_version`'s fall-through arm instead of the direct `httpv1` bind's
  unconditional `Raw`, which is where the 12 (of 252) fall-throughs the PR
  measured come from.

`util/changelog_md.py` regenerated `CHANGELOG.md`, first pass, no
line-spanning literal this round; `--check` exit 0.
`test_changelog_md.py` 47 passed.
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment) docs Pull requests that change documentation only (docs: subject prefix) fix Pull requests that fix a defect (fix: subject prefix) test Pull requests that add or correct tests (test: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

register_protocol's key space is still not unique: the three HTTP classes share one key (follow-up to #675)

1 participant