From a12c4f4c01f0fbbcd7060021fbf77327dc8be7cb Mon Sep 17 00:00:00 2001 From: zm-oai Date: Mon, 24 Aug 2026 17:22:38 +0000 Subject: [PATCH] Preserve Windows root reads with deny-read rules (#40441) ## What changed - Detect readable symbolic `:root` entries separately from effective full-disk access so narrower deny-read rules do not disable the Windows sandbox's broad-read setup. - Add explicit readable roots to that setup while omitting the filesystem root itself, and prevent read roots at or below denied paths from being granted access. ## Testing - Extend the elevated Windows sandbox integration test to verify that an allowed bundled skill under the user profile remains readable while exact and glob-based secrets stay denied. GitOrigin-RevId: 8f6f267b78718b9e8609a9205c489f253f478491 --- codex-rs/core/tests/suite/windows_sandbox.rs | 28 +++++++++++++------ .../src/resolved_permissions.rs | 11 ++++++++ codex-rs/windows-sandbox-rs/src/setup.rs | 28 +++++++++++++++++-- 3 files changed, 56 insertions(+), 11 deletions(-) diff --git a/codex-rs/core/tests/suite/windows_sandbox.rs b/codex-rs/core/tests/suite/windows_sandbox.rs index fcf27c9fd1..cd22ea91c6 100644 --- a/codex-rs/core/tests/suite/windows_sandbox.rs +++ b/codex-rs/core/tests/suite/windows_sandbox.rs @@ -273,12 +273,18 @@ async fn windows_elevated_enforces_deny_read_and_protects_setup_marker() -> anyh let workspace = TempDir::new()?; let cwd = dunce::canonicalize(workspace.path())?.abs(); let glob_secret = cwd.join("secret.env"); - let exact_secret = cwd.join("exact-secret.txt"); let public = cwd.join("public.txt"); + let user_profile = TempDir::new_in(dirs::home_dir().context("resolve user profile")?)?; + let _user_profile_guard = EnvVarGuard::set("USERPROFILE", user_profile.path().as_os_str()); + let exact_secret = user_profile.path().join("exact-secret.txt"); + std::fs::write(&exact_secret, "exact secret\n")?; + let bundled_skill_dir = user_profile.path().join(".codex/plugins/cache"); + std::fs::create_dir_all(&bundled_skill_dir)?; + let bundled_skill = bundled_skill_dir.join("SKILL.md"); let setup_marker = codex_home.path().join(".sandbox").join("setup_marker.json"); std::fs::write(&glob_secret, "glob secret\n")?; - std::fs::write(&exact_secret, "exact secret\n")?; std::fs::write(&public, "public ok\n")?; + std::fs::write(&bundled_skill, "bundled skill ok\n")?; let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ FileSystemSandboxEntry { @@ -303,7 +309,7 @@ async fn windows_elevated_enforces_deny_read_and_protects_setup_marker() -> anyh missing_path_behavior: None, }, FileSystemSandboxEntry { - path: exact_secret.into(), + path: exact_secret.clone().abs().into(), access: FileSystemAccessMode::Deny, missing_path_behavior: None, }, @@ -323,16 +329,19 @@ async fn windows_elevated_enforces_deny_read_and_protects_setup_marker() -> anyh "cmd.exe".to_string(), "/D".to_string(), "/C".to_string(), - format!( - "(type secret.env 1>NUL 2>NUL && echo GLOB-READ || echo GLOB-DENIED) & (type exact-secret.txt 1>NUL 2>NUL && echo EXACT-READ || echo EXACT-DENIED) & (type \"{}\" 1>NUL 2>NUL && echo MARKER-READ-ALLOWED || echo MARKER-READ-DENIED) & (echo tampered > \"{}\" 2>NUL && echo MARKER-WRITE-ALLOWED || echo MARKER-WRITE-DENIED) & type public.txt", - setup_marker.display(), - setup_marker.display() - ), + "(type secret.env 1>NUL 2>NUL && echo GLOB-READ || echo GLOB-DENIED) & (type %SETUP_MARKER% 1>NUL 2>NUL && echo MARKER-READ-ALLOWED || echo MARKER-READ-DENIED) & (echo tampered > %SETUP_MARKER% 2>NUL && echo MARKER-WRITE-ALLOWED || echo MARKER-WRITE-DENIED) & type public.txt & type %BUNDLED_SKILL% & (type %EXACT_SECRET% 1>NUL 2>NUL && echo EXACT-READ || echo EXACT-DENIED)".to_string(), ], cwd: cwd.clone(), expiration: 10_000.into(), capture_policy: ExecCapturePolicy::ShellTool, - env: HashMap::new(), + env: [ + ("BUNDLED_SKILL", &bundled_skill), + ("EXACT_SECRET", &exact_secret), + ("SETUP_MARKER", &setup_marker), + ] + .into_iter() + .map(|(name, path)| (name.to_string(), format!("\"{}\"", path.display()))) + .collect(), network: None, network_environment_id: None, sandbox_permissions: SandboxPermissions::UseDefault, @@ -371,6 +380,7 @@ async fn windows_elevated_enforces_deny_read_and_protects_setup_marker() -> anyh stdout.text.contains("public ok"), "allowed reads should still work: {stdout:?}" ); + assert!(stdout.text.contains("bundled skill ok")); assert!( stdout.text.contains("MARKER-READ-DENIED"), "sandboxed command should not read setup readiness: {stdout:?}" diff --git a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs index 36e9342eef..0e28b9d3dc 100644 --- a/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs +++ b/codex-rs/windows-sandbox-rs/src/resolved_permissions.rs @@ -4,6 +4,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::Root; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashMap; @@ -106,6 +107,16 @@ impl ResolvedWindowsSandboxPermissions { self.file_system.has_full_disk_read_access() } + pub(crate) fn has_symbolic_root_read_access(&self, cwd: &Path) -> bool { + self.file_system.entries.iter().any(|entry| { + matches!(&entry.path, FileSystemPath::Special { value: Root }) + && entry.access.can_read() + }) && cwd + .ancestors() + .last() + .is_some_and(|root| self.file_system.can_read_path_with_cwd(root, cwd)) + } + pub(crate) fn include_platform_defaults(&self) -> bool { self.file_system.include_platform_defaults() } diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index 361a565a24..890755d01c 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -575,6 +575,12 @@ fn gather_full_read_roots_for_permissions( .into_iter() .map(|root| root.root), ); + roots.extend( + permissions + .readable_roots_for_cwd(command_cwd) + .into_iter() + .filter(|root| root.parent().is_some() || !command_cwd.starts_with(root)), + ); canonical_existing(&roots) } @@ -584,7 +590,7 @@ pub(crate) fn gather_read_roots( env_map: &HashMap, codex_home: &Path, ) -> Vec { - if permissions.has_full_disk_read_access() { + if permissions.has_symbolic_root_read_access(command_cwd) { return gather_full_read_roots_for_permissions( command_cwd, permissions, @@ -1174,7 +1180,25 @@ fn build_payload_roots( read_roots = filter_user_profile_root_exclusions(read_roots); read_roots = filter_ssh_config_dependency_roots(read_roots); let write_root_set: HashSet = write_roots.iter().cloned().collect(); - read_roots.retain(|root| !write_root_set.contains(root)); + let deny_read_keys: Vec = overrides + .deny_read_paths + .as_deref() + .unwrap_or_default() + .iter() + .map(|path| canonical_path_key(path)) + .collect(); + read_roots.retain(|root| { + if write_root_set.contains(root) { + return false; + } + if deny_read_keys.is_empty() { + return true; + } + let root_key = canonical_path_key(root); + !deny_read_keys + .iter() + .any(|denied| Path::new(&root_key).starts_with(denied)) + }); (read_roots, write_roots) }