Keep the deployment's secrets out of a Bot's shell

.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.
 #

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<string, string | undefined>,
+  workspaceDir: string,
+): Record<string, string> {
+  const named = (env.COMPUTER_SHELL_ENV ?? "")
+    .split(",")
+    .map((name) => name.trim())
+    .filter((name) => name.length > 0);
+
+  const passed: Record<string, string> = {};
+  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 = "";

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

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_<BOT_ID>`