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 f137c121..19bcd1a5 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -128,11 +128,10 @@ describe("clearStoredAuthForRelogin", () => { expect(blob.servers["https://example.com/mcp"]).toBeUndefined(); }); - // Both key spellings are cleared, but they are two spellings of one server: - // a second request would name a grant the first one already ended. The raw - // key here holds nothing, so this also proves the walk does not stop at the - // first empty one. - it("sends one request even though both key spellings are cleared", async () => { + // Both spellings resolve to the same stored entry, so this is one grant and + // must produce one request. It also proves the walk does not stop at the + // first empty key: the raw spelling holds nothing. + it("sends one request when both key spellings name the same grant", async () => { seed(); const fetchSpy = vi .spyOn(globalThis, "fetch") @@ -148,6 +147,75 @@ describe("clearStoredAuthForRelogin", () => { } }); + // Both keys are deleted, so both must be revoked: a stale entry under the + // other spelling is still a live grant at the authorization server, and + // dropping it locally without revoking is the exact leak this closes. + // The *reported* outcome is the normalised key's, matching the precedence + // `findStoredServerState` reads with — that is the grant a connect uses. + it("revokes a stale entry under the other spelling too", async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-both-")); + const file = path.join(dir, "oauth.json"); + const metadata = { + 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"], + }; + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + // The raw spelling the transport keyed by, holding a STALE grant. + "https://example.com": { + tokens: { + access_token: "stale-a", + token_type: "Bearer", + refresh_token: "stale-r", + }, + serverMetadata: metadata, + }, + // The normalised spelling, holding the CURRENT grant. + "https://example.com/": { + tokens: { + access_token: "live-a", + token_type: "Bearer", + refresh_token: "live-r", + }, + serverMetadata: metadata, + }, + }, + idpSessions: {}, + }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); + + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 200 })); + try { + await expect( + clearStoredAuthForRelogin("https://example.com"), + ).resolves.toMatchObject({ status: "revoked" }); + const tokens = fetchSpy.mock.calls.map((call) => + new URLSearchParams(String(call[1]?.body)).get("token"), + ); + // Normalised first, then the stale raw one — neither is left behind. + expect(tokens).toEqual(["live-r", "stale-r"]); + } finally { + fetchSpy.mockRestore(); + } + + const blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + expect(blob.servers["https://example.com"]).toBeUndefined(); + expect(blob.servers["https://example.com/"]).toBeUndefined(); + }); + it("deletes the local entry even when the request fails", async () => { const file = seed(); const fetchSpy = vi diff --git a/clients/cli/__tests__/relogin-revocation.test.ts b/clients/cli/__tests__/relogin-revocation.test.ts new file mode 100644 index 00000000..be091fb3 --- /dev/null +++ b/clients/cli/__tests__/relogin-revocation.test.ts @@ -0,0 +1,205 @@ +/** + * `--relogin` token revocation, driven through `runCli` (#2144). + * + * The helper's own tests cover `clearStoredAuthForRelogin({ revoke })`. What + * they cannot cover is the wiring above it: that Commander maps `--no-revoke`, + * that the per-server `oauth.revokeOnClear` reaches the call, and that a failed + * revocation is reported without changing what `--relogin` does. A regression + * in any of those turns revocation silently back on or off, which is precisely + * the class of bug the opt-out exists to make controllable. + * + * Every case here fails to connect on purpose — the fetch double refuses + * everything but the revocation endpoint. That is fine: revocation runs before + * the connect, so the assertions are about the requests made and the warnings + * printed, not about the exit code. + */ +import { describe, it, expect, afterEach, vi } from "vitest"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { resetNodeOAuthStorageCache } from "@inspector/core/auth/node/storage-node.js"; +import { runCli } from "./helpers/cli-runner.js"; + +const SERVER_URL = "https://example.com/mcp"; +const REVOKE_URL = "https://as.example.com/revoke"; + +let dir: string | undefined; +let prevPath: string | undefined; + +afterEach(() => { + if (prevPath === undefined) delete process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + else process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = prevPath; + prevPath = undefined; + resetNodeOAuthStorageCache(); + vi.restoreAllMocks(); + if (dir) { + fs.rmSync(dir, { recursive: true, force: true }); + dir = undefined; + } +}); + +/** Seed an OAuth store holding a revocable grant for {@link SERVER_URL}. */ +function seedStore(): void { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-revoke-e2e-")); + const file = path.join(dir, "oauth.json"); + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + [SERVER_URL]: { + 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: REVOKE_URL, + response_types_supported: ["code"], + }, + }, + }, + idpSessions: {}, + }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); +} + +/** + * Answer the revocation endpoint with `status` and refuse everything else, so + * the run stops at the connect instead of reaching the network. + */ +function stubFetch(status = 200): ReturnType { + return vi.spyOn(globalThis, "fetch").mockImplementation((input) => { + if (String(input instanceof Request ? input.url : input) === REVOKE_URL) { + return Promise.resolve(new Response(null, { status })); + } + return Promise.reject(new Error("network blocked in test")); + }) as ReturnType; +} + +function revocationCalls(spy: { mock: { calls: unknown[][] } }): unknown[][] { + return spy.mock.calls.filter( + (call) => String(call[0] as string) === REVOKE_URL, + ); +} + +/** A catalog holding one HTTP server, optionally opting out of revocation. */ +function writeCatalog(revokeOnClear?: boolean): string { + const catalogDir = fs.mkdtempSync( + path.join(os.tmpdir(), "cli-relogin-catalog-"), + ); + const catalogPath = path.join(catalogDir, "mcp.json"); + fs.writeFileSync( + catalogPath, + JSON.stringify({ + mcpServers: { + remote: { + type: "streamable-http", + url: SERVER_URL, + ...(revokeOnClear !== undefined && { oauth: { revokeOnClear } }), + }, + }, + }), + "utf8", + ); + return catalogPath; +} + +describe("--relogin token revocation", () => { + it("revokes the stored grant by default", async () => { + seedStore(); + const fetchSpy = stubFetch(); + + await runCli([ + "--relogin", + "--server-url", + SERVER_URL, + "--method", + "tools/list", + ]); + + expect(revocationCalls(fetchSpy)).toHaveLength(1); + }); + + // Commander turns `--no-revoke` into `options.revoke === false`; nothing else + // proves that mapping survives a refactor of the flag. + it("--no-revoke skips the request", async () => { + seedStore(); + const fetchSpy = stubFetch(); + + await runCli([ + "--relogin", + "--no-revoke", + "--server-url", + SERVER_URL, + "--method", + "tools/list", + ]); + + expect(revocationCalls(fetchSpy)).toHaveLength(0); + }); + + it("honors the per-server oauth.revokeOnClear opt-out", async () => { + seedStore(); + const catalogPath = writeCatalog(false); + const fetchSpy = stubFetch(); + try { + await runCli([ + "--catalog", + catalogPath, + "--server", + "remote", + "--relogin", + "--method", + "tools/list", + ]); + expect(revocationCalls(fetchSpy)).toHaveLength(0); + } finally { + fs.rmSync(path.dirname(catalogPath), { recursive: true, force: true }); + } + }); + + it("revokes for a catalog server that did not opt out", async () => { + seedStore(); + const catalogPath = writeCatalog(); + const fetchSpy = stubFetch(); + try { + await runCli([ + "--catalog", + catalogPath, + "--server", + "remote", + "--relogin", + "--method", + "tools/list", + ]); + expect(revocationCalls(fetchSpy)).toHaveLength(1); + } finally { + fs.rmSync(path.dirname(catalogPath), { recursive: true, force: true }); + } + }); + + // A failed revocation must be visible — the grant is still live at the + // authorization server — without turning `--relogin` into a failure, which + // is a local delete the user still gets. + it("warns on stderr when the authorization server refuses", async () => { + seedStore(); + stubFetch(500); + + const result = await runCli([ + "--relogin", + "--server-url", + SERVER_URL, + "--method", + "tools/list", + ]); + + expect(result.stderr).toMatch(/could not revoke the OAuth grant/i); + }); +}); diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index 9297eadc..782620db 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -4,6 +4,7 @@ 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"; @@ -40,13 +41,17 @@ export async function clearStoredAuthForRelogin( // this is about to empty. Best-effort — the outcome is returned for the // caller to report, never thrown, so `--relogin` succeeds regardless. // - // Only the key that actually holds the state is revoked from: the two keys - // below are two spellings of one server, so revoking from both would send a - // second request for a grant the first one already ended. + // 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. const revocation = options?.revoke === false ? undefined - : await revokeFirstStoredKey(storage, [raw, normalized]); + : await revokeStoredKeys(storage, [normalized, raw]); await storage.clear(raw); if (normalized !== raw) { await storage.clear(normalized); @@ -58,27 +63,43 @@ export async function clearStoredAuthForRelogin( } /** - * Revoke against the first of `keys` that actually has a revocable token, - * returning that attempt's outcome. Reports the last "nothing to do" answer - * when no key holds one, so the caller can still distinguish "no tokens" from - * "this authorization server advertises no revocation endpoint". + * Revoke every distinct grant held under `keys`, in order, and report the first + * key that had something to revoke. + * + * 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. + * + * The reported outcome is the first key's 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 authorization server advertises no revocation endpoint". */ -async function revokeFirstStoredKey( +async function revokeStoredKeys( storage: NodeOAuthStorage, keys: string[], ): Promise { const fetchFn = createProxyFetch() ?? fetch; - let last: TokenRevocationOutcome | undefined; + const revokedTokens = new Set(); + let reported: TokenRevocationOutcome | undefined; + let lastSkip: TokenRevocationOutcome | undefined; for (const key of new Set(keys)) { + const token = selectRevocableToken(await storage.getTokens(key))?.token; + if (token !== undefined && revokedTokens.has(token)) continue; const outcome = await revokeStoredOAuthTokens({ serverUrl: key, storage, fetchFn, }); - if (!(outcome.status === "skipped" && outcome.reason === "no_tokens")) { - return outcome; + if (outcome.status === "skipped" && outcome.reason === "no_tokens") { + lastSkip = outcome; + continue; } - last = outcome; + if (token !== undefined) revokedTokens.add(token); + reported ??= outcome; } - return last; + return reported ?? lastSkip; } diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index d637d35c..37a60bc6 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -360,6 +360,26 @@ function oneHttp(): Record { }; } +/** An HTTP server that opted out of RFC 7009 revocation on clear (#2144). */ +function oneHttpNoRevoke(): Record { + return { + web: { + config: { type: "streamable-http", url: "http://x" }, + settings: { + requestTimeout: 0, + metadata: {}, + headers: [], + env: [], + roots: [], + maxFetchRequests: 1000, + taskTtl: 0, + connectionTimeout: 0, + oauthRevokeOnClear: false, + }, + } as never, + }; +} + function oneEmaHttp(): Record { return { ema: { @@ -666,6 +686,12 @@ beforeEach(() => { new URL("https://auth.example/start"), ); h.clientSpies.clearOAuthTokens.mockReset(); + // #2144: the clear now reads the returned revocation outcome, so the reset + // default has to be an outcome rather than `undefined`. + h.clientSpies.clearOAuthTokens.mockResolvedValue({ + status: "skipped", + reason: "no_endpoint", + }); h.clientSpies.completeOAuthFlow.mockReset(); h.clientSpies.completeOAuthFlow.mockResolvedValue(undefined); h.clientSpies.getOAuthState.mockReset(); @@ -1351,6 +1377,39 @@ describe("App (mid-session auth lifecycle events)", () => { expect(h.disconnect).toHaveBeenCalled(); }); + // #2144 — revocation is on unless the entry opted out, and the opt-out has to + // reach the client, since that is where the RFC 7009 request is made. + it("asks for revocation by default when clearing", async () => { + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0); + expect(h.clientSpies.clearOAuthTokens).toHaveBeenCalledWith({ + revoke: true, + }); + }); + + it("forwards the per-server revocation opt-out", async () => { + const r = await mount(oneHttpNoRevoke()); + await press(r, ["a", "s"]); + await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0); + expect(h.clientSpies.clearOAuthTokens).toHaveBeenCalledWith({ + revoke: false, + }); + }); + + // A failed revocation leaves the grant live at the authorization server, so + // it has to be visible rather than swallowed — the local clear succeeded and + // would otherwise look like the whole operation did. + it("reports a failed revocation without failing the clear", async () => { + h.clientSpies.clearOAuthTokens.mockResolvedValue({ + status: "failed", + detail: "unreachable", + }); + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await expectFrame(r, "unreachable"); + }); + const stepUpChallenge = { reason: "insufficient_scope" as const, requiredScopes: ["env:read"], diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 9e626f9f..e36db5fc 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -136,7 +136,10 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => { } return Promise.resolve(true); }); - clearOAuthTokens = vi.fn().mockResolvedValue(undefined); + // #2144: the clear path reads the returned RFC 7009 outcome. + clearOAuthTokens = vi + .fn() + .mockResolvedValue({ status: "skipped", reason: "no_endpoint" }); } const instances: FakeInspectorClient[] = []; return { diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 91bf2c29..aee37db1 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1326,8 +1326,18 @@ function App() { const handleClearStoredOAuthFromSettings = useCallback(() => { if (!settingsModalTarget) return; - void clearServerOAuthAndDisconnect(settingsModalTarget); - }, [settingsModalTarget, clearServerOAuthAndDisconnect]); + // Clear from *inside* the settings modal, so the draft is what the user is + // looking at: `settingsModalTarget` comes from the persisted `servers` + // list, and edits only reach it after the save debounce. Reading it would + // mean toggling "Revoke tokens on clear" and immediately clearing used the + // previous value — revoking despite an opt-out, or skipping despite an + // opt-in (#2144). The draft is the live answer; `settingsModalValue` falls + // back to the persisted entry whenever there is no draft. + void clearServerOAuthAndDisconnect({ + ...settingsModalTarget, + settings: settingsDraft ?? settingsModalTarget.settings, + }); + }, [settingsModalTarget, settingsDraft, clearServerOAuthAndDisconnect]); const onSettingsModalClose = useCallback(() => { flushSettingsDraft(); diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index 80e05a4f..22585119 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -9,6 +9,22 @@ import { revokeToken, selectRevocableToken, } from "@inspector/core/auth/revocation.js"; +import type { InspectorLogger } from "@inspector/core/logging/index.js"; + +/** A fully-typed `InspectorLogger` double, so the mock's shape is checked. */ +function fakeLogger(): InspectorLogger { + return { + level: "info", + fatal: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + info: vi.fn(), + debug: vi.fn(), + trace: vi.fn(), + silent: vi.fn(), + child: vi.fn(() => fakeLogger()), + }; +} const SERVER_URL = "https://mcp.example.com/mcp"; const REVOKE_URL = "https://as.example.com/revoke"; @@ -72,19 +88,47 @@ describe("revocationAuthMethods", () => { ).toEqual(["client_secret_post"]); }); - // RFC 8414 gives the revocation endpoint the token endpoint's default when it - // advertises no list of its own. - it("falls back to the token endpoint's list", () => { + // RFC 8414 §2 defaults an omitted revocation list to `client_secret_basic` — + // it does NOT inherit the token endpoint's. Inheriting would send POST + // credentials to an endpoint that never advertised that method. + it("ignores the token endpoint's list", () => { expect( revocationAuthMethods( - metadata({ token_endpoint_auth_methods_supported: ["none"] }), + metadata({ + token_endpoint_auth_methods_supported: ["client_secret_post"], + }), ), - ).toEqual(["none"]); + ).toEqual([]); }); - it("yields nothing when neither is advertised", () => { + it("yields nothing when the revocation list is absent", () => { expect(revocationAuthMethods(metadata())).toEqual([]); }); + + // The empty list is not "no authentication": it is what makes the SDK apply + // RFC 8414's actual default. + it("an empty list resolves to the RFC 8414 default", () => { + const { init } = buildRevocationRequest({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + clientInformation: { client_id: "cid", client_secret: "sec" }, + supportedAuthMethods: revocationAuthMethods(metadata()), + }); + expect(headerOf(init, "Authorization")).toBe(`Basic ${btoa("cid:sec")}`); + }); + + it("an empty list leaves a public client unauthenticated", () => { + const { init } = buildRevocationRequest({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + clientInformation: { client_id: "cid" }, + supportedAuthMethods: revocationAuthMethods(metadata()), + }); + expect(headerOf(init, "Authorization")).toBeUndefined(); + expect(body(init).get("client_id")).toBe("cid"); + }); }); describe("buildRevocationRequest", () => { @@ -203,6 +247,26 @@ describe("revokeToken", () => { }); }); + // The web fetch is `createRemoteFetch`, which re-issues the call as a POST to + // `/api/fetch` and drops `init.signal`; the backend's outbound fetch gets no + // signal either. A signal-only bound is therefore inert on exactly the path + // the timeout exists for, so the deadline has to hold against a fetch that + // ignores the signal entirely. + it("gives up on a fetch that never settles and ignores the signal", async () => { + const outcome = await revokeToken({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + supportedAuthMethods: [], + fetchFn: () => new Promise(() => {}), + timeoutMs: 20, + }); + expect(outcome).toMatchObject({ status: "failed", endpoint: REVOKE_URL }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "timed out", + ); + }); + // The teardown is already committed by the time this runs, so a wedged // authorization server must not be able to hold it open. it("bounds the request with an abort signal", async () => { @@ -334,18 +398,7 @@ describe("revokeStoredOAuthTokens", () => { it("warns and reports rather than throwing when the request fails", async () => { await seed(); - const warn = vi.fn(); - const logger = { - level: "info", - fatal: vi.fn(), - error: vi.fn(), - warn, - info: vi.fn(), - debug: vi.fn(), - trace: vi.fn(), - silent: vi.fn(), - child: vi.fn(), - }; + const logger = fakeLogger(); const fetchFn = vi.fn( async () => new Response(null, { status: 500, statusText: "Boom" }), ); @@ -354,52 +407,38 @@ describe("revokeStoredOAuthTokens", () => { serverUrl: SERVER_URL, storage, fetchFn, - logger: logger as unknown as Parameters< - typeof revokeStoredOAuthTokens - >[0]["logger"], + logger, }); expect(outcome.status).toBe("failed"); - expect(warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledTimes(1); }); it("logs the no-endpoint case at debug", async () => { await seed({ revocation_endpoint: undefined }); - const debug = vi.fn(); - const logger = { - level: "info", - fatal: vi.fn(), - error: vi.fn(), - warn: vi.fn(), - info: vi.fn(), - debug, - trace: vi.fn(), - silent: vi.fn(), - child: vi.fn(), - }; + const logger = fakeLogger(); await revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn: vi.fn(), - logger: logger as unknown as Parameters< - typeof revokeStoredOAuthTokens - >[0]["logger"], + logger, }); - expect(debug).toHaveBeenCalledTimes(1); + 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. + // 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 store read failure as failed", async () => { - const broken = { - ...storage, - getTokens: () => Promise.reject(new Error("store unreadable")), - } as unknown as BrowserOAuthStorage; + vi.spyOn(storage, "getTokens").mockRejectedValue( + new Error("store unreadable"), + ); await expect( revokeStoredOAuthTokens({ serverUrl: SERVER_URL, - storage: broken, + storage, fetchFn: vi.fn(), }), ).resolves.toEqual({ status: "failed", detail: "store unreadable" }); 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 3d0fd881..ca1cf41f 100644 --- a/clients/web/src/test/integration/auth/revocation-e2e.test.ts +++ b/clients/web/src/test/integration/auth/revocation-e2e.test.ts @@ -138,6 +138,31 @@ describe("OAuth token revocation (RFC 7009)", () => { return storage; } + // The fixture enforces RFC 7009 §2.1 client authentication, so this is what + // makes the passing cases below mean something: a request with the wrong + // credentials is refused, and the endpoint is not simply answering 200 to + // anything. + it("refuses a revocation request with the wrong client secret", async () => { + const response = await fetch(`${serverUrl}/oauth/revoke`, { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + Authorization: `Basic ${Buffer.from(`${CLIENT_ID}:wrong`).toString("base64")}`, + }, + body: new URLSearchParams({ token: "anything" }), + }); + expect(response.status).toBe(401); + }); + + it("refuses a revocation request naming no client at all", async () => { + const response = await fetch(`${serverUrl}/oauth/revoke`, { + method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + body: new URLSearchParams({ token: "anything" }), + }); + expect(response.status).toBe(401); + }); + it("advertises a revocation endpoint", () => { expect(metadata.revocation_endpoint).toBe(`${serverUrl}/oauth/revoke`); }); diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index 91167996..92c5f234 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -87,20 +87,23 @@ export function selectRevocableToken( } /** - * The client-authentication methods to consider for the revocation endpoint. + * The client-authentication methods the revocation endpoint advertises. * - * RFC 8414 gives `revocation_endpoint_auth_methods_supported` the same default - * as the token endpoint's when it is omitted, so fall through to - * `token_endpoint_auth_methods_supported` before giving up. An empty result - * lets {@link selectClientAuthMethod} apply the SDK's own default rather than - * this module inventing a second one. + * RFC 8414 §2 gives `revocation_endpoint_auth_methods_supported` a default of + * **`client_secret_basic`** when it is omitted — it does *not* inherit + * `token_endpoint_auth_methods_supported`. Falling through to the token + * endpoint's list would make metadata advertising only `client_secret_post` + * there send POST credentials to a revocation endpoint that never advertised + * that method, which a strict server rejects. + * + * An empty result is returned rather than a literal `["client_secret_basic"]` + * because that is what makes {@link selectClientAuthMethod} apply exactly the + * RFC's default — `client_secret_basic` when the client holds a secret, `none` + * when it does not — while still honoring a `token_endpoint_auth_method` the + * client's own registration declares. */ export function revocationAuthMethods(metadata: OAuthMetadata): string[] { - return ( - metadata.revocation_endpoint_auth_methods_supported ?? - metadata.token_endpoint_auth_methods_supported ?? - [] - ); + return metadata.revocation_endpoint_auth_methods_supported ?? []; } export interface RevocationRequestParams { @@ -195,10 +198,18 @@ export async function revokeToken( const { url, init } = buildRevocationRequest(params); const timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS; try { - const response = await params.fetchFn(url, { - ...init, - signal: AbortSignal.timeout(timeoutMs), - }); + // The signal alone is not enough to bound this. In the browser the fetch is + // `createRemoteFetch`, which re-issues the call as a POST to `/api/fetch` + // and does not forward `init.signal`; the backend's outbound fetch gets no + // signal either. So a wedged authorization server would hold the teardown + // open indefinitely on exactly the path the timeout exists for. The race is + // what actually enforces the deadline; the signal is kept because it does + // cancel the direct-fetch paths (CLI, TUI, backend) rather than merely + // abandoning them. + const response = await withDeadline( + params.fetchFn(url, { ...init, signal: AbortSignal.timeout(timeoutMs) }), + timeoutMs, + ); if (!response.ok) { return { status: "failed", @@ -221,6 +232,34 @@ export async function revokeToken( } } +/** + * Reject with a timeout error if `promise` has not settled within `timeoutMs`. + * + * The underlying request is abandoned rather than cancelled — nothing here can + * cancel a fetch the proxy already stripped the signal from. That is the right + * trade for this caller: the point is that the *teardown* proceeds, and the + * response, if it ever arrives, is a revocation we no longer need to wait for. + * The timer is cleared on the settled path so a caller is never held awake by it. + */ +async function withDeadline( + promise: Promise, + timeoutMs: number, +): Promise { + let timer: ReturnType | undefined; + const deadline = new Promise((_resolve, reject) => { + timer = setTimeout( + () => + reject(new Error(`revocation request timed out after ${timeoutMs}ms`)), + timeoutMs, + ); + }); + try { + return await Promise.race([promise, deadline]); + } finally { + if (timer !== undefined) clearTimeout(timer); + } +} + export interface RevokeStoredOAuthTokensParams { serverUrl: string; storage: OAuthStorage; diff --git a/test-servers/src/test-server-oauth.ts b/test-servers/src/test-server-oauth.ts index 44454806..90d4fae1 100644 --- a/test-servers/src/test-server-oauth.ts +++ b/test-servers/src/test-server-oauth.ts @@ -117,7 +117,7 @@ export function setupOAuthRoutes( setupAuthorizationEndpoint(app, config); setupTokenEndpoint(app, config); if (config.supportRevocation !== false) { - setupRevocationEndpoint(app); + setupRevocationEndpoint(app, config); } if (config.supportDCR) { setupDCREndpoint(app); @@ -711,21 +711,36 @@ function setupTokenEndpoint( * That is why the Inspector sends one request naming the refresh token, and a * fixture that ignored the linkage would let a regression through silently. * - * Client authentication is accepted but not enforced: the point of the fixture - * is the revocation semantics, and every client this repo drives it with is - * either public or has already authenticated at the token endpoint. + * Client authentication is **enforced**, not merely accepted. RFC 7009 §2.1 + * requires it of a confidential client, and a fixture that skipped the check + * would answer 200 to a request carrying no `Authorization` header at all — at + * which point the end-to-end test claiming to prove the Inspector authenticates + * correctly proves nothing. Both RFC 6749 §2.3.1 forms are accepted (Basic and + * the request body), as is a public client identifying itself by `client_id`. */ -function setupRevocationEndpoint(app: express.Application): void { +function setupRevocationEndpoint( + app: express.Application, + config: OAuthConfig, +): void { app.post( "/oauth/revoke", express.urlencoded({ extended: true }), - (req: Request, res: Response) => { + async (req: Request, res: Response) => { const token: unknown = req.body?.token; if (typeof token !== "string" || token === "") { res.status(400).json({ error: "invalid_request" }); return; } + const authenticated = await authenticateRevocationClient(req, config); + if (!authenticated) { + res + .status(401) + .set("WWW-Authenticate", 'Basic realm="revoke"') + .json({ error: "invalid_client" }); + return; + } + const refreshTokenData = refreshTokens.get(token); if (refreshTokenData) { for (const accessToken of refreshTokenData.accessTokens) { @@ -744,6 +759,52 @@ function setupRevocationEndpoint(app: express.Application): void { ); } +/** + * Authenticate the caller of `/oauth/revoke` (RFC 7009 §2.1). + * + * Credentials may arrive either way RFC 6749 §2.3.1 allows — an `Authorization: + * Basic` header or `client_id`/`client_secret` in the form body — because the + * Inspector picks between them from the metadata, and a fixture that only read + * one would silently pass a request whose credentials went to the other place. + * + * A request naming no client at all is rejected: the Inspector always sends at + * least `client_id` once it holds any client information, so an unidentified + * request means it lost track of its credentials. + */ +async function authenticateRevocationClient( + req: Request, + config: OAuthConfig, +): Promise { + let clientId: string | undefined; + let clientSecret: string | undefined; + + const authorization = req.get("authorization"); + if (authorization?.startsWith("Basic ")) { + const decoded = Buffer.from( + authorization.slice("Basic ".length), + "base64", + ).toString("utf8"); + const separator = decoded.indexOf(":"); + if (separator === -1) return false; + clientId = decoded.slice(0, separator); + clientSecret = decoded.slice(separator + 1); + } else { + const bodyId: unknown = req.body?.client_id; + const bodySecret: unknown = req.body?.client_secret; + if (typeof bodyId === "string") clientId = bodyId; + if (typeof bodySecret === "string") clientSecret = bodySecret; + } + + if (!clientId) return false; + const client = await findClient(clientId, config); + if (!client) return false; + // A client registered with a secret must present it; a public one must not be + // asked for one it never had. + return client.clientSecret === undefined + ? true + : clientSecret === client.clientSecret; +} + /** * Set up Dynamic Client Registration endpoint */