Keep a Bot id from naming a directory it should not

[beardthelion]

Closes #29.

profiles.directoryFor was join(root, botId), which normalizes .. away, so a Bot id of ../workspace resolved outside the profiles root and reset deleted whatever was there with rm -rf, as root. The id comes from the URL, and every acting route under /api/computers/:botId requires a signed-in user and nothing more, with the server attaching its own computer token when it forwards the id. POST /api/computers/..%2Fworkspace/computers/reset from an ordinary account was enough.

What it does

Holds a Bot id to the rules supervisor/src/names.ts already applies to container and volume names, for the same reason and in the same words: letters, digits, hyphen and underscore, starting with a letter or digit. No separators, so an id cannot escape into another path segment; no dots, which invite .. reasoning.

Enforced in two places. Where the path is built, so there is one checked way to name a profile directory, and at the computer's own request boundary, because the header is this process's input and it should not depend on the caller having checked. /health stays exempt, as it already is from the token, since it names no Bot and an orchestrator's probe should not fail on a header it never meant to send.

The check lives in its own file rather than in index.ts, which imports Playwright at module scope, so it can be tested without a browser. That is the split authorisation.ts already makes, and for the same stated reason.

COMPUTER_BOT_ID is validated at boot rather than per request: it is the id every unheadered call falls back to, so a value these rules refuse would answer 400 to everything and read as a broken computer. names.ts validates its namespace the same way.

Verification

27 cases in agent-computer/tests/bot-id.test.ts, failing before the change and passing after: the traversal forms, and the ordinary ids that have to keep working (sales, support-1, risk_analyst, shared, a uuid-shaped id, a single character).

The behaviour itself was exercised against the real createProfiles on a sandboxed tree, before and after. Before, reset("../workspace"), reset("../etc"), reset("../../above") and reset("sales/../../etc") each deleted a directory outside the profiles root. After, each is refused and every file is still there, while reset("sales") still deletes exactly that profile.

Not covered by a test: the guard in index.ts itself. That file calls serve() at module scope, so no test can import it, which is the same reason its routes have no tests today. The logic it calls is what the 27 cases cover.

agent-computer: 118 pass, 0 fail. server: same failure set as main (integration tests that want a Postgres). Typecheck and biome clean.

Not in scope

No acting route resolves :botId against a row in bots, so any signed-in user can act on any Bot's computer even with a well-formed id. That is a separate question from path confinement and is noted at the end of #29.

[davidmckayv]

Right answer, and defence in depth rather than one check: refused at the request boundary because the header is this process's input, and again where the path is built so there is one checked way to name a profile directory.

I reproduced the attack on main before reviewing this, so the fix is measured against something real rather than described. Created /tmp/canary/marker.txt inside the computer container, sent x-openbot-bot-id: ../../tmp/canary to POST /computers/reset with the computer token, got {"reset":true} back and the directory was gone: /profiles/../../tmp/canary resolved to /tmp/canary and was removed recursively, as root.

Rebuilt the image on this branch and ran the same thing. 400, That is not a usable bot id., and the file is still there. ../workspace, sales/../../etc, .. and a/b are all refused, and general-assistant, risk_analyst and a uuid-shaped agent id all still answer 200.

Matching supervisor/src/names.ts is the right call: the same class of input already had a line held for container and volume names, and a second, subtly different set of rules would be a worse answer than reusing the one that works. Validating COMPUTER_BOT_ID at boot rather than per request is right too, since a refused default would answer 400 to everything and read as a broken computer rather than a misconfigured one.

Keeping the check out of index.ts so it can be tested without Playwright is the same split authorisation.ts makes, and it is what lets the 27 cases exist at all.

139 tests pass in agent-computer. Rebased onto main, typecheck and format clean.

One interaction to flag: I have a branch for #88 that also changes profiles.ts, bounding how many browsers one computer holds. Whichever of the two lands second needs a small rebase; I will take that on my side.