From a391b3cdddfb48ac5d66e64b4f5d0f45e994c6e0 Mon Sep 17 00:00:00 2001 From: Nico Ritschel Date: Sat, 1 Aug 2026 16:27:42 -0700 Subject: [PATCH 1/3] fix: preserve symlinked destinations in atomic config writes atomicWriteFile and atomicWriteFileAsync wrote a temp file beside the literal destination path and renamed over it. rename(2) replaces a directory entry, so when the destination was itself a symlink the rename destroyed the link and left a plain file in its place. This breaks dotfiles-managed setups, where ~/.codex/config.toml is a symlink into a tracked repo. The first injected write silently converts it to a real file and the repo stops receiving updates. Nothing surfaces the divergence: the live config keeps working, so the stale repo copy looks current until someone diffs it. Resolve the destination through realpath before choosing the temp path. Both the temp file and the rename target then live inside the link's real directory, so the entry replaced is the real file and the link survives. Same-filesystem atomicity is preserved because the temp stays beside its resolved target, and an unresolvable path falls back to the literal path, which is correct for a first write and for a dangling link. --- src/config.ts | 34 ++++++++++++++++++++---- tests/config.test.ts | 62 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/src/config.ts b/src/config.ts index 723aca09e..ba803e6a1 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,6 +1,6 @@ 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 { Database } from "bun:sqlite"; @@ -104,6 +104,28 @@ 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. + */ +function resolveWriteTarget(path: string): string { + try { + return realpathSync(path); + } catch { + return path; + } +} + export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO = { write: (target, value) => writeFileSync(target, value, { encoding: "utf-8", mode: 0o600 }), harden: target => { @@ -115,13 +137,14 @@ 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); + 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 +224,14 @@ export async function atomicWriteFileAsync( truncate: target => truncateSync(target, 0), unlink: unlinkSync, }; - const tmp = `${path}.ocx.${process.pid}.${++_atomicSeq}.tmp`; + const target = resolveWriteTarget(path); + 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/tests/config.test.ts b/tests/config.test.ts index e6ed95a1b..981d35119 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 { @@ -1669,3 +1669,63 @@ 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"); + + expect(readFileSync(link, "utf8")).toBe("recovered"); + }); +}); From c3c4e95758a8262d6ae2d78d88e223952c2c95e4 Mon Sep 17 00:00:00 2001 From: Nico Ritschel Date: Sat, 1 Aug 2026 16:36:15 -0700 Subject: [PATCH 2/3] test: cover atomicWriteFileAsync symlink preservation The symlink regression tests only exercised the synchronous atomicWriteFile, so an async-only regression could have slipped through while the sync suite stayed green. Mirror all four cases against atomicWriteFileAsync: an existing symlink survives and its target receives the write, no temp file is left beside either the link or its target, a plain destination is unaffected, and a dangling symlink is replaced rather than followed into nothing. Verified by reverting the async resolveWriteTarget call in isolation -- the symlink case fails, and passes again once restored. --- tests/config.test.ts | 52 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/tests/config.test.ts b/tests/config.test.ts index 981d35119..63e2809d5 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -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(() => { @@ -1729,3 +1729,53 @@ describe("config.ts – atomic writes preserve symlinked destinations", () => { 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"); + + expect(readFileSync(link, "utf8")).toBe("recovered"); + }); +}); From 5bb6aa0a2355fca689b211699492003f6e92624a Mon Sep 17 00:00:00 2001 From: Nico Ritschel Date: Sat, 1 Aug 2026 16:49:14 -0700 Subject: [PATCH 3/3] fix: guard resolved write targets and sweep their stale temps Two follow-ups from review of the symlink-preservation change. Re-check the real-home guard against the resolved target. Callers such as saveConfig validate only their logical config dir, which passes when OPENCODEX_HOME points at a temp fixture. Resolving a symlink out of that fixture could then land on the protected home the caller had just cleared, so the guard now runs again on wherever the write actually terminates. Inert in production, where the guard is disarmed. Sweep stale response-state temps in the resolved directory too. Temps are created beside the resolved target, so a symlinked responses-state.json stranded them in the link's real directory while startup recovery only scanned the literal config dir. A crash between write and rename would have left a private snapshot temp there permanently. Both directories are now swept, collapsing to one when nothing is symlinked. Also assert the dangling-symlink cases replace the link rather than following it: reading through the link passed either way. --- src/config.ts | 20 ++++++++++++++++++-- src/responses/state.ts | 16 +++++++++++----- tests/config.test.ts | 6 ++++++ tests/responses-state.test.ts | 21 +++++++++++++++++++++ tests/test-home-guard.test.ts | 26 ++++++++++++++++++++++++++ 5 files changed, 82 insertions(+), 7 deletions(-) diff --git a/src/config.ts b/src/config.ts index ba803e6a1..d7e622e79 100644 --- a/src/config.ts +++ b/src/config.ts @@ -2,7 +2,7 @@ import { execFileSync } from "node:child_process"; import { randomUUID } from "node:crypto"; 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 { @@ -118,7 +118,7 @@ function isMissingPathError(error: unknown): boolean { * resolved target. An unresolvable path (not yet created) falls back to the literal * path, which is the correct target for a first write. */ -function resolveWriteTarget(path: string): string { +export function resolveWriteTarget(path: string): string { try { return realpathSync(path); } catch { @@ -126,6 +126,20 @@ function resolveWriteTarget(path: string): string { } } +/** + * 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 => { @@ -138,6 +152,7 @@ export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO }): void { recordOwnedConfigPath(resolveConfigDir(), path); const target = resolveWriteTarget(path); + assertResolvedTargetAllowed(path, target); const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`; let hardened = false; try { @@ -225,6 +240,7 @@ export async function atomicWriteFileAsync( unlink: unlinkSync, }; const target = resolveWriteTarget(path); + assertResolvedTargetAllowed(path, target); const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`; let hardened = false; try { diff --git a/src/responses/state.ts b/src/responses/state.ts index 4a39647dc..9bfa59470 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 63e2809d5..e92956deb 100644 --- a/tests/config.test.ts +++ b/tests/config.test.ts @@ -1726,6 +1726,9 @@ describe("config.ts – atomic writes preserve symlinked destinations", () => { 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"); }); }); @@ -1776,6 +1779,9 @@ describe("config.ts – async atomic writes preserve symlinked destinations", () 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 337b22910..550ad8199 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 7c5958d5c..9944f794e 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-"));