feat(proxy): add capture_headers config and user_id to canonical request log

[andybons]

Adds log.capture_headers config and user_id to the canonical request log line. See commit messages for details.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 3m 4s —— View job

---

Code Review

• Gather context and read changed files
• Review security issues
• Review race conditions and synchronization
• Review bugs and logic errors
• Review error handling
• Review edge cases
• Post final review

---

Summary

The feature is well-structured. The separation between pre-injection headers (snapshotted for logging) and post-injection headers (forwarded upstream) is handled correctly in both the HTTP and CONNECT paths. The injected-credential bypass logic (credResult.InjectedHeaders guard) correctly prevents stripping headers the proxy just injected for the same name. Test coverage is thorough.

Two issues worth addressing before merge:

---

Issues

1. SetCaptureHeaders bypasses the sensitive-header blocklist (medium)

validateCaptureHeaders in gatekeeper.go blocks Authorization, Proxy-Authorization, and Cookie — but only in the YAML config path. proxy.SetCaptureHeaders has no such check, so a direct library caller can pass any of those headers and they'll be captured into logs.

Move the blocklist into the proxy package (or enforce it inside SetCaptureHeaders) so the protection is canonical regardless of how the proxy is configured. Fix this →

(Inline comment left at proxy/proxy.go line 528.)

2. RequestHeaders missing from error-path log entries in the CONNECT flow (minor)

In handleConnectWithInterception, the ErrorHandler and the network/Keep-policy denial log entries inside the wrapped handler don't include RequestHeaders. Capture-header values will be absent from those log lines.

preHeaders is already stashed in context by Rewrite (via interceptPreInjHeadersKey); the same retrieval used in ModifyResponse can be applied in the error paths:

preHeaders, _ := req.Context().Value(interceptPreInjHeadersKey{}).(http.Header)
// ...
p.logRequest(r, RequestLogData{
    ...
    RequestHeaders: preHeaders,
    ...
})

The affected callsites are the ErrorHandler (~line 2144) and the two policy-denial logRequest calls inside the wrapping handler (~lines 2181–2224). Fix this →

---

Minor nit

• gatekeeper.go line 308: utf8.Valid([]byte(v)) converts string→bytes on every loop iteration; utf8.ValidString(v) is the idiomatic form (inline suggestion left).

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 3m 3s —— View job

---

Code Review

• Gather context and read changed files
• Review security issues
• Review race conditions and synchronization
• Review bugs and logic errors
• Review error handling
• Review edge cases
• Post final review

---

Summary

Both fixes from the previous review round are correctly implemented. The sensitive-header blocklist is now enforced canonically in the proxy package, and the utf8.ValidString nit is applied. Tests are thorough, covering the strip-before-forwarding behaviour, the injected-credential bypass, and all three auth modes for user_id.

One issue from the prior review remains open.

---

Remaining Issue

RequestHeaders missing from error-path log entries in the CONNECT flow (minor, unresolved from previous review)

ErrorHandler and the network/Keep-policy denial logRequest calls inside handleConnectWithInterception still don't populate RequestHeaders. If capture_headers is configured, those fields will be silently absent from denied and 502 log lines.

ErrorHandler (proxy/proxy.go ~line 2177): Rewrite already ran and stored the pre-injection snapshot in context, so it's available:

preHeaders, _ := req.Context().Value(interceptPreInjHeadersKey{}).(http.Header)
if preHeaders == nil {
    preHeaders = req.Header.Clone()
}
p.logRequest(r, RequestLogData{
    // ...
    RequestHeaders: preHeaders,
})

Policy denial calls (~lines 2213, 2242, 2265): Rewrite hasn't run yet at those points, but the request headers are still unmodified, so req.Header.Clone() is equivalent:

p.logRequest(r, RequestLogData{
    // ...
    RequestHeaders: req.Header.Clone(),
})

---

Minor observation

handleHTTP's credential-resolution error path (~line 1541) also omits RequestHeaders, but fixing it there would require moving the originalReqHeaders snapshot above the credential call — low impact since the HTTP relay path is rarely used with capture_headers.

[andybons]

Fixed the remaining issue in facdb80: RequestHeaders is now populated in all CONNECT error/denial log paths.

