From cc752e56f3110cd979905de4e87ca3f3f9ce738f Mon Sep 17 00:00:00 2001 From: Hayato Kawai Date: Wed, 18 Feb 2026 23:56:24 +0900 Subject: [PATCH 1/2] rules: add compound command evaluation with sandbox policy aggregation evaluate_command() only handled single commands and wrapper recursion. Compound commands (pipelines, &&, ||, ;) need each sub-command evaluated independently with results aggregated to enforce security policies across the full command line. Add evaluate_compound() that splits compound commands via extract_commands(), evaluates each with evaluate_command(), and aggregates results: - Action: Explicit Deny Wins (deny > ask > allow > default) - Sandbox policy: Strictest Wins via SandboxPreset::merge_strictest() (writable roots intersection, deny paths union, network intersection) - Writable roots contradiction (empty intersection) escalates to Ask Co-Authored-By: Claude Opus 4.6 --- src/config/model.rs | 232 +++++++++++++ src/rules/rule_engine.rs | 706 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 934 insertions(+), 4 deletions(-) diff --git a/src/config/model.rs b/src/config/model.rs index 8ebbb1d..618e6fd 100644 --- a/src/config/model.rs +++ b/src/config/model.rs @@ -64,6 +64,95 @@ pub struct SandboxPreset { pub network: Option, } +/// Merged sandbox policy produced by aggregating multiple `SandboxPreset`s. +/// +/// Unlike `SandboxPreset` which uses `Option` fields (unset = inherit from +/// defaults), `MergedSandboxPolicy` has concrete resolved values ready for +/// enforcement. +#[derive(Debug, Clone, PartialEq)] +pub struct MergedSandboxPolicy { + pub writable: Vec, + pub deny: Vec, + pub network_allow: Option>, +} + +impl SandboxPreset { + /// Merge multiple sandbox presets using Strictest Wins policy. + /// + /// - `writable` (writable roots): intersection across all presets + /// - `deny` (read-only subpaths): union across all presets + /// - `network.allow`: intersection; if any preset has no `network.allow` + /// (meaning no network access), the result has no network access + /// + /// Returns `None` if the input slice is empty. + pub fn merge_strictest(presets: &[&SandboxPreset]) -> Option { + if presets.is_empty() { + return None; + } + + let mut writable: Option> = None; + let mut deny: HashSet = HashSet::new(); + let mut network_allow: Option> = None; + let mut any_network_restricted = false; + + for preset in presets { + // writable: intersection + if let Some(fs) = &preset.fs { + if let Some(w) = &fs.writable { + let w_set: HashSet = w.iter().cloned().collect(); + writable = Some(match writable { + Some(existing) => existing.intersection(&w_set).cloned().collect(), + None => w_set, + }); + } + + // deny: union + if let Some(d) = &fs.deny { + deny.extend(d.iter().cloned()); + } + } + + // network.allow: intersection (if any preset restricts network, restrict all) + if let Some(net) = &preset.network { + match &net.allow { + Some(allowed) => { + let a_set: HashSet = allowed.iter().cloned().collect(); + network_allow = Some(match network_allow { + Some(existing) => existing.intersection(&a_set).cloned().collect(), + None => a_set, + }); + } + None => { + any_network_restricted = true; + } + } + } + } + + let final_network = if any_network_restricted { + Some(vec![]) + } else { + network_allow.map(|s| { + let mut v: Vec = s.into_iter().collect(); + v.sort(); + v + }) + }; + + let mut writable_vec: Vec = writable.unwrap_or_default().into_iter().collect(); + writable_vec.sort(); + + let mut deny_vec: Vec = deny.into_iter().collect(); + deny_vec.sort(); + + Some(MergedSandboxPolicy { + writable: writable_vec, + deny: deny_vec, + network_allow: final_network, + }) + } +} + #[derive(Debug, Deserialize, Clone, PartialEq)] pub struct FsPolicy { pub writable: Option>, @@ -1214,4 +1303,147 @@ mod tests { .trim_start(); assert_eq!(err.to_string(), expected); } + + // === SandboxPreset::merge_strictest === + + #[test] + fn merge_strictest_empty_returns_none() { + assert_eq!(SandboxPreset::merge_strictest(&[]), None); + } + + #[test] + fn merge_strictest_single_preset() { + let preset = SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string(), "/home".to_string()]), + deny: Some(vec!["/etc".to_string()]), + }), + network: Some(NetworkPolicy { + allow: Some(vec!["github.com".to_string()]), + }), + }; + let result = SandboxPreset::merge_strictest(&[&preset]).unwrap(); + assert_eq!(result.writable, vec!["/home", "/tmp"]); + assert_eq!(result.deny, vec!["/etc"]); + assert_eq!(result.network_allow, Some(vec!["github.com".to_string()])); + } + + #[test] + fn merge_strictest_writable_intersection() { + let a = SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec![ + "/tmp".to_string(), + "/home".to_string(), + "/var".to_string(), + ]), + deny: None, + }), + network: None, + }; + let b = SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string(), "/var".to_string()]), + deny: None, + }), + network: None, + }; + let result = SandboxPreset::merge_strictest(&[&a, &b]).unwrap(); + assert_eq!(result.writable, vec!["/tmp", "/var"]); + } + + #[test] + fn merge_strictest_deny_union() { + let a = SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: Some(vec!["/etc/passwd".to_string()]), + }), + network: None, + }; + let b = SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: Some(vec!["/etc/shadow".to_string()]), + }), + network: None, + }; + let result = SandboxPreset::merge_strictest(&[&a, &b]).unwrap(); + assert_eq!(result.deny, vec!["/etc/passwd", "/etc/shadow"]); + } + + #[test] + fn merge_strictest_network_intersection() { + let a = SandboxPreset { + fs: None, + network: Some(NetworkPolicy { + allow: Some(vec!["github.com".to_string(), "pypi.org".to_string()]), + }), + }; + let b = SandboxPreset { + fs: None, + network: Some(NetworkPolicy { + allow: Some(vec!["github.com".to_string(), "npmjs.org".to_string()]), + }), + }; + let result = SandboxPreset::merge_strictest(&[&a, &b]).unwrap(); + assert_eq!(result.network_allow, Some(vec!["github.com".to_string()])); + } + + #[test] + fn merge_strictest_network_none_means_restricted() { + let a = SandboxPreset { + fs: None, + network: Some(NetworkPolicy { + allow: Some(vec!["github.com".to_string()]), + }), + }; + let b = SandboxPreset { + fs: None, + network: Some(NetworkPolicy { allow: None }), + }; + let result = SandboxPreset::merge_strictest(&[&a, &b]).unwrap(); + assert_eq!(result.network_allow, Some(vec![])); + } + + #[test] + fn merge_strictest_writable_empty_intersection() { + let a = SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: None, + }), + network: None, + }; + let b = SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/home".to_string()]), + deny: None, + }), + network: None, + }; + let result = SandboxPreset::merge_strictest(&[&a, &b]).unwrap(); + assert!(result.writable.is_empty()); + } + + #[test] + fn merge_strictest_no_fs_preserves_other() { + let a = SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: Some(vec!["/etc".to_string()]), + }), + network: None, + }; + let b = SandboxPreset { + fs: None, + network: Some(NetworkPolicy { + allow: Some(vec!["github.com".to_string()]), + }), + }; + let result = SandboxPreset::merge_strictest(&[&a, &b]).unwrap(); + assert_eq!(result.writable, vec!["/tmp"]); + assert_eq!(result.deny, vec!["/etc"]); + assert_eq!(result.network_allow, Some(vec!["github.com".to_string()])); + } } diff --git a/src/rules/rule_engine.rs b/src/rules/rule_engine.rs index 9d3a039..9304918 100644 --- a/src/rules/rule_engine.rs +++ b/src/rules/rule_engine.rs @@ -1,7 +1,9 @@ use std::collections::{HashMap, HashSet}; use std::path::PathBuf; -use crate::config::{ActionKind, Config, Definitions, RuleEntry}; +use crate::config::{ + ActionKind, Config, Definitions, MergedSandboxPolicy, RuleEntry, SandboxPreset, +}; use crate::rules::RuleError; use crate::rules::command_parser::{ FlagSchema, ParsedCommand, extract_commands, parse_command, shell_quote_join, @@ -25,6 +27,14 @@ pub struct EvalResult { pub sandbox_preset: Option, } +/// Result of compound command evaluation: an action and an optional merged +/// sandbox policy built from all sub-commands' sandbox presets. +#[derive(Debug, PartialEq)] +pub struct CompoundEvalResult { + pub action: Action, + pub sandbox_policy: Option, +} + /// The action determined by rule evaluation. #[derive(Debug, PartialEq)] pub enum Action { @@ -58,6 +68,128 @@ pub fn evaluate_command( evaluate_command_inner(config, command, context, 0) } +/// Evaluate a potentially compound command (containing `|`, `&&`, `||`, `;`) +/// by splitting it into individual commands, evaluating each, and aggregating +/// the results. +/// +/// - Action aggregation: Explicit Deny Wins (deny > ask > allow > default) +/// - Sandbox policy aggregation: Strictest Wins (writable roots intersected, +/// deny paths unioned, network access intersected) +/// - If sandbox aggregation produces empty writable roots (contradiction), +/// the action is escalated to `Ask` +/// +/// For single (non-compound) commands, this delegates to `evaluate_command`. +pub fn evaluate_compound( + config: &Config, + command: &str, + context: &EvalContext, +) -> Result { + let commands = extract_commands(command).unwrap_or_else(|_| vec![command.to_string()]); + + let default_definitions = Definitions::default(); + let definitions = config.definitions.as_ref().unwrap_or(&default_definitions); + let sandbox_defs = definitions.sandbox.as_ref(); + + let mut merged_action: Option = None; + let mut preset_names: Vec = Vec::new(); + + for cmd in &commands { + let result = evaluate_command(config, cmd, context)?; + + // Collect sandbox preset names + if let Some(name) = result.sandbox_preset { + preset_names.push(name); + } + + // Aggregate action using Explicit Deny Wins + merged_action = Some(match merged_action { + Some(prev) => merge_actions(prev, result.action), + None => result.action, + }); + } + + let action = merged_action.unwrap_or(Action::Default); + + // Resolve sandbox presets and merge policies + let sandbox_policy = if preset_names.is_empty() { + None + } else if let Some(sandbox_map) = sandbox_defs { + // Deduplicate preset names while preserving order + let mut seen = HashSet::new(); + let unique_names: Vec<&String> = preset_names + .iter() + .filter(|n| seen.insert(n.as_str())) + .collect(); + + let presets: Vec<&SandboxPreset> = unique_names + .iter() + .filter_map(|name| sandbox_map.get(name.as_str())) + .collect(); + + if presets.is_empty() { + None + } else { + SandboxPreset::merge_strictest(&presets) + } + } else { + None + }; + + // If sandbox policy has contradicting writable roots (empty after intersection + // but presets did define writable roots), escalate action to Ask + let (final_action, final_policy) = match (action, sandbox_policy) { + (action, Some(policy)) + if has_writable_contradiction(&policy, &preset_names, sandbox_defs) => + { + let escalated = escalate_to_ask(action); + (escalated, Some(policy)) + } + (action, policy) => (action, policy), + }; + + Ok(CompoundEvalResult { + action: final_action, + sandbox_policy: final_policy, + }) +} + +/// Check if the merged policy has a writable roots contradiction: +/// at least one source preset defined writable roots, but the intersection +/// is empty. +fn has_writable_contradiction( + policy: &MergedSandboxPolicy, + preset_names: &[String], + sandbox_defs: Option<&HashMap>, +) -> bool { + if !policy.writable.is_empty() { + return false; + } + + let sandbox_map = match sandbox_defs { + Some(m) => m, + None => return false, + }; + + // Check if any source preset actually defined writable roots + preset_names.iter().any(|name| { + sandbox_map + .get(name) + .and_then(|p| p.fs.as_ref()) + .and_then(|fs| fs.writable.as_ref()) + .is_some_and(|w| !w.is_empty()) + }) +} + +/// Escalate an action to Ask if it is currently Allow or Default. +fn escalate_to_ask(action: Action) -> Action { + match action { + Action::Allow | Action::Default => Action::Ask(Some( + "sandbox policy conflict: writable roots are contradictory".to_string(), + )), + other => other, + } +} + fn evaluate_command_inner( config: &Config, command: &str, @@ -220,10 +352,21 @@ fn try_unwrap_wrapper( /// Merge two evaluation results using Explicit Deny Wins priority. fn merge_results(a: EvalResult, b: EvalResult) -> EvalResult { - let a_priority = action_priority(&a.action); - let b_priority = action_priority(&b.action); + if action_priority(&b.action) > action_priority(&a.action) { + b + } else { + a + } +} - if b_priority > a_priority { b } else { a } +/// Merge two actions using Explicit Deny Wins priority, returning the more +/// restrictive one. +fn merge_actions(a: Action, b: Action) -> Action { + if action_priority(&b) > action_priority(&a) { + b + } else { + a + } } /// Map an action to its priority for Explicit Deny Wins comparison. @@ -810,4 +953,559 @@ mod tests { let result = evaluate_command(&config, "sudo echo 'hello world'", &empty_context).unwrap(); assert!(matches!(result.action, Action::Deny(_))); } + + // ======================================== + // Compound command evaluation + // ======================================== + + #[rstest] + fn compound_pipeline_all_allow(empty_context: EvalContext) { + let config = make_config(vec![allow_rule("ls *"), allow_rule("grep *")]); + let result = evaluate_compound(&config, "ls -la | grep foo", &empty_context).unwrap(); + assert_eq!(result.action, Action::Allow); + } + + #[rstest] + fn compound_and_chain_all_allow(empty_context: EvalContext) { + let config = make_config(vec![allow_rule("echo *"), allow_rule("ls *")]); + let result = evaluate_compound(&config, "echo hello && ls -la", &empty_context).unwrap(); + assert_eq!(result.action, Action::Allow); + } + + #[rstest] + fn compound_or_chain_all_allow(empty_context: EvalContext) { + let config = make_config(vec![allow_rule("echo *"), allow_rule("ls *")]); + let result = evaluate_compound(&config, "echo hello || ls -la", &empty_context).unwrap(); + assert_eq!(result.action, Action::Allow); + } + + #[rstest] + fn compound_deny_wins_in_pipeline(empty_context: EvalContext) { + let config = make_config(vec![allow_rule("ls *"), deny_rule("rm -rf *")]); + let result = evaluate_compound(&config, "ls -la | rm -rf /", &empty_context).unwrap(); + assert!(matches!(result.action, Action::Deny(_))); + } + + #[rstest] + fn compound_deny_wins_in_and_chain(empty_context: EvalContext) { + let config = make_config(vec![allow_rule("echo *"), deny_rule("rm -rf *")]); + let result = evaluate_compound(&config, "echo hello && rm -rf /", &empty_context).unwrap(); + assert!(matches!(result.action, Action::Deny(_))); + } + + #[rstest] + fn compound_deny_wins_in_or_chain(empty_context: EvalContext) { + let config = make_config(vec![allow_rule("echo *"), deny_rule("rm -rf *")]); + let result = evaluate_compound(&config, "echo hello || rm -rf /", &empty_context).unwrap(); + assert!(matches!(result.action, Action::Deny(_))); + } + + #[rstest] + fn compound_ask_wins_over_allow(empty_context: EvalContext) { + let config = make_config(vec![allow_rule("echo *"), ask_rule("git push *")]); + let result = + evaluate_compound(&config, "echo done && git push origin", &empty_context).unwrap(); + assert!(matches!(result.action, Action::Ask(_))); + } + + #[rstest] + fn compound_deny_wins_over_ask_and_allow(empty_context: EvalContext) { + let config = make_config(vec![ + allow_rule("echo *"), + ask_rule("git push *"), + deny_rule("rm -rf *"), + ]); + let result = evaluate_compound( + &config, + "echo done && git push origin && rm -rf /", + &empty_context, + ) + .unwrap(); + assert!(matches!(result.action, Action::Deny(_))); + } + + #[rstest] + fn compound_single_command_delegates(empty_context: EvalContext) { + let config = make_config(vec![allow_rule("git status")]); + let result = evaluate_compound(&config, "git status", &empty_context).unwrap(); + assert_eq!(result.action, Action::Allow); + assert_eq!(result.sandbox_policy, None); + } + + #[rstest] + fn compound_no_matching_rules_returns_default(empty_context: EvalContext) { + let config = make_config(vec![allow_rule("git status")]); + let result = evaluate_compound(&config, "hg status | wc -l", &empty_context).unwrap(); + assert_eq!(result.action, Action::Default); + } + + // ======================================== + // Compound: sandbox policy aggregation + // ======================================== + + use crate::config::{FsPolicy, NetworkPolicy, SandboxPreset}; + + fn make_sandbox_config( + rules: Vec, + sandbox: HashMap, + ) -> Config { + Config { + rules: Some(rules), + definitions: Some(Definitions { + sandbox: Some(sandbox), + ..Default::default() + }), + ..Default::default() + } + } + + fn allow_rule_with_sandbox(pattern: &str, sandbox: &str) -> RuleEntry { + let mut rule = allow_rule(pattern); + rule.sandbox = Some(sandbox.to_string()); + rule + } + + #[rstest] + fn compound_sandbox_writable_roots_intersection(empty_context: EvalContext) { + let config = make_sandbox_config( + vec![ + allow_rule_with_sandbox("ls *", "preset_a"), + allow_rule_with_sandbox("cat *", "preset_b"), + ], + HashMap::from([ + ( + "preset_a".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec![ + "/tmp".to_string(), + "/home".to_string(), + "/var".to_string(), + ]), + deny: None, + }), + network: None, + }, + ), + ( + "preset_b".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string(), "/var".to_string()]), + deny: None, + }), + network: None, + }, + ), + ]), + ); + + let result = evaluate_compound(&config, "ls -la | cat -", &empty_context).unwrap(); + assert_eq!(result.action, Action::Allow); + let policy = result.sandbox_policy.unwrap(); + assert_eq!(policy.writable, vec!["/tmp", "/var"]); + } + + #[rstest] + fn compound_sandbox_deny_paths_union(empty_context: EvalContext) { + let config = make_sandbox_config( + vec![ + allow_rule_with_sandbox("ls *", "preset_a"), + allow_rule_with_sandbox("cat *", "preset_b"), + ], + HashMap::from([ + ( + "preset_a".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: Some(vec!["/etc/passwd".to_string()]), + }), + network: None, + }, + ), + ( + "preset_b".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: Some(vec!["/etc/shadow".to_string()]), + }), + network: None, + }, + ), + ]), + ); + + let result = evaluate_compound(&config, "ls -la | cat -", &empty_context).unwrap(); + let policy = result.sandbox_policy.unwrap(); + assert_eq!(policy.deny, vec!["/etc/passwd", "/etc/shadow"]); + } + + #[rstest] + fn compound_sandbox_network_intersection(empty_context: EvalContext) { + let config = make_sandbox_config( + vec![ + allow_rule_with_sandbox("curl *", "preset_a"), + allow_rule_with_sandbox("wget *", "preset_b"), + ], + HashMap::from([ + ( + "preset_a".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: None, + }), + network: Some(NetworkPolicy { + allow: Some(vec!["github.com".to_string(), "pypi.org".to_string()]), + }), + }, + ), + ( + "preset_b".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: None, + }), + network: Some(NetworkPolicy { + allow: Some(vec!["github.com".to_string(), "npmjs.org".to_string()]), + }), + }, + ), + ]), + ); + + let result = evaluate_compound( + &config, + "curl https://github.com && wget https://npmjs.org", + &empty_context, + ) + .unwrap(); + let policy = result.sandbox_policy.unwrap(); + assert_eq!(policy.network_allow, Some(vec!["github.com".to_string()])); + } + + #[rstest] + fn compound_sandbox_network_restricted_by_any(empty_context: EvalContext) { + let config = make_sandbox_config( + vec![ + allow_rule_with_sandbox("curl *", "net_ok"), + allow_rule_with_sandbox("ls *", "no_net"), + ], + HashMap::from([ + ( + "net_ok".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: None, + }), + network: Some(NetworkPolicy { + allow: Some(vec!["github.com".to_string()]), + }), + }, + ), + ( + "no_net".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: None, + }), + network: Some(NetworkPolicy { allow: None }), + }, + ), + ]), + ); + + let result = evaluate_compound( + &config, + "curl https://github.com && ls /tmp", + &empty_context, + ) + .unwrap(); + let policy = result.sandbox_policy.unwrap(); + // network.allow: None in no_net means no network allowed -> empty list + assert_eq!(policy.network_allow, Some(vec![])); + } + + // ======================================== + // Compound: writable roots contradiction -> ask escalation + // ======================================== + + #[rstest] + fn compound_writable_contradiction_escalates_to_ask(empty_context: EvalContext) { + let config = make_sandbox_config( + vec![ + allow_rule_with_sandbox("ls *", "only_tmp"), + allow_rule_with_sandbox("cat *", "only_home"), + ], + HashMap::from([ + ( + "only_tmp".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: None, + }), + network: None, + }, + ), + ( + "only_home".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/home".to_string()]), + deny: None, + }), + network: None, + }, + ), + ]), + ); + + let result = evaluate_compound(&config, "ls -la | cat -", &empty_context).unwrap(); + // Writable roots intersection is empty -> contradiction -> escalate to Ask + assert!(matches!(result.action, Action::Ask(_))); + } + + #[rstest] + fn compound_writable_contradiction_does_not_downgrade_deny(empty_context: EvalContext) { + let config = make_sandbox_config( + vec![ + allow_rule_with_sandbox("ls *", "only_tmp"), + deny_rule("cat *"), + ], + HashMap::from([( + "only_tmp".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: None, + }), + network: None, + }, + )]), + ); + + let result = evaluate_compound(&config, "ls -la | cat -", &empty_context).unwrap(); + // deny should not be downgraded to ask + assert!(matches!(result.action, Action::Deny(_))); + } + + #[rstest] + fn compound_same_sandbox_preset_deduplicates(empty_context: EvalContext) { + let config = make_sandbox_config( + vec![ + allow_rule_with_sandbox("ls *", "preset_a"), + allow_rule_with_sandbox("cat *", "preset_a"), + ], + HashMap::from([( + "preset_a".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: Some(vec!["/etc".to_string()]), + }), + network: None, + }, + )]), + ); + + let result = evaluate_compound(&config, "ls -la | cat -", &empty_context).unwrap(); + let policy = result.sandbox_policy.unwrap(); + // Same preset intersected with itself should preserve the values + assert_eq!(policy.writable, vec!["/tmp"]); + assert_eq!(policy.deny, vec!["/etc"]); + } + + #[rstest] + fn compound_no_sandbox_presets_returns_none(empty_context: EvalContext) { + let config = make_config(vec![allow_rule("ls *"), allow_rule("cat *")]); + let result = evaluate_compound(&config, "ls -la | cat -", &empty_context).unwrap(); + assert_eq!(result.sandbox_policy, None); + } + + #[rstest] + fn compound_semicolon_separator(empty_context: EvalContext) { + let config = make_config(vec![allow_rule("echo *"), deny_rule("rm -rf *")]); + let result = evaluate_compound(&config, "echo hello; rm -rf /", &empty_context).unwrap(); + assert!(matches!(result.action, Action::Deny(_))); + } + + #[rstest] + fn compound_mixed_operators(empty_context: EvalContext) { + let config = make_config(vec![ + allow_rule("echo *"), + allow_rule("ls *"), + ask_rule("grep *"), + ]); + let result = + evaluate_compound(&config, "echo hello | grep world && ls -la", &empty_context) + .unwrap(); + assert!(matches!(result.action, Action::Ask(_))); + } + + #[rstest] + fn compound_partial_sandbox_only_some_commands(empty_context: EvalContext) { + // Only python3 has a sandbox preset; ls does not. + // The merged policy should reflect only the single preset. + let config = make_sandbox_config( + vec![ + allow_rule("ls *"), + allow_rule_with_sandbox("python3 *", "restricted"), + ], + HashMap::from([( + "restricted".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: Some(vec!["/etc".to_string()]), + }), + network: None, + }, + )]), + ); + + let result = + evaluate_compound(&config, "ls -la | python3 script.py", &empty_context).unwrap(); + assert_eq!(result.action, Action::Allow); + let policy = result.sandbox_policy.unwrap(); + assert_eq!(policy.writable, vec!["/tmp"]); + assert_eq!(policy.deny, vec!["/etc"]); + } + + #[rstest] + fn compound_single_command_with_sandbox(empty_context: EvalContext) { + let config = make_sandbox_config( + vec![allow_rule_with_sandbox("python3 *", "restricted")], + HashMap::from([( + "restricted".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: None, + }), + network: Some(NetworkPolicy { + allow: Some(vec!["pypi.org".to_string()]), + }), + }, + )]), + ); + + let result = evaluate_compound(&config, "python3 script.py", &empty_context).unwrap(); + assert_eq!(result.action, Action::Allow); + let policy = result.sandbox_policy.unwrap(); + assert_eq!(policy.writable, vec!["/tmp"]); + assert_eq!(policy.network_allow, Some(vec!["pypi.org".to_string()])); + } + + #[rstest] + fn compound_three_presets_progressive_intersection(empty_context: EvalContext) { + let config = make_sandbox_config( + vec![ + allow_rule_with_sandbox("cmd1 *", "p1"), + allow_rule_with_sandbox("cmd2 *", "p2"), + allow_rule_with_sandbox("cmd3 *", "p3"), + ], + HashMap::from([ + ( + "p1".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec![ + "/a".to_string(), + "/b".to_string(), + "/c".to_string(), + ]), + deny: Some(vec!["/x".to_string()]), + }), + network: None, + }, + ), + ( + "p2".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/b".to_string(), "/c".to_string()]), + deny: Some(vec!["/y".to_string()]), + }), + network: None, + }, + ), + ( + "p3".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/c".to_string(), "/d".to_string()]), + deny: Some(vec!["/z".to_string()]), + }), + network: None, + }, + ), + ]), + ); + + let result = evaluate_compound( + &config, + "cmd1 arg1 && cmd2 arg2 && cmd3 arg3", + &empty_context, + ) + .unwrap(); + let policy = result.sandbox_policy.unwrap(); + // Writable: {a,b,c} ∩ {b,c} ∩ {c,d} = {c} + assert_eq!(policy.writable, vec!["/c"]); + // Deny: {x} ∪ {y} ∪ {z} + assert_eq!(policy.deny, vec!["/x", "/y", "/z"]); + } + + #[rstest] + fn compound_ask_not_overwritten_by_escalation(empty_context: EvalContext) { + // If action is already Ask (from rule evaluation), writable contradiction + // should not overwrite the existing Ask message. + let config = make_sandbox_config( + vec![ + { + let mut rule = ask_rule("ls *"); + rule.sandbox = Some("only_tmp".to_string()); + rule.message = Some("confirm ls".to_string()); + rule + }, + allow_rule_with_sandbox("cat *", "only_home"), + ], + HashMap::from([ + ( + "only_tmp".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/tmp".to_string()]), + deny: None, + }), + network: None, + }, + ), + ( + "only_home".to_string(), + SandboxPreset { + fs: Some(FsPolicy { + writable: Some(vec!["/home".to_string()]), + deny: None, + }), + network: None, + }, + ), + ]), + ); + + let result = evaluate_compound(&config, "ls -la | cat -", &empty_context).unwrap(); + // Action is Ask from the rule itself; escalation should not change it + match &result.action { + Action::Ask(msg) => { + assert_eq!(msg.as_deref(), Some("confirm ls")); + } + other => panic!("expected Ask, got {:?}", other), + } + } } From c07385675c7791d7a0c41c9fbd31ff3b509b0421 Mon Sep 17 00:00:00 2001 From: Hayato Kawai Date: Thu, 19 Feb 2026 00:52:11 +0900 Subject: [PATCH 2/2] rules: address review feedback on compound eval Gemini Code Assist review pointed out redundant iteration in has_writable_contradiction and test cases that should be parameterized per the repository style guide. Pass deduplicated unique_names to has_writable_contradiction instead of the raw preset_names list. Parameterize similar test cases using rstest in both model.rs (merge_strictest writable/network) and rule_engine.rs (compound all_allow/deny_wins across operators). Co-Authored-By: Claude Opus 4.6 --- src/config/model.rs | 92 +++++++++++++++++----------------------- src/rules/rule_engine.rs | 76 ++++++++++++++------------------- 2 files changed, 69 insertions(+), 99 deletions(-) diff --git a/src/config/model.rs b/src/config/model.rs index 618e6fd..b97ee7c 100644 --- a/src/config/model.rs +++ b/src/config/model.rs @@ -1328,28 +1328,38 @@ mod tests { assert_eq!(result.network_allow, Some(vec!["github.com".to_string()])); } - #[test] - fn merge_strictest_writable_intersection() { + #[rstest] + #[case::non_empty_intersection( + vec!["/tmp".to_string(), "/home".to_string(), "/var".to_string()], + vec!["/tmp".to_string(), "/var".to_string()], + vec!["/tmp", "/var"], + )] + #[case::empty_intersection( + vec!["/tmp".to_string()], + vec!["/home".to_string()], + vec![], + )] + fn merge_strictest_writable_intersection( + #[case] writable_a: Vec, + #[case] writable_b: Vec, + #[case] expected: Vec<&str>, + ) { let a = SandboxPreset { fs: Some(FsPolicy { - writable: Some(vec![ - "/tmp".to_string(), - "/home".to_string(), - "/var".to_string(), - ]), + writable: Some(writable_a), deny: None, }), network: None, }; let b = SandboxPreset { fs: Some(FsPolicy { - writable: Some(vec!["/tmp".to_string(), "/var".to_string()]), + writable: Some(writable_b), deny: None, }), network: None, }; let result = SandboxPreset::merge_strictest(&[&a, &b]).unwrap(); - assert_eq!(result.writable, vec!["/tmp", "/var"]); + assert_eq!(result.writable, expected); } #[test] @@ -1372,58 +1382,32 @@ mod tests { assert_eq!(result.deny, vec!["/etc/passwd", "/etc/shadow"]); } - #[test] - fn merge_strictest_network_intersection() { - let a = SandboxPreset { - fs: None, - network: Some(NetworkPolicy { - allow: Some(vec!["github.com".to_string(), "pypi.org".to_string()]), - }), - }; - let b = SandboxPreset { - fs: None, - network: Some(NetworkPolicy { - allow: Some(vec!["github.com".to_string(), "npmjs.org".to_string()]), - }), - }; - let result = SandboxPreset::merge_strictest(&[&a, &b]).unwrap(); - assert_eq!(result.network_allow, Some(vec!["github.com".to_string()])); - } - - #[test] - fn merge_strictest_network_none_means_restricted() { + #[rstest] + #[case::intersection( + Some(NetworkPolicy { allow: Some(vec!["github.com".to_string(), "pypi.org".to_string()]) }), + Some(NetworkPolicy { allow: Some(vec!["github.com".to_string(), "npmjs.org".to_string()]) }), + Some(vec!["github.com".to_string()]), + )] + #[case::none_means_restricted( + Some(NetworkPolicy { allow: Some(vec!["github.com".to_string()]) }), + Some(NetworkPolicy { allow: None }), + Some(vec![]), + )] + fn merge_strictest_network( + #[case] network_a: Option, + #[case] network_b: Option, + #[case] expected: Option>, + ) { let a = SandboxPreset { fs: None, - network: Some(NetworkPolicy { - allow: Some(vec!["github.com".to_string()]), - }), + network: network_a, }; let b = SandboxPreset { fs: None, - network: Some(NetworkPolicy { allow: None }), - }; - let result = SandboxPreset::merge_strictest(&[&a, &b]).unwrap(); - assert_eq!(result.network_allow, Some(vec![])); - } - - #[test] - fn merge_strictest_writable_empty_intersection() { - let a = SandboxPreset { - fs: Some(FsPolicy { - writable: Some(vec!["/tmp".to_string()]), - deny: None, - }), - network: None, - }; - let b = SandboxPreset { - fs: Some(FsPolicy { - writable: Some(vec!["/home".to_string()]), - deny: None, - }), - network: None, + network: network_b, }; let result = SandboxPreset::merge_strictest(&[&a, &b]).unwrap(); - assert!(result.writable.is_empty()); + assert_eq!(result.network_allow, expected); } #[test] diff --git a/src/rules/rule_engine.rs b/src/rules/rule_engine.rs index 9304918..cd0369f 100644 --- a/src/rules/rule_engine.rs +++ b/src/rules/rule_engine.rs @@ -110,17 +110,17 @@ pub fn evaluate_compound( let action = merged_action.unwrap_or(Action::Default); + // Deduplicate preset names while preserving order + let mut seen = HashSet::new(); + let unique_names: Vec<&String> = preset_names + .iter() + .filter(|n| seen.insert(n.as_str())) + .collect(); + // Resolve sandbox presets and merge policies - let sandbox_policy = if preset_names.is_empty() { + let sandbox_policy = if unique_names.is_empty() { None } else if let Some(sandbox_map) = sandbox_defs { - // Deduplicate preset names while preserving order - let mut seen = HashSet::new(); - let unique_names: Vec<&String> = preset_names - .iter() - .filter(|n| seen.insert(n.as_str())) - .collect(); - let presets: Vec<&SandboxPreset> = unique_names .iter() .filter_map(|name| sandbox_map.get(name.as_str())) @@ -139,7 +139,7 @@ pub fn evaluate_compound( // but presets did define writable roots), escalate action to Ask let (final_action, final_policy) = match (action, sandbox_policy) { (action, Some(policy)) - if has_writable_contradiction(&policy, &preset_names, sandbox_defs) => + if has_writable_contradiction(&policy, &unique_names, sandbox_defs) => { let escalated = escalate_to_ask(action); (escalated, Some(policy)) @@ -158,7 +158,7 @@ pub fn evaluate_compound( /// is empty. fn has_writable_contradiction( policy: &MergedSandboxPolicy, - preset_names: &[String], + preset_names: &[&String], sandbox_defs: Option<&HashMap>, ) -> bool { if !policy.writable.is_empty() { @@ -173,7 +173,7 @@ fn has_writable_contradiction( // Check if any source preset actually defined writable roots preset_names.iter().any(|name| { sandbox_map - .get(name) + .get(name.as_str()) .and_then(|p| p.fs.as_ref()) .and_then(|fs| fs.writable.as_ref()) .is_some_and(|w| !w.is_empty()) @@ -959,44 +959,30 @@ mod tests { // ======================================== #[rstest] - fn compound_pipeline_all_allow(empty_context: EvalContext) { - let config = make_config(vec![allow_rule("ls *"), allow_rule("grep *")]); - let result = evaluate_compound(&config, "ls -la | grep foo", &empty_context).unwrap(); - assert_eq!(result.action, Action::Allow); - } - - #[rstest] - fn compound_and_chain_all_allow(empty_context: EvalContext) { - let config = make_config(vec![allow_rule("echo *"), allow_rule("ls *")]); - let result = evaluate_compound(&config, "echo hello && ls -la", &empty_context).unwrap(); - assert_eq!(result.action, Action::Allow); - } - - #[rstest] - fn compound_or_chain_all_allow(empty_context: EvalContext) { - let config = make_config(vec![allow_rule("echo *"), allow_rule("ls *")]); - let result = evaluate_compound(&config, "echo hello || ls -la", &empty_context).unwrap(); + #[case::pipeline("ls -la | grep foo")] + #[case::and_chain("echo hello && ls -la")] + #[case::or_chain("echo hello || ls -la")] + fn compound_all_allow(#[case] command: &str, empty_context: EvalContext) { + let config = make_config(vec![ + allow_rule("ls *"), + allow_rule("grep *"), + allow_rule("echo *"), + ]); + let result = evaluate_compound(&config, command, &empty_context).unwrap(); assert_eq!(result.action, Action::Allow); } #[rstest] - fn compound_deny_wins_in_pipeline(empty_context: EvalContext) { - let config = make_config(vec![allow_rule("ls *"), deny_rule("rm -rf *")]); - let result = evaluate_compound(&config, "ls -la | rm -rf /", &empty_context).unwrap(); - assert!(matches!(result.action, Action::Deny(_))); - } - - #[rstest] - fn compound_deny_wins_in_and_chain(empty_context: EvalContext) { - let config = make_config(vec![allow_rule("echo *"), deny_rule("rm -rf *")]); - let result = evaluate_compound(&config, "echo hello && rm -rf /", &empty_context).unwrap(); - assert!(matches!(result.action, Action::Deny(_))); - } - - #[rstest] - fn compound_deny_wins_in_or_chain(empty_context: EvalContext) { - let config = make_config(vec![allow_rule("echo *"), deny_rule("rm -rf *")]); - let result = evaluate_compound(&config, "echo hello || rm -rf /", &empty_context).unwrap(); + #[case::pipeline("ls -la | rm -rf /")] + #[case::and_chain("echo hello && rm -rf /")] + #[case::or_chain("echo hello || rm -rf /")] + fn compound_deny_wins(#[case] command: &str, empty_context: EvalContext) { + let config = make_config(vec![ + allow_rule("ls *"), + allow_rule("echo *"), + deny_rule("rm -rf *"), + ]); + let result = evaluate_compound(&config, command, &empty_context).unwrap(); assert!(matches!(result.action, Action::Deny(_))); }