From f7c5b7babed389e6ec2f75ef2ab23d96d4699214 Mon Sep 17 00:00:00 2001 From: integ Date: Tue, 25 Aug 2026 18:01:00 -0500 Subject: [PATCH] fix(auth): stop blaming another sign-in when a busy port blocks browser sign-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The busy-port error asserted a cause it never observed: it told the user to "close any other OpenAI sign-in (e.g. codex login)" on the sole evidence that ports 1455/1457 were occupied. Any process holding those ports produces the same bind failure, so the advice could send a user hunting for a codex login that was never running. The message now leads with what was actually observed, offers that sign-in as the first thing to check rather than the cause, and says any other process holding the ports will do it. Same defect class as the timeout message fixed in #146/#142, found while reviewing that change. Also corrects two comments that made false claims about this code: - callback-server.ts's header described itself as a "CLI fallback" whose "primary path" is a GUI server handling /oauth/callback. No GUI server exists and no /oauth/callback route exists anywhere; this server serves the callback path its caller registers and, since #141, is the sole receiver for browser sign-in. The only occurrences of "GUI" and "/oauth/callback" in src/ were inside that comment. The UI server that owned this module's job went with the strip to the Claude-to-OpenAI bridge (d01d0eb) — for the since-removed Antigravity flow, not for OpenAI, which used device code at the time. - openai.ts's post-callback state check was labelled "defense in depth", which understates it: this flow always passes expectedState, so the server rejects a mismatch with 400 before it can reach that branch. The guard is kept as a backstop, but the comment no longer implies it is what protects the flow today. The existing busy-port test asserted only a loose regex on the port numbers, which passed unchanged against the new wording. It now compares the Error for equality: toThrow(string) is a substring match in Vitest, so an assertion written that way still lets appended copy through unasserted — verified by appending prose to the message and watching the test stay green. --- src/oauth/callback-server.ts | 10 +++++++--- src/oauth/openai.ts | 10 +++++++--- tests/oauth-openai.test.ts | 9 ++++++++- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/oauth/callback-server.ts b/src/oauth/callback-server.ts index 3bccea7d..d99836e6 100644 --- a/src/oauth/callback-server.ts +++ b/src/oauth/callback-server.ts @@ -1,6 +1,10 @@ -// src/oauth/callback-server.ts — CLI fallback local callback server for PKCE OAuth flows. -// Primary path: the GUI server handles /oauth/callback when the UI is open. -// This is only used when running `clodex providers auth ` without the GUI. +// src/oauth/callback-server.ts — loopback callback server for PKCE OAuth flows. +// It serves the callback path the caller registered with the provider, on the loopback port the +// caller supplies, and hands the authorization code back to the flow. Today its only caller is +// browser sign-in (`clodex providers auth openai --browser`, or the Browser option in the +// interactive picker), and nothing else in src/ receives an OAuth callback. The UI server that +// used to own this module's job, for the since-removed Antigravity flow, went with the strip to +// the Claude-to-OpenAI bridge (d01d0eb). import http from 'node:http'; import { listenTcpServer, waitForTcpListener } from '../listener-ready.js'; diff --git a/src/oauth/openai.ts b/src/oauth/openai.ts index 3aa9c0e7..fcc3a4ea 100644 --- a/src/oauth/openai.ts +++ b/src/oauth/openai.ts @@ -191,8 +191,9 @@ export async function runOpenAiBrowserFlow( } catch (error) { if ((error as NodeJS.ErrnoException).code === 'EADDRINUSE') { throw new Error( - `Ports ${ports.join(' and ')} are in use — close any other OpenAI sign-in ` - + '(e.g. codex login) and try again.', + `Ports ${ports.join(' and ')} are in use — browser sign-in needs one free. ` + + 'Check for another OpenAI sign-in (e.g. `codex login`), or any other process ' + + 'holding them, then try again.', ); } throw new Error( @@ -206,7 +207,10 @@ export async function runOpenAiBrowserFlow( const params = await server.waitForCallback(opts?.timeoutMs); if (params.error) throw new Error(`OpenAI sign-in failed: ${params.error}`); if (!params.code) throw new Error('OpenAI sign-in returned no authorization code'); - // Defense in depth: the callback server already filters on expectedState. + // Unreachable as written: this flow always passes expectedState, so the callback server + // answers 400 and keeps waiting rather than delivering a mismatched state here. Kept as a + // backstop in case this flow ever stops passing it — but it is not what protects the flow + // today, so do not weaken the server-side check on the strength of this one. if (params.state !== state) { throw new Error('OpenAI sign-in returned a mismatched state — try again'); } diff --git a/tests/oauth-openai.test.ts b/tests/oauth-openai.test.ts index 02cc94f7..27737d86 100644 --- a/tests/oauth-openai.test.ts +++ b/tests/oauth-openai.test.ts @@ -239,8 +239,15 @@ describe('oauth/openai', () => { }), )); const busyPorts = blockers.map(srv => (srv.address() as { port: number }).port); + // Pin the message exactly, not as a substring: it must state the observed fact (the + // ports are busy) and never assert a cause we did not observe. toThrow(string) matches a + // substring, so appended copy would slip through — compare the Error for equality. await expect(runOpenAiBrowserFlow(vi.fn(), { ports: busyPorts, timeoutMs: 200 })) - .rejects.toThrow(new RegExp(`${busyPorts[0]} and ${busyPorts[1]} are in use`)); + .rejects.toThrowError(new Error( + `Ports ${busyPorts[0]} and ${busyPorts[1]} are in use — browser sign-in needs one free. ` + + 'Check for another OpenAI sign-in (e.g. `codex login`), or any other process ' + + 'holding them, then try again.', + )); } finally { for (const srv of blockers) srv.close(); dns.setDefaultResultOrder(previousOrder);