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): address Copilot review on the OAuth recovery failure arms

Browse source 
Three findings, all valid:

- `beginInteractiveAuthorization` navigated *before* awaiting provider
  creation and flow-state recording, so a rejection could arrive after
  the browser had been handed over — and the web catch clears the resume
  snapshot on one. Fixed at the root: the navigation now resolves last,
  after every fallible step, which makes every rejection the method can
  produce a pre-navigation rejection. It is also better on its own
  terms — a provider that cannot be built means the callback could not
  have completed, so navigating first only bought a dead-end round trip.
- `resumePendingReauth` restored the pending slot unconditionally, which
  could overwrite a newer challenge deferred while it was awaiting.
  Restores only into an empty slot, so latest wins.
- the `authChallengeInteractive` catch re-read the active server rather
  than the one the challenge arrived for. `serverId` is now captured
  synchronously in the listener and used by both the body and the catch.

Three new tests, one per finding, each driving the interleaving rather
than the happy path.

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:54:53 -04:00
commit ecad587afe
4 changed files with 134 additions and 13 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -893,6 +893,38 @@ describe("useOAuthRecovery", () => {
expect(client.beginInteractiveAuthorization).not.toHaveBeenCalled();
});
it("reports a rejected challenge against the server it arrived for", async () => {
// The catch must not re-read the active server: a switch during the
// await would otherwise flag the wrong one, or drop the report (#2165).
let rejectCheck: ((err: Error) => void) | undefined;
const client = fakeClient({
checkAuthChallengeSatisfied: vi.fn(
() =>
new Promise((_resolve, reject) => {
rejectCheck = reject;
}),
),
});
const servers = [entry("a"), entry("b")];
const h = harness({ servers, activeServerId: "a", client });
await act(async () => {
client.emit("authChallengeInteractive", {
challenge: challenge(),
authorizationUrl: AUTH_URL,
});
});
await waitFor(() => expect(rejectCheck).toBeDefined());
h.rerender({ servers, activeServerId: "b", client });
await act(async () => {
rejectCheck!(new Error("remote auth state unreachable"));
});
await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a"));
expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a");
expect(h.spies.setFailedServerId).not.toHaveBeenCalledWith("b");
});
it("raises the banner on an oauthError event", async () => {
const client = fakeClient();
const h = harness({ servers: [entry("a")], activeServerId: "a", client });
@ -1065,6 +1097,51 @@ describe("useOAuthRecovery", () => {
);
});
it("does not put a stale challenge back over a newer deferral", async () => {
// The tab can go hidden mid-resume and a newer challenge defer itself
// into the slot; that one describes the session as it is now, so the
// failing attempt must not overwrite it (#2165).
let rejectResume: ((err: Error) => void) | undefined;
const client = fakeClient({
handleAuthChallenge: vi.fn(
() =>
new Promise((_resolve, reject) => {
rejectResume = reject;
}),
),
});
const h = harness({ servers: [entry("a")], activeServerId: "a", client });
await defer(client, h);
await act(async () => {
becomeVisible();
});
await waitFor(() => expect(rejectResume).toBeDefined());
// A newer challenge arrives while the resume is still in flight.
visibility.visible = false;
await act(async () => {
client.emit("authChallengeInteractive", {
challenge: challenge("insufficient_scope"),
authorizationUrl: AUTH_URL,
});
});
await act(async () => {
rejectResume!(new Error("token endpoint unreachable"));
});
await waitFor(() =>
expect(toastTitles()).toContain("Could not continue authorization"),
);
// The newer (step-up) challenge is what the next trigger acts on.
await act(async () => {
becomeVisible();
});
await waitFor(() => expect(h.api().pendingStepUp).not.toBeNull());
expect(client.handleAuthChallenge).toHaveBeenCalledTimes(1);
});
it("does not resume while disconnected, and resumes on the reconnect", async () => {
const client = fakeClient({
handleAuthChallenge: vi.fn().mockResolvedValue({ kind: "satisfied" }),

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

@ -539,8 +539,10 @@ export function useOAuthRecovery({
...(authKind === "step_up" && authChallenge && { authChallenge }),
...(recoverySource && { recoverySource }),
});
// The navigation is what this whole callback exists to reach, so a
// rejection here means the flow failed before it started (#2165). Two
// The navigation is what this whole callback exists to reach, and
// `beginInteractiveAuthorization` resolves it *last* — every await it
// can reject on runs before the browser is handed over — 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
@ -848,7 +850,15 @@ export function useOAuthRecovery({
// 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);
//
// Only into an empty slot, though: the tab can go hidden during the
// awaits above and a *newer* challenge defer itself into it, and that
// one describes the session as it is now. Latest wins — putting this
// attempt's stale challenge back over it would retry authorization
// data the server has already superseded.
if (sessionRef.current.pendingReauth === null) {
setPendingReauth(pending);
}
notifications.show({
title: "Could not continue authorization",
message: authRecoveryRetryFailedMessage(
@ -879,12 +889,17 @@ export function useOAuthRecovery({
const onAuthChallengeInteractive = (
event: TypedEvent<"authChallengeInteractive">,
): void => {
// Read at event time, not inside the async body: this is the server the
// challenge arrived for, and both the body and the catch below have to
// mean *that* one. Re-reading it after an await would let a server
// switch (or a session that cleared it) either report against a server
// this challenge has nothing to do with, or drop the report entirely.
const serverId = sessionRef.current.activeServerId;
if (!serverId) {
return;
}
void (async () => {
const { challenge, authorizationUrl } = event.detail;
const serverId = sessionRef.current.activeServerId;
if (!serverId) {
return;
}
const server = sessionRef.current.servers.find(
(s) => s.id === serverId,
);
@ -922,10 +937,6 @@ export function useOAuthRecovery({
// 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);
});

20
clients/web/src/test/core/mcp/oauthManager.test.ts
View file

@ -2847,6 +2847,26 @@ describe("OAuthManager", () => {
parseSpy.mockRestore();
});
it("does not navigate when the fallible setup rejects", async () => {
// #2165: the web client drops its OAuth resume snapshot when this
// rejects, which is only sound because the navigation resolves last.
const params = createMockParams();
storageOf(params).getClientInformation.mockRejectedValue(
new Error("client information unreadable"),
);
const manager = new OAuthManager(params);
await expect(
manager.beginInteractiveAuthorization(
new URL("https://auth.example.com/authorize"),
),
).rejects.toThrow("client information unreadable");
expect(
params.initialConfig.navigation!.navigateToAuthorization,
).not.toHaveBeenCalled();
expect(params.dispatchOAuthAuthorizationRequired).not.toHaveBeenCalled();
});
it("throws when navigation is not configured", async () => {
const params = createMockParams({
initialConfig: {

17
core/mcp/oauthManager.ts
View file

@ -974,12 +974,25 @@ export class OAuthManager {
}
}
this.requireNavigation().navigateToAuthorization(authorizationUrl);
// Every fallible step runs BEFORE the navigation, deliberately (#2165).
// `navigateToAuthorization` hands the browser to the authorization server,
// and a caller that treats a rejection as "the flow never started" — the
// web client drops its OAuth resume snapshot on one — has no way to tell a
// pre-navigation failure from a post-navigation one. Resolving the
// navigation last makes every rejection this method can produce a
// pre-navigation rejection, so that reading is always correct.
//
// It is also the better behaviour on its own terms: a provider that cannot
// be built, or client information that cannot be read, means the callback
// could not have been completed anyway, so sending the user to the
// authorization server first only buys them a round trip to a dead end.
const navigation = this.requireNavigation();
const provider = await this.createOAuthProvider();
const clientInfo = await provider.clientInformation();
await this.recordAuthorizationCodeFlowState(authorizationUrl, clientInfo);
navigation.navigateToAuthorization(authorizationUrl);
this.params.dispatchOAuthAuthorizationRequired({ url: authorizationUrl });
}