Harden allowlist-only process execution - #621
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cac1318451
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if needs_shell { | ||
| return Ok(ValidationResult::Blocked { | ||
| reason: "Shell features are not permitted in allowlist_only mode; use the direct-exec form with an explicit argument list" |
There was a problem hiding this comment.
When allowlist_only is used on Windows, a direct raw command such as execute command "C:\\Tools\\tool.exe" is classified as needs_shell before this call because contains_shell_metacharacters treats backslashes as shell metacharacters; this new unconditional block then rejects it before the canonical path allowlist check can run, even if allowed_shell_commands contains that same executable path. That makes the documented explicit-path allowlist unusable for the common Windows path form unless callers add a dummy with arguments list or rewrite paths with /, so this should distinguish path separators from actual shell syntax before blocking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR hardens WFL’s allowlist_only subprocess policy by ensuring allowlists authorize only direct executable launches (not entire shell command lines), tightening name-vs-path matching semantics, and updating tests/docs to reflect the stronger boundary.
Changes:
- Block shell-backed syntax (chaining/pipes/redirects/substitution/etc.) in
allowlist_only, requiring direct exec with an explicit argument list. - Distinguish name-only allowlist entries from explicit paths; for path entries, require canonical path equality to the invoked executable.
- Update Windows allowlist-only tests to avoid allowlisting a shell (
cmd.exe) and document the new boundaries.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/interpreter/command_sanitizer.rs |
Enforces “direct-exec only” in allowlist_only, tightens allowlist matching rules, and adds regression tests. |
tests/subprocess_security_test.rs |
Updates Windows fixture to use a non-shell allowlisted program and adds an injection/chaining regression. |
Docs/reference/configuration-reference.md |
Documents name-vs-path allowlist semantics and the “no shell features in allowlist_only” rule. |
Docs/04-advanced-features/subprocess-execution.md |
Adds user-facing guidance about allowlist-only boundaries and interpreter allowlisting risks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn is_allowlisted(&self, command: &str) -> bool { | ||
| let base_command = Self::program_basename(&self.get_command_base(command)); | ||
| self.is_program_allowlisted(&base_command) | ||
| if Self::contains_shell_metacharacters(command) { | ||
| return false; | ||
| } |
| fn is_program_allowlisted(&self, program: &str) -> bool { | ||
| let program_has_path = Self::has_path_syntax(program); | ||
| self.config.allowed_shell_commands.iter().any(|allowed| { | ||
| let allowed_has_path = Self::has_path_syntax(allowed); | ||
|
|
||
| // A name-only entry delegates resolution to the trusted process | ||
| // environment's PATH. It must not also authorize a caller-selected | ||
| // executable at `./name`, `/tmp/name`, or another explicit path. | ||
| if program_has_path != allowed_has_path { | ||
| return false; | ||
| } | ||
| if program_has_path { | ||
| let Ok(program_path) = std::fs::canonicalize(program) else { | ||
| return false; | ||
| }; | ||
| let Ok(allowed_path) = std::fs::canonicalize(allowed) else { | ||
| return false; | ||
| }; | ||
|
|
||
| #[cfg(windows)] | ||
| { | ||
| return program_path | ||
| .to_string_lossy() | ||
| .eq_ignore_ascii_case(&allowed_path.to_string_lossy()); | ||
| } |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if needs_shell { | ||
| return Ok(ValidationResult::Blocked { | ||
| reason: "Shell features are not permitted in allowlist_only mode; use the direct-exec form with an explicit argument list" | ||
| .to_string(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔍 Windows explicit-path commands in the string form are unreachable in allowlist_only
The docs now advertise that explicit executable paths can be allowlisted in allowlist_only mode (e.g. allowed_shell_commands = C:\tools\where.exe). However, needs_shell is derived from CommandSanitizer::contains_shell_metacharacters(command) (see src/interpreter/mod.rs:1683-1684), and backslash \ is in the metacharacter set (src/interpreter/command_sanitizer.rs:147-150). Consequently, a Windows string-form invocation such as execute command "C:\tools\prog.exe" (empty argument list) always sets needs_shell = true and is rejected by the new AllowlistOnly shell-feature block (command_sanitizer.rs:202-207) before the path allowlist check runs. The path-allowlist feature therefore only works via the explicit with arguments [...] (argv) form on Windows, since that form sets args non-empty and keeps needs_shell = false. This is a platform-specific usability gap, not a correctness/security bug (argv form works, and the code in mod.rs is outside this PR's diff), but reviewers may want to confirm the docs make the argv-only requirement explicit for Windows explicit paths.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ShellExecutionMode::AllowlistOnly => { | ||
| if self.is_program_allowlisted(&program_base) { | ||
| if needs_shell { | ||
| Ok(ValidationResult::RequiresShell { | ||
| reason: "Command is allowlisted".to_string(), | ||
| warnings: vec!["Using shell execution (allowlisted)".to_string()], | ||
| }) | ||
| } else { | ||
| Ok(ValidationResult::Safe) | ||
| } | ||
| // An allowlist can authorize one executable, but it cannot | ||
| // safely authorize an entire shell command line. If shell | ||
| // parsing is allowed here, a command such as | ||
| // `echo safe; unlisted-program` passes the `echo` check and the | ||
| // shell executes both commands. Require the argv/direct-exec | ||
| // form in this mode; callers that intentionally need pipes, | ||
| // redirects, expansion, or chaining must opt into `sanitized` | ||
| // or `unrestricted` explicitly. | ||
| if needs_shell { | ||
| return Ok(ValidationResult::Blocked { | ||
| reason: "Shell features are not permitted in allowlist_only mode; use the direct-exec form with an explicit argument list" | ||
| .to_string(), | ||
| }); | ||
| } | ||
|
|
||
| if self.is_program_allowlisted(program) { | ||
| Ok(ValidationResult::Safe) | ||
| } else { | ||
| Ok(ValidationResult::Blocked { | ||
| reason: format!( | ||
| "Program '{}' is not in the allowlist (allowed_shell_commands)", | ||
| program_base | ||
| program | ||
| ), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔍 Non-trivial security behavior change without a Dev Diary entry
This PR is a non-trivial security/behavior change (allowlist_only now rejects all shell-backed forms and binds path entries to a canonical executable). CLAUDE.md/AGENTS.md state that non-trivial behavior changes should ship a Dev diary/ entry in the same change. The diff does not add a new Dev Diary entry, though related prior entries exist (e.g. Dev diary/2026-07-11-subprocess-policy-enforcement.md, 2026-07-13-issue-610-phase-1-*). Reviewers should confirm whether an existing entry sufficiently covers this hardening step or whether a new one is expected per the documentation policy.
(Refers to lines 193-219)
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Superseded by #632, which preserves this security fix in the consolidated Rust-source hardening PR. The combined head is mergeable and all required CI checks are green. |
Summary
allowlist_onlyauthorize direct executable launches onlyechofrom authorizing./echo,/tmp/echo, or another explicit path with the same basenamecmd.exewith a non-shell executableSecurity impact
The previous policy checked only the first program token and then permitted the entire command through a shell. An allowlisted prefix could therefore launch additional, unlisted commands. It also reduced explicit executable paths to their basename, so an attacker-controlled binary at another path inherited the authority of a trusted name. The policy now rejects shell parsing in
allowlist_only, distinguishes name-only execution from explicit paths, and binds path entries to the same canonical executable.Validation
git diff --checkProduction readiness
allowlist_onlymust move to explicit arguments or deliberately opt intosanitized/unrestricted; explicit executable paths must be allowlisted explicitly