diff --git a/src/permission/checker.rs b/src/permission/checker.rs index c9affb0b..af344ead 100644 --- a/src/permission/checker.rs +++ b/src/permission/checker.rs @@ -25,6 +25,22 @@ pub struct PermissionChecker { mode: SecurityMode, } +/// Tool names where the input is a filesystem path. For these, `*` keeps +/// classic glob semantics (one segment, doesn't cross `/`). Everything else +/// is treated as shell/text where `*` means "any chars including /". +pub(crate) fn is_path_tool_name(tool: &str) -> bool { + matches!(tool, "read" | "write" | "edit" | "list_dir") +} + +/// Build a Pattern with the right `*` semantics for the given tool. +pub(crate) fn pattern_for_tool(tool: &str, pat: &str) -> Pattern { + if is_path_tool_name(tool) { + Pattern::new(pat) + } else { + Pattern::new_command(pat) + } +} + impl PermissionChecker { pub fn new( config: &PermissionConfig, @@ -49,11 +65,11 @@ impl PermissionChecker { let mut entries = Vec::new(); match tp { ToolPerm::Simple(action) => { - entries.push((Pattern::new("*"), *action)); + entries.push((pattern_for_tool(tool_name, "*"), *action)); } ToolPerm::Granular(map) => { for (pat, action) in map { - entries.push((Pattern::new(pat), *action)); + entries.push((pattern_for_tool(tool_name, pat), *action)); } } } @@ -63,11 +79,12 @@ impl PermissionChecker { if !rules.contains_key("bash") { let mut defaults = Vec::new(); for (pat, action) in crate::permission::default_bash_rules() { - defaults.push((Pattern::new(pat), action)); + defaults.push((pattern_for_tool("bash", pat), action)); } rules.insert("bash".to_string(), defaults); } + // External-directory rules are always path patterns by definition. let ext_dir_rules = config .external_directory .as_ref() @@ -239,14 +256,14 @@ impl PermissionChecker { } pub fn add_session_allowlist(&mut self, tool: String, pattern_str: &str) { - let pattern = Pattern::new(pattern_str); + let pattern = pattern_for_tool(&tool, pattern_str); self.session_allowlist.push((tool, pattern)); } pub fn load_session_allowlist(&mut self, entries: &[(String, String)]) { for (tool, pat) in entries { self.session_allowlist - .push((tool.clone(), Pattern::new(pat))); + .push((tool.clone(), pattern_for_tool(tool, pat))); } } @@ -318,3 +335,88 @@ fn resolve_absolute(path: &str, working_dir: &str) -> String { Path::new(working_dir).join(p).to_string_lossy().to_string() } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::permission::PermissionConfig; + + fn fresh_checker() -> PermissionChecker { + PermissionChecker::new( + &PermissionConfig::default(), + SecurityMode::Standard, + Some(std::path::PathBuf::from("/tmp")), + ) + } + + // Regression: "allow always" → `cd *` saved to session allowlist must + // satisfy the NEXT bash check for `cd /absolute/path`. Before the fix, + // path-glob semantics on `*` (`[^/]*`) refused to match the absolute + // path, so the user was re-prompted every command. + #[test] + fn regression_session_allowlist_cd_star_matches_path_arg() { + let mut checker = fresh_checker(); + checker.add_session_allowlist("bash".to_string(), "cd *"); + + // The exact scenario from the bug report. + let r1 = checker.check( + "bash", + "cd /Users/yogthos/src/work/rigging-workshop && git diff", + ); + assert!( + matches!(r1, CheckResult::Allowed), + "expected Allowed, got {:?}", + r1 + ); + + let r2 = checker.check("bash", "cd /Users/yogthos/src/work/rigging-workshop"); + assert!(matches!(r2, CheckResult::Allowed)); + } + + // Path-tool patterns still get filesystem-glob semantics — adding + // `src/*` doesn't allow nested files. Force default Ask so we can read + // the session-allowlist contribution in isolation from the default. + #[test] + fn path_tool_session_allowlist_keeps_one_segment_semantics() { + let mut cfg = PermissionConfig::default(); + cfg.default = Some(Action::Ask); + let mut checker = PermissionChecker::new( + &cfg, + SecurityMode::Standard, + Some(std::path::PathBuf::from("/tmp")), + ); + checker.add_session_allowlist("read".to_string(), "src/*"); + + // One-segment hit from the session allowlist. + assert!(matches!( + checker.check_path("read", "src/main.rs"), + CheckResult::Allowed + )); + // Nested path: not in allowlist, falls through to default Ask. + let nested = checker.check_path("read", "src/agent/main.rs"); + assert!( + matches!(nested, CheckResult::Ask), + "src/* must not match nested path; got {:?}", + nested + ); + } + + // load_session_allowlist roundtrip: persisted patterns from a previous + // session should match the way they did when saved. + #[test] + fn regression_load_session_allowlist_preserves_command_semantics() { + let mut checker = fresh_checker(); + let saved = vec![("bash".to_string(), "cd *".to_string())]; + checker.load_session_allowlist(&saved); + + let r = checker.check("bash", "cd /home/me/project"); + assert!(matches!(r, CheckResult::Allowed)); + } + + #[test] + fn pattern_for_tool_distinguishes_path_and_command_tools() { + assert!(pattern_for_tool("bash", "cd *").matches("cd /a/b/c")); + assert!(!pattern_for_tool("read", "cd *").matches("cd /a/b/c")); + assert!(pattern_for_tool("read", "cd *").matches("cd file")); + } +} diff --git a/src/permission/pattern.rs b/src/permission/pattern.rs index e36681c5..92777d08 100644 --- a/src/permission/pattern.rs +++ b/src/permission/pattern.rs @@ -8,9 +8,27 @@ pub struct Pattern { } impl Pattern { + /// Filesystem-style glob: `*` matches one path segment (no `/`); `**` + /// matches any depth. Use for path tools (`read`, `write`, `edit`, + /// `list_dir`). pub fn new(pattern: &str) -> Self { + Self::compile(pattern, /* path_style */ true) + } + + /// Shell-style glob for non-path inputs: `*` matches any chars including + /// `/`. Use for `bash` command patterns, `grep`/`find_files` patterns, + /// and other tools where the input isn't a filesystem path. + /// + /// Without this, a user pattern like `cd *` (suggested by the harness + /// for `bash` after the user accepts "allow always") would NOT match + /// `cd /Users/foo/bar` because `[^/]*` stops at the first slash. + pub fn new_command(pattern: &str) -> Self { + Self::compile(pattern, /* path_style */ false) + } + + fn compile(pattern: &str, path_style: bool) -> Self { let expanded = expand_home(pattern); - let regex_str = glob_to_regex(&expanded); + let regex_str = glob_to_regex(&expanded, path_style); let regex = Regex::new(®ex_str).unwrap_or_else(|_| Regex::new("^$").unwrap()); Pattern { regex, @@ -44,7 +62,84 @@ fn expand_home(pattern: &str) -> String { pattern.to_string() } -fn glob_to_regex(pattern: &str) -> String { +#[cfg(test)] +mod tests { + use super::*; + + // Regression: `cd *` saved via "allow always" must match the user's NEXT + // `cd /absolute/path` command. The original bug was filesystem-glob + // semantics applied to a shell-command pattern: `*` compiled to `[^/]*`, + // refusing to cross slashes. Allowlist entries for bash never fired and + // the agent re-prompted on every command. + #[test] + fn regression_command_pattern_cd_star_matches_path_arg() { + let pat = Pattern::new_command("cd *"); + assert!(pat.matches("cd /Users/yogthos/src/work/foo")); + assert!(pat.matches("cd /Users/yogthos/src/work/foo && git diff")); + assert!(pat.matches("cd foo")); + } + + #[test] + fn regression_command_pattern_anchors_to_start() { + // Don't over-rotate: `cd *` shouldn't match commands that merely + // contain `cd ` somewhere later. + let pat = Pattern::new_command("cd *"); + assert!(!pat.matches("xcd foo")); + assert!(!pat.matches("echo cd foo")); + } + + #[test] + fn path_pattern_star_still_excludes_slash() { + let pat = Pattern::new("src/*"); + assert!(pat.matches("src/main.rs")); + // Single segment only — `*` doesn't span directory boundaries. + assert!(!pat.matches("src/agent/main.rs")); + } + + #[test] + fn path_pattern_double_star_spans_directories() { + let pat = Pattern::new("src/**"); + assert!(pat.matches("src/main.rs")); + assert!(pat.matches("src/agent/main.rs")); + assert!(pat.matches("src/agent/tools/foo.rs")); + } + + #[test] + fn command_pattern_question_mark_matches_any_char() { + let pat = Pattern::new_command("file.?"); + assert!(pat.matches("file.a")); + // For commands, `?` is unrestricted. + assert!(pat.matches("file./")); + } + + #[test] + fn path_pattern_question_mark_excludes_slash() { + let pat = Pattern::new("file.?"); + assert!(pat.matches("file.a")); + assert!(!pat.matches("file./")); + } + + #[test] + fn home_expansion_works_for_both_styles() { + if let Some(home) = dirs::home_dir() { + let expected = format!("{}/foo/bar", home.display()); + assert!(Pattern::new("~/foo/*").matches(&expected)); + assert!(Pattern::new_command("~/foo/*").matches(&expected)); + } + } + + // Regex metachars in pattern text must be escaped, not interpreted. + #[test] + fn special_chars_are_escaped() { + let pat = Pattern::new_command("npm test (unit)"); + assert!(pat.matches("npm test (unit)")); + // Without escaping, `(unit)` would be a regex group and not require + // the literal parens. + assert!(!pat.matches("npm test unit")); + } +} + +fn glob_to_regex(pattern: &str, path_style: bool) -> String { let mut re = String::with_capacity(pattern.len() * 2); re.push('^'); let mut chars = pattern.chars().peekable(); @@ -59,10 +154,13 @@ fn glob_to_regex(pattern: &str) -> String { } else { re.push_str(".*"); } - } else { + } else if path_style { re.push_str("[^/]*"); + } else { + re.push_str(".*"); } } + '?' if path_style => re.push_str("[^/]"), '?' => re.push('.'), '.' => re.push_str("\\."), '\\' => re.push_str("\\\\"),