From 603b6493a9d93f110bacf8d29295acdcdc080d89 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Tue, 17 Mar 2026 00:21:00 -0700 Subject: [PATCH] fix(linux-sandbox): ignore missing writable roots (#14890) ## Summary - skip nonexistent `workspace-write` writable roots in the Linux bubblewrap mount builder instead of aborting sandbox startup - keep existing writable roots mounted normally so mixed Windows/WSL configs continue to work - add unit and Linux integration regression coverage for the missing-root case ## Context This addresses regression A from #14875. Regression B will be handled in a separate PR. The old bubblewrap integration added `ensure_mount_targets_exist` as a preflight guard because bubblewrap bind targets must exist, and failing early let Codex return a clearer error than a lower-level mount failure. That policy turned out to be too strict once bubblewrap became the default Linux sandbox: shared Windows/WSL or mixed-platform configs can legitimately contain a well-formed writable root that does not exist on the current machine. This PR keeps bubblewrap's existing-target requirement, but changes Codex to skip missing writable roots instead of treating them as fatal configuration errors. --- codex-rs/linux-sandbox/src/bwrap.rs | 69 ++++++++++++------- .../linux-sandbox/tests/suite/landlock.rs | 26 +++++++ 2 files changed, 71 insertions(+), 24 deletions(-) diff --git a/codex-rs/linux-sandbox/src/bwrap.rs b/codex-rs/linux-sandbox/src/bwrap.rs index 23e503e6b..05e7b58e2 100644 --- a/codex-rs/linux-sandbox/src/bwrap.rs +++ b/codex-rs/linux-sandbox/src/bwrap.rs @@ -16,10 +16,8 @@ use std::os::fd::AsRawFd; use std::path::Path; use std::path::PathBuf; -use codex_core::error::CodexErr; use codex_core::error::Result; use codex_protocol::protocol::FileSystemSandboxPolicy; -use codex_protocol::protocol::WritableRoot; use codex_utils_absolute_path::AbsolutePathBuf; /// Linux "platform defaults" that keep common system binaries and dynamic @@ -41,10 +39,10 @@ const LINUX_PLATFORM_DEFAULT_READ_ROOTS: &[&str] = &[ /// Options that control how bubblewrap is invoked. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) struct BwrapOptions { - /// Whether to mount a fresh `/proc` inside the PID namespace. + /// Whether to mount a fresh `/proc` inside the sandbox. /// /// This is the secure default, but some restrictive container environments - /// deny `--proc /proc` even when PID namespaces are available. + /// deny `--proc /proc`. pub mount_proc: bool, /// How networking should be configured inside the bubblewrap sandbox. pub network_mode: BwrapNetworkMode, @@ -167,7 +165,6 @@ fn create_bwrap_flags( // Request a user namespace explicitly rather than relying on bubblewrap's // auto-enable behavior, which is skipped when the caller runs as uid 0. args.push("--unshare-user".to_string()); - // Isolate the PID namespace. args.push("--unshare-pid".to_string()); if options.network_mode.should_unshare_network() { args.push("--unshare-net".to_string()); @@ -213,9 +210,15 @@ fn create_filesystem_args( file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &Path, ) -> Result { - let writable_roots = file_system_sandbox_policy.get_writable_roots_with_cwd(cwd); + // Bubblewrap requires bind mount targets to exist. Skip missing writable + // roots so mixed-platform configs can keep harmless paths for other + // environments without breaking Linux command startup. + let writable_roots = file_system_sandbox_policy + .get_writable_roots_with_cwd(cwd) + .into_iter() + .filter(|writable_root| writable_root.root.as_path().exists()) + .collect::>(); let unreadable_roots = file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd); - ensure_mount_targets_exist(&writable_roots)?; let mut args = if file_system_sandbox_policy.has_full_disk_read_access() { // Read-only root, then mount a minimal device tree. @@ -385,23 +388,6 @@ fn create_filesystem_args( }) } -/// Validate that writable roots exist before constructing mounts. -/// -/// Bubblewrap requires bind mount targets to exist. We fail fast with a clear -/// error so callers can present an actionable message. -fn ensure_mount_targets_exist(writable_roots: &[WritableRoot]) -> Result<()> { - for writable_root in writable_roots { - let root = writable_root.root.as_path(); - if !root.exists() { - return Err(CodexErr::UnsupportedOperation(format!( - "Sandbox expected writable root {root}, but it does not exist.", - root = root.display() - ))); - } - } - Ok(()) -} - fn path_to_string(path: &Path) -> String { path.to_string_lossy().to_string() } @@ -731,6 +717,41 @@ mod tests { ); } + #[test] + fn ignores_missing_writable_roots() { + let temp_dir = TempDir::new().expect("temp dir"); + let existing_root = temp_dir.path().join("existing"); + let missing_root = temp_dir.path().join("missing"); + std::fs::create_dir(&existing_root).expect("create existing root"); + + let policy = SandboxPolicy::WorkspaceWrite { + writable_roots: vec![ + AbsolutePathBuf::try_from(existing_root.as_path()).expect("absolute existing root"), + AbsolutePathBuf::try_from(missing_root.as_path()).expect("absolute missing root"), + ], + read_only_access: Default::default(), + network_access: false, + exclude_tmpdir_env_var: true, + exclude_slash_tmp: true, + }; + + let args = create_filesystem_args(&FileSystemSandboxPolicy::from(&policy), temp_dir.path()) + .expect("filesystem args"); + let existing_root = path_to_string(&existing_root); + let missing_root = path_to_string(&missing_root); + + assert!( + args.args.windows(3).any(|window| { + window == ["--bind", existing_root.as_str(), existing_root.as_str()] + }), + "existing writable root should be rebound writable", + ); + assert!( + !args.args.iter().any(|arg| arg == &missing_root), + "missing writable root should be skipped", + ); + } + #[test] fn mounts_dev_before_writable_dev_binds() { let sandbox_policy = SandboxPolicy::WorkspaceWrite { diff --git a/codex-rs/linux-sandbox/tests/suite/landlock.rs b/codex-rs/linux-sandbox/tests/suite/landlock.rs index 05e06c244..a6e2cc917 100644 --- a/codex-rs/linux-sandbox/tests/suite/landlock.rs +++ b/codex-rs/linux-sandbox/tests/suite/landlock.rs @@ -310,6 +310,32 @@ async fn test_writable_root() { .await; } +#[tokio::test] +async fn sandbox_ignores_missing_writable_roots_under_bwrap() { + if should_skip_bwrap_tests().await { + eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable"); + return; + } + + let tempdir = tempfile::tempdir().expect("tempdir"); + let existing_root = tempdir.path().join("existing"); + let missing_root = tempdir.path().join("missing"); + std::fs::create_dir(&existing_root).expect("create existing root"); + + let output = run_cmd_result_with_writable_roots( + &["bash", "-lc", "printf sandbox-ok"], + &[existing_root, missing_root], + LONG_TIMEOUT_MS, + false, + true, + ) + .await + .expect("sandboxed command should execute"); + + assert_eq!(output.exit_code, 0); + assert_eq!(output.stdout.text, "sandbox-ok"); +} + #[tokio::test] async fn test_no_new_privs_is_enabled() { let output = run_cmd_output(