From 9768021746200d03e9707d41ba28be5b70c33e1c Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 21 Aug 2026 14:17:12 -0500 Subject: [PATCH] Resolve the last component of a write, not just the directory Path confinement is described here as three layers, and the third one, resolving symlinks, stops one component short on the write path. `resolvePath` resolves the directory a write lands in and then returns the lexical target, so a link sitting where the file goes is followed by `writeFile` and the bytes land wherever it points. A workspace holding `notes.md -> /etc/crontab` takes a write to `notes.md` and puts it in `/etc/crontab`, as root, with the request refused by nothing. Nothing in this API creates a link, which is why it reads as covered. The links come from everywhere else: a shell session, an archive a Bot unpacked, a volume left over from an earlier deployment, and in a container shared between Bots, the other Bot. A dangling link is refused rather than resolved. It points at a file that does not exist yet, so there is nothing to compare against the root, and writing through one creates the file it names, which is the same escape with an extra step. Links pointing back inside still work, for writes as well as reads. The guard has to confine rather than forbid; refusing every link would pass every refusal test here and break what links are legitimately for. --- agent-computer/src/workspace.ts | 27 +++++++++++-- agent-computer/tests/workspace.test.ts | 54 +++++++++++++++++++++++++- 2 files changed, 77 insertions(+), 4 deletions(-) diff --git a/agent-computer/src/workspace.ts b/agent-computer/src/workspace.ts index 4e06f09d..8aa9c602 100644 --- a/agent-computer/src/workspace.ts +++ b/agent-computer/src/workspace.ts @@ -21,6 +21,7 @@ * can be tested against a temporary directory instead of being taken on trust. */ import { + lstat, mkdir, readdir, readFile, @@ -131,9 +132,29 @@ export function createWorkspace( } assertInside(root, realAnchor, wanted); - // For a write, return the full lexical target. It is already proven contained lexically, and the - // deepest existing directory is proven contained after symlinks, so `mkdir -p` can only create the - // rest inside the workspace. + // The last component, which the anchor above deliberately skipped. + // + // Resolving the directory proves where the write lands only while the name inside it is a name. + // If it is already a symlink, `writeFile` follows it and the bytes go wherever it points, so the + // link has to be resolved too. A dangling one is refused rather than resolved: it points at a + // file that does not exist yet, so there is nothing to check, and writing through it would + // create the file it names. + if (forWrite) { + const link = await lstat(target).catch(() => null); + if (link?.isSymbolicLink()) { + const realTarget = await realpath(target).catch(() => null); + if (!realTarget) { + throw new WorkspacePathError( + `${wanted} is a link to somewhere that does not exist, so it cannot be written through.`, + ); + } + assertInside(root, realTarget, wanted); + } + } + + // For a write, return the full lexical target. It is already proven contained lexically, the + // deepest existing directory is proven contained after symlinks, and a link at the end is proven + // to stay inside, so `mkdir -p` can only create the rest inside the workspace. return forWrite ? target : realAnchor; } diff --git a/agent-computer/tests/workspace.test.ts b/agent-computer/tests/workspace.test.ts index 47eb97fb..117c38a6 100644 --- a/agent-computer/tests/workspace.test.ts +++ b/agent-computer/tests/workspace.test.ts @@ -1,5 +1,12 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; -import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; +import { + mkdir, + mkdtemp, + readFile, + rm, + symlink, + writeFile, +} from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -206,6 +213,46 @@ describe("escaping the workspace", () => { ); }); + test("refuses to write THROUGH a symlink at the file itself", async () => { + // The directory a write lands in is resolved and the last component is not, so a link left where + // the file goes carries the bytes out. Nothing in this API makes one, which is the whole reason + // it is easy to miss: a shell session makes one, an archive a Bot unpacked carries one, and in a + // container shared between Bots the other Bot makes one. + const victim = join(outside, "victim.txt"); + await writeFile(victim, "original", "utf8"); + await symlink(victim, join(root, "notes.md")); + + await expect(workspace().write("notes.md", "owned")).rejects.toThrow( + WorkspacePathError, + ); + expect(await readFile(victim, "utf8")).toBe("original"); + }); + + test("refuses to append THROUGH a symlink at the file itself", async () => { + // Append is the same door. It takes the same resolved path and differs only in the write flag. + const victim = join(outside, "victim.txt"); + await writeFile(victim, "original", "utf8"); + await symlink(victim, join(root, "log.txt")); + + await expect( + workspace().write("log.txt", "owned", { append: true }), + ).rejects.toThrow(WorkspacePathError); + expect(await readFile(victim, "utf8")).toBe("original"); + }); + + test("refuses to write THROUGH a symlink pointing at a file that does not exist yet", async () => { + // A dangling link resolves to nothing, so a check that asks where it points learns nothing and + // lets it past. Writing through one CREATES the file it names, which is the same escape with an + // extra step. + const victim = join(outside, "not-yet.txt"); + await symlink(victim, join(root, "draft.txt")); + + await expect(workspace().write("draft.txt", "owned")).rejects.toThrow( + WorkspacePathError, + ); + expect(await readFile(victim, "utf8").catch(() => null)).toBe(null); + }); + test("a symlink pointing back INSIDE the workspace still works", async () => { // The guard must confine, not merely forbid symlinks: refusing every link would be easier and // would break legitimate use. @@ -213,6 +260,11 @@ describe("escaping the workspace", () => { await ws.write("real/data.txt", "inside"); await symlink(join(root, "real"), join(root, "alias")); expect((await ws.read("alias/data.txt")).text).toBe("inside"); + + // Writing through it too, because a guard that only forbids would pass the refusal tests above + // while breaking the thing links are legitimately for. + await ws.write("alias/data.txt", "still inside"); + expect((await ws.read("real/data.txt")).text).toBe("still inside"); }); test("a sibling directory sharing the root's name prefix is still outside", async () => {