Keep a Bot id from naming a directory it should not

agent-computer/src/bot-id.ts

@@ -0,0 +1,50 @@
+/**
+ * What a Bot id is allowed to be, before it becomes a path.
+ *
+ * The id arrives as the `x-openbot-bot-id` header, or as the `bot` query parameter on the stream, and
+ * the API server forwards whatever segment a caller put in the URL. It then becomes the directory a
+ * Chromium profile lives in, which `reset` deletes with `rm -rf`, as root. So this is the same class
+ * of input `workspace.ts` already treats as hostile, and it needs the same answer: an id that is not
+ * a plain name never reaches `join`.
+ *
+ * The rules match `supervisor/src/names.ts`, which has held this line for container and volume names
+ * since it was written: letters, digits, hyphen and underscore, starting with a letter or digit. No
+ * separators, so a name cannot escape into another path segment. No dots, which Docker permits and
+ * which invite `..` reasoning. Deliberately narrower than the filesystem allows, because the set of
+ * ids a deployment legitimately uses is narrower still.
+ *
+ * It lives in its own file rather than in `index.ts` for the reason `authorisation.ts` does: that file
+ * imports Playwright at module scope, so anything in it needs Chrome merely to be imported by a test,
+ * and a decision this size should be testable without a browser.
+ */
+
+/** Long enough for a uuid-shaped agent id, short enough to stay a sane directory name. */
+const MAX_BOT_ID = 64;
+
+const ALLOWED = /^[A-Za-z0-9][A-Za-z0-9_-]*$/;
+
+export function isPlainBotId(value: string): boolean {
+  return value.length <= MAX_BOT_ID && ALLOWED.test(value);
+}
+
+/** Thrown rather than returned, so a caller cannot use the path by forgetting to check. */
+export class UnusableBotIdError extends Error {
+  constructor(botId: string) {
+    super(
+      `A bot id may contain only letters, digits, hyphen and underscore, and must start with a letter or digit: ${JSON.stringify(botId)}`,
+    );
+    this.name = "UnusableBotIdError";
+  }
+}
+
+/**
+ * Where this Bot's browser profile lives.
+ *
+ * The allow-list is the guarantee: an id with no separator and no dot cannot address anything but a
+ * single directory inside the root. The join stays here rather than at the call site so that there is
+ * one place a profile path can be built, and it is the checked one.
+ */
+export function profileDirectoryFor(root: string, botId: string): string {
+  if (!isPlainBotId(botId)) throw new UnusableBotIdError(botId);
+  return `${root}/${botId}`;
+}

agent-computer/src/index.ts

@@ -2,6 +2,7 @@ import { serve } from "bun";
 import type { Page } from "playwright";
 import { parseAriaSnapshot, type SnapshotElement } from "./aria-snapshot";
 import { isOpenPath, matchesToken, offeredToken } from "./authorisation";
