fix(auth): read enumerated issuers exactly, and stop the TUI tone going stale (#2144)

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>

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");
+  });
+});

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<string | null>(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<string | null>(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();

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";
+}

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,

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(),

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;
 }
 

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;
   });
 

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<typeof fetch>(
+      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

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 = {

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),

core/auth/oauth-storage.ts

@@ -424,6 +424,17 @@ export class OAuthStorageBase implements OAuthStorage {
     await this.persist();
   }
 
+  async getIssuerTokens(
+    serverUrl: string,
+    issuer: string,
+  ): Promise<OAuthTokens | undefined> {
+    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<string[]> {
     await this.ensureLoaded();
     const state = this.memory.getState().getServerState(serverUrl);

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<string>();
 
   const add = async (issuer?: string): Promise<void> => {
-    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({

core/auth/storage.ts

@@ -182,6 +182,21 @@ export interface OAuthStorage {
    */
   clearDiscoveryState(serverUrl: string): Promise<void>;
 
+  /**
+   * 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<OAuthTokens | undefined>;
+
   /**
    * The authorization-server `issuer` keys holding credentials for this server
    * (SEP-2352). Empty when the entry predates issuer binding — its credentials