fix: use Node storage in the revocation e2e, and drop the now-dead storage API (#2144)

The CI coverage job failed with `ReferenceError: sessionStorage is not defined`.
`revocation-e2e.test.ts` used `BrowserOAuthStorage`, but the `integration`
vitest project runs in the node environment, not happy-dom. Node 22 gates Web
Storage behind a flag while newer Node exposes it by default, so it passed
locally and failed in CI. It uses `NodeOAuthStorage` with a temp dir now,
matching the other OAuth integration tests, with a comment naming the trap.

Also from Copilot review round 21: `listIssuers` and `getIssuerTokens` became
dead production API when `takeRevocationSnapshot` took over — a grep finds them
only in their own implementations and tests. Keeping them would widen the
exported `OAuthStorage` contract and force every implementation and mock to
carry methods nothing calls. Removed, along with their tests and the mock
entries in four suites.

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/web/src/test/core/auth/connection-state.test.ts

@@ -57,8 +57,6 @@ function createStorage(
     clearServerMetadata: vi.fn(),
     clearIdpSession: vi.fn(),
     clearEnterpriseManagedResourceServers: vi.fn(),
-    listIssuers: vi.fn().mockResolvedValue([]),
-    getIssuerTokens: vi.fn().mockResolvedValue(undefined),
     takeRevocationSnapshot: vi.fn().mockResolvedValue({ byIssuer: {} }),
     getCodeVerifier: vi.fn(),
     getDiscoveryState: vi.fn().mockResolvedValue(undefined),

clients/web/src/test/core/auth/ema/emaFlow.test.ts

@@ -74,8 +74,6 @@ function createMemoryStorage(
     saveScope: vi.fn(),
     clear: vi.fn(),
     clearEnterpriseManagedResourceServers: vi.fn(),
-    listIssuers: vi.fn().mockResolvedValue([]),
-    getIssuerTokens: vi.fn().mockResolvedValue(undefined),
     takeRevocationSnapshot: vi.fn().mockResolvedValue({ byIssuer: {} }),
   } as unknown as OAuthStorage;
 }

clients/web/src/test/core/auth/ema/idpSession.test.ts

@@ -25,8 +25,6 @@ describe("idpSession", () => {
       clearIdpSession: vi.fn(),
       clear: vi.fn(),
       clearEnterpriseManagedResourceServers: vi.fn(),
-      listIssuers: vi.fn().mockResolvedValue([]),
-      getIssuerTokens: vi.fn().mockResolvedValue(undefined),
       takeRevocationSnapshot: vi.fn().mockResolvedValue({ byIssuer: {} }),
     } as unknown as OAuthStorage;
   });

clients/web/src/test/core/auth/storage-browser.test.ts

@@ -358,79 +358,6 @@ describe("BrowserOAuthStorage", () => {
     });
   });
 
-  // #2144 — `clear` deletes every issuer slot, so anything that must act on the
-  // credentials first (RFC 7009 revocation) needs to see them all.
-  describe("listIssuers", () => {
-    it("returns every issuer holding credentials for the server", async () => {
-      await storage.saveTokens(
-        testServerUrl,
-        { access_token: "a", token_type: "Bearer" },
-        { issuer: "https://as-a.example.com" },
-      );
-      await storage.saveTokens(
-        testServerUrl,
-        { access_token: "b", token_type: "Bearer" },
-        { issuer: "https://as-b.example.com" },
-      );
-
-      expect((await storage.listIssuers(testServerUrl)).sort()).toEqual([
-        "https://as-a.example.com",
-        "https://as-b.example.com",
-      ]);
-    });
-
-    // A pre-SEP-2352 entry has its credentials in the legacy unkeyed slot,
-    // which the context-free reads answer — there is no issuer to list.
-    it("is empty for an entry with no issuer-bound credentials", async () => {
-      await storage.saveTokens(testServerUrl, {
-        access_token: "a",
-        token_type: "Bearer",
-      });
-      expect(await storage.listIssuers(testServerUrl)).toEqual([]);
-    });
-
-    it("is empty for a server with no state at all", async () => {
-      expect(await storage.listIssuers("https://unknown.example/mcp")).toEqual(
-        [],
-      );
-    });
-  });
-
-  // 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();
-    });
-  });
-
   // #2144 — the atomic take-and-clear. Split reads followed by a separate clear
   // are a check-then-act: each await yields, and an OAuth completion landing in
   // one of those gaps saves a grant the clear then destroys.
