From de63c7dcb9095f1b3d0433d4108ace3bb7de1857 Mon Sep 17 00:00:00 2001 From: hotragn Date: Thu, 20 Aug 2026 21:14:04 -0400 Subject: [PATCH] Keep the deployment's secrets out of a Bot's shell --- .env.example | 9 +++ agent-computer/src/shell.ts | 73 ++++++++++++++++++- agent-computer/tests/shell.test.ts | 113 +++++++++++++++++++++++++++++ docs/configuration.md | 1 + 4 files changed, 193 insertions(+), 3 deletions(-) create mode 100644 agent-computer/tests/shell.test.ts diff --git a/.env.example b/.env.example index cdaf0f1..92dfb3e 100644 --- a/.env.example +++ b/.env.example @@ -120,6 +120,15 @@ COMPUTER_TOKEN= # Local only. Lets a Bot browse this machine's own services; never set this in a deployment. AGENT_COMPUTER_ALLOW_PRIVATE_HOSTS=true # +# What a Bot's shell commands can read out of the environment, beyond what a command needs to run: +# PATH, the locale and terminal names, and the proxy variables are always passed. Nothing else is, +# because the computer process is handed the container's environment and in the one-container image +# that is the API's, down to the key that decrypts stored credentials. +# +# Comma separated, and read literally. Naming a secret here says to pass that secret, which is a +# decision an operator can make; taking everything by default was not. +# COMPUTER_SHELL_ENV=GITHUB_TOKEN,BUILD_CHANNEL +# # What a Bot may do on its computer, as one JSON object. Absent uses the built-in default, which # permits the acting tools and forbids nothing, and records every action either way. # diff --git a/agent-computer/src/shell.ts b/agent-computer/src/shell.ts index 40e487f..951ac83 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 sees the environment a command needs rather than the one the deployment runs + * on. * * 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 @@ -31,6 +32,72 @@ const MAX_TIMEOUT_MS = 600_000; */ const MAX_OUTPUT_BYTES = 64 * 1024; +/** + * What a command sees of the deployment it runs in. + * + * A child process inherits its parent's environment, and this parent is the computer. In the + * one-container image that process sits beside the API and `docker/s6/s6-rc.d/computer/run` hands it + * the container's environment deliberately, so what a command would inherit is everything the API + * was given: the key that decrypts stored credentials, the database URL, the model key. `env` is a + * command like any other, and the trail records that a command ran without recording what it + * returned, so reading all of them is one call that leaves an unremarkable row. + * + * AN ALLOW LIST RATHER THAN A DENY LIST. A deny list is the secrets that existed on the day it was + * written. The next variable somebody adds to a deployment is not on it, and a boundary that stops + * holding without saying so is the failure this repository has already taken two features back for. + * + * What stays is what a command needs to run at all, plus the proxy variables, because a deployment + * behind a proxy has an `apt-get` that reaches nothing without them. + * + * THIS IS A FLOOR, NOT THE BOUNDARY. `sudo` is passwordless in the image, and root in a shared + * container can read another process's environment whatever this function returns. What it removes + * is the one-word version. + */ +const ALWAYS_PASSED = [ + "PATH", + "LANG", + "LANGUAGE", + "LC_ALL", + "TERM", + "TZ", + "USER", + "LOGNAME", + "HOSTNAME", + "HTTP_PROXY", + "HTTPS_PROXY", + "NO_PROXY", + "http_proxy", + "https_proxy", + "no_proxy", +]; + +/** + * The environment for one command. + * + * `COMPUTER_SHELL_ENV` names anything else a deployment wants a command to see, comma separated. It + * is an opt-in and it is read literally: an operator who names a secret there has said to pass that + * secret, which is a different thing from a shell that takes everything by default. + */ +export function commandEnvironment( + env: Record, + workspaceDir: string, +): Record { + const named = (env.COMPUTER_SHELL_ENV ?? "") + .split(",") + .map((name) => name.trim()) + .filter((name) => name.length > 0); + + const passed: Record = {}; + for (const name of [...ALWAYS_PASSED, ...named]) { + const value = env[name]; + if (value !== undefined) passed[name] = value; + } + + // Last, so that neither list can point a command's home somewhere other than its workspace. + passed.HOME = workspaceDir; + return passed; +} + export type ShellResult = { command: string; exitCode: number; @@ -78,7 +145,7 @@ export function createShell(workspaceDir: string) { */ const child = spawn("/bin/bash", ["-lc", input.command], { cwd: workspaceDir, - env: { ...process.env, HOME: workspaceDir }, + env: commandEnvironment(process.env, 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..2aa1804 --- /dev/null +++ b/agent-computer/tests/shell.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, test } from "bun:test"; +import { commandEnvironment } from "../src/shell"; + +/** + * What a Bot's command can read out of the deployment. + * + * The case that matters is the first one. A command inherits whatever the computer process holds, + * and in the one-container image that is the API's environment: the key that decrypts every stored + * credential, the database URL, the model key. A test that only checked `PATH` survives would have + * been green while `env` returned all of them. + */ + +const DEPLOYMENT = { + PATH: "/usr/local/bin:/usr/bin:/bin", + HOME: "/home/pwuser", + LANG: "C.UTF-8", + KEY_ENCRYPTION_KEY: "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=", + DATABASE_URL: "postgres://openbot:hunter2@127.0.0.1:5432/openbot", + OPENAI_API_KEY: "sk-live-not-for-a-bot", + COMPUTER_TOKEN: "the-secret-between-the-api-and-this-process", + INTELLIGENCE_API_KEY: "cpk-live", + COPILOTKIT_LICENSE_TOKEN: "licence", + AGENT_TOOL_TOKEN: "per-agent-callback", + SUPERVISOR_TOKEN: "supervisor", +}; + +describe("the environment one command gets", () => { + test("the deployment's secrets are not in it", () => { + const environment = commandEnvironment(DEPLOYMENT, "/workspace"); + + for (const name of [ + "KEY_ENCRYPTION_KEY", + "DATABASE_URL", + "OPENAI_API_KEY", + "COMPUTER_TOKEN", + "INTELLIGENCE_API_KEY", + "COPILOTKIT_LICENSE_TOKEN", + "AGENT_TOOL_TOKEN", + "SUPERVISOR_TOKEN", + ]) { + expect(environment[name]).toBeUndefined(); + } + }); + + test("what a command needs to run at all is still there", () => { + const environment = commandEnvironment(DEPLOYMENT, "/workspace"); + + expect(environment.PATH).toBe("/usr/local/bin:/usr/bin:/bin"); + expect(environment.LANG).toBe("C.UTF-8"); + }); + + test("home is the workspace, whatever the deployment's home was", () => { + // The file tools and a command have to see one directory, and the inherited HOME is the + // container user's, not this Bot's. + const environment = commandEnvironment(DEPLOYMENT, "/workspace"); + + expect(environment.HOME).toBe("/workspace"); + }); + + test("a proxied deployment keeps its proxy variables", () => { + // Without these an `apt-get install` behind a corporate proxy hangs rather than failing, and + // installing a tool is most of why a Bot has a shell. + const environment = commandEnvironment( + { + ...DEPLOYMENT, + HTTPS_PROXY: "http://proxy.internal:8080", + no_proxy: "127.0.0.1,localhost", + }, + "/workspace", + ); + + expect(environment.HTTPS_PROXY).toBe("http://proxy.internal:8080"); + expect(environment.no_proxy).toBe("127.0.0.1,localhost"); + }); + + test("a deployment can name what else a command should see", () => { + const environment = commandEnvironment( + { + ...DEPLOYMENT, + GITHUB_TOKEN: "ghp-for-the-bot", + BUILD_CHANNEL: "nightly", + COMPUTER_SHELL_ENV: "GITHUB_TOKEN, BUILD_CHANNEL", + }, + "/workspace", + ); + + // Spaces around a name are an operator writing a list, not a different variable. + expect(environment.GITHUB_TOKEN).toBe("ghp-for-the-bot"); + expect(environment.BUILD_CHANNEL).toBe("nightly"); + // Naming two does not bring the rest with them. + expect(environment.KEY_ENCRYPTION_KEY).toBeUndefined(); + }); + + test("naming a variable that is not set does not invent an empty one", () => { + // `test -z "$X"` and `test -v X` are different questions, and a command that branches on the + // second should see what the deployment actually has. + const environment = commandEnvironment( + { ...DEPLOYMENT, COMPUTER_SHELL_ENV: "NEVER_SET" }, + "/workspace", + ); + + expect("NEVER_SET" in environment).toBe(false); + }); + + test("the pass-through list cannot move home out of the workspace", () => { + const environment = commandEnvironment( + { ...DEPLOYMENT, COMPUTER_SHELL_ENV: "HOME" }, + "/workspace", + ); + + expect(environment.HOME).toBe("/workspace"); + }); +}); diff --git a/docs/configuration.md b/docs/configuration.md index ad3cf63..91fe829 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -110,6 +110,7 @@ Google OAuth client id and secret must be configured together. If Google OAuth i - `WORKSPACE_DIR` - `PROFILES_DIR` - `COMPUTER_BOT_ID` +- `COMPUTER_SHELL_ENV` - `EGRESS_PROXY_DEFAULT` - `EGRESS_PROXY_`