core: add potentially dangerous command check#4211
Conversation
![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.
ℹ️ 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
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
We will never be able to enumerate all dangerous commands: please focus on matching safe/trusted commands.
Codex has certain tendencies for certain dangerous commands. While it's true that it's impossible to ban all dangerous commands, just approving on "rm -rf" and "git reset --hard" would go a long way towards blunting some of the most egregious patterns it gets into when frustrated. |
…nd, not just prefix.
| if let [bash, flag, script] = command | ||
| && bash == "bash" | ||
| && flag == "-lc" | ||
| && let Some(tree) = try_parse_bash(script) | ||
| && let Some(all_commands) = try_parse_word_only_commands_sequence(&tree, script) | ||
| && all_commands | ||
| .iter() | ||
| .any(|cmd| contains_dangerous_command(cmd)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| false |
There was a problem hiding this comment.
I know this code exists elsewhere: please refactor it for reuse.
| return false; | ||
| } | ||
|
|
||
| let command_string = command.join(" "); |
There was a problem hiding this comment.
Regular expressions are not good ways to match commands. For example, you have "git reset" but not "git reset" (two spaces instead of one).
Use the same strategy we do for trusted commands.
| && let Some(tree) = try_parse_bash(script) | ||
| && let Some(all_commands) = try_parse_word_only_commands_sequence(&tree, script) |
There was a problem hiding this comment.
This seems to "fail deadly". If we aren't able to parse the script, should we presume it to be dangerous, rather than assuming it to be safe?
| && let Some(all_commands) = try_parse_word_only_commands_sequence(&tree, script) | ||
| && all_commands | ||
| .iter() | ||
| .any(|cmd| contains_dangerous_command(cmd)) |
There was a problem hiding this comment.
I also don't think that this would catch: bash -lc "bash -lc \"dangerous_command\"". Should this be recursive in any way? Are we okay with a bit of a leaky sieve here since we also have the sandbox as a backstop?
fouad-openai
changed the title
| if command_might_be_dangerous(command) && !approved.contains(command) { | ||
| return SafetyCheck::AskUser; | ||
| } |
There was a problem hiding this comment.
If approval_policy is AskForApproval::Never, then I believe this should return SafetyCheck::Reject with an appropriate reason.
For example, Never might be selected because Codex is being used in a non-interactive way (like codex exec), so the agent loop would never get a response.
5 participants
Certain shell commands are potentially dangerous, and we want to check for them.
Unless the user has explicitly approved a command, we will always ask them for approval
when one of these commands is encountered, regardless of whether they are in a sandbox, or what their approval policy is.
The first (of probably many) such examples is
git reset --hard. We will be conservative and check for anygit reset