fix(proxy): keep static credentials a resolver did not answer for

[andybons]

The problem

A host may carry both a credential resolver and a static credential on different headers, each serving a different client. On api.anthropic.com a token exchange resolves Claude Code's subscription onto Authorization while an application's API key sits on x-api-key.

getCredentialsForRequest returned the resolver's credentials alone whenever it resolved anything, so the static credential never reached injectCredentials. The application's placeholder went upstream unreplaced, and Anthropic rejected it as invalid x-api-key while the proxy's own log recorded a successful injection — of the other header:

"http_host":"api.anthropic.com","http_status":401,
"credential_injected":true,"injected_headers":"authorization","grants":"claude-code-user"

The change

When a resolver returns credentials, static credentials for the host are merged in — but only those the client asked for, meaning the client sent the credential's header and the resolver left it in place. injectCredentials then selects per header, which is what it already documents.

Each part of that condition earns its keep, and the review shaped all three (thanks @claude):

• Different headers only. A static sharing the resolver's header stays dropped, so api.github.com keeps its behaviour — the token exchange and the GitHub App key both target Authorization, and the exchange must keep winning it.
• Client-sent only. An unconditional merge fed the statics into injectCredentials' auto-inject fallback: a request carrying neither header would collect every distinct-header credential, attaching the resolver's per-user token to a request that never asked for it.
• Resolver-left only. A resolver may mutate the request, and the two directions break a single-point check in opposite ways — one that strips a header leaves injectCredentials no client header to select on (auto-inject attaches both), while one that sets a header makes a static look requested (attached unasked). Requiring the header both before and after the resolver is safe under either mutation.

The eviction path is unchanged: a rejected request that carried several placeholders still evicts every injected credential, since upstream does not say which one it rejected — now recorded as a deliberate tradeoff beside the existing scoping comment.

Verification

End to end against the running binary — a fake upstream echoing the auth headers it received, a fake STS for the token exchange, patched and unpatched instances on the same config:

Client sends	Unpatched	Patched
x-api-key: <placeholder>	authorization: Bearer SUBSCRIPTION-BEARER x-api-key: <placeholder>	x-api-key: real-api-key only
Authorization: <placeholder>	authorization: Bearer SUBSCRIPTION-BEARER	identical
neither	authorization: Bearer SUBSCRIPTION-BEARER	identical

The first unpatched row reproduces the production failure exactly.

Tests in proxy/resolver_static_merge_test.go, each red-verified against the code it guards:

• TestProxy_ResolverAndStaticOnDifferentHeaders — fails on unpatched code with x-api-key = "placeholder".
• TestProxy_ResolverStillWinsItsOwnHeader — passes on unpatched code, deliberately: it guards against this fix going too far.
• TestProxy_NoPlaceholderDoesNotFanOutCredentials — fails against the unconditional merge.
• TestProxy_ResolverMutationDoesNotChangeWhatWasAskedFor — strip and set subtests; the set case fails against the post-resolve-only check.

go test ./... passes across all packages.

Noted while working, not addressed here

isValidHost rejects any credential host containing :, and config load never checks it — so host: api.example.com:8443 loads clean, deploys, and injects nothing, with only a Debug log. Happy to send a config-load guard as a separate change.

[claude]

📦 Previous Review (superseded by newer review)

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

---

Code review — PR #64

• Gather context (read changed files, diff against origin/main)
• Understand the request (code review via /code-review skill)
• Run code review
• Post findings

Summary

The fix itself is correct and well-targeted: withUnresolvedStatics (proxy/proxy.go:1610-1632) merges in only the static credentials whose header the resolver didn't answer, so api.github.com's same-header tie-break (token exchange must keep beating the GitHub App key) is preserved while api.anthropic.com's different-header case (subscription Authorization + app x-api-key) now works. Traced all three callers of getCredentialsForRequest, injectCredentials's tie-break logic, invalidateCredentialsOnAuthFailure, and getCredentialsMatch's locking/copy semantics — no aliasing, race, or nil-handling bugs. The copy-on-first-append in withUnresolvedStatics correctly avoids mutating the resolver's slice in place. Tests are solid: TestProxy_ResolverAndStaticOnDifferentHeaders and TestProxy_ResolverStillWinsItsOwnHeader are genuine end-to-end httptest cases that isolate exactly the two behaviors the PR claims to change vs. preserve.

