From cf57320b9fea784228a26bf11f3747404b204d63 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Tue, 18 Nov 2025 16:35:00 -0800 Subject: [PATCH] windows sandbox: support multiple workspace roots (#6854) The Windows sandbox did not previously support multiple workspace roots via config. Now it does --- codex-rs/Cargo.lock | 1 + codex-rs/cli/src/debug_sandbox.rs | 8 +-- codex-rs/core/src/exec.rs | 17 ++--- codex-rs/windows-sandbox-rs/Cargo.toml | 3 + .../windows-sandbox-rs/sandbox_smoketests.py | 46 +++++++++++- codex-rs/windows-sandbox-rs/src/allow.rs | 72 +++++++++++++++---- codex-rs/windows-sandbox-rs/src/lib.rs | 19 +++-- codex-rs/windows-sandbox-rs/src/policy.rs | 41 +++-------- 8 files changed, 141 insertions(+), 66 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index a7ba2a4c6..c756c30ab 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1627,6 +1627,7 @@ name = "codex-windows-sandbox" version = "0.1.0" dependencies = [ "anyhow", + "codex-protocol", "dirs-next", "dunce", "rand 0.8.5", diff --git a/codex-rs/cli/src/debug_sandbox.rs b/codex-rs/cli/src/debug_sandbox.rs index df4c2e97c..26fecd55c 100644 --- a/codex-rs/cli/src/debug_sandbox.rs +++ b/codex-rs/cli/src/debug_sandbox.rs @@ -138,11 +138,7 @@ async fn run_command_under_sandbox( { use codex_windows_sandbox::run_windows_sandbox_capture; - let policy_str = match &config.sandbox_policy { - codex_core::protocol::SandboxPolicy::DangerFullAccess => "workspace-write", - codex_core::protocol::SandboxPolicy::ReadOnly => "read-only", - codex_core::protocol::SandboxPolicy::WorkspaceWrite { .. } => "workspace-write", - }; + let policy_str = serde_json::to_string(&config.sandbox_policy)?; let sandbox_cwd = sandbox_policy_cwd.clone(); let cwd_clone = cwd.clone(); @@ -153,7 +149,7 @@ async fn run_command_under_sandbox( // Preflight audit is invoked elsewhere at the appropriate times. let res = tokio::task::spawn_blocking(move || { run_windows_sandbox_capture( - policy_str, + policy_str.as_str(), &sandbox_cwd, base_dir.as_path(), command_vec, diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index b44a40bdb..583c2233a 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -182,12 +182,11 @@ async fn exec_windows_sandbox( .. } = params; - let policy_str = match sandbox_policy { - SandboxPolicy::DangerFullAccess => "workspace-write", - SandboxPolicy::ReadOnly => "read-only", - SandboxPolicy::WorkspaceWrite { .. } => "workspace-write", - }; - + let policy_str = serde_json::to_string(sandbox_policy).map_err(|err| { + CodexErr::Io(io::Error::other(format!( + "failed to serialize Windows sandbox policy: {err}" + ))) + })?; let sandbox_cwd = cwd.clone(); let codex_home = find_codex_home().map_err(|err| { CodexErr::Io(io::Error::other(format!( @@ -196,7 +195,7 @@ async fn exec_windows_sandbox( })?; let spawn_res = tokio::task::spawn_blocking(move || { run_windows_sandbox_capture( - policy_str, + policy_str.as_str(), &sandbox_cwd, codex_home.as_ref(), command, @@ -444,7 +443,9 @@ async fn exec( stdout_stream: Option, ) -> Result { #[cfg(target_os = "windows")] - if sandbox == SandboxType::WindowsRestrictedToken { + if sandbox == SandboxType::WindowsRestrictedToken + && !matches!(sandbox_policy, SandboxPolicy::DangerFullAccess) + { return exec_windows_sandbox(params, sandbox_policy).await; } let timeout = params.timeout_duration(); diff --git a/codex-rs/windows-sandbox-rs/Cargo.toml b/codex-rs/windows-sandbox-rs/Cargo.toml index ba39bbe0f..7664ca719 100644 --- a/codex-rs/windows-sandbox-rs/Cargo.toml +++ b/codex-rs/windows-sandbox-rs/Cargo.toml @@ -12,6 +12,9 @@ anyhow = "1.0" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" dunce = "1.0" +[dependencies.codex-protocol] +package = "codex-protocol" +path = "../protocol" [dependencies.rand] version = "0.8" default-features = false diff --git a/codex-rs/windows-sandbox-rs/sandbox_smoketests.py b/codex-rs/windows-sandbox-rs/sandbox_smoketests.py index 63bf34909..fb8f90121 100644 --- a/codex-rs/windows-sandbox-rs/sandbox_smoketests.py +++ b/codex-rs/windows-sandbox-rs/sandbox_smoketests.py @@ -50,6 +50,7 @@ TIMEOUT_SEC = 20 WS_ROOT = Path(os.environ["USERPROFILE"]) / "sbx_ws_tests" OUTSIDE = Path(os.environ["USERPROFILE"]) / "sbx_ws_outside" # outside CWD for deny checks +EXTRA_ROOT = Path(os.environ["USERPROFILE"]) / "WorkspaceRoot" # additional writable root ENV_BASE = {} # extend if needed @@ -57,7 +58,13 @@ class CaseResult: def __init__(self, name: str, ok: bool, detail: str = ""): self.name, self.ok, self.detail = name, ok, detail -def run_sbx(policy: str, cmd_argv: List[str], cwd: Path, env_extra: Optional[dict] = None) -> Tuple[int, str, str]: +def run_sbx( + policy: str, + cmd_argv: List[str], + cwd: Path, + env_extra: Optional[dict] = None, + additional_root: Optional[Path] = None, +) -> Tuple[int, str, str]: env = os.environ.copy() env.update(ENV_BASE) if env_extra: @@ -68,7 +75,15 @@ def run_sbx(policy: str, cmd_argv: List[str], cwd: Path, env_extra: Optional[dic raise ValueError(f"unknown policy: {policy}") policy_flags: List[str] = ["--full-auto"] if policy == "workspace-write" else [] - argv = [*CODEX_CMD, "sandbox", "windows", *policy_flags, "--", *cmd_argv] + overrides: List[str] = [] + if policy == "workspace-write" and additional_root is not None: + # Use config override to inject an additional writable root. + overrides = [ + "-c", + f'sandbox_workspace_write.writable_roots=["{additional_root.as_posix()}"]', + ] + + argv = [*CODEX_CMD, "sandbox", "windows", *policy_flags, *overrides, "--", *cmd_argv] print(cmd_argv) cp = subprocess.run(argv, cwd=str(cwd), env=env, stdout=subprocess.PIPE, stderr=subprocess.PIPE, @@ -134,6 +149,7 @@ def main() -> int: results: List[CaseResult] = [] make_dir_clean(WS_ROOT) OUTSIDE.mkdir(exist_ok=True) + EXTRA_ROOT.mkdir(exist_ok=True) # Environment probe: some hosts allow TEMP writes even under read-only # tokens due to ACLs and restricted SID semantics. Detect and adapt tests. probe_rc, _, _ = run_sbx( @@ -165,6 +181,32 @@ def main() -> int: rc, out, err = run_sbx("workspace-write", ["cmd", "/c", f"echo nope > {outside_file}"], WS_ROOT) add("WS: write outside workspace denied", rc != 0 and assert_not_exists(outside_file), f"rc={rc}") + # 3b. WS: allow write in additional workspace root + extra_target = EXTRA_ROOT / "extra_ok.txt" + remove_if_exists(extra_target) + rc, out, err = run_sbx( + "workspace-write", + ["cmd", "/c", f"echo extra > {extra_target}"], + WS_ROOT, + additional_root=EXTRA_ROOT, + ) + add("WS: write in additional root allowed", rc == 0 and assert_exists(extra_target), f"rc={rc}, err={err}") + + # 3c. RO: deny write in additional workspace root + ro_extra_target = EXTRA_ROOT / "extra_ro.txt" + remove_if_exists(ro_extra_target) + rc, out, err = run_sbx( + "read-only", + ["cmd", "/c", f"echo nope > {ro_extra_target}"], + WS_ROOT, + additional_root=EXTRA_ROOT, + ) + add( + "RO: write in additional root denied", + rc != 0 and assert_not_exists(ro_extra_target), + f"rc={rc}", + ) + # 4. WS: allow TEMP write rc, out, err = run_sbx("workspace-write", ["cmd", "/c", "echo tempok > %TEMP%\\ws_temp_ok.txt"], WS_ROOT) add("WS: TEMP write allowed", rc == 0, f"rc={rc}") diff --git a/codex-rs/windows-sandbox-rs/src/allow.rs b/codex-rs/windows-sandbox-rs/src/allow.rs index 01254fb58..de6215999 100644 --- a/codex-rs/windows-sandbox-rs/src/allow.rs +++ b/codex-rs/windows-sandbox-rs/src/allow.rs @@ -1,37 +1,83 @@ -use crate::policy::SandboxMode; use crate::policy::SandboxPolicy; +use dunce::canonicalize; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; pub fn compute_allow_paths( policy: &SandboxPolicy, - _policy_cwd: &Path, + policy_cwd: &Path, command_cwd: &Path, env_map: &HashMap, ) -> Vec { let mut allow: Vec = Vec::new(); let mut seen = std::collections::HashSet::new(); - if matches!(policy.0, SandboxMode::WorkspaceWrite) { - let abs = command_cwd.to_path_buf(); - if seen.insert(abs.to_string_lossy().to_string()) && abs.exists() { - allow.push(abs); + + let mut add_path = |p: PathBuf| { + if seen.insert(p.to_string_lossy().to_string()) && p.exists() { + allow.push(p); + } + }; + + if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) { + add_path(command_cwd.to_path_buf()); + if let SandboxPolicy::WorkspaceWrite { writable_roots, .. } = policy { + for root in writable_roots { + let candidate = if root.is_absolute() { + root.clone() + } else { + policy_cwd.join(root) + }; + let canonical = canonicalize(&candidate).unwrap_or(candidate); + add_path(canonical); + } } } - if !matches!(policy.0, SandboxMode::ReadOnly) { + if !matches!(policy, SandboxPolicy::ReadOnly) { for key in ["TEMP", "TMP"] { if let Some(v) = env_map.get(key) { let abs = PathBuf::from(v); - if seen.insert(abs.to_string_lossy().to_string()) && abs.exists() { - allow.push(abs); - } + add_path(abs); } else if let Ok(v) = std::env::var(key) { let abs = PathBuf::from(v); - if seen.insert(abs.to_string_lossy().to_string()) && abs.exists() { - allow.push(abs); - } + add_path(abs); } } } allow } + +#[cfg(test)] +mod tests { + use super::compute_allow_paths; + use codex_protocol::protocol::SandboxPolicy; + use std::collections::HashMap; + use std::fs; + use std::path::PathBuf; + + #[test] + fn includes_additional_writable_roots() { + let command_cwd = PathBuf::from(r"C:\Workspace"); + let extra_root = PathBuf::from(r"C:\AdditionalWritableRoot"); + let _ = fs::create_dir_all(&command_cwd); + let _ = fs::create_dir_all(&extra_root); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![extra_root.clone()], + network_access: false, + exclude_tmpdir_env_var: false, + exclude_slash_tmp: false, + }; + + let allow = compute_allow_paths(&policy, &command_cwd, &command_cwd, &HashMap::new()); + + assert!( + allow.iter().any(|p| p == &command_cwd), + "command cwd should be allowed" + ); + assert!( + allow.iter().any(|p| p == &extra_root), + "additional writable root should be allowed" + ); + } +} diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index a868654e7..9d5e9aeae 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -40,7 +40,7 @@ mod windows_impl { use super::logging::log_failure; use super::logging::log_start; use super::logging::log_success; - use super::policy::SandboxMode; + use super::policy::parse_policy; use super::policy::SandboxPolicy; use super::token::convert_string_sid_to_sid; use super::winutil::format_last_error; @@ -194,7 +194,7 @@ mod windows_impl { mut env_map: HashMap, timeout_ms: Option, ) -> Result { - let policy = SandboxPolicy::parse(policy_json_or_preset)?; + let policy = parse_policy(policy_json_or_preset)?; normalize_null_device_env(&mut env_map); ensure_non_interactive_pager(&mut env_map); apply_no_network_to_env(&mut env_map)?; @@ -206,27 +206,32 @@ mod windows_impl { let logs_base_dir = Some(codex_home); log_start(&command, logs_base_dir); let cap_sid_path = cap_sid_file(codex_home); + let is_workspace_write = matches!(&policy, SandboxPolicy::WorkspaceWrite { .. }); + let (h_token, psid_to_use): (HANDLE, *mut c_void) = unsafe { - match &policy.0 { - SandboxMode::ReadOnly => { + match &policy { + SandboxPolicy::ReadOnly => { let caps = load_or_create_cap_sids(codex_home); ensure_dir(&cap_sid_path)?; fs::write(&cap_sid_path, serde_json::to_string(&caps)?)?; let psid = convert_string_sid_to_sid(&caps.readonly).unwrap(); super::token::create_readonly_token_with_cap(psid)? } - SandboxMode::WorkspaceWrite => { + SandboxPolicy::WorkspaceWrite { .. } => { let caps = load_or_create_cap_sids(codex_home); ensure_dir(&cap_sid_path)?; fs::write(&cap_sid_path, serde_json::to_string(&caps)?)?; let psid = convert_string_sid_to_sid(&caps.workspace).unwrap(); super::token::create_workspace_write_token_with_cap(psid)? } + SandboxPolicy::DangerFullAccess => { + anyhow::bail!("DangerFullAccess is not supported for sandboxing") + } } }; unsafe { - if matches!(policy.0, SandboxMode::WorkspaceWrite) { + if is_workspace_write { if let Ok(base) = super::token::get_current_token_for_restriction() { if let Ok(bytes) = super::token::get_logon_sid_bytes(base) { let mut tmp = bytes.clone(); @@ -238,7 +243,7 @@ mod windows_impl { } } - let persist_aces = matches!(policy.0, SandboxMode::WorkspaceWrite); + let persist_aces = is_workspace_write; let allow = compute_allow_paths(&policy, sandbox_policy_cwd, ¤t_dir, &env_map); let mut guards: Vec<(PathBuf, *mut c_void)> = Vec::new(); unsafe { diff --git a/codex-rs/windows-sandbox-rs/src/policy.rs b/codex-rs/windows-sandbox-rs/src/policy.rs index 32cfa79f9..4c62c71df 100644 --- a/codex-rs/windows-sandbox-rs/src/policy.rs +++ b/codex-rs/windows-sandbox-rs/src/policy.rs @@ -1,36 +1,17 @@ use anyhow::Result; -use serde::Deserialize; -use serde::Serialize; +pub use codex_protocol::protocol::SandboxPolicy; -#[derive(Clone, Debug, Serialize, Deserialize)] -pub struct PolicyJson { - pub mode: String, - #[serde(default)] - pub workspace_roots: Vec, -} - -#[derive(Clone, Debug)] -pub enum SandboxMode { - ReadOnly, - WorkspaceWrite, -} - -#[derive(Clone, Debug)] -pub struct SandboxPolicy(pub SandboxMode); - -impl SandboxPolicy { - pub fn parse(value: &str) -> Result { - match value { - "read-only" => Ok(SandboxPolicy(SandboxMode::ReadOnly)), - "workspace-write" => Ok(SandboxPolicy(SandboxMode::WorkspaceWrite)), - other => { - let pj: PolicyJson = serde_json::from_str(other)?; - Ok(match pj.mode.as_str() { - "read-only" => SandboxPolicy(SandboxMode::ReadOnly), - "workspace-write" => SandboxPolicy(SandboxMode::WorkspaceWrite), - _ => SandboxPolicy(SandboxMode::ReadOnly), - }) +pub fn parse_policy(value: &str) -> Result { + match value { + "read-only" => Ok(SandboxPolicy::ReadOnly), + "workspace-write" => Ok(SandboxPolicy::new_workspace_write_policy()), + "danger-full-access" => anyhow::bail!("DangerFullAccess is not supported for sandboxing"), + other => { + let parsed: SandboxPolicy = serde_json::from_str(other)?; + if matches!(parsed, SandboxPolicy::DangerFullAccess) { + anyhow::bail!("DangerFullAccess is not supported for sandboxing"); } + Ok(parsed) } } }