Skip to content

Commit 3bba8e7

Browse files
committed
fix(config): preserve symlinked destinations in atomic writes
Applies PR #869 (nicosuave) onto current dev plus one audit-folded amendment. atomicWriteFile/atomicWriteFileAsync wrote the temp beside the literal destination and renamed over it; rename(2) replaces the directory entry, so a symlinked destination (dotfiles-managed ~/.codex/config.toml) was silently converted to a plain file and the tracked repo stopped receiving writes (POSIX.1-2024 rename; Linux rename(2): the link will be overwritten). resolveWriteTarget realpaths the destination so temp and rename land beside the real file and the link survives; the test-only real-home guard is re-applied to the resolved target so a symlink escaping a fixture home into the protected home is still refused; the responses-state load sweeps stale temps in both the literal and the resolved directory. Audit amendment (wt4 wp2): a realpath failure no longer falls back to the literal path blindly. A genuinely absent destination keeps the literal first-write path, but an EXISTING unresolvable symlink (dangling target, unmounted volume, ELOOP, EACCES) is now refused and preserved instead of silently replaced; snapshot loading sweeps the literal dir only in that case. Tests: 202/202 config + responses-state + test-home-guard (sync+async link survival, no-temp-left, plain destination, first-write creation, dangling-link refusal, resolved-dir sweep, guard escape probe); typecheck green.
1 parent ab5a20c commit 3bba8e7

6 files changed

Lines changed: 274 additions & 11 deletions

File tree

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
# wp2 execution notes (P-phase stale check, 2026-08-02)
2+
3+
## Stale check results
4+
5+
- `atomicWriteFile` at `src/config.ts:107`, `atomicWriteFileAsync` at :187 — both confirmed in the pre-fix form (temp `${path}.ocx.${pid}.${seq}.tmp` beside the LITERAL path; `io.rename(tmp, path)`).
6+
- PR #869 diff (`/tmp/wt4-pr869.diff`, 336 lines) applies CLEAN to dev@478354ee8 + wp1 commit (`git apply --check` passes).
7+
- PR #869 state: OPEN, MERGEABLE, quality gates green, no maintainer blocker comments.
8+
- The diff's `src/responses/state.ts` hunk expects `enforceAppOwnedMemoryBudget` (77243d932) — present on current dev.
9+
10+
## PR #869 implementation shape (what will be applied)
11+
12+
1. `resolveWriteTarget(path)` (exported): `realpathSync(path)`, fallback to literal path when unresolvable (first write of a not-yet-created file).
13+
2. `assertResolvedTargetAllowed(path, target)`: re-applies the test-only real-home guard (`assertNotRealHomeUnderTest`) to the RESOLVED target — a symlink escaping a temp fixture home into the protected home is refused even though the caller's dir-level check passed. Inert in production.
14+
3. Both sync + async writers compute `tmp` beside the RESOLVED target and rename onto it.
15+
4. `src/responses/state.ts` snapshot load sweeps stale temps in BOTH the literal and the resolved directory (a symlinked snapshot strands temps in the real dir).
16+
5. Tests: `tests/config.test.ts` symlink suite (sync+async: link survives + target updated, no temps left, plain destination unaffected, first-write creation, dangling symlink replaced), `tests/responses-state.test.ts` (sweep in resolved dir), `tests/test-home-guard.test.ts` (escape-refused probe).
17+
18+
## Caller audit (criterion c5) — grep `atomicWriteFile` across `src/` @ dev 478354ee8
19+
20+
| Caller | Writes into | Symlink exposure |
21+
|---|---|---|
22+
| `src/config.ts` (owner; config.json :1627, pid :2070, runtime port :2098) | OPENCODEX_HOME | direct — config dir is dotfiles-managed in the reported case |
23+
| `src/oauth/store.ts` | OPENCODEX_HOME credential store | high-value target; fix protects token files behind symlinked dirs |
24+
| `src/codex/inject.ts`, `journal.ts`, `history-provider.ts`, `account-store.ts`, `quota.ts`, `refresh.ts`, `runtime.ts`, `features.ts` | `~/.codex/*` | the reported dotfiles case (`config.toml` symlink) |
25+
| `src/claude/desktop-3p.ts` | Claude Desktop config | user-managed file, symlink plausible |
26+
| `src/grok/inject.ts` | grok config | same shape |
27+
| `src/responses/state.ts` | OPENCODEX_HOME snapshot | covered by the sweep-in-both-dirs hunk |
28+
| `src/update/job.ts`, `notify.ts` | OPENCODEX_HOME update state | covered by shared helper |
29+
| `src/codex/catalog/*` (aggregation, bundled, effort, metadata, parsing, provider-fetch, sync) | catalog caches | covered by shared helper |
30+
31+
No caller needs an individual change: the fix lives in the shared writer + the one stale-temp sweep that scans directories.
32+
33+
## Known design decisions to audit
34+
35+
- Dangling symlink: realpath fails → literal path → rename REPLACES the dangling link (PR test asserts this). Debate point: silently replacing a dotfiles link whose target dir is temporarily unmounted. PR chose "replace"; the alternative (refuse) breaks first-write-into-new-target-dir.
36+
- TOCTOU: link swapped between realpath and rename lands the write at the old target. Accepted, documented in `010_implementation.md`; not claimed race-free.
37+
- Windows: `realpathSync` resolves junctions/symlinks on win32 too; temp stays same-volume because it sits beside the resolved target.
38+
- wt2 coordination: #840's memo-release touches `atomicWriteFileAsync` timeout-memo area; this diff does not overlap those lines (memo keyed by `path` argument, unchanged).

