Repository navigation
PR6 — Harden Agent Code Execution: purity, @fly/sprites SDK reconcile, full tests - #1489
Merged
Merged
Conversation
Replace the hand-rolled internal-deny domain list (*.internal, *.flycast,
*.tigris.dev, _api.internal) with the SDK's maintained { include: 'defaults' }
PolicyRule preset — the only lever the Sprites network-policy API exposes for
the internal surface. It is prepended before any allow whenever the allowlist
is deliberately widened, so a later misconfiguration that allowed * still could
not reach the internal targets. The v1 empty-allowlist case stays a pure
deny-all and leans on no preset semantics.
Document the known limitation: domain rules cannot block IP-literal egress, so
the empirical 6PN/metadata isolation (gate G1) is a deployment concern in the
enablement checklist, not encoded here.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Verified the SDK surface against @fly/sprites@0.0.1-rc37 type defs and aligned
the driver to it:
- Hard wall-clock timeout via spawn + kill('SIGKILL'). The SDK's promise-based
exec/execFile expose no timeout and no abort handle, so the run is driven
through spawn (same structured file+args[] form, no host shell string) which
returns a SpriteCommand we can SIGKILL on a timer. We replicate the SDK's own
execFile stream collection (stdout/stderr data listeners, exit event) and kill
the command — not the Sprite — so the warm session survives a single slow run.
- maxBuffer: cap buffered stdout+stderr at the policy output cap; an output
flood SIGKILLs the command and fails the run (host-memory DoS guard).
- Explicit storage cap: add storageGb to the policy and map it onto SpriteConfig
(ramMB/cpus/storageGB/region) so every Sprite gets explicit caps, not the
quota default.
A non-zero exit now resolves as a result (spawn's wait surfaces the code) rather
than being recovered from a thrown ExecError, dropping the ExecError duck-typing.
Tested with a non-terminating command (SIGKILL + timeout) and an over-cap output.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…put cap buildSandboxEnv was reading getValidatedEnv() in a default param, the one IO in an otherwise pure module. Make env a required injected argument so the allowlist construction is fully pure and deterministic; move the getValidatedEnv() read into defaultBuildEnv (the effect seam in tool-runners), matching the DI pattern used across the sandbox layer. Forward the policy output cap to the driver as maxBytes (mapped onto the SDK's maxBuffer) alongside the existing wall-clock timeout, so the runner bounds both the run duration and the buffered output from a single place. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Guard the spawn output collector so a not-yet-dead command's late stdout/stderr chunks are dropped once the run has settled (overflow / timeout / exit). Bounds host memory in the brief window between SIGKILL and the process actually dying on an untrusted output flood. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b6955fdd8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+209
to
+213
| command.stdout.on('data', (chunk) => { | ||
| stdoutLen = collect(stdoutChunks, chunk, stdoutLen); | ||
| }); | ||
| command.stderr.on('data', (chunk) => { | ||
| stderrLen = collect(stderrChunks, chunk, stderrLen); |
There was a problem hiding this comment.
When runBashInSandbox passes policy.maxOutputBytes as maxBytes, the new collector applies that limit independently to stdout and stderr because stdoutLen and stderrLen are tracked separately. A command that writes just under the cap to each stream will therefore buffer and return nearly twice the configured policy limit instead of failing at the documented stdout+stderr cap, weakening the host-memory/output-flood guard this change is trying to add.
Useful? React with 👍 / 👎.
The maxBuffer collector pushed each chunk before testing the cap, so a single oversized stdout/stderr frame was retained in memory before the overflow was detected. Compute the projected length first and, on overflow, SIGKILL and fail WITHOUT retaining the offending chunk — buffered memory now never exceeds the cap. Found in self-review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2witstudios
added a commit
that referenced
this pull request
Jun 3, 2026
* ci(sandbox): gate PRs into pu/flash-sandbox with Test Suite + Security The Test Suite and Security workflows only triggered on pull_request to [main, master, develop], so PRs targeting the pu/flash-sandbox integration branch (the Agent Code Execution epic) ran no real CI. Add pu/flash-sandbox to both pull_request branch filters so main's checks gate these PRs too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): PR1 — safety foundation & policy (Agent Code Execution) (#1465) * feat(sandbox): add canRunCode authorization gate with kill-switch Compose getUserDrivePermissions + getAgentAccessLevel behind a single fail-closed authorization chokepoint for agent code execution. Checks are ordered cheapest-first: a default-OFF CODE_EXECUTION_ENABLED kill-switch and cloud-only deployment gate deny before any DB round-trip. Never throws — any dependency error resolves to a denial. DB-backed helpers are injected (lazily imported in the default wiring) so the unit tests exercise the composition with fakes and never touch the database. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): add pure resolveExecutionPolicy with safe defaults Resolve an explicit per-run policy (timeout, vCPU, memory, output cap, region) instead of inheriting platform defaults. Egress is default-deny (empty allowlist) and sandboxes are ephemeral (persistent: false) on every profile; an unknown profile falls back to the most-restrictive safe minimum so a typo can never widen the blast radius. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): add pure buildSandboxEnv allowlist scrub Build the sandbox environment by allowlist — copying only a fixed set of explicitly-safe keys from the validated env — so no DB credential, signing secret, or API key can ever reach untrusted code. Never spreads process.env or the validated env wholesale; a newly-added secret is excluded by default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): add code execution quota and per-tier concurrency Add app-level sub-limits so one tenant's runaway agent can't starve or bill everyone under Vercel's account-wide caps. A per-user in-process semaphore (ceiling scales by subscription tier, modeled on upload-semaphore) plus a daily run budget via a new CODE_EXECUTION distributed-rate-limit entry applied independently to user/drive/tenant scopes. checkCodeExecutionQuota checks concurrency first so a saturated system rejects without spending any budget. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): add code execution audit record + writer Every run yields an immutable audit record (actor, redacted code, profile, exit, duration, cost, timestamp) written to the hash-chained activity log via a new code_execution ActivityOperation. Anomalous runs (timeout, OOM, blocked command, non-zero exit) additionally raise a security audit event. Builders are pure (injected timestamp); secrets in captured code are redacted before persistence. The writer is fire-and-forget — a failing sink never breaks the run it records. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(sandbox): bound code length before secret redaction Truncate submitted code to the audit cap before running redaction regexes so they only ever execute over bounded input, removing any ReDoS surface on large submissions. codeTruncated still reflects the raw submitted length. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(sandbox): freeze resolved execution policies (review F1) Policies are returned by reference from module constants; readonly only guards compile time. Freeze the constants and their egress arrays so a downstream caller cannot mutate the shared default-deny egress baseline (which would widen egress globally for every subsequent run). Add a regression test asserting the allowlist cannot be pushed to. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(sandbox): reword redaction comment to avoid literal key prefixes Keep secret-scanner-trigger patterns out of source comments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): don't fail env validation on stray kill-switch values (review P2-D) CODE_EXECUTION_ENABLED was z.enum(['true','false']), so any other value (e.g. =0 or =TRUE) made the app-wide validateEnv() throw — breaking unrelated startup and health checks instead of leaving the feature disabled. Accept any string; isCodeExecutionEnabled() already enables only on the exact value 'true'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): drop deployment-mode gate from canRunCode (review P2-C) !isCloud() wrongly denied DEPLOYMENT_MODE=tenant, which is a cloud deployment (isolated image per tenant) with the same feature set. We don't serve on-prem (a local execution path is future work, not wired here), so any DEPLOYMENT_MODE / isOnPrem gate would only add a dead branch. Remove the check entirely; authz, kill-switch, and quota remain the gates. Drops the isOnPrem dep and the not_cloud denial reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): redact secrets across the truncation boundary (review P2-A) The earlier ReDoS fix truncated to 4 KB before redacting, so a quoted secret whose closing quote fell past the cap escaped SECRET_ASSIGNMENT and leaked a prefix into the immutable audit log. Redact over a wider 16 KB scan window, then truncate the redacted result — a secret starting before the cap is fully collapsed first. Simplify STANDALONE_TOKEN to a single non-ambiguous run so the wider scan stays linear. Add a straddle-the-cap regression test. Also document the invariant that sandbox code is always model-generated (isAiGenerated, review F3) and that redaction is best-effort audit hygiene, not the security boundary (review F4). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): make quota check a non-incrementing preflight (review P2-B, F1) checkDistributedRateLimit increments the bucket per call, so checking user -> drive -> tenant in sequence charged the earlier scopes even when a later one denied — letting an exhausted drive drain a user's daily budget in unrelated drives. Switch the default dep to getDistributedRateLimitStatus (the read-only sibling) so the multi-scope check consumes nothing. Document that the check is advisory: the single real charge per run and acquireCodeExecutionSlot() are wired at execution time in PR3, which must handle acquire === false. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(sandbox): note agent-path drive-as-root coupling (review F5) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): require actor authz for agent-origin runs (review P1) Agent-origin runs only checked the agent page's drive edit access and skipped the actor's owner/admin drive-role gate entirely, letting a plain member escalate by triggering an agent that holds drive edit access. canRunCode now always clears the actor user through authorizeUser first, then — for agent origin — additionally requires the agent page to hold drive edit access. Both the human and the agent must be entitled. Adds two regression tests: agent-origin run denied when the triggering user is a plain member (insufficient_role) or has no drive membership (no_drive_access). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): resolve unknown profile via own-key check (proto bypass) resolveExecutionPolicy used `PROFILES[profile] ?? SAFE_MINIMUM_PROFILE`. Bracket lookup resolves inherited keys, so a profile of '__proto__', 'constructor', 'toString', etc. returned a truthy Object.prototype member and skipped the safe-minimum fallback — yielding a "policy" with undefined timeout, vCPU, memory, output cap, and egress allowlist. The signature accepts an arbitrary string, so an untrusted profile (PR3 wiring) reaches it. Guard with an own-property check so only real profiles resolve and everything else falls back to the most-restrictive policy, as documented. Adds a regression test for the prototype-key inputs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(sandbox): simplify audit redaction + dedupe resource id Two cleanups surfaced by a proactive review pass on the audit builder: - Drop the 16 KB REDACTION_SCAN_LIMIT window: redact the full code, then truncate. The window never added safety (anything past the 4 KB storage cap is dropped by the truncate either way) and carried its own straddle edge. Redact-before-truncate still fully collapses a secret straddling the storage cap; the regexes are linear so full-input scanning is cheap. - Extract resolveAuditResourceId() so buildActivityLogInput and buildSecurityAuditEvent derive the run's resource id from one place — forensic correlation across the activity and security logs breaks if the two precedence chains ever diverge. No behavior change to stored output; 51 sandbox tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(sandbox): add adversarial ReDoS payload for secret redaction Failing test: the SECRET_ASSIGNMENT regex backtracks polynomially on many repetitions of 'key' (CodeQL js/polynomial-redos). Redaction runs over agent-supplied code, so this is an attacker-triggerable CPU-exhaustion path. Asserts redaction completes in linear time while still redacting a real secret. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): make secret redaction regex linear-time (CodeQL js/polynomial-redos) The SECRET_ASSIGNMENT regex matched a key as `[A-Za-z0-9_-]*keyword[A-Za-z0-9_-]*` — two unbounded runs around a keyword whose characters the runs also match — so it backtracked polynomially (~O(n^2)) on inputs like "keykeykey…". Redaction runs over agent-supplied code, making it an attacker-triggerable CPU-exhaustion path. Replace it with a single linear ASSIGNMENT matcher (`[A-Za-z0-9_-]+` for the identifier, a distinct `[:=]` separator) and move the secret-keyword test into the replace callback. Every redaction regex is now a single non-ambiguous quantifier per class. Adversarial 384 KB payload: ~2 ms (was ~7 s). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci(sandbox): run Security suite + CodeQL on sandbox feature paths The security workflow is path-gated and the Agent Code Execution code lives in packages/lib/src/services/sandbox/** + the new sandbox_sessions schema, which matched none of its filters — so the most security-critical code (session isolation, resume re-authz) merged with only Lint+Unit Tests. Add the sandbox paths so the Security suite + CodeQL scan this feature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): PR2 — conversation sandbox lifecycle (Agent Code Execution) (#1469) * feat(sandbox): derive unguessable conversation session keys HMAC-SHA256 over (tenant + drive + conversation) keyed by a server-held secret. Namespaced so distinct conversations never collide onto one sandbox, and unguessable so an actor who knows a conversation id cannot reconstruct the sandbox name to probe another session's warm VM. Pure — the secret is injected by the effect layer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): plan conversation sandbox lifecycle Pure planner deciding create / resume / idle-teardown / session-end teardown / deny. Encodes two security invariants: an unauthorized actor is denied even when a warm session exists (resume re-authz — never hand back prior-actor state), and session-end always tears down regardless of authorization (cleanup is unconditional, no orphaned VMs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): map execution policy to sandbox create options Pure translation of the resolved ExecutionPolicy bounds (timeout, vCPU, memory, persistent, region) into the option object the effect layer hands the sandbox client. Explicit caps from policy; never platform defaults. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): add sandbox_sessions table for the conversation link Persists the sandboxId<->conversation link keyed by the opaque session key (unique), so later turns reconnect to the same warm sandbox. A row is deleted on teardown; lastActiveAt drives idle reclamation. Generated migration 0142; exported from schema.ts + db package exports. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): session store for the sandboxId<->conversation link Small interface over sandbox_sessions (find/save/touch/remove) with a Drizzle-backed impl that upserts on the unique session key. Lazily imports the db module so callers injecting a fake never load the DB graph; the orchestrator is unit-tested against an in-memory store. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): conversation sandbox lifecycle effects + teardown acquireConversationSandbox ties the pure pieces to injected IO: derive key, look up the link, RE-AUTHORIZE the current actor, plan, then execute create/resume/idle-teardown against the sandbox client + store. Enforces resume re-authz (deny never reconnects a warm VM) and no-orphans (stop the new VM if the link can't be persisted). teardownConversationSandbox is idempotent and never throws — stop is best-effort, the link is always removed. Adds SANDBOX_SESSION_SECRET (min-32, optional, fail-closed) and the new module exports. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(sandbox): type session store queries against real drizzle types Drop the hand-rolled structural db interface + cast in favour of writing the Drizzle queries directly in createDbSandboxSessionStore against the real, lazily-imported db/eq/table. The store interface remains the unit-test seam (the orchestrator injects an in-memory fake); the concrete queries are now fully type-checked rather than cast through 'unknown'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): fail closed on store IO errors; best-effort resume touch Wrap acquireConversationSandbox so an unexpected IO failure (store lookup, link removal, client.get) denies with reason 'error' instead of throwing — DB-backed checks must fail closed. Make the resume lastActiveAt update best-effort so a failed metadata write never denies an authorized, confirmed-live resume. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(sandbox): hash session key with SHA3-256 (repo convention) Switch the session-key HMAC digest from SHA-256 to SHA3-256 to match the repo's convention for security tokens hashed at rest (auth/token-utils.ts; CLAUDE.md 'SHA3-256 hashed at rest'). HMAC keying is retained — the key's inputs are low-entropy, so the server secret is what makes it unguessable. Output is still 64 hex chars; no behavioural change beyond the primitive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): address Codex review — blank secret + teardown IO guarding - env-validation: accept a blank SANDBOX_SESSION_SECRET placeholder (.or(z.literal('')), mirroring the URL vars) so 'SANDBOX_SESSION_SECRET=' disables sandbox acquisition (lifecycle fails closed) instead of failing app-wide env validation at instrumentation startup. A non-empty value still must be >= 32 chars. - teardownConversationSandbox: guard the store lookup in try/catch and make the link removal best-effort (safeRemove), so end/idle/crash/failure cleanup never propagates a store error — honouring its documented idempotent, never-throws contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(sandbox): tighten teardown contract wording + type guarded lookup Doc/type-only: correct the teardown doc comments to state that the lookup is guarded and the stop + link removal are best-effort (a lingering link self-corrects on next acquire), and give the guarded `existing` lookup an explicit SandboxSessionRecord|null type instead of an evolving any. No behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): PR3 — bash/writeFile/readFile tools over @vercel/sandbox (Agent Code Execution) (#1472) * feat(sandbox): add @vercel/sandbox dep + pure safety primitives Adds the @vercel/sandbox dependency and the pure, fail-closed building blocks the PR3 execution path composes inline: - egress.ts: maps a policy egress allowlist to a @vercel/sandbox network policy — `deny-all` by default, and never reachable to the cloud metadata endpoint or any RFC1918/CGNAT/link-local range even when widened for an external registry (subnet denies take precedence). - command-policy.ts: size + empty + metadata-IP block, linear regex only (no polynomial backtracking), returns allow/block without throwing. - output-limit.ts: byte-bounded truncation of untrusted stdout/stderr. - sandbox-paths.ts: confines tool file IO to the sandbox root via the shared resolvePathWithinSync validator. - sandbox-options.ts: carry the policy egress allowlist through to the client so provisioning can translate it. Adds package.json exports for the new modules. Pure functions only; no execution path is wired yet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): real @vercel/sandbox client adapter Implements PR2's SandboxClient seam against the actual SDK and extends it with the execution surface PR3 needs (runCommand / writeFiles / readFileToBuffer) on an ExecutableSandbox handle. Provisioning is locked down explicitly, never inheriting platform defaults: deny-by-default egress from the policy allowlist, allowlisted env via buildSandboxEnv (fail-safe to empty — never host secrets), explicit vCPUs/persistence, and a VM lifetime that outlives the per-run cap and the idle-reclaim window so a conversation's warm sandbox is reused across turns. The per-run timeout is applied to runCommand, not to the VM. Outbound credentials (future) are brokered via the network policy, never injected as raw secrets. The SDK statics are injected so create-param mapping, exit/stdout/stderr surfacing, and get->null on a vanished sandbox are unit-tested with a fake — never against the real Vercel API. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): per-run budget charge + bash/writeFile/readFile runners Adds chargeCodeExecutionBudget — the single real per-run charge that increments the user/drive/tenant CODE_EXECUTION windows once, sharing the scope-id construction with the non-incrementing preflight. Adds the execution orchestration that is the body of each tool's execute, with the whole safety layer inline and in order: kill-switch re-check; command/path policy before any VM work (a blocked op never provisions and is audited); quota preflight -> concurrency reservation -> budget charge only once a live authorized sandbox is in hand; acquireConversationSandbox (authz + resume re-authz) + reconnect; run/write/read; output truncation to the policy cap; audit every executed run and every blocked op; and a guaranteed concurrency-slot release in finally. The command is passed to the sandbox as a structured arg array (sh -c), never a host-side shell string. All IO is injected and unit-tested with fakes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): bash/writeFile/readFile AI SDK tool wrappers (unregistered) Thin AI SDK tool() wrappers over the conversation sandbox: each execute reads the chat context, resolves the actor (drive from the active location, tenant = drive owner, concurrency tier = acting user), and delegates to the @pagespace/lib runner where the safety layer lives. The runner deps and context resolver are injected so the wrappers are unit-tested with fakes (no DB, no real Vercel API). NOT REGISTERED: these are not spread into pageSpaceTools and are not tool_search-discoverable. Exposure to agents (registration + default-OFF feature flag) is PR4 — until then this module is unreachable from chat. Adds the optional VERCEL_TOKEN / VERCEL_TEAM_ID / VERCEL_PROJECT_ID env vars to serverEnvSchema (the SDK falls back to OIDC when absent; a partial triad never half-authenticates). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): PR4 — register flag-gated bash/writeFile/readFile tools in agent chat (#1477) * feat(sandbox): add call-time tool gate (kill-switch + authz + quota), default-OFF Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): register flag-gated bash/writeFile/readFile tools in agent chat Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(sandbox): PR5 — switch execution driver to @fly/sprites (Agent Code Execution) (#1481) * feat(sandbox): swap execution driver from @vercel/sandbox to @fly/sprites Replace the Vercel Sandbox client with a Fly Sprites driver behind the existing provider-neutral SandboxClient seam. The entire safety layer (can-run-code, quota, audit, command-policy, output-limit, sandbox-paths, session-key, lifecycle, tool-gate) is unchanged. - Add sandbox-client/{types,sprites}.ts implementing ExecSandboxClient over @fly/sprites (getOrCreate resumes/creates by session key; stop DESTROYS; per-command timeout enforced in-driver with guaranteed teardown). - Egress lockdown is reshaped to the Fly L3 NetworkPolicy: default-deny catch-all, with explicit internal-Fly denies (*.internal, _api.internal, *.flycast, Tigris) placed BEFORE any allow as SSRF defence-in-depth. - Fresh Sprites are destroyed if the egress policy can't be applied — never handed back with open egress. - env: drop Vercel OIDC triad, add SPRITES_API_TOKEN (blank → fail-closed). - SANDBOX_ROOT → /workspace; region iad1 → iad. - Pin @fly/sprites to 0.0.1-rc37 (the published 0.0.1 release regressed and dropped the network-policy + filesystem APIs); remove @vercel/sandbox. - DELETE vercel-sandbox-client.ts + its test — hard cutover, no compat shim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(sandbox): run registered bash/writeFile/readFile tools on Sprites Reconcile the merged PR4 registration with the Sprites driver. Split the flag-gated, default-OFF tools so the provider SDK never loads in the factory's tests: - sandbox-tools.ts is now the provider-agnostic factory only (schemas + context resolution + call-time gate + delegation); no DB, no backing-provider SDK import. - sandbox-tools-runtime.ts holds the production wiring (DB-backed session store, Fly Sprites driver, quota, audit, actor resolver) and the gate wiring, and exports buildSandboxTools. - ai-tools.ts imports buildSandboxTools from the runtime module; registration stays flag-gated and default-OFF (PR4 behaviour preserved, just on Sprites). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): lazy-load Sprites driver off the default path; robust ExecError shape Addresses Codex review on #1481. P1 — @fly/sprites is ESM-only and requires Node >=24, but the web images run Node 22. The static import chain (ai-tools → sandbox-tools-runtime → sprites → @fly/sprites) pulled the SDK into the module graph on every chat request, including the default code-execution-OFF path. Replace it with a dynamic import inside getSandboxClient() so the SDK is loaded only when a sandbox tool actually runs (kill-switch ON). getSandboxClient is now async; acquire/reconnect await it. The off-path no longer evaluates the unsupported SDK. P2 — Make the ExecError duck-type accept both the nested `.result` shape and the flattened `exitCode/stdout/stderr` shape the SDK also exposes, so a version skew can't turn a real non-zero exit into a transport failure. Add a test for the flat shape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * PR6 — Harden Agent Code Execution: purity, @fly/sprites SDK reconcile, full tests (#1489) * refactor(sandbox): reconcile egress to SDK include:'defaults' preset Replace the hand-rolled internal-deny domain list (*.internal, *.flycast, *.tigris.dev, _api.internal) with the SDK's maintained { include: 'defaults' } PolicyRule preset — the only lever the Sprites network-policy API exposes for the internal surface. It is prepended before any allow whenever the allowlist is deliberately widened, so a later misconfiguration that allowed * still could not reach the internal targets. The v1 empty-allowlist case stays a pure deny-all and leans on no preset semantics. Document the known limitation: domain rules cannot block IP-literal egress, so the empirical 6PN/metadata isolation (gate G1) is a deployment concern in the enablement checklist, not encoded here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(sandbox): reconcile driver to real @fly/sprites SDK surface Verified the SDK surface against @fly/sprites@0.0.1-rc37 type defs and aligned the driver to it: - Hard wall-clock timeout via spawn + kill('SIGKILL'). The SDK's promise-based exec/execFile expose no timeout and no abort handle, so the run is driven through spawn (same structured file+args[] form, no host shell string) which returns a SpriteCommand we can SIGKILL on a timer. We replicate the SDK's own execFile stream collection (stdout/stderr data listeners, exit event) and kill the command — not the Sprite — so the warm session survives a single slow run. - maxBuffer: cap buffered stdout+stderr at the policy output cap; an output flood SIGKILLs the command and fails the run (host-memory DoS guard). - Explicit storage cap: add storageGb to the policy and map it onto SpriteConfig (ramMB/cpus/storageGB/region) so every Sprite gets explicit caps, not the quota default. A non-zero exit now resolves as a result (spawn's wait surfaces the code) rather than being recovered from a thrown ExecError, dropping the ExecError duck-typing. Tested with a non-terminating command (SIGKILL + timeout) and an over-cap output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(sandbox): purity — inject validated env + forward policy output cap buildSandboxEnv was reading getValidatedEnv() in a default param, the one IO in an otherwise pure module. Make env a required injected argument so the allowlist construction is fully pure and deterministic; move the getValidatedEnv() read into defaultBuildEnv (the effect seam in tool-runners), matching the DI pattern used across the sandbox layer. Forward the policy output cap to the driver as maxBytes (mapped onto the SDK's maxBuffer) alongside the existing wall-clock timeout, so the runner bounds both the run duration and the buffered output from a single place. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(sandbox): stop retaining command output after the run settles Guard the spawn output collector so a not-yet-dead command's late stdout/stderr chunks are dropped once the run has settled (overflow / timeout / exit). Bounds host memory in the brief window between SIGKILL and the process actually dying on an untrusted output flood. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): enforce output cap before retaining the chunk The maxBuffer collector pushed each chunk before testing the cap, so a single oversized stdout/stderr frame was retained in memory before the overflow was detected. Compute the projected length first and, on overflow, SIGKILL and fail WITHOUT retaining the offending chunk — buffered memory now never exceeds the cap. Found in self-review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sandbox): address Codex/CodeRabbit review threads on Agent Code Execution Hardens the dark-shipped code-execution feature against the review round on PR #1487 (2 Codex P1 + 17 CodeRabbit threads): Sprites driver (sprites.ts): - Re-apply the deny-default egress lockdown on EVERY hand-back (fresh OR resumed), closing the crash-window where a Sprite created before lockdown could run the next command with open egress; a resumed lockdown failure rejects without destroying the warm session. - Narrow the getSprite fallback to genuine not-found only (new isSpriteNotFoundError): auth/rate-limit/outage errors now surface instead of masquerading as a vanished Sprite (which could spawn a duplicate / drop a healthy session). Pure-fn correctness: - egress: sanitizeEgressAllowlist rejects '*', IP literals, and non-host strings so a wildcard can't short-circuit the terminating deny. - output-limit: truncateToBytes is now a HARD byte cap — trims the trailing U+FFFD replacement char so the result never exceeds maxBytes. Boundaries / fail-closed: - session-key: reject an empty HMAC secret (guessable-name guard, defence in depth on top of upstream env validation). - session-store: refresh userId on the conflict upsert so audit metadata tracks the live sandbox's creator after re-provisioning. - session-manager: safeStop now reports confirmation; teardown removes the session link ONLY after a confirmed stop, keeping it on an unconfirmed stop so a retry / the idle reaper reclaims the VM instead of orphaning it. - tool-runners: audit a blocked bash `cwd` path escape (parity with writeFile/readFile path-escape auditing). Web runtime (sandbox-tools-runtime.ts): - Reset the cached client promise on a lazy-load failure (no poisoned rejection until restart). - Fail closed with an actionable message if the SDK is loaded on Node < 24 (the @fly/sprites runtime gate), so flipping the flag on a Node 22 image surfaces the deployment requirement instead of a cryptic SDK crash. CI / packaging: - security.yml + test.yml: gate direct pushes to pu/flash-sandbox and extend the security path filters to the web-layer sandbox registration files and migrations. - package.json: complete typesVersions for all 18 sandbox subpath exports. Documented (in-code) as fail-safe / enablement-gate items rather than changed: multi-scope budget charge is non-atomic but fail-safe (over-counts, never under), idle-session reclaim for denied actors is the reaper's job, and a read-side host-memory cap needs a bounded read at the SDK boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(sandbox): clarify egress internal-deny comment (sanitizer makes '*' unreachable) The allowlist sanitizer now drops '*', so the prior 'even if a later rule allowed *' example is moot; reframe the preset-first ordering as defence in depth on top of sanitization. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci(sandbox): run the sandbox test suite in the security gate security.yml now gates sandbox paths but its job only ran src/security, src/auth, and named utils — so a sandbox-only change triggered the gate without executing any sandbox test. Add a step running the full src/services/sandbox suite (fake-injected, no live Fly/DB) so the security gate actually validates the code-execution boundary it guards. (CodeRabbit thread.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was previously deployed
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.
PR6 — Harden Agent Code Execution: full purity, SDK reconciliation, full tests
Hardening-only PR on top of the merged PR1–5 epic. No behavior/scope change — it tightens purity, reconciles the Fly Sprites driver against the real
@fly/sprites@0.0.1-rc37SDK surface (verified from the published type defs and compiled JS), and adds the missing tests. Default-OFF kill-switch unchanged.SDK surface — verified, not assumed
Pulled the
0.0.1-rc37tarball and read bothdist/*.d.tsand the compileddist/exec.js. Findings that shaped the reconcile:Spriteexposes instance methodsspawn/exec/execFile/filesystem/getNetworkPolicy/updateNetworkPolicy/destroy— the driver's injectable seam already mirrored these.exec/execFilereturn a bufferedPromise<ExecResult>with no timeout and no abort handle; onlyspawnreturns aSpriteCommandwith.kill(signal)/.wait()/stdout/stderrstreams.Sprite.spawnauto-starts (confirmed inexec.js).ExecOptions.maxBuffer(default 10MB),SpriteConfig.storageGB, andPolicyRule.include(thedefaultspreset) all exist.What was impure / under-tested, and the fix
1. Purity (
buildSandboxEnv) — it readgetValidatedEnv()in a default param, the one IO in an otherwise-pure module. Nowenvis a required injected arg (fully pure, deterministic, never throws); thegetValidatedEnv()read moved todefaultBuildEnv, the effect seam intool-runners. Audited every other pure module (resolveExecutionPolicy,session-key,command-policy,sandbox-paths,output-limit, the audit-record builder,sandbox-options,tool-gatedecision logic,egress) — all already pure, noDate.now()/Math.random()/new Date()anywhere in source (clocks/ids injected).2. Hard wall-clock timeout via
kill('SIGKILL')— the old driver usedexecFile+ a race thatdestroy()ed the whole Sprite on timeout. SinceexecFileexposes no abort handle, the run now goes throughspawn(same structuredfile+args[]form → no host-side shell string) and a timerkill('SIGKILL')s the command, not the Sprite, so the warm conversation session survives a single slow run. Replicates the SDK's ownexecFilestream-collection logic. Tested with a non-terminating command.3.
maxBufferoutput cap — the policy output cap is now forwarded asmaxBytesand enforced during stream collection; an output flood SIGKILLs the command and fails the run (host-memory DoS guard). Tested with an over-cap output.4. Explicit
storageGBcap — addedstorageGbto the policy and mapped it ontoSpriteConfig(ramMB/cpus/storageGB/region), so every Sprite is provisioned with explicit caps instead of leaning on the quota default.5. Egress → SDK
{ include: 'defaults' }preset — replaced the hand-rolled internal-deny domain list with the SDK's maintaineddefaultspreset (the only internal-blocking lever the policy API exposes), prepended before any allow when the allowlist is widened. v1 empty-allowlist stays pure deny-all. Documented the known limitation that domain rules can't block IP-literal egress — that 6PN/metadata gate (G1) lives in the enablement checklist, not here.Tests
All pure fns + edge cases, plus specifically: hard-timeout SIGKILL (non-terminating cmd), output-cap SIGKILL, env-scrub leaks nothing (+ empty-env purity), command-policy block, path-traversal rejection, egress default-deny +
defaults+ allowlist ordering, resume re-authz denial, quota/concurrency denial, fail-closed on dep errors.@pagespace/lib: 194 files / 4523 tests green; sandbox: 16 files / 165 tests.typecheck+lintgreen for@pagespace/lib;webtypecheck green.@fly/spritespinAlready pinned to the exact
0.0.1-rc37(no range). Kept on rc37 deliberately: thelatest-tagged0.0.1is an older, smaller surface that lacks thefilesystemandupdateNetworkPolicyinstance methods this driver depends on (diffed both tarballs). rc37 is the only published version exposing the full surface.🤖 Generated with Claude Code
Self-review hardening (post-open)
A multi-angle review of the diff surfaced two memory-safety nits in the spawn output collector, both fixed: (a) stop retaining late
datachunks once the run has settled, and (b) enforce the output cap before retaining a chunk so a single oversized frame can't be buffered past the cap. Other review flags (timeout SIGKILLs the command not the Sprite; thedefaultspreset's contents) were confirmed as intended/by-design and documented above.