Stop handing a Bot's shell the deployment's secrets

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

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 = "";

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