diff --git a/packages/coding-agent/src/tools/file-write-fallback.ts b/packages/coding-agent/src/tools/file-write-fallback.ts index 18947fa02..dd7b14fa9 100644 --- a/packages/coding-agent/src/tools/file-write-fallback.ts +++ b/packages/coding-agent/src/tools/file-write-fallback.ts @@ -93,6 +93,17 @@ * That narrows the seam for a sandbox that also hides the ancestors of a denied * path, which is the honest cost of not handing over an unverifiable target. * + * That refusal is load-bearing for more than symlink safety, and relaxing it needs + * care. `apply_patch`'s `create` and rename-destination refuse to overwrite, and + * they decide that with `Bun.file(dst).exists()`, which reports `false` when the + * parent hides the target's metadata rather than distinguishing "absent" from + * "unknown". The non-overwrite contract holds today only because the same denied + * `lstat` that fools that check also stops this seam from brokering — a privileged + * writer, the one party that could enforce exclusivity itself, is never handed the + * path. Broker an unverifiable destination and a `create` starts clobbering a + * protected file it was told not to touch; a request field carrying explicit + * exclusive-create intent would be the prerequisite for that change. + * * ## Scope of the registry * * Handlers live in one process-wide list, and a process can host several sessions diff --git a/packages/coding-agent/test/tools/file-write-fallback.test.ts b/packages/coding-agent/test/tools/file-write-fallback.test.ts index ad128f5a3..50efeb016 100644 --- a/packages/coding-agent/test/tools/file-write-fallback.test.ts +++ b/packages/coding-agent/test/tools/file-write-fallback.test.ts @@ -495,6 +495,46 @@ describe("writeFileWithFallback", () => { }, ); }); + + it("never brokers an exclusive create whose destination cannot be proven absent", async () => { + // `apply_patch`'s `create` refuses to overwrite, and it decides that with + // `Bun.file(dst).exists()`, which reports `false` when the parent hides the + // target's metadata instead of distinguishing "absent" from "unknown". The + // non-overwrite contract survives regardless, because the same denied `lstat` + // that fools the existence check also stops the seam from brokering: a + // privileged writer is never handed a destination whose identity is unproven, + // and it is the only party that could have enforced exclusivity itself. + // + // Those are two independent guards in two files, so this pins the pair. If the + // seam is ever relaxed to broker an unverifiable path, a `create` would start + // silently clobbering a protected file it was told not to touch. + const opaque = path.join(root, "opaque"); + await fs.mkdir(opaque); + const victim = path.join(opaque, "victim.txt"); + await Bun.write(victim, "original\n"); + await fs.chmod(opaque, 0o000); + + let called = false; + disposers.push( + addFileWriteFallback(async () => { + called = true; + return true; + }), + ); + + try { + // The premise: the existence check cannot see the file it must not clobber. + expect(await Bun.file(victim).exists()).toBe(false); + + await expect( + applyPatch({ path: victim, op: "create", diff: "clobbered\n" }, { cwd: root }), + ).rejects.toMatchObject({ code: expect.stringMatching(/^(EACCES|EPERM)$/) }); + expect(called).toBe(false); + } finally { + await fs.chmod(opaque, 0o700); + } + expect(await Bun.file(victim).text()).toBe("original\n"); + }); }); });