diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index 0b4bba3149..e3f2ecd810 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -560,7 +560,7 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl( .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) .custom_flags(FILE_FLAG_BACKUP_SEMANTICS) .open(path) - .context("open ACL target for root-only update")?, + .context("open ACL target for root-only update (MAXIMUM_ALLOWED)")?, ) } else { None @@ -660,7 +660,7 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl( .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) .custom_flags(FILE_FLAG_BACKUP_SEMANTICS) .open(path) - .context("open ACL target for update")?, + .context("open ACL target for update (READ_CONTROL | WRITE_DAC)")?, }; let code3 = SetSecurityInfo( handle.as_raw_handle() as _, @@ -910,7 +910,8 @@ unsafe fn add_deny_ace(path: &Path, psid: *mut c_void, kind: DenyAceKind) -> Res if deny_ace_already_present(&read_handle, path, psid, kind)? { return Ok(false); } - return Err(write_error).context("open deny ACL target for update"); + return Err(write_error) + .context("open deny ACL target for update (READ_CONTROL | WRITE_DAC)"); } }; if matches!(kind, DenyAceKind::Read) { diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 9091f39ebf..f3de67d568 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -9,6 +9,8 @@ mod ssh_config_dependencies; pub mod environment_transport; #[cfg(any(windows, test))] mod launch_environment; +#[cfg(any(windows, test))] +mod setup_acl_error; use std::fmt; use std::sync::Arc; diff --git a/codex-rs/windows-sandbox-rs/src/setup_acl_error.rs b/codex-rs/windows-sandbox-rs/src/setup_acl_error.rs new file mode 100644 index 0000000000..be96335a49 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/setup_acl_error.rs @@ -0,0 +1,43 @@ +//! Preserve native ACL failure causes in both refresh summaries and setup logs. +//! Recording a diagnostic never changes whether a failed ACL operation is fatal. + +use anyhow::Error; +use anyhow::Result; +use std::path::Path; + +pub(crate) enum WriteAclOperation { + Check, + Grant, + Deny, +} + +pub(crate) fn record_acl_failure( + errors: &mut Vec, + operation: WriteAclOperation, + path: &Path, + error: &Error, + write_log: impl FnOnce(&str) -> Result<()>, +) -> Result<()> { + let detail = format!("{}: {error:#}", path.display()); + let (summary, log) = match operation { + WriteAclOperation::Check => ( + format!("write ACE check failed on {detail}"), + format!("write ACE check failed on {detail}; continuing"), + ), + WriteAclOperation::Grant => ( + format!("write ACE failed on {detail}"), + format!("write ACE grant failed on {detail}"), + ), + WriteAclOperation::Deny => ( + format!("deny ACE failed on {detail}"), + format!("deny ACE failed on {detail}"), + ), + }; + // Retain the ACL failure even if writing its individual log line fails. + errors.push(summary); + write_log(&log) +} + +#[cfg(test)] +#[path = "setup_acl_error_tests.rs"] +mod tests; diff --git a/codex-rs/windows-sandbox-rs/src/setup_acl_error_tests.rs b/codex-rs/windows-sandbox-rs/src/setup_acl_error_tests.rs new file mode 100644 index 0000000000..873d0c2264 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/setup_acl_error_tests.rs @@ -0,0 +1,74 @@ +use super::WriteAclOperation; +use super::record_acl_failure; +use anyhow::anyhow; +use pretty_assertions::assert_eq; +use std::io; +use std::path::Path; + +#[test] +fn records_each_acl_operation_with_its_native_error_chain() { + let path = Path::new("project/.git"); + let native_error = io::Error::from_raw_os_error(5); + // The native wording is platform-dependent; the code and cause chain are not. + let native_message = native_error.to_string(); + let error = anyhow::Error::new(native_error) + .context("open ACL target (READ_CONTROL | WRITE_DAC)") + .context("prepare protected path"); + let detail = format!( + "project/.git: prepare protected path: open ACL target (READ_CONTROL | WRITE_DAC): {native_message}" + ); + + let mut errors = Vec::new(); + let mut log = Vec::new(); + for operation in [ + WriteAclOperation::Check, + WriteAclOperation::Grant, + WriteAclOperation::Deny, + ] { + record_acl_failure(&mut errors, operation, path, &error, |message| { + log.push(message.to_owned()); + Ok(()) + }) + .expect("record ACL failure"); + } + assert_eq!( + (errors, log), + ( + vec![ + format!("write ACE check failed on {detail}"), + format!("write ACE failed on {detail}"), + format!("deny ACE failed on {detail}"), + ], + vec![ + format!("write ACE check failed on {detail}; continuing"), + format!("write ACE grant failed on {detail}"), + format!("deny ACE failed on {detail}"), + ], + ) + ); +} + +#[test] +fn log_failure_preserves_acl_failure_and_previous_roots() { + let mut errors = vec!["previous root failed".to_string()]; + let error = anyhow!("SetSecurityInfo failed: 87").context("grant write ACL"); + let log_error = record_acl_failure( + &mut errors, + WriteAclOperation::Grant, + Path::new("virtual-drive/project"), + &error, + |_| Err(anyhow!("log unavailable")), + ) + .expect_err("report log failure"); + assert_eq!( + (errors, log_error.to_string()), + ( + vec![ + "previous root failed".to_string(), + "write ACE failed on virtual-drive/project: grant write ACL: SetSecurityInfo failed: 87" + .to_string(), + ], + "log unavailable".to_string(), + ) + ); +} diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs index 516b327357..d4557688e9 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning.rs @@ -32,6 +32,8 @@ use crate::sandbox_bin_dir; use crate::sandbox_dir; use crate::sandbox_secrets_dir; use crate::set_local_user_flags; +use crate::setup_acl_error::WriteAclOperation; +use crate::setup_acl_error::record_acl_failure; use crate::setup_error_path; use crate::setup_log_writer; use crate::string_from_sid_bytes; @@ -884,7 +886,7 @@ fn lock_sandbox_bin_dir(payload: &Payload, sandbox_group_sid: &[u8]) -> Result<( anyhow::Error::new(SetupFailure::new( SetupErrorCode::HelperSandboxLockFailed, format!( - "lock sandbox bin dir {} failed: {err}", + "lock sandbox bin dir {} failed: {err:#}", sandbox_bin_dir(&payload.codex_home).display() ), )) @@ -1058,18 +1060,12 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res match path_write_aces_need_refresh(root, &[sandbox_group_psid, root_cap_psid]) { Ok(needs_refresh) => needs_refresh, Err(e) => { - refresh_errors.push(format!( - "write ACE check failed on {}: {}", - root.display(), - e - )); - log_line( - log, - &format!( - "write ACE check failed on {}: {}; continuing", - root.display(), - e - ), + record_acl_failure( + &mut refresh_errors, + WriteAclOperation::Check, + root, + &e, + |message| log_line(log, message), )?; true } @@ -1121,10 +1117,12 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res match res { Ok(_) => {} Err(e) => { - refresh_errors.push(format!("write ACE failed on {}: {}", root.display(), e)); - if log_line( - log, - &format!("write ACE grant failed on {}: {}", root.display(), e), + if record_acl_failure( + &mut refresh_errors, + WriteAclOperation::Grant, + &root, + &e, + |message| log_line(log, message), ) .is_err() { @@ -1175,10 +1173,12 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res } Ok(false) => {} Err(err) => { - refresh_errors.push(format!("deny ACE failed on {}: {err}", path.display())); - log_line( - log, - &format!("deny ACE failed on {}: {err}", path.display()), + record_acl_failure( + &mut refresh_errors, + WriteAclOperation::Deny, + path, + &err, + |message| log_line(log, message), )?; } } diff --git a/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs b/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs index 1b08a52f91..876d60501b 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_provisioning/acl_tests.rs @@ -15,12 +15,54 @@ use super::lock_sandbox_dir; use super::resolve_sid; use super::sid_bytes_to_psid; use crate::ensure_allow_write_aces; +use crate::extract_setup_failure; use crate::path_mask_allows; +use crate::read_setup_error_report; use crate::workspace_write_cap_sid_for_root; +use crate::write_setup_error_report; +use pretty_assertions::assert_eq; use std::fs; +use windows_sys::Win32::Foundation::ERROR_PATH_NOT_FOUND; use windows_sys::Win32::Foundation::HLOCAL; use windows_sys::Win32::Foundation::LocalFree; +#[test] +fn bin_lock_report_retains_native_open_error() { + let temp = tempfile::tempdir().expect("tempdir"); + let mut payload: Payload = + serde_json::from_value(super::tests::payload_json()).expect("fixture payload"); + // The no-reparse open cannot create a bin whose parent does not exist. + // It fails before resolving account SIDs or changing any permissions. + payload.codex_home = temp.path().join("missing-home"); + payload.mode = SetupMode::ProvisionOnly; + let bin = super::sandbox_bin_dir(&payload.codex_home); + let error = lock_sandbox_bin_dir(&payload, &[]).expect_err("missing bin parent"); + let failure = extract_setup_failure(&error).expect("structured lock failure"); + let expected = crate::SetupFailure::new( + crate::SetupErrorCode::HelperSandboxLockFailed, + format!( + "lock sandbox bin dir {0} failed: open directory {0}: NtCreateFile: {1}", + bin.display(), + std::io::Error::from_raw_os_error(ERROR_PATH_NOT_FOUND as i32), + ), + ); + assert_eq!(failure, &expected); + + write_setup_error_report( + temp.path(), + &crate::SetupErrorReport { + code: failure.code, + message: failure.message.clone(), + }, + ) + .expect("write structured failure"); + let report = read_setup_error_report(temp.path()) + .expect("read structured failure") + .expect("failure report"); + assert_eq!(crate::SetupFailure::from_report(report), expected); + assert!(!payload.codex_home.exists()); +} + #[test] fn provision_only_locks_plain_directory_via_handle() { let temp = tempfile::tempdir().expect("tempdir");