From 9ca99b51713066681cce211184c0ff9035496979 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 19 Aug 2026 22:26:00 +0000 Subject: [PATCH] Preserve parent repository discovery through sandbox metadata mounts (#39629) ## Why The Linux sandbox represents missing protected metadata paths such as `.git` with empty read-only directories. Repository and project discovery treated any `.git` directory as a checkout root, so this synthetic directory could hide a real parent repository and its trust configuration. ## What changed - Treat a `.git` directory as repository metadata only when it contains `HEAD`, while continuing to recognize file-based `.git` entries. - Protect missing `.git` paths with the same read-only synthetic mounts as other workspace metadata without disrupting parent-repository discovery. - Canonicalize and read-only bind the synthetic-mount registry into the sandbox, and make protected-path cleanup handle read-only directory trees. ## Testing Added coverage for repository, project-root, trust, synthetic-mount, and Landlock behavior, including nested incomplete `.git` directories and a redirected `TMPDIR`. GitOrigin-RevId: 0724c54d56531143bb28011e4ca414cd3b0212be --- .../app-server/tests/suite/v2/config_rpc.rs | 1 + .../app-server/tests/suite/v2/thread_start.rs | 3 +- .../loader/managed_project_discovery_tests.rs | 30 +++++++ codex-rs/config/src/loader/mod.rs | 37 ++++++--- .../core/src/config/config_loader_tests.rs | 1 + codex-rs/core/src/config/config_tests.rs | 40 +++++++++ codex-rs/core/src/git_info_tests.rs | 10 ++- codex-rs/core/src/turn_metadata_tests.rs | 5 ++ codex-rs/git-utils/src/info.rs | 2 +- codex-rs/git-utils/src/trust.rs | 38 ++++++--- codex-rs/linux-sandbox/src/bwrap.rs | 81 ++++--------------- codex-rs/linux-sandbox/src/linux_run_main.rs | 76 ++++++++++++++--- .../linux-sandbox/src/linux_run_main_tests.rs | 54 ++++++++++--- .../linux-sandbox/tests/suite/landlock.rs | 42 ++++++++-- codex-rs/tui/src/config_update_tests.rs | 1 + 15 files changed, 305 insertions(+), 116 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/config_rpc.rs b/codex-rs/app-server/tests/suite/v2/config_rpc.rs index 809d5fa28f..c0b0a63a44 100644 --- a/codex-rs/app-server/tests/suite/v2/config_rpc.rs +++ b/codex-rs/app-server/tests/suite/v2/config_rpc.rs @@ -871,6 +871,7 @@ async fn config_read_respects_managed_project_root_markers() -> Result<()> { ] { std::fs::create_dir_all(dir)?; } + std::fs::write(workspace.path().join(".git/HEAD"), "ref: refs/heads/main\n")?; std::fs::write( ancestor_config.join("config.toml"), "model_context_window = 32768\n", diff --git a/codex-rs/app-server/tests/suite/v2/thread_start.rs b/codex-rs/app-server/tests/suite/v2/thread_start.rs index 03f9122b84..9f62e8e701 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_start.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_start.rs @@ -1640,8 +1640,9 @@ async fn thread_start_with_nested_git_cwd_respects_effective_permissions_for_pro let repo_root = TempDir::new()?; std::fs::create_dir(repo_root.path().join(".git"))?; + std::fs::write(repo_root.path().join(".git/HEAD"), "ref: refs/heads/main\n")?; let nested = repo_root.path().join("nested/project"); - std::fs::create_dir_all(&nested)?; + std::fs::create_dir_all(nested.join(".git"))?; let mut mcp = TestAppServer::builder() .with_codex_home(codex_home.path()) diff --git a/codex-rs/config/src/loader/managed_project_discovery_tests.rs b/codex-rs/config/src/loader/managed_project_discovery_tests.rs index c74d8f7650..4ce8b846e0 100644 --- a/codex-rs/config/src/loader/managed_project_discovery_tests.rs +++ b/codex-rs/config/src/loader/managed_project_discovery_tests.rs @@ -33,6 +33,7 @@ impl Fixture { let cwd = project.join("child"); std::fs::create_dir_all(&home)?; std::fs::create_dir_all(repo.join(".git"))?; + std::fs::write(repo.join(".git/HEAD"), "ref: refs/heads/main\n")?; for (dir, model) in [(&repo, "ancestor"), (&project, "project"), (&cwd, "child")] { std::fs::create_dir_all(dir.join(".codex"))?; std::fs::write( @@ -181,6 +182,35 @@ async fn managed_project_discovery_uses_file_markers_without_changing_precedence Ok(()) } +#[tokio::test] +async fn managed_project_discovery_ignores_incomplete_nested_git_directory() -> anyhow::Result<()> { + let fixture = Fixture::new()?; + std::fs::create_dir(fixture.cwd.join(".git"))?; + std::fs::write(&fixture.managed, "project_root_markers = [\".git\"]\n")?; + + let (canonical, local) = fixture.load().await?; + assert_discovery( + &canonical, + &local, + &[&fixture.repo, &fixture.project, &fixture.cwd], + &[".git"], + )?; + + std::fs::write(fixture.home.join("config.toml"), fixture.trust("untrusted"))?; + let (canonical, local) = fixture.load().await?; + assert_discovery(&canonical, &local, &[], &[".git"])?; + let trust_key = project_trust_key(fixture.repo.as_path()); + assert!( + canonical + .all_layers_low_to_high() + .filter(|layer| matches!(layer.name, ConfigLayerSource::Project { .. })) + .all(|layer| layer.disabled_reason.as_deref().is_some_and( + |reason| reason.starts_with(&format!("{trust_key} is marked as untrusted")) + )) + ); + Ok(()) +} + #[tokio::test] async fn managed_project_discovery_uses_managed_project_trust() -> anyhow::Result<()> { let fixture = Fixture::new()?; diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index f18c74c5ae..85f4352665 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -1315,13 +1315,22 @@ async fn find_project_root( for marker in project_root_markers { let marker_path = ancestor.join(marker); let marker_path_uri = PathUri::from_abs_path(&marker_path); - if fs - .get_metadata(&marker_path_uri, /*sandbox*/ None) - .await - .is_ok() + let Ok(metadata) = fs.get_metadata(&marker_path_uri, /*sandbox*/ None).await else { + continue; + }; + if marker == ".git" + && metadata.is_directory + && fs + .get_metadata( + &PathUri::from_abs_path(&marker_path.join("HEAD")), + /*sandbox*/ None, + ) + .await + .is_err() { - return Ok(ancestor); + continue; } + return Ok(ancestor); } } Ok(cwd.clone()) @@ -1340,13 +1349,21 @@ async fn find_git_checkout_root( for dir in base.ancestors() { let dot_git = dir.join(".git"); let dot_git_uri = PathUri::from_abs_path(&dot_git); - if fs - .get_metadata(&dot_git_uri, /*sandbox*/ None) - .await - .is_ok() + let Ok(metadata) = fs.get_metadata(&dot_git_uri, /*sandbox*/ None).await else { + continue; + }; + if metadata.is_directory + && fs + .get_metadata( + &PathUri::from_abs_path(&dot_git.join("HEAD")), + /*sandbox*/ None, + ) + .await + .is_err() { - return Some(dir); + continue; } + return Some(dir); } None } diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index 14247163f4..84f7a2667d 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -3295,6 +3295,7 @@ async fn codex_home_within_project_tree_is_not_double_loaded() -> std::io::Resul tokio::fs::create_dir_all(&nested_dot_codex).await?; tokio::fs::create_dir_all(project_root.join(".git")).await?; + tokio::fs::write(project_root.join(".git/HEAD"), "ref: refs/heads/main\n").await?; tokio::fs::write( nested_dot_codex.join(CONFIG_TOML_FILE), r#"foo = "child" diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index 4dea8dafac..48d2ea64b0 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -8285,6 +8285,7 @@ async fn agent_role_file_without_developer_instructions_is_dropped_with_warning( let repo_root = TempDir::new()?; let nested_cwd = repo_root.path().join("packages").join("app"); std::fs::create_dir_all(repo_root.path().join(".git"))?; + std::fs::write(repo_root.path().join(".git/HEAD"), "ref: refs/heads/main\n")?; std::fs::create_dir_all(&nested_cwd)?; let workspace_key = repo_root.path().to_string_lossy().replace('\\', "\\\\"); @@ -8456,6 +8457,7 @@ async fn discovered_agent_role_file_without_name_is_dropped_with_warning() -> st let repo_root = TempDir::new()?; let nested_cwd = repo_root.path().join("packages").join("app"); std::fs::create_dir_all(repo_root.path().join(".git"))?; + std::fs::write(repo_root.path().join(".git/HEAD"), "ref: refs/heads/main\n")?; std::fs::create_dir_all(&nested_cwd)?; let workspace_key = repo_root.path().to_string_lossy().replace('\\', "\\\\"); @@ -8656,6 +8658,7 @@ async fn discovers_multiple_standalone_agent_role_files() -> std::io::Result<()> let repo_root = TempDir::new()?; let nested_cwd = repo_root.path().join("packages").join("app"); std::fs::create_dir_all(repo_root.path().join(".git"))?; + std::fs::write(repo_root.path().join(".git/HEAD"), "ref: refs/heads/main\n")?; std::fs::create_dir_all(&nested_cwd)?; let workspace_key = repo_root.path().to_string_lossy().replace('\\', "\\\\"); @@ -8787,6 +8790,7 @@ async fn mixed_legacy_and_standalone_agent_role_sources_merge_with_precedence() let repo_root = TempDir::new()?; let nested_cwd = repo_root.path().join("packages").join("app"); std::fs::create_dir_all(repo_root.path().join(".git"))?; + std::fs::write(repo_root.path().join(".git/HEAD"), "ref: refs/heads/main\n")?; std::fs::create_dir_all(&nested_cwd)?; let workspace_key = repo_root.path().to_string_lossy().replace('\\', "\\\\"); @@ -8933,6 +8937,7 @@ async fn higher_precedence_agent_role_can_inherit_description_from_lower_layer() let repo_root = TempDir::new()?; let nested_cwd = repo_root.path().join("packages").join("app"); std::fs::create_dir_all(repo_root.path().join(".git"))?; + std::fs::write(repo_root.path().join(".git/HEAD"), "ref: refs/heads/main\n")?; std::fs::create_dir_all(&nested_cwd)?; let workspace_key = repo_root.path().to_string_lossy().replace('\\', "\\\\"); @@ -10341,6 +10346,41 @@ mcp_oauth_callback_url = "https://example.com/callback" Ok(()) } +#[tokio::test] +async fn untrusted_parent_repo_with_incomplete_child_git_keeps_unless_trusted_approval_policy() +-> anyhow::Result<()> { + let codex_home = TempDir::new()?; + let repo = TempDir::new()?; + let cwd = repo.path().join("sub"); + std::fs::create_dir_all(repo.path().join(".git"))?; + std::fs::write(repo.path().join(".git/HEAD"), "ref: refs/heads/main\n")?; + std::fs::create_dir_all(cwd.join(".git"))?; + + let config = Config::load_from_base_config_with_overrides( + ConfigToml { + projects: Some(HashMap::from([( + repo.path().to_string_lossy().to_string(), + ProjectConfig { + trust_level: Some(TrustLevel::Untrusted), + }, + )])), + ..Default::default() + }, + ConfigOverrides { + cwd: Some(cwd), + ..Default::default() + }, + codex_home.abs(), + ) + .await?; + + assert_eq!( + config.permissions.approval_policy.value(), + AskForApproval::UnlessTrusted + ); + Ok(()) +} + #[tokio::test] async fn test_untrusted_project_gets_unless_trusted_approval_policy() -> anyhow::Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/core/src/git_info_tests.rs b/codex-rs/core/src/git_info_tests.rs index a0b46d53ef..8adf5206e6 100644 --- a/codex-rs/core/src/git_info_tests.rs +++ b/codex-rs/core/src/git_info_tests.rs @@ -12,6 +12,7 @@ use codex_exec_server::RemoveOptions; use codex_git_utils::GitInfo; use codex_git_utils::GitSha; use codex_git_utils::collect_git_info; +use codex_git_utils::get_git_repo_root; use codex_git_utils::get_has_changes_in_repo; use codex_git_utils::git_diff_to_remote; use codex_git_utils::recent_commits; @@ -643,6 +644,7 @@ async fn resolve_root_git_project_for_trust_starts_at_parent_for_file() { let proj = tmp.path().join("proj"); let nested = proj.join("nested"); std::fs::create_dir_all(proj.join(".git")).unwrap(); + std::fs::write(proj.join(".git/HEAD"), "ref: refs/heads/main\n").unwrap(); std::fs::create_dir_all(&nested).unwrap(); let file = nested.join("file.txt"); std::fs::write(&file, "contents").unwrap(); @@ -659,6 +661,7 @@ async fn resolve_root_git_project_for_trust_ignores_metadata_errors() { let proj = tmp.path().join("proj"); let nested = proj.join("nested"); std::fs::create_dir_all(proj.join(".git")).unwrap(); + std::fs::write(proj.join(".git/HEAD"), "ref: refs/heads/main\n").unwrap(); std::fs::create_dir_all(&nested).unwrap(); let fs = MetadataOverrideFileSystem { path: PathUri::from_abs_path(&nested.join(".git").abs()), @@ -678,6 +681,7 @@ async fn resolve_root_git_project_for_trust_supports_windows_namespace_paths() { let tmp = TempDir::new().expect("tempdir"); let repo = tmp.path().join("repo"); std::fs::create_dir_all(repo.join(".git")).unwrap(); + std::fs::write(repo.join(".git/HEAD"), "ref: refs/heads/main\n").unwrap(); std::fs::create_dir_all(repo.join("nested")).unwrap(); let namespace_repo = PathBuf::from(format!(r"\\?\{}", repo.display())); @@ -699,7 +703,11 @@ async fn resolve_root_git_project_for_trust_regular_repo_returns_repo_root() { Some(repo_path.clone()) ); let nested = repo_path.join("sub/dir"); - std::fs::create_dir_all(nested.as_path()).unwrap(); + std::fs::create_dir_all(nested.join(".git")).unwrap(); + assert_eq!( + get_git_repo_root(nested.as_path()), + Some(repo_path.as_path().to_path_buf()) + ); assert_eq!( resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &nested).await, Some(repo_path) diff --git a/codex-rs/core/src/turn_metadata_tests.rs b/codex-rs/core/src/turn_metadata_tests.rs index 53bc1351fe..8ede6e492d 100644 --- a/codex-rs/core/src/turn_metadata_tests.rs +++ b/codex-rs/core/src/turn_metadata_tests.rs @@ -1109,6 +1109,11 @@ async fn turn_metadata_state_git_enrichment_cancellation_is_retryable_and_errors let invalid_repo = TempDir::new().expect("invalid repo"); std::fs::create_dir(invalid_repo.path().join(".git")).expect("invalid git directory"); + std::fs::write( + invalid_repo.path().join(".git/HEAD"), + "ref: refs/heads/main\n", + ) + .expect("invalid git HEAD"); let invalid_state = Arc::new(TurnMetadataState::new( "session-a".to_string(), "thread-a".to_string(), diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 02c79e4f86..be5c6680f3 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -767,7 +767,7 @@ fn find_ancestor_git_entry(base_dir: &Path) -> Option<(PathBuf, PathBuf)> { loop { let dot_git = dir.join(".git"); - if dot_git.exists() { + if dot_git.exists() && (!dot_git.is_dir() || dot_git.join("HEAD").exists()) { return Some((dir, dot_git)); } diff --git a/codex-rs/git-utils/src/trust.rs b/codex-rs/git-utils/src/trust.rs index 439defd7cf..b5c0150017 100644 --- a/codex-rs/git-utils/src/trust.rs +++ b/codex-rs/git-utils/src/trust.rs @@ -19,15 +19,35 @@ pub async fn resolve_root_git_project_for_trust( Ok(metadata) if metadata.is_directory => cwd.clone(), _ => cwd.parent()?, }; - let repo_root = find_nearest_native_ancestor_with_markers( - fs, - &base, - vec![".git".to_string()], - FindUpErrorPolicy::Ignore, - /*sandbox*/ None, - ) - .await - .ok()??; + let mut base = base; + let repo_root = loop { + let candidate = find_nearest_native_ancestor_with_markers( + fs, + &base, + vec![".git".to_string()], + FindUpErrorPolicy::Ignore, + /*sandbox*/ None, + ) + .await + .ok()??; + let dot_git = candidate.join(".git"); + let metadata = fs + .get_metadata(&PathUri::from_abs_path(&dot_git), /*sandbox*/ None) + .await + .ok()?; + if !metadata.is_directory + || fs + .get_metadata( + &PathUri::from_abs_path(&dot_git.join("HEAD")), + /*sandbox*/ None, + ) + .await + .is_ok() + { + break candidate; + } + base = candidate.parent()?; + }; let dot_git = repo_root.join(".git"); let dot_git_uri = PathUri::from_abs_path(&dot_git); let dot_git_metadata = fs.get_metadata(&dot_git_uri, /*sandbox*/ None).await.ok()?; diff --git a/codex-rs/linux-sandbox/src/bwrap.rs b/codex-rs/linux-sandbox/src/bwrap.rs index ff46583762..097e689e84 100644 --- a/codex-rs/linux-sandbox/src/bwrap.rs +++ b/codex-rs/linux-sandbox/src/bwrap.rs @@ -592,7 +592,6 @@ fn create_filesystem_args( append_metadata_path_masks_for_writable_root( &mut read_only_subpaths, root, - mount_root, &protected_metadata_names, ); if let Some(target) = &symlink_target { @@ -668,48 +667,16 @@ fn append_protected_create_targets_for_writable_root( fn append_metadata_path_masks_for_writable_root( read_only_subpaths: &mut Vec, root: &Path, - mount_root: &Path, protected_metadata_names: &[String], ) { for name in protected_metadata_names { let path = root.join(name); - if should_leave_missing_git_for_parent_repo_discovery(mount_root, name) { - continue; - } if !read_only_subpaths.iter().any(|subpath| subpath == &path) { read_only_subpaths.push(path); } } } -fn should_leave_missing_git_for_parent_repo_discovery(mount_root: &Path, name: &str) -> bool { - let path = mount_root.join(name); - name == ".git" - && matches!( - path.symlink_metadata(), - Err(err) if err.kind() == io::ErrorKind::NotFound - ) - && mount_root - .ancestors() - .skip(1) - .any(ancestor_has_git_metadata) -} - -fn ancestor_has_git_metadata(ancestor: &Path) -> bool { - let git_path = ancestor.join(".git"); - let Ok(metadata) = git_path.symlink_metadata() else { - return false; - }; - if metadata.is_dir() { - return git_path.join("HEAD").symlink_metadata().is_ok(); - } - if metadata.is_file() { - return fs::read_to_string(git_path) - .is_ok_and(|contents| contents.trim_start().starts_with("gitdir:")); - } - false -} - fn expand_unreadable_globs_with_ripgrep( patterns: &[String], cwd: &Path, @@ -1814,7 +1781,7 @@ mod tests { } #[test] - fn missing_child_git_under_parent_repo_uses_protected_create_target() { + fn missing_child_git_under_parent_repo_is_mounted_read_only() { let temp_dir = TempDir::new().expect("temp dir"); let repo = temp_dir.path().join("repo"); let workspace = repo.join("workspace"); @@ -1833,30 +1800,22 @@ mod tests { let args = create_filesystem_args(&policy, &workspace, NO_UNREADABLE_GLOB_SCAN_MAX_DEPTH) .expect("filesystem args"); + assert_empty_directory_mounted_read_only(&args.args, &dot_git); assert_empty_directory_mounted_read_only(&args.args, &workspace.join(".agents")); assert_empty_directory_mounted_read_only(&args.args, &workspace.join(".codex")); - let dot_git_str = path_to_string(&dot_git); - assert!( - !args - .args - .windows(4) - .any(|window| window == ["--perms", "555", "--tmpfs", dot_git_str.as_str()]), - "missing child .git should not shadow parent repo discovery", - ); - assert!( - !synthetic_mount_target_paths(&args).contains(&dot_git), - "missing child .git should not be a transient mount target", - ); assert_eq!( - protected_create_target_paths(&args), - vec![dot_git], - "missing child .git should fail through protected create cleanup", + synthetic_mount_target_paths(&args), + vec![workspace.join(".codex"), dot_git, workspace.join(".agents")], + ); + assert!( + protected_create_target_paths(&args).is_empty(), + "missing child .git should be mount protected before command execution", ); } #[cfg(unix)] #[test] - fn symlinked_missing_child_git_under_parent_repo_uses_effective_mount_root() { + fn symlinked_missing_child_git_under_parent_repo_mounts_effective_path_read_only() { let temp_dir = TempDir::new().expect("temp dir"); let repo = temp_dir.path().join("repo"); let workspace = repo.join("workspace"); @@ -1879,24 +1838,16 @@ mod tests { let args = create_filesystem_args(&policy, &link_workspace, NO_UNREADABLE_GLOB_SCAN_MAX_DEPTH) .expect("filesystem args"); + assert_empty_directory_mounted_read_only(&args.args, &dot_git); assert_empty_directory_mounted_read_only(&args.args, &workspace.join(".agents")); assert_empty_directory_mounted_read_only(&args.args, &workspace.join(".codex")); - let dot_git_str = path_to_string(&dot_git); - assert!( - !args - .args - .windows(4) - .any(|window| window == ["--perms", "555", "--tmpfs", dot_git_str.as_str()]), - "symlinked missing child .git should not shadow parent repo discovery", - ); - assert!( - !synthetic_mount_target_paths(&args).contains(&dot_git), - "symlinked missing child .git should not be a transient mount target", - ); assert_eq!( - protected_create_target_paths(&args), - vec![dot_git], - "symlinked missing child .git should fail through protected create cleanup", + synthetic_mount_target_paths(&args), + vec![workspace.join(".codex"), dot_git, workspace.join(".agents")], + ); + assert!( + protected_create_target_paths(&args).is_empty(), + "symlinked missing child .git should be mount protected before command execution", ); } diff --git a/codex-rs/linux-sandbox/src/linux_run_main.rs b/codex-rs/linux-sandbox/src/linux_run_main.rs index 4228c71ea3..abedd97671 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main.rs @@ -8,9 +8,12 @@ use std::io::Read; use std::os::fd::AsRawFd; use std::os::fd::FromRawFd; use std::os::unix::ffi::OsStrExt; +use std::os::unix::fs::OpenOptionsExt; +use std::os::unix::fs::PermissionsExt; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; +use std::sync::OnceLock; use std::sync::atomic::AtomicBool; use std::sync::atomic::AtomicI32; use std::sync::atomic::Ordering; @@ -564,7 +567,7 @@ fn run_or_exec_bwrap(bwrap_args: crate::bwrap::BwrapArgs) -> ! { fn run_bwrap_in_child_with_synthetic_mount_cleanup(bwrap_args: crate::bwrap::BwrapArgs) -> ! { let crate::bwrap::BwrapArgs { - args, + mut args, preserved_files, synthetic_mount_targets, protected_create_targets, @@ -573,6 +576,20 @@ fn run_bwrap_in_child_with_synthetic_mount_cleanup(bwrap_args: crate::bwrap::Bwr let synthetic_mount_registrations = register_synthetic_mount_targets(&synthetic_mount_targets); let protected_create_registrations = register_protected_create_targets(&protected_create_targets); + let registry_root = synthetic_mount_registry_root() + .to_string_lossy() + .into_owned(); + let Some(command_separator) = args.iter().position(|arg| arg == "--") else { + panic!("bubblewrap argv is missing command separator '--'"); + }; + args.splice( + command_separator..command_separator, + [ + "--ro-bind".to_string(), + registry_root.clone(), + registry_root, + ], + ); let exec_start_pipe = create_exec_start_pipe(!protected_create_targets.is_empty()); let parent_pid = unsafe { libc::getpid() }; let pid = unsafe { libc::fork() }; @@ -1136,12 +1153,6 @@ fn cleanup_protected_create_targets(targets: &[ProtectedCreateTargetRegistration let mut violation = false; for target in targets.iter().rev() { - if synthetic_mount_marker_dir_has_active_process(&target.marker_dir) { - if target.target.path().exists() { - violation = true; - } - continue; - } violation |= remove_protected_create_target(&target.target); match fs::remove_dir(&target.marker_dir) { Ok(()) => {} @@ -1206,7 +1217,13 @@ fn try_remove_protected_create_target( ProtectedCreateRemoval::Other }; let result = if removal == ProtectedCreateRemoval::Directory { - fs::remove_dir_all(path) + match fs::remove_dir_all(path) { + Err(err) if err.kind() == std::io::ErrorKind::PermissionDenied => { + make_directory_tree_writable(path)?; + fs::remove_dir_all(path) + } + result => result, + } } else { fs::remove_file(path) }; @@ -1222,6 +1239,28 @@ fn try_remove_protected_create_target( Ok(Some(removal)) } +fn make_directory_tree_writable(path: &Path) -> std::io::Result<()> { + let metadata = match fs::symlink_metadata(path) { + Ok(metadata) => metadata, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(err) => return Err(err), + }; + if !metadata.is_dir() { + return Ok(()); + } + + let directory = OpenOptions::new() + .read(true) + .custom_flags(libc::O_PATH | libc::O_DIRECTORY | libc::O_NOFOLLOW) + .open(path)?; + let directory_path = PathBuf::from(format!("/proc/self/fd/{}", directory.as_raw_fd())); + fs::set_permissions(&directory_path, fs::Permissions::from_mode(0o700))?; + for entry in fs::read_dir(directory_path)? { + make_directory_tree_writable(&entry?.path())?; + } + Ok(()) +} + fn remove_synthetic_mount_target(target: &crate::bwrap::SyntheticMountTarget) { let path = target.path(); let metadata = match fs::symlink_metadata(path) { @@ -1309,10 +1348,23 @@ fn synthetic_mount_marker_dir(path: &Path) -> PathBuf { } fn synthetic_mount_registry_root() -> PathBuf { - let effective_uid = unsafe { libc::geteuid() }; - std::env::temp_dir().join(format!( - "codex-bwrap-synthetic-mount-targets-{effective_uid}" - )) + static REGISTRY_ROOT: OnceLock = OnceLock::new(); + + REGISTRY_ROOT + .get_or_init(|| { + let effective_uid = unsafe { libc::geteuid() }; + let temp_dir = std::env::temp_dir(); + let temp_dir = temp_dir.canonicalize().unwrap_or_else(|err| { + panic!( + "failed to resolve synthetic mount registry temp directory {}: {err}", + temp_dir.display() + ) + }); + temp_dir.join(format!( + "codex-bwrap-synthetic-mount-targets-{effective_uid}" + )) + }) + .clone() } fn hash_path(path: &Path) -> u64 { diff --git a/codex-rs/linux-sandbox/src/linux_run_main_tests.rs b/codex-rs/linux-sandbox/src/linux_run_main_tests.rs index c38b51ae79..e6fe9b3134 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main_tests.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main_tests.rs @@ -14,6 +14,8 @@ use codex_protocol::protocol::NetworkSandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; #[cfg(test)] use pretty_assertions::assert_eq; +#[cfg(test)] +use std::os::unix::fs::PermissionsExt; fn read_only_permission_profile() -> PermissionProfile { PermissionProfile::read_only() @@ -326,31 +328,34 @@ fn synthetic_mount_registry_root_is_unique_to_effective_user() { let effective_uid = unsafe { libc::geteuid() }; assert_eq!( synthetic_mount_registry_root(), - std::env::temp_dir().join(format!( - "codex-bwrap-synthetic-mount-targets-{effective_uid}" - )) + std::env::temp_dir() + .canonicalize() + .expect("resolve temp directory") + .join(format!( + "codex-bwrap-synthetic-mount-targets-{effective_uid}" + )) ); } #[test] fn cleanup_synthetic_mount_targets_waits_for_other_active_registrations() { let temp_dir = tempfile::TempDir::new().expect("tempdir"); - let empty_file = temp_dir.path().join(".git"); - std::fs::write(&empty_file, "").expect("write empty file"); - let target = crate::bwrap::SyntheticMountTarget::missing(&empty_file); + let empty_dir = temp_dir.path().join(".git"); + std::fs::create_dir(&empty_dir).expect("create empty dir"); + let target = crate::bwrap::SyntheticMountTarget::missing_empty_directory(&empty_dir); let registrations = register_synthetic_mount_targets(std::slice::from_ref(&target)); let active_marker = registrations[0].marker_dir.join("1"); std::fs::write(&active_marker, "").expect("write active marker"); cleanup_synthetic_mount_targets(®istrations); - assert!(empty_file.exists()); + assert!(empty_dir.exists()); std::fs::remove_file(active_marker).expect("remove active marker"); let registrations = register_synthetic_mount_targets(std::slice::from_ref(&target)); cleanup_synthetic_mount_targets(®istrations); - assert!(!empty_file.exists()); + assert!(!empty_dir.exists()); } #[test] @@ -412,7 +417,7 @@ fn cleanup_protected_create_targets_removes_created_path_and_reports_violation() } #[test] -fn cleanup_protected_create_targets_waits_for_other_active_registrations() { +fn cleanup_protected_create_targets_removes_path_despite_active_marker() { let temp_dir = tempfile::TempDir::new().expect("tempdir"); let dot_git = temp_dir.path().join(".git"); let target = crate::bwrap::ProtectedCreateTarget::missing(&dot_git); @@ -424,14 +429,39 @@ fn cleanup_protected_create_targets_waits_for_other_active_registrations() { let violation = cleanup_protected_create_targets(®istrations); assert!(violation); - assert!(dot_git.exists()); + assert!(!dot_git.exists()); +} + +#[test] +fn cleanup_protected_create_targets_removes_read_only_directory_and_reports_violation() { + let temp_dir = tempfile::TempDir::new().expect("tempdir"); + let dot_git = temp_dir.path().join(".git"); + let outside = temp_dir.path().join("outside"); + let target = crate::bwrap::ProtectedCreateTarget::missing(&dot_git); + + let registrations = register_protected_create_targets(&[target]); + std::fs::create_dir(&outside).expect("create outside directory"); + std::fs::set_permissions(&outside, std::fs::Permissions::from_mode(0o755)) + .expect("set outside directory permissions"); + std::fs::create_dir(&dot_git).expect("create protected path"); + std::fs::write(dot_git.join("config"), "[core]\n").expect("write protected child"); + std::os::unix::fs::symlink(&outside, dot_git.join("outside-link")) + .expect("link outside directory"); + std::fs::set_permissions(&dot_git, std::fs::Permissions::from_mode(0o000)) + .expect("make protected path read-only"); - std::fs::remove_file(active_marker).expect("remove active marker"); - let registrations = register_protected_create_targets(std::slice::from_ref(&target)); let violation = cleanup_protected_create_targets(®istrations); assert!(violation); assert!(!dot_git.exists()); + assert_eq!( + std::fs::metadata(&outside) + .expect("outside directory remains") + .permissions() + .mode() + & 0o777, + 0o755 + ); } #[test] diff --git a/codex-rs/linux-sandbox/tests/suite/landlock.rs b/codex-rs/linux-sandbox/tests/suite/landlock.rs index 1ea852b629..794b1927eb 100644 --- a/codex-rs/linux-sandbox/tests/suite/landlock.rs +++ b/codex-rs/linux-sandbox/tests/suite/landlock.rs @@ -121,6 +121,7 @@ async fn run_cmd_result_with_permission_profile( cmd, cwd, permission_profile, + create_env_from_core_vars(), timeout_ms, use_legacy_landlock, ) @@ -154,6 +155,7 @@ async fn run_cmd_result_with_cwd_and_writable_roots( cmd, cwd, permission_profile, + create_env_from_core_vars(), timeout_ms, use_legacy_landlock, ) @@ -164,6 +166,7 @@ async fn run_cmd_result_with_permission_profile_for_cwd( cmd: &[&str], cwd: AbsolutePathBuf, permission_profile: PermissionProfile, + env: HashMap, timeout_ms: u64, use_legacy_landlock: bool, ) -> Result { @@ -173,7 +176,7 @@ async fn run_cmd_result_with_permission_profile_for_cwd( cwd, expiration: timeout_ms.into(), capture_policy: ExecCapturePolicy::ShellTool, - env: create_env_from_core_vars(), + env, network: None, network_environment_id: None, sandbox_permissions: SandboxPermissions::UseDefault, @@ -750,6 +753,12 @@ async fn sandbox_keeps_parent_repo_discovery_while_blocking_child_metadata() { let tmpdir = tempfile::tempdir().expect("tempdir"); let repo = tmpdir.path().join("repo"); let subdir = repo.join("sub"); + let real_tmp = tmpdir.path().join("real-tmp"); + let redirected_tmp = tmpdir.path().join("redirected-tmp"); + let tmp_alias = tmpdir.path().join("tmp-alias"); + std::fs::create_dir(&real_tmp).expect("create real temp directory"); + std::fs::create_dir(&redirected_tmp).expect("create redirected temp directory"); + std::os::unix::fs::symlink(&real_tmp, &tmp_alias).expect("create temp directory alias"); std::fs::create_dir_all(&subdir).expect("create nested workspace"); assert!( std::process::Command::new("git") @@ -763,9 +772,22 @@ async fn sandbox_keeps_parent_repo_discovery_while_blocking_child_metadata() { ); let repo = repo.to_string_lossy(); + let redirected_tmp = redirected_tmp.to_string_lossy(); let script = format!( r#"set -e test "$(git rev-parse --show-toplevel)" = '{repo}' +touch "$TMPDIR/writable-sibling" +registry="${{TMPDIR:-/tmp}}/codex-bwrap-synthetic-mount-targets-$(id -u)" +if touch "$registry/forged-marker" 2>/dev/null; then + exit 22 +fi +redirected_registry='{redirected_tmp}'/codex-bwrap-synthetic-mount-targets-$(id -u) +for marker_dir in "$registry"/*; do + [ -d "$marker_dir" ] || continue + mkdir -p "$redirected_registry/${{marker_dir##*/}}" + touch "$redirected_registry/${{marker_dir##*/}}/1" +done +ln -sfn '{redirected_tmp}' "$TMPDIR" git status --short > status.before if grep -E '(^|[[:space:]])\.(git|codex|agents)(/|$)' status.before; then cat status.before @@ -774,13 +796,22 @@ fi "#, ); - let output = run_cmd_result_with_cwd_and_writable_roots( + let cwd = AbsolutePathBuf::try_from(subdir.as_path()).expect("cwd should be absolute"); + let permission_profile = PermissionProfile::workspace_write_with( + std::slice::from_ref(&cwd), + NetworkSandboxPolicy::Enabled, + /*exclude_tmpdir_env_var*/ false, + /*exclude_slash_tmp*/ false, + ); + let mut env = create_env_from_core_vars(); + env.insert("TMPDIR".to_string(), tmp_alias.display().to_string()); + let output = run_cmd_result_with_permission_profile_for_cwd( &["bash", "-lc", &script], - &subdir, - std::slice::from_ref(&subdir), + cwd, + permission_profile, + env, LONG_TIMEOUT_MS, /*use_legacy_landlock*/ false, - /*network_access*/ true, ) .await .expect("sandboxed command should execute"); @@ -790,6 +821,7 @@ fi "stdout:\n{}\nstderr:\n{}", output.stdout.text, output.stderr.text ); + assert!(!subdir.join(".git").exists()); let git_init_output = expect_denied( run_cmd_result_with_cwd_and_writable_roots( diff --git a/codex-rs/tui/src/config_update_tests.rs b/codex-rs/tui/src/config_update_tests.rs index 1e9f588ad6..e7f2aada8a 100644 --- a/codex-rs/tui/src/config_update_tests.rs +++ b/codex-rs/tui/src/config_update_tests.rs @@ -41,6 +41,7 @@ async fn remote_project_trust_guards_thread_start_and_preserves_repository_decis std::fs::create_dir_all(&codex_home)?; std::fs::create_dir_all(&project_cwd)?; std::fs::create_dir(project_root.join(".git"))?; + std::fs::write(project_root.join(".git/HEAD"), "ref: refs/heads/main\n")?; std::fs::create_dir(project_cwd.join(".codex"))?; let undecided_config = format!( "[{}]\n",