From f43051ac7480f9a4ae39a1e964c83c2e8179b42c Mon Sep 17 00:00:00 2001 From: Kalvin Chau Date: Mon, 18 May 2026 10:50:32 -0700 Subject: [PATCH 1/2] fix(doctor): harden shell path resolution use shell path-only lookup commands instead of plain which and validate resolved candidates as absolute executable files. apply the same executable validation to shared cli resolvers so function or alias output cannot be treated as a binary path. --- crates/acp-client/README.md | 3 +- crates/acp-client/src/types.rs | 65 ++++++++++--- crates/blox-cli/src/lib.rs | 65 ++++++++++--- crates/doctor/src/resolve.rs | 173 +++++++++++++++++++++++++++++---- 4 files changed, 258 insertions(+), 48 deletions(-) diff --git a/crates/acp-client/README.md b/crates/acp-client/README.md index 900b41fb2..8768c6c60 100644 --- a/crates/acp-client/README.md +++ b/crates/acp-client/README.md @@ -99,8 +99,7 @@ if let Some(agent) = find_acp_agent_by_id("goose") { ## How It Works 1. **Discovery**: Searches for agent executables using: - - Login shell `which` command (to get user's PATH) - - Direct command execution + - Login shell path lookup (to get user's PATH) - Common installation paths (`/opt/homebrew/bin`, `/usr/local/bin`, etc.) 2. **Execution**: diff --git a/crates/acp-client/src/types.rs b/crates/acp-client/src/types.rs index 263b36320..7fe1e43b1 100644 --- a/crates/acp-client/src/types.rs +++ b/crates/acp-client/src/types.rs @@ -171,20 +171,16 @@ const COMMON_PATHS: &[&str] = &[ /// Find a CLI binary by command name. /// /// Searches in order: -/// 1. Login shell `which` (picks up user's PATH from `.zshrc` / `.bashrc`) +/// 1. Login shell path lookup (picks up user's PATH from `.zshrc` / `.bashrc`) /// 2. Common install locations pub fn find_command(cmd: &str) -> Option { - // Strategy 1: Login shell `which` if let Some(path) = find_via_login_shell(cmd) { - if path.exists() { - return Some(path); - } + return Some(path); } - // Strategy 2: Common paths for dir in COMMON_PATHS { let path = PathBuf::from(dir).join(cmd); - if path.exists() { + if is_executable_file(&path) { return Some(path); } } @@ -193,19 +189,22 @@ pub fn find_command(cmd: &str) -> Option { } fn find_via_login_shell(cmd: &str) -> Option { - let which_cmd = format!("which {cmd}"); + let quoted = shell_quote(cmd); + let lookups = [ + ("/bin/zsh", format!("whence -p -- {quoted}")), + ("/bin/bash", format!("type -P -- {quoted}")), + ]; - for shell in &["/bin/zsh", "/bin/bash"] { + for (shell, lookup_cmd) in lookups { if let Ok(output) = std::process::Command::new(shell) - .args(["-l", "-c", &which_cmd]) + .args(["-l", "-c", &lookup_cmd]) .output() { if output.status.success() { let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path_str) = stdout.lines().rfind(|l| !l.is_empty()) { - let path_str = path_str.trim(); - if !path_str.is_empty() && path_str.starts_with('/') { - return Some(PathBuf::from(path_str)); + if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + if is_executable_file(&path) { + return Some(path); } } } @@ -214,6 +213,44 @@ fn find_via_login_shell(cmd: &str) -> Option { None } +fn shell_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) +} + +fn candidate_from_shell_output(output: &str) -> Option { + let mut lines = output + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()); + let candidate = lines.next()?; + if lines.next().is_some() { + return None; + } + + let path = PathBuf::from(candidate); + path.is_absolute().then_some(path) +} + +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = path.metadata() else { + return false; + }; + if !metadata.is_file() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + /// Map an agent ID to the `--command` value for `blox acp`. /// /// Returns `None` if the agent uses the workspace default (no flag needed). diff --git a/crates/blox-cli/src/lib.rs b/crates/blox-cli/src/lib.rs index 1ba1d7e2d..dcaeea86c 100644 --- a/crates/blox-cli/src/lib.rs +++ b/crates/blox-cli/src/lib.rs @@ -4,7 +4,7 @@ use serde::{Deserialize, Deserializer, Serialize}; use std::io::Read; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::time::{Duration, Instant}; use thiserror::Error; @@ -118,18 +118,16 @@ where /// Find a CLI binary by command name. /// /// Searches in order: -/// 1. Login shell `which` (picks up user's PATH from shell rc files) +/// 1. Login shell path lookup (picks up user's PATH from shell rc files) /// 2. Common install locations pub fn find_command(cmd: &str) -> Option { if let Some(path) = find_via_login_shell(cmd) { - if path.exists() { - return Some(path); - } + return Some(path); } for dir in COMMON_PATHS { let path = PathBuf::from(dir).join(cmd); - if path.exists() { + if is_executable_file(&path) { return Some(path); } } @@ -138,16 +136,19 @@ pub fn find_command(cmd: &str) -> Option { } fn find_via_login_shell(cmd: &str) -> Option { - let which_cmd = format!("which {cmd}"); + let quoted = shell_quote(cmd); + let lookups = [ + ("/bin/zsh", format!("whence -p -- {quoted}")), + ("/bin/bash", format!("type -P -- {quoted}")), + ]; - for shell in ["/bin/zsh", "/bin/bash"] { - if let Ok(output) = Command::new(shell).args(["-l", "-c", &which_cmd]).output() { + for (shell, lookup_cmd) in lookups { + if let Ok(output) = Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() { if output.status.success() { let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path_str) = stdout.lines().rfind(|line| !line.is_empty()) { - let path_str = path_str.trim(); - if !path_str.is_empty() && path_str.starts_with('/') { - return Some(PathBuf::from(path_str)); + if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + if is_executable_file(&path) { + return Some(path); } } } @@ -157,6 +158,44 @@ fn find_via_login_shell(cmd: &str) -> Option { None } +fn shell_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) +} + +fn candidate_from_shell_output(output: &str) -> Option { + let mut lines = output + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()); + let candidate = lines.next()?; + if lines.next().is_some() { + return None; + } + + let path = PathBuf::from(candidate); + path.is_absolute().then_some(path) +} + +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = path.metadata() else { + return false; + }; + if !metadata.is_file() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + /// Locate the `sq` binary. pub fn find_sq_binary() -> Option { find_command("sq") diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index cc495e0a6..d2becd7b5 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -1,34 +1,54 @@ //! Binary resolution and command output formatting helpers. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::Command; use super::types::ResolvedBinary; -/// Resolve a binary by trying login shell `which` then common install paths. +/// Resolve a binary by trying login shell path lookup then common install paths. pub fn resolve_binary(cmd: &str) -> ResolvedBinary { let mut lines = vec![format!("resolve '{cmd}':")]; - // Strategy 1: Login shell `which` (primary) - lines.push(" strategy 1 — login shell `which`:".to_string()); - for shell in &["/bin/zsh", "/bin/bash"] { - let which_cmd = format!("which {cmd}"); - match Command::new(shell).args(["-l", "-c", &which_cmd]).output() { + // Strategy 1: Login shell path lookup (primary) + lines.push(" strategy 1 — login shell path lookup:".to_string()); + for (shell, lookup_cmd) in shell_lookup_commands(cmd) { + match Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() { Ok(output) => { - let result = String::from_utf8_lossy(&output.stdout).trim().to_string(); - if output.status.success() && !result.is_empty() { - lines.push(format!( - " {shell} -l -c 'which {cmd}' => {result} (resolved)" - )); - return ResolvedBinary { - path: Some(PathBuf::from(&result)), - search_output: lines.join("\n"), - }; + let stdout = String::from_utf8_lossy(&output.stdout); + if output.status.success() { + match candidate_from_shell_output(stdout.as_ref()) { + Some(path) if is_executable_file(&path) => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (resolved)", + path.display() + )); + return ResolvedBinary { + path: Some(path), + search_output: lines.join("\n"), + }; + } + Some(path) => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an executable file)", + path.display() + )); + } + None if stdout.trim().is_empty() => { + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); + } + None => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an absolute path)", + summarize_output(stdout.as_ref()) + )); + } + } + } else { + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); } - lines.push(format!(" {shell} -l -c 'which {cmd}' => not found")); } Err(e) => { - lines.push(format!(" {shell} -l -c 'which {cmd}' => error: {e}")); + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => error: {e}")); } } } @@ -42,7 +62,7 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { "/home/linuxbrew/.linuxbrew/bin", ] { let path = PathBuf::from(dir).join(cmd); - if path.exists() { + if is_executable_file(&path) { lines.push(format!(" {} => found (resolved)", path.display())); return ResolvedBinary { path: Some(path), @@ -59,6 +79,61 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { } } +fn shell_lookup_commands(cmd: &str) -> Vec<(&'static str, String)> { + let quoted = shell_quote(cmd); + vec![ + ("/bin/zsh", format!("whence -p -- {quoted}")), + ("/bin/bash", format!("type -P -- {quoted}")), + ] +} + +fn shell_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) +} + +fn candidate_from_shell_output(output: &str) -> Option { + let mut lines = output + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()); + let candidate = lines.next()?; + if lines.next().is_some() { + return None; + } + + let path = PathBuf::from(candidate); + path.is_absolute().then_some(path) +} + +fn is_executable_file(path: &Path) -> bool { + let Ok(metadata) = path.metadata() else { + return false; + }; + if !metadata.is_file() { + return false; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + metadata.permissions().mode() & 0o111 != 0 + } + + #[cfg(not(unix))] + { + true + } +} + +fn summarize_output(output: &str) -> String { + let trimmed = output.trim(); + const MAX_LEN: usize = 120; + if trimmed.len() <= MAX_LEN { + return trimmed.replace('\n', "\\n"); + } + format!("{}...", trimmed[..MAX_LEN].replace('\n', "\\n")) +} + /// Format the raw output of a command invocation for debug diagnostics. pub fn format_command_output(cmd_desc: &str, output: &std::process::Output) -> String { let stdout = String::from_utf8_lossy(&output.stdout); @@ -72,3 +147,63 @@ pub fn format_command_output(cmd_desc: &str, output: &std::process::Output) -> S } raw } + +#[cfg(test)] +mod tests { + use super::{candidate_from_shell_output, is_executable_file, shell_quote}; + use std::fs::{self, File}; + use std::path::PathBuf; + + #[test] + fn candidate_accepts_single_absolute_path() { + assert_eq!( + candidate_from_shell_output("/opt/homebrew/bin/git\n"), + Some(PathBuf::from("/opt/homebrew/bin/git")) + ); + } + + #[test] + fn candidate_rejects_function_body_output() { + let output = "git () {\n\tcommand git \"$@\"\n}\n"; + assert_eq!(candidate_from_shell_output(output), None); + } + + #[test] + fn candidate_rejects_relative_or_command_name_output() { + assert_eq!(candidate_from_shell_output("git\n"), None); + } + + #[test] + fn shell_quote_handles_single_quotes() { + assert_eq!(shell_quote("git'bad"), "'git'\\''bad'"); + } + + #[test] + fn executable_file_validation_checks_file_and_mode() { + let dir = std::env::temp_dir().join(format!("doctor-resolve-test-{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + + let executable = dir.join("tool"); + File::create(&executable).unwrap(); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + fs::set_permissions(&executable, fs::Permissions::from_mode(0o644)).unwrap(); + assert!(!is_executable_file(&executable)); + + fs::set_permissions(&executable, fs::Permissions::from_mode(0o755)).unwrap(); + assert!(is_executable_file(&executable)); + } + + #[cfg(not(unix))] + { + assert!(is_executable_file(&executable)); + } + + assert!(!is_executable_file(&dir)); + let _ = fs::remove_dir_all(&dir); + } +} From 5eaf5c790537f40886f061d46e6481a4d6d9139a Mon Sep 17 00:00:00 2001 From: Kalvin Chau Date: Mon, 18 May 2026 10:53:28 -0700 Subject: [PATCH 2/2] refactor(doctor): simplify resolver control flow flatten login shell resolution branches and avoid an intermediate allocation when building lookup commands. preserve executable path validation while making output summarization safe for non-ascii command output. --- crates/acp-client/src/types.rs | 21 ++++++----- crates/blox-cli/src/lib.rs | 19 +++++----- crates/doctor/src/resolve.rs | 66 +++++++++++++++++----------------- 3 files changed, 57 insertions(+), 49 deletions(-) diff --git a/crates/acp-client/src/types.rs b/crates/acp-client/src/types.rs index 7fe1e43b1..fdcf5f2b4 100644 --- a/crates/acp-client/src/types.rs +++ b/crates/acp-client/src/types.rs @@ -196,17 +196,20 @@ fn find_via_login_shell(cmd: &str) -> Option { ]; for (shell, lookup_cmd) in lookups { - if let Ok(output) = std::process::Command::new(shell) + let Ok(output) = std::process::Command::new(shell) .args(["-l", "-c", &lookup_cmd]) .output() - { - if output.status.success() { - let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { - if is_executable_file(&path) { - return Some(path); - } - } + else { + continue; + }; + if !output.status.success() { + continue; + } + + let stdout = String::from_utf8_lossy(&output.stdout); + if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + if is_executable_file(&path) { + return Some(path); } } } diff --git a/crates/blox-cli/src/lib.rs b/crates/blox-cli/src/lib.rs index dcaeea86c..28c781ff8 100644 --- a/crates/blox-cli/src/lib.rs +++ b/crates/blox-cli/src/lib.rs @@ -143,14 +143,17 @@ fn find_via_login_shell(cmd: &str) -> Option { ]; for (shell, lookup_cmd) in lookups { - if let Ok(output) = Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() { - if output.status.success() { - let stdout = String::from_utf8_lossy(&output.stdout); - if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { - if is_executable_file(&path) { - return Some(path); - } - } + let Ok(output) = Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() else { + continue; + }; + if !output.status.success() { + continue; + } + + let stdout = String::from_utf8_lossy(&output.stdout); + if let Some(path) = candidate_from_shell_output(stdout.as_ref()) { + if is_executable_file(&path) { + return Some(path); } } } diff --git a/crates/doctor/src/resolve.rs b/crates/doctor/src/resolve.rs index d2becd7b5..9fc866d30 100644 --- a/crates/doctor/src/resolve.rs +++ b/crates/doctor/src/resolve.rs @@ -15,36 +15,37 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { match Command::new(shell).args(["-l", "-c", &lookup_cmd]).output() { Ok(output) => { let stdout = String::from_utf8_lossy(&output.stdout); - if output.status.success() { - match candidate_from_shell_output(stdout.as_ref()) { - Some(path) if is_executable_file(&path) => { - lines.push(format!( - " {shell} -l -c '{lookup_cmd}' => {} (resolved)", - path.display() - )); - return ResolvedBinary { - path: Some(path), - search_output: lines.join("\n"), - }; - } - Some(path) => { - lines.push(format!( - " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an executable file)", - path.display() - )); - } - None if stdout.trim().is_empty() => { - lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); - } - None => { - lines.push(format!( - " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an absolute path)", - summarize_output(stdout.as_ref()) - )); - } - } - } else { + if !output.status.success() { lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); + continue; + } + + match candidate_from_shell_output(stdout.as_ref()) { + Some(path) if is_executable_file(&path) => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (resolved)", + path.display() + )); + return ResolvedBinary { + path: Some(path), + search_output: lines.join("\n"), + }; + } + Some(path) => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an executable file)", + path.display() + )); + } + None if stdout.trim().is_empty() => { + lines.push(format!(" {shell} -l -c '{lookup_cmd}' => not found")); + } + None => { + lines.push(format!( + " {shell} -l -c '{lookup_cmd}' => {} (ignored: not an absolute path)", + summarize_output(stdout.as_ref()) + )); + } } } Err(e) => { @@ -79,9 +80,9 @@ pub fn resolve_binary(cmd: &str) -> ResolvedBinary { } } -fn shell_lookup_commands(cmd: &str) -> Vec<(&'static str, String)> { +fn shell_lookup_commands(cmd: &str) -> [(&'static str, String); 2] { let quoted = shell_quote(cmd); - vec![ + [ ("/bin/zsh", format!("whence -p -- {quoted}")), ("/bin/bash", format!("type -P -- {quoted}")), ] @@ -131,7 +132,8 @@ fn summarize_output(output: &str) -> String { if trimmed.len() <= MAX_LEN { return trimmed.replace('\n', "\\n"); } - format!("{}...", trimmed[..MAX_LEN].replace('\n', "\\n")) + let summary: String = trimmed.chars().take(MAX_LEN).collect(); + format!("{}...", summary.replace('\n', "\\n")) } /// Format the raw output of a command invocation for debug diagnostics.