mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
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
This commit is contained in:
1 parent
539c0e1100
commit
690995b7c1
2 files changed
+60
-4
No files matched your search
@@ -1523,8 +1523,16 @@ fn clone_git_plugin_source(
|
|||||||
/*cwd*/ None,
|
/*cwd*/ None,
|
||||||
)?;
|
)?;
|
||||||
}
|
}
|
||||||
if let Some(target) = sha.or(ref_name) {
|
if let Some(sha) = sha {
|
||||||
run_git(&["checkout", target], Some(destination))?;
|
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() {
|
} else if sparse_checkout_path.is_some() {
|
||||||
run_git(&["checkout"], Some(destination))?;
|
run_git(&["checkout"], Some(destination))?;
|
||||||
}
|
}
|
||||||
@@ -1532,6 +1540,10 @@ fn clone_git_plugin_source(
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn run_git(args: &[&str], cwd: Option<&Path>) -> Result<(), String> {
|
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<String, String> {
|
||||||
let mut command = Command::new("git");
|
let mut command = Command::new("git");
|
||||||
command.args(args);
|
command.args(args);
|
||||||
command.env("GIT_TERMINAL_PROMPT", "0");
|
command.env("GIT_TERMINAL_PROMPT", "0");
|
||||||
@@ -1543,7 +1555,7 @@ fn run_git(args: &[&str], cwd: Option<&Path>) -> Result<(), String> {
|
|||||||
.output()
|
.output()
|
||||||
.map_err(|err| format!("failed to run git {}: {err}", args.join(" ")))?;
|
.map_err(|err| format!("failed to run git {}: {err}", args.join(" ")))?;
|
||||||
if output.status.success() {
|
if output.status.success() {
|
||||||
return Ok(());
|
return Ok(String::from_utf8_lossy(&output.stdout).trim().to_string());
|
||||||
}
|
}
|
||||||
|
|
||||||
Err(format!(
|
Err(format!(
|
||||||
|
|||||||
@@ -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(&["config", "user.name", "Test User"], Some(repo.path())).expect("configure git name");
|
||||||
run_git(&["add", "."], Some(repo.path())).expect("stage git repo");
|
run_git(&["add", "."], Some(repo.path())).expect("stage git repo");
|
||||||
run_git(&["commit", "-m", "init"], Some(repo.path())).expect("commit 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(
|
let materialized = materialize_marketplace_plugin_source(
|
||||||
codex_home.path(),
|
codex_home.path(),
|
||||||
@@ -481,7 +482,7 @@ fn materialize_git_subdir_uses_sparse_checkout() {
|
|||||||
url: repo.path().display().to_string(),
|
url: repo.path().display().to_string(),
|
||||||
path: Some("plugins/toolkit".to_string()),
|
path: Some("plugins/toolkit".to_string()),
|
||||||
ref_name: None,
|
ref_name: None,
|
||||||
sha: None,
|
sha: Some(sha),
|
||||||
},
|
},
|
||||||
)
|
)
|
||||||
.expect("materialize git source");
|
.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("root.txt").exists());
|
||||||
assert!(!checkout_root.join("plugins/other/marker.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}")
|
||||||
|
);
|
||||||
|
}
|
||||||
Reference in new issue
Block a user