diff --git a/AGENTS.md b/AGENTS.md index 1fc7f2d3..0729ca37 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -101,6 +101,29 @@ v2/main/ │ │ # needs to know WHICH candidate │ │ # answered, since that is the base its │ │ # token request is made against — #2110; +│ │ # revocation.ts RFC 7009 token revocation — +│ │ # the request the three clear paths send +│ │ # BEFORE wiping local state, since the +│ │ # token, the client credentials and the +│ │ # discovered `revocation_endpoint` all live +│ │ # in the store the clear empties. Names the +│ │ # REFRESH token when there is one (§2.1 asks +│ │ # the AS to invalidate the access tokens +│ │ # under the same grant, so one request +│ │ # covers both). BEST-EFFORT by construction: +│ │ # every path returns a +│ │ # TokenRevocationOutcome rather than +│ │ # throwing — no advertised endpoint means +│ │ # nothing is sent and the AS behaves exactly +│ │ # as before, and a network error / non-2xx / +│ │ # timeout is reported so the local clear +│ │ # always finishes. Opt out per server with +│ │ # `oauth.revokeOnClear`, read at CLEAR time +│ │ # rather than connect time. `lost_authorization_state` +│ │ # recovery passes `revoke: false` — it clears a +│ │ # half-finished flow to retry it, and an +│ │ # authorization that never completed has no +│ │ # grant to revoke — #2144; │ │ # scopes.ts SEP-2350 scope union, oauthUx.ts │ │ # shared copy, mcpAuth.ts force-reauthorization, │ │ # issuerBinding.ts SEP-2352 callback-leg failure diff --git a/README.md b/README.md index ccf9d600..1f285a31 100644 --- a/README.md +++ b/README.md @@ -31,6 +31,7 @@ inspector/ │ └── launcher/ # Shared launcher — provides the `mcp-inspector` bin, dispatches to web/cli/tui ├── core/ # Shared code consumed via the `@inspector/core` alias (no package.json) │ ├── auth/ # OAuth: providers, discovery, storage, endpoint overrides, mid-session recovery (browser/node/remote backends); +│ │ # RFC 7009 token revocation on clear (revocation.ts); │ │ # plus per-server secret storage — the keychain/file/memory SecretStore │ │ # implementations, the selection policy, and the descriptor the banner and UI report │ ├── client/ # Install-level client config (`client.json`): browser-safe parse/validate + Node load/save, remote backend, secrets @@ -159,6 +160,7 @@ Each config below is a ready-made server for exercising one feature by hand. Loa | `rfc6570-templates-http.json` | Resources tab: RFC 6570 resource-template expansion | [#1919](https://github.com/modelcontextprotocol/inspector/issues/1919) | | `advertised-extensions-http.json` | Tool registration gated on advertised extensions | [#1739](https://github.com/modelcontextprotocol/inspector/issues/1739) | | `oauth-custom-resource-metadata-http.json` **(legacy era)** | OAuth discovery driven by the challenge's `resource_metadata` | [#2071](https://github.com/modelcontextprotocol/inspector/issues/2071) | +| `oauth-revocation-http.json` / `oauth-no-revocation-http.json` **(legacy era)** | RFC 7009 token revocation on clear, with and without a `revocation_endpoint` | [#2144](https://github.com/modelcontextprotocol/inspector/issues/2144) | | `logging-{legacy,modern}-http.json` | Logging, both eras | [#1629](https://github.com/modelcontextprotocol/inspector/issues/1629) | | `subscriptions-{legacy,modern}-http.json` | Resource subscriptions, both eras | [#1630](https://github.com/modelcontextprotocol/inspector/issues/1630) | | `subscriptions-never-acknowledged-http.json` | A `subscriptions/listen` answered with a bare result | [#2097](https://github.com/modelcontextprotocol/inspector/issues/2097) | @@ -438,6 +440,21 @@ The same server is worth running against `--cli` / `--tui`, which reach it by a The value now rides the normalized `AuthChallenge` as a string — it has to be serializable, because the web client's challenge crosses the remote-backend boundary as JSON — and is converted to a `URL` at the OAuth boundary, where it is handed to `auth()` as `resourceMetadataUrl` and to the CIMD pre-registration probe, which runs *before* `auth()` and would otherwise do its own default-location discovery. A malformed value is ignored rather than surfaced, matching the SDK's own `WWW-Authenticate` parser: discovery falls back to the default locations instead of failing the whole authorization on a bad header. The callback leg needs nothing extra — SDK `auth()` persists the URL in its discovery state, so it survives both the web full-page redirect and the CLI/TUI loopback callback. +#### Revoking tokens on clear (RFC 7009) + +`oauth-revocation-http.json` and `oauth-no-revocation-http.json` are the same OAuth-protected server (combined AS + resource, DCR, refresh tokens) differing in one thing: the first advertises a `revocation_endpoint`, the second advertises none. Plain streamable-HTTP — connect with the **default (legacy)** protocol era. + +Add either server, connect and complete authorization, then use **Clear OAuth state and disconnect** (Server Settings → Authorization) and watch the Network tab. + +- On `oauth-revocation-http.json` a `POST /oauth/revoke` goes out **before** the local state is dropped, naming the **refresh token**. RFC 7009 §2.1 asks the authorization server to invalidate the access tokens issued under the same grant, so one request ends both halves — the fixture implements that linkage, so re-sending the old bearer token to `/mcp` afterwards gets a 401. +- On `oauth-no-revocation-http.json` nothing is sent at all, and the clear behaves exactly as it did before the feature existed. That no-op path is what makes this safe against every authorization server with no RFC 7009 support ([#2144](https://github.com/modelcontextprotocol/inspector/issues/2144)). + +On the broken build both servers behaved like the second: the Inspector deleted its local copy and the grant stayed valid at the authorization server until it expired on its own — which for a refresh token is a long time, by design. + +Uncheck **Revoke tokens on clear** in the same panel (persisted as `oauth.revokeOnClear: false`) and the first server behaves like the second. That is not only an escape hatch: a client that disconnects still holding live tokens is a case worth reproducing when the server is the thing under test. + +The same behavior is reachable from the other clients — the TUI's **Clear OAuth State**, and the CLI's `--relogin` (with `--no-revoke` as the per-run opt-out). + #### Logging, both eras `logging-legacy-http.json` and `logging-modern-http.json` both serve `logging: true` plus a `send_notification` tool that emits a `notifications/message` at a chosen level. The legacy one is a plain streamable-HTTP server; the modern one sets `transport.modern: true`. diff --git a/clients/cli/README.md b/clients/cli/README.md index de89799d..25117bb1 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -123,8 +123,24 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--strict` | With `--method tools/list`: report tool-schema portability problems in full (path, issue, suggested fix) on stderr, and exit `6` if any is error-severity. Without it, a one-line count is printed instead. See [Schema portability](#schema-portability---strict). | | `--format ` | Output format. `text` (default) pretty-prints the result. `json` emits a single JSON object on stdout (`{ "result": … }`, plus `{ "appInfo": … }` as a sibling key for App tools) with no banners, so the whole output pipes cleanly into `jq`. | | `--relogin` | Delete stored OAuth for this server URL from the shared store before connect; interactive login still only runs if the server requires auth. Requires an HTTP/SSE URL (rejected for stdio). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth` / catalog short-circuits. | +| `--no-revoke` | With `--relogin`, skip the [RFC 7009](https://datatracker.ietf.org/doc/html/rfc7009) revocation request that would otherwise end the grant at the authorization server before the local state is deleted. The per-server `oauth.revokeOnClear` setting is the persistent form of the same opt-out; either one is enough to skip it. See [Revoking on `--relogin`](#revoking-on---relogin). | | `--stored-auth-only` | **CI / non-interactive safe:** never start interactive OAuth / step-up (and never auto-open a browser); use the shared store if present, otherwise fail immediately with `auth_required`. Prefer this over a bare pipe/CI run that would otherwise attempt interactive login. | +#### Revoking on `--relogin` + +`--relogin` deletes this server's stored OAuth state so the next connect cannot silently reuse it. By default it now also **revokes the grant at the authorization server** first, per [RFC 7009](https://datatracker.ietf.org/doc/html/rfc7009) ([#2144](https://github.com/modelcontextprotocol/inspector/issues/2144)) — otherwise the delete is invisible to the AS, and the access token, plus the refresh token when one was issued, stay valid there until they expire on their own. + +The request names the refresh token when there is one: RFC 7009 §2.1 asks the authorization server to invalidate the access tokens issued under the same grant, so a single request covers both. + +It is best-effort and never changes the exit code. An authorization server that advertises no `revocation_endpoint` gets no request at all; a network error, a non-2xx, or the short timeout prints a one-line warning on stderr and the local delete proceeds either way. + +Turn it off per run with `--no-revoke`, or per server with `oauth.revokeOnClear: false` in the catalog — either is enough, and neither can turn it on for the other. Disconnecting while still holding live tokens is a case worth reproducing when the server under test is the thing being debugged. + +```bash +mcp-inspector --cli --server-url https://example.com/mcp --relogin --method tools/list +mcp-inspector --cli --server-url https://example.com/mcp --relogin --no-revoke --method tools/list +``` + `servers/show` redacts secret-bearing fields (`env` values, sensitive headers, sensitive `settings.metadata` keys whose whole value is replaced whether or not it is structured, `requestInit` / `eventSourceInit` headers, `oauthClientSecret`). It does **not** scrub credentials embedded in a server `url` (userinfo or query tokens) or in stdio `args` — treat `detail` / raw URL fields as potentially sensitive before pasting into issues. #### App probing (`--app-info`) and machine-readable output (`--format json`) 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 1da26d7f..f137c121 100644 --- a/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts +++ b/clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, afterEach } from "vitest"; +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"; @@ -66,6 +66,139 @@ describe("clearStoredAuthForRelogin", () => { expect(blob.servers["not a url"]).toBeUndefined(); }); + // #2144 — RFC 7009. The request has to go out *before* the delete, since it + // is built from the token, the client id and the cached metadata the delete + // removes. + describe("token revocation", () => { + function seed(over: Record = {}): string { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-revoke-")); + const file = path.join(dir, "oauth.json"); + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + "https://example.com/mcp": { + 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"], + }, + ...over, + }, + }, + idpSessions: {}, + }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); + return file; + } + + it("revokes the stored grant before deleting it", async () => { + const file = seed(); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 200 })); + try { + await expect( + clearStoredAuthForRelogin("https://example.com/mcp"), + ).resolves.toMatchObject({ + status: "revoked", + tokenTypeHint: "refresh_token", + }); + expect(fetchSpy).toHaveBeenCalledTimes(1); + const [url, init] = fetchSpy.mock.calls[0]!; + expect(String(url)).toBe("https://as.example.com/revoke"); + expect(new URLSearchParams(String(init?.body)).get("token")).toBe("r"); + } finally { + fetchSpy.mockRestore(); + } + const blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + 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 () => { + seed(); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 200 })); + try { + // Normalises to the stored `https://example.com/mcp`. + await expect( + clearStoredAuthForRelogin("https://Example.com/mcp"), + ).resolves.toMatchObject({ status: "revoked" }); + expect(fetchSpy).toHaveBeenCalledTimes(1); + } finally { + fetchSpy.mockRestore(); + } + }); + + it("deletes the local entry even when the request fails", async () => { + const file = seed(); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockRejectedValue(new Error("unreachable")); + try { + await expect( + clearStoredAuthForRelogin("https://example.com/mcp"), + ).resolves.toMatchObject({ status: "failed" }); + } finally { + fetchSpy.mockRestore(); + } + const blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + expect(blob.servers["https://example.com/mcp"]).toBeUndefined(); + }); + + it("skips the request when revocation is turned off", async () => { + seed(); + const fetchSpy = vi.spyOn(globalThis, "fetch"); + try { + await expect( + clearStoredAuthForRelogin("https://example.com/mcp", { + revoke: false, + }), + ).resolves.toBeUndefined(); + expect(fetchSpy).not.toHaveBeenCalled(); + } finally { + fetchSpy.mockRestore(); + } + }); + + it("reports no_tokens when the store holds nothing for the server", async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-empty-")); + const file = path.join(dir, "oauth.json"); + fs.writeFileSync( + file, + JSON.stringify({ servers: {}, idpSessions: {} }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); + + await expect( + clearStoredAuthForRelogin("https://example.com/mcp"), + ).resolves.toEqual({ status: "skipped", reason: "no_tokens" }); + }); + }); + it("clears both raw and URL-normalised keys (bare origin / mixed-case host)", async () => { dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-norm-")); const file = path.join(dir, "oauth.json"); diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index bcbdc880..9297eadc 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -2,6 +2,11 @@ import { NodeOAuthStorage, resetNodeOAuthStorageCache, } from "@inspector/core/auth/node/storage-node.js"; +import { + revokeStoredOAuthTokens, + type TokenRevocationOutcome, +} from "@inspector/core/auth/revocation.js"; +import { createProxyFetch } from "@inspector/core/mcp/node/proxyFetch.js"; /** Same canonicalisation as CLI `normalizeServerUrl` (avoid cycles). */ function normalizeServerUrl(serverUrl: string): string { @@ -24,11 +29,24 @@ function normalizeServerUrl(serverUrl: string): string { */ export async function clearStoredAuthForRelogin( serverUrl: string | undefined, -): Promise { - if (!serverUrl?.trim()) return; + options?: { revoke?: boolean }, +): Promise { + if (!serverUrl?.trim()) return undefined; const raw = serverUrl.trim(); const normalized = normalizeServerUrl(raw); const storage = new NodeOAuthStorage(); + // RFC 7009 (#2144): revoke before the clear, since the token, the client + // credentials and the discovered `revocation_endpoint` all live in the store + // this is about to empty. Best-effort — the outcome is returned for the + // caller to report, never thrown, so `--relogin` succeeds regardless. + // + // 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. + const revocation = + options?.revoke === false + ? undefined + : await revokeFirstStoredKey(storage, [raw, normalized]); await storage.clear(raw); if (normalized !== raw) { await storage.clear(normalized); @@ -36,4 +54,31 @@ export async function clearStoredAuthForRelogin( // Drop the in-process singleton so the next connect cannot reuse a cleared // entry from the NodeOAuthStorage cache. resetNodeOAuthStorageCache(); + return revocation; +} + +/** + * 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". + */ +async function revokeFirstStoredKey( + storage: NodeOAuthStorage, + keys: string[], +): Promise { + const fetchFn = createProxyFetch() ?? fetch; + let last: TokenRevocationOutcome | undefined; + for (const key of new Set(keys)) { + const outcome = await revokeStoredOAuthTokens({ + serverUrl: key, + storage, + fetchFn, + }); + if (!(outcome.status === "skipped" && outcome.reason === "no_tokens")) { + return outcome; + } + last = outcome; + } + return last; } diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index af4bed16..f9c140bb 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -107,6 +107,7 @@ async function callMethod( callbackUrlConfig: RunnerOAuthCallbackConfig, storedAuthOnly: boolean, relogin: boolean, + revoke: boolean, ): Promise { // Clear after parse-time validation so a bad flag combo never deletes store // entries. Deletes the shared URL-keyed OAuth entry (not "ignore for this run"). @@ -116,7 +117,18 @@ async function callMethod( "--relogin requires an HTTP/SSE server URL (no OAuth store entry for stdio)", ); } - await clearStoredAuthForRelogin(serverConfig.url); + // RFC 7009 (#2144). The flag and the per-server setting are both opt-outs, + // so either one turns the revocation off; neither can turn it on for the + // other. Reported rather than thrown — `--relogin` is a local delete and + // must not start failing because an authorization server is unreachable. + const revocation = await clearStoredAuthForRelogin(serverConfig.url, { + revoke: revoke && serverSettings?.oauthRevokeOnClear !== false, + }); + if (revocation?.status === "failed") { + process.stderr.write( + `Warning: could not revoke the OAuth grant at the authorization server (${revocation.detail}); it may still be valid there.\n`, + ); + } } // Version comes from the single source of truth — the root package.json — @@ -570,6 +582,7 @@ type ParseResult = callbackUrl?: string; storedAuthOnly?: boolean; relogin?: boolean; + revoke?: boolean; } // Short-circuit modes (`--list-stored-auth`, `--print-handoff`) do their own // output and need no server connection; runCli returns immediately. @@ -765,6 +778,10 @@ async function parseArgs(argv?: string[]): Promise { "--relogin", "Delete stored OAuth for this server URL from the shared store before connect (HTTP/SSE URL keys only); interactive login runs only if the server requires auth. Rejected for stdio (no URL-keyed store entry)", ) + .option( + "--no-revoke", + "With --relogin, skip the RFC 7009 revocation request that would otherwise end the grant at the authorization server before the local state is deleted. Also skipped when the server entry sets oauth.revokeOnClear to false.", + ) .option( "--wait-for-auth ", "Poll the OAuth state file until a token for --server-url appears (or the timeout elapses), then proceed as if --use-stored-auth were set. Use after handing off to a human to complete OAuth in a browser.", @@ -820,6 +837,7 @@ async function parseArgs(argv?: string[]): Promise { useStoredAuth?: boolean; storedAuthOnly?: boolean; relogin?: boolean; + revoke?: boolean; waitForAuth?: number; listStoredAuth?: boolean; printHandoff?: boolean; @@ -1112,6 +1130,9 @@ async function parseArgs(argv?: string[]): Promise { callbackUrl: options.callbackUrl, storedAuthOnly: options.storedAuthOnly === true, relogin: options.relogin === true, + // Commander's `--no-revoke` defaults this to true; only an explicit + // `--no-revoke` makes it false. + revoke: options.revoke !== false, }; } @@ -1130,6 +1151,7 @@ export async function runCli(argv?: string[]): Promise { callbackUrl, storedAuthOnly, relogin, + revoke, } = parsed; const clientConfig = await loadRunnerClientConfig({ clientConfigPath }); // A bad --callback-url / MCP_OAUTH_CALLBACK_URL is a *usage* error, but its @@ -1159,5 +1181,6 @@ export async function runCli(argv?: string[]): Promise { callbackUrlConfig, storedAuthOnly === true, relogin === true, + revoke !== false, ); } diff --git a/clients/tui/README.md b/clients/tui/README.md index 93930003..bab524ef 100644 --- a/clients/tui/README.md +++ b/clients/tui/README.md @@ -65,7 +65,7 @@ OAuth redirect URIs must match **exactly** what you register on the authorizatio 1. Select an HTTP/SSE server and press **C** to connect. 2. If authorization is required, the TUI starts OAuth automatically (browser opens for sign-in). 3. After the callback completes, connect finishes without a second **C**. -4. Use the **Auth** tab to inspect OAuth state (same fields as web Connection Info) or **Clear OAuth state** (disconnects when connected). +4. Use the **Auth** tab to inspect OAuth state (same fields as web Connection Info) or **Clear OAuth state** (disconnects when connected). Clearing also revokes the grant at the authorization server when it advertises an RFC 7009 `revocation_endpoint` — best-effort, with any failure reported in the status line and the local clear finishing regardless. Set `oauth.revokeOnClear: false` on the server entry to skip it ([#2144](https://github.com/modelcontextprotocol/inspector/issues/2144)). See also [EMA / enterprise-managed auth](../../specification/v2_auth_ema.md) and [OAuth smoke testing](../../specification/v2_auth_smoke_testing.md) for staging servers and verification steps. diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index e58d8c77..d637d35c 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -59,9 +59,10 @@ const h = vi.hoisted(() => { authenticate: vi.fn( async () => new URL("https://auth.example/start"), ), - clearOAuthTokens: vi.fn( - async () => {}, - ), + clearOAuthTokens: vi.fn(async () => ({ + status: "skipped" as const, + reason: "no_endpoint" as const, + })), completeOAuthFlow: vi.fn( async () => {}, ), diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index d8a8aee0..fccff741 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -941,15 +941,30 @@ function App({ const handleClearOAuth = useCallback(async () => { if (!selectedInspectorClient) return; - await selectedInspectorClient.clearOAuthTokens(); + // RFC 7009 (#2144). Best-effort: the outcome is reported in the status + // line, never thrown, so clearing always completes. The per-server + // `oauthRevokeOnClear` opt-out is honored here the same way the web client + // honors it. + const revocation = await selectedInspectorClient.clearOAuthTokens({ + revoke: selectedServerEntry?.settings?.oauthRevokeOnClear !== false, + }); setOauthStatus("idle"); - setOauthMessage(null); + setOauthMessage( + revocation.status === "failed" + ? `Cleared locally, but revoking the grant at the authorization server failed: ${revocation.detail}` + : null, + ); setConnectError(null); if (inspectorStatus === "connected" || inspectorStatus === "connecting") { await disconnectInspector(); } setOauthRevision((n) => n + 1); - }, [selectedInspectorClient, inspectorStatus, disconnectInspector]); + }, [ + selectedInspectorClient, + selectedServerEntry, + inspectorStatus, + disconnectInspector, + ]); // Build current server state from InspectorClient data (tools from ManagedToolsState) const currentServerState = useMemo(() => { diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index 322cb89b..081b5cc2 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx @@ -707,6 +707,7 @@ describe("ServerSettingsForm", () => { tokenUrl: "", enterpriseManaged: false, requestRefreshToken: true, + revokeOnClear: true, }); }); @@ -751,6 +752,47 @@ describe("ServerSettingsForm", () => { ); }); + // #2144 — the RFC 7009 opt-out. On by default: leaving it off silently is + // what leaves the authorization server holding a live grant. + it("renders Revoke tokens on clear checked by default", () => { + renderWithMantine( + , + ); + expect(screen.getByLabelText("Revoke tokens on clear")).toBeChecked(); + }); + + it("renders Revoke tokens on clear unchecked when the server opted out", () => { + renderWithMantine( + , + ); + expect(screen.getByLabelText("Revoke tokens on clear")).not.toBeChecked(); + }); + + it("toggles Revoke tokens on clear through onOAuthChange", async () => { + const user = userEvent.setup(); + const onOAuthChange = vi.fn(); + renderWithMantine( + , + ); + await user.click(screen.getByLabelText("Revoke tokens on clear")); + expect(onOAuthChange).toHaveBeenLastCalledWith( + expect.objectContaining({ revokeOnClear: false }), + ); + }); + it("warns when opting out while offline_access is still in Scopes", () => { renderWithMantine( { tokenUrl: "", enterpriseManaged: true, requestRefreshToken: true, + revokeOnClear: true, }); }); @@ -1363,6 +1406,7 @@ describe("ServerSettingsForm", () => { tokenUrl: "", enterpriseManaged: false, requestRefreshToken: true, + revokeOnClear: true, }); }); diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index fec3f25a..5343a46f 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -521,6 +521,8 @@ export function ServerSettingsForm({ onInsufficientScope: settings.oauthOnInsufficientScope, // Unset means the default, on — only an explicit opt-out is stored. requestRefreshToken: settings.oauthRequestRefreshToken ?? true, + // Unset means the default, on — only an explicit opt-out is stored. + revokeOnClear: settings.oauthRevokeOnClear ?? true, }; } @@ -570,6 +572,15 @@ export function ServerSettingsForm({ }); } + function handleRevokeOnClearChange( + event: ChangeEvent, + ): void { + onOAuthChange({ + ...currentOAuth(), + revokeOnClear: event.currentTarget.checked, + }); + } + function handleAddAuthorizationParam(): void { changeAuthorizationParams([...authorizationParams, { key: "", value: "" }]); } @@ -917,6 +928,12 @@ export function ServerSettingsForm({ checked={settings.oauthRequestRefreshToken ?? true} onChange={handleRequestRefreshTokenChange} /> + {refreshTokenOptedOut && scopesIncludeOfflineAccess ? ( The Scopes field above lists offline_access, and diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx index d9933631..0d704e42 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.test.tsx @@ -156,6 +156,42 @@ describe("ServerSettingsModal", () => { ); }); + // #2144 — same omit-the-default shape as the refresh-token pair above. + it("maps the revoke-on-clear opt-out into settings, and back to unset", async () => { + const user = userEvent.setup(); + const onSettingsChange = vi.fn(); + const { rerender } = renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: /OAuth Settings/i })); + await user.click(screen.getByLabelText("Revoke tokens on clear")); + expect(onSettingsChange).toHaveBeenLastCalledWith( + expect.objectContaining({ oauthRevokeOnClear: false }), + ); + + rerender( + , + ); + await user.click(screen.getByLabelText("Revoke tokens on clear")); + expect(onSettingsChange).toHaveBeenLastCalledWith( + expect.objectContaining({ oauthRevokeOnClear: undefined }), + ); + }); + it("maps the selected protocol era into settings (#1626)", async () => { const user = userEvent.setup(); const onSettingsChange = vi.fn(); diff --git a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx index ef6394ab..b8a422c0 100644 --- a/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx +++ b/clients/web/src/components/groups/ServerSettingsModal/ServerSettingsModal.tsx @@ -191,6 +191,8 @@ export function ServerSettingsModal({ // server that never touched the switch writes no field at all. oauthRequestRefreshToken: oauth.requestRefreshToken === false ? false : undefined, + // #2144: same shape — `undefined` means on, so only the opt-out persists. + oauthRevokeOnClear: oauth.revokeOnClear === false ? false : undefined, }); } diff --git a/clients/web/src/hooks/useConnectionLifecycle.ts b/clients/web/src/hooks/useConnectionLifecycle.ts index 934b73e6..bd635b1f 100644 --- a/clients/web/src/hooks/useConnectionLifecycle.ts +++ b/clients/web/src/hooks/useConnectionLifecycle.ts @@ -912,6 +912,11 @@ export function useConnectionLifecycle({ serverId === activeServerId ? inspectorClient : null, isActiveConnection: serverId === activeServerId, oauthStorage: webOAuthStorage, + // No RFC 7009 revocation here (#2144): this clears a *half-finished* + // flow so it can be retried. The authorization never completed, so + // there is no grant to revoke — and any token still on disk belongs + // to the very session this recovery is trying to rebuild. + revoke: false, }); } catch (err) { notifications.show({ diff --git a/clients/web/src/hooks/useOAuthRecovery.test.tsx b/clients/web/src/hooks/useOAuthRecovery.test.tsx index 08b1c9bd..c3bbaa0c 100644 --- a/clients/web/src/hooks/useOAuthRecovery.test.tsx +++ b/clients/web/src/hooks/useOAuthRecovery.test.tsx @@ -21,6 +21,7 @@ import { EMPTY_SETTINGS } from "../utils/serverSettingsDefaults"; import { useSessionRef } from "./useSessionRef"; import { useTabUiState } from "./useTabUiState"; import { + revocationSuffix, useOAuthRecovery, type FetchRequestSource, type OAuthRecovery, @@ -58,6 +59,14 @@ vi.mock("@inspector/core/mcp/remote/index.js", () => ({ RemoteInspectorClientStorage: class { saveSession = saveSessionMock; }, + // #2144: `clearServerOAuthAndDisconnect` builds the backend-proxied fetch the + // revocation POST would travel on. Nothing here exercises a real request, so + // this only has to exist. + createRemoteFetch: () => remoteFetchMock, +})); + +const { remoteFetchMock } = vi.hoisted(() => ({ + remoteFetchMock: vi.fn(async () => new Response(null)), })); vi.mock("../lib/authToken", () => ({ getAuthToken: () => "test-token" })); @@ -262,7 +271,7 @@ beforeEach(() => { window.sessionStorage.clear(); window.history.replaceState({}, "", "/"); oauthStorageMock.load.mockResolvedValue(undefined); - clearServerOAuthStateMock.mockResolvedValue(true); + clearServerOAuthStateMock.mockResolvedValue({ cleared: true }); saveSessionMock.mockResolvedValue(undefined); }); @@ -1271,9 +1280,37 @@ describe("useOAuthRecovery", () => { }); }); + // #2144 — only the two outcomes a user can act on are surfaced. A skip + // describes the status quo, and reporting it would turn a confirmation into a + // notice about something that did not need to happen. + describe("revocationSuffix", () => { + it("announces a successful revocation", () => { + expect( + revocationSuffix({ + status: "revoked", + tokenTypeHint: "refresh_token", + endpoint: "https://as.example/revoke", + }), + ).toContain("revoked at the authorization server"); + }); + + it("warns that the grant may still be live after a failure", () => { + const text = revocationSuffix({ status: "failed", detail: "boom" }); + expect(text).toContain("boom"); + expect(text).toContain("may still be valid"); + }); + + it("says nothing for a skip or an absent outcome", () => { + expect( + revocationSuffix({ status: "skipped", reason: "no_endpoint" }), + ).toBe(""); + expect(revocationSuffix(undefined)).toBe(""); + }); + }); + describe("clearing stored OAuth state", () => { it("says nothing when there was nothing to clear", async () => { - clearServerOAuthStateMock.mockResolvedValue(false); + clearServerOAuthStateMock.mockResolvedValue({ cleared: false }); const h = harness({ servers: [entry("a")], activeServerId: "a" }); await act(async () => { await h.api().clearServerOAuthAndDisconnect(entry("a")); diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 7468799e..f00d120e 100644 --- a/clients/web/src/hooks/useOAuthRecovery.ts +++ b/clients/web/src/hooks/useOAuthRecovery.ts @@ -5,6 +5,7 @@ import type { InspectorClient } from "@inspector/core/mcp/index.js"; import type { TypedEvent } from "@inspector/core/mcp/inspectorClientEventTarget.js"; import type { ConnectionStatus, + InspectorServerSettings, MCPServerConfig, ServerEntry, } from "@inspector/core/mcp/types.js"; @@ -17,6 +18,7 @@ import { } from "@inspector/core/auth/index.js"; import { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js"; import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js"; +import type { TokenRevocationOutcome } from "@inspector/core/auth/revocation.js"; import type { AuthChallenge, AuthChallengeReason, @@ -31,6 +33,7 @@ import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientCo import type { OAuthDetails } from "../components/groups/ConnectionInfoContent/ConnectionInfoContent"; import { oauthDetailsFromConnectionState } from "../components/groups/ConnectionInfoContent/oauthDetailsFromConnectionState"; import { getWebRemoteOAuthStorage } from "../lib/remoteOAuthStorage"; +import { getWebProxiedFetch } from "../lib/webProxiedFetch"; import { clearServerOAuthState } from "../lib/clearServerOAuthState"; import { getAuthToken } from "../lib/authToken"; import { @@ -121,6 +124,36 @@ export interface ClearableServer { id: string; name: string; config: MCPServerConfig; + /** + * Read for `oauthRevokeOnClear` (#2144). Optional so a caller that only has + * the identity fields still type-checks; an absent value means the default, + * on. + */ + settings?: InspectorServerSettings; +} + +/** + * The sentence appended to the "OAuth state cleared" toast describing what the + * RFC 7009 leg did (#2144). + * + * Only the two outcomes a user can act on are surfaced. A success is worth + * saying because the whole point of the feature is invisible otherwise — the + * local clear looks identical either way. A failure is worth saying because the + * grant is still live at the authorization server and the user may want to + * revoke it by hand. The remaining skips ("this server advertises no revocation + * endpoint", "there were no tokens") describe the status quo and would turn a + * confirmation into a notice about something that did not need to happen. + */ +export function revocationSuffix( + outcome: TokenRevocationOutcome | undefined, +): string { + if (outcome?.status === "revoked") { + return " The grant was also revoked at the authorization server."; + } + if (outcome?.status === "failed") { + return ` Revoking the grant at the authorization server failed (${outcome.detail}), so it may still be valid there.`; + } + return ""; } export interface UseOAuthRecoveryOptions { @@ -1221,11 +1254,13 @@ export function useOAuthRecovery({ const clearServerOAuthAndDisconnect = useCallback( async (server: ClearableServer) => { const isActive = server.id === activeServerId; - const cleared = await clearServerOAuthState({ + const { cleared, revocation } = await clearServerOAuthState({ config: server.config, inspectorClient: isActive ? inspectorClient : null, isActiveConnection: isActive, oauthStorage: webOAuthStorage, + revoke: server.settings?.oauthRevokeOnClear !== false, + fetchFn: getWebProxiedFetch(getAuthToken()), }); if (!cleared) return; @@ -1243,8 +1278,8 @@ export function useOAuthRecovery({ notifications.show({ title: "OAuth state cleared", message: isActive - ? "Stored tokens and client registration were removed. Reconnect to run a fresh authorization flow." - : `Stored OAuth state was removed for "${server.name}". Connect to authorize again.`, + ? `Stored tokens and client registration were removed. Reconnect to run a fresh authorization flow.${revocationSuffix(revocation)}` + : `Stored OAuth state was removed for "${server.name}". Connect to authorize again.${revocationSuffix(revocation)}`, color: "blue", }); }, diff --git a/clients/web/src/lib/clearServerOAuthState.test.ts b/clients/web/src/lib/clearServerOAuthState.test.ts index 137bc9c3..002fb171 100644 --- a/clients/web/src/lib/clearServerOAuthState.test.ts +++ b/clients/web/src/lib/clearServerOAuthState.test.ts @@ -1,40 +1,47 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; import { BrowserOAuthStorage } from "@inspector/core/auth/browser/storage.js"; +import type { TokenRevocationOutcome } from "@inspector/core/auth/revocation.js"; import type { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import { clearServerOAuthState } from "./clearServerOAuthState"; +const SERVER_URL = "https://mcp.example.com/mcp"; + +function skipped(): TokenRevocationOutcome { + return { status: "skipped", reason: "no_endpoint" }; +} + describe("clearServerOAuthState", () => { let storage: BrowserOAuthStorage; beforeEach(async () => { storage = new BrowserOAuthStorage(); - await storage.clear("https://mcp.example.com/mcp"); + await storage.clear(SERVER_URL); }); it("clears storage by server URL when not the active connection", async () => { - await storage.saveTokens("https://mcp.example.com/mcp", { + await storage.saveTokens(SERVER_URL, { access_token: "tok", token_type: "Bearer", }); - const cleared = await clearServerOAuthState({ - config: { type: "streamable-http", url: "https://mcp.example.com/mcp" }, + const { cleared } = await clearServerOAuthState({ + config: { type: "streamable-http", url: SERVER_URL }, isActiveConnection: false, oauthStorage: storage, }); expect(cleared).toBe(true); - expect( - await storage.getTokens("https://mcp.example.com/mcp"), - ).toBeUndefined(); + expect(await storage.getTokens(SERVER_URL)).toBeUndefined(); }); it("uses the live client when clearing the active connection", async () => { - const clearOAuthTokens = vi.fn(); + const clearOAuthTokens = vi.fn( + async () => skipped(), + ); const inspectorClient = { clearOAuthTokens }; - const cleared = await clearServerOAuthState({ - config: { type: "streamable-http", url: "https://mcp.example.com/mcp" }, + const { cleared } = await clearServerOAuthState({ + config: { type: "streamable-http", url: SERVER_URL }, inspectorClient, isActiveConnection: true, oauthStorage: storage, @@ -42,15 +49,108 @@ describe("clearServerOAuthState", () => { expect(cleared).toBe(true); expect(clearOAuthTokens).toHaveBeenCalledTimes(1); + expect(clearOAuthTokens).toHaveBeenCalledWith({ revoke: true }); }); - it("returns false for stdio servers", async () => { + it("returns cleared: false for stdio servers", async () => { await expect( clearServerOAuthState({ config: { type: "stdio", command: "node", args: [] }, isActiveConnection: false, oauthStorage: storage, }), - ).resolves.toBe(false); + ).resolves.toEqual({ cleared: false }); + }); + + // #2144 — the opt-out has to reach the live client, since that is where the + // RFC 7009 request is actually made. + it("forwards revoke: false to the live client", async () => { + const clearOAuthTokens = vi.fn( + async () => skipped(), + ); + + await clearServerOAuthState({ + config: { type: "streamable-http", url: SERVER_URL }, + inspectorClient: { clearOAuthTokens }, + isActiveConnection: true, + oauthStorage: storage, + revoke: false, + }); + + expect(clearOAuthTokens).toHaveBeenCalledWith({ revoke: false }); + }); + + // #2144 — the non-active path revokes from the store directly. The whole + // point is that it happens *before* the clear: after it there is no token, + // no client id and no cached metadata left to build a request from. + it("revokes before clearing when this server is not the active connection", async () => { + await storage.saveTokens(SERVER_URL, { + access_token: "tok", + token_type: "Bearer", + refresh_token: "refresh-tok", + }); + await storage.saveServerMetadata(SERVER_URL, { + 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"], + }); + + let tokensAtRequestTime: unknown; + const fetchFn = vi.fn(async () => { + tokensAtRequestTime = await storage.getTokens(SERVER_URL); + return new Response(null, { status: 200 }); + }); + + const { revocation } = await clearServerOAuthState({ + config: { type: "streamable-http", url: SERVER_URL }, + isActiveConnection: false, + oauthStorage: storage, + fetchFn, + }); + + expect(revocation).toEqual({ + status: "revoked", + tokenTypeHint: "refresh_token", + endpoint: "https://as.example.com/revoke", + }); + expect(tokensAtRequestTime).toBeDefined(); + expect(await storage.getTokens(SERVER_URL)).toBeUndefined(); + }); + + // Without a backend-proxied fetch the request would go out on the page + // origin, where a real authorization server's missing CORS headers reject it. + // Skipping is honest; attempting it would fail loudly on real deployments and + // succeed on permissive ones. + it("skips revocation on the non-active path when given no fetch", async () => { + await storage.saveTokens(SERVER_URL, { + access_token: "tok", + token_type: "Bearer", + }); + + const { revocation } = await clearServerOAuthState({ + config: { type: "streamable-http", url: SERVER_URL }, + isActiveConnection: false, + oauthStorage: storage, + }); + + expect(revocation).toEqual({ status: "skipped", reason: "disabled" }); + expect(await storage.getTokens(SERVER_URL)).toBeUndefined(); + }); + + it("skips the request on the non-active path when revoke is off", async () => { + const fetchFn = vi.fn(); + + const { revocation } = await clearServerOAuthState({ + config: { type: "streamable-http", url: SERVER_URL }, + isActiveConnection: false, + oauthStorage: storage, + revoke: false, + fetchFn, + }); + + expect(fetchFn).not.toHaveBeenCalled(); + expect(revocation).toEqual({ status: "skipped", reason: "disabled" }); }); }); diff --git a/clients/web/src/lib/clearServerOAuthState.ts b/clients/web/src/lib/clearServerOAuthState.ts index f7eb5233..1593565b 100644 --- a/clients/web/src/lib/clearServerOAuthState.ts +++ b/clients/web/src/lib/clearServerOAuthState.ts @@ -1,3 +1,7 @@ +import { + revokeStoredOAuthTokens, + type TokenRevocationOutcome, +} from "@inspector/core/auth/revocation.js"; import type { OAuthStorage } from "@inspector/core/auth/storage.js"; import { getOAuthServerUrl } from "@inspector/core/mcp/config.js"; import type { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; @@ -10,25 +14,71 @@ export interface ClearServerOAuthStateParams { isActiveConnection: boolean; /** Shared web OAuth store; required so clear hits the same blob as connect. */ oauthStorage: OAuthStorage; + /** + * Whether to revoke the grant at the authorization server first (RFC 7009, + * #2144). Defaults to on. Two callers turn it off: a server whose settings + * opted out, and `lost_authorization_state` recovery — that path clears a + * half-finished flow in order to retry it, so there is no completed grant to + * revoke and the request would be noise at best. + */ + revoke?: boolean; + /** + * Fetch used for the revocation POST when this server is **not** the active + * connection (there is no live client to borrow one from). In the browser + * this must be the backend-proxied fetch the OAuth flow itself uses — + * `globalThis.fetch` would put the request on the page's origin, where an + * authorization server that serves no CORS headers rejects it. Omitting it + * skips revocation on that path rather than sending a request that cannot + * work. + */ + fetchFn?: typeof fetch; +} + +export interface ClearServerOAuthStateResult { + /** False when the config has no OAuth server URL — nothing was cleared. */ + cleared: boolean; + /** What the RFC 7009 leg did. Absent when nothing was cleared. */ + revocation?: TokenRevocationOutcome; } /** * Clear persisted OAuth state (tokens, DCR/CIMD client id, PKCE, etc.) for an - * HTTP MCP server. When clearing the active connection, uses the live client so - * in-memory flow state is reset too. + * HTTP MCP server, revoking the grant at the authorization server first. When + * clearing the active connection, uses the live client so in-memory flow state + * is reset too. + * + * Revocation is best-effort throughout — an authorization server advertising no + * `revocation_endpoint` is untouched, and a failure is reported in the result + * rather than thrown — so the local clear always finishes. */ export async function clearServerOAuthState( params: ClearServerOAuthStateParams, -): Promise { +): Promise { const serverUrl = getOAuthServerUrl(params.config); if (!serverUrl) { - return false; + return { cleared: false }; } + const revoke = params.revoke !== false; + if (params.isActiveConnection && params.inspectorClient) { - await params.inspectorClient.clearOAuthTokens(); - } else { - await params.oauthStorage.clear(serverUrl); + const revocation = await params.inspectorClient.clearOAuthTokens({ + revoke, + }); + return { cleared: true, revocation }; } - return true; + + // No proxied fetch on hand means no request we could usefully make, so the + // leg is reported as skipped rather than attempted against the page origin. + const fetchFn = params.fetchFn; + const revocation: TokenRevocationOutcome = + revoke && fetchFn + ? await revokeStoredOAuthTokens({ + serverUrl, + storage: params.oauthStorage, + fetchFn, + }) + : { status: "skipped", reason: "disabled" }; + await params.oauthStorage.clear(serverUrl); + return { cleared: true, revocation }; } diff --git a/clients/web/src/lib/webProxiedFetch.test.ts b/clients/web/src/lib/webProxiedFetch.test.ts new file mode 100644 index 00000000..84f7420b --- /dev/null +++ b/clients/web/src/lib/webProxiedFetch.test.ts @@ -0,0 +1,58 @@ +import { describe, it, expect, beforeEach, vi } from "vitest"; + +const { createRemoteFetchMock } = vi.hoisted(() => ({ + createRemoteFetchMock: vi.fn(), +})); + +vi.mock("@inspector/core/mcp/remote/index.js", () => ({ + createRemoteFetch: createRemoteFetchMock, +})); + +import { + getWebProxiedFetch, + resetWebProxiedFetchCacheForTests, +} from "./webProxiedFetch"; + +describe("getWebProxiedFetch", () => { + beforeEach(() => { + resetWebProxiedFetchCacheForTests(); + createRemoteFetchMock.mockReset(); + createRemoteFetchMock.mockImplementation(() => vi.fn()); + }); + + it("builds a remote fetch against the page origin and the API token", () => { + getWebProxiedFetch("tok"); + expect(createRemoteFetchMock).toHaveBeenCalledWith( + expect.objectContaining({ + baseUrl: `${window.location.protocol}//${window.location.host}`, + authToken: "tok", + }), + ); + }); + + it("reuses the instance for the same origin and token", () => { + const first = getWebProxiedFetch("tok"); + expect(getWebProxiedFetch("tok")).toBe(first); + expect(createRemoteFetchMock).toHaveBeenCalledTimes(1); + }); + + it("rebuilds when the token changes", () => { + const first = getWebProxiedFetch("tok"); + expect(getWebProxiedFetch("other")).not.toBe(first); + expect(createRemoteFetchMock).toHaveBeenCalledTimes(2); + }); + + it("forwards a fetch that reaches the injected base fetch", async () => { + const inner = vi.fn(async () => new Response(null)); + createRemoteFetchMock.mockImplementation( + ({ fetchFn }: { fetchFn: typeof fetch }) => fetchFn, + ); + const proxied = getWebProxiedFetch(); + const globalFetch = vi + .spyOn(globalThis, "fetch") + .mockImplementation(inner as typeof fetch); + await proxied("https://example.com"); + expect(inner).toHaveBeenCalled(); + globalFetch.mockRestore(); + }); +}); diff --git a/clients/web/src/lib/webProxiedFetch.ts b/clients/web/src/lib/webProxiedFetch.ts new file mode 100644 index 00000000..6c733ea5 --- /dev/null +++ b/clients/web/src/lib/webProxiedFetch.ts @@ -0,0 +1,44 @@ +import { createRemoteFetch } from "@inspector/core/mcp/remote/index.js"; + +let cached: { cacheKey: string; fetchFn: typeof fetch } | undefined; + +const defaultFetch: typeof fetch = (...args) => globalThis.fetch(...args); + +/** + * The backend-proxied fetch the browser must use for any request aimed at an + * authorization server rather than at the Inspector itself. + * + * `createWebEnvironment` hands the same thing to `InspectorClient`, so an OAuth + * request made through a live client already travels this way. This exists for + * the paths that have no client to borrow one from — clearing (and revoking) + * the stored OAuth state of a server that is not the active connection. + * + * Going direct is not an option there: an authorization server serves no CORS + * headers for a page origin it has never heard of, so `globalThis.fetch` would + * fail on nearly every real deployment while working on a permissive one — the + * worst shape of bug to carry. + * + * Cached as a cache of one, keyed the same way `getWebRemoteOAuthStorage` keys + * its store: the web app has a stable origin and a page-lifetime API token, so + * the key does not change within a session. + */ +export function getWebProxiedFetch(authToken?: string): typeof fetch { + if (typeof window === "undefined") { + throw new Error("getWebProxiedFetch requires a browser environment"); + } + const baseUrl = `${window.location.protocol}//${window.location.host}`; + const cacheKey = `${baseUrl}\0${authToken ?? ""}`; + if (cached?.cacheKey === cacheKey) { + return cached.fetchFn; + } + cached = { + cacheKey, + fetchFn: createRemoteFetch({ baseUrl, authToken, fetchFn: defaultFetch }), + }; + return cached.fetchFn; +} + +/** @internal Vitest isolation */ +export function resetWebProxiedFetchCacheForTests(): void { + cached = undefined; +} diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts new file mode 100644 index 00000000..80e05a4f --- /dev/null +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -0,0 +1,407 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import type { OAuthMetadata } from "@modelcontextprotocol/client"; +import { BrowserOAuthStorage } from "@inspector/core/auth/browser/storage.js"; +import { + DEFAULT_REVOCATION_TIMEOUT_MS, + buildRevocationRequest, + revocationAuthMethods, + revokeStoredOAuthTokens, + revokeToken, + selectRevocableToken, +} from "@inspector/core/auth/revocation.js"; + +const SERVER_URL = "https://mcp.example.com/mcp"; +const REVOKE_URL = "https://as.example.com/revoke"; + +function metadata(over: Partial = {}): OAuthMetadata { + return { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + response_types_supported: ["code"], + revocation_endpoint: REVOKE_URL, + ...over, + } as OAuthMetadata; +} + +function body(init: RequestInit): URLSearchParams { + return new URLSearchParams(String(init.body)); +} + +function headerOf(init: RequestInit, name: string): string | undefined { + return (init.headers as Record | undefined)?.[name]; +} + +describe("selectRevocableToken", () => { + // RFC 7009 §2.1 — revoking the refresh token asks the AS to invalidate the + // access tokens under the same grant, so one request covers both. Naming the + // access token instead would leave the long-lived half alive. + it("prefers the refresh token when one exists", () => { + expect( + selectRevocableToken({ + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }), + ).toEqual({ token: "r", tokenTypeHint: "refresh_token" }); + }); + + it("falls back to the access token", () => { + expect( + selectRevocableToken({ access_token: "a", token_type: "Bearer" }), + ).toEqual({ token: "a", tokenTypeHint: "access_token" }); + }); + + it("has nothing to revoke without tokens", () => { + expect(selectRevocableToken(undefined)).toBeNull(); + expect( + selectRevocableToken({ access_token: "", token_type: "Bearer" }), + ).toBeNull(); + }); +}); + +describe("revocationAuthMethods", () => { + it("prefers the revocation endpoint's own list", () => { + expect( + revocationAuthMethods( + metadata({ + revocation_endpoint_auth_methods_supported: ["client_secret_post"], + token_endpoint_auth_methods_supported: ["client_secret_basic"], + }), + ), + ).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", () => { + expect( + revocationAuthMethods( + metadata({ token_endpoint_auth_methods_supported: ["none"] }), + ), + ).toEqual(["none"]); + }); + + it("yields nothing when neither is advertised", () => { + expect(revocationAuthMethods(metadata())).toEqual([]); + }); +}); + +describe("buildRevocationRequest", () => { + it("posts a form-encoded token and hint", () => { + const { url, init } = buildRevocationRequest({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + supportedAuthMethods: [], + }); + expect(url).toBe(REVOKE_URL); + expect(init.method).toBe("POST"); + expect(headerOf(init, "Content-Type")).toBe( + "application/x-www-form-urlencoded", + ); + expect(body(init).get("token")).toBe("r"); + expect(body(init).get("token_type_hint")).toBe("refresh_token"); + }); + + it("sends a confidential client's secret in the Authorization header for client_secret_basic", () => { + const { init } = buildRevocationRequest({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + clientInformation: { client_id: "id one", client_secret: "s/ecret" }, + supportedAuthMethods: ["client_secret_basic"], + }); + // Byte-identical to the SDK's `applyBasicAuth`: the raw `id:secret`, not the + // form-urlencoded pair RFC 6749 §2.3.1 asks for. Presenting the credential + // differently here than at the token endpoint is what would let an + // authorization server accept one request and reject the other. + expect(headerOf(init, "Authorization")).toBe( + `Basic ${btoa("id one:s/ecret")}`, + ); + expect(body(init).has("client_secret")).toBe(false); + }); + + it("sends the secret in the body for client_secret_post", () => { + const { init } = buildRevocationRequest({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + clientInformation: { client_id: "id", client_secret: "s" }, + supportedAuthMethods: ["client_secret_post"], + }); + expect(headerOf(init, "Authorization")).toBeUndefined(); + expect(body(init).get("client_id")).toBe("id"); + expect(body(init).get("client_secret")).toBe("s"); + }); + + it("identifies a public client by client_id alone", () => { + const { init } = buildRevocationRequest({ + endpoint: REVOKE_URL, + token: "a", + tokenTypeHint: "access_token", + clientInformation: { client_id: "public" }, + supportedAuthMethods: ["none"], + }); + expect(headerOf(init, "Authorization")).toBeUndefined(); + expect(body(init).get("client_id")).toBe("public"); + expect(body(init).has("client_secret")).toBe(false); + }); +}); + +describe("revokeToken", () => { + it("reports a 200 as revoked", async () => { + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + await expect( + revokeToken({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + supportedAuthMethods: [], + fetchFn, + }), + ).resolves.toEqual({ + status: "revoked", + tokenTypeHint: "refresh_token", + endpoint: REVOKE_URL, + }); + }); + + it("reports a non-2xx as failed rather than throwing", async () => { + const fetchFn = vi.fn( + async () => + new Response(null, { status: 401, statusText: "Unauthorized" }), + ); + const outcome = await revokeToken({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + supportedAuthMethods: [], + fetchFn, + }); + expect(outcome).toMatchObject({ status: "failed", endpoint: REVOKE_URL }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain("401"); + }); + + it("reports a network failure as failed", async () => { + const fetchFn = vi.fn(async () => { + throw new Error("connect ECONNREFUSED"); + }); + const outcome = await revokeToken({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + supportedAuthMethods: [], + fetchFn, + }); + expect(outcome).toEqual({ + status: "failed", + endpoint: REVOKE_URL, + detail: "connect ECONNREFUSED", + }); + }); + + // 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 () => { + let seen: RequestInit | undefined; + const fetchFn = vi.fn(async (_url, init) => { + seen = init; + return new Response(null, { status: 200 }); + }); + await revokeToken({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + supportedAuthMethods: [], + fetchFn, + timeoutMs: 1, + }); + expect(seen?.signal).toBeInstanceOf(AbortSignal); + expect(DEFAULT_REVOCATION_TIMEOUT_MS).toBeGreaterThan(0); + }); +}); + +describe("revokeStoredOAuthTokens", () => { + let storage: BrowserOAuthStorage; + + beforeEach(async () => { + storage = new BrowserOAuthStorage(); + await storage.clear(SERVER_URL); + }); + + async function seed(over: Partial = {}): Promise { + await storage.saveTokens(SERVER_URL, { + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }); + await storage.saveServerMetadata(SERVER_URL, metadata(over)); + } + + it("revokes the stored refresh token", async () => { + await seed(); + await storage.saveClientInformation( + SERVER_URL, + { client_id: "cid", client_secret: "sec" }, + { registrationKind: "dcr" }, + ); + + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + expect(outcome).toMatchObject({ status: "revoked" }); + const [, init] = fetchFn.mock.calls[0]!; + expect(body(init!).get("token")).toBe("r"); + }); + + // A server configured with `oauth.clientId` stores its credentials in the + // preregistered slot, which is issuer-independent and is *not* what a plain + // `getClientInformation(serverUrl)` returns. Reading only the dynamic slot + // would send no client authentication at all for exactly the confidential + // clients most likely to require it. + it("authenticates with a preconfigured client, not just a dynamically registered one", async () => { + await seed(); + await storage.savePreregisteredClientInformation(SERVER_URL, { + client_id: "static-cid", + client_secret: "static-sec", + }); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + await revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }); + + const [, init] = fetchFn.mock.calls[0]!; + expect(headerOf(init!, "Authorization")).toBe( + `Basic ${btoa("static-cid:static-sec")}`, + ); + }); + + // The whole point of keeping this path opt-out-able: an authorization server + // with no RFC 7009 support must behave exactly as it did before the feature. + it("does nothing when the authorization server advertises no revocation endpoint", async () => { + await seed({ revocation_endpoint: undefined }); + const fetchFn = vi.fn(); + await expect( + revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }), + ).resolves.toEqual({ status: "skipped", reason: "no_endpoint" }); + expect(fetchFn).not.toHaveBeenCalled(); + }); + + it("does nothing when no metadata was ever discovered", async () => { + await storage.saveTokens(SERVER_URL, { + access_token: "a", + token_type: "Bearer", + }); + const fetchFn = vi.fn(); + await expect( + revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }), + ).resolves.toEqual({ status: "skipped", reason: "no_metadata" }); + expect(fetchFn).not.toHaveBeenCalled(); + }); + + it("does nothing when there is no stored grant", async () => { + const fetchFn = vi.fn(); + await expect( + revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }), + ).resolves.toEqual({ status: "skipped", reason: "no_tokens" }); + expect(fetchFn).not.toHaveBeenCalled(); + }); + + it("skips the request when disabled", async () => { + await seed(); + const fetchFn = vi.fn(); + await expect( + revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + enabled: false, + }), + ).resolves.toEqual({ status: "skipped", reason: "disabled" }); + expect(fetchFn).not.toHaveBeenCalled(); + }); + + 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 fetchFn = vi.fn( + async () => new Response(null, { status: 500, statusText: "Boom" }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + logger: logger as unknown as Parameters< + typeof revokeStoredOAuthTokens + >[0]["logger"], + }); + + expect(outcome.status).toBe("failed"); + expect(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(), + }; + await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn: vi.fn(), + logger: logger as unknown as Parameters< + typeof revokeStoredOAuthTokens + >[0]["logger"], + }); + expect(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. + it("reports a store read failure as failed", async () => { + const broken = { + ...storage, + getTokens: () => Promise.reject(new Error("store unreadable")), + } as unknown as BrowserOAuthStorage; + + await expect( + revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage: broken, + fetchFn: vi.fn(), + }), + ).resolves.toEqual({ status: "failed", detail: "store unreadable" }); + }); +}); diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index ec4d1df9..e9a32541 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -197,6 +197,88 @@ describe("OAuthManager", () => { expect(manager.getOAuthFlowStep()).toBeUndefined(); }); + // #2144 — the ordering is the contract, not an implementation detail: the + // revocation request is built from the token, the client id and the cached + // metadata that `clear` is about to delete. + it("revokes at the authorization server before clearing local state", async () => { + const params = createMockParams(); + const storage = params.initialConfig.storage!; + vi.mocked(storage.getTokens).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 () => { + order.push("clear"); + }); + const fetchFn = vi.fn(async () => { + order.push("revoke"); + return new Response(null, { status: 200 }); + }); + const manager = new OAuthManager({ + ...params, + effectiveAuthFetch: fetchFn, + }); + + await expect(manager.clearOAuthTokens()).resolves.toMatchObject({ + status: "revoked", + tokenTypeHint: "refresh_token", + }); + expect(order).toEqual(["revoke", "clear"]); + }); + + it("clears local state even when the revocation request fails", async () => { + const params = createMockParams(); + const storage = params.initialConfig.storage!; + vi.mocked(storage.getTokens).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"], + }); + const manager = new OAuthManager({ + ...params, + effectiveAuthFetch: vi.fn(async () => { + throw new Error("unreachable"); + }), + }); + + await expect(manager.clearOAuthTokens()).resolves.toMatchObject({ + status: "failed", + }); + expect(storage.clear).toHaveBeenCalledWith(SERVER_URL); + }); + + it("skips the request when revocation is turned off", async () => { + const params = createMockParams(); + const fetchFn = vi.fn(); + const manager = new OAuthManager({ + ...params, + effectiveAuthFetch: fetchFn, + }); + + await expect( + manager.clearOAuthTokens({ revoke: false }), + ).resolves.toEqual({ status: "skipped", reason: "disabled" }); + expect(fetchFn).not.toHaveBeenCalled(); + expect(params.initialConfig.storage!.clear).toHaveBeenCalledWith( + SERVER_URL, + ); + }); + it("no-ops when storage is not configured", async () => { const params = createMockParams({ initialConfig: { @@ -207,7 +289,10 @@ describe("OAuthManager", () => { } as OAuthManagerConfig, }); const manager = new OAuthManager(params); - await manager.clearOAuthTokens(); + await expect(manager.clearOAuthTokens()).resolves.toEqual({ + status: "skipped", + reason: "no_tokens", + }); expect(params.getServerUrl).not.toHaveBeenCalled(); }); }); diff --git a/clients/web/src/test/core/mcp/serverList.test.ts b/clients/web/src/test/core/mcp/serverList.test.ts index 8f8ca44e..f1a86384 100644 --- a/clients/web/src/test/core/mcp/serverList.test.ts +++ b/clients/web/src/test/core/mcp/serverList.test.ts @@ -1261,6 +1261,73 @@ describe("oauth.requestRefreshToken (#2068)", () => { }); }); +// #2144 — same inverted-default shape as `requestRefreshToken` above: on by +// default, and only the opt-out is written, so an entry that never touched the +// switch keeps a byte-stable round-trip. +describe("oauthRevokeOnClear (RFC 7009)", () => { + const baseSettings = { + headers: [], + env: [], + metadata: {}, + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 60000, + maxFetchRequests: 1000, + roots: [], + }; + + it("lifts an explicit false to settings.oauthRevokeOnClear", () => { + expect( + storedFieldsToInspectorSettings({ + oauth: { clientId: "cid", revokeOnClear: false }, + })?.oauthRevokeOnClear, + ).toBe(false); + }); + + it("leaves the setting unset when the field is absent (default: on)", () => { + expect( + storedFieldsToInspectorSettings({ oauth: { clientId: "cid" } }) + ?.oauthRevokeOnClear, + ).toBeUndefined(); + }); + + it("leaves the setting unset for an explicit true, which means the default", () => { + expect( + storedFieldsToInspectorSettings({ + oauth: { clientId: "cid", revokeOnClear: true }, + })?.oauthRevokeOnClear, + ).toBeUndefined(); + }); + + it("persists the opt-out under oauth on disk", () => { + expect( + inspectorSettingsToStoredFields({ + ...baseSettings, + oauthRevokeOnClear: false, + }).oauth?.revokeOnClear, + ).toBe(false); + }); + + it("writes nothing when the setting is on", () => { + expect( + inspectorSettingsToStoredFields({ + ...baseSettings, + oauthRevokeOnClear: true, + }).oauth, + ).toBeUndefined(); + }); + + it("round-trips the opt-out", () => { + const stored = inspectorSettingsToStoredFields({ + ...baseSettings, + oauthRevokeOnClear: false, + }); + expect(storedFieldsToInspectorSettings(stored)?.oauthRevokeOnClear).toBe( + false, + ); + }); +}); + describe("oauthOnInsufficientScope (SEP-2350)", () => { const baseSettings = { headers: [], diff --git a/clients/web/src/test/integration/auth/revocation-e2e.test.ts b/clients/web/src/test/integration/auth/revocation-e2e.test.ts new file mode 100644 index 00000000..3d0fd881 --- /dev/null +++ b/clients/web/src/test/integration/auth/revocation-e2e.test.ts @@ -0,0 +1,189 @@ +/** + * RFC 7009 revocation against the real OAuth test server (#2144). + * + * The unit suite (`src/test/core/auth/revocation.test.ts`) pins what is *sent*; + * this pins what the request actually *does*. The two halves that only a real + * authorization server can show are both here: that the Inspector's request is + * accepted at all (its client authentication and form encoding are right), and + * that §2.1's grant linkage holds — one request naming the refresh token kills + * the access token issued alongside it, which is the whole reason the Inspector + * sends one request rather than two. + */ +import { describe, it, expect, beforeAll, afterAll } from "vitest"; +import { createHash, randomBytes } from "node:crypto"; +import { + TestServerHttp, + createOAuthTestServerConfig, + getDefaultServerConfig, + waitForOAuthWellKnown, +} from "@modelcontextprotocol/inspector-test-server"; +import { BrowserOAuthStorage } from "@inspector/core/auth/browser/storage.js"; +import { revokeStoredOAuthTokens } from "@inspector/core/auth/revocation.js"; +import type { OAuthMetadata } from "@modelcontextprotocol/client"; + +const CLIENT_ID = "test-2144-revocation"; +const CLIENT_SECRET = "test-2144-secret"; +const REDIRECT_URL = "http://localhost:3000/oauth/callback"; + +function base64Url(buffer: Buffer): string { + return buffer + .toString("base64") + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=/g, ""); +} + +describe("OAuth token revocation (RFC 7009)", () => { + let mcpServer: TestServerHttp | null = null; + let serverUrl = ""; + let metadata: OAuthMetadata; + + beforeAll(async () => { + mcpServer = new TestServerHttp({ + ...getDefaultServerConfig(), + serverType: "streamable-http" as const, + ...createOAuthTestServerConfig({ + requireAuth: true, + staticClients: [ + { + clientId: CLIENT_ID, + clientSecret: CLIENT_SECRET, + redirectUris: [REDIRECT_URL], + }, + ], + }), + }); + const port = await mcpServer.start(); + serverUrl = `http://localhost:${port}`; + await waitForOAuthWellKnown(serverUrl); + metadata = (await ( + await fetch(`${serverUrl}/.well-known/oauth-authorization-server`) + ).json()) as OAuthMetadata; + }, 30_000); + + afterAll(async () => { + await mcpServer?.stop(); + mcpServer = null; + }, 30_000); + + /** Run a real authorization-code exchange and return the issued tokens. */ + async function authorize(): Promise<{ + access_token: string; + refresh_token: string; + }> { + const verifier = base64Url(randomBytes(32)); + const challenge = base64Url(createHash("sha256").update(verifier).digest()); + + const authorizeResponse = await fetch(`${serverUrl}/oauth/authorize`, { + method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + redirect: "manual", + body: new URLSearchParams({ + client_id: CLIENT_ID, + redirect_uri: REDIRECT_URL, + response_type: "code", + scope: "mcp", + code_challenge: challenge, + code_challenge_method: "S256", + }), + }); + const location = authorizeResponse.headers.get("location"); + expect(location).toBeTruthy(); + const code = new URL(location!).searchParams.get("code"); + expect(code).toBeTruthy(); + + const tokenResponse = await fetch(`${serverUrl}/oauth/token`, { + method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + body: new URLSearchParams({ + grant_type: "authorization_code", + code: code!, + redirect_uri: REDIRECT_URL, + client_id: CLIENT_ID, + client_secret: CLIENT_SECRET, + code_verifier: verifier, + }), + }); + expect(tokenResponse.status).toBe(200); + return (await tokenResponse.json()) as { + access_token: string; + refresh_token: string; + }; + } + + /** Whether the MCP endpoint still accepts this bearer token. */ + async function tokenAccepted(accessToken: string): Promise { + const response = await fetch(`${serverUrl}/mcp`, { + method: "POST", + headers: { Authorization: `Bearer ${accessToken}` }, + }); + return response.status !== 401; + } + + async function seededStorage(tokens: { + access_token: string; + refresh_token?: string; + }): Promise { + const storage = new BrowserOAuthStorage(); + await storage.clear(serverUrl); + await storage.saveTokens(serverUrl, { token_type: "Bearer", ...tokens }); + await storage.saveServerMetadata(serverUrl, metadata); + // Preconfigured, so it goes in the preregistered slot — the same one a + // server with `oauth.clientId` uses. That is the slot the revocation path + // must read first, or a confidential client sends no authentication at all. + await storage.savePreregisteredClientInformation(serverUrl, { + client_id: CLIENT_ID, + client_secret: CLIENT_SECRET, + }); + return storage; + } + + it("advertises a revocation endpoint", () => { + expect(metadata.revocation_endpoint).toBe(`${serverUrl}/oauth/revoke`); + }); + + // The failure this feature exists to fix: without the request, the token is + // still accepted after the Inspector has forgotten it. + it("revokes the whole grant from the stored refresh token", async () => { + const tokens = await authorize(); + expect(await tokenAccepted(tokens.access_token)).toBe(true); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl, + storage: await seededStorage(tokens), + fetchFn: fetch, + }); + + expect(outcome).toMatchObject({ + status: "revoked", + tokenTypeHint: "refresh_token", + }); + // RFC 7009 §2.1 — the access token issued under the same grant dies too, + // which is why one request is enough. + expect(await tokenAccepted(tokens.access_token)).toBe(false); + }); + + it("revokes an access token when no refresh token was issued", async () => { + const tokens = await authorize(); + const outcome = await revokeStoredOAuthTokens({ + serverUrl, + storage: await seededStorage({ access_token: tokens.access_token }), + fetchFn: fetch, + }); + + expect(outcome).toMatchObject({ + status: "revoked", + tokenTypeHint: "access_token", + }); + expect(await tokenAccepted(tokens.access_token)).toBe(false); + }); + + // RFC 7009 §2.2: an unknown token is a success, so revoking a grant the + // server has already expired must not be reported as a failure. + it("treats an already-unknown token as revoked", async () => { + const storage = await seededStorage({ access_token: "never-issued" }); + await expect( + revokeStoredOAuthTokens({ serverUrl, storage, fetchFn: fetch }), + ).resolves.toMatchObject({ status: "revoked" }); + }); +}); diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index deafe89b..55dd7dca 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -5447,8 +5447,12 @@ describe("InspectorClient", () => { expect(c.getOAuthFlowStep()).toBeUndefined(); expect(c.getOAuthFlowState()).toBeUndefined(); await expect(c.getOAuthState()).resolves.toBeUndefined(); - // clearOAuthTokens is a no-op when there is no manager - await expect(c.clearOAuthTokens()).resolves.toBeUndefined(); + // clearOAuthTokens is a no-op when there is no manager; it still + // reports an outcome so callers have one shape to read (#2144). + await expect(c.clearOAuthTokens()).resolves.toEqual({ + status: "skipped", + reason: "no_tokens", + }); }); it("setOAuthConfig throws when oauthManager is unset", () => { diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 1ab2bada..aabda5f2 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -639,6 +639,11 @@ describe("server.ts supplemental coverage", () => { expect((await res.json()).error).toMatch(/oauthRequestRefreshToken/); }); + it("rejects a non-boolean oauthRevokeOnClear (#2144)", async () => { + const res = await postSettings({ ...base, oauthRevokeOnClear: "no" }); + expect((await res.json()).error).toMatch(/oauthRevokeOnClear/); + }); + it("rejects malformed roots", async () => { const res = await postSettings({ ...base, roots: [{ uri: 1 }] }); expect((await res.json()).error).toMatch(/roots/); @@ -708,6 +713,32 @@ describe("server.ts supplemental coverage", () => { expect(body.mcpServers.srv?.oauth?.requestRefreshToken).toBeUndefined(); }); + // #2144 — same reasoning as the refresh-token pair above: a 200 only proves + // the payload validated, not that the field survived the write-through. + it("persists the revoke-on-clear opt-out through a save", async () => { + expect( + (await postSettings({ ...base, oauthRevokeOnClear: false })).status, + ).toBe(200); + + const res = await fetch(`${h.baseUrl}/api/servers`); + const body = (await res.json()) as { + mcpServers: Record; + }; + expect(body.mcpServers.srv?.oauth?.revokeOnClear).toBe(false); + }); + + it("writes no revoke-on-clear field when the setting is on", async () => { + expect( + (await postSettings({ ...base, oauthRevokeOnClear: true })).status, + ).toBe(200); + + const res = await fetch(`${h.baseUrl}/api/servers`); + const body = (await res.json()) as { + mcpServers: Record }>; + }; + expect(body.mcpServers.srv?.oauth?.revokeOnClear).toBeUndefined(); + }); + it("accepts a fully-populated valid settings payload", async () => { const res = await postSettings({ ...base, @@ -724,6 +755,7 @@ describe("server.ts supplemental coverage", () => { oauthScopes: "a b", enterpriseManaged: true, oauthRequestRefreshToken: false, + oauthRevokeOnClear: false, roots: [{ uri: "file:///x", name: "x" }], }); expect(res.status).toBe(200); @@ -892,6 +924,30 @@ describe("server.ts supplemental coverage", () => { } }); + // #2144 — same all-or-nothing rule for the revocation opt-out. + it("drops oauth whose revokeOnClear is not a boolean", async () => { + const h = await start({ + seedConfig: JSON.stringify({ + mcpServers: { + srv: { + type: "streamable-http", + url: "https://x.test/mcp", + oauth: { revokeOnClear: "no" }, + }, + }, + }), + }); + try { + const res = await fetch(`${h.baseUrl}/api/servers`); + const body = (await res.json()) as { + mcpServers: Record>; + }; + expect(body.mcpServers.srv).not.toHaveProperty("oauth"); + } finally { + await stop(h); + } + }); + it("keeps a well-formed requestRefreshToken opt-out on read (#2068)", async () => { const h = await start({ seedConfig: JSON.stringify({ diff --git a/core/auth/index.ts b/core/auth/index.ts index f38fcbe0..3535ac83 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -134,5 +134,22 @@ export { // Discovery export { discoverScopes } from "./discovery.js"; +// RFC 7009 token revocation (#2144) +export { + DEFAULT_REVOCATION_TIMEOUT_MS, + buildRevocationRequest, + revocationAuthMethods, + revokeStoredOAuthTokens, + revokeToken, + selectRevocableToken, +} from "./revocation.js"; +export type { + RevocationRequestParams, + RevokeStoredOAuthTokensParams, + RevokeTokenParams, + TokenRevocationOutcome, + TokenRevocationSkipReason, +} from "./revocation.js"; + // Logging (re-exported from core/logging) export { silentLogger } from "../logging/index.js"; diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts new file mode 100644 index 00000000..91167996 --- /dev/null +++ b/core/auth/revocation.ts @@ -0,0 +1,335 @@ +/** + * RFC 7009 OAuth 2.0 Token Revocation (#2144). + * + * Clearing the Inspector's OAuth state used to delete the *local* copy of the + * tokens and stop there: the access token, and the refresh token when one was + * issued, stayed valid at the authorization server until they expired on their + * own. From the AS's side nothing happened — the client just went quiet — so a + * day of connect/disconnect iteration left it holding a pile of live grants for + * sessions that had ended hours ago. RFC 7009 §1 describes exactly this case + * (a client invalidating its tokens when the user logs out or walks away) and + * this module is the request that closes it. + * + * Everything here is **best-effort**, and deliberately so: the local clear is + * what the user asked for, and it must finish whether or not the AS cooperates. + * An authorization server that advertises no `revocation_endpoint` is left + * behaving exactly as before, a network error or a non-2xx is reported and + * swallowed, and a short timeout keeps a slow AS from hanging the teardown. + * Every path therefore returns a {@link TokenRevocationOutcome} rather than + * throwing — a caller is telling us to forget these tokens, and there is no + * failure here that should stop it. + */ + +import type { + OAuthClientInformation, + OAuthMetadata, + OAuthTokens, +} from "@modelcontextprotocol/client"; +import { selectClientAuthMethod } from "@modelcontextprotocol/client"; +import type { InspectorLogger } from "../logging/index.js"; +import type { OAuthStorage } from "./storage.js"; + +/** + * How long a revocation request may take before it is abandoned. + * + * Short on purpose. This runs inside a disconnect the user has already + * committed to, so an unreachable or wedged authorization server must not hold + * the teardown open — five seconds is long enough for a real endpoint on a slow + * link and short enough that a dead one is not felt as a hang. + */ +export const DEFAULT_REVOCATION_TIMEOUT_MS = 5000; + +/** Why a revocation request was not sent. */ +export type TokenRevocationSkipReason = + /** The caller turned revocation off for this server. */ + | "disabled" + /** The authorization server advertises no `revocation_endpoint`. */ + | "no_endpoint" + /** Nothing is stored for this server, so there is no grant to revoke. */ + | "no_tokens" + /** Authorization-server metadata could not be resolved at all. */ + | "no_metadata"; + +/** + * What happened on the revocation leg. Never an exception: see the module + * comment — the local clear runs regardless, so this is reported, not thrown. + */ +export type TokenRevocationOutcome = + | { + status: "revoked"; + /** Which token RFC 7009 §2.1 was asked about. */ + tokenTypeHint: "refresh_token" | "access_token"; + endpoint: string; + } + | { status: "skipped"; reason: TokenRevocationSkipReason } + | { status: "failed"; detail: string; endpoint?: string }; + +/** + * The token to name in the request, and the hint that describes it. + * + * A refresh token is preferred when one exists, and not merely as a + * tie-breaker: RFC 7009 §2.1 says an AS asked to revoke a refresh token SHOULD + * also invalidate the access tokens issued under the same grant, so the single + * request covers both. Naming the access token instead would leave the refresh + * token — the long-lived half, and the one a user cannot wait out — alive. + */ +export function selectRevocableToken( + tokens: OAuthTokens | undefined, +): { token: string; tokenTypeHint: "refresh_token" | "access_token" } | null { + if (!tokens) return null; + if (tokens.refresh_token) { + return { token: tokens.refresh_token, tokenTypeHint: "refresh_token" }; + } + if (tokens.access_token) { + return { token: tokens.access_token, tokenTypeHint: "access_token" }; + } + return null; +} + +/** + * The client-authentication methods to consider for the revocation endpoint. + * + * 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. + */ +export function revocationAuthMethods(metadata: OAuthMetadata): string[] { + return ( + metadata.revocation_endpoint_auth_methods_supported ?? + metadata.token_endpoint_auth_methods_supported ?? + [] + ); +} + +export interface RevocationRequestParams { + endpoint: string; + token: string; + tokenTypeHint: "refresh_token" | "access_token"; + clientInformation?: OAuthClientInformation; + supportedAuthMethods: string[]; +} + +/** + * Build the RFC 7009 §2.1 request — an `application/x-www-form-urlencoded` POST + * carrying `token` and `token_type_hint`, plus client authentication. + * + * Split out from {@link revokeToken} because *what is sent* is the part worth + * asserting directly: a credential in the wrong place (basic vs. post) is a + * silent 401 at a real AS and indistinguishable from "the server declined" in + * an end-to-end test. + */ +export function buildRevocationRequest(params: RevocationRequestParams): { + url: string; + init: RequestInit; +} { + const body = new URLSearchParams({ + token: params.token, + token_type_hint: params.tokenTypeHint, + }); + const headers: Record = { + "Content-Type": "application/x-www-form-urlencoded", + Accept: "application/json", + }; + + const client = params.clientInformation; + if (client) { + const method = selectClientAuthMethod(client, params.supportedAuthMethods); + if (method === "client_secret_basic" && client.client_secret) { + // Deliberately byte-identical to what the SDK's `applyBasicAuth` sends at + // the token endpoint: the raw `id:secret`, *not* the form-urlencoded pair + // RFC 6749 §2.3.1 asks for. Matching the RFC here instead would mean this + // request and the token request present the same credential differently, + // so an authorization server holding a secret with a reserved character + // could accept one and reject the other — a failure that would look like + // "revocation is broken" rather than like an encoding disagreement. + // Whatever the AS accepted to mint these tokens is what ends them. + headers.Authorization = `Basic ${base64Encode(`${client.client_id}:${client.client_secret}`)}`; + } else if (method === "client_secret_post" && client.client_secret) { + body.set("client_id", client.client_id); + body.set("client_secret", client.client_secret); + } else { + // Public client (or a confidential one with no secret on hand): RFC 7009 + // §2.1 still wants the client identified. + body.set("client_id", client.client_id); + } + } + + return { + url: params.endpoint, + init: { method: "POST", headers, body: body.toString() }, + }; +} + +/** + * Base64 for the Basic credential, in both runtimes this code runs in. + * + * `btoa` is the browser's and is byte-oriented, so the percent-encoded + * credential above (which is ASCII by construction) is safe to pass it. Node + * has `Buffer`; reaching for it first would pull a Node built-in into the + * browser bundle, which the #1769 build gate rejects outright. + */ +function base64Encode(value: string): string { + if (typeof btoa === "function") return btoa(value); + /* v8 ignore next 2 -- Node 22 and every supported browser define btoa; this is the belt-and-braces branch. */ + return Buffer.from(value, "utf8").toString("base64"); +} + +export interface RevokeTokenParams extends RevocationRequestParams { + fetchFn: typeof fetch; + timeoutMs?: number; +} + +/** + * POST the revocation request and classify the answer. + * + * RFC 7009 §2.2 makes a 200 the success case *and* the response to a token the + * AS does not recognize, which is why an already-expired token is not an error + * here. Anything else — a 4xx, a 5xx, a network failure, the timeout — is + * reported as `failed` and goes no further than a warning at the call site. + */ +export async function revokeToken( + params: RevokeTokenParams, +): Promise { + 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), + }); + if (!response.ok) { + return { + status: "failed", + endpoint: url, + detail: + `revocation endpoint responded ${response.status} ${response.statusText}`.trim(), + }; + } + return { + status: "revoked", + tokenTypeHint: params.tokenTypeHint, + endpoint: url, + }; + } catch (err) { + return { + status: "failed", + endpoint: url, + detail: err instanceof Error ? err.message : String(err), + }; + } +} + +export interface RevokeStoredOAuthTokensParams { + serverUrl: string; + storage: OAuthStorage; + fetchFn: typeof fetch; + /** + * `false` skips the request entirely — the deliberate case from #2144, where + * a user wants to watch a server cope with a client that walks off still + * holding live tokens. + */ + enabled?: boolean; + timeoutMs?: number; + logger?: InspectorLogger; +} + +/** + * Revoke the tokens the Inspector holds for `serverUrl`, reading everything it + * needs out of the OAuth store. + * + * Called immediately **before** the local clear, since the store is where the + * token, the client credentials, and the discovered `revocation_endpoint` all + * live — after the clear there is nothing left to revoke with. + * + * The metadata comes from the cache the OAuth flow already populated rather + * than from a fresh discovery round-trip: the tokens being revoked were minted + * by that same authorization server, so its cached document is the document + * that describes them, and re-discovering would add two network legs to a + * teardown for no new information. A server that has never completed an OAuth + * flow has no cached metadata *and* no tokens, so it short-circuits either way. + */ +export async function revokeStoredOAuthTokens( + params: RevokeStoredOAuthTokensParams, +): Promise { + const { serverUrl, logger } = params; + if (params.enabled === false) { + return { status: "skipped", reason: "disabled" }; + } + + const outcome = await computeOutcome(params); + if (outcome.status === "failed") { + logger?.warn( + { serverUrl, endpoint: outcome.endpoint, detail: outcome.detail }, + "Token revocation failed; clearing local OAuth state anyway", + ); + } else if (outcome.status === "skipped" && outcome.reason === "no_endpoint") { + logger?.debug( + { serverUrl }, + "Skipping token revocation: authorization server metadata has no revocation_endpoint", + ); + } + return outcome; +} + +/** + * The client credentials to authenticate the revocation request with. + * + * Mirrors `BaseOAuthClientProvider.clientInformation`: the preregistered + * (static, issuer-independent) entry wins, then the per-issuer dynamic + * registration. Reading only the second would silently drop client + * authentication for every server configured with an `oauth.clientId` — the + * confidential case, where an authorization server is most likely to *require* + * it and answer 401. + */ +async function resolveClientInformation( + storage: OAuthStorage, + serverUrl: string, +): Promise { + return ( + (await storage.getClientInformation(serverUrl, true)) ?? + (await storage.getClientInformation(serverUrl, false)) + ); +} + +async function computeOutcome( + params: RevokeStoredOAuthTokensParams, +): Promise { + const { serverUrl, storage, fetchFn } = params; + + // Read the token first. It is the cheapest disqualifier, and a server with no + // stored grant should not provoke a metadata read at all. + let tokens: OAuthTokens | undefined; + let metadata: OAuthMetadata | null; + let clientInformation: OAuthClientInformation | undefined; + try { + tokens = await storage.getTokens(serverUrl); + const revocable = selectRevocableToken(tokens); + if (!revocable) return { status: "skipped", reason: "no_tokens" }; + + metadata = await storage.getServerMetadata(serverUrl); + if (!metadata) return { status: "skipped", reason: "no_metadata" }; + if (!metadata.revocation_endpoint) { + return { status: "skipped", reason: "no_endpoint" }; + } + + clientInformation = await resolveClientInformation(storage, serverUrl); + return await revokeToken({ + endpoint: metadata.revocation_endpoint, + token: revocable.token, + tokenTypeHint: revocable.tokenTypeHint, + clientInformation, + supportedAuthMethods: revocationAuthMethods(metadata), + fetchFn, + timeoutMs: params.timeoutMs, + }); + } catch (err) { + // A store that cannot be read (a corrupt blob, a remote backend that 500s) + // is not a reason to abandon the clear the user asked for. + return { + status: "failed", + detail: err instanceof Error ? err.message : String(err), + }; + } +} diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 2f115beb..6e63c1a5 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -238,6 +238,7 @@ import { type HandleAuthChallengeOptions, } from "../auth/challenge.js"; import { withOAuthEndpointOverrides } from "../auth/endpointOverrides.js"; +import type { TokenRevocationOutcome } from "../auth/revocation.js"; import type { OAuthTokens } from "@modelcontextprotocol/client"; import { silentLogger, type InspectorLogger } from "../logging/logger.js"; import { createFetchTracker } from "./fetchTracking.js"; @@ -813,6 +814,7 @@ export class InspectorClient extends InspectorClientEventTarget { return Promise.resolve(); }, initialConfig: oauthConfig, + logger: this.logger, enterpriseManagedAuth: options.enterpriseManagedAuth, installEnterpriseManagedAuth: options.installEnterpriseManagedAuth, dispatchOAuthComplete: (detail) => @@ -6722,8 +6724,25 @@ export class InspectorClient extends InspectorClientEventTarget { /** * Clears OAuth tokens and client information */ - async clearOAuthTokens(): Promise { - await this.oauthManager?.clearOAuthTokens(); + /** + * Drop this server's stored OAuth state, revoking the grant at the + * authorization server first (RFC 7009, #2144). + * + * The revocation is best-effort — an authorization server that advertises no + * `revocation_endpoint` is left behaving exactly as before, and a network + * error, a non-2xx or a timeout is reported in the returned outcome rather + * than thrown — so the local clear always completes. Pass + * `{ revoke: false }` to skip it. + */ + async clearOAuthTokens(options?: { + revoke?: boolean; + }): Promise { + return ( + (await this.oauthManager?.clearOAuthTokens(options)) ?? { + status: "skipped", + reason: "no_tokens", + } + ); } /** diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index 51e61af7..5c93d9f7 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -13,6 +13,11 @@ import type { OAuthClientInformation } from "@modelcontextprotocol/client"; import { mcpAuth } from "../auth/mcpAuth.js"; import type { OAuthStorage } from "../auth/storage.js"; import { parseOAuthState } from "../auth/utils.js"; +import { + revokeStoredOAuthTokens, + type TokenRevocationOutcome, +} from "../auth/revocation.js"; +import type { InspectorLogger } from "../logging/index.js"; import type { EnterpriseManagedAuthIdpConfig } from "../client/types.js"; import type { ClientConfig } from "../client/types.js"; import { EmaClientNotConfiguredError } from "../auth/ema/clientConfigError.js"; @@ -71,6 +76,8 @@ export interface OAuthManagerParams { dispatchOAuthComplete: (detail: { tokens: OAuthTokens }) => void; dispatchOAuthAuthorizationRequired: (detail: { url: URL }) => void; dispatchOAuthError: (detail: { error: Error }) => void; + /** Used for the best-effort RFC 7009 revocation warning (#2144). */ + logger?: InspectorLogger; } /** @@ -488,16 +495,42 @@ export class OAuthManager { } } - async clearOAuthTokens(): Promise { + /** + * Revoke the grant at the authorization server (RFC 7009), then drop the + * local OAuth state. + * + * Revocation runs **first** and reads what it needs out of the same store the + * next line wipes — the token, the client credentials and the discovered + * `revocation_endpoint` all live there, so after the clear there is nothing + * left to revoke with. It is best-effort by construction: every failure is + * reported through the returned outcome and the clear proceeds regardless, + * because forgetting the tokens is what the caller actually asked for (#2144). + * + * `options.revoke === false` skips the request. That is not only an escape + * hatch for an authorization server that mishandles it — disconnecting while + * still holding live tokens is a case a user may want to reproduce + * deliberately, to watch how a server under test copes with it. + */ + async clearOAuthTokens(options?: { + revoke?: boolean; + }): Promise { if (!this.oauthConfig?.storage) { - return; + return { status: "skipped", reason: "no_tokens" }; } const serverUrl = this.getServerUrl(); + const outcome = await revokeStoredOAuthTokens({ + serverUrl, + storage: this.oauthConfig.storage, + fetchFn: this.params.effectiveAuthFetch, + enabled: options?.revoke, + logger: this.params.logger, + }); await this.oauthConfig.storage.clear(serverUrl); this.oauthFlowState = null; this.pendingAuthorizationScope = undefined; + return outcome; } async isOAuthAuthorized(): Promise { diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 33d57e6d..0e469582 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1392,6 +1392,7 @@ export function createRemoteApp( tokenUrl?: string; enterpriseManaged?: boolean; requestRefreshToken?: boolean; + revokeOnClear?: boolean; } => { if (v === null || typeof v !== "object" || Array.isArray(v)) return false; const o = v as Record; @@ -1427,6 +1428,11 @@ export function createRemoteApp( ) { return false; } + // #2144 — shape only, same as the flag above: the read side keeps just an + // explicit `false`, so a stray `true` reads back as the default anyway. + if (o.revokeOnClear !== undefined && typeof o.revokeOnClear !== "boolean") { + return false; + } return true; }; // A roots array: each entry must have a string `uri` and, when present, a @@ -1540,7 +1546,7 @@ export function createRemoteApp( if ("oauth" in valObj && !isOauthObject(valObj.oauth)) { logWarn( { route: "/api/servers", id, droppedKey: "oauth" }, - "Dropping malformed `oauth` field — expected `{ clientId?, clientSecret?, scopes?, authorizationParams?, authorizationUrl?, tokenUrl?, enterpriseManaged?, onInsufficientScope?, requestRefreshToken? }`.", + "Dropping malformed `oauth` field — expected `{ clientId?, clientSecret?, scopes?, authorizationParams?, authorizationUrl?, tokenUrl?, enterpriseManaged?, onInsufficientScope?, requestRefreshToken?, revokeOnClear? }`.", ); delete valObj.oauth; } @@ -1852,6 +1858,15 @@ export function createRemoteApp( error: "settings.oauthRequestRefreshToken must be a boolean", }; } + if ( + obj.oauthRevokeOnClear !== undefined && + typeof obj.oauthRevokeOnClear !== "boolean" + ) { + return { + ok: false, + error: "settings.oauthRevokeOnClear must be a boolean", + }; + } if ( obj.oauthOnInsufficientScope !== undefined && obj.oauthOnInsufficientScope !== "reauthorize" && @@ -1984,6 +1999,10 @@ export function createRemoteApp( if (obj.oauthRequestRefreshToken === false) { value.oauthRequestRefreshToken = false; } + // #2144: same shape — the default is on, so only the opt-out travels. + if (obj.oauthRevokeOnClear === false) { + value.oauthRevokeOnClear = false; + } if ( obj.oauthOnInsufficientScope === "reauthorize" || obj.oauthOnInsufficientScope === "throw" diff --git a/core/mcp/serverList.ts b/core/mcp/serverList.ts index d7937da9..e37fd0a2 100644 --- a/core/mcp/serverList.ts +++ b/core/mcp/serverList.ts @@ -617,6 +617,11 @@ export function storedFieldsToInspectorSettings( if (stored.oauth?.requestRefreshToken === false) { settings.oauthRequestRefreshToken = false; } + // Same inverted default as the flag above: revocation is on unless the entry + // explicitly opted out. (#2144) + if (stored.oauth?.revokeOnClear === false) { + settings.oauthRevokeOnClear = false; + } // Mirror the stdio working directory for the form. Like the OAuth fields, an // empty string coerces to absent so the form's "(inherit)" placeholder shows. if (stored.cwd) settings.cwd = stored.cwd; @@ -750,6 +755,10 @@ export function inspectorSettingsToStoredFields( if (settings.oauthRequestRefreshToken === false) { oauthFields.requestRefreshToken = false; } + // Same omit-the-default rule as the flag above. (#2144) + if (settings.oauthRevokeOnClear === false) { + oauthFields.revokeOnClear = false; + } if (Object.keys(oauthFields).length > 0) { out.oauth = oauthFields; } diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 17170ac3..841ee819 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -193,6 +193,14 @@ export type StoredMCPServer = MCPServerConfig & { * Inspector-specific. (#2068) */ requestRefreshToken?: boolean; + /** + * Whether clearing this server's OAuth state also revokes the grant at the + * authorization server (RFC 7009). Defaults to `true`; only `false` is + * written to disk, so an entry that never touched the setting keeps a + * minimal diff. See {@link InspectorServerSettings.oauthRevokeOnClear}. + * Inspector-specific. (#2144) + */ + revokeOnClear?: boolean; /** * Custom query parameters appended to the OAuth **authorization request** * (never the token request) — e.g. Keycloak's `kc_idp_hint`, OIDC's @@ -613,6 +621,12 @@ export interface OAuthSettings { * reason as `authorizationParams`; `undefined` means the default, on. */ requestRefreshToken?: boolean; + /** + * Whether clearing the stored OAuth state revokes the grant at the + * authorization server (#2144). Optional for the same reason as + * `authorizationParams`; `undefined` means the default, on. + */ + revokeOnClear?: boolean; } /** @@ -818,6 +832,24 @@ export interface InspectorServerSettings { * the stored OAuth state is cleared. */ oauthRequestRefreshToken?: boolean; + /** + * Whether clearing this server's stored OAuth state first revokes the grant + * at the authorization server, per RFC 7009 (#2144). `undefined` (the + * default) means on; persisted as `oauth.revokeOnClear` only when explicitly + * off. + * + * On is the right default because the alternative is silent: the Inspector + * deletes its local copy and the access token — and the refresh token, which + * is long-lived by design — stay valid at the authorization server until they + * expire on their own, leaving it holding grants for sessions that ended + * hours ago. + * + * Turning it off is a testing affordance rather than only an escape hatch: a + * client that walks away still holding live tokens is a case a server author + * may want to reproduce on purpose. It is read at clear time, not at connect + * time, so toggling it takes effect on the next clear without reconnecting. + */ + oauthRevokeOnClear?: boolean; /** * When true, connect via the configured enterprise IdP (EMA) instead of * interactive OAuth to the MCP authorization server. Per-server OAuth diff --git a/docs/mcp-server-configuration.md b/docs/mcp-server-configuration.md index e45f0a8a..bfba252e 100644 --- a/docs/mcp-server-configuration.md +++ b/docs/mcp-server-configuration.md @@ -175,7 +175,7 @@ These have no analog in the broader `mcp.json` ecosystem. Each is **omitted on w | `paginatedLists` | `false` | Fetch tools/resources/prompts one page at a time instead of auto-aggregating | | `advertisedExtensions` | — | Per-extension overrides for what the Inspector declares in `capabilities.extensions` | | `maxFetchRequests` | `1000` | Network-log retention for this server (`DEFAULT_MAX_FETCH_REQUESTS`); `0` means unlimited | -| `oauth` | — | `{ clientId, clientSecret, scopes, requestRefreshToken, authorizationParams, authorizationUrl, tokenUrl, enterpriseManaged, onInsufficientScope }` | +| `oauth` | — | `{ clientId, clientSecret, scopes, requestRefreshToken, revokeOnClear, authorizationParams, authorizationUrl, tokenUrl, enterpriseManaged, onInsufficientScope }` | `metadata` is a JSON **object**, and its values may be any JSON — an object, an array, a number, a boolean, `null` — not only a string. The MCP spec does not restrict `_meta` value types in general, and the SDK models the field as a passthrough object, so the Inspector does not narrow it either ([#1910](https://github.com/modelcontextprotocol/inspector/issues/1910)). Edit it in Server Settings → Request Metadata, which is a JSON editor rather than the key/value rows `headers` and `env` use; text that is not a JSON object is flagged inline and not applied. @@ -209,7 +209,21 @@ The setting suppresses the grant declaration and the SDK's scope augmentation > **Two pieces of state outlive the setting, and neither is cleared by unchecking it.** A refresh token issued before the opt-out stays usable — the SDK's refresh path posts the stored token with `grant_type=refresh_token` without consulting the client metadata, so such a server can keep refreshing silently and you may not observe the cost described above at all. And the registration already held at the authorization server still lists the grant, because turning the setting off changes what the Inspector *declares*, not what the AS has recorded. > -> **Clear stored OAuth state** (Server Settings → Authorization) clears the Inspector's **local** copies — the tokens and the client information — which is enough for the first. It is not a revocation: it cannot reach the authorization server. For the second, what happens next depends on how the client was obtained. A **dynamically registered** client is registered afresh on the next connect, and the new registration declares only `authorization_code`; the old one still exists at the AS, unused. A **preconfigured `oauth.clientId`** is reused as-is, so changing what that client declares is done at the authorization server, not here. +> **Clear stored OAuth state** (Server Settings → Authorization) clears the Inspector's **local** copies — the tokens and the client information — and, where the authorization server supports it, revokes the grant there too (see `oauth.revokeOnClear` below), which settles the first. It does **not** touch the registration. For that, what happens next depends on how the client was obtained. A **dynamically registered** client is registered afresh on the next connect, and the new registration declares only `authorization_code`; the old one still exists at the AS, unused. A **preconfigured `oauth.clientId`** is reused as-is, so changing what that client declares is done at the authorization server, not here. + +`oauth.revokeOnClear` (default `true`) controls whether clearing this server's stored OAuth state first **revokes the grant at the authorization server**, per [RFC 7009](https://datatracker.ietf.org/doc/html/rfc7009). Uncheck **Revoke tokens on clear** in Server Settings → Authorization to turn it off; only `false` is written to disk, so a server that never touched the setting keeps a minimal entry ([#2144](https://github.com/modelcontextprotocol/inspector/issues/2144)). + +Without it, clearing is silent from the authorization server's point of view: the Inspector deletes its local copy and the access token — and the refresh token, which is long-lived by design — stay valid there until they expire on their own. A day of connect/disconnect iteration leaves the AS holding a pile of grants for sessions that ended hours ago, and nothing in the Inspector can see or end them. RFC 7009 §1 describes this exact case; the clear is that moment. + +The request names the **refresh token** when there is one. RFC 7009 §2.1 asks an authorization server to also invalidate the access tokens issued under the same grant, so one request covers both halves; naming the access token instead would leave the long-lived one alive. + +> **It is best-effort, and the local clear always finishes.** An authorization server that advertises no `revocation_endpoint` is left behaving exactly as it did before this existed — nothing is sent. A network error, a non-2xx, or a slow server that trips the short timeout is reported (a toast in the web client, a status line in the TUI, a stderr warning from the CLI) and nothing more. Forgetting the tokens is what you asked for, so no failure on this leg stops it. + +> **Turning it off is a testing affordance, not only an escape hatch.** A client that walks away still holding live tokens is a case a server author may want to reproduce deliberately, to watch how the server under test copes with it. + +> **It is read when you clear, not when you connect** — unlike the OAuth settings above it. Toggling it takes effect on the next clear, with no reconnect needed. + +The three clear paths all honor it: the web client's **Clear OAuth state and disconnect**, the TUI's **Clear OAuth State**, and the CLI's `--relogin` (which also takes a per-run `--no-revoke`; either opt-out is enough to skip the request, and neither can turn it on for the other). One path deliberately never revokes: recovering from a lost authorization state clears a *half-finished* flow so it can be retried, and an authorization that never completed has no grant to revoke. `oauth.authorizationParams` is a string→string record of extra query parameters merged into the OAuth **authorization request** URL only — never the token request. Use it for provider-specific hints the core specs don't standardize (Keycloak's `kc_idp_hint`, OIDC's `login_hint` / `prompt` / `acr_values`, Auth0's `audience`). The protocol-critical parameters — `client_id`, `code_challenge`, `code_challenge_method`, `redirect_uri`, `resource`, `response_type`, `scope`, `state` — are **reserved**: the web form rejects them inline, and any that reach the merge anyway are dropped with a warning rather than overriding what the flow set (overriding them breaks PKCE, the CSRF state binding, or RFC 8707). Edit them in Server Settings → Authorization ("Additional authorization parameters"), beside Scopes. diff --git a/test-servers/configs/oauth-no-revocation-http.json b/test-servers/configs/oauth-no-revocation-http.json new file mode 100644 index 00000000..e0f055bd --- /dev/null +++ b/test-servers/configs/oauth-no-revocation-http.json @@ -0,0 +1,20 @@ +{ + "serverInfo": { + "name": "oauth-no-revocation", + "version": "1.0.0" + }, + "tools": [{ "preset": "echo" }], + "oauth": { + "enabled": true, + "mode": "combined", + "requireAuth": true, + "scopesSupported": ["mcp"], + "supportDCR": true, + "supportRefreshTokens": true, + "supportRevocation": false + }, + "transport": { + "type": "streamable-http", + "port": 8084 + } +} diff --git a/test-servers/configs/oauth-revocation-http.json b/test-servers/configs/oauth-revocation-http.json new file mode 100644 index 00000000..25518aa2 --- /dev/null +++ b/test-servers/configs/oauth-revocation-http.json @@ -0,0 +1,20 @@ +{ + "serverInfo": { + "name": "oauth-revocation", + "version": "1.0.0" + }, + "tools": [{ "preset": "echo" }], + "oauth": { + "enabled": true, + "mode": "combined", + "requireAuth": true, + "scopesSupported": ["mcp"], + "supportDCR": true, + "supportRefreshTokens": true, + "supportRevocation": true + }, + "transport": { + "type": "streamable-http", + "port": 8083 + } +} diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index 85494d1f..d9799bff 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -670,6 +670,14 @@ export interface ServerConfig { * Whether to support refresh tokens (default: true) */ supportRefreshTokens?: boolean; + + /** + * Whether to advertise and serve the RFC 7009 `revocation_endpoint` + * (default: true). Set to `false` to reproduce an authorization server that + * offers no revocation, where the Inspector must send nothing and clear + * local state exactly as it always has. (#2144) + */ + supportRevocation?: boolean; }; /** * Serve the modern (2026-07-28) protocol era via the SDK's diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index 18e09f5a..c154a696 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -37,6 +37,8 @@ export interface ConfigFileOAuth { supportCIMD?: boolean; tokenExpirationSeconds?: number; supportRefreshTokens?: boolean; + /** RFC 7009 revocation endpoint; default true (#2144). */ + supportRevocation?: boolean; } export interface ConfigFile { diff --git a/test-servers/src/test-server-fixtures.ts b/test-servers/src/test-server-fixtures.ts index 0eeccffc..94910549 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -2910,6 +2910,8 @@ export function createOAuthTestServerConfig(options: { supportCIMD?: boolean; tokenExpirationSeconds?: number; supportRefreshTokens?: boolean; + /** RFC 7009 revocation endpoint; default true (#2144). */ + supportRevocation?: boolean; /** * Move the RFC 9728 metadata document off the well-known path and advertise * it via `WWW-Authenticate: Bearer resource_metadata="…"` (#2071). @@ -2933,6 +2935,7 @@ export function createOAuthTestServerConfig(options: { supportCIMD: options.supportCIMD ?? false, tokenExpirationSeconds: options.tokenExpirationSeconds ?? 3600, supportRefreshTokens: options.supportRefreshTokens ?? true, + supportRevocation: options.supportRevocation ?? true, }, }; } diff --git a/test-servers/src/test-server-oauth.ts b/test-servers/src/test-server-oauth.ts index 3b7a28e8..44454806 100644 --- a/test-servers/src/test-server-oauth.ts +++ b/test-servers/src/test-server-oauth.ts @@ -116,6 +116,9 @@ export function setupOAuthRoutes( if (getOAuthMode(config) === "combined") { setupAuthorizationEndpoint(app, config); setupTokenEndpoint(app, config); + if (config.supportRevocation !== false) { + setupRevocationEndpoint(app); + } if (config.supportDCR) { setupDCREndpoint(app); } @@ -243,6 +246,17 @@ function setupMetadataEndpoints( // RFC 9207 / SEP-2468: advertise iss on authorization responses so // clients must validate (and our e2e can exercise reject paths). authorization_response_iss_parameter_supported: true, + ...(config.supportRevocation !== false && { + // RFC 7009 (#2144). Advertised by default so the in-repo servers + // exercise the Inspector's revocation leg; set + // `oauth.supportRevocation: false` to reproduce an authorization + // server that offers none, where the Inspector must do nothing. + revocation_endpoint: new URL("/oauth/revoke", actualIssuerUrl).href, + revocation_endpoint_auth_methods_supported: [ + "client_secret_basic", + "none", + ], + }), ...(config.supportDCR && { registration_endpoint: new URL("/oauth/register", actualIssuerUrl) .href, @@ -645,6 +659,7 @@ function setupTokenEndpoint( storeRefreshToken(refreshToken, { clientId: client_id, scope: authCodeData.scope, + accessTokens: new Set([accessToken]), }); } @@ -665,6 +680,9 @@ function setupTokenEndpoint( const tokenScope = refreshTokenData.scope || config.scopesSupported?.[0] || "mcp"; const accessToken = generateAccessToken(tokenScope); + // Keep the grant linkage current so a later revocation of this refresh + // token also kills the access token it just minted. + refreshTokenData.accessTokens.add(accessToken); const tokenExpiration = config.tokenExpirationSeconds || 3600; res.json({ @@ -680,6 +698,52 @@ function setupTokenEndpoint( ); } +/** + * RFC 7009 token revocation (#2144). + * + * Deliberately faithful on the two points the Inspector depends on, both of + * which are easy to get wrong in a fixture: + * + * - **§2.2 — an unknown token is a success.** A client revoking a token the + * server has already expired must not be told it failed, so the only 400 here + * is a structurally invalid request (no `token` at all). + * - **§2.1 — revoking a refresh token also invalidates its access tokens.** + * 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. + */ +function setupRevocationEndpoint(app: express.Application): void { + app.post( + "/oauth/revoke", + express.urlencoded({ extended: true }), + (req: Request, res: Response) => { + const token: unknown = req.body?.token; + if (typeof token !== "string" || token === "") { + res.status(400).json({ error: "invalid_request" }); + return; + } + + const refreshTokenData = refreshTokens.get(token); + if (refreshTokenData) { + for (const accessToken of refreshTokenData.accessTokens) { + accessTokens.delete(accessToken); + accessTokenScopes.delete(accessToken); + } + refreshTokens.delete(token); + } else { + accessTokens.delete(token); + accessTokenScopes.delete(token); + } + + // §2.2: 200 whether or not the token was known to us. + res.status(200).end(); + }, + ); +} + /** * Set up Dynamic Client Registration endpoint */ @@ -732,6 +796,15 @@ interface AuthorizationCodeData { interface RefreshTokenData { clientId: string; scope?: string; + /** + * Access tokens minted under the same grant. RFC 7009 §2.1 says an + * authorization server asked to revoke a refresh token SHOULD also invalidate + * the access tokens issued from it, and the Inspector relies on exactly that + * — it sends one request naming the refresh token and expects both halves to + * die. A fixture that dropped only the refresh token would let a client that + * leaves live access tokens behind pass. (#2144) + */ + accessTokens: Set; } interface RegisteredClient {