From d0870dc510908a7649fff1eeae5aa7afc21f68f2 Mon Sep 17 00:00:00 2001 From: yishuiliunian Date: Sun, 5 Apr 2026 16:46:37 +0800 Subject: [PATCH] feat(sandbox): replace hard denies with permission-gated RequiresApproval for path operations (#80) Sandbox path violations (writes outside cwd, deny_write_globs, deny_read_globs) previously returned hard errors that blocked operations even when the user had already approved the tool. This prevented Loopal from performing legitimate ops tasks like editing /etc/nginx/nginx.conf. Now these "soft denies" flow through the existing permission system: - Bypass mode: auto-allows without prompting - Supervised mode: asks the user with sandbox context - Auto mode: routes to the LLM classifier Hard denies remain for ReadOnly mode and path resolution failures (.. traversal, symlink escape). Key changes: - PathDecision simplified to three-state: Allow / Deny / RequiresApproval - Backend gains approve_path() + check_sandbox_path() for session-scoped approval caching - Runtime pre-checks extract paths from tool input before execution, elevating effective permission to Dangerous when sandbox approval is needed - Execution-time fallback in resolve_checked() handles opaque tools (Bash/Glob/MCP) - 35 new tests across backend, sandbox, and runtime layers --- crates/loopal-backend/BUILD.bazel | 18 +- crates/loopal-backend/src/approved.rs | 41 +++++ crates/loopal-backend/src/lib.rs | 2 + crates/loopal-backend/src/local.rs | 100 +++++------ crates/loopal-backend/src/path.rs | 23 ++- crates/loopal-backend/src/platform.rs | 43 +++++ crates/loopal-backend/tests/suite.rs | 7 + .../tests/suite/approved_paths_test.rs | 43 +++++ .../tests/suite/path_approval_test.rs | 79 ++++++++ .../tests/suite/resolve_checked_test.rs | 169 ++++++++++++++++++ crates/loopal-config/src/sandbox.rs | 8 +- crates/loopal-error/src/io_error.rs | 7 + crates/loopal-runtime/src/agent_loop/mod.rs | 3 + .../src/agent_loop/sandbox_precheck.rs | 96 ++++++++++ .../src/agent_loop/tools_check.rs | 47 ++++- .../tests/agent_loop/auto_mode_test.rs | 5 +- crates/loopal-runtime/tests/suite.rs | 2 + .../tests/suite/sandbox_precheck_test.rs | 116 ++++++++++++ crates/loopal-sandbox/src/path_checker.rs | 18 +- .../tests/suite/path_checker_edge_test.rs | 13 +- .../tests/suite/path_checker_test.rs | 8 +- crates/loopal-tool-api/src/backend.rs | 16 ++ 22 files changed, 781 insertions(+), 83 deletions(-) create mode 100644 crates/loopal-backend/src/approved.rs create mode 100644 crates/loopal-backend/src/platform.rs create mode 100644 crates/loopal-backend/tests/suite.rs create mode 100644 crates/loopal-backend/tests/suite/approved_paths_test.rs create mode 100644 crates/loopal-backend/tests/suite/path_approval_test.rs create mode 100644 crates/loopal-backend/tests/suite/resolve_checked_test.rs create mode 100644 crates/loopal-runtime/src/agent_loop/sandbox_precheck.rs create mode 100644 crates/loopal-runtime/tests/suite/sandbox_precheck_test.rs diff --git a/crates/loopal-backend/BUILD.bazel b/crates/loopal-backend/BUILD.bazel index ed94c0cc..e4f5659f 100644 --- a/crates/loopal-backend/BUILD.bazel +++ b/crates/loopal-backend/BUILD.bazel @@ -1,4 +1,4 @@ -load("@rules_rust//rust:defs.bzl", "rust_library") +load("@rules_rust//rust:defs.bzl", "rust_library", "rust_test") rust_library( name = "loopal-backend", @@ -21,3 +21,19 @@ rust_library( ], proc_macro_deps = ["@crates//:async-trait"], ) + +rust_test( + name = "loopal-backend_test", + srcs = glob(["tests/**/*.rs"]), + crate_root = "tests/suite.rs", + edition = "2024", + deps = [ + ":loopal-backend", + "//crates/loopal-config", + "//crates/loopal-error", + "//crates/loopal-tool-api", + "@crates//:tempfile", + "@crates//:tokio", + ], + proc_macro_deps = ["@crates//:async-trait"], +) diff --git a/crates/loopal-backend/src/approved.rs b/crates/loopal-backend/src/approved.rs new file mode 100644 index 00000000..83b9c64d --- /dev/null +++ b/crates/loopal-backend/src/approved.rs @@ -0,0 +1,41 @@ +//! Session-scoped set of sandbox-approved paths. +//! +//! Once a path is approved (user confirmation or Bypass mode), subsequent +//! operations on it skip the `RequiresApproval` sandbox check within the +//! same session. + +use std::collections::HashSet; +use std::path::{Path, PathBuf}; + +use parking_lot::RwLock; + +/// Thread-safe approved-paths set with interior mutability. +/// +/// Wrapped in `RwLock` so it can live inside `Arc` without +/// requiring `&mut self`. The contention profile (rare writes after first +/// approval, frequent reads) is ideal for reader-writer locks. +pub struct ApprovedPaths { + inner: RwLock>, +} + +impl Default for ApprovedPaths { + fn default() -> Self { + Self::new() + } +} + +impl ApprovedPaths { + pub fn new() -> Self { + Self { + inner: RwLock::new(HashSet::new()), + } + } + + pub fn insert(&self, path: PathBuf) { + self.inner.write().insert(path); + } + + pub fn contains(&self, path: &Path) -> bool { + self.inner.read().contains(path) + } +} diff --git a/crates/loopal-backend/src/lib.rs b/crates/loopal-backend/src/lib.rs index a9fd962e..fe6220b4 100644 --- a/crates/loopal-backend/src/lib.rs +++ b/crates/loopal-backend/src/lib.rs @@ -1,8 +1,10 @@ +pub mod approved; pub mod fs; pub mod limits; pub mod local; pub mod net; pub mod path; +pub mod platform; pub mod search; pub mod shell; pub mod shell_stream; diff --git a/crates/loopal-backend/src/local.rs b/crates/loopal-backend/src/local.rs index 5e493113..11cbc022 100644 --- a/crates/loopal-backend/src/local.rs +++ b/crates/loopal-backend/src/local.rs @@ -8,12 +8,13 @@ use loopal_config::ResolvedPolicy; use loopal_error::{ProcessHandle, ToolIoError}; use loopal_tool_api::backend_types::{ EditResult, ExecResult, FetchResult, FileInfo, GlobOptions, GlobSearchResult, GrepOptions, - GrepSearchResult, LsEntry, LsResult, ReadResult, WriteResult, + GrepSearchResult, LsResult, ReadResult, WriteResult, }; use loopal_tool_api::{Backend, ExecOutcome}; +use crate::approved::ApprovedPaths; use crate::limits::ResourceLimits; -use crate::{fs, net, path, search, shell, shell_stream}; +use crate::{fs, net, path, platform, search, shell, shell_stream}; /// Production backend: local disk I/O with path checking, size limits, /// atomic writes, OS-level sandbox wrapping, and resource budgets. @@ -21,31 +22,48 @@ pub struct LocalBackend { cwd: PathBuf, policy: Option, limits: ResourceLimits, + approved: ApprovedPaths, } impl LocalBackend { pub fn new(cwd: PathBuf, policy: Option, limits: ResourceLimits) -> Arc { - // Canonicalize cwd to resolve symlinks (e.g. macOS /tmp → /private/tmp). - // On Windows, strip \\?\ prefix that canonicalize() adds. let cwd = path::strip_win_prefix(cwd.canonicalize().unwrap_or(cwd)); Arc::new(Self { cwd, policy, limits, + approved: ApprovedPaths::new(), }) } + + /// Resolve with sandbox check; falls back to approved-paths on `RequiresApproval`. + fn resolve_checked(&self, raw: &str, is_write: bool) -> Result { + match path::resolve(&self.cwd, raw, is_write, self.policy.as_ref()) { + Ok(p) => Ok(p), + Err(ToolIoError::RequiresApproval(reason)) => { + let abs = path::to_absolute(&self.cwd, raw); + if self.approved.contains(&abs) { + // Canonicalize for consistency with the Allow path + // (path::resolve returns canonical form). + Ok(abs.canonicalize().unwrap_or(abs)) + } else { + Err(ToolIoError::RequiresApproval(reason)) + } + } + Err(e) => Err(e), + } + } } #[async_trait] impl Backend for LocalBackend { async fn read(&self, p: &str, offset: usize, limit: usize) -> Result { - let resolved = path::resolve(&self.cwd, p, false, self.policy.as_ref())?; + let resolved = self.resolve_checked(p, false)?; fs::read_file(&resolved, offset, limit, &self.limits).await } async fn write(&self, p: &str, content: &str) -> Result { - let resolved = path::resolve(&self.cwd, p, true, self.policy.as_ref())?; - fs::write_file(&resolved, content).await + fs::write_file(&self.resolve_checked(p, true)?, content).await } async fn edit( @@ -55,12 +73,11 @@ impl Backend for LocalBackend { new: &str, replace_all: bool, ) -> Result { - let resolved = path::resolve(&self.cwd, p, true, self.policy.as_ref())?; - fs::edit_file(&resolved, old, new, replace_all).await + fs::edit_file(&self.resolve_checked(p, true)?, old, new, replace_all).await } async fn remove(&self, p: &str) -> Result<(), ToolIoError> { - let resolved = path::resolve(&self.cwd, p, true, self.policy.as_ref())?; + let resolved = self.resolve_checked(p, true)?; let meta = tokio::fs::metadata(&resolved).await?; if meta.is_dir() { tokio::fs::remove_dir_all(&resolved).await?; @@ -71,54 +88,30 @@ impl Backend for LocalBackend { } async fn create_dir_all(&self, p: &str) -> Result<(), ToolIoError> { - let resolved = path::resolve(&self.cwd, p, true, self.policy.as_ref())?; - tokio::fs::create_dir_all(&resolved).await?; + tokio::fs::create_dir_all(self.resolve_checked(p, true)?).await?; Ok(()) } async fn copy(&self, from: &str, to: &str) -> Result<(), ToolIoError> { - let src = path::resolve(&self.cwd, from, false, self.policy.as_ref())?; - let dst = path::resolve(&self.cwd, to, true, self.policy.as_ref())?; + let src = self.resolve_checked(from, false)?; + let dst = self.resolve_checked(to, true)?; tokio::fs::copy(&src, &dst).await?; Ok(()) } async fn rename(&self, from: &str, to: &str) -> Result<(), ToolIoError> { - let src = path::resolve(&self.cwd, from, true, self.policy.as_ref())?; - let dst = path::resolve(&self.cwd, to, true, self.policy.as_ref())?; + let src = self.resolve_checked(from, true)?; + let dst = self.resolve_checked(to, true)?; tokio::fs::rename(&src, &dst).await?; Ok(()) } async fn file_info(&self, p: &str) -> Result { - let resolved = path::resolve(&self.cwd, p, false, self.policy.as_ref())?; - fs::get_file_info(&resolved).await + fs::get_file_info(&self.resolve_checked(p, false)?).await } async fn ls(&self, p: &str) -> Result { - let resolved = path::resolve(&self.cwd, p, false, self.policy.as_ref())?; - let mut rd = tokio::fs::read_dir(&resolved).await?; - let mut entries = Vec::new(); - while let Some(entry) = rd.next_entry().await? { - let meta = entry.metadata().await?; - let ft = entry.file_type().await?; - let modified = meta.modified().ok().and_then(|t| { - t.duration_since(std::time::UNIX_EPOCH) - .ok() - .map(|d| d.as_secs()) - }); - let permissions = extract_permissions(&meta); - entries.push(LsEntry { - name: entry.file_name().to_string_lossy().into_owned(), - is_dir: ft.is_dir(), - is_symlink: ft.is_symlink(), - size: meta.len(), - modified, - permissions, - }); - } - entries.sort_by(|a, b| a.name.cmp(&b.name)); - Ok(LsResult { entries }) + platform::list_directory(&self.resolve_checked(p, false)?).await } async fn glob(&self, opts: &GlobOptions) -> Result { @@ -140,12 +133,11 @@ impl Backend for LocalBackend { } fn resolve_path(&self, raw: &str, is_write: bool) -> Result { - path::resolve(&self.cwd, raw, is_write, self.policy.as_ref()) + self.resolve_checked(raw, is_write) } async fn read_raw(&self, p: &str) -> Result { - let resolved = path::resolve(&self.cwd, p, false, self.policy.as_ref())?; - fs::read_raw_file(&resolved, &self.limits).await + fs::read_raw_file(&self.resolve_checked(p, false)?, &self.limits).await } fn cwd(&self) -> &Path { @@ -179,6 +171,7 @@ impl Backend for LocalBackend { ) .await } + async fn exec_background(&self, command: &str) -> Result { let data = shell::exec_background(&self.cwd, self.policy.as_ref(), command).await?; Ok(ProcessHandle(Box::new(data))) @@ -187,15 +180,16 @@ impl Backend for LocalBackend { async fn fetch(&self, url: &str) -> Result { net::fetch_url(url, self.policy.as_ref(), &self.limits).await } -} -#[cfg(unix)] -fn extract_permissions(meta: &std::fs::Metadata) -> Option { - use std::os::unix::fs::PermissionsExt; - Some(meta.permissions().mode()) -} + fn approve_path(&self, p: &Path) { + self.approved.insert(p.to_path_buf()); + } -#[cfg(not(unix))] -fn extract_permissions(_meta: &std::fs::Metadata) -> Option { - None + fn check_sandbox_path(&self, raw: &str, is_write: bool) -> Option { + let abs = path::to_absolute(&self.cwd, raw); + if self.approved.contains(&abs) { + return None; + } + path::check_requires_approval(&self.cwd, raw, is_write, self.policy.as_ref()) + } } diff --git a/crates/loopal-backend/src/path.rs b/crates/loopal-backend/src/path.rs index 4514e5e9..69e28833 100644 --- a/crates/loopal-backend/src/path.rs +++ b/crates/loopal-backend/src/path.rs @@ -113,7 +113,26 @@ fn check_with_policy( ) -> Result { match loopal_sandbox::path_checker::check_path(policy, path, is_write) { PathDecision::Allow => Ok(path.to_path_buf()), - PathDecision::DenyWrite(reason) => Err(ToolIoError::PermissionDenied(reason)), - PathDecision::DenyRead(reason) => Err(ToolIoError::PermissionDenied(reason)), + PathDecision::Deny(reason) => Err(ToolIoError::PermissionDenied(reason)), + PathDecision::RequiresApproval(reason) => Err(ToolIoError::RequiresApproval(reason)), + } +} + +/// Check whether a path would require sandbox approval (without executing I/O). +/// +/// Returns `Some(reason)` if approval is needed, `None` if allowed. +/// Used by the runtime's sandbox pre-check phase to route through the +/// permission system before tool execution. +pub fn check_requires_approval( + cwd: &Path, + raw: &str, + is_write: bool, + policy: Option<&ResolvedPolicy>, +) -> Option { + let path = to_absolute(cwd, raw); + let pol = policy?; + match loopal_sandbox::path_checker::check_path(pol, &path, is_write) { + PathDecision::RequiresApproval(reason) => Some(reason), + _ => None, } } diff --git a/crates/loopal-backend/src/platform.rs b/crates/loopal-backend/src/platform.rs new file mode 100644 index 00000000..27d5e423 --- /dev/null +++ b/crates/loopal-backend/src/platform.rs @@ -0,0 +1,43 @@ +//! Platform-specific helpers and directory listing. + +use std::path::Path; + +use loopal_error::ToolIoError; +use loopal_tool_api::backend_types::{LsEntry, LsResult}; + +/// Extract Unix permission bits from file metadata. +#[cfg(unix)] +pub fn extract_permissions(meta: &std::fs::Metadata) -> Option { + use std::os::unix::fs::PermissionsExt; + Some(meta.permissions().mode()) +} + +#[cfg(not(unix))] +pub fn extract_permissions(_meta: &std::fs::Metadata) -> Option { + None +} + +/// List a directory's contents sorted by name. +pub async fn list_directory(resolved: &Path) -> Result { + let mut rd = tokio::fs::read_dir(resolved).await?; + let mut entries = Vec::new(); + while let Some(entry) = rd.next_entry().await? { + let meta = entry.metadata().await?; + let ft = entry.file_type().await?; + let modified = meta.modified().ok().and_then(|t| { + t.duration_since(std::time::UNIX_EPOCH) + .ok() + .map(|d| d.as_secs()) + }); + entries.push(LsEntry { + name: entry.file_name().to_string_lossy().into_owned(), + is_dir: ft.is_dir(), + is_symlink: ft.is_symlink(), + size: meta.len(), + modified, + permissions: extract_permissions(&meta), + }); + } + entries.sort_by(|a, b| a.name.cmp(&b.name)); + Ok(LsResult { entries }) +} diff --git a/crates/loopal-backend/tests/suite.rs b/crates/loopal-backend/tests/suite.rs new file mode 100644 index 00000000..5f860b94 --- /dev/null +++ b/crates/loopal-backend/tests/suite.rs @@ -0,0 +1,7 @@ +// Single test binary for loopal-backend +#[path = "suite/approved_paths_test.rs"] +mod approved_paths_test; +#[path = "suite/path_approval_test.rs"] +mod path_approval_test; +#[path = "suite/resolve_checked_test.rs"] +mod resolve_checked_test; diff --git a/crates/loopal-backend/tests/suite/approved_paths_test.rs b/crates/loopal-backend/tests/suite/approved_paths_test.rs new file mode 100644 index 00000000..35e3ebee --- /dev/null +++ b/crates/loopal-backend/tests/suite/approved_paths_test.rs @@ -0,0 +1,43 @@ +//! Unit tests for ApprovedPaths (session-scoped approval set). + +use std::path::PathBuf; + +use loopal_backend::approved::ApprovedPaths; + +#[test] +fn empty_set_contains_nothing() { + let ap = ApprovedPaths::new(); + assert!(!ap.contains(&PathBuf::from("/etc/hosts"))); + assert!(!ap.contains(&PathBuf::from("/tmp/test.txt"))); +} + +#[test] +fn insert_then_contains() { + let ap = ApprovedPaths::new(); + let path = PathBuf::from("/etc/nginx/nginx.conf"); + ap.insert(path.clone()); + assert!(ap.contains(&path)); +} + +#[test] +fn distinct_paths_independent() { + let ap = ApprovedPaths::new(); + ap.insert(PathBuf::from("/etc/hosts")); + assert!(ap.contains(&PathBuf::from("/etc/hosts"))); + assert!(!ap.contains(&PathBuf::from("/etc/passwd"))); +} + +#[test] +fn duplicate_insert_is_idempotent() { + let ap = ApprovedPaths::new(); + let path = PathBuf::from("/tmp/test.txt"); + ap.insert(path.clone()); + ap.insert(path.clone()); + assert!(ap.contains(&path)); +} + +#[test] +fn default_is_empty() { + let ap = ApprovedPaths::default(); + assert!(!ap.contains(&PathBuf::from("/any/path"))); +} diff --git a/crates/loopal-backend/tests/suite/path_approval_test.rs b/crates/loopal-backend/tests/suite/path_approval_test.rs new file mode 100644 index 00000000..4a4ec103 --- /dev/null +++ b/crates/loopal-backend/tests/suite/path_approval_test.rs @@ -0,0 +1,79 @@ +//! Tests for path.rs: check_requires_approval function. + +use std::path::PathBuf; + +use loopal_backend::path::check_requires_approval; +use loopal_config::{NetworkPolicy, ResolvedPolicy, SandboxPolicy}; + +fn workspace_policy(cwd: &str) -> ResolvedPolicy { + let cwd_path = PathBuf::from(cwd); + let cwd_canon = cwd_path.canonicalize().unwrap_or(cwd_path); + let tmp_canon = std::env::temp_dir() + .canonicalize() + .unwrap_or_else(|_| std::env::temp_dir()); + ResolvedPolicy { + policy: SandboxPolicy::WorkspaceWrite, + writable_paths: vec![cwd_canon, tmp_canon], + deny_write_globs: vec!["**/.env".to_string()], + deny_read_globs: vec!["**/secret.key".to_string()], + network: NetworkPolicy::default(), + } +} + +#[test] +fn returns_none_when_policy_is_none() { + assert!(check_requires_approval(&PathBuf::from("/tmp"), "/etc/hosts", true, None).is_none()); +} + +#[test] +fn returns_none_for_allowed_path() { + let tmp = std::env::temp_dir(); + let policy = workspace_policy(tmp.to_str().unwrap()); + let target = tmp.join("allowed.txt"); + assert!(check_requires_approval(&tmp, target.to_str().unwrap(), true, Some(&policy)).is_none()); +} + +#[test] +fn returns_some_for_write_outside_cwd() { + let tmp = std::env::temp_dir(); + let policy = workspace_policy(tmp.to_str().unwrap()); + let reason = check_requires_approval(&tmp, "/usr/local/bin/evil", true, Some(&policy)); + assert!( + reason.is_some(), + "expected RequiresApproval for outside-cwd write" + ); + assert!(reason.unwrap().contains("outside writable")); +} + +#[test] +fn returns_some_for_deny_write_glob() { + let tmp = std::env::temp_dir(); + let policy = workspace_policy(tmp.to_str().unwrap()); + let env_path = tmp.join(".env"); + let reason = check_requires_approval(&tmp, env_path.to_str().unwrap(), true, Some(&policy)); + assert!(reason.is_some(), "expected RequiresApproval for .env write"); +} + +#[test] +fn returns_some_for_deny_read_glob() { + let tmp = std::env::temp_dir(); + let policy = workspace_policy(tmp.to_str().unwrap()); + let key_path = tmp.join("secret.key"); + let reason = check_requires_approval(&tmp, key_path.to_str().unwrap(), false, Some(&policy)); + assert!( + reason.is_some(), + "expected RequiresApproval for deny_read_glob" + ); +} + +#[test] +fn returns_none_for_read_not_in_deny_glob() { + let policy = workspace_policy("/home/user/project"); + let reason = check_requires_approval( + &PathBuf::from("/home/user/project"), + "/etc/hosts", + false, + Some(&policy), + ); + assert!(reason.is_none(), "reads outside cwd are allowed by default"); +} diff --git a/crates/loopal-backend/tests/suite/resolve_checked_test.rs b/crates/loopal-backend/tests/suite/resolve_checked_test.rs new file mode 100644 index 00000000..74b12ce6 --- /dev/null +++ b/crates/loopal-backend/tests/suite/resolve_checked_test.rs @@ -0,0 +1,169 @@ +//! Tests for LocalBackend: resolve_checked, approve_path, check_sandbox_path. + +use std::sync::Arc; + +use loopal_backend::{LocalBackend, ResourceLimits}; +use loopal_config::{NetworkPolicy, ResolvedPolicy, SandboxPolicy}; +use loopal_error::ToolIoError; +use loopal_tool_api::Backend; + +fn make_backend(cwd: &std::path::Path) -> Arc { + let cwd_canon = cwd.canonicalize().unwrap_or_else(|_| cwd.to_path_buf()); + let policy = ResolvedPolicy { + policy: SandboxPolicy::WorkspaceWrite, + writable_paths: vec![cwd_canon], + deny_write_globs: vec!["**/.env".to_string()], + deny_read_globs: vec![], + network: NetworkPolicy::default(), + }; + LocalBackend::new(cwd.to_path_buf(), Some(policy), ResourceLimits::default()) +} + +fn make_readonly_backend(cwd: &std::path::Path) -> Arc { + let policy = ResolvedPolicy { + policy: SandboxPolicy::ReadOnly, + writable_paths: vec![], + deny_write_globs: vec![], + deny_read_globs: vec![], + network: NetworkPolicy::default(), + }; + LocalBackend::new(cwd.to_path_buf(), Some(policy), ResourceLimits::default()) +} + +// ── check_sandbox_path ─────────────────────────────────────────── + +#[test] +fn check_sandbox_path_returns_none_for_allowed() { + let dir = tempfile::tempdir().unwrap(); + let backend = make_backend(dir.path()); + let target = dir.path().join("test.txt"); + assert!( + backend + .check_sandbox_path(target.to_str().unwrap(), true) + .is_none() + ); +} + +#[test] +fn check_sandbox_path_returns_reason_for_outside_cwd() { + let dir = tempfile::tempdir().unwrap(); + let backend = make_backend(dir.path()); + let reason = backend.check_sandbox_path("/usr/local/bin/evil", true); + assert!(reason.is_some()); + assert!(reason.unwrap().contains("outside writable")); +} + +#[test] +fn check_sandbox_path_returns_reason_for_deny_glob() { + let dir = tempfile::tempdir().unwrap(); + let backend = make_backend(dir.path()); + let env_path = dir.path().join(".env"); + let reason = backend.check_sandbox_path(env_path.to_str().unwrap(), true); + assert!(reason.is_some()); +} + +#[test] +fn check_sandbox_path_returns_none_after_approve() { + let dir = tempfile::tempdir().unwrap(); + let backend = make_backend(dir.path()); + let path = std::path::PathBuf::from("/usr/local/bin/evil"); + + // Before approval: needs approval + assert!( + backend + .check_sandbox_path("/usr/local/bin/evil", true) + .is_some() + ); + + // After approval: no longer needs approval + backend.approve_path(&path); + assert!( + backend + .check_sandbox_path("/usr/local/bin/evil", true) + .is_none() + ); +} + +// ── resolve_checked (via Backend methods) ──────────────────────── + +#[tokio::test] +async fn write_to_allowed_path_succeeds() { + let dir = tempfile::tempdir().unwrap(); + let backend = make_backend(dir.path()); + let target = dir.path().join("test.txt"); + let result = backend.write(target.to_str().unwrap(), "hello").await; + assert!(result.is_ok()); +} + +#[tokio::test] +async fn write_outside_cwd_returns_requires_approval() { + let dir = tempfile::tempdir().unwrap(); + let backend = make_backend(dir.path()); + let result = backend.write("/usr/local/bin/evil", "bad").await; + assert!(matches!(result, Err(ToolIoError::RequiresApproval(_)))); +} + +#[tokio::test] +async fn write_to_deny_glob_returns_requires_approval() { + let dir = tempfile::tempdir().unwrap(); + let backend = make_backend(dir.path()); + let env_path = dir.path().join(".env"); + let result = backend.write(env_path.to_str().unwrap(), "SECRET=x").await; + assert!(matches!(result, Err(ToolIoError::RequiresApproval(_)))); +} + +#[tokio::test] +async fn write_outside_cwd_succeeds_after_approval() { + let dir = tempfile::tempdir().unwrap(); + let backend = make_backend(dir.path()); + + // Create a writable target directory for the test + let target_dir = tempfile::tempdir().unwrap(); + let target = target_dir.path().join("approved.txt"); + + // Before approval: fails + assert!(matches!( + backend.write(target.to_str().unwrap(), "data").await, + Err(ToolIoError::RequiresApproval(_)) + )); + + // Approve the path (use to_absolute logic: absolute path as-is) + backend.approve_path(&target); + + // After approval: succeeds + let result = backend.write(target.to_str().unwrap(), "data").await; + assert!( + result.is_ok(), + "expected success after approval, got: {result:?}" + ); + assert_eq!(std::fs::read_to_string(&target).unwrap(), "data"); +} + +#[tokio::test] +async fn readonly_mode_hard_denies_writes() { + let dir = tempfile::tempdir().unwrap(); + let backend = make_readonly_backend(dir.path()); + let target = dir.path().join("test.txt"); + let result = backend.write(target.to_str().unwrap(), "data").await; + // ReadOnly returns PermissionDenied (hard), not RequiresApproval (soft) + assert!(matches!(result, Err(ToolIoError::PermissionDenied(_)))); +} + +#[tokio::test] +async fn approval_is_session_scoped_across_calls() { + let dir = tempfile::tempdir().unwrap(); + let backend = make_backend(dir.path()); + let target_dir = tempfile::tempdir().unwrap(); + let target = target_dir.path().join("reuse.txt"); + + backend.approve_path(&target); + + // First write + let r1 = backend.write(target.to_str().unwrap(), "first").await; + assert!(r1.is_ok()); + + // Second write to same path — no re-approval needed + let r2 = backend.write(target.to_str().unwrap(), "second").await; + assert!(r2.is_ok()); + assert_eq!(std::fs::read_to_string(&target).unwrap(), "second"); +} diff --git a/crates/loopal-config/src/sandbox.rs b/crates/loopal-config/src/sandbox.rs index 505fea65..9d073677 100644 --- a/crates/loopal-config/src/sandbox.rs +++ b/crates/loopal-config/src/sandbox.rs @@ -73,8 +73,12 @@ pub struct ResolvedPolicy { #[derive(Debug, Clone, PartialEq, Eq)] pub enum PathDecision { Allow, - DenyWrite(String), - DenyRead(String), + /// Hard deny — cannot be overridden (ReadOnly mode, path resolution failure). + Deny(String), + /// Soft deny — the operation is outside normal sandbox bounds but can be + /// approved through the permission system (Bypass auto-allows, Supervised + /// asks the user, Auto asks the LLM classifier). + RequiresApproval(String), } /// Decision from command-level sandbox check. diff --git a/crates/loopal-error/src/io_error.rs b/crates/loopal-error/src/io_error.rs index ee7af99b..a38897db 100644 --- a/crates/loopal-error/src/io_error.rs +++ b/crates/loopal-error/src/io_error.rs @@ -46,4 +46,11 @@ pub enum ToolIoError { #[error("{0}")] Other(String), + + /// The operation requires explicit user/classifier approval before it can + /// proceed (e.g. writing outside the working directory). Distinguished + /// from `PermissionDenied` (hard block) so the runtime can route it + /// through the permission system. + #[error("requires approval: {0}")] + RequiresApproval(String), } diff --git a/crates/loopal-runtime/src/agent_loop/mod.rs b/crates/loopal-runtime/src/agent_loop/mod.rs index 74fb053c..6c44655a 100644 --- a/crates/loopal-runtime/src/agent_loop/mod.rs +++ b/crates/loopal-runtime/src/agent_loop/mod.rs @@ -20,6 +20,9 @@ mod resume_session; pub mod rewind; mod run; mod runner; +/// Sandbox path pre-check utilities for the tools_check phase. +/// Public for integration testing; runtime consumers should use tools_check directly. +pub mod sandbox_precheck; pub(crate) mod token_accumulator; mod tool_collect; pub(crate) mod tool_exec; diff --git a/crates/loopal-runtime/src/agent_loop/sandbox_precheck.rs b/crates/loopal-runtime/src/agent_loop/sandbox_precheck.rs new file mode 100644 index 00000000..3f7f50cf --- /dev/null +++ b/crates/loopal-runtime/src/agent_loop/sandbox_precheck.rs @@ -0,0 +1,96 @@ +//! Sandbox path pre-check: extract paths from tool input, check sandbox +//! policy, and determine if approval is needed before tool execution. + +use loopal_tool_api::Backend; +use serde_json::Value; + +/// A path that requires sandbox approval before the tool can execute. +pub struct ApprovalNeeded { + pub path: String, + /// Whether the operation is a write (vs read). Reserved for future use + /// when command-level and network-level approval share this struct. + #[allow(dead_code)] + pub is_write: bool, + pub reason: String, +} + +/// Extract file paths from a tool's input based on tool name conventions. +/// +/// Returns `Vec<(raw_path, is_write)>`. Tools with opaque path semantics +/// (Bash, Glob, Grep, MCP, etc.) return an empty list — the execution-time +/// fallback in `LocalBackend::resolve_checked` handles those. +pub fn extract_paths(tool_name: &str, input: &Value) -> Vec<(String, bool)> { + match tool_name { + "Write" | "Edit" | "MultiEdit" => single(input, "file_path", true), + "Read" => single(input, "file_path", false), + "Delete" => single(input, "path", true), + "MoveFile" => { + let mut v = single(input, "src", true); + v.extend(single(input, "dst", true)); + v + } + "CopyFile" => { + let mut v = single(input, "src", false); + v.extend(single(input, "dst", true)); + v + } + "ApplyPatch" => patch_paths(input), + _ => Vec::new(), + } +} + +/// Check extracted paths against the sandbox, returning any that need approval. +pub fn check_paths(backend: &dyn Backend, paths: &[(String, bool)]) -> Vec { + paths + .iter() + .filter_map(|(raw, is_write)| { + backend + .check_sandbox_path(raw, *is_write) + .map(|reason| ApprovalNeeded { + path: raw.clone(), + is_write: *is_write, + reason, + }) + }) + .collect() +} + +/// Approve all paths from `needs` via `backend.approve_path()`. +pub fn approve_all(backend: &dyn Backend, needs: &[ApprovalNeeded]) { + for n in needs { + let p = std::path::Path::new(&n.path); + let abs = if p.is_absolute() { + p.to_path_buf() + } else { + backend.cwd().join(p) + }; + backend.approve_path(&abs); + } +} + +fn single(input: &Value, key: &str, is_write: bool) -> Vec<(String, bool)> { + input + .get(key) + .and_then(|v| v.as_str()) + .map(|s| vec![(s.to_string(), is_write)]) + .unwrap_or_default() +} + +fn patch_paths(input: &Value) -> Vec<(String, bool)> { + let patch = match input.get("patch").and_then(|v| v.as_str()) { + Some(p) => p, + None => return Vec::new(), + }; + patch + .lines() + .filter(|l| l.starts_with("*** ")) + .filter_map(|l| { + l.strip_prefix("*** ").map(|p| { + // Strip trailing timestamp (unified diff: `*** file\ttimestamp`). + let path = p.split('\t').next().unwrap_or(p).trim(); + (path.to_string(), true) + }) + }) + .filter(|(p, _)| !p.is_empty()) + .collect() +} diff --git a/crates/loopal-runtime/src/agent_loop/tools_check.rs b/crates/loopal-runtime/src/agent_loop/tools_check.rs index 9c65a533..d3fe97ff 100644 --- a/crates/loopal-runtime/src/agent_loop/tools_check.rs +++ b/crates/loopal-runtime/src/agent_loop/tools_check.rs @@ -5,11 +5,12 @@ use loopal_message::ContentBlock; use loopal_protocol::AgentEventPayload; -use loopal_tool_api::PermissionDecision; +use loopal_tool_api::{PermissionDecision, PermissionLevel}; use tracing::info; use super::cancel::TurnCancel; use super::runner::AgentLoopRunner; +use super::sandbox_precheck; /// Result of the precheck + permission phase. pub(super) struct CheckResult { @@ -68,7 +69,7 @@ impl AgentLoopRunner { } } - // Sandbox precheck + // Sandbox precheck (tool-level, e.g. Bash command checks) let precheck_reason = self .params .deps @@ -84,30 +85,64 @@ impl AgentLoopRunner { continue; } - // Fast-path permission check (no LLM call) + // Sandbox path pre-check: detect RequiresApproval paths before execution. + let extracted = sandbox_precheck::extract_paths(name, input); + let sandbox_needs = + sandbox_precheck::check_paths(self.tool_ctx.backend.as_ref(), &extracted); + + // Determine effective permission level — elevate to Dangerous when + // the sandbox requires approval so it flows through the permission system. let tool_perm = self .params .deps .kernel .get_tool(name) .map(|t| t.permission()); - let decision = tool_perm + let effective_perm = if sandbox_needs.is_empty() { + tool_perm + } else { + Some(PermissionLevel::Dangerous) + }; + + let decision = effective_perm .map(|p| self.params.config.permission_mode.check(p)) .unwrap_or(PermissionDecision::Allow); if decision != PermissionDecision::Ask { + // Auto-approve paths when permission mode allows it (e.g. Bypass). + if !sandbox_needs.is_empty() { + sandbox_precheck::approve_all(self.tool_ctx.backend.as_ref(), &sandbox_needs); + } approved.push((id.clone(), name.clone(), input.clone())); continue; } - // Needs further decision — collect for batch or human - needs_classify.push((orig_idx, id.clone(), name.clone(), input.clone())); + // Annotate input with sandbox reason for the permission prompt. + let annotated = if sandbox_needs.is_empty() { + input.clone() + } else { + let reasons: Vec<&str> = sandbox_needs.iter().map(|n| n.reason.as_str()).collect(); + let mut a = input.clone(); + a["sandbox_approval_reason"] = serde_json::Value::String(reasons.join("; ")); + a + }; + + needs_classify.push((orig_idx, id.clone(), name.clone(), annotated)); } // Parallel auto-classification or sequential human approval self.resolve_pending(&mut approved, &mut denied, needs_classify) .await?; + // Post-approval: approve sandbox paths for tools that were just granted permission. + for (_, name, input) in &approved { + let extracted = sandbox_precheck::extract_paths(name, input); + let needs = sandbox_precheck::check_paths(self.tool_ctx.backend.as_ref(), &extracted); + if !needs.is_empty() { + sandbox_precheck::approve_all(self.tool_ctx.backend.as_ref(), &needs); + } + } + // Mark unprocessed tools as interrupted for (id, name, _) in &remaining[processed..] { let orig_idx = tool_uses diff --git a/crates/loopal-runtime/tests/agent_loop/auto_mode_test.rs b/crates/loopal-runtime/tests/agent_loop/auto_mode_test.rs index 4bd049f9..a0519518 100644 --- a/crates/loopal-runtime/tests/agent_loop/auto_mode_test.rs +++ b/crates/loopal-runtime/tests/agent_loop/auto_mode_test.rs @@ -39,10 +39,13 @@ async fn readonly_tool_skips_classifier() { async fn supervised_tool_skips_classifier() { let (mut runner, mut event_rx) = make_auto_runner(vec![]); + // Use a path under the session's cwd so sandbox doesn't require approval. + let cwd = runner.tool_ctx.backend.cwd().to_path_buf(); + let target = cwd.join("test.txt"); let tool_uses = vec![( "tc-1".into(), "Write".into(), - serde_json::json!({"file_path": "/tmp/test.txt", "content": "hello"}), + serde_json::json!({"file_path": target.to_str().unwrap(), "content": "hello"}), )]; runner diff --git a/crates/loopal-runtime/tests/suite.rs b/crates/loopal-runtime/tests/suite.rs index d3075550..332d4a99 100644 --- a/crates/loopal-runtime/tests/suite.rs +++ b/crates/loopal-runtime/tests/suite.rs @@ -19,6 +19,8 @@ mod permission_test; mod plan_file_test; #[path = "suite/rewind_test.rs"] mod rewind_test; +#[path = "suite/sandbox_precheck_test.rs"] +mod sandbox_precheck_test; #[path = "suite/session_manager_test.rs"] mod session_manager_test; #[path = "suite/session_test.rs"] diff --git a/crates/loopal-runtime/tests/suite/sandbox_precheck_test.rs b/crates/loopal-runtime/tests/suite/sandbox_precheck_test.rs new file mode 100644 index 00000000..a0f8e6d1 --- /dev/null +++ b/crates/loopal-runtime/tests/suite/sandbox_precheck_test.rs @@ -0,0 +1,116 @@ +//! Tests for sandbox_precheck: extract_paths, patch timestamp stripping. + +use loopal_runtime::agent_loop::sandbox_precheck::extract_paths; +use serde_json::json; + +// ── extract_paths for each tool ────────────────────────────────── + +#[test] +fn write_extracts_file_path() { + let input = json!({"file_path": "/etc/nginx.conf", "content": "data"}); + let paths = extract_paths("Write", &input); + assert_eq!(paths, vec![("/etc/nginx.conf".to_string(), true)]); +} + +#[test] +fn read_extracts_file_path_as_non_write() { + let input = json!({"file_path": "/etc/hosts"}); + let paths = extract_paths("Read", &input); + assert_eq!(paths, vec![("/etc/hosts".to_string(), false)]); +} + +#[test] +fn edit_extracts_file_path() { + let input = json!({"file_path": "src/main.rs", "old_string": "a", "new_string": "b"}); + let paths = extract_paths("Edit", &input); + assert_eq!(paths, vec![("src/main.rs".to_string(), true)]); +} + +#[test] +fn multi_edit_extracts_file_path() { + let input = json!({"file_path": "src/lib.rs", "edits": []}); + let paths = extract_paths("MultiEdit", &input); + assert_eq!(paths, vec![("src/lib.rs".to_string(), true)]); +} + +#[test] +fn delete_extracts_path_field() { + let input = json!({"path": "/tmp/old.txt"}); + let paths = extract_paths("Delete", &input); + assert_eq!(paths, vec![("/tmp/old.txt".to_string(), true)]); +} + +#[test] +fn move_file_extracts_src_and_dst() { + let input = json!({"src": "/a/b", "dst": "/c/d"}); + let paths = extract_paths("MoveFile", &input); + assert_eq!(paths.len(), 2); + assert_eq!(paths[0], ("/a/b".to_string(), true)); + assert_eq!(paths[1], ("/c/d".to_string(), true)); +} + +#[test] +fn copy_file_src_is_read_dst_is_write() { + let input = json!({"src": "/a/b", "dst": "/c/d"}); + let paths = extract_paths("CopyFile", &input); + assert_eq!(paths[0], ("/a/b".to_string(), false)); // src: read + assert_eq!(paths[1], ("/c/d".to_string(), true)); // dst: write +} + +#[test] +fn unknown_tool_returns_empty() { + let input = json!({"command": "ls -la"}); + assert!(extract_paths("Bash", &input).is_empty()); + assert!(extract_paths("Glob", &input).is_empty()); + assert!(extract_paths("UnknownMcpTool", &input).is_empty()); +} + +#[test] +fn missing_file_path_returns_empty() { + let input = json!({"content": "data"}); + assert!(extract_paths("Write", &input).is_empty()); +} + +#[test] +fn null_file_path_returns_empty() { + let input = json!({"file_path": null}); + assert!(extract_paths("Write", &input).is_empty()); +} + +#[test] +fn numeric_file_path_returns_empty() { + let input = json!({"file_path": 123}); + assert!(extract_paths("Write", &input).is_empty()); +} + +// ── ApplyPatch path extraction ─────────────────────────────────── + +#[test] +fn apply_patch_extracts_star_lines() { + let input = json!({ + "patch": "*** /etc/nginx.conf\n--- old\n+++ new\n@@ -1 +1 @@\n-old\n+new" + }); + let paths = extract_paths("ApplyPatch", &input); + assert_eq!(paths, vec![("/etc/nginx.conf".to_string(), true)]); +} + +#[test] +fn apply_patch_strips_timestamp() { + let input = json!({ + "patch": "*** /etc/nginx.conf\t2024-01-01 12:00:00\n" + }); + let paths = extract_paths("ApplyPatch", &input); + assert_eq!(paths, vec![("/etc/nginx.conf".to_string(), true)]); +} + +#[test] +fn apply_patch_empty_patch_returns_empty() { + let input = json!({"patch": ""}); + assert!(extract_paths("ApplyPatch", &input).is_empty()); +} + +#[test] +fn apply_patch_no_patch_field_returns_empty() { + let input = json!({"file_path": "test.txt"}); + assert!(extract_paths("ApplyPatch", &input).is_empty()); +} diff --git a/crates/loopal-sandbox/src/path_checker.rs b/crates/loopal-sandbox/src/path_checker.rs index 7e0f2094..a2e7e521 100644 --- a/crates/loopal-sandbox/src/path_checker.rs +++ b/crates/loopal-sandbox/src/path_checker.rs @@ -15,26 +15,27 @@ pub fn check_path(policy: &ResolvedPolicy, path: &Path, is_write: bool) -> PathD // Resolve symlinks and normalize to a canonical path let canonical = match resolve_canonical(path) { Ok(p) => p, - Err(reason) => return PathDecision::DenyWrite(reason), + Err(reason) => return PathDecision::Deny(reason), }; - // Check read denials first (applies to both read and write) + // Check read denials first (applies to both read and write). + // Soft deny — can be overridden through the permission system. if let Some(reason) = check_deny_globs(&canonical, &policy.deny_read_globs, "read") { - return PathDecision::DenyRead(reason); + return PathDecision::RequiresApproval(reason); } if !is_write { return PathDecision::Allow; } - // Read-only mode blocks all writes + // Read-only mode blocks all writes — hard deny, no override. if policy.policy == SandboxPolicy::ReadOnly { - return PathDecision::DenyWrite("read-only sandbox: all writes are blocked".into()); + return PathDecision::Deny("read-only sandbox: all writes are blocked".into()); } - // Check explicit write denials + // Check explicit write denials — soft deny via permission system. if let Some(reason) = check_deny_globs(&canonical, &policy.deny_write_globs, "write") { - return PathDecision::DenyWrite(reason); + return PathDecision::RequiresApproval(reason); } // Check whether the path is under a writable directory @@ -42,7 +43,8 @@ pub fn check_path(policy: &ResolvedPolicy, path: &Path, is_write: bool) -> PathD return PathDecision::Allow; } - PathDecision::DenyWrite(format!( + // Outside writable directories — soft deny via permission system. + PathDecision::RequiresApproval(format!( "path outside writable directories: {}", canonical.display() )) diff --git a/crates/loopal-sandbox/tests/suite/path_checker_edge_test.rs b/crates/loopal-sandbox/tests/suite/path_checker_edge_test.rs index fac75f6e..36a75a0f 100644 --- a/crates/loopal-sandbox/tests/suite/path_checker_edge_test.rs +++ b/crates/loopal-sandbox/tests/suite/path_checker_edge_test.rs @@ -24,10 +24,11 @@ fn dotdot_traversal_detected() { // A path with ".." that resolves outside the writable area let path = PathBuf::from("/home/user/project/../../etc/passwd"); let decision = check_path(&policy, &path, true); - // Either the canonical resolution catches this or the ".." detection does + // Either the canonical resolution catches this (Deny) or it's + // outside writable dirs (RequiresApproval) — both are non-Allow. assert!( - matches!(decision, PathDecision::DenyWrite(_)), - "expected DenyWrite, got: {decision:?}" + !matches!(decision, PathDecision::Allow), + "expected non-Allow, got: {decision:?}" ); } @@ -42,13 +43,13 @@ fn multiple_deny_globs_checked() { let pem_path = tmp.join("cert.pem"); assert!(matches!( check_path(&policy, &pem_path, true), - PathDecision::DenyWrite(_) + PathDecision::RequiresApproval(_) )); let key_path = tmp.join("server.key"); assert!(matches!( check_path(&policy, &key_path, true), - PathDecision::DenyWrite(_) + PathDecision::RequiresApproval(_) )); let txt_path = tmp.join("readme.txt"); @@ -67,6 +68,6 @@ fn empty_writable_paths_blocks_all_writes() { let path = PathBuf::from("/tmp/some_file.txt"); assert!(matches!( check_path(&policy, &path, true), - PathDecision::DenyWrite(_) + PathDecision::RequiresApproval(_) )); } diff --git a/crates/loopal-sandbox/tests/suite/path_checker_test.rs b/crates/loopal-sandbox/tests/suite/path_checker_test.rs index 100f7123..5e436b86 100644 --- a/crates/loopal-sandbox/tests/suite/path_checker_test.rs +++ b/crates/loopal-sandbox/tests/suite/path_checker_test.rs @@ -53,7 +53,7 @@ fn readonly_blocks_all_writes() { assert_eq!(check_path(&policy, &path, false), PathDecision::Allow); assert!(matches!( check_path(&policy, &path, true), - PathDecision::DenyWrite(_) + PathDecision::Deny(_) )); } @@ -71,7 +71,7 @@ fn workspace_blocks_writes_outside_cwd() { let path = PathBuf::from("/usr/local/bin/evil"); assert!(matches!( check_path(&policy, &path, true), - PathDecision::DenyWrite(_) + PathDecision::RequiresApproval(_) )); } @@ -82,7 +82,7 @@ fn deny_write_glob_blocks_env_files() { let path = tmp.join(".env"); assert!(matches!( check_path(&policy, &path, true), - PathDecision::DenyWrite(_) + PathDecision::RequiresApproval(_) )); } @@ -94,7 +94,7 @@ fn deny_read_glob_blocks_reads() { let path = tmp.join("secret.txt"); assert!(matches!( check_path(&policy, &path, false), - PathDecision::DenyRead(_) + PathDecision::RequiresApproval(_) )); } diff --git a/crates/loopal-tool-api/src/backend.rs b/crates/loopal-tool-api/src/backend.rs index a6645d83..8d13abd3 100644 --- a/crates/loopal-tool-api/src/backend.rs +++ b/crates/loopal-tool-api/src/backend.rs @@ -132,4 +132,20 @@ pub trait Backend: Send + Sync { /// Fetch content from a URL. async fn fetch(&self, url: &str) -> Result; + + // --- Sandbox approval --- + + /// Record a path as approved for this session (sandbox RequiresApproval flow). + /// Once approved, subsequent operations on this path skip the approval check. + /// Default: no-op (for test backends that don't enforce sandbox). + fn approve_path(&self, _path: &Path) {} + + /// Pre-check whether a path operation would require sandbox approval. + /// Returns `Some(reason)` if approval is needed, `None` if the path is + /// allowed (either by policy or by prior approval). + /// Called by the runtime BEFORE tool execution to route through the + /// permission system. + fn check_sandbox_path(&self, _raw: &str, _is_write: bool) -> Option { + None + } }