diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 69af0c8152..47bcfe0970 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -35,7 +35,7 @@ mod view_image; pub(crate) mod view_image_spec; mod wait_for_environment; -use codex_sandboxing::policy_transforms::intersect_permission_profiles; +use codex_sandboxing::policy_transforms::materialize_additional_permissions; use codex_sandboxing::policy_transforms::merge_permission_profiles; use codex_sandboxing::policy_transforms::normalize_additional_permissions; use codex_utils_absolute_path::AbsolutePathBuf; @@ -291,12 +291,19 @@ pub(super) async fn apply_granted_turn_permissions( additional_permissions.as_ref(), granted_permissions.as_ref(), ); - let permissions_preapproved = match (effective_permissions.as_ref(), granted_permissions) { - (Some(effective_permissions), Some(granted_permissions)) => { - permissions_are_preapproved(effective_permissions, granted_permissions, cwd) + let preapproved_permissions = granted_permissions.as_ref().and_then(|granted| { + if additional_permissions.is_none() { + Some(granted.clone()) + } else { + effective_permissions + .as_ref() + .and_then(|effective| preapproved_permission_profile(effective, granted, cwd)) } - _ => false, - }; + }); + let permissions_preapproved = preapproved_permissions.is_some(); + // A preapproved command must execute with the stored authority, never an + // unchecked merge that could reopen one of the grant's denied paths. + let effective_permissions = preapproved_permissions.or(effective_permissions); let sandbox_permissions = if effective_permissions.is_some() && !sandbox_permissions.uses_additional_permissions() { @@ -312,26 +319,45 @@ pub(super) async fn apply_granted_turn_permissions( } } -fn permissions_are_preapproved( +fn preapproved_permission_profile( effective_permissions: &AdditionalPermissionProfile, - granted_permissions: AdditionalPermissionProfile, + granted_permissions: &AdditionalPermissionProfile, cwd: &Path, -) -> bool { - let materialized_effective_permissions = intersect_permission_profiles( - effective_permissions.clone(), - effective_permissions.clone(), - cwd, - ); - intersect_permission_profiles(effective_permissions.clone(), granted_permissions, cwd) - == materialized_effective_permissions +) -> Option { + let (Ok(effective), Ok(granted)) = ( + materialize_additional_permissions(effective_permissions.clone(), cwd), + materialize_additional_permissions(granted_permissions.clone(), cwd), + ) else { + return None; + }; + if effective.network != granted.network { + return None; + } + let unchanged = match (effective.file_system, granted.file_system) { + (Some(effective), Some(granted)) => { + effective.glob_scan_max_depth == granted.glob_scan_max_depth + && effective.entries.len() == granted.entries.len() + && effective + .entries + .iter() + .all(|entry| granted.entries.contains(entry)) + } + (None, None) => true, + (Some(_), None) | (None, Some(_)) => false, + }; + unchanged.then(|| granted_permissions.clone()) } +#[cfg(test)] +#[path = "permission_preapproval_tests.rs"] +mod permission_preapproval_tests; + #[cfg(test)] mod tests { use super::EffectiveAdditionalPermissions; use super::implicit_granted_permissions; use super::normalize_and_validate_additional_permissions; - use super::permissions_are_preapproved; + use super::preapproved_permission_profile; use crate::sandboxing::SandboxPermissions; use codex_protocol::models::AdditionalPermissionProfile; use codex_protocol::models::FileSystemPermissions; @@ -481,10 +507,9 @@ mod tests { merge_permission_profiles(Some(&requested_permissions), Some(&stored_grant)) .expect("merged permissions"); - assert!(permissions_are_preapproved( - &effective_permissions, - stored_grant, - cwd.path(), - )); + assert_eq!( + preapproved_permission_profile(&effective_permissions, &stored_grant, cwd.path()), + Some(stored_grant) + ); } } diff --git a/codex-rs/core/src/tools/handlers/permission_preapproval_tests.rs b/codex-rs/core/src/tools/handlers/permission_preapproval_tests.rs new file mode 100644 index 0000000000..13826255b6 --- /dev/null +++ b/codex-rs/core/src/tools/handlers/permission_preapproval_tests.rs @@ -0,0 +1,82 @@ +use super::preapproved_permission_profile; +use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::models::FileSystemPermissions; +use codex_protocol::models::NetworkPermissions; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_sandboxing::policy_transforms::merge_permission_profiles; +use codex_utils_absolute_path::AbsolutePathBuf; +use pretty_assertions::assert_eq; +use tempfile::tempdir; + +fn file_system_permissions(entries: Vec) -> AdditionalPermissionProfile { + AdditionalPermissionProfile { + file_system: Some(FileSystemPermissions { + entries, + glob_scan_max_depth: None, + }), + ..Default::default() + } +} + +#[test] +fn preapproval_accepts_reordered_replay_of_one_accumulated_grant() { + let cwd = tempdir().expect("tempdir"); + let root = AbsolutePathBuf::from_absolute_path(cwd.path()).expect("absolute cwd"); + let write = FileSystemSandboxEntry::new(root.clone().into(), FileSystemAccessMode::Write); + let mut granted = file_system_permissions(vec![ + FileSystemSandboxEntry::new(root.join("secrets").into(), FileSystemAccessMode::Deny), + write.clone(), + FileSystemSandboxEntry::new(root.join("readonly").into(), FileSystemAccessMode::Read), + ]); + granted.network = Some(NetworkPermissions { + enabled: Some(true), + }); + let replay = file_system_permissions(vec![write]); + let effective = merge_permission_profiles(Some(&replay), Some(&granted)).expect("permissions"); + + assert_eq!( + preapproved_permission_profile(&effective, &granted, cwd.path()), + Some(granted) + ); +} + +#[test] +fn preapproval_requires_fresh_read_and_write_beneath_a_deny() { + let cwd = tempdir().expect("tempdir"); + let root = AbsolutePathBuf::from_absolute_path(cwd.path()).expect("absolute cwd"); + let granted = file_system_permissions(vec![ + FileSystemSandboxEntry::new(root.clone().into(), FileSystemAccessMode::Write), + FileSystemSandboxEntry::new(root.join("secrets").into(), FileSystemAccessMode::Deny), + ]); + + let preapproved = [FileSystemAccessMode::Read, FileSystemAccessMode::Write].map(|access| { + let requested = file_system_permissions(vec![FileSystemSandboxEntry::new( + root.join("secrets/token.txt").into(), + access, + )]); + let effective = + merge_permission_profiles(Some(&requested), Some(&granted)).expect("permissions"); + + preapproved_permission_profile(&effective, &granted, cwd.path()) + }); + + assert_eq!(preapproved, [None, None]); +} + +#[test] +fn preapproval_fails_closed_when_materialization_rejects_both_profiles() { + let cwd = tempdir().expect("tempdir"); + let invalid = file_system_permissions(vec![FileSystemSandboxEntry::new( + FileSystemPath::GlobPattern { + pattern: "**/secrets".to_string(), + }, + FileSystemAccessMode::Write, + )]); + + assert_eq!( + preapproved_permission_profile(&invalid, &invalid, cwd.path()), + None + ); +} diff --git a/codex-rs/core/tests/suite/request_permissions.rs b/codex-rs/core/tests/suite/request_permissions.rs index aa68463d85..befb16c22f 100644 --- a/codex-rs/core/tests/suite/request_permissions.rs +++ b/codex-rs/core/tests/suite/request_permissions.rs @@ -13,6 +13,8 @@ use codex_protocol::config_types::Settings; use codex_protocol::models::AdditionalPermissionProfile as PermissionProfile; use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::PermissionProfile as CorePermissionProfile; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; @@ -27,6 +29,8 @@ use codex_protocol::request_permissions::RequestPermissionProfile; use codex_protocol::request_permissions::RequestPermissionsResponse; use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; +use core_test_support::responses::ev_apply_patch_custom_tool_call; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; use core_test_support::responses::ev_function_call; @@ -38,9 +42,12 @@ use core_test_support::responses::sse; use core_test_support::responses::sse_response; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; +use core_test_support::skip_if_remote; use core_test_support::skip_if_sandbox; +use core_test_support::skip_if_target_windows; use core_test_support::skip_if_wine_exec; use core_test_support::test_codex::TestCodex; +use core_test_support::test_codex::TestCodexHarness; use core_test_support::test_codex::local_selections; use core_test_support::test_codex::test_codex; use core_test_support::test_codex::turn_permission_fields; @@ -2208,3 +2215,272 @@ async fn request_permissions_session_grants_carry_across_turns() -> Result<()> { Ok(()) } + +const SENTINEL_PATH: &str = "denied-child-permissions/secrets/nested/sentinel.txt"; +const SENTINEL_CONTENT: &str = "untouched\n"; +const PERMISSIONS_CALL_ID: &str = "denied-child-permissions-grant"; +const WRITE_CALL_ID: &str = "denied-child-permissions-write"; + +#[derive(Clone, Copy, Debug)] +enum WriteTool { + ExecCommand, + ShellCommand, + ApplyPatch, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum ApprovalMode { + Prompt, + Never, + InlineFeatureDisabled, +} + +#[test_case(WriteTool::ExecCommand, PermissionGrantScope::Turn, ApprovalMode::Prompt; "exec_command_turn")] +#[test_case(WriteTool::ExecCommand, PermissionGrantScope::Session, ApprovalMode::Prompt; "exec_command_session")] +#[test_case(WriteTool::ShellCommand, PermissionGrantScope::Turn, ApprovalMode::Prompt; "shell_command_turn")] +#[test_case(WriteTool::ShellCommand, PermissionGrantScope::Session, ApprovalMode::Prompt; "shell_command_session")] +#[test_case(WriteTool::ApplyPatch, PermissionGrantScope::Turn, ApprovalMode::Prompt; "apply_patch_turn")] +#[test_case(WriteTool::ApplyPatch, PermissionGrantScope::Session, ApprovalMode::Prompt; "apply_patch_session")] +#[test_case(WriteTool::ExecCommand, PermissionGrantScope::Session, ApprovalMode::Never; "exec_command_session_never")] +#[test_case(WriteTool::ExecCommand, PermissionGrantScope::Session, ApprovalMode::InlineFeatureDisabled; "exec_command_inline_feature_disabled")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn denied_child_permissions_require_fresh_approval( + tool: WriteTool, + scope: PermissionGrantScope, + mode: ApprovalMode, +) -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_sandbox!(Ok(())); + skip_if_target_windows!( + Ok(()), + "this regression exercises POSIX split-policy enforcement; a disabled Windows sandbox can independently prompt for the command" + ); + if matches!(tool, WriteTool::ShellCommand) { + skip_if_remote!( + Ok(()), + "the legacy shell_command tool is only registered for a single local environment" + ); + } + + let harness = + TestCodexHarness::with_auto_env_builder(test_codex().with_config(move |config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = ApprovalsReviewer::User; + config + .permissions + .set_permission_profile(CorePermissionProfile::read_only()) + .expect("set permission profile"); + config + .features + .enable(Feature::UnifiedExec) + .expect("enable unified exec"); + let inline_permissions = if mode == ApprovalMode::InlineFeatureDisabled { + config.features.disable(Feature::ExecPermissionApprovals) + } else { + config.features.enable(Feature::ExecPermissionApprovals) + }; + inline_permissions.expect("configure inline permissions"); + config + .features + .enable(Feature::RequestPermissionsTool) + .expect("enable request_permissions"); + })) + .await?; + harness.write_file(SENTINEL_PATH, SENTINEL_CONTENT).await?; + let test = harness.test(); + let root = test + .fs() + .canonicalize( + &PathUri::from_abs_path(&harness.path_abs("denied-child-permissions")), + /*sandbox*/ None, + ) + .await? + .to_abs_path()?; + let denied_root = root.join("secrets"); + let target = denied_root.join("nested/sentinel.txt"); + let requested_permissions = requested_directory_write_permissions(root.as_path()); + let constrained_permissions = RequestPermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![ + FileSystemSandboxEntry::new(root.clone().into(), FileSystemAccessMode::Write), + FileSystemSandboxEntry::new(denied_root.clone().into(), FileSystemAccessMode::Deny), + ], + glob_scan_max_depth: None, + }), + ..Default::default() + }; + let approved_response = RequestPermissionsResponse { + permissions: constrained_permissions.clone(), + scope, + strict_auto_review: false, + }; + let fresh_permissions = requested_directory_write_permissions(target.as_path()); + let command = format!("printf changed > {:?}", target.as_path()); + let write_event = match tool { + WriteTool::ExecCommand => exec_command_event_with_request_permissions( + WRITE_CALL_ID, + &command, + &fresh_permissions, + )?, + WriteTool::ShellCommand => { + shell_event_with_request_permissions(WRITE_CALL_ID, &command, &fresh_permissions)? + } + WriteTool::ApplyPatch => ev_apply_patch_custom_tool_call( + WRITE_CALL_ID, + &format!( + "*** Begin Patch\n*** Update File: {}\n@@\n-untouched\n+changed\n*** End Patch\n", + target.display() + ), + ), + }; + let response = |id, event| sse(vec![ev_response_created(id), event, ev_completed(id)]); + let mut response_sequence = vec![response( + "grant", + request_permissions_tool_event( + PERMISSIONS_CALL_ID, + "Allow writes except in secrets", + &requested_permissions, + )?, + )]; + if scope == PermissionGrantScope::Session { + response_sequence.push(response( + "grant-complete", + ev_assistant_message("grant-message", "grant recorded"), + )); + } + response_sequence.extend([ + response("write", write_event), + response("complete", ev_assistant_message("done-message", "done")), + ]); + let responses = mount_sse_sequence(harness.server(), response_sequence).await; + + test.codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "request constrained permissions, then try the denied child".into(), + text_elements: Vec::new(), + }])) + .await?; + assert_eq!( + expect_request_permissions_event(test, PERMISSIONS_CALL_ID).await, + requested_permissions + ); + test.codex + .submit(Op::RequestPermissionsResponse { + id: PERMISSIONS_CALL_ID.to_string(), + response: approved_response.clone(), + }) + .await?; + + if scope == PermissionGrantScope::Session { + wait_for_completion(test).await; + let approval_policy = match mode { + ApprovalMode::Never => AskForApproval::Never, + ApprovalMode::Prompt | ApprovalMode::InlineFeatureDisabled => AskForApproval::OnRequest, + }; + test.codex + .start_or_steer_turn( + TurnInputRequest::user_input(vec![UserInput::Text { + text: "try the denied child using the stored session grant".into(), + text_elements: Vec::new(), + }]) + .with_thread_settings(ThreadSettingsOverrides { + approval_policy: Some(approval_policy), + ..Default::default() + }), + ) + .await?; + } + + let expected_error = match mode { + ApprovalMode::Prompt => None, + ApprovalMode::Never => Some("approval policy is Never"), + ApprovalMode::InlineFeatureDisabled => Some("additional permissions are disabled"), + }; + let event = wait_for_event(&test.codex, |event| { + matches!( + event, + EventMsg::ExecApprovalRequest(_) + | EventMsg::ApplyPatchApprovalRequest(_) + | EventMsg::TurnComplete(_) + ) || (expected_error.is_some() && matches!(event, EventMsg::ExecCommandBegin(_))) + }) + .await; + if let Some(expected_error) = expected_error { + assert!( + matches!(event, EventMsg::TurnComplete(_)), + "{mode:?} must reject before approval or execution: {event:?}" + ); + let output = responses + .function_call_output_text(WRITE_CALL_ID) + .context("rejected exec output")?; + assert!( + output.contains(expected_error), + "unexpected rejection: {output}" + ); + assert_eq!( + harness.read_file_text(SENTINEL_PATH).await?, + SENTINEL_CONTENT + ); + return Ok(()); + } + + let (decision, reason) = match (tool, event) { + ( + WriteTool::ExecCommand | WriteTool::ShellCommand, + EventMsg::ExecApprovalRequest(approval), + ) => { + assert_eq!(approval.call_id, WRITE_CALL_ID); + let expected_permissions = PermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![ + FileSystemSandboxEntry::new(target.into(), FileSystemAccessMode::Write), + FileSystemSandboxEntry::new(root.into(), FileSystemAccessMode::Write), + FileSystemSandboxEntry::new(denied_root.into(), FileSystemAccessMode::Deny), + ], + glob_scan_max_depth: None, + }), + ..Default::default() + }; + assert_eq!(approval.additional_permissions, Some(expected_permissions)); + ( + Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::denied("denied child is not approved"), + }, + approval.reason, + ) + } + (WriteTool::ApplyPatch, EventMsg::ApplyPatchApprovalRequest(approval)) => { + assert_eq!(approval.call_id, WRITE_CALL_ID); + ( + Op::PatchApproval { + id: approval.call_id, + decision: ReviewDecision::denied("denied child is not approved"), + }, + approval.reason, + ) + } + (_, event) => panic!("expected fresh {tool:?} permission approval, got {event:?}"), + }; + let content_before_denial = harness.read_file_text(SENTINEL_PATH).await?; + test.codex.submit(decision).await?; + wait_for_completion(test).await; + + assert_eq!( + reason, None, + "the first attempt must require approval, not only a retry after sandbox denial" + ); + assert_eq!(content_before_denial, SENTINEL_CONTENT); + assert_eq!( + harness.read_file_text(SENTINEL_PATH).await?, + SENTINEL_CONTENT + ); + let recorded_grant: RequestPermissionsResponse = serde_json::from_str( + &responses + .function_call_output_text(PERMISSIONS_CALL_ID) + .context("request_permissions response")?, + )?; + assert_eq!(recorded_grant, approved_response); + Ok(()) +} diff --git a/codex-rs/sandboxing/src/policy_transforms.rs b/codex-rs/sandboxing/src/policy_transforms.rs index c723961c42..d79f8a0df2 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -72,6 +72,21 @@ pub fn normalize_additional_permissions( }) } +/// Resolves cwd-dependent permission entries without filtering their authority. +/// +/// Unlike intersection, this preserves narrower grants beneath denied paths. +pub fn materialize_additional_permissions( + mut additional_permissions: AdditionalPermissionProfile, + cwd: &Path, +) -> Result { + if let Some(file_system) = additional_permissions.file_system.as_mut() { + for entry in &mut file_system.entries { + *entry = materialize_cwd_dependent_entry(entry, cwd); + } + } + normalize_additional_permissions(additional_permissions) +} + pub fn merge_permission_profiles( base: Option<&AdditionalPermissionProfile>, permissions: Option<&AdditionalPermissionProfile>, diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs index dc3387bfa5..b43863d8a2 100644 --- a/codex-rs/sandboxing/src/policy_transforms_tests.rs +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -1,5 +1,6 @@ use super::effective_file_system_sandbox_policy; use super::intersect_permission_profiles; +use super::materialize_additional_permissions; use super::merge_file_system_policy_with_additional_permissions; use super::normalize_additional_permissions; use super::should_require_platform_sandbox; @@ -230,6 +231,54 @@ fn normalize_additional_permissions_drops_empty_nested_profiles() { assert_eq!(permissions, PermissionProfile::default()); } +#[test] +fn materialize_additional_permissions_preserves_authority_and_constraints() { + use FileSystemAccessMode::Deny; + use FileSystemAccessMode::Read; + use FileSystemAccessMode::Write; + + let temp_dir = TempDir::new().expect("create temp dir"); + let cwd = AbsolutePathBuf::from_absolute_path( + canonicalize(temp_dir.path()).expect("canonicalize temp dir"), + ) + .expect("absolute temp dir"); + let project_path = |subpath: &str| FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(Some(subpath.to_owned())), + }; + let deny_glob = |pattern: String| { + FileSystemSandboxEntry::new(FileSystemPath::GlobPattern { pattern }, Deny) + }; + let profile = |entries| PermissionProfile { + file_system: Some(FileSystemPermissions { + entries, + glob_scan_max_depth: std::num::NonZeroUsize::new(/*n*/ 3), + }), + ..Default::default() + }; + let reopened = FileSystemSandboxEntry::new(cwd.join("private/reopened").into(), Write); + let requested = profile(vec![ + FileSystemSandboxEntry::new(project_path("."), Write), + FileSystemSandboxEntry::new(project_path("private"), Deny), + FileSystemSandboxEntry::skip_missing_path(project_path("readonly"), Read), + FileSystemSandboxEntry::new(project_path("private/reopened"), Write), + reopened.clone(), + deny_glob("**/*.env".to_owned()), + ]); + let expected = profile(vec![ + FileSystemSandboxEntry::new(cwd.clone().into(), Write), + FileSystemSandboxEntry::new(cwd.join("private").into(), Deny), + FileSystemSandboxEntry::skip_missing_path(cwd.join("readonly").into(), Read), + reopened, + deny_glob(cwd.join("**/*.env").to_string_lossy().into_owned()), + ]); + + assert_eq!( + materialize_additional_permissions(requested, cwd.as_path()) + .expect("materialized permissions"), + expected + ); +} + #[test] fn intersect_permission_profiles_preserves_explicit_empty_requested_reads() { let temp_dir = TempDir::new().expect("create temp dir");