From 6c499f3ea15024f862ee102903c298f284dbbcbc Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 02:58:04 -0400 Subject: [PATCH] fix(auth): read enumerated issuers exactly, and stop the TUI tone going stale (#2144) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 4. - `getTokens(url, issuer)` falls back to the legacy unkeyed slot when that issuer holds none — right for a connect, wrong for `collectGrants`, which labelled the fallback with the issuer it happened to be iterating and paired it with that issuer's credentials. During a partially migrated flow the clear could have sent an old, unbound token to a newly discovered authorization server. `OAuthStorage.getIssuerTokens` is the exact, no-fallback read; only the ctx-less read still falls back, which is the one place it belongs. - The TUI's warning tone was sticky. It lived in its own state while ~30 call sites set ordinary OAuth notes without resetting it, so one revocation failure left every later "Authorization updated" yellow, across server switches. It is now DERIVED — `oauthMessageToneFor(message, warningText)` — so it cannot outlive the message that earned it, with nothing to remember to clear. Both setters stay plain `useState` setters: a `useCallback` wrapper would be a value `react-hooks/exhaustive-deps` demands in seven dependency arrays for an identity that never changes. - Hook-level tests for the web wiring (previously missed): the per-server opt-out and the backend-proxied fetch are now asserted to reach `clearServerOAuthState`, so handing it the page-origin fetch — which a real authorization server's missing CORS headers would reject — fails a test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- .../tui/__tests__/oauthMessageTone.test.ts | 28 +++++++++++++++ clients/tui/src/App.tsx | 28 ++++++++------- clients/tui/src/oauthMessageTone.ts | 29 +++++++++++++++ .../web/src/hooks/useOAuthRecovery.test.tsx | 26 ++++++++++++++ .../test/core/auth/connection-state.test.ts | 1 + .../src/test/core/auth/ema/emaFlow.test.ts | 1 + .../src/test/core/auth/ema/idpSession.test.ts | 1 + .../web/src/test/core/auth/revocation.test.ts | 36 +++++++++++++++++++ .../test/core/auth/storage-browser.test.ts | 35 ++++++++++++++++++ .../src/test/core/mcp/oauthManager.test.ts | 1 + core/auth/oauth-storage.ts | 11 ++++++ core/auth/revocation.ts | 16 ++++++--- core/auth/storage.ts | 15 ++++++++ 13 files changed, 211 insertions(+), 17 deletions(-) create mode 100644 clients/tui/__tests__/oauthMessageTone.test.ts create mode 100644 clients/tui/src/oauthMessageTone.ts diff --git a/clients/tui/__tests__/oauthMessageTone.test.ts b/clients/tui/__tests__/oauthMessageTone.test.ts new file mode 100644 index 00000000..7d00c059 --- /dev/null +++ b/clients/tui/__tests__/oauthMessageTone.test.ts @@ -0,0 +1,28 @@ +import { describe, it, expect } from "vitest"; +import { oauthMessageToneFor } from "../src/oauthMessageTone.js"; + +const WARNING = + "Cleared locally, but revoking the grant at the authorization server failed."; + +describe("oauthMessageToneFor", () => { + it("warns while the message it was raised for is showing", () => { + expect(oauthMessageToneFor(WARNING, WARNING)).toBe("warning"); + }); + + // The bug this replaced (#2144): a stored tone survived the message that + // earned it, so every later ordinary note rendered as a warning. + it("returns to informational as soon as any other message replaces it", () => { + expect(oauthMessageToneFor("Authorization updated.", WARNING)).toBe("info"); + }); + + it("is informational when nothing has ever warned", () => { + expect(oauthMessageToneFor("Authorization updated.", null)).toBe("info"); + }); + + // `null === null` must not read as a warning — with no message on screen + // there is nothing to colour, and the previous warning is long gone. + it("is informational when there is no message", () => { + expect(oauthMessageToneFor(null, null)).toBe("info"); + expect(oauthMessageToneFor(null, WARNING)).toBe("info"); + }); +}); diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index c1645622..59e321b3 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -65,6 +65,7 @@ import { NodeOAuthStorage, runRunnerInteractiveOAuth, } from "@inspector/core/auth/node/index.js"; +import { oauthMessageToneFor } from "./oauthMessageTone.js"; import { getTuiLogger } from "./logger.js"; import { openUrl } from "./utils/openUrl.js"; import { @@ -161,13 +162,13 @@ function App({ "idle" | "authenticating" | "error" >("idle"); const [oauthMessage, setOauthMessage] = useState(null); - // #2144: a revocation failure is a partial success — the local state really - // was cleared — so it is not an `error` status, but it must not read as an - // ordinary note either: the grant may still be live at the authorization - // server. - const [oauthMessageTone, setOauthMessageTone] = useState<"info" | "warning">( - "info", - ); + // The tone is derived from the message rather than stored beside it, so it + // cannot go stale — see `oauthMessageToneFor` for why that matters here. + // Both stay plain `useState` setters on purpose: wrapping `setOauthMessage` + // in a `useCallback` would make it a value `react-hooks/exhaustive-deps` + // demands in seven dependency arrays, for an identity that never changes. + const [oauthWarningText, setOauthWarningText] = useState(null); + const oauthMessageTone = oauthMessageToneFor(oauthMessage, oauthWarningText); const [oauthRevision, setOauthRevision] = useState(0); const [pendingStepUp, setPendingStepUp] = useState<{ serverName: string; @@ -956,12 +957,13 @@ function App({ revoke: selectedServerEntry?.settings?.oauthRevokeOnClear !== false, }); setOauthStatus("idle"); - setOauthMessageTone(revocation.status === "failed" ? "warning" : "info"); - setOauthMessage( - revocation.status === "failed" - ? `Cleared locally, but revoking the grant at the authorization server failed: ${revocation.detail}. It may still be valid there.` - : null, - ); + 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.`; + setOauthWarningText(warning); + setOauthMessage(warning); + } else { + setOauthMessage(null); + } setConnectError(null); if (inspectorStatus === "connected" || inspectorStatus === "connecting") { await disconnectInspector(); diff --git a/clients/tui/src/oauthMessageTone.ts b/clients/tui/src/oauthMessageTone.ts new file mode 100644 index 00000000..447cbf3e --- /dev/null +++ b/clients/tui/src/oauthMessageTone.ts @@ -0,0 +1,29 @@ +/** + * How the Auth tab should colour the OAuth note it is currently showing. + * + * The tone is **derived** from the message rather than stored beside it + * (#2144). `App.tsx` sets an ordinary OAuth note in around thirty places, none + * of which would think to reset a tone, so a second piece of state went stale + * the moment one message raised it: a single revocation failure left every + * later "Authorization updated" rendering as a warning, including after + * switching servers. + * + * Comparing the shown message against the text that was raised as a warning + * makes the tone a function of what is on screen, so it cannot outlive it — + * the next `setOauthMessage` of anything else is informational by + * construction, with nothing to remember to clear. + * + * `warning` is for a *partial* success: the OAuth 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 would understate that. + */ +export type OAuthMessageTone = "info" | "warning"; + +export function oauthMessageToneFor( + message: string | null, + warningText: string | null, +): OAuthMessageTone { + // `null === null` must not read as a warning: with no message there is + // nothing to colour. + return message !== null && message === warningText ? "warning" : "info"; +} diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index c3bbaa0c..d55e4627 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -1342,6 +1342,32 @@ describe("useOAuthRecovery", () => { ).toBeDefined(); }); + // #2144 — this is the web client's production wiring for revocation. + // Without asserting the arguments, removing the per-server opt-out or + // handing it the page-origin fetch would leave every test green. + it("passes the per-server revoke setting and the proxied fetch", async () => { + const h = harness({ servers: [entry("a")], activeServerId: "a" }); + await act(async () => { + await h.api().clearServerOAuthAndDisconnect({ + ...entry("a"), + settings: { ...EMPTY_SETTINGS, oauthRevokeOnClear: false }, + }); + }); + expect(clearServerOAuthStateMock).toHaveBeenCalledWith( + expect.objectContaining({ revoke: false, fetchFn: remoteFetchMock }), + ); + }); + + it("defaults to revoking when the server did not opt out", async () => { + const h = harness({ servers: [entry("a")], activeServerId: "a" }); + await act(async () => { + await h.api().clearServerOAuthAndDisconnect(entry("a")); + }); + expect(clearServerOAuthStateMock).toHaveBeenCalledWith( + expect.objectContaining({ revoke: true }), + ); + }); + it("clears the resume snapshot on an explicit disconnect", () => { writeOAuthResumeSnapshot({ version: 1, diff --git a/clients/web/src/test/core/auth/connection-state.test.ts b/clients/web/src/test/core/auth/connection-state.test.ts index 34165d27..5e9f176e 100644 --- a/clients/web/src/test/core/auth/connection-state.test.ts +++ b/clients/web/src/test/core/auth/connection-state.test.ts @@ -58,6 +58,7 @@ function createStorage( clearIdpSession: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), listIssuers: vi.fn().mockResolvedValue([]), + getIssuerTokens: vi.fn().mockResolvedValue(undefined), getCodeVerifier: vi.fn(), getDiscoveryState: vi.fn().mockResolvedValue(undefined), saveDiscoveryState: vi.fn(), diff --git a/clients/web/src/test/core/auth/ema/emaFlow.test.ts b/clients/web/src/test/core/auth/ema/emaFlow.test.ts index 2acc3254..ca8eb0c6 100644 --- a/clients/web/src/test/core/auth/ema/emaFlow.test.ts +++ b/clients/web/src/test/core/auth/ema/emaFlow.test.ts @@ -75,6 +75,7 @@ function createMemoryStorage( clear: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), listIssuers: vi.fn().mockResolvedValue([]), + getIssuerTokens: vi.fn().mockResolvedValue(undefined), } as unknown as OAuthStorage; } diff --git a/clients/web/src/test/core/auth/ema/idpSession.test.ts b/clients/web/src/test/core/auth/ema/idpSession.test.ts index 94684ed9..ec72ea83 100644 --- a/clients/web/src/test/core/auth/ema/idpSession.test.ts +++ b/clients/web/src/test/core/auth/ema/idpSession.test.ts @@ -26,6 +26,7 @@ describe("idpSession", () => { clear: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), listIssuers: vi.fn().mockResolvedValue([]), + getIssuerTokens: vi.fn().mockResolvedValue(undefined), } as unknown as OAuthStorage; }); diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index 2d01fadd..efa507ae 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -435,6 +435,42 @@ describe("revokeStoredOAuthTokens", () => { ).toBe("r2"); }); + // `getTokens(url, issuer)` falls back to the legacy unkeyed slot when that + // issuer holds none. Treating the fallback as the issuer's would label an + // old, unbound token with a newly discovered authorization server and send it + // there — so an enumerated issuer is read EXACTLY. + it("does not attribute the legacy unkeyed token to an enumerated issuer", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as.example.com" }), + ); + // A legacy, unbound grant... + await storage.saveTokens(SERVER_URL, { + access_token: "legacy-a", + token_type: "Bearer", + refresh_token: "legacy-r", + }); + // ...and an issuer slot carrying only client information, which is the + // shape a partially-migrated flow leaves behind. + await storage.saveClientInformation( + SERVER_URL, + { client_id: "cid" }, + { registrationKind: "dcr", issuer: "https://as.example.com" }, + ); + + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + await revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }); + + // Exactly one request — the legacy grant, read ctx-lessly and unlabelled. + // Never two, and never the legacy token presented as that issuer's. + expect(fetchFn).toHaveBeenCalledTimes(1); + expect( + new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"), + ).toBe("legacy-r"); + }); + // 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/clients/web/src/test/core/auth/storage-browser.test.ts b/clients/web/src/test/core/auth/storage-browser.test.ts index 60344a26..65fe2fa7 100644 --- a/clients/web/src/test/core/auth/storage-browser.test.ts +++ b/clients/web/src/test/core/auth/storage-browser.test.ts @@ -396,6 +396,41 @@ describe("BrowserOAuthStorage", () => { }); }); + // The no-fallback read #2144 needs: `getTokens(url, issuer)` deliberately + // falls back to the legacy unkeyed slot, which is right for a connect and + // wrong for anything enumerating issuers. + describe("getIssuerTokens", () => { + it("returns only the tokens bound to that issuer", async () => { + await storage.saveTokens( + testServerUrl, + { access_token: "a", token_type: "Bearer" }, + { issuer: "https://as-a.example.com" }, + ); + const tokens = await storage.getIssuerTokens( + testServerUrl, + "https://as-a.example.com", + ); + expect(tokens?.access_token).toBe("a"); + }); + + it("does not fall back to the legacy unkeyed token", async () => { + await storage.saveTokens(testServerUrl, { + access_token: "legacy", + token_type: "Bearer", + }); + // `getTokens` DOES fall back — that contrast is the point of the method. + expect((await storage.getTokens(testServerUrl))?.access_token).toBe( + "legacy", + ); + expect( + await storage.getIssuerTokens( + testServerUrl, + "https://as-a.example.com", + ), + ).toBeUndefined(); + }); + }); + describe("clearServerState", () => { it("should clear all state for a server", async () => { const clientInfo: OAuthClientInformation = { diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index ac89378a..0640784d 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -68,6 +68,7 @@ function createMockParams( clearIdpSession: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), listIssuers: vi.fn().mockResolvedValue([]), + getIssuerTokens: vi.fn().mockResolvedValue(undefined), getDiscoveryState: vi.fn().mockResolvedValue(undefined), saveDiscoveryState: vi.fn().mockResolvedValue(undefined), clearDiscoveryState: vi.fn().mockResolvedValue(undefined), diff --git a/core/auth/oauth-storage.ts b/core/auth/oauth-storage.ts index 3ab6aab5..accbed2e 100644 --- a/core/auth/oauth-storage.ts +++ b/core/auth/oauth-storage.ts @@ -424,6 +424,17 @@ export class OAuthStorageBase implements OAuthStorage { await this.persist(); } + async getIssuerTokens( + serverUrl: string, + issuer: string, + ): Promise { + await this.ensureLoaded(); + const state = this.memory.getState().getServerState(serverUrl); + const tokens = state.byIssuer?.[issuer]?.tokens; + if (!tokens) return undefined; + return withIssuer(await OAuthTokensSchema.parseAsync(tokens), issuer); + } + async listIssuers(serverUrl: string): Promise { await this.ensureLoaded(); const state = this.memory.getState().getServerState(serverUrl); diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index cee59524..8755efb2 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -357,7 +357,9 @@ interface StoredGrant { * * The ctx-less read is included last and deduped: on an issuer-bound entry it * returns the active issuer's token, which the loop above has already seen; on - * a legacy entry it is the only thing that returns anything at all. + * a legacy entry it is the only thing that returns anything at all. 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 it. */ async function collectGrants( storage: OAuthStorage, @@ -367,9 +369,15 @@ async function collectGrants( const seen = new Set(); const add = async (issuer?: string): Promise => { - const revocable = selectRevocableToken( - await storage.getTokens(serverUrl, issuer), - ); + // Exact read for an enumerated issuer. `getTokens(serverUrl, issuer)` falls + // back to the legacy unkeyed slot when that issuer holds none, and treating + // the fallback as the issuer's would label an old, unbound token with a + // newly discovered authorization server and send it there. + const tokens = + issuer === undefined + ? await storage.getTokens(serverUrl) + : await storage.getIssuerTokens(serverUrl, issuer); + const revocable = selectRevocableToken(tokens); if (!revocable || seen.has(revocable.token)) return; seen.add(revocable.token); grants.push({ diff --git a/core/auth/storage.ts b/core/auth/storage.ts index 6973b6c8..73a46c35 100644 --- a/core/auth/storage.ts +++ b/core/auth/storage.ts @@ -182,6 +182,21 @@ export interface OAuthStorage { */ clearDiscoveryState(serverUrl: string): Promise; + /** + * Tokens bound to **exactly** `issuer`, with no legacy-unkeyed fallback. + * + * {@link getTokens} deliberately falls back to the legacy unkeyed slot when + * the issuer slot holds none — that is what keeps a pre-SEP-2352 entry + * working. But a caller enumerating {@link listIssuers} must not treat that + * fallback as belonging to the issuer it happened to ask for: during a + * partially migrated flow it would label an old, unbound token with a newly + * discovered authorization server and send it there (#2144). + */ + getIssuerTokens( + serverUrl: string, + issuer: string, + ): Promise; + /** * The authorization-server `issuer` keys holding credentials for this server * (SEP-2352). Empty when the entry predates issuer binding — its credentials