From e2858c9a172e82dbed698a4e6110f071ae766077 Mon Sep 17 00:00:00 2001 From: Thomas Marchand Date: Sun, 18 Jan 2026 19:47:42 +0000 Subject: [PATCH] fix: address security and UX issues from review 1. Fix path traversal vulnerability in fs.rs - Canonicalize paths to resolve ".." sequences and symlinks - Validate that resolved paths remain within workspace boundaries - Return 403 Forbidden for path traversal attempts 2. Fix OpenCode hidden agents filter bypass in new-mission-dialog - Apply hidden_agents filter from OpenAgentConfig to OpenCode agents - Matches existing behavior for Claude Code agents --- .../src/components/new-mission-dialog.tsx | 7 +- src/api/fs.rs | 82 ++++++++++++++++--- src/api/mission_runner.rs | 14 ++-- 3 files changed, 81 insertions(+), 22 deletions(-) diff --git a/dashboard/src/components/new-mission-dialog.tsx b/dashboard/src/components/new-mission-dialog.tsx index dbc6cf9..3239458 100644 --- a/dashboard/src/components/new-mission-dialog.tsx +++ b/dashboard/src/components/new-mission-dialog.tsx @@ -123,13 +123,16 @@ export function NewMissionDialog({ // Combine all agents from enabled backends const allAgents = useMemo((): CombinedAgent[] => { const result: CombinedAgent[] = []; + const openCodeHiddenAgents = config?.hidden_agents || []; const claudeCodeHiddenAgents = claudeCodeLibConfig?.hidden_agents || []; for (const backend of enabledBackends) { let agentNames: string[] = []; if (backend.id === 'opencode') { - agentNames = opencodeAgents?.map(a => a.name) || parseAgentNames(agentsPayload); + // Filter out hidden OpenCode agents + const allOpenCodeAgents = opencodeAgents?.map(a => a.name) || parseAgentNames(agentsPayload); + agentNames = allOpenCodeAgents.filter(name => !openCodeHiddenAgents.includes(name)); } else if (backend.id === 'claudecode') { // Filter out hidden Claude Code agents const allClaudeAgents = claudecodeAgents?.map(a => a.name) || []; @@ -147,7 +150,7 @@ export function NewMissionDialog({ } return result; - }, [enabledBackends, opencodeAgents, claudecodeAgents, agentsPayload, claudeCodeLibConfig]); + }, [enabledBackends, opencodeAgents, claudecodeAgents, agentsPayload, config, claudeCodeLibConfig]); // Group agents by backend for display const agentsByBackend = useMemo(() => { diff --git a/src/api/fs.rs b/src/api/fs.rs index 1d2ffa7..e5daf7c 100644 --- a/src/api/fs.rs +++ b/src/api/fs.rs @@ -178,29 +178,85 @@ async fn resolve_path_for_workspace( ) })?; + let workspace_root = workspace.path.canonicalize().map_err(|e| { + ( + StatusCode::INTERNAL_SERVER_ERROR, + format!("Failed to canonicalize workspace path: {}", e), + ) + })?; + let input = Path::new(path); - // If the path is absolute, use it directly (but validate it's within workspace) - if input.is_absolute() { - return Ok(input.to_path_buf()); - } - - // Resolve relative path against workspace path - // For "context" paths, use the workspace's context directory - if path.starts_with("./context") || path.starts_with("context") { + // Resolve the final path based on input type + let resolved = if input.is_absolute() { + input.to_path_buf() + } else if path.starts_with("./context") || path.starts_with("context") { + // For "context" paths, use the workspace's context directory let suffix = path .trim_start_matches("./") .trim_start_matches("context/") .trim_start_matches("context"); - let context_path = workspace.path.join("context"); + let context_path = workspace_root.join("context"); if suffix.is_empty() { - return Ok(context_path); + context_path + } else { + context_path.join(suffix) } - return Ok(context_path.join(suffix)); + } else { + // Default: resolve relative to workspace path + workspace_root.join(path) + }; + + // Canonicalize to resolve ".." and symlinks, then validate within workspace + // For non-existent paths, we validate the parent directory exists and is within workspace + let canonical = if resolved.exists() { + resolved.canonicalize().map_err(|e| { + ( + StatusCode::BAD_REQUEST, + format!("Failed to resolve path: {}", e), + ) + })? + } else { + // For new files, check that the parent is within workspace + let parent = resolved.parent().ok_or_else(|| { + ( + StatusCode::BAD_REQUEST, + "Invalid path: no parent directory".to_string(), + ) + })?; + if !parent.exists() { + return Err(( + StatusCode::BAD_REQUEST, + format!("Parent directory does not exist: {}", parent.display()), + )); + } + let canonical_parent = parent.canonicalize().map_err(|e| { + ( + StatusCode::BAD_REQUEST, + format!("Failed to resolve parent path: {}", e), + ) + })?; + // Reconstruct the path with canonical parent + filename + if let Some(filename) = resolved.file_name() { + canonical_parent.join(filename) + } else { + return Err((StatusCode::BAD_REQUEST, "Invalid path".to_string())); + } + }; + + // Validate that the resolved path is within the workspace + if !canonical.starts_with(&workspace_root) { + return Err(( + StatusCode::FORBIDDEN, + format!( + "Path traversal attempt: {} is outside workspace {}", + canonical.display(), + workspace_root.display() + ), + )); } - // Default: resolve relative to workspace path - Ok(workspace.path.join(path)) + Ok(canonical) } fn resolve_upload_base(path: &str) -> Result { diff --git a/src/api/mission_runner.rs b/src/api/mission_runner.rs index 00ad3ad..d20b38b 100644 --- a/src/api/mission_runner.rs +++ b/src/api/mission_runner.rs @@ -760,13 +760,10 @@ pub async fn run_claudecode_turn( for block in evt.message.content { match block { ContentBlock::Text { text } => { + // Text content is the final assistant response + // Don't send as Thinking - it will be in the final AssistantMessage if !text.is_empty() { - final_result = text.clone(); - let _ = events_tx.send(AgentEvent::Thinking { - content: text, - done: false, - mission_id: Some(mission_id), - }); + final_result = text; } } ContentBlock::ToolUse { id, name, input } => { @@ -779,10 +776,13 @@ pub async fn run_claudecode_turn( }); } ContentBlock::Thinking { thinking } => { + // Only send if this is new content not already streamed + // The streaming deltas already accumulated this, so this is + // typically the final complete thinking block if !thinking.is_empty() { let _ = events_tx.send(AgentEvent::Thinking { content: thinking, - done: false, + done: true, // Mark as done since this is the final block mission_id: Some(mission_id), }); }