From b7475363c48235d2f5abd7db65547f7bcbbfd54d Mon Sep 17 00:00:00 2001 From: Damon Date: Tue, 21 Jul 2026 04:58:51 -0400 Subject: [PATCH] fix(proxy): narrow the no-leak promise to what code can keep, then actually fence it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Joint review (8 roles, 2026-07-21). The review's premise was that the fence was test-only; it was not — stripAikeyRequestHeaders runs unconditionally in the single forward Director and every upstream path converges on serveRoute. That part is kept intact. What was wrong is the SCOPE of the claim and the quality of the assertions. The claim. I13 and the capability map said "no member identity reaches the upstream", full stop. The runtime guard covered only the X-Aikey-* header namespace, and the body had no guard at all — while the proxy itself writes metadata.user_id into bodies (oauth_inject.go). So the next feature that wants per-member upstream attribution could set metadata["user_id"] = , or add a header without the aikey prefix, and BOTH fences would stay green. A main-path body allowlist is the wrong fix: the WAF fingerprint check is byte-exact and oauth_inject's own closing note records that oauth_group POOL requests deliberately pass the real client identity upstream unchanged (a frozen synthetic persona became its own detection signal and was reverted 2026-06-29). And a user who types their union_id into a chat message ships it regardless. An absolute promise is physically unenforceable, and asserting one is the same failure shape as a page that shows "enabled" for a detector that never loaded. So the promise is narrowed to one the code can keep — "AiKey itself never ADDS member identity to an upstream request" — and then fenced at three layers, in member_identity_guard.go: L2 header-VALUE rule, wired into both the forward Director and the probe path, closing the "header without the aikey prefix" gap. Credential headers are an explicit, documented exemption: a random bearer can accidentally satisfy a shape and dropping it would fail every request on that account. L3 body rule placed INSIDE injectMetadataUserIDIfAbsent — the proxy's only body-identifier write — so a future caller inherits the guard rather than reopening the hole. Both warn through the central enum (proxy.request.identity_scrubbed, proxy.request.identity_inject_blocked) and never log the value, only the shape name. 🔴 Real defect found while writing the fence: stripAikeyRequestHeaders used h.Del(k), and Del canonicalises the key first — so for a non-canonical key written straight into the map (x-aikey-…), the case-insensitive match hit but Del removed a different, non-existent key and the entry SURVIVED. Now delete(h, k). Not a live leak, because net/http canonicalises inbound headers — a live blind spot. The value loop in the test was a dead assertion: it only scanned values it had itself just placed under X-Aikey-*, so it could never fail independently of the prefix assertion. It now poisons X-Member-Union-Id and X-Trace-Owner, outside that namespace. The static text scan is KEPT but demoted and labelled in the test as a proxy metric, not a terminal observation: identity flows as DATA inside GroupRuntimeAccount.Identity, which a text scan cannot see. Deleting it is a one-way door and it does catch the cheapest first move (a literal union_id json tag). Fixed: walk root is the repo root instead of one package, an anti-vacuity floor (scanned < 150 is fatal), a mandatory package list including internal/vkeys — the actual carrier of runtime material and precisely what the old scan was structurally blind to. probe_raw's stripAikeyHeaders now DELEGATES to stripAikeyRequestHeaders instead of keeping a second implementation of the same invariant, pinned by TestStripperConvergence_ProbeDelegatesToForwardPath. Schema: none. This repo has no DB schema; no migration, no new API field. Tests: go test ./... all ok. Six mutations, each verified red with the output pasted verbatim into the test comments — including M6, which proves the old scan's blindness: with the walk root restored to ".", an injected marker in internal/vkeys does not appear in the offender list at all. 🚫 Not verified: any of this end-to-end against a fake upstream with a packet capture. L1/L2 effectiveness is a read-the-code plus unit-test conclusion, not a wire observation. Co-Authored-By: Claude Opus 4.8 --- internal/observability/handler.go | 14 + internal/proxy/forward_and_resolve.go | 13 + internal/proxy/member_identity_guard.go | 133 +++++++ internal/proxy/middleware.go | 10 +- internal/proxy/oauth_inject.go | 23 ++ internal/proxy/probe_raw.go | 32 +- internal/proxy/sso_identity_no_leak_test.go | 385 ++++++++++++++++++-- 7 files changed, 577 insertions(+), 33 deletions(-) create mode 100644 internal/proxy/member_identity_guard.go diff --git a/internal/observability/handler.go b/internal/observability/handler.go index 021b62f..57ea720 100644 --- a/internal/observability/handler.go +++ b/internal/observability/handler.go @@ -101,6 +101,20 @@ const ( // only). Rejected locally with ErrCodeOAuthResponsesOnly instead of letting // ChatGPT's edge answer with a misleading "invalid x-api-key". EventProxyRequestDialectUnsupported = "proxy.request.dialect_unsupported" + // Fence I13 runtime guard (2026-07-21). EventProxyRequestIdentityScrubbed: + // an outbound header carried one of the control-plane member-identity shapes + // enumerated in proxy/member_identity_guard.go (that file is the only place + // in the data plane allowed to spell them out, so the vocabulary fence + // TestProxySource_DoesNotReferenceMemberIdentityFields keeps meaning + // something) and was dropped before the upstream saw it. + // EventProxyRequestIdentityBlocked: the + // proxy was about to write such a value into the request body's + // metadata.user_id and refused. Both are WARN, never INFO: they mean code + // upstream of the guard learned a member's provider identity, which is the + // thing to go fix — the guard only stops it reaching Anthropic/OpenAI. + // Neither ever logs the offending value; only the shape name. + EventProxyRequestIdentityScrubbed = "proxy.request.identity_scrubbed" + EventProxyRequestIdentityBlocked = "proxy.request.identity_inject_blocked" // EventProxyQuotaModelUnpriced: a completed request's model has no entry in // the edge price summary (D-U8/P7), so its usd was NOT counted locally — the // token quota floor backstops it and the server baseline catches up on diff --git a/internal/proxy/forward_and_resolve.go b/internal/proxy/forward_and_resolve.go index 138ee9a..47a5ac7 100644 --- a/internal/proxy/forward_and_resolve.go +++ b/internal/proxy/forward_and_resolve.go @@ -505,6 +505,19 @@ func (p *Proxy) serveRoute(w http.ResponseWriter, r *http.Request, route *vkeys. // X-Aikey-* ever reaches the upstream — fenced by // TestStripAikeyRequestHeaders. stripAikeyRequestHeaders(req.Header) + // Fence I13 second half (2026-07-21): the namespace strip above is a + // NAME rule and cannot see an identity value parked under a header + // that simply doesn't start with X-Aikey- (e.g. a future + // `X-Member-Union-Id`). This is the VALUE rule — see + // member_identity_guard.go for what it does and does not promise. + // Fail-loud: a hit means some code above learned a member's provider + // identity, so it WARNs rather than scrubbing silently. + if scrubbed := scrubMemberIdentityHeaders(req.Header); len(scrubbed) > 0 { + logger.Warn("member identity scrubbed from upstream request headers", + "event.name", observability.EventProxyRequestIdentityScrubbed, + "headers", scrubbed, + ) + } // Why: tell upstream we only accept identity (uncompressed) so the // drainer and non-streaming token extractor can parse the body // directly. Anthropic's OAuth endpoint in particular returns diff --git a/internal/proxy/member_identity_guard.go b/internal/proxy/member_identity_guard.go new file mode 100644 index 0000000..5e3febb --- /dev/null +++ b/internal/proxy/member_identity_guard.go @@ -0,0 +1,133 @@ +package proxy + +import ( + "net/http" + "regexp" +) + +// Member-identity shape guard — the runtime half of fence I13. +// +// # WHAT THIS FILE PROMISES, STATED EXACTLY +// +// The 2026-07-20 first-login change gave every SSO member three identifying +// values: the Feishu union_id / open_id, the tenant_key, and a synthetic +// account handle (sso+.@sso.local). All three live in the +// control plane and have no business on the wire to Anthropic or OpenAI. +// +// The promise this code can actually keep is: +// +// 🔴 AiKey ITSELF never puts member identity onto an upstream request. +// +// It is deliberately NOT the absolute "no member identity ever reaches the +// upstream". That absolute is physically unenforceable on this path and +// claiming it would be the exact "page says enabled, filter is bypassed" +// false-safety shape: +// +// - The main chat path forwards the client's body BYTE-FOR-BYTE. That is a +// correctness requirement, not an oversight — body rewrites are known to +// damage real-CLI traffic (see injectClaudeWAFFingerprintFull's guard on +// inboundClientIsClaudeCode) and the WAF fingerprint check is byte-exact. +// - oauth_inject.go's closing note is explicit that oauth_group POOL requests +// "pass the REAL client identity upstream unchanged" — deliberately, because +// a frozen synthetic persona became its own detection signal (2026-06-29). +// +// So a body allowlist on the main path would trade a level-2 stability risk +// (八级法则) for a level-1 gain we cannot complete anyway: a user who types +// their own union_id into a chat message will always ship it upstream. What we +// CAN fence is every value AiKey authors or annotates. That is what this does. +// +// # TWO CHOKEPOINTS +// +// 1. Headers — scrubMemberIdentityHeaders, called in serveRoute's Director +// right after stripAikeyRequestHeaders. The namespace stripper only covers +// X-Aikey-*; this covers identity VALUES under ANY header name, which is +// the gap a future `X-Member-Union-Id` would have walked straight through. +// 2. Body — injectMetadataUserIDIfAbsent, the single place the proxy writes an +// identifier into a request body. Guarded at the chokepoint, not at today's +// one call site, so a future caller inherits the fence. +// +// 🚫 If a shape here starts firing, do NOT relax the pattern — ask why the data +// plane is holding a member's provider identity. +var ( + // syntheticHandleRe matches the control plane's synthetic account handle, + // sso+.@... — produced by exactly one function + // (aikey-control-master pkg/identity.syntheticEmail). The digest is + // sha256(provider\x00union_id) truncated, so shipping the handle ships a + // stable, cross-application identifier for a named employee. + syntheticHandleRe = regexp.MustCompile(`(?i)sso\+[a-z0-9_.-]+\.[0-9a-f]{8,}@`) + + // ssoLocalDomainRe catches the handle's domain on its own, so a truncated, + // re-hashed or otherwise reformatted derivative still trips the fence. + ssoLocalDomainRe = regexp.MustCompile(`(?i)@sso\.local`) + + // feishuActorIDRe matches a Feishu union_id / open_id: on_ / ou_ followed by + // a long unbroken alphanumeric run (the real values are a 32-char digest). + // + // No \b anchor in front, deliberately: these arrive EMBEDDED, e.g. + // "…_account_on_8ed6aa…", and `_` is a word character so \b would never fire + // there — the anchored first cut of this regex silently matched nothing, + // which is why the body fence has a case pinned on exactly this string. + // The 24-char floor is what replaces the anchor as the false-positive + // defence: the persona user_id the proxy builds + // (user_<64hex>_account__session_) does contain the letters + // "on_" inside "_session_", but a dashed UUID gives it only 8 unbroken + // characters to work with. Opaque credentials are exempt entirely, below. + feishuActorIDRe = regexp.MustCompile(`(?i)(?:on|ou)_[0-9a-z]{24,}`) +) + +// memberIdentityShape returns the name of the member-identity shape found in s, +// or "" when s is clean. Name (not bool) so the WARN says WHICH shape fired — +// "identity leaked" with no discriminator is not actionable at 3am. +func memberIdentityShape(s string) string { + switch { + case syntheticHandleRe.MatchString(s): + return "synthetic_account_handle" + case ssoLocalDomainRe.MatchString(s): + return "sso_local_domain" + case feishuActorIDRe.MatchString(s): + return "feishu_actor_id" + } + return "" +} + +// identityScrubExemptHeaders are credential-bearing headers the shape scan skips. +// +// WHY an exemption exists at all: these carry opaque provider secrets — a Bearer +// token is a long random string, and a random string can accidentally satisfy +// feishuActorIDRe. Dropping Authorization on a false positive would fail EVERY +// request of that account (level-2 stability) to defend against a shape that +// cannot legitimately live in a credential anyway. These headers are set by the +// proxy itself from vault material; they are never an identity carrier. +var identityScrubExemptHeaders = map[string]struct{}{ + "Authorization": {}, + "Proxy-Authorization": {}, + "X-Api-Key": {}, + "Api-Key": {}, + "X-Goog-Api-Key": {}, +} + +// scrubMemberIdentityHeaders deletes outbound headers whose VALUE carries a +// member-identity shape, regardless of header name, and returns "=" +// for each removal so the caller can WARN (never the value itself — logging the +// leak to defend against the leak is not a fix). +// +// Complements stripAikeyRequestHeaders rather than replacing it: that one is a +// namespace rule (cheap, total, covers annotations that do not exist yet), this +// one is a value rule (covers header names that do not exist yet). Neither +// subsumes the other. +func scrubMemberIdentityHeaders(h http.Header) []string { + var removed []string + for name, values := range h { + if _, exempt := identityScrubExemptHeaders[http.CanonicalHeaderKey(name)]; exempt { + continue + } + for _, v := range values { + if shape := memberIdentityShape(v); shape != "" { + removed = append(removed, http.CanonicalHeaderKey(name)+"="+shape) + h.Del(name) + break + } + } + } + return removed +} diff --git a/internal/proxy/middleware.go b/internal/proxy/middleware.go index b19e2ab..707a402 100644 --- a/internal/proxy/middleware.go +++ b/internal/proxy/middleware.go @@ -459,7 +459,15 @@ const ( func stripAikeyRequestHeaders(h http.Header) { for k := range h { if len(k) >= 8 && strings.EqualFold(k[:8], "X-Aikey-") { - h.Del(k) + // delete(), not h.Del(): Del canonicalizes its argument before + // deleting, so for a key written straight into the map in + // non-canonical form ("x-aikey-…", as a verbatim copy from another + // hop can be) it deletes a DIFFERENT, absent key and the real entry + // survives the strip — the case-insensitive match above then reads + // as protection that isn't there. Found 2026-07-21 by the probe/ + // forward convergence fence. net/http canonicalizes inbound server + // headers, so this was not a live leak; it was a live blind spot. + delete(h, k) } } } diff --git a/internal/proxy/oauth_inject.go b/internal/proxy/oauth_inject.go index df4829d..7a57ce0 100644 --- a/internal/proxy/oauth_inject.go +++ b/internal/proxy/oauth_inject.go @@ -8,8 +8,11 @@ import ( "encoding/json" "fmt" "io" + "log/slog" "net/http" "strings" + + "github.com/AiKeyLabs/aikey-proxy/internal/observability" ) // oauthInject replaces the original API Key auth with OAuth credential injection. @@ -332,7 +335,27 @@ func setRequestBody(req *http.Request, bodyBytes []byte) { // injectMetadataUserIDIfAbsent reads the request body JSON and injects // metadata.user_id only if it's not already set. This preserves the CLI's // own user_id when proxying Claude Code requests (forward compatibility). +// +// Fence I13 body chokepoint (2026-07-21): this is the ONLY place the proxy +// writes an identifier into an outbound body, so the member-identity guard sits +// here rather than at today's single call site — a future caller ("attribute +// upstream usage per member") inherits the fence instead of re-opening the hole. +// Refusing the write is the safe failure: metadata.user_id is optional to the +// WAF's persona check in the sense that a MISSING one degrades to a fresh +// session, whereas a leaked one is unrecoverable — the third party has it. func injectMetadataUserIDIfAbsent(req *http.Request, userID string) { + if shape := memberIdentityShape(userID); shape != "" { + tc := traceFromContext(req.Context()) + slog.Warn("refused to write member identity into upstream body", + "event.name", observability.EventProxyRequestIdentityBlocked, + "identity_shape", shape, + "path", req.URL.Path, + "trace_id", tc.TraceID, + "span_id", tc.SpanID, + "request_id", tc.RequestID, + ) + return + } mutateJSONBody(req, func(body map[string]any) bool { // Skip if metadata.user_id already present. if metadata, ok := body["metadata"].(map[string]any); ok { diff --git a/internal/proxy/probe_raw.go b/internal/proxy/probe_raw.go index d3fe0f4..7a2b156 100644 --- a/internal/proxy/probe_raw.go +++ b/internal/proxy/probe_raw.go @@ -236,6 +236,18 @@ func (p *Proxy) handleProbeRaw(w http.ResponseWriter, r *http.Request, canonical // invariant per spec §4.3). stripAikeyHeaders(upstreamReq.Header) + // 4e. Fence I13 value rule (2026-07-21) — same guard the forward Director + // runs. Steps 4a/4d are NAME rules; neither can see a member-identity value + // parked under an allowlisted header name. Expected to be a no-op here + // (default-deny allowlist + fixed UA + proxy-set auth), so a hit is a real + // signal and is logged, not swallowed. + if scrubbed := scrubMemberIdentityHeaders(upstreamReq.Header); len(scrubbed) > 0 { + logger.Warn("member identity scrubbed from probe upstream request headers", + "event.name", observability.EventProxyRequestIdentityScrubbed, + "headers", scrubbed, + ) + } + // 5. Send + time. Use p.transport so HTTP_PROXY env etc are honored // the same way as the regular upstream path. transport := p.currentTransport() @@ -324,17 +336,17 @@ func copyAllowlistedHeaders(src, dst http.Header) { // but a second guard ensures even an allowlist regression doesn't leak // probe bearer to upstream. Cost: one map iteration on outbound headers // (typically < 10 entries). +// +// 2026-07-21: this used to be a second, independently written implementation of +// the same "X-Aikey-* never reaches an upstream" invariant that +// middleware.go's stripAikeyRequestHeaders enforces on the forward path — two +// bodies of code, one invariant, and fence I13 only pinned one of them. It now +// DELEGATES, so the invariant has a single source of truth and both call sites +// are covered by TestStripperConvergence_ProbeDelegatesToForwardPath. The layer +// order on this path is unchanged and still three-deep: outboundHeaderAllowlist +// (default-deny) → namespace strip (here) → member-identity value scrub. func stripAikeyHeaders(h http.Header) { - var toDelete []string - for name := range h { - canonical := http.CanonicalHeaderKey(name) - if strings.HasPrefix(canonical, "X-Aikey-") { - toDelete = append(toDelete, name) - } - } - for _, name := range toDelete { - h.Del(name) - } + stripAikeyRequestHeaders(h) } // injectProbeAuth sets the upstream Authorization (or x-api-key) header diff --git a/internal/proxy/sso_identity_no_leak_test.go b/internal/proxy/sso_identity_no_leak_test.go index d6ed177..26c9f24 100644 --- a/internal/proxy/sso_identity_no_leak_test.go +++ b/internal/proxy/sso_identity_no_leak_test.go @@ -1,19 +1,23 @@ package proxy import ( + "bytes" + "encoding/json" + "io" "net/http" + "net/http/httptest" "os" "path/filepath" "strings" "testing" ) -// 🔴 FENCE I13 — no member identity reaches the LLM upstream. +// 🔴 FENCE I13 — AiKey itself puts no member identity onto an upstream request. // // The 2026-07-20 first-login change gave every SSO member three new identifying -// values: the Feishu union_id, the tenant_key, and a synthetic account handle -// (sso+.@sso.local). All three live in the control plane. None -// has any business on the wire to Anthropic or OpenAI: +// values: the Feishu union_id / open_id, the tenant_key, and a synthetic +// account handle (sso+.@sso.local). All three live in the +// control plane. None has any business on the wire to Anthropic or OpenAI: // // - Anthropic's OAuth WAF reads unrecognized headers as "this is not a real // Claude Code session" and answers 429 with no X-RateLimit-Reset — a business @@ -21,30 +25,74 @@ import ( // - and quite apart from the WAF, shipping a customer's employee identifiers to // a third-party model provider is a data-collection surface nobody authorized. // -// The stripper works by namespace, so it covers annotations that do not exist -// yet — which is the point. A denylist of today's header names would not. +// # SCOPE — read this before adding a case +// +// The fence covers what AiKey AUTHORS or ANNOTATES, in three layers: +// +// L1 name rule stripAikeyRequestHeaders — the whole X-Aikey-* namespace, +// so annotations that do not exist yet are covered too. +// L2 value rule scrubMemberIdentityHeaders — identity SHAPES under any header +// name, so header names that do not exist yet are covered too. +// L3 body rule injectMetadataUserIDIfAbsent refuses to write an identity- +// shaped value into the outbound body. +// +// It does NOT cover a client that types its own union_id into a chat message. +// The main path forwards the client body byte-for-byte on purpose (WAF +// fingerprint is byte-exact; oauth_inject.go's closing note documents that pool +// requests pass real client identity through deliberately). See +// member_identity_guard.go for the full argument. The earlier absolute phrasing +// —"no member identity reaches the upstream"— promised more than any code here +// can keep, which is the same false-safety shape as a compliance filter that +// reports "enabled" while the detector package never loaded. + +// L1+L2. 🔴 MUTATION PROOF — verbatim runs, 2026-07-21. Re-run both before +// trusting this file. +// +// (a) delete the scrubMemberIdentityHeaders(h) call below: +// --- FAIL: TestUpstreamRequest_CarriesNoMemberIdentity (0.00s) +// sso_identity_no_leak_test.go:91: header "X-Member-Union-Id" carries "on_8ed6aa" to the upstream +// sso_identity_no_leak_test.go:91: header "X-Trace-Owner" carries "@sso.local" to the upstream +// sso_identity_no_leak_test.go:91: header "X-Trace-Owner" carries "sso+feishu" to the upstream +// Note what is ABSENT: not one "identity header … survived" line. The value +// loop failed with the prefix loop still green — which is the whole point. +// Before 2026-07-21 every poisoned value sat on an X-Aikey-* header, so the +// value loop could not fail unless the prefix loop already had; it was a +// dead assertion manufacturing the appearance of a second layer. +// +// (b) degrade stripAikeyRequestHeaders to a lone h.Del("X-Aikey-Union-Id"): +// --- FAIL: TestUpstreamRequest_CarriesNoMemberIdentity (0.00s) +// sso_identity_no_leak_test.go:87: identity header "X-Aikey-Seat-Id" survived and would reach the model provider +// sso_identity_no_leak_test.go:87: identity header "X-Aikey-Tenant-Key" survived and would reach the model provider +// sso_identity_no_leak_test.go:87: identity header "X-Aikey-Account-Id" survived and would reach the model provider +// sso_identity_no_leak_test.go:87: identity header "X-Aikey-Feishu-Token" survived and would reach the model provider func TestUpstreamRequest_CarriesNoMemberIdentity(t *testing.T) { h := http.Header{} - // Every identity-shaped annotation an internal caller might plausibly stash. - h.Set("X-Aikey-Union-Id", "on_stub_union_0001") + // Identity-shaped annotations inside the aikey namespace — L1 catches these. + h.Set("X-Aikey-Union-Id", "on_8ed6aa67826108097d9ee1438163457c") h.Set("X-Aikey-Tenant-Key", "stub_tenant_key") h.Set("X-Aikey-Account-Email", "sso+feishu.f11625f5ed9c39bf2c63d742cf65e3cc@sso.local") h.Set("x-aikey-account-id", "c4f1aec0-326d-4f20-a20a-90fcc100af00") h.Set("X-Aikey-Feishu-Token", "u-stub-user-access-token") h.Set("X-Aikey-Seat-Id", "3e527d39-d5a8-46ee-a7ac-7ca2072e65b6") + // Identity-shaped values OUTSIDE the aikey namespace. L1 is blind to these + // by construction — they are the "next feature adds one header" leak the + // review flagged, and only L2 stops them. + h.Set("X-Member-Union-Id", "on_8ed6aa67826108097d9ee1438163457c") + h.Set("X-Trace-Owner", "sso+feishu.f11625f5ed9c39bf2c63d742cf65e3cc@sso.local") // Headers the upstream legitimately needs — these must survive. h.Set("Authorization", "Bearer real-upstream-token") h.Set("Content-Type", "application/json") h.Set("anthropic-version", "2023-06-01") stripAikeyRequestHeaders(h) + scrubMemberIdentityHeaders(h) for name, values := range h { if strings.HasPrefix(strings.ToLower(name), "x-aikey-") { t.Errorf("identity header %q survived and would reach the model provider", name) } for _, v := range values { - for _, forbidden := range []string{"on_stub_union", "stub_tenant_key", "@sso.local", "sso+feishu"} { + for _, forbidden := range []string{"on_8ed6aa", "@sso.local", "sso+feishu"} { if strings.Contains(v, forbidden) { t.Errorf("header %q carries %q to the upstream", name, forbidden) } @@ -57,25 +105,323 @@ func TestUpstreamRequest_CarriesNoMemberIdentity(t *testing.T) { } } -// The second half of the same invariant, and the one a header test cannot give: -// the proxy must not KNOW these values at all. A stripper only removes what -// someone put on; this asserts nobody in the forwarding path has the vocabulary -// to put them on in the first place. +// L2 negative half. A guard that deletes everything also passes the test above, +// so pin what it must NOT touch — chiefly the credential headers it is +// deliberately exempt from scanning (a random Bearer can look like anything; +// dropping it would fail every request of that account). +func TestScrubMemberIdentityHeaders_LeavesLegitimateTrafficAlone(t *testing.T) { + h := http.Header{} + h.Set("Authorization", "Bearer sk-ant-oat01-on_abcdefghijklmnopqrstuvwxyz012345") + h.Set("X-Api-Key", "sk-on_abcdefghijklmnopqrstuvwxyz") + h.Set("User-Agent", "claude-cli/2.1.22 (external, cli)") + h.Set("X-Claude-Code-Session-Id", "9e0a3d21-1f1c-4a02-9c1b-33a1f0e9d100") + h.Set("Anthropic-Beta", "claude-code-20250219,oauth-2025-04-20") + + if removed := scrubMemberIdentityHeaders(h); len(removed) != 0 { + t.Fatalf("scrub removed legitimate headers %v (false positive breaks every request)", removed) + } + if len(h) != 5 { + t.Fatalf("header set mutated: %#v", h) + } +} + +// L3 body rule. 🔴 MUTATION PROOF — verbatim run, 2026-07-21. Remove the +// memberIdentityShape early-return from injectMetadataUserIDIfAbsent: +// +// --- FAIL: TestUpstreamBody_CarriesNoMemberIdentity (0.00s) +// --- FAIL: TestUpstreamBody_CarriesNoMemberIdentity/synthetic_handle (0.00s) +// sso_identity_no_leak_test.go:157: body reached the upstream carrying "sso+feishu.": {"messages":[],"metadata":{"user_id":"user_de9f_account_sso+feishu.f11625f5ed9c39bf2c63d742cf65e3cc@sso.local_session_c0ffee"},"model":"claude-sonnet-4-5"} +// --- FAIL: TestUpstreamBody_CarriesNoMemberIdentity/sso_local_domain (0.00s) +// sso_identity_no_leak_test.go:157: body reached the upstream carrying "@sso.local": … +// --- FAIL: TestUpstreamBody_CarriesNoMemberIdentity/feishu_union_id (0.00s) +// sso_identity_no_leak_test.go:157: body reached the upstream carrying "on_8ed6aa": … +// +// All three subcases go red, so no single shape is carrying the assertion. +// +// This is the leak the review named: the "attribute upstream usage per member" +// feature only has to compose the handle into the user_id, and both header +// fences stay green while a digest of a named employee's union_id lands at +// Anthropic. Body is the one place the proxy already writes an identifier, so +// it is the one place the fence has to sit. +func TestUpstreamBody_CarriesNoMemberIdentity(t *testing.T) { + cases := []struct { + name string + userID string + }{ + {"synthetic_handle", "user_de9f_account_sso+feishu.f11625f5ed9c39bf2c63d742cf65e3cc@sso.local_session_c0ffee"}, + {"sso_local_domain", "user_de9f_account_alice@sso.local_session_c0ffee"}, + {"feishu_union_id", "user_de9f_account_on_8ed6aa67826108097d9ee1438163457c_session_c0ffee"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/v1/messages", + bytes.NewReader([]byte(`{"model":"claude-sonnet-4-5","messages":[]}`))) + injectMetadataUserIDIfAbsent(req, c.userID) + + got, err := io.ReadAll(req.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + for _, forbidden := range []string{"sso+feishu.", "@sso.local", "on_8ed6aa"} { + if bytes.Contains(got, []byte(forbidden)) { + t.Errorf("body reached the upstream carrying %q: %s", forbidden, got) + } + } + var parsed map[string]any + if err := json.Unmarshal(got, &parsed); err != nil { + t.Fatalf("guard corrupted the body: %v (%s)", err, got) + } + if _, present := parsed["metadata"]; present { + t.Errorf("guard refused the value but still wrote metadata: %s", got) + } + if parsed["model"] != "claude-sonnet-4-5" { + t.Errorf("guard damaged unrelated body fields: %s", got) + } + }) + } +} + +// L3 negative half: the ordinary OAuth user_id (device sha256 + account UUID + +// session UUID) must still be injected. A guard that blocks everything would +// silently drop the WAF persona field and hand every OAuth user a 429 — the +// exact "business rejection disguised as rate limit" this whole path exists to +// avoid. `_session_` contains the letters "on_"; the \b in feishuActorIDRe is +// what keeps it from matching, so this case is also that regex's regression pin. +func TestInjectMetadataUserID_StillInjectsOrdinaryOAuthIdentity(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/v1/messages", + bytes.NewReader([]byte(`{"model":"claude-sonnet-4-5"}`))) + const want = "user_9f2c4a1b7d3e5f60a1b2c3d4e5f60718293a4b5c6d7e8f90a1b2c3d4e5f60718" + + "_account_c4f1aec0-326d-4f20-a20a-90fcc100af00" + + "_session_3e527d39-d5a8-46ee-a7ac-7ca2072e65b6" + injectMetadataUserIDIfAbsent(req, want) + + got, err := io.ReadAll(req.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + var parsed struct { + Metadata struct { + UserID string `json:"user_id"` + } `json:"metadata"` + } + if err := json.Unmarshal(got, &parsed); err != nil { + t.Fatalf("unmarshal: %v (%s)", err, got) + } + if parsed.Metadata.UserID != want { + t.Errorf("ordinary OAuth user_id was not injected: got %q, body %s", parsed.Metadata.UserID, got) + } +} + +// One invariant, one implementation. probe_raw.go used to carry a second, +// independently written X-Aikey-* stripper; fence I13 pinned only the forward +// one, so "unifying" the probe path by deleting its allowlist would not have +// moved a single test. It now delegates — this pins the delegation, because a +// re-fork would restore the split source of truth silently. +// +// 🔴 MUTATION PROOF — verbatim run, 2026-07-21. Re-fork stripAikeyHeaders as a +// raw-key prefix match (`strings.HasPrefix(name, "X-Aikey-")`, the shape a +// "just inline it" rewrite naturally takes): +// +// --- FAIL: TestStripperConvergence_ProbeDelegatesToForwardPath (0.00s) +// sso_identity_no_leak_test.go:231: strippers diverged: forward=http.Header{"Anthropic-Version":…, "Authorization":…} probe=http.Header{"Anthropic-Version":…, "Authorization":…, "x-aikey-raw-map-key":[]string{"leaked"}} +// +// 💡 This test paid for itself while being written: its first run went red on +// UNMUTATED code, because stripAikeyRequestHeaders used h.Del(k), and Del +// canonicalizes before deleting — so a non-canonical map key matched the +// case-insensitive test and then survived the delete. Fixed in middleware.go +// (delete(h, k)); see the comment there. +func TestStripperConvergence_ProbeDelegatesToForwardPath(t *testing.T) { + seed := func() http.Header { + h := http.Header{} + h.Set("X-Aikey-Union-Id", "on_8ed6aa67826108097d9ee1438163457c") + h.Set("x-aikey-probe-bearer", "sk-secret") + h.Set("X-AIKEY-Seat-Id", "3e527d39") + // Written straight into the map, bypassing Set's canonicalization — the + // shape a header copied verbatim from another hop can take. The forward + // stripper is case-insensitive so it catches this; a re-fork that + // prefix-matches the raw key would not, which is the divergence this + // test exists to catch. + h["x-aikey-raw-map-key"] = []string{"leaked"} + h.Set("Authorization", "Bearer keep-me") + h.Set("Anthropic-Version", "2023-06-01") + return h + } + viaForward, viaProbe := seed(), seed() + stripAikeyRequestHeaders(viaForward) + stripAikeyHeaders(viaProbe) + + if len(viaForward) != len(viaProbe) { + t.Fatalf("strippers diverged: forward=%#v probe=%#v", viaForward, viaProbe) + } + for name, values := range viaForward { + if got := viaProbe[http.CanonicalHeaderKey(name)]; len(got) != len(values) || (len(got) > 0 && got[0] != values[0]) { + t.Errorf("strippers diverged on %q: forward=%v probe=%v", name, values, got) + } + } + if viaProbe.Get("Authorization") != "Bearer keep-me" { + t.Errorf("probe stripper took the upstream credential: %#v", viaProbe) + } +} + +// The vocabulary fence: does the data plane even have the WORDS for a member's +// provider identity? +// +// 🔴 WHAT THIS IS NOT. It is a proxy metric, not a terminal observation +// (SKILL §11). Identity travels as DATA, not as field names — control-plane +// member identity arriving inside GroupRuntimeAccount.Identity (a generically +// named field whose own comment says "email / alias") would not cost this scan +// a single hit. The fences that actually守住 the wire are L1/L2/L3 above; this +// one only catches the cheapest and most common first move, a literal +// `union_id` json tag appearing in the data plane. It is kept, not deleted, +// because that first move IS how this would start and the scan costs ~40ms — +// but it is labelled so nobody mistakes green here for "no identity leaves". +// +// Two defects fixed 2026-07-21: +// - the walk root was "." = the internal/proxy package only, leaving +// internal/vkeys (the actual carrier of runtime material), internal/events +// (usage reporting), internal/supervisor and app/ — precisely where +// control-plane data enters the data plane — outside the scan. +// - no anti-vacuity assertion: a walk that reached zero files also passed. // // 🚫 If this fails, do not add a header to the stripper — ask why the data plane // is holding a member's provider identity. func TestProxySource_DoesNotReferenceMemberIdentityFields(t *testing.T) { forbidden := []string{"union_id", "tenant_key", "sso.local"} + + // Repo root: internal/proxy → ../.. + offenders, scanned, err := scanGoSourcesFor(filepath.Join("..", ".."), forbidden) + // Exactly one exemption: the guard has to name what it hunts. Same shape as + // the repo's two-arg MAX() fence, whose rule is "the grep may only hit the + // helper itself". Keep it to one file — every added exemption is a hole. + offenders = dropExempt(offenders, filepath.Join("internal", "proxy", "member_identity_guard.go")) + if err != nil { + t.Fatalf("walk proxy sources: %v", err) + } + + // Anti-vacuity 1: the scan must have actually read the data plane. 150 is a + // deliberate floor well under the ~300 non-test .go files present on + // 2026-07-21 — it catches "walk silently reached nothing", not file-count + // drift. + if scanned < 150 { + t.Fatalf("scan covered only %d files — the walk root is wrong and this fence is vacuous", scanned) + } + // Anti-vacuity 2: name the packages that must be in range, so narrowing the + // root back to one package fails loudly instead of passing quietly. + for _, pkg := range []string{ + filepath.Join("internal", "vkeys"), + filepath.Join("internal", "events"), + filepath.Join("internal", "supervisor"), + filepath.Join("internal", "proxy"), + "app", + } { + found := false + for _, p := range scannedPaths { + if strings.Contains(p, pkg) { + found = true + break + } + } + if !found { + t.Errorf("package %s was not scanned — control-plane data enters the data plane there", pkg) + } + } + + if len(offenders) > 0 { + t.Errorf("the data plane references control-plane member identity:\n %s", strings.Join(offenders, "\n ")) + } +} + +// Anti-vacuity 3: prove the scanner can fire at all. Without this the fence's +// green is indistinguishable from a scanner that matches nothing ever — and on +// 2026-07-21 the three terms had zero occurrences repo-wide, so the fence had +// been idling since the day it merged with no evidence it could ever go red. +// +// 🔴 MUTATION PROOF — verbatim runs, 2026-07-21, with `// union_id` appended to +// internal/vkeys/group_runtime.go: +// +// (a) current walk root ("../.." = repo root): +// --- FAIL: TestProxySource_DoesNotReferenceMemberIdentityFields (0.01s) +// sso_identity_no_leak_test.go:307: the data plane references control-plane member identity: +// ../../internal/vkeys/group_runtime.go contains union_id +// +// (b) same plant, walk root reverted to the pre-fix "." : +// --- FAIL: TestProxySource_DoesNotReferenceMemberIdentityFields (0.00s) +// sso_identity_no_leak_test.go:283: scan covered only 47 files — the walk root is wrong and this fence is vacuous +// +// Read (b) carefully: the planted union_id is NOT in its offender list. The old +// fence walked 47 files of internal/proxy and was structurally blind to the +// package that actually carries runtime material. The scan-root defect was +// real, not theoretical — and only the anti-vacuity floor makes it visible. +func TestSourceScanner_ActuallyFires(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "carrier.go"), + []byte("package x\n\ntype A struct{ U string `json:\"union_id\"` }\n"), 0o600); err != nil { + t.Fatalf("write fixture: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "carrier_test.go"), + []byte("package x // union_id in a test file must be ignored\n"), 0o600); err != nil { + t.Fatalf("write fixture: %v", err) + } + + offenders, scanned, err := scanGoSourcesFor(dir, []string{"union_id", "tenant_key", "sso.local"}) + if err != nil { + t.Fatalf("scan: %v", err) + } + if scanned != 1 { + t.Errorf("expected 1 non-test file scanned, got %d", scanned) + } + if len(offenders) != 1 || !strings.Contains(offenders[0], "union_id") { + t.Fatalf("scanner did not fire on a planted hit: %v", offenders) + } +} + +// dropExempt removes offender lines belonging to the named file. Separate +// helper (not a skip inside the walk) so the exemption is visible at the call +// site and cannot be widened by accident. +func dropExempt(offenders []string, exempt string) []string { + kept := offenders[:0] + for _, o := range offenders { + if strings.Contains(o, exempt) { + continue + } + kept = append(kept, o) + } + return kept +} + +// scannedPaths records the last scan's file list for the coverage assertions. +// Package-level rather than a return value only because both consumers are in +// this file and a 4th return value would read worse; tests here run in one +// process and do not run in parallel. +var scannedPaths []string + +// scanGoSourcesFor walks root for non-test .go files containing any forbidden +// term. Returns offenders, the count of files actually read, and any walk error. +func scanGoSourcesFor(root string, forbidden []string) ([]string, int, error) { var offenders []string + var scanned int + scannedPaths = nil - err := filepath.Walk(".", func(path string, info os.FileInfo, err error) error { - if err != nil || info.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil { return err } - body, readErr := os.ReadFile(path) + if info.IsDir() { + switch info.Name() { + case ".git", "node_modules", "vendor", "bin", "testdata": + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + body, readErr := os.ReadFile(path) //nolint:gosec // path comes from the repo walk if readErr != nil { return readErr } + scanned++ + scannedPaths = append(scannedPaths, path) for _, term := range forbidden { if strings.Contains(string(body), term) { offenders = append(offenders, path+" contains "+term) @@ -83,10 +429,5 @@ func TestProxySource_DoesNotReferenceMemberIdentityFields(t *testing.T) { } return nil }) - if err != nil { - t.Fatalf("walk proxy sources: %v", err) - } - if len(offenders) > 0 { - t.Errorf("the data plane references control-plane member identity:\n %s", strings.Join(offenders, "\n ")) - } + return offenders, scanned, err }