Three inline comments posted:

1. Stale docstring (proxy/proxy.go:1537-1539, flagged at line 1610) — getCredentialsForRequest's doc comment still describes the old exclusive-win behavior ("the resolver's credentials win when it returns any"). Worth a one-line update pointing at withUnresolvedStatics so a reader doesn't rely on the stale description.
2. Worth confirming intent (line 1593) — merging static credentials into the resolver's result also feeds injectCredentials's auto-inject fallback, which fires when a client sends neither placeholder header. Before this PR, a successful resolver could only auto-inject its own single credential; now it can auto-inject alongside an unrelated static credential too (e.g. a per-user subscription Authorization token getting attached to a request that only ever intended to use the static x-api-key, and never asked for Authorization at all). The underlying "auto-inject every distinct header" mechanism is pre-existing and tested for static+static hosts, but this PR extends its reach to dynamically-resolved, typically higher-sensitivity per-user credentials. Neither new test exercises the "client sends neither placeholder" case — might be worth pinning down whether that's desired.
3. Minor/non-blocking (line 1594) — the successful-resolve path now does a second p.getCredentials(host) lock+lookup per request (the first already happens unconditionally via the if staticCreds, staticMatch := p.getCredentialsMatch(host); ... init clause at line 1558). Looks like a deliberate trade-off for freshness (the existing comment about token refresh landing while the resolver was out), not an oversight — just flagging it.

Nothing here blocks merging; #1 is a quick fix, #2 is worth a maintainer decision (and possibly a test either way), #3 is optional.

[claude]

📦 Previous Review (superseded by newer review)

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

---

Code review: PR #64

• Gather context (PR body, prior review comments, diff vs origin/main, latest commit 61e4806)
• Understand the request (re-review PR fix(proxy): keep static credentials a resolver did not answer for #64 after 61e4806 addressed prior feedback)
• Run /code-review skill against current diff
• Post findings / summary

Summary

61e4806 addresses the two functional concerns from the earlier review round:

• Stale docstring — getCredentialsForRequest's comment now describes the post-fix behavior and points at the merge helper (renamed withRequestedStatics).
• Auto-inject fan-out — the merge is now scoped to headers the client actually sent (req.Header.Get(c.Name) == "" gate), so a request carrying neither placeholder still sees only the resolver's credential, matching pre-PR behavior. TestProxy_NoPlaceholderDoesNotFanOutCredentials pins this down and fails without the fix.

The third comment (double p.getCredentials(host) lookup on the resolved path) was flagged as non-blocking/likely deliberate and wasn't addressed — reasonable to leave as-is given it avoids reintroducing the staleness the surrounding comment warns about.

Two new findings from this pass, posted inline:

1. proxy/proxy.go:1595 — Merging a resolver credential and a static credential into the same request's Injected list means a single 401/403 now invalidates both via invalidateCredentialsOnAuthFailure, even when only one of them (say, a revoked static x-api-key) actually caused it. That evicts a perfectly healthy resolver-cached token (e.g. a token-exchange result), forcing an unnecessary re-resolve. This co-occurrence was impossible before this PR.
2. proxy/proxy.go:1623 — withRequestedStatics samples req.Header after entry.resolve has already run (proxy.go:1588), not before. A resolver with side effects (the token-exchange resolver deletes SubjectHeader — gatekeeper_tokenexchange.go:62-64) can cause the "did the client ask for this header" check to see a header as absent that the client actually sent, if a static credential happens to share that header name. injectCredentials avoids this class of bug by sampling client-sent headers before any mutation; this new helper doesn't follow that pattern. Likely a narrow edge case (subject headers are usually internal-only names), but worth a conscious call.

Neither finding is a security leak or a regression on the PR's stated goal — both are about the blast radius/precision of side effects introduced by allowing resolver and static credentials to coexist in one request. Core logic (placeholder selection, header-scoped merge, api.github.com's same-header exclusivity) checked out correctly against the new tests, and go test ./... context matches the PR's own verification table.