@@ -465,7 +392,9 @@ describe("BrowserOAuthStorage", () => {
       // Cleared in the same step.
       expect(await storage.getTokens(testServerUrl)).toBeUndefined();
       expect(await storage.getServerMetadata(testServerUrl)).toBeNull();
-      expect(await storage.listIssuers(testServerUrl)).toEqual([]);
+      expect(
+        (await storage.takeRevocationSnapshot(testServerUrl)).byIssuer,
+      ).toEqual({});
     });
 
     it("returns the legacy unkeyed slot too", async () => {

clients/web/src/test/core/mcp/oauthManager.test.ts

@@ -67,8 +67,6 @@ function createMockParams(
     saveIdpSession: vi.fn().mockResolvedValue(undefined),
     clearIdpSession: vi.fn(),
     clearEnterpriseManagedResourceServers: vi.fn(),
-    listIssuers: vi.fn().mockResolvedValue([]),
-    getIssuerTokens: vi.fn().mockResolvedValue(undefined),
     takeRevocationSnapshot: vi.fn().mockResolvedValue({ byIssuer: {} }),
     getDiscoveryState: vi.fn().mockResolvedValue(undefined),
     saveDiscoveryState: vi.fn().mockResolvedValue(undefined),

clients/web/src/test/integration/auth/revocation-e2e.test.ts

@@ -11,13 +11,21 @@
  */
 import { describe, it, expect, beforeAll, afterAll } from "vitest";
 import { createHash, randomBytes } from "node:crypto";
+import { mkdtempSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
 import {
   TestServerHttp,
   createOAuthTestServerConfig,
   getDefaultServerConfig,
   waitForOAuthWellKnown,
 } from "@modelcontextprotocol/inspector-test-server";
-import { BrowserOAuthStorage } from "@inspector/core/auth/browser/storage.js";
+// NOT `BrowserOAuthStorage`: the `integration` project runs in the node
+// environment, where `sessionStorage` does not exist. Node 22 gates Web Storage
+// behind a flag and newer Node exposes it by default, so a browser-backed store
+// passes on a developer's machine and fails in CI — which is exactly what
+// happened here.
+import { NodeOAuthStorage } from "@inspector/core/auth/node/storage-node.js";
 import {
   clearAndPlanRevocation,
   executeOAuthRevocation,
@@ -43,6 +51,7 @@ function base64Url(buffer: Buffer): string {
 describe("OAuth token revocation (RFC 7009)", () => {
   let mcpServer: TestServerHttp | null = null;
   let serverUrl = "";
+  let storageDir = "";
   let metadata: OAuthMetadata;
 
   beforeAll(async () => {
@@ -67,6 +76,7 @@ describe("OAuth token revocation (RFC 7009)", () => {
     });
     const port = await mcpServer.start();
     serverUrl = `http://localhost:${port}`;
+    storageDir = mkdtempSync(join(tmpdir(), "mcp-inspector-2144-"));
     await waitForOAuthWellKnown(serverUrl);
     metadata = (await (
       await fetch(`${serverUrl}/.well-known/oauth-authorization-server`)
@@ -76,6 +86,10 @@ describe("OAuth token revocation (RFC 7009)", () => {
   afterAll(async () => {
     await mcpServer?.stop();
     mcpServer = null;
+    if (storageDir) {
+      rmSync(storageDir, { recursive: true, force: true });
+      storageDir = "";
+    }
   }, 30_000);
 
   /** Run a real authorization-code exchange and return the issued tokens. */
@@ -135,8 +149,8 @@ describe("OAuth token revocation (RFC 7009)", () => {
   async function seededStorage(tokens: {
     access_token: string;
     refresh_token?: string;
-  }): Promise<BrowserOAuthStorage> {
-    const storage = new BrowserOAuthStorage();
+  }): Promise<NodeOAuthStorage> {
+    const storage = new NodeOAuthStorage(join(storageDir, "oauth.json"));
     await storage.clear(serverUrl);
     // Issuer-bound and matching the discovered metadata: an unkeyed grant
     // records no authorization server and is deliberately refused.
@@ -207,7 +221,7 @@ describe("OAuth token revocation (RFC 7009)", () => {
    * exercises the ordering the product uses rather than a convenience wrapper.
    */
   async function clearAndRevoke(
-    storage: BrowserOAuthStorage,
+    storage: NodeOAuthStorage,
   ): Promise<TokenRevocationOutcome> {
     const plan = await clearAndPlanRevocation({ serverUrl, storage });
     return executeOAuthRevocation(plan, { fetchFn: fetch });

core/auth/oauth-storage.ts

@@ -447,23 +447,6 @@ export class OAuthStorageBase implements OAuthStorage {
     return snapshot;
   }
 
-  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);
-    return Object.keys(state.byIssuer ?? {});
-  }
-
   async clear(serverUrl: string): Promise<void> {
     await this.ensureLoaded();
     this.memory.getState().clearServerState(serverUrl);

core/auth/storage.ts

@@ -223,33 +223,6 @@ export interface OAuthStorage {
    */
   takeRevocationSnapshot(serverUrl: string): Promise<RevocationSnapshot>;
 
-  /**
-   * 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
-   * live in the legacy unkeyed slot, which the ctx-less reads above answer.
-   *
-   * Exists because {@link clear} deletes **every** issuer slot: anything that
-   * must act on the credentials before they are dropped (RFC 7009 revocation,
-   * #2144) would otherwise see only the active issuer's and silently discard
-   * the rest.
-   */
-  listIssuers(serverUrl: string): Promise<string[]>;
-
   /**
    * Clear all OAuth data for a server
    */