From af87f2160e0e79ae355cb4198ccedea3d5d8c263 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 17 Apr 2026 12:31:55 -0700 Subject: [PATCH] sandboxing: intersect permission profiles semantically --- .../app-server/src/bespoke_event_handling.rs | 117 +++++++++++++++--- .../core/src/tools/handlers/apply_patch.rs | 1 + codex-rs/core/src/tools/handlers/mod.rs | 3 +- codex-rs/core/src/tools/handlers/shell.rs | 1 + .../core/src/tools/handlers/unified_exec.rs | 1 + .../core/tests/suite/request_permissions.rs | 6 +- codex-rs/sandboxing/src/policy_transforms.rs | 89 ++++++++++++- .../sandboxing/src/policy_transforms_tests.rs | 72 ++++++++++- 8 files changed, 261 insertions(+), 29 deletions(-) diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 82be46c2c0..359878cb0e 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -2566,9 +2566,12 @@ async fn on_request_permissions_response( let response = receiver.await; resolve_server_request_on_thread_listener(&thread_state, pending_request_id).await; drop(request_permissions_guard); - let Some(response) = - request_permissions_response_from_client_result(requested_permissions, response) - else { + let cwd = conversation.config_snapshot().await.cwd; + let Some(response) = request_permissions_response_from_client_result( + requested_permissions, + response, + cwd.as_path(), + ) else { return; }; @@ -2586,6 +2589,7 @@ async fn on_request_permissions_response( fn request_permissions_response_from_client_result( requested_permissions: CoreRequestPermissionProfile, response: std::result::Result, + cwd: &std::path::Path, ) -> Option { let value = match response { Ok(Ok(value)) => value, @@ -2618,6 +2622,7 @@ fn request_permissions_response_from_client_result( permissions: intersect_permission_profiles( requested_permissions.into(), response.permissions.into(), + cwd, ) .into(), scope: response.scope.to_core(), @@ -3680,6 +3685,7 @@ mod tests { let response = request_permissions_response_from_client_result( CoreRequestPermissionProfile::default(), Ok(Err(error)), + std::env::current_dir().expect("current dir").as_path(), ); assert_eq!(response, None); @@ -3709,22 +3715,10 @@ mod tests { network: Some(CoreNetworkPermissions { enabled: Some(true), }), - file_system: Some(CoreFileSystemPermissions { - entries: vec![ - FileSystemSandboxEntry { - path: FileSystemPath::Path { - path: absolute_path(input_path), - }, - access: FileSystemAccessMode::Read, - }, - FileSystemSandboxEntry { - path: FileSystemPath::Special { - value: FileSystemSpecialPath::CurrentWorkingDirectory, - }, - access: FileSystemAccessMode::Write, - }, - ], - }), + file_system: Some(CoreFileSystemPermissions::from_read_write_roots( + Some(vec![absolute_path(input_path)]), + Some(vec![absolute_path(output_path)]), + )), }; let cases = vec![ ( @@ -3778,12 +3772,14 @@ mod tests { ), ]; + let cwd = std::env::current_dir().expect("current dir"); for (granted_permissions, expected_permissions) in cases { let response = request_permissions_response_from_client_result( requested_permissions.clone(), Ok(Ok(serde_json::json!({ "permissions": granted_permissions, }))), + cwd.as_path(), ) .expect("response should be accepted"); @@ -3805,6 +3801,7 @@ mod tests { "scope": "session", "permissions": {}, }))), + std::env::current_dir().expect("current dir").as_path(), ) .expect("response should be accepted"); @@ -3817,6 +3814,88 @@ mod tests { ); } + #[test] + fn request_permissions_response_accepts_explicit_child_grant_for_requested_cwd_scope() { + let temp_dir = TempDir::new().expect("temp dir"); + let cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute cwd"); + let child = cwd.join("child"); + let requested_permissions = CoreRequestPermissionProfile { + file_system: Some(CoreFileSystemPermissions { + entries: vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }], + }), + ..Default::default() + }; + + let response = request_permissions_response_from_client_result( + requested_permissions, + Ok(Ok(serde_json::json!({ + "permissions": { + "fileSystem": { + "write": [child], + }, + }, + }))), + cwd.as_path(), + ) + .expect("response should be accepted"); + + assert_eq!( + response.permissions, + CoreRequestPermissionProfile { + file_system: Some(CoreFileSystemPermissions::from_read_write_roots( + None, + Some(vec![child]), + )), + ..Default::default() + } + ); + } + + #[test] + fn request_permissions_response_ignores_broader_cwd_grant_for_requested_child_path() { + let temp_dir = TempDir::new().expect("temp dir"); + let cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path()).expect("absolute cwd"); + let child = cwd.join("child"); + let requested_permissions = CoreRequestPermissionProfile { + file_system: Some(CoreFileSystemPermissions::from_read_write_roots( + None, + Some(vec![child]), + )), + ..Default::default() + }; + + let response = request_permissions_response_from_client_result( + requested_permissions, + Ok(Ok(serde_json::json!({ + "permissions": { + "fileSystem": { + "entries": [{ + "path": { + "type": "special", + "value": { + "kind": "current_working_directory" + } + }, + "access": "write" + }], + }, + }, + }))), + cwd.as_path(), + ) + .expect("response should be accepted"); + + assert_eq!( + response.permissions, + CoreRequestPermissionProfile::default() + ); + } + #[test] fn collab_resume_begin_maps_to_item_started_resume_agent() { let event = CollabResumeBeginEvent { diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 675be5fecf..89447175a3 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -226,6 +226,7 @@ async fn effective_patch_permissions( ); let effective_additional_permissions = apply_granted_turn_permissions( session, + turn.cwd.as_path(), crate::sandboxing::SandboxPermissions::UseDefault, write_permissions_for_paths(&file_paths, &file_system_sandbox_policy, &turn.cwd), ) diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 17c7c09be9..0217cab33a 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -169,6 +169,7 @@ pub(super) fn implicit_granted_permissions( pub(super) async fn apply_granted_turn_permissions( session: &Session, + cwd: &std::path::Path, sandbox_permissions: SandboxPermissions, additional_permissions: Option, ) -> EffectiveAdditionalPermissions { @@ -192,7 +193,7 @@ pub(super) async fn apply_granted_turn_permissions( ); let permissions_preapproved = match (effective_permissions.as_ref(), granted_permissions) { (Some(effective_permissions), Some(granted_permissions)) => { - intersect_permission_profiles(effective_permissions.clone(), granted_permissions) + intersect_permission_profiles(effective_permissions.clone(), granted_permissions, cwd) == *effective_permissions } _ => false, diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index b6bf4bc7ad..8770238942 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -424,6 +424,7 @@ impl ShellHandler { let requested_additional_permissions = additional_permissions.clone(); let effective_additional_permissions = apply_granted_turn_permissions( session.as_ref(), + turn.cwd.as_path(), exec_params.sandbox_permissions, additional_permissions, ) diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index ab5279d1b8..fd04d42f2e 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -230,6 +230,7 @@ impl ToolHandler for UnifiedExecHandler { let requested_additional_permissions = additional_permissions.clone(); let effective_additional_permissions = apply_granted_turn_permissions( context.session.as_ref(), + context.turn.cwd.as_path(), sandbox_permissions, additional_permissions, ) diff --git a/codex-rs/core/tests/suite/request_permissions.rs b/codex-rs/core/tests/suite/request_permissions.rs index 39b0cfb44b..614cd10b9f 100644 --- a/codex-rs/core/tests/suite/request_permissions.rs +++ b/codex-rs/core/tests/suite/request_permissions.rs @@ -1586,17 +1586,17 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions() .unwrap_or_else(|| panic!("expected filesystem permissions")); let (approval_reads, approval_writes) = approval_file_system .legacy_read_write_roots() - .unwrap_or_default(); + .unwrap_or_else(|| panic!("expected legacy-compatible permissions")); assert!(approval_reads.as_ref().is_none_or(Vec::is_empty)); let mut approval_writes = approval_writes.unwrap_or_default(); approval_writes.sort_by_key(|path| path.display().to_string()); - let (_expected_reads, expected_writes) = merged_permissions + let (_, expected_writes) = merged_permissions .file_system .unwrap_or_else(|| panic!("expected merged filesystem permissions")) .legacy_read_write_roots() - .unwrap_or_default(); + .unwrap_or_else(|| panic!("expected legacy-compatible permissions")); let mut expected_writes = expected_writes.unwrap_or_default(); expected_writes.sort_by_key(|path| path.display().to_string()); diff --git a/codex-rs/sandboxing/src/policy_transforms.rs b/codex-rs/sandboxing/src/policy_transforms.rs index bcd02acd55..ede9b5a147 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -1,10 +1,12 @@ use codex_protocol::models::FileSystemPermissions; use codex_protocol::models::NetworkPermissions; use codex_protocol::models::PermissionProfile; +use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxKind; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::protocol::NetworkAccess; use codex_protocol::protocol::ReadOnlyAccess; @@ -12,6 +14,8 @@ use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::canonicalize_preserving_symlinks; use std::collections::HashSet; +use std::path::Path; +use std::path::PathBuf; #[derive(Debug, Clone, PartialEq, Eq)] pub struct EffectiveSandboxPermissions { @@ -125,15 +129,26 @@ pub fn merge_permission_profiles( pub fn intersect_permission_profiles( requested: PermissionProfile, granted: PermissionProfile, + cwd: &Path, ) -> PermissionProfile { let file_system = requested .file_system .map(|requested_file_system| { - let granted_file_system = granted.file_system.unwrap_or_default(); - let entries = requested_file_system + let requested_policy = + FileSystemSandboxPolicy::restricted(requested_file_system.entries.clone()); + let entries = granted + .file_system + .unwrap_or_default() .entries .into_iter() - .filter(|entry| granted_file_system.entries.contains(entry)) + .filter(|entry| { + granted_file_system_entry_within_request( + &requested_file_system, + &requested_policy, + entry, + cwd, + ) + }) .collect(); FileSystemPermissions { entries } }) @@ -158,6 +173,74 @@ pub fn intersect_permission_profiles( } } +fn granted_file_system_entry_within_request( + requested: &FileSystemPermissions, + requested_policy: &FileSystemSandboxPolicy, + granted_entry: &FileSystemSandboxEntry, + cwd: &Path, +) -> bool { + if !granted_entry.access.can_read() { + return false; + } + + if let Some(path) = resolve_permission_path(&granted_entry.path, cwd) { + return access_covers( + requested_policy.resolve_access_with_cwd(path.as_path(), cwd), + granted_entry.access, + ); + } + + requested.entries.iter().any(|requested_entry| { + access_covers(requested_entry.access, granted_entry.access) + && requested_entry.path == granted_entry.path + }) +} + +fn access_covers(requested: FileSystemAccessMode, granted: FileSystemAccessMode) -> bool { + match granted { + FileSystemAccessMode::Read => requested.can_read(), + FileSystemAccessMode::Write => requested.can_write(), + FileSystemAccessMode::None => false, + } +} + +fn resolve_permission_path(path: &FileSystemPath, cwd: &Path) -> Option { + match path { + FileSystemPath::Path { path } => Some(path.clone()), + FileSystemPath::GlobPattern { .. } => None, + FileSystemPath::Special { value } => match value { + FileSystemSpecialPath::Root => { + let root = cwd.ancestors().last()?; + AbsolutePathBuf::from_absolute_path(root).ok() + } + FileSystemSpecialPath::CurrentWorkingDirectory => { + AbsolutePathBuf::from_absolute_path(cwd).ok() + } + FileSystemSpecialPath::ProjectRoots { subpath } => { + let cwd = AbsolutePathBuf::from_absolute_path(cwd).ok()?; + Some(match subpath { + Some(subpath) => { + AbsolutePathBuf::resolve_path_against_base(subpath, cwd.as_path()) + } + None => cwd, + }) + } + FileSystemSpecialPath::Tmpdir => { + let tmpdir = std::env::var_os("TMPDIR")?; + if tmpdir.is_empty() { + None + } else { + AbsolutePathBuf::from_absolute_path(PathBuf::from(tmpdir)).ok() + } + } + FileSystemSpecialPath::SlashTmp => AbsolutePathBuf::from_absolute_path("/tmp") + .ok() + .filter(|path| path.as_path().is_dir()), + FileSystemSpecialPath::Minimal | FileSystemSpecialPath::Unknown { .. } => None, + }, + } +} + fn merge_permission_entries( base: &[FileSystemSandboxEntry], permissions: &[FileSystemSandboxEntry], diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs index 3b74edf6f0..7cd52c82de 100644 --- a/codex-rs/sandboxing/src/policy_transforms_tests.rs +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -189,7 +189,7 @@ fn intersect_permission_profiles_preserves_explicit_empty_requested_reads() { let granted = requested.clone(); assert_eq!( - intersect_permission_profiles(requested.clone(), granted), + intersect_permission_profiles(requested.clone(), granted, temp_dir.path()), requested ); } @@ -210,7 +210,7 @@ fn intersect_permission_profiles_drops_ungranted_nonempty_path_requests() { }; assert_eq!( - intersect_permission_profiles(requested, PermissionProfile::default()), + intersect_permission_profiles(requested, PermissionProfile::default(), temp_dir.path()), PermissionProfile::default() ); } @@ -231,7 +231,73 @@ fn intersect_permission_profiles_drops_explicit_empty_reads_without_grant() { }; assert_eq!( - intersect_permission_profiles(requested, PermissionProfile::default()), + intersect_permission_profiles(requested, PermissionProfile::default(), temp_dir.path()), + PermissionProfile::default() + ); +} + +#[test] +fn intersect_permission_profiles_accepts_child_path_granted_for_requested_cwd() { + 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 child = cwd.join("child"); + let requested = PermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }], + }), + ..Default::default() + }; + let granted = PermissionProfile { + file_system: Some(FileSystemPermissions::from_read_write_roots( + None, + Some(vec![child.clone()]), + )), + ..Default::default() + }; + + assert_eq!( + intersect_permission_profiles(requested, granted.clone(), cwd.as_path()), + granted + ); +} + +#[test] +fn intersect_permission_profiles_drops_broader_cwd_grant_for_requested_child_path() { + 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 child = cwd.join("child"); + let requested = PermissionProfile { + file_system: Some(FileSystemPermissions::from_read_write_roots( + None, + Some(vec![child]), + )), + ..Default::default() + }; + let granted = PermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }], + }), + ..Default::default() + }; + + assert_eq!( + intersect_permission_profiles(requested, granted, cwd.as_path()), PermissionProfile::default() ); }