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); + } + }); +});