Bind stored tokens to their minting server; make whoami tell the truth - #387
Open
realtonyyoung wants to merge 6 commits into
Open
Bind stored tokens to their minting server; make whoami tell the truth#387realtonyyoung wants to merge 6 commits into
realtonyyoung wants to merge 6 commits into
Conversation
A stored token carried no record of which server issued it, so a multi-profile config (or a profile whose server_url was re-pointed after login) would happily send one server's token to another. The server rejects it with a bare 401, and /auth/refresh cannot heal it — it validates the token's own signature — so the CLI reported "not authenticated" for a token that was perfectly valid somewhere else. `kcap whoami` made this much harder to diagnose than it should have been: it only ever printed local file metadata, so it cheerfully called such a token valid while every request was being rejected. - Tokens now record the canonical URL of the server that minted them, compared via a single ServerIdentity helper (origin + case-sensitive path; userinfo, query and fragment are rejected rather than silently dropped, so two path-routed tenants can't collide). Pre-upgrade tokens have no binding and stay unenforced until a login or an unambiguous refresh stamps them. - One accessor, GetValidTokensForServerAsync, is now the only way a bearer reaches a request. It checks the binding on the raw snapshot BEFORE any refresh, so a token we've already decided not to use never spends a rotating credential, and it returns the issuing URL alongside the verdict so callers can name both servers without re-reading storage. Every attachment site moved onto it: the HTTP client factory, daemon SignalR and event POSTs, the watcher, attachment downloads, and the setup ping. - Token lookup follows the profile the process actually resolved rather than always the on-disk active profile. `kcap setup` is the exception that proves the rule: it deliberately configures the active profile, so it now performs every token operation against that profile explicitly. The legacy tokens.json fallback is scoped to its migration owner, since an unscoped fallback would hand that unbound credential to any repo-resolved profile lacking a token. - whoami prints the resolving profile and asks the server whether it accepts the token. Only 401/403 are treated as a verdict; an unreachable server, a missing endpoint on an older server, a redirect or a 5xx all report "could not verify" and keep exit 0, so nobody is sent to re-run `kcap login` over an outage. - Interactive clients recover from a 401 by refreshing once and resending. The refresh is conditional on the persisted token still being the rejected one, so a peer process that already refreshed is adopted instead of rotated again — which for WorkOS, whose refresh token is single-use, would invalidate the peer's session. Exactly one component owns 401-retry per client: the MCP servers keep their existing loops and opt out of the handler. Closes #386 AI-1504
PR Summary by QodoBind tokens to issuing servers and verify whoami remotely
AI Description
Diagram
High-Level Assessment
Files changed (33)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Swapping the MCP servers' 401 recovery from "re-read the store" to "force a refresh" quietly removed a case the original recovery covered: a token the server rejects while the refresh endpoint is unreachable or refuses. The old path resent whatever was stored and succeeded against a server that had only transiently rejected; the new one gave up as soon as the rotation failed, so a long-lived MCP session surfaced "Not logged in" where it previously recovered. Keep the rotation attempt — it is what heals a genuinely stale token — but treat its failure as no worse than not having tried, falling back to the stored token for the single retry. Caught by the integration suite on CI (Refreshed_token_succeeds_after_401, in both the flows and sessions MCP servers), which I had not run locally.
Eight findings, all real. The theme: checking a binding once, in one place, is not the same as a token never reaching the wrong server. - The accessor vetted its first snapshot, then re-read storage to load/refresh. A concurrent login or profile repoint between those reads produced a token for a different server carrying an Ok verdict. Re-check the token that is actually returned. - Forced refresh may hand back a token a PEER persisted rather than one we rotated, and that peer could have logged into a different server. It now takes the target and refuses an adopted token that doesn't belong to it. - The hook, spool and daemon drain paths classified only Expired and NotAuthenticated as an auth lapse, so a WrongServer client posted anyway; the resulting 401 read as permanent and the spool DROPPED queued lifecycle and transcript data over a fixable profile mismatch. - Setup's GitHub discovery activates the tenant you pick, which makes the profile captured before it ran stale — the freshly minted token was being saved under the previous profile while setup went on to configure the new one. Re-read the active profile after discovery. - The setup completion ping loaded a profile's token directly, so re-pointing an authenticated profile at another server sent the old server's bearer to the new one. Checked inline, keeping the ping refresh-free and time-bound. - The MCP retry helpers' no-token fallback attached whatever was stored without checking it against that server's fixed base URL. They now take the base URL and resolve through the server-aware accessor. - The retry handler only replayed ByteArrayContent, silently skipping recovery for the two MCP servers that post JsonContent and own no retry loop of their own. JsonContent re-serializes per send, so it is replayable. - whoami claimed to work from one raw, non-refreshing snapshot but went through the refresh-aware accessor, so diagnosing your auth could rotate a single-use WorkOS refresh token and print one token's expiry while probing another's.
… call Two more from review. A failed forced refresh fell through to the refresh-aware accessor, which saw the same expired token and refreshed again — two provider calls for one rejection, and for WorkOS that re-spends a refresh token the provider only honours once. Recovery is now a single operation: attempt the rotation, and on failure fall back to a RAW read rather than to an accessor that can refresh. The accessor also re-resolved the profile for its second read, so a concurrent `kcap use` could switch profiles mid-call and return B's token while the result still reported profile A. Load-and-refresh is now pinned to the profile resolved once at entry, and whoami reads the raw snapshot for that same profile. Deliberately NOT adopted: the suggestion that recovery withhold a stored token identical to the rejected one. That is a separate behaviour change from the double-refresh fix, and it regresses a shipped recovery the integration suite pins (Refreshed_token_succeeds_after_401) — a server that rejects transiently, during a rolling restart or from a node with stale key material, accepts the very next attempt. Callers retry at most once either way, so the cost of trying is one request; the cost of not trying is a session that stays broken.
Recovery resolved the active profile twice: once inside the forced rotation and again for the raw fallback. A `kcap use` landing during a slow rotation could therefore rotate profile A and then fall back to profile B's token — and when both profiles point at the same server the binding check has nothing to object to, so a long-lived client would quietly switch identity mid-session. Resolve once at entry and thread that profile through a profile-pinned rotation core, mirroring what the accessor already does.
…a bearer From Qodo review. A null server_url means "pre-upgrade token, binding unenforced". Login stamped it with whatever Canonicalize returned — including null, for a URL carrying user info, a query or a fragment. That silently minted a NEW credential in the legacy unenforced state, free to be sent to any later target. Login and tenant-switch now fail with a message naming the offending URL rather than saving a token they cannot bind. The setup completion ping also attached a bearer when the target server's provider is None. Setup performs no login on that path, so the token still in the profile belongs to whatever server it pointed at before — the ping would disclose it to an unrelated host. Skip it. Also: trim the whoami summary to what the class does rather than the incident that motivated it, and document the new probe and its exit codes in the README's Getting started section, not only the command listing.
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
1 participant
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.
Closes #386
Linear: AI-1504
Spec (codex-reviewed clean over 4 rounds):
docs/superpowers/specs/2026-07-28-ai1504-token-server-binding-design.mdin kcap-server.Why
A stored token carried no record of which server issued it. A multi-profile config — or a single profile whose
server_urlwas re-pointed after login — would send one server's token to another. The server rejects it with a bare 401, and/auth/refreshcan't heal it (it validates the token's own signature), so the CLI reported "not authenticated" for a token that was perfectly valid somewhere else.kcap whoamimade that much harder to diagnose than it should have been: it only printed local file metadata, so it called such a token valid while every request was being rejected. That gap is what turned the original incident into a multi-day misdiagnosis.What changed
D0 — canonical server identity. One
ServerIdentityhelper: scheme + host + effective port + case-sensitive path. Userinfo, query and fragment are rejected rather than dropped, so…/base?tenant=aand?tenant=bcan't compare equal. A non-canonicalizable side never matches (fail closed).D1 — tokens bound to their minting server.
StoredTokensgains a nullableserver_url, stamped at login and carried through refresh. Stamping authority is explicit per site — genericSaveAsyncnever infers it from ambient state, and the localhost fallback is never mistaken for evidence. GitHub refresh now targetstokens.ServerUrlfirst, so a bound token can't be posted to a server that never issued it.Enforcement lives in one accessor,
GetValidTokensForServerAsync, which is now the only way a bearer reaches a request. It checks the binding on the raw snapshot before any refresh (a token we've already decided not to use shouldn't spend a rotating credential), and returns the issuing URL alongside the verdict so callers name both servers without re-reading storage. Every attachment site moved onto it: the HTTP client factory, daemon SignalR + event POSTs, the watcher, attachment downloads, and the setup ping.Pre-upgrade tokens have no binding and stay unenforced until a login or unambiguous refresh stamps them; old CLIs ignore the new field.
D2 — lookup follows the resolved profile.
TokenStorenow prefers the profile the process resolved (KCAP_PROFILE,.kcap.json, git-remote,kcap use) over the on-disk active profile. Two carve-outs:kcap setupdeliberately configures the active profile, so it now does every token operation against that profile explicitly (no parameterless token calls remain in it).tokens.jsonfallback is scoped to its migration owner. An unscoped fallback would hand that unbound credential to any repo-resolved profile lacking a token — and being unbound, D1 couldn't catch it. There is deliberately no"default"exemption.D3 — whoami verifies. Prints the resolving profile, then asks the server. Only 401/403 are a verdict; unreachable, missing endpoint (older server), redirect and 5xx all report "could not verify" at exit 0, so nobody is sent to re-run
kcap loginover an outage.D4 — one-shot 401 recovery, single owner. A
DelegatingHandlerrefreshes once and resends. The refresh is conditional on the persisted token still being the rejected one, so a peer that already refreshed is adopted rather than rotated again — which for WorkOS (single-use refresh token) would invalidate the peer's session. The handler applies its own token per request (a stale client default would resend exactly what was just rejected), disposes the first 401, and resends exactly once, non-reentrantly.Deviation from the spec, and why
The spec asserted MCP servers use the handler-free factory and could keep their retry loops untouched. They don't — all eight call
CreateAuthenticatedClientAsync, so installing the handler there would have stacked two retry owners (the exact hazard the spec was trying to avoid). Resolved per the spec's own PR-audit rule — one retry owner per path — by having the MCP servers opt out of the handler (autoRetryUnauthorized: false) and keep their existing loops, now upgraded to the deduped force-refresh. The two MCP servers with no loop of their own keep the handler.Testing
New:
ServerIdentityTests(21),TokenServerBindingTests(15),UnauthorizedRetryHandlerTests(9),WhoamiProbeTests(12).Key assertions were mutation-tested — skipping the retry header update, dropping the first-response dispose, and reverting the refresh dedup each fail a test. Tests that could hang on an unintended refresh point at an unroutable address so a regression fails in milliseconds.
Full unit suite vs. an
origin/mainbaseline worktree on the same machine: 42 failures on this branch vs 72 on baseline, with zero failures unique to this branch (the remainder are pre-existing macOS environment flakes).Two adjustments were needed because token lookup now consults process-global resolved state — correct in production (resolved once at startup) but leaky in a shared test process: the token-store test classes reset it, and
McpFlowsServerTests.SeedDefaultTokenAsyncclears it alongside theconfig.jsonit already cleared for the same reason. That was found by bisecting a real cross-test failure, not assumed.AOT publish: 0 IL2026/IL3050 warnings. README +
help-whoami/help-usageupdated for the new whoami output and exit codes.