• ErrorHandler: retrieves preHeaders from context (set by Rewrite), falls back to req.Header.Clone()
• Policy denial & credential error paths: uses req.Header.Clone() directly (Rewrite hasn't run yet, so headers are unmodified)

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 2m 16s —— View job

---

Code Review

• Gather context and read changed files
• Review security issues
• Review race conditions and synchronization
• Review bugs and logic errors
• Review error handling
• Review edge cases
• Post final review

---

Summary

All three issues from the previous two review rounds are correctly fixed. The feature is ready to merge.

Previous issues — all resolved:

• ✅ ValidateCaptureHeaders moved to proxy package; SetCaptureHeaders now enforces the blocklist for direct library callers
• ✅ utf8.ValidString used in place of utf8.Valid([]byte(v))
• ✅ RequestHeaders populated in the ErrorHandler, network-policy denial, Keep-policy denial, and credential-error paths in handleConnectWithInterception

The pre-injection header snapshot strategy is correct throughout: Rewrite stores preInjectionHeaders in context before credential injection, ModifyResponse/ErrorHandler retrieve it (falling back to req.Header.Clone() if the snapshot isn't present), and the policy-denial paths use req.Header.Clone() directly since injection hasn't run at that point.

The injected-credential bypass guard (credResult.InjectedHeaders[strings.ToLower(headerName)]) correctly prevents stripping a header the proxy just injected — verified in TestIntercept_CaptureHeaders_PreservesInjectedCredential.

---

One nit

Redundant double validation in gatekeeper.go New

New calls validateCaptureHeaders (~line 192) and then calls p.SetCaptureHeaders (~line 403), which internally calls proxy.ValidateCaptureHeaders a second time. Since validateCaptureHeaders is a single-line wrapper (return proxy.ValidateCaptureHeaders(headers)), this is the same check twice. The early call in New can be removed — p.SetCaptureHeaders returning an error is the correct error surface.

// Can remove this block:
if err := validateCaptureHeaders(cfg.Log.CaptureHeaders); err != nil {
    return nil, err
}

Not a bug, and the early fail-fast has minor value, so fine to leave or clean up at discretion.

---

Acknowledged gap (low-impact, pre-existing)

handleHTTP credential-error path (~line 1541) still omits RequestHeaders from its logRequest call. This was noted as low-impact in the prior round and wasn't required to fix — noting only for completeness.

[claude]

Claude finished @andybons's task in 3m 23s —— View job

---

Code Review

• Gather context and read changed files
• Review security issues
• Review race conditions and synchronization
• Review bugs and logic errors
• Review error handling and edge cases
• Post final review

---

Summary

All issues from prior review rounds are fully resolved. The feature is ready to merge.

Previously open items — all closed:

• ✅ ValidateCaptureHeaders enforced in proxy package; SetCaptureHeaders protects direct library callers
• ✅ utf8.ValidString(v) used in place of utf8.Valid([]byte(v))
• ✅ RequestHeaders populated in all CONNECT error/denial paths (ErrorHandler, network-policy denial, Keep eval error, Keep deny, credential resolution error)
• ✅ Redundant double-validation removed from gatekeeper.go — p.SetCaptureHeaders is now the single validation call
• ✅ handleHTTP credential-error path now includes RequestHeaders (the "acknowledged low-impact gap" from round 2)

Review of new code in the final two commits:

e41e53e (refactor) — removes the leftover validateCaptureHeaders wrapper and its early call in New. The remaining p.SetCaptureHeaders call at line 396 provides the same validation. Clean.

facdb80 (fix) — the ErrorHandler correctly retrieves preHeaders from context (set by Rewrite), falling back to req.Header.Clone() if the snapshot isn't present. The policy-denial paths use req.Header.Clone() directly, which is correct since Rewrite hasn't run at that point and headers are unmodified.

Auth / user_id flow:

extractProxyUsername is correctly called only after auth succeeds in each branch (contextResolver, delegateAuth, authToken). The userIDKey is stored in the outer CONNECT request context r, so logRequest(r, ...) propagates it to all inner request log entries via UserIDFromContext. Bearer-auth callers get an empty username (correctly excluded from logs). The 3-mode test coverage (contextResolver, static token, Bearer) is thorough.

Capture header stripping:

The credResult.InjectedHeaders guard correctly prevents stripping a header that the proxy just injected as a credential. The snapshot/strip sequence in both handleHTTP (originalReqHeaders before injectCredentials) and handleConnectWithInterception (Rewrite captures preInjectionHeaders before injection) ensures logs reflect client-sent values without leaking injected credentials.

Security:

The sensitiveHeaders blocklist (authorization, proxy-authorization, cookie) is enforced in ValidateCaptureHeaders, which is called by SetCaptureHeaders. Both the YAML config path and direct library callers are protected. The logger in gatekeeper.go reads only the configured CaptureHeaders keys from data.RequestHeaders — so even though the full header snapshot is stored, no sensitive header value can appear in a log line unless the operator explicitly (and unsuccessfully) tried to configure one.

No new issues found.