diff --git a/AGENTS.md b/AGENTS.md index 1fc7f2d34..828636333 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -101,6 +101,43 @@ 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. +│ │ # TWO halves — clearAndPlanRevocation then +│ │ # executeOAuthRevocation — and the split is +│ │ # the contract. The first TAKES the stored +│ │ # state and CLEARS it in one atomic storage +│ │ # step (OAuthStorage.takeRevocationSnapshot); +│ │ # the second sends from what was taken. +│ │ # There is no separate `storage.clear()` in +│ │ # any caller, deliberately — the clear cannot +│ │ # be forgotten or reordered. Both halves of +│ │ # that matter: the requests need state the +│ │ # clear destroys, and the clear must not wait +│ │ # on the network, or a fresh authorization +│ │ # completing during a 5s request is deleted +│ │ # by a clear reasoning about the grant it +│ │ # replaced. NOT cross-process atomic — no +│ │ # OAuth-store mutation takes a lock, and +│ │ # `clear` alone always had that property. +│ │ # 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 43733c2ce..835c55fc9 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) | @@ -483,6 +485,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 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. The request is built from the stored state *before* the local clear and sent *after* it, so the clear never waits on the network; in the Network tab the POST therefore follows the local teardown rather than preceding it. +- 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 de89799d6..3c3ddc078 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -123,8 +123,26 @@ 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 when 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**, 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 is built from the stored state before the delete and sent after it, so the local delete never waits on the network. That matters because the OAuth store is shared: holding this process's view of it across a five-second request would let another CLI or TUI write a fresh grant that this one then erased. + +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 1da26d7f5..9b5e587d5 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"; @@ -8,6 +8,8 @@ import { } from "@inspector/core/auth/node/storage-node.js"; import { clearStoredAuthForRelogin } from "../src/clear-stored-auth-for-relogin.js"; +const AS_ISSUER = "https://as.example.com"; + describe("clearStoredAuthForRelogin", () => { let dir: string | undefined; let prevPath: string | undefined; @@ -66,6 +68,386 @@ describe("clearStoredAuthForRelogin", () => { expect(blob.servers["not a url"]).toBeUndefined(); }); + // #2144 — RFC 7009. The request is *built* before the delete (from the token, + // the client id and the cached metadata it removes) and *sent* after it, so + // the local delete never waits on the network. + 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: { + // Issuer-bound (SEP-2352), matching the cached metadata below: an + // unkeyed grant records no authorization server and is refused, so + // seeding one would make these cases assert that instead. + "https://example.com/mcp": { + activeIssuer: AS_ISSUER, + byIssuer: { + [AS_ISSUER]: { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }, + }, + }, + serverMetadata: { + issuer: AS_ISSUER, + authorization_endpoint: `${AS_ISSUER}/authorize`, + token_endpoint: `${AS_ISSUER}/token`, + revocation_endpoint: `${AS_ISSUER}/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; + } + + /** Seed both key spellings of one server, each with its own refresh token. */ + function seedBothSpellings( + normalizedToken: string, + rawToken: string, + ): void { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-two-")); + const file = path.join(dir, "oauth.json"); + const metadata = { + issuer: AS_ISSUER, + authorization_endpoint: `${AS_ISSUER}/authorize`, + token_endpoint: `${AS_ISSUER}/token`, + revocation_endpoint: `${AS_ISSUER}/revoke`, + response_types_supported: ["code"], + }; + const entry = (refresh: string) => ({ + activeIssuer: AS_ISSUER, + byIssuer: { + [AS_ISSUER]: { + tokens: { + access_token: `a-${refresh}`, + token_type: "Bearer", + refresh_token: refresh, + }, + }, + }, + serverMetadata: metadata, + }); + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + "https://example.com": entry(rawToken), + "https://example.com/": entry(normalizedToken), + }, + idpSessions: {}, + }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); + } + + it("revokes the stored grant, having snapshotted it before the delete", 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 spellings resolve to the same stored entry, so this is one grant and + // must produce one request. It also proves the walk does not stop at the + // first empty key: the raw spelling holds nothing. + it("sends one request when both key spellings name the same grant", async () => { + seed(); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .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(); + } + }); + + // Both keys are deleted, so both must be revoked: a stale entry under the + // other spelling is still a live grant at the authorization server, and + // dropping it locally without revoking is the exact leak this closes. + // The *reported* outcome is the normalised key's, matching the precedence + // `findStoredServerState` reads with — that is the grant a connect uses. + it("revokes a stale entry under the other spelling too", async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-both-")); + const file = path.join(dir, "oauth.json"); + const metadata = { + issuer: AS_ISSUER, + authorization_endpoint: `${AS_ISSUER}/authorize`, + token_endpoint: `${AS_ISSUER}/token`, + revocation_endpoint: `${AS_ISSUER}/revoke`, + response_types_supported: ["code"], + }; + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + // The raw spelling the transport keyed by, holding a STALE grant. + "https://example.com": { + activeIssuer: AS_ISSUER, + byIssuer: { + [AS_ISSUER]: { + tokens: { + access_token: "stale-a", + token_type: "Bearer", + refresh_token: "stale-r", + }, + }, + }, + serverMetadata: metadata, + }, + // The normalised spelling, holding the CURRENT grant. + "https://example.com/": { + activeIssuer: AS_ISSUER, + byIssuer: { + [AS_ISSUER]: { + tokens: { + access_token: "live-a", + token_type: "Bearer", + refresh_token: "live-r", + }, + }, + }, + serverMetadata: metadata, + }, + }, + idpSessions: {}, + }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); + + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 200 })); + try { + await expect( + clearStoredAuthForRelogin("https://example.com"), + ).resolves.toMatchObject({ status: "revoked" }); + const tokens = fetchSpy.mock.calls.map((call) => + new URLSearchParams(String(call[1]?.body)).get("token"), + ); + // Normalised first, then the stale raw one — neither is left behind. + expect(tokens).toEqual(["live-r", "stale-r"]); + } finally { + fetchSpy.mockRestore(); + } + + const blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + expect(blob.servers["https://example.com"]).toBeUndefined(); + expect(blob.servers["https://example.com/"]).toBeUndefined(); + }); + + it("deletes the local entry even when the request fails", async () => { + const file = seed(); + const fetchSpy = vi + .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(); + } + }); + + // Keys are revoked from independently. Deduplicating them could only be + // done by pre-reading one token, which would skip the second key's OTHER + // issuer-bound grants along with it; a duplicate RFC 7009 request is + // harmless (§2.2 makes an unknown token a success), a missed one is not. + it("revokes from both keys even when they hold the same token", async () => { + seedBothSpellings("same-r", "same-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 500 })); + try { + await expect( + clearStoredAuthForRelogin("https://example.com"), + ).resolves.toMatchObject({ status: "failed" }); + expect(fetchSpy).toHaveBeenCalledTimes(2); + } finally { + fetchSpy.mockRestore(); + } + }); + + // A grant still live at the authorization server is what the user needs to + // hear about; reporting the earlier success would print no warning at all. + // One budget across both keys, so a first request that outlives it fails + // rather than each key getting a fresh five seconds. + it("bounds both keys with one shared budget", async () => { + seedBothSpellings("live-r", "stale-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockImplementation(async () => { + // Outlives the injected budget, so the second key is never attempted. + await new Promise((r) => setTimeout(r, 40)); + return new Response(null, { status: 200 }); + }); + try { + const outcome = await clearStoredAuthForRelogin("https://example.com", { + budgetMs: 20, + }); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome?.status === "failed" ? outcome.detail : "").toContain( + "timed out after 20ms", + ); + } finally { + fetchSpy.mockRestore(); + } + }); + + // A plan reached with no budget left is reported rather than firing a + // request it would immediately abandon — and that report must outrank an + // earlier success, since the unattempted key's grant may still be live. + it("reports exhaustion for a key it never attempted", async () => { + seedBothSpellings("live-r", "stale-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValue(new Response(null, { status: 200 })); + try { + const outcome = await clearStoredAuthForRelogin("https://example.com", { + budgetMs: 0, + }); + expect(fetchSpy).not.toHaveBeenCalled(); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome?.status === "failed" ? outcome.detail : "").toContain( + "budget was exhausted", + ); + } finally { + fetchSpy.mockRestore(); + } + }); + + it("reports a later failure over an earlier success", async () => { + seedBothSpellings("live-r", "stale-r"); + const fetchSpy = vi + .spyOn(globalThis, "fetch") + .mockResolvedValueOnce(new Response(null, { status: 200 })) + .mockResolvedValue(new Response(null, { status: 500 })); + try { + await expect( + clearStoredAuthForRelogin("https://example.com"), + ).resolves.toMatchObject({ status: "failed" }); + } finally { + fetchSpy.mockRestore(); + } + }); + + // `getTokens` parses through `OAuthTokensSchema`, so a persisted token that + // no longer validates rejects. That must not abandon the local delete + // `--relogin` promises — over a grant that could not have been revoked. + it("still clears when the stored token cannot be parsed", async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-bad-token-")); + const file = path.join(dir, "oauth.json"); + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + "https://example.com/mcp": { + activeIssuer: AS_ISSUER, + byIssuer: { + // No `token_type` — fails OAuthTokensSchema. + [AS_ISSUER]: { tokens: { access_token: 42 } }, + }, + }, + }, + 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.toMatchObject({ status: "failed" }); + + const blob = JSON.parse(fs.readFileSync(file, "utf8")) as { + servers: Record; + }; + expect(blob.servers["https://example.com/mcp"]).toBeUndefined(); + }); + + 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/__tests__/relogin-revocation.test.ts b/clients/cli/__tests__/relogin-revocation.test.ts new file mode 100644 index 000000000..eb1698ead --- /dev/null +++ b/clients/cli/__tests__/relogin-revocation.test.ts @@ -0,0 +1,227 @@ +/** + * `--relogin` token revocation, driven through `runCli` (#2144). + * + * The helper's own tests cover `clearStoredAuthForRelogin({ revoke })`. What + * they cannot cover is the wiring above it: that Commander maps `--no-revoke`, + * that the per-server `oauth.revokeOnClear` reaches the call, and that a failed + * revocation is reported without changing what `--relogin` does. A regression + * in any of those turns revocation silently back on or off, which is precisely + * the class of bug the opt-out exists to make controllable. + * + * Every case here fails to connect on purpose — the fetch double refuses + * everything but the revocation endpoint. That is fine: revocation runs before + * the connect, so the assertions are about the requests made and the warnings + * printed, not about the exit code. + */ +import { describe, it, expect, afterEach, vi } from "vitest"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { resetNodeOAuthStorageCache } from "@inspector/core/auth/node/storage-node.js"; +import { runCli } from "./helpers/cli-runner.js"; + +const SERVER_URL = "https://example.com/mcp"; +const AS_ISSUER = "https://as.example.com"; +const REVOKE_URL = `${AS_ISSUER}/revoke`; + +let dir: string | undefined; +let prevPath: string | undefined; + +afterEach(() => { + if (prevPath === undefined) delete process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + else process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = prevPath; + prevPath = undefined; + resetNodeOAuthStorageCache(); + vi.restoreAllMocks(); + if (dir) { + fs.rmSync(dir, { recursive: true, force: true }); + dir = undefined; + } +}); + +/** Seed an OAuth store holding a revocable grant for {@link SERVER_URL}. */ +function seedStore(): void { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "cli-relogin-revoke-e2e-")); + const file = path.join(dir, "oauth.json"); + fs.writeFileSync( + file, + JSON.stringify({ + servers: { + // Issuer-bound (SEP-2352) and matching the cached metadata: an unkeyed + // grant records no authorization server and is deliberately refused. + [SERVER_URL]: { + activeIssuer: AS_ISSUER, + byIssuer: { + [AS_ISSUER]: { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }, + }, + }, + serverMetadata: { + issuer: AS_ISSUER, + authorization_endpoint: `${AS_ISSUER}/authorize`, + token_endpoint: `${AS_ISSUER}/token`, + revocation_endpoint: REVOKE_URL, + response_types_supported: ["code"], + }, + }, + }, + idpSessions: {}, + }), + "utf8", + ); + prevPath = process.env.MCP_INSPECTOR_OAUTH_STATE_PATH; + process.env.MCP_INSPECTOR_OAUTH_STATE_PATH = file; + resetNodeOAuthStorageCache(); +} + +/** + * Answer the revocation endpoint with `status` and refuse everything else, so + * the run stops at the connect instead of reaching the network. + */ +function stubFetch(status = 200): ReturnType { + return vi.spyOn(globalThis, "fetch").mockImplementation((input) => { + if (String(input instanceof Request ? input.url : input) === REVOKE_URL) { + return Promise.resolve(new Response(null, { status })); + } + return Promise.reject(new Error("network blocked in test")); + }) as ReturnType; +} + +function revocationCalls(spy: { mock: { calls: unknown[][] } }): unknown[][] { + return spy.mock.calls.filter( + (call) => String(call[0] as string) === REVOKE_URL, + ); +} + +/** A catalog holding one HTTP server, optionally opting out of revocation. */ +function writeCatalog(revokeOnClear?: boolean): string { + const catalogDir = fs.mkdtempSync( + path.join(os.tmpdir(), "cli-relogin-catalog-"), + ); + const catalogPath = path.join(catalogDir, "mcp.json"); + fs.writeFileSync( + catalogPath, + JSON.stringify({ + mcpServers: { + remote: { + type: "streamable-http", + url: SERVER_URL, + ...(revokeOnClear !== undefined && { oauth: { revokeOnClear } }), + }, + }, + }), + "utf8", + ); + return catalogPath; +} + +describe("--relogin token revocation", () => { + it("revokes the stored grant by default", async () => { + seedStore(); + const fetchSpy = stubFetch(); + + await runCli([ + "--relogin", + "--server-url", + SERVER_URL, + "--method", + "tools/list", + ]); + + expect(revocationCalls(fetchSpy)).toHaveLength(1); + }); + + // Commander turns `--no-revoke` into `options.revoke === false`; nothing else + // proves that mapping survives a refactor of the flag. + it("--no-revoke skips the request", async () => { + seedStore(); + const fetchSpy = stubFetch(); + + await runCli([ + "--relogin", + "--no-revoke", + "--server-url", + SERVER_URL, + "--method", + "tools/list", + ]); + + expect(revocationCalls(fetchSpy)).toHaveLength(0); + }); + + it("honors the per-server oauth.revokeOnClear opt-out", async () => { + seedStore(); + const catalogPath = writeCatalog(false); + const fetchSpy = stubFetch(); + try { + await runCli([ + "--catalog", + catalogPath, + "--server", + "remote", + "--relogin", + "--method", + "tools/list", + ]); + expect(revocationCalls(fetchSpy)).toHaveLength(0); + } finally { + fs.rmSync(path.dirname(catalogPath), { recursive: true, force: true }); + } + }); + + it("revokes for a catalog server that did not opt out", async () => { + seedStore(); + const catalogPath = writeCatalog(); + const fetchSpy = stubFetch(); + try { + await runCli([ + "--catalog", + catalogPath, + "--server", + "remote", + "--relogin", + "--method", + "tools/list", + ]); + expect(revocationCalls(fetchSpy)).toHaveLength(1); + } finally { + fs.rmSync(path.dirname(catalogPath), { recursive: true, force: true }); + } + }); + + // Inert-but-accepted flags are rejected in this parser (see the `--strict` + // rationale in `cli.ts`). On its own `--no-revoke` reads as "this run will + // not revoke anything", which is true only because nothing was being cleared. + it("rejects --no-revoke without --relogin", async () => { + const result = await runCli([ + "--no-revoke", + "--server-url", + SERVER_URL, + "--method", + "tools/list", + ]); + expect(result.stderr).toMatch(/--no-revoke requires --relogin/); + }); + + // A failed revocation must be visible — the grant is still live at the + // authorization server — without turning `--relogin` into a failure, which + // is a local delete the user still gets. + it("warns on stderr when the authorization server refuses", async () => { + seedStore(); + stubFetch(500); + + const result = await runCli([ + "--relogin", + "--server-url", + SERVER_URL, + "--method", + "tools/list", + ]); + + expect(result.stderr).toMatch(/could not revoke the OAuth grant/i); + }); +}); diff --git a/clients/cli/src/clear-stored-auth-for-relogin.ts b/clients/cli/src/clear-stored-auth-for-relogin.ts index bcbdc880c..6ec64e6e8 100644 --- a/clients/cli/src/clear-stored-auth-for-relogin.ts +++ b/clients/cli/src/clear-stored-auth-for-relogin.ts @@ -2,6 +2,14 @@ import { NodeOAuthStorage, resetNodeOAuthStorageCache, } from "@inspector/core/auth/node/storage-node.js"; +import { + DEFAULT_REVOCATION_TIMEOUT_MS, + clearAndPlanRevocation, + executeOAuthRevocation, + type OAuthRevocationPlan, + 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,16 +32,121 @@ function normalizeServerUrl(serverUrl: string): string { */ export async function clearStoredAuthForRelogin( serverUrl: string | undefined, -): Promise { - if (!serverUrl?.trim()) return; + options?: { + revoke?: boolean; + /** + * Total wall-clock budget for the revocation requests across both key + * spellings. Defaults to {@link DEFAULT_REVOCATION_TIMEOUT_MS}; injectable + * so the exhaustion path can be exercised without a five-second test. + */ + budgetMs?: number; + }, +): Promise { + if (!serverUrl?.trim()) return undefined; const raw = serverUrl.trim(); const normalized = normalizeServerUrl(raw); const storage = new NodeOAuthStorage(); - await storage.clear(raw); - if (normalized !== raw) { - await storage.clear(normalized); + // RFC 7009 (#2144), ordered snapshot -> clear -> revoke. Everything the + // requests need is read first, because the clear empties the store; but the + // clear then runs immediately rather than behind the network. Waiting would + // hold this process's in-memory view of a *shared*, file-backed store for up + // to five seconds, and another CLI/TUI writing a fresh grant in that window + // would be erased by the clear that followed. + // + // Both spellings are cleared, so both are planned from — a stale entry under + // the other key is a live grant at the authorization server, and deleting it + // locally without revoking is exactly the leak this closes. The normalised + // key goes first because that is the precedence `findStoredServerState` reads + // with, so the grant actually in use is the one whose outcome is reported. + // They are deliberately not deduplicated; see `sendPlans`. + const keys = normalized === raw ? [raw] : [normalized, raw]; + // Each key's state is taken and deleted in ONE atomic storage step, so the + // clear is never a separate check-then-act over an already-read snapshot. + const plans: OAuthRevocationPlan[] = []; + for (const key of keys) { + plans.push( + await clearAndPlanRevocation({ + serverUrl: key, + storage, + enabled: options?.revoke !== false, + }), + ); } // Drop the in-process singleton so the next connect cannot reuse a cleared // entry from the NodeOAuthStorage cache. resetNodeOAuthStorageCache(); + + return options?.revoke === false + ? undefined + : sendPlans(plans, options?.budgetMs ?? DEFAULT_REVOCATION_TIMEOUT_MS); +} + +/** + * Send each plan's requests, in order, and report the outcome that matters most. + * + * Both key spellings are cleared, so both are planned from: a stale entry under + * the other spelling is still a live grant at the authorization server, and + * deleting it locally without revoking is the leak this whole change closes. + * + * There is deliberately **no** cross-key deduplication. It looked cheap — the + * two keys are usually two spellings of one server — but it can only be done by + * pre-reading a single token, and a plan enumerates every issuer slot under a + * key. So a shared *active* token would have skipped the second key entirely, + * taking any additional issuer-bound grant under it with the local delete. A + * duplicate RFC 7009 request is harmless (§2.2 makes an unknown token a + * success), which is a much better trade than a missed one. + * + * One budget across both plans, for the same reason a plan shares one across + * its grants: two keys would otherwise double the wait. + * + * Reporting prefers a **failure** over any success — a grant still live at the + * authorization server is what the user needs to hear about, and a success + * would otherwise silence the warning. Failing that it is the first plan's + * outcome that was not "nothing to do", which with the keys in + * `findStoredServerState` precedence means the grant the CLI would actually + * have connected with. When no key held a token, the last "nothing to do" + * answer is returned so the caller can still tell "no tokens" from "this + * authorization server advertises no revocation endpoint". + */ +async function sendPlans( + plans: OAuthRevocationPlan[], + budgetMs: number, +): Promise { + const fetchFn = createProxyFetch() ?? fetch; + const deadlineAt = Date.now() + budgetMs; + let reported: TokenRevocationOutcome | undefined; + let lastSkip: TokenRevocationOutcome | undefined; + for (const plan of plans) { + const remainingMs = deadlineAt - Date.now(); + // A plan that already knows its answer needs no network, so the budget is + // irrelevant to it. Synthesising exhaustion here would warn that a grant + // may still be live when the key held no grant at all — a false alarm, and + // one that outranks the real outcome under the failure-first rule below. + const needsNetwork = plan.outcome === undefined; + if (needsNetwork && remainingMs <= 0) { + // Overrides an earlier success rather than deferring to it: this key's + // grant may still be live at the authorization server, and that is the + // thing the user needs to hear about. Same failure-first rule as below. + const exhausted: TokenRevocationOutcome = { + status: "failed", + detail: `the ${budgetMs}ms revocation budget was exhausted before "${plan.serverUrl}" was attempted`, + }; + if (reported?.status !== "failed") reported = exhausted; + continue; + } + const outcome = await executeOAuthRevocation(plan, { + fetchFn, + timeoutMs: needsNetwork ? remainingMs : undefined, + }); + if (outcome.status === "skipped" && outcome.reason === "no_tokens") { + lastSkip = outcome; + continue; + } + if (outcome.status === "failed") { + if (reported?.status !== "failed") reported = outcome; + } else { + reported ??= outcome; + } + } + return reported ?? lastSkip; } diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index af4bed16b..902448356 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", + "Requires --relogin. Skips the RFC 7009 revocation request that would otherwise end the grant at the authorization server when 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; @@ -849,6 +867,15 @@ async function parseArgs(argv?: string[]): Promise { } } + // `--no-revoke` only means anything alongside `--relogin` — it suppresses the + // RFC 7009 request that clear makes. Accepted on its own it is inert, and + // worse than inert: it reads as "this run will not revoke anything", which is + // true only because nothing was going to be cleared. Rejected here, ahead of + // the short-circuit returns, for the same reason `--strict` is (#2144). + if (options.revoke === false && !options.relogin) { + throw new Error("--no-revoke requires --relogin (it has no other effect)."); + } + // `--strict` is checked HERE, ahead of every short-circuit return below // (`--list-stored-auth`, `--print-handoff`, `servers/list`, `servers/show`), // rather than beside the other method-shaped validations further down. Those @@ -1112,6 +1139,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 +1160,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 +1190,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 93930003d..bab524efc 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 e58d8c775..3139174f3 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 () => {}, ), @@ -359,6 +360,26 @@ function oneHttp(): Record { }; } +/** An HTTP server that opted out of RFC 7009 revocation on clear (#2144). */ +function oneHttpNoRevoke(): Record { + return { + web: { + config: { type: "streamable-http", url: "http://x" }, + settings: { + requestTimeout: 0, + metadata: {}, + headers: [], + env: [], + roots: [], + maxFetchRequests: 1000, + taskTtl: 0, + connectionTimeout: 0, + oauthRevokeOnClear: false, + }, + } as never, + }; +} + function oneEmaHttp(): Record { return { ema: { @@ -665,6 +686,12 @@ beforeEach(() => { new URL("https://auth.example/start"), ); h.clientSpies.clearOAuthTokens.mockReset(); + // #2144: the clear now reads the returned revocation outcome, so the reset + // default has to be an outcome rather than `undefined`. + h.clientSpies.clearOAuthTokens.mockResolvedValue({ + status: "skipped", + reason: "no_endpoint", + }); h.clientSpies.completeOAuthFlow.mockReset(); h.clientSpies.completeOAuthFlow.mockResolvedValue(undefined); h.clientSpies.getOAuthState.mockReset(); @@ -1350,6 +1377,116 @@ describe("App (mid-session auth lifecycle events)", () => { expect(h.disconnect).toHaveBeenCalled(); }); + // #2144 — revocation is on unless the entry opted out, and the opt-out has to + // reach the client, since that is where the RFC 7009 request is made. + it("asks for revocation by default when clearing", async () => { + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0); + expect(h.clientSpies.clearOAuthTokens).toHaveBeenCalledWith({ + revoke: true, + }); + }); + + it("forwards the per-server revocation opt-out", async () => { + const r = await mount(oneHttpNoRevoke()); + await press(r, ["a", "s"]); + await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0); + expect(h.clientSpies.clearOAuthTokens).toHaveBeenCalledWith({ + revoke: false, + }); + }); + + // A failed revocation leaves the grant live at the authorization server, so + // it has to be visible rather than swallowed — the local clear succeeded and + // would otherwise look like the whole operation did. + it("reports a failed revocation without failing the clear", async () => { + h.clientSpies.clearOAuthTokens.mockResolvedValue({ + status: "failed", + detail: "unreachable", + }); + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + // The detail is what identifies the failure; the tone the message renders + // in is asserted against `AuthTab` directly, where the branch lives. + await expectFrame(r, "unreachable"); + }); + + // The wiring is the contract here, not just `AuthTab`'s own behavior: a + // `void`-ing arrow between them resolves instantly, which makes the pending + // state, the repeat lock and the rejection path all inert while revocation + // is still running. Driven through the real App so the arrow cannot come + // back (#2144). + it("holds the pending state until the clear actually settles", async () => { + let settle: () => void = () => {}; + h.clientSpies.clearOAuthTokens.mockImplementation( + () => + new Promise((resolve) => { + settle = () => + resolve({ status: "skipped", reason: "no_endpoint" as const }); + }), + ); + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0); + await tick(); + + // Still running: the pending line is up and the confirmation is not. + expect(r.lastFrame() ?? "").toContain("Clearing OAuth state"); + expect(r.lastFrame() ?? "").not.toContain("OAuth state cleared"); + + // And the repeat lock is live, so a second press does not start another. + await press(r, ["s"]); + expect(h.clientSpies.clearOAuthTokens).toHaveBeenCalledTimes(1); + + settle(); + await waitUntil(() => + (r.lastFrame() ?? "").includes("OAuth state cleared"), + ); + }); + + // The clear has already succeeded when the disconnect runs, so its failure + // must not propagate: `AuthTab` would report "Could not clear OAuth state", + // which is false. It goes to the disconnect error line, where the same + // failure from the `d` key already lands. + it("reports a post-clear disconnect failure as a disconnect failure", async () => { + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValue(new Error("disconnect-blew-up")); + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + + await expectFrame(r, "disconnect-blew-up"); + expect(r.lastFrame() ?? "").not.toContain("Could not clear OAuth state"); + }); + + // A transport can reject with something that is not an `Error`; the message + // line must still be readable rather than "[object Object]". + it("reports a non-Error disconnect failure from a clear", async () => { + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValue("plain-disconnect-string"); + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await expectFrame(r, "plain-disconnect-string"); + }); + + // A clear while still CONNECTING tears the attempt down too — the session is + // half-built, and leaving it up with its OAuth state gone is worse than not + // having it. + it("disconnects a connecting session when clearing", async () => { + h.ctrl.status = "connecting"; + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await waitUntil(() => h.disconnect.mock.calls.length > 0); + expect(h.disconnect).toHaveBeenCalled(); + }); + + it("says nothing when there was nothing to revoke", async () => { + const r = await mount(oneHttp()); + await press(r, ["a", "s"]); + await waitUntil(() => h.clientSpies.clearOAuthTokens.mock.calls.length > 0); + expect(r.lastFrame() ?? "").not.toContain("authorization server failed"); + }); + const stepUpChallenge = { reason: "insufficient_scope" as const, requiredScopes: ["env:read"], diff --git a/clients/tui/__tests__/AuthTab.test.tsx b/clients/tui/__tests__/AuthTab.test.tsx index 86d6b18eb..213416c54 100644 --- a/clients/tui/__tests__/AuthTab.test.tsx +++ b/clients/tui/__tests__/AuthTab.test.tsx @@ -100,6 +100,235 @@ describe("AuthTab", () => { ); }); + // #2144 — the clear is now a bounded network request, so announcing "cleared" + // on the keypress would say it while the work was still in flight. + it("shows a pending state until the clear settles, and ignores repeats", async () => { + let settle: () => void = () => {}; + const onClearOAuth = vi.fn( + () => + new Promise((resolve) => { + settle = resolve; + }), + ); + const { lastFrame, stdin } = render( + , + ); + await tick(); + + stdin.write("s"); + await tick(); + expect(lastFrame() ?? "").toContain("Clearing OAuth state"); + expect(lastFrame() ?? "").not.toContain("OAuth state cleared"); + + // A second press while one is in flight would race the first over the same + // store entry, and nothing on screen tells the user the first is running. + stdin.write("s"); + await tick(); + expect(onClearOAuth).toHaveBeenCalledTimes(1); + + settle(); + await tick(); + expect(lastFrame() ?? "").toContain("OAuth state cleared"); + }); + + // A state-based guard is not effective until React re-renders, so two `s` + // events in the SAME input turn both read the last-rendered "idle" and start + // concurrent clears against one store entry. No tick between the writes. + it("ignores a repeat delivered in the same input turn", async () => { + const onClearOAuth = vi.fn(() => new Promise(() => {})); + const { stdin } = render( + , + ); + await tick(); + + stdin.write("s"); + stdin.write("s"); + await tick(); + + expect(onClearOAuth).toHaveBeenCalledTimes(1); + }); + + // A rejection releases the lock, so the user can retry. + it("allows a retry after a rejected clear", async () => { + const onClearOAuth = vi + .fn<() => Promise>() + .mockRejectedValueOnce(new Error("nope")) + .mockResolvedValue(undefined); + const { stdin } = render( + , + ); + await tick(); + stdin.write("s"); + await tick(); + stdin.write("s"); + await tick(); + + expect(onClearOAuth).toHaveBeenCalledTimes(2); + }); + + // A stale completion owns nothing: server A settling after the user moved to + // B and started a clear there must not drop B's lock, or a second B clear + // could run concurrently against the same store entry. + it("a stale completion does not release the current server's lock", async () => { + const settlers: Array<() => void> = []; + const onClearOAuth = vi.fn( + () => + new Promise((resolve) => { + settlers.push(resolve); + }), + ); + const props = (serverName: string) => ({ + ...baseProps, + serverName, + onClearOAuth, + inspectorClient: null, + oauthStatus: "idle" as const, + oauthMessage: null, + focused: true, + }); + const { stdin, rerender } = render(); + await tick(); + + stdin.write("s"); // A's clear starts + await tick(); + rerender(); + await tick(); + stdin.write("s"); // B's clear starts + await tick(); + expect(onClearOAuth).toHaveBeenCalledTimes(2); + + // A settles late. B's clear is still running, so its lock must hold. + settlers[0]!(); + await tick(); + stdin.write("s"); + await tick(); + + expect(onClearOAuth).toHaveBeenCalledTimes(2); + }); + + // A rejection is NOT a revocation failure — those come back as outcomes and + // are reported through the message line. This is the local clear or the + // disconnect itself failing, so reporting success would be a plain lie. + it("reports a rejected clear as a failure, not as success", async () => { + const onClearOAuth = vi.fn(() => + Promise.reject(new Error("keychain locked")), + ); + const { lastFrame, stdin } = render( + , + ); + await tick(); + stdin.write("s"); + await tick(); + const frame = lastFrame() ?? ""; + expect(frame).toContain("Could not clear OAuth state"); + expect(frame).toContain("keychain locked"); + expect(frame).not.toContain("OAuth state cleared."); + expect(frame).not.toContain("Clearing OAuth state"); + }); + + // A clear started on server A must not confirm under server B: it is a + // bounded network request now, so it can settle after the user has moved on. + it("does not confirm a clear that settles after the server changed", async () => { + let settle: () => void = () => {}; + const onClearOAuth = vi.fn( + () => + new Promise((resolve) => { + settle = resolve; + }), + ); + const { lastFrame, stdin, rerender } = render( + , + ); + await tick(); + stdin.write("s"); + await tick(); + expect(lastFrame() ?? "").toContain("Clearing OAuth state"); + + rerender( + , + ); + await tick(); + settle(); + await tick(); + + const frame = lastFrame() ?? ""; + expect(frame).not.toContain("OAuth state cleared"); + expect(frame).not.toContain("Clearing OAuth state"); + }); + + // #2144 — a revocation failure is a *partial* success: the local state really + // was cleared, so it is not an `error` status, but the grant may still be + // live at the authorization server and the informational tone understates + // that. Both tones are exercised so neither branch can rot. + it("renders an idle message in the warning tone when asked", () => { + const { lastFrame } = render( + , + ); + expect(lastFrame() ?? "").toContain("Cleared locally"); + }); + + it("renders an idle message in the default informational tone", () => { + const { lastFrame } = render( + , + ); + expect(lastFrame() ?? "").toContain("Stored OAuth state cleared."); + }); + it("renders OAuth details from getOAuthState", async () => { const { client } = makeClient(sampleOAuthState); const { lastFrame } = render( diff --git a/clients/tui/__tests__/oauthMessageTone.test.ts b/clients/tui/__tests__/oauthMessageTone.test.ts new file mode 100644 index 000000000..7d00c0597 --- /dev/null +++ b/clients/tui/__tests__/oauthMessageTone.test.ts @@ -0,0 +1,28 @@ +import { describe, it, expect } from "vitest"; +import { oauthMessageToneFor } from "../src/oauthMessageTone.js"; + +const WARNING = + "Cleared locally, but revoking the grant at the authorization server failed."; + +describe("oauthMessageToneFor", () => { + it("warns while the message it was raised for is showing", () => { + expect(oauthMessageToneFor(WARNING, WARNING)).toBe("warning"); + }); + + // The bug this replaced (#2144): a stored tone survived the message that + // earned it, so every later ordinary note rendered as a warning. + it("returns to informational as soon as any other message replaces it", () => { + expect(oauthMessageToneFor("Authorization updated.", WARNING)).toBe("info"); + }); + + it("is informational when nothing has ever warned", () => { + expect(oauthMessageToneFor("Authorization updated.", null)).toBe("info"); + }); + + // `null === null` must not read as a warning — with no message on screen + // there is nothing to colour, and the previous warning is long gone. + it("is informational when there is no message", () => { + expect(oauthMessageToneFor(null, null)).toBe("info"); + expect(oauthMessageToneFor(null, WARNING)).toBe("info"); + }); +}); diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index d8a8aee0b..5b7d35f16 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -65,6 +65,7 @@ import { NodeOAuthStorage, runRunnerInteractiveOAuth, } from "@inspector/core/auth/node/index.js"; +import { oauthMessageToneFor } from "./oauthMessageTone.js"; import { getTuiLogger } from "./logger.js"; import { openUrl } from "./utils/openUrl.js"; import { @@ -161,6 +162,13 @@ function App({ "idle" | "authenticating" | "error" >("idle"); const [oauthMessage, setOauthMessage] = useState(null); + // The tone is derived from the message rather than stored beside it, so it + // cannot go stale — see `oauthMessageToneFor` for why that matters here. + // Both stay plain `useState` setters on purpose: wrapping `setOauthMessage` + // in a `useCallback` would make it a value `react-hooks/exhaustive-deps` + // demands in seven dependency arrays, for an identity that never changes. + const [oauthWarningText, setOauthWarningText] = useState(null); + const oauthMessageTone = oauthMessageToneFor(oauthMessage, oauthWarningText); const [oauthRevision, setOauthRevision] = useState(0); const [pendingStepUp, setPendingStepUp] = useState<{ serverName: string; @@ -175,6 +183,8 @@ function App({ const [connectError, setConnectError] = useState(null); // Monotonic token for in-flight disconnects — see handleDisconnect. const disconnectAttemptRef = useRef(0); + /** Retires a superseded "clear OAuth state" the way disconnects are (#2144). */ + const clearOAuthAttemptRef = useRef(0); // A failed disconnect is deliberately NOT folded into `connectError`. That // one is only rendered by InfoTab when the status is "error", which a // rejected disconnect leaves untouched (the status stays "connected"), so @@ -454,6 +464,11 @@ function App({ // server name matching again, so the stale rejection would land on the // re-selected A. Retiring the token on every switch is what closes that. disconnectAttemptRef.current++; + // Same reasoning for the clear (#2144): the A → B → A round trip would + // otherwise leave both its attempt token and the captured server name + // matching again, so a clear started on the first A could publish into the + // re-selected one. + clearOAuthAttemptRef.current++; setDisconnectError(null); const stepUp = pendingStepUpRef.current; if (stepUp && selectedServer && stepUp.serverName !== selectedServer) { @@ -781,6 +796,11 @@ function App({ // A connect attempt supersedes whatever the last disconnect reported — // including one still in flight, which is what the counter bump retires. disconnectAttemptRef.current++; + // And whatever a pending clear was going to do (#2144). The server-name + // check alone does not cover disconnect/reconnect to the SAME server: the + // name still matches on the other side of it, so a clear still in flight + // would tear down the session this connect just established. + clearOAuthAttemptRef.current++; setDisconnectError(null); const finishConnect = async () => { @@ -941,15 +961,63 @@ function App({ const handleClearOAuth = useCallback(async () => { if (!selectedInspectorClient) return; - await selectedInspectorClient.clearOAuthTokens(); + // Claim this attempt before awaiting, exactly as `handleDisconnect` does. + // The RFC 7009 leg is a bounded network request, so this callback can now + // stay suspended for seconds — long enough for the user to switch servers, + // at which point publishing this result would put server A's outcome on + // server B (and would race a second clear over the same state). (#2144) + const attempt = ++clearOAuthAttemptRef.current; + const attemptServer = selectedServer; + // RFC 7009. 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, + }); + // The local clear has happened either way — only the *reporting* is + // retired, so a superseded attempt leaves no trace on the new selection. + if ( + clearOAuthAttemptRef.current !== attempt || + selectedServerRef.current !== attemptServer + ) { + return; + } setOauthStatus("idle"); - setOauthMessage(null); + if (revocation.status === "failed") { + const warning = `Cleared locally, but revoking the grant at the authorization server failed: ${revocation.detail}. It may still be valid there.`; + setOauthWarningText(warning); + setOauthMessage(warning); + } else { + setOauthMessage(null); + } setConnectError(null); if (inspectorStatus === "connected" || inspectorStatus === "connecting") { - await disconnectInspector(); + // The clear has already succeeded by here, so a disconnect failure must + // not propagate as one: `AuthTab` would report "Could not clear OAuth + // state", which is false. It goes to the disconnect error line instead, + // where the same failure from the `d` key already lands. + try { + await disconnectInspector(); + } catch (err) { + setDisconnectError(err instanceof Error ? err.message : String(err)); + } + // Revalidate: the disconnect is a second await, and a switch during it + // would make the revision bump below land on the new selection. + if ( + clearOAuthAttemptRef.current !== attempt || + selectedServerRef.current !== attemptServer + ) { + return; + } } setOauthRevision((n) => n + 1); - }, [selectedInspectorClient, inspectorStatus, disconnectInspector]); + }, [ + selectedInspectorClient, + selectedServer, + selectedServerEntry, + inspectorStatus, + disconnectInspector, + ]); // Build current server state from InspectorClient data (tools from ManagedToolsState) const currentServerState = useMemo(() => { @@ -1741,6 +1809,7 @@ function App({ inspectorClient={selectedInspectorClient} oauthStatus={oauthStatus} oauthMessage={oauthMessage} + oauthMessageTone={oauthMessageTone} oauthRevision={oauthRevision} pendingStepUp={ pendingStepUp?.serverName === selectedServer @@ -1843,9 +1912,12 @@ function App({ focused={ focus === "tabContentList" || focus === "tabContentDetails" } - onClearOAuth={() => { - void handleClearOAuth(); - }} + // Passed directly, NOT wrapped in a `void`-ing arrow: AuthTab + // awaits this to hold its pending state, keep its repeat lock, + // and route a rejection to the failure line. Dropping the + // promise here would resolve it instantly and make all three + // inert while revocation was still running (#2144). + onClearOAuth={handleClearOAuth} connectionStatus={inspectorStatus} /> ) : null} diff --git a/clients/tui/src/components/AuthTab.tsx b/clients/tui/src/components/AuthTab.tsx index c495a4e53..d2160135c 100644 --- a/clients/tui/src/components/AuthTab.tsx +++ b/clients/tui/src/components/AuthTab.tsx @@ -28,6 +28,14 @@ interface AuthTabProps { inspectorClient: InspectorClient | null; oauthStatus: "idle" | "authenticating" | "error"; oauthMessage: string | null; + /** + * How to colour {@link oauthMessage} on the `idle` status. Defaults to + * `"info"` (cyan). `"warning"` exists for the one message that reports a + * *partial* success: the OAuth state really was cleared, so this is not an + * error, but the grant may still be live at the authorization server and + * cyan would understate that (#2144). + */ + oauthMessageTone?: "info" | "warning"; oauthRevision: number; pendingStepUp?: { challenge: AuthChallenge; @@ -39,7 +47,15 @@ interface AuthTabProps { width: number; height: number; focused?: boolean; - onClearOAuth: () => void; + /** + * Clears (and, unless opted out, revokes) this server's OAuth state. + * + * Returns a promise so the confirmation below can wait for it. The RFC 7009 + * revocation leg is a network request with a five-second bound (#2144), so + * this is no longer instantaneous, and announcing "cleared" on the keypress + * would say it while the work was still in flight. + */ + onClearOAuth: () => void | Promise; connectionStatus: ConnectionStatus; } @@ -57,6 +73,7 @@ export function AuthTab({ inspectorClient, oauthStatus, oauthMessage, + oauthMessageTone = "info", oauthRevision, pendingStepUp, onAuthorizeStepUp, @@ -73,7 +90,47 @@ export function AuthTab({ const [oauthState, setOauthState] = useState< OAuthConnectionState | undefined >(undefined); - const [clearedConfirmation, setClearedConfirmation] = useState(false); + const [clearState, setClearState] = useState< + "idle" | "clearing" | "cleared" | "failed" + >("idle"); + const [clearFailure, setClearFailure] = useState(null); + /** + * The server a clear was started for, and a sequence number. The clear is a + * bounded network request now, so it can settle after the user has moved on + * — at which point server A's confirmation must not appear under server B. + */ + const clearAttemptRef = useRef(0); + /** + * In-flight lock. A ref, not the `clearState` above: two `s` keypresses + * delivered in the same input turn both read the state React last rendered, + * so a state-based guard lets both start and race each other over the same + * store entry. A ref is set before the call and seen by the second read. + */ + const clearInFlightRef = useRef(false); + /** + * Set synchronously when a clear starts, and consumed by the reset effect + * below. The clear itself bumps `oauthRevision` on its way out, so without + * this the reset would race the confirmation it is meant to outlive — and + * which of the two lands first is not something the ordering guarantees. + */ + const ownClearRef = useRef(false); + const serverNameRef = useRef(serverName); + useEffect(() => { + serverNameRef.current = serverName; + }, [serverName]); + // A new selection retires any in-flight clear and drops the previous one's + // banner: neither belongs to the server now on screen. + useEffect(() => { + clearAttemptRef.current++; + clearInFlightRef.current = false; + // The retired clear returns before its `oauthRevision` bump, so the marker + // has no bump to skip. Left set, it would swallow the first unrelated + // revision on the server just selected and strand that server's banner. + ownClearRef.current = false; + setClearState("idle"); + setClearFailure(null); + setLastClearDisconnected(false); + }, [serverName]); const [lastClearDisconnected, setLastClearDisconnected] = useState(false); const [stepUpChoiceIndex, setStepUpChoiceIndex] = useState(0); @@ -91,7 +148,13 @@ export function AuthTab({ }, [refreshOAuthState, oauthRevision, connectionStatus]); useEffect(() => { - setClearedConfirmation(false); + // Skip exactly the revision bump our own clear caused; reset on the next. + if (ownClearRef.current) { + ownClearRef.current = false; + return; + } + setClearState("idle"); + setClearFailure(null); setLastClearDisconnected(false); }, [oauthRevision]); @@ -154,9 +217,49 @@ export function AuthTab({ const h = scrollViewRef.current.getViewportHeight() || 1; scrollViewRef.current.scrollBy(h); } else if (input.toLowerCase() === "s") { + // Ignore repeats while one is in flight: the second would race the + // first over the same store entry, and the user cannot see that the + // first is still running except by the pending line below. + if (clearInFlightRef.current) return; + clearInFlightRef.current = true; + const attempt = ++clearAttemptRef.current; + const attemptServer = serverName; setLastClearDisconnected(isLiveConnection); - onClearOAuth(); - setClearedConfirmation(true); + setClearFailure(null); + ownClearRef.current = true; + setClearState("clearing"); + // A completion is only ours if nothing has superseded it and the + // selection has not moved on. + const current = () => + clearAttemptRef.current === attempt && + serverNameRef.current === attemptServer; + void Promise.resolve(onClearOAuth()).then( + () => { + // `current()` FIRST, before touching either shared ref. A stale + // completion — server A settling after the user moved to B and + // started a clear there — would otherwise drop B's lock and let a + // second B clear run concurrently. A stale clear owns nothing. + if (!current()) return; + clearInFlightRef.current = false; + setClearState("cleared"); + }, + (err: unknown) => { + if (!current()) return; + clearInFlightRef.current = false; + // A rejection is NOT a revocation failure — those come back as + // outcomes and are reported through the message line. This is the + // local clear or the disconnect itself failing, so announcing + // "OAuth state cleared" here would be a plain lie. + // + // It also means `handleClearOAuth` never reached its + // `oauthRevision` bump, so the marker below has no bump to skip. + // Left set, it would swallow the next *unrelated* revision change + // and strand this banner after the OAuth state moved on. + ownClearRef.current = false; + setClearFailure(err instanceof Error ? err.message : String(err)); + setClearState("failed"); + }, + ); } }, { isActive: focused }, @@ -190,7 +293,9 @@ export function AuthTab({ {oauthMessage} )} {oauthStatus === "idle" && oauthMessage && ( - {oauthMessage} + + {oauthMessage} + )} {pendingStepUp ? ( @@ -320,13 +425,22 @@ export function AuthTab({ Clear OAuth State {isLiveConnection && " and disconnect"} - {clearedConfirmation && ( + {clearState === "clearing" && ( + Clearing OAuth state… + )} + {clearState === "cleared" && ( {lastClearDisconnected ? "OAuth state cleared. Disconnected." : "OAuth state cleared."} )} + {clearState === "failed" && ( + + Could not clear OAuth state + {clearFailure ? `: ${clearFailure}` : "."} + + )} diff --git a/clients/tui/src/oauthMessageTone.ts b/clients/tui/src/oauthMessageTone.ts new file mode 100644 index 000000000..447cbf3e2 --- /dev/null +++ b/clients/tui/src/oauthMessageTone.ts @@ -0,0 +1,29 @@ +/** + * How the Auth tab should colour the OAuth note it is currently showing. + * + * The tone is **derived** from the message rather than stored beside it + * (#2144). `App.tsx` sets an ordinary OAuth note in around thirty places, none + * of which would think to reset a tone, so a second piece of state went stale + * the moment one message raised it: a single revocation failure left every + * later "Authorization updated" rendering as a warning, including after + * switching servers. + * + * Comparing the shown message against the text that was raised as a warning + * makes the tone a function of what is on screen, so it cannot outlive it — + * the next `setOauthMessage` of anything else is informational by + * construction, with nothing to remember to clear. + * + * `warning` is for a *partial* success: the OAuth state really was cleared, so + * it is not an error status, but the grant may still be live at the + * authorization server and the informational tone would understate that. + */ +export type OAuthMessageTone = "info" | "warning"; + +export function oauthMessageToneFor( + message: string | null, + warningText: string | null, +): OAuthMessageTone { + // `null === null` must not read as a warning: with no message there is + // nothing to colour. + return message !== null && message === warningText ? "warning" : "info"; +} diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 2b7f0a9bf..82937f283 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -136,7 +136,10 @@ vi.mock("@inspector/core/mcp/index.js", async (importOriginal) => { } return Promise.resolve(true); }); - clearOAuthTokens = vi.fn().mockResolvedValue(undefined); + // #2144: the clear path reads the returned RFC 7009 outcome. + clearOAuthTokens = vi + .fn() + .mockResolvedValue({ status: "skipped", reason: "no_endpoint" }); } const instances: FakeInspectorClient[] = []; return { diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index b1bc9f286..16f38cd43 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -106,6 +106,7 @@ import { type PendingClientRequestContent, } from "./components/groups/PendingClientRequestModal/PendingClientRequestModal"; import { downloadJsonFile } from "./lib/downloadFile"; +import { serverWithDraftSettings } from "./utils/serverWithDraftSettings"; import { enrichProtocolEntries } from "./utils/correlateTransportErrors"; import { visibleMalformedListItems } from "./utils/malformedListReport"; import { parseDeepLink, deepLinkParseStatus } from "./utils/deepLink"; @@ -1348,15 +1349,63 @@ function App() { // target isn't resolvable. const settingsModalIsStdio = settingsModalServerType === "stdio"; + /** + * Servers whose clear is in flight (#2144). Keyed by id, not a single flag: + * the callback explicitly supports clearing a server other than the active + * one, so a global lock would silently drop B's click while A's revocation + * was still out. See `runClear`. + */ + const clearOAuthInFlightRef = useRef>(new Set()); + + /** + * Run a clear from a key/click handler, which cannot await. + * + * `clearServerOAuthAndDisconnect` does **not** own its failures — the store + * write or the disconnect can reject — so a bare `void` would produce an + * unhandled rejection and leave the user with a control that silently did + * nothing. (An RFC 7009 failure is not this: those come back as an outcome + * and are already reported in the success toast.) + */ + const runClear = useCallback( + (server: Parameters[0]) => { + // Shared by BOTH clear controls (Connection Info and Server Settings), + // because for one server they drive the same client and the same store + // entry. A ref rather than state: a double click delivers both events + // before React re-renders, so a state-based guard would let both through + // — and with revocation taking up to five seconds, that means concurrent + // RFC 7009 requests, concurrent store writes, and two contradictory + // toasts. Keyed by server so a *different* server's clear is unaffected. + if (clearOAuthInFlightRef.current.has(server.id)) return; + clearOAuthInFlightRef.current.add(server.id); + clearServerOAuthAndDisconnect(server) + .finally(() => { + clearOAuthInFlightRef.current.delete(server.id); + }) + .catch((err: unknown) => { + notifications.show({ + title: "Could not clear the stored OAuth state", + message: err instanceof Error ? err.message : String(err), + color: "red", + // The tokens may still be on disk and the session may still be up, + // so this is not a notice to let time out. + autoClose: false, + }); + }); + }, + [clearServerOAuthAndDisconnect], + ); + const handleClearConnectionOAuth = useCallback(() => { if (!activeServer) return; - void clearServerOAuthAndDisconnect(activeServer); - }, [activeServer, clearServerOAuthAndDisconnect]); + runClear(activeServer); + }, [activeServer, runClear]); const handleClearStoredOAuthFromSettings = useCallback(() => { if (!settingsModalTarget) return; - void clearServerOAuthAndDisconnect(settingsModalTarget); - }, [settingsModalTarget, clearServerOAuthAndDisconnect]); + // Clear from *inside* the settings modal, so the draft is what the user is + // looking at rather than what the debounced save has persisted (#2144). + runClear(serverWithDraftSettings(settingsModalTarget, settingsDraft)); + }, [settingsModalTarget, settingsDraft, runClear]); const onSettingsModalClose = useCallback(() => { flushSettingsDraft(); diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.test.tsx index 322cb89b4..081b5cc25 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 fec3f25ab..0ac753f99 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 d9933631d..0d704e424 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 ef6394ab0..b8a422c0e 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 934b73e65..bd635b1f7 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 08b1c9bd2..9eeed3c39 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")); @@ -1305,6 +1342,117 @@ describe("useOAuthRecovery", () => { ).toBeDefined(); }); + // #2144 — this is the web client's production wiring for revocation. + // Without asserting the arguments, removing the per-server opt-out or + // handing it the page-origin fetch would leave every test green. + it("passes the per-server revoke setting and the proxied fetch", async () => { + const h = harness({ servers: [entry("a")], activeServerId: "a" }); + await act(async () => { + await h.api().clearServerOAuthAndDisconnect({ + ...entry("a"), + settings: { ...EMPTY_SETTINGS, oauthRevokeOnClear: false }, + }); + }); + expect(clearServerOAuthStateMock).toHaveBeenCalledWith( + expect.objectContaining({ revoke: false, fetchFn: remoteFetchMock }), + ); + }); + + it("defaults to revoking when the server did not opt out", async () => { + const h = harness({ servers: [entry("a")], activeServerId: "a" }); + await act(async () => { + await h.api().clearServerOAuthAndDisconnect(entry("a")); + }); + expect(clearServerOAuthStateMock).toHaveBeenCalledWith( + expect.objectContaining({ revoke: true }), + ); + }); + + // #2144 — the RFC 7009 leg is a bounded network request, so this callback + // can stay suspended for seconds. `isActive`/`inspectorClient` are captured + // before it, so without revalidating, a switch during the wait would + // disconnect the session the user just moved to and run the session-wide + // cleanup against it. + it("does not disconnect a session switched to while a clear is in flight", async () => { + let settle: (r: { cleared: boolean }) => void = () => {}; + clearServerOAuthStateMock.mockImplementation( + () => + new Promise((resolve) => { + settle = resolve as typeof settle; + }), + ); + const client = fakeClient(); + const h = harness({ + servers: [entry("a"), entry("b")], + activeServerId: "a", + client, + }); + + let done: Promise; + await act(async () => { + done = h.api().clearServerOAuthAndDisconnect(entry("a")); + await Promise.resolve(); + }); + + // The user switches away while the clear is still running. + h.rerender({ + servers: [entry("a"), entry("b")], + activeServerId: "b", + client, + }); + + await act(async () => { + settle({ cleared: true }); + await done; + }); + + expect(client.disconnect).not.toHaveBeenCalled(); + }); + + // A disconnect/reconnect to the SAME server builds a replacement client, so + // an id-only check passes again and the old clear would run its + // session-wide cleanup — including the disconnect — against the new session. + it("does not disconnect a replacement client for the same server", async () => { + let settle: (r: { cleared: boolean }) => void = () => {}; + clearServerOAuthStateMock.mockImplementation( + () => + new Promise((resolve) => { + settle = resolve as typeof settle; + }), + ); + const original = fakeClient(); + const h = harness({ + servers: [entry("a")], + activeServerId: "a", + client: original, + }); + + let done: Promise; + await act(async () => { + done = h.api().clearServerOAuthAndDisconnect(entry("a")); + await Promise.resolve(); + }); + + // Same server, new client — a reconnect while the clear is pending. + const replacement = fakeClient(); + h.rerender({ + servers: [entry("a")], + activeServerId: "a", + client: replacement, + }); + + await act(async () => { + settle({ cleared: true }); + await done; + }); + + // Neither client is torn down: the replacement is the live session and + // must not be touched, and the original is already gone — disconnecting + // it would only drag `finalizeExplicitDisconnect` across the new one. + expect(original.disconnect).not.toHaveBeenCalled(); + expect(replacement.disconnect).not.toHaveBeenCalled(); + }); + it("clears the resume snapshot on an explicit disconnect", () => { writeOAuthResumeSnapshot({ version: 1, diff --git a/clients/web/src/hooks/useOAuthRecovery.ts b/clients/web/src/hooks/useOAuthRecovery.ts index 7468799e3..df532f0ec 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,36 +1254,78 @@ export function useOAuthRecovery({ const clearServerOAuthAndDisconnect = useCallback( async (server: ClearableServer) => { const isActive = server.id === activeServerId; - const cleared = await clearServerOAuthState({ + const client = isActive ? inspectorClient : null; + // The RFC 7009 leg is a bounded network request (#2144), so this callback + // can stay suspended for seconds — long enough for the user to close the + // modal and switch servers. `isActive` and `inspectorClient` were + // snapshotted before it, so everything below would otherwise apply to + // whatever session is active *now*: disconnecting a client the user just + // switched to, and running the session-wide UI cleanup against it. + // + // The session ref is the live answer. Only the parts that touch the + // *session* are gated on it; the store write already happened and the + // toast still belongs to the server the user asked about. + // The client identity is part of the check, not just the server id: a + // disconnect/reconnect to the SAME server builds a replacement + // `InspectorClient`, so an id-only check passes again and the old clear + // would run its session-wide cleanup against the new session. + const stillTargetsActiveSession = (): boolean => + isActive && + sessionRef.current.activeServerId === server.id && + sessionRef.current.inspectorClient === client; + + const { cleared, revocation } = await clearServerOAuthState({ config: server.config, - inspectorClient: isActive ? inspectorClient : null, + inspectorClient: client, isActiveConnection: isActive, oauthStorage: webOAuthStorage, + revoke: server.settings?.oauthRevokeOnClear !== false, + fetchFn: getWebProxiedFetch(getAuthToken()), }); if (!cleared) return; - if (isActive && inspectorClient) { + if (client && stillTargetsActiveSession()) { try { - await inspectorClient.disconnect(); + await client.disconnect(); + } catch (err) { + // The clear has already succeeded, so this must not propagate: the + // caller's catch would report "Could not clear the stored OAuth + // state", which is false. Reported as what it is, and the + // cleared-successfully toast below still goes out. + notifications.show({ + title: "Cleared, but the session did not disconnect cleanly", + message: err instanceof Error ? err.message : String(err), + color: "yellow", + }); } finally { - setConnectionInfoOAuthWhenConnected(undefined); - finalizeExplicitDisconnect(); + // Revalidate after the second await for the same reason. + if (stillTargetsActiveSession()) { + setConnectionInfoOAuthWhenConnected(undefined); + finalizeExplicitDisconnect(); + } } - } else { + } else if (!isActive || stillTargetsActiveSession()) { + // No client to disconnect — either this is a stored-only clear, or the + // active session has none yet (it is being built or torn down). Either + // way the resume snapshot is stale and must go; skipping it would leave + // OAuth recovery state pointing at credentials that no longer exist. + // Guarded so a clear whose session has moved on still publishes + // nothing. clearOAuthResumeOnExplicitDisconnect(); } 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", }); }, [ activeServerId, inspectorClient, + sessionRef, webOAuthStorage, finalizeExplicitDisconnect, clearOAuthResumeOnExplicitDisconnect, diff --git a/clients/web/src/lib/clearServerOAuthState.test.ts b/clients/web/src/lib/clearServerOAuthState.test.ts index 137bc9c31..58e818b97 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,117 @@ 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 snapshot + // is taken before the clear (after it there is no token, client id or cached + // metadata to build a request from), but the clear itself runs before the + // network — so by the time the request goes out the store is already empty. + it("clears before sending, using the snapshot it took first", async () => { + await storage.saveTokens( + SERVER_URL, + { + access_token: "tok", + token_type: "Bearer", + refresh_token: "refresh-tok", + }, + // Issuer-bound and matching the metadata below: an unkeyed grant records + // no authorization server and is deliberately refused. + { issuer: "https://as.example.com" }, + ); + 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 = "unset"; + 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", + }); + // The store was already empty when the request went out — that is the + // ordering, and it is what stops a concurrently-written grant being wiped. + expect(tokensAtRequestTime).toBeUndefined(); + expect(await storage.getTokens(SERVER_URL)).toBeUndefined(); + }); + + // 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 f7eb5233f..389bcbad4 100644 --- a/clients/web/src/lib/clearServerOAuthState.ts +++ b/clients/web/src/lib/clearServerOAuthState.ts @@ -1,3 +1,8 @@ +import { + clearAndPlanRevocation, + executeOAuthRevocation, + 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 +15,76 @@ 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 (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. 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; + // Takes the state and deletes it in ONE atomic storage step, then sends. The + // clear must not wait on the network: this server can be inactive when the + // call starts and complete a *fresh* authorization while the request is in + // flight, at which point a later clear would delete the new credentials. The + // session checks in `useOAuthRecovery` run after this helper returns and + // cannot protect the store, so the ordering is what does (#2144). + const plan = await clearAndPlanRevocation({ + serverUrl, + storage: params.oauthStorage, + enabled: revoke && fetchFn !== undefined, + }); + const revocation: TokenRevocationOutcome = fetchFn + ? await executeOAuthRevocation(plan, { fetchFn }) + : { status: "skipped", reason: "disabled" }; + 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 000000000..aeb544eb0 --- /dev/null +++ b/clients/web/src/lib/webProxiedFetch.test.ts @@ -0,0 +1,66 @@ +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("throws when window is unavailable", () => { + vi.stubGlobal("window", undefined); + expect(() => getWebProxiedFetch()).toThrow( + "getWebProxiedFetch requires a browser environment", + ); + vi.unstubAllGlobals(); + }); + + 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 000000000..6c733ea5e --- /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/connection-state.test.ts b/clients/web/src/test/core/auth/connection-state.test.ts index f8235d4a8..d328b01b6 100644 --- a/clients/web/src/test/core/auth/connection-state.test.ts +++ b/clients/web/src/test/core/auth/connection-state.test.ts @@ -57,6 +57,7 @@ function createStorage( clearServerMetadata: vi.fn(), clearIdpSession: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), + takeRevocationSnapshot: vi.fn().mockResolvedValue({ byIssuer: {} }), getCodeVerifier: vi.fn(), getDiscoveryState: vi.fn().mockResolvedValue(undefined), saveDiscoveryState: vi.fn(), diff --git a/clients/web/src/test/core/auth/ema/emaFlow.test.ts b/clients/web/src/test/core/auth/ema/emaFlow.test.ts index e5e24e544..77af62b90 100644 --- a/clients/web/src/test/core/auth/ema/emaFlow.test.ts +++ b/clients/web/src/test/core/auth/ema/emaFlow.test.ts @@ -74,6 +74,7 @@ function createMemoryStorage( saveScope: vi.fn(), clear: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), + takeRevocationSnapshot: vi.fn().mockResolvedValue({ byIssuer: {} }), } as unknown as OAuthStorage; } diff --git a/clients/web/src/test/core/auth/ema/idpSession.test.ts b/clients/web/src/test/core/auth/ema/idpSession.test.ts index 11ef63e72..465be7853 100644 --- a/clients/web/src/test/core/auth/ema/idpSession.test.ts +++ b/clients/web/src/test/core/auth/ema/idpSession.test.ts @@ -25,6 +25,7 @@ describe("idpSession", () => { clearIdpSession: vi.fn(), clear: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), + takeRevocationSnapshot: vi.fn().mockResolvedValue({ byIssuer: {} }), } as unknown as OAuthStorage; }); diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts new file mode 100644 index 000000000..d0dd7ae15 --- /dev/null +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -0,0 +1,1160 @@ +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, + aggregateOutcomes, + buildRevocationRequest, + revocationAuthMethods, + clearAndPlanRevocation, + executeOAuthRevocation, + revokeToken, + selectRevocableToken, +} from "@inspector/core/auth/revocation.js"; +import type { InspectorLogger } from "@inspector/core/logging/index.js"; +import type { TokenRevocationOutcome } from "@inspector/core/auth/revocation.js"; + +/** A fully-typed `InspectorLogger` double, so the mock's shape is checked. */ +function fakeLogger(): InspectorLogger { + return { + level: "info", + fatal: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + info: vi.fn(), + debug: vi.fn(), + trace: vi.fn(), + silent: vi.fn(), + child: vi.fn(() => fakeLogger()), + }; +} + +const SERVER_URL = "https://mcp.example.com/mcp"; +const REVOKE_URL = "https://as.example.com/revoke"; + +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 §2 defaults an omitted revocation list to `client_secret_basic` — + // it does NOT inherit the token endpoint's. Inheriting would send POST + // credentials to an endpoint that never advertised that method. + it("ignores the token endpoint's list", () => { + expect( + revocationAuthMethods( + metadata({ + token_endpoint_auth_methods_supported: ["client_secret_post"], + }), + ), + ).toEqual(["client_secret_basic"]); + }); + + // Named literally, not left empty. An empty list means "the metadata said + // nothing" to `selectClientAuthMethod`, which then honors whatever the + // client's own registration declares — so a client registered + // `client_secret_post` could put credentials in the body of a request to an + // endpoint that promised only Basic. (`OAuthClientInformation` does not carry + // that field, so this path cannot construct the case today; naming the + // default is what keeps it true if the type widens.) + it("names the RFC 8414 default when the revocation list is absent", () => { + expect(revocationAuthMethods(metadata())).toEqual(["client_secret_basic"]); + }); + + it("the default resolves to Basic for a confidential client", () => { + const { init } = buildRevocationRequest({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + clientInformation: { client_id: "cid", client_secret: "sec" }, + supportedAuthMethods: revocationAuthMethods(metadata()), + }); + expect(headerOf(init, "Authorization")).toBe(`Basic ${btoa("cid:sec")}`); + }); + + it("the default leaves a public client unauthenticated", () => { + const { init } = buildRevocationRequest({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + clientInformation: { client_id: "cid" }, + supportedAuthMethods: revocationAuthMethods(metadata()), + }); + expect(headerOf(init, "Authorization")).toBeUndefined(); + expect(body(init).get("client_id")).toBe("cid"); + }); +}); + +describe("aggregateOutcomes", () => { + const failed = { status: "failed", detail: "boom" } as const; + const revoked = { + status: "revoked", + tokenTypeHint: "refresh_token", + endpoint: REVOKE_URL, + } as const; + const skipped = { status: "skipped", reason: "no_endpoint" } as const; + + // A grant still live at the authorization server is the thing worth + // surfacing; another grant's success would silence it. + it("reports a failure over a success", () => { + expect(aggregateOutcomes([revoked, failed])).toEqual(failed); + }); + + it("reports a success over a skip", () => { + expect(aggregateOutcomes([skipped, revoked])).toEqual(revoked); + }); + + it("falls through to the first outcome when none is decisive", () => { + expect(aggregateOutcomes([skipped])).toEqual(skipped); + }); + + // `computeOutcome` returns early when there are no grants, so nothing else + // reaches this — which is why it is tested here rather than left as a + // function that returns `undefined` while typed otherwise. + it("has an answer for an empty list", () => { + expect(aggregateOutcomes([])).toEqual({ + status: "skipped", + reason: "no_tokens", + }); + }); +}); + +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"], + }); + // RFC 6749 §2.3.1 form-urlencodes each half before the colon. That is what + // keeps an id containing `:` unambiguous, and what stops `btoa` throwing on + // a non-Latin-1 secret. + expect(headerOf(init, "Authorization")).toBe( + `Basic ${btoa("id%20one:s%2Fecret")}`, + ); + 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"); + }); + + // `String(err)` is the other half of the detail: a fetch double, or a runtime + // that rejects with a non-Error, must still produce a readable message. + it("reports a non-Error rejection as failed", async () => { + const outcome = await revokeToken({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + supportedAuthMethods: [], + fetchFn: () => Promise.reject("plain string"), + }); + expect(outcome).toEqual({ + status: "failed", + endpoint: REVOKE_URL, + detail: "plain string", + }); + }); + + // A lone UTF-16 surrogate is valid JSON, so it can reach here from a + // persisted client id or secret and makes `encodeURIComponent` throw. Every + // caller has already cleared its local state by the time this runs, so a + // rejection here would break the documented best-effort guarantee. + it("reports an unencodable credential as failed rather than throwing", async () => { + const outcome = await revokeToken({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + clientInformation: { + client_id: "cid", + // Lone high surrogate. + client_secret: `bad${String.fromCharCode(0xd800)}`, + }, + supportedAuthMethods: ["client_secret_basic"], + fetchFn: vi.fn(), + }); + + expect(outcome).toMatchObject({ status: "failed", endpoint: REVOKE_URL }); + }); + + 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 web fetch is `createRemoteFetch`, which re-issues the call as a POST to + // `/api/fetch` and drops `init.signal`; the backend's outbound fetch gets no + // signal either. A signal-only bound is therefore inert on exactly the path + // the timeout exists for, so the deadline has to hold against a fetch that + // ignores the signal entirely. + it("gives up on a fetch that never settles and ignores the signal", async () => { + const outcome = await revokeToken({ + endpoint: REVOKE_URL, + token: "r", + tokenTypeHint: "refresh_token", + supportedAuthMethods: [], + fetchFn: () => new Promise(() => {}), + timeoutMs: 20, + }); + expect(outcome).toMatchObject({ status: "failed", endpoint: REVOKE_URL }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "timed out", + ); + }); + + // The teardown is already committed by the time this runs, so a wedged + // authorization server must not be able to hold it open. + it("bounds the request with an abort signal", async () => { + 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); + }); +}); + +/** + * Compose the two halves the way every caller does. The first one clears, so a + * test that asserts on the store afterwards is looking at an emptied one — the + * ordering guarantee itself is asserted separately below. + */ +async function revokeStoredOAuthTokens(params: { + serverUrl: string; + storage: BrowserOAuthStorage; + fetchFn: typeof fetch; + enabled?: boolean; + timeoutMs?: number; + logger?: InspectorLogger; +}): Promise { + const plan = await clearAndPlanRevocation({ + serverUrl: params.serverUrl, + storage: params.storage, + enabled: params.enabled, + }); + return executeOAuthRevocation(plan, { + fetchFn: params.fetchFn, + timeoutMs: params.timeoutMs, + logger: params.logger, + }); +} + +/** + * Stub the atomic take-and-clear with a hand-built snapshot. Needed wherever + * the shape under test cannot be produced through the store's own API — a + * legacy slot alongside an issuer slot, for instance, since an issuer-stamped + * save promotes and clears the legacy one. + */ +function stubSnapshot( + storage: BrowserOAuthStorage, + snapshot: Partial[0]> = {}, +): void { + vi.spyOn(storage, "takeRevocationSnapshot").mockResolvedValue( + stubSnapshotShape(snapshot), + ); +} + +function stubSnapshotShape(snapshot: { + byIssuer?: Record< + string, + { tokens?: unknown; clientInformation?: unknown } | undefined + >; + legacyTokens?: unknown; + legacyClientInformation?: unknown; + preregisteredClientInformation?: unknown; + serverMetadata?: unknown; +}): { + byIssuer: Record< + string, + { tokens?: unknown; clientInformation?: unknown } | undefined + >; + legacyTokens?: unknown; + legacyClientInformation?: unknown; + preregisteredClientInformation?: unknown; + serverMetadata?: unknown; +} { + return { byIssuer: {}, ...snapshot }; +} + +describe("revokeStoredOAuthTokens (plan + execute)", () => { + let storage: BrowserOAuthStorage; + + beforeEach(async () => { + storage = new BrowserOAuthStorage(); + await storage.clear(SERVER_URL); + }); + + const ISSUER = "https://as.example.com"; + + /** + * A revocable grant: issuer-bound, and matching the cached metadata. Unkeyed + * grants are deliberately NOT revocable (their authorization server is + * unknown), so seeding one here would make every case below assert the + * refusal instead of the behavior it is about. + */ + async function seed(over: Partial = {}): Promise { + await storage.saveTokens( + SERVER_URL, + { access_token: "a", token_type: "Bearer", refresh_token: "r" }, + { issuer: ISSUER }, + ); + 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 grant minted under DCR must be revoked with the registration that minted + // it, even after the server has since been switched to a configured + // `oauth.clientId`. RFC 7009 §2.2 answers 200 for a token the server does not + // recognise as the caller's, so using the wrong client reports `revoked` + // while the grant stays live — and the local record is already gone. + it("authenticates with the registration bound to the grant's issuer", async () => { + await seed(); + await storage.saveClientInformation( + SERVER_URL, + { client_id: "dcr-cid", client_secret: "dcr-sec" }, + { registrationKind: "dcr", issuer: ISSUER }, + ); + // A configured client id was added later; it must NOT win here. + 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("dcr-cid:dcr-sec")}`, + ); + }); + + // The other direction: a token minted with the configured client leaves the + // issuer slot empty (the SDK writes it only after DCR), so the preregistered + // entry is the right fallback — and dropping it would send no client + // authentication at all for exactly the confidential clients most likely to + // require it. + it("falls back to a preconfigured client when the issuer slot has none", 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")}`, + ); + }); + + // `clear(serverUrl)` drops EVERY `byIssuer` slot, so reading only the active + // issuer's token would leave the earlier authorization server's grant live + // while destroying the local record of it — the same leak this feature + // closes, one level down (SEP-2352 keeps credentials per issuer). + it("revokes every issuer-bound grant, not just the active one", async () => { + const issuerMetadata = metadata({ issuer: "https://as.example.com" }); + await storage.saveServerMetadata(SERVER_URL, issuerMetadata); + await storage.saveTokens( + SERVER_URL, + { access_token: "a1", token_type: "Bearer", refresh_token: "r1" }, + { issuer: "https://as.example.com" }, + ); + await storage.saveTokens( + SERVER_URL, + { access_token: "a2", token_type: "Bearer", refresh_token: "r2" }, + { issuer: "https://as.example.com" }, + ); + // A second slot under the SAME issuer would be one grant; use two distinct + // tokens under one issuer plus the ctx-less read to prove dedup instead. + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + await revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }); + + // One issuer, one grant — the ctx-less read is the same token and is deduped. + expect(fetchFn).toHaveBeenCalledTimes(1); + expect( + new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"), + ).toBe("r2"); + }); + + // `getTokens(url, issuer)` falls back to the legacy unkeyed slot when that + // issuer holds none. Treating the fallback as the issuer's would label an + // old, unbound token with a newly discovered authorization server and send it + // there — so an enumerated issuer is read EXACTLY. + it("does not attribute the legacy unkeyed token to an enumerated issuer", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as.example.com" }), + ); + // A legacy, unbound grant... + await storage.saveTokens(SERVER_URL, { + access_token: "legacy-a", + token_type: "Bearer", + refresh_token: "legacy-r", + }); + // ...and an issuer slot carrying only client information, which is the + // shape a partially-migrated flow leaves behind. + await storage.saveClientInformation( + SERVER_URL, + { client_id: "cid" }, + { registrationKind: "dcr", issuer: "https://as.example.com" }, + ); + + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + // Nothing is sent: the only grant here is the legacy one, whose + // authorization server is unknown. The point is that it is never presented + // as the enumerated issuer's — one request with `legacy-r` would be exactly + // that mistake. + expect(fetchFn).not.toHaveBeenCalled(); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "predates issuer binding", + ); + }); + + // The active issuer slot can hold client information without a token, in + // which case the ctx-less read falls back to the LEGACY grant — a distinct + // grant, even if some other issuer happens to hold the same opaque value. + // Suppressing on the value would drop it unrevoked and unreported, which is + // why the suppression keys off the store's issuer stamp instead. + it("keeps the legacy grant when another issuer holds the same token value", async () => { + // Hand-built: an issuer-stamped save promotes and clears the legacy slot, + // so the store's own API cannot produce both at once. + stubSnapshot(storage, { + byIssuer: { + "https://as-a.example.com": { + tokens: { + access_token: "shared", + token_type: "Bearer", + refresh_token: "shared-r", + }, + }, + }, + legacyTokens: { + access_token: "shared", + token_type: "Bearer", + refresh_token: "shared-r", + }, + serverMetadata: { + issuer: "https://as-a.example.com", + authorization_endpoint: "https://as-a.example.com/authorize", + token_endpoint: "https://as-a.example.com/token", + revocation_endpoint: REVOKE_URL, + response_types_supported: ["code"], + }, + }); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + // Two GRANTS despite one token value. The issuer-bound one is revoked; the + // legacy one is refused (unknown authorization server) and reported, rather + // than being silently collapsed into the other by a token-value dedup. + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "predates issuer binding", + ); + }); + + // Absence establishes nothing: a metadata document with no `issuer` cannot + // show that its revocation endpoint belongs to the grant's authorization + // server, so sending the token anyway would disclose a bearer credential to + // a server that may never have minted it. + it("refuses an issuer-bound grant when the cached metadata names no issuer", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: undefined }), + ); + await storage.saveTokens( + SERVER_URL, + { access_token: "a", token_type: "Bearer", refresh_token: "r" }, + { issuer: "https://as-a.example.com" }, + ); + const fetchFn = vi.fn(); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + expect(fetchFn).not.toHaveBeenCalled(); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "names no issuer", + ); + }); + + // "Unkeyed" means the authorization server is UNKNOWN, not that the cached + // endpoint is proven to own the grant — server metadata is a single slot a + // later discovery overwrites, while a legacy token survives until the first + // issuer-stamped save. Refusing costs such an entry its revocation, which is + // the behavior it had before this feature existed, and it earns it back on + // the next authorization. + it("refuses a legacy grant, whose authorization server is unknown", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as.example.com" }), + ); + await storage.saveTokens(SERVER_URL, { + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }); + const fetchFn = vi.fn(); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + expect(fetchFn).not.toHaveBeenCalled(); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "re-authorize", + ); + }); + + // The take-and-clear is one step, so its failure means the state is still + // there. That has to REJECT rather than report: every caller's contract is + // "the clear happened", and a `failed` outcome would say the opposite of what + // occurred. Both clients have a rejection path for exactly this. + it("rejects when the atomic take-and-clear itself fails", async () => { + vi.spyOn(storage, "takeRevocationSnapshot").mockRejectedValue( + new Error("store unwritable"), + ); + + await expect( + revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn: vi.fn(), + }), + ).rejects.toThrow("store unwritable"); + }); + + // The timeout is a budget for the WHOLE teardown, not per request. `clear` + // deletes every issuer slot, so a per-request bound would let a server with + // N of them block the disconnect for N × the timeout — at which point the + // "short timeout" bounds a single request and nothing the user feels. + it("shares one deadline across grants instead of one per grant", async () => { + // Built directly rather than through the store: three *revocable* grants + // means three tokens under one issuer, which `byIssuer` cannot hold (one + // slot per issuer). The budget logic is what is under test here, and a plan + // is a plain value. + const grant = (n: string) => ({ + issuer: "https://as.example.com", + token: `r-${n}`, + tokenTypeHint: "refresh_token" as const, + }); + // Every request hangs, so the first burns the whole budget. + const fetchFn = vi.fn(() => new Promise(() => {})); + + const started = Date.now(); + const outcome = await executeOAuthRevocation( + { + serverUrl: SERVER_URL, + grants: [grant("a"), grant("b"), grant("c")], + failures: [], + endpoint: REVOKE_URL, + supportedAuthMethods: [], + metadataIssuer: "https://as.example.com", + }, + { fetchFn, timeoutMs: 30 }, + ); + const elapsed = Date.now() - started; + + expect(outcome).toMatchObject({ status: "failed" }); + // With a per-grant bound this would be ~90ms. Generous upper bound so the + // assertion is about the shape, not the machine. + expect(elapsed).toBeLessThan(70); + // The first burned the budget; the rest are reported as never attempted. + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + + // A grant bound to an issuer the cached metadata does not describe cannot be + // revoked — that endpoint belongs to a different authorization server, and + // sending it another AS's token would hand a credential to a server that + // never minted it. Saying so is the point: the grant is being dropped. + it("reports a grant whose issuer the cached metadata does not describe", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as-b.example.com" }), + ); + await storage.saveTokens( + SERVER_URL, + { access_token: "a", token_type: "Bearer", refresh_token: "r-a" }, + { issuer: "https://as-a.example.com" }, + ); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + expect(fetchFn).not.toHaveBeenCalled(); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "as-a.example.com", + ); + }); + + // A failure on one grant must not be hidden behind another's success — a + // grant still live at the authorization server is the thing worth surfacing. + it("reports a failure over another grant's success", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as-b.example.com" }), + ); + // Revocable: bound to the issuer the metadata describes. + await storage.saveTokens( + SERVER_URL, + { access_token: "b", token_type: "Bearer", refresh_token: "r-b" }, + { issuer: "https://as-b.example.com" }, + ); + // Not revocable: bound to an issuer the cached metadata is not for. + await storage.saveTokens( + SERVER_URL, + { access_token: "a", token_type: "Bearer", refresh_token: "r-a" }, + { issuer: "https://as-a.example.com" }, + ); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(outcome).toMatchObject({ status: "failed" }); + }); + + // 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 logger = fakeLogger(); + const fetchFn = vi.fn( + async () => new Response(null, { status: 500, statusText: "Boom" }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + logger, + }); + + expect(outcome.status).toBe("failed"); + expect(logger.warn).toHaveBeenCalledTimes(1); + }); + + it("logs the no-endpoint case at debug", async () => { + await seed({ revocation_endpoint: undefined }); + const logger = fakeLogger(); + await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn: vi.fn(), + logger, + }); + expect(logger.debug).toHaveBeenCalledTimes(1); + }); + + // A snapshot the code cannot interpret is reported, not thrown: the state is + // already gone by then, so there is nothing left for the caller to retry. + // (A failure of the take-and-clear ITSELF rejects instead — see above.) + it("reports an uninterpretable snapshot as failed", async () => { + stubSnapshot(storage, { + byIssuer: { + // Not a valid `OAuthTokens` — no `token_type`. + "https://as.example.com": { tokens: { access_token: 42 } }, + }, + serverMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + revocation_endpoint: REVOKE_URL, + response_types_supported: ["code"], + }, + }); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn: vi.fn(), + }); + expect(outcome).toMatchObject({ status: "failed" }); + }); + + // The preconfigured registration is only a *fallback*, so a malformed one + // must not abort grants that carry their own valid credentials. + it("still revokes when the preconfigured client registration is malformed", async () => { + stubSnapshot(storage, { + byIssuer: { + "https://as.example.com": { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r-good", + }, + clientInformation: { client_id: "dcr-cid" }, + }, + }, + // Not a valid `OAuthClientInformation` — no `client_id`. + preregisteredClientInformation: { client_secret: "orphan" }, + serverMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + revocation_endpoint: REVOKE_URL, + response_types_supported: ["code"], + }, + }); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + // The grant was still revoked, with its OWN registration... + expect(fetchFn).toHaveBeenCalledTimes(1); + expect( + new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get( + "client_id", + ), + ).toBe("dcr-cid"); + // ...and the malformed fallback is reported rather than swallowed. + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "preconfigured client registration", + ); + }); + + // A corrupt slot must not abandon the grants that are still revocable — the + // state is gone either way, so the failure has to be reported BESIDE the + // successes rather than instead of them. + it("still revokes the readable grants when one slot cannot be parsed", async () => { + stubSnapshot(storage, { + byIssuer: { + "https://as.example.com": { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r-good", + }, + }, + // Unparseable: no `token_type`. + "https://broken.example.com": { tokens: { access_token: 42 } }, + }, + serverMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + revocation_endpoint: REVOKE_URL, + response_types_supported: ["code"], + }, + }); + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + // The good grant was still revoked... + expect(fetchFn).toHaveBeenCalledTimes(1); + expect( + new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"), + ).toBe("r-good"); + // ...and the unreadable slot is reported rather than swallowed. + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "could not read the stored grant", + ); + }); + + // A token is only meaningful to the AS that minted it, so two issuers minting + // the same opaque string are two grants. Collapsing them would drop the + // second before the issuer-mismatch check could even report it. + it("does not collapse two issuers that minted the same token value", async () => { + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as-a.example.com" }), + ); + for (const issuer of [ + "https://as-a.example.com", + "https://as-b.example.com", + ]) { + await storage.saveTokens( + SERVER_URL, + { access_token: "same", token_type: "Bearer", refresh_token: "same-r" }, + { issuer }, + ); + } + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const outcome = await revokeStoredOAuthTokens({ + serverUrl: SERVER_URL, + storage, + fetchFn, + }); + + // Only as-a matches the cached metadata, so only it is revocable — but + // as-b is REPORTED rather than silently dropped, which is what collapsing + // by token alone would have done. + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(outcome).toMatchObject({ status: "failed" }); + expect(outcome.status === "failed" ? outcome.detail : "").toContain( + "as-b.example.com", + ); + }); +}); + +// The reason the API is two halves rather than one call (#2144, review round +// 13). Revoking first and clearing afterwards delays the clear by however long +// the network takes, and a FRESH authorization completing in that window is +// then deleted by a clear reasoning about the grant it replaced. +describe("plan / clear / execute ordering", () => { + let storage: BrowserOAuthStorage; + + beforeEach(async () => { + storage = new BrowserOAuthStorage(); + await storage.clear(SERVER_URL); + await storage.saveServerMetadata( + SERVER_URL, + metadata({ issuer: "https://as.example.com" }), + ); + await storage.saveTokens( + SERVER_URL, + { access_token: "old", token_type: "Bearer", refresh_token: "old-r" }, + { issuer: "https://as.example.com" }, + ); + }); + + it("does not delete a grant written while the request is in flight", async () => { + let releaseRequest: () => void = () => {}; + const inFlight = new Promise((resolve) => { + releaseRequest = resolve; + }); + const fetchFn = vi.fn(async () => { + await inFlight; + return new Response(null, { status: 200 }); + }); + + // The caller's real sequence: take-and-clear atomically, then send. + const plan = await clearAndPlanRevocation({ + serverUrl: SERVER_URL, + storage, + }); + const sending = executeOAuthRevocation(plan, { fetchFn }); + + // A fresh authorization lands while the request is still out. + await storage.saveTokens( + SERVER_URL, + { access_token: "new", token_type: "Bearer", refresh_token: "new-r" }, + { issuer: "https://as.example.com" }, + ); + + releaseRequest(); + await sending; + + // The new grant survived, and the OLD token is what was revoked. + expect((await storage.getTokens(SERVER_URL))?.access_token).toBe("new"); + expect( + new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"), + ).toBe("old-r"); + }); + + it("sends the snapshot even though the store was emptied first", async () => { + const fetchFn = vi.fn( + async () => new Response(null, { status: 200 }), + ); + + const plan = await clearAndPlanRevocation({ + serverUrl: SERVER_URL, + storage, + }); + expect(await storage.getTokens(SERVER_URL)).toBeUndefined(); + + await expect( + executeOAuthRevocation(plan, { fetchFn }), + ).resolves.toMatchObject({ status: "revoked" }); + expect(fetchFn).toHaveBeenCalledTimes(1); + }); + + it("plans nothing when revocation is disabled", async () => { + const plan = await clearAndPlanRevocation({ + serverUrl: SERVER_URL, + storage, + enabled: false, + }); + const fetchFn = vi.fn(); + + expect(plan.grants).toHaveLength(0); + await expect(executeOAuthRevocation(plan, { fetchFn })).resolves.toEqual({ + status: "skipped", + reason: "disabled", + }); + expect(fetchFn).not.toHaveBeenCalled(); + }); +}); diff --git a/clients/web/src/test/core/auth/storage-browser.test.ts b/clients/web/src/test/core/auth/storage-browser.test.ts index 8fb99cb15..89d0cf27c 100644 --- a/clients/web/src/test/core/auth/storage-browser.test.ts +++ b/clients/web/src/test/core/auth/storage-browser.test.ts @@ -358,6 +358,67 @@ describe("BrowserOAuthStorage", () => { }); }); + // #2144 — the atomic take-and-clear. Split reads followed by a separate clear + // are a check-then-act: each await yields, and an OAuth completion landing in + // one of those gaps saves a grant the clear then destroys. + describe("takeRevocationSnapshot", () => { + it("returns every slot and clears the server in one step", async () => { + await storage.saveTokens( + testServerUrl, + { access_token: "a", token_type: "Bearer" }, + { issuer: "https://as-a.example.com" }, + ); + await storage.savePreregisteredClientInformation(testServerUrl, { + client_id: "static-cid", + }); + await storage.saveServerMetadata(testServerUrl, { + issuer: "https://as-a.example.com", + authorization_endpoint: "https://as-a.example.com/authorize", + token_endpoint: "https://as-a.example.com/token", + response_types_supported: ["code"], + }); + + const snapshot = await storage.takeRevocationSnapshot(testServerUrl); + + expect( + snapshot.byIssuer["https://as-a.example.com"]?.tokens, + ).toMatchObject({ access_token: "a" }); + expect(snapshot.preregisteredClientInformation).toMatchObject({ + client_id: "static-cid", + }); + expect(snapshot.serverMetadata).toMatchObject({ + issuer: "https://as-a.example.com", + }); + // Cleared in the same step. + expect(await storage.getTokens(testServerUrl)).toBeUndefined(); + expect(await storage.getServerMetadata(testServerUrl)).toBeNull(); + expect( + (await storage.takeRevocationSnapshot(testServerUrl)).byIssuer, + ).toEqual({}); + }); + + it("returns the legacy unkeyed slot too", async () => { + await storage.saveTokens(testServerUrl, { + access_token: "legacy", + token_type: "Bearer", + }); + + const snapshot = await storage.takeRevocationSnapshot(testServerUrl); + + expect(snapshot.legacyTokens).toMatchObject({ access_token: "legacy" }); + expect(snapshot.byIssuer).toEqual({}); + }); + + it("is empty for a server with no state", async () => { + const snapshot = await storage.takeRevocationSnapshot( + "https://unknown.example/mcp", + ); + expect(snapshot.byIssuer).toEqual({}); + expect(snapshot.legacyTokens).toBeUndefined(); + expect(snapshot.serverMetadata).toBeUndefined(); + }); + }); + describe("clearServerState", () => { it("should clear all state for a server", async () => { const clientInfo: OAuthClientInformation = { diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index ec4d1df93..f0ee3bc60 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -67,6 +67,7 @@ function createMockParams( saveIdpSession: vi.fn().mockResolvedValue(undefined), clearIdpSession: vi.fn(), clearEnterpriseManagedResourceServers: vi.fn(), + takeRevocationSnapshot: vi.fn().mockResolvedValue({ byIssuer: {} }), getDiscoveryState: vi.fn().mockResolvedValue(undefined), saveDiscoveryState: vi.fn().mockResolvedValue(undefined), clearDiscoveryState: vi.fn().mockResolvedValue(undefined), @@ -186,17 +187,128 @@ describe("OAuthManager", () => { }); describe("clearOAuthTokens", () => { - it("calls storage.clear(serverUrl) when storage is configured", async () => { + /** + * The take-and-clear is ONE atomic storage step now, so `clear` is no + * longer called separately — these assert on `takeRevocationSnapshot`. + */ + function stubSnapshot( + storage: OAuthManagerConfig["storage"], + revocable = true, + ): void { + vi.mocked(storage!.takeRevocationSnapshot).mockResolvedValue({ + byIssuer: revocable + ? { + "https://as.example.com": { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }, + }, + } + : {}, + serverMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + revocation_endpoint: "https://as.example.com/revoke", + response_types_supported: ["code"], + }, + }); + } + + it("takes and clears the server's state when storage is configured", async () => { const params = createMockParams(); const manager = new OAuthManager(params); await manager.clearOAuthTokens(); - expect(params.initialConfig.storage!.clear).toHaveBeenCalledWith( - SERVER_URL, - ); + expect( + params.initialConfig.storage!.takeRevocationSnapshot, + ).toHaveBeenCalledWith(SERVER_URL); expect(manager.getOAuthFlowState()).toBeUndefined(); expect(manager.getOAuthFlowStep()).toBeUndefined(); }); + // #2144 — the ordering is the contract. The request is built from state the + // clear destroys, so the take must come first; but the two are ONE step, so + // nothing can save a fresh grant between them, and the network wait happens + // after the state is already gone. + it("clears local state before waiting on the revocation request", async () => { + const params = createMockParams(); + const storage = params.initialConfig.storage!; + const order: string[] = []; + vi.mocked(storage.takeRevocationSnapshot).mockImplementation(async () => { + order.push("clear"); + return { + byIssuer: { + "https://as.example.com": { + tokens: { + access_token: "a", + token_type: "Bearer", + refresh_token: "r", + }, + }, + }, + serverMetadata: { + issuer: "https://as.example.com", + authorization_endpoint: "https://as.example.com/authorize", + token_endpoint: "https://as.example.com/token", + revocation_endpoint: "https://as.example.com/revoke", + response_types_supported: ["code"], + }, + }; + }); + const fetchFn = vi.fn(async () => { + order.push("revoke"); + 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(["clear", "revoke"]); + }); + + it("clears local state even when the revocation request fails", async () => { + const params = createMockParams(); + const storage = params.initialConfig.storage!; + stubSnapshot(storage); + const manager = new OAuthManager({ + ...params, + effectiveAuthFetch: vi.fn(async () => { + throw new Error("unreachable"); + }), + }); + + await expect(manager.clearOAuthTokens()).resolves.toMatchObject({ + status: "failed", + }); + expect(storage.takeRevocationSnapshot).toHaveBeenCalledWith(SERVER_URL); + }); + + it("skips the request when revocation is turned off", async () => { + const params = createMockParams(); + const storage = params.initialConfig.storage!; + stubSnapshot(storage); + const fetchFn = vi.fn(); + const manager = new OAuthManager({ + ...params, + effectiveAuthFetch: fetchFn, + }); + + await expect( + manager.clearOAuthTokens({ revoke: false }), + ).resolves.toEqual({ status: "skipped", reason: "disabled" }); + expect(fetchFn).not.toHaveBeenCalled(); + // Still cleared — that is what the caller asked for; the request is the + // optional part. + expect(storage.takeRevocationSnapshot).toHaveBeenCalledWith(SERVER_URL); + }); + it("no-ops when storage is not configured", async () => { const params = createMockParams({ initialConfig: { @@ -207,7 +319,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 8f8ca44eb..f1a863843 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 000000000..0e3b33730 --- /dev/null +++ b/clients/web/src/test/integration/auth/revocation-e2e.test.ts @@ -0,0 +1,272 @@ +/** + * 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 { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + TestServerHttp, + createOAuthTestServerConfig, + getDefaultServerConfig, + waitForOAuthWellKnown, +} from "@modelcontextprotocol/inspector-test-server"; +// NOT `BrowserOAuthStorage`: the `integration` project runs in the node +// environment, where `sessionStorage` does not exist. Node 22 gates Web Storage +// behind a flag and newer Node exposes it by default, so a browser-backed store +// passes on a developer's machine and fails in CI — which is exactly what +// happened here. +import { NodeOAuthStorage } from "@inspector/core/auth/node/storage-node.js"; +import { + clearAndPlanRevocation, + executeOAuthRevocation, +} from "@inspector/core/auth/revocation.js"; +import type { TokenRevocationOutcome } from "@inspector/core/auth/revocation.js"; +import type { OAuthMetadata } from "@modelcontextprotocol/client"; + +const CLIENT_ID = "test-2144-revocation"; +const CLIENT_SECRET = "test-2144-secret"; +const REDIRECT_URL = "http://localhost:3000/oauth/callback"; +/** A second registered client, used to prove tokens are not cross-revocable. */ +const OTHER_CLIENT_ID = "test-2144-other"; +const OTHER_CLIENT_SECRET = "test-2144-other-secret"; + +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 storageDir = ""; + 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], + }, + { + clientId: OTHER_CLIENT_ID, + clientSecret: OTHER_CLIENT_SECRET, + redirectUris: [REDIRECT_URL], + }, + ], + }), + }); + const port = await mcpServer.start(); + serverUrl = `http://localhost:${port}`; + storageDir = mkdtempSync(join(tmpdir(), "mcp-inspector-2144-")); + 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; + if (storageDir) { + rmSync(storageDir, { recursive: true, force: true }); + storageDir = ""; + } + }, 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 NodeOAuthStorage(join(storageDir, "oauth.json")); + await storage.clear(serverUrl); + // Issuer-bound and matching the discovered metadata: an unkeyed grant + // records no authorization server and is deliberately refused. + await storage.saveTokens( + serverUrl, + { token_type: "Bearer", ...tokens }, + { issuer: metadata.issuer }, + ); + 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; + } + + // The fixture enforces RFC 7009 §2.1 client authentication, so this is what + // makes the passing cases below mean something: a request with the wrong + // credentials is refused, and the endpoint is not simply answering 200 to + // anything. + it("refuses a revocation request with the wrong client secret", async () => { + const response = await fetch(`${serverUrl}/oauth/revoke`, { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + Authorization: `Basic ${Buffer.from(`${CLIENT_ID}:wrong`).toString("base64")}`, + }, + body: new URLSearchParams({ token: "anything" }), + }); + expect(response.status).toBe(401); + }); + + it("refuses a revocation request naming no client at all", async () => { + const response = await fetch(`${serverUrl}/oauth/revoke`, { + method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + body: new URLSearchParams({ token: "anything" }), + }); + expect(response.status).toBe(401); + }); + + // RFC 7009 §2.1: only the client a token was issued to may revoke it. The + // response stays 200 (§2.2 — it must not tell one client whether another's + // token exists), so the assertion is that the token still works. + it("does not revoke a token belonging to a different client", async () => { + const tokens = await authorize(); + const response = await fetch(`${serverUrl}/oauth/revoke`, { + method: "POST", + headers: { + "Content-Type": "application/x-www-form-urlencoded", + Authorization: `Basic ${Buffer.from(`${OTHER_CLIENT_ID}:${OTHER_CLIENT_SECRET}`).toString("base64")}`, + }, + body: new URLSearchParams({ + token: tokens.refresh_token, + token_type_hint: "refresh_token", + }), + }); + + expect(response.status).toBe(200); + expect(await tokenAccepted(tokens.access_token)).toBe(true); + }); + + /** + * The caller's real sequence — take-and-clear atomically, then send — so this + * exercises the ordering the product uses rather than a convenience wrapper. + */ + async function clearAndRevoke( + storage: NodeOAuthStorage, + ): Promise { + const plan = await clearAndPlanRevocation({ serverUrl, storage }); + return executeOAuthRevocation(plan, { fetchFn: fetch }); + } + + 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 clearAndRevoke(await seededStorage(tokens)); + + 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 clearAndRevoke( + await seededStorage({ access_token: tokens.access_token }), + ); + + 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(clearAndRevoke(storage)).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 deafe89bd..55dd7dcad 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 1ab2badac..aabda5f2d 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/clients/web/src/utils/serverWithDraftSettings.test.ts b/clients/web/src/utils/serverWithDraftSettings.test.ts new file mode 100644 index 000000000..dce42d968 --- /dev/null +++ b/clients/web/src/utils/serverWithDraftSettings.test.ts @@ -0,0 +1,67 @@ +import { describe, it, expect } from "vitest"; +import type { + InspectorServerSettings, + ServerEntry, +} from "@inspector/core/mcp/types.js"; +import { serverWithDraftSettings } from "./serverWithDraftSettings"; + +const settings = ( + over: Partial = {}, +): InspectorServerSettings => ({ + headers: [], + env: [], + metadata: {}, + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 60000, + maxFetchRequests: 1000, + roots: [], + ...over, +}); + +const entry = (over: Partial = {}): ServerEntry => + ({ + id: "a", + name: "Server A", + config: { type: "streamable-http", url: "https://mcp.example/mcp" }, + connection: { status: "disconnected" }, + ...over, + }) as ServerEntry; + +describe("serverWithDraftSettings", () => { + // The case the helper exists for (#2144): an action in the settings modal + // must read the value on screen, not the one the debounced save has written. + it("prefers an unsaved draft over the persisted settings", () => { + const result = serverWithDraftSettings( + entry({ settings: settings({ oauthRevokeOnClear: undefined }) }), + settings({ oauthRevokeOnClear: false }), + ); + expect(result.settings?.oauthRevokeOnClear).toBe(false); + }); + + // The other direction matters just as much: re-checking the box before the + // save lands must not leave the clear skipping revocation. + it("prefers a draft that turns the setting back on", () => { + const result = serverWithDraftSettings( + entry({ settings: settings({ oauthRevokeOnClear: false }) }), + settings({ oauthRevokeOnClear: undefined }), + ); + expect(result.settings?.oauthRevokeOnClear).toBeUndefined(); + }); + + it("returns the entry untouched when there is no draft", () => { + const original = entry({ + settings: settings({ oauthRevokeOnClear: false }), + }); + expect(serverWithDraftSettings(original, undefined)).toBe(original); + }); + + it("keeps the entry's identity and config", () => { + const original = entry({ settings: settings() }); + const result = serverWithDraftSettings(original, settings({ taskTtl: 1 })); + expect(result.id).toBe(original.id); + expect(result.name).toBe(original.name); + expect(result.config).toBe(original.config); + expect(result).not.toBe(original); + }); +}); diff --git a/clients/web/src/utils/serverWithDraftSettings.ts b/clients/web/src/utils/serverWithDraftSettings.ts new file mode 100644 index 000000000..34808428c --- /dev/null +++ b/clients/web/src/utils/serverWithDraftSettings.ts @@ -0,0 +1,34 @@ +import type { + InspectorServerSettings, + ServerEntry, +} from "@inspector/core/mcp/types.js"; + +/** + * A server entry whose `settings` reflect the **unsaved** settings draft. + * + * Actions taken from inside the Server Settings modal have to act on what the + * user is looking at, not on what has been persisted. The entry comes from the + * `servers` list, which the debounced save has not reached yet, so reading its + * `settings` means a control toggled a moment ago is still at its previous + * value. That is invisible for most settings — they are read on the next + * connect — but not for one consumed by a button in the same dialog: + * unchecking "Revoke tokens on clear" and immediately clearing would still + * revoke, and re-checking it would still skip (#2144). + * + * `draft` is nullish before the modal has produced one — `useSettingsDraft` + * types it `| null` and callers may hold `| undefined` — in which case the + * entry's own settings are already the current answer. Both are accepted so a + * caller never has to normalize one into the other at the call site, which is + * exactly where the distinction would get lost. + * + * Extracted rather than inlined at the call site so the rule has a test: the + * hook tests below `clearServerOAuthAndDisconnect` receive whatever they are + * handed and cannot tell a draft from a persisted entry, and `App.tsx`'s own + * settings harness connects a stdio server, where the OAuth section never + * renders at all. + */ +export function serverWithDraftSettings< + T extends Pick, +>(entry: T, draft: InspectorServerSettings | null | undefined): T { + return draft == null ? entry : { ...entry, settings: draft }; +} diff --git a/core/auth/index.ts b/core/auth/index.ts index f38fcbe03..0ddad0c54 100644 --- a/core/auth/index.ts +++ b/core/auth/index.ts @@ -134,5 +134,26 @@ export { // Discovery export { discoverScopes } from "./discovery.js"; +// RFC 7009 token revocation (#2144) +export { + DEFAULT_REVOCATION_TIMEOUT_MS, + aggregateOutcomes, + buildRevocationRequest, + revocationAuthMethods, + clearAndPlanRevocation, + executeOAuthRevocation, + revokeToken, + selectRevocableToken, +} from "./revocation.js"; +export type { + ExecuteOAuthRevocationParams, + OAuthRevocationPlan, + PlanOAuthRevocationParams, + RevocationRequestParams, + RevokeTokenParams, + TokenRevocationOutcome, + TokenRevocationSkipReason, +} from "./revocation.js"; + // Logging (re-exported from core/logging) export { silentLogger } from "../logging/index.js"; diff --git a/core/auth/oauth-storage.ts b/core/auth/oauth-storage.ts index f68baa64c..68b7810c2 100644 --- a/core/auth/oauth-storage.ts +++ b/core/auth/oauth-storage.ts @@ -8,7 +8,7 @@ import { OAuthClientInformationSchema, OAuthTokensSchema, } from "@modelcontextprotocol/core"; -import type { OAuthStorage } from "./storage.js"; +import type { OAuthStorage, RevocationSnapshot } from "./storage.js"; import { type OAuthMemoryStore, type ServerOAuthState, @@ -424,6 +424,29 @@ export class OAuthStorageBase implements OAuthStorage { await this.persist(); } + async takeRevocationSnapshot(serverUrl: string): Promise { + await this.ensureLoaded(); + // Everything below is synchronous on purpose: read and clear against one + // view of the in-memory state, so nothing can land between them. The + // persist is the only await, and it happens after the mutation. + const state = this.memory.getState().getServerState(serverUrl); + const snapshot: RevocationSnapshot = { + byIssuer: Object.fromEntries( + Object.entries(state.byIssuer ?? {}).map(([issuer, slot]) => [ + issuer, + { tokens: slot?.tokens, clientInformation: slot?.clientInformation }, + ]), + ), + legacyTokens: state.tokens, + legacyClientInformation: state.clientInformation, + preregisteredClientInformation: state.preregisteredClientInformation, + serverMetadata: state.serverMetadata, + }; + this.memory.getState().clearServerState(serverUrl); + await this.persist(); + return snapshot; + } + async clear(serverUrl: string): Promise { await this.ensureLoaded(); this.memory.getState().clearServerState(serverUrl); diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts new file mode 100644 index 000000000..8a438ccdc --- /dev/null +++ b/core/auth/revocation.ts @@ -0,0 +1,743 @@ +/** + * 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, + OAuthTokens, +} from "@modelcontextprotocol/client"; +import { selectClientAuthMethod } from "@modelcontextprotocol/client"; +import { + OAuthClientInformationSchema, + OAuthTokensSchema, +} from "@modelcontextprotocol/core"; +import type { InspectorLogger } from "../logging/index.js"; +import type { OAuthStorage, RevocationSnapshot } 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. + * + * It is a budget for the **whole** call, not per request: a server with several + * issuer-bound grants revokes them sequentially against one shared deadline, so + * the bound the user feels is this number regardless of how many grants the + * clear is about to delete. + */ +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 the revocation endpoint advertises. + * + * RFC 8414 §2 gives `revocation_endpoint_auth_methods_supported` a default of + * **`client_secret_basic`** when it is omitted — it does *not* inherit + * `token_endpoint_auth_methods_supported`. Falling through to the token + * endpoint's list would make metadata advertising only `client_secret_post` + * there send POST credentials to a revocation endpoint that never advertised + * that method, which a strict server rejects. + * + * The default is returned **literally** rather than as an empty list. An empty + * list does not mean "apply the RFC default" to `selectClientAuthMethod`: it + * means "the metadata said nothing", and the SDK then honors whatever + * `token_endpoint_auth_method` the client's own registration declares. A DCR + * client registered `client_secret_post` would therefore put its credentials + * in the body of a request to an endpoint that promised only Basic, and the + * revocation would fail. Naming the default is what actually enforces it. + */ +export function revocationAuthMethods(metadata: CachedMetadata): string[] { + return ( + metadata.revocation_endpoint_auth_methods_supported ?? [ + "client_secret_basic", + ] + ); +} + +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) { + // RFC 6749 §2.3.1: each half is form-urlencoded *before* the colon joins + // them. This deliberately differs from the SDK's `applyBasicAuth`, which + // base64s the raw pair — matching that was the first instinct, on the + // theory that a credential should be presented the same way here as at + // the token endpoint. It does not hold up: a client authenticated with + // `client_secret_post` never exercised the SDK's Basic path at all, so + // there is no precedent to match, and the raw form is ambiguous for a + // client id containing `:` and makes `btoa` throw outright on a + // non-Latin-1 secret. Encoding is what the server decodes. + const credentials = `${encodeURIComponent(client.client_id)}:${encodeURIComponent(client.client_secret)}`; + headers.Authorization = `Basic ${base64Encode(credentials)}`; + } 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 timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS; + try { + // Inside the try: `encodeURIComponent` throws on a lone UTF-16 surrogate, + // which is valid JSON and so can reach here from a persisted client id or + // secret. Built outside, that would reject instead of returning a `failed` + // outcome — and every caller has already cleared its local state by now, so + // a rejection would break the documented best-effort guarantee. + const { url, init } = buildRevocationRequest(params); + // The signal alone is not enough to bound this. In the browser the fetch is + // `createRemoteFetch`, which re-issues the call as a POST to `/api/fetch` + // and does not forward `init.signal`; the backend's outbound fetch gets no + // signal either. So a wedged authorization server would hold the teardown + // open indefinitely on exactly the path the timeout exists for. The race is + // what actually enforces the deadline; the signal is kept because it does + // cancel the direct-fetch paths (CLI, TUI, backend) rather than merely + // abandoning them. + const response = await withDeadline( + params.fetchFn(url, { ...init, signal: AbortSignal.timeout(timeoutMs) }), + timeoutMs, + ); + if (!response.ok) { + return { + status: "failed", + endpoint: url, + detail: + `revocation endpoint responded ${response.status} ${response.statusText}`.trim(), + }; + } + return { + status: "revoked", + tokenTypeHint: params.tokenTypeHint, + endpoint: url, + }; + } catch (err) { + return { + // `params.endpoint`, not the built `url`: construction is inside the try + // now, so `url` may not exist on this path. + status: "failed", + endpoint: params.endpoint, + detail: err instanceof Error ? err.message : String(err), + }; + } +} + +/** + * Reject with a timeout error if `promise` has not settled within `timeoutMs`. + * + * The underlying request is abandoned rather than cancelled — nothing here can + * cancel a fetch the proxy already stripped the signal from. That is the right + * trade for this caller: the point is that the *teardown* proceeds, and the + * response, if it ever arrives, is a revocation we no longer need to wait for. + * The timer is cleared on the settled path so a caller is never held awake by it. + */ +async function withDeadline( + promise: Promise, + timeoutMs: number, +): Promise { + let timer: ReturnType | undefined; + const deadline = new Promise((_resolve, reject) => { + timer = setTimeout( + () => + reject(new Error(`revocation request timed out after ${timeoutMs}ms`)), + timeoutMs, + ); + }); + try { + return await Promise.race([promise, deadline]); + } finally { + /* v8 ignore next -- the Promise executor runs synchronously, so `timer` is + always assigned by the time this runs; the guard exists only because + TypeScript cannot see that. */ + if (timer !== undefined) clearTimeout(timer); + } +} + +export interface PlanOAuthRevocationParams { + serverUrl: string; + storage: OAuthStorage; + /** + * `false` plans nothing — the deliberate case from #2144, where a user wants + * to watch a server cope with a client that walks off still holding live + * tokens. + */ + enabled?: boolean; +} + +export interface ExecuteOAuthRevocationParams { + fetchFn: typeof fetch; + timeoutMs?: number; + logger?: InspectorLogger; +} + +/** + * Everything the revocation requests need, read out of the store **before** the + * local clear empties it. + * + * Split from the sending on purpose. `clearAndPlanRevocation` takes the state + * and deletes it in one atomic storage step; the requests then go out from this + * copy, which nothing can invalidate. Revoking first and clearing afterwards + * would delay the clear by however long the network takes, and a *fresh* + * authorization completing in that window would be deleted by a clear + * reasoning about the grant it replaced. + */ +export interface OAuthRevocationPlan { + serverUrl: string; + grants: StoredGrant[]; + /** Slots that could not be read; reported beside whatever the rest do. */ + failures: TokenRevocationOutcome[]; + endpoint?: string; + supportedAuthMethods: string[]; + metadataIssuer?: string; + /** Set when there is nothing to send, and why. */ + outcome?: TokenRevocationOutcome; +} + +/** A plan that sends nothing, carrying the reason. */ +function emptyPlan( + serverUrl: string, + outcome: TokenRevocationOutcome, + failures: TokenRevocationOutcome[] = [], +): OAuthRevocationPlan { + return { + serverUrl, + grants: [], + failures, + supportedAuthMethods: [], + outcome: + failures.length > 0 ? aggregateOutcomes([...failures, outcome]) : outcome, + }; +} + +/** + * Take the server's OAuth state — deleting it — and build the revocation + * requests from what was taken. + * + * The take and the delete are one atomic storage step + * ({@link OAuthStorage.takeRevocationSnapshot}), which is what closes the + * check-then-act window: separate reads followed by a separate clear each yield + * at an `await`, and an OAuth completion landing in one of those gaps would + * save a fresh grant that the clear then destroyed. + * + * The state is cleared **even when revocation is disabled or nothing can be + * revoked** — clearing is what the caller asked for; the requests are the + * optional part. + * + * Every grant is covered, not just the active issuer's: see + * {@link collectGrants}. The metadata comes from the cache the OAuth flow + * already populated rather than from a fresh discovery round-trip — the tokens + * being revoked were minted by that same authorization server, so its cached + * document is the one that describes them, and re-discovering would add two + * network legs to a teardown for no new information. + */ +export async function clearAndPlanRevocation( + params: PlanOAuthRevocationParams, +): Promise { + const { serverUrl, storage } = params; + + let snapshot: RevocationSnapshot; + try { + snapshot = await storage.takeRevocationSnapshot(serverUrl); + } catch (err) { + // The clear did not happen, and the caller has no other way to learn that. + throw err instanceof Error ? err : new Error(String(err)); + } + + if (params.enabled === false) { + return emptyPlan(serverUrl, { status: "skipped", reason: "disabled" }); + } + + try { + const { grants, failures } = await collectGrants(snapshot); + if (grants.length === 0) { + return emptyPlan( + serverUrl, + { status: "skipped", reason: "no_tokens" }, + failures, + ); + } + + const metadata = parseServerMetadata(snapshot.serverMetadata); + if (!metadata) { + return emptyPlan( + serverUrl, + { status: "skipped", reason: "no_metadata" }, + failures, + ); + } + if (!metadata.revocation_endpoint) { + return emptyPlan( + serverUrl, + { status: "skipped", reason: "no_endpoint" }, + failures, + ); + } + + return { + serverUrl, + grants, + failures, + endpoint: metadata.revocation_endpoint, + supportedAuthMethods: revocationAuthMethods(metadata), + metadataIssuer: metadata.issuer, + }; + } catch (err) { + // The state is already gone; a snapshot we cannot interpret is reported, + // not thrown, because there is nothing left for the caller to retry. + return emptyPlan(serverUrl, { + status: "failed", + detail: err instanceof Error ? err.message : String(err), + }); + } +} + +/** + * The cached authorization-server metadata from a snapshot. + * + * Read structurally rather than schema-parsed: this is a document the SDK's own + * discovery wrote and the store round-trips as-is, and only two fields are + * used. A parse failure here would lose an otherwise revocable grant. + */ +function parseServerMetadata(raw: unknown): CachedMetadata | undefined { + if (raw === null || typeof raw !== "object") return undefined; + return raw as CachedMetadata; +} + +/** The three fields revocation reads out of cached AS metadata. */ +interface CachedMetadata { + revocation_endpoint?: string; + revocation_endpoint_auth_methods_supported?: string[]; + issuer?: string; +} + +/** + * Send the requests a {@link clearAndPlanRevocation} snapshot describes. + * + * Safe to run **after** `storage.clear(serverUrl)` — that is the point: the + * plan already holds the tokens, the credentials and the endpoint, so nothing + * here reads the store, and the local clear is never waiting on the network. + * + * Best-effort throughout: every path returns a {@link TokenRevocationOutcome} + * rather than throwing, because forgetting the tokens is what the caller + * actually asked for and no failure on this leg should undo it. + */ +export async function executeOAuthRevocation( + plan: OAuthRevocationPlan, + params: ExecuteOAuthRevocationParams, +): Promise { + const outcome = await runPlan(plan, params); + const { logger } = params; + if (outcome.status === "failed") { + logger?.warn( + { + serverUrl: plan.serverUrl, + endpoint: outcome.endpoint, + detail: outcome.detail, + }, + "Token revocation failed; local OAuth state was cleared anyway", + ); + } else if (outcome.status === "skipped" && outcome.reason === "no_endpoint") { + logger?.debug( + { serverUrl: plan.serverUrl }, + "Skipping token revocation: authorization server metadata has no revocation_endpoint", + ); + } + return outcome; +} + +/** One revocable grant held for a server, and which AS minted it. */ +export interface StoredGrant { + /** Undefined for the legacy unkeyed slot, which predates issuer binding. */ + issuer?: string; + token: string; + tokenTypeHint: "refresh_token" | "access_token"; + /** + * The credentials to authenticate the revocation request with: the + * registration bound to this grant's issuer, falling back to the + * preconfigured (static) one. + * + * Deliberately the **reverse** of `BaseOAuthClientProvider.clientInformation`, + * which prefers the preregistered entry. That order answers "who should I + * authenticate as *now*"; revocation asks "who minted *this* token", and the + * two diverge — the store lets a preregistered client and an issuer-bound + * dynamic registration coexist, so after a server is switched from DCR to a + * configured `oauth.clientId` an older DCR grant would be revoked with the + * configured client's credentials. RFC 7009 §2.2 answers 200 for a token the + * server does not recognise as the caller's, so that reports `revoked` while + * the grant stays live and the local record is already gone. + * + * Best available evidence, not proof: the store records client information + * per issuer, not per token, so a server that re-registered dynamically under + * one issuer still cannot say which registration minted which grant. Binding + * the identity to the token at save time is the real fix and is a + * storage-shape change. + */ + clientInformation?: OAuthClientInformation; +} + +/** What {@link collectGrants} found, including the slots it could not read. */ +interface CollectedGrants { + grants: StoredGrant[]; + /** + * One `failed` outcome per slot that could not be parsed. Kept apart from the + * grants so a single corrupt slot cannot abandon the ones that are still + * revocable — the state is gone either way, so the failure has to be reported + * *beside* the successes rather than instead of them. + */ + failures: TokenRevocationOutcome[]; +} + +/** + * Every grant the snapshot took, deduplicated by **issuer and token**. + * + * The snapshot is of state that has already been deleted, so this covers every + * grant the clear removed rather than just the active issuer's: a server that + * authorized against issuers A and B has two grants here, not one. Otherwise + * B would be revoked while A's grant stayed live at its authorization server — + * the same leak this feature closes, one level down. + * + * Deduplication is by issuer *and* token because a token means nothing outside + * the authorization server that minted it: two issuers that happen to mint the + * same opaque string are two grants, and collapsing them would drop the second + * before the issuer check could report it. + * + * Parsing happens here rather than in the snapshot: it is pure, so it belongs + * after the mutation — running it inside would reintroduce the `await` the + * atomic read exists to remove. + */ +async function collectGrants( + snapshot: RevocationSnapshot, +): Promise { + const grants: StoredGrant[] = []; + const failures: TokenRevocationOutcome[] = []; + const seenKeys = new Set(); + + // Parsed defensively: it is only a *fallback*, so a malformed preconfigured + // registration must not abort grants that carry their own valid credentials. + // Recorded and skipped, matching the per-slot salvage below. + let preregistered: OAuthClientInformation | undefined; + try { + preregistered = await parseClient(snapshot.preregisteredClientInformation); + } catch (err) { + failures.push({ + status: "failed", + detail: `could not read the preconfigured client registration: ${ + err instanceof Error ? err.message : String(err) + }`, + }); + } + + const add = async ( + issuer: string | undefined, + rawTokens: unknown, + rawClient: unknown, + ): Promise => { + const revocable = selectRevocableToken(await parseTokens(rawTokens)); + if (!revocable) return; + + const key = `${issuer ?? "\u0000legacy"}\u0000${revocable.token}`; + if (seenKeys.has(key)) return; + seenKeys.add(key); + + // The registration bound to THIS grant wins, with the preconfigured entry + // as the fallback — see the note on `StoredGrant.clientInformation`. + const bound = await parseClient(rawClient); + grants.push({ + issuer, + ...revocable, + clientInformation: bound ?? preregistered, + }); + }; + + /** Read one slot; a slot that cannot be parsed is reported, not fatal. */ + const addSafely = async ( + issuer: string | undefined, + rawTokens: unknown, + rawClient: unknown, + ): Promise => { + try { + await add(issuer, rawTokens, rawClient); + } catch (err) { + const where = + issuer === undefined ? "the unkeyed slot" : `issuer ${issuer}`; + failures.push({ + status: "failed", + detail: `could not read the stored grant for ${where}: ${ + err instanceof Error ? err.message : String(err) + }`, + }); + } + }; + + for (const [issuer, slot] of Object.entries(snapshot.byIssuer)) { + await addSafely(issuer, slot?.tokens, slot?.clientInformation); + } + await addSafely( + undefined, + snapshot.legacyTokens, + snapshot.legacyClientInformation, + ); + return { grants, failures }; +} + +/** Parse stored tokens, or `undefined` when the slot held none. */ +async function parseTokens(raw: unknown): Promise { + return raw === undefined || raw === null + ? undefined + : await OAuthTokensSchema.parseAsync(raw); +} + +/** Parse stored client information, or `undefined` when the slot held none. */ +async function parseClient( + raw: unknown, +): Promise { + return raw === undefined || raw === null + ? undefined + : await OAuthClientInformationSchema.parseAsync(raw); +} + +/** + * Combine per-grant outcomes into the one this function reports. + * + * A **failure outranks a success**: a grant still live at the authorization + * server is what the caller needs to surface, and reporting another grant's + * success would leave that silent. A success outranks a skip for the same + * reason in the other direction — "revoked" is the more specific truth. + * + * Exported for its own test. `computeOutcome` returns early when there are no + * grants, so the empty case cannot arise from there — which is exactly why the + * fallback needs testing somewhere: nothing else would ever exercise it, and a + * function that returns `undefined` while typed otherwise is a trap for the + * next caller. + */ +export function aggregateOutcomes( + outcomes: TokenRevocationOutcome[], +): TokenRevocationOutcome { + return ( + outcomes.find((o) => o.status === "failed") ?? + outcomes.find((o) => o.status === "revoked") ?? + outcomes[0] ?? { status: "skipped", reason: "no_tokens" } + ); +} + +/** Send one plan's requests against a single shared deadline. */ +async function runPlan( + plan: OAuthRevocationPlan, + params: ExecuteOAuthRevocationParams, +): Promise { + if (plan.outcome) return plan.outcome; + const endpoint = plan.endpoint; + /* v8 ignore next -- `clearAndPlanRevocation` always sets `outcome` when it sets + no endpoint, so this is unreachable; the guard exists to narrow the type. */ + if (!endpoint) return { status: "skipped", reason: "no_endpoint" }; + + const outcomes: TokenRevocationOutcome[] = [...plan.failures]; + // ONE deadline for the whole teardown, not one per grant. `clear` deletes + // every issuer slot, so a server with N of them would otherwise block for + // N × the timeout — at which point the "short timeout" bounds a single + // request and nothing the user experiences. Grants are revoked sequentially + // on purpose (a burst of parallel requests to one authorization server is + // not a kindness), so the budget is shared instead. + const timeoutMs = params.timeoutMs ?? DEFAULT_REVOCATION_TIMEOUT_MS; + const deadlineAt = Date.now() + timeoutMs; + + for (const grant of plan.grants) { + // Metadata is cached once per server, not per issuer, so it describes + // whichever authorization server was discovered last. Sending another + // issuer's token to *this* endpoint would disclose a bearer credential to a + // server that never minted it — worse than not revoking. So an issuer-bound + // grant must be able to PROVE the endpoint is its own, which means a + // metadata document carrying no `issuer` is a mismatch rather than a free + // pass: absence establishes nothing. The same reasoning rules out the + // unkeyed legacy grant just below — see there. + if (grant.issuer === undefined) { + // An unkeyed (pre-SEP-2352) grant records no authorization server at all, + // and "unknown" is not "whatever the cache currently holds": server-level + // metadata is a single slot that a later discovery overwrites, while the + // legacy token survives until the first issuer-stamped save. So the + // endpoint on hand may belong to an authorization server that never + // minted this token, and sending it there would disclose a bearer + // credential to a stranger. + // + // Refusing costs such an entry its revocation — which is exactly the + // behavior it had before this feature existed, so nothing regresses — and + // it earns revocation back the moment it is re-authorized, since that + // save is issuer-stamped. Not sending is the only answer that cannot be + // wrong. + outcomes.push({ + status: "failed", + detail: + "the stored grant is not bound to an authorization server (it predates issuer binding), so it could not be matched to this revocation endpoint and was cleared without revocation — re-authorize to make future clears revocable", + }); + continue; + } + if (plan.metadataIssuer !== grant.issuer) { + outcomes.push({ + status: "failed", + detail: + plan.metadataIssuer === undefined + ? `the cached authorization-server metadata names no issuer, so the grant bound to ${grant.issuer} could not be matched to this revocation endpoint and was cleared without revocation` + : `the cached authorization-server metadata is for ${plan.metadataIssuer}, so the grant bound to ${grant.issuer} was cleared without revocation`, + }); + continue; + } + + const remainingMs = deadlineAt - Date.now(); + if (remainingMs <= 0) { + outcomes.push({ + status: "failed", + endpoint, + detail: `the ${timeoutMs}ms revocation budget was exhausted before this grant was attempted${ + grant.issuer === undefined ? "" : ` (issuer ${grant.issuer})` + }`, + }); + continue; + } + + outcomes.push( + await revokeToken({ + endpoint, + token: grant.token, + tokenTypeHint: grant.tokenTypeHint, + clientInformation: grant.clientInformation, + supportedAuthMethods: plan.supportedAuthMethods, + fetchFn: params.fetchFn, + timeoutMs: remainingMs, + }), + ); + } + return aggregateOutcomes(outcomes); +} diff --git a/core/auth/storage.ts b/core/auth/storage.ts index 90163c19d..edc6451ec 100644 --- a/core/auth/storage.ts +++ b/core/auth/storage.ts @@ -32,6 +32,26 @@ export interface SaveClientInformationOptions { issuer?: string; } +/** + * A server's revocation-relevant state, taken and deleted in one step by + * {@link OAuthStorage.takeRevocationSnapshot}. Values are as stored — unparsed + * — so validation can happen after the mutation rather than inside it. + */ +export interface RevocationSnapshot { + /** Credentials per authorization-server issuer (SEP-2352). */ + byIssuer: Record< + string, + { tokens?: unknown; clientInformation?: unknown } | undefined + >; + /** The legacy unkeyed slot, for entries predating issuer binding. */ + legacyTokens?: unknown; + legacyClientInformation?: unknown; + /** The static/preconfigured registration, if any. */ + preregisteredClientInformation?: unknown; + /** Cached authorization-server metadata, if discovery ever ran. */ + serverMetadata?: unknown; +} + export interface OAuthStorage { /** * Optional preload of persisted state into memory. Getters and setters load @@ -182,6 +202,27 @@ export interface OAuthStorage { */ clearDiscoveryState(serverUrl: string): Promise; + /** + * Atomically read everything RFC 7009 revocation needs, and clear the + * server's state in the same step (#2144). + * + * Split reads followed by a separate `clear` are a check-then-act: each + * `await` yields, and an OAuth completion landing in one of those gaps saves + * a fresh grant that the clear then deletes. Doing both against one + * synchronous view of the in-memory state closes that window, and the network + * requests are sent afterwards from the returned snapshot. + * + * The values are returned **unparsed**, exactly as stored. Schema validation + * is pure and belongs after the mutation — running it in between would + * reintroduce the `await` this exists to remove. + * + * ⚠️ In-process only. Nothing in this store takes a cross-process lock — + * every mutation here is a read-modify-write over a loaded snapshot — so a + * second Inspector writing concurrently can still lose an update. That is a + * property of the store, not of this method; `clear` alone has always had it. + */ + takeRevocationSnapshot(serverUrl: string): Promise; + /** * Clear all OAuth data for a server */ diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 45c91f6f7..5a1f1fa3a 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"; @@ -816,6 +817,7 @@ export class InspectorClient extends InspectorClientEventTarget { return Promise.resolve(); }, initialConfig: oauthConfig, + logger: this.logger, enterpriseManagedAuth: options.enterpriseManagedAuth, installEnterpriseManagedAuth: options.installEnterpriseManagedAuth, dispatchOAuthComplete: (detail) => @@ -6728,10 +6730,27 @@ 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 and revoke the grant at the + * authorization server (RFC 7009, #2144). + * + * The request is planned from the stored state, the state is cleared, and + * only then is the request sent — so the clear never waits on the network. + * + * 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 51e61af7e..873be9f91 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -13,6 +13,12 @@ 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 { + clearAndPlanRevocation, + executeOAuthRevocation, + 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 +77,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 +496,48 @@ export class OAuthManager { } } - async clearOAuthTokens(): Promise { + /** + * Drop this server's local OAuth state, and revoke the grant at the + * authorization server (RFC 7009). + * + * The state is taken and deleted in one atomic step, and the requests are + * sent afterwards from what was taken. Both halves matter: the requests need + * state the clear destroys, and the clear must not wait on the network, or a + * fresh authorization completing during a five-second revocation would be + * deleted by a clear reasoning about the grant it replaced. + * + * Best-effort by construction: every failure is reported through the returned + * outcome and the clear has already happened regardless, because forgetting + * 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(); - await this.oauthConfig.storage.clear(serverUrl); + // Takes the state and deletes it in ONE atomic storage step; separate + // reads followed by a separate clear are a check-then-act, and an OAuth + // completion landing between them would be destroyed by the clear. + const plan = await clearAndPlanRevocation({ + serverUrl, + storage: this.oauthConfig.storage, + enabled: options?.revoke, + }); this.oauthFlowState = null; this.pendingAuthorizationScope = undefined; + return executeOAuthRevocation(plan, { + fetchFn: this.params.effectiveAuthFetch, + logger: this.params.logger, + }); } async isOAuthAuthorized(): Promise { diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 45185e412..69439f6db 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -1424,6 +1424,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; @@ -1459,6 +1460,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 @@ -1572,7 +1578,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; } @@ -1884,6 +1890,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" && @@ -2016,6 +2031,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 d7937da97..e37fd0a25 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 17170ac3d..5755018cc 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,28 @@ export interface InspectorServerSettings { * the stored OAuth state is cleared. */ oauthRequestRefreshToken?: boolean; + /** + * Whether clearing this server's stored OAuth state also revokes the grant + * at the authorization server, per RFC 7009 (#2144). `undefined` (the + * default) means on; persisted as `oauth.revokeOnClear` only when explicitly + * off. + * + * The request is built from the stored state *before* the clear and sent + * *after* it, so the local delete never waits on the network — see + * `core/auth/revocation.ts`. + * + * 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 e45f0a8a2..ba3ce3c48 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,25 @@ 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 also **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. + +The order is **snapshot → clear → revoke**. Everything the request needs — the token, the client credentials, the discovered `revocation_endpoint` — is read out of the store first, because the clear empties it; but the clear then runs immediately rather than behind the network. Waiting would leave a window in which a *fresh* authorization could complete and then be deleted by a clear still reasoning about the grant it replaced. + +> **A grant is only revoked where the endpoint can be shown to belong to it.** The Inspector caches authorization-server metadata once per server, not per issuer, so a token bound to a *different* issuer than the cached document names is cleared without being revoked, and reported as such — sending it would disclose a bearer credential to a server that never minted it. The same applies to a grant stored before the Inspector began binding credentials to an issuer: "unbound" means its authorization server is unknown, not that the current endpoint owns it. Such an entry is left unrevoked (exactly what it did before this feature existed) and becomes revocable again the first time it is re-authorized. + +> **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 000000000..e0f055bd1 --- /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 000000000..25518aa27 --- /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 85494d1fb..d9799bffd 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 18e09f5a1..c154a6962 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 83e058ca6..40dc504d7 100644 --- a/test-servers/src/test-server-fixtures.ts +++ b/test-servers/src/test-server-fixtures.ts @@ -3002,6 +3002,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). @@ -3025,6 +3027,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 3b7a28e88..a93099c22 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, config); + } 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, @@ -622,7 +636,7 @@ function setupTokenEndpoint( // Generate access token const tokenScope = authCodeData.scope || config.scopesSupported?.[0] || "mcp"; - const accessToken = generateAccessToken(tokenScope); + const accessToken = generateAccessToken(tokenScope, client_id); const tokenExpiration = config.tokenExpirationSeconds || 3600; const response: { @@ -645,6 +659,7 @@ function setupTokenEndpoint( storeRefreshToken(refreshToken, { clientId: client_id, scope: authCodeData.scope, + accessTokens: new Set([accessToken]), }); } @@ -664,7 +679,10 @@ function setupTokenEndpoint( const tokenScope = refreshTokenData.scope || config.scopesSupported?.[0] || "mcp"; - const accessToken = generateAccessToken(tokenScope); + const accessToken = generateAccessToken(tokenScope, client_id); + // 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,137 @@ 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 **enforced**, not merely accepted. RFC 7009 §2.1 + * requires it of a confidential client, and a fixture that skipped the check + * would answer 200 to a request carrying no `Authorization` header at all — at + * which point the end-to-end test claiming to prove the Inspector authenticates + * correctly proves nothing. Both RFC 6749 §2.3.1 forms are accepted (Basic and + * the request body), as is a public client identifying itself by `client_id`. + */ +function setupRevocationEndpoint( + app: express.Application, + config: OAuthConfig, +): void { + app.post( + "/oauth/revoke", + express.urlencoded({ extended: true }), + async (req: Request, res: Response) => { + const token: unknown = req.body?.token; + if (typeof token !== "string" || token === "") { + res.status(400).json({ error: "invalid_request" }); + return; + } + + const clientId = await authenticateRevocationClient(req, config); + if (clientId === null) { + res + .status(401) + .set("WWW-Authenticate", 'Basic realm="revoke"') + .json({ error: "invalid_client" }); + return; + } + + // §2.1: only the client the token was issued to may revoke it. A token + // belonging to someone else is left alone — and still answered 200, per + // §2.2, since the response must not tell one client whether another's + // token exists. + const refreshTokenData = refreshTokens.get(token); + if (refreshTokenData) { + if (refreshTokenData.clientId === clientId) { + for (const accessToken of refreshTokenData.accessTokens) { + forgetAccessToken(accessToken); + } + refreshTokens.delete(token); + } + } else if (accessTokenClients.get(token) === clientId) { + forgetAccessToken(token); + } + + // §2.2: 200 whether or not the token was known to us. + res.status(200).end(); + }, + ); +} + +/** Drop an access token and everything recorded about it. */ +function forgetAccessToken(token: string): void { + accessTokens.delete(token); + accessTokenScopes.delete(token); + accessTokenClients.delete(token); +} + +/** + * Authenticate the caller of `/oauth/revoke` (RFC 7009 §2.1) and return the + * `client_id` it authenticated as, or `null` when it did not authenticate. + * + * Credentials may arrive either way RFC 6749 §2.3.1 allows — an `Authorization: + * Basic` header or `client_id`/`client_secret` in the form body — because the + * Inspector picks between them from the metadata, and a fixture that only read + * one would silently pass a request whose credentials went to the other place. + * + * A request naming no client at all is rejected: the Inspector always sends at + * least `client_id` once it holds any client information, so an unidentified + * request means it lost track of its credentials. + */ +async function authenticateRevocationClient( + req: Request, + config: OAuthConfig, +): Promise { + let clientId: string | undefined; + let clientSecret: string | undefined; + + const authorization = req.get("authorization"); + if (authorization?.startsWith("Basic ")) { + const decoded = Buffer.from( + authorization.slice("Basic ".length), + "base64", + ).toString("utf8"); + const separator = decoded.indexOf(":"); + if (separator === -1) return null; + // RFC 6749 §2.3.1: each half is form-urlencoded before the colon, so the + // server decodes each half after splitting on it. Decoding is what makes a + // credential containing a reserved character (`:` in the id, `%` or `/` in + // the secret) survive the round trip. + // + // A malformed escape makes `decodeURIComponent` throw, which Express would + // turn into a 500 — so a bad credential would be reported as a server + // fault rather than as the `invalid_client` 401 this endpoint means. + try { + clientId = decodeURIComponent(decoded.slice(0, separator)); + clientSecret = decodeURIComponent(decoded.slice(separator + 1)); + } catch { + return null; + } + } else { + const bodyId: unknown = req.body?.client_id; + const bodySecret: unknown = req.body?.client_secret; + if (typeof bodyId === "string") clientId = bodyId; + if (typeof bodySecret === "string") clientSecret = bodySecret; + } + + if (!clientId) return null; + const client = await findClient(clientId, config); + if (!client) return null; + // A client registered with a secret must present it; a public one must not be + // asked for one it never had. + const ok = + client.clientSecret === undefined || clientSecret === client.clientSecret; + return ok ? clientId : null; +} + /** * Set up Dynamic Client Registration endpoint */ @@ -732,6 +881,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 { @@ -745,6 +903,13 @@ const authorizationCodes = new Map(); const accessTokens = new Set(); /** Granted OAuth scope string per access token (space-separated). */ const accessTokenScopes = new Map(); +/** + * Owning `client_id` per access token. RFC 7009 §2.1 requires an authorization + * server to verify that a token being revoked was issued to the requesting + * client, and without this the fixture had no way to tell — so any registered + * client could revoke another's access token. (#2144) + */ +const accessTokenClients = new Map(); const refreshTokens = new Map(); const registeredClients = new Map(); @@ -871,10 +1036,11 @@ function getAuthorizationCode(code: string): AuthorizationCodeData | null { return data; } -function generateAccessToken(scope?: string): string { +function generateAccessToken(scope?: string, clientId?: string): string { const token = `test_access_token_${Date.now()}_${Math.random().toString(36).substring(7)}`; accessTokens.add(token); accessTokenScopes.set(token, scope?.trim() || "mcp"); + if (clientId !== undefined) accessTokenClients.set(token, clientId); return token; }