diff --git a/AGENTS.md b/AGENTS.md index 0729ca37..d31f9d85 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -102,11 +102,19 @@ v2/main/ │ │ # answered, since that is the base its │ │ # token request is made against — #2110; │ │ # revocation.ts RFC 7009 token revocation — -│ │ # the request the three clear paths send -│ │ # BEFORE wiping local state, since the -│ │ # token, the client credentials and the -│ │ # discovered `revocation_endpoint` all live -│ │ # in the store the clear empties. Names the +│ │ # the request the three clear paths send. +│ │ # TWO halves — planOAuthRevocation then +│ │ # executeOAuthRevocation — and the order +│ │ # between them is the contract: plan → +│ │ # storage.clear() → execute. The snapshot +│ │ # must precede the clear (the token, the +│ │ # client credentials and the discovered +│ │ # `revocation_endpoint` all live in the store +│ │ # it empties); the CLEAR must precede the +│ │ # network, or a fresh authorization +│ │ # completing during a 5s request is deleted +│ │ # by a clear reasoning about the grant it +│ │ # replaced. Names the │ │ # REFRESH token when there is one (§2.1 asks │ │ # the AS to invalidate the access tokens │ │ # under the same grant, so one request diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index a4ab8c01..19dc373c 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -4,7 +4,9 @@ import { } from "@inspector/core/auth/node/storage-node.js"; import { DEFAULT_REVOCATION_TIMEOUT_MS, - revokeStoredOAuthTokens, + executeOAuthRevocation, + planOAuthRevocation, + type OAuthRevocationPlan, type TokenRevocationOutcome, } from "@inspector/core/auth/revocation.js"; import { createProxyFetch } from "@inspector/core/mcp/node/proxyFetch.js"; @@ -36,22 +38,27 @@ export async function clearStoredAuthForRelogin( const raw = serverUrl.trim(); const normalized = normalizeServerUrl(raw); const storage = new NodeOAuthStorage(); - // RFC 7009 (#2144): revoke before the clear, since the token, the client - // credentials and the discovered `revocation_endpoint` all live in the store - // this is about to empty. Best-effort — the outcome is returned for the - // caller to report, never thrown, so `--relogin` succeeds regardless. + // RFC 7009 (#2144), ordered snapshot -> clear -> revoke. Everything the + // requests need is read first, because the clear empties the store; but the + // clear then runs immediately rather than behind the network. Waiting would + // hold this process's in-memory view of a *shared*, file-backed store for up + // to five seconds, and another CLI/TUI writing a fresh grant in that window + // would be erased by the clear that followed. // - // Both spellings are cleared below, so both are revoked from — a stale entry - // under the other key is a live grant at the authorization server, and - // deleting it locally without revoking is exactly the leak this closes. The - // normalised key goes first because that is the precedence - // `findStoredServerState` reads with, so the grant actually in use is the one - // whose outcome is reported. They are deliberately not deduplicated; see - // `revokeStoredKeys`. - const revocation = + // Both spellings are cleared, so both are planned from — a stale entry under + // the other key is a live grant at the authorization server, and deleting it + // locally without revoking is exactly the leak this closes. The normalised + // key goes first because that is the precedence `findStoredServerState` reads + // with, so the grant actually in use is the one whose outcome is reported. + // They are deliberately not deduplicated; see `sendPlans`. + const keys = normalized === raw ? [raw] : [normalized, raw]; + const plans = options?.revoke === false - ? undefined - : await revokeStoredKeys(storage, [normalized, raw]); + ? [] + : await Promise.all( + keys.map((key) => planOAuthRevocation({ serverUrl: key, storage })), + ); + await storage.clear(raw); if (normalized !== raw) { await storage.clear(normalized); @@ -59,58 +66,54 @@ export async function clearStoredAuthForRelogin( // Drop the in-process singleton so the next connect cannot reuse a cleared // entry from the NodeOAuthStorage cache. resetNodeOAuthStorageCache(); - return revocation; + + return plans.length > 0 ? sendPlans(plans) : undefined; } /** - * Revoke every grant held under `keys`, in order, and report the outcome that - * matters most. + * Send each plan's requests, in order, and report the outcome that matters most. * - * Both keys are about to be deleted, so both are revoked from: a stale entry - * under the other spelling is still a live grant at the authorization server, - * and deleting it locally without revoking is the leak this whole change - * closes. + * Both key spellings are cleared, so both are planned from: a stale entry under + * the other spelling is still a live grant at the authorization server, and + * deleting it locally without revoking is the leak this whole change closes. * * There is deliberately **no** cross-key deduplication. It looked cheap — the - * two keys are usually two spellings of one server — but it can only be done - * by pre-reading a single token, and `revokeStoredOAuthTokens` enumerates every - * issuer slot under a key. So a shared *active* token would have skipped the - * second key entirely, taking any additional issuer-bound grant under it with - * the local delete. A duplicate RFC 7009 request is harmless (§2.2 makes an - * unknown token a success), which is a much better trade than a missed one. + * two keys are usually two spellings of one server — but it can only be done by + * pre-reading a single token, and a plan enumerates every issuer slot under a + * key. So a shared *active* token would have skipped the second key entirely, + * taking any additional issuer-bound grant under it with the local delete. A + * duplicate RFC 7009 request is harmless (§2.2 makes an unknown token a + * success), which is a much better trade than a missed one. + * + * One budget across both plans, for the same reason a plan shares one across + * its grants: two keys would otherwise double the wait. * * Reporting prefers a **failure** over any success — a grant still live at the * authorization server is what the user needs to hear about, and a success - * would otherwise silence the warning. Failing that it is the first key's - * outcome that was not "nothing to do", which with `keys` in + * would otherwise silence the warning. Failing that it is the first plan's + * outcome that was not "nothing to do", which with the keys in * `findStoredServerState` precedence means the grant the CLI would actually - * have connected with. When no key holds a token, the last "nothing to do" + * have connected with. When no key held a token, the last "nothing to do" * answer is returned so the caller can still tell "no tokens" from "this * authorization server advertises no revocation endpoint". */ -async function revokeStoredKeys( - storage: NodeOAuthStorage, - keys: string[], +async function sendPlans( + plans: OAuthRevocationPlan[], ): Promise { const fetchFn = createProxyFetch() ?? fetch; - // One budget across both keys, for the same reason `revokeStoredOAuthTokens` - // shares one across grants: two keys would otherwise double the wait a user - // feels before `--relogin` gets on with the local delete. const deadlineAt = Date.now() + DEFAULT_REVOCATION_TIMEOUT_MS; let reported: TokenRevocationOutcome | undefined; let lastSkip: TokenRevocationOutcome | undefined; - for (const key of new Set(keys)) { + for (const plan of plans) { const remainingMs = deadlineAt - Date.now(); if (remainingMs <= 0) { reported ??= { status: "failed", - detail: `the ${DEFAULT_REVOCATION_TIMEOUT_MS}ms revocation budget was exhausted before "${key}" was attempted`, + detail: `the ${DEFAULT_REVOCATION_TIMEOUT_MS}ms revocation budget was exhausted before "${plan.serverUrl}" was attempted`, }; continue; } - const outcome = await revokeStoredOAuthTokens({ - serverUrl: key, - storage, + const outcome = await executeOAuthRevocation(plan, { fetchFn, timeoutMs: remainingMs, }); diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 9270c198..afc33d40 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -796,6 +796,11 @@ function App({ // A connect attempt supersedes whatever the last disconnect reported — // including one still in flight, which is what the counter bump retires. disconnectAttemptRef.current++; + // And whatever a pending clear was going to do (#2144). The server-name + // check alone does not cover disconnect/reconnect to the SAME server: the + // name still matches on the other side of it, so a clear still in flight + // would tear down the session this connect just established. + clearOAuthAttemptRef.current++; setDisconnectError(null); const finishConnect = async () => { diff --git a/clients/web/src/lib/clearServerOAuthState.test.ts b/clients/web/src/lib/clearServerOAuthState.test.ts index 002fb171..14ec2fad 100644 --- a/clients/web/src/lib/clearServerOAuthState.test.ts +++ b/clients/web/src/lib/clearServerOAuthState.test.ts @@ -80,10 +80,11 @@ describe("clearServerOAuthState", () => { expect(clearOAuthTokens).toHaveBeenCalledWith({ revoke: false }); }); - // #2144 — the non-active path revokes from the store directly. The whole - // point is that it happens *before* the clear: after it there is no token, - // no client id and no cached metadata left to build a request from. - it("revokes before clearing when this server is not the active connection", async () => { + // #2144 — the non-active path revokes from the store directly. The snapshot + // is taken before the clear (after it there is no token, client id or cached + // metadata to build a request from), but the clear itself runs before the + // network — so by the time the request goes out the store is already empty. + it("clears before sending, using the snapshot it took first", async () => { await storage.saveTokens(SERVER_URL, { access_token: "tok", token_type: "Bearer", @@ -97,7 +98,7 @@ describe("clearServerOAuthState", () => { response_types_supported: ["code"], }); - let tokensAtRequestTime: unknown; + let tokensAtRequestTime: unknown = "unset"; const fetchFn = vi.fn(async () => { tokensAtRequestTime = await storage.getTokens(SERVER_URL); return new Response(null, { status: 200 }); @@ -115,7 +116,9 @@ describe("clearServerOAuthState", () => { tokenTypeHint: "refresh_token", endpoint: "https://as.example.com/revoke", }); - expect(tokensAtRequestTime).toBeDefined(); + // The store was already empty when the request went out — that is the + // ordering, and it is what stops a concurrently-written grant being wiped. + expect(tokensAtRequestTime).toBeUndefined(); expect(await storage.getTokens(SERVER_URL)).toBeUndefined(); }); diff --git a/clients/web/src/lib/clearServerOAuthState.ts b/clients/web/src/lib/clearServerOAuthState.ts index 1593565b..e4ac4068 100644 --- a/clients/web/src/lib/clearServerOAuthState.ts +++ b/clients/web/src/lib/clearServerOAuthState.ts @@ -1,5 +1,6 @@ import { - revokeStoredOAuthTokens, + executeOAuthRevocation, + planOAuthRevocation, type TokenRevocationOutcome, } from "@inspector/core/auth/revocation.js"; import type { OAuthStorage } from "@inspector/core/auth/storage.js"; @@ -43,7 +44,7 @@ export interface ClearServerOAuthStateResult { /** * Clear persisted OAuth state (tokens, DCR/CIMD client id, PKCE, etc.) for an - * HTTP MCP server, revoking the grant at the authorization server first. When + * HTTP MCP server, revoking the grant at the authorization server. When * clearing the active connection, uses the live client so in-memory flow state * is reset too. * @@ -71,14 +72,20 @@ export async function clearServerOAuthState( // No proxied fetch on hand means no request we could usefully make, so the // leg is reported as skipped rather than attempted against the page origin. const fetchFn = params.fetchFn; - const revocation: TokenRevocationOutcome = - revoke && fetchFn - ? await revokeStoredOAuthTokens({ - serverUrl, - storage: params.oauthStorage, - fetchFn, - }) - : { status: "skipped", reason: "disabled" }; + // Snapshot → clear → revoke. The clear must not wait on the network: this + // server can be inactive when the call starts and complete a *fresh* + // authorization while the request is in flight, at which point an unconditional + // clear afterwards would delete the new credentials. The session checks in + // `useOAuthRecovery` run after this helper returns and cannot protect the + // store, so the ordering is what does (#2144). + const plan = await planOAuthRevocation({ + serverUrl, + storage: params.oauthStorage, + enabled: revoke && fetchFn !== undefined, + }); await params.oauthStorage.clear(serverUrl); + const revocation: TokenRevocationOutcome = fetchFn + ? await executeOAuthRevocation(plan, { fetchFn }) + : { status: "skipped", reason: "disabled" }; return { cleared: true, revocation }; } diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index 994ed8c6..e383f155 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -6,11 +6,13 @@ import { aggregateOutcomes, buildRevocationRequest, revocationAuthMethods, - revokeStoredOAuthTokens, + executeOAuthRevocation, + planOAuthRevocation, revokeToken, selectRevocableToken, } from "@inspector/core/auth/revocation.js"; import type { InspectorLogger } from "@inspector/core/logging/index.js"; +import type { TokenRevocationOutcome } from "@inspector/core/auth/revocation.js"; /** A fully-typed `InspectorLogger` double, so the mock's shape is checked. */ function fakeLogger(): InspectorLogger { @@ -339,7 +341,32 @@ describe("revokeToken", () => { }); }); -describe("revokeStoredOAuthTokens", () => { +/** + * Compose the two halves the way every caller does, minus the `storage.clear` + * between them — these tests are about what is planned and sent, and the + * ordering guarantee itself is asserted separately below. + */ +async function revokeStoredOAuthTokens(params: { + serverUrl: string; + storage: BrowserOAuthStorage; + fetchFn: typeof fetch; + enabled?: boolean; + timeoutMs?: number; + logger?: InspectorLogger; +}): Promise { + const plan = await planOAuthRevocation({ + serverUrl: params.serverUrl, + storage: params.storage, + enabled: params.enabled, + }); + return executeOAuthRevocation(plan, { + fetchFn: params.fetchFn, + timeoutMs: params.timeoutMs, + logger: params.logger, + }); +} + +describe("revokeStoredOAuthTokens (plan + execute)", () => { let storage: BrowserOAuthStorage; beforeEach(async () => { @@ -890,3 +917,87 @@ describe("revokeStoredOAuthTokens", () => { ); }); }); + +// The reason the API is two halves rather than one call (#2144, review round +// 13). Revoking first and clearing afterwards delays the clear by however long +// the network takes, and a FRESH authorization completing in that window is +// then deleted by a clear reasoning about the grant it replaced. +describe("plan / clear / execute ordering", () => { + let storage: BrowserOAuthStorage; + + beforeEach(async () => { + storage = new BrowserOAuthStorage(); + await storage.clear(SERVER_URL); + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as.example.com" }), + ); + await storage.saveTokens( + SERVER_URL, + { access_token: "old", token_type: "Bearer", refresh_token: "old-r" }, + { issuer: "https://as.example.com" }, + ); + }); + + it("does not delete a grant written while the request is in flight", async () => { + let releaseRequest: () => void = () => {}; + const inFlight = new Promise((resolve) => { + releaseRequest = resolve; + }); + const fetchFn = vi.fn(async () => { + await inFlight; + return new Response(null, { status: 200 }); + }); + + // The caller's real sequence: snapshot, clear, then send. + const plan = await planOAuthRevocation({ serverUrl: SERVER_URL, storage }); + await storage.clear(SERVER_URL); + const sending = executeOAuthRevocation(plan, { fetchFn }); + + // A fresh authorization lands while the request is still out. + await storage.saveTokens( + SERVER_URL, + { access_token: "new", token_type: "Bearer", refresh_token: "new-r" }, + { issuer: "https://as.example.com" }, + ); + + releaseRequest(); + await sending; + + // The new grant survived, and the OLD token is what was revoked. + expect((await storage.getTokens(SERVER_URL))?.access_token).toBe("new"); + expect( + new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"), + ).toBe("old-r"); + }); + + it("sends the snapshot even though the store was emptied first", async () => { + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const plan = await planOAuthRevocation({ serverUrl: SERVER_URL, storage }); + await storage.clear(SERVER_URL); + + await expect( + executeOAuthRevocation(plan, { fetchFn }), + ).resolves.toMatchObject({ status: "revoked" }); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + + it("plans nothing when revocation is disabled", async () => { + const plan = await planOAuthRevocation({ + serverUrl: SERVER_URL, + storage, + enabled: false, + }); + const fetchFn = vi.fn(); + + expect(plan.grants).toHaveLength(0); + await expect(executeOAuthRevocation(plan, { fetchFn })).resolves.toEqual({ + status: "skipped", + reason: "disabled", + }); + expect(fetchFn).not.toHaveBeenCalled(); + }); +}); diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 0640784d..b80ec4cc 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -199,10 +199,12 @@ describe("OAuthManager", () => { expect(manager.getOAuthFlowStep()).toBeUndefined(); }); - // #2144 — the ordering is the contract, not an implementation detail: the - // revocation request is built from the token, the client id and the cached - // metadata that `clear` is about to delete. - it("revokes at the authorization server before clearing local state", async () => { + // #2144 — the ordering is the contract, not an implementation detail. The + // request is built from the token, the client id and the cached metadata + // `clear` deletes, so the snapshot has to be taken first; but the clear + // must then run BEFORE the network, or a fresh authorization completing + // during it would be deleted by a clear reasoning about the old grant. + it("clears local state before waiting on the revocation request", async () => { const params = createMockParams(); const storage = params.initialConfig.storage!; vi.mocked(storage.getTokens).mockResolvedValue({ @@ -234,7 +236,7 @@ describe("OAuthManager", () => { status: "revoked", tokenTypeHint: "refresh_token", }); - expect(order).toEqual(["revoke", "clear"]); + expect(order).toEqual(["clear", "revoke"]); }); it("clears local state even when the revocation request fails", async () => { 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 3b637e1c..46285024 100644 --- a/clients/web/src/test/integration/auth/revocation-e2e.test.ts +++ b/clients/web/src/test/integration/auth/revocation-e2e.test.ts @@ -18,7 +18,11 @@ import { waitForOAuthWellKnown, } from "@modelcontextprotocol/inspector-test-server"; import { BrowserOAuthStorage } from "@inspector/core/auth/browser/storage.js"; -import { revokeStoredOAuthTokens } from "@inspector/core/auth/revocation.js"; +import { + executeOAuthRevocation, + planOAuthRevocation, +} from "@inspector/core/auth/revocation.js"; +import type { TokenRevocationOutcome } from "@inspector/core/auth/revocation.js"; import type { OAuthMetadata } from "@modelcontextprotocol/client"; const CLIENT_ID = "test-2144-revocation"; @@ -192,6 +196,18 @@ describe("OAuth token revocation (RFC 7009)", () => { expect(await tokenAccepted(tokens.access_token)).toBe(true); }); + /** + * The caller's real sequence — snapshot, clear, send — so this exercises the + * ordering the product uses rather than a convenience wrapper. + */ + async function clearAndRevoke( + storage: BrowserOAuthStorage, + ): Promise { + const plan = await planOAuthRevocation({ serverUrl, storage }); + await storage.clear(serverUrl); + return executeOAuthRevocation(plan, { fetchFn: fetch }); + } + it("advertises a revocation endpoint", () => { expect(metadata.revocation_endpoint).toBe(`${serverUrl}/oauth/revoke`); }); @@ -202,11 +218,7 @@ describe("OAuth token revocation (RFC 7009)", () => { const tokens = await authorize(); expect(await tokenAccepted(tokens.access_token)).toBe(true); - const outcome = await revokeStoredOAuthTokens({ - serverUrl, - storage: await seededStorage(tokens), - fetchFn: fetch, - }); + const outcome = await clearAndRevoke(await seededStorage(tokens)); expect(outcome).toMatchObject({ status: "revoked", @@ -219,11 +231,9 @@ describe("OAuth token revocation (RFC 7009)", () => { it("revokes an access token when no refresh token was issued", async () => { const tokens = await authorize(); - const outcome = await revokeStoredOAuthTokens({ - serverUrl, - storage: await seededStorage({ access_token: tokens.access_token }), - fetchFn: fetch, - }); + const outcome = await clearAndRevoke( + await seededStorage({ access_token: tokens.access_token }), + ); expect(outcome).toMatchObject({ status: "revoked", @@ -236,8 +246,8 @@ describe("OAuth token revocation (RFC 7009)", () => { // server has already expired must not be reported as a failure. it("treats an already-unknown token as revoked", async () => { const storage = await seededStorage({ access_token: "never-issued" }); - await expect( - revokeStoredOAuthTokens({ serverUrl, storage, fetchFn: fetch }), - ).resolves.toMatchObject({ status: "revoked" }); + await expect(clearAndRevoke(storage)).resolves.toMatchObject({ + status: "revoked", + }); }); }); diff --git a/core/auth/index.ts b/core/auth/index.ts index 8c3e8c10..33a703af 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -140,13 +140,16 @@ export { aggregateOutcomes, buildRevocationRequest, revocationAuthMethods, - revokeStoredOAuthTokens, + executeOAuthRevocation, + planOAuthRevocation, revokeToken, selectRevocableToken, } from "./revocation.js"; export type { + ExecuteOAuthRevocationParams, + OAuthRevocationPlan, + PlanOAuthRevocationParams, RevocationRequestParams, - RevokeStoredOAuthTokensParams, RevokeTokenParams, TokenRevocationOutcome, TokenRevocationSkipReason, diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index 2eab0f90..853370cf 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -270,54 +270,157 @@ async function withDeadline( } } -export interface RevokeStoredOAuthTokensParams { +export interface PlanOAuthRevocationParams { serverUrl: string; storage: OAuthStorage; - fetchFn: typeof fetch; /** - * `false` skips the request entirely — the deliberate case from #2144, where - * a user wants to watch a server cope with a client that walks off still - * holding live tokens. + * `false` plans nothing — the deliberate case from #2144, where a user wants + * to watch a server cope with a client that walks off still holding live + * tokens. */ enabled?: boolean; +} + +export interface ExecuteOAuthRevocationParams { + fetchFn: typeof fetch; timeoutMs?: number; logger?: InspectorLogger; } /** - * Revoke the tokens the Inspector holds for `serverUrl`, reading everything it - * needs out of the OAuth store. + * Everything the revocation requests need, read out of the store **before** the + * local clear empties it. * - * Called immediately **before** the local clear, since the store is where the - * 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 - * by that same authorization server, so its cached document is the document - * that describes them, and re-discovering would add two network legs to a - * teardown for no new information. A server that has never completed an OAuth - * flow has no cached metadata *and* no tokens, so it short-circuits either way. + * Split from the sending on purpose, and the ordering it enables is the whole + * point: plan → `storage.clear(serverUrl)` → execute. Revoking first and + * clearing afterwards delays the clear by however long the network takes, and a + * *fresh* authorization completing in that window would then be deleted by a + * clear that was reasoning about the grant it replaced. Snapshotting first + * closes that window: the clear runs against what the user asked to forget, and + * the requests go out against a copy nothing can invalidate. */ -export async function revokeStoredOAuthTokens( - params: RevokeStoredOAuthTokensParams, -): Promise { - const { serverUrl, logger } = params; +export interface OAuthRevocationPlan { + serverUrl: string; + grants: StoredGrant[]; + /** Slots that could not be read; reported beside whatever the rest do. */ + failures: TokenRevocationOutcome[]; + endpoint?: string; + supportedAuthMethods: string[]; + metadataIssuer?: string; + /** Set when there is nothing to send, and why. */ + outcome?: TokenRevocationOutcome; +} + +/** A plan that sends nothing, carrying the reason. */ +function emptyPlan( + serverUrl: string, + outcome: TokenRevocationOutcome, + failures: TokenRevocationOutcome[] = [], +): OAuthRevocationPlan { + return { + serverUrl, + grants: [], + failures, + supportedAuthMethods: [], + outcome: + failures.length > 0 ? aggregateOutcomes([...failures, outcome]) : outcome, + }; +} + +/** + * Read every grant the impending `clear(serverUrl)` will delete, plus the + * endpoint and credentials needed to revoke them. + * + * Every grant 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 by that same authorization server, so its cached + * document is the one that describes them, and re-discovering would add two + * network legs to a teardown for no new information. A server that never + * completed an OAuth flow has no cached metadata *and* no tokens, so it + * short-circuits either way. + */ +export async function planOAuthRevocation( + params: PlanOAuthRevocationParams, +): Promise { + const { serverUrl, storage } = params; if (params.enabled === false) { - return { status: "skipped", reason: "disabled" }; + return emptyPlan(serverUrl, { status: "skipped", reason: "disabled" }); } - const outcome = await computeOutcome(params); + try { + const { grants, failures } = await collectGrants(storage, serverUrl); + if (grants.length === 0) { + return emptyPlan( + serverUrl, + { status: "skipped", reason: "no_tokens" }, + failures, + ); + } + + const metadata = await storage.getServerMetadata(serverUrl); + if (!metadata) { + return emptyPlan( + serverUrl, + { status: "skipped", reason: "no_metadata" }, + failures, + ); + } + if (!metadata.revocation_endpoint) { + return emptyPlan( + serverUrl, + { status: "skipped", reason: "no_endpoint" }, + failures, + ); + } + + return { + serverUrl, + grants, + failures, + endpoint: metadata.revocation_endpoint, + supportedAuthMethods: revocationAuthMethods(metadata), + metadataIssuer: metadata.issuer, + }; + } catch (err) { + // A store that cannot be read is not a reason to abandon the clear the + // user asked for. + return emptyPlan(serverUrl, { + status: "failed", + detail: err instanceof Error ? err.message : String(err), + }); + } +} + +/** + * Send the requests a {@link planOAuthRevocation} snapshot describes. + * + * Safe to run **after** `storage.clear(serverUrl)` — that is the point: the + * plan already holds the tokens, the credentials and the endpoint, so nothing + * here reads the store, and the local clear is never waiting on the network. + * + * Best-effort throughout: every path returns a {@link TokenRevocationOutcome} + * rather than throwing, because forgetting the tokens is what the caller + * actually asked for and no failure on this leg should undo it. + */ +export async function executeOAuthRevocation( + plan: OAuthRevocationPlan, + params: ExecuteOAuthRevocationParams, +): Promise { + const outcome = await runPlan(plan, params); + const { logger } = params; if (outcome.status === "failed") { logger?.warn( - { serverUrl, endpoint: outcome.endpoint, detail: outcome.detail }, - "Token revocation failed; clearing local OAuth state anyway", + { + serverUrl: plan.serverUrl, + endpoint: outcome.endpoint, + detail: outcome.detail, + }, + "Token revocation failed; local OAuth state was cleared anyway", ); } else if (outcome.status === "skipped" && outcome.reason === "no_endpoint") { logger?.debug( - { serverUrl }, + { serverUrl: plan.serverUrl }, "Skipping token revocation: authorization server metadata has no revocation_endpoint", ); } @@ -345,7 +448,7 @@ async function resolveClientInformation( } /** One revocable grant held for a server, and which AS minted it. */ -interface StoredGrant { +export interface StoredGrant { /** Undefined for the legacy unkeyed slot, which predates issuer binding. */ issuer?: string; token: string; @@ -493,100 +596,71 @@ export function aggregateOutcomes( ); } -async function computeOutcome( - params: RevokeStoredOAuthTokensParams, +/** Send one plan's requests against a single shared deadline. */ +async function runPlan( + plan: OAuthRevocationPlan, + params: ExecuteOAuthRevocationParams, ): Promise { - const { serverUrl, storage, fetchFn } = params; + if (plan.outcome) return plan.outcome; + const endpoint = plan.endpoint; + /* v8 ignore next -- `planOAuthRevocation` always sets `outcome` when it sets + no endpoint, so this is unreachable; the guard exists to narrow the type. */ + if (!endpoint) return { status: "skipped", reason: "no_endpoint" }; - try { - // Grants first. They are the cheapest disqualifier, and a server with no - // stored grant should not provoke a metadata read at all. - // - // `failures` seeds `outcomes` rather than short-circuiting: a slot that - // could not be read is still about to be deleted, so it has to be reported - // *alongside* whatever the readable grants do, not instead of them. - const { grants, failures } = await collectGrants(storage, serverUrl); - if (grants.length === 0) { - return failures.length > 0 - ? aggregateOutcomes(failures) - : { status: "skipped", reason: "no_tokens" }; + const outcomes: TokenRevocationOutcome[] = [...plan.failures]; + // ONE deadline for the whole teardown, not one per grant. `clear` deletes + // every issuer slot, so a server with N of them would otherwise block for + // N × the timeout — at which point the "short timeout" bounds a single + // request and nothing the user experiences. Grants are revoked sequentially + // on purpose (a burst of parallel requests to one authorization server is + // not a kindness), so the budget is shared instead. + const timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS; + const deadlineAt = Date.now() + timeoutMs; + + for (const grant of plan.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 disclose a bearer credential to a + // server that never minted it — worse than not revoking. So an issuer-bound + // grant must be able to PROVE the endpoint is its own, which means a + // metadata document carrying no `issuer` is a mismatch rather than a free + // pass: absence establishes nothing. Only a legacy unkeyed grant proceeds + // without the comparison, since nothing binds it to a different + // authorization server in the first place. + if (grant.issuer !== undefined && plan.metadataIssuer !== grant.issuer) { + outcomes.push({ + status: "failed", + detail: + plan.metadataIssuer === undefined + ? `the cached authorization-server metadata names no issuer, so the grant bound to ${grant.issuer} could not be matched to this revocation endpoint and was cleared without revocation` + : `the cached authorization-server metadata is for ${plan.metadataIssuer}, so the grant bound to ${grant.issuer} was cleared without revocation`, + }); + continue; } - const metadata = await storage.getServerMetadata(serverUrl); - if (!metadata) { - return aggregateOutcomes([ - ...failures, - { status: "skipped", reason: "no_metadata" }, - ]); - } - const endpoint = metadata.revocation_endpoint; - if (!endpoint) { - return aggregateOutcomes([ - ...failures, - { status: "skipped", reason: "no_endpoint" }, - ]); + const remainingMs = deadlineAt - Date.now(); + if (remainingMs <= 0) { + outcomes.push({ + status: "failed", + endpoint, + detail: `the ${timeoutMs}ms revocation budget was exhausted before this grant was attempted${ + grant.issuer === undefined ? "" : ` (issuer ${grant.issuer})` + }`, + }); + continue; } - const supportedAuthMethods = revocationAuthMethods(metadata); - const outcomes: TokenRevocationOutcome[] = [...failures]; - // ONE deadline for the whole teardown, not one per grant. `clear` deletes - // every issuer slot, so a server with N of them would otherwise block the - // disconnect for N × the timeout — at which point the "short timeout" - // bounds a single request and nothing the user experiences. Grants are - // revoked sequentially on purpose (a burst of parallel requests to one - // authorization server is not a kindness), so the budget is shared instead. - const timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS; - const deadlineAt = Date.now() + timeoutMs; - 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 disclose a bearer credential to - // a server that never minted it — worse than not revoking. So an - // issuer-bound grant must be able to PROVE the endpoint is its own, which - // means a metadata document carrying no `issuer` is a mismatch rather - // than a free pass: absence establishes nothing. Only a legacy unkeyed - // grant proceeds without the comparison, since nothing binds it to a - // different authorization server in the first place. - if (grant.issuer !== undefined && metadata.issuer !== grant.issuer) { - outcomes.push({ - status: "failed", - detail: - metadata.issuer === undefined - ? `the cached authorization-server metadata names no issuer, so the grant bound to ${grant.issuer} could not be matched to this revocation endpoint and was cleared without revocation` - : `the cached authorization-server metadata is for ${metadata.issuer}, so the grant bound to ${grant.issuer} was cleared without revocation`, - }); - continue; - } - const remainingMs = deadlineAt - Date.now(); - if (remainingMs <= 0) { - outcomes.push({ - status: "failed", - endpoint, - detail: `the ${timeoutMs}ms revocation budget was exhausted before this grant was attempted${ - grant.issuer === undefined ? "" : ` (issuer ${grant.issuer})` - }`, - }); - continue; - } - outcomes.push( - await revokeToken({ - endpoint, - token: grant.token, - tokenTypeHint: grant.tokenTypeHint, - clientInformation: grant.clientInformation, - supportedAuthMethods, - fetchFn, - timeoutMs: remainingMs, - }), - ); - } - 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. - return { - status: "failed", - detail: err instanceof Error ? err.message : String(err), - }; + outcomes.push( + await revokeToken({ + endpoint, + token: grant.token, + tokenTypeHint: grant.tokenTypeHint, + clientInformation: grant.clientInformation, + supportedAuthMethods: plan.supportedAuthMethods, + fetchFn: params.fetchFn, + timeoutMs: remainingMs, + }), + ); } + return aggregateOutcomes(outcomes); } diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index 5c93d9f7..65c11a78 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -14,7 +14,8 @@ import { mcpAuth } from "../auth/mcpAuth.js"; import type { OAuthStorage } from "../auth/storage.js"; import { parseOAuthState } from "../auth/utils.js"; import { - revokeStoredOAuthTokens, + executeOAuthRevocation, + planOAuthRevocation, type TokenRevocationOutcome, } from "../auth/revocation.js"; import type { InspectorLogger } from "../logging/index.js"; @@ -499,12 +500,15 @@ export class OAuthManager { * Revoke the grant at the authorization server (RFC 7009), then drop the * local OAuth state. * - * Revocation runs **first** and reads what it needs out of the same store the - * next line wipes — the token, the client credentials and the discovered - * `revocation_endpoint` all live there, so after the clear there is nothing - * left to revoke with. It is best-effort by construction: every failure is - * reported through the returned outcome and the clear proceeds regardless, - * because forgetting the tokens is what the caller actually asked for (#2144). + * The order is **snapshot → clear → revoke**, and it matters. Everything the + * requests need is read out of the store first, because the clear empties it; + * but the clear then runs immediately rather than behind the network, because + * a fresh authorization completing during a five-second revocation would + * otherwise be deleted by a clear reasoning about the grant it replaced. + * + * Best-effort by construction: every failure is reported through the returned + * outcome and the clear has already happened regardless, because forgetting + * the tokens is what the caller actually asked for (#2144). * * `options.revoke === false` skips the request. That is not only an escape * hatch for an authorization server that mishandles it — disconnecting while @@ -519,18 +523,19 @@ export class OAuthManager { } const serverUrl = this.getServerUrl(); - const outcome = await revokeStoredOAuthTokens({ + const plan = await planOAuthRevocation({ serverUrl, storage: this.oauthConfig.storage, - fetchFn: this.params.effectiveAuthFetch, enabled: options?.revoke, - logger: this.params.logger, }); await this.oauthConfig.storage.clear(serverUrl); this.oauthFlowState = null; this.pendingAuthorizationScope = undefined; - return outcome; + return executeOAuthRevocation(plan, { + fetchFn: this.params.effectiveAuthFetch, + logger: this.params.logger, + }); } async isOAuthAuthorized(): Promise { diff --git a/docs/mcp-server-configuration.md b/docs/mcp-server-configuration.md index bfba252e..269ddb0e 100644 --- a/docs/mcp-server-configuration.md +++ b/docs/mcp-server-configuration.md @@ -217,6 +217,8 @@ Without it, clearing is silent from the authorization server's point of view: th The request names the **refresh token** when there is one. RFC 7009 §2.1 asks an authorization server to also invalidate the access tokens issued under the same grant, so one request covers both halves; naming the access token instead would leave the long-lived one alive. +The order is **snapshot → clear → revoke**. Everything the request needs — the token, the client credentials, the discovered `revocation_endpoint` — is read out of the store first, because the clear empties it; but the clear then runs immediately rather than behind the network. Waiting would leave a window in which a *fresh* authorization could complete and then be deleted by a clear still reasoning about the grant it replaced. + > **It is best-effort, and the local clear always finishes.** An authorization server that advertises no `revocation_endpoint` is left behaving exactly as it did before this existed — nothing is sent. A network error, a non-2xx, or a slow server that trips the short timeout is reported (a toast in the web client, a status line in the TUI, a stderr warning from the CLI) and nothing more. Forgetting the tokens is what you asked for, so no failure on this leg stops it. > **Turning it off is a testing affordance, not only an escape hatch.** A client that walks away still holding live tokens is a case a server author may want to reproduce deliberately, to watch how the server under test copes with it.