Skip to content

Commit 5bb6aa0

Browse files
committed
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.
1 parent c3c4e95 commit 5bb6aa0

5 files changed

Lines changed: 82 additions & 7 deletions

File tree

src/config.ts

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { execFileSync } from "node:child_process";
22
import { randomUUID } from "node:crypto";
33
import { chmodSync, copyFileSync, existsSync, linkSync, 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 {
@@ -118,14 +118,28 @@ function isMissingPathError(error: unknown): boolean {
118118
* resolved target. An unresolvable path (not yet created) falls back to the literal
119119
* path, which is the correct target for a first write.
120120
*/
121-
function resolveWriteTarget(path: string): string {
121+
export function resolveWriteTarget(path: string): string {
122122
try {
123123
return realpathSync(path);
124124
} catch {
125125
return path;
126126
}
127127
}
128128

129+
/**
130+
* Re-apply the real-home guard to a RESOLVED write target.
131+
*
132+
* Callers such as saveConfig check only their logical config dir, which passes when
133+
* OPENCODEX_HOME points at a temp fixture. Following a symlink out of that fixture
134+
* would land on the protected home the caller's own check just cleared, so the guard
135+
* has to run again on wherever the write actually terminates. Inert in production,
136+
* where the guard is disarmed.
137+
*/
138+
function assertResolvedTargetAllowed(path: string, target: string): void {
139+
if (target === path) return;
140+
assertNotRealHomeUnderTest(dirname(target));
141+
}
142+
129143
export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO = {
130144
write: (target, value) => writeFileSync(target, value, { encoding: "utf-8", mode: 0o600 }),
131145
harden: target => {
@@ -138,6 +152,7 @@ export function atomicWriteFile(path: string, content: string, io: AtomicWriteIO
138152
}): void {
139153
recordOwnedConfigPath(resolveConfigDir(), path);
140154
const target = resolveWriteTarget(path);
155+
assertResolvedTargetAllowed(path, target);
141156
const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`;
142157
let hardened = false;
143158
try {
@@ -225,6 +240,7 @@ export async function atomicWriteFileAsync(
225240
unlink: unlinkSync,
226241
};
227242
const target = resolveWriteTarget(path);
243+
assertResolvedTargetAllowed(path, target);
228244
const tmp = `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`;
229245
let hardened = false;
230246
try {

src/responses/state.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { chmodSync, existsSync, lstatSync, mkdirSync, opendirSync, readFileSync, rmSync, 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 {
@@ -454,10 +454,16 @@ function ensureLoaded(): void {
454454
if (loaded) return;
455455
loaded = true;
456456
const path = snapshotPath();
457-
try {
458-
recoverStaleResponseStateTemps(dirname(path));
459-
} catch {
460-
/* best-effort cleanup only; snapshot loading must remain independent */
457+
// Atomic writes place their temp beside the RESOLVED target, so a symlinked
458+
// snapshot (dotfiles-managed config dir) strands temps in the link's real
459+
// directory where a scan of the literal config dir would never see them.
460+
// Both locations are swept; they collapse to one when nothing is symlinked.
461+
for (const dir of new Set([dirname(path), dirname(resolveWriteTarget(path))])) {
462+
try {
463+
recoverStaleResponseStateTemps(dir);
464+
} catch {
465+
/* best-effort cleanup only; snapshot loading must remain independent */
466+
}
461467
}
462468
try {
463469
if (existsSync(path)) {

tests/config.test.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1726,6 +1726,9 @@ describe("config.ts – atomic writes preserve symlinked destinations", () => {
17261726

17271727
atomicWriteFile(link, "recovered");
17281728

1729+
// Reading through the link cannot distinguish "link replaced" from "link kept,
1730+
// target created", so assert the link itself is gone.
1731+
expect(lstatSync(link).isSymbolicLink()).toBe(false);
17291732
expect(readFileSync(link, "utf8")).toBe("recovered");
17301733
});
17311734
});
@@ -1776,6 +1779,9 @@ describe("config.ts – async atomic writes preserve symlinked destinations", ()
17761779

17771780
await atomicWriteFileAsync(link, "recovered");
17781781

1782+
// Reading through the link cannot distinguish "link replaced" from "link kept,
1783+
// target created", so assert the link itself is gone.
1784+
expect(lstatSync(link).isSymbolicLink()).toBe(false);
17791785
expect(readFileSync(link, "utf8")).toBe("recovered");
17801786
});
17811787
});

tests/responses-state.test.ts

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

1421+
test("load sweeps stale temps in a symlinked snapshot's real directory", () => {
1422+
// Atomic writes place their temp beside the RESOLVED target, so a dotfiles-managed
1423+
// config dir strands temps where a scan of the literal home would never find them.
1424+
const realDir = mkdtempSync(join(tmpdir(), "ocx-state-real-"));
1425+
const realSnapshot = join(realDir, "responses-state.json");
1426+
writeFileSync(realSnapshot, JSON.stringify({ version: 2, states: [] }));
1427+
symlinkSync(realSnapshot, join(home, "responses-state.json"));
1428+
1429+
const deadPid = process.pid === 4242 ? 4243 : 4242;
1430+
const stranded = join(realDir, `responses-state.json.ocx.${deadPid}.1.tmp`);
1431+
writeFileSync(stranded, "private state");
1432+
const old = new Date(Date.now() - 60 * 60 * 1_000);
1433+
utimesSync(stranded, old, old);
1434+
1435+
clearResponseStateMemoryForTests();
1436+
previousResponseProviderState("trigger-load");
1437+
1438+
expect(existsSync(stranded)).toBe(false);
1439+
rmSync(realDir, { recursive: true, force: true });
1440+
});
1441+
14211442
test("stale temp recovery is best-effort when unlink fails", () => {
14221443
const deadPid = process.pid === 4242 ? 4243 : 4242;
14231444
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)