From 60c59a77998a180e8f702218b804bec23770da29 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Fri, 20 Mar 2026 18:28:25 -0700 Subject: [PATCH] fix(core) disable command_might_be_dangerous when unsandboxed (#15036) ## Summary If we are in a mode that is already explicitly un-sandboxed, then `ApprovalPolicy::Never` should not block dangerous commands. ## Testing - [x] Existing unit test covers old behavior - [x] Added a unit test for this new case --- codex-rs/core/src/exec_policy.rs | 17 ++++- codex-rs/core/src/exec_policy_tests.rs | 101 +++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 3 deletions(-) diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 0c95af4c0..a027f2461 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -549,7 +549,7 @@ pub fn render_decision_for_unmatched_command( // On Windows, ReadOnly sandbox is not a real sandbox, so special-case it // here. - let runtime_sandbox_provides_safety = + let environment_lacks_sandbox_protections = cfg!(windows) && matches!(sandbox_policy, SandboxPolicy::ReadOnly { .. }); // If the command is flagged as dangerous or we have no sandbox protection, @@ -558,9 +558,20 @@ pub fn render_decision_for_unmatched_command( // We prefer to prompt the user rather than outright forbid the command, // but if the user has explicitly disabled prompts, we must // forbid the command. - if command_might_be_dangerous(command) || runtime_sandbox_provides_safety { + if command_might_be_dangerous(command) || environment_lacks_sandbox_protections { return match approval_policy { - AskForApproval::Never => Decision::Forbidden, + AskForApproval::Never => { + let sandbox_is_explicitly_disabled = matches!( + sandbox_policy, + SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } + ); + if sandbox_is_explicitly_disabled { + // If the sandbox is explicitly disabled, we should allow the command to run + Decision::Allow + } else { + Decision::Forbidden + } + } AskForApproval::OnFailure | AskForApproval::OnRequest | AskForApproval::UnlessTrusted diff --git a/codex-rs/core/src/exec_policy_tests.rs b/codex-rs/core/src/exec_policy_tests.rs index fd3fe05e1..d6ec0bd3f 100644 --- a/codex-rs/core/src/exec_policy_tests.rs +++ b/codex-rs/core/src/exec_policy_tests.rs @@ -81,6 +81,10 @@ fn unrestricted_file_system_sandbox_policy() -> FileSystemSandboxPolicy { FileSystemSandboxPolicy::unrestricted() } +fn external_file_system_sandbox_policy() -> FileSystemSandboxPolicy { + FileSystemSandboxPolicy::external_sandbox() +} + async fn test_config() -> (TempDir, Config) { let home = TempDir::new().expect("create temp dir"); let config = ConfigBuilder::default() @@ -1686,3 +1690,100 @@ async fn verify_approval_requirement_for_unsafe_powershell_command() { (unless AskForApproval::Never is specified)."# ); } + +#[tokio::test] +async fn dangerous_command_allowed_when_sandbox_is_explicitly_disabled() { + let command = vec_str(&["rm", "-rf", "/tmp/nonexistent"]); + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: None, + command, + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ExternalSandbox { + network_access: Default::default(), + }, + file_system_sandbox_policy: external_file_system_sandbox_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: Some(ExecPolicyAmendment { + command: vec_str(&["rm", "-rf", "/tmp/nonexistent"]), + }), + }, + ) + .await; +} + +#[tokio::test] +async fn dangerous_command_forbidden_in_external_sandbox_when_policy_matches() { + let command = vec_str(&["rm", "-rf", "/tmp/nonexistent"]); + assert_exec_approval_requirement_for_command( + ExecApprovalRequirementScenario { + policy_src: Some("prefix_rule(pattern=['rm'], decision='prompt')".to_string()), + command, + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ExternalSandbox { + network_access: Default::default(), + }, + file_system_sandbox_policy: external_file_system_sandbox_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + ExecApprovalRequirement::Forbidden { + reason: "approval required by policy, but AskForApproval is set to Never".to_string(), + }, + ) + .await; +} + +struct ExecApprovalRequirementScenario { + /// Source for the Starlark `.rules` file. + policy_src: Option, + command: Vec, + approval_policy: AskForApproval, + sandbox_policy: SandboxPolicy, + file_system_sandbox_policy: FileSystemSandboxPolicy, + sandbox_permissions: SandboxPermissions, + prefix_rule: Option>, +} + +async fn assert_exec_approval_requirement_for_command( + test: ExecApprovalRequirementScenario, + expected_requirement: ExecApprovalRequirement, +) { + let ExecApprovalRequirementScenario { + policy_src, + command, + approval_policy, + sandbox_policy, + file_system_sandbox_policy, + sandbox_permissions, + prefix_rule, + } = test; + + let policy = match policy_src { + Some(src) => { + let mut parser = PolicyParser::new(); + parser + .parse("test.rules", src.as_str()) + .expect("parse policy"); + Arc::new(parser.build()) + } + None => Arc::new(Policy::empty()), + }; + + let requirement = ExecPolicyManager::new(policy) + .create_exec_approval_requirement_for_command(ExecApprovalRequest { + command: &command, + approval_policy, + sandbox_policy: &sandbox_policy, + file_system_sandbox_policy: &file_system_sandbox_policy, + sandbox_permissions, + prefix_rule, + }) + .await; + + assert_eq!(requirement, expected_requirement); +}