fix(computer): stop shell inheriting deployment environment - #68
Conversation
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.
2039f6a to
5156bdb
Compare
jeonjw85
changed the title
|
Reviewed this closely. The environment fix itself is correct and complete, and the allow-list is the right shape — the reasoning in the docstring about a deny-list being "the secrets that existed on the day it was written" is exactly right. I checked the surface rather than just the diff: I also pushed one commit here — Two things I'd want resolved. 1. The proxy variables can carry credentials — this one is introduced here
Copying the proxy is still right; a command that cannot reach the network is not a shell. The question is whether to strip userinfo from the URL before passing it, or to document that a credentialed proxy is visible to a Bot and should be granted deliberately through 2.
|
|
Addressed the proxy credentials. Allow-listed Naming the credentialed URL in Leaving |
`-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.
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.
|
I filed #66 and had a branch doing the same scrub; I have closed it, because this is the better change and it got here first. Two things in it I did not have: stripping userinfo out of the proxy URLs, and The One suggestion, offered as something to make the allow list hold rather than a problem with it.
A test asserting Worth considering whether |
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.
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.
3 participants
What this changes
computer_run_commandspawned with the computer process's own environment:What that inherits depends on how the deployment is run. Under Compose,
agent-computeris handedCOMPUTER_TOKEN. Under the one-container image the docs lead with, the computer starts viawith-contenv, so it holds the container's environment, and that container is run--env-file .env.envis then a one-word command that returnsKEY_ENCRYPTION_KEY,DATABASE_URL,OPENAI_API_KEY,INTELLIGENCE_API_KEY,COPILOTKIT_LICENSE_TOKEN,COMPUTER_TOKEN,SUPERVISOR_TOKEN,AGENT_TOOL_TOKEN. The trail records the command text and deliberately not its output, so the row this leaves reads like any other command.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.
The shell now receives an allow list: PATH, locale and terminal names, and the proxy variables. 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.HOMEis still the workspace. Anything else is named inCOMPUTER_SHELL_ENV, comma-separated, read as names — soJAVA_HOME,GOPATHpasses those two, andKEY_ENCRYPTION_KEY;rmis not a name and is not read as one. Naming a secret there is an operator's decision rather than the default.The larger question — shell off unless a supervisor is configured, or refusing
EMBEDDED_POSTGRESand a shell at boot — is left alone. Scrubbing the environment is a floor, not the container boundary; root in a shared container can still read another process's environment. That is #66's second half.Closes #66
Where it runs
Boundary and audit
envin the workspace no longer prints it.Changelog
COMPUTER_SHELL_ENVnames extras.Proof
Nine tests in
agent-computer/tests/shell.test.ts.The map a spawn would receive:
LANG/LC_ALL,TERM, and the proxy variables passKEY_ENCRYPTION_KEY,DATABASE_URL,OPENAI_API_KEY,COMPUTER_TOKEN,INTELLIGENCE_API_KEYdo not, and their values are not present as strings eitherHOMEis the workspace, including whenCOMPUTER_SHELL_ENVnamesHOMECOMPUTER_SHELL_ENV=JAVA_HOME, GOPATHcopies those two and not itselfKEY_ENCRYPTION_KEYinCOMPUTER_SHELL_ENVcopies it and still notOPENAI_API_KEYKEY_ENCRYPTION_KEY;rmandFOO=barare not names and are not copiedThe command that actually runs, against a real
/bin/bash -lc:printenvin the child does not listKEY_ENCRYPTION_KEYor its valueprintenv JAVA_HOMEreturns the value named inCOMPUTER_SHELL_ENVbash -lcis a login shell and rewritesPATHfrom the profile; the unit test still locks that PATH is in the map we handspawn. The spawn tests lock the leak, which is the thing a helper-only test would miss ifcreateShellwent back to spreadingprocess.env.biome formatandbiome lintclean onshell.tsandshell.test.ts. 9 pass, 0 fail.