From 0f75cb8e327322221323274ce2b569ead7788852 Mon Sep 17 00:00:00 2001 From: Hibryda Date: Sun, 22 Mar 2026 02:52:04 +0100 Subject: [PATCH] fix(electrobun): complete all 16 Codex #3 findings CRITICAL: - Message persistence race: snapshot batchEnd before async save - Double-start guard: startingProjects Set prevents concurrent launches - Symlink path traversal: fs.realpathSync() in path-guard.ts - Relay false success: connect() returns { ok, machineId, error } HIGH: - Session restore skips if active session exists - Remote remove: new RPC, cleans backend map - Task board poll token: stale responses discarded after drag-drop - Health concurrent tools: toolsInFlight counter (was boolean) - bttask transactions: delete wraps comments+task, addComment validates - PTY buffer cleared on reconnect - PTY large paste: chunked String.fromCharCode (8KB chunks) - Sidecar max line: 10MB limit prevents unbounded memory - btmsg authorization: group validation, channel membership checks MEDIUM: - Session retention: max 5 per project, purgeSession/untrackProject - Relay IPv6: URL parser replaces string split - PTY schema: fixed misleading base64 comment --- ui-electrobun/src/bun/btmsg-db.ts | 41 ++++++++++++++++-- ui-electrobun/src/bun/bttask-db.ts | 16 ++++++- ui-electrobun/src/bun/handlers/path-guard.ts | 29 +++++++++++-- .../src/bun/handlers/remote-handlers.ts | 17 +++++++- ui-electrobun/src/bun/pty-client.ts | 19 +++++++-- ui-electrobun/src/bun/relay-client.ts | 42 +++++++++++-------- ui-electrobun/src/bun/sidecar-manager.ts | 11 ++++- .../src/mainview/TaskBoardTab.svelte | 7 ++++ .../src/mainview/agent-store.svelte.ts | 2 + .../src/mainview/health-store.svelte.ts | 23 +++++++--- .../settings/RemoteMachinesSettings.svelte | 9 +++- ui-electrobun/src/shared/pty-rpc-schema.ts | 16 +++++-- 12 files changed, 190 insertions(+), 42 deletions(-) diff --git a/ui-electrobun/src/bun/btmsg-db.ts b/ui-electrobun/src/bun/btmsg-db.ts index f5a06e6..53d4b1e 100644 --- a/ui-electrobun/src/bun/btmsg-db.ts +++ b/ui-electrobun/src/bun/btmsg-db.ts @@ -200,6 +200,9 @@ export class BtmsgDb { // ── Direct messages ────────────────────────────────────────────────────── + /** + * Fix #13 (Codex audit): Validate sender and recipient are in the same group. + */ sendMessage(fromAgent: string, toAgent: string, content: string): string { // Get sender's group_id const sender = this.db.query<{ group_id: string }, [string]>( @@ -207,6 +210,15 @@ export class BtmsgDb { ).get(fromAgent); if (!sender) throw new Error(`Sender agent '${fromAgent}' not found`); + // Validate recipient exists and is in the same group + const recipient = this.db.query<{ group_id: string }, [string]>( + "SELECT group_id FROM agents WHERE id = ?" + ).get(toAgent); + if (!recipient) throw new Error(`Recipient agent '${toAgent}' not found`); + if (sender.group_id !== recipient.group_id) { + throw new Error(`Cross-group messaging denied: '${fromAgent}' (${sender.group_id}) -> '${toAgent}' (${recipient.group_id})`); + } + const id = randomUUID(); this.db.query( `INSERT INTO messages (id, from_agent, to_agent, content, group_id, sender_group_id) @@ -249,12 +261,23 @@ export class BtmsgDb { // ── Channels ───────────────────────────────────────────────────────────── + /** + * Fix #13 (Codex audit): Auto-add creator to channel membership on create. + */ createChannel(name: string, groupId: string, createdBy: string): string { const id = randomUUID(); - this.db.query( - `INSERT INTO channels (id, name, group_id, created_by) - VALUES (?1, ?2, ?3, ?4)` - ).run(id, name, groupId, createdBy); + const tx = this.db.transaction(() => { + this.db.query( + `INSERT INTO channels (id, name, group_id, created_by) + VALUES (?1, ?2, ?3, ?4)` + ).run(id, name, groupId, createdBy); + // Auto-add creator as channel member + this.db.query( + `INSERT OR IGNORE INTO channel_members (channel_id, agent_id) + VALUES (?1, ?2)` + ).run(id, createdBy); + }); + tx(); return id; } @@ -292,7 +315,17 @@ export class BtmsgDb { })); } + /** + * Fix #13 (Codex audit): Validate sender is a member of the channel. + */ sendChannelMessage(channelId: string, fromAgent: string, content: string): string { + const member = this.db.query<{ agent_id: string }, [string, string]>( + "SELECT agent_id FROM channel_members WHERE channel_id = ? AND agent_id = ?" + ).get(channelId, fromAgent); + if (!member) { + throw new Error(`Agent '${fromAgent}' is not a member of channel '${channelId}'`); + } + const id = randomUUID(); this.db.query( `INSERT INTO channel_messages (id, channel_id, from_agent, content) diff --git a/ui-electrobun/src/bun/bttask-db.ts b/ui-electrobun/src/bun/bttask-db.ts index c5663e0..6b65b2b 100644 --- a/ui-electrobun/src/bun/bttask-db.ts +++ b/ui-electrobun/src/bun/bttask-db.ts @@ -137,14 +137,26 @@ export class BttaskDb { return expectedVersion + 1; } + /** Fix #9 (Codex audit): Wrap delete in transaction for atomicity. */ deleteTask(taskId: string): void { - this.db.query("DELETE FROM task_comments WHERE task_id = ?").run(taskId); - this.db.query("DELETE FROM tasks WHERE id = ?").run(taskId); + const tx = this.db.transaction(() => { + this.db.query("DELETE FROM task_comments WHERE task_id = ?").run(taskId); + this.db.query("DELETE FROM tasks WHERE id = ?").run(taskId); + }); + tx(); } // ── Comments ───────────────────────────────────────────────────────────── + /** Fix #9 (Codex audit): Validate task exists before adding comment. */ addComment(taskId: string, agentId: string, content: string): string { + const task = this.db.query<{ id: string }, [string]>( + "SELECT id FROM tasks WHERE id = ?" + ).get(taskId); + if (!task) { + throw new Error(`Task '${taskId}' not found`); + } + const id = randomUUID(); this.db.query( `INSERT INTO task_comments (id, task_id, agent_id, content) diff --git a/ui-electrobun/src/bun/handlers/path-guard.ts b/ui-electrobun/src/bun/handlers/path-guard.ts index 74405c7..a9c6bfd 100644 --- a/ui-electrobun/src/bun/handlers/path-guard.ts +++ b/ui-electrobun/src/bun/handlers/path-guard.ts @@ -5,6 +5,7 @@ */ import path from "path"; +import fs from "fs"; import { settingsDb } from "../settings-db.ts"; import { homedir } from "os"; import { join } from "path"; @@ -29,14 +30,36 @@ function getAllowedRoots(): string[] { /** * Validate that a file path is within an allowed boundary. - * Returns the resolved path if valid, or null if outside boundaries. + * Fix #3 (Codex audit): Uses realpathSync to resolve symlinks, preventing + * symlink-based traversal attacks (CWE-59). + * Returns the resolved real path if valid, or null if outside boundaries. */ export function validatePath(filePath: string): string | null { - const resolved = path.resolve(filePath); + let resolved: string; + try { + // Resolve symlinks to their actual target to prevent symlink traversal + resolved = fs.realpathSync(path.resolve(filePath)); + } catch { + // If the file doesn't exist yet, resolve without symlink resolution + // but only allow if the parent directory resolves within boundaries + const parent = path.dirname(path.resolve(filePath)); + try { + const realParent = fs.realpathSync(parent); + resolved = path.join(realParent, path.basename(filePath)); + } catch { + return null; // Parent doesn't exist either — reject + } + } + const roots = getAllowedRoots(); for (const root of roots) { - const resolvedRoot = path.resolve(root); + let resolvedRoot: string; + try { + resolvedRoot = fs.realpathSync(path.resolve(root)); + } catch { + resolvedRoot = path.resolve(root); + } if (resolved === resolvedRoot || resolved.startsWith(resolvedRoot + path.sep)) { return resolved; } diff --git a/ui-electrobun/src/bun/handlers/remote-handlers.ts b/ui-electrobun/src/bun/handlers/remote-handlers.ts index cf359be..faba79f 100644 --- a/ui-electrobun/src/bun/handlers/remote-handlers.ts +++ b/ui-electrobun/src/bun/handlers/remote-handlers.ts @@ -6,10 +6,11 @@ import type { RelayClient } from "../relay-client.ts"; export function createRemoteHandlers(relayClient: RelayClient) { return { + // Fix #4 (Codex audit): relay-client.connect() now returns { ok, machineId, error } "remote.connect": async ({ url, token, label }: { url: string; token: string; label?: string }) => { try { - const machineId = await relayClient.connect(url, token, label); - return { ok: true, machineId }; + const result = await relayClient.connect(url, token, label); + return result; } catch (err) { const error = err instanceof Error ? err.message : String(err); console.error("[remote.connect]", err); @@ -28,6 +29,18 @@ export function createRemoteHandlers(relayClient: RelayClient) { } }, + // Fix #6 (Codex audit): Add remote.remove RPC that disconnects AND deletes + "remote.remove": ({ machineId }: { machineId: string }) => { + try { + relayClient.removeMachine(machineId); + return { ok: true }; + } catch (err) { + const error = err instanceof Error ? err.message : String(err); + console.error("[remote.remove]", err); + return { ok: false, error }; + } + }, + "remote.list": () => { try { return { machines: relayClient.listMachines() }; diff --git a/ui-electrobun/src/bun/pty-client.ts b/ui-electrobun/src/bun/pty-client.ts index eea1538..5c8d51a 100644 --- a/ui-electrobun/src/bun/pty-client.ts +++ b/ui-electrobun/src/bun/pty-client.ts @@ -25,7 +25,7 @@ export interface SessionInfo { export type DaemonEvent = | { type: "auth_result"; ok: boolean } | { type: "session_created"; session_id: string; pid: number } - /** data is base64-encoded bytes from the PTY. */ + /** data is base64-encoded raw bytes from the PTY daemon. Decoded to Uint8Array by the consumer. */ | { type: "session_output"; session_id: string; data: string } | { type: "session_closed"; session_id: string; exit_code: number | null } | { type: "session_list"; sessions: SessionInfo[] } @@ -60,6 +60,9 @@ export class PtyClient extends EventEmitter { /** Connect to daemon and authenticate. Fix #10: 5-second timeout. */ async connect(): Promise { + // Fix #10 (Codex audit): Clear stale buffer from any previous connection + this.buffer = ""; + return new Promise((resolve, reject) => { let token: string; try { @@ -157,14 +160,24 @@ export class PtyClient extends EventEmitter { }); } - /** Write input to a session's PTY. data is encoded as base64 for transport. */ + /** + * Write input to a session's PTY. data is encoded as base64 for transport. + * Fix #11 (Codex audit): Uses chunked approach instead of String.fromCharCode(...spread) + * which can throw RangeError on large pastes (>~65K bytes). + */ writeInput(sessionId: string, data: string | Uint8Array): void { const bytes = typeof data === "string" ? new TextEncoder().encode(data) : data; // Daemon expects base64 string per protocol.rs WriteInput { data: String } - const b64 = btoa(String.fromCharCode(...bytes)); + const CHUNK_SIZE = 8192; + let binary = ""; + for (let i = 0; i < bytes.length; i += CHUNK_SIZE) { + const chunk = bytes.subarray(i, Math.min(i + CHUNK_SIZE, bytes.length)); + binary += String.fromCharCode(...chunk); + } + const b64 = btoa(binary); this.send({ type: "write_input", session_id: sessionId, data: b64 }); } diff --git a/ui-electrobun/src/bun/relay-client.ts b/ui-electrobun/src/bun/relay-client.ts index 784572d..a43e63a 100644 --- a/ui-electrobun/src/bun/relay-client.ts +++ b/ui-electrobun/src/bun/relay-client.ts @@ -63,8 +63,12 @@ export class RelayClient { this.statusListeners.push(cb); } - /** Connect to an agor-relay instance. Returns a machine ID. */ - async connect(url: string, token: string, label?: string): Promise { + /** + * Connect to an agor-relay instance. + * Fix #4 (Codex audit): Returns { ok, machineId, error } instead of always + * returning machineId even on failure. + */ + async connect(url: string, token: string, label?: string): Promise<{ ok: boolean; machineId?: string; error?: string }> { const machineId = randomUUID(); const machine: MachineConnection = { machineId, @@ -84,14 +88,14 @@ export class RelayClient { try { await this.openWebSocket(machine); + return { ok: true, machineId }; } catch (err) { const msg = err instanceof Error ? err.message : String(err); machine.status = "error"; this.emitStatus(machineId, "error", msg); this.scheduleReconnect(machine); + return { ok: false, machineId, error: msg }; } - - return machineId; } /** Disconnect from a relay and stop reconnection attempts. */ @@ -299,16 +303,24 @@ export class RelayClient { machine.reconnectTimer = setTimeout(attempt, delay); } - /** TCP-only probe to check if the relay host is reachable. */ - private tcpProbe(url: string): Promise { + /** + * TCP-only probe to check if the relay host is reachable. + * Fix #15 (Codex audit): Uses URL() to correctly parse IPv6, ports, etc. + */ + private tcpProbe(wsUrl: string): Promise { return new Promise((resolve) => { - const host = this.extractHost(url); - if (!host) { resolve(false); return; } - - const [hostname, portStr] = host.includes(":") - ? [host.split(":")[0], host.split(":")[1]] - : [host, "9750"]; - const port = parseInt(portStr, 10); + let hostname: string; + let port: number; + try { + // Convert ws/wss to http/https so URL() can parse it + const httpUrl = wsUrl.replace(/^ws(s)?:\/\//, "http$1://"); + const parsed = new URL(httpUrl); + hostname = parsed.hostname; // strips IPv6 brackets automatically + port = parsed.port ? parseInt(parsed.port, 10) : 9750; + } catch { + resolve(false); + return; + } const socket = new Socket(); const timer = setTimeout(() => { socket.destroy(); resolve(false); }, 5_000); @@ -327,10 +339,6 @@ export class RelayClient { }); } - private extractHost(url: string): string | null { - return url.replace("wss://", "").replace("ws://", "").split("/")[0] ?? null; - } - private emitStatus(machineId: string, status: ConnectionStatus, error?: string): void { for (const cb of this.statusListeners) { try { diff --git a/ui-electrobun/src/bun/sidecar-manager.ts b/ui-electrobun/src/bun/sidecar-manager.ts index 2cd62ea..6b9feeb 100644 --- a/ui-electrobun/src/bun/sidecar-manager.ts +++ b/ui-electrobun/src/bun/sidecar-manager.ts @@ -134,6 +134,8 @@ function findNodeRuntime(): string { // ── Cleanup grace period ───────────────────────────────────────────────────── const CLEANUP_GRACE_MS = 60_000; // 60s after done/error before removing session +// Fix #12 (Codex audit): Max NDJSON line size — prevent OOM on malformed output +const MAX_LINE_SIZE = 10 * 1024 * 1024; // 10 MB // ── SidecarManager ─────────────────────────────────────────────────────────── @@ -369,6 +371,13 @@ export class SidecarManager { for await (const chunk of reader) { buffer += decoder.decode(chunk, { stream: true }); + // Fix #12 (Codex audit): Guard against unbounded buffer growth + if (buffer.length > MAX_LINE_SIZE && !buffer.includes("\n")) { + console.error(`[sidecar] Buffer exceeded ${MAX_LINE_SIZE} bytes without newline for ${sessionId}, truncating`); + buffer = ""; + continue; + } + let newlineIdx: number; while ((newlineIdx = buffer.indexOf("\n")) !== -1) { const line = buffer.slice(0, newlineIdx).trim(); @@ -379,7 +388,7 @@ export class SidecarManager { } } - // Fix #12: Parse any residual data left in the buffer after stream ends + // Parse any residual data left in the buffer after stream ends const residual = buffer.trim(); if (residual) { this.handleNdjsonLine(sessionId, session, residual); diff --git a/ui-electrobun/src/mainview/TaskBoardTab.svelte b/ui-electrobun/src/mainview/TaskBoardTab.svelte index 2a0223f..3d9a4b7 100644 --- a/ui-electrobun/src/mainview/TaskBoardTab.svelte +++ b/ui-electrobun/src/mainview/TaskBoardTab.svelte @@ -46,6 +46,9 @@ let draggedTaskId = $state(null); let dragOverCol = $state(null); + // Fix #7 (Codex audit): Poll token to discard stale responses + let pollToken = $state(0); + // ── Derived ────────────────────────────────────────────────────────── let tasksByCol = $derived( @@ -58,8 +61,11 @@ // ── Data fetching ──────────────────────────────────────────────────── async function loadTasks() { + // Fix #7 (Codex audit): Capture token before async call, discard if stale + const tokenAtStart = ++pollToken; try { const res = await appRpc.request['bttask.listTasks']({ groupId }); + if (tokenAtStart < pollToken) return; // Stale response — discard tasks = res.tasks; } catch (err) { console.error('[TaskBoard] loadTasks:', err); @@ -98,6 +104,7 @@ const task = tasks.find(t => t.id === taskId); if (!task || task.status === newStatus) return; + pollToken++; // Fix #7: Invalidate in-flight polls try { const res = await appRpc.request['bttask.updateTaskStatus']({ taskId, diff --git a/ui-electrobun/src/mainview/agent-store.svelte.ts b/ui-electrobun/src/mainview/agent-store.svelte.ts index a8128d5..b36ac88 100644 --- a/ui-electrobun/src/mainview/agent-store.svelte.ts +++ b/ui-electrobun/src/mainview/agent-store.svelte.ts @@ -259,6 +259,8 @@ function ensureListeners() { } persistMessages(session); scheduleCleanup(session.sessionId, session.projectId); + // Fix #14 (Codex audit): Enforce max sessions per project on completion + enforceMaxSessions(session.projectId); } }); diff --git a/ui-electrobun/src/mainview/health-store.svelte.ts b/ui-electrobun/src/mainview/health-store.svelte.ts index 704de6a..5327b7d 100644 --- a/ui-electrobun/src/mainview/health-store.svelte.ts +++ b/ui-electrobun/src/mainview/health-store.svelte.ts @@ -36,7 +36,8 @@ interface ProjectTracker { projectId: string; lastActivityTs: number; lastToolName: string | null; - toolInFlight: boolean; + // Fix #8 (Codex audit): Counter instead of boolean for concurrent tools + toolsInFlight: number; costSnapshots: Array<[number, number]>; // [timestamp, costUsd] totalTokens: number; totalCost: number; @@ -87,7 +88,7 @@ function computeHealth(tracker: ProjectTracker, now: number): ProjectHealth { if (tracker.status === 'inactive' || tracker.status === 'done' || tracker.status === 'error') { activityState = 'inactive'; - } else if (tracker.toolInFlight) { + } else if (tracker.toolsInFlight > 0) { activityState = 'running'; activeTool = tracker.lastToolName; } else { @@ -125,7 +126,7 @@ export function trackProject(projectId: string): void { projectId, lastActivityTs: Date.now(), lastToolName: null, - toolInFlight: false, + toolsInFlight: 0, costSnapshots: [], totalTokens: 0, totalCost: 0, @@ -142,7 +143,8 @@ export function recordActivity(projectId: string, toolName?: string): void { t.status = 'running'; if (toolName !== undefined) { t.lastToolName = toolName; - t.toolInFlight = true; + // Fix #8 (Codex audit): Increment counter for concurrent tool tracking + t.toolsInFlight++; } if (!tickInterval) startHealthTick(); } @@ -152,7 +154,8 @@ export function recordToolDone(projectId: string): void { const t = trackers.get(projectId); if (!t) return; t.lastActivityTs = Date.now(); - t.toolInFlight = false; + // Fix #8 (Codex audit): Decrement counter, floor at 0 + t.toolsInFlight = Math.max(0, t.toolsInFlight - 1); } /** Record a token/cost snapshot for burn rate calculation. */ @@ -220,6 +223,16 @@ function startHealthTick(): void { }, TICK_INTERVAL_MS); } +/** Untrack a project (e.g. when project is removed). Fix #14 (Codex audit). */ +export function untrackProject(projectId: string): void { + trackers.delete(projectId); + // Stop tick if no more tracked projects + if (trackers.size === 0 && tickInterval) { + clearInterval(tickInterval); + tickInterval = null; + } +} + /** Stop the health tick timer. */ export function stopHealthTick(): void { if (tickInterval) { diff --git a/ui-electrobun/src/mainview/settings/RemoteMachinesSettings.svelte b/ui-electrobun/src/mainview/settings/RemoteMachinesSettings.svelte index 0abb4ef..d603956 100644 --- a/ui-electrobun/src/mainview/settings/RemoteMachinesSettings.svelte +++ b/ui-electrobun/src/mainview/settings/RemoteMachinesSettings.svelte @@ -84,10 +84,11 @@ } catch { /* ignore */ } } + // Fix #6 (Codex audit): Use remote.remove RPC that disconnects AND deletes async function handleRemove(machineId: string) { try { - await appRpc.request['remote.disconnect']({ machineId }); - } catch { /* may already be disconnected */ } + await appRpc.request['remote.remove']({ machineId }); + } catch { /* may already be disconnected/removed */ } removeMachine(machineId); } @@ -131,6 +132,9 @@ {statusLabel(m.status)} + {#if m.error} + {m.error} + {/if}
{#if m.status === 'connected'} @@ -239,6 +243,7 @@ } .status-text { font-size: 0.6875rem; font-weight: 500; } + .machine-error { font-size: 0.625rem; color: var(--ctp-red); max-width: 10rem; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } .machine-actions { display: flex; gap: 0.25rem; flex-shrink: 0; } diff --git a/ui-electrobun/src/shared/pty-rpc-schema.ts b/ui-electrobun/src/shared/pty-rpc-schema.ts index 9df4586..5fab99a 100644 --- a/ui-electrobun/src/shared/pty-rpc-schema.ts +++ b/ui-electrobun/src/shared/pty-rpc-schema.ts @@ -23,11 +23,16 @@ export type PtyRPCRequests = { }; response: { ok: boolean; error?: string }; }; - /** Write raw input bytes (base64-encoded) to a PTY session. */ + /** + * Write input to a PTY session. + * `data` is raw UTF-8 text from the user (xterm onData). The pty-client + * layer encodes it to base64 before sending to the daemon; this RPC boundary + * carries raw text which the Bun handler forwards to PtyClient.writeInput(). + */ "pty.write": { params: { sessionId: string; - /** UTF-8 text typed by the user (xterm onData delivers this). */ + /** Raw UTF-8 text typed by the user (xterm onData delivers this). Encoded to base64 by pty-client before daemon transport. */ data: string; }; response: { ok: boolean }; @@ -549,11 +554,16 @@ export type PtyRPCRequests = { params: { url: string; token: string; label?: string }; response: { ok: boolean; machineId?: string; error?: string }; }; - /** Disconnect from a relay instance. */ + /** Disconnect from a relay instance (keeps machine in list for reconnect). */ "remote.disconnect": { params: { machineId: string }; response: { ok: boolean; error?: string }; }; + /** Remove a machine entirely — disconnects AND deletes from tracking. */ + "remote.remove": { + params: { machineId: string }; + response: { ok: boolean; error?: string }; + }; /** List all known remote machines with connection status. */ "remote.list": { params: Record;