From cc8f379143f7c09ee6c2ca84eb6d27bfc24c5524 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sat, 18 Jul 2026 08:06:18 +0200 Subject: [PATCH] fix(auth): invalidate stale browser OAuth operations --- .../src/controllers/authController.test.ts | 111 +++++++++++++++++- src/client/src/controllers/authController.ts | 79 ++++++++++--- 2 files changed, 170 insertions(+), 20 deletions(-) diff --git a/src/client/src/controllers/authController.test.ts b/src/client/src/controllers/authController.test.ts index 1e9481b..0f2525d 100644 --- a/src/client/src/controllers/authController.test.ts +++ b/src/client/src/controllers/authController.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { api as defaultApi, type AuthProviderOption, type OAuthFlowState, type SessionInfo, type SessionStatus } from "../api"; import { initialAppState, type AppState } from "../appState"; import { AuthController, parseAuthSlashCommand } from "./authController"; @@ -134,6 +134,108 @@ describe("AuthController", () => { }); }); + it("does not recreate an OAuth dialog when a pending response settles during cancellation", async () => { + const prompt = { requestId: "request-1", message: "Paste callback", kind: "manual" } as const; + const flow = oauthFlow({ prompt }); + const response = deferred(); + const cancellation = deferred(); + const { controller, getState } = createController( + { authDialog: { step: "oauth", flow, inputValue: "https://callback" } }, + { + respondOAuthFlow: () => response.promise, + cancelOAuthFlow: () => cancellation.promise, + }, + ); + + const responsePending = controller.respondOAuth(); + const cancellationPending = controller.cancelOAuth(); + const dialogAfterCancel = getState().authDialog; + + response.resolve(oauthFlow({ prompt, progress: ["Stale response"] })); + await responsePending; + const dialogAfterResponse = getState().authDialog; + + cancellation.resolve(oauthFlow({ status: "cancelled" })); + await cancellationPending; + + expect(dialogAfterCancel).toBeUndefined(); + expect(dialogAfterResponse).toBeUndefined(); + expect(getState().authDialog).toBeUndefined(); + }); + + it("does not let a stale OAuth response overwrite a newer flow", async () => { + vi.stubGlobal("window", { setInterval: () => 1, clearInterval: () => undefined }); + const oldPrompt = { requestId: "request-1", message: "Paste callback", kind: "manual" } as const; + const oldFlow = oauthFlow({ prompt: oldPrompt }); + const newFlow = oauthFlow({ flowId: "flow-2", prompt: { requestId: "request-2", message: "Paste callback", kind: "manual" } }); + const response = deferred(); + const providers = [authProvider("anthropic", "oauth")]; + const { controller, getState } = createController( + { authDialog: { step: "oauth", flow: oldFlow, inputValue: "https://old-callback" } }, + { + respondOAuthFlow: () => response.promise, + authProviders: () => Promise.resolve({ providers }), + startOAuthLogin: () => Promise.resolve(newFlow), + }, + ); + + try { + const responsePending = controller.respondOAuth(); + await controller.openLogin("anthropic"); + const dialogAfterNewFlow = getState().authDialog; + + response.resolve(oauthFlow({ prompt: oldPrompt, progress: ["Stale response"] })); + await responsePending; + + expect(dialogAfterNewFlow).toMatchObject({ step: "oauth", flow: { flowId: "flow-2" } }); + expect(getState().authDialog).toMatchObject({ step: "oauth", flow: { flowId: "flow-2" } }); + } finally { + response.resolve(oldFlow); + controller.dispose(); + vi.unstubAllGlobals(); + } + }); + + it("does not let an older poll restore a running flow after a newer poll stops polling", async () => { + vi.useFakeTimers(); + vi.stubGlobal("window", { setInterval: globalThis.setInterval, clearInterval: globalThis.clearInterval }); + const prompt = { requestId: "request-1", message: "Paste callback", kind: "manual" } as const; + const runningFlow = oauthFlow({ prompt }); + const stalePoll = deferred(); + const providers = [authProvider("anthropic", "oauth")]; + let pollCalls = 0; + const { controller, getState } = createController( + {}, + { + authProviders: () => Promise.resolve({ providers }), + startOAuthLogin: () => Promise.resolve(runningFlow), + oauthFlow: () => { + pollCalls += 1; + return pollCalls === 1 ? stalePoll.promise : Promise.resolve(oauthFlow({ status: "cancelled", prompt })); + }, + }, + ); + + try { + await controller.openLogin("anthropic"); + await vi.advanceTimersByTimeAsync(1000); + await vi.advanceTimersByTimeAsync(1000); + const dialogAfterPollingStopped = getState().authDialog; + + stalePoll.resolve(oauthFlow({ prompt, progress: ["Stale running poll"] })); + await flushMicrotasks(); + + expect(pollCalls).toBe(2); + expect(dialogAfterPollingStopped).toMatchObject({ step: "oauth", flow: { status: "cancelled" } }); + expect(getState().authDialog).toMatchObject({ step: "oauth", flow: { status: "cancelled" } }); + } finally { + stalePoll.resolve(runningFlow); + controller.dispose(); + vi.unstubAllGlobals(); + vi.useRealTimers(); + } + }); + it("cancels the active OAuth flow and closes the dialog even when cancellation fails", async () => { const flow = oauthFlow({ prompt: { requestId: "request-1", message: "Paste callback", kind: "manual" } }); const cancelCalls: { flowId: string; machineId: string | undefined }[] = []; @@ -246,6 +348,13 @@ async function flushMicrotasks(): Promise { await Promise.resolve(); } +function deferred(): { promise: Promise; resolve: (value: T) => void } { + let resolveDeferred: ((value: T) => void) | undefined; + const promise = new Promise((resolve) => { resolveDeferred = resolve; }); + if (resolveDeferred === undefined) throw new Error("Deferred promise was not initialized"); + return { promise, resolve: resolveDeferred }; +} + function remoteMachine(id: string): NonNullable { return { id, diff --git a/src/client/src/controllers/authController.ts b/src/client/src/controllers/authController.ts index fe1b209..b9df1c9 100644 --- a/src/client/src/controllers/authController.ts +++ b/src/client/src/controllers/authController.ts @@ -1,6 +1,9 @@ import { api as defaultApi, type AuthProviderOption, type AuthType, type OAuthFlowState, type SessionStatus } from "../api"; +import type { AuthDialogState } from "../appState"; import { selectedMachineId, type GetState, type SetState } from "./types"; +type OAuthDialogState = Extract; + export interface AuthControllerDependencies { api?: typeof defaultApi; pollIntervalMs?: number; @@ -9,6 +12,8 @@ export interface AuthControllerDependencies { export class AuthController { private readonly api: typeof defaultApi; private readonly pollIntervalMs: number; + private oauthOperationGeneration = 0; + private pollGeneration = 0; private pollTimer: number | undefined; constructor( @@ -22,6 +27,7 @@ export class AuthController { } dispose(): void { + this.oauthOperationGeneration += 1; this.stopPolling(); } @@ -128,15 +134,22 @@ export class AuthController { if (dialog?.step !== "oauth") return; const request = dialog.flow.prompt ?? dialog.flow.select; if (request === undefined) return; + const operationGeneration = this.oauthOperationGeneration; + const flowId = dialog.flow.flowId; + const requestId = request.requestId; const responseValue = value ?? dialog.inputValue ?? ""; const clean = { ...dialog }; delete clean.error; this.setState({ authDialog: { ...clean, responding: true } }); try { - const flow = await this.api.respondOAuthFlow(dialog.flow.flowId, request.requestId, responseValue, selectedMachineId(this.getState())); + const flow = await this.api.respondOAuthFlow(flowId, requestId, responseValue, selectedMachineId(this.getState())); + const current = this.currentOAuthDialog(operationGeneration, flowId); + if (flow.flowId !== flowId || current === undefined || oauthRequestId(current.flow) !== requestId) return; this.updateOAuthFlow(flow); } catch (error) { - this.setState({ authDialog: { ...dialog, responding: false, error: String(error) } }); + const current = this.currentOAuthDialog(operationGeneration, flowId); + if (current === undefined || oauthRequestId(current.flow) !== requestId) return; + this.setState({ authDialog: { ...current, responding: false, error: String(error) } }); } } @@ -146,16 +159,18 @@ export class AuthController { this.closeDialog(); return; } - this.stopPolling(); - try { - await this.api.cancelOAuthFlow(dialog.flow.flowId, selectedMachineId(this.getState())); - } catch { - // Best-effort cancel. The dialog closes either way. - } + const flowId = dialog.flow.flowId; + const machineId = selectedMachineId(this.getState()); this.closeDialog(); + try { + await this.api.cancelOAuthFlow(flowId, machineId); + } catch { + // Best-effort cancel. The dialog is already closed either way. + } } closeDialog(): void { + this.oauthOperationGeneration += 1; this.stopPolling(); this.setState({ authDialog: undefined }); } @@ -183,12 +198,15 @@ export class AuthController { private async startOAuth(provider: AuthProviderOption): Promise { if (this.rejectRemoteOAuth("login", provider)) return; + const operationGeneration = ++this.oauthOperationGeneration; + this.stopPolling(); try { const flow = await this.api.startOAuthLogin(provider.id, selectedMachineId(this.getState())); + if (operationGeneration !== this.oauthOperationGeneration) return; this.updateOAuthFlow(flow); - this.startPolling(flow.flowId); + if (flow.status === "running") this.startPolling(flow.flowId); } catch (error) { - this.setState({ error: String(error) }); + if (operationGeneration === this.oauthOperationGeneration) this.setState({ error: String(error) }); } } @@ -207,11 +225,14 @@ export class AuthController { void this.refreshStatus(); return; } - if (flow.status === "error" || flow.status === "cancelled") this.stopPolling(); + if (flow.status === "error" || flow.status === "cancelled") { + this.oauthOperationGeneration += 1; + this.stopPolling(); + } const existing = this.getState().authDialog; const previousInput = existing?.step === "oauth" && existing.flow.flowId === flow.flowId ? existing.inputValue ?? "" : ""; - const previousRequestId = existing?.step === "oauth" ? existing.flow.prompt?.requestId ?? existing.flow.select?.requestId : undefined; - const newRequestId = flow.prompt?.requestId ?? flow.select?.requestId; + const previousRequestId = existing?.step === "oauth" ? oauthRequestId(existing.flow) : undefined; + const newRequestId = oauthRequestId(flow); const sameRequest = previousRequestId !== undefined && previousRequestId === newRequestId; const inputValue = sameRequest ? previousInput : ""; const responding = sameRequest && existing?.step === "oauth" ? existing.responding === true : false; @@ -220,29 +241,45 @@ export class AuthController { private startPolling(flowId: string): void { this.stopPolling(); - this.pollTimer = window.setInterval(() => { void this.poll(flowId); }, this.pollIntervalMs); + const operationGeneration = this.oauthOperationGeneration; + const pollGeneration = this.pollGeneration; + this.pollTimer = window.setInterval(() => { void this.poll(flowId, operationGeneration, pollGeneration); }, this.pollIntervalMs); } private stopPolling(): void { + this.pollGeneration += 1; if (this.pollTimer === undefined) return; window.clearInterval(this.pollTimer); this.pollTimer = undefined; } - private async poll(flowId: string): Promise { - const dialog = this.getState().authDialog; - if (dialog?.step !== "oauth" || dialog.flow.flowId !== flowId) { + private async poll(flowId: string, operationGeneration: number, pollGeneration: number): Promise { + if (pollGeneration !== this.pollGeneration) return; + const dialog = this.currentOAuthDialog(operationGeneration, flowId); + if (dialog === undefined) { this.stopPolling(); return; } + const requestId = oauthRequestId(dialog.flow); try { - this.updateOAuthFlow(await this.api.oauthFlow(flowId, selectedMachineId(this.getState()))); + const flow = await this.api.oauthFlow(flowId, selectedMachineId(this.getState())); + const current = this.currentOAuthDialog(operationGeneration, flowId); + if (flow.flowId !== flowId || pollGeneration !== this.pollGeneration || current === undefined || oauthRequestId(current.flow) !== requestId) return; + this.updateOAuthFlow(flow); } catch (error) { + const current = this.currentOAuthDialog(operationGeneration, flowId); + if (pollGeneration !== this.pollGeneration || current === undefined || oauthRequestId(current.flow) !== requestId) return; this.stopPolling(); - this.setState({ authDialog: { ...dialog, error: String(error) } }); + this.setState({ authDialog: { ...current, error: String(error) } }); } } + private currentOAuthDialog(operationGeneration: number, flowId: string): OAuthDialogState | undefined { + if (operationGeneration !== this.oauthOperationGeneration) return undefined; + const dialog = this.getState().authDialog; + return dialog?.step === "oauth" && dialog.flow.flowId === flowId ? dialog : undefined; + } + private async refreshStatus(): Promise { const session = this.session(); if (session === undefined) return; @@ -260,6 +297,10 @@ export class AuthController { } } +function oauthRequestId(flow: OAuthFlowState): string | undefined { + return flow.prompt?.requestId ?? flow.select?.requestId; +} + export function parseAuthSlashCommand(text: string): { command: "login" | "logout"; providerId?: string } | undefined { const trimmed = text.trim(); const match = /^\/(login|logout)(?:\s+(\S+))?\s*$/u.exec(trimmed);