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

refactor(web): let trySetPendingStepUp own the step-up retry

Browse source 
Clearing the retry from the mirror effect raced: an async command continuation
can open a prompt and install its retry before a previous commit's passive
effect flushes, and that effect's stale `pendingStepUp === null` snapshot would
then delete the new prompt's retry.

Install it in `trySetPendingStepUp` instead, in the same synchronous step that
opens the prompt — `null` for an ambient prompt, the operation for a
command-scoped one. Every prompt comes through there and `handleStepUpAuthorize`
is the only reader, so a slot cleared as a side effect can leave a stale closure
behind harmlessly: the next prompt overwrites it. A refused prompt still
installs nothing (#2165).

Copilot round 3 on #2237.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S4WZSxx6XMKzRdgPce9CAd
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-03 20:49:40 -04:00
commit 258e7f1526
1 changed files with 43 additions and 33 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -317,19 +317,6 @@ export function useOAuthRecovery({
useEffect(() => {
sessionRef.current.pendingStepUp = pendingStepUp;
sessionRef.current.pendingReauth = pendingReauth;
// The retry belongs to the prompt, so it dies with it. Every arm that
// dismisses a prompt deliberately drops the retry inline — but the two
// that clear the slot as a *consequence* of something else (the
// server-switch reset below, and `resetOAuthRecoveryState` on disconnect)
// would otherwise leave it installed, and the next prompt to open without
// one of its own would inherit it: authorizing an ambient step-up on
// server B would run the command server A was left mid-way through
// (Copilot on #2237). Clearing it here covers both without any of them
// having to remember. It is a ref write in an effect, which is where a ref
// write belongs — the render-phase reset above must stay pure.
if (pendingStepUp === null) {
pendingStepUpRetryRef.current = null;
}
});
// Both slots are server-scoped, so switching servers has to clear whichever
@ -352,8 +339,32 @@ export function useOAuthRecovery({
);
});
/**
* Opens the step-up prompt, or refuses when one is already up.
*
* The retry is installed here rather than by the caller because it belongs
* to the prompt and must be written in the same synchronous step that opens
* it (#2237). Two things follow, and both are load-bearing:
*
* - A **refused** prompt installs nothing, so a command that was just told
* it cannot start does not replace the open prompt's operation (#2165).
* - An **ambient** prompt passes `null` and so *overwrites* whatever the
* previous prompt left in the ref. Nothing else has to clear it: the paths
* that drop the slot as a consequence of something else — the
* server-switch reset, `resetOAuthRecoveryState` on disconnect — can leave
* a stale closure behind, because the only thing that reads it is
* `handleStepUpAuthorize`, which needs a prompt, and every prompt comes
* through here. Clearing it from an effect instead would race: an async
* continuation can open a prompt and install its retry before a previous
* commit's passive effect flushes, and that effect's stale
* `pendingStepUp === null` snapshot would then delete the new prompt's
* retry (Copilot on #2237).
*/
const trySetPendingStepUp = useCallback(
(next: PendingStepUp): boolean => {
(
next: PendingStepUp,
retryOperation: (() => Promise<unknown>) | null,
): boolean => {
if (sessionRef.current.pendingStepUp !== null) {
notifications.show({
title: "Step-up authorization in progress",
@ -365,6 +376,7 @@ export function useOAuthRecovery({
return false;
}
setPendingStepUp(next);
pendingStepUpRetryRef.current = retryOperation;
return true;
},
[sessionRef],
@ -642,13 +654,18 @@ export function useOAuthRecovery({
}) => {
const server = sessionRef.current.servers.find((s) => s.id === serverId);
if (isStepUpConfirmation(challenge, server)) {
trySetPendingStepUp({
challenge,
authorizationUrl,
serverId,
source,
enterpriseManaged: isEmaStepUp(challenge, server),
});
// Ambient: nothing to retry, and passing `null` is what evicts a
// previous prompt's closure.
trySetPendingStepUp(
{
challenge,
authorizationUrl,
serverId,
source,
enterpriseManaged: isEmaStepUp(challenge, server),
},
null,
);
return;
}
prepareOAuthRedirect({
@ -713,23 +730,16 @@ export function useOAuthRecovery({
) {
return true;
}
// The retry belongs to the prompt, so it is installed only once the
// prompt is actually open (#2165). `trySetPendingStepUp` REFUSES a
// second prompt while one is already up — writing the ref first meant
// the refused command's operation replaced the open prompt's, and
// authorizing that prompt then ran the command the user was just told
// could not start.
if (
trySetPendingStepUp({
trySetPendingStepUp(
{
challenge: error.authChallenge,
authorizationUrl: error.authorizationUrl,
serverId: options.serverId,
source: options.source,
enterpriseManaged: isEmaStepUp(error.authChallenge, server),
})
) {
pendingStepUpRetryRef.current = options.retryOperation ?? null;
}
},
options.retryOperation ?? null,
);
return false;
}