diff --git a/src/config.ts b/src/config.ts index 723aca09e7..d7e622e795 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,8 +1,8 @@ import { execFileSync } from "node:child_process"; import { randomUUID } from "node:crypto"; -import { chmodSync, copyFileSync, existsSync, linkSync, mkdirSync, readFileSync, renameSync, truncateSync, unlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, copyFileSync, existsSync, linkSync, mkdirSync, readFileSync, realpathSync, renameSync, truncateSync, unlinkSync, writeFileSync } from "node:fs"; import { homedir } from "node:os"; -import { join, resolve } from "node:path"; +import { dirname, join, resolve } from "node:path"; import { Database } from "bun:sqlite"; import * as z from "zod/v4"; import { @@ -104,6 +104,42 @@ function isMissingPathError(error: unknown): boolean { return (error as NodeJS.ErrnoException | undefined)?.code === "ENOENT"; } +/** + * Resolve a write target through any symlink before the temp+rename dance. + * + * rename(2) replaces a directory ENTRY. When the entry is itself a symlink + * (a dotfiles-managed `~/.codex/config.toml` -> `~/dotfiles/.codex/config.toml`, + * say), renaming a sibling temp file over it destroys the link and leaves a plain + * file behind — the repo silently stops receiving writes. Resolving first puts both + * the temp file and the rename target inside the link's real directory, so the entry + * being replaced is the real file and the symlink survives. + * + * Same-filesystem atomicity is preserved because the temp file stays beside its + * resolved target. An unresolvable path (not yet created) falls back to the literal + * path, which is the correct target for a first write. + */ +export function resolveWriteTarget(path: string): string { + try { + return realpathSync(path); + } catch { + return path; + } +} + +/** + * Re-apply the real-home guard to a RESOLVED write target. + * + * Callers such as saveConfig check only their logical config dir, which passes when + * OPENCODEX_HOME points at a temp fixture. Following a symlink out of that fixture + * would land on the protected home the caller's own check just cleared, so the guard + * has to run again on wherever the write actually terminates. Inert in production, + * where the guard is disarmed. + */ +function assertResolvedTargetAllowed(path: string, target: string): void { + if (target === path) return; + assertNotRealHomeUnderTest(dirname(target)); +} + export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO = { write: (target, value) => writeFileSync(target, value, { encoding: "utf-8", mode: 0o600 }), harden: target => { @@ -115,13 +151,15 @@ export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO unlink: unlinkSync, }): void { recordOwnedConfigPath(resolveConfigDir(), path); - const tmp = `${path}.ocx.${process.pid}.${++_atomicSeq}.tmp`; + const target = resolveWriteTarget(path); + assertResolvedTargetAllowed(path, target); + const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`; let hardened = false; try { io.write(tmp, content); io.harden(tmp); hardened = true; - io.rename(tmp, path); + io.rename(tmp, target); forgetHardenedSecretPath(tmp); } catch (cause) { let scrubbed = false; @@ -201,13 +239,15 @@ export async function atomicWriteFileAsync( truncate: target => truncateSync(target, 0), unlink: unlinkSync, }; - const tmp = `${path}.ocx.${process.pid}.${++_atomicSeq}.tmp`; + const target = resolveWriteTarget(path); + assertResolvedTargetAllowed(path, target); + const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`; let hardened = false; try { await effective.write(tmp, content); await effective.harden(tmp); hardened = true; - await effective.rename(tmp, path); + await effective.rename(tmp, target); forgetHardenedSecretPath(tmp); } catch (cause) { let scrubbed = false; diff --git a/src/responses/state.ts b/src/responses/state.ts index 4a39647dc8..9bfa594701 100644 --- a/src/responses/state.ts +++ b/src/responses/state.ts @@ -1,6 +1,6 @@ import { chmodSync, existsSync, lstatSync, mkdirSync, opendirSync, readFileSync, rmSync, unlinkSync } from "node:fs"; import { dirname, join } from "node:path"; -import { atomicWriteFileAsync, getConfigDir } from "../config"; +import { atomicWriteFileAsync, getConfigDir, resolveWriteTarget } from "../config"; import { enforceAppOwnedMemoryBudget, type RetainedStoreSnapshot } from "../lib/app-owned-memory"; import type { OcxProviderContinuationState } from "../types"; import { @@ -454,10 +454,16 @@ function ensureLoaded(): void { if (loaded) return; loaded = true; const path = snapshotPath(); - try { - recoverStaleResponseStateTemps(dirname(path)); - } catch { - /* best-effort cleanup only; snapshot loading must remain independent */ + // Atomic writes place their temp beside the RESOLVED target, so a symlinked + // snapshot (dotfiles-managed config dir) strands temps in the link's real + // directory where a scan of the literal config dir would never see them. + // Both locations are swept; they collapse to one when nothing is symlinked. + for (const dir of new Set([dirname(path), dirname(resolveWriteTarget(path))])) { + try { + recoverStaleResponseStateTemps(dir); + } catch { + /* best-effort cleanup only; snapshot loading must remain independent */ + } } try { if (existsSync(path)) { diff --git a/tests/config.test.ts b/tests/config.test.ts index e6ed95a1b8..e92956deb2 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; -import { chmodSync, existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, renameSync, rmSync, truncateSync, unlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, readlinkSync, renameSync, rmSync, symlinkSync, truncateSync, unlinkSync, writeFileSync } from "node:fs"; import { homedir, tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { @@ -27,7 +27,7 @@ import { } from "../src/config"; import * as windowsAcl from "../src/lib/windows-secret-acl"; -import { AtomicWriteResidualTempError, atomicWriteFile, hardenConfigDir, hardenExistingSecret, renameAtomicFile, saveConfig } from "../src/config"; +import { AtomicWriteResidualTempError, atomicWriteFile, atomicWriteFileAsync, hardenConfigDir, hardenExistingSecret, renameAtomicFile, saveConfig } from "../src/config"; let testDir = ""; beforeEach(() => { @@ -1669,3 +1669,119 @@ describe("config.ts – Windows ACL hardening integration", () => { spy.mockRestore(); }); }); + +describe("config.ts – atomic writes preserve symlinked destinations", () => { + test("a symlinked destination survives the write and the real file receives it", () => { + // Dotfiles shape: ~/.codex/config.toml -> ~/dotfiles/.codex/config.toml + const repoDir = join(testDir, "dotfiles"); + mkdirSync(repoDir, { recursive: true }); + const realFile = join(repoDir, "config.toml"); + writeFileSync(realFile, "original", "utf-8"); + const link = join(testDir, "config.toml"); + symlinkSync(realFile, link); + + atomicWriteFile(link, "rewritten"); + + expect(lstatSync(link).isSymbolicLink()).toBe(true); + expect(readlinkSync(link)).toBe(realFile); + expect(readFileSync(realFile, "utf8")).toBe("rewritten"); + expect(readFileSync(link, "utf8")).toBe("rewritten"); + }); + + test("no temp file is left beside the link or its target", () => { + const repoDir = join(testDir, "dotfiles-clean"); + mkdirSync(repoDir, { recursive: true }); + const realFile = join(repoDir, "config.toml"); + writeFileSync(realFile, "original", "utf-8"); + const link = join(testDir, "config-clean.toml"); + symlinkSync(realFile, link); + + atomicWriteFile(link, "rewritten"); + + expect(readdirSync(repoDir).filter(name => name.includes(".ocx."))).toEqual([]); + expect(readdirSync(testDir).filter(name => name.includes(".ocx."))).toEqual([]); + }); + + test("a plain destination is unaffected", () => { + const destination = join(testDir, "plain.toml"); + atomicWriteFile(destination, "first"); + atomicWriteFile(destination, "second"); + + expect(lstatSync(destination).isSymbolicLink()).toBe(false); + expect(readFileSync(destination, "utf8")).toBe("second"); + }); + + test("a destination that does not exist yet is created at the literal path", () => { + const destination = join(testDir, "created.toml"); + expect(existsSync(destination)).toBe(false); + + atomicWriteFile(destination, "fresh"); + + expect(readFileSync(destination, "utf8")).toBe("fresh"); + }); + + test("a dangling symlink is replaced rather than followed into nothing", () => { + const link = join(testDir, "dangling.toml"); + symlinkSync(join(testDir, "gone", "config.toml"), link); + + atomicWriteFile(link, "recovered"); + + // Reading through the link cannot distinguish "link replaced" from "link kept, + // target created", so assert the link itself is gone. + expect(lstatSync(link).isSymbolicLink()).toBe(false); + expect(readFileSync(link, "utf8")).toBe("recovered"); + }); +}); + +describe("config.ts – async atomic writes preserve symlinked destinations", () => { + test("a symlinked destination survives the write and the real file receives it", async () => { + const repoDir = join(testDir, "dotfiles-async"); + mkdirSync(repoDir, { recursive: true }); + const realFile = join(repoDir, "config.toml"); + writeFileSync(realFile, "original", "utf-8"); + const link = join(testDir, "config-async.toml"); + symlinkSync(realFile, link); + + await atomicWriteFileAsync(link, "rewritten"); + + expect(lstatSync(link).isSymbolicLink()).toBe(true); + expect(readlinkSync(link)).toBe(realFile); + expect(readFileSync(realFile, "utf8")).toBe("rewritten"); + expect(readFileSync(link, "utf8")).toBe("rewritten"); + }); + + test("no temp file is left beside the link or its target", async () => { + const repoDir = join(testDir, "dotfiles-async-clean"); + mkdirSync(repoDir, { recursive: true }); + const realFile = join(repoDir, "config.toml"); + writeFileSync(realFile, "original", "utf-8"); + const link = join(testDir, "config-async-clean.toml"); + symlinkSync(realFile, link); + + await atomicWriteFileAsync(link, "rewritten"); + + expect(readdirSync(repoDir).filter(name => name.includes(".ocx."))).toEqual([]); + expect(readdirSync(testDir).filter(name => name.includes(".ocx."))).toEqual([]); + }); + + test("a plain destination is unaffected", async () => { + const destination = join(testDir, "plain-async.toml"); + await atomicWriteFileAsync(destination, "first"); + await atomicWriteFileAsync(destination, "second"); + + expect(lstatSync(destination).isSymbolicLink()).toBe(false); + expect(readFileSync(destination, "utf8")).toBe("second"); + }); + + test("a dangling symlink is replaced rather than followed into nothing", async () => { + const link = join(testDir, "dangling-async.toml"); + symlinkSync(join(testDir, "gone-async", "config.toml"), link); + + await atomicWriteFileAsync(link, "recovered"); + + // Reading through the link cannot distinguish "link replaced" from "link kept, + // target created", so assert the link itself is gone. + expect(lstatSync(link).isSymbolicLink()).toBe(false); + expect(readFileSync(link, "utf8")).toBe("recovered"); + }); +}); diff --git a/tests/responses-state.test.ts b/tests/responses-state.test.ts index 337b229108..550ad81998 100644 --- a/tests/responses-state.test.ts +++ b/tests/responses-state.test.ts @@ -1418,6 +1418,27 @@ describe("Responses previous_response_id state", () => { for (const path of [live, current, young, unrelated, directory]) expect(existsSync(path)).toBe(true); }); + test("load sweeps stale temps in a symlinked snapshot's real directory", () => { + // Atomic writes place their temp beside the RESOLVED target, so a dotfiles-managed + // config dir strands temps where a scan of the literal home would never find them. + const realDir = mkdtempSync(join(tmpdir(), "ocx-state-real-")); + const realSnapshot = join(realDir, "responses-state.json"); + writeFileSync(realSnapshot, JSON.stringify({ version: 2, states: [] })); + symlinkSync(realSnapshot, join(home, "responses-state.json")); + + const deadPid = process.pid === 4242 ? 4243 : 4242; + const stranded = join(realDir, `responses-state.json.ocx.${deadPid}.1.tmp`); + writeFileSync(stranded, "private state"); + const old = new Date(Date.now() - 60 * 60 * 1_000); + utimesSync(stranded, old, old); + + clearResponseStateMemoryForTests(); + previousResponseProviderState("trigger-load"); + + expect(existsSync(stranded)).toBe(false); + rmSync(realDir, { recursive: true, force: true }); + }); + test("stale temp recovery is best-effort when unlink fails", () => { const deadPid = process.pid === 4242 ? 4243 : 4242; const path = join(home, `responses-state.json.ocx.${deadPid}.1.tmp`); diff --git a/tests/test-home-guard.test.ts b/tests/test-home-guard.test.ts index 7c5958d5cc..9944f794e6 100644 --- a/tests/test-home-guard.test.ts +++ b/tests/test-home-guard.test.ts @@ -93,6 +93,32 @@ describe("real-home write guard", () => { expect(() => readFileSync(join(opencodexHome, "codex-accounts.json"))).toThrow(); }); + test("armed + a symlink escaping a temp home into the protected home: refused", () => { + // Atomic writes resolve their destination through symlinks, so a temp home whose + // config.json points into the protected home would otherwise pass the caller's + // dir-level check and then write the real file anyway. + const { realHome, opencodexHome } = sentinelHome(); + const protectedFile = join(opencodexHome, "config.json"); + writeFileSync(protectedFile, '{"sentinel":true}', "utf8"); + const dir = mkdtempSync(join(tmpdir(), "ocx-escape-home-")); + symlinkSync(protectedFile, join(dir, "config.json")); + + const probe = runProbe(` + import { saveConfig } from "${REPO_ROOT_URL}src/config"; + const REFUSAL = "refusing to write the real OpenCodex home"; + try { + saveConfig({ providers: {}, defaultProvider: "openai", port: 10100 } as never); + console.log("wrote"); + } catch (err) { + console.log(String(err).includes(REFUSAL) ? "refused" : "other"); + } + `, { OCX_TEST_HOME_GUARD: "1", OCX_REAL_HOME: realHome, OPENCODEX_HOME: dir }); + + expect(probe.stdout).toContain("refused"); + // The protected file must be byte-for-byte untouched. + expect(readFileSync(protectedFile, "utf8")).toBe('{"sentinel":true}'); + }); + test("armed + an unregistered temp home: writers succeed", () => { // The 54 suites that mkdtemp their own home must keep working with no opt-in. const dir = mkdtempSync(join(tmpdir(), "ocx-plain-home-"));