From 80b88a700e4300022fe1893ecca9b8376d0c85d9 Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 14 Oct 2026 19:39:32 +0000 Subject: [PATCH] Avoid false URL-launch detections in literal PowerShell commands (#54076) ## Why PowerShell safety checks can mistake a literal `start` argument for a URL-launch command, or combine a process launch with a URL from a separate command. ## What changed Classify independently parsed literal PowerShell commands separately so commands such as `Write-Output start 'https://example.com'` do not trigger URL-launch checks. Retain conservative checks for pipelines, script state, executable strings, and shell or terminal wrappers, while continuing to detect actual URL launches and destructive commands. ## Testing Add policy regression coverage for literal arguments, separate commands, URL launches, wrappers, and destructive commands, plus a Windows integration test covering execution and policy rejection. GitOrigin-RevId: 2878aa86bc36b44c60230cc1de3430922b28b81a --- codex-rs/core/src/exec_policy.rs | 23 +++- .../src/exec_policy/executable_identity.rs | 18 ++- codex-rs/core/src/exec_policy_tests.rs | 79 +++++++++++++ .../core/src/exec_policy_windows_tests.rs | 2 +- codex-rs/core/tests/suite/exec_policy.rs | 88 +++++++++++++++ .../command_safety/is_dangerous_command.rs | 9 ++ .../windows_dangerous_commands.rs | 104 +++++++++++++++--- 7 files changed, 302 insertions(+), 21 deletions(-) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 79d564e338..7e307e4f4a 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -161,6 +161,8 @@ pub(crate) enum ExecPolicyCommandOrigin { /// PowerShell wrapper, so use PowerShell-specific unmatched-command /// heuristics for the lowered words. PowerShell, + /// Strictly lowered commands with no pipeline or executable-string dependencies. + PowerShellLiteral, } #[derive(Clone, Copy)] @@ -730,6 +732,11 @@ fn dangerous_command_match_for_origin( command_platform, ) } + ExecPolicyCommandOrigin::PowerShellLiteral => { + codex_shell_command::is_dangerous_command::dangerous_powershell_literal_words_match( + command, + ) + } } } @@ -887,13 +894,15 @@ fn commands_for_exec_policy_for_platform( } if command_platform == DangerousCommandPlatform::Windows + && let Some((_, script)) = + codex_shell_command::powershell::extract_powershell_command(command) && let Some(commands) = - codex_shell_command::powershell::parse_powershell_command_into_plain_commands(command) + codex_shell_command::powershell::parse_powershell_script_into_plain_commands(script) && !commands.is_empty() { return ExecPolicyCommands { + command_origin: powershell_command_origin(script, &commands), commands, - command_origin: ExecPolicyCommandOrigin::PowerShell, }; } @@ -903,6 +912,16 @@ fn commands_for_exec_policy_for_platform( } } +fn powershell_command_origin(script: &str, commands: &[Vec]) -> ExecPolicyCommandOrigin { + if codex_shell_command::is_dangerous_command::powershell_commands_are_independent( + script, commands, + ) { + ExecPolicyCommandOrigin::PowerShellLiteral + } else { + ExecPolicyCommandOrigin::PowerShell + } +} + /// Derive a proposed execpolicy amendment when a command requires user approval /// - If any execpolicy rule prompts, return None, because an amendment would not skip that policy requirement. /// - Otherwise return the first heuristics Prompt. diff --git a/codex-rs/core/src/exec_policy/executable_identity.rs b/codex-rs/core/src/exec_policy/executable_identity.rs index c85fa184bb..d85b075285 100644 --- a/codex-rs/core/src/exec_policy/executable_identity.rs +++ b/codex-rs/core/src/exec_policy/executable_identity.rs @@ -3,6 +3,7 @@ use super::ExecPolicyCommandOrigin; use super::ExecPolicyCommands; use super::ExecPolicyManager; use super::commands_for_exec_policy_for_platform; +use super::powershell_command_origin; use crate::shell::Shell; use crate::tools::sandboxing::ExecApprovalRequirement; use codex_shell_command::is_dangerous_command::DangerousCommandPlatform; @@ -27,11 +28,18 @@ impl ExecPolicyManager { let mut policy_commands = if command_platform == DangerousCommandPlatform::Windows { match extract_powershell_command(command) { - Some((_, script)) => ExecPolicyCommands { - commands: parse_powershell_script_into_plain_commands(script) - .unwrap_or_else(|| vec![command.to_vec()]), - command_origin: ExecPolicyCommandOrigin::PowerShell, - }, + Some((_, script)) => { + let commands = parse_powershell_script_into_plain_commands(script); + let command_origin = commands + .as_ref() + .map_or(ExecPolicyCommandOrigin::PowerShell, |commands| { + powershell_command_origin(script, commands) + }); + ExecPolicyCommands { + commands: commands.unwrap_or_else(|| vec![command.to_vec()]), + command_origin, + } + } None => commands_for_exec_policy_for_platform(command, command_platform), } } else { diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index b8dbf30c85..c17b910a26 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -2485,3 +2485,82 @@ async fn exec_policy_warnings_ignore_untrusted_project_rules_without_config_toml Ok(()) } + +#[tokio::test] +async fn windows_powershell_policy_classifies_only_independent_literal_commands() { + for (script, expected) in [ + ( + "project-cli subtask start --text 'https://example.com'", + Decision::Allow, + ), + ("Write-Output start 'https://example.com'", Decision::Allow), + ( + "Start-Process notepad.exe; Invoke-WebRequest 'http://127.0.0.1/health'", + Decision::Allow, + ), + ("Start-Process 'https://example.com'", Decision::Forbidden), + ( + "Write-Output start 'https://example.com' | Out-String", + Decision::Forbidden, + ), + ( + "$target = 'https://example.com/'; Start-Process $target", + Decision::Forbidden, + ), + ( + "Write-Output 'Start-Process https://example.com'", + Decision::Forbidden, + ), + ( + "wmic process call create 'powershell.exe -Command Start-Process https://example.com'", + Decision::Forbidden, + ), + ("cmd /c start https://example.com", Decision::Forbidden), + ( + "wt cmd.exe /c start https://example.com", + Decision::Forbidden, + ), + ( + "wt.exe cmd.exe /c start https://example.com", + Decision::Forbidden, + ), + ("wt.exe cmd.exe /c echo hello", Decision::Allow), + ("Remove-Item test -Force", Decision::Forbidden), + ] { + for flags in [&["-Command"][..], &["-NonInteractive", "-Command"][..]] { + for configured_executable in ["pwsh", "other-shell"] { + let manager = ExecPolicyManager::new(Arc::new(Policy::empty())); + let command = [vec_str(&["pwsh"]), vec_str(flags), vec_str(&[script])].concat(); + let shell = crate::shell::Shell { + shell_type: crate::shell::ShellType::PowerShell, + shell_path: PathBuf::from(configured_executable), + }; + let requirement = manager + .create_exec_approval_requirement_for_shell( + ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::Never, + permission_profile: PermissionProfile::Disabled, + environment_policy: None, + windows_sandbox_level: WindowsSandboxLevel::Disabled, + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + allow_prefix_rules: AllowPrefixRules::Honor, + }, + &shell, + DangerousCommandPlatform::Windows, + ) + .await; + let actual = match requirement { + ExecApprovalRequirement::Skip { .. } => Decision::Allow, + ExecApprovalRequirement::NeedsApproval { .. } => Decision::Prompt, + ExecApprovalRequirement::Forbidden { .. } => Decision::Forbidden, + }; + assert_eq!( + actual, expected, + "{command:?}; shell: {configured_executable}" + ); + } + } + } +} diff --git a/codex-rs/core/src/exec_policy_windows_tests.rs b/codex-rs/core/src/exec_policy_windows_tests.rs index b07932eda1..e078d57e23 100644 --- a/codex-rs/core/src/exec_policy_windows_tests.rs +++ b/codex-rs/core/src/exec_policy_windows_tests.rs @@ -61,7 +61,7 @@ fn commands_for_exec_policy_parses_powershell_shell_wrapper() { commands_for_exec_policy(&command), ExecPolicyCommands { commands: vec![vec!["echo".to_string(), "blocked".to_string()]], - command_origin: ExecPolicyCommandOrigin::PowerShell, + command_origin: ExecPolicyCommandOrigin::PowerShellLiteral, } ); } diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index aef95b4df0..fb7ad89755 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -513,6 +513,94 @@ async fn unified_exec_disabled_windows_sandbox_rejects_managed_read_only_command Ok(()) } +#[tokio::test] +#[cfg_attr(not(windows), ignore = "requires native Windows PowerShell execution")] +async fn powershell_independent_literal_commands_preserve_url_launch_checks() -> Result<()> { + core_test_support::skip_if_remote!(Ok(()), "requires local Windows PowerShell execution"); + let shell_path = codex_shell_command::powershell::try_find_powershell_executable_blocking() + .ok_or_else(|| anyhow::anyhow!("Windows PowerShell is unavailable"))? + .to_path_buf(); + + for (script, expected_output) in [ + ( + "Write-Output start 'https://example.invalid/diagnostic'", + Some("https://example.invalid/diagnostic"), + ), + ( + "Start-Process cmd.exe -ArgumentList '/c exit 0' -Wait; Write-Output 'https://example.invalid/diagnostic'", + Some("https://example.invalid/diagnostic"), + ), + ("Start-Process 'https://example.invalid/diagnostic'", None), + ( + "wt.exe cmd.exe /c start https://example.invalid/diagnostic", + None, + ), + ( + "Write-Output start 'https://example.invalid/diagnostic' | Out-String", + None, + ), + ( + "$target = 'https://example.invalid/diagnostic'; Start-Process $target", + None, + ), + ] { + let server = start_mock_server().await; + let user_shell = codex_shell_command::shell_detect::DetectedShell { + shell_type: codex_shell_command::shell_detect::ShellType::PowerShell, + shell_path: shell_path.clone(), + } + .into(); + let mut builder = test_codex() + .with_user_shell(user_shell) + .with_config(|config| { + config + .features + .enable(Feature::UnifiedExec) + .expect("test config should allow feature update"); + }); + let test = builder.build_with_auto_env(&server).await?; + let call_id = "powershell-policy"; + let args = json!({ "cmd": script, "login": false, "yield_time_ms": 10_000 }); + mount_sse_once( + &server, + sse(vec![ + ev_response_created("powershell-command"), + ev_function_call(call_id, "exec_command", &serde_json::to_string(&args)?), + ev_completed("powershell-command"), + ]), + ) + .await; + let results_mock = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("done", "done"), + ev_completed("powershell-result"), + ]), + ) + .await; + + test.submit_turn("run the PowerShell command").await?; + let output = results_mock + .single_request() + .function_call_output_text(call_id) + .expect("shell call should produce a tool response"); + match expected_output { + Some(expected_output) => { + assert!( + output.contains("Process exited with code 0"), + "{script}: {output}" + ); + assert!(output.contains(expected_output), "{script}: {output}"); + } + None => assert!( + output.contains("rejected: blocked by policy"), + "{script}: {output}" + ), + } + } + Ok(()) +} + #[tokio::test] async fn execpolicy_blocks_shell_invocation() -> Result<()> { let mut builder = test_codex().with_config(|config| { 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 aa2f4d50a3..2440f17001 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 @@ -1,6 +1,7 @@ use crate::bash::parse_shell_lc_literal_commands; #[path = "windows_dangerous_commands.rs"] mod windows_dangerous_commands; +pub use windows_dangerous_commands::powershell_commands_are_independent; /// The platform whose command semantics should be used for safety checks. #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -93,6 +94,14 @@ pub fn dangerous_powershell_words_match( } } +/// Classifies a strictly lowered command from an independent literal PowerShell script. +pub fn dangerous_powershell_literal_words_match( + command: &[String], +) -> Option { + windows_dangerous_commands::is_dangerous_powershell_literal_words(command) + .then_some(DangerousCommandMatch::Other) +} + fn executable_name_lookup_key(raw: &str, platform: DangerousCommandPlatform) -> Option { match platform { DangerousCommandPlatform::Posix => raw 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 cc1ae671c8..8072b881b1 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 @@ -1,3 +1,4 @@ +use crate::command_safety::powershell_tree_sitter::try_parse_powershell_commands; use once_cell::sync::Lazy; use regex::Regex; use shlex::split as shlex_split; @@ -25,17 +26,82 @@ fn is_dangerous_powershell(command: &[String]) -> bool { if !is_powershell_executable(exe) { return false; } - // Parse the PowerShell invocation to get a flat token list we can scan for - // dangerous cmdlets/COM calls plus any URL-looking arguments. This is a - // best-effort shlex split of the script text, not a full PS parser. let Some(parsed) = parse_powershell_invocation(rest) else { return false; }; + if let Some(script) = parsed.script + && let Some(commands) = try_parse_powershell_commands(script) + && powershell_commands_are_independent(script, &commands) + { + return commands + .iter() + .any(|words| is_dangerous_powershell_literal_words(words)); + } is_dangerous_powershell_words(&parsed.tokens) } +/// Whether strictly lowered commands can be classified independently of the rest of the script. +pub fn powershell_commands_are_independent(script: &str, commands: &[Vec]) -> bool { + // Deliberately keep even quoted pipes and `||` chains on the existing path. Do not + // model pipeline data, command rebinding, or strings that may execute using script state. + !script.contains('|') + && commands.iter().all(|words| { + !matches!( + words + .first() + .and_then(|word| executable_basename(word)) + .as_deref(), + Some( + "set-alias" + | "sal" + | "new-alias" + | "nal" + | "set-item" + | "si" + | "new-item" + | "ni" + ) + ) && !words.iter().skip(1).any(|word| { + let word = word.to_ascii_lowercase(); + [ + "start-process", + "invoke-item", + "shellexecute", + "shell.application", + ] + .iter() + .any(|marker| word.contains(marker)) + }) + }) +} + +enum PowerShellCommandKind { + Opaque, + IndependentLiteral, +} + pub(crate) fn is_dangerous_powershell_words(words: &[String]) -> bool { + classify_powershell_words(words, PowerShellCommandKind::Opaque) +} + +pub(crate) fn is_dangerous_powershell_literal_words(words: &[String]) -> bool { + // Shell and terminal wrappers can interpret later words as commands, including `start`. + let kind = if words.first().is_some_and(|word| { + is_powershell_executable(word) + || matches!( + executable_basename(word).as_deref(), + Some("cmd" | "cmd.exe" | "wt" | "wt.exe") + ) + }) { + PowerShellCommandKind::Opaque + } else { + PowerShellCommandKind::IndependentLiteral + }; + classify_powershell_words(words, kind) +} + +fn classify_powershell_words(words: &[String], kind: PowerShellCommandKind) -> bool { let tokens_lc: Vec = words .iter() .map(|t| t.trim_matches('\'').trim_matches('"').to_ascii_lowercase()) @@ -43,11 +109,13 @@ pub(crate) fn is_dangerous_powershell_words(words: &[String]) -> bool { let has_url = args_have_url(words); if has_url - && tokens_lc.iter().any(|t| { - matches!( - t.as_str(), - "start-process" | "start" | "saps" | "invoke-item" | "ii" - ) || t.contains("start-process") + && tokens_lc.iter().enumerate().any(|(index, t)| { + ((index == 0 || matches!(kind, PowerShellCommandKind::Opaque)) + && matches!( + t.as_str(), + "start-process" | "start" | "saps" | "invoke-item" | "ii" + )) + || t.contains("start-process") || t.contains("invoke-item") }) { @@ -365,11 +433,12 @@ fn is_browser_executable(name: &str) -> bool { ) } -struct ParsedPowershell { +struct ParsedPowershell<'a> { tokens: Vec, + script: Option<&'a str>, } -fn parse_powershell_invocation(args: &[String]) -> Option { +fn parse_powershell_invocation(args: &[String]) -> Option> { if args.is_empty() { return None; } @@ -385,7 +454,10 @@ fn parse_powershell_invocation(args: &[String]) -> Option { return None; } let tokens = shlex_split(script)?; - return Some(ParsedPowershell { tokens }); + return Some(ParsedPowershell { + tokens, + script: Some(script), + }); } _ if lower.starts_with("-command:") || lower.starts_with("/command:") => { if idx + 1 != args.len() { @@ -393,7 +465,10 @@ fn parse_powershell_invocation(args: &[String]) -> Option { } let (_, script) = arg.split_once(':')?; let tokens = shlex_split(script)?; - return Some(ParsedPowershell { tokens }); + return Some(ParsedPowershell { + tokens, + script: Some(script), + }); } "-nologo" | "-noprofile" | "-noninteractive" | "-mta" | "-sta" => { idx += 1; @@ -403,7 +478,10 @@ fn parse_powershell_invocation(args: &[String]) -> Option { } _ => { let rest = args[idx..].to_vec(); - return Some(ParsedPowershell { tokens: rest }); + return Some(ParsedPowershell { + tokens: rest, + script: None, + }); } } }