Skip to content

Commit 1c88c10

Browse files
ndycodendycode
committed
fix(auth): clarify manual callback flow
1 parent 6936351 commit 1c88c10

4 files changed

Lines changed: 147 additions & 32 deletions

File tree

‎lib/codex-manager.ts‎

Lines changed: 84 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1144,7 +1144,50 @@ async function promptManualCallback(
11441144
console.log("");
11451145
console.log(stylePromptText(UI_COPY.oauth.pastePrompt, "accent"));
11461146
}
1147-
const answer = await rl.question(useInteractivePrompt ? "◆ " : "");
1147+
const answer = useInteractivePrompt
1148+
? await rl.question("◆ ")
1149+
: await new Promise<string | null>((resolve, reject) => {
1150+
if (input.readableEnded || input.destroyed) {
1151+
resolve(null);
1152+
return;
1153+
}
1154+
let settled = false;
1155+
const handleInputClosed = () => {
1156+
if (settled) return;
1157+
settled = true;
1158+
input.off("end", handleInputClosed);
1159+
input.off("close", handleInputClosed);
1160+
resolve(null);
1161+
};
1162+
const finish = (value: string) => {
1163+
if (settled) return;
1164+
settled = true;
1165+
input.off("end", handleInputClosed);
1166+
input.off("close", handleInputClosed);
1167+
resolve(value);
1168+
};
1169+
const fail = (error: unknown) => {
1170+
if (settled) return;
1171+
settled = true;
1172+
input.off("end", handleInputClosed);
1173+
input.off("close", handleInputClosed);
1174+
reject(error);
1175+
};
1176+
rl.question("")
1177+
.then((value) => finish(value))
1178+
.catch((error) => {
1179+
if (isAbortError(error) || isReadlineClosedError(error)) {
1180+
handleInputClosed();
1181+
return;
1182+
}
1183+
fail(error);
1184+
});
1185+
input.once("end", handleInputClosed);
1186+
input.once("close", handleInputClosed);
1187+
});
1188+
if (answer === null) {
1189+
return null;
1190+
}
11481191
if (answer.includes("\u001b")) {
11491192
return null;
11501193
}
@@ -1163,7 +1206,7 @@ async function promptManualCallback(
11631206
if (parsed.state && parsed.state !== state) return null;
11641207
return parsed.code;
11651208
} catch (error) {
1166-
if (isAbortError(error)) {
1209+
if (isAbortError(error) || isReadlineClosedError(error)) {
11671210
return null;
11681211
}
11691212
throw error;
@@ -1172,6 +1215,17 @@ async function promptManualCallback(
11721215
}
11731216
}
11741217

1218+
function isReadlineClosedError(error: unknown): boolean {
1219+
if (!(error instanceof Error)) {
1220+
return false;
1221+
}
1222+
const errorCode =
1223+
typeof error === "object" && error !== null && "code" in error
1224+
? String((error as { code?: unknown }).code)
1225+
: "";
1226+
return errorCode === "ERR_USE_AFTER_CLOSE" || /readline was closed/i.test(error.message);
1227+
}
1228+
11751229
type OAuthSignInMode = "browser" | "manual" | "cancel";
11761230

11771231
async function promptOAuthSignInMode(): Promise<OAuthSignInMode> {
@@ -1471,28 +1525,8 @@ async function runOAuthFlow(
14711525
): Promise<TokenResult> {
14721526
const { pkce, state, url } = await createAuthorizationFlow({ forceNewLogin });
14731527
const preferManualMode = options.manual || isBrowserLaunchSuppressed();
1474-
let oauthServer: Awaited<ReturnType<typeof startLocalOAuthServer>> | null = null;
1475-
try {
1476-
oauthServer = await startLocalOAuthServer({ state });
1477-
} catch (serverError) {
1478-
log.warn(
1479-
"Local OAuth callback server unavailable; falling back to manual callback entry.",
1480-
serverError instanceof Error
1481-
? {
1482-
message: serverError.message,
1483-
stack: serverError.stack,
1484-
code:
1485-
typeof serverError === "object" &&
1486-
serverError !== null &&
1487-
"code" in serverError
1488-
? String(serverError.code)
1489-
: undefined,
1490-
}
1491-
: { error: String(serverError) },
1492-
);
1493-
oauthServer = null;
1494-
}
14951528
let code: string | null = null;
1529+
let oauthServer: Awaited<ReturnType<typeof startLocalOAuthServer>> | null = null;
14961530
try {
14971531
const signInMode = preferManualMode ? "manual" : await promptOAuthSignInMode();
14981532
if (signInMode === "cancel") {
@@ -1503,6 +1537,29 @@ async function runOAuthFlow(
15031537
};
15041538
}
15051539

1540+
if (signInMode === "browser") {
1541+
try {
1542+
oauthServer = await startLocalOAuthServer({ state });
1543+
} catch (serverError) {
1544+
log.warn(
1545+
"Local OAuth callback server unavailable; falling back to manual callback entry.",
1546+
serverError instanceof Error
1547+
? {
1548+
message: serverError.message,
1549+
stack: serverError.stack,
1550+
code:
1551+
typeof serverError === "object" &&
1552+
serverError !== null &&
1553+
"code" in serverError
1554+
? String(serverError.code)
1555+
: undefined,
1556+
}
1557+
: { error: String(serverError) },
1558+
);
1559+
oauthServer = null;
1560+
}
1561+
}
1562+
15061563
if (signInMode === "browser") {
15071564
const opened = openBrowserUrl(url);
15081565
if (opened) {
@@ -1529,7 +1586,7 @@ async function runOAuthFlow(
15291586
);
15301587
}
15311588

1532-
const waitingForCallback = signInMode === "browser" && oauthServer?.ready === true;
1589+
const waitingForCallback = oauthServer?.ready === true;
15331590
if (waitingForCallback && oauthServer) {
15341591
console.log(stylePromptText(UI_COPY.oauth.waitingCallback, "muted"));
15351592
const callbackResult = await oauthServer.waitForCode(state);
@@ -1541,7 +1598,9 @@ async function runOAuthFlow(
15411598
stylePromptText(
15421599
waitingForCallback
15431600
? UI_COPY.oauth.callbackMissed
1544-
: UI_COPY.oauth.callbackUnavailable,
1601+
: signInMode === "manual"
1602+
? UI_COPY.oauth.callbackBypassed
1603+
: UI_COPY.oauth.callbackUnavailable,
15451604
"warning",
15461605
),
15471606
);

‎lib/ui/copy.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ export const UI_COPY = {
4242
browserOpened: "Browser opened.",
4343
browserOpenFail: "Could not open browser. Use this link:",
4444
waitingCallback: "Waiting for login callback on localhost:1455...",
45+
callbackBypassed: "Manual mode active. Paste the callback URL manually.",
4546
callbackUnavailable: "Callback listener unavailable. Paste the callback URL manually.",
4647
callbackMissed: "No callback received. Paste manually.",
4748
cancelled: "Sign-in cancelled.",

‎test/browser.test.ts‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -106,21 +106,33 @@ describe("auth browser utilities", () => {
106106
});
107107

108108
it("lets explicit false-like CODEX_AUTH_NO_BROWSER override a disabling BROWSER value", () => {
109+
Object.defineProperty(process, "platform", { value: "darwin" });
110+
process.env.PATH = "/usr/bin";
109111
process.env.CODEX_AUTH_NO_BROWSER = "0";
110112
process.env.BROWSER = "none";
111113

112114
expect(isBrowserLaunchSuppressed()).toBe(false);
113-
expect(openBrowserUrl("https://example.com")).toBe(false);
114-
expect(mockedSpawn).not.toHaveBeenCalled();
115+
expect(openBrowserUrl("https://example.com")).toBe(true);
116+
expect(mockedSpawn).toHaveBeenCalledWith(
117+
"open",
118+
["https://example.com"],
119+
{ stdio: "ignore", shell: false },
120+
);
115121
});
116122

117123
it("does not treat CODEX_AUTH_NO_BROWSER=false as suppression when BROWSER is disabled", () => {
124+
Object.defineProperty(process, "platform", { value: "darwin" });
125+
process.env.PATH = "/usr/bin";
118126
process.env.CODEX_AUTH_NO_BROWSER = "false";
119127
process.env.BROWSER = "none";
120128

121129
expect(isBrowserLaunchSuppressed()).toBe(false);
122-
expect(openBrowserUrl("https://example.com")).toBe(false);
123-
expect(mockedSpawn).not.toHaveBeenCalled();
130+
expect(openBrowserUrl("https://example.com")).toBe(true);
131+
expect(mockedSpawn).toHaveBeenCalledWith(
132+
"open",
133+
["https://example.com"],
134+
{ stdio: "ignore", shell: false },
135+
);
124136
});
125137

126138
it("suppresses browser launch when BROWSER is set to none", () => {

‎test/codex-manager-cli.test.ts‎

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3008,8 +3008,9 @@ describe("codex manager cli commands", () => {
30083008

30093009
expect(exitCode).toBe(0);
30103010
expect(openBrowserUrlMock).not.toHaveBeenCalled();
3011+
expect(vi.mocked(serverModule.startLocalOAuthServer)).not.toHaveBeenCalled();
30113012
expect(waitForCodeMock).not.toHaveBeenCalled();
3012-
expect(renderedLogs.some((entry) => entry.includes("Callback listener unavailable"))).toBe(
3013+
expect(renderedLogs.some((entry) => entry.includes("Manual mode active"))).toBe(
30133014
true,
30143015
);
30153016
expect(renderedLogs.some((entry) => entry.includes("No callback received"))).toBe(false);
@@ -3069,8 +3070,9 @@ describe("codex manager cli commands", () => {
30693070
expect(exitCode).toBe(0);
30703071
expect(selectMock).toHaveBeenCalled();
30713072
expect(openBrowserUrlMock).not.toHaveBeenCalled();
3073+
expect(vi.mocked(serverModule.startLocalOAuthServer)).not.toHaveBeenCalled();
30723074
expect(waitForCodeMock).not.toHaveBeenCalled();
3073-
expect(renderedLogs.some((entry) => entry.includes("Callback listener unavailable"))).toBe(
3075+
expect(renderedLogs.some((entry) => entry.includes("Manual mode active"))).toBe(
30743076
true,
30753077
);
30763078
expect(renderedLogs.some((entry) => entry.includes("No callback received"))).toBe(false);
@@ -3124,7 +3126,7 @@ describe("codex manager cli commands", () => {
31243126
expect(exitCode).toBe(0);
31253127
expect(promptQuestionMock).toHaveBeenCalled();
31263128
expect(openBrowserUrlMock).not.toHaveBeenCalled();
3127-
expect(startLocalOAuthServerMock).toHaveBeenCalledTimes(1);
3129+
expect(startLocalOAuthServerMock).not.toHaveBeenCalled();
31283130
expect(storageState.accounts).toHaveLength(1);
31293131
});
31303132

@@ -3174,6 +3176,7 @@ describe("codex manager cli commands", () => {
31743176
expect(exitCode).toBe(0);
31753177
expect(promptQuestionMock).toHaveBeenCalledWith("");
31763178
expect(openBrowserUrlMock).not.toHaveBeenCalled();
3179+
expect(vi.mocked(serverModule.startLocalOAuthServer)).not.toHaveBeenCalled();
31773180
expect(storageState.accounts).toHaveLength(1);
31783181
});
31793182

@@ -3215,6 +3218,7 @@ describe("codex manager cli commands", () => {
32153218
expect(exitCode).toBe(0);
32163219
expect(promptQuestionMock).toHaveBeenCalledWith("");
32173220
expect(openBrowserUrlMock).not.toHaveBeenCalled();
3221+
expect(vi.mocked(serverModule.startLocalOAuthServer)).not.toHaveBeenCalled();
32183222
expect(exchangeAuthorizationCodeMock).not.toHaveBeenCalled();
32193223
expect(storageState.accounts).toHaveLength(0);
32203224
});
@@ -3265,9 +3269,48 @@ describe("codex manager cli commands", () => {
32653269
expect(exitCode).toBe(0);
32663270
expect(promptQuestionMock).toHaveBeenCalledWith("");
32673271
expect(openBrowserUrlMock).not.toHaveBeenCalled();
3272+
expect(vi.mocked(serverModule.startLocalOAuthServer)).not.toHaveBeenCalled();
32683273
expect(storageState.accounts).toHaveLength(1);
32693274
});
32703275

3276+
it("returns to the menu when stdin closes without input in non-tty manual mode", async () => {
3277+
setInteractiveTTY(false);
3278+
let storageState = {
3279+
version: 3 as const,
3280+
activeIndex: 0,
3281+
activeIndexByFamily: { codex: 0 },
3282+
accounts: [] as Array<Record<string, unknown>>,
3283+
};
3284+
loadAccountsMock.mockImplementation(async () => structuredClone(storageState));
3285+
saveAccountsMock.mockImplementation(async (nextStorage) => {
3286+
storageState = structuredClone(nextStorage);
3287+
});
3288+
promptLoginModeMock.mockResolvedValueOnce({ mode: "cancel" });
3289+
promptQuestionMock.mockRejectedValueOnce(new Error("readline was closed"));
3290+
3291+
const authModule = await import("../lib/auth/auth.js");
3292+
vi.mocked(authModule.createAuthorizationFlow).mockResolvedValueOnce({
3293+
pkce: { challenge: "pkce-challenge", verifier: "pkce-verifier" },
3294+
state: "oauth-state",
3295+
url: "https://auth.openai.com/mock",
3296+
});
3297+
const exchangeAuthorizationCodeMock = vi.mocked(authModule.exchangeAuthorizationCode);
3298+
3299+
const browserModule = await import("../lib/auth/browser.js");
3300+
const openBrowserUrlMock = vi.mocked(browserModule.openBrowserUrl);
3301+
const serverModule = await import("../lib/auth/server.js");
3302+
3303+
const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js");
3304+
const exitCode = await runCodexMultiAuthCli(["auth", "login", "--manual"]);
3305+
3306+
expect(exitCode).toBe(0);
3307+
expect(promptQuestionMock).toHaveBeenCalledWith("");
3308+
expect(openBrowserUrlMock).not.toHaveBeenCalled();
3309+
expect(vi.mocked(serverModule.startLocalOAuthServer)).not.toHaveBeenCalled();
3310+
expect(exchangeAuthorizationCodeMock).not.toHaveBeenCalled();
3311+
expect(storageState.accounts).toHaveLength(0);
3312+
});
3313+
32713314
it("preserves distinct same-email workspaces when oauth login reuses a refresh token", async () => {
32723315
const now = Date.now();
32733316
let storageState = {

0 commit comments

Comments
 (0)