diff --git a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts index 66da27f1..07e7496c 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -287,10 +287,11 @@ describe("clearStoredAuthForRelogin", () => { } }); - // A token the AS never accepted is not spent, so the other key — which may - // carry the credentials or metadata that would have worked — must still be - // tried rather than skipped as a duplicate. - it("does not treat a failed attempt as having ended the grant", async () => { + // Keys are revoked from independently. Deduplicating them could only be + // done by pre-reading one token, which would skip the second key's OTHER + // issuer-bound grants along with it; a duplicate RFC 7009 request is + // harmless (§2.2 makes an unknown token a success), a missed one is not. + it("revokes from both keys even when they hold the same token", async () => { seedBothSpellings("same-r", "same-r"); const fetchSpy = vi .spyOn(globalThis, "fetch") diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index 44f7ac58..6cd96b5a 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -4,7 +4,6 @@ import { } from "@inspector/core/auth/node/storage-node.js"; import { revokeStoredOAuthTokens, - selectRevocableToken, type TokenRevocationOutcome, } from "@inspector/core/auth/revocation.js"; import { createProxyFetch } from "@inspector/core/mcp/node/proxyFetch.js"; @@ -44,10 +43,10 @@ export async function clearStoredAuthForRelogin( // 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; a second key holding the *same* token is skipped rather than - // revoked twice. + // 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 = options?.revoke === false ? undefined @@ -63,22 +62,26 @@ export async function clearStoredAuthForRelogin( } /** - * Revoke every distinct grant held under `keys`, in order, and report the first - * key that had something to revoke. + * Revoke every grant held under `keys`, 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. Two keys holding the *same* token are one grant, so the duplicate is - * skipped rather than producing a second request for something already ended — - * but only once one has actually been *revoked*, since a failed or unsupported - * attempt has ended nothing and the other key may hold the credentials that - * would have worked. + * closes. * - * Reporting prefers a **failure** over any earlier success — a grant still live - * at the authorization server is what the user needs to hear about, and an - * earlier 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 + * 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. + * + * 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 * `findStoredServerState` precedence means the grant the CLI would actually * have connected with. When no key holds a token, the last "nothing to do" * answer is returned so the caller can still tell "no tokens" from "this @@ -89,26 +92,9 @@ async function revokeStoredKeys( keys: string[], ): Promise { const fetchFn = createProxyFetch() ?? fetch; - const revokedTokens = new Set(); let reported: TokenRevocationOutcome | undefined; let lastSkip: TokenRevocationOutcome | undefined; for (const key of new Set(keys)) { - // This read is OUTSIDE `revokeStoredOAuthTokens`'s own best-effort catch, - // so it needs its own: `getTokens` parses through `OAuthTokensSchema` and - // rejects on a persisted token that no longer validates. Letting that - // escape would abandon the *local* delete `--relogin` promises, over a - // grant we could not have revoked anyway. - let token: string | undefined; - try { - token = selectRevocableToken(await storage.getTokens(key))?.token; - } catch (err) { - reported ??= { - status: "failed", - detail: err instanceof Error ? err.message : String(err), - }; - continue; - } - if (token !== undefined && revokedTokens.has(token)) continue; const outcome = await revokeStoredOAuthTokens({ serverUrl: key, storage, @@ -118,16 +104,6 @@ async function revokeStoredKeys( lastSkip = outcome; continue; } - // Only a token the authorization server actually accepted counts as spent. - // Marking one revoked on a `failed` or `no_endpoint` outcome would skip a - // duplicate entry under the other key that may carry usable metadata or - // credentials — the one chance left to end that grant. - if (token !== undefined && outcome.status === "revoked") { - revokedTokens.add(token); - } - // A failure outranks an earlier success for reporting: a grant that is - // still live is what the user needs to hear about, and reporting the - // success would print no warning while a stale grant survives. if (outcome.status === "failed") { if (reported?.status !== "failed") reported = outcome; } else { diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index efa507ae..c999cc90 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -617,13 +617,15 @@ describe("revokeStoredOAuthTokens", () => { it("reports a non-Error store failure as failed", async () => { vi.spyOn(storage, "listIssuers").mockRejectedValue("store exploded"); - await expect( - revokeStoredOAuthTokens({ - serverUrl: SERVER_URL, - storage, - fetchFn: vi.fn(), - }), - ).resolves.toEqual({ status: "failed", detail: "store exploded" }); + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + 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 () => { @@ -631,12 +633,104 @@ describe("revokeStoredOAuthTokens", () => { new Error("store unreadable"), ); - await expect( - revokeStoredOAuthTokens({ - serverUrl: SERVER_URL, - storage, - fetchFn: vi.fn(), - }), - ).resolves.toEqual({ status: "failed", detail: "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", + }; + }, + ); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + // The good grant was still revoked... + expect(fetchFn).toHaveBeenCalledTimes(1); + expect( + new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"), + ).toBe("r-good"); + // ...and the unreadable slot is reported rather than swallowed. + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "corrupt slot", + ); + }); + + // A token is only meaningful to the AS that minted it, so two issuers minting + // the same opaque string are two grants. Collapsing them would drop the + // second before the issuer-mismatch check could even report it. + it("does not collapse two issuers that minted the same token value", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as-a.example.com" }), + ); + for (const issuer of [ + "https://as-a.example.com", + "https://as-b.example.com", + ]) { + await storage.saveTokens( + SERVER_URL, + { access_token: "same", token_type: "Bearer", refresh_token: "same-r" }, + { issuer }, + ); + } + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + // Only as-a matches the cached metadata, so only it is revocable — but + // as-b is REPORTED rather than silently dropped, which is what collapsing + // by token alone would have done. + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "as-b.example.com", + ); }); }); diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index 8755efb2..4d16b216 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -346,8 +346,20 @@ interface StoredGrant { clientInformation?: OAuthClientInformation; } +/** What {@link collectGrants} found, including the slots it could not read. */ +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 + * *beside* the successes rather than instead of them. + */ + failures: TokenRevocationOutcome[]; +} + /** - * Every grant `clear(serverUrl)` is about to delete, deduplicated by token. + * Every grant `clear(serverUrl)` is about to delete. * * `clear` drops **every** `byIssuer` slot, so reading only the context-free * (active-issuer) token would leave an earlier authorization server's grant @@ -355,31 +367,46 @@ interface StoredGrant { * exists to close, just moved one level down. A server that authorized against * issuers A and B has two grants here, not one. * - * The ctx-less read is included last and deduped: on an issuer-bound entry it - * returns the active issuer's token, which the loop above has already seen; on - * a legacy entry it is the only thing that returns anything at all. 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 it. + * 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. + * + * 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. So it is + * suppressed when its token was already taken from any slot, which is exactly + * the first case and never the second. 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. */ async function collectGrants( storage: OAuthStorage, serverUrl: string, -): Promise { +): Promise { const grants: StoredGrant[] = []; - const seen = new Set(); + const failures: TokenRevocationOutcome[] = []; + const seenKeys = new Set(); + const slotTokens = new Set(); const add = async (issuer?: string): Promise => { - // Exact read for an enumerated issuer. `getTokens(serverUrl, issuer)` falls - // back to the legacy unkeyed slot when that issuer holds none, and treating - // the fallback as the issuer's would label an old, unbound token with a - // newly discovered authorization server and send it there. const tokens = issuer === undefined ? await storage.getTokens(serverUrl) : await storage.getIssuerTokens(serverUrl, issuer); const revocable = selectRevocableToken(tokens); - if (!revocable || seen.has(revocable.token)) return; - seen.add(revocable.token); + if (!revocable) return; + + if (issuer === undefined) { + // Same grant as the active issuer's slot, already collected. + if (slotTokens.has(revocable.token)) return; + } else { + const key = `${issuer}\u0000${revocable.token}`; + if (seenKeys.has(key)) return; + seenKeys.add(key); + slotTokens.add(revocable.token); + } + grants.push({ issuer, ...revocable, @@ -391,11 +418,41 @@ async function collectGrants( }); }; - for (const issuer of await storage.listIssuers(serverUrl)) { - await add(issuer); + /** Read one slot; a slot that throws is reported, not fatal to the rest. */ + const addSafely = async (issuer?: string): Promise => { + try { + await add(issuer); + } catch (err) { + const where = + issuer === undefined ? "the unkeyed slot" : `issuer ${issuer}`; + failures.push({ + status: "failed", + detail: `could not read the stored grant for ${where}: ${ + err instanceof Error ? err.message : String(err) + }`, + }); + } + }; + + // 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) + }`, + }); } - await add(); - return grants; + for (const issuer of issuers) { + await addSafely(issuer); + } + await addSafely(); + return { grants, failures }; } /** @@ -430,16 +487,34 @@ async function computeOutcome( try { // Grants first. They are the cheapest disqualifier, and a server with no // stored grant should not provoke a metadata read at all. - const grants = await collectGrants(storage, serverUrl); - if (grants.length === 0) return { status: "skipped", reason: "no_tokens" }; + // + // `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 metadata = await storage.getServerMetadata(serverUrl); - if (!metadata) return { status: "skipped", reason: "no_metadata" }; + if (!metadata) { + return aggregateOutcomes([ + ...failures, + { status: "skipped", reason: "no_metadata" }, + ]); + } const endpoint = metadata.revocation_endpoint; - if (!endpoint) return { status: "skipped", reason: "no_endpoint" }; + if (!endpoint) { + return aggregateOutcomes([ + ...failures, + { status: "skipped", reason: "no_endpoint" }, + ]); + } const supportedAuthMethods = revocationAuthMethods(metadata); - const outcomes: TokenRevocationOutcome[] = []; + const outcomes: TokenRevocationOutcome[] = [...failures]; 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