Upgrade goproxy to v1.9.0 - #200
Conversation
There was a problem hiding this comment.
Pull request overview
Upgrades goproxy to v1.9.0 while preserving HTTP/HTTPS proxy behavior and correcting bodyless-response framing.
Changes:
- Refreshes goproxy dependency and vendored implementation.
- Bypasses caching for bodyless responses while preserving 101 streams.
- Adds migration documentation and HTTPS MITM framing tests.
Show a summary per file
| File | Description |
|---|---|
go.mod |
Upgrades goproxy. |
go.sum |
Updates dependency checksums. |
vendor/modules.txt |
Records vendored v1.9.0 packages. |
vendor/github.com/elazarl/goproxy/.golangci.yml |
Adds upstream lint configuration. |
vendor/github.com/elazarl/goproxy/README.md |
Refreshes upstream documentation. |
vendor/github.com/elazarl/goproxy/actions.go |
Updates handler documentation. |
vendor/github.com/elazarl/goproxy/certs.go |
Updates CA initialization. |
vendor/github.com/elazarl/goproxy/chunked.go |
Removes obsolete chunk writer. |
vendor/github.com/elazarl/goproxy/ctx.go |
Adds dialer and modernizes context handling. |
vendor/github.com/elazarl/goproxy/dispatcher.go |
Updates conditions and host matching. |
vendor/github.com/elazarl/goproxy/doc.go |
Refreshes package documentation. |
vendor/github.com/elazarl/goproxy/h2.go |
Removes the legacy HTTP/2 relay. |
vendor/github.com/elazarl/goproxy/http.go |
Adds standard HTTP response handling. |
vendor/github.com/elazarl/goproxy/http2.go |
Adds the new HTTP/2 implementation. |
vendor/github.com/elazarl/goproxy/https.go |
Overhauls CONNECT and MITM handling. |
vendor/github.com/elazarl/goproxy/internal/http1parser/header.go |
Adds raw header-name parsing. |
vendor/github.com/elazarl/goproxy/internal/http1parser/request.go |
Adds HTTP/1 request parsing. |
vendor/github.com/elazarl/goproxy/internal/signer/counterecryptor.go |
Moves and extends deterministic signing support. |
vendor/github.com/elazarl/goproxy/internal/signer/signer.go |
Adds the internal certificate signer. |
vendor/github.com/elazarl/goproxy/logger.go |
Modernizes the logger interface. |
vendor/github.com/elazarl/goproxy/proxy.go |
Refactors proxy dispatch and configuration. |
vendor/github.com/elazarl/goproxy/README.md |
Updates upstream usage guidance. |
vendor/github.com/elazarl/goproxy/responses.go |
Normalizes generated response metadata. |
vendor/github.com/elazarl/goproxy/signer.go |
Removes the superseded signer. |
vendor/github.com/elazarl/goproxy/websocket.go |
Refactors WebSocket proxying. |
internal/cache/handlers.go |
Normalizes body-forbidden responses. |
internal/cache/handlers_test.go |
Tests bodyless and 101 cache behavior. |
proxy_test.go |
Adds HTTPS MITM framing coverage. |
docs/goproxy-v1.9.0-migration.md |
Documents the compatibility audit. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/28 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (1)
proxy_test.go:68
- This end-to-end test leaves
PROXY_CACHEdisabled, so it never exercises the cache response handler whose body/framing behavior this PR changes. The original regression only occurs when that handler replaces a body before goproxy serializes the response; direct upstream HEAD/204/304 responses can pass while the cached path remains broken. Run this matrix with an isolated enabled cache, and repeat/fixedto cover both the tee-wrapped miss and cache-hit response on the wire.
client, proxy := testProxyServer(t, testProxyConfig, nil, upstream.Certificate())
- Files reviewed: 5/28 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (1)
proxy_test.go:93
- The new end-to-end matrix only covers a fixed-length body and body-forbidden responses. It does not exercise unknown-length/chunked bodies or trailers, even though those are serialized by the newly upgraded
http.Response.Writepath and the migration audit explicitly lists them as required framing cases (docs/goproxy-v1.9.0-migration.md:457-458,482). Please add wire-level cases that read a subsequent response on the reused tunnel after each framing mode so regressions cannot leave bytes on the connection.
{name: "fixed length", method: http.MethodGet, path: "/fixed", statusCode: http.StatusOK, body: "hello"},
{name: "HEAD", method: http.MethodHead, path: "/fixed", statusCode: http.StatusOK},
{name: "no content", method: http.MethodGet, path: "/no-content", statusCode: http.StatusNoContent},
{name: "not modified", method: http.MethodGet, path: "/not-modified", statusCode: http.StatusNotModified},
- Files reviewed: 6/29 changed files
- Comments generated: 0 new
- Review effort level: Balanced
goproxy v1.9.0 compatibility assessmentThe cache was the only incompatibility found in an active Dependabot Proxy code path. In addition to fixing that incompatibility, we reviewed the other upstream changes that intersect with this repository. Active paths are guarded and covered by focused tests; new opt-in features remain disabled or unused.
Validation completed: So the compatibility position is:
|
There was a problem hiding this comment.
Review details
Suppressed comments (1)
internal/cache/handlers.go:291
205 Reset Contentis also prohibited from carrying response content, but it falls through here and is wrapped by the cache. An upstream or handler-provided body can therefore be framed and forwarded for a 205 even though this normalization is intended to cover body-forbidden responses. Includehttp.StatusResetContentand add it to the table test.
func responseMustNotHaveBody(resp *http.Response) bool {
return resp.StatusCode >= 100 && resp.StatusCode < 200 ||
resp.StatusCode == http.StatusNoContent ||
resp.StatusCode == http.StatusNotModified ||
resp.Request != nil && resp.Request.Method == http.MethodHead
- Files reviewed: 5/28 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (1)
internal/cache/handlers.go:231
- A 205 response is classified as body-forbidden here, but its existing
ContentLengthandContent-Lengthheader are preserved. Unlike 1xx/204/304, Go's HTTP writer does not suppress framing for status 205, so a malformed upstream205with a nonzero length is forwarded with that length after its body has been replaced byhttp.NoBody; the client then waits for bytes that will never arrive and the next response can be misframed. Clear the content-length state when normalizing 205 responses.
resp.Body = http.NoBody
resp.TransferEncoding = nil
resp.Header.Del("Transfer-Encoding")
- Files reviewed: 5/28 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (2)
proxy_test.go:105
- The “fixed length” case does not assert fixed-length framing:
chunkedis only checked when true, so this test still passes if the proxy rewrites/fixedas chunked. Since fixed-length behavior is one of the stated wire-level regressions covered here, assert the negative transfer-encoding case and the expectedContentLengthas well.
{name: "fixed length", method: http.MethodGet, path: "/fixed", statusCode: http.StatusOK, body: "hello"},
internal/cache/handlers.go:225
- Valid HEAD, 204, and 304 responses normally arrive with
http.NoBodyand no transfer encoding, so this emits a warning for every routine bodyless response whenever caching is enabled. That can flood production logs and makes normal protocol behavior look like a fault. Warn only when this branch actually has to discard a non-sentinel body or invalid transfer framing.
logrus.Warnf("Response has no body (method: %s, status: %d)", method, resp.StatusCode)
- Files reviewed: 5/28 changed files
- Comments generated: 0 new
- Review effort level: Balanced
What are you trying to accomplish?
Upgrade
github.com/elazarl/goproxyfrom the pinned pseudo-version tov1.9.0while preserving Dependabot Proxy's HTTP and HTTPS behavior.goproxy v1.9.0 writes intercepted HTTPS responses through Go's standard
http.Response.Writepath. This enforces HTTP framing semantics more strictly than the previous manual response writer. The response cache wrapped every response body, including responses that must not carry a body, which could cause invalid transfer framing for HEAD, informational, 204, and 304 responses.This PR updates and vendors goproxy v1.9.0, adapts the cache to the new response contract, and adds wire-level regression coverage for the upgraded MITM path.
Anything you want to highlight for special attention from reviewers?
The cache now bypasses and normalizes responses that cannot contain a body. It closes any non-sentinel body, uses
http.NoBody, and clears transfer-encoding state. A101 Switching Protocolsresponse is returned untouched because its body is the upgraded connection stream.An ordinary response with an unexpected nil body is logged and left uncached. This is distinct from both a transport failure, where goproxy supplies no response, and a valid non-nil zero-byte response body.
HTTP/2 MITM remains disabled. Enabling it would require separate work because request streams may share mutable data stored in
ProxyCtx.UserData.How will you know you've accomplished your goal?
Unit coverage verifies HEAD, informational, 204, 304, 101, unexpected nil-body, and valid zero-byte response behavior at the cache boundary.
End-to-end tests exercise fixed-length, chunked, trailer-bearing, bodyless, conditional ETag, cached, and subsequent HTTPS responses through a real CONNECT tunnel. A transport-failure test verifies that an upstream close produces a controlled 500, is logged distinctly from a valid bodyless response, and is not cached.
The following validation passes:
Checklist