openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-12 21:20:34 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge branch 'codex/psec-4394-git-04d-guarded-apply' into codex/psec-4394-git-05a-apply-filters

Browse Source 
# Conflicts:
#	codex-rs/git-utils/src/apply.rs
#	codex-rs/git-utils/src/git_command.rs
#	codex-rs/git-utils/src/git_config.rs
#	codex-rs/git-utils/src/git_config_sources/primary_sources.rs
#	codex-rs/git-utils/src/git_config_sources_tests.rs
#	codex-rs/git-utils/src/git_config_tests.rs
#	codex-rs/git-utils/src/guarded_config.rs
#	codex-rs/git-utils/src/guarded_config_tests.rs
#	codex-rs/git-utils/src/patch_paths.rs
#	codex-rs/git-utils/src/patch_paths_tests.rs
#	codex-rs/git-utils/src/repository_authority/authority.rs
This commit is contained in:
Chris Bookholt
2026-07-03 19:15:58 -07:00
parent 80c59de030 463eed4400
commit 62c56ee3ac
11 changed files with 411 additions and 382 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/git-utils/src/apply.rs
-177
View File
@@ -10,13 +10,11 @@ use std::io;
use std::path::Path;
use std::path::PathBuf;
use crate::FsmonitorOverride;
use crate::apply_output::parse_git_apply_output;
use crate::git_command::GitRunner;
use crate::guarded_config::GuardedGitConfig;
use crate::patch_paths::extract_effective_paths_from_patch_guarded;
use crate::patch_paths::stage_effective_paths_guarded;
use crate::safe_git::DISABLED_HOOKS_PATH;
#[cfg(test)]
use crate::safe_git::isolate_git_command_environment;
@@ -179,26 +177,6 @@ pub(crate) fn write_temp_patch(diff: &str) -> io::Result<(tempfile::TempDir, Pat
Ok((dir, path))
}
pub(crate) fn run_git(
git: &GitRunner,
cwd: &Path,
git_cfg: &[String],
args: &[String],
) -> io::Result<(i32, String, String)> {
let mut cmd = git.command_for_cwd(cwd)?;
for p in git_cfg {
cmd.arg(p);
}
for a in args {
cmd.arg(a);
}
let out = git.output(cmd)?;
let code = out.status.code().unwrap_or(-1);
let stdout = String::from_utf8_lossy(&out.stdout).into_owned();
let stderr = String::from_utf8_lossy(&out.stderr).into_owned();
Ok((code, stdout, stderr))
}
fn run_guarded_apply(
config: &GuardedGitConfig<'_>,
args: &[String],
@@ -225,15 +203,6 @@ fn run_guarded_apply(
))
}
pub(crate) fn safe_git_config_parts() -> Vec<String> {
vec![
"-c".to_string(),
format!("core.hooksPath={DISABLED_HOOKS_PATH}"),
"-c".to_string(),
FsmonitorOverride::Disabled.git_config_arg().to_string(),
]
}
#[cfg(all(test, unix))]
#[path = "apply_transport_tests.rs"]
mod transport_tests;
@@ -309,57 +278,6 @@ mod tests {
.replace("\r\n", "\n")
}
fn commit_filter_attributes(root: &Path, tracked_path: &str) {
std::fs::write(
root.join(".gitattributes"),
format!("{tracked_path} filter=x=y\n"),
)
.expect("write attributes");
let (add_code, _, add_err) = run(root, &["git", "add", ".gitattributes"]);
assert_eq!(add_code, 0, "add attributes: {add_err}");
let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "attributes"]);
assert_eq!(commit_code, 0, "commit attributes: {commit_err}");
}
fn configure_clean_filter(root: &Path, tracked_path: &str) {
commit_filter_attributes(root, tracked_path);
let (config_code, _, config_err) = run(
root,
&[
"git",
"config",
"filter.x=y.clean",
"git config codex.filterran true && git hash-object --stdin",
],
);
assert_eq!(config_code, 0, "configure filter: {config_err}");
}
fn configure_worktree_clean_filter(root: &Path, tracked_path: &str) {
commit_filter_attributes(root, tracked_path);
let (extension_code, _, extension_err) = run(
root,
&["git", "config", "extensions.worktreeConfig", "true"],
);
assert_eq!(extension_code, 0, "enable worktree config: {extension_err}");
let (config_code, _, config_err) = run(
root,
&[
"git",
"config",
"--worktree",
"filter.x=y.clean",
"git config codex.filterran true && git hash-object --stdin",
],
);
assert_eq!(config_code, 0, "configure worktree filter: {config_err}");
}
fn configured_filter_ran(root: &Path) -> bool {
let (code, _, _) = run(root, &["git", "config", "--get", "codex.filterran"]);
code == 0
}
#[cfg(unix)]
fn trusted_git_directory() -> PathBuf {
std::env::split_paths(&std::env::var_os("PATH").expect("PATH"))
@@ -730,101 +648,6 @@ diff --git a/ghost.txt b/ghost.txt\n--- a/ghost.txt\n+++ b/ghost.txt\n@@ -1,1 +1
);
}
#[test]
fn apply_rejects_configured_clean_filter_without_running_it() {
let _g = env_lock().lock().unwrap();
let repo = init_repo();
let root = repo.path();
std::fs::write(root.join("file.txt"), "orig\n").expect("write file");
let (add_code, _, add_err) = run(root, &["git", "add", "file.txt"]);
assert_eq!(add_code, 0, "add file: {add_err}");
let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "seed"]);
assert_eq!(commit_code, 0, "commit file: {commit_err}");
configure_clean_filter(root, "file.txt");
let diff = "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1,1 +1,1 @@\n-orig\n+next\n";
for (revert, preflight) in [(false, false), (false, true), (true, false), (true, true)] {
let request = ApplyGitRequest {
cwd: root.to_path_buf(),
diff: diff.to_string(),
revert,
preflight,
};
reset_config_source_authorization_count();
let error = apply_git_patch(&request).expect_err("reject configured filter");
assert_eq!(
config_source_authorization_count(),
1,
"{revert} {preflight}"
);
assert_eq!(error.kind(), io::ErrorKind::Unsupported);
assert!(!configured_filter_ran(root));
assert_eq!(read_file_normalized(&root.join("file.txt")), "orig\n");
}
}
#[test]
fn apply_rejects_worktree_scoped_clean_filter_without_running_it() {
let _g = env_lock().lock().unwrap();
let repo = init_repo();
let root = repo.path();
std::fs::write(root.join("file.txt"), "orig\n").expect("write file");
let (add_code, _, add_err) = run(root, &["git", "add", "file.txt"]);
assert_eq!(add_code, 0, "add file: {add_err}");
let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "seed"]);
assert_eq!(commit_code, 0, "commit file: {commit_err}");
configure_worktree_clean_filter(root, "file.txt");
let request = ApplyGitRequest {
cwd: root.to_path_buf(),
diff: "diff --git a/file.txt b/file.txt\n--- a/file.txt\n+++ b/file.txt\n@@ -1,1 +1,1 @@\n-orig\n+next\n".to_string(),
revert: false,
preflight: true,
};
let error = apply_git_patch(&request).expect_err("reject worktree filter");
assert_eq!(error.kind(), io::ErrorKind::Unsupported);
assert!(!configured_filter_ran(root));
assert_eq!(read_file_normalized(&root.join("file.txt")), "orig\n");
}
#[test]
fn apply_probe_rejects_command_scoped_clean_filter() {
let _g = env_lock().lock().unwrap();
if std::env::var_os("CODEX_GIT_UTILS_APPLY_ENV_CHILD").is_none() {
run_isolated_test(
"apply::tests::apply_probe_rejects_command_scoped_clean_filter",
&[(
"CODEX_APPLY_GIT_CFG",
OsStr::new(
"filter.codex-test.clean=git config codex.filterran true && git hash-object --stdin",
),
)],
);
return;
}
let repo = init_repo();
let root = repo.path();
std::fs::write(root.join("test.txt"), "orig\n").expect("write file");
let (add_code, _, add_err) = run(root, &["git", "add", "test.txt"]);
assert_eq!(add_code, 0, "add file: {add_err}");
let (commit_code, _, commit_err) = run(root, &["git", "commit", "-m", "seed"]);
assert_eq!(commit_code, 0, "commit file: {commit_err}");
std::fs::write(root.join(".gitattributes"), "test.txt filter=codex-test\n")
.expect("attributes");
let request = ApplyGitRequest {
cwd: root.to_path_buf(),
diff: "diff --git a/test.txt b/test.txt\n--- a/test.txt\n+++ b/test.txt\n@@ -1 +1 @@\n-orig\n+next\n".to_string(),
revert: false,
preflight: true,
};
let error = apply_git_patch(&request).expect_err("reject command-scoped filter");
assert_eq!(error.kind(), io::ErrorKind::Unsupported);
assert!(!configured_filter_ran(root));
assert_eq!(read_file_normalized(&root.join("test.txt")), "orig\n");
}
#[test]
fn resolve_git_root_rejects_core_worktree_redirection() {
let temp = tempfile::tempdir().expect("tempdir");
codex-rs/git-utils/src/git_command_tests.rs
+40
View File
@@ -1514,6 +1514,46 @@ fn active_commondir_route_is_revalidated_before_every_child() {
);
}
#[cfg(any(unix, windows))]
#[test]
fn active_worktree_identity_is_revalidated_before_every_child() {
let fixture = tempfile::tempdir().expect("fixture");
let root = fixture.path().join("repo");
let moved_root = fixture.path().join("repo-moved");
std::fs::create_dir_all(&root).expect("create repository");
run_git(&root, &["init", "-q"]);
let runner = GitRunner::for_cwd(&root).expect("runner for original repository");
let sentinel = fixture.path().join("git-child-ran");
let mut command = runner
.command_for_cwd(&root)
.expect("command for original repository");
command
.args(["config", "--file"])
.arg(&sentinel)
.args(["probe.value", "ran"]);
std::fs::rename(&root, &moved_root).expect("move original repository");
std::fs::create_dir_all(&root).expect("create replacement repository");
run_git(&root, &["init", "-q"]);
assert!(moved_root.join(".git").is_dir());
assert!(root.join(".git").is_dir());
let error = runner
.output(command)
.expect_err("same-path repository replacement must block Git child");
assert_eq!(error.kind(), io::ErrorKind::PermissionDenied, "{error}");
assert!(
error
.to_string()
.contains("guarded Git repository identity changed before Git launch"),
"{error}"
);
assert!(
!sentinel.exists(),
"Git child executed after same-path repository replacement"
);
}
#[cfg(unix)]
#[test]
fn direct_external_separate_git_dir_absolute_and_relative_routes_execute_git() {
codex-rs/git-utils/src/git_config.rs
+91 -12
View File
@@ -5,6 +5,84 @@ use std::path::Path;
use crate::git_command::GitRunner;
pub(crate) fn parse_git_boolean(value: &[u8]) -> Option<bool> {
parse_git_boolean_with_minimum(value, i128::from(i32::MIN))
}
/// Parse the Git boolean grammar while excluding numeric `INT_MIN`.
///
/// Older supported Git releases reject that one signed endpoint after unit
/// expansion, so environment gates that must succeed across versions use this
/// conservative variant. The accepted spellings otherwise share one parser.
pub(crate) fn parse_git_boolean_symmetric_i32(value: &[u8]) -> Option<bool> {
parse_git_boolean_with_minimum(value, -i128::from(i32::MAX))
}
fn parse_git_boolean_with_minimum(value: &[u8], minimum: i128) -> Option<bool> {
if value.eq_ignore_ascii_case(b"true")
|| value.eq_ignore_ascii_case(b"yes")
|| value.eq_ignore_ascii_case(b"on")
{
return Some(true);
}
if value.is_empty()
|| value.eq_ignore_ascii_case(b"false")
|| value.eq_ignore_ascii_case(b"no")
|| value.eq_ignore_ascii_case(b"off")
{
return Some(false);
}
// Git parses the remaining boolean spellings through `git_parse_int`: C
// base-0 syntax, an optional binary-unit suffix, and signed `int` bounds.
let value = std::str::from_utf8(value)
.ok()?
.trim_start_matches(|value: char| value.is_ascii_whitespace());
let (negative, unsigned) = match value.as_bytes().first() {
Some(b'-') => (true, &value[1..]),
Some(b'+') => (false, &value[1..]),
Some(_) => (false, value),
None => return None,
};
let (base, unsigned) = if unsigned.starts_with("0x") || unsigned.starts_with("0X") {
(16, &unsigned[2..])
} else if unsigned.starts_with('0') {
(8, unsigned)
} else {
(10, unsigned)
};
let digit_count = unsigned
.bytes()
.take_while(|byte| match base {
8 => matches!(byte, b'0'..=b'7'),
10 => byte.is_ascii_digit(),
16 => byte.is_ascii_hexdigit(),
_ => false,
})
.count();
if digit_count == 0 {
return None;
}
let (digits, suffix) = unsigned.split_at(digit_count);
let factor = if suffix.is_empty() {
1_i128
} else if suffix.eq_ignore_ascii_case("k") {
1024
} else if suffix.eq_ignore_ascii_case("m") {
1024 * 1024
} else if suffix.eq_ignore_ascii_case("g") {
1024 * 1024 * 1024
} else {
return None;
};
let magnitude = i128::from_str_radix(digits, base).ok()?;
let signed = if negative { -magnitude } else { magnitude };
let value = signed.checked_mul(factor)?;
(minimum..=i128::from(i32::MAX))
.contains(&value)
.then_some(value != 0)
}
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub(crate) enum GitConfigScope {
Unknown,
@@ -77,18 +155,9 @@ pub(crate) fn parse_config_entries(output: &[u8]) -> io::Result<Vec<GitConfigEnt
/// Parse the `--show-origin` form used by Git versions that predate
/// `config --show-scope`. The record order still reflects effective config
/// precedence, which is what helper selection relies on. Scope is retained as
/// best-effort metadata only; command-line entries remain distinguishable.
#[cfg(test)]
pub(crate) fn parse_effective_config_with_origins(
output: &[u8],
) -> io::Result<BTreeMap<String, GitConfigEntry>> {
Ok(parse_config_entries_with_origins(output)?
.into_iter()
.map(|entry| (entry.key.clone(), entry))
.collect())
}
/// precedence, so duplicate include directives remain ordered. Scope is
/// retained as best-effort metadata only; command-line entries remain
/// distinguishable.
pub(crate) fn parse_config_entries_with_origins(output: &[u8]) -> io::Result<Vec<GitConfigEntry>> {
if output.is_empty() {
return Ok(Vec::new());
@@ -130,6 +199,16 @@ pub(crate) fn parse_config_entries_with_origins(output: &[u8]) -> io::Result<Vec
Ok(entries)
}
#[cfg(test)]
pub(crate) fn parse_effective_config_with_origins(
output: &[u8],
) -> io::Result<BTreeMap<String, GitConfigEntry>> {
Ok(parse_config_entries_with_origins(output)?
.into_iter()
.map(|entry| (entry.key.clone(), entry))
.collect())
}
pub(crate) fn read_effective_config_entries_with_fallback(
git: &GitRunner,
cwd: &Path,
codex-rs/git-utils/src/git_config_sources/primary_sources.rs
+3 -13
View File
@@ -7,6 +7,7 @@ use super::path_safety::CONFIG_PATH_KEY;
use super::path_safety::git_var_path_from_bytes;
use super::path_safety::invalid_config_source;
use crate::git_command::GitRunner;
use crate::git_config::parse_git_boolean_symmetric_i32;
pub(super) fn default_system_config_source_candidates(
git: &GitRunner,
@@ -259,19 +260,8 @@ fn git_env_bool(git: &GitRunner, name: &str) -> io::Result<bool> {
let value = value
.to_str()
.ok_or_else(|| invalid_config_source("non-UTF-8 Git boolean environment value"))?;
match value.to_ascii_lowercase().as_str() {
"1" | "true" | "yes" | "on" => Ok(true),
"" | "0" | "false" | "no" | "off" => Ok(false),
value => value
.parse::<i64>()
.ok()
// Supported Git releases disagree on whether INT_MIN is a valid
// numeric boolean. Accept only their shared symmetric range so a
// value approved here cannot make the selected Git fail later.
.filter(|value| (-i64::from(i32::MAX)..=i64::from(i32::MAX)).contains(value))
.map(|value| value != 0)
.ok_or_else(|| invalid_config_source("invalid Git boolean environment value")),
}
parse_git_boolean_symmetric_i32(value.as_bytes())
.ok_or_else(|| invalid_config_source("invalid Git boolean environment value"))
}
#[cfg(windows)]
codex-rs/git-utils/src/git_config_sources_tests.rs
+15 -3
View File
@@ -402,9 +402,8 @@ fn rejects_worktree_fifo_primary_source_without_opening_it() {
}
#[test]
fn git_config_nosystem_accepts_cross_version_boolean_values() {
const TEST_NAME: &str =
"git_config_sources::tests::git_config_nosystem_accepts_cross_version_boolean_values";
fn git_config_nosystem_uses_the_shared_parser_with_a_cross_version_numeric_subset() {
const TEST_NAME: &str = "git_config_sources::tests::git_config_nosystem_uses_the_shared_parser_with_a_cross_version_numeric_subset";
if std::env::var_os("CODEX_GIT_CONFIG_SOURCE_CHILD").is_none() {
let repo = init_repo();
let unsafe_system = repo.path().join("system.gitconfig");
@@ -415,6 +414,11 @@ fn git_config_nosystem_accepts_cross_version_boolean_values() {
("-1", "ignored"),
("01", "ignored"),
("+1", "ignored"),
("0x1", "ignored"),
("010", "ignored"),
("1k", "ignored"),
("-1g", "ignored"),
(" 1", "ignored"),
("2147483647", "ignored"),
("-2147483647", "ignored"),
("true", "ignored"),
@@ -427,9 +431,17 @@ fn git_config_nosystem_accepts_cross_version_boolean_values() {
("no", "rejected"),
("off", "rejected"),
("not-a-bool", "invalid"),
("08", "invalid"),
("2147483648", "invalid"),
("-2147483648", "invalid"),
("-0x80000000", "invalid"),
("-020000000000", "invalid"),
("-2097152k", "invalid"),
("-2048m", "invalid"),
("-2g", "invalid"),
(" -2G", "invalid"),
("-2147483649", "invalid"),
("2g", "invalid"),
] {
run_isolated_source_test(
TEST_NAME,
codex-rs/git-utils/src/git_config_tests.rs
+100 -1
View File
@@ -28,6 +28,105 @@ command\0command line:\0merge.Name.driver\nhelper %A %B\0";
);
}
#[test]
fn git_boolean_accepts_native_git_spellings() {
for (value, expected) in [
(b"true".as_slice(), true),
(b"TRUE".as_slice(), true),
(b"yes".as_slice(), true),
(b"On".as_slice(), true),
(b"1".as_slice(), true),
(b"2".as_slice(), true),
(b"-1".as_slice(), true),
(b"+1".as_slice(), true),
(b"01".as_slice(), true),
(b"0x1".as_slice(), true),
(b"0Xf".as_slice(), true),
(b"010".as_slice(), true),
(b"1k".as_slice(), true),
(b"1M".as_slice(), true),
(b"1g".as_slice(), true),
(b" 1".as_slice(), true),
(b"2147483647".as_slice(), true),
(b"-2147483648".as_slice(), true),
(b"-2g".as_slice(), true),
(b"false".as_slice(), false),
(b"FALSE".as_slice(), false),
(b"no".as_slice(), false),
(b"Off".as_slice(), false),
(b"".as_slice(), false),
(b"0".as_slice(), false),
(b"-0".as_slice(), false),
(b"+0".as_slice(), false),
(b"00".as_slice(), false),
(b"0x0".as_slice(), false),
(b"0k".as_slice(), false),
] {
assert_eq!(parse_git_boolean(value), Some(expected), "value {value:?}");
}
}
#[test]
fn git_boolean_rejects_values_native_git_rejects() {
for value in [
b"08".as_slice(),
b"-08".as_slice(),
b"2147483648".as_slice(),
b"-2147483649".as_slice(),
b"9223372036854775807".as_slice(),
b"2g".as_slice(),
b"-3g".as_slice(),
b"0x".as_slice(),
b"0b1".as_slice(),
b"1kb".as_slice(),
b"1foo".as_slice(),
b"1_0".as_slice(),
b"1 ".as_slice(),
b" ".as_slice(),
b"+".as_slice(),
b"-".as_slice(),
b"not-a-bool".as_slice(),
b"\xff".as_slice(),
] {
assert_eq!(parse_git_boolean(value), None, "value {value:?}");
}
}
#[test]
fn symmetric_git_boolean_parser_excludes_only_int_min_spellings() {
for value in [
b"-2147483648".as_slice(),
b"-0x80000000".as_slice(),
b"-020000000000".as_slice(),
b"-2097152k".as_slice(),
b"-2048m".as_slice(),
b"-2g".as_slice(),
b" -2G".as_slice(),
] {
assert_eq!(parse_git_boolean(value), Some(true), "value {value:?}");
assert_eq!(
parse_git_boolean_symmetric_i32(value),
None,
"value {value:?}"
);
}
for value in [
b"0x1".as_slice(),
b"010".as_slice(),
b"1k".as_slice(),
b"-1g".as_slice(),
b" 1".as_slice(),
b"-2147483647".as_slice(),
] {
assert_eq!(
parse_git_boolean_symmetric_i32(value),
parse_git_boolean(value),
"value {value:?}"
);
}
}
#[test]
fn entry_parsers_preserve_duplicate_include_directives_in_order() {
let scoped = b"local\0file:.git/config\0include.path\n../unsafe.gitconfig\0\
@@ -80,7 +179,7 @@ fn rejects_malformed_config_records() {
b"global\0\0key\nvalue\0".as_slice(),
b"global\0file:/tmp/config\0\nvalue\0".as_slice(),
] {
assert!(parse_effective_config(output).is_err(), "{output:?}");
assert!(parse_config_entries(output).is_err(), "{output:?}");
}
}
codex-rs/git-utils/src/patch_paths.rs
+13 -130
View File
@@ -4,12 +4,9 @@ use std::io;
use std::path::Path;
use std::path::PathBuf;
use crate::apply::run_git;
use crate::apply::safe_git_config_parts;
use crate::apply::write_temp_patch;
use crate::git_command::GitRunner;
use crate::git_config::path_is_within;
use crate::git_config_sources::ensure_no_worktree_config_sources;
use crate::guarded_config::GuardedGitConfig;
/// Extract effective patch paths through a bound operation configuration.
@@ -23,24 +20,6 @@ pub(crate) fn extract_effective_paths_from_patch_guarded(
normalize_effective_patch_paths(forward_paths, reverse_paths)
}
/// Extract paths with Git from a cwd whose config sources have already been
/// authorized for `git_config_args`.
pub(crate) fn extract_effective_paths_from_patch(
git: &GitRunner,
authorized_cwd: &Path,
patch_path: &Path,
revert: bool,
git_config_args: &[String],
) -> io::Result<Vec<String>> {
let forward_paths =
git_apply_numstat_paths(git, authorized_cwd, patch_path, revert, git_config_args)?;
// `git apply --numstat` reports only the destination of a rename. Parse the
// opposite orientation too so both endpoints are included in the result.
let reverse_paths =
git_apply_numstat_paths(git, authorized_cwd, patch_path, !revert, git_config_args)?;
normalize_effective_patch_paths(forward_paths, reverse_paths)
}
fn normalize_effective_patch_paths(
forward_paths: Vec<String>,
reverse_paths: Vec<String>,
@@ -109,42 +88,14 @@ fn extract_paths_from_patch_from_cwd(diff_text: &str, cwd: &Path) -> Vec<String>
let git_root = crate::get_git_repo_root(cwd)
.ok_or_else(|| io::Error::other("not a Git repository"))?;
let git_root = std::fs::canonicalize(git_root)?;
ensure_no_worktree_config_sources(&git, &git_root, &[])?;
extract_effective_paths_from_patch(&git, &git_root, &patch_path, /*revert*/ false, &[])
let config = GuardedGitConfig::authorize(&git, &git_root, Vec::new())?;
extract_effective_paths_from_patch_guarded(&config, &patch_path, /*revert*/ false)
})()
.unwrap_or_default();
drop(tmpdir);
paths
}
fn git_apply_numstat_paths(
git: &GitRunner,
authorized_cwd: &Path,
patch_path: &Path,
revert: bool,
git_config_args: &[String],
) -> io::Result<Vec<String>> {
let mut cmd = git.command_for_cwd(authorized_cwd)?;
cmd.args(git_config_args);
cmd.args(["apply", "--numstat", "-z"]);
if revert {
cmd.arg("-R");
}
cmd.arg("--").arg(patch_path);
let out = git.output(cmd)?;
if !out.status.success() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
format!(
"failed to parse patch paths: {}",
String::from_utf8_lossy(&out.stderr).trim()
),
));
}
parse_numstat_paths(&out.stdout)
}
fn parse_numstat_paths(output: &[u8]) -> io::Result<Vec<String>> {
if !output.is_empty() && !output.ends_with(&[0]) {
return Err(io::Error::new(
@@ -287,51 +238,16 @@ fn invalid_windows_patch_component(component: &str) -> bool {
/// Stage only the files that actually exist on disk for the given diff.
pub fn stage_paths(git_root: &Path, diff: &str) -> io::Result<()> {
let git = GitRunner::for_cwd_io(git_root)?;
let git_config_args = safe_git_config_parts();
ensure_no_worktree_config_sources(&git, git_root, &git_config_args)?;
let requested_cwd = std::fs::canonicalize(git_root)?;
let canonical_root = crate::get_git_repo_root(&requested_cwd)
.ok_or_else(|| io::Error::other("not a Git repository"))
.and_then(std::fs::canonicalize)?;
let config = GuardedGitConfig::authorize(&git, &canonical_root, Vec::new())?;
let (tmpdir, patch_path) = write_temp_patch(diff)?;
let paths = extract_effective_paths_from_patch(
&git,
git_root,
&patch_path,
/*revert*/ true,
&git_config_args,
)?;
let paths =
extract_effective_paths_from_patch_guarded(&config, &patch_path, /*revert*/ true)?;
let _guard = tmpdir;
stage_effective_paths(&git, git_root, &paths, &git_config_args)
}
pub(crate) fn stage_effective_paths(
git: &GitRunner,
git_root: &Path,
paths: &[String],
git_config_args: &[String],
) -> io::Result<()> {
let confined = confine_patch_paths(git, git_root, paths)?;
let mut existing = Vec::new();
for path in confined.into_exact_leaves()? {
let joined = git_root.join(&path);
if let Ok(metadata) = std::fs::symlink_metadata(&joined) {
if leaf_is_traversable_directory(metadata.file_type()) {
return Err(containment_error(
"refusing to recursively stage a directory patch path",
));
}
existing.push(path);
}
}
if existing.is_empty() {
return Ok(());
}
let mut args = vec![
"--literal-pathspecs".to_string(),
"add".to_string(),
"--".to_string(),
];
args.extend(existing);
let (_code, _, _) = run_git(git, git_root, git_config_args, &args)?;
// We do not hard fail staging; best-effort is OK. Return Ok even on non-zero.
Ok(())
stage_effective_paths_guarded(&config, &paths)
}
/// Preserve the existing reverse staging behavior while constructing every
@@ -449,14 +365,14 @@ impl ConfinedPatchPaths {
}
}
#[cfg(test)]
pub(crate) fn confine_patch_paths(
git: &GitRunner,
git_root: &Path,
paths: &[String],
) -> io::Result<ConfinedPatchPaths> {
let canonical_root = std::fs::canonicalize(git_root)?;
let metadata_dirs = canonical_git_metadata_dirs(git, &canonical_root)?;
confine_patch_paths_with_metadata(&canonical_root, paths, &metadata_dirs)
let config = GuardedGitConfig::authorize(git, git_root, Vec::new())?;
confine_patch_paths_guarded(&config, paths)
}
fn confine_patch_paths_guarded(
@@ -604,39 +520,6 @@ fn containment_error(message: &'static str) -> io::Error {
io::Error::new(io::ErrorKind::PermissionDenied, message)
}
fn canonical_git_metadata_dirs(git: &GitRunner, git_root: &Path) -> io::Result<Vec<PathBuf>> {
let config_parts = safe_git_config_parts();
let queries = [
vec!["rev-parse".to_string(), "--absolute-git-dir".to_string()],
vec!["rev-parse".to_string(), "--git-common-dir".to_string()],
];
let mut metadata_dirs = std::collections::BTreeSet::new();
for args in queries {
let (code, stdout, stderr) = run_git(git, git_root, &config_parts, &args)?;
if code != 0 {
return Err(io::Error::other(format!(
"failed to resolve Git repository metadata (exit {code}): {}",
stderr.trim()
)));
}
let path = stdout.trim_end_matches(['\r', '\n']);
if path.is_empty() {
return Err(io::Error::new(
io::ErrorKind::InvalidData,
"Git returned an empty repository metadata path",
));
}
let path = PathBuf::from(path);
let absolute = if path.is_absolute() {
path
} else {
git_root.join(path)
};
metadata_dirs.insert(std::fs::canonicalize(absolute)?);
}
Ok(metadata_dirs.into_iter().collect())
}
fn canonical_git_metadata_dirs_guarded(config: &GuardedGitConfig<'_>) -> io::Result<Vec<PathBuf>> {
let queries = [
["rev-parse", "--absolute-git-dir"],
codex-rs/git-utils/src/patch_paths_tests.rs
+2 -2
View File
@@ -68,8 +68,8 @@ fn effective_paths(diff: &str, revert: bool) -> io::Result<Vec<String>> {
let git_root =
crate::get_git_repo_root(cwd).ok_or_else(|| io::Error::other("not a Git repository"))?;
let git_root = std::fs::canonicalize(git_root)?;
ensure_no_worktree_config_sources(&git, &git_root, &[])?;
let paths = extract_effective_paths_from_patch(&git, &git_root, &patch_path, revert, &[])?;
let config = GuardedGitConfig::authorize(&git, &git_root, Vec::new())?;
let paths = extract_effective_paths_from_patch_guarded(&config, &patch_path, revert)?;
drop(tmpdir);
Ok(paths)
}
codex-rs/git-utils/src/repository_authority/authority.rs
+41 -8
View File
@@ -2,6 +2,8 @@ use std::io;
use std::path::Path;
use std::path::PathBuf;
use same_file::Handle;
use crate::errors::GitReadError;
use crate::git_config::path_is_within;
use crate::path_authority::RepositoryRouteBoundaries;
@@ -29,6 +31,7 @@ mod policy;
#[derive(Debug)]
pub(crate) struct RepositoryAuthority {
active_worktree_root: PathBuf,
active_worktree_identity: Option<Handle>,
roots: Vec<PathBuf>,
worktree_roots: Vec<PathBuf>,
common_dirs: Vec<PathBuf>,
@@ -57,8 +60,11 @@ impl RepositoryAuthority {
let worktree_root = crate::get_git_repo_root(&canonical_cwd)
.and_then(|root| std::fs::canonicalize(root).ok())
.unwrap_or_else(|| canonical_cwd.clone());
let active_worktree_identity = Handle::from_path(&worktree_root)
.map_err(|error| invalid_metadata(&worktree_root, error))?;
let mut authority = Self {
active_worktree_root: worktree_root.clone(),
active_worktree_identity: Some(active_worktree_identity),
roots: Vec::new(),
worktree_roots: Vec::new(),
common_dirs: Vec::new(),
@@ -116,6 +122,7 @@ impl RepositoryAuthority {
}
pub(crate) fn revalidate_active_repository_metadata(&self) -> io::Result<()> {
self.revalidate_active_worktree_identity()?;
let Some(expected) = &self.active_metadata else {
return Ok(());
};
@@ -150,13 +157,17 @@ impl RepositoryAuthority {
pub(crate) fn ensure_active_worktree_root(&self, root: &Path) -> io::Result<()> {
let canonical_root = std::fs::canonicalize(root)?;
let same_root = canonical_root == self.active_worktree_root
|| same_file::is_same_file(&canonical_root, &self.active_worktree_root)?;
let same_root = if let Some(expected) = &self.active_worktree_identity {
Handle::from_path(&canonical_root)? == *expected
} else {
canonical_root == self.active_worktree_root
|| same_file::is_same_file(&canonical_root, &self.active_worktree_root)?
};
if !same_root {
return Err(io::Error::new(
io::ErrorKind::PermissionDenied,
format!(
"guarded Git repository root {} does not match runner repository {}",
"guarded Git repository identity {} does not match runner repository {}",
canonical_root.display(),
self.active_worktree_root.display()
),
@@ -165,6 +176,25 @@ impl RepositoryAuthority {
Ok(())
}
fn revalidate_active_worktree_identity(&self) -> io::Result<()> {
let Some(expected) = &self.active_worktree_identity else {
return Ok(());
};
let actual = Handle::from_path(&self.active_worktree_root).map_err(|error| {
io::Error::new(
io::ErrorKind::PermissionDenied,
format!("guarded Git repository identity could not be revalidated: {error}"),
)
})?;
if &actual != expected {
return Err(io::Error::new(
io::ErrorKind::PermissionDenied,
"guarded Git repository identity changed before Git launch",
));
}
Ok(())
}
#[cfg(test)]
pub(crate) fn from_test_locations(
roots: Vec<PathBuf>,
@@ -173,12 +203,15 @@ impl RepositoryAuthority {
) -> Result<Self, GitReadError> {
let route_boundaries = repository_route_boundaries(&worktree_roots, &common_dirs)
.map_err(|error| invalid_metadata(Path::new("<test>"), error))?;
let active_worktree_root = worktree_roots
.first()
.or_else(|| roots.first())
.cloned()
.unwrap_or_default();
let active_worktree_identity = Handle::from_path(&active_worktree_root).ok();
Ok(Self {
active_worktree_root: worktree_roots
.first()
.or_else(|| roots.first())
.cloned()
.unwrap_or_default(),
active_worktree_root,
active_worktree_identity,
roots,
worktree_roots,
common_dirs,
codex-rs/git-utils/src/repository_authority/plain_config.rs
+22 -31
View File
@@ -4,6 +4,7 @@ use std::path::PathBuf;
use super::bytes_to_path;
use super::read_bounded_file;
use crate::git_config::parse_git_boolean;
#[derive(Debug, Eq, PartialEq)]
pub(crate) enum CommonConfigAuthority {
@@ -50,7 +51,7 @@ pub(crate) fn inspect_plain_common_config_authority(
return Ok(CommonConfigAuthority::Unproven);
}
let bare = match unique_explicit_value(&config, "core", "bare") {
Ok(Some(value)) => match explicit_boolean(value.as_ref()) {
Ok(Some(value)) => match parse_git_boolean(value.as_ref()) {
Some(value) => Some(value),
None => return Ok(CommonConfigAuthority::Unproven),
},
@@ -58,7 +59,7 @@ pub(crate) fn inspect_plain_common_config_authority(
Err(()) => return Ok(CommonConfigAuthority::Unproven),
};
match unique_explicit_value(&config, "extensions", "worktreeConfig") {
Ok(Some(value)) if explicit_boolean(value.as_ref()) == Some(false) => {}
Ok(Some(value)) if parse_git_boolean(value.as_ref()) == Some(false) => {}
Ok(None) => {}
Ok(Some(_)) | Err(()) => return Ok(CommonConfigAuthority::Unproven),
}
@@ -99,39 +100,29 @@ fn unique_explicit_value(
let Some(sections) = config.sections_by_name(section_name) else {
return Ok(None);
};
let mut occurrences = 0usize;
let mut explicit_values = Vec::new();
let mut occurrence = None;
for section in sections {
if section.header().subsection_name().is_some() {
return Err(());
}
occurrences += section
.value_names()
.filter(|name| {
let name: &str = name.as_ref();
name.eq_ignore_ascii_case(value_name)
})
.count();
explicit_values.extend(
section
.values(value_name)
.into_iter()
.map(|value| value.as_ref().to_vec()),
);
for name in section.value_names() {
let name: &str = name.as_ref();
if !name.eq_ignore_ascii_case(value_name) {
continue;
}
if occurrence.is_some() {
return Err(());
}
let value = section
.value_implicit(name)
.ok_or(())?
.map(|value| value.as_ref().to_vec());
occurrence = Some(value);
}
}
match (occurrences, explicit_values.len()) {
(0, 0) => Ok(None),
(1, 1) => Ok(explicit_values.pop()),
_ => Err(()),
}
}
fn explicit_boolean(value: &[u8]) -> Option<bool> {
if value.eq_ignore_ascii_case(b"true") {
Some(true)
} else if value.eq_ignore_ascii_case(b"false") {
Some(false)
} else {
None
match occurrence {
None => Ok(None),
Some(Some(value)) => Ok(Some(value)),
Some(None) => Err(()),
}
}
codex-rs/git-utils/src/repository_authority_tests.rs
+84 -5
View File
@@ -81,6 +81,36 @@ fn plain_common_bare_parser_matches_native_boolean_syntax_and_is_stricter_on_dup
CommonConfigAuthority::Bare,
Some(true),
),
(
"yes",
b"[core]\n\tbare = yes\n".as_slice(),
CommonConfigAuthority::Bare,
Some(true),
),
(
"on",
b"[core]\n\tbare = on\n".as_slice(),
CommonConfigAuthority::Bare,
Some(true),
),
(
"numeric true",
b"[core]\n\tbare = 1\n".as_slice(),
CommonConfigAuthority::Bare,
Some(true),
),
(
"hex true",
b"[core]\n\tbare = 0x1\n".as_slice(),
CommonConfigAuthority::Bare,
Some(true),
),
(
"suffixed true",
b"[core]\n\tbare = 1k\n".as_slice(),
CommonConfigAuthority::Bare,
Some(true),
),
(
"false",
b"[core]\n\tbare = false\n".as_slice(),
@@ -133,13 +163,13 @@ fn plain_common_bare_parser_matches_native_boolean_syntax_and_is_stricter_on_dup
}
#[test]
fn common_bare_proof_accepts_only_explicit_boolean_literals() {
fn common_bare_proof_rejects_invalid_git_boolean_values() {
for (name, body) in [
("empty", b"[core]\n\tbare =\n".as_slice()),
("yes", b"[core]\n\tbare = yes\n".as_slice()),
("on", b"[core]\n\tbare = on\n".as_slice()),
("one", b"[core]\n\tbare = 1\n".as_slice()),
("leading zero", b"[core]\n\tbare = 08\n".as_slice()),
(
"negative leading zero",
b"[core]\n\tbare = -08\n".as_slice(),
),
(
"positive overflow",
b"[core]\n\tbare = 2147483648\n".as_slice(),
@@ -152,6 +182,8 @@ fn common_bare_proof_accepts_only_explicit_boolean_literals() {
"i64 max",
b"[core]\n\tbare = 9223372036854775807\n".as_slice(),
),
("suffix overflow", b"[core]\n\tbare = 2g\n".as_slice()),
("trailing junk", b"[core]\n\tbare = 1foo\n".as_slice()),
] {
let common = write_common_config(body);
assert_eq!(
@@ -162,6 +194,34 @@ fn common_bare_proof_accepts_only_explicit_boolean_literals() {
}
}
#[test]
fn common_bare_proof_accepts_explicit_false_worktree_config_spellings() {
for value in ["false", "no", "off", "", "0", "-0", "+0", "00", "0x0"] {
let common = write_common_config(
format!("[core]\n\tbare = yes\n[extensions]\n\tworktreeConfig = {value}\n").as_bytes(),
);
assert_eq!(
inspect_plain_common_config_authority(common.path()).expect(value),
CommonConfigAuthority::Bare,
"authority result for worktreeConfig={value:?}"
);
}
}
#[test]
fn common_bare_proof_rejects_explicit_true_worktree_config_spellings() {
for value in ["true", "yes", "on", "1", "-1", "0x1", "1k"] {
let common = write_common_config(
format!("[core]\n\tbare = yes\n[extensions]\n\tworktreeConfig = {value}\n").as_bytes(),
);
assert_eq!(
inspect_plain_common_config_authority(common.path()).expect(value),
CommonConfigAuthority::Unproven,
"authority result for worktreeConfig={value:?}"
);
}
}
#[test]
fn common_bare_proof_rejects_includes_worktree_config_and_relative_worktree() {
let include_target = tempfile::NamedTempFile::new().expect("included config");
@@ -226,6 +286,25 @@ fn common_config_absolute_worktree_is_returned_as_authority() {
);
}
#[test]
fn common_config_false_bare_spellings_allow_absolute_worktree() {
let worktree = tempfile::tempdir().expect("worktree");
for bare in ["false", "no", "off", "", "0", "-0", "+0", "00", "0x0"] {
let common = write_common_config(
format!(
"[core]\n\tbare = {bare}\n\tworktree = {}\n",
git_config_path(worktree.path())
)
.as_bytes(),
);
assert_eq!(
inspect_plain_common_config_authority(common.path()).expect(bare),
CommonConfigAuthority::Worktree(worktree.path().to_path_buf()),
"authority result for core.bare={bare:?}"
);
}
}
#[test]
fn common_config_rejects_contradictory_or_malformed_bare_with_absolute_worktree() {
let worktree = tempfile::tempdir().expect("worktree");