From efc3910f70cf433e3208fe52e29c824e006ed523 Mon Sep 17 00:00:00 2001 From: Pim Feltkamp Date: Sun, 26 Apr 2026 17:22:22 +0200 Subject: [PATCH] Fix two bugs in the OAuth browser flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug 1: state validation ran AFTER the listener responded. The callback handler sent the success page to the browser unconditionally on a code+state pair, then runBrowserFlow validated state on the CLI side and threw if it didn't match. On a forged callback (CSRF attempt) the browser showed "✓ Logged in" while the terminal showed "✗ state mismatch" — contradictory UX, and counter-productive against the threat the state nonce is supposed to defend against. Move state validation INSIDE the listener: on mismatch, the listener now responds with the same 400 errorPage every other malformed callback gets, and the CLI rejects with the same error message as before. Browser and terminal now agree. Bug 2: exchangeCodeForToken had no timeout. The 5-minute listenForCallback timer fires only until the OAuth callback arrives. Once it does, the timer is cleared and the CLI proceeds to POST /oauth2/token with no deadline of its own. If the token endpoint stalls (slow network, blackholed TCP), the CLI hangs with no Ctrl-C affordance hint. Wrap the fetch + body read in an AbortController with a 30-second total deadline. Same body-read total-deadline class as the iter-10 (Node) and iter-19 (Ruby) SDK fixes — the body read can hang independently of the response headers, so the fix has to bracket both. Both fixes verified end-to-end: state-mismatch test confirms the CLI rejects with the right error after the listener serves a 400 errorPage; typecheck and build are clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/browser-flow.ts | 85 +++++++++++++++++++++++++++++----------- 1 file changed, 62 insertions(+), 23 deletions(-) diff --git a/src/auth/browser-flow.ts b/src/auth/browser-flow.ts index a605d51..fa9675d 100644 --- a/src/auth/browser-flow.ts +++ b/src/auth/browser-flow.ts @@ -25,6 +25,8 @@ export const CALLBACK_PORT = 18765; export const CALLBACK_PATH = "/callback"; const DEFAULT_SCOPES = "read manage trade user"; const DEFAULT_TIMEOUT_MS = 5 * 60 * 1000; +/** Total wall-clock deadline for the /oauth2/token POST including body read. */ +const TOKEN_EXCHANGE_TIMEOUT_MS = 30 * 1000; export interface BrowserFlowOptions { clientId: string; @@ -59,9 +61,10 @@ export async function runBrowserFlow( authUrl.searchParams.set("state", state); authUrl.searchParams.set("scope", scopes); - const { code, receivedState } = await listenForCallback({ + const { code } = await listenForCallback({ port: CALLBACK_PORT, path: CALLBACK_PATH, + expectedState: state, timeoutMs, onReady: () => { opts.onAuthUrl?.(authUrl.toString()); @@ -70,12 +73,6 @@ export async function runBrowserFlow( }, }); - if (receivedState !== state) { - throw new Error( - "OAuth state mismatch — possible CSRF attempt. Aborted without storing a token.", - ); - } - return exchangeCodeForToken({ tokenUrl: `${webUrl}/oauth2/token`, code, @@ -87,13 +84,14 @@ export async function runBrowserFlow( interface CallbackListenerArgs { port: number; path: string; + expectedState: string; timeoutMs: number; onReady: () => void; } function listenForCallback( args: CallbackListenerArgs, -): Promise<{ code: string; receivedState: string }> { +): Promise<{ code: string }> { return new Promise((resolve, reject) => { const server = http.createServer((req, res) => { if (!req.url) { @@ -127,10 +125,24 @@ function listenForCallback( return; } + // Validate state INSIDE the listener so the browser shows the same + // outcome as the CLI. Doing this after sending the success page + // would tell the user (or attacker) "✓ Logged in" while the CLI + // simultaneously aborts — misleading and security-adjacent. + if (receivedState !== args.expectedState) { + res.writeHead(400, { "content-type": "text/html; charset=utf-8" }); + res.end(errorPage("OAuth state mismatch — possible CSRF attempt. Aborted without storing a token.")); + cleanup(); + reject(new Error( + "OAuth state mismatch — possible CSRF attempt. Aborted without storing a token.", + )); + return; + } + res.writeHead(200, { "content-type": "text/html; charset=utf-8" }); res.end(successPage()); cleanup(); - resolve({ code, receivedState }); + resolve({ code }); }); const timer = setTimeout(() => { @@ -186,23 +198,50 @@ async function exchangeCodeForToken( // server accepts this because the app row has an empty client_secret // and bshaffer's `allow_public_clients` is true by default. + // Total deadline covers connect + headers + body read. AbortSignal.timeout + // would only cover until the response arrived; the body could still hang. + const controller = new AbortController(); + const timer = setTimeout( + () => controller.abort(new Error("token exchange timed out")), + TOKEN_EXCHANGE_TIMEOUT_MS, + ); + let res: Response; + let text: string; try { - res = await fetch(args.tokenUrl, { - method: "POST", - headers: { - "content-type": "application/x-www-form-urlencoded", - accept: "application/json", - }, - body: body.toString(), - }); - } catch (err) { - throw new Error( - `Could not reach the token endpoint (${args.tokenUrl}): ${(err as Error).message}`, - ); + try { + res = await fetch(args.tokenUrl, { + method: "POST", + headers: { + "content-type": "application/x-www-form-urlencoded", + accept: "application/json", + }, + body: body.toString(), + signal: controller.signal, + }); + } catch (err) { + if (controller.signal.aborted) { + throw new Error( + `Token exchange to ${args.tokenUrl} timed out after ${TOKEN_EXCHANGE_TIMEOUT_MS / 1000}s.`, + ); + } + throw new Error( + `Could not reach the token endpoint (${args.tokenUrl}): ${(err as Error).message}`, + ); + } + try { + text = await res.text(); + } catch (err) { + if (controller.signal.aborted) { + throw new Error( + `Token exchange to ${args.tokenUrl} timed out reading the response after ${TOKEN_EXCHANGE_TIMEOUT_MS / 1000}s.`, + ); + } + throw err; + } + } finally { + clearTimeout(timer); } - - const text = await res.text(); let parsed: Record | null = null; if (text.length > 0) { try {