Merge pull request #2236 from modelcontextprotocol/v2/fix/2221-oauth-refresh-unhandled-rejection

fix: terminate the OAuth connection-details refresh promise chain

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

@@ -505,6 +505,87 @@ describe("useOAuthRecovery", () => {
       expect(h.api().connectionInfoOAuth).toBeUndefined();
     });
 
+    it("clears already-loaded details when a refresh read rejects", async () => {
+      let read: () => Promise<unknown> = () =>
+        Promise.resolve({ tokens: { access_token: "t" } });
+      const client = fakeClient({ getOAuthState: vi.fn(() => read()) });
+      const h = harness({ servers: [entry("a")], activeServerId: "a", client });
+      await waitFor(() => expect(h.api().connectionInfoOAuth).toBeDefined());
+
+      // The panel reports the *current* state, so a failed read must clear the
+      // details it already holds rather than leave a stale answer on screen.
+      read = () => Promise.reject(new Error("backend down"));
+      await act(async () => {
+        client.emit("oauthComplete", {});
+      });
+      await waitFor(() => expect(h.api().connectionInfoOAuth).toBeUndefined());
+    });
+
+    it("ignores an earlier read that rejects after a newer one succeeded", async () => {
+      let failFirst: (reason: unknown) => void = () => {};
+      let call = 0;
+      const client = fakeClient({
+        getOAuthState: vi.fn((): Promise<unknown> => {
+          call += 1;
+          if (call === 1) {
+            return new Promise((_resolve, reject) => {
+              failFirst = reject;
+            });
+          }
+          return Promise.resolve({ tokens: { access_token: "t" } });
+        }),
+      });
+      const h = harness({ servers: [entry("a")], activeServerId: "a", client });
+      await waitFor(() =>
+        expect(client.getOAuthState).toHaveBeenCalledTimes(1),
+      );
+
+      await act(async () => {
+        client.emit("oauthComplete", {});
+      });
+      await waitFor(() => expect(h.api().connectionInfoOAuth).toBeDefined());
+
+      // The stale read settles last. Without the sequence guard its catch
+      // would clear the newer read's result.
+      await act(async () => {
+        failFirst(new Error("backend down"));
+      });
+      expect(h.api().connectionInfoOAuth).toBeDefined();
+    });
+
+    it("drops a rejected read that lands after the client was replaced", async () => {
+      let failStale: (reason: unknown) => void = () => {};
+      const stale = fakeClient({
+        getOAuthState: vi.fn(
+          () =>
+            new Promise((_resolve, reject) => {
+              failStale = reject;
+            }),
+        ),
+      });
+      const props: HarnessProps = {
+        servers: [entry("a")],
+        activeServerId: "a",
+        client: stale,
+      };
+      const h = harness(props);
+      await waitFor(() => expect(stale.getOAuthState).toHaveBeenCalled());
+
+      // Reconnect. The new client's details are what the panel must keep.
+      const fresh = fakeClient({
+        getOAuthState: vi
+          .fn()
+          .mockResolvedValue({ tokens: { access_token: "t" } }),
+      });
+      h.rerender({ ...props, client: fresh });
+      await waitFor(() => expect(h.api().connectionInfoOAuth).toBeDefined());
+
+      await act(async () => {
+        failStale(new Error("backend down"));
+      });
+      expect(h.api().connectionInfoOAuth).toBeDefined();
+    });
+
     it("drops a state read that lands after the session ended", async () => {
       let settle: (value: unknown) => void = () => {};
       const client = fakeClient({

clients/web/src/hooks/useOAuthRecovery.ts

@@ -440,13 +440,31 @@ export function useOAuthRecovery({
     }
     let cancelled = false;
 
+    // Reads are concurrent — an `oauthComplete` can start a second one while
+    // the first is still in flight — and nothing makes them settle in order.
+    // Only the newest read may write, so a slow earlier one cannot overwrite
+    // (or, on rejection, clear) a newer result.
+    let latest = 0;
+
     const refresh = (): void => {
-      void inspectorClient.getOAuthState().then((state) => {
+      const seq = ++latest;
-        if (cancelled) return;
+      // void: a synchronous useEffect body cannot await. The chain is
-        setConnectionInfoOAuthWhenConnected(
+      // terminated below, so the rejection is handled rather than discarded.
-          state ? oauthDetailsFromConnectionState(state) : undefined,
+      void inspectorClient
-        );
+        .getOAuthState()
-      });
+        .then((state) => {
+          if (cancelled || seq !== latest) return;
+          setConnectionInfoOAuthWhenConnected(
+            state ? oauthDetailsFromConnectionState(state) : undefined,
+          );
+        })
+        .catch(() => {
+          // The read failed (backend down, 401 on the API token, malformed
+          // stored state). Clear rather than keep the last successful read —
+          // a stale answer is indistinguishable from a fresh one in the panel.
+          if (cancelled || seq !== latest) return;
+          setConnectionInfoOAuthWhenConnected(undefined);
+        });
     };
 
     const onAmbientAuthChallenge = (): void => {