From 799e50412e6a5af88c77c1e071f3e1245936a6ed Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 21 Apr 2026 17:28:58 -0700 Subject: [PATCH] sandboxing: materialize cwd-relative permission globs (#18867) ## Why #18275 anchors session-scoped `:cwd` and `:project_roots` grants to the request cwd before recording them for reuse. Relative deny glob entries need the same treatment. Without anchoring, a stored session permission can keep a pattern such as `**/*.env` relative, then reinterpret that deny against a later turn cwd. That makes the persisted profile depend on the cwd at reuse time instead of the cwd that was reviewed and approved. ## What changed `intersect_permission_profiles` now materializes retained `FileSystemPath::GlobPattern` entries against the request cwd, matching the existing materialization for cwd-sensitive special paths. Materialized accepted grants are now deduplicated before deny retention runs. This keeps the sticky-grant preapproval shape stable when a repeated request is merged with the stored grant and both `:cwd = write` and the materialized absolute cwd write are present. The preapproval check compares against the same materialized form, so a later request for the same cwd-relative deny glob still matches the stored anchored grant instead of re-prompting or rejecting. Tests cover both the storage path and the preapproval path: a session-scoped `:cwd = write` grant with `**/*.env = none` is stored with both the cwd write and deny glob anchored to the original request cwd, cannot be reused from a later cwd, and remains preapproved when re-requested from the original cwd after merging with the stored grant. ## Verification - `cargo test -p codex-sandboxing policy_transforms` - `cargo test -p codex-core --lib relative_deny_glob_grants_remain_preapproved_after_materialization` - `cargo clippy -p codex-sandboxing --tests -- -D clippy::redundant_clone` - `cargo clippy -p codex-core --lib -- -D clippy::redundant_clone` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/18867). * #18288 * #18287 * #18286 * #18285 * #18284 * #18283 * #18282 * #18281 * #18280 * #18279 * #18278 * #18277 * #18276 * __->__ #18867 --- codex-rs/core/src/tools/handlers/mod.rs | 63 +++++++- codex-rs/sandboxing/src/policy_transforms.rs | 41 +++--- .../sandboxing/src/policy_transforms_tests.rs | 139 +++++++++++++++++- 3 files changed, 220 insertions(+), 23 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/mod.rs b/codex-rs/core/src/tools/handlers/mod.rs index 0217cab33..af4178f71 100644 --- a/codex-rs/core/src/tools/handlers/mod.rs +++ b/codex-rs/core/src/tools/handlers/mod.rs @@ -193,8 +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, cwd) - == *effective_permissions + permissions_are_preapproved(effective_permissions, granted_permissions, cwd) } _ => false, }; @@ -213,17 +212,38 @@ pub(super) async fn apply_granted_turn_permissions( } } +fn permissions_are_preapproved( + effective_permissions: &PermissionProfile, + granted_permissions: PermissionProfile, + 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 +} + #[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 crate::sandboxing::SandboxPermissions; 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::FileSystemSpecialPath; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::GranularApprovalConfig; + use codex_sandboxing::policy_transforms::intersect_permission_profiles; + use codex_sandboxing::policy_transforms::merge_permission_profiles; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -326,4 +346,43 @@ mod tests { assert_eq!(implicit_permissions, None); } + + #[test] + fn relative_deny_glob_grants_remain_preapproved_after_materialization() { + let cwd = tempdir().expect("tempdir"); + let requested_permissions = PermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "**/*.env".to_string(), + }, + access: FileSystemAccessMode::None, + }, + ], + glob_scan_max_depth: None, + }), + ..Default::default() + }; + let stored_grant = intersect_permission_profiles( + requested_permissions.clone(), + requested_permissions.clone(), + cwd.path(), + ); + let effective_permissions = + merge_permission_profiles(Some(&requested_permissions), Some(&stored_grant)) + .expect("merged permissions"); + + assert!(permissions_are_preapproved( + &effective_permissions, + stored_grant, + cwd.path(), + )); + } } diff --git a/codex-rs/sandboxing/src/policy_transforms.rs b/codex-rs/sandboxing/src/policy_transforms.rs index 134ac84f1..9b5e444d7 100644 --- a/codex-rs/sandboxing/src/policy_transforms.rs +++ b/codex-rs/sandboxing/src/policy_transforms.rs @@ -159,20 +159,21 @@ pub fn intersect_permission_profiles( let requested_policy = FileSystemSandboxPolicy::restricted(requested_file_system.entries.clone()); let requested_read_deny_matcher = ReadDenyMatcher::new(&requested_policy, cwd); - let accepted_entries: Vec<_> = granted_file_system - .entries - .iter() - .filter(|entry| { - granted_file_system_entry_within_request( - &requested_file_system, - &requested_policy, - requested_read_deny_matcher.as_ref(), - entry, - cwd, - ) - }) - .map(|entry| materialize_cwd_dependent_entry(entry, cwd)) - .collect(); + let mut accepted_entries = Vec::new(); + for entry in granted_file_system.entries.iter().filter(|entry| { + granted_file_system_entry_within_request( + &requested_file_system, + &requested_policy, + requested_read_deny_matcher.as_ref(), + entry, + cwd, + ) + }) { + let entry = materialize_cwd_dependent_entry(entry, cwd); + if !accepted_entries.contains(&entry) { + accepted_entries.push(entry); + } + } let mut entries = accepted_entries.clone(); let requested_retained_deny_entries = retain_constraining_deny_entries( &requested_file_system.entries, @@ -383,9 +384,15 @@ fn materialize_cwd_dependent_entry( access: entry.access, }) .unwrap_or_else(|| entry.clone()), - FileSystemPath::Path { .. } - | FileSystemPath::GlobPattern { .. } - | FileSystemPath::Special { .. } => entry.clone(), + FileSystemPath::GlobPattern { pattern } => FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: AbsolutePathBuf::resolve_path_against_base(pattern, cwd) + .to_string_lossy() + .into_owned(), + }, + access: entry.access, + }, + FileSystemPath::Path { .. } | FileSystemPath::Special { .. } => entry.clone(), } } diff --git a/codex-rs/sandboxing/src/policy_transforms_tests.rs b/codex-rs/sandboxing/src/policy_transforms_tests.rs index e34158bc9..38b4bd5f0 100644 --- a/codex-rs/sandboxing/src/policy_transforms_tests.rs +++ b/codex-rs/sandboxing/src/policy_transforms_tests.rs @@ -377,6 +377,42 @@ fn intersect_permission_profiles_materializes_cwd_grant_for_reuse() { ); } +#[test] +fn intersect_permission_profiles_deduplicates_materialized_grants() { + let temp_dir = TempDir::new().expect("create temp dir"); + let cwd = + AbsolutePathBuf::from_absolute_path(temp_dir.path().join("cwd")).expect("absolute cwd"); + let permissions = PermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { path: cwd.clone() }, + access: FileSystemAccessMode::Write, + }, + ], + glob_scan_max_depth: None, + }), + ..Default::default() + }; + + assert_eq!( + intersect_permission_profiles(permissions.clone(), permissions, cwd.as_path()), + PermissionProfile { + file_system: Some(FileSystemPermissions::from_read_write_roots( + /*read*/ None, + Some(vec![cwd]), + )), + ..Default::default() + } + ); +} + #[test] fn intersect_permission_profiles_materializes_cwd_deny_entries() { let temp_dir = TempDir::new().expect("create temp dir"); @@ -509,6 +545,75 @@ fn intersect_permission_profiles_rejects_concrete_grants_matched_by_requested_de ); } +#[test] +fn intersect_permission_profiles_materializes_relative_deny_globs_for_reuse() { + let temp_dir = TempDir::new().expect("create temp dir"); + let request_cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("request-cwd")) + .expect("absolute request cwd"); + let later_cwd = AbsolutePathBuf::from_absolute_path(temp_dir.path().join("later-cwd")) + .expect("absolute later cwd"); + let cwd_write = FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }; + let deny_env_files = FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "**/*.env".to_string(), + }, + access: FileSystemAccessMode::None, + }; + let permissions = PermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![cwd_write, deny_env_files], + glob_scan_max_depth: std::num::NonZeroUsize::new(2), + }), + ..Default::default() + }; + + let intersected = + intersect_permission_profiles(permissions.clone(), permissions, request_cwd.as_path()); + + assert_eq!( + intersected, + PermissionProfile { + file_system: Some(FileSystemPermissions { + entries: vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: request_cwd.clone(), + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: request_cwd.join("**/*.env").to_string_lossy().into_owned(), + }, + access: FileSystemAccessMode::None, + }, + ], + glob_scan_max_depth: std::num::NonZeroUsize::new(2), + }), + ..Default::default() + } + ); + assert_eq!( + intersect_permission_profiles( + PermissionProfile { + file_system: Some(FileSystemPermissions::from_read_write_roots( + /*read*/ None, + Some(vec![later_cwd.join("token.env")]), + )), + ..Default::default() + }, + intersected, + later_cwd.as_path(), + ), + PermissionProfile::default() + ); +} + #[test] fn intersect_permission_profiles_drops_broader_cwd_grant_for_requested_child_path() { let temp_dir = TempDir::new().expect("create temp dir"); @@ -567,7 +672,7 @@ fn intersect_permission_profiles_uses_granted_bounded_glob_scan_depth() { }; let granted = PermissionProfile { file_system: Some(FileSystemPermissions { - entries: vec![root_write.clone(), deny_env_files.clone()], + entries: vec![root_write.clone(), deny_env_files], glob_scan_max_depth: std::num::NonZeroUsize::new(4), }), ..Default::default() @@ -577,7 +682,20 @@ fn intersect_permission_profiles_uses_granted_bounded_glob_scan_depth() { intersect_permission_profiles(requested, granted, cwd.as_path()), PermissionProfile { file_system: Some(FileSystemPermissions { - entries: vec![root_write, deny_env_files], + entries: vec![ + root_write, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: AbsolutePathBuf::resolve_path_against_base( + "**/*.env", + cwd.as_path() + ) + .to_string_lossy() + .into_owned(), + }, + access: FileSystemAccessMode::None, + }, + ], glob_scan_max_depth: std::num::NonZeroUsize::new(4), }), ..Default::default() @@ -609,7 +727,7 @@ fn intersect_permission_profiles_uses_granted_unbounded_glob_scan_depth() { }; let granted = PermissionProfile { file_system: Some(FileSystemPermissions { - entries: vec![root_write.clone(), deny_env_files.clone()], + entries: vec![root_write.clone(), deny_env_files], glob_scan_max_depth: None, }), ..Default::default() @@ -619,7 +737,20 @@ fn intersect_permission_profiles_uses_granted_unbounded_glob_scan_depth() { intersect_permission_profiles(requested, granted, cwd.as_path()), PermissionProfile { file_system: Some(FileSystemPermissions { - entries: vec![root_write, deny_env_files], + entries: vec![ + root_write, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: AbsolutePathBuf::resolve_path_against_base( + "**/*.env", + cwd.as_path() + ) + .to_string_lossy() + .into_owned(), + }, + access: FileSystemAccessMode::None, + }, + ], glob_scan_max_depth: None, }), ..Default::default()