fix(computer): stop shell inheriting deployment environment - #68
Merged
davidmckayv merged 11 commits intoAug 21, 2026
Merged
Conversation
jeonjw85
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
August 21, 2026 01:38
9 tasks
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.
jeonjw85
force-pushed
the
stop-shell-inheriting-deployment-env
branch
from
August 21, 2026 01:44
2039f6a to
5156bdb
Compare
jeonjw85
changed the title
Contributor
|
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.
|
Contributor
Author
|
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.
8 tasks
Contributor
|
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.
davidmckayv
added a commit
that referenced
this pull request
Aug 21, 2026
Two configurations that start today refuse to after this, and both were buried mid-paragraph in Added and Changed. They are four lines at the top of Unreleased now, saying what to set rather than what used to happen. Rebased onto #68, which took the deployment's environment away from a Bot's shell. Checked on the running computer rather than trusting the tests: `GOOGLE_OAUTH_CLIENT_SECRET`, which this branch introduces, is absent from a command's environment without anybody having added it to a list. That is the allowlist earning its shape.
davidmckayv
added a commit
that referenced
this pull request
Aug 21, 2026
* Sign in with Google, Microsoft or Okta, whichever a deployment has
One identity provider was a decision somebody else already made. A company
running this has Google or Entra or Okta and is not going to acquire another,
so any one of the three turns sign-in on, several turn on several, and the
sign-in screen draws a button per provider in a fixed order.
Google and Entra are named providers Better Auth knows the endpoints of. Okta
is not one place, so it goes through the generic OAuth plugin against its
issuer, and the plugin is only registered when Okta is configured. They converge
at the browser: one `signIn.social({ provider })` for all three, so the app does
not know which kind it is asking for and a deployment can gain one without a
rebuild.
The provider list moved from the build to `/api/capabilities`. It used to be
compiled into the bundle from the build machine's environment, which was
survivable until the container: one image, built once, knowing nothing about the
deployment that runs it, would have offered a sign-in screen that had never
heard of the provider the operator configured.
Nothing configured now means one administrator without a flag, so a fresh clone
reaches the product without registering an OAuth client first. The lock moved
from a flag to `NODE_ENV`: somewhere other people can reach, an unconfigured
deployment refuses to start and names what to configure, because a public URL
where every visitor is an administrator is silent and looks like it works.
`OPENBOT_SINGLE_USER=true` is how somebody says they meant it.
Two defects found by signing in for real rather than reading the code.
Better Auth 1.7 requires an `issuer` on every account and this schema, written
against 1.6, had no such column. The adapter rendered `where ( = $1 ...)` with
an empty column name and the callback failed with an internal error. Migration
0002 adds it as three statements rather than the one Drizzle generates, because
`ADD COLUMN ... NOT NULL` with no default fails outright on a table that already
has rows, and Google's rows are backfilled with Google's real issuer so they
still match at the next sign-in.
`server/package.json` also asked for `^1.6.27` while 1.7.1 was what resolved,
leaving three copies of the adapter installed. Pinned to what actually runs.
* Make the administrator list mean something after the first sign-in
Two ways a deployment could end up with nobody who can administer it, and no
way back from either.
`INITIAL_ADMIN_EMAILS` was optional. Configure sign-in without it and everybody
arrives as a plain user, nobody sees the admin screens, and nobody can promote
anyone, because the role is written from that list and no route anywhere changes
one. `.env.example` ships it commented out, so copying the example and adding a
provider was enough to do it. Sign-in now refuses to start without it.
The role was also written once, in the create hook. Adding yourself to the list
after you had already signed in did nothing at all: the row said `user`, for
ever. It is now reconciled on every sign-in, which also means an address taken
off the list loses `admin` next time it signs in. `user_roles` is a set and the
guard takes `admin` if any row says so, so reconciling deletes the rows that
should not be there rather than only inserting one, both inside a transaction:
between the two a request on another process would find no role at all and be
refused with a 403 that reads as a permissions bug.
Driven on the real path rather than reasoned about: the same account went
admin, then user with the address removed, then admin again with it restored.
The middle step is what the old hook could not do.
* Make the administrator list a floor, and put each provider's mark on its button
The list and an admin screen have to be able to disagree without one silently
undoing the other. So `INITIAL_ADMIN_EMAILS` is a floor: an address it names is
made an administrator at every sign-in and cannot be demoted, which is the way
back in when the last administrator demotes themselves by accident. Everybody
else is left exactly as they are, because their role is the admin screen's to
decide and a sign-in that rewrote it would make that screen lie the moment they
came back.
That is a change from an hour ago, when sign-in rewrote every role from the list
and would have reverted any promotion made in a screen that does not exist yet.
The buttons now carry each provider's own mark, drawn inline rather than
fetched: this is the one page somebody reaches before they have a session, so a
mark that arrives over the network is one that can be missing exactly when the
page has to look trustworthy, and it asks nothing of a third party from an
unauthenticated page.
Google's guidelines require the standard colour G at its own aspect ratio and
require their button be at least as prominent as any other sign-in option, so
all three are the same size and weight and none of them is the loud one. Okta's
is monochrome, which their guidelines allow: it is not a consumer button anybody
recognises by colour, it is whichever Okta the company uses, and it stays
legible in both themes without a second asset.
* Let an administrator decide who else is one
An environment variable was the only way to grant the administrator role, and
no route anywhere changed one. That is not how a company runs a deployment: the
people who need access arrive after the deployment does.
So a People screen. Everybody who has signed in, the providers they came
through, when they were last here, and two decisions per row.
Removing somebody is both halves or it is theatre. The deny list stops the next
sign-in and deleting their sessions stops the current one, because otherwise a
removed person keeps working until their cookie happens to expire, which can be
days. It is keyed on the email address rather than the user id: deleting the row
is not removal, since the next sign-in through the provider creates it again
with a fresh id and no memory of having been removed.
Three refusals, all enforced on the server and only mirrored in the browser.
Nobody may demote themselves or remove their own access, because either locks
them out of the screen that would undo it, and on a deployment with one
administrator that is the whole deployment. And somebody named in
INITIAL_ADMIN_EMAILS may be neither, because the floor promotes them again at
their next sign-in and the screen would be lying until then.
Every change writes a row. The table holds the current answer; the trail is the
only thing that can say who changed it and when.
Found by driving it: people who had never signed in sorted above people who just
had, because Postgres puts nulls first on a descending order. On a real
deployment that is the whole first screen given to people who have never used it.
* Take a company's own identity provider, by SAML or OIDC
The three configured providers cover a company that uses Google, Entra or Okta.
They do not cover a company that runs its own identity provider, which is most
of the ones that ask, and which cannot be configured up front because the
deployment is built before it knows whose IdP it will trust.
So they are registered while running. An administrator pastes the metadata their
identity team supplied and the provider is stored against an email domain.
Somebody signing in types their address, and the part after the @ decides which
provider they are handed to, so a company mid-merger can run two at once. No
password is asked for and none is checked here.
Registering, changing and removing one is administrator-only. Better Auth guards
those routes with `sessionMiddleware`, which asks only that somebody is signed
in, and that is the wrong bar: registering a provider for a domain means
anybody it vouches for can sign in, so a plain user reaching it could mint
themselves colleagues. The gate sits in front of the handler and is tested.
The sign-in screen grows the email box only when a provider is registered, and
the capability that says so is a boolean rather than a list: naming them would
tell anybody who loads the page which companies use this deployment.
Driven end to end. A registered SAML provider produces a real signed
SAMLRequest redirect for an address at its domain and a 404 for one that is not,
the same delete call answers 403 signed out and 200 as an administrator, and the
sign-in screen adds and drops the email box as the last provider comes and goes.
* Find an address for somebody arriving from Entra, whatever claim it is in
The sign-in flow really is the same for all three: authorization code with PKCE,
discovery, an ID token. Google and Entra run through the same function. The
claims inside that token are where they stop agreeing.
Entra does not always send `email`. Microsoft return it only when the profile
carries an email attribute, and a multi-tenant application may receive no
optional claims at all, because an external user's token is minted by their own
tenant and does not inherit this application's claim configuration. `common`,
the default tenant here, is multi-tenant. Better Auth maps `email` straight
through with no fallback, so on those deployments it arrives undefined.
That is worse here than in most products, because every authorization decision
OpenBot makes about a person is keyed on their address: INITIAL_ADMIN_EMAILS,
the role, the deny list and the People screen all read it. Somebody would sign
in successfully, match no administrator, and land as a plain user with nothing
on any screen explaining why.
So `upn` first, then `preferred_username`, and only if it looks like an address:
the OIDC spec explicitly does not promise that claim is one. If none of the
three is there, nothing is returned and Better Auth refuses the sign-in, which
is a better answer than quietly admitting somebody the deployment cannot
recognise. The reason is logged with the claims that did arrive.
Found by reading the provider Microsoft-side rather than by testing, since
there are no Entra credentials here yet.
* Generate the schema steps, and write only the data step
The issuer migration was one file I had edited by hand after Drizzle generated
it, because the generated `ADD COLUMN ... NOT NULL` fails outright on a table
that already has rows. Editing a generated file is the wrong fix: it leaves a
file that no longer matches what the generator produced.
It is three steps instead, and only the middle one is written:
0002 generated the column, nullable, and the two new tables
0003 custom the backfill
0004 generated the column made required
`drizzle-kit generate --custom` is Drizzle's own mechanism for this, and their
documentation names data seeding as the reason it exists. A generator diffs
schema against schema, so "the rows whose provider is Google get Google's
issuer" cannot come out of one: it is not in the schema.
The generatable alternative is a column default, and it is wrong rather than
merely inelegant. Every existing Google account would take the placeholder, stop
matching `https://accounts.google.com` at that person's next sign-in, and Better
Auth would create them a second account.
Driven both ways with `drizzle-kit migrate` itself rather than by hand: from
empty, and against a database already holding Google, credential and Microsoft
accounts, where the three rows come out with Google's real issuer and the
synthetic form for the rest.
Worth knowing for the check that landed in #64: `drizzle-kit check` reports
"Everything's fine" when a journal entry names a migration file that does not
exist, which is a state a rebase can produce. It cost an hour here. The drift
probe does not catch it either, since both look at schemas rather than at
whether the journal and the directory agree.
* Say what sign-in does, everywhere it is documented
The configuration reference still described Google as the only provider and
described `INITIAL_ADMIN_EMAILS` as optional, which is now a start-up failure.
It carries all three providers, what each needs, the callback URL to register,
and why the administrator list is required.
The architecture notes gain the parts a reader cannot infer from the code: that
one resolver answers both questions a run asks about a person, that the
configured list is a floor rather than a one-off, that registering an identity
provider is administrator-only where the upstream plugin asks only for a
session, and that removing somebody denies the address rather than deleting the
row, since deleting it is not removal.
Two lines in the README's feature list, because sign-in and deciding who gets in
are now things the product does rather than things it lacks.
The generated Drizzle snapshots are formatted, which is what the committed ones
already were: `drizzle-kit generate` writes them without a trailing newline and
the format check refuses that.
* Bring the docs up to what is actually merged
Audited every markdown file against everything that landed today, including the
work that was not mine.
`docs/coworkers.md` still told people to point `MANAGED_AGENT_AG_UI_URL` at
`4200`. #33 made `agent-langgraph` on `4201` the default precisely because the
proof-of-concept hand-writes the protocol and leaves the tool loop to whatever is
watching, so following that page produced the shape the change moved away from.
Three environment variables the server reads were in `.env.example` and nowhere
in the configuration reference: `AGENT_STALL_TIMEOUT_MS` from #19, which is the
only thing that notices a Bot's stream going silent; `AGENT_TOOL_TOKEN` from #34,
without which no framework Bot may call a granted tool back; and `APP_DIST_DIR`,
which the container sets so one process serves both halves.
Both documentation indexes had fallen behind their own directory and listed
neither `deployment.md` nor `releasing.md`.
`docs/development.md` gains the migration workflow the checks in #64 now enforce:
never hand-edit a generated migration, write a data step with `--custom`, and
what to do when `drizzle-kit migrate` hangs and exits non-zero with nothing
printed, which is the journal naming a file a rebase renamed. `drizzle-kit check`
calls that state fine, because it compares schemas rather than asking whether the
journal and the directory agree.
The README keeps its shape: what this is, how to run it, how to deploy it, and
where to read the rest.
* Tell the image check it meant to run without sign-in
The check boots the container with no identity provider, and the image sets
NODE_ENV=production, where that combination now refuses to start rather than
serve a deployment on which every visitor is an administrator. So the check has
to declare it, which is what the flag is for.
It was passing `OPENBOT_DEV_NO_AUTH=1`, which the code has never accepted:
both the old flag and the new one compare against the exact string "true". It
did nothing, and nothing noticed, because before this branch a deployment with
no provider still started and answered on an unauthenticated route. The refusal
turned a silent no-op into a visible failure, which is the check working.
Reproduced locally with the same command the job runs: answers on
/api/capabilities in four seconds, nothing respawning after fifteen, and the
`eventsource` import error that appeared in the failing log is absent, since it
was the crash loop rather than a fault of its own.
* Put the upgrade note where somebody upgrading will find it
Two configurations that start today refuse to after this, and both were buried
mid-paragraph in Added and Changed. They are four lines at the top of Unreleased
now, saying what to set rather than what used to happen.
Rebased onto #68, which took the deployment's environment away from a Bot's
shell. Checked on the running computer rather than trusting the tests:
`GOOGLE_OAUTH_CLIENT_SECRET`, which this branch introduces, is absent from a
command's environment without anybody having added it to a list. That is the
allowlist earning its shape.
This was referenced Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.