Bump goproxy to v1.9.0 and fix keep-alive framing desync - #201
Conversation
There was a problem hiding this comment.
Pull request overview
Upgrades goproxy to v1.9.0 while preventing cached bodyless responses from corrupting persistent MITM connections.
Changes:
- Updates and vendors goproxy v1.9.0.
- Preserves bodyless framing for HEAD, 1xx, 204, and 304 responses.
- Adds regression tests for cache misses and hits.
Show a summary per file
| File | Description |
|---|---|
go.mod |
Bumps goproxy dependency. |
go.sum |
Updates dependency checksums. |
internal/cache/handlers.go |
Handles bodyless cached responses. |
internal/cache/handlers_test.go |
Tests bodyless cache behavior. |
vendor/modules.txt |
Updates vendored module metadata. |
vendor/github.com/elazarl/goproxy/.golangci.yml |
Adds upstream lint configuration. |
vendor/github.com/elazarl/goproxy/README.md |
Updates upstream documentation. |
vendor/github.com/elazarl/goproxy/actions.go |
Updates handler documentation and types. |
vendor/github.com/elazarl/goproxy/certs.go |
Updates CA initialization and TLS defaults. |
vendor/github.com/elazarl/goproxy/chunked.go |
Removes legacy chunk writer. |
vendor/github.com/elazarl/goproxy/ctx.go |
Updates proxy context behavior. |
vendor/github.com/elazarl/goproxy/dispatcher.go |
Updates request conditions and helpers. |
vendor/github.com/elazarl/goproxy/doc.go |
Refreshes package documentation. |
vendor/github.com/elazarl/goproxy/h2.go |
Removes legacy HTTP/2 forwarding. |
vendor/github.com/elazarl/goproxy/http.go |
Adds regular HTTP response handling. |
vendor/github.com/elazarl/goproxy/http2.go |
Adds stream-aware HTTP/2 MITM support. |
vendor/github.com/elazarl/goproxy/https.go |
Reworks CONNECT, MITM, and keep-alive handling. |
vendor/github.com/elazarl/goproxy/internal/http1parser/header.go |
Adds HTTP/1 header parsing. |
vendor/github.com/elazarl/goproxy/internal/http1parser/request.go |
Adds request parsing without canonicalization. |
vendor/github.com/elazarl/goproxy/internal/signer/counterecryptor.go |
Moves and extends signer entropy support. |
vendor/github.com/elazarl/goproxy/internal/signer/signer.go |
Adds internal certificate signer. |
vendor/github.com/elazarl/goproxy/logger.go |
Updates logger interface documentation. |
vendor/github.com/elazarl/goproxy/proxy.go |
Refactors proxy configuration and routing. |
vendor/github.com/elazarl/goproxy/responses.go |
Modernizes response construction. |
vendor/github.com/elazarl/goproxy/signer.go |
Removes superseded signer implementation. |
vendor/github.com/elazarl/goproxy/websocket.go |
Refactors WebSocket proxying. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/26 changed files
- Comments generated: 1
- Review effort level: Balanced
e6a799b to
f144528
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
internal/cache/handlers_test.go:141
- This test exercises the cache hooks only; it never serializes either response through goproxy or reuses a tunnel, so it would still pass if framing bytes corrupted the next response. Add a regression test that sends a bodyless response followed by a normal response on the same MITM connection, for both cache-miss and cache-hit paths, and verifies the second status line parses correctly.
func TestCache_BodylessResponses(t *testing.T) {
internal/cache/handlers.go:229
- A HEAD response may legally advertise the non-zero length that a corresponding GET would return. Forcing
ContentLengthto zero makes a cache hit'shttp.Responsediffer from the cache miss and can cause that length metadata to be omitted or replaced when goproxy writes it. Restore the cachedContent-Lengthfor HEAD while still usinghttp.NoBodyand no transfer encoding.
resp.ContentLength = 0
- Files reviewed: 3/26 changed files
- Comments generated: 0 new
- Review effort level: Balanced
jeffwidman
left a comment
There was a problem hiding this comment.
Non-blocking observation:
Our cache attaches a body to every response, including ones that must have no body (e.g. 304, 204, HEAD). That produced a few stray bytes the client never reads.
It sounds like this is a bug already present on main regardless of the goproxy bump.
In that case, it might be simpler/safer to have a PR fixing just this, to ensure it works as expected pre-bump. And only then do the separate bump of the proxy (which you could probably do via
if you wanted).
I'm also fine with shipping as-is (I noted you kept the bump change separate from the behavioral change as two distinct commits, thanks!)
It's probably faster to ship both, but if we do revert for some other bug we might trip upon in the proxy, we should probably only revert the proxy bump, not this wrapper fix.
…r response framing and content length preservation
5704ef9 to
fae4da5
Compare
Actually we don't have a bug if we don't use the bumped one, because it never hits the no-response situation. No response only happens because of request interference on the same connection. If we don't bump, we can't even prove we fixed anything — so I think we should have both together so I can test properly when I deploy, and revert as well. |
jeffwidman
left a comment
There was a problem hiding this comment.
I am not super familiar with this code/networking protocols that it's touching, but all the explanation here makes sense.
I think we should try this in production, and see how it behaves. If there are further issues, we can fix forwards as we go, or upstream if needed changes back to goproxy.
| })) | ||
| defer upstream.Close() | ||
|
|
||
| t.Setenv("PROXY_CACHE", "false") |
There was a problem hiding this comment.
observation (not something to necessarily fix)
these env vars are global to the process, so when go test runs tests, it runs them in parallel for each package (serial within a package, parallel across packages) so it can introduce races.
I don't see a way around that here, and IMO this is just fine/convenient to use the env var... just mentioning it though.
There was a problem hiding this comment.
Review details
Suppressed comments (1)
internal/cache/handlers.go:218
- Only mark this request as cached after a bodyless entry is accepted or the cache file has opened successfully. As written, a missing/unreadable cache file falls through to the upstream request but remains tagged as a cache hit, so response logs and cache statistics incorrectly report the upstream response as cached.
proxyctx.SetValue(proxyCtx, wasCached, true)
d.cached++
- Files reviewed: 5/28 changed files
- Comments generated: 0 new
- Review effort level: Balanced
e5a7f83
What are you trying to accomplish?
Upgrade
github.com/elazarl/goproxyto v1.9.0.The bump on its own re-introduces a bug we saw with earlier goproxy versions: some jobs fail with errors like Cargo's
[8] Weird server reply (Invalid status line). This PR bumps goproxy and fixes that failure so the upgrade is safe to ship.Root cause (short version):
304,204,HEAD). That produced a few stray bytes the client never reads.This affects any ecosystem that gets these responses, not just Cargo.
Anything you want to highlight for special attention from reviewers?
How will you know you've accomplished your goal?
304/204/HEADacross both cache paths and assert the response stays correctly framed.go vet+go test ./...pass.Checklist