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): give each EMA step-up attempt ownership of its command retry

Browse source 
Copilot round 5. `pendingStepUpRetryRef` is shared, and the EMA branch
dismisses the prompt *before* its awaits — so a newer command can open
its own step-up and install its own retry while the first authorization
is in flight. Every arm that read or cleared the ref afterwards was
acting on that newer operation: running it under the wrong
authorization and reporting its failure to the wrong source, or
deleting it outright.

The attempt now takes its retry out of the ref before any await, and no
arm touches the ref again. That covers all three sites at once and
removed the interactive-arm clear added last round — the value is
already out by then. The standard step-up path redirects and clears
synchronously, so it has no such window.

Also rewrites the `writeOAuthResumeSnapshot` docstring, which still
described the byte-comparison token it no longer returns.

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 03:03:02 -04:00
commit 1634437b69
3 changed files with 80 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -2004,6 +2004,61 @@ describe("useOAuthRecovery", () => {
expect(retry).not.toHaveBeenCalled();
});
it("never touches a retry installed by a newer step-up", async () => {
// Dismissing the prompt frees the slot, so a newer command can open its
// own step-up while this authorization is still in flight. Reading the
// shared ref afterwards would run that newer command under this
// authorization; clearing it would delete it (#2165).
let settleFirst: ((outcome: unknown) => void) | undefined;
const handleAuthChallenge = vi
.fn()
.mockImplementationOnce(
() =>
new Promise((resolve) => {
settleFirst = resolve;
}),
)
.mockResolvedValue({ kind: "satisfied" });
const client = fakeClient({ handleAuthChallenge });
const h = harness({
servers: [entry("a", {}, true)],
activeServerId: "a",
client,
});
const firstRetry = vi.fn().mockResolvedValue(undefined);
await openStepUp(h, "tool", firstRetry);
let authorize: Promise<void> | undefined;
await act(async () => {
authorize = h.api().handleStepUpAuthorize();
});
await waitFor(() => expect(settleFirst).toBeDefined());
// A newer command opens its own step-up mid-flight.
const secondRetry = vi.fn().mockResolvedValue(undefined);
await openStepUp(h, "prompt", secondRetry);
await act(async () => {
settleFirst!({ kind: "failed", error: new Error("denied") });
await authorize;
});
// The old attempt reported against its own source and ran nothing.
expect(h.spies.setSourceScopedError).toHaveBeenCalledWith(
"tool",
expect.stringContaining("denied"),
);
expect(firstRetry).not.toHaveBeenCalled();
expect(secondRetry).not.toHaveBeenCalled();
// …and the newer prompt's retry survived to run on its own authorization.
await act(async () => {
await h.api().handleStepUpAuthorize();
});
expect(secondRetry).toHaveBeenCalledTimes(1);
expect(firstRetry).not.toHaveBeenCalled();
});
it("cancels back to the panel that asked", async () => {
const client = fakeClient();
const h = harness({ servers: [entry("a")], activeServerId: "a", client });

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

@ -1462,6 +1462,17 @@ export function useOAuthRecovery({
if (stepUp.enterpriseManaged) {
stepUpAuthorizeInProgressRef.current = true;
setPendingStepUp(null);
// Take this attempt's retry out of the shared ref BEFORE any await
// (#2165). Dismissing the prompt above frees the step-up slot, so a
// newer command can open its own prompt and install its own retry while
// the authorization below is in flight — and every arm that reads or
// clears the ref afterwards would then be acting on that newer
// operation: running it under this authorization and reporting its
// failure to this attempt's source, or deleting it outright. Owning the
// value locally is what makes each arm's decision apply to the attempt
// that made it, and leaves a later prompt's retry untouched.
const retry = pendingStepUpRetryRef.current;
pendingStepUpRetryRef.current = null;
notifications.show({
title: "Organization permissions",
message: emaStepUpInProgressMessage(),
@ -1482,8 +1493,6 @@ export function useOAuthRecovery({
color: "green",
autoClose: 5000,
});
const retry = pendingStepUpRetryRef.current;
pendingStepUpRetryRef.current = null;
if (retry) {
try {
await retry();
@ -1513,23 +1522,15 @@ export function useOAuthRecovery({
authChallenge: outcome.challenge,
recoverySource: stepUp.source,
});
// Handing off, so the retry goes too — as the standard (non-EMA)
// step-up below already does. A full-page redirect cannot carry a
// closure across the navigation, so keeping it buys nothing on the
// success path; and `prepareOAuthRedirect` returns before the
// navigation is resolved, so on the failure path it would survive
// as a stale operation a later, unrelated step-up could re-run
// (#2165).
pendingStepUpRetryRef.current = null;
// Nothing to drop: this attempt's retry left the shared ref
// before the awaits above, and a full-page redirect could not
// carry the closure across the navigation anyway.
return;
}
if (outcome.kind === "failed") {
// Terminal: drop the stored retry with it. The prompt is already
// dismissed, so nothing will ever consume this operation — and a
// *later* step-up does not overwrite the ref, so leaving it would
// let that unrelated authorization silently re-run this command
// (#2165).
pendingStepUpRetryRef.current = null;
// Terminal, and this attempt's retry is already out of the shared
// ref — so it dies with the attempt, and whatever a later prompt
// has installed is left alone.
const failureMessage = emaStepUpFailureMessage(outcome.error.message);
notifications.show({
title: "Organization permissions",
@ -1544,9 +1545,7 @@ export function useOAuthRecovery({
// 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 — and it
// drops the stored retry for the same reason that arm does.
pendingStepUpRetryRef.current = null;
// the `failed` outcome above is, so the two cannot disagree.
const failureMessage = emaStepUpFailureMessage(
err instanceof Error ? err.message : String(err),
);

9
clients/web/src/lib/oauthResume.ts
View file

@ -182,8 +182,13 @@ export function restoreTabUiFromSnapshot(
}
/**
* Persist the snapshot, returning the exact serialization stored — the
* "attempt token" {@link clearOwnOAuthResumeSnapshot} matches against (#2165).
* Persist the snapshot, returning the **`attemptId`** it was stored under —
* the identifier {@link clearOwnOAuthResumeSnapshot} matches on (#2165), not
* the serialized snapshot and not anything to compare bytes against.
*
* A fresh id is minted per write unless the caller supplied one, so two
* redirect attempts are always distinguishable even when their snapshots are
* byte-identical.
*
* `undefined` when nothing was stored (no `window`, privacy mode, quota), so a
* caller holding one knows a clear is meaningful.