diff --git a/clients/tui/__tests__/AuthTab.test.tsx b/clients/tui/__tests__/AuthTab.test.tsx index 65cda2d2..3c0e0ec2 100644 --- a/clients/tui/__tests__/AuthTab.test.tsx +++ b/clients/tui/__tests__/AuthTab.test.tsx @@ -100,6 +100,64 @@ describe("AuthTab", () => { ); }); + // #2144 — the clear is now a bounded network request, so announcing "cleared" + // on the keypress would say it while the work was still in flight. + it("shows a pending state until the clear settles, and ignores repeats", async () => { + let settle: () => void = () => {}; + const onClearOAuth = vi.fn( + () => + new Promise((resolve) => { + settle = resolve; + }), + ); + const { lastFrame, stdin } = render( + , + ); + await tick(); + + stdin.write("s"); + await tick(); + expect(lastFrame() ?? "").toContain("Clearing OAuth state"); + expect(lastFrame() ?? "").not.toContain("OAuth state cleared"); + + // A second press while one is in flight would race the first over the same + // store entry, and nothing on screen tells the user the first is running. + stdin.write("s"); + await tick(); + expect(onClearOAuth).toHaveBeenCalledTimes(1); + + settle(); + await tick(); + expect(lastFrame() ?? "").toContain("OAuth state cleared"); + }); + + // A failed revocation still cleared local state, and the failure is reported + // through the message line — so the confirmation must not hang forever. + it("settles the confirmation even when the clear rejects", async () => { + const onClearOAuth = vi.fn(() => Promise.reject(new Error("nope"))); + const { lastFrame, stdin } = render( + , + ); + await tick(); + stdin.write("s"); + await tick(); + expect(lastFrame() ?? "").toContain("OAuth state cleared"); + }); + // #2144 — a revocation failure is a *partial* success: the local state really // was cleared, so it is not an `error` status, but the grant may still be // live at the authorization server and the informational tone understates diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 59e321b3..2ee0ef53 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -183,6 +183,8 @@ function App({ const [connectError, setConnectError] = useState(null); // Monotonic token for in-flight disconnects — see handleDisconnect. const disconnectAttemptRef = useRef(0); + /** Retires a superseded "clear OAuth state" the way disconnects are (#2144). */ + const clearOAuthAttemptRef = useRef(0); // A failed disconnect is deliberately NOT folded into `connectError`. That // one is only rendered by InfoTab when the status is "error", which a // rejected disconnect leaves untouched (the status stays "connected"), so @@ -949,13 +951,27 @@ function App({ const handleClearOAuth = useCallback(async () => { if (!selectedInspectorClient) return; - // RFC 7009 (#2144). Best-effort: the outcome is reported in the status - // line, never thrown, so clearing always completes. The per-server - // `oauthRevokeOnClear` opt-out is honored here the same way the web client - // honors it. + // Claim this attempt before awaiting, exactly as `handleDisconnect` does. + // The RFC 7009 leg is a bounded network request, so this callback can now + // stay suspended for seconds — long enough for the user to switch servers, + // at which point publishing this result would put server A's outcome on + // server B (and would race a second clear over the same state). (#2144) + const attempt = ++clearOAuthAttemptRef.current; + const attemptServer = selectedServer; + // RFC 7009. Best-effort: the outcome is reported in the status line, never + // thrown, so clearing always completes. The per-server `oauthRevokeOnClear` + // opt-out is honored here the same way the web client honors it. const revocation = await selectedInspectorClient.clearOAuthTokens({ revoke: selectedServerEntry?.settings?.oauthRevokeOnClear !== false, }); + // The local clear has happened either way — only the *reporting* is + // retired, so a superseded attempt leaves no trace on the new selection. + if ( + clearOAuthAttemptRef.current !== attempt || + selectedServerRef.current !== attemptServer + ) { + return; + } setOauthStatus("idle"); if (revocation.status === "failed") { const warning = `Cleared locally, but revoking the grant at the authorization server failed: ${revocation.detail}. It may still be valid there.`; @@ -971,6 +987,7 @@ function App({ setOauthRevision((n) => n + 1); }, [ selectedInspectorClient, + selectedServer, selectedServerEntry, inspectorStatus, disconnectInspector, diff --git a/clients/tui/src/components/AuthTab.tsx b/clients/tui/src/components/AuthTab.tsx index 21e8f2eb..43c9eb00 100644 --- a/clients/tui/src/components/AuthTab.tsx +++ b/clients/tui/src/components/AuthTab.tsx @@ -47,7 +47,15 @@ interface AuthTabProps { width: number; height: number; focused?: boolean; - onClearOAuth: () => void; + /** + * Clears (and, unless opted out, revokes) this server's OAuth state. + * + * Returns a promise so the confirmation below can wait for it. The RFC 7009 + * revocation leg is a network request with a five-second bound (#2144), so + * this is no longer instantaneous, and announcing "cleared" on the keypress + * would say it while the work was still in flight. + */ + onClearOAuth: () => void | Promise; connectionStatus: ConnectionStatus; } @@ -82,7 +90,16 @@ export function AuthTab({ const [oauthState, setOauthState] = useState< OAuthConnectionState | undefined >(undefined); - const [clearedConfirmation, setClearedConfirmation] = useState(false); + const [clearState, setClearState] = useState<"idle" | "clearing" | "cleared">( + "idle", + ); + /** + * Set synchronously when a clear starts, and consumed by the reset effect + * below. The clear itself bumps `oauthRevision` on its way out, so without + * this the reset would race the confirmation it is meant to outlive — and + * which of the two lands first is not something the ordering guarantees. + */ + const ownClearRef = useRef(false); const [lastClearDisconnected, setLastClearDisconnected] = useState(false); const [stepUpChoiceIndex, setStepUpChoiceIndex] = useState(0); @@ -100,7 +117,12 @@ export function AuthTab({ }, [refreshOAuthState, oauthRevision, connectionStatus]); useEffect(() => { - setClearedConfirmation(false); + // Skip exactly the revision bump our own clear caused; reset on the next. + if (ownClearRef.current) { + ownClearRef.current = false; + return; + } + setClearState("idle"); setLastClearDisconnected(false); }, [oauthRevision]); @@ -163,9 +185,17 @@ export function AuthTab({ const h = scrollViewRef.current.getViewportHeight() || 1; scrollViewRef.current.scrollBy(h); } else if (input.toLowerCase() === "s") { + // Ignore repeats while one is in flight: the second would race the + // first over the same store entry, and the user cannot see that the + // first is still running except by this state. + if (clearState === "clearing") return; setLastClearDisconnected(isLiveConnection); - onClearOAuth(); - setClearedConfirmation(true); + ownClearRef.current = true; + setClearState("clearing"); + const settle = () => setClearState("cleared"); + // Settled either way: a failed revocation still cleared local state, + // and `handleClearOAuth` reports the failure through the message line. + void Promise.resolve(onClearOAuth()).then(settle, settle); } }, { isActive: focused }, @@ -331,7 +361,10 @@ export function AuthTab({ Clear OAuth State {isLiveConnection && " and disconnect"} - {clearedConfirmation && ( + {clearState === "clearing" && ( + Clearing OAuth state… + )} + {clearState === "cleared" && ( {lastClearDisconnected ? "OAuth state cleared. Disconnected." diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index c999cc90..e29f2cab 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -191,12 +191,11 @@ describe("buildRevocationRequest", () => { clientInformation: { client_id: "id one", client_secret: "s/ecret" }, supportedAuthMethods: ["client_secret_basic"], }); - // Byte-identical to the SDK's `applyBasicAuth`: the raw `id:secret`, not the - // form-urlencoded pair RFC 6749 §2.3.1 asks for. Presenting the credential - // differently here than at the token endpoint is what would let an - // authorization server accept one request and reject the other. + // RFC 6749 §2.3.1 form-urlencodes each half before the colon. That is what + // keeps an id containing `:` unambiguous, and what stops `btoa` throwing on + // a non-Latin-1 secret. expect(headerOf(init, "Authorization")).toBe( - `Basic ${btoa("id one:s/ecret")}`, + `Basic ${btoa("id%20one:s%2Fecret")}`, ); expect(body(init).has("client_secret")).toBe(false); }); @@ -471,6 +470,43 @@ describe("revokeStoredOAuthTokens", () => { ).toBe("legacy-r"); }); + // The active issuer slot can hold client information without a token, in + // which case the ctx-less read falls back to the LEGACY grant — a distinct + // grant, even if some other issuer happens to hold the same opaque value. + // Suppressing on the value would drop it unrevoked and unreported, which is + // why the suppression keys off the store's issuer stamp instead. + it("keeps the legacy grant when another issuer holds the same token value", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: undefined }), + ); + // A legacy unkeyed grant... + await storage.saveTokens(SERVER_URL, { + access_token: "shared", + token_type: "Bearer", + refresh_token: "shared-r", + }); + // ...and an issuer slot that coincidentally holds the same token value. + // Saved via the client-information path so it does not clear the legacy + // slot, then given tokens through a stubbed exact read. + vi.spyOn(storage, "listIssuers").mockResolvedValue([ + "https://as-a.example.com", + ]); + vi.spyOn(storage, "getIssuerTokens").mockResolvedValue({ + access_token: "shared", + token_type: "Bearer", + refresh_token: "shared-r", + }); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + await revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }); + + // Two grants, two requests — the issuer-bound one and the legacy one. + expect(fetchFn).toHaveBeenCalledTimes(2); + }); + // A grant bound to an issuer the cached metadata does not describe cannot be // revoked — that endpoint belongs to a different authorization server, and // sending it another AS's token would hand a credential to a server that diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index 4d16b216..188e9fdd 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -140,15 +140,17 @@ export function buildRevocationRequest(params: RevocationRequestParams): { if (client) { const method = selectClientAuthMethod(client, params.supportedAuthMethods); if (method === "client_secret_basic" && client.client_secret) { - // Deliberately byte-identical to what the SDK's `applyBasicAuth` sends at - // the token endpoint: the raw `id:secret`, *not* the form-urlencoded pair - // RFC 6749 §2.3.1 asks for. Matching the RFC here instead would mean this - // request and the token request present the same credential differently, - // so an authorization server holding a secret with a reserved character - // could accept one and reject the other — a failure that would look like - // "revocation is broken" rather than like an encoding disagreement. - // Whatever the AS accepted to mint these tokens is what ends them. - headers.Authorization = `Basic ${base64Encode(`${client.client_id}:${client.client_secret}`)}`; + // RFC 6749 §2.3.1: each half is form-urlencoded *before* the colon joins + // them. This deliberately differs from the SDK's `applyBasicAuth`, which + // base64s the raw pair — matching that was the first instinct, on the + // theory that a credential should be presented the same way here as at + // the token endpoint. It does not hold up: a client authenticated with + // `client_secret_post` never exercised the SDK's Basic path at all, so + // there is no precedent to match, and the raw form is ambiguous for a + // client id containing `:` and makes `btoa` throw outright on a + // non-Latin-1 secret. Encoding is what the server decodes. + const credentials = `${encodeURIComponent(client.client_id)}:${encodeURIComponent(client.client_secret)}`; + headers.Authorization = `Basic ${base64Encode(credentials)}`; } else if (method === "client_secret_post" && client.client_secret) { body.set("client_id", client.client_id); body.set("client_secret", client.client_secret); @@ -374,11 +376,13 @@ interface CollectedGrants { * * The ctx-less read is included last and is the one exception: it resolves to * the *active* issuer's slot (already collected above) or, on a pre-SEP-2352 - * entry, to the legacy unkeyed token — which nothing else returns. So it is - * suppressed when its token was already taken from any slot, which is exactly - * the first case and never the second. It is also the only read here allowed - * to fall back; an enumerated issuer is read exactly, so a legacy token is - * never mislabelled as belonging to one. + * entry, to the legacy unkeyed token — which nothing else returns. It is + * suppressed on the issuer *stamp* the store puts on a slot-sourced value + * rather than on the token's value, because the active slot may hold client + * information without a token: the read then falls back to the legacy grant, + * and a value collision with any other issuer would drop a real grant. It is + * also the only read here allowed to fall back; an enumerated issuer is read + * exactly, so a legacy token is never mislabelled as belonging to one. */ async function collectGrants( storage: OAuthStorage, @@ -387,7 +391,6 @@ async function collectGrants( const grants: StoredGrant[] = []; const failures: TokenRevocationOutcome[] = []; const seenKeys = new Set(); - const slotTokens = new Set(); const add = async (issuer?: string): Promise => { const tokens = @@ -398,13 +401,20 @@ async function collectGrants( if (!revocable) return; if (issuer === undefined) { - // Same grant as the active issuer's slot, already collected. - if (slotTokens.has(revocable.token)) return; + // `getTokens` stamps the resolved issuer onto a value it took from a + // byIssuer slot and leaves an unkeyed one unstamped (see `withIssuer` in + // oauth-storage.ts — the stamp is the key it came from). That, not the + // token's *value*, is what says this read duplicates a slot already + // collected: the active slot can hold client information without a token, + // in which case this falls back to the legacy grant, and a coincidental + // value collision with some other issuer would otherwise drop it + // unrevoked and unreported. + const fromSlot = (tokens as { issuer?: string } | undefined)?.issuer; + if (fromSlot !== undefined) return; } else { const key = `${issuer}\u0000${revocable.token}`; if (seenKeys.has(key)) return; seenKeys.add(key); - slotTokens.add(revocable.token); } grants.push({ diff --git a/test-servers/src/test-server-oauth.ts b/test-servers/src/test-server-oauth.ts index 4024ef65..9f763bab 100644 --- a/test-servers/src/test-server-oauth.ts +++ b/test-servers/src/test-server-oauth.ts @@ -798,8 +798,12 @@ async function authenticateRevocationClient( ).toString("utf8"); const separator = decoded.indexOf(":"); if (separator === -1) return null; - clientId = decoded.slice(0, separator); - clientSecret = decoded.slice(separator + 1); + // RFC 6749 §2.3.1: each half is form-urlencoded before the colon, so the + // server decodes each half after splitting on it. Decoding is what makes a + // credential containing a reserved character (`:` in the id, `%` or `/` in + // the secret) survive the round trip. + clientId = decodeURIComponent(decoded.slice(0, separator)); + clientSecret = decodeURIComponent(decoded.slice(separator + 1)); } else { const bodyId: unknown = req.body?.client_id; const bodySecret: unknown = req.body?.client_secret;