diff --git a/Cargo.lock b/Cargo.lock index 333b656..6c1052e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,6 +50,17 @@ dependencies = [ "syn", ] +[[package]] +name = "async-trait" +version = "0.1.89" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9035ad2d096bed7955a320ee7e2230574d28fd3c3a0f186cbea1ff3c7eed5dbb" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "autocfg" version = "1.5.1" @@ -296,6 +307,12 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "fastrand" +version = "2.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f1f227452a390804cdb637b74a86990f2a7d7ba4b7d5693aac9b4dd6defd8d6" + [[package]] name = "find-msvc-tools" version = "0.1.9" @@ -565,6 +582,12 @@ version = "0.2.186" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "68ab91017fe16c622486840e4c83c9a37afeff978bd239b5293d61ece587de66" +[[package]] +name = "linux-raw-sys" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25d58a53" + [[package]] name = "litemap" version = "0.8.2" @@ -991,6 +1014,19 @@ dependencies = [ "nom", ] +[[package]] +name = "rustix" +version = "1.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e9930ba190" +dependencies = [ + "bitflags", + "errno", + "libc", + "linux-raw-sys", + "windows-sys 0.61.2", +] + [[package]] name = "rustls" version = "0.23.41" @@ -1168,10 +1204,12 @@ checksum = "f8fadd59c855ef2080decdef8ff161eb6661b86933c9d82e5ba29dc602a55aba" name = "shoal-types" version = "0.1.0" dependencies = [ + "async-trait", "chrono", "mtp", "serde", "serde_json", + "tempfile", "thiserror", "tokio", "uuid", @@ -1243,6 +1281,19 @@ dependencies = [ "syn", ] +[[package]] +name = "tempfile" +version = "3.27.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" +dependencies = [ + "fastrand", + "getrandom 0.4.3", + "once_cell", + "rustix", + "windows-sys 0.61.2", +] + [[package]] name = "thiserror" version = "2.0.18" diff --git a/Cargo.toml b/Cargo.toml index f95f263..c99c10c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,6 +12,8 @@ serde = { version = "1", features = ["derive"] } serde_json = "1" mtp = { git = "https://git@git.methanium.net/methanium/mtp.git", features = [] } tokio = "1" +async-trait = "0.1" [dev-dependencies] tokio = { version = "1", features = ["full"] } +tempfile = "3" diff --git a/src/lib.rs b/src/lib.rs index f479187..39e809d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,6 +12,7 @@ pub mod errors; pub mod krill; pub mod mutations; pub mod project; +pub mod sandbox; pub mod sync; pub mod task; pub mod todo; @@ -34,7 +35,8 @@ pub use tools::{ format_tool_error, format_tools_json, format_tool_result, parse_tool_call_blocks, parse_tool_call_stream, tool_definitions, tool_ids_for_agent_type, ApprovalRequirement, CompositionStep, ContextVisibility, ExecutionContext, MarketplaceSearchResults, ParsedToolCall, + REEF_PROXY_TOOLS, GLOBAL_TOOLS, is_reef_proxy_tool, is_global_tool, TestAction, ToolComposition, ToolDefinition, ToolDependency, ToolDocumentation, ToolExample, - ToolListing, ToolParser, ToolPlugin, ToolTest, ToolTestResult, ToolTestSuite, + ToolListing, ToolParser, ToolPlugin, ToolExecutor, ToolRegistry, ToolTest, ToolTestResult, ToolTestSuite, ToolTestSuiteResult, ToolVersion, }; diff --git a/src/sandbox.rs b/src/sandbox.rs new file mode 100644 index 0000000..459e951 --- /dev/null +++ b/src/sandbox.rs @@ -0,0 +1,210 @@ +use std::path::{Path, PathBuf, Component}; + +/* Maximum allowed path depth as a safety measure */ +const MAX_PATH_DEPTH: usize = 64; + +/* Validate a file path to prevent directory traversal, null bytes, shell +metacharacters, and other path-based attacks. Returns the normalized path +if valid, or an error message. + +This is the canonical path validation shared by all runtimes (connector-krill, +Reef API, etc.). */ +pub fn validate_path(path: &str) -> Result { + if path.is_empty() { + return Err("Path cannot be empty".to_string()); + } + + if path.len() > 4096 { + return Err("Path exceeds maximum length (4096 characters)".to_string()); + } + + let path = Path::new(path); + + if has_directory_traversal(path) { + return Err("Path contains directory traversal ('..') which is not allowed".to_string()); + } + + if contains_null_bytes(path) { + return Err("Path contains null bytes which is not allowed".to_string()); + } + + if contains_shell_metacharacters(path) { + return Err("Path contains shell metacharacters which is not allowed".to_string()); + } + + let depth = path.components().count(); + if depth > MAX_PATH_DEPTH { + return Err(format!( + "Path depth ({}) exceeds maximum allowed ({})", + depth, MAX_PATH_DEPTH + )); + } + + Ok(path.to_path_buf()) +} + +/* Given a validated path and a sandbox root, resolve the full path and +verify it does not escape the sandbox. Handles both existing and new paths. +The `validated` path should already have passed `validate_path`. */ +pub fn resolve_sandboxed_path(validated: &Path, sandbox_root: &Path) -> Result { + let full_path = if validated.is_absolute() { + validated.to_path_buf() + } else { + sandbox_root.join(validated) + }; + + let canonical = if full_path.exists() { + full_path.canonicalize().map_err(|e| { + format!("Failed to canonicalize path: {}", e) + })? + } else { + if let Some(parent) = full_path.parent() { + if parent.exists() { + let canonical_parent = parent.canonicalize().map_err(|e| { + format!("Failed to canonicalize parent path: {}", e) + })?; + if !canonical_parent.starts_with(sandbox_root) { + return Err("Path escapes sandbox root boundaries".to_string()); + } + } else { + return Err("Parent directory does not exist".to_string()); + } + } + full_path + }; + + if !canonical.starts_with(sandbox_root) { + return Err("Path escapes sandbox root boundaries".to_string()); + } + + Ok(canonical) +} + +fn has_directory_traversal(path: &Path) -> bool { + path.components().any(|comp| matches!(comp, Component::ParentDir)) +} + +fn contains_null_bytes(path: &Path) -> bool { + path.to_str().map_or(true, |s| s.contains('\0')) +} + +fn contains_shell_metacharacters(path: &Path) -> bool { + const SHELL_METACHARACTERS: &[char] = &[ + '|', ';', '&', '$', '`', '>', '<', '(', ')', '{', '}', + '!', '#', '*', '?', '[', ']', '~', '\n', '\r', + ]; + path.to_str().map_or(true, |s| s.contains(SHELL_METACHARACTERS)) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use tempfile::tempdir; + + #[test] + fn test_rejects_empty_path() { + let result = validate_path(""); + assert!(result.is_err()); + } + + #[test] + fn test_rejects_directory_traversal() { + let attacks = vec![ + "../etc/passwd", + "../../etc/passwd", + "file/../../../etc/passwd", + "a/../../b/../../../c", + "..", + ]; + for path in attacks { + let result = validate_path(path); + assert!(result.is_err(), "Expected '{}' to be rejected", path); + } + } + + #[test] + fn test_accepts_valid_paths() { + let valid = vec![ + "file.txt", + "subdir/file.txt", + "a/b/c/d/file.rs", + "src/main.rs", + "docs/README.md", + "project_name/src/lib.rs", + ]; + for path in valid { + let result = validate_path(path); + assert!( + result.is_ok(), + "Expected '{}' to be accepted: {:?}", + path, + result.err() + ); + } + } + + #[test] + fn test_rejects_path_with_null_bytes() { + assert!(validate_path("file\0.txt").is_err()); + } + + #[test] + fn test_rejects_path_with_shell_metacharacters() { + let attacks = vec![ + "file|echo", "file;rm", "file$(id)", "file`id`", "file>out", "file>().join("/"); + assert!(validate_path(&deep).is_err()); + } + + #[test] + fn test_sandboxed_path_within_root() { + let dir = tempdir().unwrap(); + let file_path = dir.path().join("test.txt"); + fs::write(&file_path, "content").unwrap(); + + let validated = validate_path("test.txt").unwrap(); + let result = resolve_sandboxed_path(&validated, dir.path()); + assert!(result.is_ok()); + assert_eq!(result.unwrap(), file_path); + } + + #[test] + fn test_sandboxed_path_escape_via_absolute() { + let dir = tempdir().unwrap(); + let outside_file = std::env::temp_dir().join("escape_test.txt"); + fs::write(&outside_file, "content").unwrap(); + + let validated = validate_path(outside_file.to_str().unwrap()).unwrap(); + let result = resolve_sandboxed_path(&validated, dir.path()); + assert!(result.is_err()); + let _ = fs::remove_file(&outside_file); + } + + #[test] + fn test_sandboxed_path_new_file() { + let dir = tempdir().unwrap(); + + let validated = validate_path("new_file.txt").unwrap(); + let result = resolve_sandboxed_path(&validated, dir.path()); + assert!(result.is_ok()); + } + + #[test] + fn test_sandboxed_path_nonexistent_parent() { + let dir = tempdir().unwrap(); + + let validated = validate_path("nonexistent_dir/file.txt").unwrap(); + let result = resolve_sandboxed_path(&validated, dir.path()); + assert!(result.is_err()); + } +} diff --git a/src/tools.rs b/src/tools.rs index 8f7beb0..069ddaf 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -1,3 +1,4 @@ +use async_trait::async_trait; use serde::{Deserialize, Serialize}; /* Context in which a tool should be available */ @@ -227,6 +228,20 @@ impl ToolDefinition { pub fn is_compatible_with(&self, other: &Self) -> bool { self.name == other.name && !self.deprecated && !other.deprecated } + + /* Execution tier classification based on the tool's properties. + Tier 1 (Transient): global tools with no project state — always local + Tier 2 (Project Read): read-only project-scoped tools — local or Reef proxy + Tier 3 (Persistent): mutation tools + Reef-only tools — Reef-backed */ + pub fn tier(&self) -> u8 { + if is_global_tool(&self.name) { + 1 + } else if self.mutates() || self.execution_context == ExecutionContext::Reef { + 3 + } else { + 2 + } + } } /* Simple semver comparison (major.minor.patch) */ @@ -495,22 +510,6 @@ impl Default for ToolVersion { } } -/* Execution context for tool filtering. Determines which tools are -available in which execution environment (Reef vs. Krill vs. both). */ -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "snake_case")] -pub enum ToolContext { - Reef, - Krill, - Both, -} - -impl Default for ToolContext { - fn default() -> Self { - Self::Both - } -} - /* Tool category classification for UI grouping and filtering. */ #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] @@ -668,6 +667,29 @@ pub trait ToolPlugin: Send + Sync { } } +/// Shared trait for executing a tool. +/// +/// Every Krill binary (connector-krill, example-krill, custom) implements +/// this trait for each tool it supports. The trait is deliberately simple — +/// it takes parsed JSON arguments and returns either a string result or an +/// error. Wrapping in a sandbox/project context is handled by the caller. +#[async_trait] +pub trait ToolExecutor: Send + Sync { + fn definition(&self) -> ToolDefinition; + + async fn execute(&self, arguments: &serde_json::Value) -> Result; +} + +/// A registry of tools that can be looked up by name and executed. +#[async_trait] +pub trait ToolRegistry: Send + Sync { + async fn register(&self, tool: ToolDefinition, executor: Box); + + async fn list_tools(&self) -> Vec; + + async fn execute(&self, name: &str, arguments: &serde_json::Value) -> Result; +} + /* A marketplace listing for a published tool. */ #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ToolListing { @@ -820,6 +842,41 @@ NOT here: they stay Everywhere so direct chat can call them under approval routi (Phase 4). Only genuinely agent-internal collaboration tools are agent-only. */ pub const AGENT_ONLY_TOOLS: &[&str] = &["submit_batch_plan", "report_completion"]; +/* Tools that proxy to the Reef API when a `REEF_URL` / `REEF_API_URL` is +configured. These tools have both a local implementation and a Reef-backed +implementation (via Reservoir). When Reef is available the Reef path is +preferred so that file operations go through the Reservoir-backed sandbox; +when Reef is unreachable the local fallback executes instead. + +This is the Krill → Reef proxy set, not the full set of Reef-only tools. +Tools like kanban_*, document_*, and strategic items are always executed on +Reef via MTP and are NOT in this list — they have no local implementation. */ +pub const REEF_PROXY_TOOLS: &[&str] = &[ + "read_file", "file_read", + "write_file", "file_write", + "edit_file", + "list_directory", + "search_files", + "grep", + "bash", "execute", + "delete_workspace", + "list_files", +]; + +/* Tools that operate without a project scope (global). These are always +executed locally on the Krill and never need project_id injection. */ +pub const GLOBAL_TOOLS: &[&str] = &["web_search", "web_fetch", "web_api"]; + +/* Returns true if the tool name is in the global (non-project-scoped) set. */ +pub fn is_global_tool(name: &str) -> bool { + GLOBAL_TOOLS.contains(&name) +} + +/* Returns true if the tool name should proxy to Reef when Reef is available. */ +pub fn is_reef_proxy_tool(name: &str) -> bool { + REEF_PROXY_TOOLS.contains(&name) +} + /* Returns all built-in tool definitions across all categories. */ pub fn tool_definitions() -> Vec { let mut tools = vec![