feat(sandbox): persistent workspace sandboxes with an interactive terminal - #571
Open
dviejokfs wants to merge 12 commits into
Open
feat(sandbox): persistent workspace sandboxes with an interactive terminal#571dviejokfs wants to merge 12 commits into
dviejokfs wants to merge 12 commits into
Conversation
Records the decision to add a workspace lifecycle class whose state is permanent but whose compute is on-demand: workspaces suspend on idle like ephemeral sandboxes do today, and any access wakes them transparently. Deliberately not "a sandbox with no TTL" — always-running containers are what exhaust the 3 vCPU / 4 GB reference host, and suspend-and-wake gives the user the property they actually want at near-zero idle cost. Also records the two follow-ups kept out of the first phase: the interactive terminal (needs a PTY-attach method on SandboxProvider, since temps-sandbox must not depend on bollard) and AI CLI credential injection at create.
Implements ADR-036 phase one: a second sandbox lifecycle class whose
state is permanent but whose compute is on-demand.
- `sandboxes.lifecycle` ('ephemeral' | 'workspace'), plus `project_id`
and `source_repo_url`, all defaulted so existing rows and SDK clients
are unaffected.
- `resolve_id` — the single choke point for exec/fs/jobs — now wakes a
suspended workspace instead of returning 409. Ephemeral sandboxes keep
the 409, which is what `@vercel/sandbox` consumers rely on.
- Workspaces are still swept on idle. "Permanent" means the state
survives, not that the container pins RAM: a handful of idle
workspaces would evict real deployments on the reference host.
- `POST /v1/sandboxes` accepts `lifecycle` and `project_id`; a project
supplies the clone URL, branch, and git connection when no explicit
`source` is given. Gated by project scope + access guards, since it
reads that project's repo credential.
- List gains `lifecycle` and `project_id` filters, both on partial
indexes.
- CLI parity: `sandbox create --workspace --project <id>`,
`sandbox list --workspace/--lifecycle/--project`, and detail output
that distinguishes "suspends at" from "expires".
Also fixes `timeout_secs`, which was documented and named as an idle
timeout but implemented as a wall-clock deadline: `touch()` bumped
`last_activity_at` and never moved `expires_at`, so a sandbox in
continuous use was still stopped at its original deadline. The deadline
is now pushed forward on every exec and filesystem op, materialised in
the indexed column so the sweeper's partial index still applies.
…kout `sandbox create` previously accepted only a raw `--git-url` plus a numeric `--git-connection`, so creating a workspace meant looking up an ID in another command first. Every piece needed to do better already existed and was used elsewhere in the CLI — this wires it up for sandboxes. Resolution order, first match wins: explicit `--git-url`/`--tarball-url`, then `--repo owner/name` (a connected repo with no temps project), then `--project <slug>` or the project linked in `.temps/config.json`, then the git remote of the current directory, then interactive pickers. In a linked checkout `sandbox create --workspace` now needs no flags at all. - `--project` takes a slug, not a numeric ID, resolved through the same `resolveProjectSlug` chain as every other command. It took an ID when first added, which was inconsistent with the rest of the CLI. - `--repo owner/name` resolves the clone URL and connection off a git provider, so private repos clone without the caller handling a token. - `--branch <ref>` (alias of `--git-rev`) and `--new-branch <name>`, which branches after the clone and reports non-fatally if git refuses. - Prompts are gated on a TTY; in CI the error names the flag that would have avoided the prompt instead of hanging. - Create output shows which source was chosen and how it was worked out — inference is only trustworthy when it's visible. Also adds schema-level coverage for the ADR-036 migration against a real Postgres: pre-existing rows default to ephemeral, `lifecycle` is NOT NULL with the right default, both partial indexes carry their predicates, and `down` fully reverses.
Completes the ADR-036 workspace story: you can now sit in a sandbox from
your own terminal and run claude/codex interactively, not just fire
one-shot execs at it.
- `SandboxProvider::attach_pty` — a trait method, not direct Docker
access, because temps-sandbox must not depend on bollard (ADR-010).
Docker implements it via the relay ADR-008 specifies (`docker exec
socat - UNIX-CONNECT:/run/temps-pty/agent.sock`); Firecracker and local
return an explicit "not supported" rather than a terminal that silently
never echoes.
- `GET /v1/sandboxes/{id}/terminal` bridges a WebSocket to the agent,
using the same frame shape as the container terminal in
temps-deployments (binary = PTY bytes, JSON text = control) so an
xterm.js client can be added later with no server change.
- `temps sandbox shell <id> [--tab] [--cmd]`, with raw-mode stdin,
SIGWINCH forwarding, and guaranteed terminal restore on every exit
path.
Detaching does not kill the program: the agent keeps tabs alive with zero
subscribers, so reattaching to the same tab lands back in the same PID
with scrollback replayed. Verified against a real sandbox image — a
dropped connection then reattach returned `existed: true`, the same pid,
and a still-running background job.
Also fixes the sweeper/terminal conflict this exposed. Activity was only
recorded by exec and filesystem calls, so someone talking to an AI CLI in
a terminal looked idle and would have had the container stopped out from
under them — killing the agent and every PTY with it, since /run/temps-pty
is tmpfs. An attached terminal now heartbeats activity every 20s, bounded
below the 60s minimum timeout.
And fixes a live bug found on the way: every command in the npm CLI's
sandbox group built `/v1/sandbox` (singular) while the server has only
ever served `/v1/sandboxes`, so the whole group 404'd against a real
instance. The Rust CLI already had the fix and a comment warning about it;
this one did not. Now covered by a test.
A user with no MFA got `RequireVerification { mfa_setup_required: true }`,
which the UI rendered as a dialog whose only forward action was "Configure
MFA". That is a dead end, not a check: step-up re-verifies a factor you
already hold, and demanding one that was never enrolled cannot succeed.
On a fresh self-hosted instance it was worse than an inconvenience. Every
route to a CLI token is gated by `SensitiveAction::CreateApiKey` — key
creation, key reveal, and the CLI device-flow login — so the first admin
could not obtain a token at all without first enrolling MFA, and nothing
said so until the last step of the key-creation wizard.
Unenrolled principals are now allowed through. Enrolled users are
unaffected: they are still challenged when their session has no recent
elevation, which is the case the control was written for.
The tradeoff is explicit and deliberate: because the gate applies only to
enrolled users, it can be avoided by not enrolling. It therefore protects
accounts that opted into MFA — bounding the damage from a stolen session
cookie — rather than acting as a barrier an attacker with account access
must clear. Operators wanting the stricter posture should mandate MFA
enrolment at the account level, or register a custom
`SensitiveActionAuthorizer`; the UI branch for that policy is kept and its
copy now attributes the requirement to the instance rather than to Temps.
Verified against a running instance: creating an API key as an admin with
no MFA enrolled returned 201 instead of 428.
Also registers temps-sandbox in the `project_access_guard!` coverage
snapshot, which the ADR-036 project-scoped create added without updating.
Three bugs that only surfaced once a real terminal ran against a real
server. All three were invisible before because the npm CLI's sandbox
commands 404'd, so nobody ever got a response to be wrong about.
**Typing was invisible.** `temps-pty-agent` called `cfmakeraw()` on the pty
at creation, clearing ECHO for the pair. Keystrokes reached bash — commands
ran — but nothing echoed them back, so users typed blind; `stty -a` in the
session reported `-echo -icanon`. Raw mode is the child's decision: readline
and full-screen TUIs each call `tcsetattr` when they want char-at-a-time
input. The pty now keeps the kernel defaults, as ssh/script/tmux do, with a
test asserting ECHO and ICANON are on at spawn.
**Every shell died with `spawn_failed: No such file or directory`.**
`create_sandbox` hardcoded `work_dir: "/workspace"` into the row while the
image's real work dir is `/home/temps/workspace` — the agent-run path
directly above already used the `SANDBOX_WORK_DIR` constant. Nothing caught
it because `exec` and the filesystem ops take the work dir from the provider
handle; the terminal was the first consumer of the column. The row now
records the real path, and the terminal reads from the handle so sandboxes
created before this fix work too.
**`sandbox list` crashed on `data.items.length`.** The response types were
written against a flat, pre-Vercel-compat contract: the server returns
`{sandboxes, pagination}` and `{sandbox, routes}`, with `cwd` rather than
`work_dir` and `createdAt`/`timeout` as epoch/duration milliseconds — there
is no `expires_at` on the wire. Adds a `toSandboxView` projection mirroring
`web/src/components/sandboxes/helpers.ts` so the CLI and console derive it
identically, and covers the three fields that were wrong.
Verified against the running instance: created a workspace from a public
repo, confirmed the clone with `git log`, attached a terminal, and observed
`pwd` return /home/temps/workspace and typed characters echo back.
Blocking: the terminal's agent->client read was `read_frame`, built on two
`read_exact` calls, inside a `tokio::select!`. `read_exact` is not
cancellation-safe, and a select branch expression is dropped whenever a
sibling fires — here on every keystroke and every 20s heartbeat. A drop
between the length header and the payload discarded the consumed bytes, so
the next read parsed payload as a header and the stream desynchronised.
Adds `FrameCodec`, a `tokio_util` `Decoder` living beside the frame format
it decodes, and switches the handler to `FramedRead`. A `FramedRead` buffer
survives cancellation. Covered by a test that streams 20 split frames while
a timer cancels the read continuously, asserting no frame is lost, reordered
or corrupted — it fails against `read_frame`.
Major: the project-derived seed source skipped validation. The handler
validates `body.source`, which by construction never sees a URL this layer
derives from a project row, so `projects.git_url` reached the clone without
the DNS-resolution SSRF check (IP literals are checked project-side, names
are not) and without the embedded-credentials check — a legacy row carrying
`user:password@` would have been persisted to `source_repo_url` and echoed
back out of the API. Validated at the point of derivation. Deliberately not
applied to caller-supplied sources: the handler already covers those, and
duplicating it would add a DNS lookup to every create.
Major: capped concurrent terminals at 32 with a semaphore, 429 past it.
Each attach holds a `docker exec` for the session and writes an activity
row every 20s, which was an unbounded self-service DoS on the reference
host.
Minor: `tab` is now validated as `[A-Za-z0-9_-]{1,64}`. It keys a PTY plus a
64 KiB ring per distinct value in the agent, and was interpolated into
operator logs where a newline forges lines.
Minor: the step-up bypass now logs at INFO with `step_up=skipped_no_mfa`
rather than DEBUG, so an audit trail can distinguish "step-up satisfied"
from "step-up skipped for want of a factor".
Minor: corrected the `require_mfa_for_admins` guidance. That setting gates
the password-login path only and OIDC logins are intentionally unaffected,
so on an SSO instance admins keep `mfa_enabled == false` and pass this gate
— the opposite of what an operator enabling it would expect.
Minor: docs example passes the API key by reference, not literally.
Regenerated `web/src/api/client` from the merged server, which was the
blocker on the console knowing anything about workspaces. The diff is
smaller than feared — 131 insertions — because the branch is now merged up
to main, so the only new surface is this branch's own: `lifecycle`,
`project_id` and `source_repo_url` on `SandboxInner`, the `lifecycle` and
`project_id` list filters, and the `GET /v1/sandboxes/{id}/terminal`
operation.
With the types available, the Sandboxes list now distinguishes the two
classes. A workspace carries a `workspace` badge, and its countdown reads
"to suspend" / "suspended — wakes on next use" instead of "left" /
"expired", never in destructive red. Without that, a suspended workspace
renders identically to a dead sandbox — "stopped, expired" — which is
exactly the wrong thing to tell someone whose files are intact and who
only has to run a command to get it back.
The codegen key was minted for the run and deleted immediately after.
Follow-up to the /review-pr + security-auditor pass on this branch. touch() was cancelling extendTimeout() ------------------------------------- `extend_timeout` pushes `expires_at` past the idle window and deliberately leaves `timeout_secs` alone. `touch` then assigned `now + timeout_secs` unconditionally — and `touch` runs on every exec, every fs op, and every terminal heartbeat. So the first thing the long-running operation did was silently cancel the extension bought for it: extend by an hour at t0+10, exec at t0+20, deadline back to t0+3620. The deadline now only moves forward, guarded in SQL (`CASE WHEN expires_at < $1`) so `touch` stays one statement with no read-modify-write on the exec hot path. `last_activity_at` is still bumped unconditionally — it records activity, not a deadline. Regression test included. Terminal bounds --------------- - WebSocket messages were unbounded. axum defaults to 64 MiB, and the agent's 4 MiB frame check only runs *after* the message is buffered, so 32 sessions meant ~2 GiB of attacker-controlled buffering on the 4 GB reference host. Capped at 64 KiB, which is generous for keystrokes. The `MAX_FRAME_BYTES` doc comment claimed this cap already existed; it did not, and it now says what it actually bounds. - A half-open socket pinned a sandbox forever: with no keepalive the heartbeat was the only firing branch, so it kept calling `touch` indefinitely and never returned its slot — defeating the very idle timeout this branch set out to fix. Sessions now ping, close after 3 unanswered pings, and time out a blocked write instead of wedging the whole select! loop. - The global 32-slot cap was instance-wide, so one account could lock everyone else out with a 429. Added a per-user sub-cap of 4, released via RAII. - Authorization is evaluated once at upgrade and never revalidated, so a logout or revoked key left the shell working. Full revalidation needs an auth dependency the sandbox state does not carry; a 12h ceiling bounds the window in the meantime and is documented as such. - An oversized paste killed the terminal with no explanation; it now gets an error frame first. - Agent frames that fail to decode no longer fall back to defaults silently. Read-only endpoints no longer boot containers --------------------------------------------- `resolve_id` wakes a suspended workspace, and five call sites used it purely as an ownership check: `domain()`, `preview_link()`, `job_status`, `subscribe_job_logs`, `list_jobs`. A `GET` that starts a container is a surprise that costs real memory, and for the job endpoints it is pure waste — the in-memory job table was emptied when the container stopped, so waking it cannot change the answer. Added `resolve_id_no_wake` for those. Also ---- - The migration now backfills `work_dir` from the hardcoded `/workspace` to `/home/temps/workspace`. Fixing the code without the data left every pre-existing sandbox reporting a `cwd` that has never existed in the image. - `wake_workspace` bounds the container start at 120s; a wedged daemon was an indefinitely hung request. - Consolidated the two URL-credential checks on the correct one. The handler's copy scanned for the first `@` after the scheme, so it false-positived on a URL with an explicit port and an `@` in the path. - Dropped the unused `bytes` dependency; corrected the `list_for_user` index claim (the partial index only serves the non-ephemeral side). - ADR-036 said `project_id` carries `ON DELETE SET NULL`; there is no FK, on purpose, and the ADR now explains why. It also now states that the project-derived clone uses the caller's own git connection. - Recorded, on the step-up authorizer, *why* the unenrolled-user change is not the loss it looks like — enrolment itself is not re-auth protected, so the old gate was never a control for those users — and that this reasoning is load-bearing: fixing enrolment means revisiting this policy.
📓 Changelog previewThis is what your commits will add to the generated ## [Unreleased]
### Added
- **sandbox:** Add persistent workspace sandboxes with wake-on-access
- **cli:** Resolve workspace source from project, repo, or local checkout
- **sandbox:** Add interactive terminal and `sandbox shell`
- **web:** Regenerate the SDK and surface workspaces in the console
### Documentation
- **adr:** Add ADR-036 for persistent workspace sandboxes
### Fixed
- **auth:** Only require step-up from users who have enrolled MFA
- **sandbox:** Restore terminal echo, work dir, and CLI response shapes
- **sandbox:** Address review findings on the terminal and step-up policy
- **sandbox:** Address review findings on workspaces and the terminal
- **sandbox:** Normalise the PTY from the host so terminals echo on any image |
…y image The agent's `cfmakeraw()` removal fixed the blind-typing bug at the source, but the agent binary is compiled into the sandbox image — so the fix reaches a sandbox only after an operator rebuilds that image. Every existing sandbox, and every new one from an existing image, still started its PTY with ECHO off. In practice the bug was still fully present for anyone using the feature, and the workaround was to type `stty sane` blind into a shell that appeared broken. Rebuilding the image is not a step the person trying to open a terminal should have to perform first, so the host now normalises the line discipline itself: the requested program is wrapped as `stty sane 2>/dev/null; exec <cmd>`. One `stty` per tab, works on every image, and `exec` keeps the wrapper shell from lingering as a parent. Safe in front of anything: full-screen programs set their own termios on startup regardless, and a sane state is what a real terminal would hand them. `stty` failure is swallowed so a minimal image without it still gets a shell. Verified against a sandbox created from an image predating the agent fix, on a fresh tab, with no `stty sane` sent: all five characters of `ls -l` echoed back and the command ran. The wrapper can be dropped once no supported image ships the old agent; until then removing it silently reintroduces a shell you cannot type into, so it is covered by tests.
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
1 participant
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
Adds persistent workspace sandboxes (ADR-036): a sandbox you keep working in
over days, seeded from a git repo, with an interactive terminal you can run
claude/codexin from your own shell.The design point worth arguing about is in the ADR: this is deliberately not
"a sandbox with no TTL". Workspaces still suspend on idle exactly like ephemeral
sandboxes — always-running containers are what exhaust a 3 vCPU / 4 GB host, and
the failure mode (deployments evicted by idle dev containers) is invisible until
it's catastrophic. What changes is the access path: a suspended workspace wakes
transparently instead of returning 409. State is permanent; compute is on demand.
sandboxes.lifecycle(ephemeral|workspace) plusproject_idandsource_repo_url, all defaulted so existing rows and@vercel/sandboxclientsare unaffected.
GET /v1/sandboxes/{id}/terminal— WebSocket bridged to the in-sandbox PTYagent (ADR-008) via a new
SandboxProvider::attach_pty. A trait method ratherthan direct Docker access, because
temps-sandboxmust not depend on bollard(ADR-010, CI-enforced).
temps sandbox shell <id> [--tab] [--cmd], and source resolution from--project <slug>/--repo owner/name/ the local git remote / interactivepickers. In a linked checkout
sandbox create --workspaceneeds no flags.Bugs fixed along the way
Each of these was pre-existing and only surfaced because a real terminal finally
ran against a real server:
timeout_secswas documented as an idle timeout but implemented as awall-clock deadline.
touch()bumpedlast_activity_atand never movedexpires_at, so a sandbox in continuous use was still stopped on schedule./v1/sandbox(singular);the server has only ever served
/v1/sandboxes. The Rust CLI already carriedthe fix and a comment warning about it.
sandbox listcrashed, and the response types were written against apre-Vercel-compat contract (
cwdnotwork_dir;createdAt/timeoutasepoch/duration ms — there is no
expires_aton the wire).spawn_failed: No such file or directory.create_sandboxhardcoded/workspacewhile the image's real dir is/home/temps/workspace; the agent-run path two lines above already used theconstant.
cfmakeraw()atcreation, clearing ECHO;
stty -ain-session reported-echo -icanon. Rawmode is the child's decision — readline and TUIs set it themselves.
Behaviour changes reviewers should weigh
Step-up/MFA. Users with no enrolled MFA are now allowed through sensitive
actions instead of being told to enrol. Previously the first admin of a fresh
self-hosted instance could not obtain a CLI token at all — key creation, key
reveal and the CLI device flow are all gated by
SensitiveAction::CreateApiKey.The tradeoff is explicit: the gate now applies only to enrolled users, so it can
be avoided by not enrolling. It protects accounts that opted in (a stolen session
cannot unenrol — disabling MFA needs a valid TOTP) rather than being a barrier an
attacker with account access must clear.
require_mfa_for_adminsis not asufficient fallback and the docs now say so: it gates password login only, so SSO
admins keep
mfa_enabled = false.Idle timeout now behaves as documented, so an actively-used ephemeral sandbox
survives past its original deadline.
Evidence
Against a live instance (fresh DB, migrations applied from zero):
Terminal, over the real WS endpoint:
Reattach survives a dropped connection (killed mid-session, reconnected to
the same tab):
existed: true, same pid 49, scrollback replayed, backgroundjob still
Running.Interleaved I/O — typing while output streams, the case that corrupted the
first implementation:
seq 1 4000while typing character-by-character →3999/4000 numbers intact and in order, no error, marker echoed and executed.
Tab validation, real handshakes:
main,my-tab_2→ 101; space, newline,65-chars, empty → 400.
Step-up fix:
POST /api/api-keysas an admin with no MFA → 201 (was 428).Route fix:
/api/v1/sandboxes→ 200,/api/v1/sandbox→ 404.Migration: testcontainer test asserts pre-existing rows default to
ephemeral,lifecycleis NOT NULL with the right default, both partial indexescarry their predicates, and
down()fully reverses.Review
Reviewed with
/review-prplus thesecurity-auditoragent. The auditor'sverdict was mergeable — no blocking or major findings, having independently
traced terminal IDOR (closed),
cmdinjection (not injection — argv vector,constant socket path, runs in-container at the privilege
/execalready grants),and git-token handling (ephemeral
GIT_ASKPASS, never argv/URL/log).Findings that were then fixed in
34765807: a cancellation-safety bug in theterminal read loop (now a
tokio_utilDecoder, with a test that streams splitframes under continuous cancellation); the project-derived source skipping SSRF
validation; an unbounded attach count (now capped at 32 with a 429);
tabvalidation; an audit record for the step-up bypass; the
require_mfa_for_adminsdoc correction.
Known gaps
temps-pty-agentis compiledinto the image, so sandboxes on existing images still need a blind
stty saneuntil
POST /settings/sandbox-rebuildruns. That endpoint has no CLI command —a parity gap worth its own fix.
Origincheck on the terminal WebSocket. Safe today because the sessioncookie is
SameSite=Strict; an allowlist is cheap defence in depth andtemps-deployments' container terminal omits it too.before this is offered on Temps Cloud.
-e.