fix(auth): redact device-code logs

Author: ndycode

lib/auth/device-code.ts

@@ -5,6 +5,7 @@ import { AUTHORIZE_URL, CLIENT_ID, exchangeAuthorizationCode } from "./auth.js";
 
 const DEFAULT_DEVICE_CODE_MAX_WAIT_MS = 15 * 60 * 1000;
 const DEFAULT_DEVICE_CODE_INTERVAL_SECONDS = 5;
+const DEVICE_CODE_LOG_BODY_LIMIT = 120;
 
 export interface DeviceCodeSession {
 	verificationUrl: string;
@@ -62,6 +63,21 @@ function getErrorMessage(status: number, bodyText: string, fallback: string): st
 	return `${fallback} (${status}): ${trimmed}`;
 }
 
+// Auth-server error bodies can echo identifiers like device_auth_id, so logs keep
+// only a short prefix while the user-facing message still gets the full response.
+function getRedactedErrorBody(bodyText: string): string {
+	const trimmed = bodyText.trim();
+	if (!trimmed) {
+		return "<empty>";
+	}
+
+	if (trimmed.length <= DEVICE_CODE_LOG_BODY_LIMIT) {
+		return trimmed;
+	}
+
+	return `${trimmed.slice(0, DEVICE_CODE_LOG_BODY_LIMIT)}...`;
+}
+
 function parseIntervalSeconds(value: unknown): number {
 	if (typeof value === "number" && Number.isFinite(value) && value > 0) {
 		return Math.max(1, Math.floor(value));
@@ -154,7 +170,9 @@ export async function createDeviceCodeSession(options?: {
 						bodyText,
 						"Device code login could not be started",
 					);
-			logError(`device-code usercode request failed: ${response.status} ${bodyText}`);
+			logError(
+				`device-code usercode request failed: ${response.status} ${getRedactedErrorBody(bodyText)}`,
+			);
 			return {
 				type: "failed",
 				failure: {
@@ -251,7 +269,9 @@ export async function completeDeviceCodeSession(
 			}
 
 			const bodyText = await response.text().catch(() => "");
-			logError(`device-code token poll failed: ${response.status} ${bodyText}`);
+			logError(
+				`device-code token poll failed: ${response.status} ${getRedactedErrorBody(bodyText)}`,
+			);
 			return {
 				type: "failed",
 				reason: "http_error",

lib/auth/login-runner.ts

@@ -104,6 +104,13 @@ export function resolveAccountSelection(tokens: TokenSuccess): AccountSelectionR
 	};
 }
 
+/**
+ * Persists login results through the shared storage transaction so overlapping
+ * login retries serialize their read-modify-write cycle instead of racing stale
+ * snapshots. `withAccountStorageTransaction` also routes the final rename
+ * through the Windows lock retry path in `lib/storage.ts`; see
+ * `test/login-runner.test.ts` for the concurrent persist regression coverage.
+ */
 export async function persistAccountPool(
 	results: TokenSuccessWithAccount[],
 	replaceAll: boolean = false,

test/device-code.test.ts

@@ -21,6 +21,7 @@ import {
 	createDeviceCodeSession,
 } from "../lib/auth/device-code.js";
 import { exchangeAuthorizationCode } from "../lib/auth/auth.js";
+import { logError } from "../lib/logger.js";
 
 describe("device-code auth", () => {
 	beforeEach(() => {
@@ -72,6 +73,20 @@ describe("device-code auth", () => {
 		}
 	});
 
+	it("redacts auth-server bodies before logging usercode failures", async () => {
+		const sensitiveBody = `device_auth_id=secret-${"x".repeat(160)}`;
+		globalThis.fetch = vi.fn(async () => new Response(sensitiveBody, { status: 500 })) as typeof fetch;
+
+		const result = await createDeviceCodeSession();
+
+		expect(result.type).toBe("failed");
+		expect(vi.mocked(logError)).toHaveBeenCalledTimes(1);
+		const [message] = vi.mocked(logError).mock.calls[0] ?? [];
+		expect(message).toContain("device-code usercode request failed: 500");
+		expect(message).not.toContain(sensitiveBody);
+		expect(message).toContain(sensitiveBody.slice(0, 120));
+	});
+
 	it("polls until an authorization code is issued and exchanges it", async () => {
 		globalThis.fetch = vi
 			.fn()
@@ -120,4 +135,23 @@ describe("device-code auth", () => {
 			message: "Device code authorization timed out after 15 minutes",
 		});
 	});
+
+	it("redacts auth-server bodies before logging poll failures", async () => {
+		const sensitiveBody = `authorization_context=secret-${"y".repeat(160)}`;
+		globalThis.fetch = vi.fn(async () => new Response(sensitiveBody, { status: 500 })) as typeof fetch;
+
+		const result = await completeDeviceCodeSession({
+			verificationUrl: "https://auth.openai.com/codex/device",
+			userCode: "ABCD-EFGH",
+			deviceAuthId: "device-auth-1",
+			intervalSeconds: 1,
+		});
+
+		expect(result.type).toBe("failed");
+		expect(vi.mocked(logError)).toHaveBeenCalledTimes(1);
+		const [message] = vi.mocked(logError).mock.calls[0] ?? [];
+		expect(message).toContain("device-code token poll failed: 500");
+		expect(message).not.toContain(sensitiveBody);
+		expect(message).toContain(sensitiveBody.slice(0, 120));
+	});
 });

test/login-runner.test.ts

@@ -0,0 +1,96 @@
+import { promises as fs } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import {
+	persistAccountPool,
+	type TokenSuccessWithAccount,
+} from "../lib/auth/login-runner.js";
+import { loadAccounts, setStoragePathDirect } from "../lib/storage.js";
+
+function createTokenResult(
+	accountId: string,
+	refreshToken: string,
+): TokenSuccessWithAccount {
+	return {
+		type: "success",
+		access: `access-${accountId}`,
+		refresh: refreshToken,
+		expires: Date.now() + 60_000,
+		accountIdOverride: accountId,
+		accountIdSource: "manual",
+		accountLabel: accountId,
+	};
+}
+
+describe("login-runner persistAccountPool", () => {
+	let testDir: string;
+	let storagePath: string;
+
+	beforeEach(async () => {
+		testDir = join(
+			tmpdir(),
+			`login-runner-${Date.now()}-${Math.random().toString(36).slice(2)}`,
+		);
+		storagePath = join(testDir, "openai-codex-accounts.json");
+		await fs.mkdir(testDir, { recursive: true });
+		setStoragePathDirect(storagePath);
+	});
+
+	afterEach(async () => {
+		setStoragePathDirect(null);
+		vi.restoreAllMocks();
+		await fs.rm(testDir, { recursive: true, force: true });
+	});
+
+	it("serializes overlapping login persists without losing accounts", async () => {
+		const originalRename = fs.rename.bind(fs);
+		let firstRenameReleased = false;
+		let resolveFirstRename: (() => void) | undefined;
+		const firstRenameBlocked = new Promise<void>((resolve) => {
+			resolveFirstRename = resolve;
+		});
+		let resolveFirstRenameStarted: (() => void) | undefined;
+		const firstRenameStarted = new Promise<void>((resolve) => {
+			resolveFirstRenameStarted = resolve;
+		});
+		let renameCount = 0;
+
+		const renameSpy = vi
+			.spyOn(fs, "rename")
+			.mockImplementation(async (sourcePath, destinationPath) => {
+				renameCount += 1;
+				if (renameCount === 1 && !firstRenameReleased) {
+					resolveFirstRenameStarted?.();
+					await firstRenameBlocked;
+					firstRenameReleased = true;
+				}
+				return originalRename(sourcePath, destinationPath);
+			});
+
+		try {
+			const firstPersist = persistAccountPool(
+				[createTokenResult("acct-a", "refresh-a")],
+				false,
+			);
+			await firstRenameStarted;
+
+			const secondPersist = persistAccountPool(
+				[createTokenResult("acct-b", "refresh-b")],
+				false,
+			);
+
+			resolveFirstRename?.();
+			await Promise.all([firstPersist, secondPersist]);
+
+			expect(renameSpy).toHaveBeenCalledTimes(2);
+			const loaded = await loadAccounts();
+			expect(loaded?.accounts).toHaveLength(2);
+			expect(
+				new Set(loaded?.accounts.map((account) => account.accountId)),
+			).toEqual(new Set(["acct-a", "acct-b"]));
+		} finally {
+			resolveFirstRename?.();
+		}
+	});
+});