[claude]

📦 Previous Review (superseded by newer review)

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

---

Code review — PR #64 (re-review after push)

• Gather context (diff since last review, new commits)
• Check status of previously open findings
• Run code review over the current diff
• Post findings

Status of previously open threads

• Stale docstring and auto-inject fan-out — fixed in 61e4806 / documented, confirmed correct.
• Double getCredentials lookup — confirmed deliberate (freshness vs. staleness trade-off), left as is.
• Header-sampling timing (withRequestedStatics checks req.Header after the resolver runs) — commit 13e7e51 didn't change the code, but added a docstring (proxy/proxy.go:1610-1618) explaining the trade-off: reading before resolve would route a resolver-stripped header into the auto-inject fallback and hand out both credentials, which is worse than dropping the static one. Reasonable as documented, but see the new inline comment below — the same read-after-resolve ordering has a second, more dangerous failure mode this docstring doesn't cover.
• Cache-invalidation blast radius (invalidateCredentialsOnAuthFailure can now evict a resolver's cached credential off the back of a static credential's 401, since both can land in credResult.Injected together) — no code change and no reply in the thread as of this review. Still open; a maintainer call, not necessarily a blocker.

New findings from this pass

1. withRequestedStatics can be fooled by a resolver that sets a header, not just strips one (proxy/proxy.go:1630, inline comment posted). CredentialResolver is an exported, arbitrary function given innerReq to mutate before the "did the client ask for this" check runs. The shipped token-exchange resolver only deletes a header (the case the new docstring reasons about), but nothing stops a different resolver implementation from setting one, which would make withRequestedStatics treat an unsent header as client-requested and let a static credential ride the auto-inject fallback onto a request that asked for nothing — the exact fan-out this PR set out to prevent, reopened via resolver side effects instead of the merge logic. Requires a custom resolver, so it's a contract gap rather than an active bug with the code that ships today.

2. proxy/relay.go:128 — this path calls getCredentialsForRequest(r, r, host), so withRequestedStatics checks r.Header, which still has Proxy-Authorization/Proxy-Connection on it. Those two headers are filtered out only when copied into proxyReq a few lines later (relay.go:139), and injectCredentials (relay.go:147) samples its own "client asked for this" set from proxyReq. A static credential configured on a relay host under one of those two header names would always look "requested" (every client authenticating to gatekeeper sends Proxy-Authorization) and could reach the auto-inject fallback despite the actual outgoing request never carrying that header. The other two call sites (proxy.go:2274/2320, proxy.go:3097) don't have this gap — they delete Proxy-Authorization/Proxy-Connection from the outgoing request after injectCredentials runs, so the check and the injection agree. Couldn't leave this inline since relay.go isn't touched by the diff. Narrow — needs that specific header-name choice in config — but worth a fix (check proxyReq.Header here, or move the filtering earlier) or a documented restriction against naming a credential header Proxy-Authorization/Proxy-Connection.

Nothing here blocks merging on its own; both are edge cases in an extension point (custom resolvers) and an underused path (named relay), not the primary CONNECT/plain-HTTP flow this PR is fixing. The fix itself — withRequestedStatics scoped to headers the client sent, verified end-to-end with TestProxy_ResolverAndStaticOnDifferentHeaders, TestProxy_ResolverStillWinsItsOwnHeader, and TestProxy_NoPlaceholderDoesNotFanOutCredentials — is sound.

[claude]

📦 Previous Review (superseded by newer review)

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

---

Code review — PR #64 (re-review after push)

• Gather context (diff since last review: 13e7e51, a34b7ca)
• Check status of previously open findings (header-sampling timing, cache-invalidation blast radius)
• Run code review over the current diff
• Post findings

Summary

Both findings still open from the last round were resolved by documenting them as deliberate, measured tradeoffs rather than by changing behavior — and in both cases the reasoning holds up:

• Header-sampling timing (withRequestedStatics, proxy.go:1619) — 13e7e51 adds a comment explaining that reading req.Header after the resolver runs is intentional: sampling beforehand would leave a stripped subject header invisible to injectCredentials, sending it down the auto-inject path and fanning the resolver's per-user token out to a request that asked for neither credential. Andy's PR comment reports measuring both orderings against the exact collision (a resolver stripping x-api-key as its subject header, plus a static credential on x-api-key) and finding the current order strictly better. Accepted — the tradeoff is real and the alternative is worse, not just different.
• Cache-invalidation blast radius (invalidateCredentialsOnAuthFailure, proxy.go:1644) — a34b7ca documents that a resolver-backed credential can now be evicted alongside a static one on a shared-request 401/403, and that this is intentional: upstream doesn't identify which credential it rejected, so narrowing the eviction risks leaving a genuinely revoked credential cached indefinitely, against a cost of one avoidable resolve. Reasonable call given the asymmetry of the failure modes.

Both are doc-only changes (proxy/proxy.go +7/+6 lines across the two commits) — no functional code changed since the last review pass. Traced both call sites (invalidateCredentialsOnAuthFailure at proxy.go:2397 and :2855, withRequestedStatics's single call site at proxy.go:1594) to confirm the new comments accurately describe what the code actually does.

One cosmetic nit, not blocking:

proxy/proxy.go:1658 — a34b7ca's inserted paragraph splits what was previously one flowing comment block, leaving an orphaned line-wrap fragment:

// than one extra resolve.
//
// A 401 or 403 is the
// only signal gatekeeper gets that a credential resolved from a cache has gone
// stale — ...

// A 401 or 403 is the reads as an accidental line break rather than a deliberate one-line sentence. Purely cosmetic (doc comment, no behavior change) — worth a rewrap if touching this comment again, not worth a commit on its own.
Fix this →

No new functional issues found. go build/go test could not be run in this sandbox (Bash execution requires interactive approval not available here) — the diff was verified by static reading against both call sites and the existing test suite (resolver_static_merge_test.go), which already covers the three behavioral cases (different-header merge, same-header exclusivity, no-placeholder fan-out guard) discussed across this review.

This PR looks ready to merge from a correctness/security standpoint.

[claude]

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

---

Re-reviewing PR #64 after latest push

• Gather context (diff vs origin/main, prior review threads, latest commits)
• Understand the request (PR synchronize trigger → run /code-review skill)
• Run code review
• Post findings

Summary

Commit 89de6ee closes the sampling-order gap I flagged in the previous round (req.Header.Get(c.Name) checked only after entry.resolve ran, so a resolver mutating a header in either direction could fool withRequestedStatics).

The fix: sentBefore (proxy/proxy.go:1588-1595) snapshots which headers the client sent before entry.resolve runs, and withRequestedStatics (proxy/proxy.go:1630-1652) now requires both sentBefore[header] and a non-empty req.Header.Get(c.Name) after resolve — i.e., a header only counts as "asked for" if the client sent it and the resolver left it alone. This correctly handles both mutation directions:

• resolver strips a header the client sent (e.g. consumes it as a subject header) → post-check fails → static credential dropped, resolver's own credential still auto-injects. Correct: the client's header was consumed as input to the resolver, not a request for the static credential.
• resolver sets a header the client never sent → pre-check (sentBefore) fails → not treated as client-requested, so it can't fan out a resolver-backed token onto an unrelated request.

I traced this against all three production call sites (proxy.go:2290, proxy.go:3113, relay.go:128) — all pass the same request object as both ctxReq/innerReq (or the CONNECT/inner pair), so the snapshot is always taken against the object the resolver actually mutates. TestProxy_ResolverMutationDoesNotChangeWhatWasAskedFor exercises both directions with a real strip/set side effect in the resolver and end-to-end assertions on what reaches the backend — good regression coverage for exactly this class of bug.

The two remaining threads (auto-inject fan-out scoping, and eviction blast radius on invalidateCredentialsOnAuthFailure) were addressed in 61e4806 and documented as a deliberate tradeoff in a34b7ca respectively — both reasoned through in the thread replies and consistent with the code as it stands now.

No new issues found in this pass. go build/go vet/go test could not be run in this sandbox (Bash tool calls to go build, go vet, go test all required interactive approval that wasn't available here) — recommend confirming CI is green before merge, though the logic traced cleanly by inspection and the PR's own verification table plus the new mutation test cover the scenarios that mattered.