diff --git a/src/codex/main-account.ts b/src/codex/main-account.ts index cd5cfb50ba..d43c27f61d 100644 --- a/src/codex/main-account.ts +++ b/src/codex/main-account.ts @@ -1,5 +1,5 @@ import { createHash } from "node:crypto"; -import { readFileSync } from "node:fs"; +import { readFileSync, statSync } from "node:fs"; import { join } from "node:path"; import { readCodexTokens } from "./auth-collision"; import { @@ -37,6 +37,16 @@ let beforeMainAuthJsonRenameForTests: (() => void) | null = null; type MainAuthJsonCredential = { path: string; rawSha256: string; + /** + * Filesystem identity of the file the hash was taken from (#2999). + * + * A content hash cannot tell "unchanged" from "replaced with a file that happens to + * hash the same", and more importantly it is read at a different instant than the + * rename. Carrying dev+ino lets the pre-rename guard ask the sharper question: is this + * still the same file, not merely one with the same bytes. `null` when the target could + * not be stat'ed, which is treated as "cannot prove identity" rather than "matches". + */ + identity: { dev: number; ino: number } | null; root: Record; tokens: Record; accessToken?: string; @@ -73,6 +83,22 @@ function sha256(value: string): string { return createHash("sha256").update(value).digest("hex"); } +/** + * Filesystem identity of a path, or null when it cannot be read. + * + * Null is deliberately NOT "matches anything": a caller that cannot prove identity must + * fail closed, because the whole point here is refusing to overwrite a file we can no + * longer vouch for. + */ +function statIdentity(path: string): { dev: number; ino: number } | null { + try { + const stat = statSync(path); + return { dev: Number(stat.dev), ino: Number(stat.ino) }; + } catch { + return null; + } +} + function readMainAuthJsonCredential(): MainAuthJsonCredential | null { const path = resolveWriteTarget(join(resolveCodexHomeDir(), "auth.json")); let raw: string; @@ -98,6 +124,7 @@ function readMainAuthJsonCredential(): MainAuthJsonCredential | null { return { path, rawSha256: sha256(raw), + identity: statIdentity(path), root, tokens, ...(accessToken ? { accessToken } : {}), @@ -131,6 +158,21 @@ function assertMainAuthJsonSnapshotUnchanged(expected: MainAuthJsonCredential): if (!current || current.path !== expected.path || current.rawSha256 !== expected.rawSha256) { throw new MainAuthJsonChangedDuringRefreshError(); } + // Identity, not just content (#2999). A writer can land between this check and the + // rename, and rename(2) replaces unconditionally - so the narrower the question asked + // here, the smaller the window where a Codex login gets silently overwritten. An + // unreadable identity on either side fails closed: unprovable is not the same as equal. + assertMainAuthJsonIdentityUnchanged(expected); +} + +function assertMainAuthJsonIdentityUnchanged(expected: MainAuthJsonCredential): void { + const identity = statIdentity(expected.path); + if (!identity + || !expected.identity + || identity.dev !== expected.identity.dev + || identity.ino !== expected.identity.ino) { + throw new MainAuthJsonChangedDuringRefreshError(); + } } function persistRefreshedMainAuthJson( @@ -160,6 +202,9 @@ function persistRefreshedMainAuthJson( beforeMainAuthJsonRenameForTests = null; hook?.(); }, + // Runs immediately before rename(2), after the test hook has had its chance to + // simulate an external writer. Full snapshot check (content AND identity): this is + // the last look we get, so it asks everything it can rather than the cheap question. validateBeforeRename: () => assertMainAuthJsonSnapshotUnchanged(expected), }, ); diff --git a/tests/codex-main-account-refresh.test.ts b/tests/codex-main-account-refresh.test.ts index 08ff5c05bc..59a8ac6930 100644 --- a/tests/codex-main-account-refresh.test.ts +++ b/tests/codex-main-account-refresh.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdtempSync, readFileSync, readdirSync, renameSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -247,3 +247,84 @@ describe("native main token refresh", () => { } }); }); + +describe("publication never overwrites an external Codex writer (#2999)", () => { + const refreshOk = async () => ({ + access: "ocx-staged-access", + refresh: "ocx-staged-refresh", + expires: Date.now() + 3_600_000, + accountId: "account-main", + }); + + function seedExpired(authPath: string): void { + writeFileSync(authPath, JSON.stringify({ + auth_mode: "chatgpt", + tokens: { access_token: expiredJwt(), refresh_token: "old-refresh", account_id: "account-main" }, + })); + } + + test("a writer landing at the rename boundary is preserved byte-for-byte", async () => { + // Issue reproduction step 5: replace auth.json from a simulated Codex writer at the + // final pre-rename hook, then let the publisher resume. Before this guard the staged + // credential won and the user's own `codex login` result was silently replaced. + const authPath = join(home, "auth.json"); + seedExpired(authPath); + const external = JSON.stringify({ + auth_mode: "chatgpt", + tokens: { access_token: "codex-cli-wrote-this", refresh_token: "codex-refresh", account_id: "account-main" }, + }); + setMainAuthJsonBeforeRenameHookForTests(() => { writeFileSync(authPath, external); }); + + // Refusal surfaces as MainAuthJsonChangedDuringRefreshError, the existing signal for + // "the file moved under us" - the caller retries against the new state rather than + // proceeding with a credential it no longer owns. + await expect(getValidMainAccountToken({ refreshToken: refreshOk })).rejects.toThrow(); + + expect(readFileSync(authPath, "utf8")).toBe(external); + expect(readFileSync(authPath, "utf8")).not.toContain("ocx-staged-access"); + }); + + test("a same-bytes replacement with a new inode is still refused", async () => { + // The case a content hash cannot see. rename(2) replaces unconditionally, so the + // question that matters at the boundary is "is this the same FILE", not "does it hash + // the same" - an external writer that rewrote identical bytes still owns the target. + const authPath = join(home, "auth.json"); + seedExpired(authPath); + const identical = readFileSync(authPath, "utf8"); + setMainAuthJsonBeforeRenameHookForTests(() => { + // Replace via a distinct file so the inode changes while the bytes do not. + const swap = join(home, "swap.json"); + writeFileSync(swap, identical); + renameSync(swap, authPath); + }); + + await expect(getValidMainAccountToken({ refreshToken: refreshOk })).rejects.toThrow(); + + expect(readFileSync(authPath, "utf8")).toBe(identical); + expect(readFileSync(authPath, "utf8")).not.toContain("ocx-staged-access"); + }); + + test("the canonical target survives a refused publication", async () => { + // A refusal must never leave the credential missing: losing auth.json is worse than + // losing the refresh, because Codex CLI then has nothing to authenticate with. + const authPath = join(home, "auth.json"); + seedExpired(authPath); + setMainAuthJsonBeforeRenameHookForTests(() => { + writeFileSync(authPath, JSON.stringify({ auth_mode: "chatgpt", tokens: { access_token: "other" } })); + }); + + await expect(getValidMainAccountToken({ refreshToken: refreshOk })).rejects.toThrow(); + + expect(existsSync(authPath)).toBe(true); + expect(readdirSync(home).filter(name => name.startsWith("auth.json.")).length).toBe(0); + }); + + test("an uncontested publication still succeeds", async () => { + // The guard must not make the ordinary path fail closed. + const authPath = join(home, "auth.json"); + seedExpired(authPath); + const token = await getValidMainAccountToken({ refreshToken: refreshOk }); + expect(token?.accessToken).toBe("ocx-staged-access"); + expect(readFileSync(authPath, "utf8")).toContain("ocx-staged-access"); + }); +});