Resolve the name a write lands on, not only the directory above it (#74)

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

`resolvePath` resolved `dirname(target)` for a write and handed back the lexical
target, so a symlink at the last component was followed by `writeFile`. A read
through the identical link was already refused; the write side was the asymmetry.

A link at `notes.txt` pointing outside has no `..`, is not absolute, and sits
directly in the workspace, so it passed all three layers and the bytes landed
outside the volume.

The walk uses `lstat` and `readlink` rather than `realpath`, because `realpath`
throws on a dangling link while `writeFile` creates its destination regardless,
so that shape escaped through the failure path rather than the success one. Hops
are bounded, so a cycle is refused instead of surfacing an `ELOOP` from the write.

Confining rather than forbidding, as on the read side: a link pointing back
inside the workspace keeps working.

The escape is not the main cost, because a Bot holding `run_command` can write
outside directly. The cost is that the gateway decides and writes the audit row
in another process, from the path as it was asked for, so a rule written for
`credentials/` never sees the file that is written and the trail names a file
nothing touched.

* Resolve where the link lands before checking it, not after

Reapplying this after a rebase dropped it.

The leaf walk checked the raw destination first and canonicalised the directory
holding it second. `root` is itself a real path, so a destination that still runs
through a symlinked ancestor fails the lexical comparison even when it points
straight back into the workspace. Anywhere /tmp is a link to /private/tmp, which
is every macOS machine and no Linux CI runner, a legitimate in-workspace link is
refused, and that asymmetry is why this branch's own "points back inside still
works" test passes in CI and fails on a developer's machine.

Resolving the holder first fixes it, and the destination is rebuilt from the
resolved directory so what the function returns is the path that will actually be
written rather than the one that was asked for. It failed safe either way, so this
was a correctness bug rather than a hole.

---------

Co-authored-by: David McKay <davidmckayv@users.noreply.github.com>

Author: Hotragn

CHANGELOG.md

@@ -159,6 +159,13 @@ Sessions survive and nobody signs in again.
   access and refresh tokens use Better Auth's own encryption, keyed on `BETTER_AUTH_SECRET`.
 - **A failed provider registration looked like a button that did not work.** The error was rendered
   on the page behind the dialog, which was covering it.
+- **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,21 +14,32 @@
  *  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,
 } from "node:fs/promises";
-import { dirname, isAbsolute, relative, resolve, sep } from "node:path";
+import {
+  basename,
+  dirname,
+  isAbsolute,
+  join,
+  relative,
+  resolve,
+  sep,
+} from "node:path";
 
 export class WorkspacePathError extends Error {
   constructor(message: string) {
@@ -131,10 +142,48 @@ 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.
+    /*
+     * The holder is resolved BEFORE either check. `root` is a real path, so comparing it against a
+     * destination that still runs through a symlinked ancestor refuses a link that points straight
+     * back inside, which is what happens wherever the workspace sits behind one.
+     */
+    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);
+    const resolved = join(holder, basename(landing));
+    assertInside(root, resolved, wanted);
+    return resolved;
   }
 
   return {
@@ -277,6 +326,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.