modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix(web): terminate the five void-discarded OAuth recovery promises

Browse source 
Each of the five sites in `useOAuthRecovery` discarded a promise with
`void`, satisfying `@typescript-eslint/no-floating-promises` but not the
narrower rule AGENTS.md states — `void` only where the callee owns its
failures or the caller cannot await, with a comment saying which.

Each now picks a user-visible outcome:

- `prepareOAuthRedirect` clears the resume snapshot (so the next load
  cannot read a redirect that never started as an *abandoned* one),
  flags the server and raises the re-auth banner.
- the `authChallengeInteractive` handler flags and banners, where before
  a rejection drew no UI response at all.
- `resumePendingReauth` restores the pending slot and toasts, so the
  next trigger retries the deferred recovery instead of losing it.
- the `/oauth/callback` effect flags and banners what falls between its
  existing arms — the client rebuild and the post-resume scope check.
- `handleStepUpAuthorize`'s EMA branch reports exactly as its `failed`
  outcome arm does; the stored retry gets its own catch, since by then
  the permissions leg succeeded and the failure is the command's.

The two surviving `void`s carry the justification AGENTS.md asks for.

Closes #2165

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Quk1hBUtyhkXowwMXzAnAY
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-29 01:34:40 -04:00
commit f74aa757d2
5 changed files with 290 additions and 5 deletions
Download patch file Download diff file Expand all files Collapse all files

163
clients/web/src/hooks/useOAuthRecovery.test.tsx
View file

@ -570,6 +570,31 @@ describe("useOAuthRecovery", () => {
expect(active.beginInteractiveAuthorization).not.toHaveBeenCalled();
});
it("clears the snapshot and reports a redirect that never started", async () => {
// The navigation is what the snapshot describes, so a rejection here
// must not leave one behind: the next load would read it as an
// *abandoned* redirect and offer that (wrong) diagnosis (#2165).
const client = fakeClient({
beginInteractiveAuthorization: vi
.fn()
.mockRejectedValue(new Error("provider state unreadable")),
});
const h = harness({ servers: [entry("a")], activeServerId: "a", client });
await act(async () => {
h.api().prepareOAuthRedirect({
serverId: "a",
authKind: "reauth",
authorizationUrl: AUTH_URL,
});
});
await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a"));
expect(h.api().reAuthBanner?.message).toContain(
"provider state unreadable",
);
expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a");
expect(window.sessionStorage.getItem(OAUTH_RESUME_KEY)).toBeNull();
});
it("tolerates having no client at all", () => {
const h = harness({ servers: [entry("a")], client: null });
act(() =>
@ -845,6 +870,29 @@ describe("useOAuthRecovery", () => {
expect(client.beginInteractiveAuthorization).not.toHaveBeenCalled();
});
it("reports a challenge whose recovery check rejects", async () => {
// A listener cannot be awaited, so without the handler's own catch this
// challenge draws no UI response whatsoever (#2165).
const client = fakeClient({
checkAuthChallengeSatisfied: vi
.fn()
.mockRejectedValue(new Error("remote auth state unreachable")),
});
const h = harness({ servers: [entry("a")], activeServerId: "a", client });
await act(async () => {
client.emit("authChallengeInteractive", {
challenge: challenge(),
authorizationUrl: AUTH_URL,
});
});
await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a"));
expect(h.api().reAuthBanner?.message).toContain(
"remote auth state unreachable",
);
expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a");
expect(client.beginInteractiveAuthorization).not.toHaveBeenCalled();
});
it("raises the banner on an oauthError event", async () => {
const client = fakeClient();
const h = harness({ servers: [entry("a")], activeServerId: "a", client });
@ -987,6 +1035,36 @@ describe("useOAuthRecovery", () => {
expect(client.handleAuthChallenge).toHaveBeenCalledTimes(1);
});
it("restores the deferred slot when the resume itself rejects", async () => {
// The slot is cleared to keep two triggers from racing, not because the
// recovery was delivered — so a rejection has to put it back rather than
// dropping the deferred recovery silently (#2165).
const client = fakeClient({
handleAuthChallenge: vi
.fn()
.mockRejectedValue(new Error("token endpoint unreachable")),
});
const h = harness({ servers: [entry("a")], activeServerId: "a", client });
await defer(client, h);
await act(async () => {
becomeVisible();
});
await waitFor(() =>
expect(client.handleAuthChallenge).toHaveBeenCalledTimes(1),
);
expect(toastTitles()).toContain("Could not continue authorization");
expect(toastWith("token endpoint unreachable")).toBeTruthy();
// Restored, so the next trigger retries it.
await act(async () => {
becomeVisible();
});
await waitFor(() =>
expect(client.handleAuthChallenge).toHaveBeenCalledTimes(2),
);
});
it("does not resume while disconnected, and resumes on the reconnect", async () => {
const client = fakeClient({
handleAuthChallenge: vi.fn().mockResolvedValue({ kind: "satisfied" }),
@ -1167,6 +1245,43 @@ describe("useOAuthRecovery", () => {
);
});
it("flags and banners a client rebuild that throws", async () => {
// Between the arms that have their own handling (#2165): by the time
// `setupClientForServer` runs, the callback URL and the one-shot
// snapshot are both spent, so there is nothing left to retry with.
snapshot();
onCallbackUrl(`?code=abc&state=${AUTH_ID}`);
const h = harness({
servers: [entry("a")],
setupClient: () => {
throw new Error("client construction failed");
},
});
await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a"));
expect(h.api().reAuthBanner?.message).toContain(
"client construction failed",
);
expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a");
});
it("flags and banners a post-resume scope check that rejects", async () => {
snapshot({
authKind: "step_up",
authChallenge: { reason: "insufficient_scope" },
});
const client = fakeClient({
checkAuthChallengeSatisfied: vi
.fn()
.mockRejectedValue(new Error("scope check unreachable")),
});
const h = callbackHarness(`?code=abc&state=${AUTH_ID}`, {}, client);
await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a"));
expect(h.api().reAuthBanner?.message).toContain(
"scope check unreachable",
);
expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a");
});
it("restores the shell, rebuilds the client and completes", async () => {
snapshot({ remoteSessionId: "remote-9" });
const client = fakeClient();
@ -1593,6 +1708,54 @@ describe("useOAuthRecovery", () => {
);
});
it("routes an EMA step-up that rejects back to the panel that asked", async () => {
// `StepUpAuthModal` calls this handler as `void onAuthorize()`, so
// without the catch the rejection reaches nobody (#2165).
const client = fakeClient({
handleAuthChallenge: vi
.fn()
.mockRejectedValue(new Error("IdP unreachable")),
});
const h = harness({
servers: [entry("a", {}, true)],
activeServerId: "a",
client,
});
await openStepUp(h, "prompt");
await act(async () => {
await h.api().handleStepUpAuthorize();
});
expect(h.spies.setSourceScopedError).toHaveBeenCalledWith(
"prompt",
expect.stringContaining("IdP unreachable"),
);
expect(toastTitles()).toContain("Organization permissions");
});
it("reports a failed retry as the command's failure, not the step-up's", async () => {
const client = fakeClient({
handleAuthChallenge: vi.fn().mockResolvedValue({ kind: "satisfied" }),
});
const h = harness({
servers: [entry("a", {}, true)],
activeServerId: "a",
client,
});
const retry = vi.fn().mockRejectedValue(new Error("tools/call failed"));
await openStepUp(h, "tool", retry);
await act(async () => {
await h.api().handleStepUpAuthorize();
});
// The permissions leg succeeded, so this is the retried command's
// failure and is reported as such.
expect(toastTitles()).toContain("Permissions updated");
expect(toastTitles()).toContain("Retry failed");
expect(h.spies.setSourceScopedError).toHaveBeenCalledWith(
"tool",
"tools/call failed",
);
});
it("cancels back to the panel that asked", async () => {
const client = fakeClient();
const h = harness({ servers: [entry("a")], activeServerId: "a", client });

106
clients/web/src/hooks/useOAuthRecovery.ts
View file

@ -55,6 +55,7 @@ import { INSPECTOR_SERVERS_TAB } from "../utils/inspectorTabs";
import type { PendingReauth } from "../utils/pendingReauth";
import {
authRecoveryRestoredMessage,
authRecoveryRetryFailedMessage,
isReAuthBannerReason,
issuerBindingFailureCopy,
lostAuthorizationStateActionLabel,
@ -538,9 +539,30 @@ export function useOAuthRecovery({
...(authKind === "step_up" && authChallenge && { authChallenge }),
...(recoverySource && { recoverySource }),
});
void oauthClient?.beginInteractiveAuthorization(authorizationUrl);
// The navigation is what this whole callback exists to reach, so a
// rejection here means the flow failed before it started (#2165). Two
// things then have to happen, and neither is optional: the snapshot
// written a moment ago has to go — the next page load would otherwise
// read it as an *abandoned* redirect and offer that diagnosis, which is
// wrong — and the failure has to be reported the way every sibling arm
// reports one, with the red border (#1621) and the re-auth banner.
oauthClient
?.beginInteractiveAuthorization(authorizationUrl)
.catch((err: unknown) => {
clearOAuthResumeSnapshot();
setFailedServerId(serverId);
showReAuthBanner(serverId, err);
});
},
[sessionRef, inspectorClient, activeTab, ui, onBeforeOAuthRedirect],
[
sessionRef,
inspectorClient,
activeTab,
ui,
onBeforeOAuthRedirect,
setFailedServerId,
showReAuthBanner,
],
);
const tryApplyStoredAuthRecovery = useCallback(
@ -821,6 +843,20 @@ export function useOAuthRecovery({
reason: pending.challenge.reason,
});
}
} catch (err) {
// The slot was cleared above only to keep a tab-visible event and a
// reconnect from starting the same authorization twice — not because
// the recovery was delivered. It still is owed, so restore it and let
// the next trigger retry rather than losing it silently (#2165).
setPendingReauth(pending);
notifications.show({
title: "Could not continue authorization",
message: authRecoveryRetryFailedMessage(
err instanceof Error ? err.message : String(err),
),
color: "yellow",
autoClose: 6000,
});
} finally {
reauthResumeInProgressRef.current = false;
}
@ -880,7 +916,19 @@ export function useOAuthRecovery({
authorizationUrl,
source: "ambient",
});
})();
})().catch((err: unknown) => {
// An event listener cannot be awaited, so this is the only place the
// failure can be reported (#2165). Without it the challenge draws no
// UI response at all — the stored-recovery check or the remote-state
// push rejected, and the user is left on a session whose
// authorization has lapsed with nothing on screen saying so.
const serverId = sessionRef.current.activeServerId;
if (!serverId) {
return;
}
setFailedServerId(serverId);
showReAuthBanner(serverId, err);
});
};
inspectorClient.addEventListener(
@ -900,12 +948,16 @@ export function useOAuthRecovery({
tryApplyStoredAuthRecovery,
runVisibleInteractiveAuth,
deferAmbientReauth,
setFailedServerId,
showReAuthBanner,
]);
useEffect(() => {
return onBrowserTabVisible(() => {
const pending = sessionRef.current.pendingReauth;
if (pending) {
// `void`: a visibility listener cannot be awaited, and
// `resumePendingReauth` now terminates its own failures (#2165).
void resumePendingReauth(pending);
}
});
@ -921,6 +973,8 @@ export function useOAuthRecovery({
}
const pending = sessionRef.current.pendingReauth;
if (pending) {
// `void`: an effect body cannot be awaited, and `resumePendingReauth`
// now terminates its own failures (#2165).
void resumePendingReauth(pending);
}
}, [sessionRef, connectionStatus, inspectorClient, resumePendingReauth]);
@ -1236,7 +1290,17 @@ export function useOAuthRecovery({
autoClose: 6000,
});
}
})();
})().catch((err: unknown) => {
// Everything inside has its own arm; this catches what sits *between*
// them (#2165) — `setupClientForServer` throwing while it builds the
// client, and the post-resume `checkAuthChallengeSatisfied`. Both land
// after the callback URL and the one-shot resume snapshot have been
// consumed, so there is nothing left to retry with and the failure has
// to be reported here or nowhere.
connectStartRef.current = undefined;
setFailedServerId(server.id);
showReAuthBanner(server.id, err instanceof Error ? err : String(err));
});
}, [
servers,
setupClientForServerRef,
@ -1368,7 +1432,23 @@ export function useOAuthRecovery({
const retry = pendingStepUpRetryRef.current;
pendingStepUpRetryRef.current = null;
if (retry) {
await retry();
try {
await retry();
} catch (err) {
// Its own catch, not the outer one: the permissions leg
// succeeded, so a failure from here belongs to the retried
// command and is reported as that command's — routed to the
// panel that issued it rather than dressed up as a step-up
// failure (#2165).
const message = err instanceof Error ? err.message : String(err);
notifications.show({
title: "Retry failed",
message,
color: "red",
autoClose: 6000,
});
setSourceScopedError(stepUp.source, message);
}
}
return;
}
@ -1392,6 +1472,22 @@ export function useOAuthRecovery({
});
setSourceScopedError(stepUp.source, failureMessage);
}
} catch (err) {
// `StepUpAuthModal` calls this handler as `void onAuthorize()`, so a
// rejection escaping here reaches nobody: the `finally` resets the
// latch and the prompt is already dismissed, leaving the panel that
// asked for the permissions never told (#2165). Reported exactly as
// the `failed` outcome above is, so the two cannot disagree.
const failureMessage = emaStepUpFailureMessage(
err instanceof Error ? err.message : String(err),
);
notifications.show({
title: "Organization permissions",
message: failureMessage,
color: "red",
autoClose: 6000,
});
setSourceScopedError(stepUp.source, failureMessage);
} finally {
stepUpAuthorizeInProgressRef.current = false;
}

