From 957e58cc4f1e6b2cbbf833bf2f8d6bc543848129 Mon Sep 17 00:00:00 2001 From: Rob Macrae Date: Fri, 19 Jun 2026 12:42:36 +0100 Subject: [PATCH 1/4] fix: 5 bugs found in the bug hunt (PTY leak, dup-session race, input-validation 500s) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1 [sandbox] Orphaned PTY/zombie leak on terminal delete. PTY.Close() SIGKILL'd the process but only reaped (cmd.Wait) for pool slots, so non-pool PTYs (desktop / any non-egress deploy) leaked a zombie every delete. Now reap unconditionally and kill the whole process group (negative pid) so agent children (node/chromium) die too. (pty.go; sandbox revision -> main-v35-pty-close-reap) #2 [controlplane] Duplicate sessions/PTYs on concurrent create. The reuse check is check-then-act and races; 3 concurrent creates → 3 PTYs. Added a per-item in-flight gate that serializes creates within the isolate (createSession wraps createSessionImpl). #3 [controlplane] POST /dashboards/:id/items with invalid JSON → 500. Now 400. #4 [controlplane] attach with malformed body / unknown provider → 500. Wrapped json parse + provider validation + attachIntegration in try/catch → 4xx, never 500. #5 [controlplane] POST item with missing/invalid type → 201 (junk item). Now validated against the item-type union → 400; valid types still 201. Verified end-to-end via the orcabot CLI + /debug/exec: - #1 create→delete: process GONE, zombies=0 after churn. - #2 3 concurrent creates → exactly 1 session. - #3/#5 invalid JSON / missing / bogus type → 400; valid note → 201. - #4 malformed body / bogus / valid-no-account → 400/403/404, no 500. Co-Authored-By: Claude Opus 4.8 (1M context) --- controlplane/src/index.ts | 36 +++++++++++++++++++++++++--- controlplane/src/sessions/handler.ts | 31 ++++++++++++++++++++++++ sandbox/cmd/server/main.go | 4 ++-- sandbox/internal/pty/pty.go | 26 +++++++++++++------- 4 files changed, 84 insertions(+), 13 deletions(-) diff --git a/controlplane/src/index.ts b/controlplane/src/index.ts index 1a63e0ed..e4bbfe9c 100644 --- a/controlplane/src/index.ts +++ b/controlplane/src/index.ts @@ -1360,7 +1360,20 @@ async function handleRequest(request: Request, env: EnvWithBindings, ctx: Pick; + let data: Partial; + try { + data = await request.json() as Partial; + } catch { + return Response.json({ error: 'E79305: Invalid JSON body' }, { status: 400 }); + } + const VALID_ITEM_TYPES = new Set([ + 'note', 'todo', 'terminal', 'link', 'browser', 'workspace', 'prompt', 'schedule', + 'gmail', 'calendar', 'contacts', 'sheets', 'forms', 'slack', 'discord', 'telegram', + 'whatsapp', 'teams', 'matrix', 'google_chat', 'twitter', 'outlook', + ]); + if (!data || typeof data.type !== 'string' || !VALID_ITEM_TYPES.has(data.type)) { + return Response.json({ error: 'E79306: Missing or invalid item type' }, { status: 400 }); + } return dashboards.upsertItem(env, segments[1], auth.user!.id, data); } @@ -1690,14 +1703,31 @@ async function handleRequest(request: Request, env: EnvWithBindings, ctx: Pick; accountLabel?: string; highRiskConfirmations?: string[]; }; - return integrationPolicies.attachIntegration(env, segments[1], segments[3], auth.user!.id, data as Parameters[4]); + try { + data = await request.json() as typeof data; + } catch { + return Response.json({ error: 'Invalid JSON body' }, { status: 400 }); + } + if (!data || typeof data.provider !== 'string' || !data.provider) { + return Response.json({ error: 'Missing or invalid provider' }, { status: 400 }); + } + try { + return await integrationPolicies.attachIntegration(env, segments[1], segments[3], auth.user!.id, data as Parameters[4]); + } catch (e) { + // attachIntegration throws on an unknown provider (unguarded switch) — that's + // bad input, not a server fault. Surface as 400 rather than 500. + return Response.json( + { error: `Invalid integration request: ${e instanceof Error ? e.message : 'unknown'}` }, + { status: 400 } + ); + } } // PUT /dashboards/:id/terminals/:terminalId/integrations/:provider - Update integration policy diff --git a/controlplane/src/sessions/handler.ts b/controlplane/src/sessions/handler.ts index 6c570576..9f34f04d 100644 --- a/controlplane/src/sessions/handler.ts +++ b/controlplane/src/sessions/handler.ts @@ -1229,6 +1229,15 @@ async function isExistingSessionReusable( } // Create a session for a terminal item +// Per-item in-flight gate: coalesce concurrent session creates for the same +// terminal. The reuse check in the impl is check-then-act, so concurrent first +// creates race past it and each provisions a PTY (duplicate PTYs/leak). This +// serializes creates per item within the worker isolate (desktop = single +// isolate, so fully effective). The while-loop has no await between the final +// has()-check and set(), so exactly one caller becomes the leader at a time; +// waiters fall through to the impl's reuse check and return the existing session. +const sessionCreateGates = new Map>(); + export async function createSessiоn( env: EnvWithDriveCache, dashboardId: string, @@ -1237,6 +1246,28 @@ export async function createSessiоn( userName: string, preferredRegion?: string, ctx?: Pick +): Promise { + while (sessionCreateGates.has(itemId)) { + await sessionCreateGates.get(itemId)!.catch(() => {}); + } + let release!: () => void; + sessionCreateGates.set(itemId, new Promise((r) => { release = r; })); + try { + return await createSessionImpl(env, dashboardId, itemId, userId, userName, preferredRegion, ctx); + } finally { + sessionCreateGates.delete(itemId); + release(); + } +} + +async function createSessionImpl( + env: EnvWithDriveCache, + dashboardId: string, + itemId: string, + userId: string, + userName: string, + preferredRegion?: string, + ctx?: Pick ): Promise { // Check access const access = await env.DB.prepare(` diff --git a/sandbox/cmd/server/main.go b/sandbox/cmd/server/main.go index 7b81aed5..3786a5e6 100644 --- a/sandbox/cmd/server/main.go +++ b/sandbox/cmd/server/main.go @@ -1,7 +1,7 @@ // Copyright 2026 Rob Macrae. All rights reserved. // SPDX-License-Identifier: LicenseRef-Proprietary -// REVISION: main-v34-debug-exec-token +// REVISION: main-v35-pty-close-reap package main @@ -38,7 +38,7 @@ import ( "github.com/Hyper-Int/OrcaBot/sandbox/internal/ws" ) -const mainRevision = "main-v34-debug-exec-token" +const mainRevision = "main-v35-pty-close-reap" // debugExecToken is a random per-boot secret guarding POST /debug/exec. Generated // once at startup when ORCABOT_DEBUG_EXEC=1 and delivered to the host ONLY via the diff --git a/sandbox/internal/pty/pty.go b/sandbox/internal/pty/pty.go index 95ff8366..de9c1622 100644 --- a/sandbox/internal/pty/pty.go +++ b/sandbox/internal/pty/pty.go @@ -339,22 +339,32 @@ func (p *PTY) Close() error { } p.closed = true - // Kill the process if still running + // Kill the process if still running. Target the whole process group (negative + // PID) so agent children (node, chromium, etc.) die too — the PTY leader is a + // session/group leader via creack/pty, so pid == pgid. Fall back to the single + // process if it isn't a group leader. if p.cmd.Process != nil { - p.cmd.Process.Kill() + pid := p.cmd.Process.Pid + if err := syscall.Kill(-pid, syscall.SIGKILL); err != nil { + _ = p.cmd.Process.Kill() + } } - // Return the pool slot only after cmd.Wait() has reaped the zombie. + // Reap the process so it doesn't linger as a zombie. Done() starts cmd.Wait() + // via doneOnce (idempotent). This MUST run in all modes — previously it was + // gated behind the pool-slot path, so non-pool PTYs (e.g. desktop, or any + // non-egress deployment) leaked a zombie on every delete. + doneCh := p.Done() + + // For pool slots, return the slot only after cmd.Wait() has reaped the zombie. // pool.Release() uses hasProcessesForUID(/proc scan) to decide whether the slot - // is clean; a zombie remains in /proc until waited on, so calling Release() - // before Wait() permanently contaminates the slot. - // Done() starts cmd.Wait() via doneOnce (idempotent); we wait for it here. + // is clean; a zombie remains in /proc until waited on, so releasing before the + // reap would permanently contaminate the slot. if p.slotUID != 0 { if pool := GetPool(); pool != nil { uid := p.slotUID - doneCh := p.Done() // starts cmd.Wait() goroutine — idempotent go func() { - <-doneCh // zombie reaped; /proc entry gone + <-doneCh // zombie reaped; /proc entry gone pool.Release(uid) }() } From 3e8e7952dccdc3ec7ebb10c6dc6c958689c055b1 Mon Sep 17 00:00:00 2001 From: Rob Macrae Date: Fri, 19 Jun 2026 13:25:46 +0100 Subject: [PATCH 2/4] fix: #6 ghost/stale sessions + #7 dashboard-delete PTY leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #7 [HIGH] deleteDashboard never stopped its sessions, so DELETE /dashboards/:id orphaned live PTYs (shells + agent children) in the sandbox. Now stops all active/creating sessions (→ kills their PTYs via stopSession, which reaps after the #1 fix) before deleting, mirroring deleteItem. Verified: dashboard delete reaps the PTY (no leak, no zombie). #6 [MED] Ghost sessions: when a PTY died (crash/OOM/agent exit/VM restart) the control plane kept status='active' forever (ls showed 'running', attach 404'd, "stuck connecting"). The WS proxy now reconciles: on a 404 from the sandbox it marks the session stopped (by pty_id), so listings reflect reality and the client recreates instead of hanging. Verified: kill PTY → attach → session→stopped, terminal shows 'idle' not 'running'. Co-Authored-By: Claude Opus 4.8 (1M context) --- controlplane/src/dashboards/handler.ts | 17 +++++++++++++++++ controlplane/src/index.ts | 13 +++++++++++++ 2 files changed, 30 insertions(+) diff --git a/controlplane/src/dashboards/handler.ts b/controlplane/src/dashboards/handler.ts index 5a0aab83..da8d1313 100644 --- a/controlplane/src/dashboards/handler.ts +++ b/controlplane/src/dashboards/handler.ts @@ -270,6 +270,23 @@ export async function deleteDashbоard( return Response.json({ error: 'E79304: Not found or not owner' }, { status: 404 }); } + // Stop any active sessions first so their PTYs are killed in the sandbox. + // CASCADE-deleting the session rows would otherwise orphan live processes + // (shells, and agent children like node/chromium) in the VM. Mirrors deleteItem. + try { + const activeSessions = await env.DB.prepare(` + SELECT id FROM sessions WHERE dashboard_id = ? AND status IN ('creating', 'active') + `).bind(dashboardId).all<{ id: string }>(); + if (activeSessions.results.length > 0) { + const { stоpSessiоn } = await import('../sessions/handler'); + for (const session of activeSessions.results) { + await stоpSessiоn(env as EnvWithDriveCache, session.id, userId); + } + } + } catch { + // Best-effort — don't block dashboard deletion if session cleanup fails. + } + // Delete dependent records that don't have ON DELETE CASCADE // Order matters: delete from most dependent tables first diff --git a/controlplane/src/index.ts b/controlplane/src/index.ts index e4bbfe9c..5c78cb9b 100644 --- a/controlplane/src/index.ts +++ b/controlplane/src/index.ts @@ -374,6 +374,19 @@ async function prоxySandbоxWebSоcket( console.warn( `[ws-proxy] upstream PTY WS failed status=${response.status} dashboardPath=${requestUrl.pathname} sandboxSessionId=${sandboxSessionId} ptyId=${ptyId} machineId=${machineId || ''} origin=${origin}` ); + // Reconcile: the sandbox no longer has this PTY (process died, or the VM was + // restarted), but D1 may still show the session 'active' ("ghost" session → + // stale 'running' status + "stuck connecting"). Mark it stopped so the client + // recreates instead of hanging, and listings reflect reality. Best-effort. + if (response.status === 404) { + try { + await env.DB.prepare( + `UPDATE sessions SET status = 'stopped', stopped_at = ? WHERE pty_id = ? AND status != 'stopped'` + ).bind(new Date().toISOString(), ptyId).run(); + } catch { + // ignore — reconciliation is best-effort + } + } } else { console.log( `[ws-proxy] upstream PTY WS upgraded sandboxSessionId=${sandboxSessionId} ptyId=${ptyId} machineId=${machineId || ''} origin=${origin}` From d77f50322e6e0aadee104c7a28eddf8fdeabca32 Mon Sep 17 00:00:00 2001 From: Rob Macrae Date: Fri, 19 Jun 2026 14:09:52 +0100 Subject: [PATCH 3/4] test: regression coverage for the 7 bug-hunt findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1 gets fast Go unit tests; the stateful/cross-component bugs (#2-#7) get a live integration smoke, because the control-plane mock-D1 harness can't represent them (it ignores UPDATE...SET and mis-maps session inserts — it's documented as "full integration tests should use a real D1"). - sandbox/internal/pty/pty_test.go: - TestPTYCloseReapsProcess — Close() kills AND reaps (no zombie); guards #1's unconditional reap (was pool-only). - TestPTYCloseKillsProcessGroup — Close() kills the process group so in-group children die too (guards the group-kill; job-control/setsid escapees noted). - desktop/tests/regression-smoke.sh: drives the live stack via the orcabot CLI + /debug/exec and asserts #1-#7 stay fixed, plus invariants (path-traversal blocked, broker-secret output redaction). 12 checks, exits non-zero on any failure. Not in the fast PR lane (needs the desktop VM) — run locally / nightly. Verified: go test ./internal/pty green; smoke 12/12 passing. Co-Authored-By: Claude Opus 4.8 (1M context) --- desktop/tests/regression-smoke.sh | 158 ++++++++++++++++++++++++++++++ sandbox/internal/pty/pty_test.go | 96 ++++++++++++++++++ 2 files changed, 254 insertions(+) create mode 100755 desktop/tests/regression-smoke.sh diff --git a/desktop/tests/regression-smoke.sh b/desktop/tests/regression-smoke.sh new file mode 100755 index 00000000..0443ef4e --- /dev/null +++ b/desktop/tests/regression-smoke.sh @@ -0,0 +1,158 @@ +#!/usr/bin/env bash +# Orcabot desktop regression smoke test. +# +# Drives the LIVE desktop stack (control plane :8787, sandbox :8080) the same way +# the `orcabot` CLI does and asserts that the bugs found in the bug hunt stay fixed. +# These are stateful / cross-component behaviors that the control-plane mock-D1 unit +# harness can't represent faithfully (it ignores UPDATEs and mis-maps session inserts), +# so they're covered here against real processes + real D1. +# +# Coverage: +# #1 deleting a terminal reaps its PTY (no zombie / no orphan) +# #2 concurrent session-create for one item -> exactly one session +# #3 POST item with invalid JSON -> 400 (not 500) +# #4 attach with unknown provider -> 4xx (not 500) +# #5 POST item with missing/invalid type -> 400; valid -> 201 +# #6 ghost session: PTY death -> status reconciled to stopped (not stuck 'running') +# #7 dashboard delete reaps its terminals' PTYs +# + invariants: file-API path traversal blocked; broker-secret output redaction +# +# Usage: desktop/tests/regression-smoke.sh +# Requires: the stack running (`orcabot up`), python3, curl. Exits non-zero on any failure. +# NOTE: not part of the fast PR lane (needs the desktop VM). Run locally / nightly. + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +ORCABOT="$SCRIPT_DIR/../app/src-tauri/target/release/orcabot" +CP="http://127.0.0.1:8787" +SB="http://127.0.0.1:8080" +U="X-User-ID: dev-desktop" +VZ_LOG="/tmp/vz-console.log" + +PASS=0 +FAIL=0 +ok() { echo " PASS: $1"; PASS=$((PASS + 1)); } +bad() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); } + +jq_py() { python3 -c "$1"; } +code() { curl -s -o /dev/null -w "%{http_code}" -m 10 "$@"; } + +TOK="$(grep -a 'debug-exec] auth token:' "$VZ_LOG" 2>/dev/null | tail -1 | sed 's/.*auth token: //' | tr -cd 'a-f0-9')" +# Run a shell command in the sandbox VM, print stdout. +X() { + local body + body="$(python3 -c "import json,sys;print(json.dumps({'cmd':sys.argv[1],'timeout_ms':8000}))" "$1")" + curl -s -m 12 -X POST -H "X-Debug-Exec-Token: $TOK" -H 'Content-Type: application/json' \ + -d "$body" "$SB/debug/exec" | python3 -c "import json,sys;print(json.load(sys.stdin).get('stdout','').strip(),end='')" +} + +# ---- preflight ------------------------------------------------------------ +if ! curl -s -m 3 "$CP/health" >/dev/null; then + echo "control plane not reachable on :8787 — run 'orcabot up' first." >&2 + exit 2 +fi +if [ -z "$TOK" ]; then + echo "no /debug/exec token in $VZ_LOG — stack must be launched with VZ_CONSOLE_DIRECT=1 (orcabot up does this)." >&2 + exit 2 +fi + +# dedicated throwaway dashboard +DID="$(curl -s -X POST -H "$U" -H 'Content-Type: application/json' --data-raw '{"name":"regression-smoke"}' "$CP/dashboards" \ + | jq_py "import json,sys;print(json.load(sys.stdin)['dashboard']['id'])")" +echo "smoke dashboard: $DID" +new_term() { "$ORCABOT" new terminal shell --dash "$DID" 2>/dev/null | sed 's/created shell terminal //'; } +sess_field() { # $1=itemId $2=field + curl -s -H "$U" "$CP/dashboards/$DID" | jq_py "import json,sys;d=json.load(sys.stdin);print(next((s.get('$2','') for s in d['sessions'] if s['itemId']=='$1'),'GONE'))" +} + +# ---- #1 delete reaps PTY (no zombie) -------------------------------------- +echo "[#1] terminal delete reaps the PTY" +b0="$(X 'pgrep -c bash')"; ITEM="$(new_term)"; sleep 3 +b1="$(X 'pgrep -c bash')" +curl -s -o /dev/null -X DELETE -H "$U" "$CP/dashboards/$DID/items/$ITEM"; sleep 3 +b2="$(X 'pgrep -c bash')"; z="$(X 'ps -eo stat | grep -c Z')" +{ [ "$b1" -gt "$b0" ] && [ "$b2" -le "$b0" ] && [ "$z" -eq 0 ]; } \ + && ok "PTY reaped (bash $b0->$b1->$b2, zombies=$z)" \ + || bad "PTY leaked/zombied (bash $b0->$b1->$b2, zombies=$z)" + +# ---- #2 concurrent session-create -> one session -------------------------- +echo "[#2] concurrent session-create coalesces to one" +IT="$(curl -s -X POST -H "$U" -H 'Content-Type: application/json' \ + --data-raw '{"type":"terminal","content":"{\"name\":\"race\",\"bootCommand\":\"\"}"}' "$CP/dashboards/$DID/items" \ + | jq_py "import json,sys;print(json.load(sys.stdin)['item']['id'])")" +for i in 1 2 3; do curl -s -o /dev/null -X POST -H "$U" -H 'Content-Type: application/json' -d '{}' "$CP/dashboards/$DID/items/$IT/session" & done; wait +sleep 3 +n="$(curl -s -H "$U" "$CP/dashboards/$DID" | jq_py "import json,sys;print(sum(1 for s in json.load(sys.stdin)['sessions'] if s['itemId']=='$IT'))")" +[ "$n" = "1" ] && ok "exactly one session ($n)" || bad "duplicate sessions ($n)" +curl -s -o /dev/null -X DELETE -H "$U" "$CP/dashboards/$DID/items/$IT" + +# ---- #3/#5 item input validation ------------------------------------------ +echo "[#3/#5] item input validation" +c="$(code -X POST -H "$U" -H 'Content-Type: application/json' --data-raw 'not-json{' "$CP/dashboards/$DID/items")" +[ "$c" = "400" ] && ok "invalid JSON -> 400" || bad "invalid JSON -> $c (want 400)" +c="$(code -X POST -H "$U" -H 'Content-Type: application/json' --data-raw '{"content":"x"}' "$CP/dashboards/$DID/items")" +[ "$c" = "400" ] && ok "missing type -> 400" || bad "missing type -> $c (want 400)" +c="$(code -X POST -H "$U" -H 'Content-Type: application/json' --data-raw '{"type":"bogus"}' "$CP/dashboards/$DID/items")" +[ "$c" = "400" ] && ok "invalid type -> 400" || bad "invalid type -> $c (want 400)" +c="$(code -X POST -H "$U" -H 'Content-Type: application/json' --data-raw '{"type":"note","content":"ok"}' "$CP/dashboards/$DID/items")" +[ "$c" = "201" ] && ok "valid note -> 201" || bad "valid note -> $c (want 201)" + +# ---- #4 attach validation (no 500) ---------------------------------------- +echo "[#4] attach input validation (never 500)" +ITEM4="$(new_term)"; sleep 3 +PTY4="$(sess_field "$ITEM4" ptyId)" +c="$(code -X POST -H "$U" -H 'Content-Type: application/json' --data-raw 'not-json{' "$CP/dashboards/$DID/terminals/$PTY4/integrations")" +{ [ "$c" -ge 400 ] && [ "$c" -lt 500 ]; } && ok "malformed body -> $c (4xx)" || bad "malformed body -> $c (want 4xx)" +c="$(code -X POST -H "$U" -H 'Content-Type: application/json' --data-raw '{"provider":"bogus","userIntegrationId":"fake"}' "$CP/dashboards/$DID/terminals/$PTY4/integrations")" +{ [ "$c" -ge 400 ] && [ "$c" -lt 500 ]; } && ok "unknown provider -> $c (4xx)" || bad "unknown provider -> $c (want 4xx)" + +# ---- #6 ghost session reconcile ------------------------------------------- +echo "[#6] PTY death reconciles session to stopped" +before="$(X 'pgrep bash | sort | tr "\n" ","')" +ITEM6="$(new_term)"; sleep 3 +after="$(X 'pgrep bash | sort | tr "\n" ","')" +PID6="$(python3 -c "b=set('$before'.split(','))-{''};a=set('$after'.split(','))-{''};print(next(iter(a-b),''))")" +X "kill -9 $PID6 2>/dev/null; echo ." >/dev/null; sleep 1 +st_before="$(sess_field "$ITEM6" status)" +timeout 8 "$ORCABOT" tail "$ITEM6" --dash "$DID" --secs 2 >/dev/null 2>&1 # 404 -> reconcile +sleep 2 +st_after="$(sess_field "$ITEM6" status)" +{ [ "$st_before" = "active" ] && { [ "$st_after" = "stopped" ] || [ "$st_after" = "GONE" ]; }; } \ + && ok "reconciled ($st_before -> $st_after)" \ + || bad "not reconciled ($st_before -> $st_after; want active -> stopped/GONE)" +curl -s -o /dev/null -X DELETE -H "$U" "$CP/dashboards/$DID/items/$ITEM6" +curl -s -o /dev/null -X DELETE -H "$U" "$CP/dashboards/$DID/items/$ITEM4" + +# ---- #7 dashboard delete reaps PTYs --------------------------------------- +echo "[#7] dashboard delete reaps its PTYs" +ND="$(curl -s -X POST -H "$U" -H 'Content-Type: application/json' --data-raw '{"name":"regression-smoke-del"}' "$CP/dashboards" \ + | jq_py "import json,sys;print(json.load(sys.stdin)['dashboard']['id'])")" +d0="$(X 'pgrep -c bash')"; "$ORCABOT" new terminal shell --dash "$ND" >/dev/null 2>&1; sleep 3 +curl -s -o /dev/null -X DELETE -H "$U" "$CP/dashboards/$ND"; sleep 3 +d2="$(X 'pgrep -c bash')"; z="$(X 'ps -eo stat | grep -c Z')" +{ [ "$d2" -le "$d0" ] && [ "$z" -eq 0 ]; } && ok "PTY reaped on dashboard delete (bash $d0->$d2, zombies=$z)" \ + || bad "PTY leaked on dashboard delete (bash $d0->$d2, zombies=$z)" + +# ---- invariants: path traversal + output redaction ------------------------ +echo "[inv] file-API path traversal blocked" +ITEM8="$(new_term)"; sleep 3 +SSID="$(curl -s -H "$U" "$CP/dashboards/$DID" | jq_py "import json,sys;d=json.load(sys.stdin);print(next(s['sandboxSessionId'] for s in d['sessions'] if s['itemId']=='$ITEM8'))")" +out="$(curl -s -m6 -H 'X-Internal-Token: dev-sandbox-token' "$SB/sessions/$SSID/file?path=../../../../etc/passwd")" +echo "$out" | grep -q "root:" && bad "path traversal LEAKED /etc/passwd" || ok "traversal blocked" + +echo "[inv] broker-secret output redaction" +SEC="REDACTME_smoke_55512345" +curl -s -o /dev/null -X POST -H "$U" -H 'Content-Type: application/json' \ + --data-raw "{\"name\":\"SMOKE_SECRET\",\"value\":\"$SEC\",\"type\":\"secret\",\"brokerProtected\":true,\"dashboardId\":\"$DID\"}" "$CP/secrets" +ITEM9="$(new_term)"; sleep 3 +red="$(timeout 12 "$ORCABOT" tail "$ITEM9" --dash "$DID" --secs 5 --send "echo MK:${SEC}:MK" 2>/dev/null)" +echo "$red" | grep -q "$SEC" && bad "secret LEAKED in output" || ok "secret redacted" +curl -s -o /dev/null -X DELETE -H "$U" "$CP/dashboards/$DID/items/$ITEM8" +curl -s -o /dev/null -X DELETE -H "$U" "$CP/dashboards/$DID/items/$ITEM9" + +# ---- teardown ------------------------------------------------------------- +curl -s -o /dev/null -X DELETE -H "$U" "$CP/dashboards/$DID" +echo +echo "================ $PASS passed, $FAIL failed ================" +[ "$FAIL" -eq 0 ] diff --git a/sandbox/internal/pty/pty_test.go b/sandbox/internal/pty/pty_test.go index ce52fccf..0a35ff6f 100644 --- a/sandbox/internal/pty/pty_test.go +++ b/sandbox/internal/pty/pty_test.go @@ -3,6 +3,9 @@ package pty import ( "bytes" "io" + "regexp" + "strconv" + "syscall" "testing" "time" ) @@ -128,3 +131,96 @@ func TestPTYSignal(t *testing.T) { t.Fatalf("failed to send signal: %v", err) } } + +// TestPTYCloseReapsProcess guards the zombie-leak regression (bug #1): Close() +// must kill AND reap the process in ALL modes (previously cmd.Wait() only ran for +// pool slots, so non-pool PTYs left a zombie on every delete). A zombie still +// satisfies kill(pid,0)==nil; only after the parent reaps it does the PID become +// ESRCH. So we poll for ESRCH after Close(). +func TestPTYCloseReapsProcess(t *testing.T) { + p, err := New("/bin/sh", 80, 24) + if err != nil { + t.Fatalf("failed to create PTY: %v", err) + } + pid := p.cmd.Process.Pid + + if err := p.Close(); err != nil { + t.Fatalf("failed to close: %v", err) + } + + deadline := time.Now().Add(5 * time.Second) + for { + if err := syscall.Kill(pid, 0); err == syscall.ESRCH { + return // killed and reaped — no zombie + } + if time.Now().After(deadline) { + t.Fatalf("process %d still present after Close — not reaped (zombie/leak regression)", pid) + } + time.Sleep(50 * time.Millisecond) + } +} + +// TestPTYCloseKillsProcessGroup guards the orphaned-children regression (bug #1): +// Close() must kill the whole process group, not just the leader, so agent children +// (node/chromium, here a backgrounded sleep) die too instead of orphaning. +func TestPTYCloseKillsProcessGroup(t *testing.T) { + p, err := New("/bin/sh", 80, 24) + if err != nil { + t.Fatalf("failed to create PTY: %v", err) + } + + // Spawn a child inside the PTY's process group and capture its real PID. + if _, err := p.Write([]byte("set +m; sleep 300 & echo CHILDPID=$! END\n")); err != nil { + p.Close() + t.Fatalf("failed to write: %v", err) + } + + childRe := regexp.MustCompile(`CHILDPID=(\d+) END`) + childPID := 0 + matchCh := make(chan int, 1) + go func() { + buf := make([]byte, 4096) + var acc []byte + for { + n, err := p.Read(buf) + if n > 0 { + acc = append(acc, buf[:n]...) + if m := childRe.FindSubmatch(acc); m != nil { + pid, _ := strconv.Atoi(string(m[1])) + matchCh <- pid + return + } + } + if err != nil { + matchCh <- 0 + return + } + } + }() + select { + case childPID = <-matchCh: + case <-time.After(5 * time.Second): + p.Close() + t.Fatal("timeout waiting for child PID") + } + if childPID == 0 { + p.Close() + t.Fatal("could not determine child PID") + } + + if err := p.Close(); err != nil { + t.Fatalf("failed to close: %v", err) + } + + // The backgrounded child must be killed with the group. + deadline := time.Now().Add(5 * time.Second) + for { + if err := syscall.Kill(childPID, 0); err == syscall.ESRCH { + return // child died with the group + } + if time.Now().After(deadline) { + t.Fatalf("child process %d survived Close — process group not killed (orphan regression)", childPID) + } + time.Sleep(50 * time.Millisecond) + } +} From 241b1b2a2de2cdaea254d6aabb58f0207d1d1ea1 Mon Sep 17 00:00:00 2001 From: Rob Macrae Date: Sat, 20 Jun 2026 16:27:04 +0100 Subject: [PATCH 4/4] desktop(vm): robust image staging stamp + document reverse-bridge port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two findings from the fix/desktop-on-mac review: - P3 (image.rs): the staging stamp used seconds-resolution mtime + size, so a rebuild landing in the same wall-clock second as the previous one (fast SKIP_KERNEL iterations; raw sandbox.img keeps a constant size) could be treated as "unchanged" and silently boot a stale image. Use nanosecond mtime. (A content hash would be more robust but hashing a multi-GB image every launch is too slow.) - P2 (macos.rs): replace the hard-coded "8787:8787" reverse-port-forward with a named CONTROLPLANE_PORT const + a doc explaining why it's pinned on both sides (secure per-boot port delivery to the guest isn't possible yet). NOT fixed — P1 was a FALSE POSITIVE. The review (and my own first code-read) assumed the guest boots the minimal init in build-images.sh, which lacks the reverse control-plane bridge. But at runtime the Debian rootfs's systemd is PID 1 and runs /etc/rc.local (the MININIT is vestigial/dead). rc.local already starts `socat TCP-LISTEN:8787 -> VSOCK-CONNECT:2:8787` and sets CONTROLPLANE_URL. Verified live on the VM: rc-local.service runs, the reverse socat is listening, guest->control-plane /health returns 200, and the server has CONTROLPLANE_URL=http://127.0.0.1:8787. So no init change is needed. Co-Authored-By: Claude Opus 4.8 (1M context) --- desktop/app/src-tauri/src/vm/image.rs | 12 +++++++++--- desktop/app/src-tauri/src/vm/macos.rs | 18 +++++++++++++++--- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/desktop/app/src-tauri/src/vm/image.rs b/desktop/app/src-tauri/src/vm/image.rs index 30615f4f..21bc40ba 100644 --- a/desktop/app/src-tauri/src/vm/image.rs +++ b/desktop/app/src-tauri/src/vm/image.rs @@ -48,15 +48,21 @@ pub fn stage_image(src: &Path, dest: &Path) -> Result { } /// Stable signature of the SOURCE file (modification time + size). +/// +/// Uses NANOSECOND mtime, not seconds: a rebuild that lands in the same wall-clock +/// second as the previous one (fast `SKIP_KERNEL` iterations) keeps the same size +/// for the raw `sandbox.img`, so a seconds-resolution stamp would treat it as +/// "unchanged" and silently boot a stale image. A content hash would be more +/// robust still, but hashing a multi-GB image on every launch is too slow. fn source_signature(src: &Path) -> Result { let meta = fs::metadata(src)?; - let mtime = meta + let mtime_nanos = meta .modified() .ok() .and_then(|t| t.duration_since(SystemTime::UNIX_EPOCH).ok()) - .map(|d| d.as_secs()) + .map(|d| d.as_nanos()) .unwrap_or(0); - Ok(format!("{}:{}", mtime, meta.len())) + Ok(format!("{}:{}", mtime_nanos, meta.len())) } /// Path of the sidecar stamp file recording the source signature at last stage. diff --git a/desktop/app/src-tauri/src/vm/macos.rs b/desktop/app/src-tauri/src/vm/macos.rs index 7084ec91..a7d38a9c 100644 --- a/desktop/app/src-tauri/src/vm/macos.rs +++ b/desktop/app/src-tauri/src/vm/macos.rs @@ -16,6 +16,18 @@ use std::path::Path; use std::process::{Child, Command, Stdio}; use std::time::{Duration, Instant}; +/// Control-plane port for the guest→host reverse vsock bridge. +/// +/// Fixed at 8787 on BOTH sides on purpose: the host registers +/// `--reverse-port-forward {CONTROLPLANE_PORT}:{CONTROLPLANE_PORT}` and the guest +/// (systemd → `/etc/rc.local`, see `vm/scripts/build-images.sh`) runs the matching +/// `socat TCP-LISTEN:8787 -> VSOCK-CONNECT:2:8787` + sets `CONTROLPLANE_URL`. The +/// guest value is baked into the image, and we can't securely deliver a per-boot +/// port to the guest (kernel cmdline / workspace are agent-readable, so the token +/// can't ride along). Making this configurable end-to-end requires a secure guest +/// env channel first; until then both sides must use this constant. +const CONTROLPLANE_PORT: u16 = 8787; + /// macOS VM using Virtualization.framework. /// /// On macOS 13+, uses native Virtualization.framework for optimal performance. @@ -144,10 +156,10 @@ impl MacOSVM { // Reverse forward: guest vsock:8787 -> host 127.0.0.1:8787 (control plane). // Gives the sandbox a guest->host route so it can call the control plane // (integration gateway, egress/secret approvals, event callbacks). The - // guest runs socat TCP-LISTEN:8787 -> VSOCK-CONNECT:2:8787 and sets - // CONTROLPLANE_URL=http://127.0.0.1:8787. (8787 = default CONTROLPLANE_PORT.) + // guest init runs the matching socat + sets CONTROLPLANE_URL. Both sides + // are pinned to CONTROLPLANE_PORT (see the const for why it's fixed). "--reverse-port-forward", - "8787:8787", + &format!("{}:{}", CONTROLPLANE_PORT, CONTROLPLANE_PORT), ]); cmd.stdout(Stdio::inherit());