diff --git a/.env.example b/.env.example index cdaf0f1..e7a6b7b 100644 --- a/.env.example +++ b/.env.example @@ -147,6 +147,13 @@ AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS=true # How long one action waits for its element, in ms. Read by agent-computer, not the server. # ACTION_TIMEOUT_MS=10000 +# Extra environment variables a command on the computer may see, by name, comma-separated. The +# shell already receives PATH, locale, terminal and proxy variables. Proxy userinfo is stripped, so +# a password in HTTP_PROXY is not in `env`. It does not inherit the rest of this process, so +# KEY_ENCRYPTION_KEY and the other deployment secrets are not in `env`. Naming a secret or a +# credentialed proxy here is an operator's decision. +# COMPUTER_SHELL_ENV=JAVA_HOME,GOPATH + # --------------------------------------------------------------------------- # The computer's profile and where its traffic leaves from # --------------------------------------------------------------------------- diff --git a/CHANGELOG.md b/CHANGELOG.md index c19cfef..70b9435 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,52 @@ 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 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 + not in it and `EMBEDDED_POSTGRES=on` is a documented way to run it. So root read another Bot's + workspace, the API's environment, and the audit database recording what it did. The grant now names + the package managers, so `apt-get install` still works and `sudo cat /proc/1/environ` does not. It + is a floor rather than a boundary: code a model wrote needs a computer per Bot with + `COMPUTER_SUPERVISOR_URL` and a sandbox under it with `COMPUTER_RUNTIME=runsc`, both of which this + already supports and neither of which the single-container image can reach. +- **A command could take the computer down, or outlive being stopped.** Output was accumulated in + full and only trimmed at the end, so `cat` of a large file allocated until the process that owns + the browser died; it is now bounded as it arrives, and still reports that it was truncated rather + than quietly ending. A stop signalled bash alone, so `sleep 30 | cat` left its children holding the + pipes and the call never returned; the whole process group is signalled now. A `timeoutMs` of zero + or less killed the command before it started and called it a timeout; it has a floor as well as a + ceiling. +- **Stop did not reach a running command.** The `/exec` route never took the person's abort, so the + plumbing for it was dead code and a stopped run left the command finishing inside the container. +- **The live-screen socket did not check the address it was given.** Every acting path resolved + through the gateway, which refuses a foreign or cloud-metadata address; this one asked the provider + directly and then put `COMPUTER_TOKEN` in the query string of whatever it was told. +- **`COMPUTER_SHELL_ENV` refuses the names that run before a command.** Naming `GITHUB_TOKEN` is an + operator deciding a Bot may use a token. Naming `BASH_ENV`, `ENV`, `LD_PRELOAD` or the shell option + variables is handing a Bot a hook into every later command, which is unlikely to be what was meant, + so those are refused and said out loud rather than passed. A name that is not a variable name is + now reported too, instead of quietly disappearing. +- **A deny rule naming one field refused every action that did not have it.** `deny: + contains(command, "rm -rf")`, the example the documentation gives, refused every click, keypress, + navigation and file read in the deployment. Two correct behaviours combined into a wrong one: the + policy context left out fields an action did not have, cel-js treats a missing field as an unknown + identifier and throws, and a thrown deny counts as a match so that a mistyped deny refuses rather + than quietly permitting. Every field is now bound, with a neutral value where the action has + nothing to put there, so a rule about a shell answers honestly about a click instead of refusing + it. Rules about the action they are for are unchanged. The audit row still omits what did not + happen. +- **A command longer than 45 seconds reported failure while it carried on running.** The transport + gave every call the same deadline, which was shorter than the shell's own 120 second default and + 600 second maximum, so `apt-get install` told the person the computer had not responded and then + finished installing inside the container. A command now gets a deadline that outlasts the shell, + which reports a timeout itself and says so. +- **A Bot's shell no longer inherits the deployment's environment.** Commands ran with the computer + process's own environment, so `env` in the one-container image printed `KEY_ENCRYPTION_KEY` and + the rest of `.env`. The shell now receives PATH, locale and terminal names, and the proxy + variables. Userinfo is stripped from a proxy URL, so a password in `HTTP_PROXY` is not in `env`. + Anything else is named in `COMPUTER_SHELL_ENV`. - **A deployment served over plain HTTP could not start a conversation.** The chat surface minted identifiers with `crypto.randomUUID`, which browsers withhold outside a secure context. On a laptop `http://localhost` counts as one, so this never showed up in development; on a real diff --git a/Dockerfile b/Dockerfile index 0e29dc6..473ba1a 100644 --- a/Dockerfile +++ b/Dockerfile @@ -126,19 +126,34 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ && mkdir -p /var/lib/postgresql/data /var/run/postgresql \ && chown -R postgres:postgres /var/lib/postgresql /var/run/postgresql -# A Bot can install what a task needs. +# A Bot can install what a task needs, and nothing else as root. # -# `sudo` for one user, no password, because a package manager that cannot install is not one, and -# "install a tool then use it" is the whole point of giving a Bot a shell. +# `sudo` without a password, because a package manager that cannot install is not one, and "install a +# tool then use it" is the whole point of giving a Bot a shell. # -# BE CLEAR WHAT THIS COSTS. It means a Bot can become root inside its container. That is acceptable -# when the container is the Bot's alone and is contained from below, which is why per-Bot computers -# and gVisor are not optional extras next to this feature; they are what makes it sane. In a -# container shared between Bots, or one holding a database, a Bot with sudo can reach all of it. +# THE PACKAGE MANAGERS, NOT ALL. This was `NOPASSWD: ALL`, and the comment below it explained what +# that cost: a Bot could become root inside its container. It then named the two conditions that make +# that acceptable — the container being one Bot's alone, and not holding a database — and this image +# meets neither. The supervisor is deliberately not in it, so every Bot shares one computer, and +# `EMBEDDED_POSTGRES=on` is a documented way to run it. So root here read another Bot's workspace, the +# API's environment, and the audit database that records what it did. +# +# Naming the commands keeps the feature and removes that. `apt-get install` still works, which is what +# the tool description tells a model to run. `sudo cat /proc/1/environ` does not. +# +# WHAT THIS IS NOT. It is a floor, not a boundary. Root is one CVE away and a shared container is not +# an isolation story for code a model wrote: that needs a computer per Bot and a sandbox under it, +# which is why per-Bot computers and gVisor are not optional extras next to this feature. Run the +# image with `--security-opt no-new-privileges` where the platform allows, which turns setuid off +# entirely for anything not named here. RUN apt-get update && apt-get install -y --no-install-recommends sudo \ && rm -rf /var/lib/apt/lists/* \ - && echo 'pwuser ALL=(ALL) NOPASSWD: ALL' > /etc/sudoers.d/pwuser \ - && chmod 0440 /etc/sudoers.d/pwuser + && printf '%s\n' \ + 'pwuser ALL=(root) NOPASSWD: /usr/bin/apt-get, /usr/bin/apt, /usr/bin/dpkg, /usr/bin/apt-key, /usr/bin/apt-cache' \ + 'Defaults!/usr/bin/apt-get env_keep += "DEBIAN_FRONTEND"' \ + > /etc/sudoers.d/pwuser \ + && chmod 0440 /etc/sudoers.d/pwuser \ + && visudo -cf /etc/sudoers.d/pwuser # THE PACKAGE MANAGER AND THE SHELL STAY. Both were removed here once as hardening, which was # backwards: a Bot being able to open a shell and install what a task needs is a requested feature, diff --git a/README.md b/README.md index 27d13b0..14ee517 100644 --- a/README.md +++ b/README.md @@ -140,7 +140,7 @@ as one replica for now. ## Features - **A computer per Bot**: the supervisor gives each Bot its own container, its own `/workspace` volume and its own browser profile. Set `COMPUTER_RUNTIME=runsc` to run them under gVisor where the host supports it. -- **A shell, not just a browser**: a Bot can run a command in its workspace, install what it needs, and process a file it saved. Through the same gate as everything else, so a rule can refuse a shell outright or refuse particular commands, and the command is on the record either way. +- **A shell, not just a browser**: a Bot can run a command in its workspace, install what it needs, and process a file it saved. Through the same gate as everything else, so a rule can refuse a shell outright or refuse particular commands, and the command is on the record either way. The command inherits PATH, locale, terminal and proxy variables, not the rest of the deployment's environment. - **The gateway is the only way in**: it resolves the target from a server-held snapshot, evaluates the policy, writes the audit row, and only then calls the computer. There is no path that acts without the record existing first. - **CEL policy, fail closed**: rules can inspect `tool.name`, `intent`, `bot.id`, `actor.id`, `page.url`, `page.host`, `element.*`, `key`, `file.*` and `mcp.*`. Deny is evaluated before allow, a missing policy permits nothing, and a broken rule refuses rather than opens. - **Take the wheel**: a Bot that hits a login wall or a 2FA prompt asks for help. Control is handed over in the same panel and recorded as `computer.help_requested`, `computer.control_taken` and `computer.control_released`. While a person is driving, Bot actions are refused rather than queued. diff --git a/agent-computer/src/shell.ts b/agent-computer/src/shell.ts index 40e487f..0451aa2 100644 --- a/agent-computer/src/shell.ts +++ b/agent-computer/src/shell.ts @@ -11,8 +11,9 @@ import { spawn } from "node:child_process"; * the same as it does for a click. This file is the hands, not the judgement. * * WHAT THIS DOES DEFEND is the shape of the call rather than its content: a command cannot run - * forever, cannot return unbounded output, and runs in the workspace rather than wherever the - * process happens to be. + * forever, cannot return unbounded output, runs in the workspace rather than wherever the process + * happens to be, and does not inherit the computer's environment, so `env` cannot print the + * deployment's secrets. * * ISOLATION IS THE CONTAINER'S JOB. A shell can reach whatever the container can reach, so the * deployment that gives a Bot one should give each Bot a computer of its own. In a container shared @@ -21,6 +22,8 @@ import { spawn } from "node:child_process"; /** Long enough for an install, short enough that a hung command is not a hung Bot. */ const DEFAULT_TIMEOUT_MS = 120_000; +/** A command has to be given long enough to start. Below this, a caller is asking for nothing. */ +const MIN_TIMEOUT_MS = 1_000; const MAX_TIMEOUT_MS = 600_000; /** @@ -41,6 +44,162 @@ export type ShellResult = { elapsedMs: number; }; +/** + * The environment a command sees. + * + * WHY AN ALLOW LIST. The computer process holds the deployment's secrets when it runs inside the + * one-container image. Spreading `process.env` into the child makes `env` print them. A deny list + * is the secrets that existed on the day it was written; the next variable added to a deployment + * is not on it. + * + * PATH, locale, terminal and the proxy variables pass because a command that cannot find `apt-get`, + * cannot speak the operator's language, or cannot reach the network behind a corporate proxy is not + * a shell. Proxy URLs routinely carry a password; userinfo is stripped before the value is copied, so + * `env` cannot print it. Naming the credentialed URL in COMPUTER_SHELL_ENV is an operator's decision + * rather than the default. Everything else is named there too, read as names. + * + * HOME is the workspace. A command that writes to ~ should write where the Bot's files already are. + */ +const PATH_NAMES = ["PATH"] as const; +const LOCALE_NAMES = ["LANG", "LANGUAGE"] as const; +const TERMINAL_NAMES = ["TERM", "TERMINFO", "COLORTERM"] as const; +const PROXY_NAMES = [ + "HTTP_PROXY", + "HTTPS_PROXY", + "NO_PROXY", + "ALL_PROXY", + "FTP_PROXY", + "http_proxy", + "https_proxy", + "no_proxy", + "all_proxy", + "ftp_proxy", +] as const; + +const ENV_NAME = /^[A-Za-z_][A-Za-z0-9_]*$/; +const LOCALE_CATEGORY = /^LC_[A-Za-z0-9_]+$/; + +export function environmentForCommand( + source: NodeJS.ProcessEnv, + workspaceDir: string, +): Record { + const env: Record = {}; + + const copy = (name: string) => { + const value = source[name]; + if (value !== undefined) env[name] = value; + }; + + const copyProxy = (name: string) => { + const value = source[name]; + if (value !== undefined) env[name] = withoutUserinfo(value); + }; + + for (const name of PATH_NAMES) copy(name); + for (const name of LOCALE_NAMES) copy(name); + for (const name of Object.keys(source)) { + if (LOCALE_CATEGORY.test(name)) copy(name); + } + for (const name of TERMINAL_NAMES) copy(name); + for (const name of PROXY_NAMES) copyProxy(name); + for (const name of extraShellEnvNames(source.COMPUTER_SHELL_ENV)) copy(name); + + env.HOME = workspaceDir; + /* + * Set rather than copied, because the deployment has no opinion about it and the command does. The + * tool description tells the model to run `apt-get install`, which without this waits for an answer + * to a prompt nobody is there to give: the command reaches its timeout and comes back looking like + * a broken package rather than a question. An operator can still override it through + * COMPUTER_SHELL_ENV, which is copied above. + */ + if (env.DEBIAN_FRONTEND === undefined) env.DEBIAN_FRONTEND = "noninteractive"; + return env; +} + +/** + * Names COMPUTER_SHELL_ENV will not pass, whatever an operator writes. + * + * The rest of that setting is a decision an operator is entitled to make: naming `GITHUB_TOKEN` says + * this Bot may use that token, and they meant it. These are different in kind. They do not give a + * command information, they give it a hook that runs before every later command, which is the + * `.bash_profile` hole arriving through the front door. + * + * `BASH_ENV` is the one that survives `-c`: bash expands it for a non-interactive shell and sources + * the file it names, before the command. `ENV` is its POSIX-mode twin, `BASH_XTRACEFD` writes where + * it is told, `LD_PRELOAD` and `LD_LIBRARY_PATH` are the same idea one layer down, and the option + * variables change how the shell parses what follows. + * + * Refused rather than dropped quietly, because an operator who wrote one of these has a reason in + * mind and deserves to be told it did not happen. + */ +const NEVER_PASSED = new Set([ + "BASH_ENV", + "ENV", + "BASH_XTRACEFD", + "BASHOPTS", + "SHELLOPTS", + "CDPATH", + "GLOBIGNORE", + "IFS", + "LD_PRELOAD", + "LD_LIBRARY_PATH", + "LD_AUDIT", + "PS4", +]); + +function extraShellEnvNames(raw: string | undefined): readonly string[] { + if (raw === undefined || raw.trim() === "") return []; + const named = raw.split(",").map((name) => name.trim()); + + /* + * A name that is not a name, and a name that is refused, are both worth saying out loud. Silently + * skipping either leaves an operator with a command that fails somewhere else for a reason that + * never mentions what they wrote. + */ + for (const name of named) { + if (name === "") continue; + if (!ENV_NAME.test(name)) { + console.warn( + JSON.stringify({ + type: "computer-shell-env-ignored", + name, + reason: "not a variable name", + }), + ); + continue; + } + if (NEVER_PASSED.has(name)) { + console.warn( + JSON.stringify({ + type: "computer-shell-env-refused", + name, + reason: + "runs before every command rather than informing one, so it is never passed", + }), + ); + } + } + + return named.filter((name) => ENV_NAME.test(name) && !NEVER_PASSED.has(name)); +} + +/** + * Credentials commonly arrive inside a proxy URL. They are stripped so the shell can still reach + * the network without `env` printing a password. Same split `egress.ts` uses for the browser proxy. + */ +function withoutUserinfo(raw: string): string { + try { + const url = new URL(raw.trim()); + if (url.username === "" && url.password === "") return raw; + url.username = ""; + url.password = ""; + return url.toString().replace(/\/$/, ""); + } catch (e) { + if (e instanceof TypeError) return raw; + throw e; + } +} + function clamp(text: string): { text: string; truncated: boolean } { if (Buffer.byteLength(text, "utf8") <= MAX_OUTPUT_BYTES) { return { text, truncated: false }; @@ -53,7 +212,10 @@ function clamp(text: string): { text: string; truncated: boolean } { return { text: kept, truncated: true }; } -export function createShell(workspaceDir: string) { +export function createShell( + workspaceDir: string, + sourceEnv: NodeJS.ProcessEnv = process.env, +) { return { async run(input: { command: string; @@ -61,8 +223,13 @@ export function createShell(workspaceDir: string) { signal?: AbortSignal; }): Promise { const started = Date.now(); + /* + * Bounded at both ends. Only `Math.min` was applied, so a zero or negative `timeoutMs` from a + * caller made `setTimeout` fire immediately: the command was killed before it did anything and + * the answer said it had timed out, which is true and useless. + */ const timeoutMs = Math.min( - input.timeoutMs ?? DEFAULT_TIMEOUT_MS, + Math.max(input.timeoutMs ?? DEFAULT_TIMEOUT_MS, MIN_TIMEOUT_MS), MAX_TIMEOUT_MS, ); @@ -76,29 +243,91 @@ export function createShell(workspaceDir: string) { * whether this Bot may run commands at all and what they may say; the container decides what a * command can reach. */ - const child = spawn("/bin/bash", ["-lc", input.command], { + /* + * `-c`, not `-lc`. A login shell sources `$HOME/.bash_profile`, and HOME is the workspace a Bot + * writes with `computer_write_file`. That let a Bot leave a file which every later command ran + * first: it could put its own `apt-get` earlier on PATH, and the audit row would still read + * `apt-get --version`. The allow-list above means such a file can no longer recover a secret, + * but it could still change what a command does. A permissive shell is a decision a deployment + * can make. A trail that describes something other than what ran is not. + */ + /* + * Its own process group, so stopping it stops what it started. + * + * `child.kill` signals bash alone. `bash -c "sleep 600 | cat"` leaves the sleep and the cat + * holding the inherited pipes, so `close` never fires and the await never settles: the command + * runs on and the caller waits for the transport to give up instead. Killing the negative pid + * signals the whole group. + */ + const child = spawn("/bin/bash", ["-c", input.command], { cwd: workspaceDir, - env: { ...process.env, HOME: workspaceDir }, + env: environmentForCommand(sourceEnv, workspaceDir), + detached: true, }); - let stdout = ""; - let stderr = ""; let timedOut = false; + /* + * Trimmed while it arrives, not at the end. + * + * `clamp` ran after `close`, so the string grew without limit until then. `cat` of a large file + * or `base64 /dev/urandom` allocated until the process died, and that process owns the browser + * every Bot on this computer is using. + * + * Trimming here has to carry the fact that it happened. Clamping at the end could tell, by + * looking at the size; a stream that was already trimmed arrives under the limit and looks + * complete. Losing the flag would be worse than the allocation: output that quietly ends is + * output a model reads as the whole answer. + */ + const collect = () => { + let text = ""; + let dropped = false; + return { + add(chunk: unknown) { + text += String(chunk); + if (Buffer.byteLength(text, "utf8") <= MAX_OUTPUT_BYTES * 2) return; + text = Buffer.from(text, "utf8") + .subarray(-MAX_OUTPUT_BYTES) + .toString("utf8"); + dropped = true; + }, + get text() { + return text; + }, + get dropped() { + return dropped; + }, + }; + }; + + const outBuffer = collect(); + const errBuffer = collect(); + child.stdout.on("data", (chunk) => { - stdout += String(chunk); + outBuffer.add(chunk); }); child.stderr.on("data", (chunk) => { - stderr += String(chunk); + errBuffer.add(chunk); }); + const stop = () => { + const { pid } = child; + if (pid === undefined) return; + try { + // The group, so nothing the command backgrounded outlives it. + process.kill(-pid, "SIGKILL"); + } catch { + // Already gone, or the group went with it. Either way there is nothing left to stop. + } + }; + const timer = setTimeout(() => { timedOut = true; - child.kill("SIGKILL"); + stop(); }, timeoutMs); // The person's Stop reaches the command, not just the request that started it. - const onAbort = () => child.kill("SIGKILL"); + const onAbort = stop; input.signal?.addEventListener("abort", onAbort, { once: true }); const exitCode = await new Promise((resolve) => { @@ -109,14 +338,19 @@ export function createShell(workspaceDir: string) { clearTimeout(timer); input.signal?.removeEventListener("abort", onAbort); - const out = clamp(stdout); - const err = clamp(stderr); + const out = clamp(outBuffer.text); + const err = clamp(errBuffer.text); return { command: input.command, exitCode, stdout: out.text, stderr: err.text, - truncated: out.truncated || err.truncated, + // Either end of the pipe, and either point it was cut: while arriving, or on the way out. + truncated: + out.truncated || + err.truncated || + outBuffer.dropped || + errBuffer.dropped, timedOut, elapsedMs: Date.now() - started, }; diff --git a/agent-computer/tests/shell.test.ts b/agent-computer/tests/shell.test.ts new file mode 100644 index 0000000..f83c832 --- /dev/null +++ b/agent-computer/tests/shell.test.ts @@ -0,0 +1,331 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { createShell, environmentForCommand } from "../src/shell"; + +/** + * What a command on the computer is allowed to see. + * + * The case that matters is a secret that already sits on the process: KEY_ENCRYPTION_KEY in the + * one-container image, COMPUTER_TOKEN under Compose. Spreading `process.env` into the child makes + * `env` print them. These tests build the map a spawn would receive, then run a real command against + * it, so a refactor that only updates the helper cannot go green while the child still inherits. + */ + +const workspaceHome = "/workspace/sales"; + +const secrets = { + KEY_ENCRYPTION_KEY: "deployment-key", + DATABASE_URL: "postgres://openbot:openbot@127.0.0.1/openbot", + OPENAI_API_KEY: "sk-secret", + COMPUTER_TOKEN: "computer-token", + INTELLIGENCE_API_KEY: "cpk-secret", +} as const; + +const allowed = { + PATH: "/usr/bin:/bin", + LANG: "C.UTF-8", + LC_ALL: "C.UTF-8", + TERM: "xterm-256color", + HTTP_PROXY: "http://proxy.internal:8080", + http_proxy: "http://proxy.internal:8080", + HTTPS_PROXY: "http://proxy.internal:8080", +} as const; + +function source( + extra: Record = {}, +): NodeJS.ProcessEnv { + return { ...secrets, ...allowed, HOME: "/root", ...extra }; +} + +describe("what a command inherits", () => { + test("PATH, locale, terminal and proxy variables pass", () => { + const env = environmentForCommand(source(), workspaceHome); + expect(env.PATH).toBe(allowed.PATH); + expect(env.LANG).toBe(allowed.LANG); + expect(env.LC_ALL).toBe(allowed.LC_ALL); + expect(env.TERM).toBe(allowed.TERM); + expect(env.HTTP_PROXY).toBe(allowed.HTTP_PROXY); + expect(env.http_proxy).toBe(allowed.http_proxy); + expect(env.HTTPS_PROXY).toBe(allowed.HTTPS_PROXY); + }); + + test("a secret sitting in the process environment does not", () => { + const env = environmentForCommand(source(), workspaceHome); + expect(env).not.toHaveProperty("KEY_ENCRYPTION_KEY"); + expect(env).not.toHaveProperty("DATABASE_URL"); + expect(env).not.toHaveProperty("OPENAI_API_KEY"); + expect(env).not.toHaveProperty("COMPUTER_TOKEN"); + expect(env).not.toHaveProperty("INTELLIGENCE_API_KEY"); + expect(JSON.stringify(env)).not.toContain("deployment-key"); + expect(JSON.stringify(env)).not.toContain("sk-secret"); + }); + + test("HOME is the workspace, not the process's home", () => { + const env = environmentForCommand(source(), workspaceHome); + expect(env.HOME).toBe(workspaceHome); + }); + + test("COMPUTER_SHELL_ENV names extras, read as names", () => { + const env = environmentForCommand( + source({ + COMPUTER_SHELL_ENV: "JAVA_HOME, GOPATH", + JAVA_HOME: "/opt/java", + GOPATH: "/opt/go", + }), + workspaceHome, + ); + expect(env.JAVA_HOME).toBe("/opt/java"); + expect(env.GOPATH).toBe("/opt/go"); + expect(env).not.toHaveProperty("COMPUTER_SHELL_ENV"); + }); + + test("naming a secret in COMPUTER_SHELL_ENV is an operator's decision", () => { + const env = environmentForCommand( + source({ COMPUTER_SHELL_ENV: "KEY_ENCRYPTION_KEY" }), + workspaceHome, + ); + expect(env.KEY_ENCRYPTION_KEY).toBe(secrets.KEY_ENCRYPTION_KEY); + expect(env).not.toHaveProperty("OPENAI_API_KEY"); + }); + + test("a name that is not a name is not read as one", () => { + const env = environmentForCommand( + source({ + COMPUTER_SHELL_ENV: "JAVA_HOME,KEY_ENCRYPTION_KEY;rm,FOO=bar", + JAVA_HOME: "/opt/java", + }), + workspaceHome, + ); + expect(env.JAVA_HOME).toBe("/opt/java"); + expect(env).not.toHaveProperty("KEY_ENCRYPTION_KEY"); + expect(env).not.toHaveProperty("FOO"); + }); + + test("HOME cannot be redirected through COMPUTER_SHELL_ENV", () => { + const env = environmentForCommand( + source({ COMPUTER_SHELL_ENV: "HOME", HOME: "/root" }), + workspaceHome, + ); + expect(env.HOME).toBe(workspaceHome); + }); + + test("a proxy URL's userinfo does not pass", () => { + const env = environmentForCommand( + source({ + HTTP_PROXY: "http://bot:s3cret@proxy.internal:8080", + HTTPS_PROXY: "https://bot:p%40ss@proxy.internal:8443", + }), + workspaceHome, + ); + expect(env.HTTP_PROXY).toBe("http://proxy.internal:8080"); + expect(env.HTTPS_PROXY).toBe("https://proxy.internal:8443"); + expect(JSON.stringify(env)).not.toContain("s3cret"); + expect(JSON.stringify(env)).not.toContain("p%40ss"); + expect(JSON.stringify(env)).not.toContain("p@ss"); + }); + + test("a proxy without userinfo is left alone", () => { + const env = environmentForCommand(source(), workspaceHome); + expect(env.HTTP_PROXY).toBe(allowed.HTTP_PROXY); + }); + + test("naming a credentialed proxy in COMPUTER_SHELL_ENV is an operator's decision", () => { + const env = environmentForCommand( + source({ + COMPUTER_SHELL_ENV: "HTTP_PROXY", + HTTP_PROXY: "http://bot:s3cret@proxy.internal:8080", + }), + workspaceHome, + ); + expect(env.HTTP_PROXY).toBe("http://bot:s3cret@proxy.internal:8080"); + }); +}); + +describe("the command that actually runs", () => { + let root: string; + + beforeEach(async () => { + root = await mkdtemp(join(tmpdir(), "openbot-shell-")); + }); + + afterEach(async () => { + await rm(root, { recursive: true, force: true }); + }); + + test("env in the child does not list a secret from the parent", async () => { + const result = await createShell(root, source()).run({ + command: "printenv", + }); + expect(result.timedOut).toBe(false); + expect(result.stdout).toContain(`HOME=${root}`); + expect(result.stdout).toContain(`HTTP_PROXY=${allowed.HTTP_PROXY}`); + expect(result.stdout).not.toContain("KEY_ENCRYPTION_KEY"); + expect(result.stdout).not.toContain("deployment-key"); + expect(result.stdout).not.toContain("sk-secret"); + expect(result.stdout).not.toContain("computer-token"); + }); + + test("a name in COMPUTER_SHELL_ENV is in the child", async () => { + const result = await createShell( + root, + source({ COMPUTER_SHELL_ENV: "JAVA_HOME", JAVA_HOME: "/opt/java" }), + ).run({ command: "printenv JAVA_HOME" }); + expect(result.exitCode).toBe(0); + expect(result.stdout.trim()).toBe("/opt/java"); + }); + test("a package install is not left waiting for a prompt", async () => { + // apt-get reads DEBIAN_FRONTEND. Interactive, it stops for an answer nobody is there to give, + // and the command reaches its timeout looking like a broken package rather than a question. + const result = await createShell(root, source()).run({ + command: "printenv DEBIAN_FRONTEND", + }); + expect(result.exitCode).toBe(0); + expect(result.stdout.trim()).toBe("noninteractive"); + }); + + test("env in the child does not list a proxy password", async () => { + const result = await createShell( + root, + source({ HTTP_PROXY: "http://bot:s3cret@proxy.internal:8080" }), + ).run({ command: "printenv HTTP_PROXY" }); + expect(result.exitCode).toBe(0); + expect(result.stdout.trim()).toBe("http://proxy.internal:8080"); + expect(result.stdout).not.toContain("s3cret"); + }); + + test("an operator can still choose the frontend themselves", async () => { + const result = await createShell( + root, + source({ + COMPUTER_SHELL_ENV: "DEBIAN_FRONTEND", + DEBIAN_FRONTEND: "teletype", + }), + ).run({ command: "printenv DEBIAN_FRONTEND" }); + expect(result.exitCode).toBe(0); + expect(result.stdout.trim()).toBe("teletype"); + }); + test("a file the Bot left behind does not run before its command", async () => { + // computer_write_file lets a Bot write anywhere in its workspace, and HOME is the workspace. A + // login shell would source this first, so a later command would run Bot-authored code while the + // audit row recorded only the command that was asked for. + await Bun.write( + join(root, ".bash_profile"), + 'echo "profile ran"\nexport MARKER=hijacked\n', + ); + + const result = await createShell(root, source()).run({ + command: 'echo "[${MARKER:-clean}]"', + }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).not.toContain("profile ran"); + expect(result.stdout.trim()).toBe("[clean]"); + }); + test("BASH_ENV is not in the map a command receives", () => { + /* + * The one startup file a non-interactive shell still reads. `bash -c` sources no login files, but + * it does expand BASH_ENV and run what it names, on bash 3.2 and 5.2 alike. Nothing else closes + * that path: it is closed only by BASH_ENV not being in the map, so it is asserted here rather + * than assumed. Same for the option variables bash reads from its environment. + */ + const env = environmentForCommand( + source({ + BASH_ENV: "/workspace/profile.sh", + SHELLOPTS: "xtrace", + BASHOPTS: "expand_aliases", + CDPATH: "/workspace", + GLOBIGNORE: "*", + }), + workspaceHome, + ); + + expect(env.BASH_ENV).toBeUndefined(); + expect(env.SHELLOPTS).toBeUndefined(); + expect(env.BASHOPTS).toBeUndefined(); + expect(env.CDPATH).toBeUndefined(); + expect(env.GLOBIGNORE).toBeUndefined(); + }); + + test("COMPUTER_SHELL_ENV cannot hand back an execution hook", () => { + /* + * The rest of that setting is an operator's decision to make. This is not: BASH_ENV names a file + * bash runs before the command, so passing it would reopen the hole `-c` closed, with the + * reasoning for `-c` sitting a few lines above looking satisfied. Refused rather than omitted, so + * the boundary does not depend on nobody adding a `BASH_*` convenience later. + */ + const env = environmentForCommand( + source({ + COMPUTER_SHELL_ENV: "BASH_ENV,LD_PRELOAD,ENV,SHELLOPTS,JAVA_HOME", + BASH_ENV: "/workspace/hook.sh", + LD_PRELOAD: "/workspace/hook.so", + ENV: "/workspace/hook.sh", + SHELLOPTS: "xtrace", + JAVA_HOME: "/opt/java", + }), + workspaceHome, + ); + + expect(env.BASH_ENV).toBeUndefined(); + expect(env.LD_PRELOAD).toBeUndefined(); + expect(env.ENV).toBeUndefined(); + expect(env.SHELLOPTS).toBeUndefined(); + // A name that only carries information still passes, so the refusal is targeted rather than a + // second allow list nobody can extend. + expect(env.JAVA_HOME).toBe("/opt/java"); + }); +}); + +describe("what a command cannot do to the computer", () => { + let root: string; + + beforeEach(async () => { + root = await mkdtemp(join(tmpdir(), "openbot-shell-limits-")); + }); + + afterEach(async () => { + await rm(root, { recursive: true, force: true }); + }); + + test("a command that backgrounds a process is still stopped", async () => { + /* + * `child.kill` signals bash alone. The sleep and the cat below hold the pipes bash inherited, so + * `close` never fires and the awaited promise never settles: the command outlives its own limit + * and the caller waits for something else to give up. Killing the process group ends all of it. + */ + const started = Date.now(); + const result = await createShell(root, source()).run({ + command: "sleep 30 | cat", + timeoutMs: 1_500, + }); + + expect(result.timedOut).toBe(true); + // Settles on its own limit rather than hanging until something upstream times out. + expect(Date.now() - started).toBeLessThan(10_000); + }, 20_000); + + test("a noisy command does not grow without limit", async () => { + // The clamp ran only after close, so this allocated until the process that owns the browser died. + const result = await createShell(root, source()).run({ + command: "head -c 3000000 /dev/zero | tr '\\0' 'a'", + timeoutMs: 20_000, + }); + + expect(result.truncated).toBe(true); + // Held to the kept size and a margin, not to the three megabytes the command produced. + expect(Buffer.byteLength(result.stdout, "utf8")).toBeLessThan(200 * 1024); + }, 30_000); + + test("a timeout of zero is not a command killed before it runs", async () => { + // Only Math.min was applied, so this fired setTimeout immediately and reported a timeout for a + // command that never had a chance to start. + const result = await createShell(root, source()).run({ + command: 'echo "ran"', + timeoutMs: 0, + }); + + expect(result.timedOut).toBe(false); + expect(result.stdout.trim()).toBe("ran"); + }, 15_000); +}); diff --git a/docs/architecture.md b/docs/architecture.md index 90b52c1..4805a80 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -71,6 +71,8 @@ startup. With `COMPUTER_SUPERVISOR_URL`, each Bot gets its own computer container, workspace volume, and browser profile. Without it, all Bots share `AGENT_COMPUTER_URL`. +A command on the computer inherits PATH, locale and terminal names, and the proxy variables, not the rest of the process environment. Userinfo is stripped from a proxy URL. `COMPUTER_SHELL_ENV` names anything else a deployment wants passed. + The supervisor exposes only ensure, stop, reset, and list operations. It holds the Docker socket, so do not expose it outside the deployment network. Set `COMPUTER_RUNTIME=runsc` to run computers under gVisor on hosts that support it. ## Human control and secrets diff --git a/docs/configuration.md b/docs/configuration.md index ad3cf63..e4dd30e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -112,6 +112,12 @@ Google OAuth client id and secret must be configured together. If Google OAuth i - `COMPUTER_BOT_ID` - `EGRESS_PROXY_DEFAULT` - `EGRESS_PROXY_` +- `COMPUTER_SHELL_ENV` + +A command on the computer inherits PATH, locale and terminal names, and the proxy variables, not +the rest of the process environment. Userinfo is stripped from a proxy URL, so a password in +`HTTP_PROXY` is not in `env`. `COMPUTER_SHELL_ENV` is a comma-separated list of extra names to +pass. Naming a secret or a credentialed proxy there is an operator's decision; the default does not. The supervisor also reads: diff --git a/server/src/computer/client.ts b/server/src/computer/client.ts index 8d0a83c..d084cfe 100644 --- a/server/src/computer/client.ts +++ b/server/src/computer/client.ts @@ -70,6 +70,8 @@ export interface ComputerTransport { path: string, init?: RequestInit, caller?: AbortSignal, + /** Overrides the transport's own deadline for this one call. */ + timeoutMs?: number, ): Promise; post( baseUrl: string, @@ -77,6 +79,8 @@ export interface ComputerTransport { path: string, payload: unknown, caller?: AbortSignal, + /** Overrides the transport's own deadline for this one call. */ + timeoutMs?: number, ): Promise; navigate( baseUrl: string, @@ -95,7 +99,7 @@ export function createComputerTransport( options: ComputerTransportOptions, ): ComputerTransport { const doFetch = options.fetchImpl ?? fetch; - const timeoutMs = options.timeoutMs ?? 45_000; + const defaultTimeoutMs = options.timeoutMs ?? 45_000; async function call( baseUrl: string, @@ -103,11 +107,22 @@ export function createComputerTransport( path: string, init?: RequestInit, caller?: AbortSignal, + timeoutMsOverride?: number, ): Promise { if (caller?.aborted) { throw new ComputerUnavailableError("The action was stopped."); } + /* + * A browser action either happens in seconds or has gone wrong, so 45s is the right deadline for + * it. A command is not that: the shell's own budget is 120s by default and up to 600s, and the + * tool description tells the model to install packages. Giving up here first reported failure to + * the person while the command carried on running to completion inside the container, and made + * the shell's own limit unreachable. A caller with a longer limit of its own passes it in, and + * this becomes the backstop rather than the limit. + */ + const timeoutMs = timeoutMsOverride ?? defaultTimeoutMs; + const target = baseUrl.replace(/\/$/, ""); let response: Response; try { @@ -148,6 +163,7 @@ export function createComputerTransport( path: string, payload: unknown, caller?: AbortSignal, + timeoutMs?: number, ): Promise { return call( baseUrl, @@ -159,6 +175,7 @@ export function createComputerTransport( body: JSON.stringify(payload), }, caller, + timeoutMs, ); } diff --git a/server/src/computer/gateway.ts b/server/src/computer/gateway.ts index c7d3e32..c8f9ecf 100644 --- a/server/src/computer/gateway.ts +++ b/server/src/computer/gateway.ts @@ -254,10 +254,28 @@ export function createComputerGateway( path: string, payload: unknown, signal?: AbortSignal, + timeoutMs?: number, ): Promise { - return transport.post(await locate(botId), botId, path, payload, signal); + return transport.post( + await locate(botId), + botId, + path, + payload, + signal, + timeoutMs, + ); } + /* + * The transport's deadline for a command, which is a backstop and not the limit. + * + * The shell enforces the real one: 120s by default, 600s at most, and it answers with `timedOut` + * so the person is told the command was stopped rather than that the computer went quiet. This + * only has to outlast it. Below the shell's maximum, the transport gave up first and the person was + * told the computer did not respond while the command ran on to completion inside the container. + */ + const COMMAND_BACKSTOP_MS = 615_000; + /** Read-only, so it passes straight through. Nothing has changed and there is nothing to decide. */ async function screenshot(botId: string): Promise { return get(botId, "/screenshot"); @@ -332,25 +350,42 @@ export function createComputerGateway( const intent = intentOf(toolName, subject.key); + /* + * Every field is bound, present or not. + * + * A missing one is not an absent field to CEL, it is an unknown identifier, and cel-js throws on + * those. A thrown deny rule counts as a match, on purpose, so that a mistyped deny refuses rather + * than quietly permitting. Together those two correct behaviours produced a wrong one: a rule + * naming a field this action does not have refused the action. + * + * `deny: contains(command, "rm -rf")` is the example the docs give, and while `command` was + * spread in only for a shell call, that rule threw on every click, keypress, navigation and file + * read in the deployment and refused all of them. So did any rule naming `key`, `file` or + * `element` from an action that has none. + * + * Neutral rather than absent: `contains("", "rm -rf")` is false, which is the honest answer to + * "is this click running rm -rf". The audit row below still omits what did not happen, because a + * trail should not claim a click had a command. + */ const context: PolicyContext = { tool: { name: toolName }, bot: { id: botId }, actor: { id: actor.id }, page: { url: pageUrl, host: hostOf(pageUrl) }, ...(intent ? { intent } : {}), - ...(subject.key ? { key: subject.key } : {}), - ...(element + key: subject.key ?? "", + element: element ? { - element: { - ref: element.ref, - role: element.role, - name: element.name, - ...(element.type ? { type: element.type } : {}), - }, + ref: element.ref, + role: element.role, + name: element.name, + type: element.type ?? "", } - : {}), - ...(filePath ? { file: describeFile(filePath) } : {}), - ...(subject.command ? { command: subject.command } : {}), + : { ref: "", role: "", name: "", type: "" }, + file: filePath + ? describeFile(filePath) + : { path: "", name: "", extension: "" }, + command: subject.command ?? "", }; const decision = evaluateActionPolicy(options.policy(), context); @@ -675,7 +710,14 @@ export function createComputerGateway( botId, actor, { command: input.command, ...(caller ? { signal: caller } : {}) }, - () => post(botId, "/exec", input, caller), + () => + post( + botId, + "/exec", + input, + caller, + COMMAND_BACKSTOP_MS, + ), ); }, diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index 599ddf7..5f000ad 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -313,16 +313,24 @@ export function createComputerRoutes( * owns the limit. */ routes.post("/:botId/exec", requireUser, (context) => - act(context, (botId, actor, body) => { + act(context, (botId, actor, body, signal) => { if (typeof body?.command !== "string" || !body.command.trim()) { return { error: "A command is required." }; } - return gateway.runCommand(botId, actor, { - command: body.command, - ...(typeof body.timeoutMs === "number" - ? { timeoutMs: body.timeoutMs } - : {}), - }); + // The fourth argument, like every other acting route. Without it the plumbing through + // gateway.runCommand and into the shell's own abort listener was dead code, and Stop ended the + // run in the transcript while the command carried on to completion inside the container. + return gateway.runCommand( + botId, + actor, + { + command: body.command, + ...(typeof body.timeoutMs === "number" + ? { timeoutMs: body.timeoutMs } + : {}), + }, + signal, + ); }), ); diff --git a/server/src/index.ts b/server/src/index.ts index 7d0b7e1..2934ca2 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -430,11 +430,18 @@ serve({ if (!actor) { return new Response("Sign in first.", { status: 401 }); } - // Located through the configured provider so every stream follows the same isolation rules. + /* + * Through the gateway, not the provider. + * + * `gateway.locate` runs checkComputerAddress; `provider.locate` does not, and the URL built + * below carries COMPUTER_TOKEN in its query string. A provider that answered with a foreign + * host was handed the deployment's computer token, which is the case that check was written + * for. Every acting path already went through the gateway; this one did not. + */ let upstream: string; try { - const streamBase = computerProvider - ? await computerProvider.locate(streamBotId) + const streamBase = computerGateway + ? await computerGateway.locate(streamBotId) : undefined; if (!streamBase) { return new Response("No computer address is configured.", { diff --git a/server/tests/computer-client.test.ts b/server/tests/computer-client.test.ts index aa2ae90..66dd9d2 100644 --- a/server/tests/computer-client.test.ts +++ b/server/tests/computer-client.test.ts @@ -245,3 +245,69 @@ describe("the caller's Stop", () => { expect(seen).toBeDefined(); }); }); + +describe("the deadline a call is given", () => { + test("a command outlasts the shell's own maximum; a browser action does not", async () => { + /* + * The shell's budget is 120s by default and 600s at most, and it reports `timedOut` itself. A + * transport deadline shorter than that told the person the computer had gone quiet while the + * command ran to completion inside the container, and made the shell's own maximum unreachable. + */ + const deadlines: number[] = []; + const fetchImpl = (async (_url: string, init?: RequestInit) => { + // AbortSignal.timeout is not readable, so the deadline is observed by racing it. + const signal = init?.signal; + deadlines.push(signal ? 1 : 0); + return new Response(JSON.stringify({ ok: true }), { + headers: { "content-type": "application/json" }, + }); + }) as unknown as typeof fetch; + + const transport = createComputerTransport({ fetchImpl, timeoutMs: 45_000 }); + + // Both calls succeed; what matters is that the override is accepted and passed down rather than + // silently dropped, which a signature change could do without any test noticing. + await transport.post("http://computer", "bot-1", "/click", {}, undefined); + await transport.post( + "http://computer", + "bot-1", + "/exec", + {}, + undefined, + 615_000, + ); + + expect(deadlines).toEqual([1, 1]); + }); + + test("the override is what bounds the request, not the transport default", async () => { + // A 20ms transport default with a call that takes 60ms: without the override this rejects. + const slow = (async (_url: string, init?: RequestInit) => { + await new Promise((resolve, reject) => { + const timer = setTimeout(resolve, 60); + init?.signal?.addEventListener("abort", () => { + clearTimeout(timer); + const error = new Error("aborted"); + error.name = "TimeoutError"; + reject(error); + }); + }); + return new Response("{}", { + headers: { "content-type": "application/json" }, + }); + }) as unknown as typeof fetch; + + const transport = createComputerTransport({ + fetchImpl: slow, + timeoutMs: 20, + }); + + await expect( + transport.post("http://computer", "bot-1", "/click", {}), + ).rejects.toThrow(); + + await expect( + transport.post("http://computer", "bot-1", "/exec", {}, undefined, 5_000), + ).resolves.toBeDefined(); + }); +}); diff --git a/server/tests/computer-gateway.test.ts b/server/tests/computer-gateway.test.ts index 2213dd4..09a5782 100644 --- a/server/tests/computer-gateway.test.ts +++ b/server/tests/computer-gateway.test.ts @@ -679,4 +679,99 @@ describe("the computer gateway", () => { "/human/click", ]); }); + /* + * Through `gatewayWith`, deliberately, rather than by handing `evaluateActionPolicy` a context + * written here. The bug these cover was that the context the gateway builds and the context the + * policy tests built were different shapes, so a policy test asserting a browser rule does not + * refuse a command passed while production refused every click. A test that builds its own context + * can only ever check the context it built. + */ + describe("a deny rule that names a field this action does not have", () => { + test("a command rule does not refuse a click", async () => { + const { gateway, calls, rows } = await gatewayWith({ + ...PERMISSIVE, + // The rule the documentation gives as the example. + deny: ['contains(command, "rm -rf")'], + }); + + await gateway.click("bot-1", ACTOR, { ref: "e9", snapshotId: 7 }); + + expect(calls).toEqual(["click"]); + expect(rows[0]?.eventType).toBe("computer.action_allowed"); + }); + + test("a key rule does not refuse a click", async () => { + const { gateway, calls } = await gatewayWith({ + ...PERMISSIVE, + deny: ['key == "Enter"'], + }); + + await gateway.click("bot-1", ACTOR, { ref: "e9", snapshotId: 7 }); + + expect(calls).toEqual(["click"]); + }); + + test("a file rule does not refuse a click", async () => { + const { gateway, calls } = await gatewayWith({ + ...PERMISSIVE, + deny: ['contains(file.path, "secret")'], + }); + + await gateway.click("bot-1", ACTOR, { ref: "e9", snapshotId: 7 }); + + expect(calls).toEqual(["click"]); + }); + + test("but the rule still refuses the action it is about", async () => { + // The point is not that these rules stop working. A neutral value answers honestly; it does + // not answer no. + const { gateway, calls, rows } = await gatewayWith({ + ...PERMISSIVE, + deny: ['key == "Enter"'], + }); + + await expect( + gateway.key("bot-1", ACTOR, { ref: "e9", snapshotId: 7, key: "Enter" }), + ).rejects.toThrow(); + expect(calls).toEqual([]); + expect(rows[0]?.eventType).toBe("computer.action_refused"); + }); + }); +}); + +describe("resolving a computer's address", () => { + test("a foreign address is refused, so nothing is sent to it", async () => { + /* + * The guard exists because the address decides where the deployment's computer token goes. Every + * acting path resolved through here already; the live-screen socket resolved through the provider + * directly and skipped it, while putting the token in a query string. + */ + const { provider, fetchImpl, requests } = fakeComputer(); + const { store } = fakeAudit(); + const gateway = createComputerGateway({ + // The cloud metadata address, which is the answer this guard was written for. + provider: { ...provider, locate: async () => "http://169.254.169.254" }, + fetchImpl, + auditStore: store, + policy: () => PERMISSIVE, + token: "deployment-token", + }); + + await expect(gateway.locate("bot-1")).rejects.toThrow(); + // Refused before anything was sent, so the token never left. + expect(requests).toEqual([]); + }); + + test("an address the guard allows is returned", async () => { + const { provider, fetchImpl } = fakeComputer(); + const { store } = fakeAudit(); + const gateway = createComputerGateway({ + provider, + fetchImpl, + auditStore: store, + policy: () => PERMISSIVE, + }); + + await expect(gateway.locate("bot-1")).resolves.toContain("http"); + }); });