From 7bb835a174508f2820e816cd2fd64597c069835b Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Thu, 16 Apr 2026 14:06:28 -0700 Subject: [PATCH 1/2] feat(sandbox): add Windows deny-read parity Add Windows deny-read enforcement for split filesystem policies by resolving exact and glob unreadable entries into ACL targets, threading those paths through the restricted-token and elevated Windows sandbox backends, and applying deny-read ACE overlays with stale cleanup records. Exact missing paths are materialized before ACE application so sandboxed subprocesses cannot create and read them during the same run. Existing paths are planned with both lexical and canonical targets to cover reparse-point aliases. Co-authored-by: Codex --- codex-rs/core/src/config/mod.rs | 6 - codex-rs/core/src/exec.rs | 102 +++++--- codex-rs/core/src/exec_tests.rs | 104 ++++++-- codex-rs/core/src/lib.rs | 1 + codex-rs/core/src/windows_deny_read.rs | 228 ++++++++++++++++++ codex-rs/core/tests/suite/mod.rs | 2 + codex-rs/core/tests/suite/windows_sandbox.rs | 131 ++++++++++ codex-rs/windows-sandbox-rs/src/acl.rs | 129 +++++++++- .../windows-sandbox-rs/src/deny_read_acl.rs | 212 ++++++++++++++++ .../windows-sandbox-rs/src/elevated_impl.rs | 3 + codex-rs/windows-sandbox-rs/src/identity.rs | 3 + codex-rs/windows-sandbox-rs/src/lib.rs | 78 +++++- .../windows-sandbox-rs/src/setup_main_win.rs | 57 ++++- .../src/setup_orchestrator.rs | 43 ++++ 14 files changed, 1028 insertions(+), 71 deletions(-) create mode 100644 codex-rs/core/src/windows_deny_read.rs create mode 100644 codex-rs/core/tests/suite/windows_sandbox.rs create mode 100644 codex-rs/windows-sandbox-rs/src/deny_read_acl.rs diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 6320bff2e399..322d92b7a9b0 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -2023,12 +2023,6 @@ impl Config { } }) .map_err(std::io::Error::from)?; - - if cfg!(target_os = "windows") { - startup_warnings.push(format!( - "managed filesystem deny_read from {filesystem_requirements_source} is only enforced for direct file tools on Windows; shell subprocess reads are not sandboxed" - )); - } } apply_requirement_constrained_value( "approvals_reviewer", diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 396b7701bec3..d62353fe0656 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -96,15 +96,17 @@ pub struct ExecParams { /// Resolved filesystem overrides for the Windows sandbox backends. /// -/// The unelevated restricted-token backend only consumes extra deny-write -/// carveouts on top of the legacy `WorkspaceWrite` allow set. The elevated -/// backend can also consume explicit read and write roots during setup/refresh. -/// Read-root overrides are layered on top of the baseline helper/platform roots -/// that the elevated setup path needs to launch the sandboxed command. +/// Both Windows backends consume extra deny-read paths, and the unelevated +/// restricted-token backend also consumes extra deny-write carveouts on top of +/// the legacy `WorkspaceWrite` allow set. The elevated backend can also consume +/// explicit read and write roots during setup/refresh. Read-root overrides are +/// layered on top of the baseline helper/platform roots that the elevated setup +/// path needs to launch the sandboxed command. #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct WindowsSandboxFilesystemOverrides { pub(crate) read_roots_override: Option>, pub(crate) write_roots_override: Option>, + pub(crate) additional_deny_read_paths: Vec, pub(crate) additional_deny_write_paths: Vec, } @@ -490,7 +492,7 @@ async fn exec_windows_sandbox( ) -> Result { use crate::config::find_codex_home; use codex_windows_sandbox::run_windows_sandbox_capture_elevated; - use codex_windows_sandbox::run_windows_sandbox_capture_with_extra_deny_write_paths; + use codex_windows_sandbox::run_windows_sandbox_capture_with_filesystem_overrides; let ExecParams { command, @@ -539,6 +541,15 @@ async fn exec_windows_sandbox( .collect::>() }) .unwrap_or_default(); + 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(); let elevated_read_roots_override = windows_sandbox_filesystem_overrides .and_then(|overrides| overrides.read_roots_override.clone()); let elevated_write_roots_override = windows_sandbox_filesystem_overrides @@ -552,6 +563,15 @@ async fn exec_windows_sandbox( .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( @@ -567,11 +587,12 @@ async fn exec_windows_sandbox( proxy_enforced, read_roots_override: elevated_read_roots_override.as_deref(), 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, }, ) } else { - run_windows_sandbox_capture_with_extra_deny_write_paths( + run_windows_sandbox_capture_with_filesystem_overrides( policy_str.as_str(), &sandbox_cwd, codex_home.as_ref(), @@ -579,6 +600,7 @@ async fn exec_windows_sandbox( &cwd, env, timeout_ms, + &additional_deny_read_paths, &additional_deny_write_paths, windows_sandbox_private_desktop, ) @@ -971,25 +993,21 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides( )); } - if !file_system_sandbox_policy.has_full_disk_read_access() { + // The restricted-token backend can add deny ACL overlays, but it cannot + // synthesize a smaller read allowlist. A policy that leaves the filesystem + // root readable and only adds deny-read carveouts is therefore enforceable; + // a policy that removes root read access is not. + if !windows_policy_has_root_read_access(file_system_sandbox_policy, sandbox_policy_cwd) { return Err( "windows unelevated restricted-token sandbox cannot enforce split filesystem read restrictions directly; refusing to run unsandboxed" .to_string(), ); } - if !file_system_sandbox_policy - .get_unreadable_roots_with_cwd(sandbox_policy_cwd) - .is_empty() - || !file_system_sandbox_policy - .get_unreadable_globs_with_cwd(sandbox_policy_cwd) - .is_empty() - { - return Err( - "windows unelevated restricted-token sandbox cannot enforce unreadable split filesystem carveouts directly; refusing to run unsandboxed" - .to_string(), - ); - } + let additional_deny_read_paths = crate::windows_deny_read::resolve_windows_deny_read_paths( + file_system_sandbox_policy, + sandbox_policy_cwd, + )?; let legacy_writable_roots = sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd); let split_writable_roots = @@ -1053,13 +1071,14 @@ pub(crate) fn resolve_windows_restricted_token_filesystem_overrides( } } - if additional_deny_write_paths.is_empty() { + if additional_deny_read_paths.is_empty() && additional_deny_write_paths.is_empty() { return Ok(None); } Ok(Some(WindowsSandboxFilesystemOverrides { read_roots_override: None, write_roots_override: None, + additional_deny_read_paths, additional_deny_write_paths: additional_deny_write_paths .into_iter() .map(|path| AbsolutePathBuf::from_absolute_path(path).map_err(|err| err.to_string())) @@ -1073,6 +1092,16 @@ fn normalize_windows_override_path(path: &Path) -> std::result::Result bool { + let Some(root) = cwd.as_path().ancestors().last() else { + return false; + }; + file_system_sandbox_policy.can_read_path_with_cwd(root, cwd.as_path()) +} + pub(crate) fn resolve_windows_elevated_filesystem_overrides( sandbox: SandboxType, sandbox_policy: &SandboxPolicy, @@ -1096,18 +1125,10 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides( )); } - if !file_system_sandbox_policy - .get_unreadable_roots_with_cwd(sandbox_policy_cwd) - .is_empty() - || !file_system_sandbox_policy - .get_unreadable_globs_with_cwd(sandbox_policy_cwd) - .is_empty() - { - return Err( - "windows elevated sandbox cannot enforce unreadable split filesystem carveouts directly; refusing to run unsandboxed" - .to_string(), - ); - } + let additional_deny_read_paths = crate::windows_deny_read::resolve_windows_deny_read_paths( + file_system_sandbox_policy, + sandbox_policy_cwd, + )?; let split_writable_roots = file_system_sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd); @@ -1145,11 +1166,16 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides( .collect(); let split_root_path_set: BTreeSet = split_root_paths.iter().cloned().collect(); - let matches_legacy_read_access = file_system_sandbox_policy.has_full_disk_read_access() - == sandbox_policy.has_full_disk_read_access(); + // `has_full_disk_read_access()` is intentionally false when deny-read + // entries exist. For Windows setup overrides, the important question is + // whether the baseline still reads from the filesystem root and only needs + // additional deny ACLs layered on top. + let split_has_root_read_access = + windows_policy_has_root_read_access(file_system_sandbox_policy, sandbox_policy_cwd); + let matches_legacy_read_access = + split_has_root_read_access == sandbox_policy.has_full_disk_read_access(); let read_roots_override = if matches_legacy_read_access - && (file_system_sandbox_policy.has_full_disk_read_access() - || split_readable_root_set == legacy_readable_root_set) + && (split_has_root_read_access || split_readable_root_set == legacy_readable_root_set) { None } else { @@ -1198,6 +1224,7 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides( if read_roots_override.is_none() && write_roots_override.is_none() + && additional_deny_read_paths.is_empty() && additional_deny_write_paths.is_empty() { return Ok(None); @@ -1206,6 +1233,7 @@ pub(crate) fn resolve_windows_elevated_filesystem_overrides( Ok(Some(WindowsSandboxFilesystemOverrides { read_roots_override, write_roots_override, + additional_deny_read_paths, additional_deny_write_paths, })) } diff --git a/codex-rs/core/src/exec_tests.rs b/codex-rs/core/src/exec_tests.rs index 1cfa87ff3f6d..b5c0df621dd5 100644 --- a/codex-rs/core/src/exec_tests.rs +++ b/codex-rs/core/src/exec_tests.rs @@ -659,11 +659,66 @@ fn windows_restricted_token_supports_full_read_split_write_read_carveouts() { Ok(Some(WindowsSandboxFilesystemOverrides { read_roots_override: None, write_roots_override: None, + additional_deny_read_paths: vec![], additional_deny_write_paths: expected_deny_write_paths, })) ); } +#[test] +fn windows_restricted_token_supports_unreadable_split_carveouts() { + let temp_dir = tempfile::TempDir::new().expect("tempdir"); + let cwd = dunce::canonicalize(temp_dir.path()) + .expect("canonicalize temp dir") + .abs(); + let blocked = cwd.join("blocked"); + std::fs::create_dir_all(blocked.as_path()).expect("create blocked"); + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + read_only_access: codex_protocol::protocol::ReadOnlyAccess::FullAccess, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let file_system_policy = FileSystemSandboxPolicy::restricted(vec![ + codex_protocol::permissions::FileSystemSandboxEntry { + path: codex_protocol::permissions::FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Root, + }, + access: codex_protocol::permissions::FileSystemAccessMode::Read, + }, + codex_protocol::permissions::FileSystemSandboxEntry { + path: codex_protocol::permissions::FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: codex_protocol::permissions::FileSystemAccessMode::Write, + }, + codex_protocol::permissions::FileSystemSandboxEntry { + path: codex_protocol::permissions::FileSystemPath::Path { + path: blocked.clone(), + }, + access: codex_protocol::permissions::FileSystemAccessMode::None, + }, + ]); + + assert_eq!( + resolve_windows_restricted_token_filesystem_overrides( + SandboxType::WindowsRestrictedToken, + &policy, + &file_system_policy, + NetworkSandboxPolicy::Restricted, + &cwd, + WindowsSandboxLevel::RestrictedToken, + ), + Ok(Some(WindowsSandboxFilesystemOverrides { + read_roots_override: None, + write_roots_override: None, + additional_deny_read_paths: vec![blocked.clone()], + additional_deny_write_paths: vec![blocked], + })) + ); +} + #[test] fn windows_elevated_supports_split_restricted_read_roots() { let temp_dir = tempfile::TempDir::new().expect("tempdir"); @@ -696,6 +751,7 @@ fn windows_elevated_supports_split_restricted_read_roots() { Ok(Some(WindowsSandboxFilesystemOverrides { read_roots_override: Some(vec![expected_docs]), write_roots_override: None, + additional_deny_read_paths: vec![], additional_deny_write_paths: vec![], })) ); @@ -748,6 +804,7 @@ fn windows_elevated_supports_split_write_read_carveouts() { Ok(Some(WindowsSandboxFilesystemOverrides { read_roots_override: None, write_roots_override: None, + additional_deny_read_paths: vec![], additional_deny_write_paths: vec![ codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(expected_docs) .expect("absolute docs"), @@ -757,10 +814,11 @@ fn windows_elevated_supports_split_write_read_carveouts() { } #[test] -fn windows_elevated_rejects_unreadable_split_carveouts() { +fn windows_elevated_supports_unreadable_split_carveouts() { let temp_dir = tempfile::TempDir::new().expect("tempdir"); let blocked = temp_dir.path().join("blocked"); std::fs::create_dir_all(&blocked).expect("create blocked"); + let expected_blocked = dunce::canonicalize(&blocked).expect("canonical blocked"); let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: codex_protocol::protocol::ReadOnlyAccess::FullAccess, @@ -791,24 +849,37 @@ fn windows_elevated_rejects_unreadable_split_carveouts() { ]); assert_eq!( - unsupported_windows_restricted_token_sandbox_reason( + resolve_windows_elevated_filesystem_overrides( SandboxType::WindowsRestrictedToken, &policy, &file_system_policy, NetworkSandboxPolicy::Restricted, &temp_dir.path().abs(), - WindowsSandboxLevel::Elevated, + /*use_windows_elevated_backend*/ true, ), - Some( - "windows elevated sandbox cannot enforce unreadable split filesystem carveouts directly; refusing to run unsandboxed" - .to_string() - ) + Ok(Some(WindowsSandboxFilesystemOverrides { + read_roots_override: None, + write_roots_override: None, + additional_deny_read_paths: vec![ + codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path( + expected_blocked.clone(), + ) + .expect("absolute blocked"), + ], + additional_deny_write_paths: vec![ + codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(expected_blocked) + .expect("absolute blocked"), + ], + })) ); } #[test] -fn windows_elevated_rejects_unreadable_globs() { +fn windows_elevated_supports_unreadable_globs() { let temp_dir = tempfile::TempDir::new().expect("tempdir"); + let secret = temp_dir.path().join("app").join(".env"); + std::fs::create_dir_all(secret.parent().expect("parent")).expect("create parent"); + std::fs::write(&secret, "secret").expect("write secret"); let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], read_only_access: codex_protocol::protocol::ReadOnlyAccess::FullAccess, @@ -838,18 +909,23 @@ fn windows_elevated_rejects_unreadable_globs() { ]); assert_eq!( - unsupported_windows_restricted_token_sandbox_reason( + resolve_windows_elevated_filesystem_overrides( SandboxType::WindowsRestrictedToken, &policy, &file_system_policy, NetworkSandboxPolicy::Restricted, &temp_dir.path().abs(), - WindowsSandboxLevel::Elevated, + /*use_windows_elevated_backend*/ true, ), - Some( - "windows elevated sandbox cannot enforce unreadable split filesystem carveouts directly; refusing to run unsandboxed" - .to_string() - ) + Ok(Some(WindowsSandboxFilesystemOverrides { + read_roots_override: None, + write_roots_override: None, + additional_deny_read_paths: vec![ + codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path(secret) + .expect("absolute secret"), + ], + additional_deny_write_paths: vec![], + })) ); } diff --git a/codex-rs/core/src/lib.rs b/codex-rs/core/src/lib.rs index 18d1a3029280..0b5e35279269 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -117,6 +117,7 @@ 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/core/src/windows_deny_read.rs b/codex-rs/core/src/windows_deny_read.rs new file mode 100644 index 000000000000..80845348f13f --- /dev/null +++ b/codex-rs/core/src/windows_deny_read.rs @@ -0,0 +1,228 @@ +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::ReadDenyMatcher; +use codex_utils_absolute_path::AbsolutePathBuf; +use std::collections::HashSet; +use std::path::Path; +use std::path::PathBuf; + +/// Resolve split filesystem `None` read entries into concrete Windows ACL targets. +/// +/// Windows ACLs do not understand Codex filesystem glob patterns directly. Exact +/// unreadable roots can be passed through as-is, including paths that do not +/// 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( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + 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())?; + } + + let unreadable_globs = file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd.as_path()); + if unreadable_globs.is_empty() { + return Ok(paths); + } + + let glob_policy = FileSystemSandboxPolicy::restricted( + unreadable_globs + .iter() + .map(|pattern| FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: pattern.clone(), + }, + access: FileSystemAccessMode::None, + }) + .collect(), + ); + let Some(matcher) = ReadDenyMatcher::new(&glob_policy, cwd.as_path()) else { + return Ok(paths); + }; + + let mut seen_scan_dirs = HashSet::new(); + for pattern in unreadable_globs { + collect_existing_glob_matches( + &glob_scan_root(&pattern), + &matcher, + &mut paths, + &mut seen, + &mut seen_scan_dirs, + )?; + } + + Ok(paths) +} + +fn collect_existing_glob_matches( + path: &Path, + matcher: &ReadDenyMatcher, + paths: &mut Vec, + seen_paths: &mut HashSet, + seen_scan_dirs: &mut HashSet, +) -> Result<(), String> { + if !path.exists() { + return Ok(()); + } + + if matcher.is_read_denied(path) { + push_absolute_path(paths, seen_paths, path.to_path_buf())?; + } + + let Ok(metadata) = path.metadata() else { + return Ok(()); + }; + if !metadata.is_dir() { + return Ok(()); + } + + // Canonical directory keys keep recursive scans from following a symlink or + // junction cycle forever while preserving the original matched path for the + // ACL layer. + let scan_key = dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); + if !seen_scan_dirs.insert(scan_key) { + 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)?; + } + + Ok(()) +} + +fn push_absolute_path( + paths: &mut Vec, + seen: &mut HashSet, + path: PathBuf, +) -> Result<(), String> { + let absolute_path = AbsolutePathBuf::from_absolute_path(dunce::simplified(&path)) + .map_err(|err| err.to_string())?; + if seen.insert(absolute_path.to_path_buf()) { + paths.push(absolute_path); + } + Ok(()) +} + +fn glob_scan_root(pattern: &str) -> PathBuf { + // 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. + let first_glob = pattern + .char_indices() + .find(|(_, ch)| matches!(ch, '*' | '?' | '[')) + .map(|(index, _)| index) + .unwrap_or(pattern.len()); + let literal_prefix = &pattern[..first_glob]; + let Some(separator_index) = literal_prefix.rfind(['/', '\\']) else { + return PathBuf::from("."); + }; + 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]); + } + PathBuf::from(literal_prefix[..separator_index].to_string()) +} + +#[cfg(test)] +mod tests { + use super::glob_scan_root; + use super::resolve_windows_deny_read_paths; + use codex_protocol::permissions::FileSystemAccessMode; + use codex_protocol::permissions::FileSystemPath; + use codex_protocol::permissions::FileSystemSandboxEntry; + use codex_protocol::permissions::FileSystemSandboxPolicy; + use codex_utils_absolute_path::AbsolutePathBuf; + use pretty_assertions::assert_eq; + use std::collections::HashSet; + use std::path::PathBuf; + use tempfile::TempDir; + + fn unreadable_glob_entry(pattern: String) -> FileSystemSandboxEntry { + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { pattern }, + access: FileSystemAccessMode::None, + } + } + + fn unreadable_path_entry(path: PathBuf) -> FileSystemSandboxEntry { + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: AbsolutePathBuf::from_absolute_path(path).expect("absolute path"), + }, + access: FileSystemAccessMode::None, + } + } + + #[test] + fn scan_root_uses_literal_prefix_before_glob() { + assert_eq!( + glob_scan_root("/tmp/work/**/*.env"), + PathBuf::from("/tmp/work") + ); + assert_eq!( + glob_scan_root(r"C:\Users\dev\repo\**\*.env"), + PathBuf::from(r"C:\Users\dev\repo") + ); + assert_eq!(glob_scan_root(r"C:\*.env"), PathBuf::from(r"C:\")); + } + + #[test] + fn exact_missing_paths_are_preserved() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let missing = tmp.path().join("missing.env"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_path_entry(missing)]); + + assert_eq!( + resolve_windows_deny_read_paths(&policy, &cwd).expect("resolve"), + vec![ + AbsolutePathBuf::from_absolute_path( + dunce::canonicalize(tmp.path()) + .expect("canonical tempdir") + .join("missing.env") + ) + .expect("absolute missing") + ] + ); + } + + #[test] + fn glob_patterns_expand_to_existing_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"); + let notes = tmp.path().join("app").join("notes.txt"); + std::fs::create_dir_all(notes.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"); + std::fs::write(¬es, "notes").expect("write notes"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(format!( + "{}/**/*.env", + tmp.path().display() + ))]); + + let actual: HashSet = resolve_windows_deny_read_paths(&policy, &cwd) + .expect("resolve") + .into_iter() + .map(AbsolutePathBuf::into_path_buf) + .collect(); + let expected = [root_env, nested_env].into_iter().collect(); + + assert_eq!(actual, expected); + } +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 9f8389c45e2d..b6dc78577d5c 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -110,3 +110,5 @@ mod view_image; mod web_search; mod websocket_fallback; mod window_headers; +#[cfg(target_os = "windows")] +mod windows_sandbox; diff --git a/codex-rs/core/tests/suite/windows_sandbox.rs b/codex-rs/core/tests/suite/windows_sandbox.rs new file mode 100644 index 000000000000..716769757d98 --- /dev/null +++ b/codex-rs/core/tests/suite/windows_sandbox.rs @@ -0,0 +1,131 @@ +#![cfg(target_os = "windows")] + +use codex_core::exec::ExecCapturePolicy; +use codex_core::exec::ExecParams; +use codex_core::exec::process_exec_tool_call; +use codex_core::sandboxing::SandboxPermissions; +use codex_protocol::config_types::WindowsSandboxLevel; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSpecialPath; +use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::ReadOnlyAccess; +use codex_protocol::protocol::SandboxPolicy; +use core_test_support::PathExt; +use pretty_assertions::assert_eq; +use serial_test::serial; +use std::collections::HashMap; +use std::ffi::OsString; +use tempfile::TempDir; + +struct EnvVarGuard { + key: &'static str, + original: Option, +} + +impl EnvVarGuard { + fn set(key: &'static str, value: &std::ffi::OsStr) -> Self { + let original = std::env::var_os(key); + unsafe { + std::env::set_var(key, value); + } + Self { key, original } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + unsafe { + match &self.original { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + } + } +} + +#[tokio::test] +#[serial] +async fn windows_restricted_token_enforces_exact_and_glob_deny_read_policy() -> anyhow::Result<()> { + let temp_home = TempDir::new()?; + let _codex_home_guard = EnvVarGuard::set("CODEX_HOME", temp_home.path().as_os_str()); + let workspace = TempDir::new()?; + let cwd = workspace.path().abs(); + let secret = workspace.path().join("secret.env"); + let future_secret = cwd.join("future.env"); + let public = workspace.path().join("public.txt"); + std::fs::write(&secret, "glob secret\n")?; + std::fs::write(&public, "public ok\n")?; + + let sandbox_policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![], + read_only_access: ReadOnlyAccess::FullAccess, + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::CurrentWorkingDirectory, + }, + access: FileSystemAccessMode::Write, + }, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "**/*.env".to_string(), + }, + access: FileSystemAccessMode::None, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: future_secret, + }, + access: FileSystemAccessMode::None, + }, + ]); + + let output = process_exec_tool_call( + ExecParams { + command: vec![ + "cmd.exe".to_string(), + "/D".to_string(), + "/C".to_string(), + "type secret.env >NUL 2>NUL & echo exact secret 1>future.env 2>NUL & type future.env 2>NUL & type public.txt & exit /B 0" + .to_string(), + ], + cwd: cwd.clone(), + expiration: 10_000.into(), + capture_policy: ExecCapturePolicy::ShellTool, + env: HashMap::new(), + network: None, + sandbox_permissions: SandboxPermissions::UseDefault, + windows_sandbox_level: WindowsSandboxLevel::RestrictedToken, + windows_sandbox_private_desktop: false, + justification: None, + arg0: None, + }, + &sandbox_policy, + &file_system_sandbox_policy, + NetworkSandboxPolicy::Restricted, + &cwd, + &None, + /*use_legacy_landlock*/ false, + /*stdout_stream*/ None, + ) + .await?; + + assert_eq!(output.exit_code, 0); + assert!(output.stdout.text.contains("public ok")); + assert!(!output.stdout.text.contains("glob secret")); + assert!(!output.stdout.text.contains("exact secret")); + Ok(()) +} diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index f351dba190a9..71f80cf60cd1 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -22,6 +22,7 @@ use windows_sys::Win32::Security::EqualSid; use windows_sys::Win32::Security::GetAce; use windows_sys::Win32::Security::GetAclInformation; use windows_sys::Win32::Security::MapGenericMask; +use windows_sys::Win32::Security::ACCESS_DENIED_ACE; use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE; use windows_sys::Win32::Security::ACE_HEADER; use windows_sys::Win32::Security::ACL; @@ -48,6 +49,9 @@ use windows_sys::Win32::Storage::FileSystem::READ_CONTROL; use windows_sys::Win32::Storage::FileSystem::DELETE; const SE_KERNEL_OBJECT: u32 = 6; const INHERIT_ONLY_ACE: u8 = 0x08; +const ACCESS_ALLOWED_ACE_TYPE: u8 = 0; +const ACCESS_DENIED_ACE_TYPE: u8 = 1; +const GENERIC_READ_MASK: u32 = 0x8000_0000; const GENERIC_WRITE_MASK: u32 = 0x4000_0000; const DENY_ACCESS: i32 = 3; @@ -125,7 +129,7 @@ pub unsafe fn dacl_mask_allows( continue; } let hdr = &*(p_ace as *const ACE_HEADER); - if hdr.AceType != 0 { + if hdr.AceType != ACCESS_ALLOWED_ACE_TYPE { continue; // not ACCESS_ALLOWED } if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { @@ -194,7 +198,7 @@ pub unsafe fn dacl_has_write_allow_for_sid(p_dacl: *mut ACL, psid: *mut c_void) continue; } let hdr = &*(p_ace as *const ACE_HEADER); - if hdr.AceType != 0 { + if hdr.AceType != ACCESS_ALLOWED_ACE_TYPE { continue; // ACCESS_ALLOWED_ACE_TYPE } // Ignore ACEs that are inherit-only (do not apply to the current object) @@ -242,13 +246,13 @@ pub unsafe fn dacl_has_write_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) - continue; } let hdr = &*(p_ace as *const ACE_HEADER); - if hdr.AceType != 1 { + if hdr.AceType != ACCESS_DENIED_ACE_TYPE { continue; // ACCESS_DENIED_ACE_TYPE } if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { continue; } - let ace = &*(p_ace as *const ACCESS_ALLOWED_ACE); + let ace = &*(p_ace as *const ACCESS_DENIED_ACE); let base = p_ace as usize; let sid_ptr = (base + std::mem::size_of::() + std::mem::size_of::()) as *mut c_void; @@ -259,6 +263,44 @@ pub unsafe fn dacl_has_write_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) - false } +pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> bool { + if p_dacl.is_null() { + return false; + } + let mut info: ACL_SIZE_INFORMATION = std::mem::zeroed(); + let ok = GetAclInformation( + p_dacl as *const ACL, + &mut info as *mut _ as *mut c_void, + std::mem::size_of::() as u32, + AclSizeInformation, + ); + if ok == 0 { + return false; + } + let deny_read_mask = FILE_GENERIC_READ | GENERIC_READ_MASK; + for i in 0..info.AceCount { + let mut p_ace: *mut c_void = std::ptr::null_mut(); + if GetAce(p_dacl as *const ACL, i, &mut p_ace) == 0 { + continue; + } + let hdr = &*(p_ace as *const ACE_HEADER); + if hdr.AceType != ACCESS_DENIED_ACE_TYPE { + continue; // ACCESS_DENIED_ACE_TYPE + } + if (hdr.AceFlags & INHERIT_ONLY_ACE) != 0 { + continue; + } + let ace = &*(p_ace as *const ACCESS_DENIED_ACE); + let base = p_ace as usize; + let sid_ptr = + (base + std::mem::size_of::() + std::mem::size_of::()) as *mut c_void; + if EqualSid(sid_ptr, psid) != 0 && (ace.Mask & deny_read_mask) != 0 { + return true; + } + } + false +} + const WRITE_ALLOW_MASK: u32 = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE @@ -516,6 +558,85 @@ pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result Ok(added) } +/// Adds a deny ACE to prevent reads for the given SID on the target path. +/// +/// `SetEntriesInAclW` places newly-created deny ACEs before allow ACEs, which +/// keeps the resulting DACL in the order Windows expects for denies to win. +/// The ACE is inheritable so a deny applied to a materialized directory also +/// covers files and directories later created underneath it. +/// +/// # Safety +/// Caller must ensure `psid` points to a valid SID and `path` refers to an existing file or directory. +pub unsafe fn add_deny_read_ace(path: &Path, psid: *mut c_void) -> Result { + let mut p_sd: *mut c_void = std::ptr::null_mut(); + let mut p_dacl: *mut ACL = std::ptr::null_mut(); + let code = GetNamedSecurityInfoW( + to_wide(path).as_ptr(), + 1, + DACL_SECURITY_INFORMATION, + std::ptr::null_mut(), + std::ptr::null_mut(), + &mut p_dacl, + std::ptr::null_mut(), + &mut p_sd, + ); + if code != ERROR_SUCCESS { + return Err(anyhow!("GetNamedSecurityInfoW failed: {code}")); + } + let mut added = false; + if !dacl_has_read_deny_for_sid(p_dacl, psid) { + let trustee = TRUSTEE_W { + pMultipleTrustee: std::ptr::null_mut(), + MultipleTrusteeOperation: 0, + TrusteeForm: TRUSTEE_IS_SID, + TrusteeType: TRUSTEE_IS_UNKNOWN, + ptstrName: psid as *mut u16, + }; + let mut explicit: EXPLICIT_ACCESS_W = std::mem::zeroed(); + explicit.grfAccessPermissions = FILE_GENERIC_READ | GENERIC_READ_MASK; + explicit.grfAccessMode = DENY_ACCESS; + explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; + explicit.Trustee = trustee; + let mut p_new_dacl: *mut ACL = std::ptr::null_mut(); + let code2 = SetEntriesInAclW(1, &explicit, p_dacl, &mut p_new_dacl); + if code2 != ERROR_SUCCESS { + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + if !p_new_dacl.is_null() { + LocalFree(p_new_dacl as HLOCAL); + } + return Err(anyhow!("SetEntriesInAclW failed: {code2}")); + } + let code3 = SetNamedSecurityInfoW( + to_wide(path).as_ptr() as *mut u16, + 1, + DACL_SECURITY_INFORMATION, + std::ptr::null_mut(), + std::ptr::null_mut(), + p_new_dacl, + std::ptr::null_mut(), + ); + if code3 != ERROR_SUCCESS { + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + if !p_new_dacl.is_null() { + LocalFree(p_new_dacl as HLOCAL); + } + return Err(anyhow!("SetNamedSecurityInfoW failed: {code3}")); + } + added = true; + if !p_new_dacl.is_null() { + LocalFree(p_new_dacl as HLOCAL); + } + } + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + Ok(added) +} + pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) { let mut p_sd: *mut c_void = std::ptr::null_mut(); let mut p_dacl: *mut ACL = std::ptr::null_mut(); diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs b/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs new file mode 100644 index 000000000000..4459145b099c --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs @@ -0,0 +1,212 @@ +use crate::acl::add_deny_read_ace; +use crate::acl::revoke_ace; +use crate::path_normalization::canonicalize_path; +use crate::setup::sandbox_dir; +use anyhow::Context; +use anyhow::Result; +use serde::Deserialize; +use serde::Serialize; +use std::collections::HashSet; +use std::ffi::c_void; +use std::path::Path; +use std::path::PathBuf; + +/// Identifies which Windows sandbox principal owns a persistent deny-read ACL. +/// +/// The elevated backend applies deny ACEs to the shared sandbox users group, +/// while the restricted-token backend applies them to capability SIDs. Keeping +/// separate records prevents stale cleanup for one backend from revoking entries +/// that are still owned by the other backend. +#[derive(Debug, Clone, Copy)] +pub enum DenyReadAclRecordKind { + SandboxGroup, + RestrictedToken, +} + +impl DenyReadAclRecordKind { + fn file_name(self) -> &'static str { + match self { + Self::SandboxGroup => "deny_read_acls_sandbox_group.json", + Self::RestrictedToken => "deny_read_acls_restricted_token.json", + } + } +} + +#[derive(Debug, Default, Deserialize, Serialize)] +struct DenyReadAclRecord { + paths: Vec, +} + +pub fn deny_read_acl_record_path(codex_home: &Path, kind: DenyReadAclRecordKind) -> PathBuf { + sandbox_dir(codex_home).join(kind.file_name()) +} + +/// Build the exact ACL paths that should receive a deny-read ACE. +/// +/// We keep both the lexical policy path and, when it already exists, the +/// canonical target. The lexical path covers the path users configured and lets +/// missing exact denies be materialized later; the canonical path also covers +/// an existing reparse-point target so a sandbox cannot read the same object +/// through the resolved location. +pub fn plan_deny_read_acl_paths(paths: &[PathBuf]) -> Vec { + let mut planned = Vec::new(); + let mut seen = HashSet::new(); + for path in paths { + push_planned_path(&mut planned, &mut seen, path.to_path_buf()); + if path.exists() { + push_planned_path(&mut planned, &mut seen, canonicalize_path(path)); + } + } + planned +} + +fn push_planned_path(planned: &mut Vec, seen: &mut HashSet, path: PathBuf) { + if seen.insert(lexical_path_key(&path)) { + planned.push(path); + } +} + +fn lexical_path_key(path: &Path) -> String { + path.to_string_lossy() + .replace('\\', "/") + .trim_end_matches('/') + .to_ascii_lowercase() +} + +fn read_record(path: &Path) -> Result { + match std::fs::read_to_string(path) { + Ok(contents) => serde_json::from_str(&contents) + .with_context(|| format!("parse deny-read ACL record {}", path.display())), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(DenyReadAclRecord::default()), + Err(err) => Err(err) + .with_context(|| format!("read deny-read ACL record {}", path.display())), + } +} + +pub fn write_persistent_deny_read_acl_record( + codex_home: &Path, + kind: DenyReadAclRecordKind, + paths: &[PathBuf], +) -> Result<()> { + let record_path = deny_read_acl_record_path(codex_home, kind); + 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), + }; + let contents = serde_json::to_vec_pretty(&record) + .with_context(|| format!("serialize deny-read ACL record {}", record_path.display()))?; + std::fs::write(&record_path, contents) + .with_context(|| format!("write deny-read ACL record {}", record_path.display())) +} + +/// Removes deny-read ACEs that were applied for a previous policy but are not +/// present in the current policy. This uses the same broad revoke primitive as +/// the rest of the Windows sandbox ACL guard path, so callers should run stale +/// cleanup before re-granting any read ACLs for the same SID in this refresh. +/// That ordering matters because `revoke_ace` removes all ACEs for the SID, not +/// only the deny-read ACEs recorded here. +/// +/// # Safety +/// Caller must pass a valid SID pointer for the ACEs recorded under `kind`. +pub unsafe fn cleanup_stale_persistent_deny_read_acls( + codex_home: &Path, + kind: DenyReadAclRecordKind, + desired_paths: &[PathBuf], + psid: *mut c_void, +) -> Result> { + let record_path = deny_read_acl_record_path(codex_home, kind); + let record = read_record(&record_path)?; + let desired_keys = plan_deny_read_acl_paths(desired_paths) + .into_iter() + .map(|path| lexical_path_key(&path)) + .collect::>(); + let mut cleaned = Vec::new(); + for path in record.paths { + if desired_keys.contains(&lexical_path_key(&path)) || !path.exists() { + continue; + } + revoke_ace(&path, psid); + cleaned.push(path); + } + Ok(cleaned) +} + +/// Applies deny-read ACEs to explicit paths. Missing paths are materialized as +/// directories before the ACE is applied so a sandboxed command cannot create a +/// previously absent denied path and then read from it in the same run. +/// If any path fails, deny ACEs applied by this call are revoked before the +/// error is returned so a one-shot sandbox run does not leave partial state. +/// +/// # Safety +/// Caller must pass a valid SID pointer for the sandbox principal being denied. +pub unsafe fn apply_deny_read_acls(paths: &[PathBuf], psid: *mut c_void) -> Result> { + let planned = plan_deny_read_acl_paths(paths); + let mut applied = Vec::new(); + let mut seen = HashSet::new(); + let mut added_in_this_call = Vec::new(); + for path in planned { + let result = (|| -> Result { + if !path.exists() { + std::fs::create_dir_all(&path) + .with_context(|| format!("create deny-read path {}", path.display()))?; + } + add_deny_read_ace(&path, psid) + .with_context(|| format!("apply deny-read ACE to {}", path.display())) + })(); + let added = match result { + Ok(added) => added, + Err(err) => { + for added_path in &added_in_this_call { + revoke_ace(added_path, psid); + } + return Err(err); + } + }; + if added { + added_in_this_call.push(path.clone()); + } + push_planned_path(&mut applied, &mut seen, path); + } + Ok(applied) +} + +#[cfg(test)] +mod tests { + use super::plan_deny_read_acl_paths; + use pretty_assertions::assert_eq; + use std::collections::HashSet; + use std::path::PathBuf; + use tempfile::TempDir; + + #[test] + fn plan_preserves_missing_paths() { + let tmp = TempDir::new().expect("tempdir"); + let missing = tmp.path().join("future-secret.env"); + + assert_eq!(plan_deny_read_acl_paths(std::slice::from_ref(&missing)), vec![ + missing + ]); + } + + #[test] + fn plan_includes_existing_canonical_targets() { + let tmp = TempDir::new().expect("tempdir"); + let existing = tmp.path().join("secret.env"); + std::fs::write(&existing, "secret").expect("write secret"); + + let planned: HashSet = plan_deny_read_acl_paths(std::slice::from_ref(&existing)) + .into_iter() + .collect(); + let expected: HashSet = [ + existing.clone(), + dunce::canonicalize(&existing).expect("canonical path"), + ] + .into_iter() + .collect(); + + assert_eq!(planned, expected); + } +} diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index 327425bd079f..72bd108becf8 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -14,6 +14,7 @@ pub struct ElevatedSandboxCaptureRequest<'a> { pub proxy_enforced: bool, pub read_roots_override: Option<&'a [PathBuf]>, pub write_roots_override: Option<&'a [PathBuf]>, + pub deny_read_paths_override: &'a [PathBuf], pub deny_write_paths_override: &'a [PathBuf], } @@ -239,6 +240,7 @@ mod windows_impl { proxy_enforced, read_roots_override, write_roots_override, + deny_read_paths_override, deny_write_paths_override, } = request; let policy = parse_policy(policy_json_or_preset)?; @@ -260,6 +262,7 @@ mod windows_impl { codex_home, read_roots_override, write_roots_override, + deny_read_paths_override, deny_write_paths_override, proxy_enforced, )?; diff --git a/codex-rs/windows-sandbox-rs/src/identity.rs b/codex-rs/windows-sandbox-rs/src/identity.rs index 12b02105456b..01fcbc196697 100644 --- a/codex-rs/windows-sandbox-rs/src/identity.rs +++ b/codex-rs/windows-sandbox-rs/src/identity.rs @@ -138,6 +138,7 @@ pub fn require_logon_sandbox_creds( codex_home: &Path, read_roots_override: Option<&[PathBuf]>, write_roots_override: Option<&[PathBuf]>, + deny_read_paths_override: &[PathBuf], deny_write_paths_override: &[PathBuf], proxy_enforced: bool, ) -> Result { @@ -199,6 +200,7 @@ pub fn require_logon_sandbox_creds( crate::setup::SetupRootOverrides { read_roots: Some(needed_read.clone()), write_roots: Some(needed_write.clone()), + deny_read_paths: Some(deny_read_paths_override.to_vec()), deny_write_paths: Some(deny_write_paths_override.to_vec()), }, )?; @@ -217,6 +219,7 @@ pub fn require_logon_sandbox_creds( crate::setup::SetupRootOverrides { read_roots: Some(needed_read), write_roots: Some(needed_write), + deny_read_paths: Some(deny_read_paths_override.to_vec()), deny_write_paths: Some(deny_write_paths_override.to_vec()), }, )?; diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 90176b08eadf..d386df0543f7 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -14,6 +14,7 @@ windows_modules!( audit, cap, desktop, + deny_read_acl, dpapi, env, helper_materialization, @@ -46,6 +47,8 @@ mod elevated_impl; #[cfg(target_os = "windows")] mod setup_error; +#[cfg(target_os = "windows")] +pub use acl::add_deny_read_ace; #[cfg(target_os = "windows")] pub use acl::add_deny_write_ace; @@ -70,6 +73,16 @@ pub use cap::workspace_cap_sid_for_cwd; #[cfg(target_os = "windows")] pub use conpty::spawn_conpty_process_as_user; #[cfg(target_os = "windows")] +pub use deny_read_acl::DenyReadAclRecordKind; +#[cfg(target_os = "windows")] +pub use deny_read_acl::apply_deny_read_acls; +#[cfg(target_os = "windows")] +pub use deny_read_acl::cleanup_stale_persistent_deny_read_acls; +#[cfg(target_os = "windows")] +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; +#[cfg(target_os = "windows")] pub use dpapi::protect as dpapi_protect; #[cfg(target_os = "windows")] pub use dpapi::unprotect as dpapi_unprotect; @@ -180,7 +193,7 @@ pub use windows_impl::CaptureResult; #[cfg(target_os = "windows")] pub use windows_impl::run_windows_sandbox_capture; #[cfg(target_os = "windows")] -pub use windows_impl::run_windows_sandbox_capture_with_extra_deny_write_paths; +pub use windows_impl::run_windows_sandbox_capture_with_filesystem_overrides; #[cfg(target_os = "windows")] pub use windows_impl::run_windows_sandbox_legacy_preflight; #[cfg(target_os = "windows")] @@ -211,6 +224,10 @@ mod windows_impl { use super::allow::compute_allow_paths; use super::cap::load_or_create_cap_sids; use super::cap::workspace_cap_sid_for_cwd; + use super::deny_read_acl::DenyReadAclRecordKind; + use super::deny_read_acl::apply_deny_read_acls; + use super::deny_read_acl::cleanup_stale_persistent_deny_read_acls; + use super::deny_read_acl::write_persistent_deny_read_acl_record; use super::env::apply_no_network_to_env; use super::env::ensure_non_interactive_pager; use super::env::normalize_null_device_env; @@ -298,7 +315,7 @@ mod windows_impl { timeout_ms: Option, use_private_desktop: bool, ) -> Result { - run_windows_sandbox_capture_with_extra_deny_write_paths( + run_windows_sandbox_capture_with_filesystem_overrides( policy_json_or_preset, sandbox_policy_cwd, codex_home, @@ -307,12 +324,13 @@ mod windows_impl { env_map, timeout_ms, &[], + &[], use_private_desktop, ) } #[allow(clippy::too_many_arguments)] - pub fn run_windows_sandbox_capture_with_extra_deny_write_paths( + pub fn run_windows_sandbox_capture_with_filesystem_overrides( policy_json_or_preset: &str, sandbox_policy_cwd: &Path, codex_home: &Path, @@ -320,6 +338,7 @@ mod windows_impl { cwd: &Path, mut env_map: HashMap, timeout_ms: Option, + additional_deny_read_paths: &[PathBuf], additional_deny_write_paths: &[PathBuf], use_private_desktop: bool, ) -> Result { @@ -405,6 +424,20 @@ mod windows_impl { } let canonical_cwd = canonicalize_path(¤t_dir); let mut guards: Vec<(PathBuf, *mut c_void)> = Vec::new(); + if persist_aces { + // Persistent workspace-write ACEs survive between commands, so drop + // deny-read ACLs from the previous policy before applying the new + // overlay. Non-persistent runs use guards and clean up at process + // exit instead. + unsafe { + cleanup_stale_persistent_deny_read_acls( + codex_home, + DenyReadAclRecordKind::RestrictedToken, + additional_deny_read_paths, + psid_generic, + )?; + } + } unsafe { for p in &allow { let psid = if is_workspace_write && is_command_cwd_root(p, &canonical_cwd) { @@ -432,6 +465,30 @@ mod windows_impl { guards.push((p.clone(), psid_generic)); } } + // Read denies are layered after allow/deny-write setup so they can + // override broad read grants for the sandbox principal without + // changing the existing write policy computation. + let applied_deny_read_paths = + match apply_deny_read_acls(additional_deny_read_paths, psid_generic) { + Ok(paths) => paths, + Err(err) => { + if !persist_aces { + cleanup_acl_guards(&mut guards); + } + return Err(err); + } + }; + if persist_aces { + write_persistent_deny_read_acl_record( + codex_home, + DenyReadAclRecordKind::RestrictedToken, + &applied_deny_read_paths, + )?; + } else { + for path in applied_deny_read_paths { + guards.push((path, psid_generic)); + } + } allow_null_device(psid_generic); if let Some(psid) = psid_workspace { allow_null_device(psid); @@ -454,6 +511,7 @@ mod windows_impl { let created = match spawn_res { Ok(v) => v, Err(err) => { + cleanup_acl_guards(&mut guards); unsafe { CloseHandle(in_r); CloseHandle(in_w); @@ -562,11 +620,7 @@ mod windows_impl { } if !persist_aces { - unsafe { - for (p, sid) in guards { - revoke_ace(&p, sid); - } - } + cleanup_acl_guards(&mut guards); } Ok(CaptureResult { @@ -577,6 +631,14 @@ mod windows_impl { }) } + fn cleanup_acl_guards(guards: &mut Vec<(PathBuf, *mut c_void)>) { + unsafe { + for (p, sid) in guards.drain(..) { + revoke_ace(&p, sid); + } + } + } + pub fn run_windows_sandbox_legacy_preflight( sandbox_policy: &SandboxPolicy, sandbox_policy_cwd: &Path, 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 78c0a8be8e53..4d64d1fbabeb 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -6,13 +6,16 @@ use anyhow::Context; use anyhow::Result; use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64; +use codex_windows_sandbox::DenyReadAclRecordKind; use codex_windows_sandbox::LOG_FILE_NAME; use codex_windows_sandbox::SETUP_VERSION; use codex_windows_sandbox::SetupErrorCode; use codex_windows_sandbox::SetupErrorReport; use codex_windows_sandbox::SetupFailure; use codex_windows_sandbox::add_deny_write_ace; +use codex_windows_sandbox::apply_deny_read_acls; use codex_windows_sandbox::canonicalize_path; +use codex_windows_sandbox::cleanup_stale_persistent_deny_read_acls; use codex_windows_sandbox::convert_string_sid_to_sid; use codex_windows_sandbox::ensure_allow_mask_aces_with_inheritance; use codex_windows_sandbox::ensure_allow_write_aces; @@ -28,6 +31,7 @@ use codex_windows_sandbox::sandbox_secrets_dir; use codex_windows_sandbox::string_from_sid_bytes; use codex_windows_sandbox::to_wide; use codex_windows_sandbox::workspace_cap_sid_for_cwd; +use codex_windows_sandbox::write_persistent_deny_read_acl_record; use codex_windows_sandbox::write_setup_error_report; use serde::Deserialize; use serde::Serialize; @@ -84,6 +88,8 @@ struct Payload { read_roots: Vec, write_roots: Vec, #[serde(default)] + deny_read_paths: Vec, + #[serde(default)] deny_write_paths: Vec, proxy_ports: Vec, #[serde(default)] @@ -468,6 +474,29 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> { 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 { + cleanup_stale_persistent_deny_read_acls( + &payload.codex_home, + DenyReadAclRecordKind::SandboxGroup, + &payload.deny_read_paths, + sandbox_group_psid, + ) + } { + Ok(cleaned) => { + if !cleaned.is_empty() { + log_line( + log, + &format!("cleaned {} stale deny-read ACLs", cleaned.len()), + )?; + } + } + Err(err) => { + refresh_errors.push(format!("cleanup stale deny-read ACLs failed: {err}")); + log_line(log, &format!("cleanup stale deny-read ACLs failed: {err}"))?; + } + } apply_read_acls( &payload.read_roots, &subjects, @@ -477,6 +506,24 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> { "read", OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, )?; + // 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) } { + Ok(applied) => { + write_persistent_deny_read_acl_record( + &payload.codex_home, + DenyReadAclRecordKind::SandboxGroup, + &applied, + )?; + if !applied.is_empty() { + log_line(log, &format!("applied {} deny-read ACLs", applied.len()))?; + } + } + Err(err) => { + refresh_errors.push(format!("apply deny-read ACLs failed: {err}")); + log_line(log, &format!("apply deny-read ACLs failed: {err}"))?; + } + } unsafe { if !sandbox_group_psid.is_null() { LocalFree(sandbox_group_psid as HLOCAL); @@ -603,8 +650,14 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( } } - if payload.read_roots.is_empty() { - log_line(log, "no read roots to grant; skipping read ACL helper")?; + // The read ACL helper is also responsible for persistent deny-read cleanup, + // so it must run whenever deny-read paths are present even if no new read + // roots need to be granted. + if payload.read_roots.is_empty() && payload.deny_read_paths.is_empty() { + log_line( + log, + "no read roots or deny-read paths; skipping read ACL helper", + )?; } else { match read_acl_mutex_exists() { Ok(true) => { diff --git a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs index 4effc0b70df4..ffb8bbe8d0e5 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs @@ -92,6 +92,7 @@ pub struct SandboxSetupRequest<'a> { pub struct SetupRootOverrides { pub read_roots: Option>, pub write_roots: Option>, + pub deny_read_paths: Option>, pub deny_write_paths: Option>, } @@ -146,6 +147,7 @@ pub fn run_setup_refresh_with_extra_read_roots( SetupRootOverrides { read_roots: Some(read_roots), write_roots: Some(Vec::new()), + deny_read_paths: None, deny_write_paths: None, }, ) @@ -163,6 +165,7 @@ fn run_setup_refresh_inner( return Ok(()); } let (read_roots, write_roots) = build_payload_roots(&request, &overrides); + let deny_read_paths = build_payload_deny_read_paths(overrides.deny_read_paths); let deny_write_paths = build_payload_deny_write_paths(&request, overrides.deny_write_paths); let network_identity = SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced); @@ -175,6 +178,7 @@ fn run_setup_refresh_inner( command_cwd: request.command_cwd.to_path_buf(), read_roots, write_roots, + deny_read_paths, deny_write_paths, proxy_ports: offline_proxy_settings.proxy_ports, allow_local_binding: offline_proxy_settings.allow_local_binding, @@ -445,6 +449,8 @@ struct ElevationPayload { read_roots: Vec, write_roots: Vec, #[serde(default)] + deny_read_paths: Vec, + #[serde(default)] deny_write_paths: Vec, proxy_ports: Vec, #[serde(default)] @@ -746,6 +752,7 @@ pub fn run_elevated_setup( ) })?; let (read_roots, write_roots) = build_payload_roots(&request, &overrides); + let deny_read_paths = build_payload_deny_read_paths(overrides.deny_read_paths); let deny_write_paths = build_payload_deny_write_paths(&request, overrides.deny_write_paths); let network_identity = SandboxNetworkIdentity::from_policy(request.policy, request.proxy_enforced); @@ -758,6 +765,7 @@ pub fn run_elevated_setup( command_cwd: request.command_cwd.to_path_buf(), read_roots, write_roots, + deny_read_paths, deny_write_paths, proxy_ports: offline_proxy_settings.proxy_ports, allow_local_binding: offline_proxy_settings.allow_local_binding, @@ -834,6 +842,23 @@ fn build_payload_deny_write_paths( deny_write_paths } +fn build_payload_deny_read_paths(explicit_deny_read_paths: Option>) -> Vec { + // Preserve missing exact deny paths so the Windows helper can materialize + // and deny them before the sandboxed process runs. Existing paths are + // canonicalized here to make the elevated helper operate on stable targets. + explicit_deny_read_paths + .unwrap_or_default() + .into_iter() + .map(|path| { + if path.exists() { + dunce::canonicalize(&path).unwrap_or(path) + } else { + path + } + }) + .collect() +} + fn filter_sensitive_write_roots(mut roots: Vec, codex_home: &Path) -> Vec { // Never grant capability write access to CODEX_HOME or anything under CODEX_HOME/.sandbox, // CODEX_HOME/.sandbox-bin, or CODEX_HOME/.sandbox-secrets. These locations contain sandbox @@ -1237,6 +1262,7 @@ mod tests { &super::SetupRootOverrides { read_roots: Some(vec![readable_root.clone()]), write_roots: None, + deny_read_paths: None, deny_write_paths: None, }, ); @@ -1284,6 +1310,7 @@ mod tests { &super::SetupRootOverrides { read_roots: Some(vec![readable_root.clone()]), write_roots: None, + deny_read_paths: None, deny_write_paths: None, }, ); @@ -1368,4 +1395,20 @@ mod tests { .all(|path| roots.contains(&path)) ); } + + #[test] + fn build_payload_deny_read_paths_keeps_missing_paths_and_canonicalizes_existing_paths() { + let tmp = TempDir::new().expect("tempdir"); + let existing = tmp.path().join("secret.env"); + let missing = tmp.path().join("future.env"); + fs::write(&existing, "secret").expect("write existing"); + + assert_eq!( + super::build_payload_deny_read_paths(Some(vec![existing.clone(), missing.clone()])), + vec![ + dunce::canonicalize(existing).expect("canonical existing"), + missing + ] + ); + } } From 101349928fa1e2ff0e39443d0ba8a25fc7090d52 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Thu, 16 Apr 2026 15:13:21 -0700 Subject: [PATCH 2/2] refactor(sandbox): simplify Windows deny-read parity Move Windows deny-read path resolution into the Windows sandbox crate, bound non-recursive glob expansion, and avoid unnecessary deny-read setup work when no read roots or persistent records are present. Co-authored-by: Codex --- codex-rs/core/src/exec.rs | 52 +++-------- codex-rs/core/src/lib.rs | 1 - .../windows-sandbox-rs/src/deny_read_acl.rs | 14 ++- .../src/deny_read_resolver.rs} | 92 ++++++++++++++++--- codex-rs/windows-sandbox-rs/src/lib.rs | 3 + .../windows-sandbox-rs/src/setup_main_win.rs | 62 +++++++------ .../src/setup_orchestrator.rs | 17 +--- 7 files changed, 147 insertions(+), 94 deletions(-) rename codex-rs/{core/src/windows_deny_read.rs => windows-sandbox-rs/src/deny_read_resolver.rs} (71%) diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index d62353fe0656..c6f16ed91cde 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -533,45 +533,21 @@ 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_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( @@ -587,8 +563,8 @@ async fn exec_windows_sandbox( proxy_enforced, read_roots_override: elevated_read_roots_override.as_deref(), 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 { @@ -1004,7 +980,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, )?; @@ -1125,7 +1101,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 0b5e35279269..18d1a3029280 100644 --- a/codex-rs/core/src/lib.rs +++ b/codex-rs/core/src/lib.rs @@ -117,7 +117,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 4459145b099c..6dbbe27d9350 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 80845348f13f..ad24199f82fc 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), + }; + } + GlobScanPlan { + root: PathBuf::from(literal_prefix[..separator_index].to_string()), + max_depth: glob_scan_max_depth(pattern_suffix), } - PathBuf::from(literal_prefix[..separator_index].to_string()) +} + +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 d386df0543f7..18ac562caa80 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -29,6 +29,8 @@ windows_modules!( workspace_acl ); +mod deny_read_resolver; + #[cfg(target_os = "windows")] #[path = "conpty/mod.rs"] mod conpty; @@ -82,6 +84,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 4d64d1fbabeb..c5c8168b6dd9 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -463,17 +463,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 { @@ -497,15 +486,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) } { @@ -528,15 +541,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 ffb8bbe8d0e5..673a636d9ccb 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; @@ -830,13 +831,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 @@ -849,13 +844,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() }