src/config.ts

Lines changed: 61 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
import { execFileSync } from "node:child_process";
22
import { randomUUID } from "node:crypto";
3-
import { chmodSync, copyFileSync, existsSync, linkSync, mkdirSync, readFileSync, renameSync, truncateSync, unlinkSync, writeFileSync } from "node:fs";
3+
import { chmodSync, copyFileSync, existsSync, linkSync, lstatSync, mkdirSync, readFileSync, realpathSync, renameSync, truncateSync, unlinkSync, writeFileSync } from "node:fs";
44
import { homedir } from "node:os";
5-
import { join, resolve } from "node:path";
5+
import { dirname, join, resolve } from "node:path";
66
import { Database } from "bun:sqlite";
77
import * as z from "zod/v4";
88
import {
@@ -104,6 +104,57 @@ function isMissingPathError(error: unknown): boolean {
104104
return (error as NodeJS.ErrnoException | undefined)?.code === "ENOENT";
105105
}
106106

107+
/**
108+
* Resolve a write target through any symlink before the temp+rename dance.
109+
*
110+
* rename(2) replaces a directory ENTRY. When the entry is itself a symlink
111+
* (a dotfiles-managed `~/.codex/config.toml` -> `~/dotfiles/.codex/config.toml`,
112+
* say), renaming a sibling temp file over it destroys the link and leaves a plain
113+
* file behind — the repo silently stops receiving writes. Resolving first puts both
114+
* the temp file and the rename target inside the link's real directory, so the entry
115+
* being replaced is the real file and the symlink survives.
116+
*
117+
* Same-filesystem atomicity is preserved because the temp file stays beside its
118+
* resolved target. A genuinely absent destination (not yet created) falls back to
119+
* the literal path, which is the correct target for a first write.
120+
*
121+
* An EXISTING symlink that cannot be resolved — dangling because its target volume
122+
* is unmounted, an ELOOP chain, an EACCES parent — is refused instead. Falling back
123+
* to the literal path there would let the rename replace the link, recreating the
124+
* exact dotfiles-divergence failure this helper exists to prevent (audit: wt4 wp2).
125+
*/
126+
export function resolveWriteTarget(path: string): string {
127+
try {
128+
return realpathSync(path);
129+
} catch (cause) {
130+
let entry;
131+
try {
132+
entry = lstatSync(path);
133+
} catch (error) {
134+
if (isMissingPathError(error)) return path; // no entry at all — first write
135+
throw error;
136+
}
137+
if (entry.isSymbolicLink()) {
138+
throw new Error(`refusing to replace unresolvable symlinked write target: ${path}`, { cause });
139+
}
140+
return path;
141+
}
142+
}
143+
144+
/**
145+
* Re-apply the real-home guard to a RESOLVED write target.
146+
*
147+
* Callers such as saveConfig check only their logical config dir, which passes when
148+
* OPENCODEX_HOME points at a temp fixture. Following a symlink out of that fixture
149+
* would land on the protected home the caller's own check just cleared, so the guard
150+
* has to run again on wherever the write actually terminates. Inert in production,
151+
* where the guard is disarmed.
152+
*/
153+
function assertResolvedTargetAllowed(path: string, target: string): void {
154+
if (target === path) return;
155+
assertNotRealHomeUnderTest(dirname(target));
156+
}
157+
107158
export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO = {
108159
write: (target, value) => writeFileSync(target, value, { encoding: "utf-8", mode: 0o600 }),
109160
harden: target => {
@@ -117,13 +168,15 @@ export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO
117168
unlink: unlinkSync,
118169
}): void {
119170
recordOwnedConfigPath(resolveConfigDir(), path);
120-
const tmp = `${path}.ocx.${process.pid}.${++_atomicSeq}.tmp`;
171+
const target = resolveWriteTarget(path);
172+
assertResolvedTargetAllowed(path, target);
173+
const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`;
121174
let hardened = false;
122175
try {
123176
io.write(tmp, content);
124177
io.harden(tmp);
125178
hardened = true;
126-
io.rename(tmp, path);
179+
io.rename(tmp, target);
127180
forgetEphemeralSecretPath(tmp);
128181
} catch (cause) {
129182
let scrubbed = false;
@@ -203,13 +256,15 @@ export async function atomicWriteFileAsync(
203256
truncate: target => truncateSync(target, 0),
204257
unlink: unlinkSync,
205258
};
206-
const tmp = `${path}.ocx.${process.pid}.${++_atomicSeq}.tmp`;
259+
const target = resolveWriteTarget(path);
260+
assertResolvedTargetAllowed(path, target);
261+
const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`;
207262
let hardened = false;
208263
try {
209264
await effective.write(tmp, content);
210265
await effective.harden(tmp);
211266
hardened = true;
212-
await effective.rename(tmp, path);
267+
await effective.rename(tmp, target);
213268
forgetEphemeralSecretPath(tmp);
214269
} catch (cause) {
215270
let scrubbed = false;

src/responses/state.ts

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { chmodSync, existsSync, lstatSync, mkdirSync, opendirSync, readFileSync, rmSync, statSync, unlinkSync } from "node:fs";
22
import { dirname, join } from "node:path";
3-
import { atomicWriteFileAsync, getConfigDir } from "../config";
3+
import { atomicWriteFileAsync, getConfigDir, resolveWriteTarget } from "../config";
44
import { enforceAppOwnedMemoryBudget, type RetainedStoreSnapshot } from "../lib/app-owned-memory";
55
import type { OcxProviderContinuationState } from "../types";
66
import {
@@ -567,10 +567,23 @@ function ensureLoaded(): void {
567567
if (loaded) return;
568568
loaded = true;
569569
const path = snapshotPath();
570+
// Atomic writes place their temp beside the RESOLVED target, so a symlinked
571+
// snapshot (dotfiles-managed config dir) strands temps in the link's real
572+
// directory where a scan of the literal config dir would never see them.
573+
// Both locations are swept; they collapse to one when nothing is symlinked.
574+
// resolveWriteTarget refuses a dangling link; snapshot loading stays independent.
575+
let resolvedDir = dirname(path);
570576
try {
571-
recoverStaleResponseStateTemps(dirname(path));
577+
resolvedDir = dirname(resolveWriteTarget(path));
572578
} catch {
573-
/* best-effort cleanup only; snapshot loading must remain independent */
579+
/* unresolvable link: sweep the literal dir only */
580+
}
581+
for (const dir of new Set([dirname(path), resolvedDir])) {
582+
try {
583+
recoverStaleResponseStateTemps(dir);
584+
} catch {
585+
/* best-effort cleanup only; snapshot loading must remain independent */
586+
}
574587
}
575588
try {
576589
if (existsSync(path)) {

tests/config.test.ts

Lines changed: 112 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test";
2-
import { chmodSync, existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, renameSync, rmSync, truncateSync, unlinkSync, writeFileSync } from "node:fs";
2+
import { chmodSync, existsSync, lstatSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, readlinkSync, renameSync, rmSync, symlinkSync, truncateSync, unlinkSync, writeFileSync } from "node:fs";
33
import { homedir, tmpdir } from "node:os";
44
import { join, resolve } from "node:path";
55
import {
@@ -27,7 +27,7 @@ import {
2727
} from "../src/config";
2828

2929
import * as windowsAcl from "../src/lib/windows-secret-acl";
30-
import { AtomicWriteResidualTempError, atomicWriteFile, hardenConfigDir, hardenExistingSecret, renameAtomicFile, saveConfig } from "../src/config";
30+
import { AtomicWriteResidualTempError, atomicWriteFile, atomicWriteFileAsync, hardenConfigDir, hardenExistingSecret, renameAtomicFile, saveConfig } from "../src/config";
3131
let testDir = "";
3232

3333
beforeEach(() => {
@@ -1738,5 +1738,115 @@ describe("config.ts – sync writer timeout keying (#840 refinement)", () => {
17381738
if (previousUsername === undefined) delete process.env.USERNAME;
17391739
else process.env.USERNAME = previousUsername;
17401740
}
1741+
||||||| parent of d8261a286 (fix(config): preserve symlinked destinations in atomic writes)
1742+
describe("config.ts – atomic writes preserve symlinked destinations", () => {
1743+
test("a symlinked destination survives the write and the real file receives it", () => {
1744+
// Dotfiles shape: ~/.codex/config.toml -> ~/dotfiles/.codex/config.toml
1745+
const repoDir = join(testDir, "dotfiles");
1746+
mkdirSync(repoDir, { recursive: true });
1747+
const realFile = join(repoDir, "config.toml");
1748+
writeFileSync(realFile, "original", "utf-8");
1749+
const link = join(testDir, "config.toml");
1750+
symlinkSync(realFile, link);
1751+
1752+
atomicWriteFile(link, "rewritten");
1753+
1754+
expect(lstatSync(link).isSymbolicLink()).toBe(true);
1755+
expect(readlinkSync(link)).toBe(realFile);
1756+
expect(readFileSync(realFile, "utf8")).toBe("rewritten");
1757+
expect(readFileSync(link, "utf8")).toBe("rewritten");
1758+
});
1759+
1760+
test("no temp file is left beside the link or its target", () => {
1761+
const repoDir = join(testDir, "dotfiles-clean");
1762+
mkdirSync(repoDir, { recursive: true });
1763+
const realFile = join(repoDir, "config.toml");
1764+
writeFileSync(realFile, "original", "utf-8");
1765+
const link = join(testDir, "config-clean.toml");
1766+
symlinkSync(realFile, link);
1767+
1768+
atomicWriteFile(link, "rewritten");
1769+
1770+
expect(readdirSync(repoDir).filter(name => name.includes(".ocx."))).toEqual([]);
1771+
expect(readdirSync(testDir).filter(name => name.includes(".ocx."))).toEqual([]);
1772+
});
1773+
1774+
test("a plain destination is unaffected", () => {
1775+
const destination = join(testDir, "plain.toml");
1776+
atomicWriteFile(destination, "first");
1777+
atomicWriteFile(destination, "second");
1778+
1779+
expect(lstatSync(destination).isSymbolicLink()).toBe(false);
1780+
expect(readFileSync(destination, "utf8")).toBe("second");
1781+
});
1782+
1783+
test("a destination that does not exist yet is created at the literal path", () => {
1784+
const destination = join(testDir, "created.toml");
1785+
expect(existsSync(destination)).toBe(false);
1786+
1787+
atomicWriteFile(destination, "fresh");
1788+
1789+
expect(readFileSync(destination, "utf8")).toBe("fresh");
1790+
});
1791+
1792+
test("a dangling symlink is preserved and the write is refused", () => {
1793+
const link = join(testDir, "dangling.toml");
1794+
symlinkSync(join(testDir, "gone", "config.toml"), link);
1795+
1796+
// The target volume may only be temporarily unavailable; replacing the link
1797+
// would recreate the dotfiles divergence this fix exists to prevent.
1798+
expect(() => atomicWriteFile(link, "recovered")).toThrow(/unresolvable symlinked write target/);
1799+
expect(lstatSync(link).isSymbolicLink()).toBe(true);
1800+
expect(existsSync(join(testDir, "gone"))).toBe(false);
1801+
});
1802+
});
1803+
1804+
describe("config.ts – async atomic writes preserve symlinked destinations", () => {
1805+
test("a symlinked destination survives the write and the real file receives it", async () => {
1806+
const repoDir = join(testDir, "dotfiles-async");
1807+
mkdirSync(repoDir, { recursive: true });
1808+
const realFile = join(repoDir, "config.toml");
1809+
writeFileSync(realFile, "original", "utf-8");
1810+
const link = join(testDir, "config-async.toml");
1811+
symlinkSync(realFile, link);
1812+
1813+
await atomicWriteFileAsync(link, "rewritten");
1814+
1815+
expect(lstatSync(link).isSymbolicLink()).toBe(true);
1816+
expect(readlinkSync(link)).toBe(realFile);
1817+
expect(readFileSync(realFile, "utf8")).toBe("rewritten");
1818+
expect(readFileSync(link, "utf8")).toBe("rewritten");
1819+
});
1820+
1821+
test("no temp file is left beside the link or its target", async () => {
1822+
const repoDir = join(testDir, "dotfiles-async-clean");
1823+
mkdirSync(repoDir, { recursive: true });
1824+
const realFile = join(repoDir, "config.toml");
1825+
writeFileSync(realFile, "original", "utf-8");
1826+
const link = join(testDir, "config-async-clean.toml");
1827+
symlinkSync(realFile, link);
1828+
1829+
await atomicWriteFileAsync(link, "rewritten");
1830+
1831+
expect(readdirSync(repoDir).filter(name => name.includes(".ocx."))).toEqual([]);
1832+
expect(readdirSync(testDir).filter(name => name.includes(".ocx."))).toEqual([]);
1833+
});
1834+
1835+
test("a plain destination is unaffected", async () => {
1836+
const destination = join(testDir, "plain-async.toml");
1837+
await atomicWriteFileAsync(destination, "first");
1838+
await atomicWriteFileAsync(destination, "second");
1839+
1840+
expect(lstatSync(destination).isSymbolicLink()).toBe(false);
1841+
expect(readFileSync(destination, "utf8")).toBe("second");
1842+
});
1843+
1844+
test("a dangling symlink is preserved and the write is refused", async () => {
1845+
const link = join(testDir, "dangling-async.toml");
1846+
symlinkSync(join(testDir, "gone-async", "config.toml"), link);
1847+
1848+
await expect(atomicWriteFileAsync(link, "recovered")).rejects.toThrow(/unresolvable symlinked write target/);
1849+
expect(lstatSync(link).isSymbolicLink()).toBe(true);
1850+
expect(existsSync(join(testDir, "gone-async"))).toBe(false);
17411851
});
17421852
});

tests/responses-state.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1449,6 +1449,27 @@ describe("Responses previous_response_id state", () => {
14491449
for (const path of [live, current, young, unrelated, directory]) expect(existsSync(path)).toBe(true);
14501450
});
14511451

1452+
test("load sweeps stale temps in a symlinked snapshot's real directory", () => {
1453+
// Atomic writes place their temp beside the RESOLVED target, so a dotfiles-managed
1454+
// config dir strands temps where a scan of the literal home would never find them.
1455+
const realDir = mkdtempSync(join(tmpdir(), "ocx-state-real-"));
1456+
const realSnapshot = join(realDir, "responses-state.json");
1457+
writeFileSync(realSnapshot, JSON.stringify({ version: 2, states: [] }));
1458+
symlinkSync(realSnapshot, join(home, "responses-state.json"));
1459+
1460+
const deadPid = process.pid === 4242 ? 4243 : 4242;
1461+
const stranded = join(realDir, `responses-state.json.ocx.${deadPid}.1.tmp`);
1462+
writeFileSync(stranded, "private state");
1463+
const old = new Date(Date.now() - 60 * 60 * 1_000);
1464+
utimesSync(stranded, old, old);
1465+
1466+
clearResponseStateMemoryForTests();
1467+
previousResponseProviderState("trigger-load");
1468+
1469+
expect(existsSync(stranded)).toBe(false);
1470+
rmSync(realDir, { recursive: true, force: true });
1471+
});
1472+
14521473
test("stale temp recovery is best-effort when unlink fails", () => {
14531474
const deadPid = process.pid === 4242 ? 4243 : 4242;
14541475
const path = join(home, `responses-state.json.ocx.${deadPid}.1.tmp`);

tests/test-home-guard.test.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,32 @@ describe("real-home write guard", () => {
9393
expect(() => readFileSync(join(opencodexHome, "codex-accounts.json"))).toThrow();
9494
});
9595

96+
test("armed + a symlink escaping a temp home into the protected home: refused", () => {
97+
// Atomic writes resolve their destination through symlinks, so a temp home whose
98+
// config.json points into the protected home would otherwise pass the caller's
99+
// dir-level check and then write the real file anyway.
100+
const { realHome, opencodexHome } = sentinelHome();
101+
const protectedFile = join(opencodexHome, "config.json");
102+
writeFileSync(protectedFile, '{"sentinel":true}', "utf8");
103+
const dir = mkdtempSync(join(tmpdir(), "ocx-escape-home-"));
104+
symlinkSync(protectedFile, join(dir, "config.json"));
105+
106+
const probe = runProbe(`
107+
import { saveConfig } from "${REPO_ROOT_URL}src/config";
108+
const REFUSAL = "refusing to write the real OpenCodex home";
109+
try {
110+
saveConfig({ providers: {}, defaultProvider: "openai", port: 10100 } as never);
111+
console.log("wrote");
112+
} catch (err) {
113+
console.log(String(err).includes(REFUSAL) ? "refused" : "other");
114+
}
115+
`, { OCX_TEST_HOME_GUARD: "1", OCX_REAL_HOME: realHome, OPENCODEX_HOME: dir });
116+
117+
expect(probe.stdout).toContain("refused");
118+
// The protected file must be byte-for-byte untouched.
119+
expect(readFileSync(protectedFile, "utf8")).toBe('{"sentinel":true}');
120+
});
121+
96122
test("armed + an unregistered temp home: writers succeed", () => {
97123
// The 54 suites that mkdtemp their own home must keep working with no opt-in.
98124
const dir = mkdtempSync(join(tmpdir(), "ocx-plain-home-"));

0 commit comments

Comments
 (0)