+import { isPlainBotId } from "./bot-id";
 import {
   type Control,
   ControlError,
@@ -171,7 +172,17 @@ const profiles = createProfiles(process.env.PROFILES_DIR ?? "/profiles");
  * Bot to name, such as a health check, so the container stays demonstrable on its own rather than
  * refusing everything that is not the server.
  */
-const DEFAULT_BOT_ID = process.env.COMPUTER_BOT_ID ?? "shared";
+const DEFAULT_BOT_ID = (() => {
+  const configured = process.env.COMPUTER_BOT_ID ?? "shared";
+  // At boot rather than per request. This is the id every unheadered call falls back to, so a value
+  // the path rules refuse would answer 400 to everything and read as a broken computer.
+  if (!isPlainBotId(configured)) {
+    throw new Error(
+      "COMPUTER_BOT_ID may contain only letters, digits, hyphen and underscore, and must start with a letter or digit.",
+    );
+  }
+  return configured;
+})();
 
 async function currentPage(botId: string): Promise<Page> {
   return profiles.page(botId);
@@ -440,6 +451,14 @@ serve<StreamData>({
     // Resolved once per request. Everything below that touches a browser, a takeover or a snapshot
     // goes through this Bot's session, so there is no path where one Bot's call reaches another's.
     const botId = botIdOf(request);
+
+    // Refused here rather than deeper down, because the id names a directory and the API server
+    // forwards whatever URL segment a caller typed. `reset` deletes that directory as root.
+    // `/health` is exempt for the same reason it is exempt from the token: it names no Bot, and an
+    // orchestrator's probe must not fail on a header it never meant to send.
+    if (!isOpenPath(url.pathname) && !isPlainBotId(botId)) {
+      return json({ error: "That is not a usable bot id." }, 400);
+    }
     const session = sessionFor(botId);
 
     if (url.pathname === "/stream") {
@@ -450,6 +469,9 @@ serve<StreamData>({
        * still wins where there is one.
        */
       const streamBotId = botIdOf(request, url.searchParams.get("bot"));
+      if (!isPlainBotId(streamBotId)) {
+        return json({ error: "That is not a usable bot id." }, 400);
+      }
       if (server.upgrade(request, { data: { botId: streamBotId } }))
         return undefined as unknown as Response;
       return json({ error: "Expected a WebSocket upgrade." }, 400);

agent-computer/src/profiles.ts

@@ -35,6 +35,7 @@
 import { readdir, rm } from "node:fs/promises";
 import { join } from "node:path";
 import { type BrowserContext, chromium, type Page } from "playwright";
+import { profileDirectoryFor } from "./bot-id";
 import { egressFor, egressLabel } from "./egress";
 
 /** The viewport, which is what a person's click coordinates are relative to. */
@@ -114,7 +115,10 @@ export function createProfiles(root: string) {
   /** Launches in flight, so a cold computer is started once however many callers ask at once. */
   const starting = new Map<string, Promise<Page>>();
 
-  const directoryFor = (botId: string): string => join(root, botId);
+  // Checked, not joined. `join(root, botId)` normalizes `..` away, so a Bot id of `../workspace`
+  // used to resolve outside the root and `reset` would delete whatever was there.
+  const directoryFor = (botId: string): string =>
+    profileDirectoryFor(root, botId);
 
   const sweepLocks = async (dir: string): Promise<void> => {
     await Promise.all(

agent-computer/tests/bot-id.test.ts

@@ -0,0 +1,64 @@
+import { describe, expect, test } from "bun:test";
+import { isPlainBotId, profileDirectoryFor } from "../src/bot-id";
+
+/**
+ * The Bot id arrives as a request header and becomes a filesystem path, so it is the same kind of
+ * input the workspace already treats as hostile. These are the cases that must never reach `join`.
+ */
+describe("isPlainBotId", () => {
+  test.each([
+    ["sales", "an ordinary id"],
+    ["support-1", "a hyphen"],
+    ["risk_analyst", "an underscore"],
+    ["shared", "the default this process falls back to"],
+    ["7f3c1a2e9b4d4f0e8a1c2d3e4f5a6b7c", "a uuid-shaped id"],
+    ["a", "a single character"],
+  ])("accepts %s (%s)", (id) => {
+    expect(isPlainBotId(id)).toBe(true);
+  });
+
+  test.each([
+    ["..", "the parent directory"],
+    ["../workspace", "one step out, onto the durable volume"],
+    ["../../etc", "two steps out"],
+    ["sales/../../etc", "traversal after a plausible prefix"],
+    ["/etc", "an absolute path"],
+    ["a/b", "a separator"],
+    ["a\\b", "a backslash"],
+    ["profile.d", "a dot, which invites .. reasoning"],
+    [".hidden", "a leading dot"],
+    ["-lead", "a leading hyphen"],
+    ["", "empty"],
+    ["  ", "whitespace"],
+    ["sales support", "an inner space"],
+    ["sales\n", "a trailing newline"],
+    ["a".repeat(65), "longer than a uuid-shaped id"],
+  ])("refuses %s (%s)", (id) => {
+    expect(isPlainBotId(id)).toBe(false);
+  });
+});
+
+describe("profileDirectoryFor", () => {
+  test("puts an ordinary Bot in its own directory under the root", () => {
+    expect(profileDirectoryFor("/profiles", "sales")).toBe("/profiles/sales");
+  });
+
+  // The regression. Each of these resolved outside the profiles root, and `reset` runs
+  // rm -rf on whatever comes back, as root.
+  test.each([
+    ["../workspace", "/workspace"],
+    ["../etc", "/etc"],
+    ["../../above", "two levels up"],
+    ["sales/../../etc", "/etc"],
+  ])("refuses %s, which used to resolve to %s", (id) => {
+    expect(() => profileDirectoryFor("/profiles", id)).toThrow();
+  });
+
+  // Belt and braces: whatever the allow-list lets through must still land inside the root.
+  test("everything it accepts stays inside the root", () => {
+    for (const id of ["sales", "support-1", "risk_analyst", "a"]) {
+      const directory = profileDirectoryFor("/profiles", id);
+      expect(directory.startsWith("/profiles/")).toBe(true);
+    }
+  });
+});