From f66fdaaf85da66bca3ff0c373e2a501a8d4b0eeb Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Wed, 19 Aug 2026 22:23:10 -0500 Subject: [PATCH] Keep a Bot id from naming a directory it should not A Bot id arrives as a request header, and the API server forwards whatever segment a caller put in the URL. `profiles.directoryFor` was `join(root, botId)`, which normalizes `..` away, so `../workspace` resolved outside the profiles root and `reset` deleted whatever was there with `rm -rf`, as root. Reached from the product, not only from a leaked computer token: every acting route under `/api/computers/:botId` requires a signed-in user and nothing more, and the server attaches its own token when it forwards the id. So `POST /api/computers/..%2Fworkspace/computers/reset` from an ordinary account deleted the durable workspace volume. A bare `..` is normalized away by the router; the encoded form is not. The rules are the ones `supervisor/src/names.ts` already applies to container and volume names, for the same reason and in the same words: letters, digits, hyphen and underscore, starting with a letter or digit. No separators, so an id cannot escape into another path segment; no dots, which invite `..` reasoning. The check sits in its own file rather than in `index.ts`, which imports Playwright at module scope, so it can be tested without a browser, the same split `authorisation.ts` already makes. Refused at the computer's own request boundary as well as where the path is built. The header is this process's input, so it holds the line itself rather than trusting the caller to have checked. `/health` stays exempt, as it is from the token, because it names no Bot and an orchestrator's probe should not fail on a header it never meant to send. `COMPUTER_BOT_ID` is validated at boot rather than per request. It is the id every unheadered call falls back to, so a value these rules refuse would answer 400 to everything and read as a broken computer. --- agent-computer/src/bot-id.ts | 50 ++++++++++++++++++++++ agent-computer/src/index.ts | 24 ++++++++++- agent-computer/src/profiles.ts | 6 ++- agent-computer/tests/bot-id.test.ts | 64 +++++++++++++++++++++++++++++ 4 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 agent-computer/src/bot-id.ts create mode 100644 agent-computer/tests/bot-id.test.ts diff --git a/agent-computer/src/bot-id.ts b/agent-computer/src/bot-id.ts new file mode 100644 index 0000000..b3e3a2d --- /dev/null +++ b/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}`; +} diff --git a/agent-computer/src/index.ts b/agent-computer/src/index.ts index f267759..98a6c5f 100644 --- a/agent-computer/src/index.ts +++ b/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, @@ -196,7 +197,17 @@ const shell = createShell(process.env.WORKSPACE_DIR ?? "/workspace"); * 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 { return profiles.page(botId); @@ -465,6 +476,14 @@ serve({ // 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") { @@ -475,6 +494,9 @@ serve({ * 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); diff --git a/agent-computer/src/profiles.ts b/agent-computer/src/profiles.ts index d73c86f..481c94f 100644 --- a/agent-computer/src/profiles.ts +++ b/agent-computer/src/profiles.ts @@ -36,6 +36,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 { chooseEvictions, chooseIdle } from "./browser-eviction"; import { egressFor, egressLabel } from "./egress"; @@ -189,7 +190,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>(); - 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); /** * Close one Bot's browser and forget it. diff --git a/agent-computer/tests/bot-id.test.ts b/agent-computer/tests/bot-id.test.ts new file mode 100644 index 0000000..4eeb615 --- /dev/null +++ b/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); + } + }); +});