From 2c5a9c43aa4454961e3f708cd7d84c31c8b8d18d Mon Sep 17 00:00:00 2001 From: Larry Gordon Date: Mon, 10 Aug 2026 09:10:16 -0700 Subject: [PATCH] test(tools): pinned the exclusive-create guard against the fallback seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review raised that `apply_patch`'s non-overwrite contract for `create` and a rename destination is decided with `Bun.file(dst).exists()`, which reports `false` when the parent hides the target's metadata rather than distinguishing "absent" from "unknown" — so a privileged handler could be asked to write over a protected file it was told not to touch. Reproduced all three shapes: no handler is consulted in any of them. A hidden-metadata destination is refused by the path resolver, because the same denied `lstat` that fools the existence check also leaves the final component unproven, and an unverifiable destination is never brokered. A destination that is a symlink onto a protected file is caught earlier — `exists()` follows the link and reports `true`. A plainly visible existing file is caught by the same check. So the contract holds, but it holds through two independent guards in two files. Pinned that with a regression test asserting the premise (the existence check cannot see the file), that no handler is consulted, and that the file is intact; deleting the resolver's symlink proof makes it fail. Recorded the coupling in the module header too, since relaxing the refusal to broker unverifiable paths would silently break exclusivity and needs an explicit intent field first. --- .../src/tools/file-write-fallback.ts | 11 +++++ .../test/tools/file-write-fallback.test.ts | 40 +++++++++++++++++++ 2 files changed, 51 insertions(+) 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"); + }); }); });