fix(security): audit fixes — path traversal, race conditions, memory leaks, transaction safety

- lib.rs: claude_read_skill path traversal prevention (canonicalize + starts_with)
- agent-dispatcher.ts: re-entrancy guard on exit handler, clear maps in stop
- machines.svelte.ts: track UnlistenFn array + destroyMachineListeners()
- agent-runner.ts: controller.signal.aborted, async handleMessage + .catch()
- remote.rs: try_lock → async lock, abort tasks on remove
- session.rs: unchecked_transaction for save_agent_messages
- agent-bridge.ts: safe msg.event check (implicit in dispatcher changes)
This commit is contained in:
Hibryda 2026-03-08 20:03:50 +01:00
parent 73ca780b54
commit 4bdb74721d
6 changed files with 102 additions and 57 deletions

View file

@ -26,7 +26,9 @@ function log(message: string) {
rl.on('line', (line: string) => { rl.on('line', (line: string) => {
try { try {
const msg = JSON.parse(line); const msg = JSON.parse(line);
handleMessage(msg); handleMessage(msg).catch((err: unknown) => {
log(`Unhandled error in message handler: ${err}`);
});
} catch { } catch {
log(`Invalid JSON: ${line}`); log(`Invalid JSON: ${line}`);
} }
@ -53,13 +55,13 @@ interface StopMessage {
sessionId: string; sessionId: string;
} }
function handleMessage(msg: Record<string, unknown>) { async function handleMessage(msg: Record<string, unknown>) {
switch (msg.type) { switch (msg.type) {
case 'ping': case 'ping':
send({ type: 'pong' }); send({ type: 'pong' });
break; break;
case 'query': case 'query':
handleQuery(msg as unknown as QueryMessage); await handleQuery(msg as unknown as QueryMessage);
break; break;
case 'stop': case 'stop':
handleStop(msg as unknown as StopMessage); handleStop(msg as unknown as StopMessage);
@ -149,7 +151,7 @@ async function handleQuery(msg: QueryMessage) {
sessions.delete(sessionId); sessions.delete(sessionId);
const errMsg = err instanceof Error ? err.message : String(err); const errMsg = err instanceof Error ? err.message : String(err);
if (errMsg.includes('aborted') || errMsg.includes('AbortError')) { if (controller.signal.aborted) {
log(`Agent session ${sessionId} aborted`); log(`Agent session ${sessionId} aborted`);
send({ send({
type: 'agent_stopped', type: 'agent_stopped',

View file

@ -364,7 +364,19 @@ fn claude_list_skills() -> Vec<ClaudeSkill> {
#[tauri::command] #[tauri::command]
fn claude_read_skill(path: String) -> Result<String, String> { fn claude_read_skill(path: String) -> Result<String, String> {
std::fs::read_to_string(&path).map_err(|e| format!("Failed to read skill: {e}")) // Validate path is under ~/.claude/skills/ to prevent path traversal
let skills_dir = dirs::home_dir()
.ok_or("Cannot determine home directory")?
.join(".claude")
.join("skills");
let canonical_skills = skills_dir.canonicalize()
.map_err(|_| "Skills directory does not exist".to_string())?;
let canonical_path = std::path::Path::new(&path).canonicalize()
.map_err(|e| format!("Invalid skill path: {e}"))?;
if !canonical_path.starts_with(&canonical_skills) {
return Err("Access denied: path is outside skills directory".to_string());
}
std::fs::read_to_string(&canonical_path).map_err(|e| format!("Failed to read skill: {e}"))
} }
// --- Group config commands (v3) --- // --- Group config commands (v3) ---
@ -458,18 +470,18 @@ fn cli_get_group() -> Option<String> {
// --- Remote machine commands --- // --- Remote machine commands ---
#[tauri::command] #[tauri::command]
fn remote_list(state: State<'_, AppState>) -> Vec<remote::RemoteMachineInfo> { async fn remote_list(state: State<'_, AppState>) -> Result<Vec<remote::RemoteMachineInfo>, String> {
state.remote_manager.list_machines() Ok(state.remote_manager.list_machines().await)
} }
#[tauri::command] #[tauri::command]
async fn remote_add(state: State<'_, AppState>, config: RemoteMachineConfig) -> Result<String, String> { async fn remote_add(state: State<'_, AppState>, config: RemoteMachineConfig) -> Result<String, String> {
Ok(state.remote_manager.add_machine(config)) Ok(state.remote_manager.add_machine(config).await)
} }
#[tauri::command] #[tauri::command]
async fn remote_remove(state: State<'_, AppState>, machine_id: String) -> Result<(), String> { async fn remote_remove(state: State<'_, AppState>, machine_id: String) -> Result<(), String> {
state.remote_manager.remove_machine(&machine_id) state.remote_manager.remove_machine(&machine_id).await
} }
#[tauri::command] #[tauri::command]

View file

@ -69,22 +69,18 @@ impl RemoteManager {
} }
} }
pub fn list_machines(&self) -> Vec<RemoteMachineInfo> { pub async fn list_machines(&self) -> Vec<RemoteMachineInfo> {
// Use try_lock for sync context (called from Tauri command handler) let machines = self.machines.lock().await;
let machines = self.machines.try_lock(); machines.values().map(|m| RemoteMachineInfo {
match machines { id: m.id.clone(),
Ok(m) => m.values().map(|m| RemoteMachineInfo { label: m.config.label.clone(),
id: m.id.clone(), url: m.config.url.clone(),
label: m.config.label.clone(), status: m.status.clone(),
url: m.config.url.clone(), auto_connect: m.config.auto_connect,
status: m.status.clone(), }).collect()
auto_connect: m.config.auto_connect,
}).collect(),
Err(_) => Vec::new(),
}
} }
pub fn add_machine(&self, config: RemoteMachineConfig) -> String { pub async fn add_machine(&self, config: RemoteMachineConfig) -> String {
let id = uuid::Uuid::new_v4().to_string(); let id = uuid::Uuid::new_v4().to_string();
let machine = RemoteMachine { let machine = RemoteMachine {
id: id.clone(), id: id.clone(),
@ -92,16 +88,18 @@ impl RemoteManager {
status: "disconnected".to_string(), status: "disconnected".to_string(),
connection: None, connection: None,
}; };
// Use try_lock for sync context self.machines.lock().await.insert(id.clone(), machine);
if let Ok(mut machines) = self.machines.try_lock() {
machines.insert(id.clone(), machine);
}
id id
} }
pub fn remove_machine(&self, machine_id: &str) -> Result<(), String> { pub async fn remove_machine(&self, machine_id: &str) -> Result<(), String> {
let mut machines = self.machines.try_lock() let mut machines = self.machines.lock().await;
.map_err(|_| "Lock contention".to_string())?; if let Some(machine) = machines.get_mut(machine_id) {
// Abort connection tasks before removing to prevent resource leaks
if let Some(conn) = machine.connection.take() {
conn._handle.abort();
}
}
machines.remove(machine_id) machines.remove(machine_id)
.ok_or_else(|| format!("Machine {machine_id} not found"))?; .ok_or_else(|| format!("Machine {machine_id} not found"))?;
Ok(()) Ok(())

View file

@ -373,13 +373,17 @@ impl SessionDb {
messages: &[AgentMessageRecord], messages: &[AgentMessageRecord],
) -> Result<(), String> { ) -> Result<(), String> {
let conn = self.conn.lock().unwrap(); let conn = self.conn.lock().unwrap();
// Wrap DELETE+INSERTs in a transaction to prevent partial writes on crash
let tx = conn.unchecked_transaction()
.map_err(|e| format!("Begin transaction failed: {e}"))?;
// Clear previous messages for this session // Clear previous messages for this session
conn.execute( tx.execute(
"DELETE FROM agent_messages WHERE session_id = ?1", "DELETE FROM agent_messages WHERE session_id = ?1",
params![session_id], params![session_id],
).map_err(|e| format!("Delete old messages failed: {e}"))?; ).map_err(|e| format!("Delete old messages failed: {e}"))?;
let mut stmt = conn.prepare( let mut stmt = tx.prepare(
"INSERT INTO agent_messages (session_id, project_id, sdk_session_id, message_type, content, parent_id, created_at) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)" "INSERT INTO agent_messages (session_id, project_id, sdk_session_id, message_type, content, parent_id, created_at) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)"
).map_err(|e| format!("Prepare insert failed: {e}"))?; ).map_err(|e| format!("Prepare insert failed: {e}"))?;
@ -394,6 +398,8 @@ impl SessionDb {
msg.created_at, msg.created_at,
]).map_err(|e| format!("Insert message failed: {e}"))?; ]).map_err(|e| format!("Insert message failed: {e}"))?;
} }
drop(stmt);
tx.commit().map_err(|e| format!("Commit failed: {e}"))?;
Ok(()) Ok(())
} }

View file

@ -39,6 +39,7 @@ let sidecarAlive = true;
// Sidecar crash recovery state // Sidecar crash recovery state
const MAX_RESTART_ATTEMPTS = 3; const MAX_RESTART_ATTEMPTS = 3;
let restartAttempts = 0; let restartAttempts = 0;
let restarting = false;
export function isSidecarAlive(): boolean { export function isSidecarAlive(): boolean {
return sidecarAlive; return sidecarAlive;
} }
@ -68,7 +69,7 @@ export async function startAgentDispatcher(): Promise<void> {
break; break;
case 'agent_event': case 'agent_event':
handleAgentEvent(sessionId, msg.event!); if (msg.event) handleAgentEvent(sessionId, msg.event);
break; break;
case 'agent_stopped': case 'agent_stopped':
@ -88,6 +89,11 @@ export async function startAgentDispatcher(): Promise<void> {
unlistenExit = await onSidecarExited(async () => { unlistenExit = await onSidecarExited(async () => {
sidecarAlive = false; sidecarAlive = false;
// Guard against re-entrant exit handler (double-restart race)
if (restarting) return;
restarting = true;
// Mark all running sessions as errored // Mark all running sessions as errored
for (const session of getAgentSessions()) { for (const session of getAgentSessions()) {
if (session.status === 'running' || session.status === 'starting') { if (session.status === 'running' || session.status === 'starting') {
@ -96,22 +102,26 @@ export async function startAgentDispatcher(): Promise<void> {
} }
// Attempt auto-restart with exponential backoff // Attempt auto-restart with exponential backoff
if (restartAttempts < MAX_RESTART_ATTEMPTS) { try {
restartAttempts++; if (restartAttempts < MAX_RESTART_ATTEMPTS) {
const delayMs = 1000 * Math.pow(2, restartAttempts - 1); // 1s, 2s, 4s restartAttempts++;
notify('warning', `Sidecar crashed, restarting (attempt ${restartAttempts}/${MAX_RESTART_ATTEMPTS})...`); const delayMs = 1000 * Math.pow(2, restartAttempts - 1); // 1s, 2s, 4s
await new Promise((resolve) => setTimeout(resolve, delayMs)); notify('warning', `Sidecar crashed, restarting (attempt ${restartAttempts}/${MAX_RESTART_ATTEMPTS})...`);
try { await new Promise((resolve) => setTimeout(resolve, delayMs));
await restartAgent(); try {
sidecarAlive = true; await restartAgent();
// Note: restartAttempts is reset when next sidecar message arrives sidecarAlive = true;
} catch { // Note: restartAttempts is reset when next sidecar message arrives
if (restartAttempts >= MAX_RESTART_ATTEMPTS) { } catch {
notify('error', `Sidecar restart failed after ${MAX_RESTART_ATTEMPTS} attempts`); if (restartAttempts >= MAX_RESTART_ATTEMPTS) {
notify('error', `Sidecar restart failed after ${MAX_RESTART_ATTEMPTS} attempts`);
}
} }
} else {
notify('error', `Sidecar restart failed after ${MAX_RESTART_ATTEMPTS} attempts`);
} }
} else { } finally {
notify('error', `Sidecar restart failed after ${MAX_RESTART_ATTEMPTS} attempts`); restarting = false;
} }
}); });
} }
@ -300,4 +310,7 @@ export function stopAgentDispatcher(): void {
unlistenExit(); unlistenExit();
unlistenExit = null; unlistenExit = null;
} }
// Clear routing maps to prevent unbounded memory growth
toolUseToChildPane.clear();
sessionProjectMap.clear();
} }

View file

@ -71,41 +71,47 @@ export async function disconnectMachine(id: string): Promise<void> {
if (machine) machine.status = 'disconnected'; if (machine) machine.status = 'disconnected';
} }
// Stored unlisten functions for cleanup
let unlistenFns: (() => void)[] = [];
// Initialize event listeners for machine status updates // Initialize event listeners for machine status updates
export async function initMachineListeners(): Promise<void> { export async function initMachineListeners(): Promise<void> {
await onRemoteMachineReady((msg) => { // Clean up any existing listeners first
destroyMachineListeners();
unlistenFns.push(await onRemoteMachineReady((msg) => {
const machine = machines.find(m => m.id === msg.machineId); const machine = machines.find(m => m.id === msg.machineId);
if (machine) { if (machine) {
machine.status = 'connected'; machine.status = 'connected';
notify('success', `Connected to ${machine.label}`); notify('success', `Connected to ${machine.label}`);
} }
}); }));
await onRemoteMachineDisconnected((msg) => { unlistenFns.push(await onRemoteMachineDisconnected((msg) => {
const machine = machines.find(m => m.id === msg.machineId); const machine = machines.find(m => m.id === msg.machineId);
if (machine) { if (machine) {
machine.status = 'disconnected'; machine.status = 'disconnected';
notify('warning', `Disconnected from ${machine.label}`); notify('warning', `Disconnected from ${machine.label}`);
} }
}); }));
await onRemoteError((msg) => { unlistenFns.push(await onRemoteError((msg) => {
const machine = machines.find(m => m.id === msg.machineId); const machine = machines.find(m => m.id === msg.machineId);
if (machine) { if (machine) {
machine.status = 'error'; machine.status = 'error';
notify('error', `Error from ${machine.label}: ${msg.error}`); notify('error', `Error from ${machine.label}: ${msg.error}`);
} }
}); }));
await onRemoteMachineReconnecting((msg) => { unlistenFns.push(await onRemoteMachineReconnecting((msg) => {
const machine = machines.find(m => m.id === msg.machineId); const machine = machines.find(m => m.id === msg.machineId);
if (machine) { if (machine) {
machine.status = 'reconnecting'; machine.status = 'reconnecting';
notify('info', `Reconnecting to ${machine.label} in ${msg.backoffSecs}s…`); notify('info', `Reconnecting to ${machine.label} in ${msg.backoffSecs}s…`);
} }
}); }));
await onRemoteMachineReconnectReady((msg) => { unlistenFns.push(await onRemoteMachineReconnectReady((msg) => {
const machine = machines.find(m => m.id === msg.machineId); const machine = machines.find(m => m.id === msg.machineId);
if (machine) { if (machine) {
notify('info', `${machine.label} reachable — reconnecting…`); notify('info', `${machine.label} reachable — reconnecting…`);
@ -113,5 +119,13 @@ export async function initMachineListeners(): Promise<void> {
notify('error', `Auto-reconnect failed for ${machine.label}: ${e}`); notify('error', `Auto-reconnect failed for ${machine.label}: ${e}`);
}); });
} }
}); }));
}
/** Remove all event listeners to prevent leaks */
export function destroyMachineListeners(): void {
for (const unlisten of unlistenFns) {
unlisten();
}
unlistenFns = [];
} }