From eede9c17ba07546cad09f149f47a2aa9ecf039ec Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 08:36:58 -0400 Subject: [PATCH] fix: use Node storage in the revocation e2e, and drop the now-dead storage API (#2144) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- .../test/core/auth/connection-state.test.ts | 2 - .../src/test/core/auth/ema/emaFlow.test.ts | 2 - .../src/test/core/auth/ema/idpSession.test.ts | 2 - .../test/core/auth/storage-browser.test.ts | 77 +------------------ .../src/test/core/mcp/oauthManager.test.ts | 2 - .../integration/auth/revocation-e2e.test.ts | 22 +++++- core/auth/oauth-storage.ts | 17 ---- core/auth/storage.ts | 27 ------- 8 files changed, 21 insertions(+), 130 deletions(-) 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 95cbc13a..d328b01b 100644 --- a/clients/web/src/test/core/auth/connection-state.test.ts +++ b/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), 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 2075d405..77af62b9 100644 --- a/clients/web/src/test/core/auth/ema/emaFlow.test.ts +++ b/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; } 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 fb2b419a..465be785 100644 --- a/clients/web/src/test/core/auth/ema/idpSession.test.ts +++ b/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; }); 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 3e02c1dc..89d0cf27 100644 --- a/clients/web/src/test/core/auth/storage-browser.test.ts +++ b/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 () => { diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 8cea49e9..f0ee3bc6 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/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), diff --git a/clients/web/src/test/integration/auth/revocation-e2e.test.ts b/clients/web/src/test/integration/auth/revocation-e2e.test.ts index 7f56f259..0e3b3373 100644 --- a/clients/web/src/test/integration/auth/revocation-e2e.test.ts +++ b/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 { - const storage = new BrowserOAuthStorage(); + }): Promise { + 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 { const plan = await clearAndPlanRevocation({ serverUrl, storage }); return executeOAuthRevocation(plan, { fetchFn: fetch }); diff --git a/core/auth/oauth-storage.ts b/core/auth/oauth-storage.ts index 64638c20..68b7810c 100644 --- a/core/auth/oauth-storage.ts +++ b/core/auth/oauth-storage.ts @@ -447,23 +447,6 @@ export class OAuthStorageBase implements OAuthStorage { return snapshot; } - 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); - return Object.keys(state.byIssuer ?? {}); - } - async clear(serverUrl: string): Promise { await this.ensureLoaded(); this.memory.getState().clearServerState(serverUrl); diff --git a/core/auth/storage.ts b/core/auth/storage.ts index 181872ea..edc6451e 100644 --- a/core/auth/storage.ts +++ b/core/auth/storage.ts @@ -223,33 +223,6 @@ export interface OAuthStorage { */ takeRevocationSnapshot(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 - * 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; - /** * Clear all OAuth data for a server */