mirror of
https://github.com/openai/codex.git
synced 2026-10-27 18:48:37 +00:00
Fix missing .codex Linux bwrap startup
Co-authored-by: Codex noreply@op
This commit is contained in:
co-authored by
Codex noreply@op
parent
46b7e4fb2c
commit
709b9c075f
@@ -1181,6 +1181,52 @@ exclude_slash_tmp = true
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn missing_project_dot_codex_warning_condition_matches_workspace_policy() -> std::io::Result<()> {
|
||||
let cwd = TempDir::new()?;
|
||||
let workspace_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
cwd.path(),
|
||||
);
|
||||
|
||||
assert!(should_warn_missing_project_dot_codex_for_linux_bwrap(
|
||||
&workspace_policy,
|
||||
cwd.path()
|
||||
));
|
||||
|
||||
let read_only_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(
|
||||
&SandboxPolicy::new_read_only_policy(),
|
||||
cwd.path(),
|
||||
);
|
||||
assert!(!should_warn_missing_project_dot_codex_for_linux_bwrap(
|
||||
&read_only_policy,
|
||||
cwd.path()
|
||||
));
|
||||
|
||||
std::fs::create_dir(cwd.path().join(".codex"))?;
|
||||
assert!(!should_warn_missing_project_dot_codex_for_linux_bwrap(
|
||||
&workspace_policy,
|
||||
cwd.path()
|
||||
));
|
||||
|
||||
#[cfg(unix)]
|
||||
{
|
||||
let cwd = TempDir::new()?;
|
||||
let workspace_policy = FileSystemSandboxPolicy::from_legacy_sandbox_policy(
|
||||
&SandboxPolicy::new_workspace_write_policy(),
|
||||
cwd.path(),
|
||||
);
|
||||
std::os::unix::fs::symlink(cwd.path().join("missing-target"), cwd.path().join(".codex"))?;
|
||||
|
||||
assert!(!should_warn_missing_project_dot_codex_for_linux_bwrap(
|
||||
&workspace_policy,
|
||||
cwd.path()
|
||||
));
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn filter_mcp_servers_by_allowlist_enforces_identity_rules() {
|
||||
const MISMATCHED_COMMAND_SERVER: &str = "mismatched-command-should-disable";
|
||||
|
||||
@@ -148,6 +148,7 @@ pub(crate) const DEFAULT_AGENT_MAX_DEPTH: i32 = 1;
|
||||
pub(crate) const DEFAULT_AGENT_JOB_MAX_RUNTIME_SECONDS: Option<u64> = None;
|
||||
|
||||
pub const CONFIG_TOML_FILE: &str = "config.toml";
|
||||
const PROJECT_LOCAL_CODEX_DIR: &str = ".codex";
|
||||
const RESERVED_MODEL_PROVIDER_IDS: [&str; 3] = [
|
||||
OPENAI_PROVIDER_ID,
|
||||
OLLAMA_OSS_PROVIDER_ID,
|
||||
@@ -168,6 +169,42 @@ fn resolve_sqlite_home_env(resolved_cwd: &Path) -> Option<PathBuf> {
|
||||
}
|
||||
}
|
||||
|
||||
fn maybe_warn_missing_project_dot_codex_for_linux_bwrap(
|
||||
startup_warnings: &mut Vec<String>,
|
||||
file_system_sandbox_policy: &FileSystemSandboxPolicy,
|
||||
cwd: &Path,
|
||||
) {
|
||||
if !cfg!(target_os = "linux")
|
||||
|| !should_warn_missing_project_dot_codex_for_linux_bwrap(file_system_sandbox_policy, cwd)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
let dot_codex = cwd.join(PROJECT_LOCAL_CODEX_DIR);
|
||||
let dot_codex_display = dot_codex.display();
|
||||
let warning = format!(
|
||||
"Project-local `.codex` does not exist at `{dot_codex_display}`. On Linux, Codex will not create a sandbox mount target for it, so shell commands may create `.codex` without approval until it exists; existing `.codex` paths and `apply_patch` edits remain protected."
|
||||
);
|
||||
tracing::warn!("{warning}");
|
||||
startup_warnings.push(warning);
|
||||
}
|
||||
|
||||
fn should_warn_missing_project_dot_codex_for_linux_bwrap(
|
||||
file_system_sandbox_policy: &FileSystemSandboxPolicy,
|
||||
cwd: &Path,
|
||||
) -> bool {
|
||||
let dot_codex = cwd.join(PROJECT_LOCAL_CODEX_DIR);
|
||||
let dot_codex_is_missing = match dot_codex.symlink_metadata() {
|
||||
Ok(_) => false,
|
||||
Err(err) if err.kind() == ErrorKind::NotFound => true,
|
||||
Err(_) => false,
|
||||
};
|
||||
|
||||
dot_codex_is_missing
|
||||
&& file_system_sandbox_policy.can_write_path_with_cwd(cwd, cwd)
|
||||
&& !file_system_sandbox_policy.can_write_path_with_cwd(&dot_codex, cwd)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(crate) fn test_config() -> Config {
|
||||
let codex_home = tempfile::tempdir().expect("create temp dir");
|
||||
@@ -2642,6 +2679,11 @@ impl Config {
|
||||
} else {
|
||||
NetworkSandboxPolicy::from(&effective_sandbox_policy)
|
||||
};
|
||||
maybe_warn_missing_project_dot_codex_for_linux_bwrap(
|
||||
&mut startup_warnings,
|
||||
&effective_file_system_sandbox_policy,
|
||||
resolved_cwd.as_path(),
|
||||
);
|
||||
let config = Self {
|
||||
model,
|
||||
service_tier,
|
||||
|
||||
@@ -313,15 +313,10 @@ async fn sandbox_distinguishes_command_and_policy_cwds() {
|
||||
assert!(allowed_exists, "allowed path should exist");
|
||||
}
|
||||
|
||||
#[cfg(target_os = "macos")]
|
||||
#[tokio::test]
|
||||
async fn sandbox_blocks_first_time_dot_codex_creation() {
|
||||
core_test_support::skip_if_sandbox!();
|
||||
#[cfg(target_os = "linux")]
|
||||
let sandbox_env = match linux_sandbox_test_env().await {
|
||||
Some(env) => env,
|
||||
None => return,
|
||||
};
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
let sandbox_env = HashMap::new();
|
||||
|
||||
let temp = tempfile::tempdir().expect("should be able to create temp dir");
|
||||
|
||||
@@ -344,7 +344,12 @@ fn create_filesystem_args(
|
||||
.collect();
|
||||
read_only_subpaths.sort_by_key(|path| path_depth(path));
|
||||
for subpath in read_only_subpaths {
|
||||
append_read_only_subpath_args(&mut args, &subpath, &allowed_write_paths);
|
||||
append_read_only_subpath_args(
|
||||
&mut args,
|
||||
&mut preserved_files,
|
||||
&subpath,
|
||||
&allowed_write_paths,
|
||||
)?;
|
||||
}
|
||||
let mut nested_unreadable_roots: Vec<PathBuf> = unreadable_roots
|
||||
.iter()
|
||||
@@ -424,25 +429,20 @@ fn append_mount_target_parent_dir_args(args: &mut Vec<String>, mount_target: &Pa
|
||||
|
||||
fn append_read_only_subpath_args(
|
||||
args: &mut Vec<String>,
|
||||
preserved_files: &mut Vec<File>,
|
||||
subpath: &Path,
|
||||
allowed_write_paths: &[PathBuf],
|
||||
) {
|
||||
) -> Result<()> {
|
||||
if let Some(symlink_path) = find_symlink_in_path(subpath, allowed_write_paths) {
|
||||
args.push("--ro-bind".to_string());
|
||||
args.push("/dev/null".to_string());
|
||||
args.push(path_to_string(&symlink_path));
|
||||
return;
|
||||
append_empty_file_read_only_bind_args(args, preserved_files, &symlink_path)?;
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
if !subpath.exists() {
|
||||
if let Some(first_missing_component) = find_first_non_existent_component(subpath)
|
||||
&& is_within_allowed_write_paths(&first_missing_component, allowed_write_paths)
|
||||
{
|
||||
args.push("--ro-bind".to_string());
|
||||
args.push("/dev/null".to_string());
|
||||
args.push(path_to_string(&first_missing_component));
|
||||
}
|
||||
return;
|
||||
// Bubblewrap must create bind targets before mounting over them. If the
|
||||
// missing path lives under a writable host bind, that target creation
|
||||
// leaks a real host-side placeholder or fails before command startup.
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
if is_within_allowed_write_paths(subpath, allowed_write_paths) {
|
||||
@@ -450,6 +450,8 @@ fn append_read_only_subpath_args(
|
||||
args.push(path_to_string(subpath));
|
||||
args.push(path_to_string(subpath));
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn append_unreadable_root_args(
|
||||
@@ -459,9 +461,7 @@ fn append_unreadable_root_args(
|
||||
allowed_write_paths: &[PathBuf],
|
||||
) -> Result<()> {
|
||||
if let Some(symlink_path) = find_symlink_in_path(unreadable_root, allowed_write_paths) {
|
||||
args.push("--ro-bind".to_string());
|
||||
args.push("/dev/null".to_string());
|
||||
args.push(path_to_string(&symlink_path));
|
||||
append_empty_file_read_only_bind_args(args, preserved_files, &symlink_path)?;
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
@@ -469,9 +469,7 @@ fn append_unreadable_root_args(
|
||||
if let Some(first_missing_component) = find_first_non_existent_component(unreadable_root)
|
||||
&& is_within_allowed_write_paths(&first_missing_component, allowed_write_paths)
|
||||
{
|
||||
args.push("--ro-bind".to_string());
|
||||
args.push("/dev/null".to_string());
|
||||
args.push(path_to_string(&first_missing_component));
|
||||
append_empty_file_read_only_bind_args(args, preserved_files, &first_missing_component)?;
|
||||
}
|
||||
return Ok(());
|
||||
}
|
||||
@@ -506,15 +504,24 @@ fn append_unreadable_root_args(
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
args.push("--perms".to_string());
|
||||
args.push("000".to_string());
|
||||
append_empty_file_read_only_bind_args(args, preserved_files, unreadable_root)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn append_empty_file_read_only_bind_args(
|
||||
args: &mut Vec<String>,
|
||||
preserved_files: &mut Vec<File>,
|
||||
mount_target: &Path,
|
||||
) -> Result<()> {
|
||||
if preserved_files.is_empty() {
|
||||
preserved_files.push(File::open("/dev/null")?);
|
||||
}
|
||||
let null_fd = preserved_files[0].as_raw_fd().to_string();
|
||||
args.push("--perms".to_string());
|
||||
args.push("000".to_string());
|
||||
args.push("--ro-bind-data".to_string());
|
||||
args.push(null_fd);
|
||||
args.push(path_to_string(unreadable_root));
|
||||
args.push(path_to_string(mount_target));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -529,7 +536,7 @@ fn is_within_allowed_write_paths(path: &Path, allowed_write_paths: &[PathBuf]) -
|
||||
///
|
||||
/// This blocks symlink replacement attacks where a protected path is a symlink
|
||||
/// inside a writable root (e.g., `.codex -> ./decoy`). In that case we mount
|
||||
/// `/dev/null` on the symlink itself to prevent rewiring it.
|
||||
/// an inherited empty file on the symlink itself to prevent rewiring it.
|
||||
fn find_symlink_in_path(target_path: &Path, allowed_write_paths: &[PathBuf]) -> Option<PathBuf> {
|
||||
let mut current = PathBuf::new();
|
||||
|
||||
@@ -566,8 +573,8 @@ fn find_symlink_in_path(target_path: &Path, allowed_write_paths: &[PathBuf]) ->
|
||||
|
||||
/// Find the first missing path component while walking `target_path`.
|
||||
///
|
||||
/// Mounting `/dev/null` on the first missing component prevents the sandboxed
|
||||
/// process from creating the protected path hierarchy.
|
||||
/// Mounting an inherited empty file on the first missing component prevents the
|
||||
/// sandboxed process from creating the protected path hierarchy.
|
||||
fn find_first_non_existent_component(target_path: &Path) -> Option<PathBuf> {
|
||||
let mut current = PathBuf::new();
|
||||
|
||||
@@ -767,33 +774,35 @@ mod tests {
|
||||
Path::new("/"),
|
||||
)
|
||||
.expect("bwrap fs args");
|
||||
assert_eq!(args.preserved_files.len(), 0);
|
||||
assert_eq!(
|
||||
args.args,
|
||||
vec![
|
||||
&args.args[..8],
|
||||
[
|
||||
// Start from a read-only view of the full filesystem.
|
||||
"--ro-bind".to_string(),
|
||||
"/".to_string(),
|
||||
"/".to_string(),
|
||||
"--ro-bind",
|
||||
"/",
|
||||
"/",
|
||||
// Recreate a writable /dev inside the sandbox.
|
||||
"--dev".to_string(),
|
||||
"/dev".to_string(),
|
||||
"--dev",
|
||||
"/dev",
|
||||
// Make the writable root itself writable again.
|
||||
"--bind".to_string(),
|
||||
"/".to_string(),
|
||||
"/".to_string(),
|
||||
// Mask the default protected .codex subpath under that writable
|
||||
// root. Because the root is `/` in this test, the carveout path
|
||||
// appears as `/.codex`.
|
||||
"--ro-bind".to_string(),
|
||||
"/dev/null".to_string(),
|
||||
"/.codex".to_string(),
|
||||
// Rebind /dev after the root bind so device nodes remain
|
||||
// writable/usable inside the writable root.
|
||||
"--bind".to_string(),
|
||||
"/dev".to_string(),
|
||||
"/dev".to_string(),
|
||||
"--bind",
|
||||
"/",
|
||||
"/",
|
||||
]
|
||||
);
|
||||
assert!(
|
||||
!args.args.iter().any(|arg| arg == "/.codex"),
|
||||
"missing protected .codex should not be masked under bwrap: {:#?}",
|
||||
args.args
|
||||
);
|
||||
assert!(
|
||||
args.args
|
||||
.windows(3)
|
||||
.any(|window| window == ["--bind", "/dev", "/dev"]),
|
||||
"expected /dev to be rebound after the writable root: {:#?}",
|
||||
args.args
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -21,6 +21,7 @@ use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use tempfile::NamedTempFile;
|
||||
|
||||
@@ -81,6 +82,26 @@ async fn run_cmd_result_with_writable_roots(
|
||||
timeout_ms: u64,
|
||||
use_legacy_landlock: bool,
|
||||
network_access: bool,
|
||||
) -> Result<codex_protocol::exec_output::ExecToolCallOutput> {
|
||||
let cwd = std::env::current_dir().expect("cwd should exist");
|
||||
run_cmd_result_with_writable_roots_in_cwd(
|
||||
cmd,
|
||||
writable_roots,
|
||||
cwd.as_path(),
|
||||
timeout_ms,
|
||||
use_legacy_landlock,
|
||||
network_access,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn run_cmd_result_with_writable_roots_in_cwd(
|
||||
cmd: &[&str],
|
||||
writable_roots: &[PathBuf],
|
||||
cwd: &Path,
|
||||
timeout_ms: u64,
|
||||
use_legacy_landlock: bool,
|
||||
network_access: bool,
|
||||
) -> Result<codex_protocol::exec_output::ExecToolCallOutput> {
|
||||
let sandbox_policy = SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: writable_roots
|
||||
@@ -97,11 +118,12 @@ async fn run_cmd_result_with_writable_roots(
|
||||
};
|
||||
let file_system_sandbox_policy = FileSystemSandboxPolicy::from(&sandbox_policy);
|
||||
let network_sandbox_policy = NetworkSandboxPolicy::from(&sandbox_policy);
|
||||
run_cmd_result_with_policies(
|
||||
run_cmd_result_with_policies_in_cwd(
|
||||
cmd,
|
||||
sandbox_policy,
|
||||
file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
cwd,
|
||||
timeout_ms,
|
||||
use_legacy_landlock,
|
||||
)
|
||||
@@ -118,6 +140,29 @@ async fn run_cmd_result_with_policies(
|
||||
use_legacy_landlock: bool,
|
||||
) -> Result<codex_protocol::exec_output::ExecToolCallOutput> {
|
||||
let cwd = std::env::current_dir().expect("cwd should exist");
|
||||
run_cmd_result_with_policies_in_cwd(
|
||||
cmd,
|
||||
sandbox_policy,
|
||||
file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
cwd.as_path(),
|
||||
timeout_ms,
|
||||
use_legacy_landlock,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
#[expect(clippy::expect_used)]
|
||||
async fn run_cmd_result_with_policies_in_cwd(
|
||||
cmd: &[&str],
|
||||
sandbox_policy: SandboxPolicy,
|
||||
file_system_sandbox_policy: FileSystemSandboxPolicy,
|
||||
network_sandbox_policy: NetworkSandboxPolicy,
|
||||
cwd: &Path,
|
||||
timeout_ms: u64,
|
||||
use_legacy_landlock: bool,
|
||||
) -> Result<codex_protocol::exec_output::ExecToolCallOutput> {
|
||||
let cwd = cwd.to_path_buf();
|
||||
let sandbox_cwd = cwd.clone();
|
||||
let params = ExecParams {
|
||||
command: cmd.iter().copied().map(str::to_owned).collect(),
|
||||
@@ -258,6 +303,40 @@ async fn bwrap_populates_minimal_dev_nodes() {
|
||||
assert_eq!(output.exit_code, 0);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn bwrap_dev_nodes_work_when_workspace_dot_codex_is_missing() {
|
||||
if should_skip_bwrap_tests().await {
|
||||
eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable");
|
||||
return;
|
||||
}
|
||||
|
||||
let tmpdir = tempfile::tempdir().expect("tempdir");
|
||||
let output = run_cmd_result_with_writable_roots_in_cwd(
|
||||
&[
|
||||
"bash",
|
||||
"-lc",
|
||||
": >/dev/null && head -c 8 /dev/zero | od -An -tx1",
|
||||
],
|
||||
&[],
|
||||
tmpdir.path(),
|
||||
LONG_TIMEOUT_MS,
|
||||
false,
|
||||
true,
|
||||
)
|
||||
.await
|
||||
.expect("sandboxed command should execute");
|
||||
|
||||
assert_eq!(output.exit_code, 0);
|
||||
assert_eq!(
|
||||
output.stdout.text.split_whitespace().collect::<Vec<_>>(),
|
||||
vec!["00", "00", "00", "00", "00", "00", "00", "00"]
|
||||
);
|
||||
assert!(
|
||||
!tmpdir.path().join(".codex").exists(),
|
||||
"missing workspace .codex should not be materialized by bwrap startup"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn bwrap_preserves_writable_dev_shm_bind_mount() {
|
||||
if should_skip_bwrap_tests().await {
|
||||
|
||||
Reference in New Issue
Block a user