fix: consolidate logic related to checking list of safe commands #7958
Conversation
I noted that the logic for determining whether an `rg` or `git` invocation was safe diverged between `is_safe_command.rs` and `windows_safe_commands.rs`, so this PR reconciles the two in a shared `shared_rules.rs` file.
|
@codex review |
![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 "@codex address that feedback".
| if arg.starts_with('-') { | ||
| if arg.eq_ignore_ascii_case("-c") || arg.eq_ignore_ascii_case("--config") { | ||
| if iter.next().is_none() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The shared Git safelist now accepts -c/--config without validating which config keys are being set. Because Git uses config values to launch external programs (e.g., core.pager or diff.external during log/show/diff), a command like git -c core.pager=sh -c '...' log will be treated as safe and skip approval. This is a regression for non‑Windows callers where any leading option previously made the command unsafe. Consider rejecting -c/--config here or whitelisting only known‑safe keys.
Useful? React with 👍 / 👎.
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 "@codex address that feedback".
| if arg.eq_ignore_ascii_case("-c") || arg.eq_ignore_ascii_case("--config") { | ||
| if iter.next().is_none() { | ||
| return false; | ||
| } | ||
| continue; |
There was a problem hiding this comment.
Because Unix now uses is_safe_git_command, this helper explicitly allows -c/--config before a whitelisted subcommand. git -h documents that -c <name>=<value> overrides config and -p/--paginate forces a pager, so an invocation like git -c core.pager=/usr/bin/env -p log can spawn an arbitrary executable while still being treated as “safe.” Previously, any pre-subcommand option (including -c) caused Unix commands to be rejected, so this is a security regression. Consider disallowing -c/--config (and --config-env) or whitelisting specific keys/values instead.
Useful? React with 👍 / 👎.
I noted that the logic for determining whether an
rgorgitinvocation wassafe diverged between
is_safe_command.rsandwindows_safe_commands.rs, sothis PR reconciles the two in a shared
shared_rules.rsfile.