execpolicy: unwrap PowerShell -Command wrappers on Windows#20336
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6042b542e
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ 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". |
| cmd, | ||
| sandbox_permissions, | ||
| used_complex_parsing, | ||
| crate::exec_policy::UnmatchedCommandContext { |
There was a problem hiding this comment.
this struct was introduced to fix a clippy warning about too many parameters due to me adding one new one (command_origin)
| #[cfg(test)] | ||
| #[path = "exec_policy_tests.rs"] | ||
| mod tests; |
There was a problem hiding this comment.
Can you create a separate file exec_policy_windows_tests.rs and include it here in the same way, but with #[cfg(all(windows, test))] so you don't have to #[cfg(windows)] in exec_policy_windows_tests itself?
| fn commands_for_exec_policy(command: &[String]) -> (Vec<Vec<String>>, bool) { | ||
| fn commands_for_exec_policy( | ||
| command: &[String], | ||
| ) -> (Vec<Vec<String>>, bool, ExecPolicyCommandOrigin) { |
There was a problem hiding this comment.
I take responsibility for the original tuple return value being bad, so since you're touching this code, can you declare a small struct before this function to use as a return value and return that?
| use codex_protocol::permissions::FileSystemSandboxPolicy; | ||
| use codex_protocol::protocol::AskForApproval; | ||
| use codex_shell_command::is_dangerous_command::command_might_be_dangerous; | ||
| #[cfg(windows)] |
There was a problem hiding this comment.
If you only use the function in one block in this file and that block already has #[cfg(windows)], I would favor doing the import in that block so the top-level imports are less noisy.
We should add that to AGENTS.md...
| ]; | ||
|
|
||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| pub(crate) enum ExecPolicyCommandOrigin { |
There was a problem hiding this comment.
This needs a docstring because I'm a little fuzzy on what this means.
ExecPolicyCommandOrigin::PowerShell to me implies the command was targeted to be run under PowerShell, but looking at the code, it feels like it actually means "the type of shell parser that was used to parse the command." It's possible the user is on Windows but could be using a shell that isn't PowerShell, isn't it? Or no?
To that end, I'm not clear why the other variant is named Direct.
| sandbox_permissions: SandboxPermissions::UseDefault, | ||
| prefix_rule: None, | ||
| }, | ||
| ExecApprovalRequirement::Forbidden { |
There was a problem hiding this comment.
I'm confused, why isn't this ExecApprovalRequirement::NeedsApproval?
|
|
||
| #[tokio::test] | ||
| async fn execpolicy_blocks_shell_invocation() -> Result<()> { | ||
| // TODO execpolicy doesn't parse powershell commands yet |
There was a problem hiding this comment.
Well, that explains that...
| /// This is intentionally narrower than the Windows safe-command parser: it only unwraps the | ||
| /// `-Command`/`-c` body from a PowerShell invocation we already recognize, then delegates the | ||
| /// script itself to the PowerShell AST parser. | ||
| pub fn parse_powershell_command_plain_commands(command: &[String]) -> Option<Vec<Vec<String>>> { |
There was a problem hiding this comment.
Maybe parse_powershell_command_into_plain_commands()?
| let (shell, script) = extract_powershell_command(command)?; | ||
| try_parse_powershell_ast_commands(shell, script) |
There was a problem hiding this comment.
I think executable is the more appropriate name here.
| let (shell, script) = extract_powershell_command(command)?; | |
| try_parse_powershell_ast_commands(shell, script) | |
| let (executable, script) = extract_powershell_command(command)?; | |
| try_parse_powershell_ast_commands(executable, script) |
| #[cfg(windows)] | ||
| #[test] | ||
| fn parses_plain_powershell_commands() { | ||
| let commands = parse_powershell_command_plain_commands(&[ |
There was a problem hiding this comment.
Also add a test for a case where commands has more than one entry?
Why
On Windows, Codex runs shell commands through a top-level
powershell.exe -NoProfile -Command ...wrapper.execpolicywas matching that wrapper instead of the inner command, so prefix rules like["git", "push"]did not fire for PowerShell-wrapped commands even though the same normalization already happens forbash -lcon Unix.This change makes the Windows shell wrapper transparent to rule matching while preserving the existing Windows unmatched-command safelist and dangerous-command heuristics.
What changed
parse_powershell_command_plain_commands()inshell-command/src/powershell.rsto unwrap the top-level PowerShell-Commandbody withextract_powershell_command()and parse it with the existing PowerShell AST parsercore/src/exec_policy.rssocommands_for_exec_policy()treats top-level PowerShell wrappers likebash -lcand evaluates rules against the parsed inner commandsExecPolicyCommandOriginthrough unmatched-command evaluation and exposeis_safe_powershell_words()/is_dangerous_powershell_words()so Windows safelist and dangerous-command checks still work after unwrapexecpolicy_blocks_shell_invocationtest on WindowsTesting
cargo test -p codex-shell-command