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.
This commit is contained in:
parent
d37dcca7e0
commit
603b6493a9
2 changed files with 71 additions and 24 deletions
|
|
@ -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<BwrapArgs> {
|
||||
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::<Vec<_>>();
|
||||
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 {
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue