fix: report a terminal OAuth token-endpoint refusal instead of a dead retry

.claude/skills/test-servers/SKILL.md

@@ -83,6 +83,7 @@ usually looks like a missing capability rather than an error.
 | A tool result's `structuredContent` section | `structured-output-http.json` (legacy) |
 | RFC 6570 resource-template expansion | `rfc6570-templates-http.json` |
 | OAuth token revocation on clear | `oauth-revocation-http.json` (legacy) |
+| A token endpoint the SDK refuses  | `oauth-insecure-token-endpoint-http.json` (legacy) |
 | Cancelling a call mid-flight | `cancellation-modern-http.json` (modern) |
 
 ## Adding a config or preset

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

@@ -11,6 +11,7 @@ import type { ClientConfig } from "@inspector/core/client/types.js";
 import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js";
 import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
 import { EmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js";
+import { InsecureTokenEndpointError } from "@modelcontextprotocol/client";
 import { renderWithMantine, act, waitFor } from "../test/renderWithMantine";
 import { EMPTY_SETTINGS } from "../utils/serverSettingsDefaults";
 import { DEEP_LINK_SERVER_ID } from "../utils/deepLink";
@@ -258,6 +259,27 @@ const lastClient = (h: Harness): InspectorClient => {
   return client;
 };
 
+/**
+ * The last updater handed to `setReAuthBanner`, applied to a banner.
+ *
+ * Every terminal arm clears the banner with a **functional** update
+ * guarded on `serverId`, because these paths are asynchronous and a late
+ * continuation for one server must not erase a banner another raised in the
+ * meantime. The harness's setter is a spy, so the updater is never invoked for
+ * us — asserting it directly is what actually exercises the guard rather than
+ * merely reaching the line.
+ */
+const applyBannerUpdate = (
+  spy: ReturnType<typeof vi.fn>,
+  banner: { serverId: string; message: string } | null,
+) => {
+  const updater = spy.mock.calls.at(-1)?.[0] as unknown;
+  if (typeof updater !== "function") {
+    throw new Error("expected a functional setReAuthBanner update");
+  }
+  return (updater as (prev: unknown) => unknown)(banner);
+};
+
 const toastTitles = (): string[] =>
   notificationsMock.show.mock.calls.map((c) => String(c[0]?.title));
 
@@ -601,6 +623,128 @@ describe("useConnectionLifecycle", () => {
       expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a");
     });
 
+    it("reports an insecure token endpoint as terminal, without flagging the card", async () => {
+      // The terminal token-endpoint refusal (#2280). Asserted on the hook, not just the notice helper,
+      // because what makes this arm correct is its *position*: above
+      // `setFailedServerId` and above the generic toast. A helper-only test
+      // cannot see either of those go wrong.
+      connectSpy.mockRejectedValueOnce(
+        new InsecureTokenEndpointError(
+          "http://tenant.app.localhost:3300/token",
+        ),
+      );
+      const h = harness({ servers: [entry("a")] });
+
+      await act(async () => {
+        await h.api().onToggleConnection("a");
+      });
+
+      expect(toastTitles()).toContain("Token endpoint is not secure");
+      // The generic arm must not also fire — two notifications for one failure
+      // is how the raw SDK text would creep back in beside the good copy.
+      expect(toastTitles()).not.toContain('Failed to connect to "Server a"');
+      expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a");
+      // The clear is scoped: it drops this server's banner and spares another's.
+      expect(
+        applyBannerUpdate(h.spies.setReAuthBanner, {
+          serverId: "a",
+          message: "x",
+        }),
+      ).toBeNull();
+      expect(
+        applyBannerUpdate(h.spies.setReAuthBanner, {
+          serverId: "other",
+          message: "x",
+        }),
+      ).toMatchObject({ serverId: "other" });
+      // The real `connect()` sets status `"error"` and dispatches
+      // `statusChange` before rethrowing, which paints the card red and pins
+      // the monitoring sidebar open — presenting this as the failed connect
+      // attempt the notice says it is not. `connect` is mocked here, so the
+      // teardown is what this asserts; without it the client is left in that
+      // state.
+      expect(disconnectSpy).toHaveBeenCalled();
+    });
+
+    it("finds an insecure token endpoint wrapped under `cause` on the connect path", async () => {
+      // Era negotiation and the transport wrappers bury the rejection, so the
+      // shallow check this replaced would have missed exactly this shape.
+      connectSpy.mockRejectedValueOnce(
+        new Error("connect failed", {
+          cause: new InsecureTokenEndpointError("http://localhost.:8091/token"),
+        }),
+      );
+      const h = harness({ servers: [entry("a")] });
+
+      await act(async () => {
+        await h.api().onToggleConnection("a");
+      });
+
+      expect(toastTitles()).toContain("Token endpoint is not secure");
+      expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a");
+      expect(disconnectSpy).toHaveBeenCalled();
+    });
+
+    it("reports an insecure token endpoint raised by the 401 authorization attempt", async () => {
+      // The second of the two arms: `authenticate()` rejects rather than the
+      // opening handshake, which is the path a refresh takes.
+      connectSpy.mockRejectedValueOnce(unauthorized());
+      authenticateSpy.mockRejectedValueOnce(
+        new InsecureTokenEndpointError("http://localhost.:8091/token"),
+      );
+      const h = harness({ servers: [entry("a")] });
+
+      await act(async () => {
+        await h.api().onToggleConnection("a");
+      });
+
+      expect(toastTitles()).toContain("Token endpoint is not secure");
+      expect(toastTitles()).not.toContain(
+        'OAuth authorization failed for "Server a"',
+      );
+      expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a");
+      // The generic arm also records this as the connect error banner text;
+      // the terminal arm returns before that, so it must stay unset.
+      expect(h.api().connectErrorMessage).toBeUndefined();
+    });
+
+    it("reports a terminal refusal raised by the retried connect, without flagging the card", async () => {
+      // The satisfied-challenge retry still ends in a token exchange, so it can
+      // raise this on its own. Reporting it as a failed connect is doubly wrong
+      // here: the Inspector has just told the user the authorization worked
+      // (#2280).
+      connectSpy
+        .mockRejectedValueOnce(
+          new AuthRecoveryRequiredError(
+            new URL("https://as.example/authorize"),
+            { reason: "unauthorized" },
+          ),
+        )
+        .mockRejectedValueOnce(
+          new InsecureTokenEndpointError("http://localhost.:8091/token"),
+        );
+      checkSpy.mockResolvedValueOnce(true);
+      const h = harness({ servers: [entry("a")] });
+
+      await act(async () => {
+        await h.api().onToggleConnection("a");
+      });
+
+      expect(connectSpy).toHaveBeenCalledTimes(2);
+      expect(toastTitles()).toContain("Token endpoint is not secure");
+      expect(toastTitles()).not.toContain('Failed to connect to "Server a"');
+      expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("a");
+      expect(
+        applyBannerUpdate(h.spies.setReAuthBanner, {
+          serverId: "other",
+          message: "x",
+        }),
+      ).toMatchObject({ serverId: "other" });
+      expect(h.api().connectErrorMessage).toBeUndefined();
+      // The teardown the generic arm does is still required on this one.
+      expect(disconnectSpy).toHaveBeenCalled();
+    });
+
     it("retries the connect when the auth challenge is already satisfied", async () => {
       connectSpy.mockRejectedValueOnce(
         new AuthRecoveryRequiredError(new URL("https://as.example/authorize"), {
@@ -1211,6 +1355,39 @@ describe("useConnectionLifecycle", () => {
       );
     });
 
+    it("reports a terminal token-endpoint refusal from the banner action", async () => {
+      // The path a user reaches by *acting*: an ordinary re-auth banner, they
+      // click Re-authenticate, and the exchange is refused. The worst place to
+      // fall back to the raw SDK text, since they have just been told that
+      // retrying is the fix (#2280).
+      const h = harness({ servers: [entry("a")] });
+      await act(async () => {
+        await h.api().onToggleConnection("a");
+      });
+      const client = lastClient(h);
+      authenticateSpy.mockRejectedValue(
+        new InsecureTokenEndpointError("http://localhost.:8091/token"),
+      );
+      h.rerender({
+        servers: [entry("a")],
+        activeServerId: "a",
+        connectionStatus: "connected",
+        client,
+        reAuthBanner: { serverId: "a", message: "lapsed" },
+      });
+
+      await act(async () => {
+        h.api().onReauthenticateFromBanner();
+      });
+
+      await waitFor(() =>
+        expect(toastTitles()).toContain("Token endpoint is not secure"),
+      );
+      expect(toastTitles()).not.toContain(
+        'OAuth authorization failed for "Server a"',
+      );
+    });
+
     it("falls back to an unnamed failure toast for an unknown server", async () => {
       const h = harness({ servers: [entry("a")] });
       await act(async () => {

clients/web/src/hooks/useConnectionLifecycle.ts

@@ -30,8 +30,11 @@ import {
   getActiveEnterpriseManagedAuthIdp,
 } from "@inspector/core/client/types.js";
 import { isEmaClientNotConfiguredError } from "@inspector/core/auth/ema/clientConfigError.js";
+import { reportTerminalInsecureTokenEndpoint } from "../lib/insecureTokenEndpointNotice";
+import { findInsecureTokenEndpoint } from "@inspector/core/auth/insecureTokenEndpoint.js";
 import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
 import type { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js";
+import type { Dispatch, SetStateAction } from "react";
 import type { SessionRef } from "./useSessionRef";
 import type { FetchLogOptions } from "./useInspectorStores";
 import type { LastPersistedSettings } from "./useLastPersistedSettings";
@@ -172,7 +175,7 @@ export interface UseConnectionLifecycleOptions {
   prepareOAuthRedirect: (args: PrepareOAuthRedirectArgs) => void;
   finalizeExplicitDisconnect: () => void;
   reAuthBanner: ReAuthBannerState | null;
-  setReAuthBanner: (next: ReAuthBannerState | null) => void;
+  setReAuthBanner: Dispatch<SetStateAction<ReAuthBannerState | null>>;
 
   /** See `SessionResetSurface`. */
   sessionReset: SessionResetSurface;
@@ -637,6 +640,28 @@ export function useConnectionLifecycle({
           });
           return;
         }
+        // A token endpoint the SDK will not post credentials
+        // to. Terminal, so it gets a notice of its own rather than the generic
+        // "Failed to connect" toast, whose detail line would be the raw SDK
+        // text.
+        //
+        // The teardown is load-bearing, not tidiness. `connect()` sets its
+        // status to `"error"` and dispatches `statusChange` *before* rethrowing
+        // (it is not a connect-auth-recovery error), and `InspectorView` pins
+        // the monitoring sidebar open on that transition and paints the card
+        // red. Returning without it would present this as the failed connect
+        // attempt the notice explicitly says it is not. The `authenticate()`
+        // arm below already disconnects for the same reason.
+        if (findInsecureTokenEndpoint(err)) {
+          await client.disconnect().catch(() => {});
+          reportTerminalInsecureTokenEndpoint({
+            err,
+            serverId: id,
+            serverName: target.name,
+            setReAuthBanner,
+          });
+          return;
+        }
 
         // A 401 from an OAuth-protected server means we have no (valid) token
         // yet. Kick off the authorization-code flow: `authenticate()` runs
@@ -671,6 +696,24 @@ export function useConnectionLifecycle({
             // held. The fetch log survives a disconnect, so the Network
             // diagnostics this issue is about are unaffected.
             await client.disconnect().catch(() => {});
+            // The terminal token-endpoint refusal (#2280). The retried `connect()` above can raise the
+            // terminal refusal on its own — a satisfied challenge still ends in
+            // a token exchange — and reporting that as a failed connect attempt
+            // is doubly wrong here: the card goes red and the message is the
+            // raw SDK text, on the one path where the Inspector had just told
+            // the user the authorization *worked*. Placed after the teardown
+            // above, which this arm needs for the same reason the generic one
+            // does, and before the flag it must not set.
+            if (
+              reportTerminalInsecureTokenEndpoint({
+                err: recoveryErr,
+                serverId: id,
+                serverName: target.name,
+                setReAuthBanner,
+              })
+            ) {
+              return;
+            }
             setFailedServerId(id);
             const message =
               recoveryErr instanceof Error
@@ -722,6 +765,18 @@ export function useConnectionLifecycle({
               });
               return;
             }
+            // See the note on the handshake arm above (#2280). The
+            // disconnect already happened at the top of this catch.
+            if (
+              reportTerminalInsecureTokenEndpoint({
+                err: authErr,
+                serverId: id,
+                serverName: target.name,
+                setReAuthBanner,
+              })
+            ) {
+              return;
+            }
             // The connect attempt failed, same as any other handshake error —
             // flag the card (#1621) and, with it, open the monitoring sidebar
             // onto the OAuth requests that explain the failure (#2108). This
@@ -766,6 +821,7 @@ export function useConnectionLifecycle({
       setFailedServerId,
       prepareOAuthRedirect,
       finalizeExplicitDisconnect,
+      setReAuthBanner,
     ],
   );
 
@@ -962,6 +1018,23 @@ export function useConnectionLifecycle({
             authorizationUrl: authUrl,
           });
         } catch (err) {
+          // The terminal token-endpoint refusal (#2280), on the path a user reaches by *acting*:
+          // a connected session raises an ordinary re-auth banner, they click
+          // Re-authenticate, and the token exchange is refused. Without this
+          // the generic toast below reports it with the raw SDK text — the
+          // worst place to lose the guidance, since they have just been told
+          // retrying is the fix. No banner clear is needed: this callback
+          // already cleared it before starting.
+          if (
+            reportTerminalInsecureTokenEndpoint({
+              err,
+              serverId,
+              serverName: server?.name,
+              setReAuthBanner,
+            })
+          ) {
+            return;
+          }
           const message = err instanceof Error ? err.message : String(err);
           notifications.show({
             title: server