fix(local-mount): close two mount-boundary escapes in the sync path - #391
Conversation
… escapes
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAutosync now validates destination paths component by component and rejects traversal through symlinks or invalid directories. File writes use exclusive temporary sibling files and atomic replacement. Tests cover escape attempts, races, temporary-file attacks, directory creation, and valid synchronization. ChangesAutosync confinement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Sync as Project-to-mount sync
participant Resolver as resolveSafeWriteTarget
participant Directories as createDirectoriesWithin
participant Copier as safeCopyOnto
participant Filesystem
Sync->>Resolver: Resolve candidate beneath resolved root
Resolver->>Directories: Validate or create parent components
Directories->>Filesystem: Inspect or create path components
Sync->>Copier: Copy source to validated target
Copier->>Filesystem: Create exclusive temporary sibling
Copier->>Filesystem: Atomically replace target
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52edecd957
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const temp = path.join(dir, `.${path.basename(target)}.rfsync-${process.pid}-${syncTempCounter++}`); | ||
| try { | ||
| copyFileSync(source, temp, fsConstants.COPYFILE_FICLONE); |
There was a problem hiding this comment.
When project→mount sync runs while the agent controls the mount directory, this temporary name is predictable from the target basename, the service PID, and a counter. The agent can pre-create that path as a symlink or hardlink to an outside victim; because copyFileSync is called without an exclusive-create flag and follows destination symlinks, it overwrites the victim before the supposedly safe rename occurs. Use an exclusively created, non-substitutable temporary file rather than copying onto a predictable pathname.
Useful? React with 👍 / 👎.
| */ | ||
| 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++}`); |
There was a problem hiding this comment.
For a valid target whose basename is near the filesystem's per-component limit (for example, a 250-byte ASCII filename on a filesystem with NAME_MAX=255), prepending/appending this metadata makes the temporary component too long. copyFileSync then raises ENAMETOOLONG, which safeCopyOnto silently converts to false, so auto-sync stops updating that file in either direction even though the target path itself is valid. Use a short temporary name independent of the full target basename.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/local-mount/src/auto-sync.ts (1)
678-697: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve mount file permissions in
doMountToProject.
safeCopyOnto(mountAbs, target)creates a temporary file and renames it over the target. Skippingmodeapplies the mount source mode to the temp file at copy time, so non-default mount permissions are kept after the rename. If the mount file mode is unavailable, omit themodeargument and avoid changing this behavior on futuresafeCopyOntorefactor.🐛 Proposed fix to preserve the mount file's mode
- if (!safeCopyOnto(mountAbs, target)) return false; - const mountStat = safeFileStat(mountAbs); + const mountStat = safeFileStat(mountAbs); + const mode = mountStat?.mode !== undefined ? mountStat.mode & 0o777 : undefined; + if (!safeCopyOnto(mountAbs, target, mode)) return false; if (mountStat) preserveMtime(target, mountStat);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-mount/src/auto-sync.ts` around lines 678 - 697, Update doMountToProject to read the mount source mode via safeFileStat(mountAbs) and pass that mode to safeCopyOnto as its optional mode argument when available; omit the argument when unavailable, preserving the existing copy behavior. Keep the subsequent mtime preservation and state updates unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/local-mount/src/auto-sync.ts`:
- Around line 879-904: Harden createDirectoriesWithin against concurrent symlink
swaps by replacing the check-then-mkdir traversal with a directory-fd/O_NOFOLLOW
approach, or otherwise re-verifying each component after creation before
proceeding; ensure mkdirSync never creates outside root. Add or update the
corresponding confinement coverage in
packages/local-mount/src/auto-sync-confinement.test.ts at the cited range to
exercise this race-safe behavior.
---
Outside diff comments:
In `@packages/local-mount/src/auto-sync.ts`:
- Around line 678-697: Update doMountToProject to read the mount source mode via
safeFileStat(mountAbs) and pass that mode to safeCopyOnto as its optional mode
argument when available; omit the argument when unavailable, preserving the
existing copy behavior. Keep the subsequent mtime preservation and state updates
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58390e5a-ed55-44ac-a700-c5d6307e3e0d
📒 Files selected for processing (4)
packages/local-mount/CHANGELOG.mdpackages/local-mount/src/auto-sync-confinement.test.tspackages/local-mount/src/auto-sync.tspackages/local-mount/src/mount-reflink.test.ts
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 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
All three findings addressed in P1 — predictable, non-exclusive temporary file. Correct and serious. The name was My own matrix missed this because it never tested a pre-planted temporary. That gap is now covered. P2 — temporary basename could exceed NAME_MAX. Also correct, and the failure mode is nastier than it looks: Major — check-then-mkdir race in On non-Linux there is no equivalent without a native Verification: confinement matrix 14/14 on macOS and Linux (the platform branch is exercised on both), package suite 89/89.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/local-mount/src/auto-sync-confinement.test.ts (1)
263-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify or strengthen the collision-refusal claim.
The comment at Line 264-267 states that this test "asserts the property directly." The random temporary name generator has a negligible chance of producing the exact planted name
.rfsync-deadbeefdeadbeefde. The test at Line 275 only confirms that an unrelated sync ofok.txtsucceeds and does not touchs.victim. It does not exercise theCOPYFILE_EXCL/EEXISTrefusal path, because the write took.txtnever collides with the planted symlink.This test verifies that a planted entry at a plausible temp name does not interfere with unrelated syncs. It does not verify that a real collision is refused rather than followed. The comment overstates the guarantee, which can mislead future maintainers about what the security-critical refusal path is exercised by.
Two options:
- Narrow the comment to describe only what is verified: presence of a planted entry does not disrupt unrelated syncs.
- If the temp-name generator is injectable or mockable, add a test that forces the generator to return a name matching a pre-planted symlink and assert the copy is refused rather than following the symlink.
✏️ Example comment correction
- // 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. + // The exclusive create is what makes collisions safe: COPYFILE_EXCL fails + // if anything is already at the name, symlink included. Exhaustively + // forcing a collision with the random generator is impractical here, so + // this test only verifies that a planted entry at a plausible name does + // not interfere with an unrelated sync elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/local-mount/src/auto-sync-confinement.test.ts` around lines 263 - 277, Update the test comment in the symlink-planted test to describe only the behavior actually exercised: a pre-existing plausible temporary entry does not disrupt an unrelated sync or modify the victim. Remove claims that it directly verifies collision refusal or the exclusive-create path; leave the test logic unchanged unless the temporary-name generator can be deterministically controlled to exercise that collision.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/local-mount/src/auto-sync-confinement.test.ts`:
- Around line 279-285: Update the “leaves no temporary files behind” test to
assert that each syncAttempt call for a.txt and b.txt succeeds before checking
the mount directory. Preserve the existing temporary-file cleanup assertion so
the test requires both real writes and subsequent cleanup.
---
Nitpick comments:
In `@packages/local-mount/src/auto-sync-confinement.test.ts`:
- Around line 263-277: Update the test comment in the symlink-planted test to
describe only the behavior actually exercised: a pre-existing plausible
temporary entry does not disrupt an unrelated sync or modify the victim. Remove
claims that it directly verifies collision refusal or the exclusive-create path;
leave the test logic unchanged unless the temporary-name generator can be
deterministically controlled to exercise that collision.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fc9b4770-86f6-4a3a-99cb-1a1b8737bf5e
📒 Files selected for processing (3)
packages/local-mount/src/auto-sync-confinement.test.tspackages/local-mount/src/auto-sync.tspackages/local-mount/src/mount-reflink.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/local-mount/src/mount-reflink.test.ts
- packages/local-mount/src/auto-sync.ts
| 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([]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the return value of syncAttempt before checking cleanup.
Line 281 and Line 282 discard the return value of syncAttempt. If a regression causes the sync to be silently refused instead of written, no temporary file would ever be created, and the assertion at Line 283 would still pass. The test would give a false positive for cleanup coverage.
Add an explicit check that each sync actually succeeded, so this test only passes when a real write-and-cleanup cycle occurred.
🐛 Proposed fix
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(syncAttempt(s, path.join(s.mount, 'a.txt'))).toBe('written');
+ expect(syncAttempt(s, path.join(s.mount, 'b.txt'))).toBe('written');
expect(readdirSync(s.mount).filter((f) => f.startsWith('.rfsync-'))).toEqual([]);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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([]); | |
| }); | |
| }); | |
| it('leaves no temporary files behind', () => { | |
| const s = sandbox(); | |
| expect(syncAttempt(s, path.join(s.mount, 'a.txt'))).toBe('written'); | |
| expect(syncAttempt(s, path.join(s.mount, 'b.txt'))).toBe('written'); | |
| expect(readdirSync(s.mount).filter((f) => f.startsWith('.rfsync-'))).toEqual([]); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/local-mount/src/auto-sync-confinement.test.ts` around lines 279 -
285, Update the “leaves no temporary files behind” test to assert that each
syncAttempt call for a.txt and b.txt succeeds before checking the mount
directory. Preserve the existing temporary-file cleanup assertion so the test
requires both real writes and subsequent cleanup.
1 participant
Summary
Two confirmed escapes in the project↔mount sync path let a write land outside the mount. Both are fixed structurally, with an adversarial suite that reproduces them.
Verified by content, not by return value — in both cases a file outside the boundary went from its own content to the payload from inside:
realpathcan't resolve it (a hardlink has no target). The parent was resolved and the final component was checked for being a symlink, but nothing checked link count, socopyFileSyncwrote straight through.isSymlinkTarget(target)andcopyFileSync(target)are two separate path lookups. A swap in between was followed.Plus one refusal-with-a-side-effect:
resolveSafeWriteTargetranmkdirSync(parent, { recursive: true })before validating the resolved parent, so a symlinked component created directories outside the root and only then refused.doMountToProjectshares the same helper, so the same exposure applied writing into the user's real project directory from the agent-writable mount.The fix
Copy to a temporary sibling in the already-validated parent, then
renameover the target.renamereplaces the directory entry — it neither writes through a hardlink nor follows a symlink, and it cannot be raced into doing so. Another check would not have closed this; the check and the use were two different lookups, which is the defect.Side benefits: the write becomes atomic (no partial or zero-length window), and the readonly
chmoddance disappears — the mode is applied to the temp file before the rename, so a0o444mount copy no longer has to be made writable first. That was a small window where the readonly guarantee didn't hold.Reflink cloning is preserved. The copy into the temp file still requests
COPYFILE_FICLONE.Directory creation now walks one component at a time and refuses a symlinked component rather than traversing it.
recursive: truecreates through a symlink, which is the traversal it was meant to prevent.Tests
auto-sync-confinement.test.ts— 11 cases driving the real resolver and caller sequence, not a copy. Traversal, absolute path, symlinked final component, symlinked intermediate, hardlink, post-check swap, refusal side effects, plus positive controls (new file, nested dirs, replace existing, symlinked mount root) so a fix that refuses everything can't pass.The suite asserts containment — nothing outside the mount is modified — rather than refusal. For a sync engine, severing a hostile link and writing correctly inside the boundary is the right response; refusing would stall sync on that path forever, which an adversary triggers by planting one link.
expectInsideMountpins that the write still lands, so "safe" can't quietly become "broken".mount-reflink.test.tsexpectations updated: the copy destination is now the temp sibling. The reflink flag assertion — the point of that test — is unchanged.Package suite: 86/86.
Provenance
The matrix comes from the Agent Relay × Ratify design-partner spike, where it found four defects in equivalent code — three 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. Pointing it at this code was the point of the exercise.
Review notes
resolveSafeWriteTarget,isSymlinkTarget, andsafeCopyOntoare exported so the suite drives the real code rather than a copy. They are not part of the package's public API.resolveSafeWriteTargetrequiresrootto be already realpath'd — its only caller does this atmount.ts:195. Now documented on the function; it was an undocumented precondition.🤖 Generated with Claude Code