feat(proxy): support Claude Code subscription traffic#172
Draft
SantiagoDePolonia wants to merge 3 commits intomainfrom
Draft
feat(proxy): support Claude Code subscription traffic#172SantiagoDePolonia wants to merge 3 commits intomainfrom
SantiagoDePolonia wants to merge 3 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an experimental HTTP forward-proxy mode with optional TLS MITM for configured hosts (default Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Proxy as Forward Proxy
participant CA as CA Loader
participant Upstream as Upstream Server
participant Audit as Audit Logger
participant Usage as Usage Logger
Client->>Proxy: CONNECT api.anthropic.com:443
Proxy->>CA: Load CA cert/key
CA-->>Proxy: CA loaded
Proxy->>Client: 200 Connection Established
Proxy->>Proxy: Generate forged cert for host
Proxy->>Client: TLS Handshake (forged cert)
Client->>Proxy: Encrypted HTTPS request (/v1/messages)
Proxy->>Proxy: Decrypt, parse request, capture body/headers
Proxy->>Audit: Emit request audit entry (redacted headers)
Proxy->>Upstream: Forward HTTPS request (direct transport)
Upstream-->>Proxy: SSE stream response (message_start, deltas, done)
Proxy->>Proxy: Parse Anthropic events, assemble content
Proxy->>Audit: Emit response audit entry (assembled body)
Proxy->>Usage: Extract and emit usage entry
Proxy->>Client: Encrypted SSE response (proxied)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/auditlog/stream_observer.go (1)
10-17:⚠️ Potential issue | 🔴 CriticalRace condition: Shared state accessed without synchronization.
Similar to the usage observer,
StreamLogObserverhas a race condition indicated by pipeline failures. Theentry,builder, andclosedfields are modified inOnStreamClose()while potentially being accessed concurrently.🔒 Proposed fix to add mutex protection
type StreamLogObserver struct { logger LoggerInterface entry *LogEntry builder *streamResponseBuilder logBodies bool closed bool startTime time.Time + mu sync.Mutex }Protect
OnJSONEventandOnStreamClose:func (o *StreamLogObserver) OnJSONEvent(event map[string]any) { + o.mu.Lock() + defer o.mu.Unlock() if !o.logBodies || o.builder == nil { return } // ... rest of method } func (o *StreamLogObserver) OnStreamClose() { + o.mu.Lock() + defer o.mu.Unlock() if o.closed { return } // ... rest of method }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auditlog/stream_observer.go` around lines 10 - 17, StreamLogObserver has a race on shared fields (entry, builder, closed) because OnJSONEvent and OnStreamClose access/modify them concurrently; add a sync.Mutex (e.g., mu) to the StreamLogObserver struct and use mu.Lock()/mu.Unlock() to protect all reads and writes to entry, builder, and closed inside OnJSONEvent and OnStreamClose (ensure early returns check closed under the lock and mutations like setting closed=true and nil-ing builder/entry happen while holding the lock).internal/usage/stream_observer.go (1)
6-15:⚠️ Potential issue | 🔴 CriticalRace condition:
cachedEntryaccessed without synchronization.The pipeline failure indicates a data race between
OnStreamClose()writing shared state and test assertions reading it. TheStreamUsageObserverstruct lacks synchronization primitives, butcachedEntryis written inOnJSONEventand read/written inOnStreamClose, which can be called from different goroutines during streaming.🔒 Proposed fix to add mutex protection
type StreamUsageObserver struct { logger LoggerInterface pricingResolver PricingResolver cachedEntry *UsageEntry model string provider string requestID string endpoint string closed bool + mu sync.Mutex }Then protect access in
OnJSONEventandOnStreamClose:func (o *StreamUsageObserver) OnJSONEvent(chunk map[string]any) { + o.mu.Lock() + defer o.mu.Unlock() entry := o.extractUsageFromEvent(chunk) if entry != nil { o.cachedEntry = mergeUsageEntries(o.cachedEntry, entry) } } func (o *StreamUsageObserver) OnStreamClose() { + o.mu.Lock() + defer o.mu.Unlock() if o.closed { return } o.closed = true if o.cachedEntry != nil && o.logger != nil { o.logger.Write(o.cachedEntry) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usage/stream_observer.go` around lines 6 - 15, StreamUsageObserver has a race on cachedEntry (and related state like closed) because OnJSONEvent and OnStreamClose can run concurrently; add a sync.Mutex (or sync.RWMutex) field to the StreamUsageObserver struct and use it to guard all reads and writes to cachedEntry and closed inside the OnJSONEvent and OnStreamClose methods (and any other methods that touch those fields) so updates and inspections are atomic and race-free.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/config.go`:
- Around line 349-360: The config exposes ExperimentalForwardProxyEnabled,
ExperimentalForwardProxyMITMHosts, ExperimentalForwardProxyCACertFile and
ExperimentalForwardProxyCAKeyFile such that enabling the feature without CA
paths or explicit MITM hosts causes runtime failure; add config-time validation
in the Config load/validate path (e.g., implement/extend Config.Validate or the
function that parses the YAML) to reject configurations where
ExperimentalForwardProxyEnabled is true but either
ExperimentalForwardProxyMITMHosts is empty or the CA files
(ExperimentalForwardProxyCACertFile/ExperimentalForwardProxyCAKeyFile) are
empty/missing, returning a clear error; alternatively, make
ExperimentalForwardProxyMITMHosts default to an empty slice and ensure the
validator requires explicit non-empty hosts when enabled so the app fails fast
with a descriptive message.
In `@internal/auditlog/stream_wrapper.go`:
- Around line 77-91: The buildAnthropicMessageResponse method reads mutable
fields on streamResponseBuilder (ID, Role, Model, FinishReason, Content) without
synchronization; acquire the builder's mutex at the start of
buildAnthropicMessageResponse and release it after capturing/snapshotting those
fields so you construct the map from a consistent, locked snapshot. Also ensure
the same mutex is used to guard all mutations to those fields in the event
parser functions (the writers that set ID, Role, Model, FinishReason, and write
to Content) so reads and writes use the same lock and eliminate the race.
In `@internal/server/forward_proxy_test.go`:
- Around line 244-248: The TLS config for the test HTTP client is missing a
MinVersion; update the TLSClientConfig used in the test transport (the
tls.Config instance passed as TLSClientConfig) to include a MinVersion field
(e.g., set MinVersion to tls.VersionTLS12) so the test client enforces a minimum
TLS version during connections.
In `@internal/server/forward_proxy.go`:
- Around line 287-289: Add an explanatory code comment where the proxy
short-circuits requests to "/api/event_logging/v2/batch" (the block that calls
h.serveSyntheticEventLoggingSuccess(clientConn, req, start, requestID, model,
bodyBytes)) clarifying that this path is intentionally intercepted and a
synthetic 204 No Content is returned instead of forwarding upstream to
suppress/handle Claude Code telemetry; mirror the same explanatory comment for
the similar interception logic around the later block (lines handling the same
path near the 338-353 region) so future readers know this is deliberate behavior
and not a bug.
- Around line 177-185: The two io.Copy goroutines in handleTunnelConnect can
block if one side errors; modify them to propagate shutdown by capturing each
goroutine's error and, on exit, signal the peer using connection half-close or
deadlines: after io.Copy returns for upstreamConn->clientConn, call
CloseWrite/CloseRead (use a type assertion to interface{ CloseWrite() error } or
interface{ CloseRead() error } on clientConn/upstreamConn) or set a short
ReadDeadline on the peer to unblock the other goroutine, then fully Close the
connection; do the symmetric action in the other goroutine (capture error,
signal peer, then Close). Ensure you reference the existing upstreamConn and
clientConn variables and perform type assertions safely before calling
CloseWrite/CloseRead.
- Around line 409-461: The handleHTTPProxy method uses plain http.Error
responses (in forwardProxyHandler.handleHTTPProxy) for read/body/roundtrip
failures; replace each http.Error call with a JSON OpenAI-shaped
core.GatewayError response by constructing a gatewayErr via
core.NewProviderError (use http.StatusBadRequest for request-body read errors
and http.StatusBadGateway for upstream errors), set Content-Type:
application/json, write the gatewayErr.HTTPStatusCode(), and json.Encode a
payload like map[string]any{"error": gatewayErr.ToJSON()}; ensure you still
create and populate the audit entry (newProxyAuditEntry) and write it before
sending the gateway error for upstream failures.
In `@internal/server/http.go`:
- Around line 279-291: The current block silently logs errors from
NewForwardProxyHandler and continues with plain routing; when
ExperimentalForwardProxy.Enabled is true we must fail fast. Change the error
branch after calling NewForwardProxyHandler to return or propagate the error
(wrap with context like "failed to enable experimental forward proxy") instead
of just slog.Error, so startup aborts rather than falling back to plain root;
ensure the caller in internal/app/app.go receives and returns this error up the
chain so initialization stops when NewForwardProxyHandler fails.
- Around line 279-291: The forward proxy handler (forwardProxyHandler.ServeHTTP
and its helpers handleConnect, handleMITMConnect, handleHTTPProxy) currently
bypasses AuthMiddleware and only uses proxyCredentialHash for audit logging;
update the proxy path so authentication is enforced before any proxy handling:
either wrap the created proxyHandler with the existing AuthMiddleware when
assigning root (where NewForwardProxyHandler is invoked) or inject the same
auth/validator into NewForwardProxyHandler and have
handleConnect/handleMITMConnect/handleHTTPProxy call the shared validation
routine (e.g., validate master key or existing auth validator) and return an
HTTP 401/403 on failure; also ensure proxyCredentialHash values are used for
access control checks (not only audit) so unauthorized CONNECT/absolute-form
requests are blocked.
In `@README.md`:
- Around line 172-174: Add two rows to the environment variables key-settings
table to list EXPERIMENTAL_FORWARD_PROXY_CA_CERT_FILE and
EXPERIMENTAL_FORWARD_PROXY_CA_KEY_FILE (matching the paragraph that mentions
them); set an appropriate default (e.g., empty) and a short description like
"Path to CA cert file for forward-proxy MITM" and "Path to CA key file for
forward-proxy MITM" so the README's table includes these two env vars alongside
EXPERIMENTAL_FORWARD_PROXY_ENABLED, EXPERIMENTAL_FORWARD_PROXY_MITM_HOSTS, and
CACHE_TYPE.
---
Outside diff comments:
In `@internal/auditlog/stream_observer.go`:
- Around line 10-17: StreamLogObserver has a race on shared fields (entry,
builder, closed) because OnJSONEvent and OnStreamClose access/modify them
concurrently; add a sync.Mutex (e.g., mu) to the StreamLogObserver struct and
use mu.Lock()/mu.Unlock() to protect all reads and writes to entry, builder, and
closed inside OnJSONEvent and OnStreamClose (ensure early returns check closed
under the lock and mutations like setting closed=true and nil-ing builder/entry
happen while holding the lock).
In `@internal/usage/stream_observer.go`:
- Around line 6-15: StreamUsageObserver has a race on cachedEntry (and related
state like closed) because OnJSONEvent and OnStreamClose can run concurrently;
add a sync.Mutex (or sync.RWMutex) field to the StreamUsageObserver struct and
use it to guard all reads and writes to cachedEntry and closed inside the
OnJSONEvent and OnStreamClose methods (and any other methods that touch those
fields) so updates and inspections are atomic and race-free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c142eed4-5fd6-43d1-81b8-26c0fb429381
📒 Files selected for processing (13)
README.mdconfig/config.example.yamlconfig/config.godocs/guides/claude-code.mdxinternal/app/app.gointernal/auditlog/auditlog_test.gointernal/auditlog/stream_observer.gointernal/auditlog/stream_wrapper.gointernal/server/forward_proxy.gointernal/server/forward_proxy_test.gointernal/server/http.gointernal/usage/stream_observer.gointernal/usage/stream_observer_test.go
Comment on lines
+349
to
+360
| // ExperimentalForwardProxyEnabled enables an HTTP forward proxy entrypoint that can | ||
| // optionally MITM selected HTTPS hosts for traffic inspection. Default: false. | ||
| ExperimentalForwardProxyEnabled bool `yaml:"experimental_forward_proxy_enabled" env:"EXPERIMENTAL_FORWARD_PROXY_ENABLED"` | ||
| // ExperimentalForwardProxyMITMHosts lists the hosts whose HTTPS CONNECT traffic | ||
| // should be terminated and inspected. Other hosts are tunneled blindly. | ||
| ExperimentalForwardProxyMITMHosts []string `yaml:"experimental_forward_proxy_mitm_hosts" env:"EXPERIMENTAL_FORWARD_PROXY_MITM_HOSTS"` | ||
| // ExperimentalForwardProxyCACertFile points at the PEM-encoded CA certificate used | ||
| // to mint leaf certificates for inspected HTTPS hosts. | ||
| ExperimentalForwardProxyCACertFile string `yaml:"experimental_forward_proxy_ca_cert_file" env:"EXPERIMENTAL_FORWARD_PROXY_CA_CERT_FILE"` | ||
| // ExperimentalForwardProxyCAKeyFile points at the PEM-encoded CA private key used | ||
| // to mint leaf certificates for inspected HTTPS hosts. | ||
| ExperimentalForwardProxyCAKeyFile string `yaml:"experimental_forward_proxy_ca_key_file" env:"EXPERIMENTAL_FORWARD_PROXY_CA_KEY_FILE"` |
Contributor
There was a problem hiding this comment.
Default MITM host + empty CA paths creates a brittle enabled state.
With Line 431 defaulting ExperimentalForwardProxyMITMHosts and Lines 357-360 defaulting CA paths to empty, turning on only EXPERIMENTAL_FORWARD_PROXY_ENABLED leads to runtime proxy init failure. Add config-time validation (or default MITM hosts to empty) so this fails fast with a clear config error.
🛠️ Proposed fix
func Load() (*LoadResult, error) {
...
+ if cfg.Server.ExperimentalForwardProxyEnabled &&
+ len(cfg.Server.ExperimentalForwardProxyMITMHosts) > 0 &&
+ (strings.TrimSpace(cfg.Server.ExperimentalForwardProxyCACertFile) == "" ||
+ strings.TrimSpace(cfg.Server.ExperimentalForwardProxyCAKeyFile) == "") {
+ return nil, fmt.Errorf("experimental forward proxy MITM requires both EXPERIMENTAL_FORWARD_PROXY_CA_CERT_FILE and EXPERIMENTAL_FORWARD_PROXY_CA_KEY_FILE")
+ }
...
}Alternative: set default ExperimentalForwardProxyMITMHosts to empty and require explicit opt-in hosts.
Also applies to: 431-431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/config.go` around lines 349 - 360, The config exposes
ExperimentalForwardProxyEnabled, ExperimentalForwardProxyMITMHosts,
ExperimentalForwardProxyCACertFile and ExperimentalForwardProxyCAKeyFile such
that enabling the feature without CA paths or explicit MITM hosts causes runtime
failure; add config-time validation in the Config load/validate path (e.g.,
implement/extend Config.Validate or the function that parses the YAML) to reject
configurations where ExperimentalForwardProxyEnabled is true but either
ExperimentalForwardProxyMITMHosts is empty or the CA files
(ExperimentalForwardProxyCACertFile/ExperimentalForwardProxyCAKeyFile) are
empty/missing, returning a clear error; alternatively, make
ExperimentalForwardProxyMITMHosts default to an empty slice and ensure the
validator requires explicit non-empty hosts when enabled so the app fails fast
with a descriptive message.
Comment on lines
+77
to
+91
| func (b *streamResponseBuilder) buildAnthropicMessageResponse() map[string]any { | ||
| return map[string]any{ | ||
| "id": b.ID, | ||
| "type": "message", | ||
| "role": b.Role, | ||
| "model": b.Model, | ||
| "content": []map[string]any{ | ||
| { | ||
| "type": "text", | ||
| "text": b.Content.String(), | ||
| }, | ||
| }, | ||
| "stop_reason": b.FinishReason, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Synchronize Anthropic response snapshot to fix the CI race.
Line 77 reads mutable stream builder state (ID, Role, Model, FinishReason, Content) without synchronization, and CI already reports a race on this path. Please guard both field mutation and snapshot reads with the same mutex before constructing ResponseBody.
🔧 Proposed fix direction
type streamResponseBuilder struct {
+ mu sync.Mutex
// ChatCompletion fields
ID string
...
}
func (b *streamResponseBuilder) buildAnthropicMessageResponse() map[string]any {
+ b.mu.Lock()
+ id := b.ID
+ role := b.Role
+ model := b.Model
+ stopReason := b.FinishReason
+ text := b.Content.String()
+ b.mu.Unlock()
+
return map[string]any{
- "id": b.ID,
+ "id": id,
"type": "message",
- "role": b.Role,
- "model": b.Model,
+ "role": role,
+ "model": model,
"content": []map[string]any{
{
"type": "text",
- "text": b.Content.String(),
+ "text": text,
},
},
- "stop_reason": b.FinishReason,
+ "stop_reason": stopReason,
}
}Also lock all builder writes in event parsers with the same mutex.
🧰 Tools
🪛 GitHub Actions: CI
[error] 78-84: Race participant: streamResponseBuilder.buildAnthropicMessageResponse() writes to shared map/state concurrently with reads in TestForwardProxyMITMAnthropicStreamingUsageAndAudit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auditlog/stream_wrapper.go` around lines 77 - 91, The
buildAnthropicMessageResponse method reads mutable fields on
streamResponseBuilder (ID, Role, Model, FinishReason, Content) without
synchronization; acquire the builder's mutex at the start of
buildAnthropicMessageResponse and release it after capturing/snapshotting those
fields so you construct the map from a consistent, locked snapshot. Also ensure
the same mutex is used to guard all mutations to those fields in the event
parser functions (the writers that set ID, Role, Model, FinishReason, and write
to Content) so reads and writes use the same lock and eliminate the race.
Comment on lines
+244
to
+248
| Proxy: http.ProxyURL(proxyURL), | ||
| TLSClientConfig: &tls.Config{ | ||
| RootCAs: pool, | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding TLS MinVersion for test client.
Static analysis flags missing MinVersion in the TLS config. While this is test code, it's good practice to set a minimum version.
♻️ Proposed fix
return &http.Client{
Transport: &http.Transport{
Proxy: http.ProxyURL(proxyURL),
TLSClientConfig: &tls.Config{
RootCAs: pool,
+ MinVersion: tls.VersionTLS12,
},
},
}📝 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.
Suggested change
| Proxy: http.ProxyURL(proxyURL), | |
| TLSClientConfig: &tls.Config{ | |
| RootCAs: pool, | |
| }, | |
| }, | |
| Proxy: http.ProxyURL(proxyURL), | |
| TLSClientConfig: &tls.Config{ | |
| RootCAs: pool, | |
| MinVersion: tls.VersionTLS12, | |
| }, | |
| }, |
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 244-246: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
RootCAs: pool,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/forward_proxy_test.go` around lines 244 - 248, The TLS config
for the test HTTP client is missing a MinVersion; update the TLSClientConfig
used in the test transport (the tls.Config instance passed as TLSClientConfig)
to include a MinVersion field (e.g., set MinVersion to tls.VersionTLS12) so the
test client enforces a minimum TLS version during connections.
Comment on lines
+177
to
+185
| go func() { | ||
| _, _ = io.Copy(upstreamConn, clientConn) | ||
| _ = upstreamConn.Close() | ||
| }() | ||
| go func() { | ||
| _, _ = io.Copy(clientConn, upstreamConn) | ||
| _ = clientConn.Close() | ||
| }() | ||
| } |
Contributor
There was a problem hiding this comment.
Tunnel goroutines may block indefinitely if one direction fails.
In handleTunnelConnect, two goroutines copy data bidirectionally. If one direction encounters an error (e.g., client disconnects), the other goroutine may block indefinitely waiting for data. Consider setting deadlines or using CloseWrite/CloseRead to signal shutdown.
🛡️ Proposed improvement with deadline signaling
+ done := make(chan struct{})
go func() {
_, _ = io.Copy(upstreamConn, clientConn)
- _ = upstreamConn.Close()
+ if tc, ok := upstreamConn.(*net.TCPConn); ok {
+ _ = tc.CloseWrite()
+ }
+ close(done)
}()
go func() {
_, _ = io.Copy(clientConn, upstreamConn)
- _ = clientConn.Close()
+ <-done
+ _ = upstreamConn.Close()
+ _ = clientConn.Close()
}()Alternatively, set read deadlines after one direction completes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/forward_proxy.go` around lines 177 - 185, The two io.Copy
goroutines in handleTunnelConnect can block if one side errors; modify them to
propagate shutdown by capturing each goroutine's error and, on exit, signal the
peer using connection half-close or deadlines: after io.Copy returns for
upstreamConn->clientConn, call CloseWrite/CloseRead (use a type assertion to
interface{ CloseWrite() error } or interface{ CloseRead() error } on
clientConn/upstreamConn) or set a short ReadDeadline on the peer to unblock the
other goroutine, then fully Close the connection; do the symmetric action in the
other goroutine (capture error, signal peer, then Close). Ensure you reference
the existing upstreamConn and clientConn variables and perform type assertions
safely before calling CloseWrite/CloseRead.
Comment on lines
+287
to
+289
| if req.URL.Path == "/api/event_logging/v2/batch" { | ||
| return h.serveSyntheticEventLoggingSuccess(clientConn, req, start, requestID, model, bodyBytes) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the synthetic event logging interception.
The /api/event_logging/v2/batch path returns a synthetic 204 No Content without forwarding to upstream. This appears intentional to intercept Claude Code telemetry. Consider adding a code comment explaining this behavior and its purpose.
📝 Add explanatory comment
+ // Claude Code sends telemetry to /api/event_logging/v2/batch.
+ // Intercept and return success without forwarding to prevent telemetry
+ // leakage while keeping the client happy.
if req.URL.Path == "/api/event_logging/v2/batch" {
return h.serveSyntheticEventLoggingSuccess(clientConn, req, start, requestID, model, bodyBytes)
}Also applies to: 338-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/forward_proxy.go` around lines 287 - 289, Add an explanatory
code comment where the proxy short-circuits requests to
"/api/event_logging/v2/batch" (the block that calls
h.serveSyntheticEventLoggingSuccess(clientConn, req, start, requestID, model,
bodyBytes)) clarifying that this path is intentionally intercepted and a
synthetic 204 No Content is returned instead of forwarding upstream to
suppress/handle Claude Code telemetry; mirror the same explanatory comment for
the similar interception logic around the later block (lines handling the same
path near the 338-353 region) so future readers know this is deliberate behavior
and not a bug.
Comment on lines
+409
to
+461
| func (h *forwardProxyHandler) handleHTTPProxy(w http.ResponseWriter, r *http.Request) { | ||
| start := time.Now().UTC() | ||
| requestID := ensureProxyRequestID(r.Header) | ||
|
|
||
| bodyBytes, err := io.ReadAll(r.Body) | ||
| if err != nil { | ||
| http.Error(w, "failed to read proxy request body", http.StatusBadRequest) | ||
| return | ||
| } | ||
| model, _ := extractProxyRequestInfo(bodyBytes) | ||
|
|
||
| upstreamReq := r.Clone(context.Background()) | ||
| upstreamReq.RequestURI = "" | ||
| upstreamReq.Body = io.NopCloser(bytes.NewReader(bodyBytes)) | ||
| upstreamReq.ContentLength = int64(len(bodyBytes)) | ||
| upstreamReq.Header = cloneHeader(r.Header) | ||
| stripProxyHeaders(upstreamReq.Header) | ||
| upstreamReq.Header.Del("Accept-Encoding") | ||
|
|
||
| resp, err := h.transport.RoundTrip(upstreamReq) | ||
| if err != nil { | ||
| entry := h.newProxyAuditEntry(start, requestID, r, r.URL.Path, model) | ||
| entry.StatusCode = http.StatusBadGateway | ||
| entry.ErrorType = "proxy_error" | ||
| entry.Data.ErrorMessage = err.Error() | ||
| h.writeAuditEntry(entry) | ||
| http.Error(w, "proxy upstream request failed", http.StatusBadGateway) | ||
| return | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() | ||
|
|
||
| respBody, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| http.Error(w, "failed to read proxy upstream response", http.StatusBadGateway) | ||
| return | ||
| } | ||
|
|
||
| entry := h.newProxyAuditEntry(start, requestID, r, r.URL.Path, model) | ||
| entry.StatusCode = resp.StatusCode | ||
| entry.DurationNs = time.Since(start).Nanoseconds() | ||
| entry.Data.RequestBody, entry.Data.RequestBodyTooBigToHandle = proxyBodyForAudit(bodyBytes) | ||
| entry.Data.ResponseBody, entry.Data.ResponseBodyTooBigToHandle = proxyBodyForAudit(respBody) | ||
| entry.Data.ResponseHeaders = proxyHeaderMap(resp.Header) | ||
| h.writeAuditEntry(entry) | ||
|
|
||
| if usageEntry := h.extractAnthropicUsageEntry(respBody, requestID, model, r.URL.Path); usageEntry != nil { | ||
| h.writeUsageEntry(usageEntry) | ||
| } | ||
|
|
||
| copyProxyResponseHeaders(w.Header(), resp.Header) | ||
| w.WriteHeader(resp.StatusCode) | ||
| _, _ = w.Write(respBody) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other proxy handlers in the codebase handle errors
rg -n 'http\.Error' internal/server/ --type go
rg -n 'GatewayError' internal/server/ --type goRepository: ENTERPILOT/GOModel
Length of output: 6877
🏁 Script executed:
# Check error_support.go to understand GatewayError usage
cat -n internal/server/error_support.go | head -30Repository: ENTERPILOT/GOModel
Length of output: 955
🏁 Script executed:
# Check if there are any tests for proxy error responses
fd -e test.go -e _test.go internal/server/forward_proxy | xargs grep -l "handleHTTPProxy\|Error\|proxy" 2>/dev/null | head -5Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Check if GatewayError is used elsewhere in forward_proxy.go
rg -n "GatewayError" internal/server/forward_proxy.goRepository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Look for context on proxy's purpose - check comments or documentation
rg -B 5 -A 5 "func (h \*forwardProxyHandler) handleHTTPProxy" internal/server/forward_proxy.goRepository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Check if there's a pattern for how errors should be written in this codebase
rg -n "w.WriteHeader\|json.NewEncoder" internal/server/forward_proxy.goRepository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Get context around handleHTTPProxy function
sed -n '395,420p' internal/server/forward_proxy.goRepository: ENTERPILOT/GOModel
Length of output: 812
🏁 Script executed:
# Check what type of proxy this is and its purpose
rg -B 10 "handleHTTPProxy" internal/server/forward_proxy.go | head -20Repository: ENTERPILOT/GOModel
Length of output: 547
🏁 Script executed:
# Check if proxy responses are meant to be transparent or modified
rg -A 20 "copyProxyResponseHeaders\|func (h \*forwardProxyHandler)" internal/server/forward_proxy.go | head -40Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Check if there are any specifications about proxy behavior
fd -e md -e txt README -i PROXY | head -5Repository: ENTERPILOT/GOModel
Length of output: 154
🏁 Script executed:
# Look for where handleHTTPProxy is registered and how it fits into the server
rg -n "handleHTTPProxy\|forward_proxy" internal/server/ --type go | grep -v "func\|//"Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Check isForwardProxyRequest to understand what constitutes a proxy request
rg -A 10 "func isForwardProxyRequest" internal/server/forward_proxy.goRepository: ENTERPILOT/GOModel
Length of output: 463
🏁 Script executed:
# Check the broader context - what is this proxy for?
rg -B 5 "forwardProxyHandler\|forward.?proxy" internal/server/ --type go | grep -i "new\|init\|register" | head -10Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Check if there's any documentation in comments about proxy error handling
rg -B 5 "handleHTTPProxy\|handleConnect" internal/server/forward_proxy.go | grep -E "//|/\*" | head -20Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Understand the architecture - does proxy need to be OpenAI-compatible?
rg "OpenAI\|openai" internal/ --type go | head -10Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Check what a GatewayError looks like and if it can be used with http.ResponseWriter
rg -A 15 "type GatewayError" internal/Repository: ENTERPILOT/GOModel
Length of output: 969
🏁 Script executed:
# Check if there's a utility to write GatewayError to http.ResponseWriter
rg "ToJSON\|WriteHeader" internal/core/ --type go -A 3 | head -30Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Look for how GatewayError is supposed to be serialized
rg -A 5 "func.*ToJSON" internal/core/Repository: ENTERPILOT/GOModel
Length of output: 1005
🏁 Script executed:
# Check if there are any other places using http.ResponseWriter that write errors properly
rg -B 5 -A 5 "w.WriteHeader.*http\." internal/server/ --type go | grep -v "proxy" | head -40Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Understand the proxy's role in the architecture
rg -B 5 "ServeHTTP.*forwardProxyHandler" internal/server/forward_proxy.goRepository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Check if proxy errors should match API error format
rg "json.NewEncoder\|json.Marshal" internal/server/ --type go -B 2 -A 2 | head -40Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Check the full GatewayError.ToJSON implementation and HTTPStatusCode method
rg -A 20 "func (e \*GatewayError) ToJSON\|func (e \*GatewayError) HTTPStatusCode" internal/core/errors.goRepository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Check if there's a helper function to write GatewayError to http.ResponseWriter
rg -n "WriteHeader.*GatewayError\|json.NewEncoder.*GatewayError" internal/server/ --type go -A 5Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Look at a pattern of how API handlers write JSON errors
rg -B 3 -A 3 "c\.JSON.*gatewayErr" internal/server/ --type go | head -40Repository: ENTERPILOT/GOModel
Length of output: 917
🏁 Script executed:
# Check if there are comments or TODOs about proxy error handling
rg -B 2 -A 2 "TODO\|FIXME\|TODO" internal/server/forward_proxy.go | head -20Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Verify what the intent is - does proxy need to maintain upstream response format?
rg -B 5 -A 15 "copyProxyResponseHeaders\|WriteHeader.*resp\.StatusCode" internal/server/forward_proxy.go | head -50Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Check if there's a utility function for writing GatewayError to http.ResponseWriter
rg -n "func.*write.*Error\|func.*Write.*Error" internal/server/ --type go -A 5Repository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# See the full GatewayError struct and its methods
cat -n internal/core/errors.go | head -80Repository: ENTERPILOT/GOModel
Length of output: 2928
🏁 Script executed:
# Check if there are handler tests that show expected error response formats
rg -l "handleHTTPProxy\|forwardProxy" internal/server/ --type go | xargs grep -l "test\|Test"Repository: ENTERPILOT/GOModel
Length of output: 46
🏁 Script executed:
# Verify the coding guideline scope - does it apply to all handlers?
cat -n CLAUDE.md 2>/dev/null || cat -n claude.md 2>/dev/null || echo "No CLAUDE.md found"Repository: ENTERPILOT/GOModel
Length of output: 7057
🏁 Script executed:
# Check if there's any distinction made for proxy handlers in error handling
rg "forward.*proxy\|proxy.*error\|proxy.*error.*handling" internal/server/ --type go -i | head -20Repository: ENTERPILOT/GOModel
Length of output: 44
Replace http.Error() calls with properly formatted core.GatewayError responses.
Lines 415, 435, and 442 use http.Error() which returns plain text. Per the error handling guideline, all errors returned to clients—including proxy errors—must be core.GatewayError instances serialized to JSON with the OpenAI-compatible shape. Replace these with:
gatewayErr := core.NewProviderError("", http.StatusBadRequest, "failed to read proxy request body", err)
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(gatewayErr.HTTPStatusCode())
json.NewEncoder(w).Encode(map[string]any{"error": gatewayErr.ToJSON()})Adjust the error type and message for lines 435 and 442 accordingly (e.g., http.StatusBadGateway for proxy upstream failures).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/forward_proxy.go` around lines 409 - 461, The handleHTTPProxy
method uses plain http.Error responses (in forwardProxyHandler.handleHTTPProxy)
for read/body/roundtrip failures; replace each http.Error call with a JSON
OpenAI-shaped core.GatewayError response by constructing a gatewayErr via
core.NewProviderError (use http.StatusBadRequest for request-body read errors
and http.StatusBadGateway for upstream errors), set Content-Type:
application/json, write the gatewayErr.HTTPStatusCode(), and json.Encode a
payload like map[string]any{"error": gatewayErr.ToJSON()}; ensure you still
create and populate the audit entry (newProxyAuditEntry) and write it before
sending the gateway error for upstream failures.
Comment on lines
+279
to
+291
| root := http.Handler(e) | ||
| if cfg != nil && cfg.ExperimentalForwardProxy != nil && cfg.ExperimentalForwardProxy.Enabled { | ||
| proxyCfg := *cfg.ExperimentalForwardProxy | ||
| proxyCfg.AuditLogger = auditLogger | ||
| proxyCfg.UsageLogger = usageLogger | ||
| proxyCfg.PricingResolver = pricingResolver | ||
| proxyHandler, err := NewForwardProxyHandler(root, &proxyCfg) | ||
| if err != nil { | ||
| slog.Error("failed to enable experimental forward proxy", "error", err) | ||
| } else { | ||
| root = proxyHandler | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Do not silently disable proxy mode when explicit enablement fails.
At Line 286-289, proxy construction errors are logged and startup continues with plain API routing. This fail-open behavior can silently drop expected proxy/audit/usage capture. When explicitly enabled, initialization should fail fast.
🔁 Proposed refactor direction
-func New(provider core.RoutableProvider, cfg *Config) *Server {
+func New(provider core.RoutableProvider, cfg *Config) (*Server, error) {
...
if cfg != nil && cfg.ExperimentalForwardProxy != nil && cfg.ExperimentalForwardProxy.Enabled {
...
proxyHandler, err := NewForwardProxyHandler(root, &proxyCfg)
if err != nil {
- slog.Error("failed to enable experimental forward proxy", "error", err)
+ return nil, fmt.Errorf("failed to enable experimental forward proxy: %w", err)
} else {
root = proxyHandler
}
}
- return &Server{...}
+ return &Server{...}, nil
}Then propagate the error from internal/app/app.go.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/http.go` around lines 279 - 291, The current block silently
logs errors from NewForwardProxyHandler and continues with plain routing; when
ExperimentalForwardProxy.Enabled is true we must fail fast. Change the error
branch after calling NewForwardProxyHandler to return or propagate the error
(wrap with context like "failed to enable experimental forward proxy") instead
of just slog.Error, so startup aborts rather than falling back to plain root;
ensure the caller in internal/app/app.go receives and returns this error up the
chain so initialization stops when NewForwardProxyHandler fails.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect proxy entrypoint and auth-related logic
rg -n -C4 'func \(h \*forwardProxyHandler\) ServeHTTP|CONNECT|Proxy-Authorization|Authorization|MasterKey|Auth|authenticate|401|407' internal/server/forward_proxy.go
# 2) Confirm test coverage for proxy auth behavior
rg -n -C3 'ForwardProxy|forward proxy|CONNECT|master key|GOMODEL_MASTER_KEY|401|407|Proxy-Authorization' --type go internal/server/*_test.goRepository: ENTERPILOT/GOModel
Length of output: 17321
🏁 Script executed:
# Get full ServeHTTP method and related proxy handling logic
sed -n '122,200p' internal/server/forward_proxy.go
# Check for any auth/master key validation in proxy handling
rg -n 'masterKey|AuthMiddleware|CheckAuth|Unauthorized|401' internal/server/forward_proxy.go
# Look for proxy credential validation
rg -n -A5 -B5 'proxyCredentialHash|validateProxyCredentials|proxyAuth' internal/server/forward_proxy.goRepository: ENTERPILOT/GOModel
Length of output: 2887
🏁 Script executed:
# Get handleHTTPProxy implementation
sed -n '130,300p' internal/server/forward_proxy.go | grep -A50 'func (h \*forwardProxyHandler) handleHTTPProxy'
# Search for any master key validation in forward proxy
rg -n 'masterKey|cfg.MasterKey' internal/server/forward_proxy.goRepository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# View more of the forward_proxy.go file to find handleHTTPProxy
rg -n 'func (h \*forwardProxyHandler) handleHTTPProxy' -A 30 internal/server/forward_proxy.go
# Also check if there's any masterKey reference anywhere in the forward proxy implementation
rg -n 'masterKey' internal/server/forward_proxy.goRepository: ENTERPILOT/GOModel
Length of output: 44
🏁 Script executed:
# Get full file to understand structure
wc -l internal/server/forward_proxy.go
# Search for all method names
rg -n 'func \(h \*forwardProxyHandler\)' internal/server/forward_proxy.go
# Search for handleHTTPProxy without regex
grep -n 'handleHTTPProxy' internal/server/forward_proxy.goRepository: ENTERPILOT/GOModel
Length of output: 1767
🏁 Script executed:
# View handleHTTPProxy implementation
sed -n '409,462p' internal/server/forward_proxy.go
# Check entire ServeHTTP to handleHTTPProxy flow and look for auth
sed -n '122,145p' internal/server/forward_proxy.goRepository: ENTERPILOT/GOModel
Length of output: 2574
🏁 Script executed:
# Check if there are any tests for proxy auth scenarios
grep -n -i 'masterkey\|master key\|authorization\|auth' internal/server/forward_proxy_test.go | head -30
# Look for proxy tests with auth requirements
grep -n 'func Test.*Proxy' internal/server/forward_proxy_test.go
# Check if startTestForwardProxy sets up master key
sed -n '221,260p' internal/server/forward_proxy_test.goRepository: ENTERPILOT/GOModel
Length of output: 2051
🏁 Script executed:
# Final verification: check if there's any auth enforcement before proxy handler is called
sed -n '270,295p' internal/server/http.go
# Verify the proxy handler wraps the full Echo instance with no pre-auth
grep -n -B5 -A10 'NewForwardProxyHandler' internal/server/http.goRepository: ENTERPILOT/GOModel
Length of output: 1572
Verify proxy-mode requests still enforce authentication.
Proxy requests (CONNECT and absolute-form URIs) handled by forwardProxyHandler.ServeHTTP bypass AuthMiddleware entirely. handleConnect, handleMITMConnect, and handleHTTPProxy proceed without master key validation. Credentials extracted via proxyCredentialHash are used only for audit logging, not access control. Tests do not cover unauthorized proxy request scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/http.go` around lines 279 - 291, The forward proxy handler
(forwardProxyHandler.ServeHTTP and its helpers handleConnect, handleMITMConnect,
handleHTTPProxy) currently bypasses AuthMiddleware and only uses
proxyCredentialHash for audit logging; update the proxy path so authentication
is enforced before any proxy handling: either wrap the created proxyHandler with
the existing AuthMiddleware when assigning root (where NewForwardProxyHandler is
invoked) or inject the same auth/validator into NewForwardProxyHandler and have
handleConnect/handleMITMConnect/handleHTTPProxy call the shared validation
routine (e.g., validate master key or existing auth validator) and return an
HTTP 401/403 on failure; also ensure proxyCredentialHash values are used for
access control checks (not only audit) so unauthorized CONNECT/absolute-form
requests are blocked.
Comment on lines
+172
to
174
| | `EXPERIMENTAL_FORWARD_PROXY_ENABLED` | `false` | Enable the experimental HTTP forward proxy wrapper for client traffic inspection | | ||
| | `EXPERIMENTAL_FORWARD_PROXY_MITM_HOSTS` | `api.anthropic.com` | Comma-separated HTTPS hosts to inspect; other CONNECT targets are tunneled blindly | | ||
| | `CACHE_TYPE` | `local` | Cache backend (`local` or `redis`) | |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add CA file env vars to the key-settings table for parity.
The paragraph on Line 182 mentions EXPERIMENTAL_FORWARD_PROXY_CA_CERT_FILE and EXPERIMENTAL_FORWARD_PROXY_CA_KEY_FILE, but they are not listed in the table. Adding them there improves discoverability.
Also applies to: 182-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 172 - 174, Add two rows to the environment variables
key-settings table to list EXPERIMENTAL_FORWARD_PROXY_CA_CERT_FILE and
EXPERIMENTAL_FORWARD_PROXY_CA_KEY_FILE (matching the paragraph that mentions
them); set an appropriate default (e.g., empty) and a short description like
"Path to CA cert file for forward-proxy MITM" and "Path to CA key file for
forward-proxy MITM" so the README's table includes these two env vars alongside
EXPERIMENTAL_FORWARD_PROXY_ENABLED, EXPERIMENTAL_FORWARD_PROXY_MITM_HOSTS, and
CACHE_TYPE.
…ntegration # Conflicts: # internal/server/http.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
config/config.go (1)
437-448:⚠️ Potential issue | 🟠 MajorAdd forward-proxy config validation in
Load()to fail fast.With Line 519 defaulting MITM hosts and CA paths defaulting empty, setting only
EXPERIMENTAL_FORWARD_PROXY_ENABLED=truecan enter an invalid startup path. Validate this during config loading.Proposed fix
func Load() (*LoadResult, error) { @@ if err := ValidateCacheConfig(&cfg.Cache); err != nil { return nil, err } + if cfg.Server.ExperimentalForwardProxyEnabled && + len(cfg.Server.ExperimentalForwardProxyMITMHosts) > 0 && + (strings.TrimSpace(cfg.Server.ExperimentalForwardProxyCACertFile) == "" || + strings.TrimSpace(cfg.Server.ExperimentalForwardProxyCAKeyFile) == "") { + return nil, fmt.Errorf( + "experimental forward proxy MITM requires both EXPERIMENTAL_FORWARD_PROXY_CA_CERT_FILE and EXPERIMENTAL_FORWARD_PROXY_CA_KEY_FILE", + ) + } return &LoadResult{ Config: cfg, RawProviders: rawProviders, }, nil }Also applies to: 519-519
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config.go` around lines 437 - 448, Add validation inside the Load() function that checks when ExperimentalForwardProxyEnabled is true and then ensures ExperimentalForwardProxyMITMHosts is non-empty and both ExperimentalForwardProxyCACertFile and ExperimentalForwardProxyCAKeyFile are non-empty (or other acceptable combinations your design requires); if any required value is missing, return an error (or fail fast) from Load() with a clear message referencing the missing field(s). This validation should live after defaults are applied (so the defaulting on line ~519 is considered) and reference the struct fields ExperimentalForwardProxyEnabled, ExperimentalForwardProxyMITMHosts, ExperimentalForwardProxyCACertFile, and ExperimentalForwardProxyCAKeyFile so the startup cannot proceed with an invalid forward-proxy configuration.README.md (1)
175-176:⚠️ Potential issue | 🟡 MinorAdd CA env vars to the key-settings table for consistency.
Line 185 documents
EXPERIMENTAL_FORWARD_PROXY_CA_CERT_FILEandEXPERIMENTAL_FORWARD_PROXY_CA_KEY_FILE, but the table omits them.Suggested doc patch
| `EXPERIMENTAL_FORWARD_PROXY_ENABLED` | `false` | Enable the experimental HTTP forward proxy wrapper for client traffic inspection | | `EXPERIMENTAL_FORWARD_PROXY_MITM_HOSTS` | `api.anthropic.com` | Comma-separated HTTPS hosts to inspect; other CONNECT targets are tunneled blindly | +| `EXPERIMENTAL_FORWARD_PROXY_CA_CERT_FILE` | (empty) | Path to CA certificate file used for forward-proxy MITM | +| `EXPERIMENTAL_FORWARD_PROXY_CA_KEY_FILE` | (empty) | Path to CA private key file used for forward-proxy MITM | | `CACHE_TYPE` | `local` | Cache backend (`local` or `redis`) |Also applies to: 185-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 175 - 176, The README table for key settings is missing the two CA env vars referenced later; add rows for EXPERIMENTAL_FORWARD_PROXY_CA_CERT_FILE and EXPERIMENTAL_FORWARD_PROXY_CA_KEY_FILE to the same table where EXPERIMENTAL_FORWARD_PROXY_ENABLED and EXPERIMENTAL_FORWARD_PROXY_MITM_HOSTS are listed, giving sensible defaults (e.g., empty or file paths) and a short description such as "Path to CA certificate file used by the forward proxy" and "Path to CA private key file used by the forward proxy" so the table matches the existing documentation at lines mentioning those symbols.internal/server/http.go (2)
203-206:⚠️ Potential issue | 🔴 CriticalDo not move proxy traffic outside the master-key gate.
AuthMiddleware(cfg.MasterKey, authSkipPaths)only runs inside Echo, but this block wraps Echo withNewForwardProxyHandler(...). CONNECT and absolute-form requests handled by the proxy never traverse the existing auth middleware, so enabling proxy mode can turn a master-key-protected server into an unauthenticated forward proxy. Enforce the same validation in the proxy entrypoint, or wraprootwith an equivalenthttp.Handlerauth gate before proxy dispatch.Also applies to: 288-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/http.go` around lines 203 - 206, The proxy handler is being registered outside the master-key-auth gate, allowing CONNECT/absolute-form proxy requests to bypass AuthMiddleware; ensure proxy entrypoints are validated by either wrapping the Echo router/root with the same master-key check or applying an equivalent http.Handler auth gate before calling NewForwardProxyHandler or dispatching proxy traffic. Specifically, enforce the master-key check used by AuthMiddleware(cfg.MasterKey, authSkipPaths) for the proxy path(s) (the proxy entrypoint and root handler), or wrap root with a middleware that performs the same validation prior to NewForwardProxyHandler invocation so CONNECT/absolute-form requests cannot bypass authentication.
67-67:⚠️ Potential issue | 🟠 MajorReturn an error when proxy bootstrap fails.
When
ExperimentalForwardProxy.Enabledis explicit, logging and continuing here makes startup fail open: the server comes up in plain API mode and silently stops doing the audit/usage capture this feature is meant to provide. Bubble the initialization error out ofNew(...)instead of falling back.🔁 Suggested fix
-func New(provider core.RoutableProvider, cfg *Config) *Server { +func New(provider core.RoutableProvider, cfg *Config) (*Server, error) { ... proxyHandler, err := NewForwardProxyHandler(root, &proxyCfg) if err != nil { - slog.Error("failed to enable experimental forward proxy", "error", err) - } else { - root = proxyHandler + return nil, fmt.Errorf("failed to enable experimental forward proxy: %w", err) } + root = proxyHandler ... - return &Server{ + return &Server{ echo: e, handler: handler, root: root, responseCacheMiddleware: rcm, - } + }, nil }You'll also need to add
fmtto the imports and propagate the new error from the caller.Also applies to: 294-299, 301-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/http.go` at line 67, The constructor New(provider core.RoutableProvider, cfg *Config) *Server should return an error when proxy bootstrap fails instead of silently logging and continuing; change its signature to New(provider core.RoutableProvider, cfg *Config) (*Server, error), propagate and return any error from the forward-proxy initialization (the ExperimentalForwardProxy.Enabled branch and the related bootstrap calls around the other mentioned blocks), and remove the fallback-to-plain behavior so startup fails closed; add fmt to imports if you need to wrap or annotate the error message and update all callers to handle the (*Server, error) return accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@config/config.go`:
- Around line 437-448: Add validation inside the Load() function that checks
when ExperimentalForwardProxyEnabled is true and then ensures
ExperimentalForwardProxyMITMHosts is non-empty and both
ExperimentalForwardProxyCACertFile and ExperimentalForwardProxyCAKeyFile are
non-empty (or other acceptable combinations your design requires); if any
required value is missing, return an error (or fail fast) from Load() with a
clear message referencing the missing field(s). This validation should live
after defaults are applied (so the defaulting on line ~519 is considered) and
reference the struct fields ExperimentalForwardProxyEnabled,
ExperimentalForwardProxyMITMHosts, ExperimentalForwardProxyCACertFile, and
ExperimentalForwardProxyCAKeyFile so the startup cannot proceed with an invalid
forward-proxy configuration.
In `@internal/server/http.go`:
- Around line 203-206: The proxy handler is being registered outside the
master-key-auth gate, allowing CONNECT/absolute-form proxy requests to bypass
AuthMiddleware; ensure proxy entrypoints are validated by either wrapping the
Echo router/root with the same master-key check or applying an equivalent
http.Handler auth gate before calling NewForwardProxyHandler or dispatching
proxy traffic. Specifically, enforce the master-key check used by
AuthMiddleware(cfg.MasterKey, authSkipPaths) for the proxy path(s) (the proxy
entrypoint and root handler), or wrap root with a middleware that performs the
same validation prior to NewForwardProxyHandler invocation so
CONNECT/absolute-form requests cannot bypass authentication.
- Line 67: The constructor New(provider core.RoutableProvider, cfg *Config)
*Server should return an error when proxy bootstrap fails instead of silently
logging and continuing; change its signature to New(provider
core.RoutableProvider, cfg *Config) (*Server, error), propagate and return any
error from the forward-proxy initialization (the
ExperimentalForwardProxy.Enabled branch and the related bootstrap calls around
the other mentioned blocks), and remove the fallback-to-plain behavior so
startup fails closed; add fmt to imports if you need to wrap or annotate the
error message and update all callers to handle the (*Server, error) return
accordingly.
In `@README.md`:
- Around line 175-176: The README table for key settings is missing the two CA
env vars referenced later; add rows for EXPERIMENTAL_FORWARD_PROXY_CA_CERT_FILE
and EXPERIMENTAL_FORWARD_PROXY_CA_KEY_FILE to the same table where
EXPERIMENTAL_FORWARD_PROXY_ENABLED and EXPERIMENTAL_FORWARD_PROXY_MITM_HOSTS are
listed, giving sensible defaults (e.g., empty or file paths) and a short
description such as "Path to CA certificate file used by the forward proxy" and
"Path to CA private key file used by the forward proxy" so the table matches the
existing documentation at lines mentioning those symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9dbe724b-4f36-4096-8606-0f893a14d80f
📒 Files selected for processing (8)
README.mdconfig/config.example.yamlconfig/config.gointernal/app/app.gointernal/auditlog/auditlog_test.gointernal/server/forward_proxy_test.gointernal/server/handlers_test.gointernal/server/http.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
claude -prun stored/v1/messagesusage and assistant content in GoModel's audit logsSummary by CodeRabbit
New Features
/v1/messagesstreaming and usage extractionDocumentation
Tests