From 8aa3a11adaa0630ebe5ddcdb802ada6a112db415 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 20 Apr 2026 19:42:55 -0700 Subject: [PATCH] sandboxing: intersect permission profiles semantically --- .../app-server/src/bespoke_event_handling.rs | 102 +++++++++++++++++- .../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 | 102 +++++++++++++++++- .../sandboxing/src/policy_transforms_tests.rs | 80 +++++++++++++- 8 files changed, 279 insertions(+), 17 deletions(-) diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index c001a77bfa..0b96727ace 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -2597,9 +2597,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; }; @@ -2617,6 +2620,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, @@ -2649,6 +2653,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(), @@ -3023,6 +3028,10 @@ mod tests { use codex_protocol::mcp::CallToolResult; use codex_protocol::models::FileSystemPermissions as CoreFileSystemPermissions; use codex_protocol::models::NetworkPermissions as CoreNetworkPermissions; + use codex_protocol::permissions::FileSystemAccessMode; + use codex_protocol::permissions::FileSystemPath; + use codex_protocol::permissions::FileSystemSandboxEntry; + use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::plan_tool::PlanItemArg; use codex_protocol::plan_tool::StepStatus; use codex_protocol::protocol::CollabResumeBeginEvent; @@ -3707,6 +3716,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); @@ -3793,12 +3803,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"); @@ -3820,6 +3832,7 @@ mod tests { "scope": "session", "permissions": {}, }))), + std::env::current_dir().expect("current dir").as_path(), ) .expect("response should be accepted"); @@ -3832,6 +3845,89 @@ 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, + }], + glob_scan_max_depth: None, + }), + ..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 9d40bbad0c..033315a694 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -259,6 +259,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 2b8c01eaae..8ec505cf82 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -6,6 +6,7 @@ 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; @@ -14,6 +15,8 @@ use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::canonicalize_preserving_symlinks; use std::collections::HashSet; use std::num::NonZeroUsize; +use std::path::Path; +use std::path::PathBuf; #[derive(Debug, Clone, PartialEq, Eq)] pub struct EffectiveSandboxPermissions { @@ -146,21 +149,42 @@ 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: Vec<_> = requested_file_system + let requested_policy = + FileSystemSandboxPolicy::restricted(requested_file_system.entries.clone()); + let mut entries: Vec<_> = granted_file_system .entries - .into_iter() - .filter(|entry| granted_file_system.entries.contains(entry)) + .iter() + .filter(|entry| { + granted_file_system_entry_within_request( + &requested_file_system, + &requested_policy, + entry, + cwd, + ) + }) + .cloned() .collect(); + for entry in requested_file_system + .entries + .iter() + .chain(granted_file_system.entries.iter()) + .filter(|entry| entry.access == FileSystemAccessMode::None) + { + if !entries.contains(entry) { + entries.push(entry.clone()); + } + } FileSystemPermissions { glob_scan_max_depth: merge_glob_scan_max_depth( - &entries, + &requested_file_system.entries, requested_file_system.glob_scan_max_depth.map(usize::from), - &entries, + &granted_file_system.entries, granted_file_system.glob_scan_max_depth.map(usize::from), ) .and_then(NonZeroUsize::new), @@ -230,6 +254,74 @@ enum GlobScanDepth { Unbounded, } +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 8fcff965fe..4423a7cf13 100644 --- a/codex-rs/sandboxing/src/policy_transforms_tests.rs +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -244,7 +244,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 ); } @@ -265,7 +265,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() ); } @@ -286,13 +286,82 @@ 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, + }], + glob_scan_max_depth: None, + }), + ..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, + }], + glob_scan_max_depth: None, + }), + ..Default::default() + }; + + assert_eq!( + intersect_permission_profiles(requested, granted, cwd.as_path()), PermissionProfile::default() ); } #[test] fn intersect_permission_profiles_uses_granted_bounded_glob_scan_depth() { + let cwd = std::env::current_dir().expect("current dir"); let deny_env_files = FileSystemSandboxEntry { path: FileSystemPath::GlobPattern { pattern: "**/*.env".to_string(), @@ -315,7 +384,7 @@ fn intersect_permission_profiles_uses_granted_bounded_glob_scan_depth() { }; assert_eq!( - intersect_permission_profiles(requested, granted), + intersect_permission_profiles(requested, granted, cwd.as_path()), PermissionProfile { file_system: Some(FileSystemPermissions { entries: vec![deny_env_files], @@ -328,6 +397,7 @@ fn intersect_permission_profiles_uses_granted_bounded_glob_scan_depth() { #[test] fn intersect_permission_profiles_uses_granted_unbounded_glob_scan_depth() { + let cwd = std::env::current_dir().expect("current dir"); let deny_env_files = FileSystemSandboxEntry { path: FileSystemPath::GlobPattern { pattern: "**/*.env".to_string(), @@ -350,7 +420,7 @@ fn intersect_permission_profiles_uses_granted_unbounded_glob_scan_depth() { }; assert_eq!( - intersect_permission_profiles(requested, granted), + intersect_permission_profiles(requested, granted, cwd.as_path()), PermissionProfile { file_system: Some(FileSystemPermissions { entries: vec![deny_env_files],