diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index f241c4828c..8a996f4ad3 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -607,47 +607,23 @@ async fn exec_windows_sandbox( let proxy_enforced = network.is_some(); let use_elevated = windows_sandbox_uses_elevated_backend(sandbox_level, proxy_enforced); let additional_deny_write_paths = windows_sandbox_filesystem_overrides - .map(|overrides| { - overrides - .additional_deny_write_paths - .iter() - .map(AbsolutePathBuf::to_path_buf) - .collect::>() - }) - .unwrap_or_default(); + .map(|overrides| overrides.additional_deny_write_paths.clone()) + .unwrap_or_default() + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect::>(); let additional_deny_read_paths = windows_sandbox_filesystem_overrides - .map(|overrides| { - overrides - .additional_deny_read_paths - .iter() - .map(AbsolutePathBuf::to_path_buf) - .collect::>() - }) - .unwrap_or_default(); + .map(|overrides| overrides.additional_deny_read_paths.clone()) + .unwrap_or_default() + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect::>(); let elevated_read_roots_override = windows_sandbox_filesystem_overrides .and_then(|overrides| overrides.read_roots_override.clone()); let elevated_read_roots_include_platform_defaults = windows_sandbox_filesystem_overrides .is_some_and(|overrides| overrides.read_roots_include_platform_defaults); let elevated_write_roots_override = windows_sandbox_filesystem_overrides .and_then(|overrides| overrides.write_roots_override.clone()); - let elevated_deny_write_paths = windows_sandbox_filesystem_overrides - .map(|overrides| { - overrides - .additional_deny_write_paths - .iter() - .map(AbsolutePathBuf::to_path_buf) - .collect::>() - }) - .unwrap_or_default(); - let elevated_deny_read_paths = windows_sandbox_filesystem_overrides - .map(|overrides| { - overrides - .additional_deny_read_paths - .iter() - .map(AbsolutePathBuf::to_path_buf) - .collect::>() - }) - .unwrap_or_default(); let spawn_res = tokio::task::spawn_blocking(move || { if use_elevated { run_windows_sandbox_capture_elevated( @@ -665,8 +641,8 @@ async fn exec_windows_sandbox( read_roots_include_platform_defaults: elevated_read_roots_include_platform_defaults, write_roots_override: elevated_write_roots_override.as_deref(), - deny_read_paths_override: &elevated_deny_read_paths, - deny_write_paths_override: &elevated_deny_write_paths, + deny_read_paths_override: &additional_deny_read_paths, + deny_write_paths_override: &additional_deny_write_paths, }, ) } else { @@ -1082,7 +1058,7 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides( ); } - let additional_deny_read_paths = crate::windows_deny_read::resolve_windows_deny_read_paths( + let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths( file_system_sandbox_policy, sandbox_policy_cwd, )?; @@ -1204,7 +1180,7 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides( )); } - let additional_deny_read_paths = crate::windows_deny_read::resolve_windows_deny_read_paths( + let additional_deny_read_paths = codex_windows_sandbox::resolve_windows_deny_read_paths( file_system_sandbox_policy, sandbox_policy_cwd, )?; diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 5c442a53ee..0cdf0e2d46 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -111,7 +111,6 @@ pub mod review_format; pub mod review_prompts; mod thread_manager; pub(crate) mod web_search; -pub(crate) mod windows_deny_read; pub(crate) mod windows_sandbox_read_grants; pub use thread_manager::ForkSnapshot; pub use thread_manager::NewThread; diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs b/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs index 4459145b09..6dbbe27d93 100644 --- a/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs @@ -89,12 +89,24 @@ pub fn write_persistent_deny_read_acl_record( paths: &[PathBuf], ) -> Result<()> { let record_path = deny_read_acl_record_path(codex_home, kind); + let planned_paths = plan_deny_read_acl_paths(paths); + if planned_paths.is_empty() { + match std::fs::remove_file(&record_path) { + Ok(()) => return Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(err) => { + return Err(err).with_context(|| { + format!("remove deny-read ACL record {}", record_path.display()) + }); + } + } + } if let Some(parent) = record_path.parent() { std::fs::create_dir_all(parent) .with_context(|| format!("create deny-read ACL record dir {}", parent.display()))?; } let record = DenyReadAclRecord { - paths: plan_deny_read_acl_paths(paths), + paths: planned_paths, }; let contents = serde_json::to_vec_pretty(&record) .with_context(|| format!("serialize deny-read ACL record {}", record_path.display()))?; diff --git a/codex-rs/core/src/windows_deny_read.rs b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs similarity index 71% rename from codex-rs/core/src/windows_deny_read.rs rename to codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs index 80845348f1..ad24199f82 100644 --- a/codex-rs/core/src/windows_deny_read.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs @@ -8,6 +8,11 @@ use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; +struct GlobScanPlan { + root: PathBuf, + max_depth: Option, +} + /// Resolve split filesystem `None` read entries into concrete Windows ACL targets. /// /// Windows ACLs do not understand Codex filesystem glob patterns directly. Exact @@ -15,7 +20,7 @@ use std::path::PathBuf; /// exist yet. Glob entries are snapshot-expanded to the files/directories that /// already exist under their literal scan root; future exact paths are handled /// later by materializing them before the deny ACE is applied. -pub(crate) fn resolve_windows_deny_read_paths( +pub fn resolve_windows_deny_read_paths( file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &AbsolutePathBuf, ) -> Result, String> { @@ -48,12 +53,15 @@ pub(crate) fn resolve_windows_deny_read_paths( let mut seen_scan_dirs = HashSet::new(); for pattern in unreadable_globs { + let scan_plan = glob_scan_plan(&pattern); collect_existing_glob_matches( - &glob_scan_root(&pattern), + &scan_plan.root, &matcher, &mut paths, &mut seen, &mut seen_scan_dirs, + scan_plan.max_depth, + 0, )?; } @@ -66,6 +74,8 @@ fn collect_existing_glob_matches( paths: &mut Vec, seen_paths: &mut HashSet, seen_scan_dirs: &mut HashSet, + max_depth: Option, + depth: usize, ) -> Result<(), String> { if !path.exists() { return Ok(()); @@ -90,11 +100,23 @@ fn collect_existing_glob_matches( return Ok(()); } + if max_depth.is_some_and(|max_depth| depth >= max_depth) { + return Ok(()); + } + let Ok(entries) = std::fs::read_dir(path) else { return Ok(()); }; for entry in entries.flatten() { - collect_existing_glob_matches(&entry.path(), matcher, paths, seen_paths, seen_scan_dirs)?; + collect_existing_glob_matches( + &entry.path(), + matcher, + paths, + seen_paths, + seen_scan_dirs, + max_depth, + depth + 1, + )?; } Ok(()) @@ -113,7 +135,7 @@ fn push_absolute_path( Ok(()) } -fn glob_scan_root(pattern: &str) -> PathBuf { +fn glob_scan_plan(pattern: &str) -> GlobScanPlan { // Start scanning at the deepest literal directory prefix before the first // glob metacharacter. For example, `C:\repo\**\*.env` only scans `C:\repo` // instead of the current directory or drive root. @@ -124,22 +146,43 @@ fn glob_scan_root(pattern: &str) -> PathBuf { .unwrap_or(pattern.len()); let literal_prefix = &pattern[..first_glob]; let Some(separator_index) = literal_prefix.rfind(['/', '\\']) else { - return PathBuf::from("."); + return GlobScanPlan { + root: PathBuf::from("."), + max_depth: glob_scan_max_depth(pattern), + }; }; + let pattern_suffix = &pattern[separator_index + 1..]; let is_drive_root_separator = separator_index > 0 && literal_prefix .as_bytes() .get(separator_index - 1) .is_some_and(|ch| *ch == b':'); if separator_index == 0 || is_drive_root_separator { - return PathBuf::from(&literal_prefix[..=separator_index]); + return GlobScanPlan { + root: PathBuf::from(&literal_prefix[..=separator_index]), + max_depth: glob_scan_max_depth(pattern_suffix), + }; } - PathBuf::from(literal_prefix[..separator_index].to_string()) + GlobScanPlan { + root: PathBuf::from(literal_prefix[..separator_index].to_string()), + max_depth: glob_scan_max_depth(pattern_suffix), + } +} + +fn glob_scan_max_depth(pattern_suffix: &str) -> Option { + let components = pattern_suffix + .split(['/', '\\']) + .filter(|component| !component.is_empty()) + .collect::>(); + if components.contains(&"**") { + return None; + } + Some(components.len()) } #[cfg(test)] mod tests { - use super::glob_scan_root; + use super::glob_scan_plan; use super::resolve_windows_deny_read_paths; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; @@ -170,14 +213,21 @@ mod tests { #[test] fn scan_root_uses_literal_prefix_before_glob() { assert_eq!( - glob_scan_root("/tmp/work/**/*.env"), + glob_scan_plan("/tmp/work/**/*.env").root, PathBuf::from("/tmp/work") ); assert_eq!( - glob_scan_root(r"C:\Users\dev\repo\**\*.env"), + glob_scan_plan(r"C:\Users\dev\repo\**\*.env").root, PathBuf::from(r"C:\Users\dev\repo") ); - assert_eq!(glob_scan_root(r"C:\*.env"), PathBuf::from(r"C:\")); + assert_eq!(glob_scan_plan(r"C:\*.env").root, PathBuf::from(r"C:\")); + } + + #[test] + fn scan_depth_is_bounded_for_non_recursive_globs() { + assert_eq!(glob_scan_plan("/tmp/work/*.env").max_depth, Some(1)); + assert_eq!(glob_scan_plan("/tmp/work/*/*.env").max_depth, Some(2)); + assert_eq!(glob_scan_plan("/tmp/work/**/*.env").max_depth, None); } #[test] @@ -225,4 +275,24 @@ mod tests { assert_eq!(actual, expected); } + + #[test] + fn non_recursive_globs_do_not_expand_nested_matches() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let root_env = tmp.path().join(".env"); + let nested_env = tmp.path().join("app").join(".env"); + std::fs::create_dir_all(nested_env.parent().expect("parent")).expect("create parent"); + std::fs::write(&root_env, "secret").expect("write root env"); + std::fs::write(&nested_env, "secret").expect("write nested env"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(format!( + "{}/*.env", + tmp.path().display() + ))]); + + assert_eq!( + resolve_windows_deny_read_paths(&policy, &cwd).expect("resolve"), + vec![AbsolutePathBuf::from_absolute_path(root_env).expect("absolute root env")] + ); + } } diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index ed9f44aafb..887b29426f 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -34,6 +34,8 @@ windows_modules!( workspace_acl ); +mod deny_read_resolver; + #[cfg(target_os = "windows")] #[path = "conpty/mod.rs"] mod conpty; @@ -115,6 +117,7 @@ pub use deny_read_acl::cleanup_stale_persistent_deny_read_acls; pub use deny_read_acl::plan_deny_read_acl_paths; #[cfg(target_os = "windows")] pub use deny_read_acl::write_persistent_deny_read_acl_record; +pub use deny_read_resolver::resolve_windows_deny_read_paths; #[cfg(target_os = "windows")] pub use dpapi::protect as dpapi_protect; #[cfg(target_os = "windows")] diff --git a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs index 0840467e6b..c5bea18012 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -469,17 +469,6 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> { let sandbox_group_sid = resolve_sandbox_users_group_sid()?; let sandbox_group_psid = sid_bytes_to_psid(&sandbox_group_sid)?; let mut refresh_errors: Vec = Vec::new(); - let users_sid = resolve_sid("Users")?; - let users_psid = sid_bytes_to_psid(&users_sid)?; - let auth_sid = resolve_sid("Authenticated Users")?; - let auth_psid = sid_bytes_to_psid(&auth_sid)?; - let everyone_sid = resolve_sid("Everyone")?; - let everyone_psid = sid_bytes_to_psid(&everyone_sid)?; - let rx_psids = vec![users_psid, auth_psid, everyone_psid]; - let subjects = ReadAclSubjects { - sandbox_group_psid, - rx_psids: &rx_psids, - }; // Stale cleanup must happen before the helper re-grants read ACLs because // the cleanup primitive revokes all ACEs for the sandbox group SID. match unsafe { @@ -503,15 +492,39 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> { log_line(log, &format!("cleanup stale deny-read ACLs failed: {err}"))?; } } - apply_read_acls( - &payload.read_roots, - &subjects, - log, - &mut refresh_errors, - FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, - "read", - OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, - )?; + if !payload.read_roots.is_empty() { + let users_sid = resolve_sid("Users")?; + let users_psid = sid_bytes_to_psid(&users_sid)?; + let auth_sid = resolve_sid("Authenticated Users")?; + let auth_psid = sid_bytes_to_psid(&auth_sid)?; + let everyone_sid = resolve_sid("Everyone")?; + let everyone_psid = sid_bytes_to_psid(&everyone_sid)?; + let rx_psids = vec![users_psid, auth_psid, everyone_psid]; + let subjects = ReadAclSubjects { + sandbox_group_psid, + rx_psids: &rx_psids, + }; + apply_read_acls( + &payload.read_roots, + &subjects, + log, + &mut refresh_errors, + FILE_GENERIC_READ | FILE_GENERIC_EXECUTE, + "read", + OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, + )?; + unsafe { + if !users_psid.is_null() { + LocalFree(users_psid as HLOCAL); + } + if !auth_psid.is_null() { + LocalFree(auth_psid as HLOCAL); + } + if !everyone_psid.is_null() { + LocalFree(everyone_psid as HLOCAL); + } + } + } // Deny-read ACEs are applied after read grants so the DACL ends with // explicit deny entries that take precedence over the broad read allowlist. match unsafe { apply_deny_read_acls(&payload.deny_read_paths, sandbox_group_psid) } { @@ -534,15 +547,6 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> { if !sandbox_group_psid.is_null() { LocalFree(sandbox_group_psid as HLOCAL); } - if !users_psid.is_null() { - LocalFree(users_psid as HLOCAL); - } - if !auth_psid.is_null() { - LocalFree(auth_psid as HLOCAL); - } - if !everyone_psid.is_null() { - LocalFree(everyone_psid as HLOCAL); - } } if !refresh_errors.is_empty() { log_line( diff --git a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs index 63e92517a9..9e01430307 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs @@ -15,6 +15,7 @@ use crate::allow::compute_allow_paths; use crate::helper_materialization::helper_bin_dir; use crate::logging::log_note; use crate::path_normalization::canonical_path_key; +use crate::path_normalization::canonicalize_path; use crate::policy::SandboxPolicy; use crate::setup_error::SetupErrorCode; use crate::setup_error::SetupFailure; @@ -813,13 +814,7 @@ fn build_payload_deny_write_paths( let mut deny_write_paths: Vec = explicit_deny_write_paths .unwrap_or_default() .into_iter() - .map(|path| { - if path.exists() { - dunce::canonicalize(&path).unwrap_or(path) - } else { - path - } - }) + .map(|path| canonicalize_path(&path)) .collect(); deny_write_paths.extend(allow_deny_paths.deny); deny_write_paths @@ -832,13 +827,7 @@ fn build_payload_deny_read_paths(explicit_deny_read_paths: Option>) explicit_deny_read_paths .unwrap_or_default() .into_iter() - .map(|path| { - if path.exists() { - dunce::canonicalize(&path).unwrap_or(path) - } else { - path - } - }) + .map(|path| canonicalize_path(&path)) .collect() }