fix(auth): close the fail-open issuer check and the remaining clear races (#2144)

Copilot review rounds 8 and 9.

- A cached metadata document with no `issuer` was letting every issuer-bound
  token go to its single revocation endpoint. Absence establishes nothing, so
  that is now a mismatch: an issuer-bound grant must match, and only a legacy
  unkeyed grant — which nothing binds to a different authorization server —
  proceeds without the comparison. This was a credential-disclosure risk, not
  just a correctness one.
- The ctx-less grant read was skipped whenever it carried an issuer stamp, on
  the assumption that slot had already been enumerated. If `listIssuers` fails
  there is no enumerated slot behind the stamp, so the one readable grant went
  unrevoked while `clear` deleted it. Both reads now share one issuer+token
  key, and the stamped issuer is retained rather than discarded.
- `AuthTab`'s repeat guard read `clearState`, which is not updated until React
  re-renders, so two `s` events in one input turn both started a clear against
  the same store entry. It is a ref now, set before the call.
- A rejected clear left `ownClearRef` set — `handleClearOAuth` never reached
  its `oauthRevision` bump — so the next unrelated revision was swallowed and
  the failure banner stranded. Cleared on that path.
- The web `clearServerOAuthAndDisconnect` had the staleness the TUI path was
  already fixed for: `isActive`/`inspectorClient` are captured before a
  five-second await, so a switch during it disconnected the session the user
  had just moved to and ran the session-wide cleanup against it. Revalidated
  against `sessionRef` after each await.

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>

clients/tui/__tests__/AuthTab.test.tsx

@@ -138,6 +138,55 @@ describe("AuthTab", () => {
     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<void>(() => {}));
+    const { stdin } = render(
+      <AuthTab
+        {...baseProps}
+        onClearOAuth={onClearOAuth}
+        inspectorClient={null}
+        oauthStatus="idle"
+        oauthMessage={null}
+        focused
+      />,
+    );
+    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<void>>()
+      .mockRejectedValueOnce(new Error("nope"))
+      .mockResolvedValue(undefined);
+    const { stdin } = render(
+      <AuthTab
+        {...baseProps}
+        onClearOAuth={onClearOAuth}
+        inspectorClient={null}
+        oauthStatus="idle"
+        oauthMessage={null}
+        focused
+      />,
+    );
+    await tick();
+    stdin.write("s");
+    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.

clients/tui/src/components/AuthTab.tsx

@@ -100,6 +100,13 @@ export function AuthTab({
    * — 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
@@ -115,6 +122,7 @@ export function AuthTab({
   // banner: neither belongs to the server now on screen.
   useEffect(() => {
     clearAttemptRef.current++;
+    clearInFlightRef.current = false;
     setClearState("idle");
     setClearFailure(null);
     setLastClearDisconnected(false);
@@ -207,8 +215,9 @@ export function AuthTab({
       } 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 this state.
+        // first is still running except by the pending line below.
-        if (clearState === "clearing") return;
+        if (clearInFlightRef.current) return;
+        clearInFlightRef.current = true;
         const attempt = ++clearAttemptRef.current;
         const attemptServer = serverName;
         setLastClearDisconnected(isLiveConnection);
@@ -222,13 +231,21 @@ export function AuthTab({
           serverNameRef.current === attemptServer;
         void Promise.resolve(onClearOAuth()).then(
           () => {
+            clearInFlightRef.current = false;
             if (current()) setClearState("cleared");
           },
           (err: unknown) => {
+            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;
             if (!current()) return;
             setClearFailure(err instanceof Error ? err.message : String(err));
             setClearState("failed");

clients/web/src/hooks/useOAuthRecovery.test.tsx

@@ -1368,6 +1368,47 @@ describe("useOAuthRecovery", () => {
       );
     });
 
+    // #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<void>;
+      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();
+    });
+
     it("clears the resume snapshot on an explicit disconnect", () => {
       writeOAuthResumeSnapshot({
         version: 1,

clients/web/src/hooks/useOAuthRecovery.ts

@@ -1254,9 +1254,23 @@ export function useOAuthRecovery({
   const clearServerOAuthAndDisconnect = useCallback(
     async (server: ClearableServer) => {
       const isActive = server.id === activeServerId;
+      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.
+      const stillTargetsActiveSession = (): boolean =>
+        isActive && sessionRef.current.activeServerId === server.id;
+
       const { cleared, revocation } = await clearServerOAuthState({
         config: server.config,
-        inspectorClient: isActive ? inspectorClient : null,
+        inspectorClient: client,
         isActiveConnection: isActive,
         oauthStorage: webOAuthStorage,
         revoke: server.settings?.oauthRevokeOnClear !== false,
@@ -1264,14 +1278,17 @@ export function useOAuthRecovery({
       });
       if (!cleared) return;
 
-      if (isActive && inspectorClient) {
+      if (client && stillTargetsActiveSession()) {
         try {
-          await inspectorClient.disconnect();
+          await client.disconnect();
         } finally {
-          setConnectionInfoOAuthWhenConnected(undefined);
+          // Revalidate after the second await for the same reason.
-          finalizeExplicitDisconnect();
+          if (stillTargetsActiveSession()) {
+            setConnectionInfoOAuthWhenConnected(undefined);
+            finalizeExplicitDisconnect();
+          }
         }
-      } else {
+      } else if (!isActive) {
         clearOAuthResumeOnExplicitDisconnect();
       }
 
@@ -1286,6 +1303,7 @@ export function useOAuthRecovery({
     [
       activeServerId,
       inspectorClient,
+      sessionRef,
       webOAuthStorage,
       finalizeExplicitDisconnect,
       clearOAuthResumeOnExplicitDisconnect,

clients/web/src/test/core/auth/revocation.test.ts

@@ -476,9 +476,11 @@ describe("revokeStoredOAuthTokens", () => {
   // 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 () => {
+    // The metadata names the same issuer as the slot below, so BOTH grants are
+    // genuinely revocable — otherwise this would pass for the wrong reason.
     await storage.saveServerMetadata(
       SERVER_URL,
-      metadata({ issuer: undefined }),
+      metadata({ issuer: "https://as-a.example.com" }),
     );
     // A legacy unkeyed grant...
     await storage.saveTokens(SERVER_URL, {
@@ -507,15 +509,96 @@ describe("revokeStoredOAuthTokens", () => {
     expect(fetchFn).toHaveBeenCalledTimes(2);
   });
 
+  // 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<typeof fetch>();
+
+    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",
+    );
+  });
+
+  // A legacy unkeyed grant is not bound to any authorization server, so there
+  // is nothing to contradict — it proceeds without the comparison.
+  it("still revokes a legacy grant when the 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",
+    });
+    const fetchFn = vi.fn<typeof fetch>(
+      async () => new Response(null, { status: 200 }),
+    );
+
+    await expect(
+      revokeStoredOAuthTokens({ serverUrl: SERVER_URL, storage, fetchFn }),
+    ).resolves.toMatchObject({ status: "revoked" });
+    expect(fetchFn).toHaveBeenCalledTimes(1);
+  });
+
+  // If `listIssuers` fails there is no enumerated slot behind the stamp, so
+  // skipping a stamped ctx-less read would leave the one grant the store can
+  // still produce unrevoked — while `clear` deleted it anyway.
+  it("revokes the active grant even when the issuer list cannot be read", async () => {
+    await storage.saveServerMetadata(
+      SERVER_URL,
+      metadata({ issuer: "https://as-a.example.com" }),
+    );
+    await storage.saveTokens(
+      SERVER_URL,
+      { access_token: "a", token_type: "Bearer", refresh_token: "r-active" },
+      { issuer: "https://as-a.example.com" },
+    );
+    vi.spyOn(storage, "listIssuers").mockRejectedValue(new Error("no list"));
+    const fetchFn = vi.fn<typeof fetch>(
+      async () => new Response(null, { status: 200 }),
+    );
+
+    const outcome = await revokeStoredOAuthTokens({
+      serverUrl: SERVER_URL,
+      storage,
+      fetchFn,
+    });
+
+    expect(
+      new URLSearchParams(String(fetchFn.mock.calls[0]![1]!.body)).get("token"),
+    ).toBe("r-active");
+    // The listing failure is still surfaced — it outranks the success.
+    expect(outcome).toMatchObject({ status: "failed" });
+  });
+
   // 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 () => {
-    await storage.saveServerMetadata(
+    await storage.saveServerMetadata(SERVER_URL, metadata({ issuer: "a" }));
-      SERVER_URL,
+    // All three bound to the issuer the cached metadata describes, so all three
-      metadata({ issuer: undefined }),
+    // are genuinely revocable and the budget is what stops them.
-    );
     vi.spyOn(storage, "listIssuers").mockResolvedValue(["a", "b", "c"]);
     vi.spyOn(storage, "getIssuerTokens").mockImplementation(
       async (_url: string, issuer: string) => ({

core/auth/revocation.ts

@@ -405,30 +405,29 @@ async function collectGrants(
     const revocable = selectRevocableToken(tokens);
     if (!revocable) return;
 
-    if (issuer === undefined) {
+    // `getTokens` stamps the resolved issuer onto a value it took from a
-      // `getTokens` stamps the resolved issuer onto a value it took from a
+    // byIssuer slot and leaves an unkeyed one unstamped (see `withIssuer` in
-      // byIssuer slot and leaves an unkeyed one unstamped (see `withIssuer` in
+    // oauth-storage.ts — the stamp is the key it came from), so the ctx-less
-      // oauth-storage.ts — the stamp is the key it came from). That, not the
+    // read reports which issuer its answer belongs to, if any.
-      // token's *value*, is what says this read duplicates a slot already
+    const grantIssuer =
-      // collected: the active slot can hold client information without a token,
+      issuer ?? (tokens as { issuer?: string } | undefined)?.issuer;
-      // in which case this falls back to the legacy grant, and a coincidental
+    // Keyed by issuer AND token: a token means nothing outside the
-      // value collision with some other issuer would otherwise drop it
+    // authorization server that minted it, so two issuers minting the same
-      // unrevoked and unreported.
+    // opaque value are two grants. Deduping the ctx-less read on its *stamp*
-      const fromSlot = (tokens as { issuer?: string } | undefined)?.issuer;
+    // alone would be wrong too — if `listIssuers` failed there is no enumerated
-      if (fromSlot !== undefined) return;
+    // slot behind the stamp, and skipping would leave the one readable grant
-    } else {
+    // unrevoked while `clear` deleted it.
-      const key = `${issuer}\u0000${revocable.token}`;
+    const key = `${grantIssuer ?? "\u0000legacy"}\u0000${revocable.token}`;
-      if (seenKeys.has(key)) return;
+    if (seenKeys.has(key)) return;
-      seenKeys.add(key);
+    seenKeys.add(key);
-    }
 
     grants.push({
-      issuer,
+      issuer: grantIssuer,
       ...revocable,
       clientInformation: await resolveClientInformation(
         storage,
         serverUrl,
-        issuer,
+        grantIssuer,
       ),
     });
   };
@@ -541,17 +540,20 @@ async function computeOutcome(
     for (const grant of 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 hand a credential to a server
+      // issuer's token to *this* endpoint would disclose a bearer credential to
-      // that never minted it — worse than not revoking. So say plainly that the
+      // a server that never minted it — worse than not revoking. So an
-      // grant is being dropped unrevoked rather than doing either silently.
+      // issuer-bound grant must be able to PROVE the endpoint is its own, which
-      if (
+      // means a metadata document carrying no `issuer` is a mismatch rather
-        grant.issuer !== undefined &&
+      // than a free pass: absence establishes nothing. Only a legacy unkeyed
-        metadata.issuer !== undefined &&
+      // grant proceeds without the comparison, since nothing binds it to a
-        grant.issuer !== metadata.issuer
+      // different authorization server in the first place.
-      ) {
+      if (grant.issuer !== undefined && metadata.issuer !== grant.issuer) {
         outcomes.push({
           status: "failed",
-          detail: `the cached authorization-server metadata is for ${metadata.issuer}, so the grant bound to ${grant.issuer} was cleared without revocation`,
+          detail:
+            metadata.issuer === 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 ${metadata.issuer}, so the grant bound to ${grant.issuer} was cleared without revocation`,
         });
         continue;
       }