From b52abff279ffe57de90cabc536e30737002b7a73 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Thu, 26 Mar 2026 15:15:59 -0700 Subject: [PATCH] chore: move bwrap config helpers into dedicated module (#15898) ## Summary - move the bwrap PATH lookup and warning helpers out of config/mod.rs - move the related tests into a dedicated bwrap_tests.rs file ## Validation - git diff --check - skipped heavier local tests per request Follow-up to #15791. --- codex-rs/Cargo.lock | 1 + codex-rs/core/src/config/config_tests.rs | 112 ----------------------- codex-rs/core/src/config/mod.rs | 52 +---------- codex-rs/linux-sandbox/src/launcher.rs | 3 +- codex-rs/sandboxing/Cargo.toml | 1 + codex-rs/sandboxing/src/bwrap.rs | 46 ++++++++++ codex-rs/sandboxing/src/bwrap_tests.rs | 104 +++++++++++++++++++++ codex-rs/sandboxing/src/lib.rs | 11 +++ 8 files changed, 167 insertions(+), 163 deletions(-) create mode 100644 codex-rs/sandboxing/src/bwrap.rs create mode 100644 codex-rs/sandboxing/src/bwrap_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index b084aec05..9487d74a1 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2511,6 +2511,7 @@ dependencies = [ "tempfile", "tracing", "url", + "which 8.0.0", ] [[package]] diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 421bfa6bd..a62919a44 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -5692,118 +5692,6 @@ shell_tool = true Ok(()) } -#[cfg(target_os = "linux")] -#[test] -fn system_bwrap_warning_reports_missing_system_bwrap() { - let warning = - system_bwrap_warning_for_lookup(None).expect("missing system bwrap should emit a warning"); - - assert!(warning.contains("could not find system bubblewrap")); -} - -#[cfg(target_os = "linux")] -#[test] -fn system_bwrap_warning_skips_too_old_system_bwrap() { - let fake_bwrap = write_fake_bwrap( - r#"#!/bin/sh -if [ "$1" = "--help" ]; then - echo 'usage: bwrap [OPTION...] COMMAND' - exit 0 -fi -exit 1 -"#, - ); - let fake_bwrap_path: &Path = fake_bwrap.as_ref(); - - assert_eq!( - 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() { - assert!(system_bwrap_warning().is_none()); -} - -#[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; - - // 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 = 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(); - fs::write(&path, contents).expect("write fake bwrap"); - let permissions = fs::Permissions::from_mode(0o755); - fs::set_permissions(&path, permissions).expect("chmod fake bwrap"); - 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 6db6f0e61..8435e6132 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -118,7 +118,7 @@ pub use codex_config::Constrained; pub use codex_config::ConstraintError; pub use codex_config::ConstraintResult; pub use codex_network_proxy::NetworkProxyAuditMetadata; - +pub use codex_sandboxing::system_bwrap_warning; pub use managed_features::ManagedFeatures; pub use network_proxy_spec::NetworkProxySpec; pub use network_proxy_spec::StartedNetworkProxy; @@ -144,61 +144,12 @@ 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_PROGRAM: &str = "bwrap"; const RESERVED_MODEL_PROVIDER_IDS: [&str; 3] = [ OPENAI_PROVIDER_ID, OLLAMA_OSS_PROVIDER_ID, LMSTUDIO_OSS_PROVIDER_ID, ]; -#[cfg(target_os = "linux")] -pub fn system_bwrap_warning() -> Option { - system_bwrap_warning_for_lookup(find_system_bwrap_in_path()) -} - -#[cfg(not(target_os = "linux"))] -pub fn system_bwrap_warning() -> Option { - None -} - -#[cfg(target_os = "linux")] -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(), - ), - } -} - -#[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 { let raw = std::env::var(codex_state::SQLITE_HOME_ENV).ok()?; let trimmed = raw.trim(); @@ -212,6 +163,7 @@ fn resolve_sqlite_home_env(resolved_cwd: &Path) -> Option { Some(resolved_cwd.join(path)) } } + #[cfg(test)] pub(crate) fn test_config() -> Config { let codex_home = tempfile::tempdir().expect("create temp dir"); diff --git a/codex-rs/linux-sandbox/src/launcher.rs b/codex-rs/linux-sandbox/src/launcher.rs index c36f1c5ff..cfaa70f93 100644 --- a/codex-rs/linux-sandbox/src/launcher.rs +++ b/codex-rs/linux-sandbox/src/launcher.rs @@ -8,6 +8,7 @@ use std::process::Command; use std::sync::OnceLock; use crate::vendored_bwrap::exec_vendored_bwrap; +use codex_sandboxing::find_system_bwrap_in_path; use codex_utils_absolute_path::AbsolutePathBuf; #[derive(Debug, Clone, PartialEq, Eq)] @@ -34,7 +35,7 @@ pub(crate) fn exec_bwrap(argv: Vec, preserved_files: Vec) -> ! { fn preferred_bwrap_launcher() -> BubblewrapLauncher { static LAUNCHER: OnceLock = OnceLock::new(); LAUNCHER - .get_or_init(|| match codex_core::config::find_system_bwrap_in_path() { + .get_or_init(|| match find_system_bwrap_in_path() { Some(path) => preferred_bwrap_launcher_for_path(&path), None => BubblewrapLauncher::Vendored, }) diff --git a/codex-rs/sandboxing/Cargo.toml b/codex-rs/sandboxing/Cargo.toml index fc340d650..cd7558ec9 100644 --- a/codex-rs/sandboxing/Cargo.toml +++ b/codex-rs/sandboxing/Cargo.toml @@ -21,6 +21,7 @@ libc = { workspace = true } serde_json = { workspace = true } tracing = { workspace = true, features = ["log"] } url = { workspace = true } +which = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } diff --git a/codex-rs/sandboxing/src/bwrap.rs b/codex-rs/sandboxing/src/bwrap.rs new file mode 100644 index 000000000..00cf55f7f --- /dev/null +++ b/codex-rs/sandboxing/src/bwrap.rs @@ -0,0 +1,46 @@ +use std::path::Path; +use std::path::PathBuf; + +const SYSTEM_BWRAP_PROGRAM: &str = "bwrap"; + +pub fn system_bwrap_warning() -> Option { + system_bwrap_warning_for_lookup(find_system_bwrap_in_path()) +} + +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(), + ), + } +} + +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) +} + +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) + } + }) +} + +#[cfg(test)] +#[path = "bwrap_tests.rs"] +mod tests; diff --git a/codex-rs/sandboxing/src/bwrap_tests.rs b/codex-rs/sandboxing/src/bwrap_tests.rs new file mode 100644 index 000000000..e379de3fb --- /dev/null +++ b/codex-rs/sandboxing/src/bwrap_tests.rs @@ -0,0 +1,104 @@ +use super::*; +use pretty_assertions::assert_eq; +use std::path::Path; +use std::path::PathBuf; +use tempfile::tempdir; + +#[test] +fn system_bwrap_warning_reports_missing_system_bwrap() { + let warning = + system_bwrap_warning_for_lookup(None).expect("missing system bwrap should emit a warning"); + + assert!(warning.contains("could not find system bubblewrap")); +} + +#[test] +fn system_bwrap_warning_skips_too_old_system_bwrap() { + let fake_bwrap = write_fake_bwrap( + r#"#!/bin/sh +if [ "$1" = "--help" ]; then + echo 'usage: bwrap [OPTION...] COMMAND' + exit 0 +fi +exit 1 +"#, + ); + let fake_bwrap_path: &Path = fake_bwrap.as_ref(); + + assert_eq!( + system_bwrap_warning_for_lookup(Some(fake_bwrap_path.to_path_buf())), + None, + "Do not warn even if bwrap does not support `--argv0`", + ); +} + +#[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) + ); +} + +#[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) + ); +} + +fn write_fake_bwrap(contents: &str) -> tempfile::TempPath { + write_fake_bwrap_in( + &std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), + contents, + ) +} + +fn write_fake_bwrap_in(dir: &Path, contents: &str) -> tempfile::TempPath { + use std::fs; + use std::os::unix::fs::PermissionsExt; + use tempfile::NamedTempFile; + + // 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 = 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(); + fs::write(&path, contents).expect("write fake bwrap"); + let permissions = fs::Permissions::from_mode(0o755); + fs::set_permissions(&path, permissions).expect("chmod fake bwrap"); + path +} + +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 +} diff --git a/codex-rs/sandboxing/src/lib.rs b/codex-rs/sandboxing/src/lib.rs index 5eca9d480..eb6b4e33b 100644 --- a/codex-rs/sandboxing/src/lib.rs +++ b/codex-rs/sandboxing/src/lib.rs @@ -1,3 +1,5 @@ +#[cfg(target_os = "linux")] +mod bwrap; pub mod landlock; pub mod macos_permissions; mod manager; @@ -7,6 +9,10 @@ pub mod seatbelt; #[cfg(target_os = "macos")] mod seatbelt_permissions; +#[cfg(target_os = "linux")] +pub use bwrap::find_system_bwrap_in_path; +#[cfg(target_os = "linux")] +pub use bwrap::system_bwrap_warning; pub use manager::SandboxCommand; pub use manager::SandboxExecRequest; pub use manager::SandboxManager; @@ -15,3 +21,8 @@ pub use manager::SandboxTransformRequest; pub use manager::SandboxType; pub use manager::SandboxablePreference; pub use manager::get_platform_sandbox; + +#[cfg(not(target_os = "linux"))] +pub fn system_bwrap_warning() -> Option { + None +}