Skip to content

fix: Upstream Sync Report 2026-08-30: net/http trailer strictness and HTTP/3... - #532

Open
yhay81 wants to merge 1 commit into
imroc:masterfrom
yhay81:agent/issue-530
Open

fix: Upstream Sync Report 2026-08-30: net/http trailer strictness and HTTP/3...#532
yhay81 wants to merge 1 commit into
imroc:masterfrom
yhay81:agent/issue-530

Conversation

@yhay81

@yhay81 yhay81 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fixes #530.

Summary

Upstream Sync Report 2026-08-30: net/http trailer strictness and HTTP/3 userinfo rejection

Validation

  • Mechanical gate: +103/-5, tests passed
  • Adversarial review: approved

🤖 AI-authored PR, operated by @yhay81.

@imroc imroc left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I did a line-by-line comparison against the upstream patches and full local validation. Verification is green (details below), but I found two issues that need to be addressed before this can be merged — leaving them here for the author/maintainer.

Verification

  • go build ./..., go vet ./..., go test ./... — all pass locally (Go 1.25/1.26 CI also green).
  • TestResponseWithBareLFInTrailerClosesConnection is a good end-to-end regression: it checks both the read error and that the connection is not reused.

Issue 1: transfer.go is not semantically equivalent to upstream cff1e3d

Upstream cff1e3d validates trailers with a state machine where every CR must be immediately followed by LF (a lone CR is rejected), in addition to rejecting bare LF. This PR only rejects bare LF.

Verified on this branch:

Input (after final 0\r\n) Upstream cff1e3d This PR
X-One: a\nX-Two: b\r\n\r\n http: invalid trailer http: invalid trailer
X-One: a\rZ\r\n\r\n http: invalid trailer accepted (returns nil) ❌
X-One: a\r\r\n\r\n http: invalid trailer accepted (returns nil) ❌

For a faithful sync, seeUpcomingDoubleCRLF should port the upstream containsValidTrailers helper verbatim (the CR/LF state machine), adapted to req's reader type. Minor secondary difference: for an unterminated trailer containing a bare LF, upstream reports http: suspiciously long trailer after chunked body (buffer never ends in \r\n\r\n) while this PR reports http: invalid trailer earlier — both are errors, so this is cosmetic, but porting the upstream shape removes it too.

Issue 2: the internal/http3 change is dead code

parseHeaders has exactly one production caller: updateResponseFromHeaders (internal/http3/headers.go:236), which always passes isRequest=false (it is called from internal/http3/stream.go:365 to decode responses). req has no HTTP/3 server-side request decoding path, so the new isRequest && ... check in parseHeaders can never execute in production; the test only exercises the dead path.

Upstream quic-go 148fd03 adds the check in requestFromHeaders, the server-side request decoder, which req does not inline. On the client side req already builds :authority from req.Host/req.URL.Host (internal/http3/request_writer.go:103-110) — url.URL.Host never contains userinfo, and httpguts.ValidHostHeader rejects @ (it is not in the valid host byte set). So I don't currently see a reachable gap in req's HTTP/3 client; the http3 hunk and its test can be dropped unless there's a path I'm missing.

Given this PR touches modified stdlib (transfer.go) and internal/http3/, I'm leaving the merge decision to the maintainer per repo policy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upstream Sync Report 2026-08-30: net/http trailer strictness and HTTP/3 userinfo rejection

2 participants