From 575707965f964b29f710fac7507a4d17e1eece21 Mon Sep 17 00:00:00 2001 From: David McKay Date: Thu, 20 Aug 2026 18:39:08 -0700 Subject: [PATCH] Stop handing a Bot's shell the deployment's secrets A command ran with the whole environment of the process that started it. That process needs COMPUTER_TOKEN to authenticate its own callers, and in the one-container image the same environment carries DATABASE_URL, KEY_ENCRYPTION_KEY, the licence and the model key, because there is one environment and every service reads it. So the Bot was handed the credential that exists to keep the Bot out. COMPUTER_TOKEN is the only thing in front of the control surface on 4100, and the shell runs inside the process that serves it, so one command could drive the browser over loopback with no policy decision and no audit row. The gateway was optional for the one actor it exists to constrain. The database URL and the encryption key together opened the credential vault and the trail that is supposed to record what happened. A command now gets HOME, PATH, DEBIAN_FRONTEND and the locale. An allow-list rather than a deny-list, because the name worth removing is the one nobody thought of, and a variable added next year is covered without anybody remembering this file. DEBIAN_FRONTEND is not incidental: apt-get otherwise waits for an answer nobody is there to give, and the command times out looking like a broken package rather than a prompt. The test asserts the whole set rather than listing names to exclude, and runs a real shell, because the question is what a process actually inherits and a mock would only confirm what we believed it inherits. --- CHANGELOG.md | 11 +++ agent-computer/src/shell.ts | 41 +++++++- .../tests/shell-environment.test.ts | 98 +++++++++++++++++++ 3 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 agent-computer/tests/shell-environment.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 4659b05..2f39d67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,17 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ### Fixed +- **A Bot's shell was handed the deployment's secrets.** A command ran with the whole environment of + the process that started it, and in the one-container image that environment carries + `COMPUTER_TOKEN`, `DATABASE_URL`, `KEY_ENCRYPTION_KEY`, the licence and the model key. The token is + the only thing in front of the computer's control surface, and the shell runs inside the process + that serves it, so a command could drive the browser over loopback with no policy decision and no + audit row: the gateway was optional for the one actor it exists to constrain. The database URL and + the encryption key together opened the credential vault and the trail meant to record what + happened. A command now receives `HOME`, `PATH`, `DEBIAN_FRONTEND` and the locale, and nothing + else. `apt-get` also stops waiting for a prompt nobody can answer. + + - **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/agent-computer/src/shell.ts b/agent-computer/src/shell.ts index 40e487f..a2cbb21 100644 --- a/agent-computer/src/shell.ts +++ b/agent-computer/src/shell.ts @@ -19,6 +19,43 @@ import { spawn } from "node:child_process"; * between Bots, a shell is shared too. */ +/** + * The environment a Bot's command is allowed to see. + * + * An allow-list rather than a deny-list, because the interesting names are the ones nobody thought + * of. This process is started with the container's whole environment on purpose: it needs + * `COMPUTER_TOKEN` to authenticate its own callers. In the one-container image that environment also + * carries `DATABASE_URL`, `KEY_ENCRYPTION_KEY`, the licence and the model key, because there is one + * environment and every service reads it. + * + * Handing that to a command a Bot wrote would undo the gateway. `COMPUTER_TOKEN` is the only thing in + * front of the control surface on port 4100, and this shell runs inside the process that serves it, + * so a Bot holding the token could drive the browser over loopback with no policy decision and no + * audit row. The rest is worse in a quieter way: the database URL and the encryption key together + * open the credential vault and the trail that is supposed to record what happened. + * + * What is left is what a command needs to run at all. Without `PATH` nothing resolves, and the tool + * description tells the model to install packages, which needs one. + */ +const INHERITED = ["PATH", "TERM", "LANG", "LC_ALL", "TZ"] as const; + +function commandEnvironment(workspaceDir: string): NodeJS.ProcessEnv { + const environment: NodeJS.ProcessEnv = { + // The workspace, so a relative path means the same thing here as it does to the file tools. + HOME: workspaceDir, + // Fallback for a container that sets no PATH of its own; an inherited one overwrites it below. + PATH: "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", + // `apt-get install` without this waits for an answer nobody is there to give, and the command + // times out looking like a broken package rather than a prompt. + DEBIAN_FRONTEND: "noninteractive", + }; + for (const name of INHERITED) { + const value = process.env[name]; + if (value !== undefined) environment[name] = value; + } + return environment; +} + /** Long enough for an install, short enough that a hung command is not a hung Bot. */ const DEFAULT_TIMEOUT_MS = 120_000; const MAX_TIMEOUT_MS = 600_000; @@ -78,7 +115,9 @@ export function createShell(workspaceDir: string) { */ const child = spawn("/bin/bash", ["-lc", input.command], { cwd: workspaceDir, - env: { ...process.env, HOME: workspaceDir }, + // Not `process.env`. See INHERITED above: this process holds the deployment's secrets and a + // command a Bot wrote is the last thing that should be able to read them. + env: commandEnvironment(workspaceDir), }); let stdout = ""; diff --git a/agent-computer/tests/shell-environment.test.ts b/agent-computer/tests/shell-environment.test.ts new file mode 100644 index 0000000..534e0fe --- /dev/null +++ b/agent-computer/tests/shell-environment.test.ts @@ -0,0 +1,98 @@ +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 } from "../src/shell"; + +/** + * What a Bot's command can read out of the process that runs it. + * + * This process is started with the container's whole environment, deliberately: it needs + * `COMPUTER_TOKEN` to authenticate its own callers. In the one-container image that environment also + * carries `DATABASE_URL`, `KEY_ENCRYPTION_KEY` and the licence, because there is one environment and + * every service reads it. + * + * A command a Bot wrote must not see any of that. `COMPUTER_TOKEN` is the sharpest case: it is the + * only thing standing in front of the control surface on port 4100, and this shell runs inside the + * process that serves it. A Bot holding that token could drive the browser over loopback, which skips + * the policy decision and the audit row that are the entire point of the gateway. + * + * The assertion is a whole-set one rather than a list of names to exclude, because the dangerous name + * is the one nobody thought of. A variable added to a deployment next year is covered by this without + * anybody remembering to add it here. + * + * Against a real shell, not a mock: the question is what a process actually inherits, and a mock + * would only answer what we believed it inherits. The marker below uses a name nothing else reads, + * because a test that sets `DATABASE_URL` on the real environment breaks every other test that + * reads one. + */ + +const ALLOWED = new Set([ + "HOME", + "PATH", + "DEBIAN_FRONTEND", + "TERM", + "LANG", + "LC_ALL", + "TZ", + // bash sets these itself for any shell it starts; they are not inherited from this process. + "PWD", + "SHLVL", + "_", + "OLDPWD", + "SHELL", +]); + +const MARKER = "OPENBOT_SHELL_ENV_TEST_MARKER"; +const MARKER_VALUE = "this-must-not-reach-a-bot-command"; + +let workspace: string; + +beforeEach(async () => { + workspace = await mkdtemp(join(tmpdir(), "openbot-shell-env-")); + process.env[MARKER] = MARKER_VALUE; +}); + +afterEach(async () => { + delete process.env[MARKER]; + await rm(workspace, { recursive: true, force: true }); +}); + +describe("a command a Bot wrote", () => { + test("sees only the variables it was explicitly given", async () => { + const result = await createShell(workspace).run({ + // One name per line, values dropped: the question is which variables exist at all. + command: "env | cut -d= -f1 | sort -u", + }); + + expect(result.exitCode).toBe(0); + const leaked = result.stdout + .split("\n") + .map((line) => line.trim()) + .filter((name) => name.length > 0 && !ALLOWED.has(name)); + + expect(leaked).toEqual([]); + }); + + test("cannot read a secret held by the process that runs it", async () => { + const result = await createShell(workspace).run({ + command: `echo "[$${MARKER}]"`, + }); + + expect(result.exitCode).toBe(0); + expect(result.stdout.trim()).toBe("[]"); + expect(result.stdout).not.toContain(MARKER_VALUE); + }); + + test("still has the environment a command needs to work", async () => { + // The point is an allow-list, not an empty environment. Without PATH nothing resolves, and the + // tool description tells the model to install packages, which needs one. + const result = await createShell(workspace).run({ + command: 'echo "$HOME"; command -v sh >/dev/null && echo "path works"', + }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain(workspace); + expect(result.stdout).toContain("path works"); + }); +});