From ae4840bf246ebfa390fdf68c0fd87c796bb313f4 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Wed, 27 May 2026 17:47:57 -0700 Subject: [PATCH] refactor(permissions): lower Windows enforcement from effective filesystem permissions Co-authored-by: Codex noreply@openai.com --- codex-rs/Cargo.lock | 1 + codex-rs/windows-sandbox-rs/Cargo.toml | 1 + .../src/deny_read_resolver.rs | 38 ++++- codex-rs/windows-sandbox-rs/src/lib.rs | 1 + .../src/resolved_permissions.rs | 161 +++++++++++------- 5 files changed, 135 insertions(+), 67 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 1b1bc0e59a..622dd2a587 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4148,6 +4148,7 @@ dependencies = [ "chrono", "codex-otel", "codex-protocol", + "codex-sandboxing", "codex-utils-absolute-path", "codex-utils-pty", "codex-utils-string", diff --git a/codex-rs/windows-sandbox-rs/Cargo.toml b/codex-rs/windows-sandbox-rs/Cargo.toml index 53c81f0b03..bc63e74c5d 100644 --- a/codex-rs/windows-sandbox-rs/Cargo.toml +++ b/codex-rs/windows-sandbox-rs/Cargo.toml @@ -31,6 +31,7 @@ codex-utils-pty = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-string = { workspace = true } codex-otel = { workspace = true } +codex-sandboxing = { workspace = true } dunce = "1.0" glob = { workspace = true } serde = { version = "1.0", features = ["derive"] } diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs index ce07df1726..3952aa1454 100644 --- a/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs @@ -1,8 +1,12 @@ +use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::NetworkSandboxPolicy; use codex_protocol::permissions::ReadDenyMatcher; +use codex_sandboxing::EffectiveFilesystemPermissions; +use codex_sandboxing::FilesystemPermissionsContext; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashSet; use std::path::Path; @@ -23,15 +27,41 @@ struct GlobScanPlan { pub fn resolve_windows_deny_read_paths( file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &AbsolutePathBuf, +) -> Result, String> { + let file_system_sandbox_policy = file_system_sandbox_policy + .clone() + .materialize_project_roots_with_workspace_roots(std::slice::from_ref(cwd)); + let permission_profile = PermissionProfile::from_runtime_permissions( + &file_system_sandbox_policy, + NetworkSandboxPolicy::Restricted, + ); + let effective_file_system = EffectiveFilesystemPermissions::from_profile( + &permission_profile, + FilesystemPermissionsContext { + policy_evaluation_cwd: cwd, + }, + ) + .map_err(|err| err.to_string())?; + resolve_windows_deny_read_paths_from_effective_permissions(&effective_file_system, cwd) +} + +/// Resolves effective read-deny entries into concrete Windows ACL targets. +pub fn resolve_windows_deny_read_paths_from_effective_permissions( + effective_file_system: &EffectiveFilesystemPermissions, + cwd: &AbsolutePathBuf, ) -> Result, String> { let mut paths = Vec::new(); let mut seen = HashSet::new(); - for path in file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd.as_path()) { - push_absolute_path(&mut paths, &mut seen, path.into_path_buf())?; + for path in &effective_file_system.unreadable_roots { + push_absolute_path(&mut paths, &mut seen, path.to_path_buf())?; } - let unreadable_globs = file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd.as_path()); + let unreadable_globs = effective_file_system + .unreadable_globs + .iter() + .map(|glob| glob.pattern().to_string()) + .collect::>(); if unreadable_globs.is_empty() { return Ok(paths); } @@ -53,7 +83,7 @@ pub fn resolve_windows_deny_read_paths( for pattern in unreadable_globs { let mut seen_scan_dirs = HashSet::new(); - let scan_plan = glob_scan_plan(&pattern, file_system_sandbox_policy.glob_scan_max_depth); + let scan_plan = glob_scan_plan(&pattern, effective_file_system.glob_scan_max_depth); collect_existing_glob_matches( &scan_plan.root, &matcher, diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 26e22dde07..4dd4fd4f6a 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -124,6 +124,7 @@ pub use deny_read_acl::apply_deny_read_acls; #[cfg(target_os = "windows")] pub use deny_read_acl::plan_deny_read_acl_paths; pub use deny_read_resolver::resolve_windows_deny_read_paths; +pub use deny_read_resolver::resolve_windows_deny_read_paths_from_effective_permissions; #[cfg(target_os = "windows")] pub use deny_read_state::sync_persistent_deny_read_acls; #[cfg(target_os = "windows")] diff --git a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs index 75c4c216a3..cbf2d7bbb6 100644 --- a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs +++ b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs @@ -1,10 +1,14 @@ +use anyhow::Context; use anyhow::Result; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxKind; -use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::WritableRoot; +use codex_sandboxing::EffectiveFilesystemPermissions; +use codex_sandboxing::FilesystemPermissionsContext; +use codex_sandboxing::FilesystemPermissionsMode; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; use std::path::Path; @@ -15,9 +19,11 @@ use std::path::PathBuf; /// Most Windows sandbox code needs resolved runtime permissions plus a few /// Windows-specific path conventions, not the user/config-facing /// `PermissionProfile` enum itself. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug)] pub struct ResolvedWindowsSandboxPermissions { - file_system: FileSystemSandboxPolicy, + effective_file_system: EffectiveFilesystemPermissions, + writable_roots: Vec, + has_writable_tmpdir_entry: bool, network: NetworkSandboxPolicy, } @@ -44,7 +50,10 @@ pub fn token_mode_for_permission_profile( permission_profile, cwd, )?; - if permissions.file_system.has_full_disk_write_access() { + if permissions + .effective_file_system + .has_full_disk_write_access() + { anyhow::bail!( "permission profile requests full-disk filesystem writes, which cannot be enforced by the Windows sandbox" ); @@ -57,37 +66,81 @@ pub fn token_mode_for_permission_profile( } impl ResolvedWindowsSandboxPermissions { - pub fn try_from_permission_profile(permission_profile: &PermissionProfile) -> Result { + /// Resolves a managed permission profile for the Windows compatibility boundary. + /// + /// Normal runtime callers provide already-materialized workspace roots. For + /// callers that still provide symbolic `:workspace_roots`, this adapter + /// explicitly binds them to the permission root supplied by the caller. + pub fn try_from_permission_profile_for_cwd( + permission_profile: &PermissionProfile, + cwd: &Path, + ) -> Result { if !matches!(permission_profile, PermissionProfile::Managed { .. }) { anyhow::bail!( "only managed permission profiles can be enforced by the Windows sandbox" ); } + let permission_profile_cwd = AbsolutePathBuf::from_absolute_path(cwd) + .context("permission profile cwd must be absolute for the Windows sandbox")?; + let permission_profile = permission_profile + .clone() + .materialize_project_roots_with_workspace_roots(std::slice::from_ref( + &permission_profile_cwd, + )); let (file_system, network) = permission_profile.to_runtime_permissions(); if !matches!(file_system.kind, FileSystemSandboxKind::Restricted) { anyhow::bail!( "only restricted managed filesystem permissions can be enforced by the Windows sandbox" ); } + let effective_file_system = EffectiveFilesystemPermissions::from_profile( + &permission_profile, + FilesystemPermissionsContext { + policy_evaluation_cwd: &permission_profile_cwd, + }, + )?; + let has_writable_tmpdir_entry = + file_system + .entries + .iter() + .any(|FileSystemSandboxEntry { path, access }| { + matches!( + path, + FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Tmpdir, + } + ) && access.can_write() + }); + let mut windows_writable_file_system = file_system; + windows_writable_file_system + .entries + .retain(|FileSystemSandboxEntry { path, .. }| { + !matches!( + path, + FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Tmpdir + | codex_protocol::permissions::FileSystemSpecialPath::SlashTmp, + } + ) + }); + let windows_writable_profile = + PermissionProfile::from_runtime_permissions(&windows_writable_file_system, network); + let writable_roots = EffectiveFilesystemPermissions::from_profile( + &windows_writable_profile, + FilesystemPermissionsContext { + policy_evaluation_cwd: &permission_profile_cwd, + }, + )? + .writable_roots; + Ok(Self { - file_system, + effective_file_system, + writable_roots, + has_writable_tmpdir_entry, network, }) } - /// Resolves a managed permission profile and binds symbolic `:workspace_roots` - /// entries to the permission root supplied by the caller. - pub fn try_from_permission_profile_for_cwd( - permission_profile: &PermissionProfile, - cwd: &Path, - ) -> Result { - let mut permissions = Self::try_from_permission_profile(permission_profile)?; - permissions.file_system = permissions - .file_system - .materialize_project_roots_with_cwd(cwd); - Ok(permissions) - } - pub(crate) fn should_apply_network_block(&self) -> bool { !self.network.is_enabled() } @@ -97,21 +150,25 @@ impl ResolvedWindowsSandboxPermissions { } pub(crate) fn is_enforceable_by_windows_sandbox(&self) -> bool { - matches!(self.file_system.kind, FileSystemSandboxKind::Restricted) + matches!( + self.effective_file_system.mode, + FilesystemPermissionsMode::Restricted + ) } pub(crate) fn has_full_disk_read_access(&self) -> bool { - self.file_system.has_full_disk_read_access() + self.effective_file_system.has_full_disk_read_access() } pub(crate) fn include_platform_defaults(&self) -> bool { - self.file_system.include_platform_defaults() + self.effective_file_system.include_platform_defaults } - pub(crate) fn readable_roots_for_cwd(&self, cwd: &Path) -> Vec { - self.file_system - .get_readable_roots_with_cwd(cwd) - .into_iter() + pub(crate) fn readable_roots_for_cwd(&self, _cwd: &Path) -> Vec { + self.effective_file_system + .readable_roots + .iter() + .cloned() .map(AbsolutePathBuf::into_path_buf) .collect() } @@ -126,25 +183,13 @@ impl ResolvedWindowsSandboxPermissions { pub(crate) fn writable_roots_for_cwd( &self, - cwd: &Path, + _cwd: &Path, env_map: &HashMap, ) -> Vec { - let mut file_system = self.file_system.clone(); - file_system - .entries - .retain(|FileSystemSandboxEntry { path, .. }| { - !matches!( - path, - FileSystemPath::Special { - value: codex_protocol::permissions::FileSystemSpecialPath::Tmpdir - | codex_protocol::permissions::FileSystemSpecialPath::SlashTmp, - } - ) - }); - - let mut roots = file_system - .get_writable_roots_with_cwd(cwd) - .into_iter() + let mut roots = self + .writable_roots + .iter() + .cloned() .map(|root| WindowsWritableRoot { root: root.root.into_path_buf(), read_only_subpaths: root @@ -155,7 +200,7 @@ impl ResolvedWindowsSandboxPermissions { }) .collect::>(); - if self.has_writable_tmpdir_entry() { + if self.has_writable_tmpdir_entry { roots.extend(windows_temp_env_roots(env_map).into_iter().map(|root| { WindowsWritableRoot { root, @@ -166,20 +211,6 @@ impl ResolvedWindowsSandboxPermissions { roots } - - fn has_writable_tmpdir_entry(&self) -> bool { - self.file_system - .entries - .iter() - .any(|FileSystemSandboxEntry { path, access }| { - matches!( - path, - FileSystemPath::Special { - value: codex_protocol::permissions::FileSystemSpecialPath::Tmpdir, - } - ) && access.can_write() - }) - } } fn windows_temp_env_roots(env_map: &HashMap) -> Vec { @@ -217,8 +248,9 @@ mod tests { env_map.insert("TEMP".to_string(), temp_dir.to_string_lossy().to_string()); env_map.insert("TMP".to_string(), temp_dir.to_string_lossy().to_string()); - let permissions = ResolvedWindowsSandboxPermissions::try_from_permission_profile( + let permissions = ResolvedWindowsSandboxPermissions::try_from_permission_profile_for_cwd( &PermissionProfile::workspace_write(), + &cwd, ) .expect("managed permission profile"); let roots = permissions @@ -308,8 +340,9 @@ mod tests { #[test] fn permission_profile_rejects_disabled_profiles() { - let err = ResolvedWindowsSandboxPermissions::try_from_permission_profile( + let err = ResolvedWindowsSandboxPermissions::try_from_permission_profile_for_cwd( &PermissionProfile::Disabled, + Path::new("/"), ) .expect_err("disabled profile should not resolve for sandbox enforcement"); @@ -326,9 +359,11 @@ mod tests { network: NetworkSandboxPolicy::Restricted, }; - let err = - ResolvedWindowsSandboxPermissions::try_from_permission_profile(&permission_profile) - .expect_err("unrestricted profile should not resolve for sandbox enforcement"); + let err = ResolvedWindowsSandboxPermissions::try_from_permission_profile_for_cwd( + &permission_profile, + Path::new("/"), + ) + .expect_err("unrestricted profile should not resolve for sandbox enforcement"); assert!( err.to_string()