From e30df5af7d2ca38f139741c07fadf16adb28d5c0 Mon Sep 17 00:00:00 2001 From: Sewer56 Date: Tue, 7 Apr 2026 18:44:58 +0100 Subject: [PATCH] Changed: Expand shell patterns in permission rules at construction time - Add canonical expand_pattern in normalize.rs; expand_shell is now a thin wrapper - Rule::new() expands ~/, $HOME/, $VAR in patterns with zero-alloc passthrough - Deduplicate shell expansion tests into normalize.rs; permissions.rs keeps integration tests --- .../src/path/allowed_glob/mod.rs | 2 +- .../src/path/allowed_glob/normalize.rs | 134 ++++++++++++------ src/llm-coding-tools-core/src/path/mod.rs | 2 +- src/llm-coding-tools-core/src/permissions.rs | 78 +++++++++- 4 files changed, 168 insertions(+), 48 deletions(-) diff --git a/src/llm-coding-tools-core/src/path/allowed_glob/mod.rs b/src/llm-coding-tools-core/src/path/allowed_glob/mod.rs index 208a645..e01a881 100644 --- a/src/llm-coding-tools-core/src/path/allowed_glob/mod.rs +++ b/src/llm-coding-tools-core/src/path/allowed_glob/mod.rs @@ -3,7 +3,7 @@ //! Provides [`AllowedGlobResolver`] which restricts path access to allowed //! directories with glob pattern filtering. -mod normalize; +pub(crate) mod normalize; mod policy; use super::{path_analysis, resolve_new_file_fast, PathResolver}; diff --git a/src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs b/src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs index 91b8edb..3bc63d5 100644 --- a/src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs +++ b/src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs @@ -26,15 +26,22 @@ pub(crate) fn normalize_path(path: &Path) -> Cow<'_, str> { } } -/// Expands shell-like patterns (`~/`, `$HOME/`, `$VAR`, `${VAR:-default}`) in a -/// path string. +/// Expands shell-like patterns (`~/`, `$HOME/`, `$VAR`, `${VAR:-default}`). /// -/// Returns the expanded path on success, or a `ToolError::InvalidPath` if -/// expansion fails (e.g., environment variable not set or contains non-Unicode -/// data). Uses `shellexpand` which internally uses `dirs::home_dir()` for -/// cross-platform home detection. +/// Returns `Cow::Borrowed` for patterns without shell metacharacters (zero allocation). +/// All other `expand_*` functions in this crate are thin wrappers around this one. +pub(crate) fn expand_pattern( + pattern: &str, +) -> Result, shellexpand::LookupError> { + shellexpand::full(pattern) +} + +/// Expands shell-like patterns in a path string, returning a [`PathBuf`]. +/// +/// Wraps [`expand_pattern`] with fail-fast error handling: returns +/// `ToolError::InvalidPath` if expansion fails (e.g., unset variable). pub(crate) fn expand_shell(path: &str) -> ToolResult { - shellexpand::full(path) + expand_pattern(path) .map(|cow| PathBuf::from(cow.into_owned())) .map_err(|e| { ToolError::InvalidPath(format!( @@ -47,6 +54,7 @@ pub(crate) fn expand_shell(path: &str) -> ToolResult { #[cfg(test)] mod tests { use super::*; + use rstest::rstest; use temp_env; use tempfile::TempDir; @@ -94,50 +102,20 @@ mod tests { } #[test] - fn expands_home_tilde() { - #[cfg(windows)] - { - let expected_home = dirs::home_dir().expect("home directory should exist"); - let expected_home = strip_verbatim(expected_home.canonicalize().unwrap()); - - let result = strip_verbatim(expand_shell("~/project").unwrap()); - assert!(result.starts_with(&expected_home)); - assert!(result.ends_with("project")); - } - - #[cfg(not(windows))] - { - let temp_dir = TempDir::new().unwrap(); - let temp_home_path = temp_dir.path().canonicalize().unwrap(); - temp_env::with_var("HOME", Some(&temp_home_path), || { - let result = expand_shell("~/project").unwrap(); - assert!(result.starts_with(&temp_home_path)); - assert!(result.ends_with("project")); - }); - } - } - - #[test] - fn expands_home_dollar() { + fn expand_shell_produces_pathbuf_on_success() { let temp_dir = TempDir::new().unwrap(); - let temp_home_path = temp_dir.path().canonicalize().unwrap(); - let temp_home_path = strip_verbatim(temp_home_path); + let temp_home = temp_dir.path().canonicalize().unwrap(); + let temp_home = strip_verbatim(temp_home); - temp_env::with_var("HOME", Some(&temp_home_path), || { + temp_env::with_var("HOME", Some(&temp_home), || { let result = strip_verbatim(expand_shell("$HOME/workspace").unwrap()); - assert!(result.starts_with(&temp_home_path)); + assert!(result.starts_with(&temp_home)); assert!(result.ends_with("workspace")); }); } #[test] - fn leaves_path_without_shell_patterns_unchanged() { - let result = expand_shell("/some/path").unwrap(); - assert_eq!(result, PathBuf::from("/some/path")); - } - - #[test] - fn returns_error_for_unset_environment_variable() { + fn expand_shell_returns_error_for_unset_environment_variable() { temp_env::with_var("DEFINITELY_NOT_SET_12345", None::<&str>, || { let result = expand_shell("$DEFINITELY_NOT_SET_12345/project"); assert!(result.is_err()); @@ -146,4 +124,74 @@ mod tests { assert!(err.to_string().contains("DEFINITELY_NOT_SET_12345")); }); } + + // --- expand_pattern --- + + #[cfg(not(windows))] + #[test] + fn expand_pattern_should_expand_tilde() { + let temp_dir = TempDir::new().unwrap(); + let temp_home = temp_dir.path().canonicalize().unwrap(); + + temp_env::with_var("HOME", Some(&temp_home), || { + let expanded = expand_pattern("~/projects/*").unwrap(); + let expanded_str = expanded.as_ref(); + assert!( + expanded_str.starts_with(temp_home.to_str().unwrap()), + "expected expansion to start with {:?}, got {:?}", + temp_home, + expanded_str + ); + assert!( + expanded_str.ends_with("/projects/*"), + "expected expansion to end with /projects/*, got {:?}", + expanded_str + ); + }); + } + + #[test] + fn expand_pattern_should_expand_dollar_home() { + let temp_dir = TempDir::new().unwrap(); + let temp_home = temp_dir.path().canonicalize().unwrap(); + + temp_env::with_var("HOME", Some(&temp_home), || { + let expanded = expand_pattern("$HOME/.config/*").unwrap(); + let expanded_str = expanded.as_ref(); + assert!( + expanded_str.starts_with(temp_home.to_str().unwrap()), + "expected expansion to start with {:?}, got {:?}", + temp_home, + expanded_str + ); + assert!( + expanded_str.ends_with("/.config/*"), + "expected expansion to end with /.config/*, got {:?}", + expanded_str + ); + }); + } + + #[rstest] + #[case::absolute("/workspace/src/lib.rs")] + #[case::wildcard("*.rs")] + #[case::prefix_wildcard("orchestrator-*")] + #[case::exact("bash")] + #[case::star("*")] + fn expand_pattern_should_borrow_when_no_shell_chars(#[case] pattern: &str) { + let result = expand_pattern(pattern).unwrap(); + assert!( + matches!(result, Cow::Borrowed(_)), + "expected Borrowed for {:?}, got Owned", + pattern + ); + assert_eq!(result.as_ref(), pattern); + } + + #[test] + fn expand_pattern_should_return_error_on_failure() { + temp_env::with_var("DEFINITELY_NOT_SET_99999", None::<&str>, || { + assert!(expand_pattern("$DEFINITELY_NOT_SET_99999/path").is_err()); + }); + } } diff --git a/src/llm-coding-tools-core/src/path/mod.rs b/src/llm-coding-tools-core/src/path/mod.rs index b389b8b..fd7e940 100644 --- a/src/llm-coding-tools-core/src/path/mod.rs +++ b/src/llm-coding-tools-core/src/path/mod.rs @@ -7,7 +7,7 @@ mod absolute; mod allowed; -mod allowed_glob; +pub(crate) mod allowed_glob; pub use absolute::AbsolutePathResolver; pub use allowed::AllowedPathResolver; diff --git a/src/llm-coding-tools-core/src/permissions.rs b/src/llm-coding-tools-core/src/permissions.rs index 2ff6519..05f0f29 100644 --- a/src/llm-coding-tools-core/src/permissions.rs +++ b/src/llm-coding-tools-core/src/permissions.rs @@ -52,10 +52,11 @@ //! assert_eq!(rules.evaluate("task", "other-agent"), PermissionAction::Deny); //! ``` -use serde::{Deserialize, Serialize}; - use crate::internal::hash64::hash_u64; use crate::internal::hash64::Hash64; +use crate::path::allowed_glob::normalize::expand_pattern; +use serde::{Deserialize, Serialize}; +use std::borrow::Cow; /// Permission level for tool access. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] @@ -127,7 +128,12 @@ impl Rule { action: PermissionAction, ) -> Self { let permission = permission.into(); - let pattern = pattern.into(); + let pattern_box: Box = pattern.into(); + let pattern: Box = match expand_pattern(&pattern_box) { + Ok(Cow::Borrowed(_)) => pattern_box, + Ok(Cow::Owned(s)) => s.into_boxed_str(), + Err(_) => pattern_box, + }; Self { permission_hash: hash_u64(&permission), pattern_hash: hash_u64(&pattern), @@ -432,6 +438,8 @@ fn evaluate_single_rule(rule: &Rule, permission: &str, subject: &str) -> Permiss mod tests { use super::*; use rstest::rstest; + use temp_env; + use tempfile::TempDir; fn build_and_eval( rules: &[(&str, &str, PermissionAction)], @@ -723,4 +731,68 @@ mod tests { let combined = Ruleset::merged([&r1, &r2]); assert_eq!(combined.evaluate("a", "x"), PermissionAction::Allow); } + + // --- Rule::new with expansion integration --- + + /// Verifies that a rule created with `~/` pattern matches the expanded home path. + #[cfg(not(windows))] + #[test] + fn rule_with_tilde_pattern_should_match_expanded_path() { + let temp_dir = TempDir::new().unwrap(); + let temp_home = temp_dir.path().canonicalize().unwrap(); + + temp_env::with_var("HOME", Some(&temp_home), || { + let mut ruleset = Ruleset::new(); + ruleset.push(Rule::new("read", "~/projects/*", PermissionAction::Allow)); + + let subject = format!("{}/projects/src/lib.rs", temp_home.to_str().unwrap()); + assert_eq!( + ruleset.evaluate("read", &subject), + PermissionAction::Allow, + "expanded ~/ pattern should match real path" + ); + }); + } + + /// Verifies that a rule created with `$HOME/` pattern matches the expanded path. + #[test] + fn rule_with_dollar_home_pattern_should_match_expanded_path() { + let temp_dir = TempDir::new().unwrap(); + let temp_home = temp_dir.path().canonicalize().unwrap(); + + temp_env::with_var("HOME", Some(&temp_home), || { + let mut ruleset = Ruleset::new(); + ruleset.push(Rule::new( + "read", + "$HOME/.config/*", + PermissionAction::Allow, + )); + + let subject = format!("{}/.config/app.conf", temp_home.to_str().unwrap()); + assert_eq!( + ruleset.evaluate("read", &subject), + PermissionAction::Allow, + "expanded $HOME/ pattern should match real path" + ); + }); + } + + /// Verifies that rules without shell patterns evaluate correctly after + /// the expansion change (regression guard). + #[test] + fn rule_without_shell_patterns_evaluates_correctly() { + let mut ruleset = Ruleset::new(); + ruleset.push(Rule::new("bash", "*", PermissionAction::Allow)); + ruleset.push(Rule::new("task", "orchestrator-*", PermissionAction::Allow)); + + assert_eq!( + ruleset.evaluate("bash", "anything"), + PermissionAction::Allow + ); + assert_eq!( + ruleset.evaluate("task", "orchestrator-builder"), + PermissionAction::Allow + ); + assert_eq!(ruleset.evaluate("task", "other"), PermissionAction::Deny); + } }