diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index eee826d66..7dd37a945 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -9,7 +9,6 @@ use windows_sys::Win32::Foundation::ERROR_SUCCESS; use windows_sys::Win32::Foundation::HLOCAL; use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; use windows_sys::Win32::Security::AclSizeInformation; -use windows_sys::Win32::Security::Authorization::GetEffectiveRightsFromAclW; use windows_sys::Win32::Security::Authorization::GetNamedSecurityInfoW; use windows_sys::Win32::Security::Authorization::GetSecurityInfo; use windows_sys::Win32::Security::Authorization::SetEntriesInAclW; @@ -258,100 +257,18 @@ pub unsafe fn dacl_has_write_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) - false } -// Compute effective rights for a trustee SID against a DACL and decide if write is effectively allowed. -// This accounts for deny ACEs and ordering; falls back to a conservative per-ACE scan if the API fails. -#[allow(dead_code)] -pub unsafe fn dacl_effective_allows_write(p_dacl: *mut ACL, psid: *mut c_void) -> bool { - if p_dacl.is_null() { - return false; - } - 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 access: u32 = 0; - let ok = GetEffectiveRightsFromAclW(p_dacl, &trustee, &mut access); - if ok == ERROR_SUCCESS { - // Map generic bits to avoid “missing” write when generic permissions are present. - let mut mapped_access = access; - if (access & GENERIC_WRITE_MASK) != 0 { - mapped_access |= FILE_GENERIC_WRITE | FILE_WRITE_DATA | FILE_APPEND_DATA; - } - if (access & READ_CONTROL) != 0 { - mapped_access |= FILE_GENERIC_READ; - } - let write_bits = FILE_GENERIC_WRITE - | FILE_WRITE_DATA - | FILE_APPEND_DATA - | FILE_WRITE_EA - | FILE_WRITE_ATTRIBUTES; - return (mapped_access & write_bits) != 0; - } - // Fallback: simple allow ACE scan (already ignores inherit-only) - dacl_has_write_allow_for_sid(p_dacl, psid) -} - -#[allow(dead_code)] -pub unsafe fn dacl_effective_allows_mask( - p_dacl: *mut ACL, - psid: *mut c_void, - desired_mask: u32, -) -> bool { - if p_dacl.is_null() { - return false; - } - use windows_sys::Win32::Security::Authorization::GetEffectiveRightsFromAclW; - use windows_sys::Win32::Security::Authorization::TRUSTEE_IS_SID; - use windows_sys::Win32::Security::Authorization::TRUSTEE_IS_UNKNOWN; - use windows_sys::Win32::Security::Authorization::TRUSTEE_W; - - 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 access: u32 = 0; - let ok = GetEffectiveRightsFromAclW(p_dacl, &trustee, &mut access); - if ok == ERROR_SUCCESS { - // Map generic bits to avoid “missing” when generic permissions are present. - let mut mapped_access = access; - if (access & GENERIC_WRITE_MASK) != 0 { - mapped_access |= FILE_GENERIC_WRITE | FILE_WRITE_DATA | FILE_APPEND_DATA; - } - if (access & READ_CONTROL) != 0 { - mapped_access |= FILE_GENERIC_READ; - } - return (mapped_access & desired_mask) == desired_mask; - } - // Fallbacks on error: if write bits are requested, reuse the write helper; otherwise fail closed. - if (desired_mask & FILE_GENERIC_WRITE) != 0 { - return dacl_effective_allows_write(p_dacl, psid); - } - false -} - -#[allow(dead_code)] const WRITE_ALLOW_MASK: u32 = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE | FILE_DELETE_CHILD; -/// Ensure all provided SIDs have an allow ACE with the requested mask on the path. -/// Returns true if any ACE was added. -/// -/// # Safety -/// Caller must pass valid SID pointers and an existing path; free the returned security descriptor with `LocalFree`. -#[allow(dead_code)] -pub unsafe fn ensure_allow_mask_aces( + +unsafe fn ensure_allow_mask_aces_with_inheritance_impl( path: &Path, sids: &[*mut c_void], allow_mask: u32, + inheritance: u32, ) -> Result { let (p_dacl, p_sd) = fetch_dacl_handle(path)?; let mut entries: Vec = Vec::new(); @@ -362,7 +279,7 @@ pub unsafe fn ensure_allow_mask_aces( entries.push(EXPLICIT_ACCESS_W { grfAccessPermissions: allow_mask, grfAccessMode: 2, // SET_ACCESS - grfInheritance: CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE, + grfInheritance: inheritance, Trustee: TRUSTEE_W { pMultipleTrustee: std::ptr::null_mut(), MultipleTrusteeOperation: 0, @@ -393,6 +310,9 @@ pub unsafe fn ensure_allow_mask_aces( ); if code3 == ERROR_SUCCESS { added = true; + if !p_new_dacl.is_null() { + LocalFree(p_new_dacl as HLOCAL); + } } else { if !p_new_dacl.is_null() { LocalFree(p_new_dacl as HLOCAL); @@ -402,9 +322,6 @@ pub unsafe fn ensure_allow_mask_aces( } return Err(anyhow!("SetNamedSecurityInfoW failed: {}", code3)); } - if !p_new_dacl.is_null() { - LocalFree(p_new_dacl as HLOCAL); - } } else { if !p_sd.is_null() { LocalFree(p_sd as HLOCAL); @@ -418,12 +335,43 @@ pub unsafe fn ensure_allow_mask_aces( Ok(added) } +/// Ensure all provided SIDs have an allow ACE with the requested mask on the path. +/// Returns true if any ACE was added. +/// +/// # Safety +/// Caller must pass valid SID pointers and an existing path; free the returned security descriptor with `LocalFree`. +pub unsafe fn ensure_allow_mask_aces_with_inheritance( + path: &Path, + sids: &[*mut c_void], + allow_mask: u32, + inheritance: u32, +) -> Result { + ensure_allow_mask_aces_with_inheritance_impl(path, sids, allow_mask, inheritance) +} + +/// Ensure all provided SIDs have an allow ACE with the requested mask on the path. +/// Returns true if any ACE was added. +/// +/// # Safety +/// Caller must pass valid SID pointers and an existing path; free the returned security descriptor with `LocalFree`. +pub unsafe fn ensure_allow_mask_aces( + path: &Path, + sids: &[*mut c_void], + allow_mask: u32, +) -> Result { + ensure_allow_mask_aces_with_inheritance( + path, + sids, + allow_mask, + CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE, + ) +} + /// Ensure all provided SIDs have a write-capable allow ACE on the path. /// Returns true if any ACE was added. /// /// # Safety /// Caller must pass valid SID pointers and an existing path; free the returned security descriptor with `LocalFree`. -#[allow(dead_code)] pub unsafe fn ensure_allow_write_aces(path: &Path, sids: &[*mut c_void]) -> Result { ensure_allow_mask_aces(path, sids, WRITE_ALLOW_MASK) } diff --git a/codex-rs/windows-sandbox-rs/src/command_runner_win.rs b/codex-rs/windows-sandbox-rs/src/command_runner_win.rs index 717138335..8f0558e09 100644 --- a/codex-rs/windows-sandbox-rs/src/command_runner_win.rs +++ b/codex-rs/windows-sandbox-rs/src/command_runner_win.rs @@ -33,6 +33,12 @@ use windows_sys::Win32::System::Threading::TerminateProcess; use windows_sys::Win32::System::Threading::WaitForSingleObject; use windows_sys::Win32::System::Threading::INFINITE; +#[path = "cwd_junction.rs"] +mod cwd_junction; + +#[allow(dead_code)] +mod read_acl_mutex; + #[derive(Debug, Deserialize)] struct RunnerRequest { policy_json_or_preset: String, @@ -146,16 +152,52 @@ pub fn main() -> Result<()> { let h_stdin = open_pipe(&req.stdin_pipe, FILE_GENERIC_READ)?; let h_stdout = open_pipe(&req.stdout_pipe, FILE_GENERIC_WRITE)?; let h_stderr = open_pipe(&req.stderr_pipe, FILE_GENERIC_WRITE)?; + let stdio = Some((h_stdin, h_stdout, h_stderr)); + + // While the read-ACL helper is running, PowerShell can fail to start in the requested CWD due + // to unreadable ancestors. Use a junction CWD for that window; once the helper finishes, go + // back to using the real requested CWD (no probing, no extra state). + let use_junction = match read_acl_mutex::read_acl_mutex_exists() { + Ok(exists) => exists, + Err(err) => { + // Fail-safe: if we can't determine the state, assume the helper might be running and + // use the junction path to avoid CWD failures on unreadable ancestors. + log_note( + &format!("junction: read_acl_mutex_exists failed: {err}; assuming read ACL helper is running"), + log_dir, + ); + true + } + }; + if use_junction { + log_note( + "junction: read ACL helper running; using junction CWD", + log_dir, + ); + } + let effective_cwd = if use_junction { + cwd_junction::create_cwd_junction(&req.cwd, log_dir).unwrap_or_else(|| req.cwd.clone()) + } else { + req.cwd.clone() + }; + log_note( + &format!( + "runner: effective cwd={} (requested {})", + effective_cwd.display(), + req.cwd.display() + ), + log_dir, + ); // Build command and env, spawn with CreateProcessAsUserW. let spawn_result = unsafe { create_process_as_user( h_token, &req.command, - &req.cwd, + &effective_cwd, &req.env_map, Some(&req.codex_home), - Some((h_stdin, h_stdout, h_stderr)), + stdio, ) }; let (proc_info, _si) = match spawn_result { @@ -219,9 +261,5 @@ pub fn main() -> Result<()> { if exit_code != 0 { eprintln!("runner child exited with code {}", exit_code); } - log_note( - &format!("runner exit pid={} code={}", proc_info.hProcess, exit_code), - log_dir, - ); std::process::exit(exit_code); } diff --git a/codex-rs/windows-sandbox-rs/src/cwd_junction.rs b/codex-rs/windows-sandbox-rs/src/cwd_junction.rs new file mode 100644 index 000000000..4765686b3 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/cwd_junction.rs @@ -0,0 +1,142 @@ +#![cfg(target_os = "windows")] + +use codex_windows_sandbox::log_note; +use std::collections::hash_map::DefaultHasher; +use std::hash::Hash; +use std::hash::Hasher; +use std::os::windows::fs::MetadataExt as _; +use std::os::windows::process::CommandExt as _; +use std::path::Path; +use std::path::PathBuf; +use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_REPARSE_POINT; + +fn junction_name_for_path(path: &Path) -> String { + let mut hasher = DefaultHasher::new(); + path.to_string_lossy().hash(&mut hasher); + format!("{:x}", hasher.finish()) +} + +fn junction_root_for_userprofile(userprofile: &str) -> PathBuf { + PathBuf::from(userprofile) + .join(".codex") + .join(".sandbox") + .join("cwd") +} + +pub fn create_cwd_junction(requested_cwd: &Path, log_dir: Option<&Path>) -> Option { + let userprofile = std::env::var("USERPROFILE").ok()?; + let junction_root = junction_root_for_userprofile(&userprofile); + if let Err(err) = std::fs::create_dir_all(&junction_root) { + log_note( + &format!( + "junction: failed to create {}: {err}", + junction_root.display() + ), + log_dir, + ); + return None; + } + + let junction_path = junction_root.join(junction_name_for_path(requested_cwd)); + if junction_path.exists() { + // Reuse an existing junction if it looks like a reparse point; this keeps the hot path + // cheap and avoids repeatedly shelling out to mklink. + match std::fs::symlink_metadata(&junction_path) { + Ok(md) if (md.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT) != 0 => { + log_note( + &format!("junction: reusing existing {}", junction_path.display()), + log_dir, + ); + return Some(junction_path); + } + Ok(_) => { + // Unexpected: something else exists at this path (regular directory/file). We'll + // try to remove it if possible and recreate the junction. + log_note( + &format!( + "junction: existing path is not a reparse point, recreating {}", + junction_path.display() + ), + log_dir, + ); + } + Err(err) => { + log_note( + &format!( + "junction: failed to stat existing {}: {err}", + junction_path.display() + ), + log_dir, + ); + return None; + } + } + + if let Err(err) = std::fs::remove_dir(&junction_path) { + log_note( + &format!( + "junction: failed to remove existing {}: {err}", + junction_path.display() + ), + log_dir, + ); + return None; + } + } + + let link = junction_path.to_string_lossy().to_string(); + let target = requested_cwd.to_string_lossy().to_string(); + + // Use `cmd /c` so we can call `mklink` (a cmd builtin). We must quote paths so CWDs + // containing spaces work reliably. + // + // IMPORTANT: `std::process::Command::args()` will apply Windows quoting/escaping rules when + // constructing the command line. Passing a single argument that itself contains quotes can + // confuse `cmd.exe` and cause mklink to fail with "syntax is incorrect". We avoid that by + // using `raw_arg` to pass the tokens exactly as `cmd.exe` expects to parse them. + // + // Paths cannot contain quotes on Windows, so no extra escaping is needed here. + let link_quoted = format!("\"{link}\""); + let target_quoted = format!("\"{target}\""); + log_note( + &format!("junction: creating via cmd /c mklink /J {link_quoted} {target_quoted}"), + log_dir, + ); + let output = match std::process::Command::new("cmd") + .raw_arg("/c") + .raw_arg("mklink") + .raw_arg("/J") + .raw_arg(&link_quoted) + .raw_arg(&target_quoted) + .output() + { + Ok(output) => output, + Err(err) => { + log_note(&format!("junction: mklink failed to run: {err}"), log_dir); + return None; + } + }; + if output.status.success() && junction_path.exists() { + log_note( + &format!( + "junction: created {} -> {}", + junction_path.display(), + requested_cwd.display() + ), + log_dir, + ); + return Some(junction_path); + } + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + log_note( + &format!( + "junction: mklink failed status={:?} stdout={} stderr={}", + output.status, + stdout.trim(), + stderr.trim() + ), + log_dir, + ); + None +} diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index fb75e6f20..7dfcd6e0a 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -7,7 +7,6 @@ mod windows_impl { use crate::env::inherit_path_env; use crate::env::normalize_null_device_env; use crate::identity::require_logon_sandbox_creds; - use crate::logging::debug_log; use crate::logging::log_failure; use crate::logging::log_note; use crate::logging::log_start; @@ -15,7 +14,6 @@ mod windows_impl { use crate::policy::parse_policy; use crate::policy::SandboxPolicy; use crate::token::convert_string_sid_to_sid; - use crate::winutil::format_last_error; use crate::winutil::quote_windows_arg; use crate::winutil::to_wide; use anyhow::Result; @@ -94,7 +92,7 @@ mod windows_impl { fn inject_git_safe_directory( env_map: &mut HashMap, cwd: &Path, - logs_base_dir: Option<&Path>, + _logs_base_dir: Option<&Path>, ) { if let Some(git_root) = find_git_root(cwd) { let mut cfg_count: usize = env_map @@ -109,10 +107,6 @@ mod windows_impl { env_map.insert(format!("GIT_CONFIG_VALUE_{cfg_count}"), git_path); cfg_count += 1; env_map.insert("GIT_CONFIG_COUNT".to_string(), cfg_count.to_string()); - log_note( - &format!("injected git safe.directory for {}", git_root.display()), - logs_base_dir, - ); } } @@ -237,7 +231,6 @@ mod windows_impl { log_start(&command, logs_base_dir); let sandbox_creds = require_logon_sandbox_creds(&policy, sandbox_policy_cwd, cwd, &env_map, codex_home)?; - log_note("cli creds ready", logs_base_dir); // Build capability SID for ACL grants. if matches!( &policy, @@ -283,13 +276,6 @@ mod windows_impl { &stderr_name, PIPE_ACCESS_DUPLEX | PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT, )?; - log_note( - &format!( - "cli pipes created stdin={} stdout={} stderr={}", - stdin_name, stdout_name, stderr_name - ), - logs_base_dir, - ); // Launch runner as sandbox user via CreateProcessWithLogonW. let runner_exe = find_runner_exe(); @@ -326,10 +312,6 @@ mod windows_impl { ); return Err(e.into()); } - log_note( - &format!("cli request file written path={}", req_file.display()), - logs_base_dir, - ); let runner_full_cmd = format!( "{} {}", quote_windows_arg(&runner_cmdline), @@ -338,14 +320,6 @@ mod windows_impl { let mut cmdline_vec: Vec = to_wide(&runner_full_cmd); let exe_w: Vec = to_wide(&runner_cmdline); let cwd_w: Vec = to_wide(cwd); - log_note( - &format!("cli prep done request_file={}", req_file.display()), - logs_base_dir, - ); - log_note( - &format!("cli about to launch runner cmd={}", runner_full_cmd), - logs_base_dir, - ); // Minimal CPWL launch: inherit env, no desktop override, no handle inheritance. let env_block: Option> = None; @@ -380,23 +354,8 @@ mod windows_impl { }; if spawn_res == 0 { let err = unsafe { GetLastError() } as i32; - let dbg = format!( - "CreateProcessWithLogonW failed: {} ({}) | cwd={} | cmd={} | req_file={} | env=inherit | si_flags={}", - err, - format_last_error(err), - cwd.display(), - runner_full_cmd, - req_file.display(), - si.dwFlags, - ); - debug_log(&dbg, logs_base_dir); - log_note(&dbg, logs_base_dir); return Err(anyhow::anyhow!("CreateProcessWithLogonW failed: {}", err)); } - log_note( - &format!("cli runner launched pid={}", pi.hProcess), - logs_base_dir, - ); // Pipes are no longer passed as std handles; no stdin payload is sent. connect_pipe(h_stdin_pipe)?; diff --git a/codex-rs/windows-sandbox-rs/src/env.rs b/codex-rs/windows-sandbox-rs/src/env.rs index 65950a0d5..8fa2f0127 100644 --- a/codex-rs/windows-sandbox-rs/src/env.rs +++ b/codex-rs/windows-sandbox-rs/src/env.rs @@ -29,7 +29,6 @@ pub fn ensure_non_interactive_pager(env_map: &mut HashMap) { } // Keep PATH and PATHEXT stable for callers that rely on inheriting the parent process env. -#[allow(dead_code)] pub fn inherit_path_env(env_map: &mut HashMap) { if !env_map.contains_key("PATH") { if let Ok(path) = env::var("PATH") { diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 3a1c5c82a..4ffab10dc 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -20,6 +20,8 @@ pub use acl::allow_null_device; #[cfg(target_os = "windows")] pub use acl::ensure_allow_mask_aces; #[cfg(target_os = "windows")] +pub use acl::ensure_allow_mask_aces_with_inheritance; +#[cfg(target_os = "windows")] pub use acl::ensure_allow_write_aces; #[cfg(target_os = "windows")] pub use acl::fetch_dacl_handle; diff --git a/codex-rs/windows-sandbox-rs/src/process.rs b/codex-rs/windows-sandbox-rs/src/process.rs index 786fe416a..2dbd152ce 100644 --- a/codex-rs/windows-sandbox-rs/src/process.rs +++ b/codex-rs/windows-sandbox-rs/src/process.rs @@ -16,22 +16,12 @@ use windows_sys::Win32::System::Console::GetStdHandle; use windows_sys::Win32::System::Console::STD_ERROR_HANDLE; use windows_sys::Win32::System::Console::STD_INPUT_HANDLE; use windows_sys::Win32::System::Console::STD_OUTPUT_HANDLE; -use windows_sys::Win32::System::JobObjects::AssignProcessToJobObject; -use windows_sys::Win32::System::JobObjects::CreateJobObjectW; -use windows_sys::Win32::System::JobObjects::JobObjectExtendedLimitInformation; -use windows_sys::Win32::System::JobObjects::SetInformationJobObject; -use windows_sys::Win32::System::JobObjects::JOBOBJECT_EXTENDED_LIMIT_INFORMATION; -use windows_sys::Win32::System::JobObjects::JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; use windows_sys::Win32::System::Threading::CreateProcessAsUserW; -use windows_sys::Win32::System::Threading::GetExitCodeProcess; -use windows_sys::Win32::System::Threading::WaitForSingleObject; use windows_sys::Win32::System::Threading::CREATE_UNICODE_ENVIRONMENT; -use windows_sys::Win32::System::Threading::INFINITE; use windows_sys::Win32::System::Threading::PROCESS_INFORMATION; use windows_sys::Win32::System::Threading::STARTF_USESTDHANDLES; use windows_sys::Win32::System::Threading::STARTUPINFOW; -#[allow(dead_code)] pub fn make_env_block(env: &HashMap) -> Vec { let mut items: Vec<(String, String)> = env.iter().map(|(k, v)| (k.clone(), v.clone())).collect(); @@ -51,7 +41,6 @@ pub fn make_env_block(env: &HashMap) -> Vec { w } -#[allow(dead_code)] unsafe fn ensure_inheritable_stdio(si: &mut STARTUPINFOW) -> Result<()> { for kind in [STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, STD_ERROR_HANDLE] { let h = GetStdHandle(kind); @@ -72,7 +61,6 @@ unsafe fn ensure_inheritable_stdio(si: &mut STARTUPINFOW) -> Result<()> { /// # Safety /// Caller must provide a valid primary token handle (`h_token`) with appropriate access, /// and the `argv`, `cwd`, and `env_map` must remain valid for the duration of the call. -#[allow(dead_code)] pub unsafe fn create_process_as_user( h_token: HANDLE, argv: &[String], @@ -148,56 +136,3 @@ pub unsafe fn create_process_as_user( } Ok((pi, si)) } - -/// # Safety -/// Caller must provide valid process information handles. -#[allow(dead_code)] -pub unsafe fn wait_process_and_exitcode(pi: &PROCESS_INFORMATION) -> Result { - let res = WaitForSingleObject(pi.hProcess, INFINITE); - if res != 0 { - return Err(anyhow!("WaitForSingleObject failed: {}", GetLastError())); - } - let mut code: u32 = 0; - if GetExitCodeProcess(pi.hProcess, &mut code) == 0 { - return Err(anyhow!("GetExitCodeProcess failed: {}", GetLastError())); - } - Ok(code as i32) -} - -/// # Safety -/// Caller must close the returned job handle. -#[allow(dead_code)] -pub unsafe fn create_job_kill_on_close() -> Result { - let h = CreateJobObjectW(std::ptr::null_mut(), std::ptr::null()); - if h == 0 { - return Err(anyhow!("CreateJobObjectW failed: {}", GetLastError())); - } - let mut limits: JOBOBJECT_EXTENDED_LIMIT_INFORMATION = std::mem::zeroed(); - limits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE; - let ok = SetInformationJobObject( - h, - JobObjectExtendedLimitInformation, - &mut limits as *mut _ as *mut c_void, - std::mem::size_of::() as u32, - ); - if ok == 0 { - return Err(anyhow!( - "SetInformationJobObject failed: {}", - GetLastError() - )); - } - Ok(h) -} - -/// # Safety -/// Caller must pass valid handles for a job object and a process. -#[allow(dead_code)] -pub unsafe fn assign_to_job(h_job: HANDLE, h_process: HANDLE) -> Result<()> { - if AssignProcessToJobObject(h_job, h_process) == 0 { - return Err(anyhow!( - "AssignProcessToJobObject failed: {}", - GetLastError() - )); - } - Ok(()) -} 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 9a2f5ec24..02db1ffc9 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_main_win.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_main_win.rs @@ -6,7 +6,7 @@ use base64::engine::general_purpose::STANDARD as BASE64; use base64::Engine; use codex_windows_sandbox::convert_string_sid_to_sid; use codex_windows_sandbox::dpapi_protect; -use codex_windows_sandbox::ensure_allow_mask_aces; +use codex_windows_sandbox::ensure_allow_mask_aces_with_inheritance; use codex_windows_sandbox::ensure_allow_write_aces; use codex_windows_sandbox::load_or_create_cap_sids; use codex_windows_sandbox::log_note; @@ -32,7 +32,6 @@ use std::path::PathBuf; use std::process::Command; use std::process::Stdio; use std::sync::mpsc; -use std::time::Instant; use windows::core::Interface; use windows::core::BSTR; use windows::Win32::Foundation::VARIANT_TRUE; @@ -267,14 +266,14 @@ fn resolve_sid(name: &str) -> Result> { } } -fn spawn_read_acl_helper(payload: &Payload, log: &mut File) -> Result<()> { +fn spawn_read_acl_helper(payload: &Payload, _log: &mut File) -> Result<()> { let mut read_payload = payload.clone(); read_payload.mode = SetupMode::ReadAclsOnly; read_payload.refresh_only = true; let payload_json = serde_json::to_vec(&read_payload)?; let payload_b64 = BASE64.encode(payload_json); let exe = std::env::current_exe().context("locate setup helper")?; - let child = Command::new(&exe) + Command::new(&exe) .arg(payload_b64) .stdin(Stdio::null()) .stdout(Stdio::null()) @@ -282,8 +281,6 @@ fn spawn_read_acl_helper(payload: &Payload, log: &mut File) -> Result<()> { .creation_flags(0x08000000) // CREATE_NO_WINDOW .spawn() .context("spawn read ACL helper")?; - let pid = child.id(); - log_line(log, &format!("spawned read ACL helper pid={pid}"))?; Ok(()) } @@ -295,20 +292,18 @@ struct ReadAclSubjects<'a> { fn apply_read_acls( read_roots: &[PathBuf], - subjects: ReadAclSubjects<'_>, + subjects: &ReadAclSubjects<'_>, log: &mut File, refresh_errors: &mut Vec, + access_mask: u32, + access_label: &str, + inheritance: u32, ) -> Result<()> { - log_line( - log, - &format!("read ACL: processing {} read roots", read_roots.len()), - )?; - let read_mask = FILE_GENERIC_READ | FILE_GENERIC_EXECUTE; for root in read_roots { if !root.exists() { log_line( log, - &format!("read root {} missing; skipping", root.display()), + &format!("{access_label} root {} missing; skipping", root.display()), )?; continue; } @@ -316,25 +311,20 @@ fn apply_read_acls( root, subjects.rx_psids, None, - read_mask, + access_mask, + access_label, refresh_errors, log, )?; if builtin_has { - log_line( - log, - &format!( - "Users/AU/Everyone already has RX on {}; skipping", - root.display() - ), - )?; continue; } let offline_has = read_mask_allows_or_log( root, &[subjects.offline_psid], Some("offline"), - read_mask, + access_mask, + access_label, refresh_errors, log, )?; @@ -342,23 +332,20 @@ fn apply_read_acls( root, &[subjects.online_psid], Some("online"), - read_mask, + access_mask, + access_label, refresh_errors, log, )?; if offline_has && online_has { - log_line( - log, - &format!( - "sandbox users already have RX on {}; skipping", - root.display() - ), - )?; continue; } log_line( log, - &format!("granting read ACE to {} for sandbox users", root.display()), + &format!( + "granting {access_label} ACE to {} for sandbox users", + root.display() + ), )?; let mut successes = usize::from(offline_has) + usize::from(online_has); let mut missing_psids: Vec<*mut c_void> = Vec::new(); @@ -372,33 +359,30 @@ fn apply_read_acls( missing_labels.push("online"); } if !missing_psids.is_empty() { - let started = Instant::now(); - match unsafe { ensure_allow_mask_aces(root, &missing_psids, read_mask) } { + let result = unsafe { + ensure_allow_mask_aces_with_inheritance( + root, + &missing_psids, + access_mask, + inheritance, + ) + }; + match result { Ok(_) => { - let elapsed_ms = started.elapsed().as_millis(); successes = 2; - log_line( - log, - &format!( - "grant read ACE succeeded on {} for {} in {elapsed_ms}ms", - root.display(), - missing_labels.join(", ") - ), - )?; } Err(err) => { - let elapsed_ms = started.elapsed().as_millis(); let label_list = missing_labels.join(", "); for label in &missing_labels { refresh_errors.push(format!( - "grant read ACE failed on {} for {label}: {err}", + "grant {access_label} ACE failed on {} for {label}: {err}", root.display() )); } log_line( log, &format!( - "grant read ACE failed on {} for {} in {elapsed_ms}ms: {err}", + "grant {access_label} ACE failed on {} for {}: {err}", root.display(), label_list ), @@ -407,12 +391,11 @@ fn apply_read_acls( } } if successes == 2 { - log_line(log, &format!("granted read ACE to {}", root.display()))?; } else { log_line( log, &format!( - "read ACE incomplete on {} (success {successes}/2)", + "{access_label} ACE incomplete on {} (success {successes}/2)", root.display() ), )?; @@ -426,6 +409,7 @@ fn read_mask_allows_or_log( psids: &[*mut c_void], label: Option<&str>, read_mask: u32, + access_label: &str, refresh_errors: &mut Vec, log: &mut File, ) -> Result { @@ -436,7 +420,7 @@ fn read_mask_allows_or_log( .map(|value| format!(" for {value}")) .unwrap_or_default(); refresh_errors.push(format!( - "read mask check failed on {}{}: {}", + "{access_label} mask check failed on {}{}: {}", root.display(), label_suffix, e @@ -444,7 +428,7 @@ fn read_mask_allows_or_log( log_line( log, &format!( - "read mask check failed on {}{}: {}; continuing", + "{access_label} mask check failed on {}{}: {}; continuing", root.display(), label_suffix, e @@ -532,7 +516,7 @@ fn lock_sandbox_dir( dir: &Path, real_user: &str, sandbox_user_sids: &[Vec], - log: &mut File, + _log: &mut File, ) -> Result<()> { std::fs::create_dir_all(dir)?; let system_sid = resolve_sid("SYSTEM")?; @@ -630,10 +614,6 @@ fn lock_sandbox_dir( } } } - log_line( - log, - &format!("sandbox dir ACL applied at {}", dir.display()), - )?; Ok(()) } @@ -719,8 +699,6 @@ fn real_main() -> Result<()> { .append(true) .open(&log_path) .context("open log")?; - log_line(&mut log, "setup binary started")?; - log_note("setup binary started", Some(sbx_dir.as_path())); let result = run_setup(&payload, &mut log, &sbx_dir); if let Err(err) = &result { let _ = log_line(&mut log, &format!("setup error: {err:?}")); @@ -762,7 +740,15 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> { online_psid, rx_psids: &rx_psids, }; - apply_read_acls(&payload.read_roots, subjects, log, &mut refresh_errors)?; + 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 !offline_psid.is_null() { LocalFree(offline_psid as HLOCAL); @@ -806,7 +792,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( Some(random_password()) }; if refresh_only { - log_line(log, "refresh-only mode: skipping user creation/firewall")?; } else { log_line( log, @@ -827,33 +812,17 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( let offline_psid = sid_bytes_to_psid(&offline_sid)?; let online_psid = sid_bytes_to_psid(&online_sid)?; let offline_sid_str = string_from_sid_bytes(&offline_sid).map_err(anyhow::Error::msg)?; - log_line( - log, - &format!( - "resolved SIDs offline={} online={}", - offline_sid_str, - string_from_sid_bytes(&online_sid).map_err(anyhow::Error::msg)? - ), - )?; + let caps = load_or_create_cap_sids(&payload.codex_home)?; let cap_psid = unsafe { convert_string_sid_to_sid(&caps.workspace) .ok_or_else(|| anyhow::anyhow!("convert capability SID failed"))? }; let mut refresh_errors: Vec = Vec::new(); - log_line(log, &format!("resolved capability SID {}", caps.workspace))?; if !refresh_only { run_netsh_firewall(&offline_sid_str, log)?; } - log_line( - log, - &format!( - "refresh: delegating {} read roots; processing {} write roots", - payload.read_roots.len(), - payload.write_roots.len() - ), - )?; if payload.read_roots.is_empty() { log_line(log, "no read roots to grant; skipping read ACL helper")?; } else { @@ -919,14 +888,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( false } }; - log_line( - log, - &format!( - "write check {label} on {} => {}", - root.display(), - if has { "present" } else { "missing" } - ), - )?; if !has { need_grant = true; } @@ -940,14 +901,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( ), )?; grant_tasks.push(root.clone()); - } else { - log_line( - log, - &format!( - "write ACE already present for all sandbox SIDs on {}", - root.display() - ), - )?; } } @@ -981,20 +934,7 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( drop(tx); for (root, res) in rx { match res { - Ok(added) => { - if log_line( - log, - &format!( - "write ACE {} on {}", - if added { "added" } else { "already present" }, - root.display() - ), - ) - .is_err() - { - // ignore log errors inside scoped thread - } - } + Ok(_) => {} Err(e) => { refresh_errors.push(format!("write ACE failed on {}: {}", root.display(), e)); if log_line( @@ -1027,7 +967,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( &[offline_sid.clone(), online_sid.clone()], log, )?; - log_line(log, "sandbox dir ACL applied")?; write_secrets( &payload.codex_home, &payload.offline_username, @@ -1037,10 +976,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( &payload.read_roots, &payload.write_roots, )?; - log_line( - log, - "sandbox users and marker written (sandbox_users.json, setup_marker.json)", - )?; } unsafe { if !offline_psid.is_null() { @@ -1060,7 +995,6 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( )?; anyhow::bail!("setup refresh had errors"); } - log_line(log, "setup binary completed")?; log_note("setup binary completed", Some(sbx_dir)); Ok(()) } diff --git a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs index c26d54481..97ff1f0a8 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs @@ -80,10 +80,6 @@ pub fn run_setup_refresh( let json = serde_json::to_vec(&payload)?; let b64 = BASE64_STANDARD.encode(json); let exe = find_setup_exe(); - log_note( - &format!("setup refresh: invoking {}", exe.display()), - Some(&sandbox_dir(codex_home)), - ); // Refresh should never request elevation; ensure verb isn't set and we don't trigger UAC. let mut cmd = Command::new(&exe); cmd.arg(&b64).stdout(Stdio::null()).stderr(Stdio::null()); diff --git a/codex-rs/windows-sandbox-rs/src/token.rs b/codex-rs/windows-sandbox-rs/src/token.rs index 3b25a5a05..fc0e840eb 100644 --- a/codex-rs/windows-sandbox-rs/src/token.rs +++ b/codex-rs/windows-sandbox-rs/src/token.rs @@ -144,7 +144,6 @@ pub unsafe fn convert_string_sid_to_sid(s: &str) -> Option<*mut c_void> { /// # Safety /// Caller must close the returned token handle. -#[allow(dead_code)] pub unsafe fn get_current_token_for_restriction() -> Result { let desired = TOKEN_DUPLICATE | TOKEN_QUERY @@ -285,7 +284,6 @@ unsafe fn enable_single_privilege(h_token: HANDLE, name: &str) -> Result<()> { /// # Safety /// Caller must close the returned token handle. -#[allow(dead_code)] pub unsafe fn create_workspace_write_token_with_cap( psid_capability: *mut c_void, ) -> Result<(HANDLE, *mut c_void)> { @@ -297,7 +295,6 @@ pub unsafe fn create_workspace_write_token_with_cap( /// # Safety /// Caller must close the returned token handle. -#[allow(dead_code)] pub unsafe fn create_readonly_token_with_cap( psid_capability: *mut c_void, ) -> Result<(HANDLE, *mut c_void)> { diff --git a/codex-rs/windows-sandbox-rs/src/winutil.rs b/codex-rs/windows-sandbox-rs/src/winutil.rs index 02f7a8cce..5fbda726e 100644 --- a/codex-rs/windows-sandbox-rs/src/winutil.rs +++ b/codex-rs/windows-sandbox-rs/src/winutil.rs @@ -85,7 +85,6 @@ pub fn format_last_error(err: i32) -> String { } } -#[allow(dead_code)] pub fn string_from_sid_bytes(sid: &[u8]) -> Result { unsafe { let mut str_ptr: *mut u16 = std::ptr::null_mut();