From 4f96001fa795670412ce82808c9c877050f9d281 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Thu, 30 Apr 2026 17:56:20 -0700 Subject: [PATCH] execpolicy: unwrap PowerShell -Command wrappers on Windows (#20336) ## Why On Windows, Codex runs shell commands through a top-level `powershell.exe -NoProfile -Command ...` wrapper. `execpolicy` was 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 for `bash -lc` on 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 - add `parse_powershell_command_plain_commands()` in `shell-command/src/powershell.rs` to unwrap the top-level PowerShell `-Command` body with `extract_powershell_command()` and parse it with the existing PowerShell AST parser - update `core/src/exec_policy.rs` so `commands_for_exec_policy()` treats top-level PowerShell wrappers like `bash -lc` and evaluates rules against the parsed inner commands - carry a small `ExecPolicyCommandOrigin` through unmatched-command evaluation and expose `is_safe_powershell_words()` / `is_dangerous_powershell_words()` so Windows safelist and dangerous-command checks still work after unwrap - add Windows-focused tests for wrapped PowerShell prompt/allow matches, wrapper parsing, and unmatched safe/dangerous inner commands, and re-enable the end-to-end `execpolicy_blocks_shell_invocation` test on Windows ## Testing - `cargo test -p codex-shell-command` --- codex-rs/core/src/exec_policy.rs | 130 +++++++++++++++--- codex-rs/core/src/exec_policy_tests.rs | 103 +++++++++----- .../core/src/exec_policy_windows_tests.rs | 125 +++++++++++++++++ .../tools/runtimes/shell/unix_escalation.rs | 15 +- codex-rs/core/tests/suite/exec_policy.rs | 5 - .../command_safety/is_dangerous_command.rs | 26 ++++ .../src/command_safety/is_safe_command.rs | 28 ++++ .../shell-command/src/command_safety/mod.rs | 1 + .../src/command_safety/powershell_parser.rs | 10 ++ .../windows_dangerous_commands.rs | 15 +- .../command_safety/windows_safe_commands.rs | 4 +- codex-rs/shell-command/src/powershell.rs | 49 +++++++ 12 files changed, 434 insertions(+), 77 deletions(-) create mode 100644 codex-rs/core/src/exec_policy_windows_tests.rs diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index d09266636..57e85cece 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -98,6 +98,43 @@ static BANNED_PREFIX_SUGGESTIONS: &[&[&str]] = &[ &["osascript"], ]; +/// Describes which unmatched-command heuristics should classify the command +/// words being evaluated by exec-policy. +/// +/// The command tokens may be the original argv or a shell-specific lowering of +/// a wrapper such as `bash -lc ...` or `powershell.exe -Command ...`. We only +/// need to distinguish the PowerShell case because its safelist and dangerous +/// heuristics operate on PowerShell-flavored inner command words rather than +/// the generic command classifier. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum ExecPolicyCommandOrigin { + /// Use the generic unmatched-command heuristics. + Generic, + #[cfg(windows)] + /// The command words came from the `-Command` body of a top-level + /// PowerShell wrapper, so use PowerShell-specific unmatched-command + /// heuristics for the lowered words. + PowerShell, +} + +#[derive(Clone, Copy)] +pub(crate) struct UnmatchedCommandContext<'a> { + pub(crate) approval_policy: AskForApproval, + pub(crate) permission_profile: &'a PermissionProfile, + pub(crate) file_system_sandbox_policy: &'a FileSystemSandboxPolicy, + pub(crate) sandbox_cwd: &'a Path, + pub(crate) sandbox_permissions: SandboxPermissions, + pub(crate) used_complex_parsing: bool, + pub(crate) command_origin: ExecPolicyCommandOrigin, +} + +#[derive(Debug, Eq, PartialEq)] +struct ExecPolicyCommands { + commands: Vec>, + used_complex_parsing: bool, + command_origin: ExecPolicyCommandOrigin, +} + pub(crate) fn child_uses_parent_exec_policy(parent_config: &Config, child_config: &Config) -> bool { fn exec_policy_config_folders(config: &Config) -> Vec { config @@ -246,20 +283,27 @@ impl ExecPolicyManager { prefix_rule, } = req; let exec_policy = self.current(); - let (commands, used_complex_parsing) = commands_for_exec_policy(command); + let ExecPolicyCommands { + commands, + used_complex_parsing, + command_origin, + } = commands_for_exec_policy(command); // Keep heredoc prefix parsing for rule evaluation so existing // allow/prompt/forbidden rules still apply, but avoid auto-derived // amendments when only the heredoc fallback parser matched. let auto_amendment_allowed = !used_complex_parsing; let exec_policy_fallback = |cmd: &[String]| { render_decision_for_unmatched_command( - approval_policy, - &permission_profile, - file_system_sandbox_policy, - sandbox_cwd, cmd, - sandbox_permissions, - used_complex_parsing, + UnmatchedCommandContext { + approval_policy, + permission_profile: &permission_profile, + file_system_sandbox_policy, + sandbox_cwd, + sandbox_permissions, + used_complex_parsing, + command_origin, + }, ) }; let match_options = MatchOptions { @@ -581,16 +625,27 @@ pub async fn load_exec_policy(config_stack: &ConfigLayerStack) -> Result, ) -> Decision { - if is_known_safe_command(command) && !used_complex_parsing { + let UnmatchedCommandContext { + approval_policy, + permission_profile, + file_system_sandbox_policy, + sandbox_cwd, + sandbox_permissions, + used_complex_parsing, + command_origin, + } = context; + let is_known_safe = match command_origin { + ExecPolicyCommandOrigin::Generic => is_known_safe_command(command), + #[cfg(windows)] + ExecPolicyCommandOrigin::PowerShell => { + codex_shell_command::is_safe_command::is_safe_powershell_words(command) + } + }; + if is_known_safe && !used_complex_parsing { return Decision::Allow; } @@ -609,7 +664,14 @@ pub fn render_decision_for_unmatched_command( // We prefer to prompt the user rather than outright forbid the command, // but if the user has explicitly disabled prompts, we must // forbid the command. - if command_might_be_dangerous(command) || environment_lacks_sandbox_protections { + let command_is_dangerous = match command_origin { + ExecPolicyCommandOrigin::Generic => command_might_be_dangerous(command), + #[cfg(windows)] + ExecPolicyCommandOrigin::PowerShell => { + codex_shell_command::is_dangerous_command::is_dangerous_powershell_words(command) + } + }; + if command_is_dangerous || environment_lacks_sandbox_protections { return match approval_policy { AskForApproval::Never => { let sandbox_is_explicitly_disabled = matches!( @@ -637,7 +699,7 @@ pub fn render_decision_for_unmatched_command( Decision::Allow } AskForApproval::UnlessTrusted => { - // We already checked `is_known_safe_command(command)` and it + // We already checked the unmatched-command safelist and it // returned false, so we must prompt. Decision::Prompt } @@ -698,18 +760,44 @@ fn default_policy_path(codex_home: &Path) -> PathBuf { codex_home.join(RULES_DIR_NAME).join(DEFAULT_POLICY_FILE) } -fn commands_for_exec_policy(command: &[String]) -> (Vec>, bool) { +fn commands_for_exec_policy(command: &[String]) -> ExecPolicyCommands { if let Some(commands) = parse_shell_lc_plain_commands(command) && !commands.is_empty() { - return (commands, false); + return ExecPolicyCommands { + commands, + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::Generic, + }; + } + + #[cfg(windows)] + { + if let Some(commands) = + codex_shell_command::powershell::parse_powershell_command_into_plain_commands(command) + && !commands.is_empty() + { + return ExecPolicyCommands { + commands, + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::PowerShell, + }; + } } if let Some(single_command) = parse_shell_lc_single_command_prefix(command) { - return (vec![single_command], true); + return ExecPolicyCommands { + commands: vec![single_command], + used_complex_parsing: true, + command_origin: ExecPolicyCommandOrigin::Generic, + }; } - (vec![command.to_vec()], false) + ExecPolicyCommands { + commands: vec![command.to_vec()], + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::Generic, + } } /// Derive a proposed execpolicy amendment when a command requires user approval diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index 2db07a0fc..9463b85f8 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -34,6 +34,10 @@ use tempfile::TempDir; use tempfile::tempdir; use toml::Value as TomlValue; +#[cfg(windows)] +#[path = "exec_policy_windows_tests.rs"] +mod windows_tests; + fn config_stack_for_dot_codex_folder(dot_codex_folder: &Path) -> ConfigLayerStack { let dot_codex_folder = AbsolutePathBuf::from_absolute_path(dot_codex_folder).expect("absolute dot_codex_folder"); @@ -660,7 +664,14 @@ async fn evaluates_bash_lc_inner_commands() { fn commands_for_exec_policy_falls_back_for_empty_shell_script() { let command = vec!["bash".to_string(), "-lc".to_string(), "".to_string()]; - assert_eq!(commands_for_exec_policy(&command), (vec![command], false)); + assert_eq!( + commands_for_exec_policy(&command), + ExecPolicyCommands { + commands: vec![command], + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::Generic, + } + ); } #[test] @@ -671,7 +682,14 @@ fn commands_for_exec_policy_falls_back_for_whitespace_shell_script() { " \n\t ".to_string(), ]; - assert_eq!(commands_for_exec_policy(&command), (vec![command], false)); + assert_eq!( + commands_for_exec_policy(&command), + ExecPolicyCommands { + commands: vec![command], + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::Generic, + } + ); } #[tokio::test] @@ -961,19 +979,24 @@ fn unmatched_granular_policy_still_prompts_for_restricted_sandbox_escalation() { assert_eq!( Decision::Prompt, render_decision_for_unmatched_command( - AskForApproval::Granular(GranularApprovalConfig { - sandbox_approval: true, - rules: true, - skill_approval: true, - request_permissions: true, - mcp_elicitations: true, - }), - &permission_profile_from_sandbox_policy(&SandboxPolicy::new_read_only_policy()), - &read_only_file_system_sandbox_policy(), - Path::new("/tmp"), &command, - SandboxPermissions::RequireEscalated, - /*used_complex_parsing*/ false, + UnmatchedCommandContext { + approval_policy: AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: true, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + permission_profile: &permission_profile_from_sandbox_policy( + &SandboxPolicy::new_read_only_policy(), + ), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), + sandbox_cwd: Path::new("/tmp"), + sandbox_permissions: SandboxPermissions::RequireEscalated, + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::Generic, + }, ) ); } @@ -986,13 +1009,16 @@ fn unmatched_on_request_uses_split_filesystem_policy_for_escalation_prompts() { assert_eq!( Decision::Prompt, render_decision_for_unmatched_command( - AskForApproval::OnRequest, - &PermissionProfile::Disabled, - &restricted_file_system_policy, - Path::new("/tmp"), &command, - SandboxPermissions::RequireEscalated, - /*used_complex_parsing*/ false, + UnmatchedCommandContext { + approval_policy: AskForApproval::OnRequest, + permission_profile: &PermissionProfile::Disabled, + file_system_sandbox_policy: &restricted_file_system_policy, + sandbox_cwd: Path::new("/tmp"), + sandbox_permissions: SandboxPermissions::RequireEscalated, + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::Generic, + }, ) ); } @@ -1976,10 +2002,20 @@ struct ExecApprovalRequirementScenario { prefix_rule: Option>, } -async fn assert_exec_approval_requirement_for_command( +fn policy_from_src(policy_src: Option<&str>) -> Arc { + match policy_src { + Some(src) => { + let mut parser = PolicyParser::new(); + parser.parse("test.rules", src).expect("parse policy"); + Arc::new(parser.build()) + } + None => Arc::new(Policy::empty()), + } +} + +async fn exec_approval_requirement_for_command( test: ExecApprovalRequirementScenario, - expected_requirement: ExecApprovalRequirement, -) { +) -> ExecApprovalRequirement { let ExecApprovalRequirementScenario { policy_src, command, @@ -1990,19 +2026,10 @@ async fn assert_exec_approval_requirement_for_command( prefix_rule, } = test; - let policy = match policy_src { - Some(src) => { - let mut parser = PolicyParser::new(); - parser - .parse("test.rules", src.as_str()) - .expect("parse policy"); - Arc::new(parser.build()) - } - None => Arc::new(Policy::empty()), - }; + let policy = policy_from_src(policy_src.as_deref()); let permission_profile = permission_profile_from_sandbox_policy(&sandbox_policy); - let requirement = ExecPolicyManager::new(policy) + ExecPolicyManager::new(policy) .create_exec_approval_requirement_for_command(ExecApprovalRequest { command: &command, approval_policy, @@ -2012,8 +2039,14 @@ async fn assert_exec_approval_requirement_for_command( sandbox_permissions, prefix_rule, }) - .await; + .await +} +async fn assert_exec_approval_requirement_for_command( + test: ExecApprovalRequirementScenario, + expected_requirement: ExecApprovalRequirement, +) { + let requirement = exec_approval_requirement_for_command(test).await; assert_eq!(requirement, expected_requirement); } diff --git a/codex-rs/core/src/exec_policy_windows_tests.rs b/codex-rs/core/src/exec_policy_windows_tests.rs new file mode 100644 index 000000000..c1552f7e1 --- /dev/null +++ b/codex-rs/core/src/exec_policy_windows_tests.rs @@ -0,0 +1,125 @@ +use super::*; +use pretty_assertions::assert_eq; +use std::path::Path; + +#[tokio::test] +async fn evaluates_powershell_inner_commands_against_prompt_rules() { + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: Some(r#"prefix_rule(pattern=["echo"], decision="prompt")"#.to_string()), + command: vec![ + "powershell.exe".to_string(), + "-NoProfile".to_string(), + "-Command".to_string(), + "echo blocked".to_string(), + ], + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + ExecApprovalRequirement::Forbidden { + reason: PROMPT_CONFLICT_REASON.to_string(), + }, + ) + .await; +} + +#[tokio::test] +async fn evaluates_powershell_inner_commands_against_allow_rules() { + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: Some(r#"prefix_rule(pattern=["echo"], decision="allow")"#.to_string()), + command: vec![ + "powershell.exe".to_string(), + "-NoProfile".to_string(), + "-Command".to_string(), + "echo blocked".to_string(), + ], + approval_policy: AskForApproval::UnlessTrusted, + sandbox_policy: SandboxPolicy::new_read_only_policy(), + file_system_sandbox_policy: read_only_file_system_sandbox_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + }, + ) + .await; +} + +#[test] +fn commands_for_exec_policy_parses_powershell_shell_wrapper() { + let command = vec![ + "powershell.exe".to_string(), + "-NoProfile".to_string(), + "-Command".to_string(), + "echo blocked".to_string(), + ]; + + assert_eq!( + commands_for_exec_policy(&command), + ExecPolicyCommands { + commands: vec![vec!["echo".to_string(), "blocked".to_string()]], + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::PowerShell, + } + ); +} + +#[test] +fn unmatched_safe_powershell_words_are_allowed() { + let command = vec!["Get-Content".to_string(), "Cargo.toml".to_string()]; + + assert_eq!( + Decision::Allow, + render_decision_for_unmatched_command( + &command, + UnmatchedCommandContext { + approval_policy: AskForApproval::UnlessTrusted, + permission_profile: &permission_profile_from_sandbox_policy( + &SandboxPolicy::new_read_only_policy(), + ), + file_system_sandbox_policy: &read_only_file_system_sandbox_policy(), + sandbox_cwd: Path::new("/tmp"), + sandbox_permissions: SandboxPermissions::UseDefault, + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::PowerShell, + }, + ) + ); +} + +#[tokio::test] +async fn unmatched_dangerous_powershell_inner_commands_require_approval() { + let inner_command = vec![ + "Remove-Item".to_string(), + "test".to_string(), + "-Force".to_string(), + ]; + + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: None, + command: vec![ + "powershell.exe".to_string(), + "-NoProfile".to_string(), + "-Command".to_string(), + "Remove-Item test -Force".to_string(), + ], + approval_policy: AskForApproval::OnRequest, + sandbox_policy: SandboxPolicy::DangerFullAccess, + file_system_sandbox_policy: unrestricted_file_system_sandbox_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(inner_command)), + }, + ) + .await; +} diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 41aa552d8..fef8db5ca 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -674,13 +674,16 @@ fn evaluate_intercepted_exec_policy( let fallback = |cmd: &[String]| { crate::exec_policy::render_decision_for_unmatched_command( - approval_policy, - &permission_profile, - file_system_sandbox_policy, - sandbox_cwd, cmd, - sandbox_permissions, - used_complex_parsing, + crate::exec_policy::UnmatchedCommandContext { + approval_policy, + permission_profile: &permission_profile, + file_system_sandbox_policy, + sandbox_cwd, + sandbox_permissions, + used_complex_parsing, + command_origin: crate::exec_policy::ExecPolicyCommandOrigin::Generic, + }, ) }; diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index 20fef4576..3f6c9cfdc 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -81,11 +81,6 @@ fn assert_no_matched_rules_invariant(output_item: &Value) { #[tokio::test] async fn execpolicy_blocks_shell_invocation() -> Result<()> { - // TODO execpolicy doesn't parse powershell commands yet - if cfg!(windows) { - return Ok(()); - } - let mut builder = test_codex().with_config(|config| { let policy_path = config.codex_home.join("rules").join("policy.rules"); fs::create_dir_all( diff --git a/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs b/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs index 611bafd2b..19babd82e 100644 --- a/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs +++ b/codex-rs/shell-command/src/command_safety/is_dangerous_command.rs @@ -28,6 +28,21 @@ pub fn command_might_be_dangerous(command: &[String]) -> bool { false } +/// Returns whether already-tokenized PowerShell words should be treated as +/// dangerous by the Windows unmatched-command heuristics. +pub fn is_dangerous_powershell_words(command: &[String]) -> bool { + #[cfg(windows)] + { + windows_dangerous_commands::is_dangerous_powershell_words(command) + } + + #[cfg(not(windows))] + { + let _ = command; + false + } +} + fn is_git_global_option_with_value(arg: &str) -> bool { matches!( arg, @@ -190,4 +205,15 @@ mod tests { assert!(git_global_option_requires_prompt("-C")); assert!(git_global_option_requires_prompt("-C/path/to/repo")); } + + #[test] + fn direct_powershell_words_reuse_windows_dangerous_detection() { + let command = vec_str(&["Remove-Item", "test", "-Force"]); + + if cfg!(windows) { + assert!(is_dangerous_powershell_words(&command)); + } else { + assert!(!is_dangerous_powershell_words(&command)); + } + } } diff --git a/codex-rs/shell-command/src/command_safety/is_safe_command.rs b/codex-rs/shell-command/src/command_safety/is_safe_command.rs index 26fe235fb..5e2ffec9f 100644 --- a/codex-rs/shell-command/src/command_safety/is_safe_command.rs +++ b/codex-rs/shell-command/src/command_safety/is_safe_command.rs @@ -6,6 +6,8 @@ use crate::command_safety::is_dangerous_command::executable_name_lookup_key; use crate::command_safety::is_dangerous_command::find_git_subcommand; use crate::command_safety::is_dangerous_command::git_global_option_requires_prompt; use crate::command_safety::windows_safe_commands::is_safe_command_windows; +#[cfg(windows)] +use crate::command_safety::windows_safe_commands::is_safe_powershell_words as is_safe_powershell_words_windows; pub fn is_known_safe_command(command: &[String]) -> bool { let command: Vec = command @@ -44,6 +46,21 @@ pub fn is_known_safe_command(command: &[String]) -> bool { false } +/// Returns whether already-tokenized PowerShell words are read-only enough to +/// be auto-approved by the Windows safelist. +pub fn is_safe_powershell_words(command: &[String]) -> bool { + #[cfg(windows)] + { + is_safe_powershell_words_windows(command) + } + + #[cfg(not(windows))] + { + let _ = command; + false + } +} + fn is_safe_to_call_with_exec(command: &[String]) -> bool { let Some(cmd0) = command.first().map(String::as_str) else { return false; @@ -638,4 +655,15 @@ mod tests { "> redirection should be rejected" ); } + + #[test] + fn direct_powershell_words_use_windows_safelist() { + let command = vec_str(&["Get-Content", "Cargo.toml"]); + + if cfg!(windows) { + assert!(is_safe_powershell_words(&command)); + } else { + assert!(!is_safe_powershell_words(&command)); + } + } } diff --git a/codex-rs/shell-command/src/command_safety/mod.rs b/codex-rs/shell-command/src/command_safety/mod.rs index 31fa64f8a..2040364e5 100644 --- a/codex-rs/shell-command/src/command_safety/mod.rs +++ b/codex-rs/shell-command/src/command_safety/mod.rs @@ -3,3 +3,4 @@ mod powershell_parser; pub mod is_dangerous_command; pub mod is_safe_command; pub(crate) mod windows_safe_commands; +pub(crate) use powershell_parser::try_parse_powershell_ast_commands; diff --git a/codex-rs/shell-command/src/command_safety/powershell_parser.rs b/codex-rs/shell-command/src/command_safety/powershell_parser.rs index bf09e60be..580f177f9 100644 --- a/codex-rs/shell-command/src/command_safety/powershell_parser.rs +++ b/codex-rs/shell-command/src/command_safety/powershell_parser.rs @@ -34,6 +34,16 @@ pub(super) fn parse_with_powershell_ast(executable: &str, script: &str) -> Power parse_with_cached_process(&mut parser_processes, executable, script) } +pub(crate) fn try_parse_powershell_ast_commands( + executable: &str, + script: &str, +) -> Option>> { + match parse_with_powershell_ast(executable, script) { + PowershellParseOutcome::Commands(commands) => Some(commands), + PowershellParseOutcome::Unsupported | PowershellParseOutcome::Failed => None, + } +} + #[derive(Debug, PartialEq, Eq)] pub(super) enum PowershellParseOutcome { Commands(Vec>), diff --git a/codex-rs/shell-command/src/command_safety/windows_dangerous_commands.rs b/codex-rs/shell-command/src/command_safety/windows_dangerous_commands.rs index c7f13adab..9e5089d08 100644 --- a/codex-rs/shell-command/src/command_safety/windows_dangerous_commands.rs +++ b/codex-rs/shell-command/src/command_safety/windows_dangerous_commands.rs @@ -34,12 +34,15 @@ fn is_dangerous_powershell(command: &[String]) -> bool { return false; }; - let tokens_lc: Vec = parsed - .tokens + is_dangerous_powershell_words(&parsed.tokens) +} + +pub(crate) fn is_dangerous_powershell_words(words: &[String]) -> bool { + let tokens_lc: Vec = words .iter() .map(|t| t.trim_matches('\'').trim_matches('"').to_ascii_lowercase()) .collect(); - let has_url = args_have_url(&parsed.tokens); + let has_url = args_have_url(words); if has_url && tokens_lc.iter().any(|t| { @@ -83,11 +86,7 @@ fn is_dangerous_powershell(command: &[String]) -> bool { } // Check for force delete operations (e.g., Remove-Item -Force) - if has_force_delete_cmdlet(&tokens_lc) { - return true; - } - - false + has_force_delete_cmdlet(&tokens_lc) } fn is_dangerous_cmd(command: &[String]) -> bool { diff --git a/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs b/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs index b6c5d863b..1dd628f42 100644 --- a/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs +++ b/codex-rs/shell-command/src/command_safety/windows_safe_commands.rs @@ -9,7 +9,7 @@ pub fn is_safe_command_windows(command: &[String]) -> bool { if let Some(commands) = try_parse_powershell_command_sequence(command) { commands .iter() - .all(|cmd| is_safe_powershell_command(cmd.as_slice())) + .all(|cmd| is_safe_powershell_words(cmd.as_slice())) } else { // Only PowerShell invocations are allowed on Windows for now; anything else is unsafe. false @@ -142,7 +142,7 @@ fn quote_argument(arg: &str) -> String { /// Validates that a parsed PowerShell command stays within our read-only safelist. /// Everything before this is parsing, and rejecting things that make us feel uncomfortable. -fn is_safe_powershell_command(words: &[String]) -> bool { +pub(crate) fn is_safe_powershell_words(words: &[String]) -> bool { if words.is_empty() { // Examples rejected here: "pwsh -Command ''" and "pwsh -Command \"\"". return false; diff --git a/codex-rs/shell-command/src/powershell.rs b/codex-rs/shell-command/src/powershell.rs index 0b118f727..f7b294c0f 100644 --- a/codex-rs/shell-command/src/powershell.rs +++ b/codex-rs/shell-command/src/powershell.rs @@ -2,6 +2,7 @@ use std::path::PathBuf; use codex_utils_absolute_path::AbsolutePathBuf; +use crate::command_safety::try_parse_powershell_ast_commands; use crate::shell_detect::ShellType; use crate::shell_detect::detect_shell_type; @@ -68,6 +69,18 @@ pub fn extract_powershell_command(command: &[String]) -> Option<(&str, &str)> { None } +/// Parse the script body from a top-level PowerShell wrapper into argv-like commands. +/// +/// 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_into_plain_commands( + command: &[String], +) -> Option>> { + let (executable, script) = extract_powershell_command(command)?; + try_parse_powershell_ast_commands(executable, script) +} + /// This function attempts to find a powershell.exe executable on the system. pub fn try_find_powershell_executable_blocking() -> Option { try_find_powershellish_executable_in_path(&["powershell.exe"]) @@ -139,6 +152,8 @@ fn is_powershellish_executable_available(powershell_or_pwsh_exe: &std::path::Pat #[cfg(test)] mod tests { use super::extract_powershell_command; + #[cfg(windows)] + use super::parse_powershell_command_into_plain_commands; #[test] fn extracts_basic_powershell_command() { @@ -186,4 +201,38 @@ mod tests { let (_shell, script) = extract_powershell_command(&cmd).expect("extract"); assert_eq!(script, "Get-ChildItem | Select-String foo"); } + + #[cfg(windows)] + #[test] + fn parses_plain_powershell_commands() { + let commands = parse_powershell_command_into_plain_commands(&[ + "powershell.exe".to_string(), + "-NoProfile".to_string(), + "-Command".to_string(), + "echo hi".to_string(), + ]) + .expect("parse"); + + assert_eq!(commands, vec![vec!["echo".to_string(), "hi".to_string()]]); + } + + #[cfg(windows)] + #[test] + fn parses_multiple_plain_powershell_commands() { + let commands = parse_powershell_command_into_plain_commands(&[ + "powershell.exe".to_string(), + "-NoProfile".to_string(), + "-Command".to_string(), + "Write-Output foo | Measure-Object".to_string(), + ]) + .expect("parse"); + + assert_eq!( + commands, + vec![ + vec!["Write-Output".to_string(), "foo".to_string()], + vec!["Measure-Object".to_string()], + ] + ); + } }