fix(web): drop the step-up retry when its prompt is cleared
The retry closure belongs to the prompt, but the two paths that clear the step-up slot as a *consequence* of something else — the server-switch reset and `resetOAuthRecoveryState` on disconnect — left `pendingStepUpRetryRef` installed. The next prompt to open without a retry of its own would inherit it, so authorizing an ambient step-up on server B ran the command server A was left mid-way through. Clear it in the existing mirror effect whenever the slot is null, so no arm has to remember. A ref write in an effect, since the render-phase reset must stay pure. Copilot 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:
parent
2ba25d63e3
commit
3c8fdb0814
2 changed files with 56 additions and 0 deletions
|
|
@ -391,6 +391,49 @@ describe("useOAuthRecovery", () => {
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("does not carry the left server's retry into a later ambient step-up", async () => {
|
||||||
|
const client = fakeClient({
|
||||||
|
handleAuthChallenge: vi.fn().mockResolvedValue({ kind: "satisfied" }),
|
||||||
|
});
|
||||||
|
const servers = [entry("a", {}, true), entry("b", {}, true)];
|
||||||
|
const h = harness({ servers, activeServerId: "a", client });
|
||||||
|
const retryA = vi.fn().mockResolvedValue(undefined);
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
await h
|
||||||
|
.api()
|
||||||
|
.handleCommandScopedAuthRecovery(
|
||||||
|
new AuthRecoveryRequiredError(
|
||||||
|
AUTH_URL,
|
||||||
|
challenge("insufficient_scope"),
|
||||||
|
),
|
||||||
|
{ serverId: "a", source: "tool", retryOperation: retryA },
|
||||||
|
);
|
||||||
|
});
|
||||||
|
expect(h.api().pendingStepUp?.serverId).toBe("a");
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
h.rerender({ servers, activeServerId: "b", client });
|
||||||
|
});
|
||||||
|
expect(h.api().pendingStepUp).toBeNull();
|
||||||
|
|
||||||
|
// Server B raises its own step-up, which carries no retry of its own.
|
||||||
|
await act(async () => {
|
||||||
|
client.emit("authChallengeInteractive", {
|
||||||
|
challenge: challenge("insufficient_scope"),
|
||||||
|
authorizationUrl: AUTH_URL,
|
||||||
|
});
|
||||||
|
});
|
||||||
|
await waitFor(() => expect(h.api().pendingStepUp?.serverId).toBe("b"));
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
await h.api().handleStepUpAuthorize();
|
||||||
|
});
|
||||||
|
// Authorizing B must not run the command A was left mid-way through.
|
||||||
|
expect(retryA).not.toHaveBeenCalled();
|
||||||
|
expect(toastTitles()).toContain("Permissions updated");
|
||||||
|
});
|
||||||
|
|
||||||
it("refuses a second step-up while one is open", async () => {
|
it("refuses a second step-up while one is open", async () => {
|
||||||
const client = fakeClient();
|
const client = fakeClient();
|
||||||
const h = harness({ servers: [entry("a")], activeServerId: "a", client });
|
const h = harness({ servers: [entry("a")], activeServerId: "a", client });
|
||||||
|
|
|
||||||
|
|
@ -317,6 +317,19 @@ export function useOAuthRecovery({
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
sessionRef.current.pendingStepUp = pendingStepUp;
|
sessionRef.current.pendingStepUp = pendingStepUp;
|
||||||
sessionRef.current.pendingReauth = pendingReauth;
|
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
|
// Both slots are server-scoped, so switching servers has to clear whichever
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue