diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index fc7095a9a5..5da5203270 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -967,7 +967,7 @@ impl ExecPolicyManager { matched_prefix, decision: Decision::Allow, .. - } if matched_prefix.len() == outer.len() + } if matched_prefix.as_slice() == outer ) }) }); @@ -1263,7 +1263,7 @@ fn create_untrusted_wrapper_approval_requirement( matched_prefix, decision: Decision::Allow, .. - } if matched_prefix.len() == candidate.argv.len() + } if matched_prefix.as_slice() == candidate.argv ) }); every_candidate_explicit_allow &= raw_full_outer_allow; @@ -1281,7 +1281,6 @@ fn create_untrusted_wrapper_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, @@ -1328,15 +1327,13 @@ fn create_untrusted_wrapper_approval_requirement( evaluation.decision = Decision::Prompt; } - let permission_or_backend_gate = permission_delta_requires_outer_authority( + let requires_composed_authority = untrusted_composition_requires_full_authority( context.permission_profile, context.sandbox_permissions, - ) || missing_managed_windows_sandbox_backend( - context.permission_profile, context.windows_sandbox_level, ); if evaluation.decision != Decision::Forbidden - && permission_or_backend_gate + && requires_composed_authority && !composed_full_authority { evaluation @@ -1744,6 +1741,24 @@ fn permission_delta_requires_outer_authority( } } +#[cfg_attr(not(windows), allow(dead_code))] +fn untrusted_composition_requires_full_authority( + permission_profile: &PermissionProfile, + sandbox_permissions: SandboxPermissions, + windows_sandbox_level: WindowsSandboxLevel, +) -> bool { + let lacks_filesystem_containment = match permission_profile { + PermissionProfile::Disabled => true, + PermissionProfile::Managed { .. } => { + !profile_has_managed_filesystem_restrictions(permission_profile) + } + PermissionProfile::External { .. } => false, + }; + lacks_filesystem_containment + || permission_delta_requires_outer_authority(permission_profile, sandbox_permissions) + || missing_managed_windows_sandbox_backend(permission_profile, windows_sandbox_level) +} + fn missing_managed_windows_sandbox_backend( permission_profile: &PermissionProfile, windows_sandbox_level: WindowsSandboxLevel, diff --git a/codex-rs/core/src/exec_policy_powershell_tests.rs b/codex-rs/core/src/exec_policy_powershell_tests.rs index 993a2f1465..9b100d1514 100644 --- a/codex-rs/core/src/exec_policy_powershell_tests.rs +++ b/codex-rs/core/src/exec_policy_powershell_tests.rs @@ -553,7 +553,7 @@ fn untrusted_permission_and_windows_backend_gates_require_composed_authority() { PermissionProfile::Disabled, WSL::Disabled, SP::RequireEscalated, - false, + true, ), ( "external default", @@ -610,6 +610,158 @@ fn untrusted_permission_and_windows_backend_gates_require_composed_authority() { } } +#[test] +fn untrusted_without_filesystem_containment_requires_complete_composed_authority() { + let outer = untrusted_powershell_command("Get-Location"); + let inner = vec_str(&["Get-Location"]); + let partial_policy = format!( + "{}\n{}", + prefix_rule_for(&outer[..1], "allow"), + prefix_rule_for(&inner, "allow") + ); + let full_policy = format!( + "{}\n{}", + prefix_rule_for(&outer, "allow"), + prefix_rule_for(&inner, "allow") + ); + let disabled = PermissionProfile::Disabled; + + for (name, approval_policy, expected) in [ + ( + "on request", + AskForApproval::OnRequest, + one_shot(/*reason*/ None), + ), + ( + "unless trusted", + AskForApproval::UnlessTrusted, + one_shot(/*reason*/ None), + ), + ( + "never", + AskForApproval::Never, + ExecApprovalRequirement::Forbidden { + reason: PROMPT_CONFLICT_REASON.to_string(), + }, + ), + ( + "granular sandbox approval disabled", + granular(/*rules*/ false, /*sandbox_approval*/ false), + ExecApprovalRequirement::Forbidden { + reason: REJECT_SANDBOX_APPROVAL_REASON.to_string(), + }, + ), + ( + "granular sandbox approval enabled", + granular(/*rules*/ false, /*sandbox_approval*/ true), + one_shot(/*reason*/ None), + ), + ] { + pretty_assertions::assert_eq!( + composed_untrusted_requirement( + Some(&partial_policy), + &outer, + std::slice::from_ref(&inner), + approval_policy, + &disabled, + WindowsSandboxLevel::Disabled, + SandboxPermissions::UseDefault, + ), + expected, + "{name}", + ); + } + + pretty_assertions::assert_eq!( + composed_untrusted_requirement( + Some(&prefix_rule_for(&outer, "allow")), + &outer, + std::slice::from_ref(&inner), + AskForApproval::OnRequest, + &disabled, + WindowsSandboxLevel::Disabled, + SandboxPermissions::UseDefault, + ), + one_shot(/*reason*/ None), + "a heuristic-safe inner command is not explicit authority without a sandbox", + ); + + pretty_assertions::assert_eq!( + composed_untrusted_requirement( + Some(&full_policy), + &outer, + std::slice::from_ref(&inner), + AskForApproval::OnRequest, + &disabled, + WindowsSandboxLevel::Disabled, + SandboxPermissions::UseDefault, + ), + untrusted_skip(/*bypass_sandbox*/ true), + "complete composed authority remains sufficient without a sandbox", + ); + + let managed_unrestricted = PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::unrestricted(), + NetworkSandboxPolicy::Enabled, + ); + let managed_full_disk_write = PermissionProfile::from_runtime_permissions( + &FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Write, + }]), + NetworkSandboxPolicy::Enabled, + ); + for (name, profile) in [ + ("managed unrestricted", managed_unrestricted), + ("managed full-disk write", managed_full_disk_write), + ] { + pretty_assertions::assert_eq!( + composed_untrusted_requirement( + Some(&partial_policy), + &outer, + std::slice::from_ref(&inner), + AskForApproval::OnRequest, + &profile, + WindowsSandboxLevel::RestrictedToken, + SandboxPermissions::UseDefault, + ), + one_shot(/*reason*/ None), + "partial authority must prompt for {name}", + ); + pretty_assertions::assert_eq!( + composed_untrusted_requirement( + Some(&full_policy), + &outer, + std::slice::from_ref(&inner), + AskForApproval::OnRequest, + &profile, + WindowsSandboxLevel::RestrictedToken, + SandboxPermissions::UseDefault, + ), + untrusted_skip(/*bypass_sandbox*/ true), + "complete authority remains sufficient for {name}", + ); + } + + pretty_assertions::assert_eq!( + composed_untrusted_requirement( + Some(&partial_policy), + &outer, + std::slice::from_ref(&inner), + AskForApproval::OnRequest, + &PermissionProfile::External { + network: NetworkSandboxPolicy::Restricted, + }, + WindowsSandboxLevel::RestrictedToken, + SandboxPermissions::UseDefault, + ), + untrusted_skip(/*bypass_sandbox*/ false), + "an externally enforced sandbox keeps existing partial-authority behavior", + ); +} + #[test] fn untrusted_wrapper_identity_uses_exact_outer_and_restrictive_basename_rules() { let path_a = vec_str(&[r"C:\workspace-a\powershell.exe", "-Command", "echo allowed"]); diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index 4edac8460c..e250a6a4de 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -1714,6 +1714,76 @@ async fn configured_delegators_without_rules_preserve_legacy_sandboxed_behavior( } } +#[tokio::test] +async fn posix_wrappers_require_complete_authority_without_filesystem_containment() { + let inner = vec_str(&["echo", "hello"]); + let model_command = vec![ + host_program_path("zsh"), + "-lc".to_string(), + shlex_try_join(inner.iter().map(String::as_str)).expect("quote inner command"), + ]; + let full_inner_allow = prefix_rule_for(&inner, "allow"); + let full_model_outer_allow = prefix_rule_for(&model_command, "allow"); + let nested_wrapper = vec![ + posix_script_program_path("sh"), + "-c".to_string(), + shlex_try_join(inner.iter().map(String::as_str)).expect("quote nested body"), + ]; + let configured_command = vec![ + host_program_path("zsh"), + "-lc".to_string(), + shlex_try_join(nested_wrapper.iter().map(String::as_str)).expect("quote nested wrapper"), + ]; + let full_nested_wrapper_allow = prefix_rule_for(&nested_wrapper, "allow"); + + for (name, provenance, command, partial_policy, full_policy) in [ + ( + "local model-resolved wrapper", + ShellApprovalProvenance::local_model_resolved(), + model_command, + full_model_outer_allow.clone(), + format!("{full_model_outer_allow}\n{full_inner_allow}"), + ), + ( + "configured nested wrapper", + ShellApprovalProvenance::configured(), + configured_command, + full_nested_wrapper_allow.clone(), + format!("{full_nested_wrapper_allow}\n{full_inner_allow}"), + ), + ] { + assert_eq!( + requirement_with_provenance( + Some(&partial_policy), + &command, + AskForApproval::OnRequest, + PermissionProfile::Disabled, + SandboxPermissions::UseDefault, + provenance, + ) + .await, + ExecApprovalRequirement::NeedsOneShotApproval { reason: None }, + "partial authority must prompt for {name}", + ); + assert_eq!( + requirement_with_provenance( + Some(&full_policy), + &command, + AskForApproval::OnRequest, + PermissionProfile::Disabled, + SandboxPermissions::UseDefault, + provenance, + ) + .await, + ExecApprovalRequirement::Skip { + bypass_sandbox: true, + proposed_execpolicy_amendment: None, + }, + "complete authority remains sufficient for {name}", + ); + } +} + #[tokio::test] async fn configured_exact_allow_can_authorize_an_opaque_nested_shell_only() { let inner = vec![ diff --git a/codex-rs/core/src/exec_policy_windows_tests.rs b/codex-rs/core/src/exec_policy_windows_tests.rs index 5c6a072a28..5a7d1865f0 100644 --- a/codex-rs/core/src/exec_policy_windows_tests.rs +++ b/codex-rs/core/src/exec_policy_windows_tests.rs @@ -146,7 +146,11 @@ async fn only_exact_outer_rules_can_bypass_runtime_resolution() { "host_executable(name = \"powershell\", paths = [\"{executable}\"])\n\ prefix_rule(pattern=[\"powershell\", {rest}], decision=\"allow\")" ), - Some(true), + Some(false), + ), + ( + format!("prefix_rule(pattern=[\"powershell\", {rest}], decision=\"allow\")"), + Some(false), ), ( format!( @@ -455,6 +459,76 @@ host_executable(name = "git", paths = ["C:\\trusted\\git.exe"]) ); } +#[tokio::test] +async fn mapped_namespace_literal_restrictions_apply_to_direct_and_inner_commands() { + for (kind, command) in [ + ( + "direct", + vec![ + r"\\?\C:\attacker\git.exe.".to_string(), + "status".to_string(), + ], + ), + ( + "PowerShell inner", + powershell_command(r"\\?\C:\attacker\git.exe. status"), + ), + ] { + let rendered = render_shlex_command(&command); + + assert_eq!( + windows_requirement( + String::new(), + &command, + AskForApproval::OnRequest, + PermissionProfile::Disabled, + WindowsSandboxLevel::Disabled, + SandboxPermissions::UseDefault, + ) + .await, + skip_outer(&command, false), + "unmatched {kind} command must take the allow fallback", + ); + + for (wire_decision, expected) in [ + ( + "prompt", + ExecApprovalRequirement::NeedsApproval { + reason: Some(format!("`{rendered}` requires approval by policy")), + proposed_execpolicy_amendment: None, + }, + ), + ( + "forbidden", + ExecApprovalRequirement::Forbidden { + reason: format!( + "`{rendered}` rejected: policy forbids commands starting with `git.exe.`" + ), + }, + ), + ] { + assert_eq!( + windows_requirement( + format!( + r#" +prefix_rule(pattern = ["git.exe."], decision = "{wire_decision}") +host_executable(name = "git", paths = ["C:\\trusted\\git.exe"]) +"# + ), + &command, + AskForApproval::OnRequest, + PermissionProfile::Disabled, + WindowsSandboxLevel::Disabled, + SandboxPermissions::UseDefault, + ) + .await, + expected, + "{kind}/{wire_decision}", + ); + } + } +} + #[test] fn commands_for_exec_policy_keeps_bare_powershell_alias_opaque() { let command = vec![ diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 8d6da51b15..973200ebcd 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -20,6 +20,7 @@ use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::ApplyPatchApprovalRequestEvent; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::ExecApprovalPurpose; use codex_protocol::protocol::ExecApprovalRequestEvent; use codex_protocol::protocol::ExecPolicyAmendment; use codex_protocol::protocol::GranularApprovalConfig; @@ -36,6 +37,7 @@ use core_test_support::responses::ev_function_call_with_namespace; use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_sse_once; use core_test_support::responses::mount_sse_once_match; +use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; @@ -2166,6 +2168,117 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[cfg(unix)] +async fn sandbox_retry_uses_fresh_callback_identity_and_one_shot_decisions() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let approval_policy = AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: true, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: true, + }); + let sandbox_policy = SandboxPolicy::new_read_only_policy(); + let sandbox_policy_for_config = sandbox_policy.clone(); + let test = test_codex() + .with_model("gpt-5.4") + .with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(approval_policy); + config + .features + .enable(Feature::UnifiedExec) + .expect("enable unified exec"); + config + .set_legacy_sandbox_policy(sandbox_policy_for_config) + .expect("set sandbox policy"); + }) + .build(&server) + .await?; + + let call_id = "sandbox-retry-callback-identity"; + let target = test.cwd.path().join("sandbox-retry-callback-identity.txt"); + let _ = fs::remove_file(&target); + let command = format!("touch {target:?}"); + let event = exec_command_event( + call_id, + &command, + /*yield_time_ms*/ Some(30_000), + SandboxPermissions::UseDefault, + /*justification*/ None, + )?; + + let responses = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-sandbox-retry-1"), + event, + ev_completed("resp-sandbox-retry-1"), + ]), + sse(vec![ + ev_assistant_message("msg-sandbox-retry-1", "done"), + ev_completed("resp-sandbox-retry-2"), + ]), + ], + ) + .await; + + submit_turn( + &test, + "retry the sandbox-denied write", + approval_policy, + sandbox_policy, + ) + .await?; + + let approval = expect_exec_approval(&test, &command).await; + let approval_id = approval + .approval_id + .clone() + .unwrap_or_else(|| panic!("sandbox retry must carry a callback ID: {approval:?}")); + assert_eq!(approval.call_id, call_id); + assert_ne!(approval_id, call_id); + assert_eq!( + approval.approval_purpose, + Some(ExecApprovalPurpose::SandboxRetry) + ); + assert_eq!( + approval.effective_available_decisions(), + vec![ReviewDecision::Approved, ReviewDecision::Abort] + ); + assert_eq!( + approval.reason.as_deref(), + Some("command failed; retry without sandbox?") + ); + assert!( + !target.exists(), + "sandbox-denied write must not complete before retry approval" + ); + + test.codex + .submit(Op::ExecApproval { + id: approval_id, + turn_id: Some(approval.turn_id), + decision: ReviewDecision::Approved, + }) + .await?; + wait_for_completion(&test).await; + + let output_item = responses.requests()[1].function_call_output(call_id); + let result = parse_result(&output_item); + assert_eq!(result.exit_code, Some(0), "approved retry must succeed"); + assert!( + target.exists(), + "approved retry must create the target file" + ); + fs::remove_file(target)?; + + Ok(()) +} + #[tokio::test(flavor = "current_thread")] #[cfg(unix)] async fn approving_apply_patch_for_session_skips_future_prompts_for_same_file() -> Result<()> { diff --git a/codex-rs/execpolicy/README.md b/codex-rs/execpolicy/README.md index 77c986f0fb..0dd116aaf5 100644 --- a/codex-rs/execpolicy/README.md +++ b/codex-rs/execpolicy/README.md @@ -43,7 +43,7 @@ host_executable( - With host-executable resolution enabled, if no exact rule matches, execpolicy may fall back from `/usr/bin/git` to basename rules for `git`. - If `host_executable(name="git", ...)` exists, basename fallback is only allowed for listed absolute paths. - If no `host_executable()` entry exists for a basename, basename fallback is allowed. - - Windows verbatim/device namespace paths cannot be listed by `host_executable`; use an ordinary absolute spelling or an exact prefix rule. If either a namespace command's literal basename or its ordinary normalized alias has host-executable metadata, the namespace spelling does not inherit basename rules. With no corresponding metadata, its literal basename rule may still match. Exact full-path rules always retain their authored decision. + - Windows verbatim/device namespace paths cannot be listed by `host_executable`; use an ordinary absolute spelling or an exact prefix rule. If either a namespace command's literal basename or its ordinary normalized alias has host-executable metadata, basename `allow` is filtered while basename `prompt` and `forbidden` remain in force without a `resolvedProgram`. With no corresponding metadata, its literal basename rule may still match with its authored decision. Exact full-path rules always retain their authored decision. - The library's restrictive basename overlay is separate from ordinary fallback: it ignores host-executable path lists for `prompt` and `forbidden`, but always discards basename `allow`. ## CLI diff --git a/codex-rs/execpolicy/src/policy.rs b/codex-rs/execpolicy/src/policy.rs index dd0c86a5d4..84292a2d38 100644 --- a/codex-rs/execpolicy/src/policy.rs +++ b/codex-rs/execpolicy/src/policy.rs @@ -361,7 +361,7 @@ impl Policy { .host_executables_by_name .contains_key(&normalized_basename) { - return Vec::new(); + return self.match_restrictive_basename_rules(cmd); } } let Ok(program) = AbsolutePathBuf::try_from(first.clone()) else { diff --git a/codex-rs/execpolicy/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs index f97fa6fc29..bfe03f5118 100644 --- a/codex-rs/execpolicy/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -1126,13 +1126,13 @@ host_executable(name = "git", paths = ["{}"]) #[cfg(windows)] #[test] -fn namespace_literal_aliases_do_not_bypass_equivalent_host_mapping() -> Result<()> { +fn mapped_namespace_literal_aliases_filter_allow_and_preserve_restrictions() -> Result<()> { let options = MatchOptions { resolve_host_executables: true, }; for executable_suffix in [".exe", ".cmd", ".bat", ".com"] { - for literal_tail in [".", " ", "...", " ", ". . ", ".. . "] { + for literal_tail in ["", ".", " ", "...", " ", ". . ", ".. . "] { let mixed_case = executable_suffix == ".com" && literal_tail == ".. . "; let raw_stem = if mixed_case { "GiT" } else { "git" }; let raw_suffix = if mixed_case { @@ -1144,7 +1144,7 @@ fn namespace_literal_aliases_do_not_bypass_equivalent_host_mapping() -> Result<( let literal_rule_key = raw_literal_name.to_ascii_lowercase(); let ordinary_name = format!("git{executable_suffix}"); let trusted_path = format!(r"C:\trusted\{ordinary_name}"); - let mapping_sources = [ + let mut mapping_sources = vec![ format!( r#"host_executable(name = "git", paths = ["{}"] )"#, starlark_string(&trusted_path) @@ -1156,6 +1156,8 @@ fn namespace_literal_aliases_do_not_bypass_equivalent_host_mapping() -> Result<( format!(r#"host_executable(name = "{literal_rule_key}", paths = [])"#), format!(r#"host_executable(name = "{raw_literal_name}", paths = [])"#), ]; + mapping_sources.sort(); + mapping_sources.dedup(); let namespace_paths = [ format!(r"\\?\C:\attacker\{raw_literal_name}"), format!(r"\\?\UNC\server\share\{raw_literal_name}"), @@ -1164,23 +1166,38 @@ fn namespace_literal_aliases_do_not_bypass_equivalent_host_mapping() -> Result<( ]; for mapping_source in mapping_sources { - let policy = parse_policy(&format!( - r#" -prefix_rule(pattern = ["{literal_rule_key}"], decision = "allow") + for (wire_decision, decision) in [ + ("allow", Decision::Allow), + ("prompt", Decision::Prompt), + ("forbidden", Decision::Forbidden), + ] { + let policy = parse_policy(&format!( + r#" +prefix_rule(pattern = ["{literal_rule_key}"], decision = "{wire_decision}") {mapping_source} "# - ))?; - for executable in &namespace_paths { - assert!( - policy - .matches_for_command_with_options( + ))?; + for executable in &namespace_paths { + let expected = if decision == Decision::Allow { + Vec::new() + } else { + vec![RuleMatch::PrefixRuleMatch { + matched_prefix: vec![literal_rule_key.clone()], + decision, + resolved_program: None, + justification: None, + }] + }; + assert_eq!( + policy.matches_for_command_with_options( &[executable.clone(), "status".to_string()], None, &options, - ) - .is_empty(), - "suffix={executable_suffix:?} tail={literal_tail:?} mapping={mapping_source:?} executable={executable:?}" - ); + ), + expected, + "suffix={executable_suffix:?} tail={literal_tail:?} decision={wire_decision:?} mapping={mapping_source:?} executable={executable:?}" + ); + } } } }