diff --git a/codex-rs/windows-sandbox-rs/BUILD.bazel b/codex-rs/windows-sandbox-rs/BUILD.bazel index e3f0112a85..387e73261d 100644 --- a/codex-rs/windows-sandbox-rs/BUILD.bazel +++ b/codex-rs/windows-sandbox-rs/BUILD.bazel @@ -7,4 +7,5 @@ codex_rust_crate( "Cargo.toml", "codex-windows-sandbox-setup.manifest", ], + unit_test_timeout = "long", ) diff --git a/codex-rs/windows-sandbox-rs/src/protected_metadata.rs b/codex-rs/windows-sandbox-rs/src/protected_metadata.rs index c45e11f887..8ea780fc79 100644 --- a/codex-rs/windows-sandbox-rs/src/protected_metadata.rs +++ b/codex-rs/windows-sandbox-rs/src/protected_metadata.rs @@ -1,7 +1,12 @@ +#![allow(dead_code)] + use crate::setup::ProtectedMetadataMode; use crate::setup::ProtectedMetadataTarget; +use crate::winutil::to_wide; +use anyhow::anyhow; use anyhow::Context; use anyhow::Result; +use std::collections::HashMap; use std::collections::HashSet; use std::fs::Metadata; use std::io; @@ -9,7 +14,27 @@ use std::os::windows::fs::FileTypeExt; use std::os::windows::fs::MetadataExt; use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; +use std::sync::Mutex; +use std::thread; +use windows_sys::Win32::Foundation::CloseHandle; +use windows_sys::Win32::Foundation::FALSE; +use windows_sys::Win32::Foundation::HANDLE; +use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; +use windows_sys::Win32::Foundation::TRUE; +use windows_sys::Win32::Foundation::WAIT_FAILED; +use windows_sys::Win32::Foundation::WAIT_OBJECT_0; use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_REPARSE_POINT; +use windows_sys::Win32::Storage::FileSystem::FILE_NOTIFY_CHANGE_CREATION; +use windows_sys::Win32::Storage::FileSystem::FILE_NOTIFY_CHANGE_DIR_NAME; +use windows_sys::Win32::Storage::FileSystem::FILE_NOTIFY_CHANGE_FILE_NAME; +use windows_sys::Win32::Storage::FileSystem::FindCloseChangeNotification; +use windows_sys::Win32::Storage::FileSystem::FindFirstChangeNotificationW; +use windows_sys::Win32::Storage::FileSystem::FindNextChangeNotification; +use windows_sys::Win32::System::Threading::CreateEventW; +use windows_sys::Win32::System::Threading::INFINITE; +use windows_sys::Win32::System::Threading::SetEvent; +use windows_sys::Win32::System::Threading::WaitForMultipleObjects; /// Layer: Windows enforcement layer. Existing metadata objects can be protected /// with ACLs; missing names are monitored and removed if the sandbox creates @@ -25,6 +50,14 @@ impl ProtectedMetadataGuard { self.deny_paths.iter() } + pub(crate) fn into_runtime(self) -> Result { + let monitor = MissingCreationMonitor::start(&self.monitored_paths)?; + Ok(ProtectedMetadataRuntime { + guard: self, + monitor, + }) + } + pub(crate) fn cleanup_created_monitored_paths(&self) -> Result> { let mut removed = Vec::new(); for path in &self.monitored_paths { @@ -39,6 +72,276 @@ impl ProtectedMetadataGuard { } } +/// Layer: Windows enforcement runtime. Owns the prepared guard plus any active +/// OS change listeners for the lifetime of one sandboxed command, then reports +/// whether protected metadata was created during that command. +pub(crate) struct ProtectedMetadataRuntime { + guard: ProtectedMetadataGuard, + monitor: MissingCreationMonitor, +} + +impl ProtectedMetadataRuntime { + pub(crate) fn finish(mut self) -> Result> { + let mut removed = self.monitor.finish()?; + removed.extend(self.guard.cleanup_created_monitored_paths()?); + Ok(unique_paths(removed)) + } +} + +/// Layer: Windows OS-event listener. Watches parents of missing protected +/// metadata names and removes matching filesystem objects as soon as Windows +/// reports creation, rename, or file-name changes under those parents. +struct MissingCreationMonitor { + stop_event: HANDLE, + listeners: Vec>, + removed_paths: Arc>>, + errors: Arc>>, +} + +impl MissingCreationMonitor { + fn start(paths: &[PathBuf]) -> Result { + if paths.is_empty() { + return Ok(Self { + stop_event: 0, + listeners: Vec::new(), + removed_paths: Arc::new(Mutex::new(Vec::new())), + errors: Arc::new(Mutex::new(Vec::new())), + }); + } + + let stop_event = unsafe { CreateEventW(std::ptr::null(), TRUE, FALSE, std::ptr::null()) }; + if stop_event == 0 { + return Err(anyhow!( + "failed to create protected metadata monitor stop event: {}", + io::Error::last_os_error() + )); + } + + let mut monitor = Self { + stop_event, + listeners: Vec::new(), + removed_paths: Arc::new(Mutex::new(Vec::new())), + errors: Arc::new(Mutex::new(Vec::new())), + }; + + for (parent, watched_paths) in monitored_paths_by_parent(paths) { + match monitor.spawn_listener(parent, watched_paths) { + Ok(listener) => monitor.listeners.push(listener), + Err(err) => { + monitor.stop_listeners(); + return Err(err); + } + } + } + + Ok(monitor) + } + + fn spawn_listener( + &self, + parent: PathBuf, + watched_paths: Vec, + ) -> Result> { + let parent_wide = to_wide(&parent); + let change_handle = unsafe { + FindFirstChangeNotificationW( + parent_wide.as_ptr(), + FALSE, + FILE_NOTIFY_CHANGE_FILE_NAME + | FILE_NOTIFY_CHANGE_DIR_NAME + | FILE_NOTIFY_CHANGE_CREATION, + ) + }; + if change_handle == INVALID_HANDLE_VALUE { + return Err(anyhow!( + "failed to monitor protected metadata parent {}: {}", + parent.display(), + io::Error::last_os_error() + )); + } + + let stop_event = self.stop_event; + let removed_paths = Arc::clone(&self.removed_paths); + let errors = Arc::clone(&self.errors); + let parent_display = parent.display().to_string(); + let parent_display_for_listener = parent_display.clone(); + thread::Builder::new() + .name("codex-protected-metadata-monitor".to_string()) + .spawn(move || { + enforce_monitored_paths(&watched_paths, &removed_paths, &errors); + loop { + let handles = [change_handle, stop_event]; + let wait_result = unsafe { + WaitForMultipleObjects( + handles.len() as u32, + handles.as_ptr(), + FALSE, + INFINITE, + ) + }; + + if wait_result == WAIT_OBJECT_0 { + enforce_monitored_paths(&watched_paths, &removed_paths, &errors); + if unsafe { FindNextChangeNotification(change_handle) } == 0 { + record_monitor_error( + &errors, + format!( + "failed to resume protected metadata monitor for {}: {}", + parent_display_for_listener, + io::Error::last_os_error() + ), + ); + break; + } + } else if wait_result == WAIT_OBJECT_0 + 1 { + break; + } else if wait_result == WAIT_FAILED { + record_monitor_error( + &errors, + format!( + "failed while waiting for protected metadata changes under {}: {}", + parent_display_for_listener, + io::Error::last_os_error() + ), + ); + break; + } else { + record_monitor_error( + &errors, + format!( + "unexpected protected metadata wait result {wait_result} for {parent_display_for_listener}" + ), + ); + break; + } + } + unsafe { + FindCloseChangeNotification(change_handle); + } + }) + .map_err(|err| { + unsafe { + FindCloseChangeNotification(change_handle); + } + anyhow!( + "failed to start protected metadata monitor for {parent_display}: {err}" + ) + }) + } + + fn finish(&mut self) -> Result> { + self.stop_listeners(); + let errors = self + .errors + .lock() + .map_err(|_| anyhow!("protected metadata monitor error state is poisoned"))? + .clone(); + if !errors.is_empty() { + return Err(anyhow!( + "protected metadata monitor failed: {}", + errors.join("; ") + )); + } + + let removed = self + .removed_paths + .lock() + .map_err(|_| anyhow!("protected metadata monitor removal state is poisoned"))? + .clone(); + Ok(unique_paths(removed)) + } + + fn stop_listeners(&mut self) { + if self.stop_event != 0 { + if unsafe { SetEvent(self.stop_event) } == 0 { + record_monitor_error( + &self.errors, + format!( + "failed to stop protected metadata monitor: {}", + io::Error::last_os_error() + ), + ); + } + while let Some(listener) = self.listeners.pop() { + if listener.join().is_err() { + record_monitor_error( + &self.errors, + "protected metadata monitor listener panicked".to_string(), + ); + } + } + unsafe { + CloseHandle(self.stop_event); + } + self.stop_event = 0; + } + } +} + +impl Drop for MissingCreationMonitor { + fn drop(&mut self) { + self.stop_listeners(); + } +} + +fn monitored_paths_by_parent(paths: &[PathBuf]) -> Vec<(PathBuf, Vec)> { + let mut grouped: HashMap)> = HashMap::new(); + for path in paths { + let Some(parent) = path.parent() else { + continue; + }; + let entry = grouped + .entry(path_text_key(parent)) + .or_insert_with(|| (parent.to_path_buf(), Vec::new())); + entry.1.push(path.clone()); + } + grouped.into_values().collect() +} + +fn enforce_monitored_paths( + paths: &[PathBuf], + removed_paths: &Arc>>, + errors: &Arc>>, +) { + for path in paths { + match existing_metadata_path(path) { + Ok(Some(existing_path)) => { + if let Err(err) = remove_metadata_path(&existing_path) { + record_monitor_error( + errors, + format!( + "failed to remove protected metadata {}: {err:#}", + existing_path.display() + ), + ); + continue; + } + match removed_paths.lock() { + Ok(mut removed) => removed.push(existing_path), + Err(_) => record_monitor_error( + errors, + "protected metadata monitor removal state is poisoned".to_string(), + ), + } + } + Ok(None) => {} + Err(err) => record_monitor_error( + errors, + format!( + "failed to inspect protected metadata {}: {err:#}", + path.display() + ), + ), + } + } +} + +fn record_monitor_error(errors: &Arc>>, message: String) { + if let Ok(mut errors) = errors.lock() { + errors.push(message); + } +} + pub(crate) fn prepare_protected_metadata_targets( targets: &[ProtectedMetadataTarget], ) -> ProtectedMetadataGuard { @@ -93,9 +396,20 @@ fn path_text_key(path: &Path) -> String { .to_ascii_lowercase() } +fn unique_paths(paths: Vec) -> Vec { + let mut seen = HashSet::new(); + let mut unique = Vec::new(); + for path in paths { + if seen.insert(path_text_key(&path)) { + unique.push(path); + } + } + unique +} + fn existing_metadata_path(path: &Path) -> Result> { match std::fs::symlink_metadata(path) { - Ok(_) => return Ok(Some(path.to_path_buf())), + Ok(_) => return Ok(matching_metadata_child(path)?.or_else(|| Some(path.to_path_buf()))), Err(err) if err.kind() == io::ErrorKind::NotFound => {} Err(err) => { return Err(err) @@ -103,6 +417,10 @@ fn existing_metadata_path(path: &Path) -> Result> { } } + matching_metadata_child(path) +} + +fn matching_metadata_child(path: &Path) -> Result> { let Some(parent) = path.parent() else { return Ok(None); }; @@ -171,6 +489,8 @@ mod tests { use super::*; use crate::setup::ProtectedMetadataMode; use crate::setup::ProtectedMetadataTarget; + use std::time::Duration; + use std::time::Instant; #[test] fn cleanup_created_monitored_paths_removes_case_variant() { @@ -196,6 +516,37 @@ mod tests { assert!(!created.exists()); } + #[test] + fn missing_creation_monitor_removes_created_case_variant() { + let temp_dir = tempfile::TempDir::new().expect("tempdir"); + let target = temp_dir.path().join(".git"); + let created = temp_dir.path().join(".GIT"); + let runtime = prepare_protected_metadata_targets(&[ProtectedMetadataTarget { + path: target, + mode: ProtectedMetadataMode::MissingCreationMonitor, + }]) + .into_runtime() + .expect("runtime"); + + std::fs::create_dir_all(&created).expect("create metadata"); + let deadline = Instant::now() + Duration::from_secs(2); + while created.exists() && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(20)); + } + + assert!( + !created.exists(), + "monitor should remove protected metadata before final cleanup" + ); + let removed = runtime.finish().expect("finish"); + assert!( + removed + .iter() + .any(|path| path.file_name().and_then(std::ffi::OsStr::to_str) == Some(".GIT")), + "removed paths should include the created case variant: {removed:?}" + ); + } + #[test] fn existing_deny_paths_include_symlink_target() { let temp_dir = tempfile::TempDir::new().expect("tempdir");