Merge branch 'v2/main' into v2/chore/2163-agent-skills
This commit is contained in:
Cliff Hall
GitHub
commit
cc204e3e28
9 changed files with 986 additions and 29 deletions
|
|
@ -570,6 +570,72 @@ 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("ends a failed connect-scoped attempt instead of leaving it spinning", async () => {
|
||||
// `connect()` rejecting with an auth-recovery error holds the status at
|
||||
// "connecting"; the redirect was what would have ended that attempt, so
|
||||
// if it never happens nothing else does (#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,
|
||||
preRedirectContext: "connect",
|
||||
});
|
||||
});
|
||||
await waitFor(() => expect(client.disconnect).toHaveBeenCalled());
|
||||
expect(h.spies.setFailedServerId).toHaveBeenCalledWith("a");
|
||||
await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a"));
|
||||
});
|
||||
|
||||
it("leaves a non-connect attempt's session alone", async () => {
|
||||
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: "step_up",
|
||||
authorizationUrl: AUTH_URL,
|
||||
});
|
||||
});
|
||||
await waitFor(() => expect(h.api().reAuthBanner?.serverId).toBe("a"));
|
||||
expect(client.disconnect).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("tolerates having no client at all", () => {
|
||||
const h = harness({ servers: [entry("a")], client: null });
|
||||
act(() =>
|
||||
|
|
@ -845,6 +911,61 @@ 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("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 });
|
||||
|
|
@ -987,6 +1108,122 @@ 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 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 resurrect a challenge whose session ended mid-resume", async () => {
|
||||
// A server switch clears the slot on purpose. The in-flight attempt then
|
||||
// rejects, finds it empty, and must NOT put its own challenge back — it
|
||||
// belongs to a session that has ended (#2165).
|
||||
let rejectResume: ((err: Error) => void) | undefined;
|
||||
const client = fakeClient({
|
||||
handleAuthChallenge: vi.fn(
|
||||
() =>
|
||||
new Promise((_resolve, reject) => {
|
||||
rejectResume = reject;
|
||||
}),
|
||||
),
|
||||
});
|
||||
const servers = [entry("a"), entry("b")];
|
||||
const h = harness({ servers, activeServerId: "a", client });
|
||||
await defer(client, h);
|
||||
|
||||
await act(async () => {
|
||||
becomeVisible();
|
||||
});
|
||||
await waitFor(() => expect(rejectResume).toBeDefined());
|
||||
|
||||
h.rerender({ servers, activeServerId: "b", client });
|
||||
await act(async () => {
|
||||
rejectResume!(new Error("token endpoint unreachable"));
|
||||
});
|
||||
await waitFor(() =>
|
||||
expect(toastTitles()).toContain("Could not continue authorization"),
|
||||
);
|
||||
|
||||
// …and says so, rather than promising a retry it will not make.
|
||||
expect(toastWith("the session it belonged to has ended")).toBeTruthy();
|
||||
expect(toastWith("will try again")).toBeUndefined();
|
||||
|
||||
// Nothing was restored, so returning to the tab resumes nothing.
|
||||
await act(async () => {
|
||||
becomeVisible();
|
||||
});
|
||||
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" }),
|
||||
|
|
@ -1167,6 +1404,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 +1867,224 @@ 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("drops the stored retry when an EMA step-up fails", async () => {
|
||||
// The prompt is already dismissed and a later step-up does not overwrite
|
||||
// the ref, so a retained operation would be re-run by an unrelated
|
||||
// authorization (#2165).
|
||||
const handleAuthChallenge = vi
|
||||
.fn()
|
||||
.mockResolvedValueOnce({ kind: "failed", error: new Error("denied") })
|
||||
.mockResolvedValue({ kind: "satisfied" });
|
||||
const client = fakeClient({ handleAuthChallenge });
|
||||
const h = harness({
|
||||
servers: [entry("a", {}, true)],
|
||||
activeServerId: "a",
|
||||
client,
|
||||
});
|
||||
const retry = vi.fn().mockResolvedValue(undefined);
|
||||
await openStepUp(h, "tool", retry);
|
||||
await act(async () => {
|
||||
await h.api().handleStepUpAuthorize();
|
||||
});
|
||||
expect(retry).not.toHaveBeenCalled();
|
||||
|
||||
// A later, unrelated EMA step-up succeeds — and must not re-run it.
|
||||
await openStepUp(h, "tool");
|
||||
await act(async () => {
|
||||
await h.api().handleStepUpAuthorize();
|
||||
});
|
||||
expect(toastTitles()).toContain("Permissions updated");
|
||||
expect(retry).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("drops the stored retry when an EMA step-up rejects", async () => {
|
||||
const handleAuthChallenge = vi
|
||||
.fn()
|
||||
.mockRejectedValueOnce(new Error("IdP unreachable"))
|
||||
.mockResolvedValue({ kind: "satisfied" });
|
||||
const client = fakeClient({ handleAuthChallenge });
|
||||
const h = harness({
|
||||
servers: [entry("a", {}, true)],
|
||||
activeServerId: "a",
|
||||
client,
|
||||
});
|
||||
const retry = vi.fn().mockResolvedValue(undefined);
|
||||
await openStepUp(h, "tool", retry);
|
||||
await act(async () => {
|
||||
await h.api().handleStepUpAuthorize();
|
||||
});
|
||||
|
||||
await openStepUp(h, "tool");
|
||||
await act(async () => {
|
||||
await h.api().handleStepUpAuthorize();
|
||||
});
|
||||
expect(toastTitles()).toContain("Permissions updated");
|
||||
expect(retry).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("drops the stored retry when an EMA step-up hands off to a redirect", async () => {
|
||||
// The redirect unloads the page, so the closure cannot survive it
|
||||
// anyway — and `prepareOAuthRedirect` returns before the navigation is
|
||||
// resolved, so on its failure path a retained operation would be left
|
||||
// for a later, unrelated step-up to re-run (#2165).
|
||||
const handleAuthChallenge = vi
|
||||
.fn()
|
||||
.mockResolvedValueOnce({
|
||||
kind: "interactive",
|
||||
challenge: challenge("insufficient_scope"),
|
||||
authorizationUrl: AUTH_URL,
|
||||
})
|
||||
.mockResolvedValue({ kind: "satisfied" });
|
||||
const client = fakeClient({ handleAuthChallenge });
|
||||
const h = harness({
|
||||
servers: [entry("a", {}, true)],
|
||||
activeServerId: "a",
|
||||
client,
|
||||
});
|
||||
const retry = vi.fn().mockResolvedValue(undefined);
|
||||
await openStepUp(h, "tool", retry);
|
||||
await act(async () => {
|
||||
await h.api().handleStepUpAuthorize();
|
||||
});
|
||||
expect(client.beginInteractiveAuthorization).toHaveBeenCalled();
|
||||
|
||||
await openStepUp(h, "tool");
|
||||
await act(async () => {
|
||||
await h.api().handleStepUpAuthorize();
|
||||
});
|
||||
expect(toastTitles()).toContain("Permissions updated");
|
||||
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("keeps the open prompt's retry when a second step-up is refused", async () => {
|
||||
// `trySetPendingStepUp` refuses the second prompt, so the second
|
||||
// command's operation must not replace the first's — authorizing the
|
||||
// open prompt would otherwise run the command the user was just told
|
||||
// could not start (#2165).
|
||||
const client = fakeClient({
|
||||
handleAuthChallenge: vi.fn().mockResolvedValue({ kind: "satisfied" }),
|
||||
});
|
||||
const h = harness({
|
||||
servers: [entry("a", {}, true)],
|
||||
activeServerId: "a",
|
||||
client,
|
||||
});
|
||||
const firstRetry = vi.fn().mockResolvedValue(undefined);
|
||||
const refusedRetry = vi.fn().mockResolvedValue(undefined);
|
||||
await openStepUp(h, "tool", firstRetry);
|
||||
await openStepUp(h, "prompt", refusedRetry);
|
||||
expect(toastTitles()).toContain("Step-up authorization in progress");
|
||||
|
||||
await act(async () => {
|
||||
await h.api().handleStepUpAuthorize();
|
||||
});
|
||||
expect(firstRetry).toHaveBeenCalledTimes(1);
|
||||
expect(refusedRetry).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("cancels back to the panel that asked", async () => {
|
||||
const client = fakeClient();
|
||||
const h = harness({ servers: [entry("a")], activeServerId: "a", client });
|
||||
|
|
|
|||
|
|
@ -44,6 +44,7 @@ import {
|
|||
applyOAuthResumeUi,
|
||||
buildTabUiSnapshot,
|
||||
clearOAuthResumeSnapshot,
|
||||
clearOwnOAuthResumeSnapshot,
|
||||
consumeOAuthResumeSnapshot,
|
||||
oauthResumeInsufficientScopeMessage,
|
||||
oauthResumeToastMessage,
|
||||
|
|
@ -55,6 +56,8 @@ import { INSPECTOR_SERVERS_TAB } from "../utils/inspectorTabs";
|
|||
import type { PendingReauth } from "../utils/pendingReauth";
|
||||
import {
|
||||
authRecoveryRestoredMessage,
|
||||
authRecoveryAbandonedMessage,
|
||||
authRecoveryRetryFailedMessage,
|
||||
isReAuthBannerReason,
|
||||
issuerBindingFailureCopy,
|
||||
lostAuthorizationStateActionLabel,
|
||||
|
|
@ -528,7 +531,7 @@ export function useOAuthRecovery({
|
|||
onBeforeOAuthRedirect(authorizationUrl);
|
||||
// Write immediately before navigation so implicit client teardown (during
|
||||
// connect setup) cannot clear the snapshot after we persist it.
|
||||
writeOAuthResumeSnapshot({
|
||||
const snapshotToken = writeOAuthResumeSnapshot({
|
||||
version: 1,
|
||||
serverId,
|
||||
activeTab,
|
||||
|
|
@ -538,9 +541,47 @@ export function useOAuthRecovery({
|
|||
...(authKind === "step_up" && authChallenge && { authChallenge }),
|
||||
...(recoverySource && { recoverySource }),
|
||||
});
|
||||
void oauthClient?.beginInteractiveAuthorization(authorizationUrl);
|
||||
// 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
|
||||
// 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.
|
||||
//
|
||||
// Scoped to the snapshot *this* attempt wrote: nothing single-flights
|
||||
// `prepareOAuthRedirect`, so a later attempt may already have replaced
|
||||
// it, and deleting that one would strand the redirect that is actually
|
||||
// in flight.
|
||||
oauthClient
|
||||
?.beginInteractiveAuthorization(authorizationUrl)
|
||||
.catch(async (err: unknown) => {
|
||||
clearOwnOAuthResumeSnapshot(snapshotToken);
|
||||
// A connect-scoped redirect is the tail of a `connect()` that
|
||||
// rejected with an `AuthRecoveryRequiredError`, which deliberately
|
||||
// holds the status at `"connecting"` rather than moving it to
|
||||
// `"error"`. The redirect was the thing that would have ended that
|
||||
// attempt, so with it gone nothing else does: the toggle spins and
|
||||
// the active-server lock is held. Tear it down first, exactly as the
|
||||
// sibling arm in `useConnectionLifecycle` does for the same reason.
|
||||
if (preRedirectContext === "connect") {
|
||||
await oauthClient?.disconnect().catch(() => {});
|
||||
}
|
||||
setFailedServerId(serverId);
|
||||
showReAuthBanner(serverId, err);
|
||||
});
|
||||
},
|
||||
[sessionRef, inspectorClient, activeTab, ui, onBeforeOAuthRedirect],
|
||||
[
|
||||
sessionRef,
|
||||
inspectorClient,
|
||||
activeTab,
|
||||
ui,
|
||||
onBeforeOAuthRedirect,
|
||||
setFailedServerId,
|
||||
showReAuthBanner,
|
||||
],
|
||||
);
|
||||
|
||||
const tryApplyStoredAuthRecovery = useCallback(
|
||||
|
|
@ -649,14 +690,23 @@ export function useOAuthRecovery({
|
|||
) {
|
||||
return true;
|
||||
}
|
||||
pendingStepUpRetryRef.current = options.retryOperation ?? null;
|
||||
trySetPendingStepUp({
|
||||
challenge: error.authChallenge,
|
||||
authorizationUrl: error.authorizationUrl,
|
||||
serverId: options.serverId,
|
||||
source: options.source,
|
||||
enterpriseManaged: isEmaStepUp(error.authChallenge, server),
|
||||
});
|
||||
// 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({
|
||||
challenge: error.authChallenge,
|
||||
authorizationUrl: error.authorizationUrl,
|
||||
serverId: options.serverId,
|
||||
source: options.source,
|
||||
enterpriseManaged: isEmaStepUp(error.authChallenge, server),
|
||||
})
|
||||
) {
|
||||
pendingStepUpRetryRef.current = options.retryOperation ?? null;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
|
|
@ -821,6 +871,47 @@ 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).
|
||||
//
|
||||
// Only into the session this attempt belongs to, and only into an
|
||||
// empty slot.
|
||||
//
|
||||
// The awaits above can outlive the session entirely — a server switch
|
||||
// or a disconnect clears the slot on purpose, and restoring into that
|
||||
// would resurrect a challenge for a session that has ended. The client
|
||||
// identity is part of the check, not just the server id: reconnecting
|
||||
// to the SAME server builds a replacement `InspectorClient`, which an
|
||||
// id-only check would wave through.
|
||||
//
|
||||
// And the tab can go hidden mid-flight with a *newer* challenge
|
||||
// deferring itself into the slot; that one describes the session as it
|
||||
// is now. The restore is a functional update so it sees the queued
|
||||
// state rather than the render-time value, and yields to whatever is
|
||||
// already there — latest wins.
|
||||
const stillThisSession =
|
||||
sessionRef.current.activeServerId === pending.serverId &&
|
||||
sessionRef.current.inspectorClient === client;
|
||||
if (stillThisSession) {
|
||||
setPendingReauth((prev) => prev ?? pending);
|
||||
}
|
||||
const detail = err instanceof Error ? err.message : String(err);
|
||||
notifications.show({
|
||||
title: "Could not continue authorization",
|
||||
// The copy has to match what actually happens. When the session is
|
||||
// gone nothing was restored and no retry is coming, so promising one
|
||||
// would leave the user waiting on it. When it survives, something is
|
||||
// pending either way — this attempt's challenge, or the newer one
|
||||
// the functional update yielded to.
|
||||
message: stillThisSession
|
||||
? authRecoveryRetryFailedMessage(detail)
|
||||
: authRecoveryAbandonedMessage(detail),
|
||||
color: "yellow",
|
||||
autoClose: 6000,
|
||||
});
|
||||
} finally {
|
||||
reauthResumeInProgressRef.current = false;
|
||||
}
|
||||
|
|
@ -843,12 +934,19 @@ 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`: an event listener cannot await, and the `.catch` below is
|
||||
// what terminates the chain.
|
||||
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,
|
||||
);
|
||||
|
|
@ -880,7 +978,15 @@ 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.
|
||||
setFailedServerId(serverId);
|
||||
showReAuthBanner(serverId, err);
|
||||
});
|
||||
};
|
||||
|
||||
inspectorClient.addEventListener(
|
||||
|
|
@ -900,12 +1006,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 +1031,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 +1348,21 @@ export function useOAuthRecovery({
|
|||
autoClose: 6000,
|
||||
});
|
||||
}
|
||||
})();
|
||||
})().catch((err: unknown) => {
|
||||
// `void`: an effect body cannot await, and this `.catch` is what
|
||||
// terminates the chain — the justification AGENTS.md asks of every
|
||||
// surviving `void`.
|
||||
//
|
||||
// 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,
|
||||
|
|
@ -1345,6 +1471,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(),
|
||||
|
|
@ -1365,10 +1502,24 @@ export function useOAuthRecovery({
|
|||
color: "green",
|
||||
autoClose: 5000,
|
||||
});
|
||||
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;
|
||||
}
|
||||
|
|
@ -1380,9 +1531,15 @@ export function useOAuthRecovery({
|
|||
authChallenge: outcome.challenge,
|
||||
recoverySource: stepUp.source,
|
||||
});
|
||||
// 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, 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",
|
||||
|
|
@ -1392,6 +1549,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,6 +11,7 @@ import {
|
|||
readOAuthResumeSnapshot,
|
||||
restoreTabUiFromSnapshot,
|
||||
writeOAuthResumeSnapshot,
|
||||
clearOwnOAuthResumeSnapshot,
|
||||
type OAuthResumeSnapshot,
|
||||
} from "./oauthResume.js";
|
||||
import {
|
||||
|
|
@ -52,8 +53,10 @@ describe("oauthResume", () => {
|
|||
authKind: "reauth",
|
||||
tabUi: {},
|
||||
};
|
||||
writeOAuthResumeSnapshot(snapshot);
|
||||
expect(consumeOAuthResumeSnapshot()).toEqual(snapshot);
|
||||
const attemptId = writeOAuthResumeSnapshot(snapshot);
|
||||
// The write stamps the per-attempt id `clearOwnOAuthResumeSnapshot`
|
||||
// matches on (#2165); everything else round-trips unchanged.
|
||||
expect(consumeOAuthResumeSnapshot()).toEqual({ ...snapshot, attemptId });
|
||||
expect(readOAuthResumeSnapshot()).toBeUndefined();
|
||||
expect(consumeOAuthResumeSnapshot()).toBeUndefined();
|
||||
});
|
||||
|
|
@ -86,8 +89,8 @@ describe("oauthResume", () => {
|
|||
},
|
||||
remoteSessionId: "remote-abc",
|
||||
};
|
||||
writeOAuthResumeSnapshot(snapshot);
|
||||
expect(readOAuthResumeSnapshot()).toEqual(snapshot);
|
||||
const attemptId = writeOAuthResumeSnapshot(snapshot);
|
||||
expect(readOAuthResumeSnapshot()).toEqual({ ...snapshot, attemptId });
|
||||
expect(storage.get(OAUTH_PENDING_SERVER_KEY)).toBeUndefined();
|
||||
clearOAuthResumeSnapshot();
|
||||
expect(readOAuthResumeSnapshot()).toBeUndefined();
|
||||
|
|
@ -349,6 +352,59 @@ describe("oauthResume", () => {
|
|||
}
|
||||
});
|
||||
|
||||
it("clearOwnOAuthResumeSnapshot removes only the attempt it was given", () => {
|
||||
// Deliberately IDENTICAL snapshots: two concurrent redirects for the same
|
||||
// server with the same shell state serialize the same way while carrying
|
||||
// different authorization URLs, which the snapshot does not record. A byte
|
||||
// comparison would call them one attempt and delete the wrong one (#2165).
|
||||
const snapshot = {
|
||||
version: 1,
|
||||
serverId: "a",
|
||||
activeTab: "tools",
|
||||
authKind: "reauth",
|
||||
tabUi: {},
|
||||
} as const;
|
||||
const first = writeOAuthResumeSnapshot({ ...snapshot });
|
||||
const second = writeOAuthResumeSnapshot({ ...snapshot });
|
||||
expect(first).toBeTruthy();
|
||||
expect(second).not.toBe(first);
|
||||
|
||||
expect(clearOwnOAuthResumeSnapshot(first)).toBe(false);
|
||||
expect(readOAuthResumeSnapshot()?.attemptId).toBe(second);
|
||||
|
||||
expect(clearOwnOAuthResumeSnapshot(second)).toBe(true);
|
||||
expect(window.sessionStorage.getItem(OAUTH_RESUME_KEY)).toBeNull();
|
||||
});
|
||||
|
||||
it("clearOwnOAuthResumeSnapshot ignores a snapshot with no attemptId", () => {
|
||||
// An older build's snapshot, mid-redirect across an upgrade — dropping it
|
||||
// would strand a live callback.
|
||||
window.sessionStorage.setItem(
|
||||
OAUTH_RESUME_KEY,
|
||||
JSON.stringify({
|
||||
version: 1,
|
||||
serverId: "a",
|
||||
activeTab: "tools",
|
||||
authKind: "reauth",
|
||||
tabUi: {},
|
||||
}),
|
||||
);
|
||||
expect(clearOwnOAuthResumeSnapshot("some-attempt")).toBe(false);
|
||||
expect(readOAuthResumeSnapshot()?.serverId).toBe("a");
|
||||
});
|
||||
|
||||
it("clearOwnOAuthResumeSnapshot is a no-op without a token", () => {
|
||||
writeOAuthResumeSnapshot({
|
||||
version: 1,
|
||||
serverId: "a",
|
||||
activeTab: "tools",
|
||||
authKind: "reauth",
|
||||
tabUi: {},
|
||||
});
|
||||
expect(clearOwnOAuthResumeSnapshot(undefined)).toBe(false);
|
||||
expect(readOAuthResumeSnapshot()?.serverId).toBe("a");
|
||||
});
|
||||
|
||||
it("writeOAuthResumeSnapshot swallows setItem failures", () => {
|
||||
vi.stubGlobal("sessionStorage", {
|
||||
getItem: () => null,
|
||||
|
|
@ -368,6 +424,68 @@ describe("oauthResume", () => {
|
|||
).not.toThrow();
|
||||
});
|
||||
|
||||
it("clearOwnOAuthResumeSnapshot swallows storage failures", () => {
|
||||
vi.stubGlobal("sessionStorage", {
|
||||
getItem: () => {
|
||||
throw new Error("blocked");
|
||||
},
|
||||
setItem: () => {},
|
||||
removeItem: () => {},
|
||||
});
|
||||
expect(clearOwnOAuthResumeSnapshot("attempt")).toBe(false);
|
||||
});
|
||||
|
||||
it("clearOwnOAuthResumeSnapshot is a no-op with nothing stored", () => {
|
||||
expect(clearOwnOAuthResumeSnapshot("attempt")).toBe(false);
|
||||
});
|
||||
|
||||
it("writeOAuthResumeSnapshot mints an attemptId per write", () => {
|
||||
const token = writeOAuthResumeSnapshot({
|
||||
version: 1,
|
||||
serverId: "a",
|
||||
activeTab: "tools",
|
||||
authKind: "reauth",
|
||||
tabUi: {},
|
||||
});
|
||||
expect(token).toEqual(expect.any(String));
|
||||
expect(readOAuthResumeSnapshot()?.attemptId).toBe(token);
|
||||
});
|
||||
|
||||
it("writeOAuthResumeSnapshot falls back when randomUUID is unavailable", () => {
|
||||
// `crypto.randomUUID` needs a secure context, which a plain-HTTP
|
||||
// non-loopback host is not.
|
||||
const original = Object.getOwnPropertyDescriptor(
|
||||
globalThis.crypto,
|
||||
"randomUUID",
|
||||
);
|
||||
Object.defineProperty(globalThis.crypto, "randomUUID", {
|
||||
configurable: true,
|
||||
value: undefined,
|
||||
});
|
||||
try {
|
||||
const token = writeOAuthResumeSnapshot({
|
||||
version: 1,
|
||||
serverId: "a",
|
||||
activeTab: "tools",
|
||||
authKind: "reauth",
|
||||
tabUi: {},
|
||||
});
|
||||
expect(token).toEqual(expect.any(String));
|
||||
expect(clearOwnOAuthResumeSnapshot(token)).toBe(true);
|
||||
} finally {
|
||||
// `randomUUID` is inherited from `Crypto.prototype`, so there is
|
||||
// normally no OWN descriptor to put back — restoring only when one
|
||||
// existed would leave the `undefined` own property in place and force
|
||||
// every later test in this file onto the fallback path.
|
||||
if (original) {
|
||||
Object.defineProperty(globalThis.crypto, "randomUUID", original);
|
||||
} else {
|
||||
delete (globalThis.crypto as { randomUUID?: unknown }).randomUUID;
|
||||
}
|
||||
}
|
||||
expect(globalThis.crypto.randomUUID).toEqual(expect.any(Function));
|
||||
});
|
||||
|
||||
it("clearOAuthResumeSnapshot swallows removeItem failures", () => {
|
||||
vi.stubGlobal("sessionStorage", {
|
||||
getItem: () => null,
|
||||
|
|
@ -422,6 +540,10 @@ describe("oauthResume", () => {
|
|||
it("clearOAuthResumeSnapshot is a no-op", () => {
|
||||
expect(() => clearOAuthResumeSnapshot()).not.toThrow();
|
||||
});
|
||||
|
||||
it("clearOwnOAuthResumeSnapshot is a no-op", () => {
|
||||
expect(clearOwnOAuthResumeSnapshot("{}")).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
it("restoreTabUiFromSnapshot returns early when tabUi is undefined", () => {
|
||||
|
|
|
|||
|
|
@ -57,6 +57,19 @@ export interface OAuthResumeSnapshot {
|
|||
authChallenge?: AuthChallenge;
|
||||
/** Command-scoped recovery source when redirect was triggered by a user action. */
|
||||
recoverySource?: OAuthRecoverySource;
|
||||
/**
|
||||
* Identifies the redirect attempt that wrote this snapshot (#2165).
|
||||
*
|
||||
* Stamped by {@link writeOAuthResumeSnapshot} and matched by
|
||||
* {@link clearOwnOAuthResumeSnapshot}, so an attempt that fails before
|
||||
* navigating cannot delete a *later* attempt's snapshot. Nothing else reads
|
||||
* it: the callback identifies its server by `serverId`, as before.
|
||||
*
|
||||
* Optional because a snapshot written by an older build (across a redirect
|
||||
* that spans an upgrade) has none, and dropping such a snapshot would strand
|
||||
* a live callback.
|
||||
*/
|
||||
attemptId?: string;
|
||||
}
|
||||
|
||||
export interface LiftedTabUiState {
|
||||
|
|
@ -168,17 +181,51 @@ export function restoreTabUiFromSnapshot(
|
|||
}
|
||||
}
|
||||
|
||||
export function writeOAuthResumeSnapshot(snapshot: OAuthResumeSnapshot): void {
|
||||
/**
|
||||
* 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.
|
||||
*/
|
||||
export function writeOAuthResumeSnapshot(
|
||||
snapshot: OAuthResumeSnapshot,
|
||||
): string | undefined {
|
||||
if (typeof window === "undefined") {
|
||||
return;
|
||||
return undefined;
|
||||
}
|
||||
const attemptId = snapshot.attemptId ?? newAttemptId();
|
||||
try {
|
||||
window.sessionStorage.setItem(OAUTH_RESUME_KEY, JSON.stringify(snapshot));
|
||||
window.sessionStorage.setItem(
|
||||
OAUTH_RESUME_KEY,
|
||||
JSON.stringify({ ...snapshot, attemptId }),
|
||||
);
|
||||
return attemptId;
|
||||
} catch {
|
||||
// Best-effort — privacy mode / quota.
|
||||
return undefined;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A per-attempt identifier. `randomUUID` where it exists (it needs a secure
|
||||
* context, which a `file://` page or a plain-HTTP non-loopback host is not),
|
||||
* and otherwise a value that only has to be unique among the handful of
|
||||
* redirect attempts one page can have in flight — never a security token.
|
||||
*/
|
||||
function newAttemptId(): string {
|
||||
const uuid = globalThis.crypto?.randomUUID?.bind(globalThis.crypto);
|
||||
if (uuid) {
|
||||
return uuid();
|
||||
}
|
||||
return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2)}`;
|
||||
}
|
||||
|
||||
export function readOAuthResumeSnapshot(): OAuthResumeSnapshot | undefined {
|
||||
if (typeof window === "undefined") {
|
||||
return undefined;
|
||||
|
|
@ -231,6 +278,45 @@ function readLegacyPendingServerSnapshot(): OAuthResumeSnapshot | undefined {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Clear the snapshot **only if it is still the one this attempt wrote** (#2165).
|
||||
*
|
||||
* `prepareOAuthRedirect` has no single-flight guard, so a redirect that fails
|
||||
* before navigating can reject after a *later* attempt has already written its
|
||||
* own snapshot. An unconditional clear there would delete the newer attempt's
|
||||
* callback-routing state — a worse failure than the stale snapshot it is
|
||||
* trying to avoid, since that redirect is actually in flight.
|
||||
*
|
||||
* Matched on the snapshot's `attemptId` rather than on its bytes: two
|
||||
* concurrent redirects for the same server with the same shell state serialize
|
||||
* identically while carrying *different* authorization URLs, which the
|
||||
* snapshot does not record — so a byte comparison would report them as the
|
||||
* same attempt and delete the wrong one.
|
||||
*
|
||||
* Returns whether anything was removed.
|
||||
*/
|
||||
export function clearOwnOAuthResumeSnapshot(
|
||||
attemptId: string | undefined,
|
||||
): boolean {
|
||||
if (typeof window === "undefined" || attemptId === undefined) {
|
||||
return false;
|
||||
}
|
||||
try {
|
||||
const raw = window.sessionStorage.getItem(OAUTH_RESUME_KEY);
|
||||
if (!raw) {
|
||||
return false;
|
||||
}
|
||||
const stored = JSON.parse(raw) as Partial<OAuthResumeSnapshot>;
|
||||
if (stored?.attemptId !== attemptId) {
|
||||
return false;
|
||||
}
|
||||
window.sessionStorage.removeItem(OAUTH_RESUME_KEY);
|
||||
return true;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
export function clearOAuthResumeSnapshot(): void {
|
||||
if (typeof window === "undefined") {
|
||||
return;
|
||||
|
|
|
|||
|
|
@ -1,6 +1,8 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import {
|
||||
authRecoveryRestoredMessage,
|
||||
authRecoveryRetryFailedMessage,
|
||||
authRecoveryAbandonedMessage,
|
||||
emaStepUpFailureMessage,
|
||||
emaStepUpInProgressMessage,
|
||||
emaStepUpSuccessMessage,
|
||||
|
|
@ -208,6 +210,25 @@ 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("authRecoveryAbandonedMessage promises no retry", () => {
|
||||
const base =
|
||||
"Could not continue the pending authorization, and the session it belonged to has ended. Reconnect to authorize again.";
|
||||
expect(authRecoveryAbandonedMessage()).toBe(base);
|
||||
expect(authRecoveryAbandonedMessage(" ")).toBe(base);
|
||||
expect(authRecoveryAbandonedMessage(" nope ")).toBe(`${base} (nope)`);
|
||||
expect(authRecoveryAbandonedMessage()).not.toContain("try again");
|
||||
});
|
||||
|
||||
it("oauthResumeAbandonedMessage reauth is retry-agnostic", () => {
|
||||
expect(
|
||||
oauthResumeAbandonedMessage("reauth", { recoverySource: "tool" }),
|
||||
|
|
|
|||
|
|
@ -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: {
|
||||
|
|
|
|||
|
|
@ -1,5 +1,7 @@
|
|||
export {
|
||||
authRecoveryRestoredMessage,
|
||||
authRecoveryRetryFailedMessage,
|
||||
authRecoveryAbandonedMessage,
|
||||
isActionTriggeredOAuthRecovery,
|
||||
oauthPreRedirectToastCopy,
|
||||
oauthResumeAbandonedMessage,
|
||||
|
|
|
|||
|
|
@ -46,6 +46,34 @@ 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;
|
||||
}
|
||||
|
||||
/**
|
||||
* Toast when a deferred authorization recovery fails *and* the session it
|
||||
* belonged to has ended, so nothing was left pending to retry (#2165).
|
||||
*
|
||||
* Distinct from {@link authRecoveryRetryFailedMessage} because promising a
|
||||
* retry the code has explicitly declined to make is worse than saying nothing:
|
||||
* the user waits for something that will never happen.
|
||||
*/
|
||||
export function authRecoveryAbandonedMessage(detail?: string): string {
|
||||
const base =
|
||||
"Could not continue the pending authorization, and the session it belonged to has ended. Reconnect to authorize again.";
|
||||
return detail?.trim() ? `${base} (${detail.trim()})` : base;
|
||||
}
|
||||
|
||||
export function oauthResumeAbandonedMessage(
|
||||
authKind: OAuthInteractiveAuthKind,
|
||||
options?: { recoverySource?: OAuthRecoverySource },
|
||||
|
|
|
|||
|
|
@ -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 });
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue