fix(proxy): narrow the no-leak promise to what code can keep, then actually fence it#10
Merged
damonleelcx merged 2 commits intoJul 21, 2026
Conversation
…tually fence it
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"] = <handle>, 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 <noreply@anthropic.com>
…shu-first-login-no-email-verify
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
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.
Re-opens the proxy half. #9 was merged before the 8-role review landed, so these fixes are not in
develop-v1.0.4.The review's premise was wrong, and that matters
The fence was not test-only:
stripAikeyRequestHeadersruns unconditionally in the single forward Director and every upstream path converges onserveRoute. That part is kept intact. What was actually wrong is the scope of the claim and the quality of the assertions.The claim was unenforceable
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 writesmetadata.user_idinto bodies (oauth_inject.go).So the next feature wanting per-member upstream attribution could set
metadata["user_id"] = <handle>, or add a header without theaikeyprefix, 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:injectMetadataUserIDIfAbsent— the proxy's only body-identifier write — so a future caller inherits the guard rather than reopening the hole.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
stripAikeyRequestHeadersusedh.Del(k), andDelcanonicalises the key first — so for a non-canonical key written straight into the map (x-aikey-…), the case-insensitive match hit butDelremoved a different, non-existent key and the entry survived. Nowdelete(h, k).Not a live leak, because
net/httpcanonicalises inbound headers — a live blind spot.Dead assertion
The value loop only scanned values it had itself just placed under
X-Aikey-*, so it could never fail independently of the prefix assertion. It now poisonsX-Member-Union-IdandX-Trace-Owner, outside that namespace.The static text scan: kept, but demoted
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 literalunion_idjson tag). Fixed: walk root is the repo root instead of one package, an anti-vacuity floor (scanned < 150is fatal), and a mandatory package list includinginternal/vkeys— the actual carrier of runtime material and precisely what the old scan was structurally blind to.probe_raw'sstripAikeyHeadersnow delegates tostripAikeyRequestHeadersinstead of keeping a second implementation of the same invariant, pinned byTestStripperConvergence_ProbeDelegatesToForwardPath.Schema
None. This repo has no DB schema; no migration, no new API field.
Verification
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 ininternal/vkeysdoes not appear in the offender list at all.Not verified
🚫 None 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.
🤖 Generated with Claude Code