Resolve the name a write lands on, not only the directory above it

CHANGELOG.md

@@ -38,6 +38,13 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged.
   Which way it went is printed at start-up either way.
 
 ### Fixed
+- **A write could follow a symlink out of the Bot's workspace.** The confinement resolved the
+  directory a write would land in but not the name it would land on, so a link left at `notes.txt`
+  pointing outside was followed by the write; a read through the identical link was already refused.
+  The gateway had already decided and written the audit row against the path as it was asked for, so a
+  rule written for `credentials/` never saw the file that was written and the trail named a file
+  nothing had touched. A dangling link escaped the same way, because resolving the path throws where
+  the write would still land. Links pointing back inside the workspace continue to work.
 - **A Bot could become root inside its container.** `sudo` was granted as `NOPASSWD: ALL`, and the
   comment above it named the two conditions that made that acceptable: the container being one Bot's
   alone, and not holding a database. The image meets neither, because the supervisor is deliberately

agent-computer/src/workspace.ts

@@ -14,16 +14,19 @@
  *  3. The resolved path must still be inside the root after symlinks are followed. This is the layer
  *     people miss: a symlink placed inside the workspace (by an earlier write, or by a page the Bot
  *     downloaded something from) passes the lexical check and then points anywhere on the filesystem.
- *     For a write, the file may not exist yet, so it is the deepest existing ancestor that gets
- *     resolved, which is the directory the write will actually land in.
+ *     For a write, the file may not exist yet, so the deepest existing ancestor gets resolved, which
+ *     is the directory the write will land in, AND the name itself is resolved when something is
+ *     already there, because `writeFile` follows a link at the last component too.
  *
  * A factory taking its root as an argument rather than reading the environment, so the confinement
  * can be tested against a temporary directory instead of being taken on trust.
  */
 import {
+  lstat,
   mkdir,
   readdir,
   readFile,
+  readlink,
   realpath,
   stat,
   writeFile,
@@ -131,10 +134,42 @@ 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.
-    return forWrite ? target : realAnchor;
+    if (!forWrite) return realAnchor;
+
+    /*
+     * Layer three again, for the last component rather than the directory holding it.
+     *
+     * Containing `dirname(target)` proves where a NEW file would be created. It proves nothing about
+     * a name that already exists, and `writeFile` follows a symlink at the last component the same
+     * way `readFile` does. A link at `notes.txt` pointing at `/root/.ssh/authorized_keys` passes
+     * every check above, having no `..`, not being absolute, and sitting directly in the workspace,
+     * and the bytes land outside the volume. The read side already refuses the identical link; the
+     * write side was the asymmetry.
+     *
+     * The link has to get there first, which takes a shell or an archive that was unpacked with one,
+     * so this is not a fresh escape for a Bot that already has `run_command`: that Bot can write
+     * outside directly. What it is, is a hole in what the gateway can still see. The decision and the
+     * audit row are both made against the path as the Bot asked for it, so a rule written for
+     * `credentials/` or `*.env` is evaluated against `notes.txt` and never sees the file that gets
+     * written, and the row names a file in the workspace that nothing touched. A deployment that
+     * denies `run_command` and allows writes is relying on exactly that, and so is one reading the
+     * trail afterwards. A permissive workspace is a decision a deployment can make. A trail that
+     * describes a different file from the one on disk is not.
+     */
+    const landing = await writeDestination(target, wanted);
+    if (landing === target) return target;
+
+    // A link was followed, so the destination gets the checks the requested path already passed:
+    // inside lexically, and inside after the directory holding it is resolved.
+    assertInside(root, landing, wanted);
+    const holder = await realpath(dirname(landing)).catch(() => null);
+    if (holder === null) {
+      throw new WorkspacePathError(
+        `${wanted} points at somewhere that does not exist, so where a write would land cannot be established.`,
+      );
+    }
+    assertInside(root, holder, wanted);
+    return landing;
   }
 
   return {
@@ -277,6 +312,45 @@ function assertInside(root: string, candidate: string, shown?: string): void {
   }
 }
 
