diff --git a/crates/goose-cli/src/commands/review/handler.rs b/crates/goose-cli/src/commands/review/handler.rs index 8c6f6d4cd..32d579c44 100644 --- a/crates/goose-cli/src/commands/review/handler.rs +++ b/crates/goose-cli/src/commands/review/handler.rs @@ -6,6 +6,7 @@ use std::process::Command; use crate::session::{build_session, SessionBuilderConfig}; use goose::checks::{discover, DiscoveredReview}; +use goose::subprocess::git_command; use super::orchestrator::{ emit_findings, run_checks_in_parallel, run_main_pass_in_parallel, Severity, @@ -312,7 +313,7 @@ fn print_discovered_summary(d: &DiscoveredReview) { } fn find_repo_root() -> Result { - let out = Command::new("git") + let out = git_command() .args(["rev-parse", "--show-toplevel"]) .output() .context("failed to invoke git")?; @@ -331,15 +332,15 @@ fn find_repo_root() -> Result { /// C-style escapes (`"dir/\303\251.txt"`), which downstream parsers /// would have to round-trip-decode just to spell the filename. We turn /// it off everywhere so callers always get clean UTF-8 paths. -fn git_command(repo_root: &Path) -> Command { - let mut cmd = Command::new("git"); +fn review_git_command(repo_root: &Path) -> Command { + let mut cmd = git_command(); cmd.current_dir(repo_root) .args(["-c", "core.quotePath=off"]); cmd } fn touched_files(repo_root: &Path, range: Option<&str>, files: &[String]) -> Result> { - let mut cmd = git_command(repo_root); + let mut cmd = review_git_command(repo_root); cmd.arg("diff").arg("--name-only"); match range { Some(r) => { @@ -370,7 +371,7 @@ fn touched_files(repo_root: &Path, range: Option<&str>, files: &[String]) -> Res } fn collect_diff(repo_root: &Path, range: Option<&str>, files: &[String]) -> Result { - let mut cmd = git_command(repo_root); + let mut cmd = review_git_command(repo_root); cmd.arg("diff"); match range { Some(r) => { @@ -394,7 +395,7 @@ fn collect_diff(repo_root: &Path, range: Option<&str>, files: &[String]) -> Resu } fn collect_diff_stat(repo_root: &Path, range: Option<&str>, files: &[String]) -> Result { - let mut cmd = git_command(repo_root); + let mut cmd = review_git_command(repo_root); cmd.arg("diff").arg("--stat"); match range { Some(r) => { @@ -425,7 +426,7 @@ fn collect_diff_stat(repo_root: &Path, range: Option<&str>, files: &[String]) -> /// brand-new files to the review when no `--range` is given (default /// `git diff HEAD` would silently drop them). fn untracked_files(repo_root: &Path, files: &[String]) -> Result> { - let mut cmd = git_command(repo_root); + let mut cmd = review_git_command(repo_root); cmd.args(["ls-files", "--others", "--exclude-standard"]); if !files.is_empty() { cmd.arg("--"); diff --git a/crates/goose-cli/src/recipes/github_recipe.rs b/crates/goose-cli/src/recipes/github_recipe.rs index 22558136c..a15e32b7c 100644 --- a/crates/goose-cli/src/recipes/github_recipe.rs +++ b/crates/goose-cli/src/recipes/github_recipe.rs @@ -5,7 +5,7 @@ use goose::recipe::RECIPE_FILE_EXTENSIONS; use serde::{Deserialize, Serialize}; use goose::recipe::read_recipe_file_content::RecipeFile; -use goose::subprocess::SubprocessExt; +use goose::subprocess::{git_command, SubprocessExt}; use std::env; use std::fs; @@ -190,7 +190,7 @@ fn ensure_repo_cloned(recipe_repo_full_name: &str) -> Result { fn fetch_origin(local_repo_path: &Path) -> Result<()> { let error_message: String = format!("Failed to fetch at {}", local_repo_path.to_str().unwrap()); - let status = Command::new("git") + let status = git_command() .args(["fetch", "origin"]) .current_dir(local_repo_path) .set_no_window() @@ -213,7 +213,7 @@ fn get_folder_from_github(local_repo_path: &Path, recipe_name: &str) -> Result

Result<()> { - let output = Command::new("git") + let output = git_command() .arg("clone") .arg("--depth") .arg("1") @@ -551,7 +550,7 @@ mod tests { } fn run_git(repo: &Path, args: &[&str]) { - let output = Command::new("git") + let output = git_command() .args(args) .current_dir(repo) .set_no_window() diff --git a/crates/goose/src/subprocess.rs b/crates/goose/src/subprocess.rs index b01006423..1849e55c6 100644 --- a/crates/goose/src/subprocess.rs +++ b/crates/goose/src/subprocess.rs @@ -26,6 +26,19 @@ pub trait SubprocessExt { fn set_no_window(&mut self) -> &mut Self; } +/// Creates a Git command that rejects implicit bare repositories and cannot run a +/// repository-configured fsmonitor hook. +pub fn git_command() -> std::process::Command { + let mut command = std::process::Command::new("git"); + command.args([ + "-c", + "safe.bareRepository=explicit", + "-c", + "core.fsmonitor=false", + ]); + command +} + impl SubprocessExt for Command { fn set_no_window(&mut self) -> &mut Self { #[cfg(windows)] diff --git a/crates/goose/tests/git_command_security.rs b/crates/goose/tests/git_command_security.rs new file mode 100644 index 000000000..fe509b75d --- /dev/null +++ b/crates/goose/tests/git_command_security.rs @@ -0,0 +1,85 @@ +use goose::subprocess::git_command; +use std::fs; +use std::path::Path; +use std::process::{Command, Output}; + +fn run_git(cwd: &Path, args: &[&str]) -> Output { + Command::new("git") + .args(args) + .current_dir(cwd) + .output() + .unwrap() +} + +fn assert_git_succeeded(output: &Output) { + assert!( + output.status.success(), + "git failed: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn rejects_implicitly_discovered_bare_repository() { + let temp_dir = tempfile::tempdir().unwrap(); + assert_git_succeeded(&run_git( + temp_dir.path(), + &["init", "--bare", "embedded.git"], + )); + + let nested_dir = temp_dir.path().join("embedded.git/nested"); + fs::create_dir(&nested_dir).unwrap(); + + assert_git_succeeded(&run_git( + &nested_dir, + &["-c", "safe.bareRepository=all", "rev-parse", "--git-dir"], + )); + + let output = git_command() + .args(["rev-parse", "--git-dir"]) + .current_dir(&nested_dir) + .output() + .unwrap(); + + assert!(!output.status.success(), "implicit bare repo was accepted"); +} + +#[cfg(unix)] +#[test] +fn does_not_execute_repository_fsmonitor_hook() { + use std::os::unix::fs::PermissionsExt; + + let temp_dir = tempfile::tempdir().unwrap(); + let repo_dir = temp_dir.path().join("repo"); + fs::create_dir(&repo_dir).unwrap(); + assert_git_succeeded(&run_git(&repo_dir, &["init"])); + + fs::write(repo_dir.join("tracked.txt"), "content").unwrap(); + assert_git_succeeded(&run_git(&repo_dir, &["add", "tracked.txt"])); + + let marker_path = temp_dir.path().join("fsmonitor-ran"); + let hook_path = temp_dir.path().join("fsmonitor-hook"); + fs::write( + &hook_path, + format!("#!/bin/sh\n: > '{}'\n", marker_path.display()), + ) + .unwrap(); + fs::set_permissions(&hook_path, fs::Permissions::from_mode(0o755)).unwrap(); + assert_git_succeeded(&run_git( + &repo_dir, + &["config", "core.fsmonitor", hook_path.to_str().unwrap()], + )); + + assert_git_succeeded(&run_git(&repo_dir, &["status", "--porcelain"])); + assert!(marker_path.exists(), "fsmonitor hook fixture did not run"); + fs::remove_file(&marker_path).unwrap(); + + let output = git_command() + .args(["status", "--porcelain"]) + .current_dir(&repo_dir) + .output() + .unwrap(); + + assert_git_succeeded(&output); + assert!(!marker_path.exists(), "repository fsmonitor hook ran"); +} diff --git a/ui/desktop/src/main.ts b/ui/desktop/src/main.ts index b866a6d6d..39a8b0f9b 100644 --- a/ui/desktop/src/main.ts +++ b/ui/desktop/src/main.ts @@ -252,7 +252,17 @@ function listGitWorktreeDirs(dir: string): Promise { execFile( 'git', - ['-C', dir, 'worktree', 'list', '--porcelain'], + [ + '-c', + 'safe.bareRepository=explicit', + '-c', + 'core.fsmonitor=false', + '-C', + dir, + 'worktree', + 'list', + '--porcelain', + ], { timeout: 3000 }, (error, stdout) => { if (error) { diff --git a/ui/text/src/slashCommands.tsx b/ui/text/src/slashCommands.tsx index a71e3bd7a..bb837bd9d 100644 --- a/ui/text/src/slashCommands.tsx +++ b/ui/text/src/slashCommands.tsx @@ -16,10 +16,21 @@ export interface SlashCommand { } function isGitRepo(cwd: string): boolean { - const result = spawnSync("git", ["rev-parse", "--is-inside-work-tree"], { - cwd, - stdio: ["ignore", "ignore", "ignore"], - }); + const result = spawnSync( + "git", + [ + "-c", + "safe.bareRepository=explicit", + "-c", + "core.fsmonitor=false", + "rev-parse", + "--is-inside-work-tree", + ], + { + cwd, + stdio: ["ignore", "ignore", "ignore"], + }, + ); return result.status === 0; } @@ -28,7 +39,15 @@ const MAX_DIFF_BYTES = 2_000_000; function readDiff(cwd: string): { text: string; truncated: boolean } | null { const result = spawnSync( "git", - ["--no-pager", "diff", "--no-color"], + [ + "-c", + "safe.bareRepository=explicit", + "-c", + "core.fsmonitor=false", + "--no-pager", + "diff", + "--no-color", + ], { cwd, encoding: "utf8",