-
Notifications
You must be signed in to change notification settings - Fork 5.9k
feat: expand the set of commands that can be safely identified as "trusted" #1668
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.
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
Conversation
6866e53 to
18e266e
Compare
`nl` is a line-numbering tool that should be on the _trusted _ list, as there is nothing concerning on https://gtfobins.github.io/gtfobins/nl/ that would merit exclusion. `true` and `false` are also safe, though not particularly useful given how `is_known_safe_command()` works today, but that will change with #1668.
PR SummaryAdds a dedicated ReviewLooks great—clear separation of concerns and stronger safety checks. A few small nits:
Otherwise the implementation and tests are solid—ship it! |
| // a conservative allow‑list of shell operators that themselves do not | ||
| // introduce side effects ( "&&", "||", ";", and "|" ). If every | ||
| // individual command in the script is itself a known‑safe command, then | ||
| // the composite expression is considered safe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reasonable, I like it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR updates
is_known_safe_command()to account for "safe operators" to expand the set of commands that can be run without approval. This concept existed in the TypeScript CLI, and we are [finally!] porting it to the Rust one:codex/codex-cli/src/approvals.ts
Lines 531 to 541 in c9e2def
The idea is that if we have
EXPR1 SAFE_OP EXPR2andEXPR1andEXPR2are considered safe independently, thenEXPR1 SAFE_OP EXPR2should be considered safe. Currently,SAFE_OPincludes&&,||,;, and|.In the TypeScript implementation, we relied on https://www.npmjs.com/package/shell-quote to parse the string of Bash, as it could provide a "lightweight" parse tree, parsing
'beep || boop > /byte'as:Though in this PR, we introduce the use of https://crates.io/crates/tree-sitter-bash for parsing (which incidentally we were already using in
codex-apply-patch), which gives us a richer parse tree. (Incidentally, if you have never played with tree-sitter, try the playground and select Bash from the dropdown to see how it parses various expressions.)As a concrete example, prior to this change, our implementation of
is_known_safe_command()could verify things like:but not:
With this change, the version with
|| trueis also accepted.Admittedly, this PR does not expand the safety check to support subshells, so it would reject, e.g.
bash -lc 'ls || (pwd && echo hi)', but that can be addressed in a subsequent PR.