diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index 6cd96b5a..a4ab8c01 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -3,6 +3,7 @@ import { resetNodeOAuthStorageCache, } from "@inspector/core/auth/node/storage-node.js"; import { + DEFAULT_REVOCATION_TIMEOUT_MS, revokeStoredOAuthTokens, type TokenRevocationOutcome, } from "@inspector/core/auth/revocation.js"; @@ -92,13 +93,26 @@ async function revokeStoredKeys( keys: string[], ): Promise { const fetchFn = createProxyFetch() ?? fetch; + // One budget across both keys, for the same reason `revokeStoredOAuthTokens` + // shares one across grants: two keys would otherwise double the wait a user + // feels before `--relogin` gets on with the local delete. + const deadlineAt = Date.now() + DEFAULT_REVOCATION_TIMEOUT_MS; let reported: TokenRevocationOutcome | undefined; let lastSkip: TokenRevocationOutcome | undefined; for (const key of new Set(keys)) { + const remainingMs = deadlineAt - Date.now(); + if (remainingMs <= 0) { + reported ??= { + status: "failed", + detail: `the ${DEFAULT_REVOCATION_TIMEOUT_MS}ms revocation budget was exhausted before "${key}" was attempted`, + }; + continue; + } const outcome = await revokeStoredOAuthTokens({ serverUrl: key, storage, fetchFn, + timeoutMs: remainingMs, }); if (outcome.status === "skipped" && outcome.reason === "no_tokens") { lastSkip = outcome; diff --git a/clients/tui/__tests__/AuthTab.test.tsx b/clients/tui/__tests__/AuthTab.test.tsx index 3c0e0ec2..e72b56e8 100644 --- a/clients/tui/__tests__/AuthTab.test.tsx +++ b/clients/tui/__tests__/AuthTab.test.tsx @@ -138,10 +138,13 @@ describe("AuthTab", () => { 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"))); + // A rejection is NOT a revocation failure — those come back as outcomes and + // are reported through the message line. This is the local clear or the + // disconnect itself failing, so reporting success would be a plain lie. + it("reports a rejected clear as a failure, not as success", async () => { + const onClearOAuth = vi.fn(() => + Promise.reject(new Error("keychain locked")), + ); const { lastFrame, stdin } = render( { await tick(); stdin.write("s"); await tick(); - expect(lastFrame() ?? "").toContain("OAuth state cleared"); + const frame = lastFrame() ?? ""; + expect(frame).toContain("Could not clear OAuth state"); + expect(frame).toContain("keychain locked"); + expect(frame).not.toContain("OAuth state cleared."); + expect(frame).not.toContain("Clearing OAuth state"); + }); + + // A clear started on server A must not confirm under server B: it is a + // bounded network request now, so it can settle after the user has moved on. + it("does not confirm a clear that settles after the server changed", async () => { + let settle: () => void = () => {}; + const onClearOAuth = vi.fn( + () => + new Promise((resolve) => { + settle = resolve; + }), + ); + const { lastFrame, stdin, rerender } = render( + , + ); + await tick(); + stdin.write("s"); + await tick(); + expect(lastFrame() ?? "").toContain("Clearing OAuth state"); + + rerender( + , + ); + await tick(); + settle(); + await tick(); + + const frame = lastFrame() ?? ""; + expect(frame).not.toContain("OAuth state cleared"); + expect(frame).not.toContain("Clearing OAuth state"); }); // #2144 — a revocation failure is a *partial* success: the local state really diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 2ee0ef53..48e75bab 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -464,6 +464,11 @@ function App({ // server name matching again, so the stale rejection would land on the // re-selected A. Retiring the token on every switch is what closes that. disconnectAttemptRef.current++; + // Same reasoning for the clear (#2144): the A → B → A round trip would + // otherwise leave both its attempt token and the captured server name + // matching again, so a clear started on the first A could publish into the + // re-selected one. + clearOAuthAttemptRef.current++; setDisconnectError(null); const stepUp = pendingStepUpRef.current; if (stepUp && selectedServer && stepUp.serverName !== selectedServer) { @@ -983,6 +988,14 @@ function App({ setConnectError(null); if (inspectorStatus === "connected" || inspectorStatus === "connecting") { await disconnectInspector(); + // Revalidate: the disconnect is a second await, and a switch during it + // would make the revision bump below land on the new selection. + if ( + clearOAuthAttemptRef.current !== attempt || + selectedServerRef.current !== attemptServer + ) { + return; + } } setOauthRevision((n) => n + 1); }, [ diff --git a/clients/tui/src/components/AuthTab.tsx b/clients/tui/src/components/AuthTab.tsx index 43c9eb00..d184f12f 100644 --- a/clients/tui/src/components/AuthTab.tsx +++ b/clients/tui/src/components/AuthTab.tsx @@ -90,9 +90,16 @@ export function AuthTab({ const [oauthState, setOauthState] = useState< OAuthConnectionState | undefined >(undefined); - const [clearState, setClearState] = useState<"idle" | "clearing" | "cleared">( - "idle", - ); + const [clearState, setClearState] = useState< + "idle" | "clearing" | "cleared" | "failed" + >("idle"); + const [clearFailure, setClearFailure] = useState(null); + /** + * The server a clear was started for, and a sequence number. The clear is a + * bounded network request now, so it can settle after the user has moved on + * — at which point server A's confirmation must not appear under server B. + */ + const clearAttemptRef = useRef(0); /** * Set synchronously when a clear starts, and consumed by the reset effect * below. The clear itself bumps `oauthRevision` on its way out, so without @@ -100,6 +107,18 @@ export function AuthTab({ * which of the two lands first is not something the ordering guarantees. */ const ownClearRef = useRef(false); + const serverNameRef = useRef(serverName); + useEffect(() => { + serverNameRef.current = serverName; + }, [serverName]); + // A new selection retires any in-flight clear and drops the previous one's + // banner: neither belongs to the server now on screen. + useEffect(() => { + clearAttemptRef.current++; + setClearState("idle"); + setClearFailure(null); + setLastClearDisconnected(false); + }, [serverName]); const [lastClearDisconnected, setLastClearDisconnected] = useState(false); const [stepUpChoiceIndex, setStepUpChoiceIndex] = useState(0); @@ -123,6 +142,7 @@ export function AuthTab({ return; } setClearState("idle"); + setClearFailure(null); setLastClearDisconnected(false); }, [oauthRevision]); @@ -189,13 +209,31 @@ export function AuthTab({ // 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; + const attempt = ++clearAttemptRef.current; + const attemptServer = serverName; setLastClearDisconnected(isLiveConnection); + setClearFailure(null); 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); + // A completion is only ours if nothing has superseded it and the + // selection has not moved on. + const current = () => + clearAttemptRef.current === attempt && + serverNameRef.current === attemptServer; + void Promise.resolve(onClearOAuth()).then( + () => { + if (current()) setClearState("cleared"); + }, + (err: unknown) => { + // A rejection is NOT a revocation failure — those come back as + // outcomes and are reported through the message line. This is the + // local clear or the disconnect itself failing, so announcing + // "OAuth state cleared" here would be a plain lie. + if (!current()) return; + setClearFailure(err instanceof Error ? err.message : String(err)); + setClearState("failed"); + }, + ); } }, { isActive: focused }, @@ -371,6 +409,12 @@ export function AuthTab({ : "OAuth state cleared."} )} + {clearState === "failed" && ( + + Could not clear OAuth state + {clearFailure ? `: ${clearFailure}` : "."} + + )} diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index e29f2cab..b97620d9 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -507,6 +507,43 @@ describe("revokeStoredOAuthTokens", () => { expect(fetchFn).toHaveBeenCalledTimes(2); }); + // The timeout is a budget for the WHOLE teardown, not per request. `clear` + // deletes every issuer slot, so a per-request bound would let a server with + // N of them block the disconnect for N × the timeout — at which point the + // "short timeout" bounds a single request and nothing the user feels. + it("shares one deadline across grants instead of one per grant", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: undefined }), + ); + vi.spyOn(storage, "listIssuers").mockResolvedValue(["a", "b", "c"]); + vi.spyOn(storage, "getIssuerTokens").mockImplementation( + async (_url: string, issuer: string) => ({ + access_token: `a-${issuer}`, + token_type: "Bearer", + refresh_token: `r-${issuer}`, + }), + ); + // Every request hangs, so each one burns the whole remaining budget. + const fetchFn = vi.fn(() => new Promise(() => {})); + + const started = Date.now(); + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + timeoutMs: 30, + }); + const elapsed = Date.now() - started; + + expect(outcome).toMatchObject({ status: "failed" }); + // Three grants: with a per-grant bound this would be ~90ms. Generous upper + // bound so the assertion is about the shape, not the machine. + expect(elapsed).toBeLessThan(70); + // The first burned the budget; the rest are reported as never attempted. + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + // 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 188e9fdd..be867c5e 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -36,6 +36,11 @@ import type { OAuthStorage } from "./storage.js"; * committed to, so an unreachable or wedged authorization server must not hold * the teardown open — five seconds is long enough for a real endpoint on a slow * link and short enough that a dead one is not felt as a hang. + * + * It is a budget for the **whole** call, not per request: a server with several + * issuer-bound grants revokes them sequentially against one shared deadline, so + * the bound the user feels is this number regardless of how many grants the + * clear is about to delete. */ export const DEFAULT_REVOCATION_TIMEOUT_MS = 5000; @@ -525,6 +530,14 @@ async function computeOutcome( const supportedAuthMethods = revocationAuthMethods(metadata); const outcomes: TokenRevocationOutcome[] = [...failures]; + // ONE deadline for the whole teardown, not one per grant. `clear` deletes + // every issuer slot, so a server with N of them would otherwise block the + // disconnect for N × the timeout — at which point the "short timeout" + // bounds a single request and nothing the user experiences. Grants are + // revoked sequentially on purpose (a burst of parallel requests to one + // authorization server is not a kindness), so the budget is shared instead. + const timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS; + const deadlineAt = Date.now() + timeoutMs; for (const grant of grants) { // Metadata is cached once per server, not per issuer, so it describes // whichever authorization server was discovered last. Sending another @@ -542,6 +555,17 @@ async function computeOutcome( }); continue; } + const remainingMs = deadlineAt - Date.now(); + if (remainingMs <= 0) { + outcomes.push({ + status: "failed", + endpoint, + detail: `the ${timeoutMs}ms revocation budget was exhausted before this grant was attempted${ + grant.issuer === undefined ? "" : ` (issuer ${grant.issuer})` + }`, + }); + continue; + } outcomes.push( await revokeToken({ endpoint, @@ -550,7 +574,7 @@ async function computeOutcome( clientInformation: grant.clientInformation, supportedAuthMethods, fetchFn, - timeoutMs: params.timeoutMs, + timeoutMs: remainingMs, }), ); }