diff --git a/README.md b/README.md index 1f285a31..0fc04bb0 100644 --- a/README.md +++ b/README.md @@ -446,7 +446,7 @@ The value now rides the normalized `AuthChallenge` as a string — it has to be Add either server, connect and complete authorization, then use **Clear OAuth state and disconnect** (Server Settings → Authorization) and watch the Network tab. -- On `oauth-revocation-http.json` a `POST /oauth/revoke` goes out **before** the local state is dropped, naming the **refresh token**. RFC 7009 §2.1 asks the authorization server to invalidate the access tokens issued under the same grant, so one request ends both halves — the fixture implements that linkage, so re-sending the old bearer token to `/mcp` afterwards gets a 401. +- On `oauth-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. diff --git a/clients/cli/README.md b/clients/cli/README.md index 25117bb1..3c3ddc07 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -123,12 +123,14 @@ Options that specify the MCP server (catalog/config file, ad-hoc command/URL, en | `--strict` | With `--method tools/list`: report tool-schema portability problems in full (path, issue, suggested fix) on stderr, and exit `6` if any is error-severity. Without it, a one-line count is printed instead. See [Schema portability](#schema-portability---strict). | | `--format ` | Output format. `text` (default) pretty-prints the result. `json` emits a single JSON object on stdout (`{ "result": … }`, plus `{ "appInfo": … }` as a sibling key for App tools) with no banners, so the whole output pipes cleanly into `jq`. | | `--relogin` | Delete stored OAuth for this server URL from the shared store before connect; interactive login still only runs if the server requires auth. Requires an HTTP/SSE URL (rejected for stdio). Conflicts with `--stored-auth-only` / `--use-stored-auth` / `--wait-for-auth` / catalog short-circuits. | -| `--no-revoke` | With `--relogin`, skip the [RFC 7009](https://datatracker.ietf.org/doc/html/rfc7009) revocation request that would otherwise end the grant at the authorization server before the local state is deleted. The per-server `oauth.revokeOnClear` setting is the persistent form of the same opt-out; either one is enough to skip it. See [Revoking on `--relogin`](#revoking-on---relogin). | +| `--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** first, per [RFC 7009](https://datatracker.ietf.org/doc/html/rfc7009) ([#2144](https://github.com/modelcontextprotocol/inspector/issues/2144)) — otherwise the delete is invisible to the AS, and the access token, plus the refresh token when one was issued, stay valid there until they expire on their own. +`--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. diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index 4fefc309..90244835 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -780,7 +780,7 @@ async function parseArgs(argv?: string[]): Promise { ) .option( "--no-revoke", - "Requires --relogin. Skips the RFC 7009 revocation request that would otherwise end the grant at the authorization server before the local state is deleted. Also skipped when the server entry sets oauth.revokeOnClear to false.", + "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 ", diff --git a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx index 5343a46f..0ac753f9 100644 --- a/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx +++ b/clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx @@ -930,7 +930,7 @@ export function ServerSettingsForm({ /> diff --git a/clients/web/src/lib/clearServerOAuthState.ts b/clients/web/src/lib/clearServerOAuthState.ts index e4ac4068..b07e2db2 100644 --- a/clients/web/src/lib/clearServerOAuthState.ts +++ b/clients/web/src/lib/clearServerOAuthState.ts @@ -16,8 +16,8 @@ export interface ClearServerOAuthStateParams { /** Shared web OAuth store; required so clear hits the same blob as connect. */ oauthStorage: OAuthStorage; /** - * Whether to revoke the grant at the authorization server first (RFC 7009, - * #2144). Defaults to on. Two callers turn it off: a server whose settings + * 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. diff --git a/clients/web/src/test/core/auth/revocation.test.ts b/clients/web/src/test/core/auth/revocation.test.ts index e383f155..6b4f797a 100644 --- a/clients/web/src/test/core/auth/revocation.test.ts +++ b/clients/web/src/test/core/auth/revocation.test.ts @@ -282,6 +282,27 @@ describe("revokeToken", () => { }); }); + // 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"); diff --git a/core/auth/revocation.ts b/core/auth/revocation.ts index 853370cf..35859617 100644 --- a/core/auth/revocation.ts +++ b/core/auth/revocation.ts @@ -202,9 +202,14 @@ export interface RevokeTokenParams extends RevocationRequestParams { export async function revokeToken( params: RevokeTokenParams, ): Promise { - const { url, init } = buildRevocationRequest(params); 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 @@ -232,8 +237,10 @@ export async function revokeToken( }; } 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: url, + endpoint: params.endpoint, detail: err instanceof Error ? err.message : String(err), }; } diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 6e63c1a5..453415ad 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -6722,11 +6722,11 @@ export class InspectorClient extends InspectorClientEventTarget { } /** - * Clears OAuth tokens and client information - */ - /** - * Drop this server's stored OAuth state, revoking the grant at the - * authorization server first (RFC 7009, #2144). + * 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 diff --git a/core/mcp/oauthManager.ts b/core/mcp/oauthManager.ts index 65c11a78..ed0c7f56 100644 --- a/core/mcp/oauthManager.ts +++ b/core/mcp/oauthManager.ts @@ -497,8 +497,8 @@ export class OAuthManager { } /** - * Revoke the grant at the authorization server (RFC 7009), then drop the - * local OAuth state. + * Drop this server's local OAuth state, and revoke the grant at the + * authorization server (RFC 7009). * * The order is **snapshot → clear → revoke**, and it matters. Everything the * requests need is read out of the store first, because the clear empties it; diff --git a/core/mcp/types.ts b/core/mcp/types.ts index 841ee819..5755018c 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -833,11 +833,15 @@ export interface InspectorServerSettings { */ oauthRequestRefreshToken?: boolean; /** - * Whether clearing this server's stored OAuth state first revokes the grant + * 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 diff --git a/docs/mcp-server-configuration.md b/docs/mcp-server-configuration.md index 269ddb0e..a5b8fcd9 100644 --- a/docs/mcp-server-configuration.md +++ b/docs/mcp-server-configuration.md @@ -211,7 +211,7 @@ The setting suppresses the grant declaration and the SDK's scope augmentation > > **Clear stored OAuth state** (Server Settings → Authorization) clears the Inspector's **local** copies — the tokens and the client information — and, where the authorization server supports it, revokes the grant there too (see `oauth.revokeOnClear` below), which settles the first. It does **not** touch the registration. For that, what happens next depends on how the client was obtained. A **dynamically registered** client is registered afresh on the next connect, and the new registration declares only `authorization_code`; the old one still exists at the AS, unused. A **preconfigured `oauth.clientId`** is reused as-is, so changing what that client declares is done at the authorization server, not here. -`oauth.revokeOnClear` (default `true`) controls whether clearing this server's stored OAuth state first **revokes the grant at the authorization server**, per [RFC 7009](https://datatracker.ietf.org/doc/html/rfc7009). Uncheck **Revoke tokens on clear** in Server Settings → Authorization to turn it off; only `false` is written to disk, so a server that never touched the setting keeps a minimal entry ([#2144](https://github.com/modelcontextprotocol/inspector/issues/2144)). +`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. diff --git a/test-servers/src/test-server-oauth.ts b/test-servers/src/test-server-oauth.ts index 9f763bab..a93099c2 100644 --- a/test-servers/src/test-server-oauth.ts +++ b/test-servers/src/test-server-oauth.ts @@ -802,8 +802,16 @@ async function authenticateRevocationClient( // 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. - clientId = decodeURIComponent(decoded.slice(0, separator)); - clientSecret = decodeURIComponent(decoded.slice(separator + 1)); + // + // 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;