diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index 19dc373c..12b17725 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -4,8 +4,8 @@ import { } from "@inspector/core/auth/node/storage-node.js"; import { DEFAULT_REVOCATION_TIMEOUT_MS, + clearAndPlanRevocation, executeOAuthRevocation, - planOAuthRevocation, type OAuthRevocationPlan, type TokenRevocationOutcome, } from "@inspector/core/auth/revocation.js"; @@ -52,22 +52,23 @@ export async function clearStoredAuthForRelogin( // 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 - ? [] - : await Promise.all( - keys.map((key) => planOAuthRevocation({ serverUrl: key, storage })), - ); - - await storage.clear(raw); - if (normalized !== raw) { - await storage.clear(normalized); + // Each key's state is taken and deleted in ONE atomic storage step, so the + // clear is never a separate check-then-act over an already-read snapshot. + const plans: OAuthRevocationPlan[] = []; + for (const key of keys) { + plans.push( + await clearAndPlanRevocation({ + serverUrl: key, + storage, + enabled: options?.revoke !== false, + }), + ); } // Drop the in-process singleton so the next connect cannot reuse a cleared // entry from the NodeOAuthStorage cache. resetNodeOAuthStorageCache(); - return plans.length > 0 ? sendPlans(plans) : undefined; + return options?.revoke === false ? undefined : sendPlans(plans); } /** @@ -106,7 +107,12 @@ async function sendPlans( let lastSkip: TokenRevocationOutcome | undefined; for (const plan of plans) { const remainingMs = deadlineAt - Date.now(); - if (remainingMs <= 0) { + // A plan that already knows its answer needs no network, so the budget is + // irrelevant to it. Synthesising exhaustion here would warn that a grant + // may still be live when the key held no grant at all — a false alarm, and + // one that outranks the real outcome under the failure-first rule below. + const needsNetwork = plan.outcome === undefined; + if (needsNetwork && remainingMs <= 0) { reported ??= { status: "failed", detail: `the ${DEFAULT_REVOCATION_TIMEOUT_MS}ms revocation budget was exhausted before "${plan.serverUrl}" was attempted`, @@ -115,7 +121,7 @@ async function sendPlans( } const outcome = await executeOAuthRevocation(plan, { fetchFn, - timeoutMs: remainingMs, + timeoutMs: needsNetwork ? remainingMs : undefined, }); if (outcome.status === "skipped" && outcome.reason === "no_tokens") { lastSkip = outcome; diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index 0a57d854..3139174f 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -1445,6 +1445,41 @@ describe("App (mid-session auth lifecycle events)", () => { ); }); + // The clear has already succeeded when the disconnect runs, so its failure + // must not propagate: `AuthTab` would report "Could not clear OAuth state", + // which is false. It goes to the disconnect error line, where the same + // failure from the `d` key already lands. + it("reports a post-clear disconnect failure as a disconnect failure", async () => { + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValue(new Error("disconnect-blew-up")); + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + + await expectFrame(r, "disconnect-blew-up"); + expect(r.lastFrame() ?? "").not.toContain("Could not clear OAuth state"); + }); + + // A transport can reject with something that is not an `Error`; the message + // line must still be readable rather than "[object Object]". + it("reports a non-Error disconnect failure from a clear", async () => { + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValue("plain-disconnect-string"); + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await expectFrame(r, "plain-disconnect-string"); + }); + + // A clear while still CONNECTING tears the attempt down too — the session is + // half-built, and leaving it up with its OAuth state gone is worse than not + // having it. + it("disconnects a connecting session when clearing", async () => { + h.ctrl.status = "connecting"; + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await waitUntil(() => h.disconnect.mock.calls.length > 0); + expect(h.disconnect).toHaveBeenCalled(); + }); + it("says nothing when there was nothing to revoke", async () => { const r = await mount(oneHttp()); await press(r, ["a", "s"]); diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index afc33d40..5b7d35f1 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -992,7 +992,15 @@ function App({ } setConnectError(null); if (inspectorStatus === "connected" || inspectorStatus === "connecting") { - await disconnectInspector(); + // The clear has already succeeded by here, so a disconnect failure must + // not propagate as one: `AuthTab` would report "Could not clear OAuth + // state", which is false. It goes to the disconnect error line instead, + // where the same failure from the `d` key already lands. + try { + await disconnectInspector(); + } catch (err) { + setDisconnectError(err instanceof Error ? err.message : String(err)); + } // Revalidate: the disconnect is a second await, and a switch during it // would make the revision bump below land on the new selection. if ( diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index d0d73cdc..13779d56 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -1287,6 +1287,16 @@ export function useOAuthRecovery({ if (client && stillTargetsActiveSession()) { try { await client.disconnect(); + } catch (err) { + // The clear has already succeeded, so this must not propagate: the + // caller's catch would report "Could not clear the stored OAuth + // state", which is false. Reported as what it is, and the + // cleared-successfully toast below still goes out. + notifications.show({ + title: "Cleared, but the session did not disconnect cleanly", + message: err instanceof Error ? err.message : String(err), + color: "yellow", + }); } finally { // Revalidate after the second await for the same reason. if (stillTargetsActiveSession()) { diff --git a/clients/web/src/lib/clearServerOAuthState.ts b/clients/web/src/lib/clearServerOAuthState.ts index b07e2db2..389bcbad 100644 --- a/clients/web/src/lib/clearServerOAuthState.ts +++ b/clients/web/src/lib/clearServerOAuthState.ts @@ -1,6 +1,6 @@ import { + clearAndPlanRevocation, executeOAuthRevocation, - planOAuthRevocation, type TokenRevocationOutcome, } from "@inspector/core/auth/revocation.js"; import type { OAuthStorage } from "@inspector/core/auth/storage.js"; @@ -72,18 +72,17 @@ 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; - // 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({ + // Takes the state and deletes it in ONE atomic storage step, then sends. 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 a later clear 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 clearAndPlanRevocation({ 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" }; 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 5e9f176e..95cbc13a 100644 --- a/clients/web/src/test/core/auth/connection-state.test.ts +++ b/clients/web/src/test/core/auth/connection-state.test.ts @@ -59,6 +59,7 @@ function createStorage( 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), 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 ca8eb0c6..2075d405 100644 --- a/clients/web/src/test/core/auth/ema/emaFlow.test.ts +++ b/clients/web/src/test/core/auth/ema/emaFlow.test.ts @@ -76,6 +76,7 @@ function createMemoryStorage( 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 ec72ea83..fb2b419a 100644 --- a/clients/web/src/test/core/auth/ema/idpSession.test.ts +++ b/clients/web/src/test/core/auth/ema/idpSession.test.ts @@ -27,6 +27,7 @@ describe("idpSession", () => { 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/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index 1d9168c7..888c9637 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -6,8 +6,8 @@ import { aggregateOutcomes, buildRevocationRequest, revocationAuthMethods, + clearAndPlanRevocation, executeOAuthRevocation, - planOAuthRevocation, revokeToken, selectRevocableToken, } from "@inspector/core/auth/revocation.js"; @@ -363,8 +363,8 @@ describe("revokeToken", () => { }); /** - * 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 + * Compose the two halves the way every caller does. The first one clears, so a + * test that asserts on the store afterwards is looking at an emptied one — the * ordering guarantee itself is asserted separately below. */ async function revokeStoredOAuthTokens(params: { @@ -375,7 +375,7 @@ async function revokeStoredOAuthTokens(params: { timeoutMs?: number; logger?: InspectorLogger; }): Promise { - const plan = await planOAuthRevocation({ + const plan = await clearAndPlanRevocation({ serverUrl: params.serverUrl, storage: params.storage, enabled: params.enabled, @@ -387,6 +387,43 @@ async function revokeStoredOAuthTokens(params: { }); } +/** + * Stub the atomic take-and-clear with a hand-built snapshot. Needed wherever + * the shape under test cannot be produced through the store's own API — a + * legacy slot alongside an issuer slot, for instance, since an issuer-stamped + * save promotes and clears the legacy one. + */ +function stubSnapshot( + storage: BrowserOAuthStorage, + snapshot: Partial[0]> = {}, +): void { + vi.spyOn(storage, "takeRevocationSnapshot").mockResolvedValue( + stubSnapshotShape(snapshot), + ); +} + +function stubSnapshotShape(snapshot: { + byIssuer?: Record< + string, + { tokens?: unknown; clientInformation?: unknown } | undefined + >; + legacyTokens?: unknown; + legacyClientInformation?: unknown; + preregisteredClientInformation?: unknown; + serverMetadata?: unknown; +}): { + byIssuer: Record< + string, + { tokens?: unknown; clientInformation?: unknown } | undefined + >; + legacyTokens?: unknown; + legacyClientInformation?: unknown; + preregisteredClientInformation?: unknown; + serverMetadata?: unknown; +} { + return { byIssuer: {}, ...snapshot }; +} + describe("revokeStoredOAuthTokens (plan + execute)", () => { let storage: BrowserOAuthStorage; @@ -568,28 +605,30 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { // Suppressing on the value would drop it unrevoked and unreported, which is // why the suppression keys off the store's issuer stamp instead. it("keeps the legacy grant when another issuer holds the same token value", async () => { - // The metadata names the same issuer as the slot below, so BOTH grants are - // genuinely revocable — otherwise this would pass for the wrong reason. - await storage.saveServerMetadata( - SERVER_URL, - metadata({ issuer: "https://as-a.example.com" }), - ); - // A legacy unkeyed grant... - await storage.saveTokens(SERVER_URL, { - access_token: "shared", - token_type: "Bearer", - refresh_token: "shared-r", - }); - // ...and an issuer slot that coincidentally holds the same token value. - // Saved via the client-information path so it does not clear the legacy - // slot, then given tokens through a stubbed exact read. - vi.spyOn(storage, "listIssuers").mockResolvedValue([ - "https://as-a.example.com", - ]); - vi.spyOn(storage, "getIssuerTokens").mockResolvedValue({ - access_token: "shared", - token_type: "Bearer", - refresh_token: "shared-r", + // Hand-built: an issuer-stamped save promotes and clears the legacy slot, + // so the store's own API cannot produce both at once. + stubSnapshot(storage, { + byIssuer: { + "https://as-a.example.com": { + tokens: { + access_token: "shared", + token_type: "Bearer", + refresh_token: "shared-r", + }, + }, + }, + legacyTokens: { + access_token: "shared", + token_type: "Bearer", + refresh_token: "shared-r", + }, + serverMetadata: { + issuer: "https://as-a.example.com", + authorization_endpoint: "https://as-a.example.com/authorize", + token_endpoint: "https://as-a.example.com/token", + revocation_endpoint: REVOKE_URL, + response_types_supported: ["code"], + }, }); const fetchFn = vi.fn( async () => new Response(null, { status: 200 }), @@ -601,10 +640,9 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { fetchFn, }); - // Still two GRANTS — that is what this is about. The issuer-bound one is - // revoked; the legacy one is refused (unknown authorization server) and - // reported, rather than being silently collapsed into the other by a - // token-value dedup. + // Two GRANTS despite one token value. The issuer-bound one is revoked; the + // legacy one is refused (unknown authorization server) and reported, rather + // than being silently collapsed into the other by a token-value dedup. expect(fetchFn).toHaveBeenCalledTimes(1); expect(outcome).toMatchObject({ status: "failed" }); expect(outcome.status === "failed" ? outcome.detail : "").toContain( @@ -672,35 +710,22 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { ); }); - // If `listIssuers` fails there is no enumerated slot behind the stamp, so - // skipping a stamped ctx-less read would leave the one grant the store can - // still produce unrevoked — while `clear` deleted it anyway. - it("revokes the active grant even when the issuer list cannot be read", async () => { - await storage.saveServerMetadata( - SERVER_URL, - metadata({ issuer: "https://as-a.example.com" }), - ); - await storage.saveTokens( - SERVER_URL, - { access_token: "a", token_type: "Bearer", refresh_token: "r-active" }, - { issuer: "https://as-a.example.com" }, - ); - vi.spyOn(storage, "listIssuers").mockRejectedValue(new Error("no list")); - const fetchFn = vi.fn( - async () => new Response(null, { status: 200 }), + // The take-and-clear is one step, so its failure means the state is still + // there. That has to REJECT rather than report: every caller's contract is + // "the clear happened", and a `failed` outcome would say the opposite of what + // occurred. Both clients have a rejection path for exactly this. + it("rejects when the atomic take-and-clear itself fails", async () => { + vi.spyOn(storage, "takeRevocationSnapshot").mockRejectedValue( + new Error("store unwritable"), ); - const outcome = await revokeStoredOAuthTokens({ - serverUrl: SERVER_URL, - storage, - fetchFn, - }); - - expect( - new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"), - ).toBe("r-active"); - // The listing failure is still surfaced — it outranks the success. - expect(outcome).toMatchObject({ status: "failed" }); + await expect( + revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn: vi.fn(), + }), + ).rejects.toThrow("store unwritable"); }); // The timeout is a budget for the WHOLE teardown, not per request. `clear` @@ -708,32 +733,35 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { // N of them block the disconnect for N × the timeout — at which point the // "short timeout" bounds a single request and nothing the user feels. it("shares one deadline across grants instead of one per grant", async () => { - await storage.saveServerMetadata(SERVER_URL, metadata({ issuer: "a" })); - // All three bound to the issuer the cached metadata describes, so all three - // are genuinely revocable and the budget is what stops them. - vi.spyOn(storage, "listIssuers").mockResolvedValue(["a", "b", "c"]); - vi.spyOn(storage, "getIssuerTokens").mockImplementation( - async (_url: string, issuer: string) => ({ - access_token: `a-${issuer}`, - token_type: "Bearer", - refresh_token: `r-${issuer}`, - }), - ); - // Every request hangs, so each one burns the whole remaining budget. + // Built directly rather than through the store: three *revocable* grants + // means three tokens under one issuer, which `byIssuer` cannot hold (one + // slot per issuer). The budget logic is what is under test here, and a plan + // is a plain value. + const grant = (n: string) => ({ + issuer: "https://as.example.com", + token: `r-${n}`, + tokenTypeHint: "refresh_token" as const, + }); + // Every request hangs, so the first burns the whole budget. const fetchFn = vi.fn(() => new Promise(() => {})); const started = Date.now(); - const outcome = await revokeStoredOAuthTokens({ - serverUrl: SERVER_URL, - storage, - fetchFn, - timeoutMs: 30, - }); + const outcome = await executeOAuthRevocation( + { + serverUrl: SERVER_URL, + grants: [grant("a"), grant("b"), grant("c")], + failures: [], + endpoint: REVOKE_URL, + supportedAuthMethods: [], + metadataIssuer: "https://as.example.com", + }, + { fetchFn, timeoutMs: 30 }, + ); const elapsed = Date.now() - started; expect(outcome).toMatchObject({ status: "failed" }); - // Three grants: with a per-grant bound this would be ~90ms. Generous upper - // bound so the assertion is about the shape, not the machine. + // With a per-grant bound this would be ~90ms. Generous upper bound so the + // assertion is about the shape, not the machine. expect(elapsed).toBeLessThan(70); // The first burned the budget; the rest are reported as never attempted. expect(fetchFn).toHaveBeenCalledTimes(1); @@ -878,12 +906,23 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { expect(logger.debug).toHaveBeenCalledTimes(1); }); - // A store that cannot be read is not a reason to abandon the clear the user - // 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"); + // A snapshot the code cannot interpret is reported, not thrown: the state is + // already gone by then, so there is nothing left for the caller to retry. + // (A failure of the take-and-clear ITSELF rejects instead — see above.) + it("reports an uninterpretable snapshot as failed", async () => { + stubSnapshot(storage, { + byIssuer: { + // Not a valid `OAuthTokens` — no `token_type`. + "https://as.example.com": { tokens: { access_token: 42 } }, + }, + serverMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + revocation_endpoint: REVOKE_URL, + response_types_supported: ["code"], + }, + }); const outcome = await revokeStoredOAuthTokens({ serverUrl: SERVER_URL, @@ -891,57 +930,32 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { fetchFn: vi.fn(), }); expect(outcome).toMatchObject({ status: "failed" }); - expect(outcome.status === "failed" ? outcome.detail : "").toContain( - "store exploded", - ); - }); - - it("reports a store read failure as failed", async () => { - vi.spyOn(storage, "getTokens").mockRejectedValue( - new Error("store unreadable"), - ); - - const outcome = await revokeStoredOAuthTokens({ - serverUrl: SERVER_URL, - storage, - fetchFn: vi.fn(), - }); - expect(outcome).toMatchObject({ status: "failed" }); - expect(outcome.status === "failed" ? outcome.detail : "").toContain( - "store unreadable", - ); }); // A corrupt slot must not abandon the grants that are still revocable — the - // clear deletes them all either way, so the failure has to be reported - // BESIDE the successes rather than instead of them. - it("still revokes the readable grants when one issuer slot cannot be read", async () => { - await storage.saveServerMetadata( - SERVER_URL, - metadata({ issuer: "https://as.example.com" }), - ); - await storage.saveTokens( - SERVER_URL, - { access_token: "a", token_type: "Bearer", refresh_token: "r-good" }, - { issuer: "https://as.example.com" }, - ); - // A second issuer whose exact read throws. - vi.spyOn(storage, "listIssuers").mockResolvedValue([ - "https://as.example.com", - "https://broken.example.com", - ]); - vi.spyOn(storage, "getIssuerTokens").mockImplementation( - async (_url: string, issuer: string) => { - if (issuer === "https://broken.example.com") { - throw new Error("corrupt slot"); - } - return { - access_token: "a", - token_type: "Bearer", - refresh_token: "r-good", - }; + // state is gone either way, so the failure has to be reported BESIDE the + // successes rather than instead of them. + it("still revokes the readable grants when one slot cannot be parsed", async () => { + stubSnapshot(storage, { + byIssuer: { + "https://as.example.com": { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r-good", + }, + }, + // Unparseable: no `token_type`. + "https://broken.example.com": { tokens: { access_token: 42 } }, }, - ); + serverMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + revocation_endpoint: REVOKE_URL, + response_types_supported: ["code"], + }, + }); const fetchFn = vi.fn( async () => new Response(null, { status: 200 }), ); @@ -960,7 +974,7 @@ describe("revokeStoredOAuthTokens (plan + execute)", () => { // ...and the unreadable slot is reported rather than swallowed. expect(outcome).toMatchObject({ status: "failed" }); expect(outcome.status === "failed" ? outcome.detail : "").toContain( - "corrupt slot", + "could not read the stored grant", ); }); @@ -1034,9 +1048,11 @@ describe("plan / clear / execute ordering", () => { 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); + // The caller's real sequence: take-and-clear atomically, then send. + const plan = await clearAndPlanRevocation({ + serverUrl: SERVER_URL, + storage, + }); const sending = executeOAuthRevocation(plan, { fetchFn }); // A fresh authorization lands while the request is still out. @@ -1061,8 +1077,11 @@ describe("plan / clear / execute ordering", () => { async () => new Response(null, { status: 200 }), ); - const plan = await planOAuthRevocation({ serverUrl: SERVER_URL, storage }); - await storage.clear(SERVER_URL); + const plan = await clearAndPlanRevocation({ + serverUrl: SERVER_URL, + storage, + }); + expect(await storage.getTokens(SERVER_URL)).toBeUndefined(); await expect( executeOAuthRevocation(plan, { fetchFn }), @@ -1071,7 +1090,7 @@ describe("plan / clear / execute ordering", () => { }); it("plans nothing when revocation is disabled", async () => { - const plan = await planOAuthRevocation({ + const plan = await clearAndPlanRevocation({ serverUrl: SERVER_URL, storage, enabled: false, 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 65fe2fa7..3e02c1dc 100644 --- a/clients/web/src/test/core/auth/storage-browser.test.ts +++ b/clients/web/src/test/core/auth/storage-browser.test.ts @@ -431,6 +431,65 @@ describe("BrowserOAuthStorage", () => { }); }); + // #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. + describe("takeRevocationSnapshot", () => { + it("returns every slot and clears the server in one step", async () => { + await storage.saveTokens( + testServerUrl, + { access_token: "a", token_type: "Bearer" }, + { issuer: "https://as-a.example.com" }, + ); + await storage.savePreregisteredClientInformation(testServerUrl, { + client_id: "static-cid", + }); + await storage.saveServerMetadata(testServerUrl, { + issuer: "https://as-a.example.com", + authorization_endpoint: "https://as-a.example.com/authorize", + token_endpoint: "https://as-a.example.com/token", + response_types_supported: ["code"], + }); + + const snapshot = await storage.takeRevocationSnapshot(testServerUrl); + + expect( + snapshot.byIssuer["https://as-a.example.com"]?.tokens, + ).toMatchObject({ access_token: "a" }); + expect(snapshot.preregisteredClientInformation).toMatchObject({ + client_id: "static-cid", + }); + expect(snapshot.serverMetadata).toMatchObject({ + issuer: "https://as-a.example.com", + }); + // Cleared in the same step. + expect(await storage.getTokens(testServerUrl)).toBeUndefined(); + expect(await storage.getServerMetadata(testServerUrl)).toBeNull(); + expect(await storage.listIssuers(testServerUrl)).toEqual([]); + }); + + it("returns the legacy unkeyed slot too", async () => { + await storage.saveTokens(testServerUrl, { + access_token: "legacy", + token_type: "Bearer", + }); + + const snapshot = await storage.takeRevocationSnapshot(testServerUrl); + + expect(snapshot.legacyTokens).toMatchObject({ access_token: "legacy" }); + expect(snapshot.byIssuer).toEqual({}); + }); + + it("is empty for a server with no state", async () => { + const snapshot = await storage.takeRevocationSnapshot( + "https://unknown.example/mcp", + ); + expect(snapshot.byIssuer).toEqual({}); + expect(snapshot.legacyTokens).toBeUndefined(); + expect(snapshot.serverMetadata).toBeUndefined(); + }); + }); + 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 573def15..8cea49e9 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -69,6 +69,7 @@ function createMockParams( 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), clearDiscoveryState: vi.fn().mockResolvedValue(undefined), @@ -188,45 +189,75 @@ describe("OAuthManager", () => { }); describe("clearOAuthTokens", () => { - it("calls storage.clear(serverUrl) when storage is configured", async () => { + /** + * The take-and-clear is ONE atomic storage step now, so `clear` is no + * longer called separately — these assert on `takeRevocationSnapshot`. + */ + function stubSnapshot( + storage: OAuthManagerConfig["storage"], + revocable = true, + ): void { + vi.mocked(storage!.takeRevocationSnapshot).mockResolvedValue({ + byIssuer: revocable + ? { + "https://as.example.com": { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }, + }, + } + : {}, + serverMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + revocation_endpoint: "https://as.example.com/revoke", + response_types_supported: ["code"], + }, + }); + } + + it("takes and clears the server's state when storage is configured", async () => { const params = createMockParams(); const manager = new OAuthManager(params); await manager.clearOAuthTokens(); - expect(params.initialConfig.storage!.clear).toHaveBeenCalledWith( - SERVER_URL, - ); + expect( + params.initialConfig.storage!.takeRevocationSnapshot, + ).toHaveBeenCalledWith(SERVER_URL); expect(manager.getOAuthFlowState()).toBeUndefined(); expect(manager.getOAuthFlowStep()).toBeUndefined(); }); - // #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. + // #2144 — the ordering is the contract. The request is built from state the + // clear destroys, so the take must come first; but the two are ONE step, so + // nothing can save a fresh grant between them, and the network wait happens + // after the state is already gone. it("clears local state before waiting on the revocation request", async () => { const params = createMockParams(); const storage = params.initialConfig.storage!; - // Issuer-bound and matching the metadata: an unkeyed grant records no - // authorization server and is deliberately refused. - vi.mocked(storage.listIssuers).mockResolvedValue([ - "https://as.example.com", - ]); - vi.mocked(storage.getIssuerTokens).mockResolvedValue({ - access_token: "a", - token_type: "Bearer", - refresh_token: "r", - }); - vi.mocked(storage.getServerMetadata).mockResolvedValue({ - issuer: "https://as.example.com", - authorization_endpoint: "https://as.example.com/authorize", - token_endpoint: "https://as.example.com/token", - revocation_endpoint: "https://as.example.com/revoke", - response_types_supported: ["code"], - }); const order: string[] = []; - vi.mocked(storage.clear).mockImplementation(async () => { + vi.mocked(storage.takeRevocationSnapshot).mockImplementation(async () => { order.push("clear"); + return { + byIssuer: { + "https://as.example.com": { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }, + }, + }, + serverMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + revocation_endpoint: "https://as.example.com/revoke", + response_types_supported: ["code"], + }, + }; }); const fetchFn = vi.fn(async () => { order.push("revoke"); @@ -247,20 +278,7 @@ describe("OAuthManager", () => { it("clears local state even when the revocation request fails", async () => { const params = createMockParams(); const storage = params.initialConfig.storage!; - vi.mocked(storage.listIssuers).mockResolvedValue([ - "https://as.example.com", - ]); - vi.mocked(storage.getIssuerTokens).mockResolvedValue({ - access_token: "a", - token_type: "Bearer", - }); - vi.mocked(storage.getServerMetadata).mockResolvedValue({ - issuer: "https://as.example.com", - authorization_endpoint: "https://as.example.com/authorize", - token_endpoint: "https://as.example.com/token", - revocation_endpoint: "https://as.example.com/revoke", - response_types_supported: ["code"], - }); + stubSnapshot(storage); const manager = new OAuthManager({ ...params, effectiveAuthFetch: vi.fn(async () => { @@ -271,11 +289,13 @@ describe("OAuthManager", () => { await expect(manager.clearOAuthTokens()).resolves.toMatchObject({ status: "failed", }); - expect(storage.clear).toHaveBeenCalledWith(SERVER_URL); + expect(storage.takeRevocationSnapshot).toHaveBeenCalledWith(SERVER_URL); }); it("skips the request when revocation is turned off", async () => { const params = createMockParams(); + const storage = params.initialConfig.storage!; + stubSnapshot(storage); const fetchFn = vi.fn(); const manager = new OAuthManager({ ...params, @@ -286,9 +306,9 @@ describe("OAuthManager", () => { manager.clearOAuthTokens({ revoke: false }), ).resolves.toEqual({ status: "skipped", reason: "disabled" }); expect(fetchFn).not.toHaveBeenCalled(); - expect(params.initialConfig.storage!.clear).toHaveBeenCalledWith( - SERVER_URL, - ); + // Still cleared — that is what the caller asked for; the request is the + // optional part. + expect(storage.takeRevocationSnapshot).toHaveBeenCalledWith(SERVER_URL); }); it("no-ops when storage is not configured", 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 f5a6ff4e..7f56f259 100644 --- a/clients/web/src/test/integration/auth/revocation-e2e.test.ts +++ b/clients/web/src/test/integration/auth/revocation-e2e.test.ts @@ -19,8 +19,8 @@ import { } from "@modelcontextprotocol/inspector-test-server"; import { BrowserOAuthStorage } from "@inspector/core/auth/browser/storage.js"; import { + clearAndPlanRevocation, executeOAuthRevocation, - planOAuthRevocation, } from "@inspector/core/auth/revocation.js"; import type { TokenRevocationOutcome } from "@inspector/core/auth/revocation.js"; import type { OAuthMetadata } from "@modelcontextprotocol/client"; @@ -203,14 +203,13 @@ describe("OAuth token revocation (RFC 7009)", () => { }); /** - * The caller's real sequence — snapshot, clear, send — so this exercises the - * ordering the product uses rather than a convenience wrapper. + * The caller's real sequence — take-and-clear atomically, then 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); + const plan = await clearAndPlanRevocation({ serverUrl, storage }); return executeOAuthRevocation(plan, { fetchFn: fetch }); } diff --git a/core/auth/index.ts b/core/auth/index.ts index 33a703af..0ddad0c5 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -140,8 +140,8 @@ export { aggregateOutcomes, buildRevocationRequest, revocationAuthMethods, + clearAndPlanRevocation, executeOAuthRevocation, - planOAuthRevocation, revokeToken, selectRevocableToken, } from "./revocation.js"; diff --git a/core/auth/oauth-storage.ts b/core/auth/oauth-storage.ts index accbed2e..64638c20 100644 --- a/core/auth/oauth-storage.ts +++ b/core/auth/oauth-storage.ts @@ -8,7 +8,7 @@ import { OAuthClientInformationSchema, OAuthTokensSchema, } from "@modelcontextprotocol/core"; -import type { OAuthStorage } from "./storage.js"; +import type { OAuthStorage, RevocationSnapshot } from "./storage.js"; import { type OAuthMemoryStore, type ServerOAuthState, @@ -424,6 +424,29 @@ export class OAuthStorageBase implements OAuthStorage { await this.persist(); } + async takeRevocationSnapshot(serverUrl: string): Promise { + await this.ensureLoaded(); + // Everything below is synchronous on purpose: read and clear against one + // view of the in-memory state, so nothing can land between them. The + // persist is the only await, and it happens after the mutation. + const state = this.memory.getState().getServerState(serverUrl); + const snapshot: RevocationSnapshot = { + byIssuer: Object.fromEntries( + Object.entries(state.byIssuer ?? {}).map(([issuer, slot]) => [ + issuer, + { tokens: slot?.tokens, clientInformation: slot?.clientInformation }, + ]), + ), + legacyTokens: state.tokens, + legacyClientInformation: state.clientInformation, + preregisteredClientInformation: state.preregisteredClientInformation, + serverMetadata: state.serverMetadata, + }; + this.memory.getState().clearServerState(serverUrl); + await this.persist(); + return snapshot; + } + async getIssuerTokens( serverUrl: string, issuer: string, diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index 3a6717e5..ae36917d 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -22,12 +22,15 @@ import type { OAuthClientInformation, - OAuthMetadata, OAuthTokens, } from "@modelcontextprotocol/client"; import { selectClientAuthMethod } from "@modelcontextprotocol/client"; +import { + OAuthClientInformationSchema, + OAuthTokensSchema, +} from "@modelcontextprotocol/core"; import type { InspectorLogger } from "../logging/index.js"; -import type { OAuthStorage } from "./storage.js"; +import type { OAuthStorage, RevocationSnapshot } from "./storage.js"; /** * How long a revocation request may take before it is abandoned. @@ -107,7 +110,7 @@ export function selectRevocableToken( * when it does not — while still honoring a `token_endpoint_auth_method` the * client's own registration declares. */ -export function revocationAuthMethods(metadata: OAuthMetadata): string[] { +export function revocationAuthMethods(metadata: CachedMetadata): string[] { return metadata.revocation_endpoint_auth_methods_supported ?? []; } @@ -298,13 +301,12 @@ export interface ExecuteOAuthRevocationParams { * Everything the revocation requests need, read out of the store **before** the * local clear empties it. * - * 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. + * Split from the sending on purpose. `clearAndPlanRevocation` takes the state + * and deletes it in one atomic storage step; the requests then go out from this + * copy, which nothing can invalidate. Revoking first and clearing afterwards + * would delay the clear by however long the network takes, and a *fresh* + * authorization completing in that window would be deleted by a clear + * reasoning about the grant it replaced. */ export interface OAuthRevocationPlan { serverUrl: string; @@ -335,28 +337,45 @@ function emptyPlan( } /** - * Read every grant the impending `clear(serverUrl)` will delete, plus the - * endpoint and credentials needed to revoke them. + * Take the server's OAuth state — deleting it — and build the revocation + * requests from what was taken. + * + * The take and the delete are one atomic storage step + * ({@link OAuthStorage.takeRevocationSnapshot}), which is what closes the + * check-then-act window: separate reads followed by a separate clear each yield + * at an `await`, and an OAuth completion landing in one of those gaps would + * save a fresh grant that the clear then destroyed. + * + * The state is cleared **even when revocation is disabled or nothing can be + * revoked** — clearing is what the caller asked for; the requests are the + * optional part. * * 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. + * network legs to a teardown for no new information. */ -export async function planOAuthRevocation( +export async function clearAndPlanRevocation( params: PlanOAuthRevocationParams, ): Promise { const { serverUrl, storage } = params; + + let snapshot: RevocationSnapshot; + try { + snapshot = await storage.takeRevocationSnapshot(serverUrl); + } catch (err) { + // The clear did not happen, and the caller has no other way to learn that. + throw err instanceof Error ? err : new Error(String(err)); + } + if (params.enabled === false) { return emptyPlan(serverUrl, { status: "skipped", reason: "disabled" }); } try { - const { grants, failures } = await collectGrants(storage, serverUrl); + const { grants, failures } = await collectGrants(snapshot); if (grants.length === 0) { return emptyPlan( serverUrl, @@ -365,7 +384,7 @@ export async function planOAuthRevocation( ); } - const metadata = await storage.getServerMetadata(serverUrl); + const metadata = parseServerMetadata(snapshot.serverMetadata); if (!metadata) { return emptyPlan( serverUrl, @@ -390,8 +409,8 @@ export async function planOAuthRevocation( metadataIssuer: metadata.issuer, }; } catch (err) { - // A store that cannot be read is not a reason to abandon the clear the - // user asked for. + // The state is already gone; a snapshot we cannot interpret is reported, + // not thrown, because there is nothing left for the caller to retry. return emptyPlan(serverUrl, { status: "failed", detail: err instanceof Error ? err.message : String(err), @@ -399,6 +418,25 @@ export async function planOAuthRevocation( } } +/** + * The cached authorization-server metadata from a snapshot. + * + * Read structurally rather than schema-parsed: this is a document the SDK's own + * discovery wrote and the store round-trips as-is, and only two fields are + * used. A parse failure here would lose an otherwise revocable grant. + */ +function parseServerMetadata(raw: unknown): CachedMetadata | undefined { + if (raw === null || typeof raw !== "object") return undefined; + return raw as CachedMetadata; +} + +/** The three fields revocation reads out of cached AS metadata. */ +interface CachedMetadata { + revocation_endpoint?: string; + revocation_endpoint_auth_methods_supported?: string[]; + issuer?: string; +} + /** * Send the requests a {@link planOAuthRevocation} snapshot describes. * @@ -434,50 +472,33 @@ export async function executeOAuthRevocation( return outcome; } -/** - * The client credentials to authenticate the revocation request with. - * - * Deliberately the **reverse** of `BaseOAuthClientProvider.clientInformation`, - * which prefers the preregistered (static) entry. That order answers "who - * should I authenticate as *now*"; revocation asks a different question — "who - * minted *this* token" — and the two diverge. The store lets a preregistered - * client and an issuer-bound dynamic registration coexist - * (`savePreregisteredClientInformation` does not clear the issuer slot), so - * after a server is switched from DCR to a configured `oauth.clientId`, an - * older DCR grant would be revoked with the configured client's credentials. - * RFC 7009 §2.2 makes a 200 the answer for a token the server does not - * recognise as the caller's, so that reports `revoked` while the grant stays - * live — the worst possible outcome, since the local record is gone. - * - * So the registration bound to this grant's `issuer` wins where the store has - * one, and the preregistered entry is the fallback. That covers both - * directions: a token minted with the configured client leaves the issuer slot - * empty (the SDK only writes it after DCR) and falls through correctly. - * - * This is best available evidence, not proof: the store records client - * information per issuer, not per token, so a server that re-registered - * dynamically under one issuer still cannot distinguish which registration - * minted which grant. Binding the client identity to the token at save time is - * the real fix and is a storage-shape change beyond this PR. - */ -async function resolveClientInformation( - storage: OAuthStorage, - serverUrl: string, - issuer?: string, -): Promise { - return ( - (issuer !== undefined - ? await storage.getClientInformation(serverUrl, false, issuer) - : undefined) ?? (await storage.getClientInformation(serverUrl, true)) - ); -} - /** One revocable grant held for a server, and which AS minted it. */ export interface StoredGrant { /** Undefined for the legacy unkeyed slot, which predates issuer binding. */ issuer?: string; token: string; tokenTypeHint: "refresh_token" | "access_token"; + /** + * The credentials to authenticate the revocation request with: the + * registration bound to this grant's issuer, falling back to the + * preconfigured (static) one. + * + * Deliberately the **reverse** of `BaseOAuthClientProvider.clientInformation`, + * which prefers the preregistered entry. That order answers "who should I + * authenticate as *now*"; revocation asks "who minted *this* token", and the + * two diverge — the store lets a preregistered client and an issuer-bound + * dynamic registration coexist, so after a server is switched from DCR to a + * configured `oauth.clientId` an older DCR grant would be revoked with the + * configured client's credentials. RFC 7009 §2.2 answers 200 for a token the + * server does not recognise as the caller's, so that reports `revoked` while + * the grant stays live and the local record is already gone. + * + * Best available evidence, not proof: the store records client information + * per issuer, not per token, so a server that re-registered dynamically under + * one issuer still cannot say which registration minted which grant. Binding + * the identity to the token at save time is the real fix and is a + * storage-shape change. + */ clientInformation?: OAuthClientInformation; } @@ -485,85 +506,73 @@ export interface StoredGrant { interface CollectedGrants { grants: StoredGrant[]; /** - * One `failed` outcome per slot whose read threw. Kept apart from the grants - * so a single corrupt slot cannot abandon the ones that are still revocable - * — the clear deletes them all either way, so the failure has to be reported + * One `failed` outcome per slot that could not be parsed. Kept apart from the + * grants so a single corrupt slot cannot abandon the ones that are still + * revocable — the state is gone either way, so the failure has to be reported * *beside* the successes rather than instead of them. */ failures: TokenRevocationOutcome[]; } /** - * Every grant `clear(serverUrl)` is about to delete. + * Every grant the snapshot took, deduplicated by **issuer and 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 snapshot is of state that has already been deleted, so this covers every + * grant the clear removed rather than just the active issuer's: a server that + * authorized against issuers A and B has two grants here, not one. Otherwise + * B would be revoked while A's grant stayed live at its authorization server — + * the same leak this feature closes, one level down. * - * Deduplication is by **issuer *and* token**, not by token alone. A token is - * only meaningful to the authorization server that minted it, so two issuers - * that happen to mint the same opaque string are two grants; collapsing them - * would drop the second before the issuer-mismatch check could even report it. + * Deduplication is by issuer *and* token because a token means nothing outside + * the authorization server that minted it: two issuers that happen to mint the + * same opaque string are two grants, and collapsing them would drop the second + * before the issuer check could report it. * - * The ctx-less read is included last and is the one exception: it resolves to - * the *active* issuer's slot (already collected above) or, on a pre-SEP-2352 - * entry, to the legacy unkeyed token — which nothing else returns. It is - * suppressed on the issuer *stamp* the store puts on a slot-sourced value - * rather than on the token's value, because the active slot may hold client - * information without a token: the read then falls back to the legacy grant, - * and a value collision with any other issuer would drop a real grant. It is - * also the only read here allowed to fall back; an enumerated issuer is read - * exactly, so a legacy token is never mislabelled as belonging to one. + * Parsing happens here rather than in the snapshot: it is pure, so it belongs + * after the mutation — running it inside would reintroduce the `await` the + * atomic read exists to remove. */ async function collectGrants( - storage: OAuthStorage, - serverUrl: string, + snapshot: RevocationSnapshot, ): Promise { const grants: StoredGrant[] = []; const failures: TokenRevocationOutcome[] = []; const seenKeys = new Set(); - const add = async (issuer?: string): Promise => { - const tokens = - issuer === undefined - ? await storage.getTokens(serverUrl) - : await storage.getIssuerTokens(serverUrl, issuer); - const revocable = selectRevocableToken(tokens); + const preregistered = await parseClient( + snapshot.preregisteredClientInformation, + ); + + const add = async ( + issuer: string | undefined, + rawTokens: unknown, + rawClient: unknown, + ): Promise => { + const revocable = selectRevocableToken(await parseTokens(rawTokens)); if (!revocable) return; - // `getTokens` stamps the resolved issuer onto a value it took from a - // byIssuer slot and leaves an unkeyed one unstamped (see `withIssuer` in - // oauth-storage.ts — the stamp is the key it came from), so the ctx-less - // read reports which issuer its answer belongs to, if any. - const grantIssuer = - issuer ?? (tokens as { issuer?: string } | undefined)?.issuer; - // Keyed by issuer AND token: a token means nothing outside the - // authorization server that minted it, so two issuers minting the same - // opaque value are two grants. Deduping the ctx-less read on its *stamp* - // alone would be wrong too — if `listIssuers` failed there is no enumerated - // slot behind the stamp, and skipping would leave the one readable grant - // unrevoked while `clear` deleted it. - const key = `${grantIssuer ?? "\u0000legacy"}\u0000${revocable.token}`; + const key = `${issuer ?? "\u0000legacy"}\u0000${revocable.token}`; if (seenKeys.has(key)) return; seenKeys.add(key); + // The registration bound to THIS grant wins, with the preconfigured entry + // as the fallback — see the note on `StoredGrant.clientInformation`. + const bound = await parseClient(rawClient); grants.push({ - issuer: grantIssuer, + issuer, ...revocable, - clientInformation: await resolveClientInformation( - storage, - serverUrl, - grantIssuer, - ), + clientInformation: bound ?? preregistered, }); }; - /** Read one slot; a slot that throws is reported, not fatal to the rest. */ - const addSafely = async (issuer?: string): Promise => { + /** Read one slot; a slot that cannot be parsed is reported, not fatal. */ + const addSafely = async ( + issuer: string | undefined, + rawTokens: unknown, + rawClient: unknown, + ): Promise => { try { - await add(issuer); + await add(issuer, rawTokens, rawClient); } catch (err) { const where = issuer === undefined ? "the unkeyed slot" : `issuer ${issuer}`; @@ -576,27 +585,33 @@ async function collectGrants( } }; - // A `listIssuers` failure is fatal on its own — there is nothing to - // enumerate — but the ctx-less read below can still find a legacy grant, so - // it is recorded rather than thrown. - let issuers: string[] = []; - try { - issuers = await storage.listIssuers(serverUrl); - } catch (err) { - failures.push({ - status: "failed", - detail: `could not list the stored authorization servers: ${ - err instanceof Error ? err.message : String(err) - }`, - }); + for (const [issuer, slot] of Object.entries(snapshot.byIssuer)) { + await addSafely(issuer, slot?.tokens, slot?.clientInformation); } - for (const issuer of issuers) { - await addSafely(issuer); - } - await addSafely(); + await addSafely( + undefined, + snapshot.legacyTokens, + snapshot.legacyClientInformation, + ); return { grants, failures }; } +/** Parse stored tokens, or `undefined` when the slot held none. */ +async function parseTokens(raw: unknown): Promise { + return raw === undefined || raw === null + ? undefined + : await OAuthTokensSchema.parseAsync(raw); +} + +/** Parse stored client information, or `undefined` when the slot held none. */ +async function parseClient( + raw: unknown, +): Promise { + return raw === undefined || raw === null + ? undefined + : await OAuthClientInformationSchema.parseAsync(raw); +} + /** * Combine per-grant outcomes into the one this function reports. * diff --git a/core/auth/storage.ts b/core/auth/storage.ts index 73a46c35..181872ea 100644 --- a/core/auth/storage.ts +++ b/core/auth/storage.ts @@ -32,6 +32,26 @@ export interface SaveClientInformationOptions { issuer?: string; } +/** + * A server's revocation-relevant state, taken and deleted in one step by + * {@link OAuthStorage.takeRevocationSnapshot}. Values are as stored — unparsed + * — so validation can happen after the mutation rather than inside it. + */ +export interface RevocationSnapshot { + /** Credentials per authorization-server issuer (SEP-2352). */ + byIssuer: Record< + string, + { tokens?: unknown; clientInformation?: unknown } | undefined + >; + /** The legacy unkeyed slot, for entries predating issuer binding. */ + legacyTokens?: unknown; + legacyClientInformation?: unknown; + /** The static/preconfigured registration, if any. */ + preregisteredClientInformation?: unknown; + /** Cached authorization-server metadata, if discovery ever ran. */ + serverMetadata?: unknown; +} + export interface OAuthStorage { /** * Optional preload of persisted state into memory. Getters and setters load @@ -182,6 +202,27 @@ export interface OAuthStorage { */ clearDiscoveryState(serverUrl: string): Promise; + /** + * Atomically read everything RFC 7009 revocation needs, and clear the + * server's state in the same step (#2144). + * + * 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 fresh grant that the clear then deletes. Doing both against one + * synchronous view of the in-memory state closes that window, and the network + * requests are sent afterwards from the returned snapshot. + * + * The values are returned **unparsed**, exactly as stored. Schema validation + * is pure and belongs after the mutation — running it in between would + * reintroduce the `await` this exists to remove. + * + * ⚠️ In-process only. Nothing in this store takes a cross-process lock — + * every mutation here is a read-modify-write over a loaded snapshot — so a + * second Inspector writing concurrently can still lose an update. That is a + * property of the store, not of this method; `clear` alone has always had it. + */ + takeRevocationSnapshot(serverUrl: string): Promise; + /** * Tokens bound to **exactly** `issuer`, with no legacy-unkeyed fallback. * diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index ed0c7f56..873be9f9 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -14,8 +14,8 @@ import { mcpAuth } from "../auth/mcpAuth.js"; import type { OAuthStorage } from "../auth/storage.js"; import { parseOAuthState } from "../auth/utils.js"; import { + clearAndPlanRevocation, executeOAuthRevocation, - planOAuthRevocation, type TokenRevocationOutcome, } from "../auth/revocation.js"; import type { InspectorLogger } from "../logging/index.js"; @@ -500,11 +500,11 @@ export class OAuthManager { * Drop this server's local OAuth state, and revoke the grant at the * authorization server (RFC 7009). * - * 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. + * The state is taken and deleted in one atomic step, and the requests are + * sent afterwards from what was taken. Both halves matter: the requests need + * state the clear destroys, and the clear must not wait on the network, or a + * fresh authorization completing during a five-second revocation would 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 @@ -523,12 +523,14 @@ export class OAuthManager { } const serverUrl = this.getServerUrl(); - const plan = await planOAuthRevocation({ + // Takes the state and deletes it in ONE atomic storage step; separate + // reads followed by a separate clear are a check-then-act, and an OAuth + // completion landing between them would be destroyed by the clear. + const plan = await clearAndPlanRevocation({ serverUrl, storage: this.oauthConfig.storage, enabled: options?.revoke, }); - await this.oauthConfig.storage.clear(serverUrl); this.oauthFlowState = null; this.pendingAuthorizationScope = undefined;