From 98b976032afb31437e38e90bc40820f8406ab1bf Mon Sep 17 00:00:00 2001 From: luvs01 Date: Fri, 18 Sep 2026 03:50:09 +0900 Subject: [PATCH 1/2] Scope Codex OAuth cancellation to the originating flowId The provider-keyed loginAbort map let a cancel request for an expired login modal abort a newer login flow for the same provider. Bind each attempt to its flowId, verify the pending flow before cancelling, and reject stale or missing flow ids with 400. --- src/codex/auth-api/login-flow.ts | 15 +++++-- src/oauth/index.ts | 19 +++++---- .../codex-integration/codex-auth-api.test.ts | 42 ++++++++++++------- 3 files changed, 48 insertions(+), 28 deletions(-) diff --git a/src/codex/auth-api/login-flow.ts b/src/codex/auth-api/login-flow.ts index fa6d1c6fbf7..a4fcefeee2e 100644 --- a/src/codex/auth-api/login-flow.ts +++ b/src/codex/auth-api/login-flow.ts @@ -216,7 +216,7 @@ export async function handleCodexAuthLoginStart(req: Request, config: OcxConfig, const result = await startLoginFlow("chatgpt", { forceLogin: true, ...(useDeviceFlow ? { flow: "device" as const } : {}), - }); + }, { flowId }); // Open the browser server-side (same pattern as /api/oauth/login in management-api.ts). // The GUI's window.open is popup-blocked because it runs after an await, not a direct click. @@ -527,10 +527,17 @@ export async function handleCodexAuthLoginCode(req: Request): Promise } export async function handleCodexAuthLoginCancel(req: Request): Promise { - const body = (await req.json().catch(() => ({}))) as { flowId?: string }; + const body = (await req.json().catch(() => ({}))) as { flowId?: unknown }; + const flowId = typeof body.flowId === "string" ? body.flowId.trim() : ""; + if (!flowId) return jsonResponse({ error: "flowId required" }, 400); const { cancelLoginFlow } = await import("../../oauth"); - const cancelled = cancelLoginFlow("chatgpt"); - expireCodexAuthFlow(body.flowId ?? null); + // Import may yield; validate afterwards so a stale modal cannot cancel a replacement flow. + const flow = codexAuthLoginState.get(flowId); + if (!flow || flow.status !== "pending") { + return jsonResponse({ error: "login flow expired or unknown" }, 400); + } + const cancelled = cancelLoginFlow("chatgpt", flowId); + expireCodexAuthFlow(flowId); return jsonResponse({ ok: true, cancelled }); } diff --git a/src/oauth/index.ts b/src/oauth/index.ts index 471e16534ca..5a946ff6b4f 100644 --- a/src/oauth/index.ts +++ b/src/oauth/index.ts @@ -177,6 +177,7 @@ export interface LoginOpts { } export interface LoginFlowLifecycle { + flowId?: string; /** Runs after background credential/config persistence settles, before status becomes done. */ onSettled?: () => void | Promise; } @@ -1711,7 +1712,7 @@ export async function runLogin( * submitManualLoginCode(), which feeds OAuthController.onManualCodeInput. */ const loginState = new Map(); -const loginAbort = new Map(); +const loginAbort = new Map(); const kiroLoginSettling = new Set(); /** Pending paste for a login in progress: either a waiter or a stashed early submission. */ @@ -1894,17 +1895,17 @@ export function oauthLoginSummary(maskEmails = true): Array<{ provider: string; } export function clearLoginState(provider: string): void { - loginAbort.get(provider)?.abort("cleared"); + loginAbort.get(provider)?.controller.abort("cleared"); loginAbort.delete(provider); clearManualCodeSlot(provider); loginState.delete(provider); } -export function cancelLoginFlow(provider: string): boolean { - const ctrl = loginAbort.get(provider); +export function cancelLoginFlow(provider: string, flowId?: string): boolean { + const active = loginAbort.get(provider); const existing = loginState.get(provider); - if (!ctrl && (!existing || existing.done)) return false; - ctrl?.abort("cancelled"); + if ((flowId !== undefined && active?.flowId !== flowId) || (!active && (!existing || existing.done))) return false; + active?.controller.abort("cancelled"); loginAbort.delete(provider); clearManualCodeSlot(provider); loginState.set(provider, { done: true, error: "Login cancelled" }); @@ -1925,7 +1926,7 @@ export async function startLoginFlow( clearManualCodeSlot(provider); loginState.set(provider, { done: false }); const abort = new AbortController(); - loginAbort.set(provider, abort); + loginAbort.set(provider, { controller: abort, flowId: lifecycle?.flowId }); if (provider === "kiro") kiroLoginSettling.add(provider); return new Promise((resolve, reject) => { let urlResolved = false; @@ -1940,7 +1941,7 @@ export async function startLoginFlow( signal: abort.signal, }; const abandonIfNotOwner = (error?: unknown): boolean => { - if (loginAbort.get(provider) === abort) return false; + if (loginAbort.get(provider)?.controller === abort) return false; if (!urlResolved) reject(error ?? new Error("OAuth login was superseded")); return true; }; @@ -1978,7 +1979,7 @@ export async function startLoginFlow( // Background: runLogin persists the credential + provider entry to disk. The lifecycle hook // lets a long-lived server config adopt that settled state before clients observe done=true. const assertCurrentOwner = (): void => { - if (loginAbort.get(provider) !== abort) throw new OAuthLoginSupersededError(); + if (loginAbort.get(provider)?.controller !== abort) throw new OAuthLoginSupersededError(); }; void runLogin(provider, ctrl, opts, { assertCurrentOwner }).then( () => settle(), diff --git a/tests/codex-integration/codex-auth-api.test.ts b/tests/codex-integration/codex-auth-api.test.ts index d02474985e4..ed8516b3d82 100644 --- a/tests/codex-integration/codex-auth-api.test.ts +++ b/tests/codex-integration/codex-auth-api.test.ts @@ -4807,21 +4807,6 @@ describe("codex-auth API", () => { } }); - test("POST /api/codex-auth/login/cancel expires the pending flow", async () => { - const flowId = "flow-cancel-test"; - const req = new Request("http://localhost/api/codex-auth/login/cancel", { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ flowId }), - }); - const resp = await handleCodexAuthAPI(req, new URL(req.url), {} as any); - expect(resp!.status).toBe(200); - const statusReq = new Request(`http://localhost/api/codex-auth/login-status?flowId=${flowId}`, { method: "GET" }); - const statusResp = await handleCodexAuthAPI(statusReq, new URL(statusReq.url), {} as any); - const data = await statusResp!.json() as { status: string; error?: string }; - expect(data).toMatchObject({ status: "error", error: "Login cancelled" }); - }); - describe("POST /api/codex-auth/login/code", () => { async function startPendingFlow() { const oauth = await import("../../src/oauth"); @@ -4864,6 +4849,33 @@ describe("codex-auth API", () => { }); } + test("cancels only the OAuth attempt owned by the pending flow", async () => { + const flow = await startPendingFlow(); + const cancelSpy = spyOn(flow.oauth, "cancelLoginFlow").mockReturnValue(true); + try { + const staleReq = new Request("http://localhost/api/codex-auth/login/cancel", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ flowId: "flow-from-old-modal" }), + }); + const staleResp = await handleCodexAuthAPI(staleReq, new URL(staleReq.url), makeConfig()); + expect(staleResp!.status).toBe(400); + expect(cancelSpy).not.toHaveBeenCalled(); + + const req = new Request("http://localhost/api/codex-auth/login/cancel", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ flowId: flow.flowId }), + }); + const resp = await handleCodexAuthAPI(req, new URL(req.url), makeConfig()); + expect(resp!.status).toBe(200); + expect(cancelSpy).toHaveBeenCalledWith("chatgpt", flow.flowId); + } finally { + cancelSpy.mockRestore(); + await flow.cleanup(); + } + }); + test("accepts a manual code only for the pending flow without reflecting it", async () => { const flow = await startPendingFlow(); const pasted = "http://localhost:1455/auth/callback?code=secret-code&state=expected"; From 3ed2ebf8f7edb5a3c8f72d08ac490f20b0200193 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Sat, 19 Sep 2026 15:38:05 +0900 Subject: [PATCH 2/2] fix(cli): require --flow for codex login cancel before the server 400 A bare ocx account cancel chatgpt posted { flowId: undefined } and surfaced as a bare 400. Refuse it as a usage error that names the flag ocx account login prints, matching the existing code-path requirement, and mark --flow as required for codex in the usage line. --- src/cli/account-auth.ts | 5 ++++- tests/cli/cli-account.test.ts | 11 +++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/cli/account-auth.ts b/src/cli/account-auth.ts index 73838f9da8e..bb31e066f0b 100644 --- a/src/cli/account-auth.ts +++ b/src/cli/account-auth.ts @@ -35,7 +35,7 @@ function writeStdoutFully(text: string): void { const USAGE = `Usage: ocx account login [--id ] [--reauth] [--device] [--code -] [--no-wait] [--json] ocx account code [--flow ] [--json] (reads the code from stdin) - ocx account cancel [--flow ] [--json] + ocx account cancel [--flow ] [--json] (--flow is required for codex) ocx account reset-credits [--consume --yes [--operation-id ]] [--json] ocx account grok-reset-coupons [] [--consume --yes [--token-id ] [--operation-id ]] [--json] @@ -266,6 +266,9 @@ async function cancel(argv: string[], deps: RuntimeApiDeps): Promise { if (!provider) throw new CliUsageError("provider is required", USAGE); rejectArgs(args, USAGE); const codex = CODEX_NAMES.has(provider); + if (codex && !flowId) { + throw new CliUsageError("Codex login cancel requires --flow (printed by 'ocx account login').", USAGE); + } const result = await runtimeRequest(codex ? "/api/codex-auth/login/cancel" : "/api/oauth/login/cancel", { method: "POST", body: JSON.stringify(codex ? { flowId } : { provider }), diff --git a/tests/cli/cli-account.test.ts b/tests/cli/cli-account.test.ts index 7772accc49d..00fa4f71a90 100644 --- a/tests/cli/cli-account.test.ts +++ b/tests/cli/cli-account.test.ts @@ -1701,6 +1701,17 @@ describe("ocx account CLI (issue #180 matrix)", () => { expect(result.output).not.toContain("--flow"); }); + test("codex cancel without --flow is a usage error, not a server 400", async () => { + // The Codex route requires a flowId; sending { flowId: undefined } used + // to surface as a bare 400. Fail fast with the flag the login printed. + const before = requests.length; + const result = await run(["cancel", "chatgpt"]); + + expect(result.code).toBe(2); + expect(result.stderr).toContain("--flow"); + expect(requests).toHaveLength(before); + }); + test("an empty pipe is a usage error, not an empty credential POST", async () => { const before = requests.length; const result = await run(