From 690995b7c1e8c8099c684402b5957535bada4e80 Mon Sep 17 00:00:00 2001 From: caseysilver-oai Date: Wed, 22 Jul 2026 01:13:59 +0000 Subject: [PATCH] Verify Git plugin SHA checkouts (#34644) ## Why Git can interpret a requested commit SHA as a branch name when the remote's default branch has the same name. This can cause a marketplace plugin source to materialize a different commit than the one it pinned. ## What changed Resolve `HEAD` after checking out a SHA-pinned Git plugin source and reject the source when the resolved commit does not exactly match the requested SHA. Ref-name checkouts retain their existing behavior. ## Testing Add a regression test with a default branch named after another commit's SHA and verify that materialization rejects the mismatched checkout. GitOrigin-RevId: c19cbd98ee6167dad7b2ee72e283b3bd30b59713 --- codex-rs/core-plugins/src/loader.rs | 18 +++++++-- codex-rs/core-plugins/src/loader_tests.rs | 46 ++++++++++++++++++++++- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index e01cd049575a..f458a1d2a798 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -1523,8 +1523,16 @@ fn clone_git_plugin_source( /*cwd*/ None, )?; } - if let Some(target) = sha.or(ref_name) { - run_git(&["checkout", target], Some(destination))?; + if let Some(sha) = sha { + run_git(&["checkout", sha], Some(destination))?; + let checked_out_sha = run_git_output(&["rev-parse", "HEAD"], Some(destination))?; + if !checked_out_sha.eq_ignore_ascii_case(sha) { + return Err(format!( + "checked out Git SHA {checked_out_sha} does not match requested SHA {sha}" + )); + } + } else if let Some(ref_name) = ref_name { + run_git(&["checkout", ref_name], Some(destination))?; } else if sparse_checkout_path.is_some() { run_git(&["checkout"], Some(destination))?; } @@ -1532,6 +1540,10 @@ fn clone_git_plugin_source( } fn run_git(args: &[&str], cwd: Option<&Path>) -> Result<(), String> { + run_git_output(args, cwd).map(drop) +} + +fn run_git_output(args: &[&str], cwd: Option<&Path>) -> Result { let mut command = Command::new("git"); command.args(args); command.env("GIT_TERMINAL_PROMPT", "0"); @@ -1543,7 +1555,7 @@ fn run_git(args: &[&str], cwd: Option<&Path>) -> Result<(), String> { .output() .map_err(|err| format!("failed to run git {}: {err}", args.join(" ")))?; if output.status.success() { - return Ok(()); + return Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()); } Err(format!( diff --git a/codex-rs/core-plugins/src/loader_tests.rs b/codex-rs/core-plugins/src/loader_tests.rs index f0835ff61475..022c34855993 100644 --- a/codex-rs/core-plugins/src/loader_tests.rs +++ b/codex-rs/core-plugins/src/loader_tests.rs @@ -474,6 +474,7 @@ fn materialize_git_subdir_uses_sparse_checkout() { run_git(&["config", "user.name", "Test User"], Some(repo.path())).expect("configure git name"); run_git(&["add", "."], Some(repo.path())).expect("stage git repo"); run_git(&["commit", "-m", "init"], Some(repo.path())).expect("commit git repo"); + let sha = run_git_output(&["rev-parse", "HEAD"], Some(repo.path())).expect("resolve commit"); let materialized = materialize_marketplace_plugin_source( codex_home.path(), @@ -481,7 +482,7 @@ fn materialize_git_subdir_uses_sparse_checkout() { url: repo.path().display().to_string(), path: Some("plugins/toolkit".to_string()), ref_name: None, - sha: None, + sha: Some(sha), }, ) .expect("materialize git source"); @@ -500,3 +501,46 @@ fn materialize_git_subdir_uses_sparse_checkout() { assert!(!checkout_root.join("root.txt").exists()); assert!(!checkout_root.join("plugins/other/marker.txt").exists()); } + +#[test] +fn materialize_git_source_rejects_sha_that_resolves_to_hostile_default_branch() { + let codex_home = tempfile::tempdir().expect("create codex home"); + let repo = tempfile::tempdir().expect("create git repo"); + run_git(&["init"], Some(repo.path())).expect("init git repo"); + run_git( + &["config", "user.email", "test@example.com"], + Some(repo.path()), + ) + .expect("configure git email"); + run_git(&["config", "user.name", "Test User"], Some(repo.path())).expect("configure git name"); + + fs::write(repo.path().join("marker.txt"), "benign").expect("write benign marker"); + run_git(&["add", "."], Some(repo.path())).expect("stage git repo"); + run_git(&["commit", "-m", "benign"], Some(repo.path())).expect("commit benign revision"); + let benign_sha = + run_git_output(&["rev-parse", "HEAD"], Some(repo.path())).expect("resolve commit A"); + + fs::write(repo.path().join("marker.txt"), "malicious").expect("write malicious marker"); + run_git(&["add", "."], Some(repo.path())).expect("stage malicious revision"); + run_git(&["commit", "-m", "malicious"], Some(repo.path())).expect("commit malicious revision"); + let malicious_sha = + run_git_output(&["rev-parse", "HEAD"], Some(repo.path())).expect("resolve commit B"); + run_git(&["branch", "-m", &benign_sha], Some(repo.path())) + .expect("name default branch after commit A"); + + let err = materialize_marketplace_plugin_source( + codex_home.path(), + &MarketplacePluginSource::Git { + url: repo.path().display().to_string(), + path: None, + ref_name: None, + sha: Some(benign_sha.clone()), + }, + ) + .expect_err("hostile default branch must not satisfy SHA pinning"); + + assert_eq!( + err, + format!("checked out Git SHA {malicious_sha} does not match requested SHA {benign_sha}") + ); +}