From 967a1507d68999d90073e97cbe6decd95f7cb5aa Mon Sep 17 00:00:00 2001 From: Jeongwan Jeon Date: Fri, 21 Aug 2026 10:38:06 +0900 Subject: [PATCH 01/11] fix(computer): stop shell inheriting deployment environment --- agent-computer/src/shell.ts | 78 ++++++++++++++- agent-computer/tests/shell.test.ts | 147 +++++++++++++++++++++++++++++ 2 files changed, 221 insertions(+), 4 deletions(-) create mode 100644 agent-computer/tests/shell.test.ts diff --git a/agent-computer/src/shell.ts b/agent-computer/src/shell.ts index 40e487f..8430b8d 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 @@ -41,6 +42,72 @@ 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. Everything else is named in COMPUTER_SHELL_ENV, read as names, so passing a secret is an + * operator's decision rather than the default. + * + * 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; + }; + + 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) copy(name); + for (const name of extraShellEnvNames(source.COMPUTER_SHELL_ENV)) copy(name); + + env.HOME = workspaceDir; + return env; +} + +function extraShellEnvNames(raw: string | undefined): readonly string[] { + if (raw === undefined || raw.trim() === "") return []; + return raw + .split(",") + .map((name) => name.trim()) + .filter((name) => ENV_NAME.test(name)); +} + function clamp(text: string): { text: string; truncated: boolean } { if (Buffer.byteLength(text, "utf8") <= MAX_OUTPUT_BYTES) { return { text, truncated: false }; @@ -53,7 +120,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; @@ -78,7 +148,7 @@ export function createShell(workspaceDir: string) { */ const child = spawn("/bin/bash", ["-lc", input.command], { cwd: workspaceDir, - env: { ...process.env, HOME: workspaceDir }, + env: environmentForCommand(sourceEnv, workspaceDir), }); let stdout = ""; diff --git a/agent-computer/tests/shell.test.ts b/agent-computer/tests/shell.test.ts new file mode 100644 index 0000000..8a888ae --- /dev/null +++ b/agent-computer/tests/shell.test.ts @@ -0,0 +1,147 @@ +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); + }); +}); + +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"); + }); +}); From 04933ca9234b8cc35547dfba36d9efad2ace12b7 Mon Sep 17 00:00:00 2001 From: Jeongwan Jeon Date: Fri, 21 Aug 2026 10:38:09 +0900 Subject: [PATCH 02/11] docs: add COMPUTER_SHELL_ENV to computer configuration --- .env.example | 6 ++++++ docs/configuration.md | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/.env.example b/.env.example index cdaf0f1..0ad91a4 100644 --- a/.env.example +++ b/.env.example @@ -147,6 +147,12 @@ 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. 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 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/docs/configuration.md b/docs/configuration.md index ad3cf63..351e37f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -112,6 +112,11 @@ 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. `COMPUTER_SHELL_ENV` is a comma-separated list of extra names +to pass. Naming a secret there is an operator's decision; the default does not. The supervisor also reads: From 7dcddbec83cc3d07fad17648968b50451175994a Mon Sep 17 00:00:00 2001 From: Jeongwan Jeon Date: Fri, 21 Aug 2026 10:38:12 +0900 Subject: [PATCH 03/11] docs: note command environment allowlist --- docs/architecture.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/architecture.md b/docs/architecture.md index 90b52c1..e0c384d 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. `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 From 8cffcdbb0eddfeeaa07581591b6561ba2a9cdc02 Mon Sep 17 00:00:00 2001 From: Jeongwan Jeon Date: Fri, 21 Aug 2026 10:38:28 +0900 Subject: [PATCH 04/11] docs: record shell environment change --- CHANGELOG.md | 4 ++++ README.md | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c19cfef..1c51028 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,10 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ### Fixed +- **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. 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/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. From 5156bdb9144c010e8d891a0b200acc2371f01876 Mon Sep 17 00:00:00 2001 From: David McKay Date: Thu, 20 Aug 2026 18:42:53 -0700 Subject: [PATCH 05/11] Do not leave a package install waiting for a prompt apt-get reads DEBIAN_FRONTEND, and the shell tool description tells the model to run it. Interactive, the install stops for an answer nobody is there to give: the command reaches its timeout and comes back looking like a broken package rather than a question. Set rather than copied, because the deployment has no opinion about it and the command does. An operator who wants a different frontend can still name it in COMPUTER_SHELL_ENV, which is copied first. --- agent-computer/src/shell.ts | 8 ++++++++ agent-computer/tests/shell.test.ts | 21 +++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/agent-computer/src/shell.ts b/agent-computer/src/shell.ts index 8430b8d..6e840f3 100644 --- a/agent-computer/src/shell.ts +++ b/agent-computer/src/shell.ts @@ -97,6 +97,14 @@ export function environmentForCommand( 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; } diff --git a/agent-computer/tests/shell.test.ts b/agent-computer/tests/shell.test.ts index 8a888ae..deb99d9 100644 --- a/agent-computer/tests/shell.test.ts +++ b/agent-computer/tests/shell.test.ts @@ -144,4 +144,25 @@ describe("the command that actually runs", () => { 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("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"); + }); }); From 2e883a62f2fd913e3255b74cf02593357371d8e4 Mon Sep 17 00:00:00 2001 From: Jeongwan Jeon Date: Fri, 21 Aug 2026 10:50:56 +0900 Subject: [PATCH 06/11] fix(computer): strip credentials from inherited proxy URLs --- agent-computer/src/shell.ts | 29 ++++++++++++++++++--- agent-computer/tests/shell.test.ts | 41 ++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/agent-computer/src/shell.ts b/agent-computer/src/shell.ts index 6e840f3..c061ff4 100644 --- a/agent-computer/src/shell.ts +++ b/agent-computer/src/shell.ts @@ -52,8 +52,9 @@ export type ShellResult = { * * 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. Everything else is named in COMPUTER_SHELL_ENV, read as names, so passing a secret is an - * operator's decision rather than the default. + * 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. */ @@ -87,13 +88,18 @@ export function environmentForCommand( 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) copy(name); + for (const name of PROXY_NAMES) copyProxy(name); for (const name of extraShellEnvNames(source.COMPUTER_SHELL_ENV)) copy(name); env.HOME = workspaceDir; @@ -116,6 +122,23 @@ function extraShellEnvNames(raw: string | undefined): readonly string[] { .filter((name) => ENV_NAME.test(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 }; diff --git a/agent-computer/tests/shell.test.ts b/agent-computer/tests/shell.test.ts index deb99d9..e37d561 100644 --- a/agent-computer/tests/shell.test.ts +++ b/agent-computer/tests/shell.test.ts @@ -110,6 +110,37 @@ describe("what a command inherits", () => { ); 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", () => { @@ -154,6 +185,16 @@ describe("the command that actually runs", () => { 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, From f825848ece17399eecfac874e62baf7d243d0c8a Mon Sep 17 00:00:00 2001 From: Jeongwan Jeon Date: Fri, 21 Aug 2026 10:51:03 +0900 Subject: [PATCH 07/11] docs: note proxy userinfo is stripped from the shell environment --- .env.example | 7 ++++--- CHANGELOG.md | 3 ++- docs/architecture.md | 2 +- docs/configuration.md | 5 +++-- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/.env.example b/.env.example index 0ad91a4..e7a6b7b 100644 --- a/.env.example +++ b/.env.example @@ -148,9 +148,10 @@ AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS=true # 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. 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 here is an operator's decision. +# 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 # --------------------------------------------------------------------------- diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c51028..52e707d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +30,8 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. - **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. Anything else is named in `COMPUTER_SHELL_ENV`. + 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/docs/architecture.md b/docs/architecture.md index e0c384d..4805a80 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -71,7 +71,7 @@ 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. `COMPUTER_SHELL_ENV` names anything else a deployment wants passed. +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. diff --git a/docs/configuration.md b/docs/configuration.md index 351e37f..e4dd30e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -115,8 +115,9 @@ Google OAuth client id and secret must be configured together. If Google OAuth i - `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. `COMPUTER_SHELL_ENV` is a comma-separated list of extra names -to pass. Naming a secret there is an operator's decision; the default does 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: From eb804bef04bb5ef289597c312df7e3a3e569db05 Mon Sep 17 00:00:00 2001 From: David McKay Date: Thu, 20 Aug 2026 18:55:08 -0700 Subject: [PATCH 08/11] Stop a file the Bot left behind running before its command `-lc` made this a login shell, so it sourced $HOME/.bash_profile, and HOME is the workspace a Bot writes with computer_write_file. A Bot could leave a file that every later command ran first: put its own apt-get earlier on PATH, and the audit row still read `apt-get --version`. Proven against this branch before the change: the trail said `apt-get --version` and the output was "[.bash_profile ran] [hijacked apt-get]". The allow-list already means such a file cannot recover a secret. It could still change what a command does, and a trail that describes something other than what ran is worse than a shell that is too permissive, because nobody can tell. `-c` reads no startup files but still expands BASH_ENV and runs what it names, on bash 3.2 and 5.2 alike. Nothing but the allow-list closes that, so the test asserts BASH_ENV is absent from the map rather than assuming it, along with the option variables bash reads from its environment. --- agent-computer/src/shell.ts | 10 +++++++- agent-computer/tests/shell.test.ts | 41 ++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/agent-computer/src/shell.ts b/agent-computer/src/shell.ts index c061ff4..cda11d7 100644 --- a/agent-computer/src/shell.ts +++ b/agent-computer/src/shell.ts @@ -177,7 +177,15 @@ export function createShell( * 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. + */ + const child = spawn("/bin/bash", ["-c", input.command], { cwd: workspaceDir, env: environmentForCommand(sourceEnv, workspaceDir), }); diff --git a/agent-computer/tests/shell.test.ts b/agent-computer/tests/shell.test.ts index e37d561..4fd671d 100644 --- a/agent-computer/tests/shell.test.ts +++ b/agent-computer/tests/shell.test.ts @@ -206,4 +206,45 @@ describe("the command that actually runs", () => { 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(); + }); }); From fa7fe3ab33237330065268db7f157df8802ba502 Mon Sep 17 00:00:00 2001 From: David McKay Date: Thu, 20 Aug 2026 18:58:32 -0700 Subject: [PATCH 09/11] Bind every policy field, and let a command outlast a browser action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things stood between the shell and a release. A deny rule naming a field the action does not have refused the action. The context left out what an action had nothing to put in, cel-js throws on an unknown identifier rather than treating it as absent, and a thrown deny counts as a match so a mistyped deny refuses rather than quietly permitting. Each of those is right on its own. Together, `deny: contains(command, "rm -rf")` — the example in the docs — refused every click, keypress, navigation and file read in the deployment. Proven before the change: that rule throws on a click context and the click is refused; bound to a neutral value it evaluates false. Every field is now bound. The audit row still omits what did not happen, because a trail should not claim a click had a command. The tests for this go through the gateway rather than handing the policy a context written in the test. That is why this survived: a policy test asserting a browser rule does not refuse a command passed, while production refused every click, because the two contexts were different shapes. And the transport gave every call one deadline, 45s, shorter than the shell's own 120s default and 600s maximum. The tool description tells the model to install a package, so the person was told the computer had not responded while apt-get ran to completion inside the container, and the shell's own limit was unreachable. A command now carries a deadline that outlasts the shell, which reports the timeout itself. --- CHANGELOG.md | 14 ++++++ server/src/computer/client.ts | 19 +++++++- server/src/computer/gateway.ts | 68 ++++++++++++++++++++++----- server/tests/computer-client.test.ts | 66 ++++++++++++++++++++++++++ server/tests/computer-gateway.test.ts | 58 +++++++++++++++++++++++ 5 files changed, 211 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52e707d..6e2bbd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,20 @@ 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 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 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/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..e4e40e0 100644 --- a/server/tests/computer-gateway.test.ts +++ b/server/tests/computer-gateway.test.ts @@ -679,4 +679,62 @@ 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"); + }); + }); }); From e1a8daab7fdf6623cb6b7fe839e5a5752c9f3851 Mon Sep 17 00:00:00 2001 From: David McKay Date: Thu, 20 Aug 2026 19:51:49 -0700 Subject: [PATCH 10/11] Close the rest of what the shell opened Six things, all of them reachable from the feature as documented. Output was accumulated whole and trimmed only after close, so a command that prints a few megabytes allocated until the process that owns the browser died. Bounded as it arrives now, and the trim carries the fact that it happened: a stream already cut arrives under the limit and would otherwise look complete, which is worse than the allocation because a model reads it as the whole answer. A stop signalled bash and nothing bash started. `sleep 30 | cat` left the children holding the inherited pipes, so close never fired and the call waited for something upstream to give up. The process group is signalled now. A timeoutMs of zero passed Math.min and fired setTimeout immediately, killing the command before it ran and reporting a timeout. It has a floor. /exec never took the person's abort, so the plumbing through runCommand into the shell's own listener was dead code and Stop left the command finishing. The live-screen socket asked the provider for an address instead of the gateway, skipping the check the gateway does, and then put COMPUTER_TOKEN in the query string of whatever it was told. Every acting path already went through the gateway; this one did not. And COMPUTER_SHELL_ENV now refuses BASH_ENV, ENV, LD_PRELOAD and the option variables. That setting is meant for an operator deciding a Bot may use a token. These do not inform a command, they run before every later one, which is the .bash_profile hole arriving by the front door with the reasoning for -c sitting a few lines above looking satisfied. Refused loudly, and a name that is not a variable name is reported now rather than silently dropped. --- CHANGELOG.md | 17 +++ agent-computer/src/shell.ts | 153 +++++++++++++++++++++++--- agent-computer/tests/shell.test.ts | 81 ++++++++++++++ server/src/computer/routes.ts | 22 ++-- server/src/index.ts | 13 ++- server/tests/computer-gateway.test.ts | 37 +++++++ 6 files changed, 299 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e2bbd9..417666f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,23 @@ 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 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 diff --git a/agent-computer/src/shell.ts b/agent-computer/src/shell.ts index cda11d7..0451aa2 100644 --- a/agent-computer/src/shell.ts +++ b/agent-computer/src/shell.ts @@ -22,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; /** @@ -114,12 +116,71 @@ export function environmentForCommand( 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 []; - return raw - .split(",") - .map((name) => name.trim()) - .filter((name) => ENV_NAME.test(name)); + 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)); } /** @@ -162,8 +223,13 @@ export function createShell( 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, ); @@ -185,29 +251,83 @@ export function createShell( * 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: 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) => { @@ -218,14 +338,19 @@ export function createShell( 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 index 4fd671d..f83c832 100644 --- a/agent-computer/tests/shell.test.ts +++ b/agent-computer/tests/shell.test.ts @@ -247,4 +247,85 @@ describe("the command that actually runs", () => { 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/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-gateway.test.ts b/server/tests/computer-gateway.test.ts index e4e40e0..09a5782 100644 --- a/server/tests/computer-gateway.test.ts +++ b/server/tests/computer-gateway.test.ts @@ -738,3 +738,40 @@ describe("the computer gateway", () => { }); }); }); + +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"); + }); +}); From 870f3254542ccf5bfee474c6384828df852d3140 Mon Sep 17 00:00:00 2001 From: David McKay Date: Thu, 20 Aug 2026 20:01:51 -0700 Subject: [PATCH 11/11] Let a Bot install a package without letting it become root sudo was NOPASSWD: ALL. The comment above it was already clear about the cost, and clear about what made it acceptable: a container that is one Bot's alone and does not hold a database. This image is 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. Root there reads 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 the rest. apt-get, apt, dpkg, apt-key and apt-cache, which is what the tool description tells a model to run. Verified in the built image: sudo apt-get works, sudo cat, sudo sh and sudo -i are refused. The sudoers file is syntax-checked at build with visudo -cf, because a malformed drop-in disables sudo entirely rather than failing loudly. This 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. The answers already exist here: COMPUTER_SUPERVISOR_URL for a computer per Bot, COMPUTER_RUNTIME=runsc to put gVisor under it. What is missing is that the single-container image cannot reach either, which is worth saying rather than implying the narrowed grant settles it. --- CHANGELOG.md | 9 +++++++++ Dockerfile | 33 ++++++++++++++++++++++++--------- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 417666f..70b9435 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,15 @@ 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 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,