Conversation
Replace the manual http.ReadRequest loop in proxyConnect with a per-connection http.Server which has HTTP/2 capabilities. Enable ForceAttemptHTTP2 on the outbound transport. Updates ZenPrivacy/zen-desktop#604
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a single-connection net.Listener, restructures CONNECT/TLS handling to perform an explicit TLS handshake and run a per-connection http.Server, and refactors WebSocket proxying to a hijack-and-tunnel flow. All changes are internal to the proxy package. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
proxy/proxy.go (1)
62-66: Replace deprecatedDialwithDialContext.
http.Transport.Dialis deprecated. UseDialContextinstead, which allows the transport to cancel in-flight dials when requests are cancelled. If both are set,DialContexttakes priority.♻️ Proposed fix
p.requestTransport = &http.Transport{ - Dial: p.netDialer.Dial, + DialContext: p.netDialer.DialContext, ForceAttemptHTTP2: true, MaxIdleConns: 100, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 62 - 66, Replace the deprecated http.Transport.Dial usage in the p.requestTransport initialization with DialContext so the transport can cancel in-flight dials; update the transport on p.requestTransport to set DialContext: use p.netDialer.DialContext as the value (matching the signature func(ctx context.Context, network, addr string) (net.Conn, error)) and remove the old Dial field so DialContext is used by http.Transport. Ensure you reference p.requestTransport and p.netDialer.DialContext in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proxy/proxy.go`:
- Around line 292-294: The guard that sets req.Body = nil only checks for
req.ContentLength == 0 but misses the chunked/unknown case (req.ContentLength ==
-1) where the server may provide a non-nil reader that is actually empty; update
the logic in proxy.go around the req.ContentLength check to also handle
ContentLength == -1 by peeking one byte from req.Body (wrap the existing
req.Body in a bufio.Reader), and if Peek(1) returns EOF treat the body as empty
and set req.Body = nil; if Peek succeeds, replace req.Body with an io.NopCloser
wrapping the bufio.Reader so the original body data is preserved for the rest of
the pipeline.
- Around line 248-256: The per-connection http.Server instance srv created in
the connectHandler path is missing a ReadHeaderTimeout, exposing a Slowloris
risk; update srv (the one created alongside p.connectHandler(connReq, host, ln))
to set ReadHeaderTimeout (e.g., reuse p.server.ReadHeaderTimeout or set to
10*time.Second) so each tunnel enforces a header read deadline and prevents
clients from holding connections indefinitely.
- Around line 236-244: The TLS handshake currently uses context.Background() in
the tlsConn.HandshakeContext call, allowing a stalled client to hang the
goroutine forever; change this to a time-bounded context (e.g., use
context.WithTimeout(context.Background(), tlsHandshakeTimeout)) before calling
tlsConn.HandshakeContext, store the cancel func and defer cancel(), and handle a
deadline/timeout error path (log a timeout and close/return). Update or add a
tlsHandshakeTimeout constant and ensure the existing logic around
tlsConn.HandshakeContext, redacted.Redacted(host), p.addTransparentHost(host)
and logging remains but that the handshake is always bounded and cleaned up via
defer cancel().
---
Nitpick comments:
In `@proxy/proxy.go`:
- Around line 62-66: Replace the deprecated http.Transport.Dial usage in the
p.requestTransport initialization with DialContext so the transport can cancel
in-flight dials; update the transport on p.requestTransport to set DialContext:
use p.netDialer.DialContext as the value (matching the signature func(ctx
context.Context, network, addr string) (net.Conn, error)) and remove the old
Dial field so DialContext is used by http.Transport. Ensure you reference
p.requestTransport and p.netDialer.DialContext in the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
proxy/listener.goproxy/proxy.goproxy/websocket.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Go (windows-latest)
🧰 Additional context used
🧬 Code graph analysis (1)
proxy/websocket.go (3)
proxy/proxy.go (1)
Proxy(31-41)internal/redacted/redacted_nonprod.go (1)
Redacted(10-12)internal/redacted/redacted_prod.go (1)
Redacted(8-10)
🪛 GitHub Check: Lint Go (ubuntu-latest)
proxy/proxy.go
[failure] 248-248:
G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec)
🔇 Additional comments (2)
proxy/listener.go (1)
1-46: LGTM — clean single-use listener implementation.The
sync.Onceensures the connection is handed out exactly once, the channel provides a race-free, idempotent shutdown signal via theselectguard inClose, andnet.ErrClosed(a non-Timeouterror) causeshttp.Server.Serveto exit immediately without retrying — all exactly as intended.proxy/websocket.go (1)
15-48: LGTM — hijack-and-tunnel refactor is clean.
deferon bothclientConnandtargetConnhandles all error paths correctly, and thehttp.Hijackertype-assertion guard replaces the previouspanicwith a graceful 500. Thetls.Dialerwithout explicitServerNameis fine — Go extracts the host from the dialed address for SNI automatically.
| if err := tlsConn.HandshakeContext(context.Background()); err != nil { | ||
| msg := err.Error() | ||
| if strings.Contains(msg, "tls: ") { | ||
| log.Printf("adding %s to ignored hosts", redacted.Redacted(host)) | ||
| p.addTransparentHost(host) | ||
| } | ||
| log.Printf("TLS handshake(%s): %v", redacted.Redacted(connReq.Host), err) | ||
| return | ||
| } |
There was a problem hiding this comment.
TLS handshake has no timeout — potential indefinite goroutine leak.
context.Background() carries no deadline. A client that sends the CONNECT request, receives 200 OK, then stalls mid-TLS-handshake will hold the goroutine and the TCP connection indefinitely. Each such connection is a goroutine pinned inside proxyConnect. Use a time-bounded context to cap the handshake duration.
🛡️ Proposed fix
+ hsCtx, hsCancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer hsCancel()
- if err := tlsConn.HandshakeContext(context.Background()); err != nil {
+ if err := tlsConn.HandshakeContext(hsCtx); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := tlsConn.HandshakeContext(context.Background()); err != nil { | |
| msg := err.Error() | |
| if strings.Contains(msg, "tls: ") { | |
| log.Printf("adding %s to ignored hosts", redacted.Redacted(host)) | |
| p.addTransparentHost(host) | |
| } | |
| log.Printf("TLS handshake(%s): %v", redacted.Redacted(connReq.Host), err) | |
| return | |
| } | |
| hsCtx, hsCancel := context.WithTimeout(context.Background(), 10*time.Second) | |
| defer hsCancel() | |
| if err := tlsConn.HandshakeContext(hsCtx); err != nil { | |
| msg := err.Error() | |
| if strings.Contains(msg, "tls: ") { | |
| log.Printf("adding %s to ignored hosts", redacted.Redacted(host)) | |
| p.addTransparentHost(host) | |
| } | |
| log.Printf("TLS handshake(%s): %v", redacted.Redacted(connReq.Host), err) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@proxy/proxy.go` around lines 236 - 244, The TLS handshake currently uses
context.Background() in the tlsConn.HandshakeContext call, allowing a stalled
client to hang the goroutine forever; change this to a time-bounded context
(e.g., use context.WithTimeout(context.Background(), tlsHandshakeTimeout))
before calling tlsConn.HandshakeContext, store the cancel func and defer
cancel(), and handle a deadline/timeout error path (log a timeout and
close/return). Update or add a tlsHandshakeTimeout constant and ensure the
existing logic around tlsConn.HandshakeContext, redacted.Redacted(host),
p.addTransparentHost(host) and logging remains but that the handshake is always
bounded and cleaned up via defer cancel().
…handshake timeout
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
proxy/proxy.go (3)
249-257: Per-connectionsrvstill missingReadHeaderTimeout— Slowloris exposure (gosec G112).This was already flagged by the linter (
G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server) and in a previous review round. The fix is to propagateReadHeaderTimeoutfrom the outer server or set it explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 249 - 257, Per-connection http.Server instance named srv is missing ReadHeaderTimeout which allows Slowloris attacks; update the code that constructs/assigns srv to copy the ReadHeaderTimeout from the outer http.Server (or set a reasonable explicit timeout) by assigning srv.ReadHeaderTimeout = outerServer.ReadHeaderTimeout (or a chosen duration) where the per-connection srv is created/returned so the per-connection server inherits the header read timeout.
237-245: TLS handshake still usescontext.Background()— unbounded goroutine leak risk.A stalling client that sends the CONNECT request then goes silent mid-TLS-handshake will hold the goroutine indefinitely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 237 - 245, The TLS handshake is being started with context.Background(), which allows a stuck client to block a goroutine indefinitely; update the code that calls the TLS handshake (look for the function/method that wraps the CONNECT handling and the call to tlsConn.Handshake or tlsConn.HandshakeContext) to use the request/handler context (e.g., use the existing ctx or create ctx, cancel := context.WithTimeout(ctx, timeout) and defer cancel) instead of context.Background(), and if you wrap the raw net.Conn in tls.Client, call tlsConn.HandshakeContext(ctx) (or set conn read/write deadlines as a fallback) so the handshake is cancellable and goroutines are not leaked.
293-295:ContentLength == -1(chunked/unknown body) case still unguarded.Already raised in a previous review round — a chunked request with an empty body arrives with
ContentLength == -1and a non-nilreq.Body, bypassing this guard and causing strict upstream servers to reject the request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 293 - 295, When handling proxied requests in proxy.go, the chunked/unknown-body case (req.ContentLength == -1) is still unguarded: if req.Body is non-nil but actually empty, upstream servers reject it. Fix by adding an explicit probe when req.ContentLength == -1: peek/read a single byte from req.Body (using a buffered wrapper) to detect EOF; if EOF, replace req.Body with nil (and set req.ContentLength = 0 and remove "chunked" from req.TransferEncoding), otherwise restore the body (wrap the read byte back) and proceed normally. Apply this logic in the request-path function that forwards requests (the handler that references req.ContentLength and req.Body / req.TransferEncoding).
🧹 Nitpick comments (1)
proxy/proxy.go (1)
62-67: Consider settingIdleConnTimeoutandMaxIdleConnsPerHost.Two transport knobs left at zero/default:
IdleConnTimeoutis unset, so idle connections are never evicted from the pool — Go'sDefaultTransportsets it to 90 s. Stale connections can accumulate over time.DefaultMaxIdleConnsPerHostis 2, so even withMaxIdleConns: 100, only 2 idle HTTP/1.1 connections are reused per upstream host. For a MITM proxy where multiple clients hit the same upstream, this can cause unnecessary TCP/TLS churn. HTTP/2 multiplexing reduces the impact on H2 upstreams, but it's still worth being explicit.♻️ Proposed addition
p.requestTransport = &http.Transport{ DialContext: p.netDialer.DialContext, ForceAttemptHTTP2: true, TLSHandshakeTimeout: 20 * time.Second, + IdleConnTimeout: 90 * time.Second, MaxIdleConns: 100, + MaxIdleConnsPerHost: 10, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 62 - 67, Update the http.Transport initialization (the variable/struct named transport or where NewProxy builds the transport) to explicitly set IdleConnTimeout (e.g., 90*time.Second or make it configurable) and MaxIdleConnsPerHost (e.g., 100 or match MaxIdleConns) in addition to any existing MaxIdleConns; modify the http.Transport literal/constructor that currently omits IdleConnTimeout/MaxIdleConnsPerHost so it includes IdleConnTimeout: 90*time.Second and MaxIdleConnsPerHost: 100 (or a sensible configurable constant) to avoid never-evicted idle sockets and to allow more idle connection reuse per upstream host.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proxy/proxy.go`:
- Around line 362-374: In writeResp, surface and handle the io.Copy error
instead of discarding it: check the return error from io.Copy when copying
resp.Body to the client and log/return it so connection-level failures are
visible. Also forward upstream trailers: before calling WriteHeader, declare all
trailer names by adding them to the "Trailer" response header (e.g. for each key
in resp.Trailer add the key to w.Header()["Trailer"]), then after io.Copy
completes iterate resp.Trailer and write each trailer value into the response
writer’s headers so they are sent to the client (use the same trailer names you
declared); reference writeResp, io.Copy, resp.Trailer, WriteHeader, and
w.Header()/\"Trailer\" for where to apply these changes.
---
Duplicate comments:
In `@proxy/proxy.go`:
- Around line 249-257: Per-connection http.Server instance named srv is missing
ReadHeaderTimeout which allows Slowloris attacks; update the code that
constructs/assigns srv to copy the ReadHeaderTimeout from the outer http.Server
(or set a reasonable explicit timeout) by assigning srv.ReadHeaderTimeout =
outerServer.ReadHeaderTimeout (or a chosen duration) where the per-connection
srv is created/returned so the per-connection server inherits the header read
timeout.
- Around line 237-245: The TLS handshake is being started with
context.Background(), which allows a stuck client to block a goroutine
indefinitely; update the code that calls the TLS handshake (look for the
function/method that wraps the CONNECT handling and the call to
tlsConn.Handshake or tlsConn.HandshakeContext) to use the request/handler
context (e.g., use the existing ctx or create ctx, cancel :=
context.WithTimeout(ctx, timeout) and defer cancel) instead of
context.Background(), and if you wrap the raw net.Conn in tls.Client, call
tlsConn.HandshakeContext(ctx) (or set conn read/write deadlines as a fallback)
so the handshake is cancellable and goroutines are not leaked.
- Around line 293-295: When handling proxied requests in proxy.go, the
chunked/unknown-body case (req.ContentLength == -1) is still unguarded: if
req.Body is non-nil but actually empty, upstream servers reject it. Fix by
adding an explicit probe when req.ContentLength == -1: peek/read a single byte
from req.Body (using a buffered wrapper) to detect EOF; if EOF, replace req.Body
with nil (and set req.ContentLength = 0 and remove "chunked" from
req.TransferEncoding), otherwise restore the body (wrap the read byte back) and
proceed normally. Apply this logic in the request-path function that forwards
requests (the handler that references req.ContentLength and req.Body /
req.TransferEncoding).
---
Nitpick comments:
In `@proxy/proxy.go`:
- Around line 62-67: Update the http.Transport initialization (the
variable/struct named transport or where NewProxy builds the transport) to
explicitly set IdleConnTimeout (e.g., 90*time.Second or make it configurable)
and MaxIdleConnsPerHost (e.g., 100 or match MaxIdleConns) in addition to any
existing MaxIdleConns; modify the http.Transport literal/constructor that
currently omits IdleConnTimeout/MaxIdleConnsPerHost so it includes
IdleConnTimeout: 90*time.Second and MaxIdleConnsPerHost: 100 (or a sensible
configurable constant) to avoid never-evicted idle sockets and to allow more
idle connection reuse per upstream host.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy/proxy.go
📜 Review details
🔇 Additional comments (1)
proxy/proxy.go (1)
271-274:singleConnListener.Close()is safe to call multiple times.After
proxyWebsocketTLShijacks the connection, the HTTP server transitions it toStateHijacked(notStateClosed), so theConnStatecallback will not callln.Close()for this connection. The explicitln.Close()on line 273 is therefore necessary.The implementation is idempotent: it uses a select statement that receives from the channel on the first call (executing the default branch to close it) and on subsequent calls the select receives from the already-closed channel (executing the first case with the "Already closed" comment). Both paths return nil safely, making multiple calls harmless.
| // writeResp writes the response (status code, headers, and body) to the ResponseWriter. | ||
| // It is the caller's responsibility to close the response body after calling the function. | ||
| func writeResp(w http.ResponseWriter, resp *http.Response) { | ||
| for h, v := range resp.Header { | ||
| for _, vv := range v { | ||
| w.Header().Add(h, vv) | ||
| } | ||
| } | ||
| w.WriteHeader(resp.StatusCode) | ||
| if resp.Body != nil { | ||
| io.Copy(w, resp.Body) | ||
| } | ||
| } |
There was a problem hiding this comment.
io.Copy error silently dropped; upstream trailers not forwarded.
Two issues in writeResp:
-
Dropped write/read error (Line 372):
io.Copycan fail if the upstream body read errors or the client write errors mid-stream. The error is silently discarded, making connection-level failures invisible in logs. -
Trailers not forwarded:
resp.Traileris never written. For protocols that use HTTP trailers (e.g., gRPC-Web over HTTP/2), upstream trailers will be silently dropped. Go'shttp.ResponseWritersupports thehttp.TrailerPrefixmechanism for trailers not known beforeWriteHeader.
♻️ Proposed fix for (1)
if resp.Body != nil {
- io.Copy(w, resp.Body)
+ if _, err := io.Copy(w, resp.Body); err != nil {
+ log.Printf("writeResp: copying body: %v", err)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // writeResp writes the response (status code, headers, and body) to the ResponseWriter. | |
| // It is the caller's responsibility to close the response body after calling the function. | |
| func writeResp(w http.ResponseWriter, resp *http.Response) { | |
| for h, v := range resp.Header { | |
| for _, vv := range v { | |
| w.Header().Add(h, vv) | |
| } | |
| } | |
| w.WriteHeader(resp.StatusCode) | |
| if resp.Body != nil { | |
| io.Copy(w, resp.Body) | |
| } | |
| } | |
| if resp.Body != nil { | |
| if _, err := io.Copy(w, resp.Body); err != nil { | |
| log.Printf("writeResp: copying body: %v", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@proxy/proxy.go` around lines 362 - 374, In writeResp, surface and handle the
io.Copy error instead of discarding it: check the return error from io.Copy when
copying resp.Body to the client and log/return it so connection-level failures
are visible. Also forward upstream trailers: before calling WriteHeader, declare
all trailer names by adding them to the "Trailer" response header (e.g. for each
key in resp.Trailer add the key to w.Header()["Trailer"]), then after io.Copy
completes iterate resp.Trailer and write each trailer value into the response
writer’s headers so they are sent to the client (use the same trailer names you
declared); reference writeResp, io.Copy, resp.Trailer, WriteHeader, and
w.Header()/\"Trailer\" for where to apply these changes.
Prevents stale connections from accumulating over time.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proxy/proxy.go (1)
269-300:⚠️ Potential issue | 🔴 CriticalClear
RequestURIbefore outboundRoundTrip.The
connectHandlerreceives a request fromhttp.Server, which automatically populatesRequestURIas a server-side field. Before callingRoundTripat line 299, this field must be cleared, as Go's HTTP client transport expects client-request form withRequestURIempty. Without this, the transport may reject the request. This pattern is already applied in theproxyHTTPhandler in the same file.Fix
req.URL.Host = connReq.Host req.URL.Scheme = "https" + req.RequestURI = ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 269 - 300, The RequestURI field from the server-side request must be cleared before making the outbound client request; in the connectHandler flow clear req.RequestURI (set to empty string) just before calling p.requestTransport.RoundTrip(req) to ensure the HTTP client transport sees a client-form request (same pattern used in proxyHTTP); make this change near the call to p.requestTransport.RoundTrip so the transport does not reject the request.
♻️ Duplicate comments (3)
proxy/proxy.go (3)
236-245:⚠️ Potential issue | 🟠 MajorBound the TLS handshake with a timeout.
Line 238still usescontext.Background(), so a stalled client can hold this goroutine/connection indefinitely during handshake.🛡️ Proposed fix
- if err := tlsConn.HandshakeContext(context.Background()); err != nil { + hsCtx, hsCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer hsCancel() + if err := tlsConn.HandshakeContext(hsCtx); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 236 - 245, The TLS handshake currently calls tlsConn.HandshakeContext(context.Background()) which can block indefinitely; replace the background context with a short timeout context (e.g., using context.WithTimeout from the incoming request/context or a per-connection timeout constant), pass that timeout context into tlsConn.HandshakeContext, and ensure you call the returned cancel() to avoid leaks; keep existing error handling (logging and p.addTransparentHost(host)) unchanged so TLS errors still trigger the same logic.
366-375:⚠️ Potential issue | 🟡 Minor
writeRespdrops stream errors and trailers.At
Line 374,io.Copyerrors are discarded, and upstream trailers are never forwarded. This can hide broken streams and lose HTTP/2 trailer metadata.♻️ Proposed fix
-func writeResp(w http.ResponseWriter, resp *http.Response) { +func writeResp(w http.ResponseWriter, resp *http.Response) { + for trailerKey := range resp.Trailer { + w.Header().Add("Trailer", trailerKey) + } for h, v := range resp.Header { for _, vv := range v { w.Header().Add(h, vv) } } w.WriteHeader(resp.StatusCode) if resp.Body != nil { - io.Copy(w, resp.Body) + if _, err := io.Copy(w, resp.Body); err != nil { + log.Printf("writeResp: copying body: %v", err) + return + } + } + for h, v := range resp.Trailer { + for _, vv := range v { + w.Header().Add(h, vv) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 366 - 375, The writeResp function currently discards io.Copy errors and never forwards upstream trailers; change writeResp to return error (func writeResp(w http.ResponseWriter, resp *http.Response) error), propagate and return the error from io.Copy instead of ignoring it, and after copying the body (and before/after writing the status as appropriate) iterate resp.Trailer and add each trailer header to the response writer (using w.Header().Add) so HTTP/2 trailers are forwarded; update any callers to handle the returned error and ensure resp.Body is closed if not already handled.
293-297:⚠️ Potential issue | 🟡 MinorHandle empty
ContentLength == -1body edge case too.
Line 295only handlesContentLength == 0. Empty chunked/unknown-length bodies can still be forwarded as chunked, which some strict upstreams reject.♻️ Proposed fix
+import "bufio" ... - if req.ContentLength == 0 { - req.Body = nil - } + if req.Body != nil { + if req.ContentLength == 0 { + req.Body = nil + } else if req.ContentLength == -1 { + br := bufio.NewReader(req.Body) + if _, err := br.Peek(1); errors.Is(err, io.EOF) { + req.Body = nil + } else { + req.Body = io.NopCloser(br) + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 293 - 297, In the RoundTrip/request-preparation code where you currently set req.Body = nil only for ContentLength == 0, also treat unknown-length bodies (ContentLength == -1) as empty to avoid forwarding as chunked; change the condition on req.ContentLength to check for req.ContentLength == 0 || req.ContentLength == -1 and set req.Body = nil accordingly (reference the req variable and the surrounding RoundTrip/transport request preparation code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@proxy/proxy.go`:
- Around line 269-300: The RequestURI field from the server-side request must be
cleared before making the outbound client request; in the connectHandler flow
clear req.RequestURI (set to empty string) just before calling
p.requestTransport.RoundTrip(req) to ensure the HTTP client transport sees a
client-form request (same pattern used in proxyHTTP); make this change near the
call to p.requestTransport.RoundTrip so the transport does not reject the
request.
---
Duplicate comments:
In `@proxy/proxy.go`:
- Around line 236-245: The TLS handshake currently calls
tlsConn.HandshakeContext(context.Background()) which can block indefinitely;
replace the background context with a short timeout context (e.g., using
context.WithTimeout from the incoming request/context or a per-connection
timeout constant), pass that timeout context into tlsConn.HandshakeContext, and
ensure you call the returned cancel() to avoid leaks; keep existing error
handling (logging and p.addTransparentHost(host)) unchanged so TLS errors still
trigger the same logic.
- Around line 366-375: The writeResp function currently discards io.Copy errors
and never forwards upstream trailers; change writeResp to return error (func
writeResp(w http.ResponseWriter, resp *http.Response) error), propagate and
return the error from io.Copy instead of ignoring it, and after copying the body
(and before/after writing the status as appropriate) iterate resp.Trailer and
add each trailer header to the response writer (using w.Header().Add) so HTTP/2
trailers are forwarded; update any callers to handle the returned error and
ensure resp.Body is closed if not already handled.
- Around line 293-297: In the RoundTrip/request-preparation code where you
currently set req.Body = nil only for ContentLength == 0, also treat
unknown-length bodies (ContentLength == -1) as empty to avoid forwarding as
chunked; change the condition on req.ContentLength to check for
req.ContentLength == 0 || req.ContentLength == -1 and set req.Body = nil
accordingly (reference the req variable and the surrounding RoundTrip/transport
request preparation code).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy/proxy.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint Go (windows-latest)
- GitHub Check: Lint Go (macos-latest)
🧰 Additional context used
🧬 Code graph analysis (1)
proxy/proxy.go (2)
internal/redacted/redacted_nonprod.go (1)
Redacted(10-12)internal/redacted/redacted_prod.go (1)
Redacted(8-10)
Per field documentation: "It is an error to set this field in an HTTP client request."
There was a problem hiding this comment.
♻️ Duplicate comments (1)
proxy/proxy.go (1)
373-375:⚠️ Potential issue | 🟡 Minor
io.Copyerror still silently dropped.The trailer forwarding has been addressed (lines 376-380), but the
io.Copyerror is still discarded. This makes mid-stream read/write failures invisible in logs, complicating debugging.🔧 Proposed fix
if resp.Body != nil { - io.Copy(w, resp.Body) + if _, err := io.Copy(w, resp.Body); err != nil { + log.Printf("writeResp: copying body: %v", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 373 - 375, The io.Copy call that streams resp.Body to w currently drops its error; change it to capture the returned error (e.g., n, err := io.Copy(w, resp.Body) or just _, err := io.Copy(...)) and, if err != nil, log it with contextual info (response status, request URL/host, and number of bytes copied if available) using the existing logger in this file so mid-stream read/write failures are visible; ensure any necessary cleanup (closing resp.Body) remains intact.
🧹 Nitpick comments (3)
proxy/proxy.go (3)
240-243: Fragile error detection via string matching.Using
strings.Contains(err.Error(), "tls: ")to identify TLS errors is brittle—error message text isn't part of Go's stability guarantees. Unfortunately,crypto/tlsdoesn't export all error types forerrors.Aschecks.This is a known limitation and the fallback behavior (not adding to transparent list) is safe. Consider adding a comment noting this fragility for future maintainers.
Also applies to: 301-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 240 - 243, The code currently detects TLS-related errors by checking strings.Contains(msg, "tls: "), which is fragile because error text isn't stable; instead of changing behavior, add a clear comment next to the check (the occurrence using strings.Contains(msg, "tls: ") that logs and calls p.addTransparentHost(host) and the second identical occurrence later) explaining that this string-match is a pragmatic fallback due to unexported crypto/tls error types, that errors.As isn't possible here, and that the fallback of not adding the host is safe for future maintainers; reference the surrounding symbols msg, host, log.Printf, and p.addTransparentHost in the comment so it's easy to find.
185-185: Sameio.Copyerror dropped here as inwriteResp.For consistency with a potential fix in
writeResp, consider logging errors here as well. Alternatively, refactorproxyHTTPto usewriteRespto consolidate response writing logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` at line 185, The call io.Copy(w, resp.Body) inside proxyHTTP drops any error; either capture and log the returned error similarly to the fix in writeResp (e.g., err := io.Copy(...) then processLogger.Errorf or return it) or refactor proxyHTTP to call the existing writeResp function to consolidate response-writing behavior; locate the io.Copy usage in proxyHTTP and update it to surface and log errors or delegate to writeResp so response copying follows the same error-handling path.
250-263: Minor:TLSConfigon per-connection server appears unused.Since the TLS handshake is performed manually before
srv.Serve(ln), andServe()(notServeTLS()) is called on the already-TLS-wrapped connection, theTLSConfigfield on line 252 has no effect. Consider removing it to avoid confusion.The
ReadHeaderTimeoutis now properly set, which addresses the Slowloris concern.♻️ Proposed cleanup
srv := &http.Server{ Handler: p.connectHandler(connReq, host, ln), - TLSConfig: tlsConfig, ConnState: func(_ net.Conn, state http.ConnState) { if state == http.StateClosed { ln.Close() } }, ReadHeaderTimeout: 20 * time.Second, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/proxy.go` around lines 250 - 263, The TLSConfig field on the per-connection http.Server (srv) is unused because you perform the TLS handshake earlier and call srv.Serve(ln) on an already-TLS-wrapped listener; remove the TLSConfig assignment from the srv struct literal (leave ReadHeaderTimeout and ConnState) to avoid confusion, and keep the existing manual handshake code that wraps ln before calling srv.Serve(ln) in the connectHandler/Serve path (references: srv, TLSConfig, connectHandler, Serve, ln, connReq.Host).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@proxy/proxy.go`:
- Around line 373-375: The io.Copy call that streams resp.Body to w currently
drops its error; change it to capture the returned error (e.g., n, err :=
io.Copy(w, resp.Body) or just _, err := io.Copy(...)) and, if err != nil, log it
with contextual info (response status, request URL/host, and number of bytes
copied if available) using the existing logger in this file so mid-stream
read/write failures are visible; ensure any necessary cleanup (closing
resp.Body) remains intact.
---
Nitpick comments:
In `@proxy/proxy.go`:
- Around line 240-243: The code currently detects TLS-related errors by checking
strings.Contains(msg, "tls: "), which is fragile because error text isn't
stable; instead of changing behavior, add a clear comment next to the check (the
occurrence using strings.Contains(msg, "tls: ") that logs and calls
p.addTransparentHost(host) and the second identical occurrence later) explaining
that this string-match is a pragmatic fallback due to unexported crypto/tls
error types, that errors.As isn't possible here, and that the fallback of not
adding the host is safe for future maintainers; reference the surrounding
symbols msg, host, log.Printf, and p.addTransparentHost in the comment so it's
easy to find.
- Line 185: The call io.Copy(w, resp.Body) inside proxyHTTP drops any error;
either capture and log the returned error similarly to the fix in writeResp
(e.g., err := io.Copy(...) then processLogger.Errorf or return it) or refactor
proxyHTTP to call the existing writeResp function to consolidate
response-writing behavior; locate the io.Copy usage in proxyHTTP and update it
to surface and log errors or delegate to writeResp so response copying follows
the same error-handling path.
- Around line 250-263: The TLSConfig field on the per-connection http.Server
(srv) is unused because you perform the TLS handshake earlier and call
srv.Serve(ln) on an already-TLS-wrapped listener; remove the TLSConfig
assignment from the srv struct literal (leave ReadHeaderTimeout and ConnState)
to avoid confusion, and keep the existing manual handshake code that wraps ln
before calling srv.Serve(ln) in the connectHandler/Serve path (references: srv,
TLSConfig, connectHandler, Serve, ln, connReq.Host).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy/proxy.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Go (windows-latest)
🧰 Additional context used
🧬 Code graph analysis (1)
proxy/proxy.go (2)
internal/redacted/redacted_prod.go (1)
Redacted(8-10)internal/redacted/redacted_nonprod.go (1)
Redacted(10-12)
🔇 Additional comments (4)
proxy/proxy.go (4)
62-68: LGTM!Good transport configuration for HTTP/2 support:
DialContextis the modern replacement for the deprecatedDialForceAttemptHTTP2: trueenables HTTP/2 on upstream connections- Connection pool settings (
MaxIdleConns,IdleConnTimeout) are reasonable defaults
227-231: LGTM!Correct ALPN configuration with
"h2"listed first to prefer HTTP/2 when supported by the client.
236-246: TLS handshake still lacks timeout — goroutine leak risk remains.The handshake uses
context.Background()with no deadline. A client that stalls mid-TLS-handshake will hold the goroutine and TCP connection indefinitely.🛡️ Proposed fix
+ hsCtx, hsCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer hsCancel() - if err := tlsConn.HandshakeContext(context.Background()); err != nil { + if err := tlsConn.HandshakeContext(hsCtx); err != nil {
272-277: Correct HTTP/1.1 check for WebSocket upgrades.WebSocket upgrades via the
Upgradeheader mechanism are HTTP/1.1-specific. HTTP/2 uses a different mechanism (RFC 8441 CONNECT protocol). Thereq.ProtoMajor == 1guard correctly prevents the HTTP/1.1 WebSocket path from running on HTTP/2 connections.
1 participant
What does this PR do?
Adds HTTP/2 support to MITM proxy by replacing the manual http.ReadRequest loop in proxyConnect with a per-connection http.Server which has HTTP/2 capabilities. Enables ForceAttemptHTTP2 on the outbound transport.
How did you verify your code works?
What are the relevant issues?
Updates ZenPrivacy/zen-desktop#604