fix(auth): address Copilot review round 2 on the RFC 7009 revocation (#2144)

- `revokeStoredKeys`' pre-read of `getTokens` sat outside the best-effort
  catch. `getTokens` parses through `OAuthTokensSchema`, so a persisted token
  that no longer validates rejected and abandoned the local delete `--relogin`
  promises — over a grant that could not have been revoked anyway. Caught and
  retained as a failed outcome; the clear proceeds.
- The multi-key bookkeeping was wrong twice. A token is now marked spent only
  on a `revoked` outcome, so a failed or unsupported attempt no longer skips a
  duplicate entry under the other key that may hold the credentials or metadata
  that would have worked. And a failure now outranks an earlier success for
  reporting, so a stale grant left live cannot be hidden behind the first key's
  200.
- `--no-revoke` is now rejected without `--relogin`, ahead of the short-circuit
  returns, matching the parser's existing stance on accepted-but-inert flags
  (`--strict`). On its own it reads as "this run will not revoke anything",
  true only because nothing was being cleared.
- Extracted the App draft merge as `utils/serverWithDraftSettings`, so the rule
  has a test. The hook tests receive whatever they are handed and cannot tell a
  draft from a persisted entry, and `App.test.tsx`'s settings harness connects
  a stdio server where the OAuth section never renders.

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>

Author: cliffhall

clients/cli/__tests__/clear-stored-auth-for-relogin.test.ts

@@ -103,6 +103,44 @@ describe("clearStoredAuthForRelogin", () => {
       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: "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 entry = (refresh: string) => ({
+        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 before deleting it", async () => {
       const file = seed();
       const fetchSpy = vi
@@ -249,6 +287,74 @@ describe("clearStoredAuthForRelogin", () => {
       }
     });
 
+    // A token the AS never accepted is not spent, so the other key — which may
+    // carry the credentials or metadata that would have worked — must still be
+    // tried rather than skipped as a duplicate.
+    it("does not treat a failed attempt as having ended the grant", 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.
+    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": {
+              // No `token_type` — fails OAuthTokensSchema.
+              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<string, unknown>;
+      };
+      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");

clients/cli/__tests__/relogin-revocation.test.ts

@@ -185,6 +185,20 @@ describe("--relogin token revocation", () => {
     }
   });
 
+  // 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.

clients/cli/src/clear-stored-auth-for-relogin.ts

@@ -70,13 +70,19 @@ export async function clearStoredAuthForRelogin(
  * under the other spelling is still a live grant at the authorization server,
  * and deleting it locally without revoking is the leak this whole change
  * closes. Two keys holding the *same* token are one grant, so the duplicate is
- * skipped rather than producing a second request for something already ended.
+ * skipped rather than producing a second request for something already ended —
+ * but only once one has actually been *revoked*, since a failed or unsupported
+ * attempt has ended nothing and the other key may hold the credentials that
+ * would have worked.
  *
- * The reported outcome is the first key's that was not "nothing to do", which
- * with `keys` in `findStoredServerState` precedence means the grant the CLI
- * would actually have connected with. When no key holds a token, the last
- * "nothing to do" answer is returned so the caller can still tell "no tokens"
- * from "this authorization server advertises no revocation endpoint".
+ * Reporting prefers a **failure** over any earlier success — a grant still live
+ * at the authorization server is what the user needs to hear about, and an
+ * earlier success would otherwise silence the warning. Failing that, it is the
+ * first key's outcome that was not "nothing to do", which with `keys` in
+ * `findStoredServerState` precedence means the grant the CLI would actually
+ * have connected with. When no key holds a token, the last "nothing to do"
+ * answer is returned so the caller can still tell "no tokens" from "this
+ * authorization server advertises no revocation endpoint".
  */
 async function revokeStoredKeys(
   storage: NodeOAuthStorage,
@@ -87,7 +93,21 @@ async function revokeStoredKeys(
   let reported: TokenRevocationOutcome | undefined;
   let lastSkip: TokenRevocationOutcome | undefined;
   for (const key of new Set(keys)) {
-    const token = selectRevocableToken(await storage.getTokens(key))?.token;
+    // This read is OUTSIDE `revokeStoredOAuthTokens`'s own best-effort catch,
+    // so it needs its own: `getTokens` parses through `OAuthTokensSchema` and
+    // rejects on a persisted token that no longer validates. Letting that
+    // escape would abandon the *local* delete `--relogin` promises, over a
+    // grant we could not have revoked anyway.
+    let token: string | undefined;
+    try {
+      token = selectRevocableToken(await storage.getTokens(key))?.token;
+    } catch (err) {
+      reported ??= {
+        status: "failed",
+        detail: err instanceof Error ? err.message : String(err),
+      };
+      continue;
+    }
     if (token !== undefined && revokedTokens.has(token)) continue;
     const outcome = await revokeStoredOAuthTokens({
       serverUrl: key,
@@ -98,8 +118,21 @@ async function revokeStoredKeys(
       lastSkip = outcome;
       continue;
     }
-    if (token !== undefined) revokedTokens.add(token);
-    reported ??= outcome;
+    // Only a token the authorization server actually accepted counts as spent.
+    // Marking one revoked on a `failed` or `no_endpoint` outcome would skip a
+    // duplicate entry under the other key that may carry usable metadata or
+    // credentials — the one chance left to end that grant.
+    if (token !== undefined && outcome.status === "revoked") {
+      revokedTokens.add(token);
+    }
+    // A failure outranks an earlier success for reporting: a grant that is
+    // still live is what the user needs to hear about, and reporting the
+    // success would print no warning while a stale grant survives.
+    if (outcome.status === "failed") {
+      if (reported?.status !== "failed") reported = outcome;
+    } else {
+      reported ??= outcome;
+    }
   }
   return reported ?? lastSkip;
 }

clients/cli/src/cli.ts

@@ -780,7 +780,7 @@ async function parseArgs(argv?: string[]): Promise<ParseResult> {
     )
     .option(
       "--no-revoke",
-      "With --relogin, skip the RFC 7009 revocation request that would otherwise end the grant at the authorization server before the local state is deleted. Also skipped when the server entry sets oauth.revokeOnClear to false.",
+      "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.",
     )
     .option(
       "--wait-for-auth <sec>",
@@ -867,6 +867,15 @@ async function parseArgs(argv?: string[]): Promise<ParseResult> {
     }
   }
 
+  // `--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

clients/web/src/App.tsx

@@ -92,6 +92,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";
@@ -1327,16 +1328,10 @@ function App() {
   const handleClearStoredOAuthFromSettings = useCallback(() => {
     if (!settingsModalTarget) return;
     // Clear from *inside* the settings modal, so the draft is what the user is
-    // looking at: `settingsModalTarget` comes from the persisted `servers`
-    // list, and edits only reach it after the save debounce. Reading it would
-    // mean toggling "Revoke tokens on clear" and immediately clearing used the
-    // previous value — revoking despite an opt-out, or skipping despite an
-    // opt-in (#2144). The draft is the live answer; `settingsModalValue` falls
-    // back to the persisted entry whenever there is no draft.
-    void clearServerOAuthAndDisconnect({
-      ...settingsModalTarget,
-      settings: settingsDraft ?? settingsModalTarget.settings,
-    });
+    // looking at rather than what the debounced save has persisted (#2144).
+    void clearServerOAuthAndDisconnect(
+      serverWithDraftSettings(settingsModalTarget, settingsDraft),
+    );
   }, [settingsModalTarget, settingsDraft, clearServerOAuthAndDisconnect]);
 
   const onSettingsModalClose = useCallback(() => {

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> = {},
+): InspectorServerSettings => ({
+  headers: [],
+  env: [],
+  metadata: {},
+  connectionTimeout: 0,
+  requestTimeout: 0,
+  taskTtl: 60000,
+  maxFetchRequests: 1000,
+  roots: [],
+  ...over,
+});
+
+const entry = (over: Partial<ServerEntry> = {}): 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);
+  });
+});

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<ServerEntry, "settings">,
+>(entry: T, draft: InspectorServerSettings | null | undefined): T {
+  return draft == null ? entry : { ...entry, settings: draft };
+}