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 () => {