diff --git a/clients/tui/__tests__/AuthTab.test.tsx b/clients/tui/__tests__/AuthTab.test.tsx index e72b56e8..a330cab8 100644 --- a/clients/tui/__tests__/AuthTab.test.tsx +++ b/clients/tui/__tests__/AuthTab.test.tsx @@ -138,6 +138,55 @@ describe("AuthTab", () => { expect(lastFrame() ?? "").toContain("OAuth state cleared"); }); + // A state-based guard is not effective until React re-renders, so two `s` + // events in the SAME input turn both read the last-rendered "idle" and start + // concurrent clears against one store entry. No tick between the writes. + it("ignores a repeat delivered in the same input turn", async () => { + const onClearOAuth = vi.fn(() => new Promise(() => {})); + const { stdin } = render( + , + ); + await tick(); + + stdin.write("s"); + stdin.write("s"); + await tick(); + + expect(onClearOAuth).toHaveBeenCalledTimes(1); + }); + + // A rejection releases the lock, so the user can retry. + it("allows a retry after a rejected clear", async () => { + const onClearOAuth = vi + .fn<() => Promise>() + .mockRejectedValueOnce(new Error("nope")) + .mockResolvedValue(undefined); + const { stdin } = render( + , + ); + await tick(); + stdin.write("s"); + await tick(); + stdin.write("s"); + await tick(); + + expect(onClearOAuth).toHaveBeenCalledTimes(2); + }); + // 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. diff --git a/clients/tui/src/components/AuthTab.tsx b/clients/tui/src/components/AuthTab.tsx index d184f12f..00d713cd 100644 --- a/clients/tui/src/components/AuthTab.tsx +++ b/clients/tui/src/components/AuthTab.tsx @@ -100,6 +100,13 @@ export function AuthTab({ * — at which point server A's confirmation must not appear under server B. */ const clearAttemptRef = useRef(0); + /** + * In-flight lock. A ref, not the `clearState` above: two `s` keypresses + * delivered in the same input turn both read the state React last rendered, + * so a state-based guard lets both start and race each other over the same + * store entry. A ref is set before the call and seen by the second read. + */ + const clearInFlightRef = useRef(false); /** * Set synchronously when a clear starts, and consumed by the reset effect * below. The clear itself bumps `oauthRevision` on its way out, so without @@ -115,6 +122,7 @@ export function AuthTab({ // banner: neither belongs to the server now on screen. useEffect(() => { clearAttemptRef.current++; + clearInFlightRef.current = false; setClearState("idle"); setClearFailure(null); setLastClearDisconnected(false); @@ -207,8 +215,9 @@ export function AuthTab({ } 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; + // first is still running except by the pending line below. + if (clearInFlightRef.current) return; + clearInFlightRef.current = true; const attempt = ++clearAttemptRef.current; const attemptServer = serverName; setLastClearDisconnected(isLiveConnection); @@ -222,13 +231,21 @@ export function AuthTab({ serverNameRef.current === attemptServer; void Promise.resolve(onClearOAuth()).then( () => { + clearInFlightRef.current = false; if (current()) setClearState("cleared"); }, (err: unknown) => { + clearInFlightRef.current = false; // 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. + // + // It also means `handleClearOAuth` never reached its + // `oauthRevision` bump, so the marker below has no bump to skip. + // Left set, it would swallow the next *unrelated* revision change + // and strand this banner after the OAuth state moved on. + ownClearRef.current = false; if (!current()) return; setClearFailure(err instanceof Error ? err.message : String(err)); setClearState("failed"); diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index d55e4627..fc7f4fa1 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -1368,6 +1368,47 @@ describe("useOAuthRecovery", () => { ); }); + // #2144 — the RFC 7009 leg is a bounded network request, so this callback + // can stay suspended for seconds. `isActive`/`inspectorClient` are captured + // before it, so without revalidating, a switch during the wait would + // disconnect the session the user just moved to and run the session-wide + // cleanup against it. + it("does not disconnect a session switched to while a clear is in flight", async () => { + let settle: (r: { cleared: boolean }) => void = () => {}; + clearServerOAuthStateMock.mockImplementation( + () => + new Promise((resolve) => { + settle = resolve as typeof settle; + }), + ); + const client = fakeClient(); + const h = harness({ + servers: [entry("a"), entry("b")], + activeServerId: "a", + client, + }); + + let done: Promise; + await act(async () => { + done = h.api().clearServerOAuthAndDisconnect(entry("a")); + await Promise.resolve(); + }); + + // The user switches away while the clear is still running. + h.rerender({ + servers: [entry("a"), entry("b")], + activeServerId: "b", + client, + }); + + await act(async () => { + settle({ cleared: true }); + await done; + }); + + expect(client.disconnect).not.toHaveBeenCalled(); + }); + it("clears the resume snapshot on an explicit disconnect", () => { writeOAuthResumeSnapshot({ version: 1, diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index f00d120e..d58afc40 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -1254,9 +1254,23 @@ export function useOAuthRecovery({ const clearServerOAuthAndDisconnect = useCallback( async (server: ClearableServer) => { const isActive = server.id === activeServerId; + const client = isActive ? inspectorClient : null; + // The RFC 7009 leg is a bounded network request (#2144), so this callback + // can stay suspended for seconds — long enough for the user to close the + // modal and switch servers. `isActive` and `inspectorClient` were + // snapshotted before it, so everything below would otherwise apply to + // whatever session is active *now*: disconnecting a client the user just + // switched to, and running the session-wide UI cleanup against it. + // + // The session ref is the live answer. Only the parts that touch the + // *session* are gated on it; the store write already happened and the + // toast still belongs to the server the user asked about. + const stillTargetsActiveSession = (): boolean => + isActive && sessionRef.current.activeServerId === server.id; + const { cleared, revocation } = await clearServerOAuthState({ config: server.config, - inspectorClient: isActive ? inspectorClient : null, + inspectorClient: client, isActiveConnection: isActive, oauthStorage: webOAuthStorage, revoke: server.settings?.oauthRevokeOnClear !== false, @@ -1264,14 +1278,17 @@ export function useOAuthRecovery({ }); if (!cleared) return; - if (isActive && inspectorClient) { + if (client && stillTargetsActiveSession()) { try { - await inspectorClient.disconnect(); + await client.disconnect(); } finally { - setConnectionInfoOAuthWhenConnected(undefined); - finalizeExplicitDisconnect(); + // Revalidate after the second await for the same reason. + if (stillTargetsActiveSession()) { + setConnectionInfoOAuthWhenConnected(undefined); + finalizeExplicitDisconnect(); + } } - } else { + } else if (!isActive) { clearOAuthResumeOnExplicitDisconnect(); } @@ -1286,6 +1303,7 @@ export function useOAuthRecovery({ [ activeServerId, inspectorClient, + sessionRef, webOAuthStorage, finalizeExplicitDisconnect, clearOAuthResumeOnExplicitDisconnect, diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index b97620d9..994ed8c6 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -476,9 +476,11 @@ describe("revokeStoredOAuthTokens", () => { // 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 () => { + // The metadata names the same issuer as the slot below, so BOTH grants are + // genuinely revocable — otherwise this would pass for the wrong reason. await storage.saveServerMetadata( SERVER_URL, - metadata({ issuer: undefined }), + metadata({ issuer: "https://as-a.example.com" }), ); // A legacy unkeyed grant... await storage.saveTokens(SERVER_URL, { @@ -507,15 +509,96 @@ describe("revokeStoredOAuthTokens", () => { expect(fetchFn).toHaveBeenCalledTimes(2); }); + // Absence establishes nothing: a metadata document with no `issuer` cannot + // show that its revocation endpoint belongs to the grant's authorization + // server, so sending the token anyway would disclose a bearer credential to + // a server that may never have minted it. + it("refuses an issuer-bound grant when the cached metadata names no issuer", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: undefined }), + ); + await storage.saveTokens( + SERVER_URL, + { access_token: "a", token_type: "Bearer", refresh_token: "r" }, + { issuer: "https://as-a.example.com" }, + ); + const fetchFn = vi.fn(); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + expect(fetchFn).not.toHaveBeenCalled(); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "names no issuer", + ); + }); + + // A legacy unkeyed grant is not bound to any authorization server, so there + // is nothing to contradict — it proceeds without the comparison. + it("still revokes a legacy grant when the metadata names no issuer", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: undefined }), + ); + await storage.saveTokens(SERVER_URL, { + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + await expect( + revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }), + ).resolves.toMatchObject({ status: "revoked" }); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + + // If `listIssuers` fails there is no enumerated slot behind the stamp, so + // skipping a stamped ctx-less read would leave the one grant the store can + // still produce unrevoked — while `clear` deleted it anyway. + it("revokes the active grant even when the issuer list cannot be read", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as-a.example.com" }), + ); + await storage.saveTokens( + SERVER_URL, + { access_token: "a", token_type: "Bearer", refresh_token: "r-active" }, + { issuer: "https://as-a.example.com" }, + ); + vi.spyOn(storage, "listIssuers").mockRejectedValue(new Error("no list")); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + expect( + new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"), + ).toBe("r-active"); + // The listing failure is still surfaced — it outranks the success. + expect(outcome).toMatchObject({ status: "failed" }); + }); + // 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 }), - ); + await storage.saveServerMetadata(SERVER_URL, metadata({ issuer: "a" })); + // All three bound to the issuer the cached metadata describes, so all three + // are genuinely revocable and the budget is what stops them. vi.spyOn(storage, "listIssuers").mockResolvedValue(["a", "b", "c"]); vi.spyOn(storage, "getIssuerTokens").mockImplementation( async (_url: string, issuer: string) => ({ diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index be867c5e..2eab0f90 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -405,30 +405,29 @@ async function collectGrants( const revocable = selectRevocableToken(tokens); if (!revocable) return; - if (issuer === undefined) { - // `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); - } + // `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), so the ctx-less + // read reports which issuer its answer belongs to, if any. + const grantIssuer = + issuer ?? (tokens as { issuer?: string } | undefined)?.issuer; + // Keyed by issuer AND token: a token means nothing outside the + // authorization server that minted it, so two issuers minting the same + // opaque value are two grants. Deduping the ctx-less read on its *stamp* + // alone would be wrong too — if `listIssuers` failed there is no enumerated + // slot behind the stamp, and skipping would leave the one readable grant + // unrevoked while `clear` deleted it. + const key = `${grantIssuer ?? "\u0000legacy"}\u0000${revocable.token}`; + if (seenKeys.has(key)) return; + seenKeys.add(key); grants.push({ - issuer, + issuer: grantIssuer, ...revocable, clientInformation: await resolveClientInformation( storage, serverUrl, - issuer, + grantIssuer, ), }); }; @@ -541,17 +540,20 @@ async function computeOutcome( 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 - // issuer's token to *this* endpoint would hand a credential to a server - // that never minted it — worse than not revoking. So say plainly that the - // grant is being dropped unrevoked rather than doing either silently. - if ( - grant.issuer !== undefined && - metadata.issuer !== undefined && - grant.issuer !== metadata.issuer - ) { + // issuer's token to *this* endpoint would disclose a bearer credential to + // a server that never minted it — worse than not revoking. So an + // issuer-bound grant must be able to PROVE the endpoint is its own, which + // means a metadata document carrying no `issuer` is a mismatch rather + // than a free pass: absence establishes nothing. Only a legacy unkeyed + // grant proceeds without the comparison, since nothing binds it to a + // different authorization server in the first place. + if (grant.issuer !== undefined && metadata.issuer !== grant.issuer) { outcomes.push({ status: "failed", - detail: `the cached authorization-server metadata is for ${metadata.issuer}, so the grant bound to ${grant.issuer} was cleared without revocation`, + detail: + metadata.issuer === undefined + ? `the cached authorization-server metadata names no issuer, so the grant bound to ${grant.issuer} could not be matched to this revocation endpoint and was cleared without revocation` + : `the cached authorization-server metadata is for ${metadata.issuer}, so the grant bound to ${grant.issuer} was cleared without revocation`, }); continue; }