+/**
+ * How many links a chain may pass through before it is treated as a cycle rather than a path.
+ *
+ * Linux gives up at 40. Anything approaching this is a loop or a deliberate attempt to make the walk
+ * expensive, and neither is a file a Bot needs to write.
+ */
+const MAX_LINK_HOPS = 32;
+
+/**
+ * Where a write to `target` would actually put the bytes.
+ *
+ * Returns `target` unchanged when nothing is there or what is there is not a link, which is every
+ * ordinary write. Only a name that is already a symlink walks.
+ *
+ * Walked with `lstat` and `readlink` rather than resolved with `realpath`, because `realpath` throws
+ * on a DANGLING link and `writeFile` creates the file at its destination regardless. A link aimed at
+ * a name that does not exist yet would otherwise escape through the failure path rather than the
+ * success one, which is the harder version of the bug to notice.
+ *
+ * Confining rather than forbidding, the same as the read side. A link that points back inside the
+ * workspace keeps working: refusing every link would be easier and would break legitimate use.
+ */
+async function writeDestination(
+  target: string,
+  shown: string,
+): Promise<string> {
+  let current = target;
+  for (let hop = 0; hop <= MAX_LINK_HOPS; hop += 1) {
+    const entry = await lstat(current).catch(() => null);
+    // Nothing there, or something that is not a link. This is where the write lands.
+    if (entry === null || !entry.isSymbolicLink()) return current;
+    // A relative link is relative to the directory the link sits in, not to the workspace root.
+    current = resolve(dirname(current), await readlink(current));
+  }
+  throw new WorkspacePathError(
+    `${shown} is a chain of links that does not settle, so where a write would land cannot be established.`,
+  );
+}
+
 /** The closest ancestor of `target` that exists, never above `root`. */
 async function nearestExistingAncestor(
   root: string,

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,74 @@ describe("escaping the workspace", () => {
     );
   });
 
+  test("refuses to write THROUGH a symlinked FILE that points outside", async () => {
+    // The asymmetry between the two tests above. Resolving `dirname` catches a link standing in for a
+    // directory; a link standing in for the FILE has the workspace as its parent and passes, and then
+    // `writeFile` follows it. Refusing is only half of what this asserts: the file outside has to be
+    // untouched afterwards, because an error thrown after the bytes landed would still be an escape.
+    const secret = join(outside, "secret.txt");
+    await symlink(secret, join(root, "notes.txt"));
+    await expect(workspace().write("notes.txt", "owned")).rejects.toThrow(
+      WorkspacePathError,
+    );
+    expect(await readFile(secret, "utf8")).toBe("a private key");
+  });
+
+  test("refuses to append THROUGH a symlinked file that points outside", async () => {
+    // `append` is a separate flag reaching a separate `writeFile` mode, so it is a separate way in.
+    const secret = join(outside, "secret.txt");
+    await symlink(secret, join(root, "log.txt"));
+    await expect(
+      workspace().write("log.txt", "owned", { append: true }),
+    ).rejects.toThrow(WorkspacePathError);
+    expect(await readFile(secret, "utf8")).toBe("a private key");
+  });
+
+  test("refuses to write through a DANGLING link that points outside", async () => {
+    // The harder half. `realpath` throws on a link whose destination does not exist, so a check built
+    // on it treats this as "no such file" and lets the write through the failure path, while
+    // `writeFile` creates the destination regardless. Nothing exists here to prove the escape with,
+    // so the assertion is that the file was never created outside.
+    const notThere = join(outside, "planted.txt");
+    await symlink(notThere, join(root, "fresh.txt"));
+    await expect(workspace().write("fresh.txt", "owned")).rejects.toThrow(
+      WorkspacePathError,
+    );
+    await expect(readFile(notThere, "utf8")).rejects.toThrow();
+  });
+
+  test("refuses a chain of links that ends up outside", async () => {
+    // One hop is the obvious case and the only one a single `readlink` would catch.
+    await symlink(join(outside, "secret.txt"), join(root, "second.txt"));
+    await symlink(join(root, "second.txt"), join(root, "first.txt"));
+    await expect(workspace().write("first.txt", "owned")).rejects.toThrow(
+      WorkspacePathError,
+    );
+    expect(await readFile(join(outside, "secret.txt"), "utf8")).toBe(
+      "a private key",
+    );
+  });
+
+  test("refuses a cycle of links rather than following it forever", async () => {
+    // Two links pointing at each other never reach something that is not a link. The walk has to stop
+    // on its own and say why, instead of spinning or surfacing an ELOOP from the write.
+    await symlink(join(root, "b.txt"), join(root, "a.txt"));
+    await symlink(join(root, "a.txt"), join(root, "b.txt"));
+    await expect(workspace().write("a.txt", "owned")).rejects.toThrow(
+      WorkspacePathError,
+    );
+  });
+
+  test("writing THROUGH a link that points back inside still works", async () => {
+    // Confining, not forbidding, on the write side too. Refusing every link would pass the tests
+    // above and quietly break a Bot that keeps `latest.csv` pointing at the newest report.
+    const ws = workspace();
+    await ws.write("real/data.txt", "before");
+    await symlink(join(root, "real/data.txt"), join(root, "alias.txt"));
+    await ws.write("alias.txt", "after");
+    expect((await ws.read("real/data.txt")).text).toBe("after");
+  });
+
   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.