Repository navigation
Fix/combined code review fixes - #5
Merged
Merged
Conversation
Completed a full code review of the orcabot monorepo covering: - Frontend (React/Next.js) - Controlplane (Cloudflare Workers) - Sandbox (Go) Created 10 fix branches addressing: - 5 security issues (XSS, auth bypass, CORS, access control) - 3 resource leaks (goroutines, WebSocket cleanup) - 2 error handling issues See CODE_REVIEW_FINDINGS.md for full details and recommended merge order. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Security fixes: - Remove allow-same-origin from BrowserBlock iframe sandbox to prevent XSS (keep allow-popups-to-escape-sandbox for OAuth flows) - Fix CORS policy to properly reject null origins and use allowlist (ALLOWED_ORIGINS env var for production, wildcard in dev mode) Resource leak fixes: - Fix PTY.Done() goroutine leak by caching done channel with sync.Once - Fix Hub.readLoop() goroutine leak by adding readLoopDone signal channel for proper cleanup when PTY read fails Error handling fixes: - Add try-catch with logging context to useTerminal hook cleanup (logs which subscription cleanup failed for diagnostics) - Add .catch() handler for WebSocket Blob-to-ArrayBuffer conversion - Add explicit initPromise await in DurableObject.fetch() for defense-in-depth Note: proxy-userid-auth unchanged - empty string for non-owners correctly enforces owner-only input policy. Recipe access control unchanged - global recipes remain accessible to all authenticated users as intended. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
robdmac
added a commit
that referenced
this pull request
Jun 20, 2026
…validation 500s) #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) <noreply@anthropic.com>
robdmac
added a commit
that referenced
this pull request
Jun 20, 2026
* fix: 5 bugs found in the bug hunt (PTY leak, dup-session race, input-validation 500s) #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) <noreply@anthropic.com> * fix: #6 ghost/stale sessions + #7 dashboard-delete PTY leak #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) <noreply@anthropic.com> * test: regression coverage for the 7 bug-hunt findings #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) <noreply@anthropic.com> * desktop(vm): robust image staging stamp + document reverse-bridge port 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Fixes included:
BrowserBlock.tsx: Removed allow-same-origin, kept allow-popups-to-escape-sandbox for OAuth
controlplane/index.ts: CORS validation with ALLOWED_ORIGINS env, rejects null origins
controlplane/types.ts: Added ALLOWED_ORIGINS to Env interface
useTerminal.ts: Try/catch with logging for each cleanup subscription
BaseWebSocketManager.ts: Added .catch() for Blob conversion errors
pty.go: Fixed Done() goroutine leak with sync.Once
hub.go: Fixed readLoop leak with readLoopDone signal channel
DurableObject.ts: Added explicit initPromise await in fetch()