Skip to content

Expand shell patterns in permission rules at construction time #92

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Sewer56 merged 1 commit into from
Apr 7, 2026
Merged

Expand shell patterns in permission rules at construction time #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/llm-coding-tools-core/src/path/allowed_glob/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
134 changes: 91 additions & 43 deletions src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Cow<'_, str>, shellexpand::LookupError<std::env::VarError>> {
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<PathBuf> {
shellexpand::full(path)
expand_pattern(path)
.map(|cow| PathBuf::from(cow.into_owned()))
.map_err(|e| {
ToolError::InvalidPath(format!(
Expand All @@ -47,6 +54,7 @@ pub(crate) fn expand_shell(path: &str) -> ToolResult<PathBuf> {
#[cfg(test)]
mod tests {
use super::*;
use rstest::rstest;
use temp_env;
use tempfile::TempDir;

Expand Down Expand Up @@ -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());
Expand All @@ -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());
});
}
}
2 changes: 1 addition & 1 deletion src/llm-coding-tools-core/src/path/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

mod absolute;
mod allowed;
mod allowed_glob;
pub(crate) mod allowed_glob;

pub use absolute::AbsolutePathResolver;
pub use allowed::AllowedPathResolver;
Expand Down
78 changes: 75 additions & 3 deletions src/llm-coding-tools-core/src/permissions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -127,7 +128,12 @@ impl Rule {
action: PermissionAction,
) -> Self {
let permission = permission.into();
let pattern = pattern.into();
let pattern_box: Box<str> = pattern.into();
let pattern: Box<str> = match expand_pattern(&pattern_box) {
Ok(Cow::Borrowed(_)) => pattern_box,
Ok(Cow::Owned(s)) => s.into_boxed_str(),
Err(_) => pattern_box,
};
Comment on lines +131 to +136
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if there are other callers of Rule::new that might be affected by silent fallback
rg -nP --type=rust '\bRule::new\s*\(' -C2

Repository: Sewer56/llm-coding-tools

Length of output: 16756

🏁 Script executed:

#!/bin/bash
# First, let's look at the Rule::new implementation in detail (lines 125-150)
sed -n '125,150p' src/llm-coding-tools-core/src/permissions.rs

Repository: Sewer56/llm-coding-tools

Length of output: 988

🏁 Script executed:

#!/bin/bash
# Now let's check the expand_pattern function to understand its error handling
rg -nP 'fn expand_pattern|pub fn expand_pattern' -A10 src/llm-coding-tools-core/src/permissions.rs

Repository: Sewer56/llm-coding-tools

Length of output: 50

🏁 Script executed:

#!/bin/bash
# Check if there are tests specifically for pattern expansion failures
rg -nP 'expand_pattern.*Err|test.*expand|test.*unset' -C3 src/llm-coding-tools-core/src/permissions.rs

Repository: Sewer56/llm-coding-tools

Length of output: 50

🏁 Script executed:

#!/bin/bash
# Look at extensions.rs to see how patterns from YAML are passed to Rule::new
cat -n src/llm-coding-tools-agents/src/extensions.rs | sed -n '40,70p'

Repository: Sewer56/llm-coding-tools

Length of output: 1218

🏁 Script executed:

#!/bin/bash
# Search for expand_pattern definition more broadly
rg -n 'expand_pattern' src/llm-coding-tools-core/src/permissions.rs | head -20

Repository: Sewer56/llm-coding-tools

Length of output: 201

🏁 Script executed:

#!/bin/bash
# Check the imports at the top of permissions.rs to see where expand_pattern comes from
head -70 src/llm-coding-tools-core/src/permissions.rs | cat -n

Repository: Sewer56/llm-coding-tools

Length of output: 3261

🏁 Script executed:

#!/bin/bash
# Look for the expand_pattern function in all files
fd -e rs -x rg -l 'fn expand_pattern' {}

Repository: Sewer56/llm-coding-tools

Length of output: 130

🏁 Script executed:

#!/bin/bash
# Check tests that use pattern expansion - look around lines 734-797 mentioned in the scratchpad
sed -n '730,800p' src/llm-coding-tools-core/src/permissions.rs

Repository: Sewer56/llm-coding-tools

Length of output: 2614

🏁 Script executed:

#!/bin/bash
# Now examine the expand_pattern function to understand when it returns Err
cat src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs | head -100

Repository: Sewer56/llm-coding-tools

Length of output: 3048

🏁 Script executed:

#!/bin/bash
# Look for expand_pattern's full implementation
rg -n 'pub fn expand_pattern' -A30 src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs

Repository: Sewer56/llm-coding-tools

Length of output: 50

🏁 Script executed:

#!/bin/bash
# Check if there are any tests for expansion failures (e.g. with unset vars)
rg -nP 'expand_pattern|Err\(' src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs | head -30

Repository: Sewer56/llm-coding-tools

Length of output: 775

🏁 Script executed:

#!/bin/bash
# Look for the full expand_pattern_should_return_error_on_failure test
sed -n '192,200p' src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs

Repository: Sewer56/llm-coding-tools

Length of output: 298

🏁 Script executed:

#!/bin/bash
# Check if there are any tests in permissions.rs that test expansion failures in Rule::new
rg -nP 'test.*expansion.*fail|test.*unset.*var|Err.*expand' src/llm-coding-tools-core/src/permissions.rs

Repository: Sewer56/llm-coding-tools

Length of output: 50

🏁 Script executed:

#!/bin/bash
# Check git log to see if there's any discussion about the fallback behavior
git log --oneline -n 20 -- src/llm-coding-tools-core/src/permissions.rs

Repository: Sewer56/llm-coding-tools

Length of output: 147

🏁 Script executed:

#!/bin/bash
# Look at the git commit to understand the design intent
git show e30df5a --stat | head -20

Repository: Sewer56/llm-coding-tools

Length of output: 1150

🏁 Script executed:

#!/bin/bash
# Check if there are any comments or docs explaining why the error is silently ignored
sed -n '125,150p' src/llm-coding-tools-core/src/permissions.rs | grep -A5 -B5 'Err'

Repository: Sewer56/llm-coding-tools

Length of output: 583

🏁 Script executed:

#!/bin/bash
# Verify the exact behavior: create a simple test scenario
# Check how rules are evaluated - do they store the unexpanded pattern?
rg -n 'fn evaluate|pattern_hash' src/llm-coding-tools-core/src/permissions.rs | head -15

Repository: Sewer56/llm-coding-tools

Length of output: 1004

🏁 Script executed:

#!/bin/bash
# Let's verify: if expansion fails, the unexpanded pattern gets stored
# Check if there are any tests that verify this behavior explicitly
rg -nP 'UNSET|expansion.*fail|fallback' -B5 -A5 src/llm-coding-tools-core/src/permissions.rs

Repository: Sewer56/llm-coding-tools

Length of output: 50

🏁 Script executed:

#!/bin/bash
# Double-check the commit message and PR intent
git show e30df5a --format=fuller | head -40

Repository: Sewer56/llm-coding-tools

Length of output: 1358

🏁 Script executed:

#!/bin/bash
# Check if there's any logging or warning infrastructure in place for this
rg -n 'log::|warn!|tracing::' src/llm-coding-tools-core/src/permissions.rs | head -10

Repository: Sewer56/llm-coding-tools

Length of output: 50

🏁 Script executed:

#!/bin/bash
# Let's verify the actual behavior by checking what happens with an unexpanded pattern
# Check if there are any runtime tests or examples that might show this scenario
rg -nP 'HOME.*unset|expansion.*fail|unexpanded' -B3 -A3 src/llm-coding-tools-core/src/

Repository: Sewer56/llm-coding-tools

Length of output: 875

🏁 Script executed:

#!/bin/bash
# Final check: look at the actual matching logic to confirm unexpanded patterns won't match
sed -n '255,290p' src/llm-coding-tools-core/src/permissions.rs

Repository: Sewer56/llm-coding-tools

Length of output: 1345

🏁 Script executed:

#!/bin/bash
# Let's check if there are integration tests that verify the behavior when vars are unset
rg -nP '#\[test\]' -A20 src/llm-coding-tools-core/src/permissions.rs | grep -A20 'unset\|UNSET\|fail'

Repository: Sewer56/llm-coding-tools

Length of output: 50

🏁 Script executed:

#!/bin/bash
# Check the extensions.rs tests to see if they test expansion failures
sed -n '64,150p' src/llm-coding-tools-agents/src/extensions.rs

Repository: Sewer56/llm-coding-tools

Length of output: 2946

Silent fallback on pattern expansion failure may cause permission rules to silently fail matching.

When expand_pattern fails (e.g., $UNSET_VAR or HOME unset), the original unexpanded pattern is stored in the rule. This causes silent mismatches:

  • A rule created with Rule::new("read", "~/workspace/*", Allow) when HOME is unset will store the literal ~/workspace/*
  • When evaluated against a real path like /home/user/workspace/file.txt, it will never match because the literal pattern won't match the expanded path

This is particularly problematic because from_permission_config in extensions.rs passes user-supplied patterns from YAML configuration directly to Rule::new. A config like:

permissions:
  read:
    "~/workspace/*": allow

will silently break if HOME is unset at rule construction time, with no warning to the user.

Consider either:

  1. Logging a warning when expansion fails, so users can diagnose misconfigurations
  2. Returning Result from Rule::new to surface expansion failures to callers (similar to how expand_shell handles errors)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/llm-coding-tools-core/src/permissions.rs` around lines 131 - 136,
Rule::new currently swallows expand_pattern errors and stores the unexpanded
literal, causing silent mismatches; change Rule::new to return Result<Rule,
ExpandError> (or a suitable error type) instead of constructing a Rule on
expansion failure, propagate this Result to callers such as
from_permission_config in extensions.rs, and update call sites to handle or
surface the error so misconfigured patterns (e.g., missing $HOME) are reported
instead of silently failing; ensure you reference expand_pattern, Rule::new, and
from_permission_config when making the signature and call-site changes.

Self {
permission_hash: hash_u64(&permission),
pattern_hash: hash_u64(&pattern),
Expand Down Expand Up @@ -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)],
Expand Down Expand Up @@ -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);
}
}