11
clients/web/src/test/core/auth/oauthUx.test.ts
View file

@ -1,6 +1,7 @@
import { describe, it, expect } from "vitest";
import {
authRecoveryRestoredMessage,
authRecoveryRetryFailedMessage,
emaStepUpFailureMessage,
emaStepUpInProgressMessage,
emaStepUpSuccessMessage,
@ -208,6 +209,16 @@ describe("oauthUx resume/restore copy", () => {
);
});
it("authRecoveryRetryFailedMessage appends only a non-blank detail", () => {
const base =
"Could not continue the pending authorization. The Inspector will try again the next time this tab becomes active or the session reconnects.";
expect(authRecoveryRetryFailedMessage()).toBe(base);
expect(authRecoveryRetryFailedMessage(" ")).toBe(base);
expect(authRecoveryRetryFailedMessage(" token endpoint 500 ")).toBe(
`${base} (token endpoint 500)`,
);
});
it("oauthResumeAbandonedMessage reauth is retry-agnostic", () => {
expect(
oauthResumeAbandonedMessage("reauth", { recoverySource: "tool" }),

1
clients/web/src/utils/oauthUx.ts
View file

@ -1,5 +1,6 @@
export {
authRecoveryRestoredMessage,
authRecoveryRetryFailedMessage,
isActionTriggeredOAuthRecovery,
oauthPreRedirectToastCopy,
oauthResumeAbandonedMessage,

14
core/auth/oauthUx.ts
View file

@ -46,6 +46,20 @@ export function authRecoveryRestoredMessage(options?: {
: "Session credentials were updated.";
}
/**
* Toast when a *deferred* authorization recovery (#2165) fails partway
* through — the tab-visible / reconnect resume, not a user-initiated one.
*
* Phrased as "will try again" rather than as a final failure because the
* pending recovery is restored when this fires: the slot was cleared only to
* keep two triggers from racing, and it is still owed.
*/
export function authRecoveryRetryFailedMessage(detail?: string): string {
const base =
"Could not continue the pending authorization. The Inspector will try again the next time this tab becomes active or the session reconnects.";
return detail?.trim() ? `${base} (${detail.trim()})` : base;
}
export function oauthResumeAbandonedMessage(
authKind: OAuthInteractiveAuthKind,
options?: { recoverySource?: OAuthRecoverySource },