From 31dd9f93773d48b7bacd272bcdcba8fbc3bb2b59 Mon Sep 17 00:00:00 2001 From: Gaya KACI <163902005+kacigaya@users.noreply.github.com> Date: Thu, 5 Nov 2026 17:06:11 +0000 Subject: [PATCH 1/2] feat(tools): centralize execution policy Reject invalid or out-of-scope calls before approval. Bind grants to reviewed arguments and resolved targets, then recheck before dispatch. Require approval for sensitive diffs and reject replacement symlinks during file reads. --- README.md | 21 +- src/agent.rs | 58 ++-- src/models.rs | 2 +- src/tests/agent.rs | 84 ++++- src/tests/policy.rs | 387 ++++++++++++++++++++++++ src/tests/tools.rs | 2 +- src/tools/mod.rs | 207 +++---------- src/tools/policy.rs | 383 +++++++++++++++++++++++ src/ui/mod.rs | 2 +- web/content/getting-started/commands.md | 15 + 10 files changed, 954 insertions(+), 207 deletions(-) create mode 100644 src/tests/policy.rs create mode 100644 src/tools/policy.rs diff --git a/README.md b/README.md index b7c682ad..f2c0a8ad 100644 --- a/README.md +++ b/README.md @@ -67,9 +67,24 @@ call IDs and matching results, including errors and denied actions. Incomplete native tool streams do not execute tools. Ollama keeps the JSON response format for models that do not support native tools. -Approvals, workspace file controls, and command timeouts still apply. Shell -commands do not run in an OS sandbox. `/clear` removes chat and tool history. -Sessions stay in memory; closing the app removes them. +One execution policy checks each tool call in normal turns and audits. It allows +the call, denies it, or requests approval. Invalid arguments, conflicting +aliases, workspace escapes, and calls outside the audit scope are denied before +approval. Sensitive file reads and diffs require approval. These checks also +apply to the Ollama JSON fallback. + +Approval covers the reviewed arguments and resolved targets. Execution checks +them again. If a target changes while approval is pending, the call fails and +must be submitted again. File changes reject symlink paths. Patch checks cover +all source and destination paths. + +Shell commands require approval and show their resolved working directory. The +policy does not restrict paths or network targets inside shell command text. +Commands do not run in an OS sandbox. File access controls, public URL checks, +DNS checks, and timeouts still apply. + +`/clear` removes chat and tool history. Sessions stay in memory; closing the app +removes them. ## Stop a turn diff --git a/src/agent.rs b/src/agent.rs index 1d593018..c5b9bbdd 100644 --- a/src/agent.rs +++ b/src/agent.rs @@ -13,7 +13,7 @@ use crate::models::{ }; use crate::providers::{Provider, build_provider_with_secret_store}; use crate::secrets::{SETTING_MODEL, SETTING_PROVIDER, SecretMutationReport, SecretStore}; -use crate::tools::{ToolRuntime, audit_tool_allowed, tool_can_interrupt}; +use crate::tools::{ExecutionMode, PolicyDecision, ToolRuntime, tool_can_interrupt}; const MAX_STEPS: usize = 15; const AUDIT_MAX_STEPS: usize = 30; @@ -549,29 +549,27 @@ impl Agent { ); continue; } - if audit_scope.is_some() && !audit_tool_allowed(&call.name, allow_audit_mutations) { - self.record_tool_result( - &request, - serde_json::json!({"error": "unavailable in this audit mode"}), - true, - ); - continue; - } - - if native_tools && let Err(error) = crate::tools::schema::validate_call(call) { - self.record_tool_result( - &request, - serde_json::json!({"error": error.to_string()}), - true, - ); + let mode = match audit_scope { + Some(scope) => ExecutionMode::Audit { + scope, + allow_mutations: allow_audit_mutations, + }, + None => ExecutionMode::Workspace, + }; + let mut prepared = self.tools.prepare_call(call, mode); + if let PolicyDecision::Deny(reason) = prepared.decision() { + self.record_tool_result(&request, serde_json::json!({"error": reason}), true); continue; } - let summary = self.tools.arg_summary(&call.name, &call.arguments); - if self.tools.call_requires_approval(call) { + let summary = self + .tools + .arg_summary(&prepared.call().name, &prepared.call().arguments); + if let PolicyDecision::RequireApproval(reason) = prepared.decision() { + let approval_summary = format!("{summary}; {reason}"); let (response, approval) = tokio::sync::oneshot::channel(); let _ = events.send(AgentEvent::ApprovalRequired { name: call.name.clone(), - args_summary: summary.clone(), + args_summary: approval_summary, response, }); let approved = tokio::select! { @@ -595,6 +593,7 @@ impl Agent { ); continue; } + prepared.approve(); } if cancellation.is_cancelled() { @@ -606,27 +605,14 @@ impl Agent { continue; } + let interruptible = tool_can_interrupt(&prepared.call().name); let _ = events.send(AgentEvent::ToolCall { - name: call.name.clone(), + name: prepared.call().name.clone(), args_summary: summary, }); - - let operation = async { - match audit_scope { - Some(scope) => { - self.tools - .execute_audit(call, scope, allow_audit_mutations) - .await - } - None => { - self.tools - .execute_with_cancellation(call, cancellation) - .await - } - } - }; + let operation = self.tools.execute_prepared(prepared, cancellation); // File changes must finish or roll back. Commands handle process cleanup themselves. - let result = if tool_can_interrupt(&call.name) { + let result = if interruptible { tokio::select! { biased; _ = cancellation.cancelled() => Err(anyhow::anyhow!("Tool cancelled")), diff --git a/src/models.rs b/src/models.rs index 154c8b42..d9cb246d 100644 --- a/src/models.rs +++ b/src/models.rs @@ -117,7 +117,7 @@ impl ConversationMessage { } } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct ToolCall { pub name: String, #[serde(default)] diff --git a/src/tests/agent.rs b/src/tests/agent.rs index 304fb54e..1c859c96 100644 --- a/src/tests/agent.rs +++ b/src/tests/agent.rs @@ -485,14 +485,7 @@ async fn native_turn_records_batch_results_and_executes_approved_mutation_once() result.unwrap(); assert_eq!( events, - ( - vec![ - "writeFile".to_string(), - "readFile".to_string(), - "readFile".to_string() - ], - 1 - ) + (vec!["writeFile".to_string(), "readFile".to_string()], 1) ); assert_eq!( std::fs::read_to_string(directory.path().join("result.txt")).unwrap(), @@ -929,3 +922,78 @@ async fn cancelling_an_approval_keeps_completed_changes_and_closes_remaining_cal assert!(result.is_error); } } + +#[tokio::test] +async fn policy_denies_invalid_and_outside_calls_before_approval_for_all_protocols() { + use crate::models::{AgentEvent, Completion, ToolCall}; + use serde_json::json; + for native in [true, false] { + let directory = tempfile::tempdir().unwrap(); + let workspace = directory.path().join("workspace"); + std::fs::create_dir(&workspace).unwrap(); + let mut agent = test_agent(ProviderKind::OpenAi); + agent.tools = ToolRuntime::new(workspace).unwrap(); + let calls = vec![ + ToolCall { + name: "Write".to_string(), + arguments: json!({"file_path":"../blocked", "content":"no"}), + }, + ToolCall { + name: "Bash".to_string(), + arguments: json!({"command":"must not run", "timeout":"30000"}), + }, + ]; + let first = if native { + Completion { + tool_calls: calls + .into_iter() + .enumerate() + .map(|(index, call)| crate::models::ToolRequest { + id: format!("denied_{index}"), + provider_id: None, + call, + }) + .collect(), + ..Completion::default() + } + } else { + Completion::text(json!({"assistant":"Checking", "tool_calls":calls}).to_string()) + }; + let final_text = if native { + "No actions were run".to_string() + } else { + json!({"assistant":"No actions were run", "tool_calls":[]}).to_string() + }; + agent.provider = Box::new(ScriptedProvider { + replies: std::sync::Mutex::new(std::collections::VecDeque::from([ + first, + Completion::text(final_text), + ])), + seen: Default::default(), + native, + }); + let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel(); + agent + .handle_user_message( + ConversationMessage::user("inspect"), + tx, + &TurnCancellation::default(), + ) + .await + .unwrap(); + while let Ok(event) = rx.try_recv() { + assert!(!matches!( + event, + AgentEvent::ApprovalRequired { .. } | AgentEvent::ToolCall { .. } + )); + } + let results = agent + .history + .iter() + .filter_map(|m| m.tool_result.as_ref()) + .collect::>(); + assert_eq!(results.len(), 2); + assert!(results.iter().all(|r| r.is_error)); + assert!(!directory.path().join("blocked").exists()); + } +} diff --git a/src/tests/policy.rs b/src/tests/policy.rs new file mode 100644 index 00000000..5ccef2fb --- /dev/null +++ b/src/tests/policy.rs @@ -0,0 +1,387 @@ +use std::fs; + +use serde_json::{Value, json}; +use tempfile::tempdir; + +use super::{ExecutionMode, PolicyDecision}; +use crate::models::{ToolCall, TurnCancellation}; +use crate::tools::ToolRuntime; + +fn call(name: &str, arguments: Value) -> ToolCall { + ToolCall { + name: name.to_string(), + arguments, + } +} + +#[test] +fn decisions_cover_normal_and_audit_modes() { + let root = tempdir().unwrap(); + fs::write(root.path().join("source.rs"), "code").unwrap(); + fs::write(root.path().join(".env"), "secret").unwrap(); + let tools = ToolRuntime::new(root.path().to_path_buf()).unwrap(); + let normal = tools.prepare_call( + &call("readFile", json!({"path":"source.rs"})), + ExecutionMode::Workspace, + ); + assert_eq!(normal.decision(), &PolicyDecision::Allow); + for request in [ + call("readFile", json!({"path":".env"})), + call("writeFile", json!({"path":"new.rs", "content":"code"})), + call("runCommand", json!({"command":"true"})), + call("auditDependencies", json!({})), + ] { + assert!(matches!( + tools + .prepare_call(&request, ExecutionMode::Workspace) + .decision(), + PolicyDecision::RequireApproval(_) + )); + } + for allow_mutations in [false, true] { + let mode = ExecutionMode::Audit { + scope: root.path(), + allow_mutations, + }; + for request in [ + call("runCommand", json!({"command":"true"})), + call("webFetch", json!({"url":"https://example.com"})), + ] { + assert!(matches!( + tools.prepare_call(&request, mode).decision(), + PolicyDecision::Deny(_) + )); + } + let write = tools.prepare_call( + &call("writeFile", json!({"path":"new.rs", "content":"code"})), + mode, + ); + assert_eq!( + matches!(write.decision(), PolicyDecision::RequireApproval(_)), + allow_mutations + ); + assert_eq!( + matches!(write.decision(), PolicyDecision::Deny(_)), + !allow_mutations + ); + } +} + +#[tokio::test] +async fn execution_requires_approval_for_sensitive_reads_mutations_commands_and_osv() { + let root = tempdir().unwrap(); + fs::write(root.path().join(".env"), "must not read").unwrap(); + let tools = ToolRuntime::new(root.path().to_path_buf()).unwrap(); + for request in [ + call("readFile", json!({"path":".env"})), + call("writeFile", json!({"path":"blocked", "content":"no"})), + call( + "runCommand", + json!({"command":"this command must not start"}), + ), + call("auditDependencies", json!({})), + ] { + let prepared = tools.prepare_call(&request, ExecutionMode::Workspace); + let error = tools + .execute_prepared(prepared, &TurnCancellation::default()) + .await + .unwrap_err(); + assert!(error.to_string().contains("requires user approval")); + } + assert!(!root.path().join("blocked").exists()); +} + +#[tokio::test] +async fn approval_cannot_override_denials_or_change_approved_arguments() { + let root = tempdir().unwrap(); + let tools = ToolRuntime::new(root.path().to_path_buf()).unwrap(); + let request = call("writeFile", json!({"path":"blocked", "content":"no"})); + let mut denied = tools.prepare_call( + &request, + ExecutionMode::Audit { + scope: root.path(), + allow_mutations: false, + }, + ); + denied.approve(); + assert!( + tools + .execute_prepared(denied, &TurnCancellation::default()) + .await + .is_err() + ); + assert!(!root.path().join("blocked").exists()); + let mut request = call( + "writeFile", + json!({"path":"approved", "content":"reviewed"}), + ); + let mut prepared = tools.prepare_call(&request, ExecutionMode::Workspace); + prepared.approve(); + request.arguments = json!({"path":"different", "content":"unreviewed"}); + tools + .execute_prepared(prepared, &TurnCancellation::default()) + .await + .unwrap(); + assert_eq!( + fs::read_to_string(root.path().join("approved")).unwrap(), + "reviewed" + ); + assert!(!root.path().join("different").exists()); +} + +#[tokio::test] +async fn alias_normalization_and_audit_defaults_use_the_execution_target() { + let root = tempdir().unwrap(); + let scope = root.path().join("src"); + fs::create_dir(&scope).unwrap(); + fs::write(scope.join("inside.rs"), "code").unwrap(); + fs::write(root.path().join("outside.rs"), "outside").unwrap(); + let scope = scope.canonicalize().unwrap(); + let tools = ToolRuntime::new(root.path().to_path_buf()).unwrap(); + let mode = ExecutionMode::Audit { + scope: &scope, + allow_mutations: false, + }; + let default = tools.prepare_call(&call("Glob", json!({"pattern":"*.rs"})), mode); + assert_eq!(default.decision(), &PolicyDecision::Allow); + assert_eq!(default.call().name, "searchFiles"); + assert_eq!(default.call().arguments["path"], scope.to_str().unwrap()); + assert_eq!(default.call().arguments["excludeGenerated"], true); + let result = tools + .execute_prepared(default, &TurnCancellation::default()) + .await + .unwrap(); + assert_eq!(result["matches"], json!(["inside.rs"])); + for args in [ + json!({"pattern":"*.rs", "cwd":"."}), + json!({"pattern":"*.rs", "path":"src", "cwd":"."}), + ] { + assert!(matches!( + tools.prepare_call(&call("Glob", args), mode).decision(), + PolicyDecision::Deny(_) + )); + } + let read = tools.prepare_call( + &call("Read", json!({"file_path":"src/inside.rs", "limit":1})), + mode, + ); + assert_eq!(read.decision(), &PolicyDecision::Allow); + assert_eq!(read.call().arguments["maxLines"], 1); + assert_eq!(read.call().arguments["lineNumbers"], true); +} + +#[test] +fn invalid_shapes_unknown_tools_private_urls_and_invalid_cwd_are_denied() { + let root = tempdir().unwrap(); + fs::write(root.path().join("file"), "data").unwrap(); + let tools = ToolRuntime::new(root.path().to_path_buf()).unwrap(); + for request in [ + call("unknown", json!({})), + call("readFile", json!({"path":3})), + call("readFile", json!({"path":""})), + call("readFile", json!({"path":"."})), + call("runCommand", json!({"command":" "})), + call("runCommand", json!({"command":"true", "timeout":"30000"})), + call("runCommand", json!({"command":"true", "cwd":"file"})), + call("webFetch", json!({"url":"http://127.0.0.1/"})), + call("webFetch", json!({"url":"file:///etc/passwd"})), + call("writeFile", json!({"path":"../outside", "content":"no"})), + ] { + assert!( + matches!( + tools + .prepare_call(&request, ExecutionMode::Workspace) + .decision(), + PolicyDecision::Deny(_) + ), + "{request:?}" + ); + } +} + +#[test] +fn every_patch_target_is_checked_including_moves_and_missing_parent_directories() { + let root = tempdir().unwrap(); + let scope = root.path().join("src"); + fs::create_dir(&scope).unwrap(); + fs::write(scope.join("inside"), "code\n").unwrap(); + fs::write(root.path().join("outside"), "outside\n").unwrap(); + let tools = ToolRuntime::new(root.path().to_path_buf()).unwrap(); + let mode = ExecutionMode::Audit { + scope: &scope, + allow_mutations: true, + }; + for patch in [ + "*** Begin Patch\n*** Add File: src/new\n+okay\n*** Add File: outside-new/nested\n+no\n*** End Patch", + "*** Begin Patch\n*** Delete File: outside\n*** End Patch", + "*** Begin Patch\n*** Update File: outside\n@@\n-outside\n+no\n*** End Patch", + "*** Begin Patch\n*** Update File: src/inside\n*** Move to: moved/nested\n@@\n-code\n+no\n*** End Patch", + ] { + let prepared = tools.prepare_call(&call("apply_patch", json!({"patch":patch})), mode); + assert!( + matches!(prepared.decision(), PolicyDecision::Deny(reason) if reason.contains("outside audit scope")) + ); + } + let valid = tools.prepare_call(&call("applyPatch", json!({"patchText":"*** Begin Patch\n*** Add File: src/new/nested\n+okay\n*** End Patch"})), mode); + assert!(matches!( + valid.decision(), + PolicyDecision::RequireApproval(_) + )); + assert!( + !scope.join("new").exists(), + "policy evaluation created directories" + ); + assert_eq!(fs::read_to_string(scope.join("inside")).unwrap(), "code\n"); +} + +#[test] +fn paths_in_a_sensitive_workspace_parent_remain_normal_source_reads() { + let root = tempdir().unwrap(); + let workspace = root.path().join("secrets/project"); + fs::create_dir_all(&workspace).unwrap(); + fs::write(workspace.join("main.rs"), "code").unwrap(); + let tools = ToolRuntime::new(workspace).unwrap(); + let prepared = tools.prepare_call( + &call("readFile", json!({"path":"main.rs"})), + ExecutionMode::Workspace, + ); + assert_eq!(prepared.decision(), &PolicyDecision::Allow); +} + +#[cfg(unix)] +#[tokio::test] +async fn changed_symlink_targets_require_a_new_review_even_after_approval() { + use std::os::unix::fs::symlink; + let root = tempdir().unwrap(); + fs::write(root.path().join(".env.one"), "one").unwrap(); + fs::write(root.path().join(".env.two"), "two").unwrap(); + let alias = root.path().join("config"); + symlink(".env.one", &alias).unwrap(); + let tools = ToolRuntime::new(root.path().to_path_buf()).unwrap(); + let mut prepared = tools.prepare_call( + &call("Read", json!({"path":"config"})), + ExecutionMode::Workspace, + ); + assert!(matches!( + prepared.decision(), + PolicyDecision::RequireApproval(_) + )); + prepared.approve(); + fs::remove_file(&alias).unwrap(); + symlink(".env.two", &alias).unwrap(); + let error = tools + .execute_prepared(prepared, &TurnCancellation::default()) + .await + .unwrap_err(); + assert!(error.to_string().contains("targets changed")); +} + +#[cfg(unix)] +#[tokio::test] +async fn changed_mutation_parents_and_outside_audit_roots_are_denied() { + use std::os::unix::fs::symlink; + let root = tempdir().unwrap(); + let outside = tempdir().unwrap(); + let parent = root.path().join("new"); + fs::create_dir(&parent).unwrap(); + let tools = ToolRuntime::new(root.path().to_path_buf()).unwrap(); + let mut prepared = tools.prepare_call( + &call("writeFile", json!({"path":"new/blocked", "content":"no"})), + ExecutionMode::Workspace, + ); + prepared.approve(); + fs::remove_dir(&parent).unwrap(); + symlink(outside.path(), &parent).unwrap(); + assert!( + tools + .execute_prepared(prepared, &TurnCancellation::default()) + .await + .is_err() + ); + assert!(!outside.path().join("blocked").exists()); + let outside_scope = tools.prepare_call( + &call("listDirectory", json!({})), + ExecutionMode::Audit { + scope: outside.path(), + allow_mutations: true, + }, + ); + assert!(matches!(outside_scope.decision(), PolicyDecision::Deny(_))); +} + +#[cfg(unix)] +#[tokio::test] +async fn allowed_reads_cannot_gain_sensitive_access_when_the_target_changes() { + use std::os::unix::fs::symlink; + let root = tempdir().unwrap(); + fs::write(root.path().join("public"), "public").unwrap(); + fs::write(root.path().join(".env"), "must not read").unwrap(); + let alias = root.path().join("config"); + symlink("public", &alias).unwrap(); + let tools = ToolRuntime::new(root.path().to_path_buf()).unwrap(); + let mut prepared = tools.prepare_call( + &call("Read", json!({"path":"config"})), + ExecutionMode::Workspace, + ); + assert_eq!(prepared.decision(), &PolicyDecision::Allow); + // Approving an allowed call does not grant permission for a later sensitive target. + prepared.approve(); + fs::remove_file(&alias).unwrap(); + symlink(".env", &alias).unwrap(); + let error = tools + .execute_prepared(prepared, &TurnCancellation::default()) + .await + .unwrap_err(); + assert!(error.to_string().contains("requires user approval")); +} + +#[tokio::test] +async fn sensitive_diffs_require_approval_before_exposing_file_contents() { + let root = tempdir().unwrap(); + fs::write(root.path().join(".env"), "fake-secret-value\n").unwrap(); + let tools = ToolRuntime::new(root.path().to_path_buf()).unwrap(); + let prepared = tools.prepare_call( + &call("generateDiff", json!({"path":".env", "content":""})), + ExecutionMode::Workspace, + ); + assert!(matches!( + prepared.decision(), + PolicyDecision::RequireApproval(_) + )); + assert!( + tools + .execute_prepared(prepared, &TurnCancellation::default()) + .await + .is_err() + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn file_content_reads_reject_symlink_changes_after_the_final_policy_check() { + use std::os::unix::fs::symlink; + let root = tempdir().unwrap(); + fs::write(root.path().join(".env"), "fake-secret-value").unwrap(); + let tools = ToolRuntime::new(root.path().to_path_buf()).unwrap(); + for name in ["readFile", "generateDiff"] { + let target = root.path().join("public"); + fs::write(&target, "public").unwrap(); + let args = if name == "generateDiff" { + json!({"path":"public", "content":""}) + } else { + json!({"path":"public"}) + }; + let prepared = tools.prepare_call(&call(name, args), ExecutionMode::Workspace); + assert_eq!(prepared.decision(), &PolicyDecision::Allow); + fs::remove_file(&target).unwrap(); + symlink(".env", &target).unwrap(); + // Exercise the read itself after the policy check, without a second policy evaluation. + let result = if name == "readFile" { + tools.read_file(&prepared.call().arguments).await + } else { + tools.generate_diff(&prepared.call().arguments).await + }; + assert!(result.is_err(), "{name} followed a replacement symlink"); + fs::remove_file(&target).unwrap(); + } +} diff --git a/src/tests/tools.rs b/src/tests/tools.rs index b5952ce2..aca54d08 100644 --- a/src/tests/tools.rs +++ b/src/tests/tools.rs @@ -226,7 +226,7 @@ fn approval_summaries_expose_command_risk_and_mutation_size() { assert_eq!( tools.arg_summary("runCommand", &json!({"command": "cargo test"})), - "UNSANDBOXED shell command: cargo test" + "UNSANDBOXED shell command in .: cargo test" ); assert_eq!( tools.arg_summary( diff --git a/src/tools/mod.rs b/src/tools/mod.rs index 57386f19..f5d2c9b1 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -1,5 +1,10 @@ +mod policy; pub mod schema; +pub use policy::{ExecutionMode, PolicyDecision}; +#[cfg(test)] +use policy::{audit_tool_allowed, tool_requires_approval}; + use std::io::Read as _; use std::io::{Error as IoError, ErrorKind}; use std::net::{IpAddr, SocketAddr, ToSocketAddrs}; @@ -66,13 +71,6 @@ pub fn tool_can_interrupt(name: &str) -> bool { ) } -pub fn tool_requires_approval(name: &str) -> bool { - matches!( - canonical_tool_name(name), - "runCommand" | "auditDependencies" | "writeFile" | "editFile" | "multiEdit" | "applyPatch" - ) -} - fn is_sensitive_path(path: &Path) -> bool { let components = path .components() @@ -146,25 +144,6 @@ fn sensitive_data_filename(name: &str) -> bool { }) || STEMS.iter().any(|stem| name == format!("{stem}.yml")) } -pub fn audit_tool_allowed(name: &str, allow_mutations: bool) -> bool { - matches!( - canonical_tool_name(name), - "listSkills" - | "searchSkills" - | "readSkill" - | "readFile" - | "listDirectory" - | "searchFiles" - | "grep" - | "fileInfo" - | "auditDependencies" - ) || allow_mutations - && matches!( - canonical_tool_name(name), - "generateDiff" | "writeFile" | "editFile" | "multiEdit" | "applyPatch" - ) -} - #[derive(Clone)] pub struct ToolRuntime { workspace_root: PathBuf, @@ -227,17 +206,9 @@ impl ToolRuntime { self.skills.prompt_section().await } - pub fn call_requires_approval(&self, call: &ToolCall) -> bool { - if tool_requires_approval(&call.name) { - return true; - } - - canonical_tool_name(&call.name) == "readFile" - && path_arg(&call.arguments).is_some_and(|path| self.read_path_is_sensitive(path)) - } - fn read_path_is_sensitive(&self, path: &str) -> bool { - is_sensitive_path(Path::new(path)) + let input = Path::new(path); + is_sensitive_path(input.strip_prefix(&self.workspace_root).unwrap_or(input)) || self .resolve_in_workspace(path) .is_ok_and(|resolved| self.is_sensitive_workspace_path(&resolved)) @@ -252,7 +223,8 @@ impl ToolRuntime { let canonical = canonical_tool_name(name); let detailed = match canonical { "runCommand" => Some(format!( - "UNSANDBOXED shell command: {}", + "UNSANDBOXED shell command in {}: {}", + args.get("cwd").and_then(Value::as_str).unwrap_or("."), args.get("command") .and_then(Value::as_str) .unwrap_or_default() @@ -298,7 +270,9 @@ impl ToolRuntime { .and_then(Value::as_str) .unwrap_or_default() )), - "readFile" if path_arg(args).is_some_and(|path| self.read_path_is_sensitive(path)) => { + "readFile" | "generateDiff" + if path_arg(args).is_some_and(|path| self.read_path_is_sensitive(path)) => + { Some(format!( "SENSITIVE file read: {}", path_arg(args).unwrap_or_default() @@ -355,16 +329,25 @@ impl ToolRuntime { .collect() } - pub async fn execute(&self, call: &ToolCall) -> Result { + // Handler tests grant approval explicitly. Production uses execute_prepared. + #[cfg(test)] + async fn execute(&self, call: &ToolCall) -> Result { self.execute_with_cancellation(call, &TurnCancellation::default()) .await } - pub async fn execute_with_cancellation( + #[cfg(test)] + async fn execute_with_cancellation( &self, call: &ToolCall, cancellation: &TurnCancellation, ) -> Result { + let mut prepared = self.prepare_call(call, ExecutionMode::Workspace); + prepared.approve(); + self.execute_prepared(prepared, cancellation).await + } + + async fn dispatch(&self, call: &ToolCall, cancellation: &TurnCancellation) -> Result { if cancellation.is_cancelled() { return Err(anyhow!("Tool cancelled before execution")); } @@ -390,94 +373,23 @@ impl ToolRuntime { } } - pub async fn execute_audit( + #[cfg(test)] + async fn execute_audit( &self, call: &ToolCall, scope: &Path, allow_mutations: bool, ) -> Result { - if !audit_tool_allowed(&call.name, allow_mutations) { - return Err(anyhow!( - "Tool '{}' is unavailable in this audit mode", - call.name - )); - } - - let mut scoped_call = call.clone(); - let name = canonical_tool_name(&scoped_call.name); - if matches!( - name, - "listDirectory" | "searchFiles" | "grep" | "auditDependencies" - ) && tool_path_arg(name, &scoped_call.arguments).is_none() - { - scoped_call.arguments["path"] = json!(scope.display().to_string()); - } - if matches!(name, "searchFiles" | "grep") { - scoped_call.arguments["excludeGenerated"] = json!(true); - } - if name == "readFile" { - scoped_call.arguments["lineNumbers"] = json!(true); - } - - let resolved = match name { - "readFile" | "listDirectory" | "searchFiles" | "grep" | "fileInfo" - | "auditDependencies" | "generateDiff" | "editFile" | "multiEdit" => { - let path = tool_path_arg(name, &scoped_call.arguments).unwrap_or("."); - Some(self.resolve_in_workspace(path)?) - } - "writeFile" => { - let path = required_path(&scoped_call.arguments)?; - Some(self.resolve_existing_or_missing_in_workspace(path)?) - } - "applyPatch" => { - self.ensure_patch_in_scope(&scoped_call.arguments, scope)?; - None - } - _ => None, - }; - if let Some(resolved) = resolved - && !resolved.starts_with(scope) - { - return Err(anyhow!( - "Path '{}' is outside audit scope '{}'", - resolved.display(), - scope.display() - )); - } - - self.execute(&scoped_call).await - } - - fn ensure_patch_in_scope(&self, args: &Value, scope: &Path) -> Result<()> { - let patch_text = required_any_str(args, &["patchText", "patch_text", "patch"])?; - for op in parse_apply_patch(patch_text)? { - let mut paths = match op { - PatchOp::Add { path, .. } => vec![(path, false)], - PatchOp::Delete { path } => vec![(path, true)], - PatchOp::Update { path, move_to, .. } => { - let mut paths = vec![(path, true)]; - if let Some(move_to) = move_to { - paths.push((move_to, false)); - } - paths - } - }; - for (path, must_exist) in paths.drain(..) { - let resolved = if must_exist { - self.resolve_in_workspace(&path)? - } else { - self.resolve_existing_or_missing_in_workspace(&path)? - }; - if !resolved.starts_with(scope) { - return Err(anyhow!( - "Path '{}' is outside audit scope '{}'", - resolved.display(), - scope.display() - )); - } - } - } - Ok(()) + let mut prepared = self.prepare_call( + call, + ExecutionMode::Audit { + scope, + allow_mutations, + }, + ); + prepared.approve(); + self.execute_prepared(prepared, &TurnCancellation::default()) + .await } pub fn resolve_audit_scope(&self, target: &str) -> Result<(PathBuf, String)> { @@ -526,8 +438,9 @@ impl ToolRuntime { .map(|v| v as usize) .unwrap_or(0); - let resolved = self.resolve_in_workspace(path)?; - let content = read_text_file(&resolved).await?; + let relative = self.workspace_relative_path(path)?; + let resolved = self.workspace_root.join(&relative); + let content = self.read_workspace_text(&relative).await?; let total_lines = content.lines().count(); let selected = content @@ -727,7 +640,8 @@ impl ToolRuntime { } } - let content = match read_text_file(path).await { + let relative = path.strip_prefix(&self.workspace_root)?; + let content = match self.read_workspace_text(relative).await { Ok(v) => v, Err(_) => { skipped_files += 1; @@ -783,7 +697,9 @@ impl ToolRuntime { let mut packages = Vec::::new(); for lockfile in &lockfiles { - let content = read_text_file(lockfile).await?; + let content = self + .read_workspace_text(lockfile.strip_prefix(&self.workspace_root)?) + .await?; for (name, version) in parse_cargo_lock(&content) { packages.push(CargoPackage { lockfile: lockfile.display().to_string(), @@ -1042,15 +958,19 @@ impl ToolRuntime { async fn generate_diff(&self, args: &Value) -> Result { let path = required_path(args)?; let proposed = required_any_string(args, &["content"])?; - let resolved = self.resolve_in_workspace(path)?; - let original = read_text_file(&resolved).await?; + let relative = self.workspace_relative_path(path)?; + let resolved = self.workspace_root.join(&relative); + let original = self.read_workspace_text(&relative).await?; if original.lines().count() > MAX_DIFF_LINES || proposed.lines().count() > MAX_DIFF_LINES { return Err(anyhow!( "Diff input exceeds the {}-line safety limit; split the edit into smaller files", MAX_DIFF_LINES )); } - let diff = unified_diff(path, &original, proposed); + let diff_path = resolved + .strip_prefix(&self.workspace_root) + .unwrap_or(&resolved); + let diff = unified_diff(&diff_path.to_string_lossy(), &original, proposed); Ok(json!({ "path": resolved.display().to_string(), @@ -1865,16 +1785,6 @@ fn path_arg(args: &Value) -> Option<&str> { .and_then(Value::as_str) } -fn tool_path_arg<'a>(name: &str, args: &'a Value) -> Option<&'a str> { - if name == "searchFiles" { - args.get("cwd") - .and_then(Value::as_str) - .or_else(|| path_arg(args)) - } else { - path_arg(args) - } -} - fn required_any_str<'a>(args: &'a Value, keys: &[&str]) -> Result<&'a str> { for key in keys { if let Some(value) = args @@ -2802,23 +2712,6 @@ async fn read_capped(mut reader: impl AsyncRead + Unpin, limit: usize) -> Result Ok((output, truncated)) } -async fn read_text_file(path: &Path) -> Result { - let file = fs::File::open(path).await?; - let mut bytes = Vec::new(); - file.take(MAX_TEXT_FILE_BYTES as u64 + 1) - .read_to_end(&mut bytes) - .await?; - if bytes.len() > MAX_TEXT_FILE_BYTES { - return Err(anyhow!( - "Text file '{}' exceeds the {}-byte safety limit", - path.display(), - MAX_TEXT_FILE_BYTES - )); - } - String::from_utf8(bytes) - .map_err(|err| anyhow!("Text file '{}' is not valid UTF-8: {}", path.display(), err)) -} - fn is_public_ip(ip: IpAddr) -> bool { match ip { IpAddr::V4(v4) => { diff --git a/src/tools/policy.rs b/src/tools/policy.rs new file mode 100644 index 00000000..1505873a --- /dev/null +++ b/src/tools/policy.rs @@ -0,0 +1,383 @@ +use std::path::{Path, PathBuf}; + +use anyhow::{Result, anyhow, bail, ensure}; +use serde_json::{Value, json}; + +use super::{ + PatchOp, ToolRuntime, canonical_tool_name, parse_apply_patch, path_arg, required_str, schema, + validate_public_url, +}; +use crate::models::{ToolCall, TurnCancellation}; + +pub(super) fn tool_requires_approval(name: &str) -> bool { + matches!( + canonical_tool_name(name), + "runCommand" | "auditDependencies" | "writeFile" | "editFile" | "multiEdit" | "applyPatch" + ) +} + +pub(super) fn audit_tool_allowed(name: &str, allow_mutations: bool) -> bool { + matches!( + canonical_tool_name(name), + "listSkills" + | "searchSkills" + | "readSkill" + | "readFile" + | "listDirectory" + | "searchFiles" + | "grep" + | "fileInfo" + | "auditDependencies" + ) || allow_mutations + && matches!( + canonical_tool_name(name), + "generateDiff" | "writeFile" | "editFile" | "multiEdit" | "applyPatch" + ) +} + +#[derive(Debug, Clone, Copy)] +pub enum ExecutionMode<'a> { + Workspace, + Audit { + scope: &'a Path, + allow_mutations: bool, + }, +} + +#[derive(Debug, PartialEq, Eq)] +pub enum PolicyDecision { + Allow, + RequireApproval(&'static str), + Deny(String), +} + +pub struct PreparedCall<'a> { + requested: ToolCall, + call: ToolCall, + targets: Vec, + mode: ExecutionMode<'a>, + decision: PolicyDecision, + approved: bool, +} + +impl PreparedCall<'_> { + pub fn decision(&self) -> &PolicyDecision { + &self.decision + } + pub fn call(&self) -> &ToolCall { + &self.call + } + + pub fn approve(&mut self) { + if matches!(self.decision, PolicyDecision::RequireApproval(_)) { + self.approved = true; + } + } +} + +impl ToolRuntime { + pub(super) fn call_requires_approval(&self, call: &ToolCall) -> bool { + if tool_requires_approval(&call.name) { + return true; + } + + matches!(canonical_tool_name(&call.name), "readFile" | "generateDiff") + && path_arg(&call.arguments).is_some_and(|path| self.read_path_is_sensitive(path)) + } + + pub fn prepare_call<'a>( + &self, + requested: &ToolCall, + mode: ExecutionMode<'a>, + ) -> PreparedCall<'a> { + let mut prepared = PreparedCall { + requested: requested.clone(), + call: requested.clone(), + targets: Vec::new(), + mode, + decision: PolicyDecision::Allow, + approved: false, + }; + match self.resolve_policy_call(requested, mode) { + Ok((call, targets)) => { + prepared.decision = if self.call_requires_approval(&call) + || self.call_requires_approval(requested) + { + PolicyDecision::RequireApproval(match call.name.as_str() { + "runCommand" => "Shell commands run without an OS sandbox", + "auditDependencies" => { + "Dependency checks send public package versions to OSV.dev" + } + "readFile" | "generateDiff" => "Sensitive file reads require approval", + _ => "File changes require approval", + }) + } else { + PolicyDecision::Allow + }; + prepared.call = call; + prepared.targets = targets; + } + Err(error) => prepared.decision = PolicyDecision::Deny(error.to_string()), + } + prepared + } + + pub async fn execute_prepared( + &self, + prepared: PreparedCall<'_>, + cancellation: &TurnCancellation, + ) -> Result { + ensure!( + !cancellation.is_cancelled(), + "Tool cancelled before execution" + ); + check_approval(&prepared.decision, prepared.approved)?; + // Resolve again after approval. A grant only covers the reviewed arguments and targets. + let current = self.prepare_call(&prepared.requested, prepared.mode); + check_approval(¤t.decision, prepared.approved)?; + ensure!( + current.call == prepared.call && current.targets == prepared.targets, + "Tool targets changed after policy review; submit the call again" + ); + self.dispatch(¤t.call, cancellation).await + } + + fn resolve_policy_call( + &self, + requested: &ToolCall, + mode: ExecutionMode<'_>, + ) -> Result<(ToolCall, Vec)> { + let mut call = normalize_call(requested)?; + let scope = match mode { + ExecutionMode::Workspace => self.workspace_root.clone(), + ExecutionMode::Audit { + scope, + allow_mutations, + } => { + ensure!( + audit_tool_allowed(&call.name, allow_mutations), + "unavailable in this audit mode" + ); + self.resolve_in_workspace(path_text(scope)?)? + } + }; + if matches!(mode, ExecutionMode::Audit { .. }) { + if matches!( + call.name.as_str(), + "listDirectory" | "searchFiles" | "grep" | "auditDependencies" + ) && call.arguments.get("path").is_none() + { + call.arguments["path"] = json!(path_text(&scope)?); + } + if matches!(call.name.as_str(), "searchFiles" | "grep") { + call.arguments["excludeGenerated"] = json!(true); + } + if call.name == "readFile" { + call.arguments["lineNumbers"] = json!(true); + } + } + schema::validate_call(&call)?; + if call.name == "runCommand" { + required_str(&call.arguments, "command")?; + } + let mut targets = vec![self.workspace_root.clone(), scope.clone()]; + let path_key = match call.name.as_str() { + "readFile" | "listDirectory" | "searchFiles" | "grep" | "fileInfo" + | "auditDependencies" | "generateDiff" | "writeFile" | "editFile" | "multiEdit" => { + Some("path") + } + "runCommand" => Some("cwd"), + _ => None, + }; + if let Some(key) = path_key { + let input = call + .arguments + .get(key) + .and_then(Value::as_str) + .unwrap_or("."); + if matches!( + call.name.as_str(), + "readFile" | "fileInfo" | "generateDiff" | "writeFile" | "editFile" | "multiEdit" + ) { + required_str(&call.arguments, "path")?; + } + let resolved = match call.name.as_str() { + "writeFile" => self.policy_mutation_target(input, false)?, + "editFile" | "multiEdit" => self.policy_mutation_target(input, true)?, + "fileInfo" => self.resolve_existing_or_missing_in_workspace(input)?, + _ => self.resolve_in_workspace(input)?, + }; + ensure!( + resolved.starts_with(&scope), + "Path '{}' is outside audit scope '{}'", + resolved.display(), + scope.display() + ); + if matches!( + call.name.as_str(), + "readFile" | "generateDiff" | "editFile" | "multiEdit" + ) { + ensure!( + resolved.is_file(), + "Path '{}' is not a regular file", + resolved.display() + ); + } + if matches!( + call.name.as_str(), + "listDirectory" | "searchFiles" | "runCommand" + ) { + ensure!( + resolved.is_dir(), + "Path '{}' is not a directory", + resolved.display() + ); + } + if matches!(call.name.as_str(), "listDirectory" | "searchFiles" | "grep") { + ensure!( + !self.is_sensitive_workspace_path(&resolved), + "Sensitive paths cannot be searched or listed" + ); + } + call.arguments[key] = json!(path_text(&resolved)?); + targets.push(resolved); + } + if call.name == "applyPatch" { + for operation in parse_apply_patch(required_str(&call.arguments, "patchText")?)? { + let paths = match operation { + PatchOp::Add { path, .. } => vec![(path, false)], + PatchOp::Delete { path } => vec![(path, true)], + PatchOp::Update { path, move_to, .. } => { + let mut paths = vec![(path, true)]; + if let Some(target) = move_to { + paths.push((target, false)); + } + paths + } + }; + for (path, must_exist) in paths { + let target = self.policy_mutation_target(&path, must_exist)?; + ensure!( + target.starts_with(&scope), + "Path '{}' is outside audit scope '{}'", + target.display(), + scope.display() + ); + targets.push(target); + } + } + } + if call.name == "webFetch" { + let url = reqwest::Url::parse(required_str(&call.arguments, "url")?)?; + ensure!( + matches!(url.scheme(), "http" | "https"), + "Unsupported URL scheme '{}'", + url.scheme() + ); + validate_public_url(&url).map_err(|error| anyhow!(error))?; + call.arguments["url"] = json!(url.as_str()); + } + Ok((call, targets)) + } + + fn policy_mutation_target(&self, input: &str, must_exist: bool) -> Result { + let relative = self.workspace_relative_path(input)?; + let mut path = self.workspace_root.clone(); + for part in relative.components() { + path.push(part); + match std::fs::symlink_metadata(&path) { + Ok(metadata) => ensure!( + !metadata.file_type().is_symlink(), + "Symlink paths are not allowed for file changes: '{}'", + path.display() + ), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => break, + Err(error) => return Err(error.into()), + } + } + if must_exist { + self.resolve_in_workspace(input) + } else { + self.resolve_existing_or_missing_in_workspace(input) + } + } +} + +fn check_approval(decision: &PolicyDecision, approved: bool) -> Result<()> { + match decision { + PolicyDecision::Deny(reason) => bail!("{reason}"), + PolicyDecision::RequireApproval(_) if !approved => bail!("Tool requires user approval"), + _ => Ok(()), + } +} + +fn path_text(path: &Path) -> Result<&str> { + path.to_str() + .ok_or_else(|| anyhow!("Tool paths must be valid UTF-8")) +} + +fn normalize_call(requested: &ToolCall) -> Result { + let mut call = requested.clone(); + call.name = canonical_tool_name(&call.name).to_string(); + ensure!( + call.arguments.is_object(), + "Tool arguments must be a JSON object" + ); + let arguments = &mut call.arguments; + for (canonical, aliases) in [ + ("path", &["file_path"][..]), + ("oldString", &["old_string"][..]), + ("newString", &["new_string"][..]), + ("replaceAll", &["replace_all"][..]), + ("patchText", &["patch_text", "patch"][..]), + ] { + normalize_aliases(arguments, canonical, aliases)?; + } + if call.name == "readFile" { + normalize_aliases(arguments, "maxLines", &["limit"])?; + } + if call.name == "webFetch" { + normalize_aliases(arguments, "maxBytes", &["max_bytes"])?; + } + if call.name == "searchFiles" { + normalize_aliases(arguments, "path", &["cwd"])?; + } + if call.name == "multiEdit" + && let Some(edits) = arguments.get_mut("edits").and_then(Value::as_array_mut) + { + for edit in edits { + ensure!(edit.is_object(), "Each edit must be an object"); + for (canonical, alias) in [ + ("oldString", "old_string"), + ("newString", "new_string"), + ("replaceAll", "replace_all"), + ] { + normalize_aliases(edit, canonical, &[alias])?; + } + } + } + Ok(call) +} + +fn normalize_aliases(arguments: &mut Value, canonical: &str, aliases: &[&str]) -> Result<()> { + let object = arguments + .as_object_mut() + .ok_or_else(|| anyhow!("Tool arguments must be an object"))?; + for alias in aliases { + if let Some(value) = object.remove(*alias) { + if let Some(existing) = object.get(canonical) { + ensure!( + existing == &value, + "Conflicting arguments '{canonical}' and '{alias}'" + ); + } else { + object.insert(canonical.to_string(), value); + } + } + } + Ok(()) +} + +#[cfg(test)] +#[path = "../tests/policy.rs"] +mod tests; diff --git a/src/ui/mod.rs b/src/ui/mod.rs index a20091b9..20acd260 100644 --- a/src/ui/mod.rs +++ b/src/ui/mod.rs @@ -728,7 +728,7 @@ async fn handle_submit( state.push_message( UiRole::Assistant, format!( - "Commands:\n{help}\n\nKeys:\nEsc - Stop the active turn\nCtrl+C - Exit the app" + "Commands:\n{help}\n\nKeys:\nEsc - Stop the active turn\nCtrl+C - Exit the app\n\nTools: Invalid or out-of-scope requests are denied before approval." ), ); state.input.clear(); diff --git a/web/content/getting-started/commands.md b/web/content/getting-started/commands.md index 44ff94d3..dfd58874 100644 --- a/web/content/getting-started/commands.md +++ b/web/content/getting-started/commands.md @@ -49,6 +49,21 @@ Local tools available to the agent loop: Model-requested commands and file mutations pause until you approve them with `y` or deny that tool with `n` or `Enter`. `Esc` stops the entire turn. Shell commands are clearly labeled as unsandboxed; the workspace controls their starting directory, not their operating-system access. +Each tool call receives an allow, deny, or approval decision before execution. +The same policy applies to normal turns, audits, and the Ollama JSON fallback. +Invalid arguments, conflicting aliases, workspace escapes, and calls outside +the audit scope are denied before an approval prompt appears. Sensitive file +reads and diffs require approval. + +Approval covers the reviewed arguments and resolved targets. The runtime checks +them again before dispatch. A changed target requires a new call and review. +Patch checks include every source and destination. File changes reject symlink +paths. + +The policy checks a shell command's working directory. It does not restrict +paths or network targets inside the command text. Shell commands require +approval and do not run in an OS sandbox. + Claude Code, Codex, and OpenCode-compatible tool aliases are provided for common read/write/edit/shell/search operations. Fenced `diff` blocks in assistant responses are rendered as highlighted diffs. From 595e33848ee0ce7ca38bcd9d26b939c927714431 Mon Sep 17 00:00:00 2001 From: Gaya KACI <163902005+kacigaya@users.noreply.github.com> Date: Thu, 5 Nov 2026 19:26:57 +0000 Subject: [PATCH 2/2] test(tools): use canonical audit fixture paths Use one canonical workspace path for runtime and audit requests. This tests audit scope rejection on macOS and Windows instead of an earlier workspace prefix rejection. --- src/tests/tools.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/tests/tools.rs b/src/tests/tools.rs index aca54d08..af599206 100644 --- a/src/tests/tools.rs +++ b/src/tests/tools.rs @@ -265,7 +265,8 @@ async fn audit_mode_rejects_tools_and_paths_outside_scope() { fs::write(scope.join("inside.rs"), "safe").expect("inside"); fs::write(workspace.join("outside.rs"), "outside").expect("outside"); - let tools = ToolRuntime::new(workspace).expect("runtime"); + let workspace = workspace.canonicalize().expect("canonical workspace"); + let tools = ToolRuntime::new(workspace.clone()).expect("runtime"); let scope = scope.canonicalize().expect("canonical scope"); let blocked_tool = ToolCall { name: "webFetch".to_string(), @@ -282,7 +283,7 @@ async fn audit_mode_rejects_tools_and_paths_outside_scope() { let outside_read = ToolCall { name: "readFile".to_string(), - arguments: json!({"path": root.path().join("workspace/outside.rs").display().to_string()}), + arguments: json!({"path": workspace.join("outside.rs").display().to_string()}), }; assert!( tools @@ -328,7 +329,7 @@ async fn audit_mode_rejects_tools_and_paths_outside_scope() { let outside_write = ToolCall { name: "writeFile".to_string(), arguments: json!({ - "path": root.path().join("workspace/not-scoped.rs").display().to_string(), + "path": workspace.join("not-scoped.rs").display().to_string(), "content": "blocked", }), }; @@ -355,6 +356,7 @@ async fn audit_mode_rejects_tools_and_paths_outside_scope() { .to_string() .contains("outside audit scope") ); + assert!(!workspace.join("not-scoped.rs").exists()); } #[test]