fix(passthrough): strip dangerous upstream response headers

Close the response-header relay hole (security review finding 5, round
3, 2026-08-22): proxyUpstream copied every upstream response header to
the client except hop-by-hop ones -- an upstream provider, or an
in-cluster MCP/A2A target reached with no credential trust boundary of
its own, could set Set-Cookie on the gateway's own origin (the same
origin serving the WAN-exposed /admin dashboard), or return
Access-Control-*/Strict-Transport-Security/Content-Security-Policy
headers to rewrite the browser's security or CORS policy toward that
origin.

Added dangerousResponseHeaders (exact-match: Set-Cookie,
Strict-Transport-Security, Content-Security-Policy) and
dangerousResponseHeaderPrefixes (Access-Control-*), stripped in
proxyUpstream right where hop-by-hop headers already are -- so both
callers (handlePassthrough here, handleTargetProxy in mcp_a2a.go) get
the fix for free.

A deny-list, not an allowlist: grepping this repo finds it only ever
reads Content-Type and Retry-After off a response (the latter only for
the unified routes' own internal retry decision, never from
passthrough). Everything else -- including provider rate-limit
telemetry headers a client's own SDK commonly reads -- is opaque to this
gateway but real passthrough transparency; an allowlist would silently
break that for every header this package doesn't already know to name.
This mirrors the request side's own already-hardened deny-list shape
(dangerousClientHeaders/providerCredentialRetargetHeaders).

Test: an upstream setting all four dangerous headers plus Content-Type,
Retry-After, and an ordinary custom header proves the four are stripped
while the rest reach the client unchanged.
This commit is contained in:
2026-08-22 14:49:53 +01:00
parent ff819e01d7
commit 60036c43df
2 changed files with 125 additions and 1 deletions
+63
View File
@@ -532,6 +532,69 @@ func TestHandlePassthrough_HopByHopHeadersStripped(t *testing.T) {
}
}
// TestHandlePassthrough_DangerousResponseHeadersStripped is the security
// review finding 5 (round 3, 2026-08-22) regression test: an upstream
// setting Set-Cookie, Access-Control-*, Strict-Transport-Security, or
// Content-Security-Policy on its response must never have those reach
// the client — an upstream (or an in-cluster MCP/A2A target) must never
// get to plant a cookie or dictate a security/CORS policy on this
// gateway's own origin. An ordinary response header, and the two headers
// this repo's own code actually reads off a response (Content-Type,
// Retry-After), must still pass through unchanged.
func TestHandlePassthrough_DangerousResponseHeadersStripped(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Set-Cookie", "session=stolen; Path=/")
w.Header().Set("Access-Control-Allow-Origin", "*")
w.Header().Set("Access-Control-Allow-Credentials", "true")
w.Header().Set("Strict-Transport-Security", "max-age=31536000; includeSubDomains")
w.Header().Set("Content-Security-Policy", "default-src 'evil.example'")
w.Header().Set("Content-Type", "application/json")
w.Header().Set("Retry-After", "30")
w.Header().Set("X-Upstream-Custom", "keep-me")
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`{}`))
}))
defer srv.Close()
cfg := CreateConfig()
cfg.Providers = map[string]*ProviderConfig{
"openai": {Type: "openai", BaseURL: srv.URL, APIKey: "sk-up"},
}
cfg.Groups = map[string]*GroupConfig{"default": {}}
cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice"}}}
next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
h, err := New(context.Background(), next, cfg, "llmgw")
if err != nil {
t.Fatalf("New: %v", err)
}
req := httptest.NewRequest(http.MethodGet, "/openai/v1/thing", nil)
req.Header.Set("Authorization", "Bearer sk-alice")
rec := httptest.NewRecorder()
h.ServeHTTP(rec, req)
if rec.Code != http.StatusOK {
t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String())
}
for _, hdr := range []string{
"Set-Cookie", "Access-Control-Allow-Origin", "Access-Control-Allow-Credentials",
"Strict-Transport-Security", "Content-Security-Policy",
} {
if v := rec.Header().Get(hdr); v != "" {
t.Errorf("client saw dangerous response header %s = %q, want stripped", hdr, v)
}
}
if rec.Header().Get("Content-Type") != "application/json" {
t.Error("client must still see Content-Type — the strip is a deny-list, not an allowlist")
}
if rec.Header().Get("Retry-After") != "30" {
t.Error("client must still see Retry-After — the strip is a deny-list, not an allowlist")
}
if rec.Header().Get("X-Upstream-Custom") != "keep-me" {
t.Error("client must still see an ordinary upstream response header — the strip must not be a full allowlist inversion")
}
}
// TestHandlePassthrough_NonStreamJSON_AccountsUsage proves a non-streaming
// application/json response's usage is extracted and accounted against
// the caller's own limiter counters.