add bash executor with safety checks

alexylon · alexylon · commit 0aac8ba08904 · 2025-12-06T12:06:19.000+02:00
diff --git a/README.md b/README.md
@@ -101,6 +101,16 @@ All file operations are sandboxed to your current working directory:
 - ❌ Cannot access absolute paths (`/etc/passwd`)
 - ❌ Cannot follow symlinks outside workspace
 
+Bash command execution is restricted to read-only operations:
+
+- ✅ Can run tests and build commands (`cargo test`, `cargo build`)
+- ✅ Can read files and list directories (`cat`, `ls`, `grep`)
+- ❌ Cannot use `sudo` or privilege escalation
+- ❌ Cannot modify files (`rm`, `mv`, `cp`, `chmod`, `mkdir`, `touch`)
+- ❌ Cannot access absolute paths or parent directories
+- ❌ Cannot change directories (`cd`, `pushd`, `popd`)
+- ❌ Cannot use output redirection (`>`, `>>`)
+
 **Best Practice:** Run `sofos` from your project directory, use git to track changes.
 
 ## Development
diff --git a/src/conversation.rs b/src/conversation.rs
@@ -18,10 +18,11 @@ impl ConversationHistory {
             "3. List directory contents",
             "4. Create directories",
             "5. Search the web for information",
+            "6. Execute read-only bash commands (for testing code)",
         ];
 
         if has_code_search {
-            features.push("6. Search code using ripgrep");
+            features.push("7. Search code using ripgrep");
         }
 
         let edit_instruction = if has_morph {
@@ -41,8 +42,19 @@ When helping users:
 - Use your tools to read files before suggesting changes
 {}
 - Search the web when you need current information or documentation
+- Execute bash commands to test code and verify functionality (read-only, no sudo or file modifications)
 - Explain your reasoning when using tools
 
+Testing after code changes:
+- After editing code files (not comments, README, or documentation), ALWAYS test the changes using execute_bash
+- Run appropriate build/test commands based on the project type:
+  * Rust: 'cargo build' and/or 'cargo test'
+  * JavaScript/TypeScript: 'npm run build' and/or 'npm test'
+  * Python: 'python -m pytest' or 'python -m unittest'
+  * Go: 'go build' and/or 'go test'
+- If tests fail, fix the errors and test again
+- Do NOT run tests for changes to: comments only, README.md, documentation files, or configuration files
+
 Your goal is to help users with coding tasks efficiently and accurately."#,
             features.join("\n"),
             edit_instruction
diff --git a/src/repl.rs b/src/repl.rs
@@ -180,11 +180,21 @@ impl Repl {
 
         if !tool_uses.is_empty() {
             for (_tool_id, tool_name, tool_input) in &tool_uses {
-                println!(
-                    "{} {}",
-                    "Using tool:".bright_yellow().bold(),
-                    tool_name.bright_yellow()
-                );
+                if tool_name == "execute_bash" {
+                    if let Some(command) = tool_input.get("command").and_then(|v| v.as_str()) {
+                        println!(
+                            "{} {}",
+                            "Executing:".bright_green().bold(),
+                            command.bright_cyan()
+                        );
+                    }
+                } else {
+                    println!(
+                        "{} {}",
+                        "Using tool:".bright_yellow().bold(),
+                        tool_name.bright_yellow()
+                    );
+                }
 
                 let result = runtime.block_on(self.tool_executor.execute(tool_name, tool_input));
 
diff --git a/src/tools/bashexec.rs b/src/tools/bashexec.rs
@@ -0,0 +1,210 @@
+use crate::error::{Result, SofosError};
+use std::path::PathBuf;
+use std::process::Command;
+
+pub struct BashExecutor {
+    workspace: PathBuf,
+}
+
+impl BashExecutor {
+    pub fn new(workspace: PathBuf) -> Result<Self> {
+        Ok(Self { workspace })
+    }
+
+    pub fn execute(&self, command: &str) -> Result<String> {
+        if !self.is_safe_command(command) {
+            return Err(SofosError::ToolExecution(
+                "Command is not allowed. Only read-only commands are permitted.".to_string(),
+            ));
+        }
+
+        let output = Command::new("sh")
+            .arg("-c")
+            .arg(command)
+            .current_dir(&self.workspace)
+            .output()
+            .map_err(|e| SofosError::ToolExecution(format!("Failed to execute command: {}", e)))?;
+
+        let stdout = String::from_utf8_lossy(&output.stdout);
+        let stderr = String::from_utf8_lossy(&output.stderr);
+
+        if !output.status.success() {
+            return Ok(format!(
+                "Command failed with exit code: {}\nSTDOUT:\n{}\nSTDERR:\n{}",
+                output.status.code().unwrap_or(-1),
+                stdout,
+                stderr
+            ));
+        }
+
+        let mut result = String::new();
+        if !stdout.is_empty() {
+            result.push_str("STDOUT:\n");
+            result.push_str(&stdout);
+        }
+        if !stderr.is_empty() {
+            if !result.is_empty() {
+                result.push_str("\n");
+            }
+            result.push_str("STDERR:\n");
+            result.push_str(&stderr);
+        }
+
+        if result.is_empty() {
+            result = "Command executed successfully (no output)".to_string();
+        }
+
+        Ok(result)
+    }
+
+    fn is_safe_command(&self, command: &str) -> bool {
+        let command_lower = command.to_lowercase();
+
+        if command_lower.starts_with("sudo") || command_lower.contains(" sudo ") {
+            return false;
+        }
+
+        if command.contains("..") {
+            return false;
+        }
+
+        let directory_commands = ["cd ", "cd\t", "cd;", "cd&", "cd|",
+                                  "pushd ", "pushd\t", "pushd;", "pushd&", "pushd|",
+                                  "popd ", "popd\t", "popd;", "popd&", "popd|"];
+        for cmd in &directory_commands {
+            if command_lower.starts_with(cmd.trim_end())
+                || command_lower.contains(&format!(" {}", cmd.trim_end()))
+                || command_lower.contains(&format!(";{}", cmd.trim_end()))
+                || command_lower.contains(&format!("&&{}", cmd.trim_end()))
+                || command_lower.contains(&format!("||{}", cmd.trim_end()))
+                || command_lower.contains(&format!("|{}", cmd.trim_end()))
+            {
+                return false;
+            }
+        }
+
+        if command.starts_with('/') {
+            return false;
+        }
+
+        // Catches: cat /etc/passwd, ls /tmp, etc.
+        if command.contains(" /") {
+            return false;
+        }
+
+        // Check absolute paths after pipes, semicolons, and logical operators
+        if command.contains("|/") || command.contains(";/") 
+            || command.contains("&&/") || command.contains("||/") {
+            return false;
+        }
+
+        let forbidden_commands = [
+            "rm", "mv", "cp", "chmod", "chown", "chgrp",
+            "mkdir", "rmdir", "touch", "ln", "dd",
+            "mkfs", "mount", "umount", "kill", "killall",
+            "pkill", "shutdown", "reboot", "halt", "poweroff",
+            "useradd", "userdel", "groupadd", "groupdel",
+            "passwd", "systemctl", "service", "fdisk",
+            "parted", "mkswap", "swapon", "swapoff",
+        ];
+
+        for forbidden in &forbidden_commands {
+            if command_lower.starts_with(forbidden)
+                || command_lower.starts_with(&format!("{} ", forbidden))
+            {
+                return false;
+            }
+            // Check chained commands
+            if command_lower.contains(&format!("| {}", forbidden))
+                || command_lower.contains(&format!("; {}", forbidden))
+                || command_lower.contains(&format!("&& {}", forbidden))
+                || command_lower.contains(&format!("|| {}", forbidden))
+            {
+                return false;
+            }
+        }
+
+        if command.contains('>') || command.contains(">>") {
+            return false;
+        }
+
+        // Here-doc can be used for malicious input
+        if command.contains("<<") {
+            return false;
+        }
+
+        true
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_safe_commands() {
+        let executor = BashExecutor::new(PathBuf::from(".")).unwrap();
+
+        assert!(executor.is_safe_command("ls -la"));
+        assert!(executor.is_safe_command("cat file.txt"));
+        assert!(executor.is_safe_command("grep pattern file.txt"));
+        assert!(executor.is_safe_command("cargo test"));
+        assert!(executor.is_safe_command("cargo build"));
+        assert!(executor.is_safe_command("echo hello"));
+        assert!(executor.is_safe_command("pwd"));
+    }
+
+    #[test]
+    fn test_unsafe_commands() {
+        let executor = BashExecutor::new(PathBuf::from(".")).unwrap();
+
+        assert!(!executor.is_safe_command("sudo ls"));
+        assert!(!executor.is_safe_command("rm file.txt"));
+        assert!(!executor.is_safe_command("mv file1 file2"));
+        assert!(!executor.is_safe_command("chmod 777 file"));
+        assert!(!executor.is_safe_command("echo hello > file.txt"));
+        assert!(!executor.is_safe_command("cat file.txt >> output.txt"));
+        assert!(!executor.is_safe_command("ls | rm file.txt"));
+        assert!(!executor.is_safe_command("ls && rm file.txt"));
+    }
+
+    #[test]
+    fn test_path_traversal_blocked() {
+        let executor = BashExecutor::new(PathBuf::from(".")).unwrap();
+
+        assert!(!executor.is_safe_command("cat ../file.txt"));
+        assert!(!executor.is_safe_command("ls ../../etc"));
+        assert!(!executor.is_safe_command("cat ../../../etc/passwd"));
+        assert!(!executor.is_safe_command("cat file.txt && ls .."));
+        assert!(!executor.is_safe_command("ls | cat ../secret"));
+    }
+
+    #[test]
+    fn test_absolute_paths_blocked() {
+        let executor = BashExecutor::new(PathBuf::from(".")).unwrap();
+
+        assert!(!executor.is_safe_command("/bin/ls"));
+        assert!(!executor.is_safe_command("/etc/passwd"));
+        assert!(!executor.is_safe_command("cat /etc/passwd"));
+        assert!(!executor.is_safe_command("ls /tmp"));
+        assert!(!executor.is_safe_command("cat /home/user/secret"));
+        assert!(!executor.is_safe_command("ls && cat /etc/passwd"));
+        assert!(!executor.is_safe_command("echo test || cat /etc/passwd"));
+        assert!(!executor.is_safe_command("ls | grep /etc/passwd"));
+        assert!(!executor.is_safe_command("true;/bin/bash"));
+    }
+
+    #[test]
+    fn test_directory_change_blocked() {
+        let executor = BashExecutor::new(PathBuf::from(".")).unwrap();
+
+        assert!(!executor.is_safe_command("cd /tmp"));
+        assert!(!executor.is_safe_command("cd .."));
+        assert!(!executor.is_safe_command("cd / && ls"));
+        assert!(!executor.is_safe_command("ls && cd /tmp"));
+        assert!(!executor.is_safe_command("ls | cd /tmp"));
+        assert!(!executor.is_safe_command("pushd /tmp"));
+        assert!(!executor.is_safe_command("popd"));
+        assert!(!executor.is_safe_command("ls && pushd .."));
+    }
+}
diff --git a/src/tools/codesearch.rs b/src/tools/codesearch.rs
@@ -25,7 +25,6 @@ impl CodeSearchTool {
     ) -> Result<String> {
         let mut cmd = Command::new("rg");
 
-        // Basic options
         cmd.arg("--heading")
             .arg("--line-number")
             .arg("--color=never")
diff --git a/src/tools/filesystem.rs b/src/tools/filesystem.rs
@@ -19,14 +19,12 @@ impl FileSystemTool {
     /// Validate and resolve a path relative to the workspace
     /// Returns an error if the path attempts to escape the workspace
     fn validate_path(&self, path: &str) -> Result<PathBuf> {
-        // Reject absolute paths
         if Path::new(path).is_absolute() {
             return Err(SofosError::PathViolation(
                 "Absolute paths are not allowed".to_string(),
             ));
         }
 
-        // Reject paths with parent directory traversal
         if path.contains("..") {
             return Err(SofosError::PathViolation(
                 "Parent directory traversal (..) is not allowed".to_string(),
@@ -60,7 +58,6 @@ impl FileSystemTool {
             }
         };
 
-        // Ensure the canonical path is within the workspace
         if !canonical.starts_with(&self.workspace) {
             return Err(SofosError::PathViolation(format!(
                 "Path '{}' is outside the workspace",
@@ -238,20 +235,17 @@ mod tests {
 
     #[test]
     fn test_list_nested_subdirectories() {
-        let temp = TempDir::new().unwrap();
-        let fs_tool = FileSystemTool::new(temp.path().to_path_buf()).unwrap();
+        let temp_dir = tempfile::tempdir().unwrap();
+        let fs_tool = FileSystemTool::new(temp_dir.path().to_path_buf()).unwrap();
 
-        // Create nested structure
         fs_tool.create_directory("parent/child").unwrap();
         fs_tool.write_file("parent/file1.txt", "test1").unwrap();
         fs_tool.write_file("parent/child/file2.txt", "test2").unwrap();
 
-        // Test listing parent
         let parent_entries = fs_tool.list_directory("parent").unwrap();
         assert!(parent_entries.contains(&"child/".to_string()));
         assert!(parent_entries.contains(&"file1.txt".to_string()));
 
-        // Test listing child
         let child_entries = fs_tool.list_directory("parent/child").unwrap();
         assert_eq!(child_entries, vec!["file2.txt"]);
     }
diff --git a/src/tools/mod.rs b/src/tools/mod.rs
@@ -1,10 +1,12 @@
+pub mod bashexec;
 pub mod codesearch;
 pub mod filesystem;
 pub mod search;
 pub mod types;
 
 use crate::api::MorphClient;
 use crate::error::{Result, SofosError};
+use bashexec::BashExecutor;
 use codesearch::CodeSearchTool;
 use filesystem::FileSystemTool;
 use search::WebSearchTool;
@@ -17,6 +19,7 @@ pub struct ToolExecutor {
     fs_tool: FileSystemTool,
     search_tool: WebSearchTool,
     code_search_tool: Option<CodeSearchTool>,
+    bash_executor: BashExecutor,
     morph_client: Option<MorphClient>,
 }
 
@@ -31,9 +34,10 @@ impl ToolExecutor {
         };
 
         Ok(Self {
-            fs_tool: FileSystemTool::new(workspace)?,
+            fs_tool: FileSystemTool::new(workspace.clone())?,
             search_tool: WebSearchTool::new()?,
             code_search_tool,
+            bash_executor: BashExecutor::new(workspace)?,
             morph_client,
         })
     }
@@ -146,6 +150,14 @@ impl ToolExecutor {
                 self.fs_tool.write_file(path, &merged_code)?;
                 Ok(format!("Successfully applied fast edit to '{}'", path))
             }
+            "execute_bash" => {
+                let command = input["command"]
+                    .as_str()
+                    .ok_or_else(|| SofosError::ToolExecution("Missing 'command' parameter".to_string()))?;
+
+                let result = self.bash_executor.execute(command)?;
+                Ok(result)
+            }
             _ => Err(SofosError::ToolExecution(format!(
                 "Unknown tool: {}",
                 tool_name
diff --git a/src/tools/types.rs b/src/tools/types.rs
