From b6050b42ae7e13b1eb26e9732a1582bd3c055505 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Thu, 26 Mar 2026 12:13:51 -0700 Subject: [PATCH] fix: resolve bwrap from trusted PATH entry (#15791) ## Summary - resolve system bwrap from PATH instead of hardcoding /usr/bin/bwrap - skip PATH entries that resolve inside the current workspace before launching the sandbox helper - keep the vendored bubblewrap fallback when no trusted system bwrap is found ## Validation - cargo test -p codex-core bwrap --lib - cargo test -p codex-linux-sandbox - just fix -p codex-core - just fix -p codex-linux-sandbox - just fmt - just argument-comment-lint - cargo clean --- codex-rs/core/README.md | 14 ++--- codex-rs/core/src/config/config_tests.rs | 65 ++++++++++++++++++++++-- codex-rs/core/src/config/mod.rs | 43 ++++++++++++---- codex-rs/linux-sandbox/README.md | 30 ++++++----- codex-rs/linux-sandbox/src/launcher.rs | 7 +-- 5 files changed, 121 insertions(+), 38 deletions(-) diff --git a/codex-rs/core/README.md b/codex-rs/core/README.md index 4b8cb793b..1605d28a0 100644 --- a/codex-rs/core/README.md +++ b/codex-rs/core/README.md @@ -59,13 +59,13 @@ only when the split filesystem policy round-trips through the legacy cases like `/repo = write`, `/repo/a = none`, `/repo/a/b = write`, where the more specific writable child must reopen under a denied parent. -The Linux sandbox helper prefers `/usr/bin/bwrap` whenever it is available. If -`/usr/bin/bwrap` is present but too old to support `--argv0`, the helper keeps -using system bubblewrap and switches to a no-`--argv0` compatibility path for -the inner re-exec. If `/usr/bin/bwrap` is missing, it falls back to the -vendored bubblewrap path compiled into the binary and Codex surfaces a startup -warning through its normal notification path instead of printing directly from -the sandbox helper. +The Linux sandbox helper prefers the first `bwrap` found on `PATH` outside the +current working directory whenever it is available. If `bwrap` is present but +too old to support `--argv0`, the helper keeps using system bubblewrap and +switches to a no-`--argv0` compatibility path for the inner re-exec. If +`bwrap` is missing, it falls back to the vendored bubblewrap path compiled into +the binary and Codex surfaces a startup warning through its normal notification +path instead of printing directly from the sandbox helper. ### Windows diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 949906ac4..421bfa6bd 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -5695,8 +5695,8 @@ shell_tool = true #[cfg(target_os = "linux")] #[test] fn system_bwrap_warning_reports_missing_system_bwrap() { - let warning = system_bwrap_warning_for_path(Path::new("/definitely/not/a/bwrap")) - .expect("missing system bwrap should emit a warning"); + let warning = + system_bwrap_warning_for_lookup(None).expect("missing system bwrap should emit a warning"); assert!(warning.contains("could not find system bubblewrap")); } @@ -5716,12 +5716,48 @@ exit 1 let fake_bwrap_path: &Path = fake_bwrap.as_ref(); assert_eq!( - system_bwrap_warning_for_path(fake_bwrap_path), + system_bwrap_warning_for_lookup(Some(fake_bwrap_path.to_path_buf())), None, "Do not warn even if bwrap does not support `--argv0`", ); } +#[cfg(target_os = "linux")] +#[test] +fn finds_first_executable_bwrap_in_search_paths() { + let temp_dir = tempdir().expect("temp dir"); + let cwd = temp_dir.path().join("cwd"); + let first_dir = temp_dir.path().join("first"); + let second_dir = temp_dir.path().join("second"); + std::fs::create_dir_all(&cwd).expect("create cwd"); + std::fs::create_dir_all(&first_dir).expect("create first dir"); + std::fs::create_dir_all(&second_dir).expect("create second dir"); + std::fs::write(first_dir.join("bwrap"), "not executable").expect("write non-executable bwrap"); + let expected_bwrap = write_named_fake_bwrap_in(&second_dir); + + assert_eq!( + find_system_bwrap_in_search_paths(vec![first_dir, second_dir], &cwd), + Some(expected_bwrap) + ); +} + +#[cfg(target_os = "linux")] +#[test] +fn skips_workspace_local_bwrap_in_search_paths() { + let temp_dir = tempdir().expect("temp dir"); + let cwd = temp_dir.path().join("cwd"); + let trusted_dir = temp_dir.path().join("trusted"); + std::fs::create_dir_all(&cwd).expect("create cwd"); + std::fs::create_dir_all(&trusted_dir).expect("create trusted dir"); + let _workspace_bwrap = write_named_fake_bwrap_in(&cwd); + let expected_bwrap = write_named_fake_bwrap_in(&trusted_dir); + + assert_eq!( + find_system_bwrap_in_search_paths(vec![cwd.clone(), trusted_dir], &cwd), + Some(expected_bwrap) + ); +} + #[cfg(not(target_os = "linux"))] #[test] fn system_bwrap_warning_is_disabled_off_linux() { @@ -5730,6 +5766,14 @@ fn system_bwrap_warning_is_disabled_off_linux() { #[cfg(target_os = "linux")] fn write_fake_bwrap(contents: &str) -> tempfile::TempPath { + write_fake_bwrap_in( + &std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + contents, + ) +} + +#[cfg(target_os = "linux")] +fn write_fake_bwrap_in(dir: &Path, contents: &str) -> tempfile::TempPath { use std::fs; use std::os::unix::fs::PermissionsExt; use tempfile::NamedTempFile; @@ -5737,9 +5781,8 @@ fn write_fake_bwrap(contents: &str) -> tempfile::TempPath { // Bazel can mount the OS temp directory `noexec`, so prefer the current // working directory for fake executables and fall back to the default temp // dir outside that environment. - let temp_file = std::env::current_dir() + let temp_file = NamedTempFile::new_in(dir) .ok() - .and_then(|dir| NamedTempFile::new_in(dir).ok()) .unwrap_or_else(|| NamedTempFile::new().expect("temp file")); // Linux rejects exec-ing a file that is still open for writing. let path = temp_file.into_temp_path(); @@ -5749,6 +5792,18 @@ fn write_fake_bwrap(contents: &str) -> tempfile::TempPath { path } +#[cfg(target_os = "linux")] +fn write_named_fake_bwrap_in(dir: &Path) -> PathBuf { + use std::fs; + use std::os::unix::fs::PermissionsExt; + + let path = dir.join("bwrap"); + fs::write(&path, "#!/bin/sh\n").expect("write fake bwrap"); + let permissions = fs::Permissions::from_mode(0o755); + fs::set_permissions(&path, permissions).expect("chmod fake bwrap"); + path +} + #[tokio::test] async fn approvals_reviewer_defaults_to_manual_only_without_guardian_feature() -> std::io::Result<()> { diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index b2b55feec..6db6f0e61 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -145,7 +145,7 @@ pub(crate) const DEFAULT_AGENT_JOB_MAX_RUNTIME_SECONDS: Option = None; pub const CONFIG_TOML_FILE: &str = "config.toml"; const OPENAI_BASE_URL_ENV_VAR: &str = "OPENAI_BASE_URL"; #[cfg(target_os = "linux")] -const SYSTEM_BWRAP_PATH: &str = "/usr/bin/bwrap"; +const SYSTEM_BWRAP_PROGRAM: &str = "bwrap"; const RESERVED_MODEL_PROVIDER_IDS: [&str; 3] = [ OPENAI_PROVIDER_ID, OLLAMA_OSS_PROVIDER_ID, @@ -154,7 +154,7 @@ const RESERVED_MODEL_PROVIDER_IDS: [&str; 3] = [ #[cfg(target_os = "linux")] pub fn system_bwrap_warning() -> Option { - system_bwrap_warning_for_path(Path::new(SYSTEM_BWRAP_PATH)) + system_bwrap_warning_for_lookup(find_system_bwrap_in_path()) } #[cfg(not(target_os = "linux"))] @@ -163,15 +163,40 @@ pub fn system_bwrap_warning() -> Option { } #[cfg(target_os = "linux")] -fn system_bwrap_warning_for_path(system_bwrap_path: &Path) -> Option { - if !system_bwrap_path.is_file() { - return Some(format!( - "Codex could not find system bubblewrap at {}. Please install bubblewrap with your package manager. Codex will use the vendored bubblewrap in the meantime.", - system_bwrap_path.display() - )); +fn system_bwrap_warning_for_lookup(system_bwrap_path: Option) -> Option { + match system_bwrap_path { + Some(_) => None, + None => Some( + "Codex could not find system bubblewrap on PATH. Please install bubblewrap with your package manager. Codex will use the vendored bubblewrap in the meantime." + .to_string(), + ), } +} - None +#[cfg(target_os = "linux")] +pub fn find_system_bwrap_in_path() -> Option { + let search_path = std::env::var_os("PATH")?; + let cwd = std::env::current_dir().ok()?; + find_system_bwrap_in_search_paths(std::iter::once(PathBuf::from(search_path)), &cwd) +} + +#[cfg(target_os = "linux")] +fn find_system_bwrap_in_search_paths( + search_paths: impl IntoIterator, + cwd: &Path, +) -> Option { + let search_path = std::env::join_paths(search_paths).ok()?; + let cwd = std::fs::canonicalize(cwd).unwrap_or_else(|_| cwd.to_path_buf()); + which::which_in_all(SYSTEM_BWRAP_PROGRAM, Some(search_path), &cwd) + .ok()? + .find_map(|path| { + let path = std::fs::canonicalize(path).ok()?; + if path.starts_with(&cwd) { + None + } else { + Some(path) + } + }) } fn resolve_sqlite_home_env(resolved_cwd: &Path) -> Option { diff --git a/codex-rs/linux-sandbox/README.md b/codex-rs/linux-sandbox/README.md index 5898a9c68..d8af1959a 100644 --- a/codex-rs/linux-sandbox/README.md +++ b/codex-rs/linux-sandbox/README.md @@ -7,25 +7,27 @@ This crate is responsible for producing: - the `codex-exec` CLI can check if its arg0 is `codex-linux-sandbox` and, if so, execute as if it were `codex-linux-sandbox` - this should also be true of the `codex` multitool CLI -On Linux, the bubblewrap pipeline prefers the system `/usr/bin/bwrap` whenever -it is available. If `/usr/bin/bwrap` is present but too old to support +On Linux, the bubblewrap pipeline prefers the first `bwrap` found on `PATH` +outside the current working directory whenever it is available. If `bwrap` is +present but too old to support `--argv0`, the helper keeps using system bubblewrap and switches to a -no-`--argv0` compatibility path for the inner re-exec. If `/usr/bin/bwrap` is -missing, the helper falls back to the vendored bubblewrap path compiled into -this binary. -Codex also surfaces a startup warning when `/usr/bin/bwrap` is missing so users -know it is falling back to the vendored helper. +no-`--argv0` compatibility path for the inner re-exec. If `bwrap` is missing, +the helper falls back to the vendored bubblewrap path compiled into this +binary. +Codex also surfaces a startup warning when `bwrap` is missing so users know it +is falling back to the vendored helper. **Current Behavior** - Legacy `SandboxPolicy` / `sandbox_mode` configs remain supported. - Bubblewrap is the default filesystem sandbox pipeline. -- If `/usr/bin/bwrap` is present, the helper uses it. -- If `/usr/bin/bwrap` is present but too old to support `--argv0`, the helper - uses a no-`--argv0` compatibility path for the inner re-exec. -- If `/usr/bin/bwrap` is missing, the helper falls back to the vendored - bubblewrap path. -- If `/usr/bin/bwrap` is missing, Codex also surfaces a startup warning instead - of printing directly from the sandbox helper. +- If `bwrap` is present on `PATH` outside the current working directory, the + helper uses it. +- If `bwrap` is present but too old to support `--argv0`, the helper uses a + no-`--argv0` compatibility path for the inner re-exec. +- If `bwrap` is missing, the helper falls back to the vendored bubblewrap + path. +- If `bwrap` is missing, Codex also surfaces a startup warning instead of + printing directly from the sandbox helper. - Legacy Landlock + mount protections remain available as an explicit legacy fallback path. - Set `features.use_legacy_landlock = true` (or CLI `-c use_legacy_landlock=true`) diff --git a/codex-rs/linux-sandbox/src/launcher.rs b/codex-rs/linux-sandbox/src/launcher.rs index e68eea272..c36f1c5ff 100644 --- a/codex-rs/linux-sandbox/src/launcher.rs +++ b/codex-rs/linux-sandbox/src/launcher.rs @@ -10,8 +10,6 @@ use std::sync::OnceLock; use crate::vendored_bwrap::exec_vendored_bwrap; use codex_utils_absolute_path::AbsolutePathBuf; -const SYSTEM_BWRAP_PATH: &str = "/usr/bin/bwrap"; - #[derive(Debug, Clone, PartialEq, Eq)] enum BubblewrapLauncher { System(SystemBwrapLauncher), @@ -36,7 +34,10 @@ pub(crate) fn exec_bwrap(argv: Vec, preserved_files: Vec) -> ! { fn preferred_bwrap_launcher() -> BubblewrapLauncher { static LAUNCHER: OnceLock = OnceLock::new(); LAUNCHER - .get_or_init(|| preferred_bwrap_launcher_for_path(Path::new(SYSTEM_BWRAP_PATH))) + .get_or_init(|| match codex_core::config::find_system_bwrap_in_path() { + Some(path) => preferred_bwrap_launcher_for_path(&path), + None => BubblewrapLauncher::Vendored, + }) .clone() }