agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(tui): prevent repository-configured code execution in /diff (#24954)

Browse Source 
## Why

`/diff` is intended to display working-tree changes, but its Git
invocations honored repository-selected executable helpers. A repository
could configure diff/text conversion helpers, clean/process filters,
`core.fsmonitor`, or `post-index-change` hooks that execute when a user
runs `/diff`.

Fixes
[PSEC-4395](https://linear.app/openai/issue/PSEC-4395/codex-cli-diff-executes-repository-selected-diff-helpers).

## What Changed

- Pass `--no-textconv` and `--no-ext-diff` for tracked and untracked
diff generation.
- Discover configured `filter.<driver>.clean` and `.process` entries,
then neutralize the selected drivers through structured
`GIT_CONFIG_KEY_*` / `GIT_CONFIG_VALUE_*` overrides, including driver
names containing `=`.
- Run all `/diff` Git probes with `core.fsmonitor=false` and a null
`core.hooksPath`.
- Use short submodule reporting while ignoring dirty submodule
worktrees, since inspecting a checked-out submodule for dirtiness can
execute filters from that child repository. This intentionally omits
dirty-only submodule markers in order to preserve the non-executing
security boundary.
- Add real-Git marker tests covering filters, fsmonitor, hooks, and
configured helpers inside checked-out submodules.

## How to Test

1. In a repository with ordinary tracked and untracked edits, run
`/diff`.
2. Confirm the normal working-tree diff is shown for top-level files.
3. Run the targeted tests below; they configure executable marker
helpers for repository filters, fsmonitor, hooks, and a checked-out
submodule, then verify `/diff` does not invoke them.
4. Confirm a dirty-only submodule does not cause Codex to enter the
submodule and execute its configured helper.

Targeted tests:
- `just test -p codex-tui get_git_diff_`

Validation note: `just test -p codex-tui` runs the new coverage, but
this worktree currently also has two unrelated failing guardian tests:
`app::tests::update_feature_flags_disabling_guardian_clears_review_policy_and_restores_default`
and
`app::tests::update_feature_flags_disabling_guardian_clears_manual_review_policy_without_history`.
This commit is contained in:
Felipe Coury
2026-05-28 16:53:59 -03:00
committed by GitHub
parent b90ec46387
commit 2e0c4f4977
1 changed files with 446 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui/src/get_git_diff.rs
+446 -47
View File
@@ -13,6 +13,13 @@ use crate::workspace_command::WorkspaceCommandExecutor;
use crate::workspace_command::WorkspaceCommandOutput;
const DIFF_COMMAND_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 30);
const DISABLE_FSMONITOR_CONFIG: &str = "core.fsmonitor=false";
const DISABLE_HOOKS_CONFIG: &str = if cfg!(windows) {
"core.hooksPath=NUL"
} else {
"core.hooksPath=/dev/null"
};
const EXECUTABLE_FILTER_CONFIG_PATTERN: &str = r"^filter\..*\.(clean|process)$";
/// Return value of [`get_git_diff`].
///
@@ -27,9 +34,22 @@ pub(crate) async fn get_git_diff(
return Ok((false, String::new()));
}
// Run tracked diff and untracked file listing in parallel.
// Keep `/diff` informational: repository configuration must not select executable diff helpers.
let diff_config_overrides = diff_filter_config_overrides(runner, cwd).await?;
let (tracked_diff_res, untracked_output_res) = tokio::join!(
run_git_capture_diff(runner, cwd, &["diff", "--color"]),
run_git_capture_diff(
runner,
cwd,
&diff_config_overrides,
&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
]
),
run_git_capture_stdout(runner, cwd, &["ls-files", "--others", "--exclude-standard"]),
);
let tracked_diff = tracked_diff_res?;
@@ -48,8 +68,19 @@ pub(crate) async fn get_git_diff(
.map(str::trim)
.filter(|s| !s.is_empty())
{
let args = ["diff", "--color", "--no-index", "--", null_path, file];
let diff = run_git_capture_diff(runner, cwd, &args).await?;
let args = [
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
"--no-index",
"--",
null_path,
file,
];
let diff = run_git_capture_diff(runner, cwd, &diff_config_overrides, &args).await?;
untracked_diff.push_str(&diff);
}
@@ -63,7 +94,7 @@ async fn run_git_capture_stdout(
cwd: &Path,
args: &[&str],
) -> Result<String, String> {
let output = run_git_command(runner, cwd, args).await?;
let output = run_git_command(runner, cwd, &[], args).await?;
if output.success() {
Ok(output.stdout)
} else {
@@ -79,9 +110,10 @@ async fn run_git_capture_stdout(
async fn run_git_capture_diff(
runner: &dyn WorkspaceCommandExecutor,
cwd: &Path,
config_overrides: &[(String, String)],
args: &[&str],
) -> Result<String, String> {
let output = run_git_command(runner, cwd, args).await?;
let output = run_git_command(runner, cwd, config_overrides, args).await?;
if output.success() || output.exit_code == 1 {
Ok(output.stdout)
} else {
@@ -92,32 +124,88 @@ async fn run_git_capture_diff(
}
}
/// Return Git configuration overrides that prevent configured filter drivers
/// from executing while generating diffs.
async fn diff_filter_config_overrides(
runner: &dyn WorkspaceCommandExecutor,
cwd: &Path,
) -> Result<Vec<(String, String)>, String> {
let args = [
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
];
let output = run_git_command(runner, cwd, &[], &args).await?;
if output.exit_code != 0 && output.exit_code != 1 {
return Err(format!(
"git {:?} failed with status {}",
args, output.exit_code
));
}
let mut drivers = output
.stdout
.split('\0')
.filter_map(|key| {
key.strip_suffix(".clean")
.or_else(|| key.strip_suffix(".process"))
})
.map(str::to_string)
.collect::<Vec<_>>();
drivers.sort();
drivers.dedup();
Ok(drivers
.into_iter()
.flat_map(|driver| {
[
(format!("{driver}.clean"), String::new()),
(format!("{driver}.process"), String::new()),
(format!("{driver}.required"), "false".to_string()),
]
})
.collect())
}
/// Determine if the current directory is inside a Git repository.
async fn inside_git_repo(
runner: &dyn WorkspaceCommandExecutor,
cwd: &Path,
) -> Result<bool, String> {
let output = run_git_command(runner, cwd, &["rev-parse", "--is-inside-work-tree"]).await?;
let output = run_git_command(runner, cwd, &[], &["rev-parse", "--is-inside-work-tree"]).await?;
Ok(output.success())
}
async fn run_git_command(
runner: &dyn WorkspaceCommandExecutor,
cwd: &Path,
config_overrides: &[(String, String)],
args: &[&str],
) -> Result<WorkspaceCommandOutput, String> {
let mut argv = Vec::with_capacity(args.len() + 1);
let mut argv = Vec::with_capacity(args.len() + 5);
argv.push("git".to_string());
argv.extend([
"-c".to_string(),
DISABLE_FSMONITOR_CONFIG.to_string(),
"-c".to_string(),
DISABLE_HOOKS_CONFIG.to_string(),
]);
argv.extend(args.iter().map(|arg| (*arg).to_string()));
runner
.run(
WorkspaceCommand::new(argv)
.cwd(cwd.to_path_buf())
.timeout(DIFF_COMMAND_TIMEOUT)
.disable_output_cap(),
)
.await
.map_err(|err| err.to_string())
let mut command = WorkspaceCommand::new(argv)
.cwd(cwd.to_path_buf())
.timeout(DIFF_COMMAND_TIMEOUT)
.disable_output_cap();
if !config_overrides.is_empty() {
command = command.env("GIT_CONFIG_COUNT", config_overrides.len().to_string());
for (index, (key, value)) in config_overrides.iter().enumerate() {
command = command
.env(format!("GIT_CONFIG_KEY_{index}"), key)
.env(format!("GIT_CONFIG_VALUE_{index}"), value);
}
}
runner.run(command).await.map_err(|err| err.to_string())
}
#[cfg(test)]
@@ -125,17 +213,24 @@ mod tests {
use super::*;
use crate::workspace_command::WorkspaceCommandError;
use pretty_assertions::assert_eq;
use std::collections::HashMap;
use std::collections::VecDeque;
#[cfg(unix)]
use std::fs;
use std::future::Future;
#[cfg(unix)]
use std::os::unix::fs::PermissionsExt;
use std::path::PathBuf;
use std::pin::Pin;
#[cfg(unix)]
use std::process::Command as ProcessCommand;
use std::sync::Mutex;
#[tokio::test]
async fn get_git_diff_returns_not_git_for_non_git_cwd() {
let cwd = PathBuf::from("/workspace");
let runner = FakeRunner::new(vec![response(
&["git", "rev-parse", "--is-inside-work-tree"],
git_command(&["rev-parse", "--is-inside-work-tree"]),
/*exit_code*/ 128,
"",
)]);
@@ -145,40 +240,61 @@ mod tests {
assert_eq!(result, Ok((false, String::new())));
assert_commands(
&runner.commands(),
&[&["git", "rev-parse", "--is-inside-work-tree"]],
&[git_command(&["rev-parse", "--is-inside-work-tree"])],
&cwd,
);
}
#[tokio::test]
async fn get_git_diff_concatenates_tracked_and_untracked_diffs() {
async fn get_git_diff_disables_helpers_for_tracked_and_untracked_diffs() {
let cwd = PathBuf::from("/workspace");
let runner = FakeRunner::new(vec![
response(
&["git", "rev-parse", "--is-inside-work-tree"],
git_command(&["rev-parse", "--is-inside-work-tree"]),
/*exit_code*/ 0,
"true\n",
),
response(
&["git", "diff", "--color"],
git_command(&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
]),
/*exit_code*/ 0,
"filter.evil.clean\0filter.evil.process\0",
),
response(
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
]),
/*exit_code*/ 1,
"tracked\n",
),
response(
&["git", "ls-files", "--others", "--exclude-standard"],
git_command(&["ls-files", "--others", "--exclude-standard"]),
/*exit_code*/ 0,
"new.txt\n",
),
response(
&[
"git",
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
"--no-index",
"--",
null_device(),
"new.txt",
],
]),
/*exit_code*/ 1,
"untracked\n",
),
@@ -187,24 +303,44 @@ mod tests {
let result = get_git_diff(&runner, &cwd).await;
assert_eq!(result, Ok((true, "tracked\nuntracked\n".to_string())));
let commands = runner.commands();
assert_commands(
&runner.commands(),
&commands,
&[
&["git", "rev-parse", "--is-inside-work-tree"],
&["git", "diff", "--color"],
&["git", "ls-files", "--others", "--exclude-standard"],
&[
"git",
git_command(&["rev-parse", "--is-inside-work-tree"]),
git_command(&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
]),
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
]),
git_command(&["ls-files", "--others", "--exclude-standard"]),
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
"--no-index",
"--",
null_device(),
"new.txt",
],
]),
],
&cwd,
);
assert_eq!(commands[2].env, filter_override_env("filter.evil"));
assert_eq!(commands[4].env, filter_override_env("filter.evil"));
}
#[tokio::test]
@@ -212,17 +348,35 @@ mod tests {
let cwd = PathBuf::from("/workspace");
let runner = FakeRunner::new(vec![
response(
&["git", "rev-parse", "--is-inside-work-tree"],
git_command(&["rev-parse", "--is-inside-work-tree"]),
/*exit_code*/ 0,
"true\n",
),
response(
&["git", "diff", "--color"],
git_command(&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
]),
/*exit_code*/ 1,
"",
),
response(
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
]),
/*exit_code*/ 1,
"tracked\n",
),
response(
&["git", "ls-files", "--others", "--exclude-standard"],
git_command(&["ls-files", "--others", "--exclude-standard"]),
/*exit_code*/ 0,
"",
),
@@ -238,13 +392,35 @@ mod tests {
let cwd = PathBuf::from("/workspace");
let runner = FakeRunner::new(vec![
response(
&["git", "rev-parse", "--is-inside-work-tree"],
git_command(&["rev-parse", "--is-inside-work-tree"]),
/*exit_code*/ 0,
"true\n",
),
response(&["git", "diff", "--color"], /*exit_code*/ 2, ""),
response(
&["git", "ls-files", "--others", "--exclude-standard"],
git_command(&[
"config",
"--null",
"--name-only",
"--get-regexp",
EXECUTABLE_FILTER_CONFIG_PATTERN,
]),
/*exit_code*/ 1,
"",
),
response(
git_command(&[
"diff",
"--no-textconv",
"--no-ext-diff",
"--submodule=short",
"--ignore-submodules=dirty",
"--color",
]),
/*exit_code*/ 2,
"",
),
response(
git_command(&["ls-files", "--others", "--exclude-standard"]),
/*exit_code*/ 0,
"",
),
@@ -255,14 +431,180 @@ mod tests {
.expect_err("unexpected git diff status should fail");
assert!(
error.contains("git [\"diff\", \"--color\"] failed with status 2"),
error.contains(
"git [\"diff\", \"--no-textconv\", \"--no-ext-diff\", \"--submodule=short\", \"--ignore-submodules=dirty\", \"--color\"] failed with status 2"
),
"unexpected error: {error}",
);
}
fn response(argv: &[&str], exit_code: i32, stdout: &str) -> FakeResponse {
#[cfg(unix)]
#[tokio::test]
async fn get_git_diff_does_not_execute_configured_filters_fsmonitor_or_hooks() {
let tempdir = tempfile::tempdir().expect("create temp directory");
let repo = tempdir.path().join("repo");
fs::create_dir(&repo).expect("create test repository directory");
run_git_setup(&repo, &["init", "-q"]);
run_git_setup(&repo, &["config", "user.name", "test"]);
run_git_setup(&repo, &["config", "user.email", "test@example.com"]);
fs::write(repo.join(".gitattributes"), "*.txt filter=x=y\n").expect("write attributes");
fs::write(repo.join("tracked.txt"), "before\n").expect("write tracked file");
fs::write(repo.join("unchanged.txt"), "unchanged\n").expect("write unchanged file");
run_git_setup(
&repo,
&["add", ".gitattributes", "tracked.txt", "unchanged.txt"],
);
run_git_setup(&repo, &["commit", "-qm", "initial"]);
let filter_helper = tempdir.path().join("filter-helper.sh");
let fsmonitor_helper = tempdir.path().join("fsmonitor-helper.sh");
let hooks_dir = tempdir.path().join("hooks");
let hook_helper = hooks_dir.join("post-index-change");
fs::create_dir(&hooks_dir).expect("create hooks directory");
write_marker_helper(&filter_helper);
write_marker_helper(&fsmonitor_helper);
write_marker_helper(&hook_helper);
run_git_setup(
&repo,
&[
"config",
"filter.x=y.clean",
filter_helper.to_str().expect("filter helper path"),
],
);
run_git_setup(
&repo,
&[
"config",
"filter.x=y.process",
filter_helper.to_str().expect("filter helper path"),
],
);
run_git_setup(&repo, &["config", "filter.x=y.required", "true"]);
run_git_setup(
&repo,
&[
"config",
"core.fsmonitor",
fsmonitor_helper.to_str().expect("fsmonitor helper path"),
],
);
run_git_setup(
&repo,
&[
"config",
"core.hooksPath",
hooks_dir.to_str().expect("hooks directory path"),
],
);
std::thread::sleep(Duration::from_secs(/*secs*/ 1));
fs::write(repo.join("unchanged.txt"), "unchanged\n").expect("refresh unchanged file");
fs::write(repo.join("tracked.txt"), "after\n").expect("modify tracked file");
let result = get_git_diff(&LocalRunner, &repo)
.await
.expect("generate diff without invoking helpers");
assert!(result.1.contains("before"));
assert!(result.1.contains("after"));
assert!(!filter_helper.with_extension("sh.ran").exists());
assert!(!fsmonitor_helper.with_extension("sh.ran").exists());
assert!(!hook_helper.with_extension("sh.ran").exists());
}
#[cfg(unix)]
#[tokio::test]
async fn get_git_diff_does_not_execute_helpers_while_checking_dirty_submodules() {
let tempdir = tempfile::tempdir().expect("create temp directory");
let child = tempdir.path().join("child");
let repo = tempdir.path().join("repo");
fs::create_dir(&child).expect("create child repository directory");
fs::create_dir(&repo).expect("create parent repository directory");
run_git_setup(&child, &["init", "-q"]);
run_git_setup(&child, &["config", "user.name", "test"]);
run_git_setup(&child, &["config", "user.email", "test@example.com"]);
fs::write(child.join(".gitattributes"), "*.txt filter=evil\n")
.expect("write child attributes");
fs::write(child.join("tracked.txt"), "before\n").expect("write child tracked file");
run_git_setup(&child, &["add", ".gitattributes", "tracked.txt"]);
run_git_setup(&child, &["commit", "-qm", "initial"]);
run_git_setup(&repo, &["init", "-q"]);
run_git_setup(&repo, &["config", "user.name", "test"]);
run_git_setup(&repo, &["config", "user.email", "test@example.com"]);
run_git_setup(
&repo,
&[
"-c",
"protocol.file.allow=always",
"submodule",
"add",
"-q",
child.to_str().expect("child repository path"),
"child",
],
);
run_git_setup(&repo, &["commit", "-qm", "add submodule"]);
let helper = tempdir.path().join("submodule-helper.sh");
write_marker_helper(&helper);
let checkout = repo.join("child");
run_git_setup(
&checkout,
&[
"config",
"filter.evil.clean",
helper.to_str().expect("submodule helper path"),
],
);
run_git_setup(&checkout, &["config", "filter.evil.required", "true"]);
std::thread::sleep(Duration::from_secs(/*secs*/ 1));
fs::write(checkout.join("tracked.txt"), "before\n").expect("refresh child tracked file");
let result = get_git_diff(&LocalRunner, &repo)
.await
.expect("generate diff without inspecting submodule worktrees");
assert!(result.1.is_empty());
assert!(!helper.with_extension("sh.ran").exists());
}
fn git_command(args: &[&str]) -> Vec<String> {
let mut argv = vec![
"git".to_string(),
"-c".to_string(),
DISABLE_FSMONITOR_CONFIG.to_string(),
"-c".to_string(),
DISABLE_HOOKS_CONFIG.to_string(),
];
argv.extend(args.iter().map(|arg| (*arg).to_string()));
argv
}
fn filter_override_env(driver: &str) -> HashMap<String, Option<String>> {
HashMap::from([
("GIT_CONFIG_COUNT".to_string(), Some("3".to_string())),
(
"GIT_CONFIG_KEY_0".to_string(),
Some(format!("{driver}.clean")),
),
("GIT_CONFIG_VALUE_0".to_string(), Some(String::new())),
(
"GIT_CONFIG_KEY_1".to_string(),
Some(format!("{driver}.process")),
),
("GIT_CONFIG_VALUE_1".to_string(), Some(String::new())),
(
"GIT_CONFIG_KEY_2".to_string(),
Some(format!("{driver}.required")),
),
("GIT_CONFIG_VALUE_2".to_string(), Some("false".to_string())),
])
}
fn response(argv: Vec<String>, exit_code: i32, stdout: &str) -> FakeResponse {
FakeResponse {
argv: argv.iter().map(|arg| (*arg).to_string()).collect(),
argv,
output: WorkspaceCommandOutput {
exit_code,
stdout: stdout.to_string(),
@@ -275,15 +617,32 @@ mod tests {
if cfg!(windows) { "NUL" } else { "/dev/null" }
}
fn assert_commands(commands: &[WorkspaceCommand], expected: &[&[&str]], cwd: &Path) {
#[cfg(unix)]
fn run_git_setup(cwd: &Path, args: &[&str]) {
let status = ProcessCommand::new("git")
.args(args)
.current_dir(cwd)
.status()
.expect("run git setup command");
assert!(status.success(), "git setup command failed: {args:?}");
}
#[cfg(unix)]
fn write_marker_helper(path: &Path) {
fs::write(path, "#!/bin/sh\nprintf ran >> \"$0.ran\"\nexit 1\n")
.expect("write helper script");
let mut permissions = fs::metadata(path)
.expect("read helper metadata")
.permissions();
permissions.set_mode(/*mode*/ 0o755);
fs::set_permissions(path, permissions).expect("make helper executable");
}
fn assert_commands(commands: &[WorkspaceCommand], expected: &[Vec<String>], cwd: &Path) {
let actual: Vec<Vec<String>> = commands
.iter()
.map(|command| command.argv.clone())
.collect();
let expected: Vec<Vec<String>> = expected
.iter()
.map(|argv| argv.iter().map(|arg| (*arg).to_string()).collect())
.collect();
assert_eq!(actual, expected);
for command in commands {
@@ -336,4 +695,44 @@ mod tests {
})
}
}
#[cfg(unix)]
struct LocalRunner;
#[cfg(unix)]
impl WorkspaceCommandExecutor for LocalRunner {
fn run(
&self,
command: WorkspaceCommand,
) -> Pin<
Box<
dyn Future<Output = Result<WorkspaceCommandOutput, WorkspaceCommandError>>
+ Send
+ '_,
>,
> {
Box::pin(async move {
let mut process = ProcessCommand::new(&command.argv[0]);
process
.args(&command.argv[1..])
.current_dir(command.cwd.expect("test command cwd"));
for (key, value) in command.env {
match value {
Some(value) => {
process.env(key, value);
}
None => {
process.env_remove(key);
}
}
}
let output = process.output().expect("run test command");
Ok(WorkspaceCommandOutput {
exit_code: output.status.code().expect("test command exit code"),
stdout: String::from_utf8(output.stdout).expect("utf8 stdout"),
stderr: String::from_utf8(output.stderr).expect("utf8 stderr"),
})
})
}
}
}