fix(auth): keep the failure path total, and align the docs with the new order (#2144)

Copilot review round 14. Two code fixes and a documentation sweep.

- `buildRevocationRequest` ran outside `revokeToken`'s best-effort try.
  `encodeURIComponent` throws on a lone UTF-16 surrogate — valid JSON, so it
  can reach there from a persisted client id or secret — and every caller has
  already cleared its local state by then, so a rejection instead of a `failed`
  outcome would break the documented guarantee. Construction is inside the try
  now, and the catch reports `params.endpoint` rather than a `url` that may not
  exist on that path.
- The fixture's Basic decode turned a malformed percent escape into an Express
  500, reporting a bad credential as a server fault rather than as the
  `invalid_client` 401 the endpoint means. Decoding failure is failed
  authentication.
- Ten places still said revocation happens *before* the clear, which the
  reorder in the previous commit made false: the settings-form description, the
  `--no-revoke` help text, both READMEs, `docs/mcp-server-configuration.md`,
  three JSDoc blocks, and the PR body. Swept, and the ordering is stated where
  it is load-bearing rather than asserted in passing. Also merged the two
  adjacent JSDoc blocks on `InspectorClient.clearOAuthTokens`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>

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.

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 <text\|json>`       | 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.
 

clients/cli/src/cli.ts

@@ -780,7 +780,7 @@ async function parseArgs(argv?: string[]): Promise<ParseResult> {
     )
     .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 <sec>",

clients/web/src/components/groups/ServerSettingsForm/ServerSettingsForm.tsx

@@ -930,7 +930,7 @@ export function ServerSettingsForm({
               />
               <Checkbox
                 label="Revoke tokens on clear"
-                description="Calls the authorization server's RFC 7009 revocation endpoint before clearing the stored OAuth state, so the grant ends when the session does instead of staying valid until it expires. Servers that advertise no revocation_endpoint are unaffected. Uncheck it to reproduce a client that disconnects still holding live tokens."
+                description="Calls the authorization server's RFC 7009 revocation endpoint when the stored OAuth state is cleared, so the grant ends when the session does instead of staying valid until it expires. Servers that advertise no revocation_endpoint are unaffected. Uncheck it to reproduce a client that disconnects still holding live tokens."
                 checked={settings.oauthRevokeOnClear ?? true}
                 onChange={handleRevokeOnClearChange}
               />

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.

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<typeof fetch>(),
+    });
+
+    expect(outcome).toMatchObject({ status: "failed", endpoint: REVOKE_URL });
+  });
+
   it("reports a network failure as failed", async () => {
     const fetchFn = vi.fn<typeof fetch>(async () => {
       throw new Error("connect ECONNREFUSED");

core/auth/revocation.ts

@@ -202,9 +202,14 @@ export interface RevokeTokenParams extends RevocationRequestParams {
 export async function revokeToken(
   params: RevokeTokenParams,
 ): Promise<TokenRevocationOutcome> {
-  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),
     };
   }

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

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;

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

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.
 

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;