-
Notifications
You must be signed in to change notification settings - Fork 0
fix(local-mount): close two mount-boundary escapes in the sync path #391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
238c8c7
test(local-mount): adversarial confinement matrix — 2 confirmed mount…
khaliqgant 203d028
fix(local-mount): close two mount-boundary escapes in the sync path
khaliqgant 52edecd
docs(local-mount): changelog for the mount-boundary fixes
khaliqgant 7ba28ea
fix(local-mount): harden the temporary file and the directory walk
khaliqgant File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,331 @@ | ||
| /** | ||
| * 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; | ||
| * 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 | ||
| * 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, safeCopyOnto } 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<string, string> { | ||
| const out: Record<string, string> = {}; | ||
| 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] = '<broken>'; | ||
| else if (st.isDirectory()) { | ||
| out[key] = '<dir>'; | ||
| walk(full, key); | ||
| } else if (st.isFile()) out[key] = readFileSync(full, 'utf8'); | ||
| else out[key] = '<special>'; | ||
| } | ||
| }; | ||
| 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?.(); | ||
| return safeCopyOnto(s.source, target) ? 'written' : 'refused'; | ||
| } | ||
|
|
||
| /** | ||
| * 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); | ||
| try { | ||
| run(); | ||
| } catch { | ||
| /* throwing is an acceptable refusal */ | ||
| } | ||
| 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); | ||
| } | ||
| } | ||
|
|
||
| /** 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', () => { | ||
| 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(); | ||
| 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', () => { | ||
| // 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); | ||
| }), | ||
| ); | ||
| // rename replaces the symlink itself rather than following it. | ||
| expectInsideMount(target); | ||
| expect(readFileSync(s.victim, 'utf8')).toBe(VICTIM); | ||
| }); | ||
| }); | ||
|
|
||
| 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 — 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. | ||
|
|
||
| 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); | ||
| } | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the return value of
syncAttemptbefore 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
🤖 Prompt for AI Agents