From d2e4c0a1fd8e5c7bc3304af977781bf4627b854b Mon Sep 17 00:00:00 2001 From: argenis de la rosa Date: Wed, 4 Mar 2026 21:36:07 -0500 Subject: [PATCH] fix(shell): add configurable redirect policy and strip mode --- src/config/schema.rs | 13 +- src/cron/scheduler.rs | 24 +++- src/security/mod.rs | 2 +- src/security/policy.rs | 268 ++++++++++++++++++++++++++++++++++++++++- src/tools/process.rs | 5 +- src/tools/shell.rs | 73 ++++++++++- 6 files changed, 370 insertions(+), 15 deletions(-) diff --git a/src/config/schema.rs b/src/config/schema.rs index 532fa6887..79bb13dff 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -1,6 +1,6 @@ use crate::config::traits::ChannelConfig; use crate::providers::{is_glm_alias, is_zai_alias}; -use crate::security::{AutonomyLevel, DomainMatcher}; +use crate::security::{AutonomyLevel, DomainMatcher, ShellRedirectPolicy}; use anyhow::{Context, Result}; use directories::UserDirs; use schemars::JsonSchema; @@ -2296,6 +2296,13 @@ pub struct AutonomyConfig { #[serde(default = "default_true")] pub block_high_risk_commands: bool, + /// Redirect handling mode for shell commands. + /// + /// - `block` (default): reject unquoted redirects. + /// - `strip`: normalize common stderr/null redirects before execution. + #[serde(default)] + pub shell_redirect_policy: ShellRedirectPolicy, + /// Additional environment variables allowed for shell tool subprocesses. /// /// These names are explicitly allowlisted and merged with the built-in safe @@ -2450,6 +2457,7 @@ impl Default for AutonomyConfig { max_cost_per_day_cents: 500, require_approval_for_medium_risk: true, block_high_risk_commands: true, + shell_redirect_policy: ShellRedirectPolicy::Block, shell_env_passthrough: vec![], auto_approve: default_auto_approve(), always_ask: default_always_ask(), @@ -6638,6 +6646,7 @@ mod tests { assert_eq!(a.max_cost_per_day_cents, 500); assert!(a.require_approval_for_medium_risk); assert!(a.block_high_risk_commands); + assert_eq!(a.shell_redirect_policy, ShellRedirectPolicy::Block); assert!(a.shell_env_passthrough.is_empty()); assert!(a.non_cli_excluded_tools.contains(&"shell".to_string())); assert!(a.non_cli_excluded_tools.contains(&"delegate".to_string())); @@ -6660,6 +6669,7 @@ always_ask = [] allowed_roots = [] "#; let parsed: AutonomyConfig = toml::from_str(raw).unwrap(); + assert_eq!(parsed.shell_redirect_policy, ShellRedirectPolicy::Block); assert!(parsed.non_cli_excluded_tools.contains(&"shell".to_string())); assert!(parsed .non_cli_excluded_tools @@ -6825,6 +6835,7 @@ default_temperature = 0.7 max_cost_per_day_cents: 1000, require_approval_for_medium_risk: false, block_high_risk_commands: true, + shell_redirect_policy: ShellRedirectPolicy::Strip, shell_env_passthrough: vec!["DATABASE_URL".into()], auto_approve: vec!["file_read".into()], always_ask: vec![], diff --git a/src/cron/scheduler.rs b/src/cron/scheduler.rs index 789c3e2b3..26d09875f 100644 --- a/src/cron/scheduler.rs +++ b/src/cron/scheduler.rs @@ -414,6 +414,8 @@ async fn run_job_command_with_timeout( job: &CronJob, timeout: Duration, ) -> (bool, String) { + let effective_command = security.apply_shell_redirect_policy(&job.command); + if !security.can_act() { return ( false, @@ -428,7 +430,7 @@ async fn run_job_command_with_timeout( ); } - if !security.is_command_allowed(&job.command) { + if !security.is_command_allowed(&effective_command) { return ( false, format!( @@ -438,7 +440,7 @@ async fn run_job_command_with_timeout( ); } - if let Some(path) = security.forbidden_path_argument(&job.command) { + if let Some(path) = security.forbidden_path_argument(&effective_command) { return ( false, format!("blocked by security policy: forbidden path argument: {path}"), @@ -454,7 +456,7 @@ async fn run_job_command_with_timeout( let child = match Command::new("sh") .arg("-lc") - .arg(&job.command) + .arg(&effective_command) .current_dir(&config.workspace_dir) .stdin(Stdio::null()) .stdout(Stdio::piped()) @@ -491,7 +493,7 @@ mod tests { use super::*; use crate::config::Config; use crate::cron::{self, DeliveryConfig}; - use crate::security::SecurityPolicy; + use crate::security::{SecurityPolicy, ShellRedirectPolicy}; use chrono::{Duration as ChronoDuration, Utc}; use std::sync::OnceLock; use tempfile::TempDir; @@ -694,6 +696,20 @@ mod tests { assert!(output.contains("command not allowed")); } + #[tokio::test] + async fn run_job_command_strip_policy_normalizes_common_stderr_redirects() { + let tmp = TempDir::new().unwrap(); + let mut config = test_config(&tmp).await; + config.autonomy.allowed_commands = vec!["echo".into()]; + config.autonomy.shell_redirect_policy = ShellRedirectPolicy::Strip; + let job = test_job("echo scheduler-strip 2>&1"); + let security = SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir); + + let (success, output) = run_job_command(&config, &security, &job).await; + assert!(success); + assert!(output.contains("scheduler-strip")); + } + #[tokio::test] async fn run_job_command_blocks_readonly_mode() { let tmp = TempDir::new().unwrap(); diff --git a/src/security/mod.rs b/src/security/mod.rs index c7318926b..a5bebf9af 100644 --- a/src/security/mod.rs +++ b/src/security/mod.rs @@ -51,7 +51,7 @@ pub use estop::{EstopLevel, EstopManager, EstopState, ResumeSelector}; pub use otp::OtpValidator; #[allow(unused_imports)] pub use pairing::PairingGuard; -pub use policy::{AutonomyLevel, SecurityPolicy}; +pub use policy::{AutonomyLevel, SecurityPolicy, ShellRedirectPolicy}; #[allow(unused_imports)] pub use secrets::SecretStore; #[allow(unused_imports)] diff --git a/src/security/policy.rs b/src/security/policy.rs index 3c4c40a66..0249d754b 100644 --- a/src/security/policy.rs +++ b/src/security/policy.rs @@ -32,6 +32,25 @@ impl std::str::FromStr for AutonomyLevel { } } +/// Policy for handling shell redirect operators. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(rename_all = "snake_case")] +pub enum ShellRedirectPolicy { + /// Block redirect operators (`<`, `>`, `>>`, etc.) in unquoted shell input. + #[default] + Block, + /// Strip common LLM-generated stderr/null redirects before validation/execution. + /// + /// Supported normalization: + /// - `2>&1`, `1>&2`, `>&1` + /// - `2>/dev/null`, `2>>/dev/null`, `>/dev/null`, `>>/dev/null` + /// - `&>/dev/null`, `>&/dev/null` + /// - `|&` -> `|` + /// + /// Other redirect forms remain blocked by command policy. + Strip, +} + /// Risk score for shell command execution. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CommandRiskLevel { @@ -105,6 +124,7 @@ pub struct SecurityPolicy { pub max_cost_per_day_cents: u32, pub require_approval_for_medium_risk: bool, pub block_high_risk_commands: bool, + pub shell_redirect_policy: ShellRedirectPolicy, pub shell_env_passthrough: Vec, pub tracker: ActionTracker, } @@ -157,6 +177,7 @@ impl Default for SecurityPolicy { max_cost_per_day_cents: 500, require_approval_for_medium_risk: true, block_high_risk_commands: true, + shell_redirect_policy: ShellRedirectPolicy::Block, shell_env_passthrough: vec![], tracker: ActionTracker::new(), } @@ -419,6 +440,179 @@ fn contains_unquoted_char(command: &str, target: char) -> bool { false } +fn is_token_boundary_char(ch: char) -> bool { + ch.is_whitespace() || matches!(ch, ';' | '\n' | '|' | '&' | ')' | '(') +} + +fn starts_with_literal(chars: &[char], start: usize, literal: &str) -> bool { + let literal_chars: Vec = literal.chars().collect(); + chars + .get(start..start + literal_chars.len()) + .is_some_and(|slice| slice == literal_chars) +} + +fn consume_stream_merge_redirect(chars: &[char], start: usize) -> Option { + // Matches: + // - 2>&1 + // - 1>&2 + // - >&1 + let mut i = start; + while i < chars.len() && chars[i].is_ascii_digit() { + i += 1; + } + if i >= chars.len() || chars[i] != '>' { + return None; + } + i += 1; + while i < chars.len() && chars[i].is_whitespace() { + i += 1; + } + if i >= chars.len() || chars[i] != '&' { + return None; + } + i += 1; + while i < chars.len() && chars[i].is_whitespace() { + i += 1; + } + let fd_start = i; + while i < chars.len() && chars[i].is_ascii_digit() { + i += 1; + } + if i == fd_start { + return None; + } + Some(i - start) +} + +fn consume_dev_null_redirect(chars: &[char], start: usize) -> Option { + // Matches: + // - [fd]>/dev/null + // - [fd]>>/dev/null + // - [fd]< /dev/null + // - &>/dev/null + // - >&/dev/null + let mut i = start; + if chars[i] == '&' { + i += 1; + if i >= chars.len() || chars[i] != '>' { + return None; + } + i += 1; + } else { + while i < chars.len() && chars[i].is_ascii_digit() { + i += 1; + } + if i >= chars.len() || !matches!(chars[i], '>' | '<') { + return None; + } + let op = chars[i]; + i += 1; + if op == '>' && i < chars.len() && chars[i] == '>' { + i += 1; + } + if op == '>' && i < chars.len() && chars[i] == '&' { + i += 1; + } + } + + while i < chars.len() && chars[i].is_whitespace() { + i += 1; + } + if !starts_with_literal(chars, i, "/dev/null") { + return None; + } + i += "/dev/null".chars().count(); + if i < chars.len() && !is_token_boundary_char(chars[i]) { + return None; + } + Some(i - start) +} + +fn strip_supported_redirects(command: &str) -> String { + let chars: Vec = command.chars().collect(); + let mut out = String::with_capacity(command.len()); + let mut quote = QuoteState::None; + let mut escaped = false; + let mut i = 0usize; + + while i < chars.len() { + let ch = chars[i]; + match quote { + QuoteState::Single => { + if ch == '\'' { + quote = QuoteState::None; + } + out.push(ch); + i += 1; + } + QuoteState::Double => { + if escaped { + escaped = false; + out.push(ch); + i += 1; + continue; + } + if ch == '\\' { + escaped = true; + out.push(ch); + i += 1; + continue; + } + if ch == '"' { + quote = QuoteState::None; + } + out.push(ch); + i += 1; + } + QuoteState::None => { + if escaped { + escaped = false; + out.push(ch); + i += 1; + continue; + } + if ch == '\\' { + escaped = true; + out.push(ch); + i += 1; + continue; + } + if ch == '\'' { + quote = QuoteState::Single; + out.push(ch); + i += 1; + continue; + } + if ch == '"' { + quote = QuoteState::Double; + out.push(ch); + i += 1; + continue; + } + + // Normalize `|&` to `|` since shell tool already captures stderr. + if ch == '|' && chars.get(i + 1).is_some_and(|next| *next == '&') { + out.push('|'); + i += 2; + continue; + } + + if let Some(consumed) = consume_stream_merge_redirect(&chars, i) + .or_else(|| consume_dev_null_redirect(&chars, i)) + { + i += consumed; + continue; + } + + out.push(ch); + i += 1; + } + } + } + + out +} + /// Detect unquoted shell variable expansions like `$HOME`, `$1`, `$?`. /// /// Escaped dollars (`\$`) are ignored. Variables inside single quotes are @@ -561,6 +755,14 @@ fn is_allowlist_entry_match(allowed: &str, executable: &str, executable_base: &s } impl SecurityPolicy { + /// Apply configured redirect policy to a shell command before validation/execution. + pub fn apply_shell_redirect_policy(&self, command: &str) -> String { + match self.shell_redirect_policy { + ShellRedirectPolicy::Block => command.to_string(), + ShellRedirectPolicy::Strip => strip_supported_redirects(command), + } + } + // ── Risk Classification ────────────────────────────────────────────── // Risk is assessed per-segment (split on shell operators), and the // highest risk across all segments wins. This prevents bypasses like @@ -688,15 +890,17 @@ impl SecurityPolicy { command: &str, approved: bool, ) -> Result { - if !self.is_command_allowed(command) { + let effective_command = self.apply_shell_redirect_policy(command); + + if !self.is_command_allowed(&effective_command) { return Err(format!("Command not allowed by security policy: {command}")); } - if let Some(path) = self.forbidden_path_argument(command) { + if let Some(path) = self.forbidden_path_argument(&effective_command) { return Err(format!("Path blocked by security policy: {path}")); } - let risk = self.command_risk_level(command); + let risk = self.command_risk_level(&effective_command); if risk == CommandRiskLevel::High { if self.block_high_risk_commands { @@ -1088,6 +1292,7 @@ impl SecurityPolicy { max_cost_per_day_cents: autonomy_config.max_cost_per_day_cents, require_approval_for_medium_risk: autonomy_config.require_approval_for_medium_risk, block_high_risk_commands: autonomy_config.block_high_risk_commands, + shell_redirect_policy: autonomy_config.shell_redirect_policy, shell_env_passthrough: autonomy_config.shell_env_passthrough.clone(), tracker: ActionTracker::new(), } @@ -1451,6 +1656,7 @@ mod tests { max_cost_per_day_cents: 1000, require_approval_for_medium_risk: false, block_high_risk_commands: false, + shell_redirect_policy: ShellRedirectPolicy::Strip, shell_env_passthrough: vec!["DATABASE_URL".into()], ..crate::config::AutonomyConfig::default() }; @@ -1465,6 +1671,7 @@ mod tests { assert_eq!(policy.max_cost_per_day_cents, 1000); assert!(!policy.require_approval_for_medium_risk); assert!(!policy.block_high_risk_commands); + assert_eq!(policy.shell_redirect_policy, ShellRedirectPolicy::Strip); assert_eq!(policy.shell_env_passthrough, vec!["DATABASE_URL"]); assert_eq!(policy.workspace_dir, PathBuf::from("/tmp/test-workspace")); } @@ -1509,6 +1716,7 @@ mod tests { assert!(p.max_cost_per_day_cents > 0); assert!(p.require_approval_for_medium_risk); assert!(p.block_high_risk_commands); + assert_eq!(p.shell_redirect_policy, ShellRedirectPolicy::Block); assert!(p.shell_env_passthrough.is_empty()); } @@ -1701,6 +1909,60 @@ mod tests { assert!(!p.is_command_allowed("cat&1"); + assert!(!merged.contains("2>&1")); + assert!(merged.contains("echo hello")); + + let devnull = p.apply_shell_redirect_policy("echo hello 2>/dev/null"); + assert!(!devnull.contains("/dev/null")); + assert!(devnull.contains("echo hello")); + + let pipeline = p.apply_shell_redirect_policy("echo hello |& cat"); + assert!(!pipeline.contains("|&")); + assert!(pipeline.contains("| cat")); + + let quoted = p.apply_shell_redirect_policy("echo '2>&1' \"|&\" '2>/dev/null'"); + assert_eq!(quoted, "echo '2>&1' \"|&\" '2>/dev/null'"); + } + + #[test] + fn strip_policy_allows_normalized_stderr_redirects() { + let p = SecurityPolicy { + shell_redirect_policy: ShellRedirectPolicy::Strip, + allowed_commands: vec!["echo".into()], + ..default_policy() + }; + + assert!(p + .validate_command_execution("echo hello 2>&1", false) + .is_ok()); + assert!(p + .validate_command_execution("echo hello 2>/dev/null", false) + .is_ok()); + } + + #[test] + fn strip_policy_keeps_unsupported_redirects_blocked() { + let p = SecurityPolicy { + shell_redirect_policy: ShellRedirectPolicy::Strip, + ..default_policy() + }; + + assert!(p + .validate_command_execution("echo hello > out.txt", false) + .is_err()); + assert!(p + .validate_command_execution("cat cmd, Err(e) => { diff --git a/src/tools/shell.rs b/src/tools/shell.rs index 91338f292..b16fac274 100644 --- a/src/tools/shell.rs +++ b/src/tools/shell.rs @@ -138,6 +138,7 @@ impl Tool for ShellTool { async fn execute(&self, args: serde_json::Value) -> anyhow::Result { let command = extract_command_argument(&args) .ok_or_else(|| anyhow::anyhow!("Missing 'command' parameter"))?; + let effective_command = self.security.apply_shell_redirect_policy(&command); let approved = args .get("approved") .and_then(|v| v.as_bool()) @@ -162,7 +163,7 @@ impl Tool for ShellTool { } } - if let Some(path) = self.security.forbidden_path_argument(&command) { + if let Some(path) = self.security.forbidden_path_argument(&effective_command) { return Ok(ToolResult { success: false, output: String::new(), @@ -183,7 +184,7 @@ impl Tool for ShellTool { // (CWE-200), then re-add only safe, functional variables. let mut cmd = match self .runtime - .build_shell_command(&command, &self.security.workspace_dir) + .build_shell_command(&effective_command, &self.security.workspace_dir) { Ok(cmd) => cmd, Err(e) => { @@ -228,7 +229,7 @@ impl Tool for ShellTool { if let Some(detector) = &self.syscall_detector { let _ = detector.inspect_command_output( - &command, + &effective_command, &stdout, &stderr, output.status.code(), @@ -266,7 +267,9 @@ mod tests { use super::*; use crate::config::{AuditConfig, SyscallAnomalyConfig}; use crate::runtime::{NativeRuntime, RuntimeAdapter}; - use crate::security::{AutonomyLevel, SecurityPolicy, SyscallAnomalyDetector}; + use crate::security::{ + AutonomyLevel, SecurityPolicy, ShellRedirectPolicy, SyscallAnomalyDetector, + }; use tempfile::TempDir; fn test_security(autonomy: AutonomyLevel) -> Arc { @@ -277,6 +280,18 @@ mod tests { }) } + fn test_security_with_redirect_policy( + autonomy: AutonomyLevel, + shell_redirect_policy: ShellRedirectPolicy, + ) -> Arc { + Arc::new(SecurityPolicy { + autonomy, + workspace_dir: std::env::temp_dir(), + shell_redirect_policy, + ..SecurityPolicy::default() + }) + } + fn test_runtime() -> Arc { Arc::new(NativeRuntime::new()) } @@ -487,6 +502,56 @@ mod tests { .contains("not allowed")); } + #[tokio::test] + async fn shell_strip_policy_allows_common_stderr_redirects() { + let tool = ShellTool::new( + test_security_with_redirect_policy( + AutonomyLevel::Supervised, + ShellRedirectPolicy::Strip, + ), + test_runtime(), + ); + + let merged = tool + .execute(json!({"command": "echo redirect-ok 2>&1"})) + .await + .expect("2>&1 should be normalized under strip policy"); + assert!(merged.success); + assert!(merged.output.contains("redirect-ok")); + + let devnull = tool + .execute(json!({"command": "ls definitely_missing_shell_redirect 2>/dev/null"})) + .await + .expect("2>/dev/null should be normalized under strip policy"); + assert!(!devnull.success); + assert!(devnull + .error + .as_deref() + .unwrap_or("") + .contains("definitely_missing_shell_redirect")); + } + + #[tokio::test] + async fn shell_strip_policy_still_blocks_unsupported_redirects() { + let tool = ShellTool::new( + test_security_with_redirect_policy( + AutonomyLevel::Supervised, + ShellRedirectPolicy::Strip, + ), + test_runtime(), + ); + let result = tool + .execute(json!({"command": "echo blocked > out.txt"})) + .await + .expect("unsupported redirect should still be blocked"); + assert!(!result.success); + assert!(result + .error + .as_deref() + .unwrap_or("") + .contains("not allowed")); + } + fn test_security_with_env_cmd() -> Arc { Arc::new(SecurityPolicy { autonomy: AutonomyLevel::Supervised,