Expand shell patterns in permission rules at construction time#92
Expand shell patterns in permission rules at construction time#92
Conversation
- 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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
==========================================
+ Coverage 81.44% 81.46% +0.02%
==========================================
Files 106 106
Lines 4343 4349 +6
==========================================
+ Hits 3537 3543 +6
Misses 806 806
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
WalkthroughThe pull request exposes the Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/llm-coding-tools-core/src/permissions.rs (1)
780-797: Consider adding a test for the expansion failure fallback.This test verifies patterns without shell syntax still work (good regression guard), but there's no test verifying the behavior when expansion fails. A test like the following would document the fallback behavior:
#[test] fn rule_with_unset_env_var_stores_unexpanded_pattern() { temp_env::with_var("DEFINITELY_NOT_SET_12345", None::<&str>, || { let rule = Rule::new("read", "$DEFINITELY_NOT_SET_12345/path/*", PermissionAction::Allow); // Documents that unexpanded pattern is stored on failure assert_eq!(rule.pattern(), "$DEFINITELY_NOT_SET_12345/path/*"); }); }This would make the fallback behavior explicit and serve as documentation for future maintainers.
🤖 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 780 - 797, Add a new unit test that verifies the expansion-failure fallback: use temp_env::with_var to unset an env var, create a Rule via Rule::new("read", "$DEFINITELY_NOT_SET_12345/path/*", PermissionAction::Allow), and assert that rule.pattern() returns the original unexpanded string "$DEFINITELY_NOT_SET_12345/path/*" to document that on expansion failure the raw pattern is stored; place the test alongside the other rule tests and name it e.g. rule_with_unset_env_var_stores_unexpanded_pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm-coding-tools-core/src/permissions.rs`:
- Around line 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.
---
Nitpick comments:
In `@src/llm-coding-tools-core/src/permissions.rs`:
- Around line 780-797: Add a new unit test that verifies the expansion-failure
fallback: use temp_env::with_var to unset an env var, create a Rule via
Rule::new("read", "$DEFINITELY_NOT_SET_12345/path/*", PermissionAction::Allow),
and assert that rule.pattern() returns the original unexpanded string
"$DEFINITELY_NOT_SET_12345/path/*" to document that on expansion failure the raw
pattern is stored; place the test alongside the other rule tests and name it
e.g. rule_with_unset_env_var_stores_unexpanded_pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea23cbe5-3caf-432c-bc4c-1020180b3e2f
📒 Files selected for processing (4)
src/llm-coding-tools-core/src/path/allowed_glob/mod.rssrc/llm-coding-tools-core/src/path/allowed_glob/normalize.rssrc/llm-coding-tools-core/src/path/mod.rssrc/llm-coding-tools-core/src/permissions.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Semver Checks (Serdesai Full+Linux)
- GitHub Check: Semver Checks (Serdesai Full)
- GitHub Check: Async macOS
- GitHub Check: Async Linux
- GitHub Check: Blocking Windows
- GitHub Check: Blocking macOS
- GitHub Check: Async Windows
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:04.465Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
📚 Learning: 2026-03-28T02:14:04.465Z
Learnt from: Sewer56
Repo: Sewer56/llm-coding-tools PR: 69
File: src/llm-coding-tools-bubblewrap/src/profile/validation.rs:57-67
Timestamp: 2026-03-28T02:14:04.465Z
Learning: In `src/llm-coding-tools-bubblewrap/src/profile/` (Rust, llm-coding-tools-bubblewrap crate), the `Builder` API paths (workspace, synthetic_home, cache_root, mount lists, overlays, etc.) are always set by trusted application/operator code — the library consumer is the trusted party. Path normalization and `..`-component hardening in validators like `validate_absolute_path` is therefore NOT required to defend against traversal attacks. Untrusted input (LLM-generated shell commands) only enters through `wrap_command`/`execute_command_with_mode`, not through the `Builder`.
Applied to files:
src/llm-coding-tools-core/src/path/allowed_glob/mod.rssrc/llm-coding-tools-core/src/path/mod.rssrc/llm-coding-tools-core/src/permissions.rssrc/llm-coding-tools-core/src/path/allowed_glob/normalize.rs
🔇 Additional comments (9)
src/llm-coding-tools-core/src/path/allowed_glob/normalize.rs (4)
29-37: LGTM! Clean extraction of the pattern expansion logic.The new
expand_patternfunction correctly wrapsshellexpand::fulland exposes theCowreturn type, enabling zero-allocation passthrough when no shell metacharacters are present. This is a good design for reuse across modules.
43-52: LGTM! Clean refactor using the new helper.The function correctly delegates to
expand_patternand handles the conversion toPathBuf. The error message now includes more context about what failed.
130-151: Good coverage for tilde expansion.The
#[cfg(not(windows))]guard is appropriate since tilde expansion relies on the UnixHOMEenvironment variable behavior.
175-189: Nice use of parameterized tests for the zero-allocation passthrough.The rstest cases effectively verify that patterns without shell metacharacters return
Cow::Borrowed, confirming the optimization works as intended.src/llm-coding-tools-core/src/path/allowed_glob/mod.rs (1)
6-6: LGTM! Visibility change enables crate-internal reuse.The
pub(crate)visibility is appropriate for sharingexpand_patternwithpermissions.rswhile keeping it internal to the crate.src/llm-coding-tools-core/src/path/mod.rs (1)
10-10: LGTM! Required visibility adjustment for the module hierarchy.This change, along with the
pub(crate) mod normalize;change, establishes the import pathcrate::path::allowed_glob::normalize::expand_patternused inpermissions.rs.src/llm-coding-tools-core/src/permissions.rs (3)
57-59: LGTM! Necessary imports for the expansion integration.
738-755: Good test coverage for tilde expansion when HOME is set.The test correctly verifies that a
~/projects/*pattern matches the expanded path.
757-778: Good test coverage for $HOME expansion.The test correctly verifies that
$HOME/.config/*expands and matches real paths.
| 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, | ||
| }; |
There was a problem hiding this comment.
🧩 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*\(' -C2Repository: 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.rsRepository: 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.rsRepository: 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.rsRepository: 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 -20Repository: 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 -nRepository: 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.rsRepository: 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 -100Repository: 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.rsRepository: 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 -30Repository: 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.rsRepository: 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.rsRepository: 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.rsRepository: 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 -20Repository: 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 -15Repository: 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.rsRepository: 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 -40Repository: 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 -10Repository: 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.rsRepository: 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.rsRepository: 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)whenHOMEis 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/*": allowwill silently break if HOME is unset at rule construction time, with no warning to the user.
Consider either:
- Logging a warning when expansion fails, so users can diagnose misconfigurations
- Returning
ResultfromRule::newto surface expansion failures to callers (similar to howexpand_shellhandles 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.
1 participant
Summary
~/,$HOME/,$VAR,${VAR:-default}) in permission rules are now expanded eagerly when aRuleis constructed, rather than being stored as raw patterns that may never match real paths.expand_pattern()function innormalize.rsthat returnsCow<str>(zero-allocation for patterns without shell metacharacters), withexpand_shell()reimplemented as a thin wrapper.$HOME, and plain patterns all evaluate correctly through the ruleset.Changed files
permissions.rsRule::new; add integration testsnormalize.rsexpand_pattern(), refactor & add unit testsmod.rs(path, allowed_glob)pub(crate)for cross-module access