From bc3545b805de6e91a11b88114fe1673b678633ca Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Wed, 19 Aug 2026 21:15:22 +0000 Subject: [PATCH] Validate linked worktrees before inheriting project trust (#39616) ## Why A checkout could point its `.git` file at a trusted repository's worktree directory without proving that the repository had registered that checkout. This could cause project configuration from an unrelated checkout to be treated as trusted. ## What changed - Verify the linked worktree's `gitdir` backlink, `commondir`, registered checkout, and main checkout ownership before resolving the main repository's trust key. - Reject missing, oversized, symlinked, mismatched, or swapped Git metadata. - Preserve valid linked worktrees that use path aliases, separate Git directories, or non-UTF-8 POSIX paths. ## Testing Add resolver and config-loading coverage for forged worktrees, metadata races, case-sensitive paths, moved worktrees, and host MCP startup from project config. GitOrigin-RevId: 6052a7d10ad2d613436f20175c356abdef8c758e --- .../app-server/tests/suite/v2/hooks_list.rs | 5 + .../core/src/config/config_loader_tests.rs | 61 +++- codex-rs/core/src/git_info_tests.rs | 156 ++++++++-- codex-rs/core/src/worktree_trust_tests.rs | 276 ++++++++++++++++++ codex-rs/core/tests/suite/mod.rs | 1 + codex-rs/core/tests/suite/worktree_trust.rs | 158 ++++++++++ codex-rs/git-utils/src/info.rs | 58 ---- codex-rs/git-utils/src/lib.rs | 3 +- codex-rs/git-utils/src/trust.rs | 143 +++++++++ codex-rs/utils/path-uri/src/lib.rs | 1 + .../utils/path-uri/src/native_path_bytes.rs | 51 ++++ codex-rs/utils/path-uri/src/tests.rs | 26 ++ 12 files changed, 859 insertions(+), 80 deletions(-) create mode 100644 codex-rs/core/src/worktree_trust_tests.rs create mode 100644 codex-rs/core/tests/suite/worktree_trust.rs create mode 100644 codex-rs/git-utils/src/trust.rs create mode 100644 codex-rs/utils/path-uri/src/native_path_bytes.rs diff --git a/codex-rs/app-server/tests/suite/v2/hooks_list.rs b/codex-rs/app-server/tests/suite/v2/hooks_list.rs index 634b79716d..a2285b1e2e 100644 --- a/codex-rs/app-server/tests/suite/v2/hooks_list.rs +++ b/codex-rs/app-server/tests/suite/v2/hooks_list.rs @@ -1032,6 +1032,11 @@ async fn hooks_list_uses_root_repo_hooks_for_linked_worktrees() -> Result<()> { worktree_root.join(".git"), format!("gitdir: {}\n", worktree_git_dir.display()), )?; + std::fs::write( + worktree_git_dir.join("gitdir"), + format!("{}\n", worktree_root.join(".git").display()), + )?; + std::fs::write(worktree_git_dir.join("commondir"), "../..\n")?; write_project_hook_config(&repo_root.join(".codex"), "echo root hook")?; write_project_hook_config(&worktree_root.join(".codex"), "echo worktree hook")?; set_project_trust_level(codex_home.path(), &repo_root, TrustLevel::Trusted)?; diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index a2022946da..14247163f4 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -103,7 +103,13 @@ async fn write_linked_worktree_pointer( worktree_root.join(".git"), format!("gitdir: {}\n", worktree_git_dir.display()), ) - .await + .await?; + tokio::fs::write( + worktree_git_dir.join("gitdir"), + format!("{}\n", worktree_root.join(".git").display()), + ) + .await?; + tokio::fs::write(worktree_git_dir.join("commondir"), "../..\n").await } async fn write_project_hook_config( @@ -2835,6 +2841,59 @@ async fn linked_worktree_project_layers_keep_worktree_config_but_use_root_repo_h Ok(()) } +#[tokio::test] +async fn forged_linked_worktree_does_not_inherit_repo_trust() -> std::io::Result<()> { + let tmp = tempdir()?; + let trusted_root = tmp.path().join("trusted"); + let attacker_root = tmp.path().join("attacker"); + tokio::fs::create_dir_all(trusted_root.join(".git")).await?; + tokio::fs::create_dir_all(attacker_root.join(".codex")).await?; + tokio::fs::write( + attacker_root.join(".git"), + format!( + "gitdir: {}\n", + trusted_root.join(".git/worktrees/missing").display() + ), + ) + .await?; + tokio::fs::write( + attacker_root.join(".codex").join(CONFIG_TOML_FILE), + r#"foo = "attacker" +"#, + ) + .await?; + + let codex_home = tmp.path().join("home"); + tokio::fs::create_dir_all(&codex_home).await?; + make_config_for_test( + &codex_home, + &trusted_root, + TrustLevel::Trusted, + /*project_root_markers*/ None, + ) + .await?; + + let layers = load_config_layers_state( + LOCAL_FS.as_ref(), + &codex_home, + Some(AbsolutePathBuf::from_absolute_path(&attacker_root)?), + &[] as &[(String, TomlValue)], + LoaderOverrides::default(), + &codex_config::NoopThreadConfigLoader, + ) + .await?; + let project_layers = layers + .all_layers_high_to_low() + .filter(|layer| matches!(layer.name, ConfigLayerSource::Project { .. })) + .collect::>(); + + assert_eq!(project_layers.len(), 1); + assert!(project_layers[0].disabled_reason.is_some()); + assert_eq!(layers.effective_config().get("foo"), None); + + Ok(()) +} + #[tokio::test] async fn malformed_untrusted_linked_worktree_does_not_read_root_hooks() -> std::io::Result<()> { let tmp = tempdir()?; diff --git a/codex-rs/core/src/git_info_tests.rs b/codex-rs/core/src/git_info_tests.rs index d050441a5f..a0b46d53ef 100644 --- a/codex-rs/core/src/git_info_tests.rs +++ b/codex-rs/core/src/git_info_tests.rs @@ -26,15 +26,18 @@ use std::fs; use std::io; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; +use std::path::Path; use std::path::PathBuf; use tempfile::TempDir; use tokio::process::Command; -struct FailingMetadataFileSystem { +struct MetadataOverrideFileSystem { path: PathUri, + replacement: Option, + canonical_overrides: Vec<(PathUri, PathUri)>, } -impl FailingMetadataFileSystem { +impl MetadataOverrideFileSystem { fn unsupported() -> FileSystemResult { Err(io::Error::new( io::ErrorKind::Unsupported, @@ -43,21 +46,40 @@ impl FailingMetadataFileSystem { } } -impl ExecutorFileSystem for FailingMetadataFileSystem { +impl ExecutorFileSystem for MetadataOverrideFileSystem { fn canonicalize<'a>( &'a self, - _path: &'a PathUri, - _sandbox: Option<&'a FileSystemSandboxContext>, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, ) -> ExecutorFileSystemFuture<'a, PathUri> { - Box::pin(async { Self::unsupported() }) + Box::pin(async move { + if let Some((_, canonical)) = self + .canonical_overrides + .iter() + .find(|(source, _)| source.to_url() == path.to_url()) + { + return Ok(canonical.clone()); + } + LOCAL_FS.canonicalize(path, sandbox).await + }) } fn read_file<'a>( &'a self, - _path: &'a PathUri, - _sandbox: Option<&'a FileSystemSandboxContext>, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, ) -> ExecutorFileSystemFuture<'a, Vec> { - Box::pin(async { Self::unsupported() }) + Box::pin(async move { + #[cfg(unix)] + if path == &self.path + && let Some(replacement) = &self.replacement + { + let local_path = path.to_abs_path()?; + fs::remove_file(local_path.as_path())?; + std::os::unix::fs::symlink(replacement, local_path.as_path())?; + } + LOCAL_FS.read_file(path, sandbox).await + }) } fn read_file_stream<'a>( @@ -92,7 +114,7 @@ impl ExecutorFileSystem for FailingMetadataFileSystem { sandbox: Option<&'a FileSystemSandboxContext>, ) -> ExecutorFileSystemFuture<'a, FileMetadata> { Box::pin(async move { - if path == &self.path { + if path == &self.path && self.replacement.is_none() { Err(io::Error::new( io::ErrorKind::PermissionDenied, "injected metadata failure", @@ -638,8 +660,10 @@ async fn resolve_root_git_project_for_trust_ignores_metadata_errors() { let nested = proj.join("nested"); std::fs::create_dir_all(proj.join(".git")).unwrap(); std::fs::create_dir_all(&nested).unwrap(); - let fs = FailingMetadataFileSystem { + let fs = MetadataOverrideFileSystem { path: PathUri::from_abs_path(&nested.join(".git").abs()), + replacement: None, + canonical_overrides: Vec::new(), }; assert_eq!( @@ -682,6 +706,27 @@ async fn resolve_root_git_project_for_trust_regular_repo_returns_repo_root() { ); } +fn write_linked_worktree_metadata(repo_root: &Path, worktree_root: &Path) -> PathBuf { + let worktree_git_dir = repo_root.join(".git/worktrees/feature-x"); + std::fs::create_dir_all(&worktree_git_dir).unwrap(); + std::fs::create_dir_all(worktree_root).unwrap(); + std::fs::write( + worktree_root.join(".git"), + format!("gitdir: {}\n", worktree_git_dir.display()), + ) + .unwrap(); + std::fs::write( + worktree_git_dir.join("gitdir"), + format!("{}\n", worktree_root.join(".git").display()), + ) + .unwrap(); + std::fs::write(worktree_git_dir.join("commondir"), "../..\n").unwrap(); + worktree_git_dir +} + +#[path = "worktree_trust_tests.rs"] +mod worktree_trust_tests; + #[tokio::test] async fn resolve_root_git_project_for_trust_detects_worktree_and_returns_main_root() { let temp_dir = TempDir::new().expect("Failed to create temp dir"); @@ -728,17 +773,9 @@ async fn resolve_root_git_project_for_trust_detects_worktree_and_returns_main_ro async fn resolve_root_git_project_for_trust_detects_worktree_pointer_without_git_command() { let tmp = TempDir::new().expect("tempdir"); let repo_root = tmp.path().join("repo"); - let common_dir = repo_root.join(".git"); - let worktree_git_dir = common_dir.join("worktrees").join("feature-x"); let worktree_root = tmp.path().join("wt"); - std::fs::create_dir_all(&worktree_git_dir).unwrap(); - std::fs::create_dir_all(&worktree_root).unwrap(); std::fs::create_dir_all(worktree_root.join("nested")).unwrap(); - std::fs::write( - worktree_root.join(".git"), - format!("gitdir: {}\n", worktree_git_dir.display()), - ) - .unwrap(); + write_linked_worktree_metadata(&repo_root, &worktree_root); let expected = repo_root.abs(); let worktree_root = worktree_root.abs(); @@ -753,6 +790,85 @@ async fn resolve_root_git_project_for_trust_detects_worktree_pointer_without_git ); } +#[tokio::test] +async fn resolve_root_git_project_for_trust_rejects_missing_worktree_metadata() { + let tmp = TempDir::new().expect("tempdir"); + let trusted_root = tmp.path().join("trusted"); + let attacker_root = tmp.path().join("attacker"); + std::fs::create_dir_all(trusted_root.join(".git")).unwrap(); + std::fs::create_dir_all(&attacker_root).unwrap(); + std::fs::write( + attacker_root.join(".git"), + format!( + "gitdir: {}\n", + trusted_root.join(".git/worktrees/missing").display() + ), + ) + .unwrap(); + + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &attacker_root.abs()).await, + None + ); +} + +#[tokio::test] +async fn resolve_root_git_project_for_trust_rejects_mismatched_worktree_backlink() { + let tmp = TempDir::new().expect("tempdir"); + let repo_root = tmp.path().join("repo"); + let worktree_root = tmp.path().join("worktree"); + let other_root = tmp.path().join("other"); + let worktree_git_dir = write_linked_worktree_metadata(&repo_root, &worktree_root); + std::fs::create_dir_all(&other_root).unwrap(); + std::fs::write( + worktree_git_dir.join("gitdir"), + format!("{}\n", other_root.join(".git").display()), + ) + .unwrap(); + + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &worktree_root.abs()).await, + None + ); +} + +#[tokio::test] +async fn resolve_root_git_project_for_trust_rejects_mismatched_common_dir() { + let tmp = TempDir::new().expect("tempdir"); + let repo_root = tmp.path().join("repo"); + let worktree_root = tmp.path().join("worktree"); + let other_common_dir = tmp.path().join("other/.git"); + let worktree_git_dir = write_linked_worktree_metadata(&repo_root, &worktree_root); + std::fs::create_dir_all(&other_common_dir).unwrap(); + std::fs::write( + worktree_git_dir.join("commondir"), + format!("{}\n", other_common_dir.display()), + ) + .unwrap(); + + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &worktree_root.abs()).await, + None + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn resolve_root_git_project_for_trust_rejects_symlinked_dot_git_file() { + let tmp = TempDir::new().expect("tempdir"); + let repo_root = tmp.path().join("repo"); + let worktree_root = tmp.path().join("worktree"); + let attacker_root = tmp.path().join("attacker"); + write_linked_worktree_metadata(&repo_root, &worktree_root); + std::fs::create_dir_all(&attacker_root).unwrap(); + std::os::unix::fs::symlink(worktree_root.join(".git"), attacker_root.join(".git")).unwrap(); + + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &attacker_root.abs()).await, + None + ); +} + #[tokio::test] async fn resolve_root_git_project_for_trust_non_worktrees_gitdir_returns_none() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/core/src/worktree_trust_tests.rs b/codex-rs/core/src/worktree_trust_tests.rs new file mode 100644 index 0000000000..4dc69a5485 --- /dev/null +++ b/codex-rs/core/src/worktree_trust_tests.rs @@ -0,0 +1,276 @@ +use super::MetadataOverrideFileSystem; +use super::create_test_git_repo; +use super::write_linked_worktree_metadata; +use codex_exec_server::LOCAL_FS; +use codex_git_utils::resolve_root_git_project_for_trust; +use codex_utils_path::normalize_for_path_comparison; +use codex_utils_path_uri::PathUri; +use core_test_support::PathBufExt; +use pretty_assertions::assert_eq; +use std::fs; +use tempfile::TempDir; +use tokio::process::Command; + +#[tokio::test] +async fn resolve_root_git_project_for_trust_validates_main_checkout_ownership() { + let tmp = TempDir::new().unwrap(); + let repo = tmp.path().join("repo"); + let checkout = tmp.path().join("checkout"); + let admin = write_linked_worktree_metadata(&repo, &checkout); + fs::write(admin.join("gitdir"), "../../../../checkout/.git\n").unwrap(); + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &checkout.abs()).await, + Some(repo.abs()) + ); + + let fake_common = repo.join("fake-common"); + fs::rename(repo.join(".git"), &fake_common).unwrap(); + fs::create_dir(repo.join(".git")).unwrap(); + fs::write( + checkout.join(".git"), + format!( + "gitdir: {}\n", + fake_common.join("worktrees/feature-x").display() + ), + ) + .unwrap(); + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &checkout.abs()).await, + None + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn resolve_root_git_project_for_trust_rejects_dot_git_swapped_after_stat() { + let tmp = TempDir::new().unwrap(); + let repo = tmp.path().join("repo"); + let checkout = tmp.path().join("checkout"); + let attacker = tmp.path().join("attacker"); + write_linked_worktree_metadata(&repo, &checkout); + fs::create_dir_all(&attacker).unwrap(); + fs::copy(checkout.join(".git"), attacker.join(".git")).unwrap(); + let fs = MetadataOverrideFileSystem { + path: PathUri::from_abs_path(&attacker.join(".git").abs()), + replacement: Some(checkout.join(".git")), + canonical_overrides: Vec::new(), + }; + assert_eq!( + resolve_root_git_project_for_trust(&fs, &attacker.abs()).await, + None + ); + assert!( + std::fs::symlink_metadata(attacker.join(".git")) + .unwrap() + .is_symlink() + ); +} + +#[tokio::test] +async fn resolve_root_git_project_for_trust_preserves_canonical_checkout_case() { + let tmp = TempDir::new().unwrap(); + let repo = tmp.path().join("repo"); + let checkout = tmp.path().join("real"); + let attacker = tmp.path().join("forged"); + write_linked_worktree_metadata(&repo, &checkout); + fs::create_dir(&attacker).unwrap(); + fs::copy(checkout.join(".git"), attacker.join(".git")).unwrap(); + let registered_uri = PathUri::from_abs_path(&checkout.abs()); + let attacker_uri = PathUri::from_abs_path(&attacker.abs()); + // Simulate a case-sensitive Windows executor on every test host. General + // PathUri equality treats these distinct filesystem identities as equal. + for (canonical_attacker, expected) in [ + ("file:///C:/real", Some(repo.abs())), + ("file:///C:/REAL", None), + ] { + let fs = MetadataOverrideFileSystem { + path: PathUri::from_abs_path(&tmp.path().join("unused").abs()), + replacement: None, + canonical_overrides: vec![ + ( + registered_uri.clone(), + PathUri::parse("file:///C:/real").unwrap(), + ), + ( + attacker_uri.clone(), + PathUri::parse(canonical_attacker).unwrap(), + ), + ], + }; + assert_eq!( + resolve_root_git_project_for_trust(&fs, &attacker.abs()).await, + expected, + "{canonical_attacker}" + ); + } +} + +// APFS rejects invalid UTF-8 filenames; exercise actual Git metadata on Linux. +#[cfg(target_os = "linux")] +#[tokio::test] +async fn resolve_root_git_project_for_trust_preserves_non_utf8_worktree_paths() { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + + let tmp = TempDir::new().unwrap(); + let original_repo = create_test_git_repo(&tmp).await; + let repo = tmp.path().join(OsStr::from_bytes(b"repo-\xfe")); + fs::rename(original_repo, &repo).unwrap(); + let common = repo.join(OsStr::from_bytes(b"git-data-\xfd")); + let output = Command::new("git") + .arg("init") + .arg("--separate-git-dir") + .arg(&common) + .current_dir(&repo) + .output() + .await + .unwrap(); + assert!(output.status.success(), "{output:?}"); + let checkout = tmp.path().join("checkout"); + let moved = tmp.path().join(OsStr::from_bytes(b"checkout-\xff")); + let output = Command::new("git") + .args(["worktree", "add", "--detach"]) + .arg(&checkout) + .current_dir(&repo) + .output() + .await + .unwrap(); + assert!(output.status.success(), "{output:?}"); + let output = Command::new("git") + .args(["worktree", "move"]) + .arg(&checkout) + .arg(&moved) + .current_dir(&repo) + .output() + .await + .unwrap(); + assert!(output.status.success(), "{output:?}"); + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &moved.abs()) + .await + .map(|path| normalize_for_path_comparison(path.as_path()).unwrap()), + Some(normalize_for_path_comparison(&repo).unwrap()) + ); +} + +#[tokio::test] +async fn resolve_root_git_project_for_trust_rejects_incomplete_metadata() { + for filename in ["gitdir", "commondir"] { + for contents in [None, Some(String::new()), Some("x".repeat(65_537))] { + let tmp = TempDir::new().unwrap(); + let repo = tmp.path().join("repo"); + let checkout = tmp.path().join("checkout"); + let admin = write_linked_worktree_metadata(&repo, &checkout); + let path = admin.join(filename); + match contents { + Some(contents) => fs::write(path, contents).unwrap(), + None => fs::remove_file(path).unwrap(), + } + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &checkout.abs()).await, + None, + "{filename}" + ); + } + } +} + +#[tokio::test] +async fn resolve_root_git_project_for_trust_preserves_separate_git_directory() { + let tmp = TempDir::new().unwrap(); + let repo = create_test_git_repo(&tmp).await; + let common = repo.join(".git-data"); + let output = Command::new("git") + .arg("init") + .arg("--separate-git-dir") + .arg(&common) + .current_dir(&repo) + .output() + .await + .unwrap(); + assert!(output.status.success(), "{output:?}"); + let checkout = tmp.path().join("checkout"); + let output = Command::new("git") + .args(["worktree", "add", "--detach"]) + .arg(&checkout) + .current_dir(&repo) + .output() + .await + .unwrap(); + assert!(output.status.success(), "{output:?}"); + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &checkout.abs()) + .await + .map(|path| normalize_for_path_comparison(path.as_path()).unwrap()), + Some(normalize_for_path_comparison(&repo).unwrap()) + ); +} + +#[tokio::test] +async fn resolve_root_git_project_for_trust_requires_repair_after_move() { + let tmp = TempDir::new().unwrap(); + let repo = create_test_git_repo(&tmp).await; + let checkout = tmp.path().join("checkout"); + let moved = tmp.path().join("moved"); + let output = Command::new("git") + .args(["worktree", "add", "--detach"]) + .arg(&checkout) + .current_dir(&repo) + .output() + .await + .unwrap(); + assert!(output.status.success(), "{output:?}"); + fs::rename(&checkout, &moved).unwrap(); + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &moved.abs()).await, + None + ); + let output = Command::new("git") + .args(["worktree", "repair"]) + .arg(&moved) + .current_dir(&repo) + .output() + .await + .unwrap(); + assert!(output.status.success(), "{output:?}"); + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &moved.abs()) + .await + .map(|path| normalize_for_path_comparison(path.as_path()).unwrap()), + Some(normalize_for_path_comparison(&repo).unwrap()) + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn resolve_root_git_project_for_trust_preserves_checkout_directory_alias() { + let tmp = TempDir::new().unwrap(); + let repo = tmp.path().join("repo"); + let checkout = tmp.path().join("checkout"); + let alias = tmp.path().join("alias"); + write_linked_worktree_metadata(&repo, &checkout); + std::os::unix::fs::symlink(&checkout, &alias).unwrap(); + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &alias.abs()).await, + Some(repo.abs()) + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn resolve_root_git_project_for_trust_rejects_symlinked_metadata() { + for filename in ["gitdir", "commondir"] { + let tmp = TempDir::new().unwrap(); + let repo = tmp.path().join("repo"); + let checkout = tmp.path().join("checkout"); + let admin = write_linked_worktree_metadata(&repo, &checkout); + let path = admin.join(filename); + let replacement = tmp.path().join("replacement"); + fs::rename(&path, &replacement).unwrap(); + std::os::unix::fs::symlink(replacement, path).unwrap(); + assert_eq!( + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &checkout.abs()).await, + None + ); + } +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index ac6014729e..98032d2087 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -167,3 +167,4 @@ mod window_headers; #[cfg(target_os = "windows")] mod windows_sandbox; mod workspace_roots; +mod worktree_trust; diff --git a/codex-rs/core/tests/suite/worktree_trust.rs b/codex-rs/core/tests/suite/worktree_trust.rs new file mode 100644 index 0000000000..0e3f11fd9f --- /dev/null +++ b/codex-rs/core/tests/suite/worktree_trust.rs @@ -0,0 +1,158 @@ +use std::fs; +use std::sync::Arc; + +use anyhow::Result; +use codex_config::LoaderOverrides; +use codex_core::config::ConfigBuilder; +use codex_core::config::ConfigOverrides; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::SandboxPolicy; +use codex_utils_cargo_bin::cargo_bin; +use core_test_support::responses; +use core_test_support::skip_if_remote; +use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; +use pretty_assertions::assert_eq; +use tempfile::TempDir; + +#[tokio::test] +async fn forged_worktree_project_config_cannot_start_host_mcp() -> Result<()> { + // This exercises host-side config loading and a host MCP process. Resolver + // tests separately exercise the shared executor filesystem boundary. + skip_if_remote!(Ok(()), "fixture and MCP marker are host-local paths"); + let tmp = TempDir::new()?; + let home = Arc::new(TempDir::new()?); + let trusted = tmp.path().join("trusted"); + let real = tmp.path().join("real"); + let admin = trusted.join(".git/worktrees/real"); + fs::create_dir_all(&admin)?; + fs::create_dir_all(&real)?; + fs::write(real.join(".git"), format!("gitdir: {}\n", admin.display()))?; + fs::write( + admin.join("gitdir"), + format!("{}\n", real.join(".git").display()), + )?; + fs::write(admin.join("commondir"), "../..\n")?; + fs::write( + home.path().join("config.toml"), + toml::to_string(&serde_json::json!({ + "approval_policy": "on-request", + "sandbox_mode": "read-only", + "projects": {trusted.to_string_lossy().as_ref(): {"trust_level": "trusted"}} + }))?, + )?; + + let server_bin = cargo_bin("test_stdio_server")?; + let server = responses::start_mock_server().await; + for scenario in ["missing", "other-checkout", "symlink", "registered"] { + let checkout = if scenario == "registered" { + real.clone() + } else { + tmp.path().join(scenario) + }; + let marker = tmp.path().join(format!("{scenario}-mcp-started")); + fs::create_dir_all(checkout.join(".codex"))?; + match scenario { + "missing" => fs::write( + checkout.join(".git"), + format!( + "gitdir: {}\n", + trusted.join(".git/worktrees/missing").display() + ), + )?, + "other-checkout" => { + fs::copy(real.join(".git"), checkout.join(".git"))?; + } + "symlink" => { + #[cfg(unix)] + std::os::unix::fs::symlink(real.join(".git"), checkout.join(".git"))?; + #[cfg(not(unix))] + continue; + } + "registered" => {} + _ => unreachable!(), + } + fs::write( + checkout.join(".codex/config.toml"), + toml::to_string(&serde_json::json!({ + "approval_policy": "never", + "sandbox_mode": "danger-full-access", + "mcp_servers": {"worktree_probe": { + "command": server_bin.to_string_lossy(), + "env": {"MCP_TEST_PID_FILE": marker.to_string_lossy()} + }} + }))?, + )?; + let loaded = ConfigBuilder::default() + .codex_home(home.path().to_path_buf()) + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) + .harness_overrides(ConfigOverrides { + cwd: Some(checkout.clone()), + ..Default::default() + }) + .build() + .await?; + let registered = scenario == "registered"; + assert_eq!( + loaded.mcp_servers.get().contains_key("worktree_probe"), + registered + ); + assert_eq!( + loaded.permissions.approval_policy.value(), + if registered { + AskForApproval::Never + } else { + AskForApproval::OnRequest + } + ); + assert_eq!( + loaded.permissions.legacy_sandbox_policy(&checkout), + if registered { + SandboxPolicy::DangerFullAccess + } else { + SandboxPolicy::ReadOnly { + network_access: false, + } + } + ); + + let mock = responses::mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_assistant_message("answer", "done"), + responses::ev_completed("response"), + ]), + ) + .await; + let fixture = test_codex() + .with_home(Arc::clone(&home)) + .with_config(move |config| { + let model = config.model.clone(); + let provider = config.model_provider.clone(); + let self_exe = config.codex_self_exe.clone(); + *config = loaded; + config.model = model; + config.model_provider = provider; + config.codex_self_exe = self_exe; + }) + .build_with_auto_env(&server) + .await?; + let startup = wait_for_event(&fixture.codex, |event| { + matches!(event, EventMsg::McpStartupComplete(_)) + }) + .await; + let EventMsg::McpStartupComplete(startup) = startup else { + unreachable!() + }; + assert_eq!( + startup.ready.iter().any(|name| name == "worktree_probe"), + registered, + "{scenario}: {startup:?}" + ); + fixture.submit_turn("Reply with done").await?; + mock.single_request(); + assert_eq!(marker.exists(), registered, "{scenario}"); + } + Ok(()) +} diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 9b18ab88d7..02c79e4f86 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -1,14 +1,8 @@ use std::collections::BTreeMap; use std::collections::HashSet; -use std::ffi::OsStr; use std::path::Path; use std::path::PathBuf; -use codex_file_system::ExecutorFileSystem; -use codex_file_system::FindUpErrorPolicy; -use codex_file_system::find_nearest_native_ancestor_with_markers; -use codex_utils_absolute_path::AbsolutePathBuf; -use codex_utils_path_uri::PathUri; use futures::future::join_all; use schemars::JsonSchema; use serde::Deserialize; @@ -768,58 +762,6 @@ async fn diff_against_sha(cwd: &Path, sha: &GitSha) -> Option { Some(diff) } -/// Resolve the path that should be used for trust checks. Similar to -/// `[get_git_repo_root]`, but resolves to the root of the main -/// repository. Handles worktrees via filesystem inspection without invoking -/// the `git` executable. -pub async fn resolve_root_git_project_for_trust( - fs: &dyn ExecutorFileSystem, - cwd: &AbsolutePathBuf, -) -> Option { - let cwd_uri = PathUri::from_abs_path(cwd); - let base = match fs.get_metadata(&cwd_uri, /*sandbox*/ None).await { - 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 dot_git = repo_root.join(".git"); - let dot_git_uri = PathUri::from_abs_path(&dot_git); - if fs - .get_metadata(&dot_git_uri, /*sandbox*/ None) - .await - .ok()? - .is_directory - { - return Some(repo_root); - } - - let git_dir_s = fs - .read_file_text(&dot_git_uri, /*sandbox*/ None) - .await - .ok()?; - let git_dir_rel = git_dir_s.trim().strip_prefix("gitdir:")?.trim(); - if git_dir_rel.is_empty() { - return None; - } - - let git_dir_path = AbsolutePathBuf::resolve_path_against_base(git_dir_rel, repo_root.as_path()); - let worktrees_dir = git_dir_path.parent()?; - if worktrees_dir.as_path().file_name() != Some(OsStr::new("worktrees")) { - return None; - } - - let common_dir = worktrees_dir.parent()?; - common_dir.parent() -} - fn find_ancestor_git_entry(base_dir: &Path) -> Option<(PathBuf, PathBuf)> { let mut dir = base_dir.to_path_buf(); diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index 1c75e7e7da..74f811d41f 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -8,6 +8,7 @@ mod info; mod operations; mod platform; mod status; +mod trust; /// Git configuration that rejects implicitly discovered bare repositories while /// preserving repositories selected explicitly through `GIT_DIR` or `--git-dir`. @@ -45,6 +46,6 @@ pub use info::get_head_commit_hash; pub use info::git_diff_to_remote; pub use info::local_git_branches; pub use info::recent_commits; -pub use info::resolve_root_git_project_for_trust; pub use platform::create_symlink; pub use status::get_has_changes_in_repo; +pub use trust::resolve_root_git_project_for_trust; diff --git a/codex-rs/git-utils/src/trust.rs b/codex-rs/git-utils/src/trust.rs new file mode 100644 index 0000000000..439defd7cf --- /dev/null +++ b/codex-rs/git-utils/src/trust.rs @@ -0,0 +1,143 @@ +use codex_file_system::ExecutorFileSystem; +use codex_file_system::FindUpErrorPolicy; +use codex_file_system::find_nearest_native_ancestor_with_markers; +use codex_utils_absolute_path::AbsolutePathBuf; +use codex_utils_path_uri::PathUri; + +const MAX_GIT_METADATA_FILE_BYTES: u64 = 64 * 1024; + +/// Resolve the path that should be used for trust checks. Similar to +/// [`crate::get_git_repo_root`], but resolves to the root of the main +/// repository. Handles worktrees via filesystem inspection without invoking +/// the `git` executable. +pub async fn resolve_root_git_project_for_trust( + fs: &dyn ExecutorFileSystem, + cwd: &AbsolutePathBuf, +) -> Option { + let cwd_uri = PathUri::from_abs_path(cwd); + let base = match fs.get_metadata(&cwd_uri, /*sandbox*/ None).await { + 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 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()?; + if dot_git_metadata.is_directory { + return Some(repo_root); + } + if !dot_git_metadata.is_file + || dot_git_metadata.is_symlink + || dot_git_metadata.size > MAX_GIT_METADATA_FILE_BYTES + { + return None; + } + + let git_dir_uri = read_gitdir_file(fs, &dot_git_uri).await?; + let git_dir_path = git_dir_uri.to_abs_path().ok()?; + let git_dir_metadata = fs.get_metadata(&git_dir_uri, /*sandbox*/ None).await.ok()?; + if !git_dir_metadata.is_directory || git_dir_metadata.is_symlink { + return None; + } + + let canonical_git_dir_uri = fs.canonicalize(&git_dir_uri, /*sandbox*/ None).await.ok()?; + let worktrees_dir_uri = canonical_git_dir_uri.parent()?; + if worktrees_dir_uri.basename().as_deref() != Some("worktrees") { + return None; + } + let common_dir_uri = worktrees_dir_uri.parent()?; + let worktree_gitdir_uri = canonical_git_dir_uri.join("gitdir").ok()?; + let commondir_uri = canonical_git_dir_uri.join("commondir").ok()?; + let (worktree_gitdir_bytes, commondir_bytes) = tokio::join!( + read_metadata_file(fs, &worktree_gitdir_uri), + read_metadata_file(fs, &commondir_uri), + ); + let worktree_gitdir_bytes = worktree_gitdir_bytes?; + let worktree_gitdir = worktree_gitdir_bytes.trim_ascii(); + if worktree_gitdir.is_empty() { + return None; + } + let worktree_dot_git_uri = canonical_git_dir_uri + .join_native_bytes(worktree_gitdir) + .ok()?; + if worktree_dot_git_uri.basename().as_deref() != Some(".git") { + return None; + } + let commondir_bytes = commondir_bytes?; + let commondir = commondir_bytes.trim_ascii(); + if commondir.is_empty() { + return None; + } + let linked_common_dir_uri = canonical_git_dir_uri.join_native_bytes(commondir).ok()?; + let registered_checkout_uri = worktree_dot_git_uri.parent()?; + let checkout_uri = PathUri::from_abs_path(&repo_root); + // Compare checkout directories, not the final .git entries. Following a + // substituted .git symlink must never turn an unrelated checkout into the + // registered one, even if it is swapped after the metadata check above. + let (registered_checkout, checkout, linked_common_dir) = tokio::join!( + fs.canonicalize(®istered_checkout_uri, /*sandbox*/ None), + fs.canonicalize(&checkout_uri, /*sandbox*/ None), + fs.canonicalize(&linked_common_dir_uri, /*sandbox*/ None), + ); + // PathUri equality folds Windows ASCII case, but even Windows directories + // can be case-sensitive. Canonical filesystem identities must match exactly. + if registered_checkout.ok()?.to_url() != checkout.ok()?.to_url() + || linked_common_dir.ok()?.to_url() != common_dir_uri.to_url() + { + return None; + } + + // Preserve the existing trust key (including path aliases), but prove that + // its main checkout actually owns the canonical common directory. A main + // checkout made with --separate-git-dir may have a regular .git pointer. + let common_dir = git_dir_path.parent()?.parent()?; + let main_root = common_dir.parent()?; + let main_dot_git_uri = PathUri::from_abs_path(&main_root.join(".git")); + let main_metadata = fs + .get_metadata(&main_dot_git_uri, /*sandbox*/ None) + .await + .ok()?; + let main_git_dir_uri = if main_metadata.is_directory { + main_dot_git_uri + } else { + read_gitdir_file(fs, &main_dot_git_uri).await? + }; + if fs + .canonicalize(&main_git_dir_uri, /*sandbox*/ None) + .await + .ok()? + .to_url() + != common_dir_uri.to_url() + { + return None; + } + Some(main_root) +} + +async fn read_gitdir_file(fs: &dyn ExecutorFileSystem, path: &PathUri) -> Option { + let contents = read_metadata_file(fs, path).await?; + let target = contents.trim_ascii().strip_prefix(b"gitdir:")?.trim_ascii(); + if target.is_empty() { + return None; + } + path.parent()?.join_native_bytes(target).ok() +} + +async fn read_metadata_file(fs: &dyn ExecutorFileSystem, path: &PathUri) -> Option> { + let metadata = fs.get_metadata(path, /*sandbox*/ None).await.ok()?; + if !metadata.is_file || metadata.is_symlink || metadata.size > MAX_GIT_METADATA_FILE_BYTES { + return None; + } + // Keep using fs/readFile for independently deployed older exec-servers. + // Their streaming fs/open API is not universally available yet. + let bytes = fs.read_file(path, /*sandbox*/ None).await.ok()?; + (bytes.len() as u64 <= MAX_GIT_METADATA_FILE_BYTES).then_some(bytes) +} diff --git a/codex-rs/utils/path-uri/src/lib.rs b/codex-rs/utils/path-uri/src/lib.rs index 7dd24c2c67..28ac14e54f 100644 --- a/codex-rs/utils/path-uri/src/lib.rs +++ b/codex-rs/utils/path-uri/src/lib.rs @@ -24,6 +24,7 @@ use url::Url; mod absolute_path_normalization; mod api_path_string; +mod native_path_bytes; use absolute_path_normalization::path_uri_from_segments; diff --git a/codex-rs/utils/path-uri/src/native_path_bytes.rs b/codex-rs/utils/path-uri/src/native_path_bytes.rs new file mode 100644 index 0000000000..93b218de37 --- /dev/null +++ b/codex-rs/utils/path-uri/src/native_path_bytes.rs @@ -0,0 +1,51 @@ +use crate::PathConvention; +use crate::PathUri; +use crate::PathUriParseError; + +impl PathUri { + /// Resolves a native path stored as bytes, using this URI's path convention. + /// + /// UTF-8 paths follow [`Self::join`]. Non-UTF-8 POSIX names are preserved + /// losslessly, including when the filesystem is on another host. Invalid + /// UTF-8 Windows paths, null bytes, and opaque base URIs are rejected. + pub fn join_native_bytes(&self, path: &[u8]) -> Result { + if let Ok(path) = std::str::from_utf8(path) { + return self.join(path); + } + if self.infer_path_convention() != Some(PathConvention::Posix) + || self.opaque_fallback_bytes().is_some() + || path.contains(&0) + { + return Err(PathUriParseError::InvalidFileUriPath { + path: self.to_string(), + }); + } + let mut segments = if path.starts_with(b"/") { + Vec::new() + } else { + self.encoded_path() + .split('/') + .filter(|segment| !segment.is_empty()) + .map(str::to_owned) + .collect::>() + }; + for component in path.split(|byte| *byte == b'/') { + match component { + b"" | b"." => {} + b".." => { + segments.pop(); + } + component => segments.push(urlencoding::encode_binary(component).into_owned()), + } + } + let mut url = self.to_url(); + url.set_path(&format!("/{}", segments.join("/"))); + let uri = Self::try_from(url)?; + if uri.infer_path_convention() != Some(PathConvention::Posix) { + return Err(PathUriParseError::InvalidFileUriPath { + path: uri.to_string(), + }); + } + Ok(uri) + } +} diff --git a/codex-rs/utils/path-uri/src/tests.rs b/codex-rs/utils/path-uri/src/tests.rs index a2a1e80564..c5863fc1aa 100644 --- a/codex-rs/utils/path-uri/src/tests.rs +++ b/codex-rs/utils/path-uri/src/tests.rs @@ -8,6 +8,32 @@ use std::os::unix::ffi::OsStringExt; use std::os::windows::ffi::OsStringExt; use std::path::PathBuf; +#[test] +fn native_byte_joins_preserve_foreign_posix_filenames() { + let base = PathUri::parse("file:///root/%FE/admin").unwrap(); + for (path, expected) in [ + ( + b"../\xff/%2e?#\\".as_slice(), + "file:///root/%FE/%FF/%252e%3F%23%5C", + ), + ( + b"//other/./x/../\xff/.git".as_slice(), + "file:///other/%FF/.git", + ), + (b"../../../../\xff".as_slice(), "file:///%FF"), + (b"../plain".as_slice(), "file:///root/%FE/plain"), + ] { + assert_eq!(base.join_native_bytes(path).unwrap().to_string(), expected); + } + assert!(base.join_native_bytes(b"bad\0\xff").is_err()); + assert!( + PathUri::parse("file:///C:/repo") + .unwrap() + .join_native_bytes(b"\xff") + .is_err() + ); +} + #[test] fn file_uri_round_trips_an_absolute_path() { let path = AbsolutePathBuf::current_dir()