atomicWrite: follows symlinks through a predictable temp path, and collides under parallelism #115

New issue

Closed

opened 2026-08-13 15:43:25 +00:00 by john · 1 comment

john commented

2026-08-13 15:43:25 +00:00

Owner

lib/planning-files.mjs:1808-1812:

export function atomicWrite(file, text) {
  const tmp = `${file}.tmp`;
  writeFileSync(tmp, text);
  renameSync(tmp, file);
}

writeFileSync follows symlinks, and tmp is a fixed, repo-relative, predictable path a hostile repo can pre-populate.

Proven

repo/ROADMAP.md.tmp planted as a symlink to ../target/victim.txt, then atomicWrite('repo/ROADMAP.md', 'PWNED CONTENT'):

victim.txt OUTSIDE the repo now contains PWNED CONTENT
ROADMAP.md is itself the symlink afterwards, so the redirect persists for every future write

Same primitive through STATE.md.tmp, config.json.tmp, CHANGELOG.md.tmp, .gitignore.tmp, .claude-plugin/plugin.json.tmp, phases/N/UAT.md.tmp, CAPTURE.md.tmp. lib/trace.mjs:278 appendEvent is the append-only variant of the same thing.

Chained with #114 it is worse: land a config.json.tmp symlink pointing at ~/.claude/settings.json and config.mjs set writes the whole object through, hooks block included.

The part that stings

The READ path already defends against exactly this. planning.mjs:2751-2758 lstats and skips symlinks in debt-harvest, with a comment saying why. The write path never got the same treatment.

Second defect, same function

The .tmp name is fixed, and parallelization.enabled defaults true. Two concurrent writers of one file share one temp path, which defeats the atomicity the function exists to provide. The header comment promises "a crash must never leave a torn file"; concurrent writers can do exactly that.

Fix

writeFileSync(tmp, text, { flag: 'wx' }). On EEXIST, lstatSync and refuse if it is a symlink; unlink a plain stale tmp and retry. Randomize the suffix while in there, which closes the collision too. Test both arms: a symlinked tmp must refuse rather than write through, and two writers must not share a path.

`lib/planning-files.mjs:1808-1812`: ```js export function atomicWrite(file, text) { const tmp = `${file}.tmp`; writeFileSync(tmp, text); renameSync(tmp, file); } ``` `writeFileSync` follows symlinks, and `tmp` is a fixed, repo-relative, predictable path a hostile repo can pre-populate. ## Proven `repo/ROADMAP.md.tmp` planted as a symlink to `../target/victim.txt`, then `atomicWrite('repo/ROADMAP.md', 'PWNED CONTENT')`: - `victim.txt` OUTSIDE the repo now contains `PWNED CONTENT` - `ROADMAP.md` is itself the symlink afterwards, so the redirect persists for every future write Same primitive through `STATE.md.tmp`, `config.json.tmp`, `CHANGELOG.md.tmp`, `.gitignore.tmp`, `.claude-plugin/plugin.json.tmp`, `phases/N/UAT.md.tmp`, `CAPTURE.md.tmp`. `lib/trace.mjs:278 appendEvent` is the append-only variant of the same thing. Chained with #114 it is worse: land a `config.json.tmp` symlink pointing at `~/.claude/settings.json` and `config.mjs set` writes the whole object through, hooks block included. ## The part that stings The READ path already defends against exactly this. `planning.mjs:2751-2758` lstats and skips symlinks in debt-harvest, with a comment saying why. The write path never got the same treatment. ## Second defect, same function The `.tmp` name is fixed, and `parallelization.enabled` defaults true. Two concurrent writers of one file share one temp path, which defeats the atomicity the function exists to provide. The header comment promises "a crash must never leave a torn file"; concurrent writers can do exactly that. ## Fix `writeFileSync(tmp, text, { flag: 'wx' })`. On `EEXIST`, `lstatSync` and refuse if it is a symlink; unlink a plain stale tmp and retry. Randomize the suffix while in there, which closes the collision too. Test both arms: a symlinked tmp must refuse rather than write through, and two writers must not share a path.

john added this to the v3.2.0 milestone

2026-08-13 15:43:25 +00:00

john added the

bug

security

labels

2026-08-13 15:43:25 +00:00

john closed this issue

2026-08-14 17:01:42 +00:00

john commented

2026-08-14 17:02:00 +00:00

Author

Owner

Fixed in v3.2.0 (PR #124). FSW-01, phase 2. atomicWrite lstats and refuses a symlinked temp path instead of writing through it, and temp paths are per-writer (...tmp) so two concurrent writers cannot share one. appendEvent returns {written:false, reason:'symlinked-trace'} rather than following.

Verified through the phase UAT walk. The milestone audit traced 12/12 requirements with 0 broken and 36/36 acceptance criteria covered.

Fixed in v3.2.0 (PR #124). FSW-01, phase 2. atomicWrite lstats and refuses a symlinked temp path instead of writing through it, and temp paths are per-writer (<file>.<pid>.<n>.tmp) so two concurrent writers cannot share one. appendEvent returns {written:false, reason:'symlinked-trace'} rather than following. Verified through the phase UAT walk. The milestone audit traced 12/12 requirements with 0 broken and 36/36 acceptance criteria covered.