diff --git a/codex-rs/core/src/command_canonicalization.rs b/codex-rs/core/src/command_canonicalization.rs index fc110cf9de..b5c3948577 100644 --- a/codex-rs/core/src/command_canonicalization.rs +++ b/codex-rs/core/src/command_canonicalization.rs @@ -1,29 +1,9 @@ -use codex_shell_command::bash::extract_bash_command; -use codex_shell_command::bash::parse_shell_lc_plain_commands; - -const CANONICAL_BASH_SCRIPT_PREFIX: &str = "__codex_shell_script__"; - /// Canonicalize command argv for approval-cache matching. /// -/// Bash word-only scripts retain their historical inner-command identity. For -/// PowerShell, the executable, wrapper flags, and exact script are all part of -/// the key because each affects the authorization boundary. +/// Approval identity preserves the literal executable, wrapper arguments, and +/// script text. Each affects the authorization boundary, so even semantically +/// similar argv vectors remain distinct cache entries. pub(crate) fn canonicalize_command_for_approval(command: &[String]) -> Vec { - if let Some(commands) = parse_shell_lc_plain_commands(command) - && let [single_command] = commands.as_slice() - { - return single_command.clone(); - } - - if let Some((_shell, script)) = extract_bash_command(command) { - let shell_mode = command.get(1).cloned().unwrap_or_default(); - return vec![ - CANONICAL_BASH_SCRIPT_PREFIX.to_string(), - shell_mode, - script.to_string(), - ]; - } - command.to_vec() } diff --git a/codex-rs/core/src/command_canonicalization_tests.rs b/codex-rs/core/src/command_canonicalization_tests.rs index b6af11ee6d..188570f31c 100644 --- a/codex-rs/core/src/command_canonicalization_tests.rs +++ b/codex-rs/core/src/command_canonicalization_tests.rs @@ -2,6 +2,10 @@ use super::canonicalize_command_for_approval; use pretty_assertions::assert_eq; use std::collections::HashSet; +fn posix(shell: &str, mode: &str, script: &str) -> Vec { + vec![shell.to_string(), mode.to_string(), script.to_string()] +} + fn powershell(executable: &str, command_flag: &str, script: &str) -> Vec { vec![ executable.to_string(), @@ -12,54 +16,58 @@ fn powershell(executable: &str, command_flag: &str, script: &str) -> Vec } #[test] -fn canonicalizes_word_only_shell_scripts_to_inner_command() { - let command_a = vec![ - "/bin/bash".to_string(), - "-lc".to_string(), - "cargo test -p codex-core".to_string(), - ]; - let command_b = vec![ - "bash".to_string(), - "-lc".to_string(), - "cargo test -p codex-core".to_string(), +fn preserves_exact_posix_approval_identity() { + let base = posix("/bin/bash", "-lc", "printf '%s\\n' value"); + let commands = vec![ + base.clone(), + posix("bash", "-lc", "printf '%s\\n' value"), + posix("./bash", "-lc", "printf '%s\\n' value"), + posix("/tmp/workspace/bash", "-lc", "printf '%s\\n' value"), + posix("/bin/zsh", "-lc", "printf '%s\\n' value"), + posix("/bin/sh", "-lc", "printf '%s\\n' value"), + posix("/bin/bash", "-c", "printf '%s\\n' value"), + posix("/bin/bash", "-lc", "printf '%s\\n' value"), + posix("/bin/bash", "-lc", "printf \"%s\\n\" value"), + posix("/bin/bash", "-lc", "printf '%s\\n' *"), + posix("/bin/bash", "-lc", "printf '%s\\n' ~"), + posix("/bin/bash", "-lc", "printf '%s\\n' \"$(id)\""), + posix("/bin/bash", "-lc", "printf '%s\\n' value >out"), + posix("/bin/bash", "-lc", "python3 <<'PY'\nprint('value')\nPY"), + posix("/bin/bash", "-lc", ""), + posix("/bin/bash", "-lc", "printf '%s\\n' changed"), + vec![ + "printf".to_string(), + "%s\\n".to_string(), + "value".to_string(), + ], ]; + let keys: HashSet<_> = commands + .iter() + .map(|command| canonicalize_command_for_approval(command)) + .collect(); + + assert_eq!(keys.len(), commands.len()); assert_eq!( - canonicalize_command_for_approval(&command_a), - vec![ - "cargo".to_string(), - "test".to_string(), - "-p".to_string(), - "codex-core".to_string(), - ] - ); - assert_eq!( - canonicalize_command_for_approval(&command_a), - canonicalize_command_for_approval(&command_b) + canonicalize_command_for_approval(&base), + canonicalize_command_for_approval(&base) ); + for command in commands { + assert_eq!(canonicalize_command_for_approval(&command), command); + } } #[test] -fn canonicalizes_heredoc_scripts_to_stable_script_key() { - let script = "python3 <<'PY'\nprint('hello')\nPY"; - let command_a = vec![ - "/bin/zsh".to_string(), - "-lc".to_string(), - script.to_string(), - ]; - let command_b = vec!["zsh".to_string(), "-lc".to_string(), script.to_string()]; +fn posix_identity_does_not_collide_with_raw_marker_argv() { + let shell = posix("/bin/bash", "-lc", "echo marker"); + let mut marker_argv = vec!["__codex_shell_script__".to_string()]; + marker_argv.extend(shell.iter().cloned()); - assert_eq!( - canonicalize_command_for_approval(&command_a), - vec![ - "__codex_shell_script__".to_string(), - "-lc".to_string(), - script.to_string(), - ] - ); - assert_eq!( - canonicalize_command_for_approval(&command_a), - canonicalize_command_for_approval(&command_b) + assert_eq!(canonicalize_command_for_approval(&shell), shell); + assert_eq!(canonicalize_command_for_approval(&marker_argv), marker_argv); + assert_ne!( + canonicalize_command_for_approval(&shell), + canonicalize_command_for_approval(&marker_argv) ); } diff --git a/codex-rs/core/src/exec_env_tests.rs b/codex-rs/core/src/exec_env_tests.rs index 73c0944c14..1e3d6feea8 100644 --- a/codex-rs/core/src/exec_env_tests.rs +++ b/codex-rs/core/src/exec_env_tests.rs @@ -280,13 +280,100 @@ fn create_env_preserves_existing_pathext_case_insensitively_on_windows() { let result = create_env_from_vars(vars, &policy, /*thread_id*/ None); - let pathext_vars = result - .iter() - .filter(|(key, _)| key.eq_ignore_ascii_case("PATHEXT")) - .collect::>(); + assert_eq!( + result, + HashMap::from([( + "PATHEXT".to_string(), + ".COM;.EXE;.BAT;.CMD;.PS1".to_string(), + )]) + ); +} - assert_eq!(pathext_vars.len(), 1); - assert_eq!(pathext_vars[0].1, ".COM;.EXE;.BAT;.CMD;.PS1"); +#[test] +#[cfg(target_os = "windows")] +fn create_env_prefers_configured_canonical_path_over_inherited_aliases() { + let vars = make_vars(&[ + ("Path", "C:\\inherited-mixed"), + ("path", "C:\\inherited-lower"), + ]); + let mut policy = ShellEnvironmentPolicy { + inherit: ShellEnvironmentPolicyInherit::All, + ignore_default_excludes: true, + ..Default::default() + }; + policy + .r#set + .insert("PATH".to_string(), "C:\\configured".to_string()); + + let result = create_env_from_vars(vars, &policy, /*thread_id*/ None); + + assert_eq!( + result.get("PATH").map(String::as_str), + Some("C:\\configured") + ); + assert_eq!( + result + .keys() + .filter(|key| key.eq_ignore_ascii_case("PATH")) + .count(), + 1 + ); +} + +#[test] +#[cfg(target_os = "windows")] +fn create_env_prefers_configured_path_alias_over_inherited_canonical_key() { + let vars = make_vars(&[("PATH", "C:\\inherited")]); + let mut policy = ShellEnvironmentPolicy { + inherit: ShellEnvironmentPolicyInherit::All, + ignore_default_excludes: true, + ..Default::default() + }; + policy + .r#set + .insert("path".to_string(), "C:\\configured-alias".to_string()); + + let result = create_env_from_vars(vars, &policy, /*thread_id*/ None); + + assert_eq!( + result.get("PATH").map(String::as_str), + Some("C:\\configured-alias") + ); + assert_eq!( + result + .keys() + .filter(|key| key.eq_ignore_ascii_case("PATH")) + .count(), + 1 + ); +} + +#[test] +#[cfg(target_os = "windows")] +fn create_env_prefers_configured_canonical_pathext_and_removes_aliases() { + let vars = make_vars(&[("PathExt", ".COM;.EXE;.BAT;.CMD")]); + let mut policy = ShellEnvironmentPolicy { + inherit: ShellEnvironmentPolicyInherit::All, + ignore_default_excludes: true, + ..Default::default() + }; + policy + .r#set + .insert("pathext".to_string(), ".EXE;.CMD".to_string()); + policy + .r#set + .insert("PATHEXT".to_string(), ".EXE".to_string()); + + let result = create_env_from_vars(vars, &policy, /*thread_id*/ None); + + assert_eq!(result.get("PATHEXT").map(String::as_str), Some(".EXE")); + assert_eq!( + result + .keys() + .filter(|key| key.eq_ignore_ascii_case("PATHEXT")) + .count(), + 1 + ); } #[test] diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 54afe95668..4dda333295 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -36,8 +36,12 @@ use crate::config::Config; use crate::sandboxing::SandboxPermissions; use crate::tools::sandboxing::ExecApprovalRequirement; use crate::tools::sandboxing::unsandboxed_execution_allowed; +use codex_shell_command::bash::extract_bash_command; use codex_shell_command::bash::parse_shell_lc_plain_commands; use codex_shell_command::bash::parse_shell_lc_single_command_prefix; +use codex_shell_command::bash::try_parse_shell; +use codex_shell_command::shell_detect::ShellType; +use codex_shell_command::shell_detect::detect_shell_type; use codex_utils_absolute_path::AbsolutePathBuf; use shlex::try_join as shlex_try_join; @@ -139,6 +143,484 @@ struct ExecPolicyCommands { command_origin: ExecPolicyCommandOrigin, } +const MAX_POSIX_POLICY_DEPTH: usize = 8; +const MAX_POSIX_POLICY_CANDIDATES: usize = 64; +const MAX_POSIX_POLICY_SCRIPT_BYTES: usize = 64 * 1024; + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum ShellRuntimeSource { + EnvironmentSelected, + ModelResolved, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum ShellSelectionSource { + Configured, + ModelSelected, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) struct ShellApprovalProvenance { + runtime: ShellRuntimeSource, + selection: ShellSelectionSource, +} + +impl ShellApprovalProvenance { + pub(crate) const fn configured() -> Self { + Self { + runtime: ShellRuntimeSource::EnvironmentSelected, + selection: ShellSelectionSource::Configured, + } + } + + pub(crate) const fn local_model_resolved() -> Self { + Self { + runtime: ShellRuntimeSource::ModelResolved, + selection: ShellSelectionSource::ModelSelected, + } + } + + pub(crate) const fn remote_model_hint() -> Self { + Self { + runtime: ShellRuntimeSource::EnvironmentSelected, + selection: ShellSelectionSource::ModelSelected, + } + } + + const fn requires_outer_policy(self) -> bool { + matches!(self.runtime, ShellRuntimeSource::ModelResolved) + } + + pub(crate) const fn is_local_model_resolved(self) -> bool { + self.requires_outer_policy() + } + + const fn selection_is_model_supplied(self) -> bool { + matches!(self.selection, ShellSelectionSource::ModelSelected) + } + + const fn allows_generated_amendment(self) -> bool { + !self.selection_is_model_supplied() + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum PosixAnalysisCompleteness { + Complete, + Incomplete, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum ExecPolicyCandidateRole { + UntrustedWrapper, + InnerCommand, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +struct ExecPolicyCandidate { + argv: Vec, + role: ExecPolicyCandidateRole, + command_origin: ExecPolicyCommandOrigin, +} + +#[derive(Debug, Eq, PartialEq)] +struct PosixPolicyAnalysis { + candidates: Vec, + completeness: PosixAnalysisCompleteness, + contains_untrusted_wrapper: bool, + script_bytes: usize, +} + +impl PosixPolicyAnalysis { + fn new() -> Self { + Self { + candidates: Vec::new(), + completeness: PosixAnalysisCompleteness::Complete, + contains_untrusted_wrapper: false, + script_bytes: 0, + } + } + + fn mark_incomplete(&mut self) { + self.completeness = PosixAnalysisCompleteness::Incomplete; + } + + fn push_candidate(&mut self, candidate: ExecPolicyCandidate) -> bool { + if self.candidates.len() >= MAX_POSIX_POLICY_CANDIDATES { + self.mark_incomplete(); + return false; + } + self.candidates.push(candidate); + true + } + + fn add_script_bytes(&mut self, bytes: usize) -> bool { + let Some(total) = self.script_bytes.checked_add(bytes) else { + self.mark_incomplete(); + return false; + }; + if total > MAX_POSIX_POLICY_SCRIPT_BYTES { + self.mark_incomplete(); + return false; + } + self.script_bytes = total; + true + } +} + +fn is_posix_shell_executable(program: &str) -> bool { + matches!( + detect_shell_type(Path::new(program)), + Some(ShellType::Bash | ShellType::Sh | ShellType::Zsh) + ) +} + +fn executable_basename_lowercase(program: &str) -> String { + let basename = program + .rsplit(['/', '\\']) + .next() + .unwrap_or(program) + .to_ascii_lowercase(); + basename + .strip_suffix(".exe") + .unwrap_or(&basename) + .to_string() +} + +fn executable_spelling_is_absolute(program: &str) -> bool { + // Intentionally use controller-native path semantics. A spelling that is + // absolute only for a foreign remote target conservatively cannot establish + // authority to bypass this controller's sandbox or approval boundary. + Path::new(program).is_absolute() +} + +fn executable_may_hide_nested_execution(program: &str) -> bool { + let basename = executable_basename_lowercase(program); + matches!( + basename.as_str(), + "." | "source" + | "trap" + | "env" + | "sudo" + | "doas" + | "su" + | "runuser" + | "command" + | "nice" + | "nohup" + | "timeout" + | "time" + | "watch" + | "chroot" + | "setsid" + | "setpriv" + | "stdbuf" + | "ionice" + | "taskset" + | "exec" + | "eval" + | "builtin" + | "noglob" + | "nocorrect" + | "xargs" + | "parallel" + | "busybox" + | "ash" + | "csh" + | "dash" + | "fish" + | "ksh" + | "mksh" + | "rc" + | "tcsh" + | "cmd" + | "powershell" + | "pwsh" + ) +} + +fn transparent_executor_is_query_only(command: &[String]) -> bool { + let arguments = command.get(1..).unwrap_or_default(); + match command.first().map(String::as_str) { + Some("command") => { + arguments.is_empty() + || arguments + .first() + .is_some_and(|flag| matches!(flag.as_str(), "-v" | "-V")) + } + Some("trap") => { + arguments.is_empty() + || matches!(arguments, [flag] if matches!(flag.as_str(), "-l" | "-p")) + } + Some(_) | None => false, + } +} + +fn command_may_hide_nested_execution(command: &[String]) -> bool { + let Some(program) = command.first() else { + return true; + }; + let basename = executable_basename_lowercase(program); + if executable_may_hide_nested_execution(program) { + return !transparent_executor_is_query_only(command); + } + + basename == "find" + && command + .iter() + .skip(1) + .any(|argument| matches!(argument.as_str(), "-exec" | "-execdir" | "-ok" | "-okdir")) +} + +/// Returns true when a configured shell body that the strict argv extractor +/// could not fully reduce still has structural evidence of descendant +/// execution. A single complex command (for example, a heredoc or redirect) +/// remains on the legacy configured-shell path, while parse errors, multiple +/// commands, nested shells, and known delegators fail closed when policy rules +/// are active. +fn incomplete_posix_body_may_hide_descendant_execution(command: &[String]) -> bool { + let Some((_shell, script)) = extract_bash_command(command) else { + return true; + }; + if script.len() > MAX_POSIX_POLICY_SCRIPT_BYTES { + return true; + } + let Some(tree) = try_parse_shell(script) else { + return true; + }; + let root = tree.root_node(); + if root.has_error() { + return true; + } + + let mut stack = vec![root]; + let mut command_count = 0usize; + while let Some(node) = stack.pop() { + if matches!( + node.kind(), + "c_style_for_statement" + | "case_statement" + | "command_substitution" + | "compound_statement" + | "for_statement" + | "function_definition" + | "if_statement" + | "process_substitution" + | "subshell" + | "while_statement" + ) { + return true; + } + if node.kind() == "command" { + command_count += 1; + if command_count > 1 { + return true; + } + + let mut cursor = node.walk(); + let mut command_argv = Vec::new(); + let mut saw_command_name = false; + let mut has_dynamic_argument = false; + for child in node.named_children(&mut cursor) { + match child.kind() { + "command_name" => { + let Some(word) = child.named_child(0) else { + return true; + }; + if word.kind() != "word" || word.named_child_count() != 0 { + return true; + } + let Ok(program) = word.utf8_text(script.as_bytes()) else { + return true; + }; + command_argv.push(program.to_string()); + saw_command_name = true; + } + "word" | "number" | "concatenation" | "expansion" | "simple_expansion" + if saw_command_name => + { + has_dynamic_argument |= matches!( + child.kind(), + "concatenation" | "expansion" | "simple_expansion" + ) || child.named_child_count() != 0; + let Ok(argument) = child.utf8_text(script.as_bytes()) else { + return true; + }; + command_argv.push(argument.to_string()); + } + "raw_string" | "string" if saw_command_name => { + if child.kind() == "string" { + let mut argument_cursor = child.walk(); + has_dynamic_argument |= + child.named_children(&mut argument_cursor).any(|part| { + matches!( + part.kind(), + "arithmetic_expansion" + | "command_substitution" + | "expansion" + | "process_substitution" + | "simple_expansion" + ) + }); + } + let Ok(argument) = child.utf8_text(script.as_bytes()) else { + return true; + }; + let unquoted = argument + .strip_prefix('\'') + .and_then(|argument| argument.strip_suffix('\'')) + .or_else(|| { + argument + .strip_prefix('"') + .and_then(|argument| argument.strip_suffix('"')) + }) + .unwrap_or(argument); + command_argv.push(unquoted.to_string()); + } + "variable_assignment" if !saw_command_name => {} + "file_redirect" | "heredoc_redirect" => {} + _ => return true, + } + } + let Some(program) = command_argv.first() else { + return true; + }; + let basename = executable_basename_lowercase(program); + if has_dynamic_argument + && (basename == "find" + || (executable_may_hide_nested_execution(program) + && !transparent_executor_is_query_only(&command_argv))) + { + return true; + } + if is_posix_shell_executable(program) + || command_may_hide_nested_execution(&command_argv) + { + return true; + } + } + + let mut cursor = node.walk(); + stack.extend(node.children(&mut cursor)); + } + false +} + +fn analyze_posix_policy( + command: &[String], + provenance: ShellApprovalProvenance, +) -> Option { + let program = command.first()?; + if !is_posix_shell_executable(program) { + return None; + } + + let mut analysis = PosixPolicyAnalysis::new(); + analyze_posix_wrapper( + command, + provenance.requires_outer_policy(), + /*depth*/ 0, + &mut analysis, + ); + Some(analysis) +} + +fn opaque_untrusted_wrapper_analysis(command: &[String]) -> PosixPolicyAnalysis { + PosixPolicyAnalysis { + candidates: vec![ExecPolicyCandidate { + argv: command.to_vec(), + role: ExecPolicyCandidateRole::UntrustedWrapper, + command_origin: ExecPolicyCommandOrigin::Generic, + }], + completeness: PosixAnalysisCompleteness::Incomplete, + contains_untrusted_wrapper: true, + script_bytes: 0, + } +} + +fn analyze_posix_wrapper( + command: &[String], + untrusted_wrapper: bool, + depth: usize, + analysis: &mut PosixPolicyAnalysis, +) { + if untrusted_wrapper { + analysis.contains_untrusted_wrapper = true; + if !analysis.push_candidate(ExecPolicyCandidate { + argv: command.to_vec(), + role: ExecPolicyCandidateRole::UntrustedWrapper, + command_origin: ExecPolicyCommandOrigin::Generic, + }) { + return; + } + } + + if depth >= MAX_POSIX_POLICY_DEPTH { + analysis.mark_incomplete(); + return; + } + + let Some((_shell, script)) = extract_bash_command(command) else { + analysis.mark_incomplete(); + return; + }; + if script.trim().is_empty() || !analysis.add_script_bytes(script.len()) { + analysis.mark_incomplete(); + return; + } + + if let Some(commands) = parse_shell_lc_plain_commands(command) + && !commands.is_empty() + { + for inner in commands { + analyze_posix_inner_command(inner, depth, analysis); + } + return; + } + + if let Some(inner) = parse_shell_lc_single_command_prefix(command) { + analyze_posix_inner_command(inner, depth, analysis); + } + analysis.mark_incomplete(); +} + +fn analyze_posix_inner_command( + command: Vec, + parent_depth: usize, + analysis: &mut PosixPolicyAnalysis, +) { + if command.is_empty() { + analysis.mark_incomplete(); + return; + } + + if command + .first() + .is_some_and(|program| is_posix_shell_executable(program)) + { + analyze_posix_wrapper( + &command, + /*untrusted_wrapper*/ true, + parent_depth + 1, + analysis, + ); + return; + } + + let may_hide_nested_execution = command_may_hide_nested_execution(&command); + if !analysis.push_candidate(ExecPolicyCandidate { + argv: command, + role: ExecPolicyCandidateRole::InnerCommand, + command_origin: ExecPolicyCommandOrigin::Generic, + }) { + return; + } + if may_hide_nested_execution { + analysis.contains_untrusted_wrapper = true; + analysis.mark_incomplete(); + } +} + pub(crate) fn child_uses_parent_exec_policy(parent_config: &Config, child_config: &Config) -> bool { fn exec_policy_config_folders(config: &Config) -> Vec { config @@ -295,6 +777,34 @@ impl ExecPolicyManager { pub(crate) async fn create_exec_approval_requirement_for_command( &self, req: ExecApprovalRequest<'_>, + ) -> ExecApprovalRequirement { + let permission_expansion_was_requested = + req.sandbox_permissions.requests_sandbox_override(); + self.create_exec_approval_requirement_for_configured_command( + req, + permission_expansion_was_requested, + ) + .await + } + + pub(crate) async fn create_exec_approval_requirement_for_configured_command( + &self, + req: ExecApprovalRequest<'_>, + permission_expansion_was_requested: bool, + ) -> ExecApprovalRequirement { + self.create_exec_approval_requirement_for_command_with_provenance( + req, + ShellApprovalProvenance::configured(), + permission_expansion_was_requested, + ) + .await + } + + pub(crate) async fn create_exec_approval_requirement_for_command_with_provenance( + &self, + req: ExecApprovalRequest<'_>, + provenance: ShellApprovalProvenance, + permission_expansion_was_requested: bool, ) -> ExecApprovalRequirement { let ExecApprovalRequest { command, @@ -304,6 +814,8 @@ impl ExecPolicyManager { sandbox_permissions, prefix_rule, } = req; + let permission_expansion_was_requested = + permission_expansion_was_requested || sandbox_permissions.requests_sandbox_override(); let exec_policy = self.current(); #[cfg(windows)] let (parsed_powershell, powershell_outer_authority) = @@ -346,6 +858,53 @@ impl ExecPolicyManager { let exec_policy_commands = commands_for_exec_policy(command); #[cfg(not(windows))] let powershell_outer_authority = false; + + let posix_analysis = analyze_posix_policy(command, provenance); + let command_rules_active = exec_policy.rules().iter_all().next().is_some(); + if let Some(analysis) = posix_analysis.as_ref() + && (provenance.requires_outer_policy() + || (command_rules_active + && (analysis.contains_untrusted_wrapper + || (analysis.completeness == PosixAnalysisCompleteness::Incomplete + && incomplete_posix_body_may_hide_descendant_execution(command))))) + { + return create_untrusted_wrapper_approval_requirement( + exec_policy.as_ref(), + command, + analysis, + /*allow_exact_opaque_wrapper_for_environment_runtime*/ + !provenance.requires_outer_policy(), + UnmatchedCommandContext { + approval_policy, + permission_profile: &permission_profile, + windows_sandbox_level, + sandbox_permissions, + used_complex_parsing: matches!( + analysis.completeness, + PosixAnalysisCompleteness::Incomplete + ), + command_origin: ExecPolicyCommandOrigin::Generic, + }, + ); + } + if provenance.requires_outer_policy() { + let analysis = opaque_untrusted_wrapper_analysis(command); + return create_untrusted_wrapper_approval_requirement( + exec_policy.as_ref(), + command, + &analysis, + /*allow_exact_opaque_wrapper_for_environment_runtime*/ false, + UnmatchedCommandContext { + approval_policy, + permission_profile: &permission_profile, + windows_sandbox_level, + sandbox_permissions, + used_complex_parsing: true, + command_origin: ExecPolicyCommandOrigin::Generic, + }, + ); + } + let ExecPolicyCommands { commands, used_complex_parsing, @@ -354,7 +913,9 @@ impl ExecPolicyManager { // 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 auto_amendment_allowed = !used_complex_parsing + && provenance.allows_generated_amendment() + && !permission_expansion_was_requested; let exec_policy_fallback = |cmd: &[String]| { render_decision_for_unmatched_command( cmd, @@ -416,16 +977,31 @@ impl ExecPolicyManager { .max() .unwrap_or(Decision::Forbidden); - let permission_delta_requires_outer = - permission_delta_requires_outer_authority(&permission_profile, sandbox_permissions); - let parsed_powershell_needs_outer_approval = parsed_powershell_outer.is_some() - && !exact_outer_allow - && (permission_delta_requires_outer + let every_command_explicit_allow = !commands.is_empty() + && commands.iter().all(|command| { + exec_policy + .matches_for_command_with_options( + command, + /*heuristics_fallback*/ None, + &match_options, + ) + .iter() + .any(|rule_match| { + is_policy_match(rule_match) && rule_match.decision() == Decision::Allow + }) + }); + let effective_full_policy_authority = evaluation.decision == Decision::Allow + && parsed_powershell_outer.map_or(every_command_explicit_allow, |_| exact_outer_allow); + let permission_or_backend_gate = + permission_delta_requires_outer_authority(&permission_profile, sandbox_permissions) || missing_managed_windows_sandbox_backend( &permission_profile, windows_sandbox_level, - )); - if evaluation.decision != Decision::Forbidden && parsed_powershell_needs_outer_approval { + ); + if evaluation.decision != Decision::Forbidden + && permission_or_backend_gate + && !effective_full_policy_authority + { evaluation .matched_rules .push(RuleMatch::HeuristicsRuleMatch { @@ -455,7 +1031,7 @@ impl ExecPolicyManager { None }; - match evaluation.decision { + let requirement = match evaluation.decision { Decision::Forbidden => ExecApprovalRequirement::Forbidden { reason: derive_forbidden_reason(command, &evaluation), }, @@ -468,6 +1044,9 @@ impl ExecPolicyManager { None => ExecApprovalRequirement::NeedsApproval { reason: derive_prompt_reason(command, &evaluation), proposed_execpolicy_amendment: requested_amendment.or_else(|| { + if !auto_amendment_allowed { + return None; + } match ( parsed_powershell_outer, causes.rules, @@ -489,33 +1068,19 @@ impl ExecPolicyManager { } Decision::Allow => ExecApprovalRequirement::Skip { // Lowered PowerShell command names do not bind runtime module, profile, or PATH - // resolution. Only an exact full-wrapper Allow may authorize sandbox bypass. - bypass_sandbox: if parsed_powershell_outer.is_some() { - exact_outer_allow - } else { - commands.iter().all(|command| { - exec_policy - .matches_for_command_with_options( - command, - /*heuristics_fallback*/ None, - &match_options, - ) - .iter() - .any(|rule_match| { - is_policy_match(rule_match) - && rule_match.decision() == Decision::Allow - }) - }) - }, - proposed_execpolicy_amendment: if let Some(outer) = parsed_powershell_outer { - (!exact_outer_allow).then(|| ExecPolicyAmendment::new(outer.to_vec())) - } else if auto_amendment_allowed { - try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules) - } else { + // resolution. Only effective aggregate Allow plus exact authored authority may + // authorize sandbox bypass. + bypass_sandbox: effective_full_policy_authority, + proposed_execpolicy_amendment: if !auto_amendment_allowed { None + } else if let Some(outer) = parsed_powershell_outer { + (!exact_outer_allow).then(|| ExecPolicyAmendment::new(outer.to_vec())) + } else { + try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules) }, }, - } + }; + narrow_requirement_for_shell_provenance(requirement, provenance) } pub(crate) async fn append_amendment_and_update( @@ -615,28 +1180,38 @@ impl ExecPolicyManager { } } -#[cfg(windows)] -fn create_untrusted_powershell_approval_requirement( +fn create_untrusted_wrapper_approval_requirement( exec_policy: &Policy, - outer_argv: &[String], - commands: &[Vec], + display_argv: &[String], + analysis: &PosixPolicyAnalysis, + allow_exact_opaque_wrapper_for_environment_runtime: bool, context: UnmatchedCommandContext<'_>, ) -> ExecApprovalRequirement { let match_options = MatchOptions { resolve_host_executables: true, }; - let inner_fallback = - |command: &[String]| render_decision_for_unmatched_command(command, context); - let mut matched_rules = Vec::new(); - let mut every_inner_explicit_allow = !commands.is_empty(); - for command in commands { + let mut every_candidate_explicit_allow = !analysis.candidates.is_empty(); + + // Preserve the parent's PowerShell match ordering (inner commands, then + // outer wrapper) while applying per-candidate origins and match options. + for candidate in analysis + .candidates + .iter() + .filter(|candidate| candidate.role == ExecPolicyCandidateRole::InnerCommand) + { + let candidate_context = UnmatchedCommandContext { + command_origin: candidate.command_origin, + ..context + }; + let inner_fallback = + |command: &[String]| render_decision_for_unmatched_command(command, candidate_context); let inner_matches = exec_policy.matches_for_command_with_options( - command, + &candidate.argv, Some(&inner_fallback), &match_options, ); - every_inner_explicit_allow &= !command.is_empty() + every_candidate_explicit_allow &= !candidate.argv.is_empty() && inner_matches.iter().any(|rule_match| { matches!( rule_match, @@ -655,32 +1230,45 @@ fn create_untrusted_powershell_approval_requirement( Decision::Prompt } }; - matched_rules.extend( - exec_policy - .matches_for_command_with_restrictive_host_rules(outer_argv, Some(&outer_fallback)), - ); - let raw_match_options = MatchOptions { resolve_host_executables: false, }; - let raw_full_outer_allow = exec_policy - .matches_for_command_with_options( - outer_argv, - /*heuristics_fallback*/ None, - &raw_match_options, - ) + for candidate in analysis + .candidates .iter() - .any(|rule_match| { - matches!( - rule_match, - RuleMatch::PrefixRuleMatch { - matched_prefix, - decision: Decision::Allow, - .. - } if matched_prefix.len() == outer_argv.len() - ) - }); - let composed_full_authority = raw_full_outer_allow && every_inner_explicit_allow; + .filter(|candidate| candidate.role == ExecPolicyCandidateRole::UntrustedWrapper) + { + matched_rules.extend(exec_policy.matches_for_command_with_restrictive_host_rules( + &candidate.argv, + Some(&outer_fallback), + )); + let raw_full_outer_allow = candidate + .argv + .first() + .is_some_and(|program| executable_spelling_is_absolute(program)) + && exec_policy + .matches_for_command_with_options( + &candidate.argv, + /*heuristics_fallback*/ None, + &raw_match_options, + ) + .iter() + .any(|rule_match| { + matches!( + rule_match, + RuleMatch::PrefixRuleMatch { + matched_prefix, + decision: Decision::Allow, + .. + } if matched_prefix.len() == candidate.argv.len() + ) + }); + every_candidate_explicit_allow &= raw_full_outer_allow; + } + + let complete_composed_full_authority = analysis.completeness + == PosixAnalysisCompleteness::Complete + && every_candidate_explicit_allow; let mut evaluation = Evaluation { decision: matched_rules @@ -690,6 +1278,53 @@ fn create_untrusted_powershell_approval_requirement( .unwrap_or(Decision::Forbidden), matched_rules, }; + + // Preserve configured-shell compatibility for a directly authored exact + // allow of an opaque nested shell argv (for example, `/bin/sh script`). + // This exception is unavailable to a locally model-resolved outer runtime, + // and it does not apply when any parsed inner leaf, Prompt, or Forbidden is + // present. + let exact_opaque_environment_wrapper_authority = + allow_exact_opaque_wrapper_for_environment_runtime + && analysis.completeness == PosixAnalysisCompleteness::Incomplete + && analysis + .candidates + .iter() + .all(|candidate| candidate.role == ExecPolicyCandidateRole::UntrustedWrapper) + && every_candidate_explicit_allow + && evaluation.decision == Decision::Allow; + let composed_full_authority = evaluation.decision == Decision::Allow + && (complete_composed_full_authority || exact_opaque_environment_wrapper_authority); + + if analysis.completeness == PosixAnalysisCompleteness::Incomplete + && evaluation.decision != Decision::Forbidden + && !exact_opaque_environment_wrapper_authority + { + if exec_policy.rules().iter_all().next().is_some() { + let reason = if analysis.contains_untrusted_wrapper { + "cannot completely inspect an untrusted shell wrapper" + } else { + "cannot completely inspect nested execution in this shell command" + }; + return ExecApprovalRequirement::Forbidden { + reason: format!( + "`{}` rejected: {reason} while command policy rules are active", + render_shlex_command(display_argv), + ), + }; + } + // With no command rules to conceal, incomplete analysis may proceed + // only through a callback-scoped prompt. Never let safe-command + // heuristics turn an opaque wrapper into a sandboxed Skip. + evaluation + .matched_rules + .push(RuleMatch::HeuristicsRuleMatch { + command: display_argv.to_vec(), + decision: Decision::Prompt, + }); + evaluation.decision = Decision::Prompt; + } + let permission_or_backend_gate = permission_delta_requires_outer_authority( context.permission_profile, context.sandbox_permissions, @@ -704,7 +1339,7 @@ fn create_untrusted_powershell_approval_requirement( evaluation .matched_rules .push(RuleMatch::HeuristicsRuleMatch { - command: outer_argv.to_vec(), + command: display_argv.to_vec(), decision: Decision::Prompt, }); evaluation.decision = evaluation @@ -717,7 +1352,7 @@ fn create_untrusted_powershell_approval_requirement( match evaluation.decision { Decision::Forbidden => ExecApprovalRequirement::Forbidden { - reason: derive_forbidden_reason(outer_argv, &evaluation), + reason: derive_forbidden_reason(display_argv, &evaluation), }, Decision::Prompt => { match prompt_is_rejected_by_policy( @@ -728,7 +1363,7 @@ fn create_untrusted_powershell_approval_requirement( reason: reason.to_string(), }, None => ExecApprovalRequirement::NeedsOneShotApproval { - reason: derive_prompt_reason(outer_argv, &evaluation), + reason: derive_prompt_reason(display_argv, &evaluation), }, } } @@ -739,6 +1374,63 @@ fn create_untrusted_powershell_approval_requirement( } } +#[cfg(windows)] +fn create_untrusted_powershell_approval_requirement( + exec_policy: &Policy, + outer_argv: &[String], + commands: &[Vec], + context: UnmatchedCommandContext<'_>, +) -> ExecApprovalRequirement { + let mut candidates = commands + .iter() + .cloned() + .map(|argv| ExecPolicyCandidate { + argv, + role: ExecPolicyCandidateRole::InnerCommand, + command_origin: context.command_origin, + }) + .collect::>(); + candidates.push(ExecPolicyCandidate { + argv: outer_argv.to_vec(), + role: ExecPolicyCandidateRole::UntrustedWrapper, + command_origin: ExecPolicyCommandOrigin::Generic, + }); + let analysis = PosixPolicyAnalysis { + candidates, + completeness: PosixAnalysisCompleteness::Complete, + contains_untrusted_wrapper: true, + script_bytes: 0, + }; + create_untrusted_wrapper_approval_requirement( + exec_policy, + outer_argv, + &analysis, + /*allow_exact_opaque_wrapper_for_environment_runtime*/ false, + context, + ) +} + +fn narrow_requirement_for_shell_provenance( + requirement: ExecApprovalRequirement, + provenance: ShellApprovalProvenance, +) -> ExecApprovalRequirement { + if !provenance.selection_is_model_supplied() { + return requirement; + } + + match requirement { + ExecApprovalRequirement::NeedsApproval { reason, .. } => { + ExecApprovalRequirement::NeedsOneShotApproval { reason } + } + ExecApprovalRequirement::Skip { bypass_sandbox, .. } => ExecApprovalRequirement::Skip { + bypass_sandbox, + proposed_execpolicy_amendment: None, + }, + ExecApprovalRequirement::NeedsOneShotApproval { .. } + | ExecApprovalRequirement::Forbidden { .. } => requirement, + } +} + impl Default for ExecPolicyManager { fn default() -> Self { Self::new(Arc::new(Policy::empty())) @@ -922,6 +1614,19 @@ pub(crate) fn render_decision_for_unmatched_command( && windows_sandbox_level == WindowsSandboxLevel::Disabled && profile_has_managed_filesystem_restrictions(permission_profile); + // A requested permission expansion is a separate authority boundary. It + // must be considered before a known-safe command can inherit trust from + // the generic safelist. Effective preapproved permissions are normalized + // to UseDefault before reaching exec-policy. + if permission_delta_requires_outer_authority(permission_profile, sandbox_permissions) { + return match approval_policy { + AskForApproval::Never => Decision::Forbidden, + AskForApproval::OnRequest + | AskForApproval::UnlessTrusted + | AskForApproval::Granular(_) => Decision::Prompt, + }; + } + if is_known_safe && !used_complex_parsing && (approval_policy == AskForApproval::UnlessTrusted diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index a84cfa6ca8..2c9008530f 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -83,6 +83,40 @@ fn starlark_string(value: &str) -> String { value.replace('\\', "\\\\").replace('"', "\\\"") } +fn prefix_rule_for(pattern: &[String], decision: &str) -> String { + let pattern = pattern + .iter() + .map(|token| format!("\"{}\"", starlark_string(token))) + .collect::>() + .join(", "); + format!("prefix_rule(pattern=[{pattern}], decision=\"{decision}\")") +} + +async fn requirement_with_provenance( + policy_src: Option<&str>, + command: &[String], + approval_policy: AskForApproval, + permission_profile: PermissionProfile, + sandbox_permissions: SandboxPermissions, + provenance: ShellApprovalProvenance, +) -> ExecApprovalRequirement { + let permission_expansion_was_requested = sandbox_permissions.requests_sandbox_override(); + ExecPolicyManager::new(policy_from_src(policy_src)) + .create_exec_approval_requirement_for_command_with_provenance( + ExecApprovalRequest { + command, + approval_policy, + permission_profile, + windows_sandbox_level: WindowsSandboxLevel::RestrictedToken, + sandbox_permissions, + prefix_rule: None, + }, + provenance, + permission_expansion_was_requested, + ) + .await +} + async fn write_project_trust_config( codex_home: &Path, trusted_projects: &[(&Path, TrustLevel)], @@ -906,14 +940,7 @@ EOF"# }, ExecApprovalRequirement::NeedsApproval { reason: None, - proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ - "zsh".to_string(), - "-lc".to_string(), - r#"cat <<'EOF' > /some/important/folder/test.txt -hello world -EOF"# - .to_string(), - ])), + proposed_execpolicy_amendment: None, }, ) .await; @@ -1142,6 +1169,977 @@ fn known_safe_on_request_still_prompts_for_restricted_sandbox_escalation() { ); } +#[test] +fn known_safe_sandbox_override_is_checked_before_the_safelist() { + let command = vec_str(&["echo", "hello"]); + let granular = GranularApprovalConfig { + sandbox_approval: true, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }; + + for (approval_policy, expected) in [ + (AskForApproval::OnRequest, Decision::Prompt), + (AskForApproval::UnlessTrusted, Decision::Prompt), + (AskForApproval::Granular(granular), Decision::Prompt), + (AskForApproval::Never, Decision::Forbidden), + ] { + assert_eq!( + render_decision_for_unmatched_command( + &command, + UnmatchedCommandContext { + approval_policy, + permission_profile: &PermissionProfile::workspace_write(), + windows_sandbox_level: WindowsSandboxLevel::RestrictedToken, + sandbox_permissions: SandboxPermissions::RequireEscalated, + used_complex_parsing: false, + command_origin: ExecPolicyCommandOrigin::Generic, + }, + ), + expected, + "{approval_policy:?}", + ); + } +} + +#[tokio::test] +async fn model_resolved_posix_wrapper_composes_exact_outer_and_every_inner() { + let outer = vec![ + host_program_path("sh"), + "-c".to_string(), + "echo ok".to_string(), + ]; + let inner_allow = prefix_rule_for(&vec_str(&["echo"]), "allow"); + let full_outer_allow = prefix_rule_for(&outer, "allow"); + let short_outer_allow = prefix_rule_for(&outer[..1], "allow"); + let basename_outer_allow = prefix_rule_for(&vec_str(&["sh"]), "allow"); + let basename_outer_prompt = prefix_rule_for(&vec_str(&["sh"]), "prompt"); + let basename_outer_forbidden = prefix_rule_for(&vec_str(&["sh"]), "forbidden"); + + assert_eq!( + requirement_with_provenance( + Some(&format!("{full_outer_allow}\n{inner_allow}")), + &outer, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + }, + ); + + assert_eq!( + requirement_with_provenance( + Some(&format!("{short_outer_allow}\n{inner_allow}")), + &outer, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + ); + + assert_eq!( + requirement_with_provenance( + Some(&format!("{basename_outer_allow}\n{inner_allow}")), + &outer, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::NeedsOneShotApproval { reason: None }, + ); + + assert_eq!( + requirement_with_provenance( + Some(&full_outer_allow), + &outer, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + "a heuristic-safe inner command is not explicit authority", + ); + + let rendered = render_shlex_command(&outer); + assert_eq!( + requirement_with_provenance( + Some(&format!( + "{full_outer_allow}\n{basename_outer_prompt}\n{inner_allow}" + )), + &outer, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::NeedsOneShotApproval { + reason: Some(format!("`{rendered}` requires approval by policy")), + }, + "a restrictive basename Prompt must survive an exact outer Allow", + ); + assert_eq!( + requirement_with_provenance( + Some(&format!( + "{full_outer_allow}\n{basename_outer_forbidden}\n{inner_allow}" + )), + &outer, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::Forbidden { + reason: format!("`{rendered}` rejected: policy forbids commands starting with `sh`"), + }, + "a restrictive basename Forbidden must survive an exact outer Allow", + ); + + let path_b_program = host_absolute_path(&[ + "workspace-b", + "bin", + if cfg!(windows) { "sh.exe" } else { "sh" }, + ]); + let path_b = vec![path_b_program, "-c".to_string(), "echo ok".to_string()]; + assert_eq!( + requirement_with_provenance( + Some(&format!("{full_outer_allow}\n{inner_allow}")), + &path_b, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::NeedsOneShotApproval { reason: None }, + "an exact rule for path A must not authorize path B", + ); +} + +#[tokio::test] +async fn model_resolved_posix_wrapper_uses_strictest_outer_and_inner_decision() { + let outer = vec![ + host_program_path("bash"), + "-lc".to_string(), + "echo first; later value".to_string(), + ]; + let full_outer_allow = prefix_rule_for(&outer, "allow"); + let echo_allow = prefix_rule_for(&vec_str(&["echo"]), "allow"); + let later_prompt = prefix_rule_for(&vec_str(&["later"]), "prompt"); + let later_forbidden = prefix_rule_for(&vec_str(&["later"]), "forbidden"); + let rendered = render_shlex_command(&outer); + + assert_eq!( + requirement_with_provenance( + Some(&format!("{full_outer_allow}\n{echo_allow}\n{later_prompt}")), + &outer, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::NeedsOneShotApproval { + reason: Some(format!("`{rendered}` requires approval by policy")), + }, + ); + + assert_eq!( + requirement_with_provenance( + Some(&format!( + "{full_outer_allow}\n{echo_allow}\n{later_forbidden}" + )), + &outer, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::Forbidden { + reason: format!("`{rendered}` rejected: policy forbids commands starting with `later`"), + }, + ); +} + +#[tokio::test] +async fn nested_posix_wrapper_cannot_hide_an_inner_forbidden_rule() { + let command = vec_str(&["bash", "-lc", "bash -lc 'rm -rf target'"]); + let rm_forbidden = prefix_rule_for(&vec_str(&["rm"]), "forbidden"); + + assert_eq!( + requirement_with_provenance( + Some(&rm_forbidden), + &command, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::configured(), + ) + .await, + ExecApprovalRequirement::Forbidden { + reason: format!( + "`{}` rejected: policy forbids commands starting with `rm`", + render_shlex_command(&command) + ), + }, + ); +} + +#[tokio::test] +async fn incomplete_untrusted_posix_analysis_is_one_shot_without_rules_and_terminal_with_rules() { + let opaque = vec_str(&["/workspace/bin/sh", "-c", "echo hello > marker.txt"]); + + assert_eq!( + requirement_with_provenance( + /*policy_src*/ None, + &opaque, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::NeedsOneShotApproval { reason: None }, + ); + + assert!(matches!( + requirement_with_provenance( + /*policy_src*/ None, + &opaque, + AskForApproval::Never, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::Forbidden { .. } + )); + + let unrelated_rule = prefix_rule_for(&vec_str(&["unrelated"]), "allow"); + assert_eq!( + requirement_with_provenance( + Some(&unrelated_rule), + &opaque, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::Forbidden { + reason: format!( + "`{}` rejected: cannot completely inspect an untrusted shell wrapper while command policy rules are active", + render_shlex_command(&opaque) + ), + }, + ); +} + +#[tokio::test] +async fn configured_incomplete_posix_body_cannot_hide_a_forbidden_descendant() { + let rm_forbidden = prefix_rule_for(&vec_str(&["rm"]), "forbidden"); + for command in [ + vec_str(&[ + "/bin/bash", + "-lc", + "echo ok > out; bash -lc 'rm -rf target'", + ]), + vec_str(&[ + "/bin/bash", + "-lc", + "for target in one; do rm -rf \"$target\"; done", + ]), + vec_str(&["/bin/bash", "-lc", r#"find . "$FLAGS" rm -rf {} ';' > out"#]), + vec_str(&["/bin/bash", "-lc", r#"env "$ARGS" > out"#]), + ] { + assert!(matches!( + requirement_with_provenance( + Some(&rm_forbidden), + &command, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::configured(), + ) + .await, + ExecApprovalRequirement::Forbidden { .. } + )); + + assert!(matches!( + requirement_with_provenance( + /*policy_src*/ None, + &command, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::configured(), + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + .. + } + )); + } +} + +#[tokio::test] +async fn opaque_non_posix_model_runtime_is_one_shot_without_rules_and_terminal_with_rules() { + let commands = [vec_str(&["cmd.exe", "/C", "echo hello"]), { + #[cfg(not(windows))] + { + vec_str(&["pwsh", "-Command", "Write-Output hello"]) + } + #[cfg(windows)] + { + vec_str(&["cmd.exe", "/D", "/C", "echo hello"]) + } + }]; + + for command in commands { + assert_eq!( + requirement_with_provenance( + /*policy_src*/ None, + &command, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::NeedsOneShotApproval { reason: None }, + "{command:?}", + ); + + let unrelated_rule = prefix_rule_for(&vec_str(&["unrelated"]), "allow"); + assert!( + matches!( + requirement_with_provenance( + Some(&unrelated_rule), + &command, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::Forbidden { .. } + ), + "{command:?}", + ); + } +} + +#[tokio::test] +async fn incomplete_untrusted_posix_analysis_respects_granular_sandbox_approval() { + let opaque = vec_str(&["/workspace/bin/sh", "-c", "echo hi > marker"]); + + for (sandbox_approval, expected) in [ + ( + false, + ExecApprovalRequirement::Forbidden { + reason: REJECT_SANDBOX_APPROVAL_REASON.to_string(), + }, + ), + ( + true, + ExecApprovalRequirement::NeedsOneShotApproval { reason: None }, + ), + ] { + assert_eq!( + requirement_with_provenance( + /*policy_src*/ None, + &opaque, + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval, + rules: false, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + expected, + "sandbox_approval={sandbox_approval}", + ); + } +} + +#[tokio::test] +async fn delegators_fail_closed_but_literal_shell_name_arguments_remain_complete() { + let echo_allow = prefix_rule_for(&vec_str(&["echo"]), "allow"); + for script in [ + "env bash -c 'echo hidden'", + "command ./bash -c 'echo hidden'", + "exec bash -c 'echo hidden'", + "eval 'echo hidden'", + ". ./payload.sh", + "source ./payload.sh", + "trap 'rm -rf target' EXIT", + "xargs rm -rf", + "find . -name target -exec rm -rf {} ';'", + "dash -c 'rm -rf target'", + "env FOO=bar -u bash", + r#""$CMD" arg"#, + ] { + let delegated = vec![ + "/workspace/bin/sh".to_string(), + "-c".to_string(), + script.to_string(), + ]; + for provenance in [ + ShellApprovalProvenance::local_model_resolved(), + ShellApprovalProvenance::configured(), + ] { + assert!( + matches!( + requirement_with_provenance( + Some(&echo_allow), + &delegated, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + provenance, + ) + .await, + ExecApprovalRequirement::Forbidden { .. } + ), + "{script}, {provenance:?}", + ); + } + } + + let literal = vec![ + host_program_path("sh"), + "-c".to_string(), + "echo bash".to_string(), + ]; + let full_outer_allow = prefix_rule_for(&literal, "allow"); + assert_eq!( + requirement_with_provenance( + Some(&format!("{full_outer_allow}\n{echo_allow}")), + &literal, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + }, + ); +} + +#[tokio::test] +async fn configured_delegators_without_rules_preserve_legacy_sandboxed_behavior() { + for script in [ + "env echo hi", + "sudo echo hi", + "xargs echo", + "find . -exec echo {} ';'", + ] { + let command = vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + script.to_string(), + ]; + for provenance in [ + ShellApprovalProvenance::configured(), + ShellApprovalProvenance::remote_model_hint(), + ] { + assert!( + matches!( + requirement_with_provenance( + /*policy_src*/ None, + &command, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + provenance, + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + .. + } + ), + "{script}, {provenance:?}", + ); + } + + assert_eq!( + requirement_with_provenance( + /*policy_src*/ None, + &command, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::NeedsOneShotApproval { reason: None }, + "{script}", + ); + } +} + +#[tokio::test] +async fn configured_exact_allow_can_authorize_an_opaque_nested_shell_only() { + let inner = vec![host_program_path("sh"), "/tmp/approved-script".to_string()]; + let command = vec![ + host_program_path("zsh"), + "-lc".to_string(), + shlex_try_join(inner.iter().map(String::as_str)).expect("quote nested shell command"), + ]; + let inner_allow = prefix_rule_for(&inner, "allow"); + + assert_eq!( + requirement_with_provenance( + Some(&inner_allow), + &command, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::RequireEscalated, + ShellApprovalProvenance::configured(), + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + }, + ); + + let outer_allow = prefix_rule_for(&command, "allow"); + assert!(matches!( + requirement_with_provenance( + Some(&format!("{outer_allow}\n{inner_allow}")), + &command, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::RequireEscalated, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + ExecApprovalRequirement::Forbidden { .. } + )); +} + +#[tokio::test] +async fn bare_nested_shell_allow_never_establishes_wrapper_authority() { + let outer_shell = host_program_path("zsh"); + let bare_wrapper = vec_str(&["bash", "-lc", "echo hello"]); + let bare = vec![ + outer_shell.clone(), + "-lc".to_string(), + shlex_try_join(bare_wrapper.iter().map(String::as_str)).expect("quote bare nested wrapper"), + ]; + let echo_allow = prefix_rule_for(&vec_str(&["echo"]), "allow"); + let bare_rules = format!("{}\n{echo_allow}", prefix_rule_for(&bare_wrapper, "allow")); + + assert_eq!( + requirement_with_provenance( + Some(&bare_rules), + &bare, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::configured(), + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + ); + + assert_eq!( + requirement_with_provenance( + Some(&bare_rules), + &bare, + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: false, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + PermissionProfile::workspace_write(), + SandboxPermissions::RequireEscalated, + ShellApprovalProvenance::configured(), + ) + .await, + ExecApprovalRequirement::Forbidden { + reason: REJECT_SANDBOX_APPROVAL_REASON.to_string(), + }, + ); + + let relative_wrapper = vec_str(&["./bash", "-lc", "echo hello"]); + let relative = vec![ + outer_shell.clone(), + "-lc".to_string(), + shlex_try_join(relative_wrapper.iter().map(String::as_str)) + .expect("quote relative nested wrapper"), + ]; + let relative_rules = format!( + "{}\n{echo_allow}", + prefix_rule_for(&relative_wrapper, "allow") + ); + assert_eq!( + requirement_with_provenance( + Some(&relative_rules), + &relative, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::configured(), + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + ); + + assert_eq!( + requirement_with_provenance( + Some(&relative_rules), + &relative, + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: false, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + PermissionProfile::workspace_write(), + SandboxPermissions::RequireEscalated, + ShellApprovalProvenance::configured(), + ) + .await, + ExecApprovalRequirement::Forbidden { + reason: REJECT_SANDBOX_APPROVAL_REASON.to_string(), + }, + ); + + let absolute_wrapper = vec![ + host_program_path("bash"), + "-lc".to_string(), + "echo hello".to_string(), + ]; + let absolute = vec![ + outer_shell.clone(), + "-lc".to_string(), + shlex_try_join(absolute_wrapper.iter().map(String::as_str)) + .expect("quote absolute nested wrapper"), + ]; + let absolute_rules = format!( + "{}\n{echo_allow}", + prefix_rule_for(&absolute_wrapper, "allow") + ); + assert_eq!( + requirement_with_provenance( + Some(&absolute_rules), + &absolute, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::RequireEscalated, + ShellApprovalProvenance::configured(), + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + }, + ); + + let opaque_bare = vec![outer_shell, "-lc".to_string(), "bash".to_string()]; + let opaque_bare_allow = prefix_rule_for(&vec_str(&["bash"]), "allow"); + assert!(matches!( + requirement_with_provenance( + Some(&opaque_bare_allow), + &opaque_bare, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::configured(), + ) + .await, + ExecApprovalRequirement::Forbidden { .. } + )); +} + +#[test] +fn wrapper_authority_uses_controller_native_absolute_paths() { + assert!(executable_spelling_is_absolute(&host_program_path("bash"))); + + let foreign_absolute = if cfg!(windows) { + "/usr/bin/bash" + } else { + r"C:\Program Files\Git\bin\bash.exe" + }; + assert!( + !executable_spelling_is_absolute(foreign_absolute), + "foreign-target absolute paths conservatively cannot establish bypass authority" + ); +} + +#[test] +fn posix_analysis_is_bounded_and_plain_find_is_not_a_delegator() { + for query in [ + vec_str(&["command", "-v", "bash"]), + vec_str(&["trap", "-p"]), + ] { + assert!( + !command_may_hide_nested_execution(&query), + "query-only form was classified as a delegator: {query:?}" + ); + } + for external in [ + vec_str(&["/repo/command", "-v", "bash"]), + vec_str(&["./trap", "-p"]), + vec_str(&["command.exe", "-v", "bash"]), + vec_str(&["COMMAND", "-v", "bash"]), + ] { + assert!( + command_may_hide_nested_execution(&external), + "external executable was treated as a shell builtin: {external:?}" + ); + } + + let mut nested_script = "echo leaf".to_string(); + for _ in 0..=MAX_POSIX_POLICY_DEPTH { + nested_script = format!( + "bash -lc {}", + shlex_try_join([nested_script.as_str()]).expect("quote nested script") + ); + } + let depth_limited = vec!["/bin/bash".to_string(), "-lc".to_string(), nested_script]; + let depth_analysis = analyze_posix_policy( + &depth_limited, + ShellApprovalProvenance::local_model_resolved(), + ) + .expect("POSIX analysis"); + assert_eq!( + depth_analysis.completeness, + PosixAnalysisCompleteness::Incomplete + ); + assert!(depth_analysis.contains_untrusted_wrapper); + + let candidate_script = (0..=MAX_POSIX_POLICY_CANDIDATES) + .map(|index| format!("echo value{index}")) + .collect::>() + .join("; "); + let candidate_limited = vec!["/bin/bash".to_string(), "-lc".to_string(), candidate_script]; + let candidate_analysis = + analyze_posix_policy(&candidate_limited, ShellApprovalProvenance::configured()) + .expect("POSIX analysis"); + assert_eq!( + candidate_analysis.completeness, + PosixAnalysisCompleteness::Incomplete + ); + assert_eq!( + candidate_analysis.candidates.len(), + MAX_POSIX_POLICY_CANDIDATES + ); + + let oversized = vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "x".repeat(MAX_POSIX_POLICY_SCRIPT_BYTES + 1), + ]; + let oversized_analysis = + analyze_posix_policy(&oversized, ShellApprovalProvenance::local_model_resolved()) + .expect("POSIX analysis"); + assert_eq!( + oversized_analysis.completeness, + PosixAnalysisCompleteness::Incomplete + ); + + let plain_find = vec_str(&["/bin/bash", "-lc", "find . -name target"]); + let plain_find_analysis = + analyze_posix_policy(&plain_find, ShellApprovalProvenance::configured()) + .expect("POSIX analysis"); + assert_eq!( + plain_find_analysis.completeness, + PosixAnalysisCompleteness::Complete + ); + assert!(!plain_find_analysis.contains_untrusted_wrapper); +} + +#[tokio::test] +async fn remote_model_hint_does_not_distrust_the_environment_runtime_but_never_amends() { + let safe = vec_str(&["echo", "hello"]); + assert_eq!( + requirement_with_provenance( + /*policy_src*/ None, + &safe, + AskForApproval::OnRequest, + PermissionProfile::workspace_write(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::remote_model_hint(), + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + }, + ); + + let unsafe_command = vec_str(&["cargo", "build"]); + assert_eq!( + requirement_with_provenance( + /*policy_src*/ None, + &unsafe_command, + AskForApproval::UnlessTrusted, + PermissionProfile::read_only(), + SandboxPermissions::UseDefault, + ShellApprovalProvenance::remote_model_hint(), + ) + .await, + ExecApprovalRequirement::NeedsOneShotApproval { reason: None }, + ); +} + +#[tokio::test] +async fn model_resolved_rule_and_permission_prompts_require_both_granular_categories() { + let command = vec_str(&["/workspace/bin/sh", "-c", "echo hello"]); + let policy_src = format!( + "{}\n{}\n{}", + prefix_rule_for(&command, "allow"), + prefix_rule_for(&vec_str(&["echo"]), "allow"), + prefix_rule_for(&vec_str(&["echo", "hello"]), "prompt"), + ); + let rendered = render_shlex_command(&command); + + for (rules, sandbox_approval, expected) in [ + ( + false, + true, + ExecApprovalRequirement::Forbidden { + reason: REJECT_RULES_APPROVAL_REASON.to_string(), + }, + ), + ( + true, + false, + ExecApprovalRequirement::Forbidden { + reason: REJECT_SANDBOX_APPROVAL_REASON.to_string(), + }, + ), + ( + true, + true, + ExecApprovalRequirement::NeedsOneShotApproval { + reason: Some(format!("`{rendered}` requires approval by policy")), + }, + ), + ] { + assert_eq!( + requirement_with_provenance( + Some(&policy_src), + &command, + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval, + rules, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + PermissionProfile::workspace_write(), + SandboxPermissions::RequireEscalated, + ShellApprovalProvenance::local_model_resolved(), + ) + .await, + expected, + "rules={rules}, sandbox_approval={sandbox_approval}", + ); + } +} + +#[tokio::test] +async fn model_selected_requested_prefix_cannot_reenable_an_amendment() { + let command = vec_str(&["/workspace/bin/sh", "-c", "echo hello"]); + let requirement = ExecPolicyManager::default() + .create_exec_approval_requirement_for_command_with_provenance( + ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::OnRequest, + permission_profile: PermissionProfile::workspace_write(), + windows_sandbox_level: WindowsSandboxLevel::RestrictedToken, + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: Some(vec_str(&["echo"])), + }, + ShellApprovalProvenance::local_model_resolved(), + /*permission_expansion_was_requested*/ false, + ) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::NeedsOneShotApproval { reason: None }, + ); +} + +#[tokio::test] +async fn preapproved_permission_expansion_cannot_generate_a_sticky_amendment() { + let command = vec_str(&["cargo", "build"]); + let manager = ExecPolicyManager::default(); + for (sandbox_permissions, permission_expansion_was_requested) in [ + (SandboxPermissions::UseDefault, true), + (SandboxPermissions::RequireEscalated, false), + ] { + let requirement = manager + .create_exec_approval_requirement_for_configured_command( + ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::UnlessTrusted, + permission_profile: PermissionProfile::read_only(), + windows_sandbox_level: WindowsSandboxLevel::RestrictedToken, + sandbox_permissions, + prefix_rule: Some(vec_str(&["cargo"])), + }, + permission_expansion_was_requested, + ) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::NeedsApproval { + reason: None, + proposed_execpolicy_amendment: None, + }, + "sandbox_permissions={sandbox_permissions:?}, permission_expansion_was_requested={permission_expansion_was_requested}", + ); + } +} + #[test] fn managed_cwd_write_profile_has_filesystem_restrictions() { let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ @@ -1233,10 +2231,7 @@ async fn exec_approval_requirement_prompts_for_inline_additional_permissions_und }, ExecApprovalRequirement::NeedsApproval { reason: None, - proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ - "touch".to_string(), - "requested-dir/requested-but-unused.txt".to_string(), - ])), + proposed_execpolicy_amendment: None, }, ) .await; @@ -1255,10 +2250,7 @@ async fn exec_approval_requirement_prompts_for_known_safe_escalation_under_on_re }, ExecApprovalRequirement::NeedsApproval { reason: None, - proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ - "echo".to_string(), - "hello".to_string(), - ])), + proposed_execpolicy_amendment: None, }, ) .await; @@ -1372,6 +2364,97 @@ async fn mixed_rule_and_sandbox_prompt_requires_every_granular_category_in_eithe } } +#[tokio::test] +async fn same_command_policy_prompt_and_sandbox_override_require_both_categories() { + let command = vec_str(&["git", "status"]); + let prompt_policies = [ + prefix_rule_for(&vec_str(&["git"]), "prompt"), + format!( + "{}\n{}", + prefix_rule_for(&vec_str(&["git"]), "allow"), + prefix_rule_for(&command, "prompt"), + ), + format!( + "{}\n{}", + prefix_rule_for(&vec_str(&["git"]), "prompt"), + prefix_rule_for(&command, "allow"), + ), + ]; + + for policy_src in prompt_policies { + for (rules, sandbox_approval, expected) in [ + ( + false, + true, + ExecApprovalRequirement::Forbidden { + reason: REJECT_RULES_APPROVAL_REASON.to_string(), + }, + ), + ( + true, + false, + ExecApprovalRequirement::Forbidden { + reason: REJECT_SANDBOX_APPROVAL_REASON.to_string(), + }, + ), + ( + true, + true, + ExecApprovalRequirement::NeedsApproval { + reason: Some(format!( + "`{}` requires approval by policy", + render_shlex_command(&command) + )), + proposed_execpolicy_amendment: None, + }, + ), + ] { + assert_eq!( + requirement_with_provenance( + Some(&policy_src), + &command, + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval, + rules, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + PermissionProfile::workspace_write(), + SandboxPermissions::RequireEscalated, + ShellApprovalProvenance::configured(), + ) + .await, + expected, + "policy={policy_src:?}, rules={rules}, sandbox_approval={sandbox_approval}", + ); + } + } + + let effective_allow = prefix_rule_for(&vec_str(&["git"]), "allow"); + assert_eq!( + requirement_with_provenance( + Some(&effective_allow), + &command, + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: false, + rules: false, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }), + PermissionProfile::workspace_write(), + SandboxPermissions::RequireEscalated, + ShellApprovalProvenance::configured(), + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + }, + ); +} + #[tokio::test] async fn exec_approval_requirement_falls_back_to_heuristics() { let command = vec!["cargo".to_string(), "build".to_string()]; @@ -1475,10 +2558,7 @@ async fn request_rule_uses_prefix_rule() { requirement, ExecApprovalRequirement::NeedsApproval { reason: None, - proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ - "cargo".to_string(), - "install".to_string(), - ])), + proposed_execpolicy_amendment: None, } ); } @@ -1507,11 +2587,7 @@ async fn request_rule_falls_back_when_prefix_rule_does_not_approve_all_commands( requirement, ExecApprovalRequirement::NeedsApproval { reason: None, - proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(vec![ - "rm".to_string(), - "-rf".to_string(), - "/tmp/codex".to_string(), - ])), + proposed_execpolicy_amendment: None, } ); } @@ -1747,7 +2823,7 @@ prefix_rule(pattern=["cat"], decision="allow") let command = vec![ "bash".to_string(), "-lc".to_string(), - "cat LOG.md && curl -fsSL https://example.invalid/setup.sh -o setup.sh && bash setup.sh" + "cat LOG.md && curl -fsSL https://example.invalid/setup.sh -o setup.sh && chmod +x setup.sh" .to_string(), ]; @@ -1775,7 +2851,7 @@ async fn multi_segment_shell_bypasses_sandbox_when_every_segment_matches_policy_ let policy_src = r#" prefix_rule(pattern=["cat"], decision="allow") prefix_rule(pattern=["curl"], decision="allow") -prefix_rule(pattern=["bash"], decision="allow") +prefix_rule(pattern=["chmod"], decision="allow") "#; assert_exec_approval_requirement_for_command( @@ -1784,7 +2860,7 @@ prefix_rule(pattern=["bash"], decision="allow") command: vec![ "bash".to_string(), "-lc".to_string(), - "cat LOG.md && curl -fsSL https://example.invalid/setup.sh -o setup.sh && bash setup.sh" + "cat LOG.md && curl -fsSL https://example.invalid/setup.sh -o setup.sh && chmod +x setup.sh" .to_string(), ], approval_policy: AskForApproval::OnRequest, diff --git a/codex-rs/core/src/shell.rs b/codex-rs/core/src/shell.rs index ccd1ea20d2..38ddeefca2 100644 --- a/codex-rs/core/src/shell.rs +++ b/codex-rs/core/src/shell.rs @@ -1,7 +1,10 @@ use codex_exec_server::ShellInfo; use codex_shell_command::shell_detect::DetectedShell; +use codex_shell_command::shell_detect::ModelShellResolveError; use serde::Deserialize; use serde::Serialize; +use std::ffi::OsStr; +use std::path::Path; use std::path::PathBuf; pub use codex_shell_command::shell_detect::ShellType; @@ -81,10 +84,30 @@ fn ultimate_fallback_shell() -> Shell { codex_shell_command::shell_detect::ultimate_fallback_shell().into() } +/// Legacy configured-shell compatibility helper. +/// +/// Production model-selected shell input must use +/// [`resolve_model_provided_shell_in`] so a missing executable cannot fall +/// back to a different shell. pub fn get_shell_by_model_provided_path(shell_path: &PathBuf) -> Shell { codex_shell_command::shell_detect::get_shell_by_model_provided_path(shell_path).into() } +pub(crate) fn resolve_model_provided_shell_in( + shell_path: &Path, + search_path: &OsStr, + path_ext: Option<&OsStr>, + cwd: &Path, +) -> Result { + codex_shell_command::shell_detect::resolve_model_provided_shell_in( + shell_path, + search_path, + path_ext, + cwd, + ) + .map(Into::into) +} + pub fn get_shell(shell_type: ShellType, path: Option<&PathBuf>) -> Option { codex_shell_command::shell_detect::get_shell(shell_type, path).map(Into::into) } diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 87a9301093..abac1c4494 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -165,22 +165,37 @@ async fn run_exec_like(args: RunExecLikeArgs) -> Result bool { false } -#[derive(Debug)] +#[derive(Debug, Eq, PartialEq)] pub(crate) struct ResolvedCommand { pub(crate) command: Vec, pub(crate) shell_type: ShellType, + pub(crate) shell_approval_provenance: ShellApprovalProvenance, } fn post_unified_exec_tool_use_payload( @@ -96,7 +96,8 @@ fn post_unified_exec_tool_use_payload( pub(crate) fn get_command( args: &ExecCommandArgs, - session_shell: Arc, + selected_shell: Arc, + shell_approval_provenance: ShellApprovalProvenance, shell_mode: &UnifiedExecShellMode, allow_login_shell: bool, ) -> Result { @@ -111,17 +112,11 @@ pub(crate) fn get_command( }; match shell_mode { - UnifiedExecShellMode::Direct => { - let model_shell = args - .shell - .as_ref() - .map(|shell_str| get_shell_by_model_provided_path(&PathBuf::from(shell_str))); - let shell = model_shell.as_ref().unwrap_or(session_shell.as_ref()); - Ok(ResolvedCommand { - command: shell.derive_exec_args(&args.cmd, use_login_shell), - shell_type: shell.shell_type, - }) - } + UnifiedExecShellMode::Direct => Ok(ResolvedCommand { + command: selected_shell.derive_exec_args(&args.cmd, use_login_shell), + shell_type: selected_shell.shell_type, + shell_approval_provenance, + }), UnifiedExecShellMode::ZshFork(zsh_fork_config) => { if args.shell.is_some() { return Err( @@ -136,6 +131,7 @@ pub(crate) fn get_command( args.cmd.clone(), ], shell_type: ShellType::Zsh, + shell_approval_provenance: ShellApprovalProvenance::configured(), }) } } diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index 546ad459e1..50db9d351d 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -1,8 +1,11 @@ use std::path::Path; use std::sync::Arc; +use crate::exec_env::create_env; +use crate::exec_policy::ShellApprovalProvenance; use crate::function_tool::FunctionCallError; use crate::maybe_emit_implicit_skill_invocation; +use crate::shell::resolve_model_provided_shell_in; use crate::tools::context::ExecCommandToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; @@ -32,7 +35,7 @@ use codex_otel::TOOL_CALL_UNIFIED_EXEC_METRIC; use codex_sandboxing::SandboxManager; use codex_sandboxing::SandboxType; use codex_sandboxing::SandboxablePreference; -use codex_shell_command::shell_detect::detect_shell_type; +use codex_shell_command::shell_detect::detect_shell_type_from_hint; use codex_tools::ToolName; use codex_tools::ToolSpec; use codex_utils_output_truncation::approx_token_count; @@ -58,6 +61,29 @@ pub struct ExecCommandHandler { options: ExecCommandHandlerOptions, } +#[cfg(windows)] +pub(super) fn shell_environment_value<'a>( + environment: &'a std::collections::HashMap, + name: &str, +) -> Option<&'a str> { + if let Some(value) = environment.get(name) { + return Some(value); + } + environment + .iter() + .filter(|(key, _)| key.eq_ignore_ascii_case(name)) + .min_by(|(left, _), (right, _)| left.cmp(right)) + .map(|(_, value)| value.as_str()) +} + +#[cfg(not(windows))] +pub(super) fn shell_environment_value<'a>( + environment: &'a std::collections::HashMap, + name: &str, +) -> Option<&'a str> { + environment.get(name).map(String::as_str) +} + impl Default for ExecCommandHandler { fn default() -> Self { Self { @@ -204,41 +230,83 @@ impl ExecCommandHandler { shell_mode_for_environment(&turn.unified_exec_shell_mode, environment.as_ref()); // Remote environments may use a different OS and must build commands with their native // shell; fall back to the session shell when the environment did not report one. - let shell = turn_environment + let environment_shell = turn_environment .shell .clone() .map(Arc::new) .unwrap_or_else(|| session.user_shell()); - // TODO(anp): Resolve requested shells in remote environments instead of restricting - // commands to the reported default shell. - if environment.is_remote() - && let Some(requested_shell) = args.shell.take() - { - let Some(remote_shell) = turn_environment.shell.as_ref() else { - return Err(FunctionCallError::RespondToModel(format!( - "environment `{}` does not report a shell", - turn_environment.environment_id - ))); - }; - if detect_shell_type(Path::new(&requested_shell)) != Some(remote_shell.shell_type) { - return Err(FunctionCallError::RespondToModel(format!( - "environment `{}` only supports `{}`", - turn_environment.environment_id, - remote_shell.name() - ))); + let (selected_shell, shell_approval_provenance) = if environment.is_remote() { + match args.shell.take() { + Some(requested_shell) => { + let Some(remote_shell) = turn_environment.shell.as_ref() else { + return Err(FunctionCallError::RespondToModel(format!( + "environment `{}` does not report a shell", + turn_environment.environment_id + ))); + }; + if detect_shell_type_from_hint(&requested_shell) + != Some(remote_shell.shell_type) + { + return Err(FunctionCallError::RespondToModel(format!( + "environment `{}` only supports `{}`", + turn_environment.environment_id, + remote_shell.name() + ))); + } + ( + Arc::new(remote_shell.clone()), + ShellApprovalProvenance::remote_model_hint(), + ) + } + None => (environment_shell, ShellApprovalProvenance::configured()), } - } - let process_id = manager.allocate_process_id().await; + } else if matches!(&shell_mode, codex_tools::UnifiedExecShellMode::Direct) { + match args.shell.as_deref() { + Some(requested_shell) => { + let resolution_cwd = native_cwd.as_ref().ok_or_else(|| { + FunctionCallError::RespondToModel( + "cannot resolve a local shell against a foreign working directory" + .to_string(), + ) + })?; + let shell_env = create_env( + &turn.config.permissions.shell_environment_policy, + /*thread_id*/ None, + ); + let search_path = + shell_environment_value(&shell_env, "PATH").unwrap_or_default(); + let path_ext = + shell_environment_value(&shell_env, "PATHEXT").map(std::ffi::OsStr::new); + let resolved_shell = resolve_model_provided_shell_in( + Path::new(requested_shell), + std::ffi::OsStr::new(search_path), + path_ext, + resolution_cwd.as_path(), + ) + .map_err(|err| FunctionCallError::RespondToModel(err.to_string()))?; + ( + Arc::new(resolved_shell), + ShellApprovalProvenance::local_model_resolved(), + ) + } + None => (environment_shell, ShellApprovalProvenance::configured()), + } + } else { + (environment_shell, ShellApprovalProvenance::configured()) + }; let resolved_command = get_command( &args, - shell, + selected_shell, + shell_approval_provenance, &shell_mode, turn.config.permissions.allow_login_shell, ) .map_err(FunctionCallError::RespondToModel)?; let command = resolved_command.command; let shell_type = resolved_command.shell_type; + let shell_approval_provenance = resolved_command.shell_approval_provenance; let command_for_display = codex_shell_command::parse_command::shlex_join(&command); + let process_id = manager.allocate_process_id().await; let ExecCommandArgs { tty, @@ -361,6 +429,7 @@ impl ExecCommandHandler { .permissions_preapproved, justification, prefix_rule, + shell_approval_provenance, }, &context, ) diff --git a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs index 6f859a8cc4..a41065219a 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec_tests.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec_tests.rs @@ -1,4 +1,6 @@ use super::*; +use crate::exec_policy::ShellApprovalProvenance; +use crate::shell::Shell; use crate::shell::ShellType; use crate::shell::default_user_shell; use codex_exec_server::Environment; @@ -7,6 +9,8 @@ use codex_tools::ZshForkConfig; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_output_truncation::TruncationPolicy; use pretty_assertions::assert_eq; +use std::collections::HashMap; +use std::path::PathBuf; use std::sync::Arc; use crate::session::step_context::StepContext; @@ -22,6 +26,46 @@ use tokio::sync::Mutex; const TEST_TRUNCATION_POLICY: TruncationPolicy = TruncationPolicy::Tokens(10_000); +fn selected_shell(shell_type: ShellType, shell_path: impl Into) -> Arc { + Arc::new(Shell { + shell_type, + shell_path: shell_path.into(), + }) +} + +#[test] +fn shell_environment_lookup_prefers_exact_canonical_key() { + let environment = HashMap::from([ + ("Path".to_string(), "inherited-path".to_string()), + ("PATH".to_string(), "configured-path".to_string()), + ("PathExt".to_string(), ".CMD".to_string()), + ("PATHEXT".to_string(), ".EXE".to_string()), + ]); + + assert_eq!( + super::exec_command::shell_environment_value(&environment, "PATH"), + Some("configured-path") + ); + assert_eq!( + super::exec_command::shell_environment_value(&environment, "PATHEXT"), + Some(".EXE") + ); +} + +#[test] +fn shell_environment_lookup_case_folds_only_on_windows() { + let environment = HashMap::from([("Path".to_string(), "inherited-path".to_string())]); + + assert_eq!( + super::exec_command::shell_environment_value(&environment, "PATH"), + if cfg!(windows) { + Some("inherited-path") + } else { + None + } + ); +} + async fn invocation_for_payload( tool_name: &str, call_id: &str, @@ -53,6 +97,7 @@ fn test_get_command_uses_default_shell_when_unspecified() -> anyhow::Result<()> let resolved = get_command( &args, Arc::new(default_user_shell()), + ShellApprovalProvenance::configured(), &UnifiedExecShellMode::Direct, /*allow_login_shell*/ true, ) @@ -61,33 +106,42 @@ fn test_get_command_uses_default_shell_when_unspecified() -> anyhow::Result<()> assert_eq!(command.len(), 3); assert_eq!(command[2], "echo hello"); + assert_eq!( + resolved.shell_approval_provenance, + ShellApprovalProvenance::configured() + ); Ok(()) } #[test] -fn test_get_command_respects_explicit_bash_shell() -> anyhow::Result<()> { - let json = r#"{"cmd": "echo hello", "shell": "/bin/bash"}"#; +fn test_get_command_uses_already_resolved_explicit_bash_shell() -> anyhow::Result<()> { + let json = r#"{"cmd": "echo hello", "shell": "bash"}"#; let args: ExecCommandArgs = parse_arguments(json)?; - assert_eq!(args.shell.as_deref(), Some("/bin/bash")); + assert_eq!(args.shell.as_deref(), Some("bash")); + let resolved_path = if cfg!(windows) { + r"C:\resolved\bash.exe" + } else { + "/resolved/bash" + }; let resolved = get_command( &args, - Arc::new(default_user_shell()), + selected_shell(ShellType::Bash, resolved_path), + ShellApprovalProvenance::local_model_resolved(), &UnifiedExecShellMode::Direct, /*allow_login_shell*/ true, ) .map_err(anyhow::Error::msg)?; let command = resolved.command; - assert_eq!(command.last(), Some(&"echo hello".to_string())); - if command - .iter() - .any(|arg| arg.eq_ignore_ascii_case("-Command")) - { - assert!(command.contains(&"-NoProfile".to_string())); - } + assert_eq!(command.first().map(String::as_str), Some(resolved_path)); + assert_eq!(command.last().map(String::as_str), Some("echo hello")); + assert_eq!( + resolved.shell_approval_provenance, + ShellApprovalProvenance::local_model_resolved() + ); Ok(()) } @@ -115,7 +169,8 @@ fn test_get_command_respects_explicit_powershell_shell() -> anyhow::Result<()> { let resolved = get_command( &args, - Arc::new(default_user_shell()), + selected_shell(ShellType::PowerShell, powershell_path), + ShellApprovalProvenance::local_model_resolved(), &UnifiedExecShellMode::Direct, /*allow_login_shell*/ true, ) @@ -124,6 +179,10 @@ fn test_get_command_respects_explicit_powershell_shell() -> anyhow::Result<()> { assert_eq!(command[2], "echo hello"); assert_eq!(resolved.shell_type, ShellType::PowerShell); + assert_eq!( + resolved.shell_approval_provenance, + ShellApprovalProvenance::local_model_resolved() + ); Ok(()) } @@ -137,7 +196,8 @@ fn test_get_command_respects_explicit_cmd_shell() -> anyhow::Result<()> { let resolved = get_command( &args, - Arc::new(default_user_shell()), + selected_shell(ShellType::Cmd, "cmd"), + ShellApprovalProvenance::local_model_resolved(), &UnifiedExecShellMode::Direct, /*allow_login_shell*/ true, ) @@ -145,6 +205,46 @@ fn test_get_command_respects_explicit_cmd_shell() -> anyhow::Result<()> { let command = resolved.command; assert_eq!(command[2], "echo hello"); + assert_eq!( + resolved.shell_approval_provenance, + ShellApprovalProvenance::local_model_resolved() + ); + Ok(()) +} + +#[test] +fn test_get_command_preserves_remote_hint_provenance_after_discarding_spelling() +-> anyhow::Result<()> { + let json = r#"{"cmd": "pwd", "shell": "/attacker/bash"}"#; + let mut args: ExecCommandArgs = parse_arguments(json)?; + assert_eq!(args.shell.take().as_deref(), Some("/attacker/bash")); + let reported_shell = if cfg!(windows) { + r"C:\environment\bash.exe" + } else { + "/environment/bash" + }; + + let resolved = get_command( + &args, + selected_shell(ShellType::Bash, reported_shell), + ShellApprovalProvenance::remote_model_hint(), + &UnifiedExecShellMode::Direct, + /*allow_login_shell*/ false, + ) + .map_err(anyhow::Error::msg)?; + + assert_eq!( + resolved, + ResolvedCommand { + command: vec![ + reported_shell.to_string(), + "-c".to_string(), + "pwd".to_string(), + ], + shell_type: ShellType::Bash, + shell_approval_provenance: ShellApprovalProvenance::remote_model_hint(), + } + ); Ok(()) } @@ -156,6 +256,7 @@ fn test_get_command_rejects_explicit_login_when_disallowed() -> anyhow::Result<( let err = get_command( &args, Arc::new(default_user_shell()), + ShellApprovalProvenance::configured(), &UnifiedExecShellMode::Direct, /*allow_login_shell*/ false, ) @@ -189,6 +290,7 @@ fn test_get_command_rejects_explicit_shell_in_zsh_fork_mode() -> anyhow::Result< let err = get_command( &args, Arc::new(default_user_shell()), + ShellApprovalProvenance::configured(), &shell_mode, /*allow_login_shell*/ true, ) @@ -201,6 +303,39 @@ fn test_get_command_rejects_explicit_shell_in_zsh_fork_mode() -> anyhow::Result< Ok(()) } +#[test] +fn test_get_command_marks_zsh_fork_configured() -> anyhow::Result<()> { + let args: ExecCommandArgs = parse_arguments(r#"{"cmd": "echo hello"}"#)?; + let shell_zsh_path = AbsolutePathBuf::from_absolute_path(if cfg!(windows) { + r"C:\opt\codex\zsh" + } else { + "/opt/codex/zsh" + })?; + let shell_mode = UnifiedExecShellMode::ZshFork(ZshForkConfig { + shell_zsh_path, + main_execve_wrapper_exe: AbsolutePathBuf::from_absolute_path(if cfg!(windows) { + r"C:\opt\codex\codex-execve-wrapper" + } else { + "/opt/codex/codex-execve-wrapper" + })?, + }); + + let resolved = get_command( + &args, + Arc::new(default_user_shell()), + ShellApprovalProvenance::configured(), + &shell_mode, + /*allow_login_shell*/ false, + ) + .map_err(anyhow::Error::msg)?; + + assert_eq!( + resolved.shell_approval_provenance, + ShellApprovalProvenance::configured() + ); + Ok(()) +} + #[tokio::test] async fn shell_mode_for_environment_uses_direct_mode_for_remote_environments() -> anyhow::Result<()> { diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 00868b7d4d..488426966d 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -7,6 +7,7 @@ the process manager to spawn PTYs once an ExecRequest is prepared. use crate::command_canonicalization::canonicalize_command_for_approval; use crate::exec::ExecCapturePolicy; use crate::exec::ExecExpiration; +use crate::exec_policy::ShellApprovalProvenance; use crate::guardian::GuardianApprovalRequest; use crate::guardian::GuardianNetworkAccessTrigger; use crate::guardian::review_approval_request; @@ -43,11 +44,13 @@ use crate::unified_exec::UnifiedExecProcess; use crate::unified_exec::UnifiedExecProcessManager; use codex_network_proxy::ManagedNetworkSandboxContext; use codex_network_proxy::NetworkProxy; +use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::error::CodexErr; use codex_protocol::error::SandboxErr; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::protocol::ReviewDecision; use codex_sandboxing::SandboxCommand; +use codex_sandboxing::SandboxType; use codex_sandboxing::SandboxablePreference; use codex_shell_command::powershell::prefix_powershell_script_with_utf8; use codex_tools::UnifiedExecShellMode; @@ -80,6 +83,7 @@ pub struct UnifiedExecRequest { pub additional_permissions_preapproved: bool, pub justification: Option, pub exec_approval_requirement: ExecApprovalRequirement, + pub shell_approval_provenance: ShellApprovalProvenance, } /// Cache key for approval decisions that can be reused across equivalent @@ -134,6 +138,57 @@ fn build_unified_exec_sandbox_command( }) } +fn maybe_wrap_environment_shell_command_with_snapshot( + command: &[String], + shell_approval_provenance: ShellApprovalProvenance, + session_shell: &crate::shell::Shell, + shell_snapshot: Option<&codex_utils_absolute_path::AbsolutePathBuf>, + explicit_env_overrides: &HashMap, + env: &HashMap, + runtime_path_prepends: &RuntimePathPrepends, +) -> Vec { + if shell_approval_provenance.is_local_model_resolved() { + return command.to_vec(); + } + + maybe_wrap_shell_lc_with_snapshot( + command, + session_shell, + shell_snapshot, + explicit_env_overrides, + env, + runtime_path_prepends, + ) +} + +fn apply_post_policy_powershell_runtime_adjustments( + command: &[String], + shell_type: ShellType, + shell_approval_provenance: ShellApprovalProvenance, + sandbox: SandboxType, + windows_sandbox_level: WindowsSandboxLevel, +) -> Vec { + // A locally model-selected executable is part of the command identity that + // was resolved and evaluated by policy. Do not mutate its approved argv at + // runtime; configured and remote-environment shells retain the established + // profile and UTF-8 adjustments. + if shell_approval_provenance.is_local_model_resolved() { + return command.to_vec(); + } + + let command = disable_powershell_profile_for_elevated_windows_sandbox( + command, + Some(&shell_type), + sandbox, + windows_sandbox_level, + ); + if matches!(shell_type, ShellType::PowerShell) { + prefix_powershell_script_with_utf8(&command) + } else { + command + } +} + impl<'a> UnifiedExecRuntime<'a> { /// Creates a runtime bound to the shared unified-exec process manager. pub fn new(manager: &'a UnifiedExecProcessManager, shell_mode: UnifiedExecShellMode) -> Self { @@ -384,8 +439,9 @@ impl<'a> ToolRuntime for UnifiedExecRunt let command = if environment_is_remote { base_command.to_vec() } else { - maybe_wrap_shell_lc_with_snapshot( + maybe_wrap_environment_shell_command_with_snapshot( base_command, + req.shell_approval_provenance, shell, shell_snapshot_location.as_ref(), &explicit_env_overrides, @@ -393,17 +449,13 @@ impl<'a> ToolRuntime for UnifiedExecRunt &runtime_path_prepends, ) }; - let command = disable_powershell_profile_for_elevated_windows_sandbox( + let command = apply_post_policy_powershell_runtime_adjustments( &command, - Some(&req.shell_type), + req.shell_type, + req.shell_approval_provenance, attempt.sandbox, attempt.windows_sandbox_level, ); - let command = if matches!(req.shell_type, ShellType::PowerShell) { - prefix_powershell_script_with_utf8(&command) - } else { - command - }; if let UnifiedExecShellMode::ZshFork(zsh_fork_config) = &self.shell_mode { let command = build_unified_exec_sandbox_command( @@ -713,6 +765,99 @@ mod tests { assert_eq!(decision, ReviewDecision::Approved); } + #[cfg(unix)] + #[test] + fn local_model_resolved_shell_reaches_spawn_command_without_snapshot_wrapper() { + let temp_dir = tempdir().expect("create snapshot temp dir"); + let snapshot_path = temp_dir.path().join("shell.snapshot"); + std::fs::write(&snapshot_path, "export SNAPSHOT_MARKER=1\n").expect("write shell snapshot"); + let snapshot = AbsolutePathBuf::try_from(snapshot_path).expect("absolute snapshot path"); + let session_shell = crate::shell::Shell { + shell_type: ShellType::Sh, + shell_path: "/bin/sh".into(), + }; + let command = vec![ + "/workspace/fake/sh".to_string(), + "-lc".to_string(), + "echo exact".to_string(), + ]; + let explicit_env_overrides = HashMap::new(); + let env = HashMap::new(); + let runtime_path_prepends = RuntimePathPrepends::default(); + + let model_resolved = maybe_wrap_environment_shell_command_with_snapshot( + &command, + ShellApprovalProvenance::local_model_resolved(), + &session_shell, + Some(&snapshot), + &explicit_env_overrides, + &env, + &runtime_path_prepends, + ); + let configured = maybe_wrap_environment_shell_command_with_snapshot( + &command, + ShellApprovalProvenance::configured(), + &session_shell, + Some(&snapshot), + &explicit_env_overrides, + &env, + &runtime_path_prepends, + ); + + assert_eq!(model_resolved, command); + assert_eq!(model_resolved[0], "/workspace/fake/sh"); + assert_ne!(configured, model_resolved); + assert_eq!(configured[0], "/bin/sh"); + } + + #[test] + fn local_model_resolved_powershell_argv_is_not_mutated_after_policy() { + let command = vec![ + "C:/workspace/fake/powershell.exe".to_string(), + "-Command".to_string(), + "Write-Output exact".to_string(), + ]; + let transformed = vec![ + "C:/workspace/fake/powershell.exe".to_string(), + "-NoProfile".to_string(), + "-Command".to_string(), + format!( + "{}Write-Output exact", + codex_shell_command::powershell::UTF8_OUTPUT_PREFIX + ), + ]; + + for (name, provenance, expected) in [ + ( + "local model-resolved", + ShellApprovalProvenance::local_model_resolved(), + &command, + ), + ( + "configured", + ShellApprovalProvenance::configured(), + &transformed, + ), + ( + "remote model hint", + ShellApprovalProvenance::remote_model_hint(), + &transformed, + ), + ] { + assert_eq!( + apply_post_policy_powershell_runtime_adjustments( + &command, + ShellType::PowerShell, + provenance, + SandboxType::WindowsRestrictedToken, + WindowsSandboxLevel::Elevated, + ), + *expected, + "{name} behavior changed" + ); + } + } + #[tokio::test] async fn unified_exec_uses_the_trusted_sandbox_cwd() { let cwd_dir = tempdir().expect("create process temp dir"); @@ -745,6 +890,7 @@ mod tests { bypass_sandbox: false, proposed_execpolicy_amendment: None, }, + shell_approval_provenance: ShellApprovalProvenance::configured(), }; assert_eq!( @@ -844,6 +990,7 @@ mod tests { additional_permissions_preapproved: false, justification: None, exec_approval_requirement, + shell_approval_provenance: ShellApprovalProvenance::configured(), } } diff --git a/codex-rs/core/src/unified_exec/mod.rs b/codex-rs/core/src/unified_exec/mod.rs index 34d88c8149..bfd13a347f 100644 --- a/codex-rs/core/src/unified_exec/mod.rs +++ b/codex-rs/core/src/unified_exec/mod.rs @@ -36,6 +36,7 @@ use rand::Rng; use rand::rng; use tokio::sync::Mutex; +use crate::exec_policy::ShellApprovalProvenance; use crate::sandboxing::SandboxPermissions; use crate::session::session::Session; use crate::session::turn_context::TurnContext; @@ -107,6 +108,7 @@ pub(crate) struct ExecCommandRequest { pub additional_permissions_preapproved: bool, pub justification: Option, pub prefix_rule: Option>, + pub shell_approval_provenance: ShellApprovalProvenance, } #[derive(Debug)] diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index ef22918cd2..f2ee1793d2 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -1134,18 +1134,23 @@ impl UnifiedExecProcessManager { .session .services .exec_policy - .create_exec_approval_requirement_for_command(ExecApprovalRequest { - command: &request.command, - approval_policy: context.turn.approval_policy.value(), - permission_profile: context.turn.permission_profile(), - windows_sandbox_level: context.turn.windows_sandbox_level, - sandbox_permissions: if request.additional_permissions_preapproved { - crate::sandboxing::SandboxPermissions::UseDefault - } else { - request.sandbox_permissions + .create_exec_approval_requirement_for_command_with_provenance( + ExecApprovalRequest { + command: &request.command, + approval_policy: context.turn.approval_policy.value(), + permission_profile: context.turn.permission_profile(), + windows_sandbox_level: context.turn.windows_sandbox_level, + sandbox_permissions: if request.additional_permissions_preapproved { + crate::sandboxing::SandboxPermissions::UseDefault + } else { + request.sandbox_permissions + }, + prefix_rule: request.prefix_rule.clone(), }, - prefix_rule: request.prefix_rule.clone(), - }) + request.shell_approval_provenance, + /*permission_expansion_was_requested*/ + request.sandbox_permissions.requests_sandbox_override(), + ) .await; let req = UnifiedExecToolRequest { command: request.command.clone(), @@ -1172,6 +1177,7 @@ impl UnifiedExecProcessManager { additional_permissions_preapproved: request.additional_permissions_preapproved, justification: request.justification.clone(), exec_approval_requirement, + shell_approval_provenance: request.shell_approval_provenance, }; let tool_ctx = ToolCtx { session: context.session.clone(), diff --git a/codex-rs/core/src/unified_exec/process_manager_tests.rs b/codex-rs/core/src/unified_exec/process_manager_tests.rs index 096cbe0a13..4af7bccfae 100644 --- a/codex-rs/core/src/unified_exec/process_manager_tests.rs +++ b/codex-rs/core/src/unified_exec/process_manager_tests.rs @@ -292,6 +292,7 @@ async fn failed_initial_end_for_unstored_process_uses_fallback_output() { additional_permissions_preapproved: false, justification: None, prefix_rule: None, + shell_approval_provenance: crate::exec_policy::ShellApprovalProvenance::configured(), }; let transcript = Arc::new(tokio::sync::Mutex::new(HeadTailBuffer::default())); diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index cc33286f16..8d6da51b15 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -1045,7 +1045,7 @@ fn scenarios() -> Vec { outcome: Outcome::ExecApprovalWithAmendment { decision: ReviewDecision::Denied, expected_reason: None, - expected_execpolicy_amendment: Some(&["echo", "known-safe-escalation"]), + expected_execpolicy_amendment: None, }, expectation: Expectation::CommandFailure { output_contains: "rejected by user", @@ -1792,6 +1792,164 @@ fn scenarios() -> Vec { ] } +#[cfg(unix)] +#[test_case("./ls", "ls" ; "direct_relative_executable")] +#[test_case("./zsh -c ls", "zsh" ; "configured_shell_with_relative_nested_shell")] +#[tokio::test(flavor = "current_thread")] +async fn fake_shell_sandbox_override_prompts_before_execution( + authored_command: &str, + executable_name: &str, +) -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let approval_policy = AskForApproval::OnRequest; + let sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let sandbox_policy_for_config = sandbox_policy.clone(); + let configured_shell = + codex_core::shell::get_shell_by_model_provided_path(&PathBuf::from("/bin/sh")); + let expected_command = + configured_shell.derive_exec_args(authored_command, /*use_login_shell*/ false); + let mut builder = test_codex() + .with_user_shell(configured_shell) + .with_config(move |config| { + config.permissions.allow_login_shell = false; + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config + .set_legacy_sandbox_policy(sandbox_policy_for_config) + .expect("set workspace-write sandbox policy"); + config.approvals_reviewer = ApprovalsReviewer::User; + }); + let test = builder.build(&server).await?; + + let fake_executable = test.cwd.path().join(executable_name); + let sentinel = test.cwd.path().join(format!("{executable_name}.started")); + fs::write( + &fake_executable, + "#!/bin/sh\nprintf '%s\\n' started > \"$0.started\"\n", + )?; + let mut permissions = fs::metadata(&fake_executable)?.permissions(); + permissions.set_mode(0o755); + fs::set_permissions(&fake_executable, permissions)?; + assert!(!sentinel.exists(), "sentinel must start absent"); + + let call_id = format!("fake-shell-sandbox-override-{executable_name}"); + let event = shell_event( + &call_id, + authored_command, + /*timeout_ms*/ 1_000, + SandboxPermissions::RequireEscalated, + )?; + mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-fake-shell-1"), + event, + ev_completed("resp-fake-shell-1"), + ]), + ) + .await; + let results_mock = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-fake-shell-1", "done"), + ev_completed("resp-fake-shell-2"), + ]), + ) + .await; + + submit_turn( + &test, + "run the fake shell with an explicit sandbox override", + approval_policy, + sandbox_policy, + ) + .await?; + + // Shell-command begin/end events describe the logical tool call, not the + // child-process lifetime: the begin event intentionally precedes policy + // evaluation. The executable sentinel is the authority boundary here. + let approval = loop { + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) + | EventMsg::ExecCommandBegin(_) + | EventMsg::ExecCommandEnd(_) + | EventMsg::TurnComplete(_) + ) + }) + .await; + assert!( + !sentinel.exists(), + "fake executable must not run before approval" + ); + match event { + EventMsg::ExecApprovalRequest(approval) => break approval, + EventMsg::ExecCommandBegin(_) => continue, + EventMsg::ExecCommandEnd(end) => { + panic!("logical shell call ended before approval: {end:?}") + } + EventMsg::TurnComplete(_) => panic!("expected approval before turn completion"), + _ => unreachable!(), + } + }; + assert_eq!(approval.call_id, call_id); + assert_eq!(approval.command, expected_command); + assert_eq!(approval.proposed_execpolicy_amendment, None); + assert!( + !sentinel.exists(), + "fake executable must remain stopped while approval is pending" + ); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Denied, + }) + .await?; + + loop { + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecCommandEnd(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + assert!( + !sentinel.exists(), + "denying approval must not run the fake executable" + ); + if matches!(event, EventMsg::TurnComplete(_)) { + break; + } + } + assert!( + !sentinel.exists(), + "denying approval must not run the fake executable" + ); + + let output = parse_result( + &results_mock + .single_request() + .function_call_output(call_id.as_str()), + ); + assert!( + output.stdout.contains("rejected by user"), + "tool result should report the normal user rejection: {}", + output.stdout + ); + + Ok(()) +} + #[test_case(ScenarioGroup::DangerFullAccess ; "danger_full_access")] #[test_case(ScenarioGroup::ReadOnly ; "read_only")] #[test_case(ScenarioGroup::WorkspaceWrite ; "workspace_write")] @@ -3211,7 +3369,7 @@ exec {remote_bash_exec} "$@" #[tokio::test(flavor = "current_thread")] #[cfg(unix)] -async fn invalid_requested_prefix_rule_falls_back_for_compound_command() -> Result<()> { +async fn permission_expansion_does_not_offer_fallback_rule_for_compound_command() -> Result<()> { let server = start_mock_server().await; let approval_policy = AskForApproval::OnRequest; let sandbox_policy = SandboxPolicy::new_read_only_policy(); @@ -3230,7 +3388,7 @@ async fn invalid_requested_prefix_rule_falls_back_for_compound_command() -> Resu let event = shell_event_with_prefix_rule( call_id, command, - /*timeout_ms*/ 1_000, + /*timeout_ms*/ 10_000, SandboxPermissions::RequireEscalated, Some(vec!["touch".to_string()]), )?; @@ -3244,6 +3402,14 @@ async fn invalid_requested_prefix_rule_falls_back_for_compound_command() -> Resu ]), ) .await; + let _results = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-invalid-prefix-no-amendment", "done"), + ev_completed("resp-invalid-prefix-no-amendment-results"), + ]), + ) + .await; submit_turn( &test, @@ -3254,17 +3420,22 @@ async fn invalid_requested_prefix_rule_falls_back_for_compound_command() -> Resu .await?; let approval = expect_exec_approval(&test, command).await; - let amendment = approval - .proposed_execpolicy_amendment - .expect("should have a proposed execpolicy amendment"); - assert!(amendment.command.contains(&command.to_string())); + assert_eq!(approval.proposed_execpolicy_amendment, None); + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Denied, + }) + .await?; + wait_for_completion(&test).await; Ok(()) } #[tokio::test(flavor = "current_thread")] #[cfg(unix)] -async fn approving_fallback_rule_for_compound_command_works() -> Result<()> { +async fn approving_permission_expansion_does_not_persist_compound_command() -> Result<()> { let server = start_mock_server().await; let approval_policy = AskForApproval::OnRequest; let sandbox_policy = SandboxPolicy::new_read_only_policy(); @@ -3283,7 +3454,7 @@ async fn approving_fallback_rule_for_compound_command_works() -> Result<()> { let event = shell_event_with_prefix_rule( call_id, command, - /*timeout_ms*/ 1_000, + /*timeout_ms*/ 10_000, SandboxPermissions::RequireEscalated, Some(vec!["touch".to_string()]), )?; @@ -3297,6 +3468,14 @@ async fn approving_fallback_rule_for_compound_command_works() -> Result<()> { ]), ) .await; + let first_results = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-invalid-prefix-first", "done"), + ev_completed("resp-invalid-prefix-first-results"), + ]), + ) + .await; submit_turn( &test, @@ -3308,29 +3487,27 @@ async fn approving_fallback_rule_for_compound_command_works() -> Result<()> { let approval = expect_exec_approval(&test, command).await; let approval_id = approval.effective_approval_id(); - let amendment = approval - .proposed_execpolicy_amendment - .expect("should have a proposed execpolicy amendment"); - assert!(amendment.command.contains(&command.to_string())); + assert_eq!(approval.proposed_execpolicy_amendment, None); test.codex .submit(Op::ExecApproval { id: approval_id, turn_id: None, - decision: ReviewDecision::ApprovedExecpolicyAmendment { - proposed_execpolicy_amendment: amendment.clone(), - }, + decision: ReviewDecision::Approved, }) .await?; wait_for_completion(&test).await; + let first_output = parse_result(&first_results.single_request().function_call_output(call_id)); + assert_eq!(first_output.exit_code.unwrap_or(0), 0); + let call_id = "invalid-prefix-rule-again"; let command = "touch /tmp/codex-fallback-rule-test.txt && echo hello > /tmp/codex-fallback-rule-test.txt"; let event = shell_event_with_prefix_rule( call_id, command, - /*timeout_ms*/ 1_000, + /*timeout_ms*/ 10_000, SandboxPermissions::RequireEscalated, Some(vec!["touch".to_string()]), )?; @@ -3361,16 +3538,24 @@ async fn approving_fallback_rule_for_compound_command_works() -> Result<()> { ) .await?; - wait_for_completion_without_approval(&test).await; + let approval = expect_exec_approval(&test, command).await; + assert_eq!(approval.proposed_execpolicy_amendment, None); + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::Denied, + }) + .await?; + wait_for_completion(&test).await; let second_output = parse_result( &second_results .single_request() .function_call_output(call_id), ); - assert_eq!(second_output.exit_code.unwrap_or(0), 0); assert!( - second_output.stdout.is_empty(), + second_output.stdout.contains("rejected by user"), "unexpected stdout: {}", second_output.stdout ); diff --git a/codex-rs/core/tests/suite/exec_policy.rs b/codex-rs/core/tests/suite/exec_policy.rs index 6f2483315e..7992d491a4 100644 --- a/codex-rs/core/tests/suite/exec_policy.rs +++ b/codex-rs/core/tests/suite/exec_policy.rs @@ -8,12 +8,12 @@ use codex_protocol::config_types::Settings; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; -#[cfg(windows)] +#[cfg(any(unix, windows))] use codex_protocol::protocol::ExecApprovalPurpose; use codex_protocol::protocol::Op; -#[cfg(windows)] +#[cfg(any(unix, windows))] use codex_protocol::protocol::ReviewDecision; -#[cfg(windows)] +#[cfg(any(unix, windows))] use codex_protocol::protocol::TurnAbortReason; use codex_protocol::user_input::UserInput; use core_test_support::responses::ev_assistant_message; @@ -30,6 +30,8 @@ use core_test_support::wait_for_event; use serde_json::Value; use serde_json::json; use std::fs; +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; #[cfg(windows)] use std::path::PathBuf; @@ -103,7 +105,6 @@ fn installed_windows_powershell() -> PathBuf { .into_path_buf() } -#[cfg(windows)] fn enable_unified_exec(config: &mut codex_core::config::Config) { config .features @@ -111,6 +112,164 @@ fn enable_unified_exec(config: &mut codex_core::config::Config) { .expect("test config should allow feature update"); } +#[cfg(unix)] +#[tokio::test] +async fn unified_exec_model_selected_posix_shell_requires_one_shot_approval_before_execution() +-> Result<()> { + let server = start_mock_server().await; + let mut builder = test_codex().with_config(enable_unified_exec); + let test = builder.build(&server).await?; + + let model_shell_dir = test.config.cwd.join("model-shell"); + fs::create_dir_all(&model_shell_dir)?; + let model_shell = model_shell_dir.join("sh"); + let sentinel = test.config.cwd.join("model-shell-started.txt"); + let sentinel_arg = sentinel + .to_str() + .expect("the sentinel path must be valid UTF-8") + .replace('\'', "'\"'\"'"); + fs::write( + &model_shell, + format!("#!/bin/sh\nprintf started > '{sentinel_arg}'\nexec /bin/sh \"$@\"\n"), + )?; + fs::set_permissions(&model_shell, fs::Permissions::from_mode(0o755))?; + let requested_shell = "./model-shell/sh"; + let resolved_model_shell = model_shell + .to_str() + .expect("the model-selected shell path must be valid UTF-8") + .to_string(); + + let expected_command = vec![resolved_model_shell, "-c".to_string(), "ls".to_string()]; + let call_id = "unified-exec-model-selected-posix-one-shot"; + let args = json!({ + "shell": requested_shell, + "cmd": "ls", + "login": false, + "yield_time_ms": 1_000, + }); + mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-model-selected-posix-1"), + ev_function_call(call_id, "exec_command", &serde_json::to_string(&args)?), + ev_completed("resp-model-selected-posix-1"), + ]), + ) + .await; + let results_mock = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("msg-model-selected-posix-1", "done"), + ev_completed("resp-model-selected-posix-2"), + ]), + ) + .await; + + submit_user_turn( + &test, + "run a read-only command with a model-selected POSIX shell", + AskForApproval::UnlessTrusted, + PermissionProfile::Disabled, + /*collaboration_mode*/ None, + ) + .await?; + + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) + | EventMsg::ExecCommandBegin(_) + | EventMsg::ExecCommandEnd(_) + | EventMsg::TurnAborted(_) + | EventMsg::TurnComplete(_) + ) + }) + .await; + assert!( + !sentinel.exists(), + "the model-selected POSIX shell must not start before approval" + ); + let approval = match event { + EventMsg::ExecApprovalRequest(approval) => approval, + EventMsg::ExecCommandBegin(begin) => { + panic!("model-selected POSIX shell began before approval: {begin:?}") + } + EventMsg::ExecCommandEnd(end) => { + panic!("model-selected POSIX shell completed before approval: {end:?}") + } + EventMsg::TurnAborted(aborted) => { + panic!("turn aborted before one-shot approval: {aborted:?}") + } + EventMsg::TurnComplete(_) => panic!("expected one-shot approval before completion"), + _ => unreachable!(), + }; + assert_eq!(approval.call_id, call_id); + assert_eq!(approval.command, expected_command); + assert_eq!(approval.cwd, test.config.cwd); + let approval_id = approval + .approval_id + .clone() + .expect("one-shot approval must carry a callback ID"); + assert_ne!(approval_id, call_id); + assert_eq!( + approval.approval_purpose, + Some(ExecApprovalPurpose::Initial) + ); + assert_eq!( + approval.effective_approval_purpose(), + ExecApprovalPurpose::Initial + ); + assert_eq!( + approval.effective_available_decisions(), + vec![ReviewDecision::Approved, ReviewDecision::Abort] + ); + assert_eq!(approval.proposed_execpolicy_amendment, None); + assert!(!sentinel.exists()); + + let approval_turn_id = approval.turn_id.clone(); + test.codex + .submit(Op::ExecApproval { + id: approval_id, + turn_id: Some(approval_turn_id.clone()), + decision: ReviewDecision::Abort, + }) + .await?; + + let aborted = match wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecCommandBegin(_) + | EventMsg::ExecCommandEnd(_) + | EventMsg::TurnAborted(_) + | EventMsg::TurnComplete(_) + ) + }) + .await + { + EventMsg::TurnAborted(aborted) => aborted, + EventMsg::ExecCommandBegin(begin) => { + panic!("model-selected POSIX shell began after abort: {begin:?}") + } + EventMsg::ExecCommandEnd(end) => { + panic!("model-selected POSIX shell completed after abort: {end:?}") + } + EventMsg::TurnComplete(_) => panic!("aborted approval unexpectedly completed the turn"), + _ => unreachable!(), + }; + assert_eq!(aborted.turn_id.as_deref(), Some(approval_turn_id.as_str())); + assert_eq!(aborted.reason, TurnAbortReason::Interrupted); + assert!( + !sentinel.exists(), + "aborting one-shot approval must never start the model-selected POSIX shell" + ); + assert!( + results_mock.requests().is_empty(), + "aborting one-shot approval must not continue the turn" + ); + + Ok(()) +} + #[cfg(windows)] #[tokio::test] async fn unified_exec_workspace_powershell_path_requires_one_shot_approval_before_execution() diff --git a/codex-rs/core/tests/suite/remote_env.rs b/codex-rs/core/tests/suite/remote_env.rs index 5f8c503496..c05efe2964 100644 --- a/codex-rs/core/tests/suite/remote_env.rs +++ b/codex-rs/core/tests/suite/remote_env.rs @@ -248,32 +248,27 @@ async fn remote_test_env_exposes_target_shell_to_model() -> Result<()> { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn explicit_remote_shell_runs_in_remote_cwd() -> Result<()> { +async fn compatible_remote_path_hint_uses_environment_shell_in_remote_cwd() -> Result<()> { const CALL_ID: &str = "remote-explicit-shell"; skip_if_no_remote_env!(Ok(())); - let (shell, command) = match test_target_os() { - TestTargetOs::Linux => ( - "bash", - r#"case "$PWD" in /tmp/codex-core-test-cwd-*) ;; *) echo "unexpected cwd: $PWD" >&2; exit 1 ;; esac"#, - ), - TestTargetOs::Windows => ( - "powershell", - r#"$cwd = (Get-Location).Path; if ($cwd -notlike 'C:\codex-core-test-cwd-*') { Write-Error "unexpected cwd: $cwd"; exit 1 }"#, - ), + let (shell, expected_cwd_prefix) = match test_target_os() { + TestTargetOs::Linux => ("/attacker/bash", "/tmp/codex-core-test-cwd-"), + TestTargetOs::Windows => (r"C:\attacker\PowerShell.ExE", r"C:\codex-core-test-cwd-"), TestTargetOs::MacOs => unreachable!("remote test targets do not run macOS"), }; let server = start_mock_server().await; let arguments = serde_json::to_string(&json!({ - "cmd": command, + "cmd": "pwd", "shell": shell, "login": false, "yield_time_ms": 10_000, }))?; let mut builder = test_codex().with_config(|config| { config.use_experimental_unified_exec_tool = true; + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::UnlessTrusted); config .features .enable(Feature::UnifiedExec) @@ -312,9 +307,14 @@ async fn explicit_remote_shell_runs_in_remote_cwd() -> Result<()> { .function_call_output_content_and_success(CALL_ID) .context("remote shell tool result should be present")?; assert_ne!(success, Some(false)); + let output = output.context("remote shell command should return output")?; assert!( - output.is_some_and(|output| output.contains("Process exited with code 0")), - "remote shell command should exit successfully", + output.contains("Process exited with code 0"), + "remote shell command should exit successfully: {output}", + ); + assert!( + output.contains(expected_cwd_prefix), + "remote shell command should run in the remote cwd: {output}", ); Ok(()) diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 42e0214fa1..8579fe8dfb 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -2024,6 +2024,162 @@ async fn write_stdin_returns_exit_metadata_and_clears_session() -> Result<()> { Ok(()) } +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn model_shell_resolution_failure_precedes_process_allocation_and_events() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + + let server = start_mock_server().await; + + let mut builder = test_codex().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 invalid_call_id = "uexec-missing-model-shell"; + let invalid_args = serde_json::json!({ + "shell": "./missing/sh", + "cmd": "true", + "login": false, + }); + let start_call_id = "uexec-after-missing-model-shell"; + let start_args = serde_json::json!({ + "cmd": "tail -f /dev/null", + "yield_time_ms": 10, + "tty": false, + }); + let interrupt_call_id = "uexec-after-missing-model-shell-interrupt"; + let interrupt_args = serde_json::json!({ + "chars": "\u{3}", + "session_id": 1000, + "yield_time_ms": 1000, + }); + + let responses = vec![ + sse(vec![ + ev_response_created("resp-missing-shell-1"), + ev_function_call( + invalid_call_id, + "exec_command", + &serde_json::to_string(&invalid_args)?, + ), + ev_completed("resp-missing-shell-1"), + ]), + sse(vec![ + ev_response_created("resp-missing-shell-2"), + ev_function_call( + start_call_id, + "exec_command", + &serde_json::to_string(&start_args)?, + ), + ev_completed("resp-missing-shell-2"), + ]), + sse(vec![ + ev_response_created("resp-missing-shell-3"), + ev_function_call( + interrupt_call_id, + "write_stdin", + &serde_json::to_string(&interrupt_args)?, + ), + ev_completed("resp-missing-shell-3"), + ]), + sse(vec![ + ev_response_created("resp-missing-shell-4"), + ev_assistant_message("msg-missing-shell", "done"), + ev_completed("resp-missing-shell-4"), + ]), + ]; + let request_log = mount_sse_sequence(&server, responses).await; + + submit_unified_exec_turn( + &test, + "resolve a model-selected shell before allocating a process", + PermissionProfile::Disabled, + ) + .await?; + + let mut valid_begin = None; + loop { + let event = wait_for_event(&test.codex, |_| true).await; + match event { + EventMsg::ExecApprovalRequest(approval) if approval.call_id == invalid_call_id => { + panic!("shell resolution failure must precede approval: {approval:?}") + } + EventMsg::ExecCommandBegin(begin) if begin.call_id == invalid_call_id => { + panic!("shell resolution failure must precede begin events: {begin:?}") + } + EventMsg::ExecCommandEnd(end) if end.call_id == invalid_call_id => { + panic!("shell resolution failure must precede end events: {end:?}") + } + EventMsg::ExecCommandBegin(begin) if begin.call_id == start_call_id => { + assert!( + valid_begin.is_none(), + "expected one begin event for the valid follow-up command" + ); + valid_begin = Some(begin); + } + EventMsg::TurnComplete(_) => break, + _ => {} + } + } + + let valid_begin = valid_begin.expect("the valid follow-up command should begin normally"); + assert_eq!( + valid_begin.process_id.as_deref(), + Some("1000"), + "failed shell resolution must not consume the first process ID" + ); + + let invalid_output = request_log + .function_call_output_text(invalid_call_id) + .expect("missing typed shell-resolution failure output"); + assert!( + invalid_output.contains("model-provided shell") + && invalid_output.contains("does not exist"), + "unexpected shell-resolution failure: {invalid_output:?}" + ); + assert!( + !invalid_output.contains("Process running with session ID"), + "failed shell resolution must not claim a running process: {invalid_output:?}" + ); + + let start_output = parse_unified_exec_output( + &request_log + .function_call_output_text(start_call_id) + .expect("missing valid follow-up exec_command output"), + )?; + assert_eq!( + start_output.process_id.as_deref(), + Some("1000"), + "the first successfully allocated process should retain deterministic ID 1000" + ); + assert!( + start_output.exit_code.is_none(), + "the valid process should still be running before write_stdin interrupts it" + ); + + let interrupt_output = parse_unified_exec_output( + &request_log + .function_call_output_text(interrupt_call_id) + .expect("missing write_stdin interrupt output"), + )?; + assert!( + interrupt_output.process_id.is_none(), + "write_stdin should clear the interrupted process from the session map" + ); + assert_eq!( + interrupt_output.exit_code, + Some(130), + "write_stdin should preserve the normal Unix SIGINT exit status" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn write_stdin_ctrl_c_interrupts_non_tty_session() -> Result<()> { // TODO(anp): Add a target-Windows test for explicit interrupt handling. diff --git a/codex-rs/core/tests/suite/unified_exec_process_events.rs b/codex-rs/core/tests/suite/unified_exec_process_events.rs index 8ebd332ea2..fb5e258394 100644 --- a/codex-rs/core/tests/suite/unified_exec_process_events.rs +++ b/codex-rs/core/tests/suite/unified_exec_process_events.rs @@ -46,6 +46,12 @@ enum PushedExecScenario { ReplayGap, } +#[derive(Debug)] +struct ExecServerObservation { + process_read_requests: usize, + process_start_argv: Vec, +} + async fn read_exec_server_json(websocket: &mut WebSocketStream) -> Value { loop { match timeout(Duration::from_secs(5), websocket.next()) @@ -109,7 +115,7 @@ async fn send_environment_info(websocket: &mut WebSocketStream) { async fn serve_exec_with_pushed_events( listener: TcpListener, scenario: PushedExecScenario, -) -> usize { +) -> ExecServerObservation { let mut websocket = accept_initialized_exec_server(listener).await; send_environment_info(&mut websocket).await; @@ -144,6 +150,16 @@ async fn serve_exec_with_pushed_events( .as_str() .expect("process/start should include processId") .to_string(); + let process_start_argv = process_start["params"]["argv"] + .as_array() + .expect("process/start should include argv") + .iter() + .map(|arg| { + arg.as_str() + .expect("process/start argv entries should be strings") + .to_string() + }) + .collect(); let replay_output = |seq| -> &'static [u8] { match seq { @@ -350,7 +366,10 @@ async fn serve_exec_with_pushed_events( }), ) .await; - return process_read_requests; + return ExecServerObservation { + process_read_requests, + process_start_argv, + }; } method => panic!("unexpected exec-server request: {method:?}"), } @@ -448,7 +467,7 @@ async fn exec_command_consumes_pushed_remote_process_events( _ => {} } } - let process_read_requests = timeout(Duration::from_secs(5), exec_server) + let observation = timeout(Duration::from_secs(5), exec_server) .await .context("fake exec-server should observe process cleanup")??; let request = response_mock @@ -464,26 +483,161 @@ async fn exec_command_consumes_pushed_remote_process_events( assert!(saw_exec_command_begin); assert!(output.contains("Process exited with code 0")); assert!(output.contains(COMPLETE_OUTPUT)); - assert_eq!(process_read_requests, 0, "unexpected compatibility read"); + assert_eq!( + observation.process_read_requests, 0, + "unexpected compatibility read" + ); } PushedExecScenario::DirectDenied => { assert!(!saw_exec_command_begin); assert!(output.contains("Process exited with code 1")); - assert_eq!(process_read_requests, 0, "unexpected compatibility read"); + assert_eq!( + observation.process_read_requests, 0, + "unexpected compatibility read" + ); } PushedExecScenario::LegacyExit => { assert!(!saw_exec_command_begin); assert!(output.contains("Process exited with code 1")); - assert_eq!(process_read_requests, 1, "expected compatibility read"); + assert_eq!( + observation.process_read_requests, 1, + "expected compatibility read" + ); } PushedExecScenario::ReplayGap => { assert_ne!(success, Some(false)); assert!(saw_exec_command_begin); assert_eq!(output.matches(RECOVERED_OUTPUT).count(), 1); assert_eq!(output.matches(RETAINED_OUTPUT).count(), 1); - assert_eq!(process_read_requests, 1, "expected replay recovery read"); + assert_eq!( + observation.process_read_requests, 1, + "expected replay recovery read" + ); } } Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_explicit_shell_hint_uses_environment_reported_executable() -> Result<()> { + const MODEL_SHELL_HINT: &str = r"C:\attacker\ZSH.ExE"; + const COMMAND: &str = "echo remote-hint-ok"; + + let listener = TcpListener::bind("127.0.0.1:0").await?; + let server = start_mock_server().await; + let response_mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call( + CALL_ID, + "exec_command", + &json!({ + "shell": MODEL_SHELL_HINT, + "cmd": COMMAND, + "login": false, + "yield_time_ms": 1_000, + }) + .to_string(), + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", "done"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + let exec_server_url = format!("ws://{}", listener.local_addr()?); + let exec_server = tokio::spawn(serve_exec_with_pushed_events( + listener, + PushedExecScenario::Complete, + )); + let mut builder = test_codex() + .with_exec_server_url(exec_server_url) + .with_config(|config| { + config.project_doc_max_bytes = 0; + config.use_experimental_unified_exec_tool = true; + config + .features + .enable(Feature::UnifiedExec) + .expect("test config should allow feature update"); + }); + let test = timeout(Duration::from_secs(5), builder.build(&server)) + .await + .context("thread startup should connect to the fake exec-server")??; + + let (sandbox_policy, permission_profile) = + turn_permission_fields(PermissionProfile::Disabled, test.config.cwd.as_path()); + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "run a remote command with an explicit shell hint".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: codex_protocol::protocol::ThreadSettingsOverrides { + approval_policy: Some(AskForApproval::Never), + sandbox_policy: Some(sandbox_policy), + permission_profile, + collaboration_mode: Some(codex_protocol::config_types::CollaborationMode { + mode: codex_protocol::config_types::ModeKind::Default, + settings: codex_protocol::config_types::Settings { + model: test.session_configured.model.clone(), + reasoning_effort: None, + developer_instructions: None, + }, + }), + ..Default::default() + }, + }) + .await?; + loop { + let event = timeout(Duration::from_secs(5), test.codex.next_event()) + .await + .context("turn should complete")?? + .msg; + if matches!(event, EventMsg::TurnComplete(_)) { + break; + } + } + + let observation = timeout(Duration::from_secs(5), exec_server) + .await + .context("fake exec-server should observe process cleanup")??; + assert_eq!( + observation.process_start_argv, + ["/bin/zsh", "-c", COMMAND], + "the model hint must not replace the environment-reported shell" + ); + assert!( + observation + .process_start_argv + .iter() + .all(|arg| !arg.to_ascii_lowercase().contains("attacker")), + "process/start must not contain the model-supplied path" + ); + assert_eq!( + observation.process_read_requests, 0, + "unexpected compatibility read" + ); + + let request = response_mock + .last_request() + .context("model should receive the exec_command output")?; + let (output, success) = request + .function_call_output_content_and_success(CALL_ID) + .context("exec_command output should be model visible")?; + let output = output.context("exec_command output should contain text")?; + assert_ne!(success, Some(false)); + assert!(output.contains("Process exited with code 0")); + assert!(output.contains(COMPLETE_OUTPUT)); + + Ok(()) +} diff --git a/codex-rs/exec-server/src/local_process.rs b/codex-rs/exec-server/src/local_process.rs index 3c488f0ad7..5a3382c269 100644 --- a/codex-rs/exec-server/src/local_process.rs +++ b/codex-rs/exec-server/src/local_process.rs @@ -1120,6 +1120,43 @@ mod tests { assert_eq!(child_env(¶ms), expected); } + #[cfg(target_os = "windows")] + #[test] + fn child_env_policy_rebuild_coalesces_windows_shell_search_variables() { + let mut params = test_exec_params(HashMap::new()); + params.env_policy = Some(ExecEnvPolicy { + inherit: ShellEnvironmentPolicyInherit::None, + ignore_default_excludes: true, + exclude: Vec::new(), + r#set: HashMap::from([ + ("Path".to_string(), r"C:\configured-alias".to_string()), + ("PATH".to_string(), r"C:\configured-canonical".to_string()), + ("PathExt".to_string(), ".EXE;.CMD".to_string()), + ]), + include_only: Vec::new(), + }); + + let env = child_env(¶ms); + + assert_eq!( + env.get("PATH").map(String::as_str), + Some(r"C:\configured-canonical") + ); + assert_eq!(env.get("PATHEXT").map(String::as_str), Some(".EXE;.CMD")); + assert_eq!( + env.keys() + .filter(|key| key.eq_ignore_ascii_case("PATH")) + .count(), + 1 + ); + assert_eq!( + env.keys() + .filter(|key| key.eq_ignore_ascii_case("PATHEXT")) + .count(), + 1 + ); + } + #[tokio::test] async fn exit_before_shutdown_records_success() { let (backend, metrics, exporter) = telemetry_backend(); diff --git a/codex-rs/protocol/src/shell_environment.rs b/codex-rs/protocol/src/shell_environment.rs index 9edb91c2f3..2a356d896e 100644 --- a/codex-rs/protocol/src/shell_environment.rs +++ b/codex-rs/protocol/src/shell_environment.rs @@ -40,9 +40,50 @@ where env_map.insert("PATHEXT".to_string(), ".COM;.EXE;.BAT;.CMD".to_string()); } } + #[cfg(target_os = "windows")] + normalize_windows_shell_search_environment(&mut env_map, policy); env_map } +/// Canonicalize the case-insensitive variables that control Windows +/// executable lookup before the environment is used for resolution or launch. +/// +/// The map is case-sensitive even on Windows, so inherited and configured +/// spellings can otherwise coexist. A configured entry wins over an inherited +/// entry, and the canonical spelling wins if the policy itself contains more +/// than one case variant. +#[cfg(any(target_os = "windows", test))] +fn normalize_windows_shell_search_environment( + env: &mut HashMap, + policy: &ShellEnvironmentPolicy, +) { + for canonical_key in ["PATH", "PATHEXT"] { + let configured_value = windows_environment_value(&policy.r#set, canonical_key) + .filter(|(key, value)| env.get(*key) == Some(*value)) + .map(|(_, value)| value.clone()); + let value = configured_value.or_else(|| { + windows_environment_value(env, canonical_key).map(|(_, value)| value.clone()) + }); + + env.retain(|key, _| !key.eq_ignore_ascii_case(canonical_key)); + if let Some(value) = value { + env.insert(canonical_key.to_string(), value); + } + } +} + +#[cfg(any(target_os = "windows", test))] +fn windows_environment_value<'a>( + env: &'a HashMap, + canonical_key: &str, +) -> Option<(&'a String, &'a String)> { + env.get_key_value(canonical_key).or_else(|| { + env.iter() + .filter(|(key, _)| key.eq_ignore_ascii_case(canonical_key)) + .min_by(|(left, _), (right, _)| left.cmp(right)) + }) +} + pub fn populate_env( vars: I, policy: &ShellEnvironmentPolicy, @@ -115,6 +156,39 @@ const UNIX_CORE_ENV_VARS: &[&str] = &[ "USER", ]; +#[cfg(test)] +mod normalization_tests { + use super::*; + use pretty_assertions::assert_eq; + + #[test] + fn windows_shell_search_normalization_prefers_configured_canonical_keys() { + let mut env = HashMap::from([ + ("Path".to_string(), r"C:\inherited".to_string()), + ("path".to_string(), r"C:\configured-alias".to_string()), + ("PATH".to_string(), r"C:\configured-canonical".to_string()), + ("PathExt".to_string(), ".COM;.EXE;.CMD".to_string()), + ]); + let policy = ShellEnvironmentPolicy { + r#set: HashMap::from([ + ("path".to_string(), r"C:\configured-alias".to_string()), + ("PATH".to_string(), r"C:\configured-canonical".to_string()), + ]), + ..Default::default() + }; + + normalize_windows_shell_search_environment(&mut env, &policy); + + assert_eq!( + env, + HashMap::from([ + ("PATH".to_string(), r"C:\configured-canonical".to_string()), + ("PATHEXT".to_string(), ".COM;.EXE;.CMD".to_string()), + ]) + ); + } +} + #[cfg(target_os = "windows")] pub const WINDOWS_CORE_ENV_VARS: &[&str] = &[ // Core path resolution @@ -209,6 +283,48 @@ mod windows_tests { assert_eq!(result, expected); } + + #[test] + fn create_env_policy_rebuild_coalesces_windows_path_and_pathext() { + let vars = make_vars(&[ + ("Path", r"C:\inherited-bin"), + ("PATH", r"C:\other-inherited-bin"), + ("PathExt", ".COM;.EXE;.BAT;.CMD"), + ]); + let mut policy = ShellEnvironmentPolicy { + inherit: ShellEnvironmentPolicyInherit::All, + ignore_default_excludes: true, + ..Default::default() + }; + policy + .r#set + .insert("PATH".to_string(), r"C:\configured-bin".to_string()); + policy + .r#set + .insert("pathext".to_string(), ".EXE".to_string()); + + let result = create_env_from_vars(vars, &policy, /*thread_id*/ None); + + assert_eq!( + result.get("PATH").map(String::as_str), + Some(r"C:\configured-bin") + ); + assert_eq!(result.get("PATHEXT").map(String::as_str), Some(".EXE")); + assert_eq!( + result + .keys() + .filter(|key| key.eq_ignore_ascii_case("PATH")) + .count(), + 1 + ); + assert_eq!( + result + .keys() + .filter(|key| key.eq_ignore_ascii_case("PATHEXT")) + .count(), + 1 + ); + } } #[cfg(all(test, not(target_os = "windows")))] diff --git a/codex-rs/shell-command/src/shell_detect.rs b/codex-rs/shell-command/src/shell_detect.rs index 69a66f0563..fdbd104d93 100644 --- a/codex-rs/shell-command/src/shell_detect.rs +++ b/codex-rs/shell-command/src/shell_detect.rs @@ -1,3 +1,5 @@ +use std::ffi::OsStr; +use std::path::Path; use std::path::PathBuf; use serde::Deserialize; @@ -30,6 +32,81 @@ pub struct DetectedShell { pub shell_path: PathBuf, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ModelShellResolveError { + Unsupported(PathBuf), + MissingBareName(PathBuf), + MissingPath(PathBuf), + NotAFile(PathBuf), + NotExecutable(PathBuf), + UnsupportedWindowsLaunch(PathBuf), + UnsupportedWindowsPathNamespace(PathBuf), + UnresolvedWindowsRelativePath(PathBuf), + NonUtf8ResolvedPath(PathBuf), + RelativeWorkingDirectory(PathBuf), +} + +impl std::fmt::Display for ModelShellResolveError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Unsupported(path) => { + write!( + formatter, + "unsupported model-provided shell `{}`", + path.display() + ) + } + Self::MissingBareName(path) => write!( + formatter, + "model-provided shell `{}` was not found on the invocation PATH", + path.display() + ), + Self::MissingPath(path) => write!( + formatter, + "model-provided shell `{}` does not exist", + path.display() + ), + Self::NotAFile(path) => write!( + formatter, + "model-provided shell `{}` is not a regular file", + path.display() + ), + Self::NotExecutable(path) => write!( + formatter, + "model-provided shell `{}` is not executable", + path.display() + ), + Self::UnsupportedWindowsLaunch(path) => write!( + formatter, + "model-provided Windows shell `{}` must resolve to an .exe executable", + path.display() + ), + Self::UnsupportedWindowsPathNamespace(path) => write!( + formatter, + "Windows shell resolution refuses remote, device, or verbatim path namespace `{}`", + path.display() + ), + Self::UnresolvedWindowsRelativePath(path) => write!( + formatter, + "model-provided Windows shell path `{}` is drive-relative and cannot be resolved against the selected working directory", + path.display() + ), + Self::NonUtf8ResolvedPath(path) => write!( + formatter, + "resolved model-provided shell path `{}` is not representable in command argv", + path.display() + ), + Self::RelativeWorkingDirectory(path) => write!( + formatter, + "cannot resolve a model-provided shell against relative working directory `{}`", + path.display() + ), + } + } +} + +impl std::error::Error for ModelShellResolveError {} + impl DetectedShell { pub fn name(&self) -> &'static str { self.shell_type.name() @@ -38,6 +115,14 @@ impl DetectedShell { pub fn detect_shell_type(shell_path: impl AsRef) -> Option { let shell_path = shell_path.as_ref(); + #[cfg(windows)] + { + return shell_path + .as_os_str() + .to_str() + .and_then(detect_shell_type_from_hint); + } + #[cfg(not(windows))] match shell_path.as_os_str().to_str() { Some("zsh") => Some(ShellType::Zsh), Some("sh") => Some(ShellType::Sh), @@ -58,6 +143,36 @@ pub fn detect_shell_type(shell_path: impl AsRef) -> Option Option { + let file_name = shell_hint.rsplit(['/', '\\']).next()?; + if file_name.is_empty() { + return None; + } + let stem = file_name + .rsplit_once('.') + .filter(|(_, extension)| extension.eq_ignore_ascii_case("exe")) + .map_or(file_name, |(stem, _)| stem); + + if stem.eq_ignore_ascii_case("zsh") { + Some(ShellType::Zsh) + } else if stem.eq_ignore_ascii_case("sh") { + Some(ShellType::Sh) + } else if stem.eq_ignore_ascii_case("cmd") { + Some(ShellType::Cmd) + } else if stem.eq_ignore_ascii_case("bash") { + Some(ShellType::Bash) + } else if stem.eq_ignore_ascii_case("pwsh") || stem.eq_ignore_ascii_case("powershell") { + Some(ShellType::PowerShell) + } else { + None + } +} + #[cfg(unix)] fn get_user_shell_path() -> Option { let uid = unsafe { libc::getuid() }; @@ -252,12 +367,231 @@ pub fn ultimate_fallback_shell() -> DetectedShell { } } +/// Legacy configured-shell compatibility helper. +/// +/// This may fall back to the user's configured/default shell and therefore +/// must not be used for model-selected executable input. Model-selected shells +/// must go through [`resolve_model_provided_shell_in`]. pub fn get_shell_by_model_provided_path(shell_path: &PathBuf) -> DetectedShell { detect_shell_type(shell_path) .and_then(|shell_type| get_shell(shell_type, Some(shell_path))) .unwrap_or_else(ultimate_fallback_shell) } +/// Resolves a model-selected shell exactly once against the invocation's PATH +/// and selected execution cwd. +/// +/// Unlike configured-shell detection, this never falls back to another +/// executable. The returned path is the one policy and runtime must both use. +pub fn resolve_model_provided_shell_in( + shell_path: &Path, + search_path: &OsStr, + path_ext: Option<&OsStr>, + cwd: &Path, +) -> Result { + let shell_type = detect_shell_type(shell_path) + .ok_or_else(|| ModelShellResolveError::Unsupported(shell_path.to_path_buf()))?; + if !cwd.is_absolute() { + return Err(ModelShellResolveError::RelativeWorkingDirectory( + cwd.to_path_buf(), + )); + } + + #[cfg(not(windows))] + let is_bare_name = shell_path.components().count() == 1; + #[cfg(windows)] + let resolved_path = resolve_model_shell_path_windows( + shell_path, + search_path, + path_ext.unwrap_or_else(|| OsStr::new("")), + cwd, + )?; + #[cfg(not(windows))] + let resolved_path = { + let _ = path_ext; + if is_bare_name { + std::env::split_paths(search_path) + .map(|directory| resolve_relative_path(&directory, cwd).join(shell_path)) + .find(|candidate| validate_model_shell_path(candidate).is_ok()) + .ok_or_else(|| ModelShellResolveError::MissingBareName(shell_path.to_path_buf()))? + } else { + resolve_relative_path(shell_path, cwd) + } + }; + + if resolved_path.to_str().is_none() { + return Err(ModelShellResolveError::NonUtf8ResolvedPath(resolved_path)); + } + validate_model_shell_path(&resolved_path)?; + Ok(DetectedShell { + shell_type, + shell_path: resolved_path, + }) +} + +fn resolve_relative_path(path: &Path, cwd: &Path) -> PathBuf { + let absolute = if path.is_absolute() { + path.to_path_buf() + } else { + cwd.join(path) + }; + absolute + .components() + .filter(|component| !matches!(component, std::path::Component::CurDir)) + .collect() +} + +#[cfg(any(windows, test))] +#[cfg_attr(not(windows), allow(dead_code))] +fn resolve_model_shell_path_windows( + shell_path: &Path, + search_path: &OsStr, + path_ext: &OsStr, + cwd: &Path, +) -> Result { + validate_windows_local_path_namespace(shell_path)?; + let path_extensions = path_ext + .to_string_lossy() + .split(';') + .filter(|extension| extension.starts_with('.') && extension.len() > 1) + .map(str::to_owned) + .collect::>(); + let candidate_names = windows_executable_candidates(shell_path, &path_extensions); + + let has_separator = shell_path.components().count() > 1; + if has_separator { + let exact_path = resolve_windows_path_against_cwd(shell_path, cwd)?; + if !exact_path.is_absolute() { + return Err(ModelShellResolveError::UnresolvedWindowsRelativePath( + shell_path.to_path_buf(), + )); + } + let mut first_specific_error = None; + for candidate in candidate_names + .into_iter() + .map(|candidate| resolve_relative_path(&candidate, cwd)) + { + match validate_model_shell_path(&candidate) { + Ok(()) => return Ok(candidate), + Err(ModelShellResolveError::MissingPath(_)) => {} + Err(err) if first_specific_error.is_none() => first_specific_error = Some(err), + Err(_) => {} + } + } + if first_specific_error.is_none() + && shell_path.extension().is_none() + && std::fs::metadata(&exact_path).is_ok_and(|metadata| metadata.is_file()) + { + return Err(ModelShellResolveError::UnsupportedWindowsLaunch(exact_path)); + } + return Err(first_specific_error.unwrap_or(ModelShellResolveError::MissingPath(exact_path))); + } + + for directory in std::env::split_paths(search_path) { + // Preserve PATH ordering: validate an entry immediately before it + // would be searched, without inspecting later unused entries. + let resolved_directory = resolve_windows_path_against_cwd(&directory, cwd)?; + if !resolved_directory.is_absolute() { + return Err(ModelShellResolveError::UnresolvedWindowsRelativePath( + directory, + )); + } + for candidate_name in &candidate_names { + let candidate = resolved_directory.join(candidate_name); + if validate_model_shell_path(&candidate).is_ok() { + return Ok(candidate); + } + } + } + + Err(ModelShellResolveError::MissingBareName( + shell_path.to_path_buf(), + )) +} + +#[cfg(any(windows, test))] +#[cfg_attr(not(windows), allow(dead_code))] +fn validate_windows_local_path_namespace(path: &Path) -> Result<(), ModelShellResolveError> { + let Some(std::path::Component::Prefix(prefix)) = path.components().next() else { + return Ok(()); + }; + + // Only ordinary drive-letter paths are accepted as prefixed syntax because + // they do not directly encode a remote host or device namespace. UNC, + // device, and every verbatim namespace can cause remote or device I/O + // during metadata lookup and must not be probed. + // + // This is not proof that the storage is local: a mapped drive or a reparse + // point can still redirect a normal drive path before approval. Closing + // that residual requires a two-phase or handle-based resolution design. + match prefix.kind() { + std::path::Prefix::Disk(_) => Ok(()), + _ => Err(ModelShellResolveError::UnsupportedWindowsPathNamespace( + path.to_path_buf(), + )), + } +} + +#[cfg(any(windows, test))] +#[cfg_attr(not(windows), allow(dead_code))] +fn resolve_windows_path_against_cwd( + path: &Path, + cwd: &Path, +) -> Result { + validate_windows_local_path_namespace(path)?; + + let has_prefix = matches!( + path.components().next(), + Some(std::path::Component::Prefix(_)) + ); + if !path.is_absolute() && !has_prefix { + // Relative and root-relative paths inherit directory or drive context + // from cwd, so validate that namespace before joining or probing. + validate_windows_local_path_namespace(cwd)?; + } + + Ok(resolve_relative_path(path, cwd)) +} + +#[cfg(any(windows, test))] +#[cfg_attr(not(windows), allow(dead_code))] +fn windows_executable_candidates(shell_path: &Path, path_extensions: &[String]) -> Vec { + match shell_path.extension() { + Some(extension) if extension.eq_ignore_ascii_case("exe") => { + vec![shell_path.to_path_buf()] + } + Some(_) => Vec::new(), + None => path_extensions + .iter() + .filter(|extension| extension.eq_ignore_ascii_case(".exe")) + .map(|extension| { + let mut candidate = shell_path.as_os_str().to_os_string(); + candidate.push(extension); + PathBuf::from(candidate) + }) + .collect(), + } +} + +fn validate_model_shell_path(path: &Path) -> Result<(), ModelShellResolveError> { + let metadata = std::fs::metadata(path) + .map_err(|_| ModelShellResolveError::MissingPath(path.to_path_buf()))?; + if !metadata.is_file() { + return Err(ModelShellResolveError::NotAFile(path.to_path_buf())); + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + if metadata.permissions().mode() & 0o111 == 0 { + return Err(ModelShellResolveError::NotExecutable(path.to_path_buf())); + } + } + + Ok(()) +} + pub fn get_shell(shell_type: ShellType, path: Option<&PathBuf>) -> Option { match shell_type { ShellType::Zsh => get_zsh_shell(path), @@ -365,4 +699,424 @@ mod tests { Some(ShellType::Cmd) ); } + + #[test] + fn detects_remote_shell_hints_independently_of_controller_path_syntax() { + for (hint, expected) in [ + ("bash", Some(ShellType::Bash)), + ("/attacker/bash", Some(ShellType::Bash)), + (r"C:\attacker\BASH.ExE", Some(ShellType::Bash)), + (r"\\server\share\PwSh.EXE", Some(ShellType::PowerShell)), + ("/opt/PowerShell", Some(ShellType::PowerShell)), + (r"C:\Windows\System32\cmd.exe", Some(ShellType::Cmd)), + ("/tmp/fish", None), + (r"C:\attacker\powershell.exe:payload", None), + ("/tmp/bash/", None), + (r"C:\tmp\bash\", None), + ] { + assert_eq!( + detect_shell_type_from_hint(hint), + expected, + "unexpected type for {hint:?}" + ); + } + } + + #[cfg(unix)] + fn write_executable(path: &Path) { + use std::os::unix::fs::PermissionsExt; + + std::fs::write(path, "#!/bin/sh\nexit 0\n").expect("write fake shell"); + let mut permissions = std::fs::metadata(path) + .expect("fake shell metadata") + .permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(path, permissions).expect("make fake shell executable"); + } + + #[cfg(unix)] + #[test] + fn model_shell_resolver_uses_only_supplied_path() { + let temp_dir = tempfile::tempdir().expect("temp dir"); + let bin = temp_dir.path().join("bin"); + std::fs::create_dir(&bin).expect("create bin"); + let shell = bin.join("sh"); + write_executable(&shell); + + assert_eq!( + resolve_model_provided_shell_in( + Path::new("sh"), + bin.as_os_str(), + /*path_ext*/ None, + temp_dir.path(), + ), + Ok(DetectedShell { + shell_type: ShellType::Sh, + shell_path: shell, + }) + ); + assert_eq!( + resolve_model_provided_shell_in( + Path::new("sh"), + OsStr::new(""), + /*path_ext*/ None, + temp_dir.path(), + ), + Err(ModelShellResolveError::MissingBareName(PathBuf::from("sh"))) + ); + } + + #[cfg(unix)] + #[test] + fn model_shell_resolver_resolves_relative_input_and_path_against_selected_cwd() { + let temp_dir = tempfile::tempdir().expect("temp dir"); + let tools = temp_dir.path().join("tools"); + std::fs::create_dir(&tools).expect("create tools"); + let shell = tools.join("bash"); + write_executable(&shell); + + for (requested, search_path) in [ + (Path::new("./tools/bash"), OsStr::new("")), + (Path::new("bash"), OsStr::new("tools")), + ] { + assert_eq!( + resolve_model_provided_shell_in( + requested, + search_path, + /*path_ext*/ None, + temp_dir.path(), + ), + Ok(DetectedShell { + shell_type: ShellType::Bash, + shell_path: shell.clone(), + }) + ); + } + } + + #[cfg(unix)] + #[test] + fn model_shell_resolver_rejects_unsupported_missing_and_non_launchable_inputs() { + let temp_dir = tempfile::tempdir().expect("temp dir"); + let non_executable = temp_dir.path().join("bash"); + std::fs::write(&non_executable, "#!/bin/sh\n").expect("write non-executable shell"); + let directory = temp_dir.path().join("sh"); + std::fs::create_dir(&directory).expect("create shell-named directory"); + let missing = temp_dir.path().join("zsh"); + + assert_eq!( + resolve_model_provided_shell_in( + &non_executable, + OsStr::new(""), + /*path_ext*/ None, + temp_dir.path(), + ), + Err(ModelShellResolveError::NotExecutable(non_executable)) + ); + assert_eq!( + resolve_model_provided_shell_in( + &directory, + OsStr::new(""), + /*path_ext*/ None, + temp_dir.path(), + ), + Err(ModelShellResolveError::NotAFile(directory)) + ); + assert_eq!( + resolve_model_provided_shell_in( + &missing, + OsStr::new(""), + /*path_ext*/ None, + temp_dir.path(), + ), + Err(ModelShellResolveError::MissingPath(missing)) + ); + assert_eq!( + resolve_model_provided_shell_in( + Path::new("fish"), + OsStr::new(""), + /*path_ext*/ None, + temp_dir.path(), + ), + Err(ModelShellResolveError::Unsupported(PathBuf::from("fish"))) + ); + assert_eq!( + resolve_model_provided_shell_in( + Path::new("sh"), + OsStr::new(""), + /*path_ext*/ None, + Path::new("."), + ), + Err(ModelShellResolveError::RelativeWorkingDirectory( + PathBuf::from(".") + )) + ); + } + + #[cfg(unix)] + #[test] + fn model_shell_resolver_rejects_non_utf8_resolved_path() { + use std::os::unix::ffi::OsStringExt; + + let temp_dir = tempfile::tempdir().expect("temp dir"); + let non_utf8_cwd = temp_dir + .path() + .join(std::ffi::OsString::from_vec(b"cwd-\xff".to_vec())); + let shell = non_utf8_cwd.join("bash"); + + assert_eq!( + resolve_model_provided_shell_in( + Path::new("./bash"), + OsStr::new(""), + /*path_ext*/ None, + &non_utf8_cwd, + ), + Err(ModelShellResolveError::NonUtf8ResolvedPath(shell)) + ); + } + + #[cfg(windows)] + #[test] + fn windows_model_shell_resolver_uses_supplied_pathext() { + let temp_dir = tempfile::tempdir().expect("temp dir"); + let bin = temp_dir.path().join("bin"); + std::fs::create_dir(&bin).expect("create bin"); + for (requested, file_name) in [("PowerShell.ExE", "PowerShell.ExE"), ("PWSH", "PWSH.EXE")] { + let shell = bin.join(file_name); + std::fs::write(&shell, "fixture").expect("write fake executable"); + assert_eq!( + resolve_model_provided_shell_in( + Path::new(requested), + bin.as_os_str(), + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Ok(DetectedShell { + shell_type: ShellType::PowerShell, + shell_path: shell, + }) + ); + } + assert_eq!( + resolve_model_provided_shell_in( + Path::new("powershell"), + bin.as_os_str(), + Some(OsStr::new(".CMD")), + temp_dir.path(), + ), + Err(ModelShellResolveError::MissingBareName(PathBuf::from( + "powershell" + ))) + ); + + let extensionless = temp_dir.path().join("bash"); + std::fs::write(&extensionless, "fixture").expect("write extensionless executable"); + assert_eq!( + resolve_model_provided_shell_in( + &extensionless, + OsStr::new(""), + Some(OsStr::new("")), + temp_dir.path(), + ), + Err(ModelShellResolveError::UnsupportedWindowsLaunch( + extensionless.clone() + )) + ); + let sibling_exe = temp_dir.path().join("bash.EXE"); + std::fs::write(&sibling_exe, "fixture").expect("write sibling exe"); + assert_eq!( + resolve_model_provided_shell_in( + &extensionless, + OsStr::new(""), + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Ok(DetectedShell { + shell_type: ShellType::Bash, + shell_path: sibling_exe, + }) + ); + + for (requested, file_name, path_ext) in [ + ("cmd", "cmd.CMD", ".CMD"), + ("powershell", "powershell.BAT", ".BAT"), + ] { + std::fs::write(bin.join(file_name), "fixture").expect("write script candidate"); + assert_eq!( + resolve_model_provided_shell_in( + Path::new(requested), + bin.as_os_str(), + Some(OsStr::new(path_ext)), + temp_dir.path(), + ), + Err(ModelShellResolveError::MissingBareName(PathBuf::from( + requested + ))) + ); + } + + let directory = temp_dir.path().join("sh.exe"); + std::fs::create_dir(&directory).expect("create shell-named directory"); + assert_eq!( + resolve_model_provided_shell_in( + &directory, + OsStr::new(""), + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Err(ModelShellResolveError::NotAFile(directory)) + ); + + let drive_relative = PathBuf::from(r"C:relative\powershell.exe"); + assert_eq!( + resolve_model_provided_shell_in( + &drive_relative, + OsStr::new(""), + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Err(ModelShellResolveError::UnresolvedWindowsRelativePath( + drive_relative + )) + ); + let later_shell = bin.join("bash.EXE"); + std::fs::write(&later_shell, "fixture").expect("write later PATH shell"); + let drive_relative_path = PathBuf::from(r"C:relative-bin"); + let search_path = std::env::join_paths([drive_relative_path.as_path(), bin.as_path()]) + .expect("join PATH entries"); + assert_eq!( + resolve_model_provided_shell_in( + Path::new("bash"), + &search_path, + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Err(ModelShellResolveError::UnresolvedWindowsRelativePath( + drive_relative_path + )) + ); + } + + #[cfg(windows)] + #[test] + fn windows_model_shell_resolver_rejects_remote_device_and_verbatim_namespaces() { + let temp_dir = tempfile::tempdir().expect("temp dir"); + + for requested in [ + PathBuf::from(r"\\server\share\powershell.exe"), + PathBuf::from(r"//server/share/powershell.exe"), + PathBuf::from(r"\/server\share/powershell.exe"), + PathBuf::from(r"\\.\GLOBALROOT\Device\HarddiskVolume1\powershell.exe"), + PathBuf::from(r"\\?\C:\tools\powershell.exe"), + PathBuf::from(r"\\?\C:/tools/powershell.exe"), + PathBuf::from(r"\\?\UNC\server\share\powershell.exe"), + PathBuf::from(r"\\?\UNC\server/share/powershell.exe"), + PathBuf::from(r"\\?\Volume{12345678-1234-1234-1234-123456789abc}\powershell.exe"), + PathBuf::from(r"\\?\Volume{12345678-1234-1234-1234-123456789abc}/powershell.exe"), + ] { + assert_eq!( + resolve_model_provided_shell_in( + &requested, + OsStr::new(""), + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Err(ModelShellResolveError::UnsupportedWindowsPathNamespace( + requested.clone() + )), + "unsafe namespace should fail before any executable probe: {requested:?}" + ); + } + } + + #[cfg(windows)] + #[test] + fn windows_model_shell_resolver_validates_path_namespaces_in_lookup_order() { + let temp_dir = tempfile::tempdir().expect("temp dir"); + let bin = temp_dir.path().join("bin"); + std::fs::create_dir(&bin).expect("create bin"); + let shell = bin.join("powershell.EXE"); + std::fs::write(&shell, "fixture").expect("write fake executable"); + let unsafe_directory = PathBuf::from(r"\\server\share\bin"); + + let unsafe_first = + std::env::join_paths([unsafe_directory.as_path(), bin.as_path()]).expect("join PATH"); + assert_eq!( + resolve_model_provided_shell_in( + Path::new("powershell"), + &unsafe_first, + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Err(ModelShellResolveError::UnsupportedWindowsPathNamespace( + unsafe_directory.clone() + )) + ); + + let local_first = + std::env::join_paths([bin.as_path(), unsafe_directory.as_path()]).expect("join PATH"); + assert_eq!( + resolve_model_provided_shell_in( + Path::new("powershell"), + &local_first, + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Ok(DetectedShell { + shell_type: ShellType::PowerShell, + shell_path: shell, + }), + "a valid earlier PATH candidate must return before an unused unsafe entry" + ); + } + + #[cfg(windows)] + #[test] + fn windows_model_shell_resolver_checks_unsafe_cwd_only_when_resolution_uses_it() { + let temp_dir = tempfile::tempdir().expect("temp dir"); + let relative_dir = temp_dir.path().join("tools"); + std::fs::create_dir(&relative_dir).expect("create tools"); + let shell = relative_dir.join("pwsh.exe"); + std::fs::write(&shell, "fixture").expect("write fake executable"); + + assert_eq!( + resolve_model_provided_shell_in( + Path::new(r"tools\pwsh.exe"), + OsStr::new(""), + Some(OsStr::new(".EXE")), + temp_dir.path(), + ), + Ok(DetectedShell { + shell_type: ShellType::PowerShell, + shell_path: shell.clone(), + }) + ); + + let unsafe_cwd = PathBuf::from(r"\\server\share\cwd"); + assert_eq!( + resolve_model_provided_shell_in( + Path::new(r".\pwsh.exe"), + OsStr::new(""), + Some(OsStr::new(".EXE")), + &unsafe_cwd, + ), + Err(ModelShellResolveError::UnsupportedWindowsPathNamespace( + unsafe_cwd.clone() + )) + ); + + assert_eq!( + resolve_model_provided_shell_in( + &shell, + OsStr::new(""), + Some(OsStr::new(".EXE")), + &unsafe_cwd, + ), + Ok(DetectedShell { + shell_type: ShellType::PowerShell, + shell_path: shell, + }), + "an absolute local shell does not use cwd during resolution" + ); + } }