From 238c8c755a53cb8da3f43090ded3787673f251af Mon Sep 17 00:00:00 2001 From: Khaliq Gant Date: Fri, 31 Jul 2026 20:50:44 +0200 Subject: [PATCH 1/4] =?UTF-8?q?test(local-mount):=20adversarial=20confinem?= =?UTF-8?q?ent=20matrix=20=E2=80=94=202=20confirmed=20mount=20escapes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies the adversarial matrix from the Agent Relay x Ratify design-partner spike to the project<->mount sync path. Three of eleven cases fail against current behaviour. Two are confirmed escapes in which a file OUTSIDE the mount is overwritten with content from inside it: 1. HARDLINK. A hardlink inside the mount pointing at a file outside it is path-indistinguishable from a real file, and realpath cannot resolve it — a hardlink has no target. resolveSafeWriteTarget resolves the parent and the caller checks isSymlinkTarget, but nothing checks the link count, so copyFileSync writes straight through to the outside file. 2. TOCTOU. isSymlinkTarget(target) and copyFileSync(target) are two separate path operations. A target swapped for a symlink between them is followed. No O_NOFOLLOW, so the check cannot bind to the object it validated. Both were verified by content, not by return value: the outside file goes from "IMPORTANT PRE-EXISTING CONTENT" to the payload. 3. REFUSAL WITH A SIDE EFFECT. resolveSafeWriteTarget runs mkdirSync(parent, { recursive: true }) BEFORE validating the resolved parent, so a symlinked component causes directories to be created outside the mount and only then is the write refused. Not a defect, but worth documenting: resolveSafeWriteTarget requires `root` to be already realpath'd. Its only caller satisfies this (mount.ts:195), so an unresolved root is not a production path — the positive control asserts the caller's actual usage rather than the function in isolation. Positive controls pass: new file, nested directories, replacing an existing file, and a mount root reached through a symlink. A fix must keep them passing; confinement that blocks legitimate writes fails as loudly as one that leaks. No fix here — this commit is the reproduction. The two internals are exported for the suite so it drives the real resolver rather than a copy. Co-Authored-By: Claude Opus 5 --- .../src/auto-sync-confinement.test.ts | 251 ++++++++++++++++++ packages/local-mount/src/auto-sync.ts | 11 +- 2 files changed, 260 insertions(+), 2 deletions(-) create mode 100644 packages/local-mount/src/auto-sync-confinement.test.ts diff --git a/packages/local-mount/src/auto-sync-confinement.test.ts b/packages/local-mount/src/auto-sync-confinement.test.ts new file mode 100644 index 00000000..2e4da96c --- /dev/null +++ b/packages/local-mount/src/auto-sync-confinement.test.ts @@ -0,0 +1,251 @@ +/** + * Adversarial confinement matrix for the project↔mount sync path. + * + * Relayfile materializes agent-visible files onto real disk, so the mount + * boundary is a real filesystem boundary: an agent that can influence what + * exists inside the mount can potentially influence what gets written outside + * it. `resolveSafeWriteTarget` is what stands in the way. + * + * This suite drives the **real resolver and the real caller sequence** from + * `doProjectToMount`: + * + * const target = resolveSafeWriteTarget(ctx.realMountDir, mountAbs); + * if (!target) return false; + * if (isSymlinkTarget(target)) return false; + * copyFileSync(projectAbs, target, COPYFILE_FICLONE); + * + * Two contracts are asserted separately, because conflating them is what hid + * the worst defect in the equivalent code elsewhere — a refusal that deleted + * the file it was protecting: + * + * C1 A refusal makes no observable change outside the mount. Every negative + * case asserts the refusal *and* that outside state is byte-identical + * afterwards. Asserting only the return value passes against an + * implementation that damages data and then reports failure. + * + * C2 A completed write is all-or-nothing. + * + * Provenance: this matrix comes from the Agent Relay × Ratify design-partner + * spike, where it found four distinct defects in equivalent code — three found + * by us, one by Identities AI. Every one was platform- or timing-dependent and + * none would have been caught by asserting on a return value. It is applied here + * to find out empirically which cases this implementation handles, rather than + * reasoning about it from a reading. + */ + +import { afterEach, describe, expect, it } from 'vitest'; +import { + constants as fsConstants, + copyFileSync, + existsSync, + linkSync, + mkdirSync, + mkdtempSync, + readFileSync, + readdirSync, + realpathSync, + rmSync, + statSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +import { isSymlinkTarget, resolveSafeWriteTarget } from './auto-sync.js'; + +const VICTIM = 'IMPORTANT PRE-EXISTING CONTENT'; +const PAYLOAD = 'PAYLOAD-FROM-INSIDE-THE-MOUNT'; + +interface Sandbox { + base: string; + mount: string; + outside: string; + victim: string; + source: string; +} + +const dirs: string[] = []; + +afterEach(() => { + for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); +}); + +function sandbox(): Sandbox { + const base = realpathSync(mkdtempSync(path.join(os.tmpdir(), 'rf-confine-'))); + const mount = path.join(base, 'mount'); + const outside = path.join(base, 'outside'); + const project = path.join(base, 'project'); + mkdirSync(mount); + mkdirSync(outside); + mkdirSync(project); + const victim = path.join(outside, 'secret.txt'); + writeFileSync(victim, VICTIM); + const source = path.join(project, 'source.txt'); + writeFileSync(source, PAYLOAD); + dirs.push(base); + return { base, mount, outside, victim, source }; +} + +/** relative path -> content, for everything under `dir`. */ +function snapshot(dir: string): Record { + const out: Record = {}; + const walk = (p: string, rel: string): void => { + for (const name of readdirSync(p).sort()) { + const full = path.join(p, name); + const key = rel ? `${rel}/${name}` : name; + const st = statSync(full, { throwIfNoEntry: false }); + if (!st) out[key] = ''; + else if (st.isDirectory()) { + out[key] = ''; + walk(full, key); + } else if (st.isFile()) out[key] = readFileSync(full, 'utf8'); + else out[key] = ''; + } + }; + walk(dir, ''); + return out; +} + +/** + * The real caller sequence, verbatim from `doProjectToMount`. + * Returns 'refused' or 'written'. `between` fires in the race window that + * exists between the symlink check and the copy. + */ +function syncAttempt(s: Sandbox, mountAbs: string, between?: () => void): 'refused' | 'written' { + const target = resolveSafeWriteTarget(s.mount, mountAbs); + if (!target) return 'refused'; + if (isSymlinkTarget(target)) return 'refused'; + between?.(); + copyFileSync(s.source, target, fsConstants.COPYFILE_FICLONE); + return 'written'; +} + +/** Assert C1: refused, and nothing that existed outside the mount changed. */ +function expectContained(s: Sandbox, run: () => 'refused' | 'written'): void { + const before = snapshot(s.outside); + let outcome: 'refused' | 'written' | 'threw' = 'threw'; + try { + outcome = run(); + } catch { + outcome = 'threw'; + } + const after = snapshot(s.outside); + + for (const [key, value] of Object.entries(before)) { + expect(after[key], `pre-existing outside entry ${key} was mutated or deleted`).toBe(value); + } + for (const [key, value] of Object.entries(after)) { + expect(value, `payload escaped the mount into ${key}`).not.toBe(PAYLOAD); + } + expect(outcome, 'the write should not have completed').not.toBe('written'); +} + +describe('relayfile mount confinement — escapes', () => { + it('refuses traversal above the mount', () => { + const s = sandbox(); + expectContained(s, () => syncAttempt(s, path.join(s.mount, '..', 'outside', 'secret.txt'))); + }); + + it('refuses an absolute path outside the mount', () => { + const s = sandbox(); + expectContained(s, () => syncAttempt(s, s.victim)); + }); + + it('refuses a final component that is a symlink pointing outside', () => { + const s = sandbox(); + symlinkSync(s.victim, path.join(s.mount, 'link.txt')); + expectContained(s, () => syncAttempt(s, path.join(s.mount, 'link.txt'))); + }); + + it('refuses an intermediate directory that is a symlink pointing outside', () => { + const s = sandbox(); + symlinkSync(s.outside, path.join(s.mount, 'docs')); + expectContained(s, () => syncAttempt(s, path.join(s.mount, 'docs', 'secret.txt'))); + }); + + it('refuses a hardlink inside the mount pointing at a file outside it', () => { + // A hardlink is path-indistinguishable from a real file and realpath cannot + // resolve it — it has no target. Link count is the only signal. + const s = sandbox(); + linkSync(s.victim, path.join(s.mount, 'hard.txt')); + expectContained(s, () => syncAttempt(s, path.join(s.mount, 'hard.txt'))); + }); + + it('refuses when the target is swapped for a symlink after the check', () => { + // The window between `isSymlinkTarget(target)` and `copyFileSync(target)`. + const s = sandbox(); + const target = path.join(s.mount, 'notes.md'); + writeFileSync(target, 'placeholder'); + expectContained(s, () => + syncAttempt(s, target, () => { + rmSync(target, { force: true }); + symlinkSync(s.victim, target); + }), + ); + }); +}); + +describe('relayfile mount confinement — refusals must not mutate', () => { + it('does not create directories outside the mount while refusing', () => { + // resolveSafeWriteTarget runs mkdirSync(parent, { recursive: true }) before + // it validates the resolved parent. If the parent traverses a symlink out + // of the mount, directories are created outside and only then is the write + // refused — a refusal with a side effect. + const s = sandbox(); + symlinkSync(s.outside, path.join(s.mount, 'docs')); + const before = snapshot(s.outside); + + syncAttempt(s, path.join(s.mount, 'docs', 'deep', 'nested', 'file.txt')); + + const after = snapshot(s.outside); + const created = Object.keys(after).filter((k) => !(k in before)); + expect(created, 'refusal created entries outside the mount').toEqual([]); + }); +}); + +describe('relayfile mount confinement — positive controls', () => { + // A confinement fix that refuses everything is as broken as one that permits + // escapes. These must keep passing. + + it('writes a new file inside the mount', () => { + const s = sandbox(); + const outcome = syncAttempt(s, path.join(s.mount, 'new.txt')); + expect(outcome).toBe('written'); + expect(readFileSync(path.join(s.mount, 'new.txt'), 'utf8')).toBe(PAYLOAD); + }); + + it('creates nested directories inside the mount', () => { + const s = sandbox(); + const outcome = syncAttempt(s, path.join(s.mount, 'a', 'b', 'c.txt')); + expect(outcome).toBe('written'); + expect(readFileSync(path.join(s.mount, 'a/b/c.txt'), 'utf8')).toBe(PAYLOAD); + }); + + it('replaces an existing file inside the mount', () => { + const s = sandbox(); + writeFileSync(path.join(s.mount, 'exists.txt'), 'OLD'); + const outcome = syncAttempt(s, path.join(s.mount, 'exists.txt')); + expect(outcome).toBe('written'); + expect(readFileSync(path.join(s.mount, 'exists.txt'), 'utf8')).toBe(PAYLOAD); + }); + + it('supports a mount root reached through a symlink (as the caller resolves it)', () => { + // `resolveSafeWriteTarget` has an undocumented precondition: `root` must + // already be realpath'd. Its only caller satisfies it — mount.ts:195 does + // `realpathSync(resolvedMountDir)` before building the context — so passing + // an unresolved root is not a production path. Asserted the way the caller + // actually uses it; worth a doc comment on the function so the next caller + // does not discover the precondition the hard way. + const s = sandbox(); + const linked = path.join(s.base, 'mount-link'); + symlinkSync(s.mount, linked); + const rootAsCallerPassesIt = realpathSync(linked); + const target = resolveSafeWriteTarget(rootAsCallerPassesIt, path.join(rootAsCallerPassesIt, 'ok.txt')); + expect(target, 'a symlinked mount root must remain usable').not.toBeNull(); + if (target) { + copyFileSync(s.source, target); + expect(existsSync(path.join(s.mount, 'ok.txt'))).toBe(true); + } + }); +}); diff --git a/packages/local-mount/src/auto-sync.ts b/packages/local-mount/src/auto-sync.ts index b3985e8d..24daf07b 100644 --- a/packages/local-mount/src/auto-sync.ts +++ b/packages/local-mount/src/auto-sync.ts @@ -802,7 +802,8 @@ function safeFileStat(p: string): Stats | null { } } -function isSymlinkTarget(target: string): boolean { +/** @internal exported for the adversarial confinement suite. */ +export function isSymlinkTarget(target: string): boolean { // If the target already exists as a symlink, writing through it would // follow the link and potentially escape the mount/project root. Refuse. try { @@ -837,7 +838,13 @@ function sameContentBytes(left: string, right: string): boolean { } } -function resolveSafeWriteTarget(root: string, candidate: string): string | null { +/** + * Exported for the adversarial confinement suite in + * auto-sync-confinement.test.ts. Not part of the package's public API — the + * test drives the real resolver rather than a copy of it, because a copy proves + * nothing about this code. + */ +export function resolveSafeWriteTarget(root: string, candidate: string): string | null { const resolvedRoot = path.resolve(root); const resolvedCandidate = path.resolve(candidate); if ( From 203d028e41f3e550686385a0479213e841ce3156 Mon Sep 17 00:00:00 2001 From: Khaliq Gant Date: Fri, 31 Jul 2026 21:41:37 +0200 Subject: [PATCH 2/4] fix(local-mount): close two mount-boundary escapes in the sync path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both were confirmed by content, not by return value: a file outside the mount went from its own content to the payload from inside. 1. HARDLINK. A hardlink inside the mount pointing at a file outside it is path-indistinguishable from a real file, and realpath cannot resolve it — a hardlink has no target. The parent was resolved and the final component was checked for being a symlink, but nothing checked the link count, so copyFileSync wrote straight through to the outside file. 2. TOCTOU. isSymlinkTarget(target) and copyFileSync(target) are two separate path lookups. A target swapped for a symlink between them was followed. Both are closed structurally rather than with another check: content is copied into a temporary sibling inside the already-validated parent and renamed over the target. rename replaces the directory *entry*, so it neither writes through a hardlink nor follows a symlink, and it cannot be raced into doing so. The write also becomes atomic — no partial or zero-length window — and reflink cloning is preserved, since the copy into the temporary file still requests COPYFILE_FICLONE. Also fixed: resolveSafeWriteTarget ran mkdirSync(parent, { recursive: true }) BEFORE validating the resolved parent, so a symlinked component created directories outside the root and only then refused — a refusal with a side effect. Directories are now created one component at a time, refusing to traverse a symlink. `recursive: true` also creates *through* a symlinked component, which is the traversal it was meant to prevent. The readonly chmod dance is gone with it. The mode is applied to the temporary file before the rename, so a 0o444 mount copy no longer has to be made writable first — that was a window in which the readonly guarantee did not hold. Verified by the adversarial matrix added in the previous commit: 11/11, with positive controls asserting legitimate writes still land. The matrix asserts containment (nothing outside the mount is modified) rather than refusal, because for a sync engine severing a hostile link and writing correctly inside the boundary is right — refusing would stall sync on that path forever, which an adversary can trigger by planting one link. Full package suite: 86/86. Co-Authored-By: Claude Opus 5 --- .../src/auto-sync-confinement.test.ts | 52 +++++++-- packages/local-mount/src/auto-sync.ts | 108 +++++++++++++++--- .../local-mount/src/mount-reflink.test.ts | 14 ++- 3 files changed, 145 insertions(+), 29 deletions(-) diff --git a/packages/local-mount/src/auto-sync-confinement.test.ts b/packages/local-mount/src/auto-sync-confinement.test.ts index 2e4da96c..c832238e 100644 --- a/packages/local-mount/src/auto-sync-confinement.test.ts +++ b/packages/local-mount/src/auto-sync-confinement.test.ts @@ -12,7 +12,7 @@ * const target = resolveSafeWriteTarget(ctx.realMountDir, mountAbs); * if (!target) return false; * if (isSymlinkTarget(target)) return false; - * copyFileSync(projectAbs, target, COPYFILE_FICLONE); + * safeCopyOnto(projectAbs, target, mode); * * Two contracts are asserted separately, because conflating them is what hid * the worst defect in the equivalent code elsewhere — a refusal that deleted @@ -52,7 +52,7 @@ import { import os from 'node:os'; import path from 'node:path'; -import { isSymlinkTarget, resolveSafeWriteTarget } from './auto-sync.js'; +import { isSymlinkTarget, resolveSafeWriteTarget, safeCopyOnto } from './auto-sync.js'; const VICTIM = 'IMPORTANT PRE-EXISTING CONTENT'; const PAYLOAD = 'PAYLOAD-FROM-INSIDE-THE-MOUNT'; @@ -117,18 +117,35 @@ function syncAttempt(s: Sandbox, mountAbs: string, between?: () => void): 'refus if (!target) return 'refused'; if (isSymlinkTarget(target)) return 'refused'; between?.(); - copyFileSync(s.source, target, fsConstants.COPYFILE_FICLONE); - return 'written'; + return safeCopyOnto(s.source, target) ? 'written' : 'refused'; } -/** Assert C1: refused, and nothing that existed outside the mount changed. */ +/** + * Assert the containment property: **nothing outside the mount is modified, + * deleted, or receives the payload.** + * + * Deliberately does not assert *refusal*. There are two defensible responses to + * a hostile target, and which one is right depends on what the component is + * for: + * + * - **Refuse** — correct for a one-shot authorized write, where the caller + * wants to know its request could not be honoured safely. + * - **Sever and write inside the boundary** — correct for a *sync engine*. + * `rename` replaces the directory entry rather than writing through it, so + * a hardlinked or symlinked target is detached and the mount receives the + * correct content. Refusing would stall sync on that path indefinitely, + * which is a denial of service an adversary can trigger by planting one + * link. + * + * Relayfile takes the second. `expectInsideMount` below pins that the write + * still lands, so "safe" cannot quietly become "broken". + */ function expectContained(s: Sandbox, run: () => 'refused' | 'written'): void { const before = snapshot(s.outside); - let outcome: 'refused' | 'written' | 'threw' = 'threw'; try { - outcome = run(); + run(); } catch { - outcome = 'threw'; + /* throwing is an acceptable refusal */ } const after = snapshot(s.outside); @@ -138,7 +155,12 @@ function expectContained(s: Sandbox, run: () => 'refused' | 'written'): void { for (const [key, value] of Object.entries(after)) { expect(value, `payload escaped the mount into ${key}`).not.toBe(PAYLOAD); } - expect(outcome, 'the write should not have completed').not.toBe('written'); +} + +/** The content landed at this path inside the mount — sync was not stalled. */ +function expectInsideMount(target: string): void { + expect(existsSync(target), 'the write should still land inside the mount').toBe(true); + expect(readFileSync(target, 'utf8')).toBe(PAYLOAD); } describe('relayfile mount confinement — escapes', () => { @@ -168,8 +190,13 @@ describe('relayfile mount confinement — escapes', () => { // A hardlink is path-indistinguishable from a real file and realpath cannot // resolve it — it has no target. Link count is the only signal. const s = sandbox(); - linkSync(s.victim, path.join(s.mount, 'hard.txt')); - expectContained(s, () => syncAttempt(s, path.join(s.mount, 'hard.txt'))); + const target = path.join(s.mount, 'hard.txt'); + linkSync(s.victim, target); + expectContained(s, () => syncAttempt(s, target)); + // The link is severed, not followed: the mount copy is updated and the + // file it pointed at outside keeps its content. + expectInsideMount(target); + expect(readFileSync(s.victim, 'utf8')).toBe(VICTIM); }); it('refuses when the target is swapped for a symlink after the check', () => { @@ -183,6 +210,9 @@ describe('relayfile mount confinement — escapes', () => { symlinkSync(s.victim, target); }), ); + // rename replaces the symlink itself rather than following it. + expectInsideMount(target); + expect(readFileSync(s.victim, 'utf8')).toBe(VICTIM); }); }); diff --git a/packages/local-mount/src/auto-sync.ts b/packages/local-mount/src/auto-sync.ts index 24daf07b..76779d07 100644 --- a/packages/local-mount/src/auto-sync.ts +++ b/packages/local-mount/src/auto-sync.ts @@ -5,6 +5,8 @@ import { existsSync, lstatSync, mkdirSync, + unlinkSync, + renameSync, readdirSync, readFileSync, realpathSync, @@ -687,7 +689,7 @@ function doMountToProject( updateState(state, relPosix, mountAbs, target); return false; } - copyFileSync(mountAbs, target, fsConstants.COPYFILE_FICLONE); + if (!safeCopyOnto(mountAbs, target)) return false; const mountStat = safeFileStat(mountAbs); if (mountStat) preserveMtime(target, mountStat); updateState(state, relPosix, mountAbs, target); @@ -709,22 +711,15 @@ function doProjectToMount( updateState(state, relPosix, target, projectAbs); return false; } - // The mount copy of a readonly file has mode 0o444, which blocks - // copyFileSync from overwriting it. Temporarily restore write permission. - if (existsSync(target)) { - try { chmodSync(target, 0o644); } catch { /* best effort */ } - } - copyFileSync(projectAbs, target, fsConstants.COPYFILE_FICLONE); + // The mode is applied to the temporary file before the rename, so a readonly + // (0o444) mount copy no longer has to be chmod'd writable first. That + // temporary un-protection was a small window in which the readonly guarantee + // did not hold; renaming over the target removes the need for it entirely. const sourceStat = safeFileStat(projectAbs); + const mode = readonly ? 0o444 : sourceStat?.mode !== undefined ? sourceStat.mode & 0o777 : undefined; + + if (!safeCopyOnto(projectAbs, target, mode)) return false; if (sourceStat) preserveMtime(target, sourceStat); - if (readonly) { - try { chmodSync(target, 0o444); } catch { /* best effort */ } - } else { - const mode = safeFileStat(projectAbs)?.mode; - if (mode !== undefined) { - try { chmodSync(target, mode & 0o777); } catch { /* best effort */ } - } - } updateState(state, relPosix, target, projectAbs); return true; } @@ -845,6 +840,8 @@ function sameContentBytes(left: string, right: string): boolean { * nothing about this code. */ export function resolveSafeWriteTarget(root: string, candidate: string): string | null { + // `root` must already be realpath'd — the only caller does this at + // mount.ts:195. Passing an unresolved root will reject everything. const resolvedRoot = path.resolve(root); const resolvedCandidate = path.resolve(candidate); if ( @@ -855,7 +852,17 @@ export function resolveSafeWriteTarget(root: string, candidate: string): string } const parent = path.dirname(resolvedCandidate); try { - mkdirSync(parent, { recursive: true }); + // Directories are created one component at a time, refusing to traverse a + // symlink, and only after the component is known to be safe. + // + // `mkdirSync(parent, { recursive: true })` used to run BEFORE the resolved + // parent was validated, so a symlinked component caused directories to be + // created outside the root and only then was the write refused — a refusal + // with a side effect. `recursive: true` also creates *through* a symlinked + // component, which is the traversal it was supposed to prevent. + if (!createDirectoriesWithin(resolvedRoot, parent)) { + return null; + } const realParent = realpathSync(parent); if ( realParent !== resolvedRoot && @@ -869,6 +876,75 @@ export function resolveSafeWriteTarget(root: string, candidate: string): string } } +/** + * Create every missing component of `dir` beneath `root`, one at a time, + * refusing to follow or create through a symlink. Returns false on the first + * component that is not a real directory. + */ +function createDirectoriesWithin(root: string, dir: string): boolean { + if (dir === root) return true; + + const relative = path.relative(root, dir); + if (relative.startsWith('..') || path.isAbsolute(relative)) return false; + + let current = root; + for (const segment of relative.split(path.sep)) { + if (!segment) continue; + current = path.join(current, segment); + const info = lstatSync(current, { throwIfNoEntry: false }); + if (!info) { + mkdirSync(current); // one component, never recursive + continue; + } + // A symlink here is refused rather than followed, and nothing has been + // created outside the root by the time we return. + if (info.isSymbolicLink() || !info.isDirectory()) return false; + } + return true; +} + +/** + * Copy `source` onto `target` without ever writing *through* whatever `target` + * currently names. + * + * The content is written to a temporary sibling inside the already-validated + * parent directory and then renamed over the target. That is what makes this + * safe, and it closes two confirmed escapes that a check-then-copy sequence + * could not: + * + * - **Hardlink.** A hardlink inside the root pointing at a file outside it is + * path-indistinguishable from a real file and `realpath` cannot resolve it, + * because a hardlink has no target. `copyFileSync` onto that name wrote + * straight through to the outside file. `rename` replaces the *directory + * entry* instead, so the linked file keeps its content. + * + * - **TOCTOU.** `isSymlinkTarget(target)` followed by `copyFileSync(target)` + * is two path lookups, and a target swapped for a symlink in between was + * followed. `rename` does not follow a final symlink — it replaces it. + * + * It also makes the write atomic: a reader sees the old file or the new one, + * never a partial or zero-length one, and an interrupted copy leaves the target + * untouched. Reflink cloning is preserved, since the copy into the temporary + * file still uses COPYFILE_FICLONE. + */ +export function safeCopyOnto(source: string, target: string, mode?: number): boolean { + const dir = path.dirname(target); + const temp = path.join(dir, `.${path.basename(target)}.rfsync-${process.pid}-${syncTempCounter++}`); + try { + copyFileSync(source, temp, fsConstants.COPYFILE_FICLONE); + if (mode !== undefined) { + try { chmodSync(temp, mode); } catch { /* best effort */ } + } + renameSync(temp, target); + return true; + } catch { + try { unlinkSync(temp); } catch { /* best effort */ } + return false; + } +} + +let syncTempCounter = 0; + function walk( root: string, ctx: AutoSyncContext, diff --git a/packages/local-mount/src/mount-reflink.test.ts b/packages/local-mount/src/mount-reflink.test.ts index 2b72ec24..05e84e8e 100644 --- a/packages/local-mount/src/mount-reflink.test.ts +++ b/packages/local-mount/src/mount-reflink.test.ts @@ -99,7 +99,12 @@ describe('createMount reflink copies', () => { await waitFor(() => readFileSync(path.join(projectDir, 'file.txt'), 'utf8') === 'edited-in-mount'); expect(copyFileSyncMock).toHaveBeenCalledWith( expect.stringMatching(/file\.txt$/), - expect.stringMatching(/file\.txt$/), + // Auto-sync copies into a temporary sibling and renames it over the + // target, so the write never goes *through* whatever the target + // currently names (a hardlink, or a symlink swapped in mid-operation). + // The reflink request itself is unchanged — COPYFILE_FICLONE is still + // what the copy asks for, which is what this test is about. + expect.stringMatching(/file\.txt\.rfsync-\d+-\d+$/), fsConstants.COPYFILE_FICLONE ); @@ -110,7 +115,12 @@ describe('createMount reflink copies', () => { ); expect(copyFileSyncMock).toHaveBeenCalledWith( expect.stringMatching(/file\.txt$/), - expect.stringMatching(/file\.txt$/), + // Auto-sync copies into a temporary sibling and renames it over the + // target, so the write never goes *through* whatever the target + // currently names (a hardlink, or a symlink swapped in mid-operation). + // The reflink request itself is unchanged — COPYFILE_FICLONE is still + // what the copy asks for, which is what this test is about. + expect.stringMatching(/file\.txt\.rfsync-\d+-\d+$/), fsConstants.COPYFILE_FICLONE ); } finally { From 52edecd957dd86448f6da576246efa987dac6054 Mon Sep 17 00:00:00 2001 From: Khaliq Gant Date: Fri, 31 Jul 2026 21:41:57 +0200 Subject: [PATCH 3/4] docs(local-mount): changelog for the mount-boundary fixes Co-Authored-By: Claude Opus 5 --- packages/local-mount/CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/local-mount/CHANGELOG.md b/packages/local-mount/CHANGELOG.md index 0f98184b..f0479426 100644 --- a/packages/local-mount/CHANGELOG.md +++ b/packages/local-mount/CHANGELOG.md @@ -6,7 +6,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -_No unreleased changes._ +### Fixed + +- Auto-sync no longer writes through a hardlink or a symlink at the destination. A hardlink inside the mount pointing at a file outside it, or a target swapped for a symlink between the check and the copy, could overwrite a file outside the mount or project directory. Content is now copied into a temporary sibling and renamed over the target, which replaces the directory entry rather than writing through it. The write is also atomic — readers never see a partial or zero-length file — and reflink cloning is unchanged. +- Creating a destination directory no longer creates directories outside the root before refusing. Path components are created one at a time and a symlinked component is refused rather than traversed. ## [0.10.37] - 2026-07-26 From 7ba28ea9114da103d4ca592b38d820aa766605e8 Mon Sep 17 00:00:00 2001 From: Khaliq Gant Date: Fri, 31 Jul 2026 23:11:28 +0200 Subject: [PATCH 4/4] fix(local-mount): harden the temporary file and the directory walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review findings on this PR. The first one is the same defect class this PR set out to fix, reintroduced by the fix itself — worth stating plainly. P1, temporary file was predictable and non-exclusive. The name was derived from the target basename plus pid and a counter, and copyFileSync follows a destination symlink. An agent that controls the mount could pre-create that exact path pointing at a file outside it, and the copy would overwrite the victim before the safe rename ran. The temporary name is now random and short, and the copy passes COPYFILE_EXCL so the create fails if anything already occupies the name — a symlink included. Reflink cloning is unaffected; COPYFILE_FICLONE is still requested alongside it. P2, temporary basename could exceed NAME_MAX. A derived name for a valid basename near the per-component limit made the temporary name too long, copyFileSync failed ENAMETOOLONG, and auto-sync then silently stopped updating that file in either direction. The random name is short and independent of the target, which fixes this as a side effect of the P1 fix. Major, check-then-mkdir race in createDirectoriesWithin. Each component was validated with lstat and the next created by recomputed path, so an already-accepted directory could be swapped for an outside-directed symlink before the following segment was created. Components are now opened and held with O_NOFOLLOW | O_DIRECTORY for the duration, and on Linux the next component is resolved relative to the held descriptor via /proc/self/fd, so a swap on disk cannot redirect the create. Elsewhere the walk still falls back to paths; the held descriptors pin each inode so the caller's realpath containment check cannot be defeated by inode reuse, and the write is refused either way. The residual on non-Linux — a directory may be created outside the root before the refusal — is documented at the function. Three new cases cover the temporary file directly: the name is not derived from the basename (asserted with a 240-character filename), an entry planted at a temporary-style name is never written through, and no temporary files survive. Confinement matrix 14/14 on macOS and Linux. Package suite 89/89. Co-Authored-By: Claude Opus 5 --- .../src/auto-sync-confinement.test.ts | 50 ++++++++++ packages/local-mount/src/auto-sync.ts | 98 ++++++++++++++++--- .../local-mount/src/mount-reflink.test.ts | 16 +-- 3 files changed, 142 insertions(+), 22 deletions(-) diff --git a/packages/local-mount/src/auto-sync-confinement.test.ts b/packages/local-mount/src/auto-sync-confinement.test.ts index c832238e..917d636c 100644 --- a/packages/local-mount/src/auto-sync-confinement.test.ts +++ b/packages/local-mount/src/auto-sync-confinement.test.ts @@ -234,6 +234,56 @@ describe('relayfile mount confinement — refusals must not mutate', () => { }); }); +describe('relayfile mount confinement — the temporary file', () => { + // The temp file is part of the attack surface, not an implementation detail. + // An earlier version of this fix named it from the target basename plus pid + // and a counter — predictable to an agent that controls the mount, which + // could pre-create that exact path as a symlink to a file outside and have + // the copy overwrite the victim before the safe rename ever ran. That is the + // escape this function exists to close, reintroduced by the fix. + + it('does not derive the temporary name from the target basename', () => { + // Both unpredictability and length depend on this. A derived name also + // overflows NAME_MAX for a long-but-valid basename, which would silently + // stop syncing that file in either direction. + const s = sandbox(); + const longName = `${'x'.repeat(240)}.txt`; + const target = path.join(s.mount, longName); + + expect(syncAttempt(s, target)).toBe('written'); + expectInsideMount(target); + + for (const entry of readdirSync(s.mount)) { + if (entry === longName) continue; + expect(entry.length, `temporary name ${entry} is too long`).toBeLessThan(64); + expect(entry).not.toContain('x'.repeat(20)); + } + }); + + it('refuses rather than following a symlink planted at the temporary path', () => { + // The exclusive create is what makes this safe: it fails if anything is + // already at the name, symlink included. Verified by pre-creating every + // name the generator could plausibly produce is impossible, so this asserts + // the property directly — an existing entry is never written through. + const s = sandbox(); + const planted = path.join(s.mount, '.rfsync-deadbeefdeadbeefde'); + symlinkSync(s.victim, planted); + + const before = readFileSync(s.victim, 'utf8'); + // A normal sync alongside the planted name must still work, and must not + // touch the victim through it. + expect(syncAttempt(s, path.join(s.mount, 'ok.txt'))).toBe('written'); + expect(readFileSync(s.victim, 'utf8')).toBe(before); + }); + + it('leaves no temporary files behind', () => { + const s = sandbox(); + syncAttempt(s, path.join(s.mount, 'a.txt')); + syncAttempt(s, path.join(s.mount, 'b.txt')); + expect(readdirSync(s.mount).filter((f) => f.startsWith('.rfsync-'))).toEqual([]); + }); +}); + describe('relayfile mount confinement — positive controls', () => { // A confinement fix that refuses everything is as broken as one that permits // escapes. These must keep passing. diff --git a/packages/local-mount/src/auto-sync.ts b/packages/local-mount/src/auto-sync.ts index 76779d07..6f83d80f 100644 --- a/packages/local-mount/src/auto-sync.ts +++ b/packages/local-mount/src/auto-sync.ts @@ -1,10 +1,12 @@ import { chmodSync, + closeSync, constants as fsConstants, copyFileSync, existsSync, lstatSync, mkdirSync, + openSync, unlinkSync, renameSync, readdirSync, @@ -14,6 +16,7 @@ import { statSync, } from 'node:fs'; import type { Stats } from 'node:fs'; +import { randomBytes } from 'node:crypto'; import path from 'node:path'; import watcher, { type AsyncSubscription } from '@parcel/watcher'; import { preserveMtime, statsImplySameContent } from './stat-compare.js'; @@ -887,22 +890,68 @@ function createDirectoriesWithin(root: string, dir: string): boolean { const relative = path.relative(root, dir); if (relative.startsWith('..') || path.isAbsolute(relative)) return false; - let current = root; - for (const segment of relative.split(path.sep)) { - if (!segment) continue; - current = path.join(current, segment); - const info = lstatSync(current, { throwIfNoEntry: false }); - if (!info) { - mkdirSync(current); // one component, never recursive - continue; + // Each component is opened and held for the duration, and — where the + // platform allows — the next component is resolved *relative to that + // descriptor* rather than by recomputed path. + // + // Checking a component with `lstat` and then creating the next one by path is + // a race: an already-accepted directory can be swapped for an + // outside-directed symlink before the following segment is created, and + // `mkdirSync` would then create directories outside the root. Resolving + // through the held descriptor means a swap on disk cannot redirect the + // create, because the descriptor still refers to the directory that was + // validated. + // + // On Linux `/proc/self/fd//name` gives that resolution from the kernel. + // Elsewhere there is no equivalent without a native `openat`, so the walk + // falls back to paths and the held descriptors serve a narrower purpose: they + // pin each inode so it cannot be freed and recycled, which keeps the caller's + // subsequent `realpath` containment check meaningful. The write is still + // refused in that case — the residual is that a directory may have been + // created outside the root before the refusal. + const held: number[] = []; + try { + let parentFd = openSync(root, fsConstants.O_RDONLY | fsConstants.O_DIRECTORY); + held.push(parentFd); + let currentPath = root; + + for (const segment of relative.split(path.sep)) { + if (!segment) continue; + + currentPath = path.join(currentPath, segment); + const childPath = DESCRIPTOR_RELATIVE + ? `/proc/self/fd/${parentFd}/${segment}` + : currentPath; + + const info = lstatSync(childPath, { throwIfNoEntry: false }); + if (!info) { + mkdirSync(childPath); // one component, never recursive + } else if (info.isSymbolicLink() || !info.isDirectory()) { + // Refused rather than followed. + return false; + } + + // O_NOFOLLOW so a symlink that appeared since the lstat cannot be opened; + // O_DIRECTORY so anything no longer a directory cannot be either. + parentFd = openSync( + childPath, + fsConstants.O_RDONLY | fsConstants.O_DIRECTORY | fsConstants.O_NOFOLLOW + ); + held.push(parentFd); + } + return true; + } catch { + return false; + } finally { + for (const fd of held) { + try { closeSync(fd); } catch { /* already closed */ } } - // A symlink here is refused rather than followed, and nothing has been - // created outside the root by the time we return. - if (info.isSymbolicLink() || !info.isDirectory()) return false; } - return true; } +/** True where `/proc/self/fd` provides kernel-side descriptor-relative resolution. */ +const DESCRIPTOR_RELATIVE = process.platform === 'linux'; + /** * Copy `source` onto `target` without ever writing *through* whatever `target` * currently names. @@ -929,9 +978,28 @@ function createDirectoriesWithin(root: string, dir: string): boolean { */ export function safeCopyOnto(source: string, target: string, mode?: number): boolean { const dir = path.dirname(target); - const temp = path.join(dir, `.${path.basename(target)}.rfsync-${process.pid}-${syncTempCounter++}`); + + // The temporary name is RANDOM and SHORT, and the copy is EXCLUSIVE. Both + // properties are load-bearing: + // + // - Random + exclusive, because the agent controls the mount. A name + // derived from the target basename plus pid and a counter is predictable, + // and `copyFileSync` follows a destination symlink — so the agent could + // pre-create that exact path pointing at a file outside the mount and the + // copy would overwrite the victim *before* the safe rename ever ran. That + // is the same escape this function exists to close, reintroduced by the + // fix; COPYFILE_EXCL makes the create fail if anything is already there, + // symlink included, and randomness means there is nothing to pre-empt. + // + // - Short and independent of the target basename, because a basename near + // the filesystem's per-component limit (NAME_MAX, typically 255) would + // make a derived temporary name too long. `copyFileSync` then fails + // ENAMETOOLONG, which this function reports as a refusal, and auto-sync + // silently stops updating that file in either direction. + const temp = path.join(dir, `.rfsync-${randomBytes(9).toString('hex')}`); + try { - copyFileSync(source, temp, fsConstants.COPYFILE_FICLONE); + copyFileSync(source, temp, fsConstants.COPYFILE_FICLONE | fsConstants.COPYFILE_EXCL); if (mode !== undefined) { try { chmodSync(temp, mode); } catch { /* best effort */ } } @@ -943,8 +1011,6 @@ export function safeCopyOnto(source: string, target: string, mode?: number): boo } } -let syncTempCounter = 0; - function walk( root: string, ctx: AutoSyncContext, diff --git a/packages/local-mount/src/mount-reflink.test.ts b/packages/local-mount/src/mount-reflink.test.ts index 05e84e8e..04ee53dc 100644 --- a/packages/local-mount/src/mount-reflink.test.ts +++ b/packages/local-mount/src/mount-reflink.test.ts @@ -103,9 +103,11 @@ describe('createMount reflink copies', () => { // target, so the write never goes *through* whatever the target // currently names (a hardlink, or a symlink swapped in mid-operation). // The reflink request itself is unchanged — COPYFILE_FICLONE is still - // what the copy asks for, which is what this test is about. - expect.stringMatching(/file\.txt\.rfsync-\d+-\d+$/), - fsConstants.COPYFILE_FICLONE + // what the copy asks for, which is what this test is about. COPYFILE_EXCL + // is paired with it so the create fails if anything already occupies the + // temporary name, which is what stops a planted symlink being followed. + expect.stringMatching(/\.rfsync-[0-9a-f]+$/), + fsConstants.COPYFILE_FICLONE | fsConstants.COPYFILE_EXCL ); copyFileSyncMock.mockClear(); @@ -119,9 +121,11 @@ describe('createMount reflink copies', () => { // target, so the write never goes *through* whatever the target // currently names (a hardlink, or a symlink swapped in mid-operation). // The reflink request itself is unchanged — COPYFILE_FICLONE is still - // what the copy asks for, which is what this test is about. - expect.stringMatching(/file\.txt\.rfsync-\d+-\d+$/), - fsConstants.COPYFILE_FICLONE + // what the copy asks for, which is what this test is about. COPYFILE_EXCL + // is paired with it so the create fails if anything already occupies the + // temporary name, which is what stops a planted symlink being followed. + expect.stringMatching(/\.rfsync-[0-9a-f]+$/), + fsConstants.COPYFILE_FICLONE | fsConstants.COPYFILE_EXCL ); } finally { await auto.stop();