diff --git a/controlplane/src/auth/access.ts b/controlplane/src/auth/access.ts index c5410056..340d816f 100644 --- a/controlplane/src/auth/access.ts +++ b/controlplane/src/auth/access.ts @@ -65,9 +65,12 @@ export async function checkRecipеAccess( return { hasAccess: false }; } - // Recipes without dashboard_id are accessible to any authenticated user + // Dashboard-less ("global") recipes are scoped to their creator, not to every + // authenticated user. Legacy rows with a null created_by are inaccessible + // (fail-closed). if (!recipe.dashboard_id) { - return { hasAccess: true, recipe }; + const hasAccess = Boolean(recipe.created_by) && recipe.created_by === userId; + return { hasAccess, recipe: hasAccess ? recipe : undefined }; } const { hasAccess } = await checkDashbоardAccess(env, recipe.dashboard_id as string, userId, requiredRole); diff --git a/controlplane/src/dashboards/DurableObject.ts b/controlplane/src/dashboards/DurableObject.ts index 36ff7939..c48f46be 100644 --- a/controlplane/src/dashboards/DurableObject.ts +++ b/controlplane/src/dashboards/DurableObject.ts @@ -241,6 +241,22 @@ export class DashboardDO implements DurableObject { }); } + // Purge durable state + sockets on dashboard delete (DOs can't be swept later). + if (path === '/destroy' && request.method === 'POST') { + try { + for (const ws of this.state.getWebSockets()) { + try { ws.close(1001, 'dashboard deleted'); } catch { /* already closing */ } + } + } catch { /* no sockets */ } + this.dashboard = null; + this.items.clear(); + this.terminalSessions.clear(); + this.edges.clear(); + this.presence.clear(); + await this.state.storage.deleteAll(); + return Response.json({ success: true }); + } + if (path === '/item' && request.method === 'PUT') { const item = await request.json() as DashboardItem; this.items.set(item.id, item); diff --git a/controlplane/src/dashboards/handler.ts b/controlplane/src/dashboards/handler.ts index 8f068387..5c98d964 100644 --- a/controlplane/src/dashboards/handler.ts +++ b/controlplane/src/dashboards/handler.ts @@ -372,6 +372,16 @@ export async function deleteDashbоard( // form_responses, terminal_integrations) await env.DB.prepare(`DELETE FROM dashboards WHERE id = ?`).bind(dashboardId).run(); + // Purge the DO after D1 is gone (D1 delete doesn't touch it, and DOs can't be + // swept later). Best-effort. + try { + const doId = env.DASHBOARD.idFromName(dashboardId); + const stub = env.DASHBOARD.get(doId); + await stub.fetch(new Request('http://do/destroy', { method: 'POST' })); + } catch (e) { + console.error(`[deleteDashboard] DashboardDO purge failed for ${dashboardId}: ${e}`); + } + return new Response(null, { status: 204 }); } diff --git a/controlplane/src/db/schema.ts b/controlplane/src/db/schema.ts index 23a572b6..e1950074 100644 --- a/controlplane/src/db/schema.ts +++ b/controlplane/src/db/schema.ts @@ -645,11 +645,13 @@ CREATE TABLE IF NOT EXISTS recipes ( name TEXT NOT NULL, description TEXT NOT NULL DEFAULT '', steps TEXT NOT NULL DEFAULT '[]', + created_by TEXT, created_at TEXT NOT NULL DEFAULT (datetime('now')), updated_at TEXT NOT NULL DEFAULT (datetime('now')) ); CREATE INDEX IF NOT EXISTS idx_recipes_dashboard ON recipes(dashboard_id); +CREATE INDEX IF NOT EXISTS idx_recipes_created_by ON recipes(created_by); -- Executions (workflow runs) CREATE TABLE IF NOT EXISTS executions ( @@ -1193,6 +1195,15 @@ export async function initializeDatabase(db: D1Database): Promise { // Column already exists. } + // Owner column so checkRecipeAccess can scope dashboard-less recipes to their creator. + try { + await db.prepare(` + ALTER TABLE recipes ADD COLUMN created_by TEXT + `).run(); + } catch { + // Column already exists. + } + try { await db.prepare(` ALTER TABLE dashboards ADD COLUMN cloud_id TEXT diff --git a/controlplane/src/egress/handler.ts b/controlplane/src/egress/handler.ts index 828f02fb..88bf7b06 100644 --- a/controlplane/src/egress/handler.ts +++ b/controlplane/src/egress/handler.ts @@ -118,7 +118,8 @@ export async function handleApproveEgress( ); } - // If "always allow", persist to the allowlist only after sandbox accepted. + // Allowlist only if no active deny exists (deny precedence); with the atomic + // deny batch below this stops a racing allow/deny landing in both lists. if (body.decision === 'allow_always') { const entryId = generateId(); await env.DB.prepare(` @@ -128,26 +129,35 @@ export async function handleApproveEgress( SELECT 1 FROM egress_allowlist WHERE dashboard_id = ? AND domain = ? AND revoked_at IS NULL ) - `).bind(entryId, dashboardId, normalizedDomain, userId, dashboardId, normalizedDomain).run(); + AND NOT EXISTS ( + SELECT 1 FROM egress_blocklist + WHERE dashboard_id = ? AND domain = ? AND revoked_at IS NULL + ) + `).bind( + entryId, dashboardId, normalizedDomain, userId, + dashboardId, normalizedDomain, + dashboardId, normalizedDomain, + ).run(); } - // If "deny always", persist to the blocklist and revoke any conflicting allowlist - // entry for the same domain so the deny is unambiguous (deny wins). + // If "deny always", revoke any conflicting allowlist entry and persist to the + // blocklist as one atomic batch (deny wins). if (body.decision === 'deny_always') { - await env.DB.prepare(` - UPDATE egress_allowlist SET revoked_at = datetime('now') - WHERE dashboard_id = ? AND domain = ? AND revoked_at IS NULL - `).bind(dashboardId, normalizedDomain).run(); - const entryId = generateId(); - await env.DB.prepare(` - INSERT INTO egress_blocklist (id, dashboard_id, domain, created_by, created_at) - SELECT ?, ?, ?, ?, datetime('now') - WHERE NOT EXISTS ( - SELECT 1 FROM egress_blocklist + await env.DB.batch([ + env.DB.prepare(` + UPDATE egress_allowlist SET revoked_at = datetime('now') WHERE dashboard_id = ? AND domain = ? AND revoked_at IS NULL - ) - `).bind(entryId, dashboardId, normalizedDomain, userId, dashboardId, normalizedDomain).run(); + `).bind(dashboardId, normalizedDomain), + env.DB.prepare(` + INSERT INTO egress_blocklist (id, dashboard_id, domain, created_by, created_at) + SELECT ?, ?, ?, ?, datetime('now') + WHERE NOT EXISTS ( + SELECT 1 FROM egress_blocklist + WHERE dashboard_id = ? AND domain = ? AND revoked_at IS NULL + ) + `).bind(entryId, dashboardId, normalizedDomain, userId, dashboardId, normalizedDomain), + ]); } // Log the decision after successful sandbox forward. @@ -582,7 +592,9 @@ export async function handleListEgressAudit( dashboardId: string, ): Promise { const url = new URL(request.url); - const limit = Math.min(parseInt(url.searchParams.get('limit') || '50'), 200); + // Guard against a NaN limit (parseInt('abc')) binding non-finite into D1. + const parsedLimit = parseInt(url.searchParams.get('limit') || '50', 10); + const limit = Number.isFinite(parsedLimit) ? Math.min(Math.max(parsedLimit, 1), 200) : 50; const entries = await env.DB.prepare(` SELECT id, domain, port, decision, decided_by, created_at diff --git a/controlplane/src/index.ts b/controlplane/src/index.ts index 85105b93..4f7e530a 100644 --- a/controlplane/src/index.ts +++ b/controlplane/src/index.ts @@ -1,7 +1,7 @@ // Copyright 2026 Rob Macrae. All rights reserved. // SPDX-License-Identifier: LicenseRef-Proprietary -// REVISION: controlplane-v17-pty-ws-edge-debug -console.log(`[controlplane] REVISION: controlplane-v17-pty-ws-edge-debug loaded at ${new Date().toISOString()}`); +// REVISION: controlplane-v18-idor-input-validation +console.log(`[controlplane] REVISION: controlplane-v18-idor-input-validation loaded at ${new Date().toISOString()}`); /** * OrcaBot Control Plane - Cloudflare Worker Entry Point @@ -531,6 +531,15 @@ export default { { status: 501 } ), origin, allowedOrigins); } + // Malformed request body (SyntaxError from request.json()) is client error → + // 400, not 500. Logged so a stray internal parse error isn't silent. + if (error instanceof SyntaxError) { + console.warn('Request body parse error (400):', error.message); + return cоrsRespоnse(Response.json( + { error: 'E40001: Invalid JSON body' }, + { status: 400 } + ), origin, allowedOrigins); + } console.error('Request error:', error); return cоrsRespоnse(Response.json( { error: error instanceof Error ? error.message : 'Internal server error' }, @@ -1517,6 +1526,9 @@ async function handleRequest(request: Request, env: EnvWithBindings, ctx: Pick | undefined; await Promise.race([ Promise.allSettled(resolvePromises), - new Promise(resolve => setTimeout(resolve, RESOLVE_TIMEOUT_MS)), + new Promise(resolve => { resolveTimer = setTimeout(resolve, RESOLVE_TIMEOUT_MS); }), ]); + if (resolveTimer !== undefined) clearTimeout(resolveTimer); } // Enforce subscription channel scoping (Telegram uses chatId, Slack/Discord use channelId). @@ -1891,12 +1894,19 @@ export async function createSubscription( } } - await env.DB.prepare(` + // Atomic guard for the Telegram "one active sub per user" invariant (the check + // above races). The `? = 'telegram'` term makes it a no-op for other providers. + const subInsert = await env.DB.prepare(` INSERT INTO messaging_subscriptions ( id, dashboard_id, item_id, user_id, provider, channel_id, channel_name, chat_id, team_id, webhook_id, webhook_secret, status, user_integration_id - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 'active', ?) + ) + SELECT ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 'active', ? + WHERE NOT EXISTS ( + SELECT 1 FROM messaging_subscriptions + WHERE user_id = ? AND provider = 'telegram' AND status IN ('pending', 'active') AND ? = 'telegram' + ) `).bind( id, dashboardId, itemId, userId, provider, data.channelId || null, @@ -1906,8 +1916,16 @@ export async function createSubscription( webhookId, webhookSecret, resolvedIntegrationId, + userId, provider, ).run(); + if (!subInsert.meta.changes && provider === 'telegram') { + throw new Error( + `Only one active Telegram subscription per bot is allowed. ` + + `Delete the existing one first to create a new one.` + ); + } + // For Discord: register /orcabot slash command as a guild command (instant availability). // Resolve guild_id from the integration metadata or by looking up the channel. if (provider === 'discord' && resolvedIntegrationId) { diff --git a/controlplane/src/recipes/handler.test.ts b/controlplane/src/recipes/handler.test.ts index 5f261d68..d712ec33 100644 --- a/controlplane/src/recipes/handler.test.ts +++ b/controlplane/src/recipes/handler.test.ts @@ -111,15 +111,19 @@ describe('Recipe Handlers', () => { }); describe('Access control', () => { - it('should allow access to recipes without dashboard_id', async () => { - // Recipe without dashboard is accessible to any authenticated user - const recipe = await seedRecipe(ctx.db, { name: 'Global Recipe' }); + it('should scope dashboard-less recipes to their owner', async () => { + // A global (no dashboard) recipe is accessible to its CREATOR... + const recipe = await seedRecipe(ctx.db, { name: 'Global Recipe', createdBy: testUser.id }); - const response = await getRecipе(ctx.env, recipe.id, testUser.id); - - expect(response.status).toBe(200); - const data = await response.json() as Record; + const ownerResponse = await getRecipе(ctx.env, recipe.id, testUser.id); + expect(ownerResponse.status).toBe(200); + const data = await ownerResponse.json() as Record; expect(data.recipe.name).toBe('Global Recipe'); + + // ...but NOT to a different user. Previously this returned the recipe to + // anyone (cross-tenant IDOR); it must now 404. + const otherResponse = await getRecipе(ctx.env, recipe.id, 'user-2'); + expect(otherResponse.status).toBe(404); }); it('should return 404 for non-existent recipes', async () => { diff --git a/controlplane/src/recipes/handler.ts b/controlplane/src/recipes/handler.ts index c0c50eea..fa6d0685 100644 --- a/controlplane/src/recipes/handler.ts +++ b/controlplane/src/recipes/handler.ts @@ -55,13 +55,14 @@ export async function listRecipеs( SELECT * FROM recipes WHERE dashboard_id = ? ORDER BY updated_at DESC `).bind(dashboardId).all(); } else { - // Get recipes from dashboards user is a member of, plus global recipes (no dashboard_id) + // Dashboards the user is a member of, plus the user's own global recipes + // (global recipes are owner-scoped via created_by). result = await env.DB.prepare(` SELECT r.* FROM recipes r LEFT JOIN dashboard_members dm ON r.dashboard_id = dm.dashboard_id - WHERE r.dashboard_id IS NULL OR dm.user_id = ? + WHERE (r.dashboard_id IS NULL AND r.created_by = ?) OR dm.user_id = ? ORDER BY r.updated_at DESC - `).bind(userId).all(); + `).bind(userId, userId).all(); } const recipes = result.results.map(r => ({ @@ -125,14 +126,15 @@ export async function createRecipе( const now = new Date().toISOString(); await env.DB.prepare(` - INSERT INTO recipes (id, dashboard_id, name, description, steps, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?) + INSERT INTO recipes (id, dashboard_id, name, description, steps, created_by, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) `).bind( id, data.dashboardId || null, data.name, data.description || '', JSON.stringify(data.steps || []), + userId, now, now ).run(); diff --git a/controlplane/src/subscriptions/handler.ts b/controlplane/src/subscriptions/handler.ts index c436594c..c30b111e 100644 --- a/controlplane/src/subscriptions/handler.ts +++ b/controlplane/src/subscriptions/handler.ts @@ -115,9 +115,14 @@ export async function createCheckoutSession( } } - // Create Stripe customer if needed (or if previous one was stale) + // Idempotency key dedupes concurrent creates, but must rotate: a permanent key + // makes Stripe replay a since-deleted customer for ~24h. Scope to a short window + // + new/recreate marker so only concurrent attempts share it. if (!stripeCustomerId) { - const customer = await stripe.createCustomer(env.STRIPE_SECRET_KEY, email, email); + const genWindow = Math.floor(Date.now() / 600_000); // 10-minute window + const gen = existing?.stripe_customer_id ? 'recreate' : 'new'; + const idempotencyKey = `customer-create-${userId}-${gen}-${genWindow}`; + const customer = await stripe.createCustomer(env.STRIPE_SECRET_KEY, email, email, idempotencyKey); stripeCustomerId = customer.id; // Insert/update subscription row with new customer ID diff --git a/controlplane/src/subscriptions/stripe-client.ts b/controlplane/src/subscriptions/stripe-client.ts index 36d20bed..e7f7dd05 100644 --- a/controlplane/src/subscriptions/stripe-client.ts +++ b/controlplane/src/subscriptions/stripe-client.ts @@ -29,11 +29,16 @@ async function stripeRequest>( method: string, path: string, params?: Record, + idempotencyKey?: string, ): Promise { const url = `${STRIPE_API_BASE}${path}`; const headers: Record = { Authorization: `Bearer ${secretKey}`, }; + // Stripe dedupes requests sharing an Idempotency-Key, returning the original object. + if (idempotencyKey) { + headers["Idempotency-Key"] = idempotencyKey; + } const init: RequestInit = { method, headers }; @@ -64,11 +69,12 @@ export async function createCustomer( secretKey: string, email: string, name: string, + idempotencyKey?: string, ): Promise<{ id: string }> { const result = await stripeRequest<{ id: string }>(secretKey, "POST", "/customers", { email, name, - }); + }, idempotencyKey); return { id: result.id }; } diff --git a/controlplane/tests/helpers.ts b/controlplane/tests/helpers.ts index 0fc36e05..4d3ebc70 100644 --- a/controlplane/tests/helpers.ts +++ b/controlplane/tests/helpers.ts @@ -152,19 +152,24 @@ export async function seedRecipe( dashboardId?: string; name?: string; steps?: unknown[]; + // Dashboard-less recipes are now owner-scoped (checkRecipeAccess). Default to + // the standard test user ('user-1') so seeded global recipes are owned by the + // user the tests authenticate as. + createdBy?: string; } = {} ) { const id = data.id || `recipe-${Date.now()}-${Math.random()}`; const name = data.name || 'Test Recipe'; const steps = JSON.stringify(data.steps || []); + const createdBy = data.createdBy || 'user-1'; const now = new Date().toISOString(); await db.prepare(` - INSERT INTO recipes (id, dashboard_id, name, description, steps, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?) - `).bind(id, data.dashboardId || null, name, '', steps, now, now).run(); + INSERT INTO recipes (id, dashboard_id, name, description, steps, created_by, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `).bind(id, data.dashboardId || null, name, '', steps, createdBy, now, now).run(); - return { id, name, steps: data.steps || [] }; + return { id, name, steps: data.steps || [], createdBy }; } export async function seedSchedule( diff --git a/desktop/app/src-tauri/Cargo.lock b/desktop/app/src-tauri/Cargo.lock index fa3042b3..beceea43 100644 --- a/desktop/app/src-tauri/Cargo.lock +++ b/desktop/app/src-tauri/Cargo.lock @@ -2566,6 +2566,7 @@ dependencies = [ "serde_json", "sha2", "signal-hook", + "tar", "tauri", "tauri-build", "tauri-plugin-dialog", diff --git a/desktop/app/src-tauri/Cargo.toml b/desktop/app/src-tauri/Cargo.toml index d4e031d7..9579cf70 100644 --- a/desktop/app/src-tauri/Cargo.toml +++ b/desktop/app/src-tauri/Cargo.toml @@ -28,6 +28,11 @@ serde_json = "1" walkdir = "2" libc = "0.2" flate2 = "1" +# In-process tar extraction for `orcabot import` (.orcabot bundles). Used instead +# of shelling out to system `tar`, which does ZERO path validation and lets GNU +# tar honor `..` members + materialize symlinks (host escape / symlink planting). +# Lightweight, pure-Rust; its main transitive dep (`filetime`) is already declared. +tar = "0.4" # Verify the on-demand VM image download against a hash baked into the binary. sha2 = "0.10" filetime = "0.2" diff --git a/desktop/app/src-tauri/gen/schemas/capabilities.json b/desktop/app/src-tauri/gen/schemas/capabilities.json index 894b8f36..2c53c5a5 100644 --- a/desktop/app/src-tauri/gen/schemas/capabilities.json +++ b/desktop/app/src-tauri/gen/schemas/capabilities.json @@ -1 +1 @@ -{"default":{"identifier":"default","description":"Default capabilities for the main window","remote":{"urls":["http://localhost:*"]},"local":true,"windows":["main"],"permissions":["core:default","dialog:default","dialog:allow-open",{"identifier":"opener:allow-open-url","allow":[{"url":"http://*"},{"url":"https://*"}]},"allow-get-workspace-path","allow-import-folder","allow-switch-to-cli","allow-quit-app","allow-get-surface-token","allow-open-url","allow-reveal-workspace","allow-get-ports","allow-get-app-version","allow-read-startup-log","allow-verify-orcabot-account","allow-set-cloud-credential","allow-sign-in-google-loopback","allow-cancel-google-sign-in","allow-rollback-sign-in","allow-get-cloud-account","allow-clear-cloud-credential","allow-list-cloud-dashboards","allow-get-cloud-dashboard","allow-download-cloud-workspace"]}} \ No newline at end of file +{"default":{"identifier":"default","description":"Default capabilities for the main window","remote":{"urls":["http://localhost:*"]},"local":true,"windows":["main"],"permissions":["core:default","dialog:default","dialog:allow-open",{"identifier":"opener:allow-open-url","allow":[{"url":"http://*"},{"url":"https://*"}]},"allow-get-workspace-path","allow-import-folder","allow-switch-to-cli","allow-quit-app","allow-get-surface-token","allow-reveal-workspace","allow-get-ports","allow-get-app-version","allow-read-startup-log","allow-verify-orcabot-account","allow-set-cloud-credential","allow-sign-in-google-loopback","allow-cancel-google-sign-in","allow-rollback-sign-in","allow-get-cloud-account","allow-clear-cloud-credential","allow-list-cloud-dashboards","allow-get-cloud-dashboard","allow-download-cloud-workspace"]}} \ No newline at end of file diff --git a/desktop/app/src-tauri/src/bin/orcabot.rs b/desktop/app/src-tauri/src/bin/orcabot.rs index c13991c5..7044841b 100644 --- a/desktop/app/src-tauri/src/bin/orcabot.rs +++ b/desktop/app/src-tauri/src/bin/orcabot.rs @@ -1,4 +1,4 @@ -// REVISION: orcabot-cli-v22-dynamic-ports +// REVISION: orcabot-cli-v24-import-stage-then-merge // // `orcabot` — command-line control for the Orcabot desktop stack. // @@ -57,7 +57,7 @@ const DEFAULT_CONTROLPLANE_PORT: u16 = 8787; const DEFAULT_SANDBOX_PORT: u16 = 8080; const DEFAULT_FRONTEND_PORT: u16 = 8788; const VZ_CONSOLE_LOG: &str = "/tmp/vz-console.log"; -const REVISION: &str = "orcabot-cli-v22-dynamic-ports"; +const REVISION: &str = "orcabot-cli-v24-import-stage-then-merge"; pub fn run() { let args: Vec = std::env::args().collect(); @@ -555,6 +555,167 @@ fn cmd_export(rest: &[String]) -> i32 { } } +/// `Read` wrapper that errors once total bytes read cross `cap`. Wrapping the gzip +/// decoder bounds total decompression across the whole archive (skipped entries +/// included). Errors rather than EOF-ing so tar can't read it as a clean truncation. +struct CappedReader { + inner: R, + remaining: u64, +} + +impl CappedReader { + fn new(inner: R, cap: u64) -> Self { + CappedReader { inner, remaining: cap } + } +} + +impl Read for CappedReader { + fn read(&mut self, buf: &mut [u8]) -> std::io::Result { + if self.remaining == 0 { + return Err(std::io::Error::new( + std::io::ErrorKind::Other, + "bundle decompresses too large (zip-bomb guard)", + )); + } + let cap = std::cmp::min(buf.len() as u64, self.remaining) as usize; + let n = self.inner.read(&mut buf[..cap])?; + self.remaining -= n as u64; + Ok(n) + } +} + +/// Removes a directory tree when dropped (cleans up the staging dir on every exit path). +struct DirGuard(PathBuf); + +impl Drop for DirGuard { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } +} + +/// Extract a `.orcabot` bundle (tar.gz) into a private staging dir, returning +/// `(manifest.json bytes, staged workspace-relative paths)`. Nothing is written to +/// the live workspace here — the caller validates and merges later, so a bad bundle +/// leaves it untouched. Each entry is vetted: skip `..`/absolute paths and +/// symlink/hardlink members; extract regular files + on-demand dirs only. Zip-bomb +/// guard: the gzip decoder is wrapped in `CappedReader` (bounds total decompression) +/// and file bodies stream to disk; only `manifest.json` is buffered, under its own cap. +fn extract_bundle_to_staging( + bundle: &str, + stage_dir: &std::path::Path, +) -> Result<(Vec, Vec), String> { + use std::path::Component; + + // Decompressed-size and entry-count ceilings. A legit session bundle is far + // under these; they exist purely to bound a hostile/zip-bomb archive. + const MAX_TOTAL_BYTES: u64 = 4 * 1024 * 1024 * 1024; // 4 GiB decompressed, all entries + const MAX_ENTRIES: usize = 200_000; + // manifest.json is the ONLY member we legitimately hold in memory. + const MAX_MANIFEST_BYTES: u64 = 8 * 1024 * 1024; + + let file = File::open(bundle).map_err(|e| format!("open bundle: {e}"))?; + let gz = flate2::read::GzDecoder::new(file); + // Cap the DECODER itself. `+1` so a legit bundle sitting exactly at the + // ceiling reads its last byte without false-tripping the guard. + let capped = CappedReader::new(gz, MAX_TOTAL_BYTES + 1); + let mut archive = tar::Archive::new(capped); + let entries = archive.entries().map_err(|e| format!("read archive: {e}"))?; + + let mut manifest: Option> = None; + let mut staged: Vec = Vec::new(); + let mut count: usize = 0; + + for entry in entries { + let mut entry = entry.map_err(|e| format!("read entry: {e}"))?; + count += 1; + if count > MAX_ENTRIES { + return Err("bundle has too many entries (zip-bomb guard)".into()); + } + + // Reject link members outright — this is the symlink/hardlink-planting + // vector. We never create a link, only regular files under the workspace. + let etype = entry.header().entry_type(); + if etype.is_symlink() || etype.is_hard_link() { + let p = entry + .path() + .map(|p| p.display().to_string()) + .unwrap_or_default(); + eprintln!("orcabot: skip link entry (not extracted): {p}"); + continue; + } + + let path = entry + .path() + .map_err(|e| format!("entry path: {e}"))? + .into_owned(); + + // Lexical guard: every component must be a plain name (reject "..", + // absolute/root, drive prefixes). This is the `..`-traversal kill. + if path.components().any(|c| !matches!(c, Component::Normal(_) | Component::CurDir)) { + eprintln!("orcabot: skip unsafe entry path: {}", path.display()); + continue; + } + + if etype.is_dir() { + continue; // dirs are created on demand at merge time + } + if !etype.is_file() { + continue; // char/block/fifo/etc. are never extracted + } + + // Route the entry by its top-level component. + let comps: Vec = path + .components() + .filter_map(|c| match c { + Component::Normal(s) => Some(s.to_os_string()), + _ => None, + }) + .collect(); + + if comps.len() == 1 && comps[0] == "manifest.json" { + // The one member we must hold in memory — under a small dedicated cap + // (`take(max+1)` detects overflow without unbounded allocation). + let mut buf: Vec = Vec::new(); + let n = std::io::Read::take(&mut entry, MAX_MANIFEST_BYTES + 1) + .read_to_end(&mut buf) + .map_err(|e| format!("read manifest: {e}"))? as u64; + if n > MAX_MANIFEST_BYTES { + return Err("manifest.json is implausibly large".into()); + } + manifest = Some(buf); + continue; + } + + if comps.first().map(|s| s == "workspace").unwrap_or(false) { + // Strip the leading "workspace/" to get the workspace-relative path. + let rel = comps[1..] + .iter() + .map(|s| s.to_string_lossy().into_owned()) + .collect::>() + .join("/"); + if rel.is_empty() { + continue; + } + // Stream to the private staging dir. Components are all plain names + // (verified above) so a lexical join can't escape; the authoritative + // O_NOFOLLOW guard runs later at merge against the live workspace. + let dest = stage_dir.join(&rel); + if let Some(parent) = dest.parent() { + fs::create_dir_all(parent).map_err(|e| format!("stage {rel}: {e}"))?; + } + let mut out = File::create(&dest).map_err(|e| format!("stage {rel}: {e}"))?; + std::io::copy(&mut entry, &mut out).map_err(|e| format!("stage {rel}: {e}"))?; + staged.push(rel); + continue; + } + // Unknown top-level entry — ignore (legit bundles only ship + // manifest.json + workspace/). + } + + let manifest = manifest.ok_or_else(|| "bundle has no manifest.json".to_string())?; + Ok((manifest, staged)) +} + fn cmd_import(rest: &[String]) -> i32 { if !controlplane_healthy() { eprintln!("orcabot: run `orcabot up` first"); @@ -574,25 +735,47 @@ fn cmd_import(rest: &[String]) -> i32 { return 1; } + // 1) STAGE — extract into a private temp dir (guard cleans it up on every exit). let stage = std::env::temp_dir().join(format!("orcabot-import-{}", std::process::id())); let _ = fs::remove_dir_all(&stage); - let _ = fs::create_dir_all(&stage); - let untar = Command::new("tar") - .args(["-xzf", bundle, "-C", &stage.to_string_lossy()]) - .status(); - if !matches!(untar, Ok(s) if s.success()) { - eprintln!("orcabot: failed to extract bundle"); - let _ = fs::remove_dir_all(&stage); + if let Err(e) = fs::create_dir_all(&stage) { + eprintln!("orcabot: cannot create staging dir: {e}"); return 1; } - let manifest: serde_json::Value = match fs::read_to_string(stage.join("manifest.json")) - .ok() - .and_then(|s| serde_json::from_str(&s).ok()) + #[cfg(unix)] { - Some(m) => m, - None => { + use std::os::unix::fs::PermissionsExt; + // Private staging dir: only this user can traverse/read the staged files. + let _ = fs::set_permissions(&stage, fs::Permissions::from_mode(0o700)); + } + let _stage_guard = DirGuard(stage.clone()); + + let (manifest_bytes, staged_files) = match extract_bundle_to_staging(bundle, &stage) { + Ok(v) => v, + Err(e) => { + eprintln!("orcabot: failed to extract bundle: {e}"); + return 1; + } + }; + + // 2) VALIDATE — parse the manifest before creating anything. A corrupt/missing + // manifest bails here with the live workspace still untouched. + let manifest: serde_json::Value = match serde_json::from_slice(&manifest_bytes) { + Ok(m) => m, + Err(_) => { eprintln!("orcabot: bundle has no valid manifest.json"); - let _ = fs::remove_dir_all(&stage); + return 1; + } + }; + + // Resolve the live workspace root up front (read-only; does not overwrite any + // files) so a broken workspace path fails before we create the dashboard. + let ws = workspace_dir(); + let _ = fs::create_dir_all(&ws); + let ws_canon = match ws.canonicalize() { + Ok(c) => c, + Err(e) => { + eprintln!("orcabot: cannot resolve workspace {}: {e}", ws.display()); return 1; } }; @@ -603,6 +786,7 @@ fn cmd_import(rest: &[String]) -> i32 { manifest.get("name").and_then(|x| x.as_str()).unwrap_or("dashboard") ) }); + // 3) CREATE — dashboard + items + edges. let nd = match cp_call("POST", "/dashboards", Some(serde_json::json!({ "name": name }))) { Ok(v) => v .get("dashboard") @@ -612,7 +796,6 @@ fn cmd_import(rest: &[String]) -> i32 { .to_string(), Err(e) => { eprintln!("orcabot: create dashboard: {e}"); - let _ = fs::remove_dir_all(&stage); return 1; } }; @@ -656,20 +839,39 @@ fn cmd_import(rest: &[String]) -> i32 { } } - // Restore workspace files (merge into the shared workspace). - let src_ws = stage.join("workspace"); - if src_ws.exists() { - let dest = workspace_dir(); - let _ = fs::create_dir_all(&dest); - // cp -R /. / (merge, preserve) - let _ = Command::new("cp") - .args(["-R", &format!("{}/.", src_ws.to_string_lossy()), &dest.to_string_lossy()]) - .status(); + // 4) MERGE — only now copy the staged workspace files into the live shared + // workspace, each through the O_NOFOLLOW component walk in + // `safe_workspace_write` (race-safe against a symlink planted mid-merge by a + // workspace-sharing sandbox process — same discipline as `pull`). Reaching + // here means the manifest validated and the dashboard was created, so a + // partial overwrite of the live workspace is no longer possible from an + // upstream failure. + let mut restored = 0usize; + for rel in &staged_files { + // Staged files were written by us and contain no symlinks, so a plain read + // is safe; the authoritative anti-symlink guard is on the WRITE side. + let data = match fs::read(stage.join(rel)) { + Ok(d) => d, + Err(e) => { + eprintln!("orcabot: skip {rel}: {e}"); + continue; + } + }; + if safe_workspace_dest(&ws_canon, rel).is_none() { + eprintln!("orcabot: skip unsafe workspace path: {rel}"); + continue; + } + if let Err(e) = safe_workspace_write(&ws_canon, rel, &data) { + eprintln!("orcabot: skip {rel}: {e}"); + continue; + } + restored += 1; } - let _ = fs::remove_dir_all(&stage); + + // 5) CLEANUP — `_stage_guard` removes the private staging dir as it drops here. println!( - "orcabot: imported '{name}' -> dashboard {} ({} components, {edge_n} edges, workspace restored)", + "orcabot: imported '{name}' -> dashboard {} ({} components, {edge_n} edges, {restored} workspace files restored)", nd, idmap.len() ); diff --git a/desktop/tests/browser-smoke.sh b/desktop/tests/browser-smoke.sh index 909823cb..1910b2f1 100755 --- a/desktop/tests/browser-smoke.sh +++ b/desktop/tests/browser-smoke.sh @@ -20,8 +20,8 @@ set -uo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" ORCABOT="${ORCABOT_BIN:-$SCRIPT_DIR/../app/src-tauri/target/release/orcabot}" -CP="http://127.0.0.1:8787" -SB="http://127.0.0.1:8080" +CP="${CP:-http://127.0.0.1:8787}" +SB="${SB:-http://127.0.0.1:8080}" U="X-User-ID: dev-desktop" VZ_LOG="/tmp/vz-console.log" READY_TIMEOUT="${READY_TIMEOUT:-45}" # chromium cold boot can be slow diff --git a/desktop/tests/first-prompt-smoke.sh b/desktop/tests/first-prompt-smoke.sh index 82ae258b..0c91608c 100755 --- a/desktop/tests/first-prompt-smoke.sh +++ b/desktop/tests/first-prompt-smoke.sh @@ -24,7 +24,7 @@ set -uo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" ORCABOT="${ORCABOT_BIN:-$SCRIPT_DIR/../app/src-tauri/target/release/orcabot}" -CP="http://127.0.0.1:8787" +CP="${CP:-http://127.0.0.1:8787}" U="X-User-ID: dev-desktop" ITER="${1:-15}" TAIL_SECS="${FIRST_PROMPT_TAIL_SECS:-3}" diff --git a/desktop/tests/regression-smoke.sh b/desktop/tests/regression-smoke.sh index 2c61fca8..8fcd147a 100755 --- a/desktop/tests/regression-smoke.sh +++ b/desktop/tests/regression-smoke.sh @@ -25,8 +25,8 @@ 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" +CP="${CP:-http://127.0.0.1:8787}" +SB="${SB:-http://127.0.0.1:8080}" U="X-User-ID: dev-desktop" VZ_LOG="/tmp/vz-console.log" @@ -118,11 +118,18 @@ c="$(code -X POST "${AUTH[@]}" -H 'Content-Type: application/json' --data-raw '{ { [ "$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" +# 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's retry loop. So we must let the +# session age past that window before killing, or the reconcile is (correctly) +# suppressed and the session stays 'active'. +RECONCILE_GRACE="${RECONCILE_GRACE:-33}" +echo "[#6] PTY death reconciles session to stopped (after ${RECONCILE_GRACE}s grace)" 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),''))")" +sleep "$RECONCILE_GRACE" # clear the 30s reconcile grace period 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 diff --git a/desktop/tests/security-smoke.sh b/desktop/tests/security-smoke.sh index 9ba5836f..9d349d52 100755 --- a/desktop/tests/security-smoke.sh +++ b/desktop/tests/security-smoke.sh @@ -17,7 +17,7 @@ # Usage: desktop/tests/security-smoke.sh set -uo pipefail -CP="http://127.0.0.1:8787" +CP="${CP:-http://127.0.0.1:8787}" ENDPOINT="/dashboards" # user-scoped; requires auth TOKEN_FILE="${ORCABOT_SURFACE_TOKEN_FILE:-$HOME/Library/Application Support/com.orcabot.desktop/surface-token}" DEV=(-H "X-User-ID: dev-desktop" -H "X-User-Email: desktop@localhost" -H "X-User-Name: Desktop User") diff --git a/sandbox/cmd/server/main.go b/sandbox/cmd/server/main.go index 39229dbb..25f125f6 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-v39-file-read-redaction +// REVISION: main-v40-import-gzip-bomb-cap package main @@ -40,7 +40,7 @@ import ( "github.com/Hyper-Int/OrcaBot/sandbox/internal/ws" ) -const mainRevision = "main-v39-file-read-redaction" +const mainRevision = "main-v40-import-gzip-bomb-cap" // 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 @@ -54,8 +54,38 @@ const ( // Upper bound on a bulk workspace import body (compressed tar.gz). Big enough // for real project workspaces; caps memory/disk a single request can consume. maxWorkspaceImportBytes = 1024 * 1024 * 1024 + // Ceilings on a single import's *decompressed* payload, guarding against a + // gzip bomb (a small compressed body that expands enormously). These are + // independent of the compressed-body cap (maxWorkspaceImportBytes). + maxImportDecompressedBytes = 2 * 1024 * 1024 * 1024 // 2 GiB total across all entries + maxImportEntries = 100_000 // total tar entries examined ) +// errImportTooLarge is returned when a decompressed import exceeds the ceilings. +var errImportTooLarge = errors.New("import exceeds decompressed size/entry limit") + +// cappedReader caps the total number of bytes that can be read through it, +// returning errImportTooLarge once the limit is crossed. Wrapping the gzip +// output in this bounds decompression even for entries the extractor skips +// (tar.Reader.Next still decompresses skipped entry data), defeating gzip bombs. +type cappedReader struct { + r io.Reader + n int64 + limit int64 +} + +func (c *cappedReader) Read(p []byte) (int, error) { + if c.n > c.limit { + return 0, errImportTooLarge + } + n, err := c.r.Read(p) + c.n += int64(n) + if c.n > c.limit { + return n, errImportTooLarge + } + return n, err +} + var errTooManyEntries = errors.New("too many files to list") func init() { @@ -1323,8 +1353,10 @@ func extractTarGzToWоrkspace(ws *fs.Workspace, body io.Reader) (int, int, error } defer gz.Close() - tr := tar.NewReader(gz) + // Bound total decompressed bytes (gzip-bomb guard) and total entry count. + tr := tar.NewReader(&cappedReader{r: gz, limit: maxImportDecompressedBytes}) written, skipped := 0, 0 + entries := 0 for { hdr, err := tr.Next() if err == io.EOF { @@ -1333,6 +1365,10 @@ func extractTarGzToWоrkspace(ws *fs.Workspace, body io.Reader) (int, int, error if err != nil { return written, skipped, err } + entries++ + if entries > maxImportEntries { + return written, skipped, errImportTooLarge + } if hdr.Typeflag != tar.TypeReg { continue // dirs/symlinks/etc — files carry their own parent path } diff --git a/sandbox/go.mod b/sandbox/go.mod index f17c4ff9..1a31489b 100644 --- a/sandbox/go.mod +++ b/sandbox/go.mod @@ -4,10 +4,7 @@ go 1.23 require ( github.com/creack/pty v1.1.24 + github.com/fsnotify/fsnotify v1.9.0 github.com/gorilla/websocket v1.5.3 -) - -require ( - github.com/fsnotify/fsnotify v1.9.0 // indirect - golang.org/x/sys v0.13.0 // indirect + golang.org/x/sys v0.13.0 ) diff --git a/sandbox/internal/broker/secrets_broker.go b/sandbox/internal/broker/secrets_broker.go index 1423ba1a..5395acb3 100644 --- a/sandbox/internal/broker/secrets_broker.go +++ b/sandbox/internal/broker/secrets_broker.go @@ -17,8 +17,8 @@ import ( "time" ) -// REVISION: broker-v6-provider-error-toast -const brokerRevision = "broker-v6-provider-error-toast" +// REVISION: broker-v7-evict-logged-routes +const brokerRevision = "broker-v7-evict-logged-routes" func init() { log.Printf("[broker] REVISION: %s loaded at %s", brokerRevision, time.Now().Format(time.RFC3339)) @@ -125,6 +125,20 @@ func (b *SecretsBroker) ClearConfigsForSession(sessionID string) { } } +// EvictLoggedRoutesForSession removes the per-(session,provider) routing-log dedup +// entries for a session. loggedRoutes is a sync.Map keyed "sessionID:provider" that +// otherwise grows unbounded as sessions are created and deleted over a VM's lifetime; +// call this on session teardown to release those keys. +func (b *SecretsBroker) EvictLoggedRoutesForSession(sessionID string) { + prefix := sessionID + ":" + b.loggedRoutes.Range(func(key, _ any) bool { + if k, ok := key.(string); ok && strings.HasPrefix(k, prefix) { + b.loggedRoutes.Delete(k) + } + return true + }) +} + // RemoveConfig removes a specific provider configuration. // Use this to remove individual secrets without clearing all configs. func (b *SecretsBroker) RemoveConfig(providerID string) { diff --git a/sandbox/internal/browser/browser.go b/sandbox/internal/browser/browser.go index 8c8c9665..1e90690d 100644 --- a/sandbox/internal/browser/browser.go +++ b/sandbox/internal/browser/browser.go @@ -1,7 +1,7 @@ // Copyright 2026 Rob Macrae. All rights reserved. // SPDX-License-Identifier: LicenseRef-Proprietary -// REVISION: browser-v10-no-sandbox-warning +// REVISION: browser-v11-status-kill-wait-reap package browser import ( @@ -377,6 +377,8 @@ func (c *Controller) Status() Status { for _, p := range c.processes { if p.Process != nil { _ = p.Process.Kill() + // Reap so killed children don't linger as zombies (matches Stop()). + _, _ = p.Process.Wait() } } c.processes = nil diff --git a/sandbox/internal/egress/proxy.go b/sandbox/internal/egress/proxy.go index 6d31cb96..7b645b6a 100644 --- a/sandbox/internal/egress/proxy.go +++ b/sandbox/internal/egress/proxy.go @@ -1,11 +1,12 @@ // Copyright 2026 Rob Macrae. All rights reserved. // SPDX-License-Identifier: LicenseRef-Proprietary -// REVISION: egress-proxy-v10-unified-decide +// REVISION: egress-proxy-v11-held-conn-ctx-cancel package egress import ( + "context" "fmt" "io" "log" @@ -20,7 +21,7 @@ import ( "github.com/Hyper-Int/OrcaBot/sandbox/internal/id" ) -const proxyRevision = "egress-proxy-v10-unified-decide" +const proxyRevision = "egress-proxy-v11-held-conn-ctx-cancel" func init() { log.Printf("[egress-proxy] REVISION: %s loaded at %s", proxyRevision, time.Now().Format(time.RFC3339)) @@ -280,7 +281,7 @@ func isLocalhost(host string) bool { // // Returns DecisionDefault/DecisionAllowOnce/DecisionAllowAlways when permitted, or // DecisionDeny/DecisionTimeout when blocked. Callers permit on the allow set. -func (p *EgressProxy) decide(host string, port int) string { +func (p *EgressProxy) decide(ctx context.Context, host string, port int) string { if isLocalhost(host) { return DecisionDefault } @@ -298,7 +299,7 @@ func (p *EgressProxy) decide(host string, port int) string { if p.allowlist.IsTracker(host) { return DecisionDeny } - return p.holdForApproval(host, port) + return p.holdForApproval(ctx, host, port) } // permitted reports whether a decision from decide() allows the connection. @@ -309,7 +310,7 @@ func permitted(decision string) bool { // handleConnect handles HTTPS CONNECT tunneling. func (p *EgressProxy) handleConnect(w http.ResponseWriter, r *http.Request) { host, port := splitHostPort(r.Host, 443) - if permitted(p.decide(host, port)) { + if permitted(p.decide(r.Context(), host, port)) { p.tunnelConnect(w, host, port) return } @@ -324,7 +325,7 @@ func (p *EgressProxy) handleHTTP(w http.ResponseWriter, r *http.Request) { return } - if !permitted(p.decide(host, port)) { + if !permitted(p.decide(r.Context(), host, port)) { http.Error(w, "Egress denied: domain not approved", http.StatusForbidden) return } @@ -367,12 +368,14 @@ func (p *EgressProxy) handleHTTP(w http.ResponseWriter, r *http.Request) { // if blocked (denied, tracker, user-denied, or timed out). // REVISION: egress-proxy-v10-unified-decide func (p *EgressProxy) CheckAndHold(host string, port int) string { - return p.decide(host, port) + // Browser navigation has no request lifetime to cancel against; use a + // background context so waitForDecision only unblocks on decision/timeout/stop. + return p.decide(context.Background(), host, port) } // holdForApproval blocks the current goroutine until the user approves/denies // or the timeout expires. Multiple connections to the same domain coalesce. -func (p *EgressProxy) holdForApproval(domain string, port int) string { +func (p *EgressProxy) holdForApproval(ctx context.Context, domain string, port int) string { domain = strings.ToLower(domain) p.pendingMu.Lock() @@ -381,7 +384,7 @@ func (p *EgressProxy) holdForApproval(domain string, port int) string { existing.Waiters++ p.pendingMu.Unlock() - return p.waitForDecision(existing) + return p.waitForDecision(ctx, existing) } // Create new pending entry @@ -445,18 +448,22 @@ func (p *EgressProxy) holdForApproval(domain string, port int) string { log.Printf("[egress-proxy] Holding connection to %s:%d (request_id=%s)", domain, port, requestID) - return p.waitForDecision(pending) + return p.waitForDecision(ctx, pending) } -// waitForDecision blocks until a decision arrives or the proxy stops. -// The timeout goroutine in holdForApproval handles expiry by closing doneCh. -func (p *EgressProxy) waitForDecision(pending *Pending) string { +// waitForDecision blocks until a decision arrives, the client disconnects, or the +// proxy stops. The timeout goroutine in holdForApproval handles expiry by closing +// doneCh. A client that abandons the held connection (ctx canceled) is treated as a +// deny so we don't pin the goroutine + hijacked conn until the 60s timeout. +func (p *EgressProxy) waitForDecision(ctx context.Context, pending *Pending) string { select { case <-pending.doneCh: if pending.decision == "" { return DecisionDeny } return pending.decision + case <-ctx.Done(): + return DecisionDeny case <-p.stopCh: return DecisionDeny } diff --git a/sandbox/internal/egress/proxy_test.go b/sandbox/internal/egress/proxy_test.go index 7d9f98df..ee8e493f 100644 --- a/sandbox/internal/egress/proxy_test.go +++ b/sandbox/internal/egress/proxy_test.go @@ -4,6 +4,7 @@ package egress import ( + "context" "net/http/httptest" "strings" "sync" @@ -85,7 +86,7 @@ func TestProxy_DecideHonorsDenyAndTracker(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got := permitted(proxy.decide(tc.host, 443)) + got := permitted(proxy.decide(context.Background(), tc.host, 443)) if got != tc.want { t.Errorf("decide(%q) permitted=%v, want %v", tc.host, got, tc.want) } @@ -98,7 +99,7 @@ func TestProxy_DecideHonorsDenyAndTracker(t *testing.T) { // A user "Always Allow" must override the tracker denylist. al.AddUserDomain("www.googletagmanager.com", "user-1") - if !permitted(proxy.decide("www.googletagmanager.com", 443)) { + if !permitted(proxy.decide(context.Background(), "www.googletagmanager.com", 443)) { t.Error("expected user-approved tracker to be permitted (user allow wins)") } } @@ -213,7 +214,7 @@ func TestProxy_Coalescing(t *testing.T) { wg.Add(1) go func(idx int) { defer wg.Done() - decisions[idx] = proxy.holdForApproval("coalesce-test.com", 443) + decisions[idx] = proxy.holdForApproval(context.Background(), "coalesce-test.com", 443) }(i) } diff --git a/sandbox/internal/egress/transparent_linux.go b/sandbox/internal/egress/transparent_linux.go index 26c71bb9..ef1f78e9 100644 --- a/sandbox/internal/egress/transparent_linux.go +++ b/sandbox/internal/egress/transparent_linux.go @@ -9,6 +9,7 @@ package egress import ( "bytes" + "context" "fmt" "io" "log" @@ -185,7 +186,10 @@ func (p *EgressProxy) handleTransparent(conn net.Conn) { // Full egress decision ladder (same source of truth as the CONNECT/HTTP paths): // localhost → deny → allowlist → tracker → hold. decide() handles localhost and // emits the allow audit. - if !permitted(p.decide(domain, origPort)) { + // The transparent path is a raw redirected conn with no request context, so it + // keeps its original behavior (no held-connection cancel-on-disconnect; that + // applies to the CONNECT/HTTP proxy paths which carry r.Context()). + if !permitted(p.decide(context.Background(), domain, origPort)) { return // denied (permanent/tracker/user) or timed out } diff --git a/sandbox/internal/fs/workspace.go b/sandbox/internal/fs/workspace.go index 5fbb9eea..88aecc85 100644 --- a/sandbox/internal/fs/workspace.go +++ b/sandbox/internal/fs/workspace.go @@ -1,6 +1,6 @@ // Copyright 2026 Rob Macrae. All rights reserved. // SPDX-License-Identifier: LicenseRef-Proprietary -// REVISION: workspace-v2-path-traversal-normalize +// REVISION: workspace-v4-openat-nofollow-write-walk package fs @@ -14,7 +14,7 @@ import ( "time" ) -const workspaceRevision = "workspace-v2-path-traversal-normalize" +const workspaceRevision = "workspace-v4-openat-nofollow-write-walk" func init() { log.Printf("[workspace] REVISION: %s loaded at %s", workspaceRevision, time.Now().Format(time.RFC3339)) @@ -82,28 +82,42 @@ func (w *Workspace) resоlvePath(path string) (string, error) { // This prevents symlink-based escapes (e.g., /workspace/link -> /etc) resolved, err := filepath.EvalSymlinks(fullPath) if err != nil { - // If the path doesn't exist, check the parent directory instead - // This allows creating new files while still preventing symlink escapes + // Target doesn't exist yet. Resolve the longest existing ancestor's + // symlinks and require it to stay in the workspace; the nonexistent suffix + // can't contain symlinks, so appending it lexically is safe. (A plain Abs() + // fallback would miss an existing in-workspace symlink pointing outside.) if os.IsNotExist(err) { - parent := filepath.Dir(fullPath) - base := filepath.Base(fullPath) - - resolvedParent, parentErr := filepath.EvalSymlinks(parent) - if parentErr != nil { - // Parent doesn't exist either - check if it's within workspace - // Use Abs as fallback for new directory trees - resolvedParent, parentErr = filepath.Abs(parent) - if parentErr != nil { - return "", parentErr + existing := fullPath + var suffix []string // path components below the longest existing ancestor, top-most last + for { + resolved, rerr := filepath.EvalSymlinks(existing) + if rerr == nil { + if !isPathWithin(resolved, w.root) { + return "", ErrPathTraversal + } + joined := resolved + for i := len(suffix) - 1; i >= 0; i-- { + joined = filepath.Join(joined, suffix[i]) + } + // Belt-and-suspenders: the fully reconstructed path must also + // remain within the workspace. + if !isPathWithin(joined, w.root) { + return "", ErrPathTraversal + } + return joined, nil } + if !os.IsNotExist(rerr) { + return "", rerr + } + parent := filepath.Dir(existing) + if parent == existing { + // Walked to the filesystem root without an existing ancestor + // (w.root should always exist, so this is unreachable in practice). + return "", ErrPathTraversal + } + suffix = append(suffix, filepath.Base(existing)) + existing = parent } - - // Check parent is within workspace - if !isPathWithin(resolvedParent, w.root) { - return "", ErrPathTraversal - } - - return filepath.Join(resolvedParent, base), nil } return "", err } @@ -187,46 +201,58 @@ func (w *Workspace) Read(path string) ([]byte, error) { return data, nil } -// Write writes content to a file, creating directories as needed +// Write writes content to a file, creating directories as needed. Rather than +// validate a string path then write to it (a symlink-TOCTOU: a workspace process +// could swap an ancestor for a symlink in between, escaping /workspace — a host +// mount on desktop), it walks every component with openat + O_NOFOLLOW via +// safeWrite, so containment is enforced by the kernel at open time. func (w *Workspace) Write(path string, content []byte) error { - resolved, err := w.resоlvePath(path) + // Cheap lexical pre-filter (reject raw "..", re-root, split into + // components). The kernel-enforced O_NOFOLLOW walk below is the real + // containment guard; this just rejects obviously-bad input early and + // produces the relative component list to walk. + rel, err := w.relComponents(path) if err != nil { return err } + return safeWrite(w.root, rel, content) +} - // Create parent directories - dir := filepath.Dir(resolved) - if err := os.MkdirAll(dir, 0755); err != nil { - return err +// relComponents applies the lexical containment guard used across the workspace +// (reject raw ".." before Clean, re-root absolute paths) and returns the path's +// components relative to the workspace root. It returns ErrPathTraversal for any +// input that does not name a file strictly within the workspace. +func (w *Workspace) relComponents(path string) ([]string, error) { + // Check raw input for ".." segments BEFORE cleaning. filepath.Clean + // resolves ".." which can hide traversal attempts (e.g. "/../../tmp/evil" + // cleans to "/tmp/evil", losing the ".." evidence). + for _, part := range strings.Split(path, "/") { + if part == ".." { + return nil, ErrPathTraversal + } } - // Write atomically: a fresh temp file + rename. Writing directly with - // O_TRUNC blocks indefinitely when the destination is held open by another - // process over a shared mount (virtiofs) — e.g. a cache file the host has - // mmap'd. rename() replaces the dir entry without opening the held file. - tmp, err := os.CreateTemp(dir, ".orcawrite-*.tmp") - if err != nil { - return err - } - tmpName := tmp.Name() - if _, err := tmp.Write(content); err != nil { - tmp.Close() - os.Remove(tmpName) - return err - } - if err := tmp.Close(); err != nil { - os.Remove(tmpName) - return err + cleaned := filepath.Clean(path) + cleaned = strings.TrimPrefix(cleaned, "/") + // "" or "." means the workspace root itself — no file component to write. + if cleaned == "" || cleaned == "." { + return nil, ErrPathTraversal } - if err := os.Chmod(tmpName, 0644); err != nil { - os.Remove(tmpName) - return err + + out := make([]string, 0) + for _, c := range strings.Split(cleaned, "/") { + if c == "" || c == "." { + continue + } + if c == ".." { // belt-and-suspenders: Clean shouldn't leave these + return nil, ErrPathTraversal + } + out = append(out, c) } - if err := os.Rename(tmpName, resolved); err != nil { - os.Remove(tmpName) - return err + if len(out) == 0 { + return nil, ErrPathTraversal } - return nil + return out, nil } // Delete removes a file or directory (recursively) diff --git a/sandbox/internal/fs/workspace_test.go b/sandbox/internal/fs/workspace_test.go index 9fd55449..a6a7e956 100644 --- a/sandbox/internal/fs/workspace_test.go +++ b/sandbox/internal/fs/workspace_test.go @@ -333,6 +333,70 @@ func TestWorkspaceSiblingDirectoryBypass(t *testing.T) { } } +// Security test - openat+O_NOFOLLOW write walk rejects a symlink at an +// intermediate directory component (the TOCTOU vector: an attacker swaps a +// validated ancestor dir for a symlink before the write lands). +func TestWorkspaceWriteSymlinkIntermediateComponent(t *testing.T) { + parent, err := os.MkdirTemp("", "workspace-test-*") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(parent) + + workspaceRoot := filepath.Join(parent, "workspace") + outside := filepath.Join(parent, "outside") + os.Mkdir(workspaceRoot, 0755) + os.Mkdir(outside, 0755) + + ws := NewWоrkspace(workspaceRoot) + + // Real intermediate dir, then replace a deeper component with a symlink + // pointing outside the workspace. + os.MkdirAll(filepath.Join(workspaceRoot, "a"), 0755) + if err := os.Symlink(outside, filepath.Join(workspaceRoot, "a", "b")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + + // Writing through the symlinked "b" component must be rejected — and nothing + // may be created under the outside dir. + err = ws.Write("/a/b/pwned.txt", []byte("escape")) + if err != ErrPathTraversal { + t.Errorf("expected ErrPathTraversal for symlinked intermediate component, got: %v", err) + } + if _, serr := os.Stat(filepath.Join(outside, "pwned.txt")); !os.IsNotExist(serr) { + t.Error("write escaped the workspace through a symlinked intermediate component") + } +} + +// Write must atomically overwrite an existing regular file (rename-in-place). +func TestWorkspaceWriteOverwrite(t *testing.T) { + root := setupTestWorkspace(t) + defer os.RemoveAll(root) + + ws := NewWоrkspace(root) + + if err := ws.Write("/a/b/file.txt", []byte("first")); err != nil { + t.Fatalf("initial write failed: %v", err) + } + if err := ws.Write("/a/b/file.txt", []byte("second")); err != nil { + t.Fatalf("overwrite failed: %v", err) + } + data, err := os.ReadFile(filepath.Join(root, "a", "b", "file.txt")) + if err != nil { + t.Fatalf("read failed: %v", err) + } + if string(data) != "second" { + t.Errorf("expected 'second', got %q", string(data)) + } + // No stray temp files left behind in the target dir. + entries, _ := os.ReadDir(filepath.Join(root, "a", "b")) + for _, e := range entries { + if len(e.Name()) >= 11 && e.Name()[:11] == ".orcawrite-" { + t.Errorf("leftover temp file: %s", e.Name()) + } + } +} + // Security test - symlink escape prevention func TestWorkspaceSymlinkEscape(t *testing.T) { root := setupTestWorkspace(t) diff --git a/sandbox/internal/fs/workspace_write_other.go b/sandbox/internal/fs/workspace_write_other.go new file mode 100644 index 00000000..72ae5e5d --- /dev/null +++ b/sandbox/internal/fs/workspace_write_other.go @@ -0,0 +1,52 @@ +// Copyright 2026 Rob Macrae. All rights reserved. +// SPDX-License-Identifier: LicenseRef-Proprietary + +// REVISION: workspace-v4-openat-nofollow-write-walk + +//go:build !unix + +package fs + +import ( + "os" + "path/filepath" +) + +// safeWrite is the non-Unix fallback. It cannot use openat + O_NOFOLLOW, so it +// falls back to the historical path-based create-dirs + atomic temp-file+rename +// behavior. The sandbox only ships on Linux (VM) and macOS (host tooling), both +// of which use the hardened Unix implementation; this exists only so the package +// keeps compiling on other platforms. +func safeWrite(root string, rel []string, content []byte) error { + full := root + for _, c := range rel { + full = filepath.Join(full, c) + } + dir := filepath.Dir(full) + if err := os.MkdirAll(dir, 0755); err != nil { + return err + } + tmp, err := os.CreateTemp(dir, ".orcawrite-*.tmp") + if err != nil { + return err + } + tmpName := tmp.Name() + if _, err := tmp.Write(content); err != nil { + tmp.Close() + os.Remove(tmpName) + return err + } + if err := tmp.Close(); err != nil { + os.Remove(tmpName) + return err + } + if err := os.Chmod(tmpName, 0644); err != nil { + os.Remove(tmpName) + return err + } + if err := os.Rename(tmpName, full); err != nil { + os.Remove(tmpName) + return err + } + return nil +} diff --git a/sandbox/internal/fs/workspace_write_unix.go b/sandbox/internal/fs/workspace_write_unix.go new file mode 100644 index 00000000..27ad5050 --- /dev/null +++ b/sandbox/internal/fs/workspace_write_unix.go @@ -0,0 +1,125 @@ +// Copyright 2026 Rob Macrae. All rights reserved. +// SPDX-License-Identifier: LicenseRef-Proprietary + +// REVISION: workspace-v4-openat-nofollow-write-walk + +//go:build unix + +package fs + +import ( + "errors" + "fmt" + "math/rand/v2" + "os" + + "golang.org/x/sys/unix" +) + +// safeWrite writes content to rel (already-validated plain path components) under +// root, walking every component with openat + O_NOFOLLOW relative to the root fd — +// so a symlink component (even one raced in after validation) fails the open with +// ELOOP (surfaced as ErrPathTraversal), making the TOCTOU unwinnable. The final +// file is written to a temp name and renameat'd into place (atomic; never writes +// through a symlink). +func safeWrite(root string, rel []string, content []byte) error { + // Open the trusted (already-canonicalized) workspace root as a directory fd. + dirfd, err := unix.Open(root, unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return err + } + // dirfd advances down the tree; ensure the fd we currently hold is closed. + defer func() { + if dirfd >= 0 { + unix.Close(dirfd) + } + }() + + fileName := rel[len(rel)-1] + dirs := rel[:len(rel)-1] + + // Create + open each dir component relative to the current fd; O_NOFOLLOW + // rejects a symlink component so the walk can't leave the workspace. + for _, comp := range dirs { + if err := unix.Mkdirat(dirfd, comp, 0755); err != nil && !errors.Is(err, unix.EEXIST) { + return err + } + next, oerr := unix.Openat(dirfd, comp, unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if oerr != nil { + // Classify the failure while the parent fd is still open, then close. + cerr := classifyComponentErr(dirfd, comp, oerr) + unix.Close(dirfd) + dirfd = -1 // already closed; stop the deferred double-close + return cerr + } + unix.Close(dirfd) // done with the parent; advance + dirfd = next + } + + // dirfd is the final directory, reached without traversing a symlink. + return atomicWriteAt(dirfd, fileName, content) +} + +// atomicWriteAt writes content to name inside dirfd atomically: a temp file +// (O_EXCL + O_NOFOLLOW, relative to dirfd) then renameat into place. +func atomicWriteAt(dirfd int, name string, content []byte) error { + var tmpName string + var tmpFd int + for attempt := 0; ; attempt++ { + tmpName = fmt.Sprintf(".orcawrite-%d-%x.tmp", os.Getpid(), rand.Uint64()) + fd, err := unix.Openat(dirfd, tmpName, + unix.O_WRONLY|unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0644) + if err == nil { + tmpFd = fd + break + } + if errors.Is(err, unix.EEXIST) && attempt < 100 { + continue // name collision, retry with a new random suffix + } + return err + } + + f := os.NewFile(uintptr(tmpFd), tmpName) + if _, err := f.Write(content); err != nil { + f.Close() + unix.Unlinkat(dirfd, tmpName, 0) + return err + } + if err := f.Close(); err != nil { + unix.Unlinkat(dirfd, tmpName, 0) + return err + } + + if err := unix.Renameat(dirfd, tmpName, dirfd, name); err != nil { + unix.Unlinkat(dirfd, tmpName, 0) + return mapWalkErr(err) + } + return nil +} + +// mapWalkErr translates a kernel ELOOP (a component was a symlink, refused by +// O_NOFOLLOW) into the workspace's ErrPathTraversal so callers get a stable +// error type regardless of whether the symlink was static or raced in. +func mapWalkErr(err error) error { + if errors.Is(err, unix.ELOOP) { + return ErrPathTraversal + } + return err +} + +// classifyComponentErr maps an openat failure on a symlink component to +// ErrPathTraversal. O_NOFOLLOW reports ELOOP on Linux, ENOTDIR on darwin, so we +// lstat to tell portably. The lstat only picks the error type — the O_NOFOLLOW +// open above is the real gate and has already failed closed. +func classifyComponentErr(dirfd int, comp string, err error) error { + if errors.Is(err, unix.ELOOP) { + return ErrPathTraversal + } + var st unix.Stat_t + if serr := unix.Fstatat(dirfd, comp, &st, unix.AT_SYMLINK_NOFOLLOW); serr == nil { + if st.Mode&unix.S_IFMT == unix.S_IFLNK { + return ErrPathTraversal + } + } + return err +} diff --git a/sandbox/internal/pty/hub.go b/sandbox/internal/pty/hub.go index b50ef472..6bee8daa 100644 --- a/sandbox/internal/pty/hub.go +++ b/sandbox/internal/pty/hub.go @@ -1,7 +1,7 @@ // Copyright 2026 Rob Macrae. All rights reserved. // SPDX-License-Identifier: LicenseRef-Proprietary -// REVISION: hub-v5-local-cpr-response +// REVISION: hub-v6-run-cleanup-all-paths package pty @@ -15,7 +15,7 @@ import ( "time" ) -const hubRevision = "hub-v5-local-cpr-response" +const hubRevision = "hub-v6-run-cleanup-all-paths" func init() { log.Printf("[pty/hub] REVISION: %s loaded at %s", hubRevision, time.Now().Format(time.RFC3339)) @@ -193,7 +193,29 @@ func NewHub(p *PTY, creatorID string) *Hub { } // Run starts the hub's event loop +// cleanup runs the terminal teardown shared by EVERY exit path of Run: stop +// turn-taking, cancel the idle timer, and close all client output channels. It is +// deferred in Run so the self-stop paths (idle timeout, readLoop death) get the +// same cleanup as an explicit Stop() — otherwise those paths returned before the +// <-h.stop case ran, leaking client channels and never stopping turn-taking. +func (h *Hub) cleanup() { + h.mu.Lock() + defer h.mu.Unlock() + h.turn.Stop() + if h.idleTimer != nil { + h.idleTimer.Stop() + h.idleTimer = nil + } + for client := range h.clients { + close(client) + delete(h.clients, client) + } +} + func (h *Hub) Run() { + // All termination paths funnel through cleanup exactly once. + defer h.cleanup() + // Start reading from PTY go h.readLооp() @@ -252,19 +274,7 @@ func (h *Hub) Run() { return case <-h.stop: - h.mu.Lock() - h.turn.Stop() - // Cancel idle timer if running - if h.idleTimer != nil { - h.idleTimer.Stop() - h.idleTimer = nil - } - // Close all client channels - for client := range h.clients { - close(client) - delete(h.clients, client) - } - h.mu.Unlock() + // Cleanup runs via the deferred h.cleanup() (shared by all exit paths). return } } diff --git a/sandbox/internal/sessions/manager.go b/sandbox/internal/sessions/manager.go index 8e5f10c2..db5d9e3d 100644 --- a/sandbox/internal/sessions/manager.go +++ b/sandbox/internal/sessions/manager.go @@ -218,6 +218,14 @@ func (m *Manager) Delete(id string) error { delete(m.sessions, id) m.mu.Unlock() + // REVISION: manager-v2-evict-logged-routes + // Release this session's per-session state on the shared broker so it doesn't + // accumulate over the VM's lifetime (loggedRoutes dedup keys are never + // otherwise evicted). + if m.broker != nil { + m.broker.EvictLoggedRoutesForSession(id) + } + // Close PTYs and agent if err := session.Clоse(); err != nil { return err diff --git a/sandbox/internal/sessions/session.go b/sandbox/internal/sessions/session.go index 3e6c0f3c..5af7b4bf 100644 --- a/sandbox/internal/sessions/session.go +++ b/sandbox/internal/sessions/session.go @@ -1,7 +1,7 @@ // Copyright 2026 Rob Macrae. All rights reserved. // SPDX-License-Identifier: LicenseRef-Proprietary -// REVISION: session-v22-pool-empty-mcp-env +// REVISION: session-v23-browser-close-race // Package sessions manages session lifecycle. // @@ -40,7 +40,7 @@ import ( "github.com/Hyper-Int/OrcaBot/sandbox/internal/statecache" ) -const sessionRevision = "session-v22-pool-empty-mcp-env" +const sessionRevision = "session-v23-browser-close-race" // Allow UUID-style IDs and internal random IDs while rejecting shell metacharacters. // This protects shell-interpolated call sites (e.g. Claude apiKeyHelper command). @@ -71,7 +71,7 @@ func applyEgressProxyEnv(envVars map[string]string) { // 127.0.0.0/8 covers the full IPv4 loopback range (consistent with iptables ! -d 127.0.0.0/8). // curl and wget honour CIDR notation; the proxy-side isLocalhost() uses net.IP.IsLoopback() // as the authoritative gate for clients that send non-127.0.0.1 loopback addresses. - // REVISION: session-v22-pool-empty-mcp-env + // REVISION: session-v23-browser-close-race envVars["NO_PROXY"] = "127.0.0.0/8,::1,localhost" envVars["no_proxy"] = "127.0.0.0/8,::1,localhost" } @@ -119,6 +119,11 @@ type Session struct { agent *agent.Controller workspace *fs.Workspace browser *browser.Controller + // closed is set true (under mu) by Close(). Once set, no browser may be + // (re)started — guards against the pre-warm goroutine starting an orphaned + // Chromium stack on a torn-down session. Guarded by mu. + // REVISION: session-v23-browser-close-race + closed bool // Secrets broker for secure API key handling broker *broker.SecretsBroker @@ -293,6 +298,10 @@ func (s *Session) Wоrkspace() *fs.Workspace { func (s *Session) StartBrowser() (browser.Status, error) { s.mu.Lock() + if s.closed { + s.mu.Unlock() + return browser.Status{}, fmt.Errorf("session closed") + } if s.browser == nil { s.browser = browser.NewControllerWithEgress(s.workspace.Root(), s.egressProxyPort) } @@ -325,6 +334,10 @@ func (s *Session) BrowserStatus() browser.Status { func (s *Session) OpenBrowserURL(target string) error { s.mu.Lock() + if s.closed { + s.mu.Unlock() + return fmt.Errorf("session closed") + } if s.browser == nil { s.browser = browser.NewControllerWithEgress(s.workspace.Root(), s.egressProxyPort) } @@ -754,7 +767,7 @@ func (s *Session) CreatePTYWithOptions(opts CreatePTYOptions) (*PTYInfo, error) } // Per-PTY config directory: non-pool mode only. Same rationale as CreatePTY. - // REVISION: session-v22-pool-empty-mcp-env + // REVISION: session-v23-browser-close-race if pty.GetPool() == nil { orcabotPtyDir := filepath.Join(s.workspace.Root(), ".orcabot", "pty", ptyID) if err := os.MkdirAll(orcabotPtyDir, 0750); err == nil { @@ -769,7 +782,7 @@ func (s *Session) CreatePTYWithOptions(opts CreatePTYOptions) (*PTYInfo, error) // Allocate pool slot BEFORE writing any workspace config files. // Same ordering invariant as CreatePTY — see comment there for rationale. - // REVISION: session-v22-pool-empty-mcp-env + // REVISION: session-v23-browser-close-race cleanupSecretsWithToken := func() { s.integrationTokensMu.Lock() delete(s.integrationTokens, ptyID) @@ -797,7 +810,7 @@ func (s *Session) CreatePTYWithOptions(opts CreatePTYOptions) (*PTYInfo, error) if agentType != mcp.AgentTypeUnknown { userTools := s.fetchUserMCPTools() // Pool mode: empty mcpEnv — same rationale as CreatePTY. - // REVISION: session-v22-pool-empty-mcp-env + // REVISION: session-v23-browser-close-race mcpEnv := map[string]string{} if pty.GetPool() == nil { mcpEnv["ORCABOT_SESSION_ID"] = s.ID @@ -890,7 +903,7 @@ func (s *Session) CreatePTYWithOptions(opts CreatePTYOptions) (*PTYInfo, error) } // Spawn PTY. Pool slot was allocated before workspace config writes above. - // REVISION: session-v22-pool-empty-mcp-env + // REVISION: session-v23-browser-close-race var p *pty.PTY if poolSlot != nil { p, err = pty.NewWithCommandEnvIDSlot(poolSlot, s.ID, command, 80, 24, actualWorkDir, envVars) @@ -1088,6 +1101,11 @@ func (s *Session) DeletePTY(id string) error { // Note: The secrets broker is shared and managed by the Manager, not stopped here. func (s *Session) Clоse() error { s.mu.Lock() + // Mark closed BEFORE releasing the lock so any concurrent StartBrowser (e.g. the + // pre-warm goroutine firing after a quick delete) sees closed and refuses to + // launch — no orphaned Chromium stack. Read s.browser under the lock too (it was + // previously read racily, outside the lock). + s.closed = true ptys := make([]*PTYInfo, 0, len(s.ptys)) for _, info := range s.ptys { ptys = append(ptys, info) @@ -1095,6 +1113,7 @@ func (s *Session) Clоse() error { s.ptys = make(map[string]*PTYInfo) agentCtrl := s.agent s.agent = nil + browserCtrl := s.browser s.mu.Unlock() for _, info := range ptys { @@ -1105,8 +1124,8 @@ func (s *Session) Clоse() error { agentCtrl.Stоp() } - if s.browser != nil { - s.browser.Stop() + if browserCtrl != nil { + browserCtrl.Stop() } // Stop Drive sync if running @@ -1207,16 +1226,23 @@ func (s *Session) NotifyIntegrations(ptyID string, providers []string, ptyToken curr[p] = true } + // Run attach/detach SYNCHRONOUSLY (not `go ...`) while holding knownProvidersMu. + // A bare goroutine gave attach and detach no relative ordering, so a detach could + // run before its preceding attach and leave a syncer running with no attachment + // (refCount desync). Dispatching under the serializing knownProvidersMu preserves + // call order across concurrent NotifyIntegrations calls. Both handlers lock the + // separate driveSyncMu (no shared lock with knownProvidersMu → no deadlock). + // Detect newly attached Drive if curr["google_drive"] && !prev["google_drive"] { log.Printf("[session] Drive integration attached via PTY %s: dashboardID=%s session=%s", ptyID, s.DashboardID, s.ID) - go s.OnDriveIntegrationAttached(ptyToken) + s.OnDriveIntegrationAttached(ptyToken) } // Detect detached Drive if !curr["google_drive"] && prev["google_drive"] { log.Printf("[session] Drive integration detached via PTY %s: dashboardID=%s session=%s", ptyID, s.DashboardID, s.ID) - go s.OnDriveIntegrationDetached() + s.OnDriveIntegrationDetached() } s.knownProviders[ptyID] = curr