Skip to content

Upgrade goproxy to v1.9.0 - #200

Open
thavaahariharangit wants to merge 12 commits into
mainfrom
harry/upgrade-goproxy-v1.9.0
Open

Upgrade goproxy to v1.9.0#200
thavaahariharangit wants to merge 12 commits into
mainfrom
harry/upgrade-goproxy-v1.9.0

Conversation

@thavaahariharangit

@thavaahariharangit thavaahariharangit commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

What are you trying to accomplish?

Upgrade github.com/elazarl/goproxy from the pinned pseudo-version to v1.9.0 while preserving Dependabot Proxy's HTTP and HTTPS behavior.

goproxy v1.9.0 writes intercepted HTTPS responses through Go's standard http.Response.Write path. 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. A 101 Switching Protocols response 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:

go test ./internal/cache
go test .
go test ./...
go test -race ./...
script/test

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@thavaahariharangit
thavaahariharangit requested a review from a team as a code owner August 12, 2026 12:07
Copilot AI balanced review requested due to automatic review settings August 12, 2026 12:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Comment thread proxy_test.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review details

Suppressed comments (1)

proxy_test.go:68

  • This end-to-end test leaves PROXY_CACHE disabled, 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 /fixed to 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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.Write path 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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review details

  • Files reviewed: 6/29 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Comment thread internal/cache/handlers.go

Copy link
Copy Markdown
Contributor Author

goproxy v1.9.0 compatibility assessment

The 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.

Upstream change Dependabot Proxy impact Guard or evidence in this PR
HTTP/1 MITM response framing and streaming Active and behaviorally significant End-to-end HTTPS tests cover fixed-length, chunked, trailer-bearing, HEAD, 204, 304, cached, conditional ETag, and subsequent responses on one proxy connection.
Trailer forwarding Active The HTTPS MITM test verifies that an upstream response trailer reaches the client.
Bodyless responses Required a cache adaptation The cache bypasses 1xx, HEAD, 204, and 304 responses, normalizes them to http.NoBody, and clears transfer-encoding state.
101 Switching Protocols Must retain the upgraded stream The cache returns 101 unchanged and a focused test verifies that the stream is neither wrapped nor closed.
Unexpected ordinary nil body Invalid response shape at the cache boundary The cache logs the condition and returns without creating a cache entry; a valid non-nil zero-byte stream remains cacheable.
Upstream transport failure Active An upstream-close test verifies a controlled 500, distinct nil-response logging, no cached failure, and successful recovery on the next request.
HTTP/2 MITM New but inactive AllowHTTP2 remains at its default false. Enabling it is intentionally separate work because concurrent streams would require reviewing mutable ProxyCtx.UserData.
WebSocket handling refactor Potentially relevant through upgrades The cache guard preserves the 101 upgraded stream. The rest of the WebSocket implementation remains owned by goproxy and is not modified here.
Per-request dialer and ConnectionErrHandler Opt-in and unused Existing transport and safe-dialer configuration is retained and passes the complete suite.
Header canonicalization and KeepAcceptEncoding Opt-in and unused Existing defaults are retained.
Host-matching changes No direct dependency Dependabot handlers use repository-owned host-matching helpers.
CA and signer refactor Existing public integration remains active Real HTTPS CONNECT tests validate certificate generation and trust using the configured CA.
HAR and concurrency-limiter extensions Unused Neither extension is imported or enabled by Dependabot Proxy.
Removed or renamed APIs No repository usage The complete repository builds and tests against v1.9.0 without compatibility shims.
Go 1.24 requirement and dependency updates Compatible Dependabot Proxy uses Go 1.26; local, race, vendored, and container builds pass.

Validation completed:

go test ./internal/cache
go test .
go test ./...
go test -race ./...
script/test

So the compatibility position is:

  • Adapted: cache/body-framing behavior.
  • Behaviorally verified: active HTTP/1 MITM framing, trailers, connection reuse, ETag revalidation, cache behavior, and transport failure.
  • Source-compatible: existing public goproxy APIs and upstream internal refactors.
  • Intentionally inactive: HTTP/2 MITM and other opt-in extensions.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review details

Suppressed comments (1)

internal/cache/handlers.go:291

  • 205 Reset Content is 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. Include http.StatusResetContent and 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

Comment thread internal/cache/handlers.go

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review details

Suppressed comments (1)

internal/cache/handlers.go:231

  • A 205 response is classified as body-forbidden here, but its existing ContentLength and Content-Length header are preserved. Unlike 1xx/204/304, Go's HTTP writer does not suppress framing for status 205, so a malformed upstream 205 with a nonzero length is forwarded with that length after its body has been replaced by http.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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review details

Suppressed comments (2)

proxy_test.go:105

  • The “fixed length” case does not assert fixed-length framing: chunked is only checked when true, so this test still passes if the proxy rewrites /fixed as chunked. Since fixed-length behavior is one of the stated wire-level regressions covered here, assert the negative transfer-encoding case and the expected ContentLength as 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.NoBody and 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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review details

  • Files reviewed: 5/28 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

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.

3 participants