From 781aa69d19afda21e497df96e5a466a4931771ba Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 02:40:28 -0400 Subject: [PATCH] fix(auth): revoke every issuer-bound grant, not just the active one (#2144) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 3. `clear(serverUrl)` deletes EVERY `byIssuer` slot, but revocation read only the context-free (active-issuer) token. A server that had authorized against issuers A and B therefore revoked B, then destroyed the local record of A while A's grant stayed live at its authorization server — the same leak this feature exists to close, one level down. - `OAuthStorage.listIssuers(serverUrl)` is the new seam (one implementation: every backend extends `OAuthStorageBase`). `collectGrants` walks it plus the ctx-less read, deduping by token, so every grant the clear will delete is covered. - Metadata is cached once per server, not per issuer, so a grant bound to an issuer the cached document does not describe cannot be revoked: sending its token to that endpoint would hand a credential to a server that never minted it. That grant is reported as failed — dropped unrevoked — rather than silently discarded or misdirected. - `aggregateOutcomes` gives a failure precedence over another grant's success, so one live grant cannot hide behind another's 200. Exported for its own test, since `computeOutcome` returns early on the empty case. Also from that round: - The fixture's /oauth/revoke now verifies token ownership (RFC 7009 §2.1). Access tokens record their owning `client_id`; a token belonging to another client is left alone and still answered 200 (§2.2 — the response must not tell one client whether another's token exists). New e2e case proves a second registered client cannot revoke the first's grant. - The TUI rendered the revocation-failure note in cyan, the informational tone. It is a partial success — the local clear really happened — so it is not an `error` status, but the grant may still be live and cyan understated that. `AuthTab` takes an `oauthMessageTone`; the message says so explicitly. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- clients/tui/__tests__/App.test.tsx | 9 + clients/tui/__tests__/AuthTab.test.tsx | 29 ++++ clients/tui/src/App.tsx | 11 +- clients/tui/src/components/AuthTab.tsx | 13 +- .../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 | 160 ++++++++++++++++++ .../test/core/auth/storage-browser.test.ts | 38 +++++ .../src/test/core/mcp/oauthManager.test.ts | 1 + .../integration/auth/revocation-e2e.test.ts | 29 ++++ core/auth/index.ts | 1 + core/auth/oauth-storage.ts | 6 + core/auth/revocation.ts | 159 +++++++++++++---- core/auth/storage.ts | 12 ++ test-servers/src/test-server-oauth.ts | 60 ++++--- 16 files changed, 479 insertions(+), 52 deletions(-) diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index 37a60bc6..f5d56734 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -1407,9 +1407,18 @@ describe("App (mid-session auth lifecycle events)", () => { }); const r = await mount(oneHttp()); await press(r, ["a", "s"]); + // The detail is what identifies the failure; the tone the message renders + // in is asserted against `AuthTab` directly, where the branch lives. await expectFrame(r, "unreachable"); }); + it("says nothing when there was nothing to revoke", async () => { + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0); + expect(r.lastFrame() ?? "").not.toContain("authorization server failed"); + }); + const stepUpChallenge = { reason: "insufficient_scope" as const, requiredScopes: ["env:read"], diff --git a/clients/tui/__tests__/AuthTab.test.tsx b/clients/tui/__tests__/AuthTab.test.tsx index 86d6b18e..65cda2d2 100644 --- a/clients/tui/__tests__/AuthTab.test.tsx +++ b/clients/tui/__tests__/AuthTab.test.tsx @@ -100,6 +100,35 @@ describe("AuthTab", () => { ); }); + // #2144 — a revocation failure is a *partial* success: the local 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 understates + // that. Both tones are exercised so neither branch can rot. + it("renders an idle message in the warning tone when asked", () => { + const { lastFrame } = render( + , + ); + expect(lastFrame() ?? "").toContain("Cleared locally"); + }); + + it("renders an idle message in the default informational tone", () => { + const { lastFrame } = render( + , + ); + expect(lastFrame() ?? "").toContain("Stored OAuth state cleared."); + }); + it("renders OAuth details from getOAuthState", async () => { const { client } = makeClient(sampleOAuthState); const { lastFrame } = render( diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index fccff741..c1645622 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -161,6 +161,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", + ); const [oauthRevision, setOauthRevision] = useState(0); const [pendingStepUp, setPendingStepUp] = useState<{ serverName: string; @@ -949,9 +956,10 @@ 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}` + ? `Cleared locally, but revoking the grant at the authorization server failed: ${revocation.detail}. It may still be valid there.` : null, ); setConnectError(null); @@ -1756,6 +1764,7 @@ function App({ inspectorClient={selectedInspectorClient} oauthStatus={oauthStatus} oauthMessage={oauthMessage} + oauthMessageTone={oauthMessageTone} oauthRevision={oauthRevision} pendingStepUp={ pendingStepUp?.serverName === selectedServer diff --git a/clients/tui/src/components/AuthTab.tsx b/clients/tui/src/components/AuthTab.tsx index c495a4e5..21e8f2eb 100644 --- a/clients/tui/src/components/AuthTab.tsx +++ b/clients/tui/src/components/AuthTab.tsx @@ -28,6 +28,14 @@ interface AuthTabProps { inspectorClient: InspectorClient | null; oauthStatus: "idle" | "authenticating" | "error"; oauthMessage: string | null; + /** + * How to colour {@link oauthMessage} on the `idle` status. Defaults to + * `"info"` (cyan). `"warning"` exists for the one message that reports a + * *partial* success: the OAuth state really was cleared, so this is not an + * error, but the grant may still be live at the authorization server and + * cyan would understate that (#2144). + */ + oauthMessageTone?: "info" | "warning"; oauthRevision: number; pendingStepUp?: { challenge: AuthChallenge; @@ -57,6 +65,7 @@ export function AuthTab({ inspectorClient, oauthStatus, oauthMessage, + oauthMessageTone = "info", oauthRevision, pendingStepUp, onAuthorizeStepUp, @@ -190,7 +199,9 @@ export function AuthTab({ {oauthMessage} )} {oauthStatus === "idle" && oauthMessage && ( - {oauthMessage} + + {oauthMessage} + )} {pendingStepUp ? ( 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 f8235d4a..34165d27 100644 --- a/clients/web/src/test/core/auth/connection-state.test.ts +++ b/clients/web/src/test/core/auth/connection-state.test.ts @@ -57,6 +57,7 @@ function createStorage( clearServerMetadata: vi.fn(), clearIdpSession: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), + listIssuers: vi.fn().mockResolvedValue([]), 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 e5e24e54..2acc3254 100644 --- a/clients/web/src/test/core/auth/ema/emaFlow.test.ts +++ b/clients/web/src/test/core/auth/ema/emaFlow.test.ts @@ -74,6 +74,7 @@ function createMemoryStorage( saveScope: vi.fn(), clear: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), + listIssuers: vi.fn().mockResolvedValue([]), } 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 11ef63e7..94684ed9 100644 --- a/clients/web/src/test/core/auth/ema/idpSession.test.ts +++ b/clients/web/src/test/core/auth/ema/idpSession.test.ts @@ -25,6 +25,7 @@ describe("idpSession", () => { clearIdpSession: vi.fn(), clear: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), + listIssuers: vi.fn().mockResolvedValue([]), } 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 22585119..2d01fadd 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -3,6 +3,7 @@ import type { OAuthMetadata } from "@modelcontextprotocol/client"; import { BrowserOAuthStorage } from "@inspector/core/auth/browser/storage.js"; import { DEFAULT_REVOCATION_TIMEOUT_MS, + aggregateOutcomes, buildRevocationRequest, revocationAuthMethods, revokeStoredOAuthTokens, @@ -131,6 +132,40 @@ describe("revocationAuthMethods", () => { }); }); +describe("aggregateOutcomes", () => { + const failed = { status: "failed", detail: "boom" } as const; + const revoked = { + status: "revoked", + tokenTypeHint: "refresh_token", + endpoint: REVOKE_URL, + } as const; + const skipped = { status: "skipped", reason: "no_endpoint" } as const; + + // A grant still live at the authorization server is the thing worth + // surfacing; another grant's success would silence it. + it("reports a failure over a success", () => { + expect(aggregateOutcomes([revoked, failed])).toEqual(failed); + }); + + it("reports a success over a skip", () => { + expect(aggregateOutcomes([skipped, revoked])).toEqual(revoked); + }); + + it("falls through to the first outcome when none is decisive", () => { + expect(aggregateOutcomes([skipped])).toEqual(skipped); + }); + + // `computeOutcome` returns early when there are no grants, so nothing else + // reaches this — which is why it is tested here rather than left as a + // function that returns `undefined` while typed otherwise. + it("has an answer for an empty list", () => { + expect(aggregateOutcomes([])).toEqual({ + status: "skipped", + reason: "no_tokens", + }); + }); +}); + describe("buildRevocationRequest", () => { it("posts a form-encoded token and hint", () => { const { url, init } = buildRevocationRequest({ @@ -229,6 +264,23 @@ describe("revokeToken", () => { expect(outcome.status === "failed" ? outcome.detail : "").toContain("401"); }); + // `String(err)` is the other half of the detail: a fetch double, or a runtime + // that rejects with a non-Error, must still produce a readable message. + it("reports a non-Error rejection as failed", async () => { + const outcome = await revokeToken({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + supportedAuthMethods: [], + fetchFn: () => Promise.reject("plain string"), + }); + expect(outcome).toEqual({ + status: "failed", + endpoint: REVOKE_URL, + detail: "plain string", + }); + }); + it("reports a network failure as failed", async () => { const fetchFn = vi.fn(async () => { throw new Error("connect ECONNREFUSED"); @@ -351,6 +403,102 @@ describe("revokeStoredOAuthTokens", () => { ); }); + // `clear(serverUrl)` drops EVERY `byIssuer` slot, so reading only the active + // issuer's token would leave the earlier authorization server's grant live + // while destroying the local record of it — the same leak this feature + // closes, one level down (SEP-2352 keeps credentials per issuer). + it("revokes every issuer-bound grant, not just the active one", async () => { + const issuerMetadata = metadata({ issuer: "https://as.example.com" }); + await storage.saveServerMetadata(SERVER_URL, issuerMetadata); + await storage.saveTokens( + SERVER_URL, + { access_token: "a1", token_type: "Bearer", refresh_token: "r1" }, + { issuer: "https://as.example.com" }, + ); + await storage.saveTokens( + SERVER_URL, + { access_token: "a2", token_type: "Bearer", refresh_token: "r2" }, + { issuer: "https://as.example.com" }, + ); + // A second slot under the SAME issuer would be one grant; use two distinct + // tokens under one issuer plus the ctx-less read to prove dedup instead. + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + await revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }); + + // One issuer, one grant — the ctx-less read is the same token and is deduped. + expect(fetchFn).toHaveBeenCalledTimes(1); + expect( + new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"), + ).toBe("r2"); + }); + + // 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 + // never minted it. Saying so is the point: the grant is being dropped. + it("reports a grant whose issuer the cached metadata does not describe", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as-b.example.com" }), + ); + await storage.saveTokens( + SERVER_URL, + { access_token: "a", token_type: "Bearer", refresh_token: "r-a" }, + { issuer: "https://as-a.example.com" }, + ); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + 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( + "as-a.example.com", + ); + }); + + // A failure on one grant must not be hidden behind another's success — a + // grant still live at the authorization server is the thing worth surfacing. + it("reports a failure over another grant's success", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as-b.example.com" }), + ); + // Revocable: bound to the issuer the metadata describes. + await storage.saveTokens( + SERVER_URL, + { access_token: "b", token_type: "Bearer", refresh_token: "r-b" }, + { issuer: "https://as-b.example.com" }, + ); + // Not revocable: bound to an issuer the cached metadata is not for. + await storage.saveTokens( + SERVER_URL, + { access_token: "a", token_type: "Bearer", refresh_token: "r-a" }, + { issuer: "https://as-a.example.com" }, + ); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(outcome).toMatchObject({ status: "failed" }); + }); + // The whole point of keeping this path opt-out-able: an authorization server // with no RFC 7009 support must behave exactly as it did before the feature. it("does nothing when the authorization server advertises no revocation endpoint", async () => { @@ -430,6 +578,18 @@ describe("revokeStoredOAuthTokens", () => { // asked for, so the read is inside the try. Spying on the real instance keeps // the `OAuthStorage` contract intact — a spread-and-cast stand-in would type // as storage while being a plain object with none of its methods. + it("reports a non-Error store failure as failed", async () => { + vi.spyOn(storage, "listIssuers").mockRejectedValue("store exploded"); + + await expect( + revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn: vi.fn(), + }), + ).resolves.toEqual({ status: "failed", detail: "store exploded" }); + }); + it("reports a store read failure as failed", async () => { vi.spyOn(storage, "getTokens").mockRejectedValue( new Error("store unreadable"), 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 8fb99cb1..60344a26 100644 --- a/clients/web/src/test/core/auth/storage-browser.test.ts +++ b/clients/web/src/test/core/auth/storage-browser.test.ts @@ -358,6 +358,44 @@ 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( + [], + ); + }); + }); + 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 e9a32541..ac89378a 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -67,6 +67,7 @@ function createMockParams( saveIdpSession: vi.fn().mockResolvedValue(undefined), clearIdpSession: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), + listIssuers: vi.fn().mockResolvedValue([]), getDiscoveryState: vi.fn().mockResolvedValue(undefined), saveDiscoveryState: vi.fn().mockResolvedValue(undefined), clearDiscoveryState: 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 ca1cf41f..3b637e1c 100644 --- a/clients/web/src/test/integration/auth/revocation-e2e.test.ts +++ b/clients/web/src/test/integration/auth/revocation-e2e.test.ts @@ -24,6 +24,9 @@ import type { OAuthMetadata } from "@modelcontextprotocol/client"; const CLIENT_ID = "test-2144-revocation"; const CLIENT_SECRET = "test-2144-secret"; const REDIRECT_URL = "http://localhost:3000/oauth/callback"; +/** A second registered client, used to prove tokens are not cross-revocable. */ +const OTHER_CLIENT_ID = "test-2144-other"; +const OTHER_CLIENT_SECRET = "test-2144-other-secret"; function base64Url(buffer: Buffer): string { return buffer @@ -50,6 +53,11 @@ describe("OAuth token revocation (RFC 7009)", () => { clientSecret: CLIENT_SECRET, redirectUris: [REDIRECT_URL], }, + { + clientId: OTHER_CLIENT_ID, + clientSecret: OTHER_CLIENT_SECRET, + redirectUris: [REDIRECT_URL], + }, ], }), }); @@ -163,6 +171,27 @@ describe("OAuth token revocation (RFC 7009)", () => { expect(response.status).toBe(401); }); + // RFC 7009 §2.1: only the client a token was issued to may revoke it. The + // response stays 200 (§2.2 — it must not tell one client whether another's + // token exists), so the assertion is that the token still works. + it("does not revoke a token belonging to a different client", async () => { + const tokens = await authorize(); + const response = await fetch(`${serverUrl}/oauth/revoke`, { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + Authorization: `Basic ${Buffer.from(`${OTHER_CLIENT_ID}:${OTHER_CLIENT_SECRET}`).toString("base64")}`, + }, + body: new URLSearchParams({ + token: tokens.refresh_token, + token_type_hint: "refresh_token", + }), + }); + + expect(response.status).toBe(200); + expect(await tokenAccepted(tokens.access_token)).toBe(true); + }); + it("advertises a revocation endpoint", () => { expect(metadata.revocation_endpoint).toBe(`${serverUrl}/oauth/revoke`); }); diff --git a/core/auth/index.ts b/core/auth/index.ts index 3535ac83..8c3e8c10 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -137,6 +137,7 @@ export { discoverScopes } from "./discovery.js"; // RFC 7009 token revocation (#2144) export { DEFAULT_REVOCATION_TIMEOUT_MS, + aggregateOutcomes, buildRevocationRequest, revocationAuthMethods, revokeStoredOAuthTokens, diff --git a/core/auth/oauth-storage.ts b/core/auth/oauth-storage.ts index f68baa64..3ab6aab5 100644 --- a/core/auth/oauth-storage.ts +++ b/core/auth/oauth-storage.ts @@ -424,6 +424,12 @@ export class OAuthStorageBase implements OAuthStorage { await this.persist(); } + 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/revocation.ts b/core/auth/revocation.ts index 92c5f234..cee59524 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -256,6 +256,9 @@ async function withDeadline( try { return await Promise.race([promise, deadline]); } finally { + /* v8 ignore next -- the Promise executor runs synchronously, so `timer` is + always assigned by the time this runs; the guard exists only because + TypeScript cannot see that. */ if (timer !== undefined) clearTimeout(timer); } } @@ -279,8 +282,10 @@ export interface RevokeStoredOAuthTokensParams { * needs out of the OAuth store. * * Called immediately **before** the local clear, since the store is where the - * token, the client credentials, and the discovered `revocation_endpoint` all - * live — after the clear there is nothing left to revoke with. + * tokens, the client credentials, and the discovered `revocation_endpoint` all + * live — after the clear there is nothing left to revoke with. Every grant the + * clear will delete is covered, not just the active issuer's: see + * {@link collectGrants}. * * The metadata comes from the cache the OAuth flow already populated rather * than from a fresh discovery round-trip: the tokens being revoked were minted @@ -316,19 +321,96 @@ export async function revokeStoredOAuthTokens( * The client credentials to authenticate the revocation request with. * * Mirrors `BaseOAuthClientProvider.clientInformation`: the preregistered - * (static, issuer-independent) entry wins, then the per-issuer dynamic - * registration. Reading only the second would silently drop client - * authentication for every server configured with an `oauth.clientId` — the - * confidential case, where an authorization server is most likely to *require* - * it and answer 401. + * (static, issuer-independent) entry wins, then the registration bound to + * `issuer`. Reading only the second would silently drop client authentication + * for every server configured with an `oauth.clientId` — the confidential case, + * where an authorization server is most likely to *require* it and answer 401. */ async function resolveClientInformation( storage: OAuthStorage, serverUrl: string, + issuer?: string, ): Promise { return ( (await storage.getClientInformation(serverUrl, true)) ?? - (await storage.getClientInformation(serverUrl, false)) + (await storage.getClientInformation(serverUrl, false, issuer)) + ); +} + +/** One revocable grant held for a server, and which AS minted it. */ +interface StoredGrant { + /** Undefined for the legacy unkeyed slot, which predates issuer binding. */ + issuer?: string; + token: string; + tokenTypeHint: "refresh_token" | "access_token"; + clientInformation?: OAuthClientInformation; +} + +/** + * Every grant `clear(serverUrl)` is about to delete, deduplicated by token. + * + * `clear` drops **every** `byIssuer` slot, so reading only the context-free + * (active-issuer) token would leave an earlier authorization server's grant + * live while destroying the local record of it — the exact leak this feature + * exists to close, just moved one level down. A server that authorized against + * issuers A and B has two grants here, not one. + * + * 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. + */ +async function collectGrants( + storage: OAuthStorage, + serverUrl: string, +): Promise { + const grants: StoredGrant[] = []; + const seen = new Set(); + + const add = async (issuer?: string): Promise => { + const revocable = selectRevocableToken( + await storage.getTokens(serverUrl, issuer), + ); + if (!revocable || seen.has(revocable.token)) return; + seen.add(revocable.token); + grants.push({ + issuer, + ...revocable, + clientInformation: await resolveClientInformation( + storage, + serverUrl, + issuer, + ), + }); + }; + + for (const issuer of await storage.listIssuers(serverUrl)) { + await add(issuer); + } + await add(); + return grants; +} + +/** + * Combine per-grant outcomes into the one this function reports. + * + * A **failure outranks a success**: a grant still live at the authorization + * server is what the caller needs to surface, and reporting another grant's + * success would leave that silent. A success outranks a skip for the same + * reason in the other direction — "revoked" is the more specific truth. + * + * Exported for its own test. `computeOutcome` returns early when there are no + * grants, so the empty case cannot arise from there — which is exactly why the + * fallback needs testing somewhere: nothing else would ever exercise it, and a + * function that returns `undefined` while typed otherwise is a trap for the + * next caller. + */ +export function aggregateOutcomes( + outcomes: TokenRevocationOutcome[], +): TokenRevocationOutcome { + return ( + outcomes.find((o) => o.status === "failed") ?? + outcomes.find((o) => o.status === "revoked") ?? + outcomes[0] ?? { status: "skipped", reason: "no_tokens" } ); } @@ -337,32 +419,49 @@ async function computeOutcome( ): Promise { const { serverUrl, storage, fetchFn } = params; - // Read the token first. It is the cheapest disqualifier, and a server with no - // stored grant should not provoke a metadata read at all. - let tokens: OAuthTokens | undefined; - let metadata: OAuthMetadata | null; - let clientInformation: OAuthClientInformation | undefined; try { - tokens = await storage.getTokens(serverUrl); - const revocable = selectRevocableToken(tokens); - if (!revocable) return { status: "skipped", reason: "no_tokens" }; + // Grants first. They are the cheapest disqualifier, and a server with no + // stored grant should not provoke a metadata read at all. + const grants = await collectGrants(storage, serverUrl); + if (grants.length === 0) return { status: "skipped", reason: "no_tokens" }; - metadata = await storage.getServerMetadata(serverUrl); + const metadata = await storage.getServerMetadata(serverUrl); if (!metadata) return { status: "skipped", reason: "no_metadata" }; - if (!metadata.revocation_endpoint) { - return { status: "skipped", reason: "no_endpoint" }; - } + const endpoint = metadata.revocation_endpoint; + if (!endpoint) return { status: "skipped", reason: "no_endpoint" }; - clientInformation = await resolveClientInformation(storage, serverUrl); - return await revokeToken({ - endpoint: metadata.revocation_endpoint, - token: revocable.token, - tokenTypeHint: revocable.tokenTypeHint, - clientInformation, - supportedAuthMethods: revocationAuthMethods(metadata), - fetchFn, - timeoutMs: params.timeoutMs, - }); + const supportedAuthMethods = revocationAuthMethods(metadata); + const outcomes: TokenRevocationOutcome[] = []; + 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 + ) { + 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`, + }); + continue; + } + outcomes.push( + await revokeToken({ + endpoint, + token: grant.token, + tokenTypeHint: grant.tokenTypeHint, + clientInformation: grant.clientInformation, + supportedAuthMethods, + fetchFn, + timeoutMs: params.timeoutMs, + }), + ); + } + return aggregateOutcomes(outcomes); } catch (err) { // A store that cannot be read (a corrupt blob, a remote backend that 500s) // is not a reason to abandon the clear the user asked for. diff --git a/core/auth/storage.ts b/core/auth/storage.ts index 90163c19..6973b6c8 100644 --- a/core/auth/storage.ts +++ b/core/auth/storage.ts @@ -182,6 +182,18 @@ export interface OAuthStorage { */ clearDiscoveryState(serverUrl: 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 */ diff --git a/test-servers/src/test-server-oauth.ts b/test-servers/src/test-server-oauth.ts index 90d4fae1..4024ef65 100644 --- a/test-servers/src/test-server-oauth.ts +++ b/test-servers/src/test-server-oauth.ts @@ -636,7 +636,7 @@ function setupTokenEndpoint( // Generate access token const tokenScope = authCodeData.scope || config.scopesSupported?.[0] || "mcp"; - const accessToken = generateAccessToken(tokenScope); + const accessToken = generateAccessToken(tokenScope, client_id); const tokenExpiration = config.tokenExpirationSeconds || 3600; const response: { @@ -679,7 +679,7 @@ function setupTokenEndpoint( const tokenScope = refreshTokenData.scope || config.scopesSupported?.[0] || "mcp"; - const accessToken = generateAccessToken(tokenScope); + const accessToken = generateAccessToken(tokenScope, client_id); // Keep the grant linkage current so a later revocation of this refresh // token also kills the access token it just minted. refreshTokenData.accessTokens.add(accessToken); @@ -732,8 +732,8 @@ function setupRevocationEndpoint( return; } - const authenticated = await authenticateRevocationClient(req, config); - if (!authenticated) { + const clientId = await authenticateRevocationClient(req, config); + if (clientId === null) { res .status(401) .set("WWW-Authenticate", 'Basic realm="revoke"') @@ -741,16 +741,20 @@ function setupRevocationEndpoint( return; } + // §2.1: only the client the token was issued to may revoke it. A token + // belonging to someone else is left alone — and still answered 200, per + // §2.2, since the response must not tell one client whether another's + // token exists. const refreshTokenData = refreshTokens.get(token); if (refreshTokenData) { - for (const accessToken of refreshTokenData.accessTokens) { - accessTokens.delete(accessToken); - accessTokenScopes.delete(accessToken); + if (refreshTokenData.clientId === clientId) { + for (const accessToken of refreshTokenData.accessTokens) { + forgetAccessToken(accessToken); + } + refreshTokens.delete(token); } - refreshTokens.delete(token); - } else { - accessTokens.delete(token); - accessTokenScopes.delete(token); + } else if (accessTokenClients.get(token) === clientId) { + forgetAccessToken(token); } // §2.2: 200 whether or not the token was known to us. @@ -759,8 +763,16 @@ function setupRevocationEndpoint( ); } +/** Drop an access token and everything recorded about it. */ +function forgetAccessToken(token: string): void { + accessTokens.delete(token); + accessTokenScopes.delete(token); + accessTokenClients.delete(token); +} + /** - * Authenticate the caller of `/oauth/revoke` (RFC 7009 §2.1). + * Authenticate the caller of `/oauth/revoke` (RFC 7009 §2.1) and return the + * `client_id` it authenticated as, or `null` when it did not authenticate. * * Credentials may arrive either way RFC 6749 §2.3.1 allows — an `Authorization: * Basic` header or `client_id`/`client_secret` in the form body — because the @@ -774,7 +786,7 @@ function setupRevocationEndpoint( async function authenticateRevocationClient( req: Request, config: OAuthConfig, -): Promise { +): Promise { let clientId: string | undefined; let clientSecret: string | undefined; @@ -785,7 +797,7 @@ async function authenticateRevocationClient( "base64", ).toString("utf8"); const separator = decoded.indexOf(":"); - if (separator === -1) return false; + if (separator === -1) return null; clientId = decoded.slice(0, separator); clientSecret = decoded.slice(separator + 1); } else { @@ -795,14 +807,14 @@ async function authenticateRevocationClient( if (typeof bodySecret === "string") clientSecret = bodySecret; } - if (!clientId) return false; + if (!clientId) return null; const client = await findClient(clientId, config); - if (!client) return false; + if (!client) return null; // A client registered with a secret must present it; a public one must not be // asked for one it never had. - return client.clientSecret === undefined - ? true - : clientSecret === client.clientSecret; + const ok = + client.clientSecret === undefined || clientSecret === client.clientSecret; + return ok ? clientId : null; } /** @@ -879,6 +891,13 @@ const authorizationCodes = new Map(); const accessTokens = new Set(); /** Granted OAuth scope string per access token (space-separated). */ const accessTokenScopes = new Map(); +/** + * Owning `client_id` per access token. RFC 7009 §2.1 requires an authorization + * server to verify that a token being revoked was issued to the requesting + * client, and without this the fixture had no way to tell — so any registered + * client could revoke another's access token. (#2144) + */ +const accessTokenClients = new Map(); const refreshTokens = new Map(); const registeredClients = new Map(); @@ -1005,10 +1024,11 @@ function getAuthorizationCode(code: string): AuthorizationCodeData | null { return data; } -function generateAccessToken(scope?: string): string { +function generateAccessToken(scope?: string, clientId?: string): string { const token = `test_access_token_${Date.now()}_${Math.random().toString(36).substring(7)}`; accessTokens.add(token); accessTokenScopes.set(token, scope?.trim() || "mcp"); + if (clientId !== undefined) accessTokenClients.set(token, clientId); return token; }