Fix/bug hunt round 2#271
Merged
Merged
Conversation
…t reconcile Running the full smoke suite against a dev stack surfaced two STALE-TEST issues (no product bugs — 0.9.1 passes all four suites): - Dynamic ports: the stack now picks a free sandbox port when :8080 is taken (e.g. Docker), landing on :8081. The scripts hardcoded CP/SB, so the /debug/exec + sandbox calls hit the wrong port. Make CP/SB env-overridable (same defaults) so `SB=http://127.0.0.1:8081 regression-smoke.sh` works. - Ghost-session #6: the ws-proxy reconcile has a deliberate 30s grace period after session creation (index.ts, "within grace period, not marking stopped") so a PTY 404 during provisioning doesn't kill the frontend retry loop. The test killed the PTY ~5s in — inside the grace window — so reconcile was correctly suppressed and it read active->active. Let the session age past the window (RECONCILE_GRACE=33s) before killing; verified active->GONE. Full suite now green: security 3/3, regression 12/12, first-prompt 10/10, browser 2/2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ
Security + robustness fixes from the round-2 hunt (verified live where noted):
- SECURITY (HIGH, verified live): cross-user recipe IDOR. Dashboard-less
("global") recipes were readable/editable/deletable/executable by ANY
authenticated user (checkRecipeAccess returned hasAccess:true for a null
dashboard_id, with no owner column). Add recipes.created_by (schema + ALTER
migration), populate it on create, scope global recipes to their owner in
checkRecipeAccess and listRecipes. Legacy null-owner rows fail closed.
- ROBUSTNESS (HIGH, verified live): ~45 endpoints returned 500 on a malformed
JSON body (unguarded request.json() → SyntaxError → top-level catch → 500).
Convert SyntaxError to 400 once at the top-level handler; add explicit
required-field validation to POST edges + POST members (shape 500s); clamp
NaN limit/offset in the integration + egress audit handlers.
- SECURITY (MEDIUM): egress allow_always/deny_always race could leave a domain
active in BOTH lists (later un-deny silently re-allowed it). allow_always now
refuses when an active deny exists; deny_always runs revoke+block atomically
via batch(). SQLite write-serialization makes them mutually exclusive.
- LEAK (MEDIUM): DashboardDO persisted its state blob (and kept collaborator
sockets) forever after dashboard delete — DOs can't be enumerated to sweep.
Add a /destroy route (deleteAll + close sockets); call it from deleteDashboard.
- RACE (MEDIUM): duplicate dashboard invitations and duplicate Telegram
subscriptions from concurrent check-then-insert. Both inserts are now atomic
(INSERT ... WHERE NOT EXISTS) and gate on meta.changes.
- RACE (MEDIUM): concurrent Stripe checkout created duplicate customers. Pass a
userId-keyed Stripe idempotency key so createCustomer dedupes server-side.
- LEAK (LOW): messaging fast-path left a dangling 1.5s setTimeout; clear it when
the resolves win the race.
Bump controlplane REVISION to v18-bughunt-round-2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ
Concurrency + resource-leak fixes (go build + go vet clean, package tests pass): - LEAK (HIGH, verified live): browser pre-warm orphaned a full Chromium stack (Xvfb+x11vnc+websockify+chromium, ~300MB) when a session was deleted within the 3s prewarm window — Close() read s.browser unlocked (data race) and no `closed` flag stopped a browser started after Close ran. Add a mutex-guarded `closed` flag; StartBrowser/OpenBrowserURL refuse once closed; Close reads/stops the browser under s.mu. (sessions/session.go) - RACE (MEDIUM): drive-sync refCount desync — attach/detach dispatched as bare goroutines with no ordering could run detach-before-attach, leaking a syncer. Dispatch synchronously in call order under the serializing lock. - DoS (LOW-MED): workspace tar.gz import had no decompressed-size/entry cap (gzip bomb). Add a 2 GiB decompressed cap + 100k entry cap. (cmd/server/main.go) - PATH SAFETY (LOW): resolvePath's lexical Abs fallback (non-existent parent) let an in-workspace symlink + missing intermediate escape via MkdirAll. Resolve the longest existing ancestor with EvalSymlinks + containment re-check. (internal/fs/workspace.go) - LEAK (LOW-MED): hub self-stop paths (idle/readLoopDone) skipped client-channel close + turn.Stop(). Unify via a deferred cleanup() run on all paths. - LEAK (LOW): broker loggedRoutes sync.Map never evicted per session; add EvictLoggedRoutesForSession, called from Manager.Delete. - LEAK (LOW): egress held CONNECT ignored client disconnect (pinned goroutine + conn for 60s). Thread request context; select on ctx.Done(). - ZOMBIES (LOW): browser Status() killed processes without Wait(); reap them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ
…k planting CRITICAL (Linux desktop) path-safety fix in the `orcabot import` command. `cmd_import` shelled out to system `tar -xzf` on an untrusted `.orcabot` bundle with zero path validation, then `cp -R` into the workspace. On GNU tar (Linux) a `../../../.ssh/authorized_keys` member escapes to the HOST filesystem; on all platforms a symlink member is planted into the workspace (macOS bsdtar blocks the `..` traversal but not the symlink). This is the exact asymmetry vs the hardened `pull` path next to it. Replace the shell-out + cp with in-process extraction (flate2 GzDecoder + tar::Archive) that validates every entry: - reject any path containing `..`, absolute, or a root/drive component; - reject symlink AND hardlink members outright (kills link planting); - only write regular files, through the existing safe_workspace_dest / safe_workspace_write O_NOFOLLOW component walk `pull` already uses; - cap total decompressed size (4 GiB) and entry count (200k) — zip-bomb guard. `tar` was already in the dependency tree transitively (via tauri-plugin-updater), so this adds no new crates. cargo check --bin orcabot passes. Also regenerates gen/schemas/capabilities.json to drop the stale allow-open-url entry left by PR #270's source change (was leaving every build dirty). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ
…cipes The recipe IDOR fix makes dashboard-less recipes owner-scoped via created_by. - seedRecipe now sets created_by (default 'user-1', the standard test user) so seeded global recipes are owned by the user the tests authenticate as. - The "access to recipes without dashboard_id" test asserted the OLD insecure behavior (readable by anyone). Rewrite it to verify the fix: the owner gets 200, a different user now gets 404. Restores the suite to 81/81 (13 recipe/schedule tests had regressed on the fix). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ
Shorten the explanatory comments added with the round-2 fixes and remove the internal "bug hunt" references / revision tag. No behavior change; 81/81 tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ
- deleteDashboard: purge the DashboardDO after the D1 delete succeeds, not before — so a concurrent authorized mutation can't re-persist DO state in the gap, and a failed D1 delete no longer leaves an existing dashboard with its DO cleared. - subscriptions: the Stripe customer-create idempotency key must not be permanent (Stripe replays a since-deleted customer / an initial 5xx for its ~24h key retention). Scope it to a 10-minute rotating window + a new/recreate generation marker, so only genuinely concurrent attempts share a key. - index: log the SyntaxError→400 body-parse guard so a mislabeled internal parse error isn't silent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ
…, stream files Two P1 issues found reviewing the earlier import hardening: - Files were written into the live workspace during extraction, before the manifest validated and the dashboard was created — a missing/corrupt manifest, truncated archive, or control-plane failure left the workspace half-overwritten. Rework to stage → validate → create → merge: extract to a private temp dir first (Drop-guarded cleanup on every exit), validate + create, then write into the live workspace via safe_workspace_write only at the end. The workspace is untouched on any pre-merge failure. - Archive-bomb caps didn't bound decompression (skipped/unsupported entries bypassed the per-file cap) and buffered each file into a 4 GiB Vec. Cap the gzip decoder itself (counts bytes drained to skip entries too) and stream file bodies to disk with io.copy; manifest.json keeps a small dedicated in-memory cap. Symlink/hardlink rejection, `..`/absolute rejection, entry-count cap, and the O_NOFOLLOW final writes are all preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ
…nux build - Workspace.Write used check-then-path-write (validate ancestor, then path-based MkdirAll/CreateTemp/Rename) — a workspace process could swap a validated ancestor for a symlink between check and write (TOCTOU), redirecting a root-owned write outside /workspace (host-shared virtiofs on desktop). Replace with a per-component openat + O_NOFOLLOW descriptor walk (new workspace_write_unix.go; mirrors the Rust pull path): every element is opened relative to its verified parent fd, so a raced symlink fails the open (ELOOP/ENOTDIR) — the TOCTOU is unwinnable. Atomic renameat preserved; non-unix fallback keeps the package building. New containment tests. - Fix a Linux-only build break introduced by the round-2 egress ctx-cancel change: transparent_linux.go still called the old 2-arg decide(); it's build-tagged linux-only so the macOS go build missed it. Pass context.Background() (the transparent path has no request context). Verified with GOOS=linux go build ./... Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ
Trim the verbose explanatory comments across the fixes to the minimal detail — one or two lines each. No behavior change; builds (linux + native Go, cargo, tsc) and tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ
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.
No description provided.