From d5137d76c1f8b24f075768a8f7e247efed62a938 Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Wed, 4 Mar 2026 13:19:13 +0100 Subject: [PATCH] git: Add trusted worktree support to git integrations (#50649) This PR cleans up the git command spawning by wrapping everything in GitBinary instead to follow a builder/factory pattern. It also extends trusted workspace support to git commands. I also added a `clippy.toml` configuration to our git crate that warns against using `Command` struct to spawn git commands instead of going through `GitBinary`. This should help us maintain the factory pattern in the future Before you mark this PR as ready for review, make sure that you have: - [x] Added solid test coverage and/or screenshots from doing manual testing - [x] Done a self-review taking into account security and performance aspects Release Notes: - git: Add trusted workspace support for Zed's git integration --- crates/fs/src/fake_git_repo.rs | 12 +- crates/fs/src/fs.rs | 1 + crates/git/clippy.toml | 28 + crates/git/src/blame.rs | 24 +- crates/git/src/commit.rs | 17 +- crates/git/src/repository.rs | 661 +++++++++--------- crates/project/src/git_store.rs | 61 +- crates/project/tests/integration/git_store.rs | 205 ++++++ 8 files changed, 644 insertions(+), 365 deletions(-) create mode 100644 crates/git/clippy.toml diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index 99295c69d45427c799e3d850d605f63d3950ee57..06ebea9157f97a0323297cd3ae142c4b306fe4ef 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -20,7 +20,7 @@ use ignore::gitignore::GitignoreBuilder; use parking_lot::Mutex; use rope::Rope; use smol::{channel::Sender, future::FutureExt as _}; -use std::{path::PathBuf, sync::Arc}; +use std::{path::PathBuf, sync::Arc, sync::atomic::AtomicBool}; use text::LineEnding; use util::{paths::PathStyle, rel_path::RelPath}; @@ -32,6 +32,7 @@ pub struct FakeGitRepository { pub(crate) dot_git_path: PathBuf, pub(crate) repository_dir_path: PathBuf, pub(crate) common_dir_path: PathBuf, + pub(crate) is_trusted: Arc, } #[derive(Debug, Clone)] @@ -1035,4 +1036,13 @@ impl GitRepository for FakeGitRepository { fn commit_data_reader(&self) -> Result { anyhow::bail!("commit_data_reader not supported for FakeGitRepository") } + + fn set_trusted(&self, trusted: bool) { + self.is_trusted + .store(trusted, std::sync::atomic::Ordering::Release); + } + + fn is_trusted(&self) -> bool { + self.is_trusted.load(std::sync::atomic::Ordering::Acquire) + } } diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 2db9e48a2e77bdb3e49fce0b16ea9b67ffaacbc0..0fde444171042eda859edcac7915c456ab91e265 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -2776,6 +2776,7 @@ impl Fs for FakeFs { repository_dir_path: repository_dir_path.to_owned(), common_dir_path: common_dir_path.to_owned(), checkpoints: Arc::default(), + is_trusted: Arc::default(), }) as _ }, ) diff --git a/crates/git/clippy.toml b/crates/git/clippy.toml new file mode 100644 index 0000000000000000000000000000000000000000..fb3926840493fd5981c1861e7cea96bd54b9647f --- /dev/null +++ b/crates/git/clippy.toml @@ -0,0 +1,28 @@ +allow-private-module-inception = true +avoid-breaking-exported-api = false +ignore-interior-mutability = [ + # Suppresses clippy::mutable_key_type, which is a false positive as the Eq + # and Hash impls do not use fields with interior mutability. + "agent_ui::context::AgentContextKey" +] +disallowed-methods = [ + { path = "std::process::Command::spawn", reason = "Spawning `std::process::Command` can block the current thread for an unknown duration", replacement = "smol::process::Command::spawn" }, + { path = "std::process::Command::output", reason = "Spawning `std::process::Command` can block the current thread for an unknown duration", replacement = "smol::process::Command::output" }, + { path = "std::process::Command::status", reason = "Spawning `std::process::Command` can block the current thread for an unknown duration", replacement = "smol::process::Command::status" }, + { path = "std::process::Command::stdin", reason = "`smol::process::Command::from()` does not preserve stdio configuration", replacement = "smol::process::Command::stdin" }, + { path = "std::process::Command::stdout", reason = "`smol::process::Command::from()` does not preserve stdio configuration", replacement = "smol::process::Command::stdout" }, + { path = "std::process::Command::stderr", reason = "`smol::process::Command::from()` does not preserve stdio configuration", replacement = "smol::process::Command::stderr" }, + { path = "smol::Timer::after", reason = "smol::Timer introduces non-determinism in tests", replacement = "gpui::BackgroundExecutor::timer" }, + { path = "serde_json::from_reader", reason = "Parsing from a buffer is much slower than first reading the buffer into a Vec/String, see https://github.com/serde-rs/json/issues/160#issuecomment-253446892. Use `serde_json::from_slice` instead." }, + { path = "serde_json_lenient::from_reader", reason = "Parsing from a buffer is much slower than first reading the buffer into a Vec/String, see https://github.com/serde-rs/json/issues/160#issuecomment-253446892, Use `serde_json_lenient::from_slice` instead." }, + { path = "cocoa::foundation::NSString::alloc", reason = "NSString must be autoreleased to avoid memory leaks. Use `ns_string()` helper instead." }, + { path = "smol::process::Command::new", reason = "Git commands must go through `GitBinary::build_command` to ensure security flags like `-c core.fsmonitor=false` are always applied.", replacement = "GitBinary::build_command" }, + { path = "util::command::new_command", reason = "Git commands must go through `GitBinary::build_command` to ensure security flags like `-c core.fsmonitor=false` are always applied.", replacement = "GitBinary::build_command" }, + { path = "util::command::Command::new", reason = "Git commands must go through `GitBinary::build_command` to ensure security flags like `-c core.fsmonitor=false` are always applied.", replacement = "GitBinary::build_command" }, +] +disallowed-types = [ + # { path = "std::collections::HashMap", replacement = "collections::HashMap" }, + # { path = "std::collections::HashSet", replacement = "collections::HashSet" }, + # { path = "indexmap::IndexSet", replacement = "collections::IndexSet" }, + # { path = "indexmap::IndexMap", replacement = "collections::IndexMap" }, +] \ No newline at end of file diff --git a/crates/git/src/blame.rs b/crates/git/src/blame.rs index 9dc184bf2ac253c8bc24f6203f13d6654ac2b64b..c44aea74051bb7c190a091703d6c60807fc4e27e 100644 --- a/crates/git/src/blame.rs +++ b/crates/git/src/blame.rs @@ -1,11 +1,11 @@ use crate::Oid; use crate::commit::get_messages; -use crate::repository::RepoPath; +use crate::repository::{GitBinary, RepoPath}; use anyhow::{Context as _, Result}; use collections::{HashMap, HashSet}; use futures::AsyncWriteExt; use serde::{Deserialize, Serialize}; -use std::{ops::Range, path::Path}; +use std::ops::Range; use text::{LineEnding, Rope}; use time::OffsetDateTime; use time::UtcOffset; @@ -21,15 +21,13 @@ pub struct Blame { } impl Blame { - pub async fn for_path( - git_binary: &Path, - working_directory: &Path, + pub(crate) async fn for_path( + git: &GitBinary, path: &RepoPath, content: &Rope, line_ending: LineEnding, ) -> Result { - let output = - run_git_blame(git_binary, working_directory, path, content, line_ending).await?; + let output = run_git_blame(git, path, content, line_ending).await?; let mut entries = parse_git_blame(&output)?; entries.sort_unstable_by(|a, b| a.range.start.cmp(&b.range.start)); @@ -40,7 +38,7 @@ impl Blame { } let shas = unique_shas.into_iter().collect::>(); - let messages = get_messages(working_directory, &shas) + let messages = get_messages(git, &shas) .await .context("failed to get commit messages")?; @@ -52,8 +50,7 @@ const GIT_BLAME_NO_COMMIT_ERROR: &str = "fatal: no such ref: HEAD"; const GIT_BLAME_NO_PATH: &str = "fatal: no such path"; async fn run_git_blame( - git_binary: &Path, - working_directory: &Path, + git: &GitBinary, path: &RepoPath, contents: &Rope, line_ending: LineEnding, @@ -61,12 +58,7 @@ async fn run_git_blame( let mut child = { let span = ztracing::debug_span!("spawning git-blame command", path = path.as_unix_str()); let _enter = span.enter(); - util::command::new_command(git_binary) - .current_dir(working_directory) - .arg("blame") - .arg("--incremental") - .arg("--contents") - .arg("-") + git.build_command(["blame", "--incremental", "--contents", "-"]) .arg(path.as_unix_str()) .stdin(Stdio::piped()) .stdout(Stdio::piped()) diff --git a/crates/git/src/commit.rs b/crates/git/src/commit.rs index 3f3526afc4ba8fa146592684a6d3acfc44ce7e73..46e050ce155fc049a670fdfa26101eb729b34352 100644 --- a/crates/git/src/commit.rs +++ b/crates/git/src/commit.rs @@ -1,11 +1,11 @@ use crate::{ BuildCommitPermalinkParams, GitHostingProviderRegistry, GitRemote, Oid, parse_git_remote_url, - status::StatusCode, + repository::GitBinary, status::StatusCode, }; use anyhow::{Context as _, Result}; use collections::HashMap; use gpui::SharedString; -use std::{path::Path, sync::Arc}; +use std::sync::Arc; #[derive(Clone, Debug, Default)] pub struct ParsedCommitMessage { @@ -48,7 +48,7 @@ impl ParsedCommitMessage { } } -pub async fn get_messages(working_directory: &Path, shas: &[Oid]) -> Result> { +pub(crate) async fn get_messages(git: &GitBinary, shas: &[Oid]) -> Result> { if shas.is_empty() { return Ok(HashMap::default()); } @@ -63,12 +63,12 @@ pub async fn get_messages(working_directory: &Path, shas: &[Oid]) -> Result Result>()) } -async fn get_messages_impl(working_directory: &Path, shas: &[Oid]) -> Result> { +async fn get_messages_impl(git: &GitBinary, shas: &[Oid]) -> Result> { const MARKER: &str = ""; - let output = util::command::new_command("git") - .current_dir(working_directory) - .arg("show") + let output = git + .build_command(["show"]) .arg("-s") .arg(format!("--format=%B{}", MARKER)) .args(shas.iter().map(ToString::to_string)) diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 6dba1400dffe1fd00844dd7241f39f48a7a759a6..f5a856325cc80071f2c8ef500e7b07aa24035f59 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -21,6 +21,7 @@ use text::LineEnding; use std::collections::HashSet; use std::ffi::{OsStr, OsString}; +use std::sync::atomic::AtomicBool; use std::process::ExitStatus; use std::str::FromStr; @@ -961,6 +962,9 @@ pub trait GitRepository: Send + Sync { ) -> BoxFuture<'_, Result<()>>; fn commit_data_reader(&self) -> Result; + + fn set_trusted(&self, trusted: bool); + fn is_trusted(&self) -> bool; } pub enum DiffType { @@ -987,6 +991,7 @@ pub struct RealGitRepository { pub any_git_binary_path: PathBuf, any_git_binary_help_output: Arc>>, executor: BackgroundExecutor, + is_trusted: Arc, } impl RealGitRepository { @@ -1005,6 +1010,7 @@ impl RealGitRepository { any_git_binary_path, executor, any_git_binary_help_output: Arc::new(Mutex::new(None)), + is_trusted: Arc::new(AtomicBool::new(false)), }) } @@ -1016,20 +1022,24 @@ impl RealGitRepository { .map(Path::to_path_buf) } + fn git_binary(&self) -> Result { + Ok(GitBinary::new( + self.any_git_binary_path.clone(), + self.working_directory() + .with_context(|| "Can't run git commands without a working directory")?, + self.executor.clone(), + self.is_trusted(), + )) + } + async fn any_git_binary_help_output(&self) -> SharedString { if let Some(output) = self.any_git_binary_help_output.lock().clone() { return output; } - let git_binary_path = self.any_git_binary_path.clone(); - let executor = self.executor.clone(); - let working_directory = self.working_directory(); + let git_binary = self.git_binary(); let output: SharedString = self .executor - .spawn(async move { - GitBinary::new(git_binary_path, working_directory?, executor) - .run(["help", "-a"]) - .await - }) + .spawn(async move { git_binary?.run(["help", "-a"]).await }) .await .unwrap_or_default() .into(); @@ -1072,6 +1082,7 @@ pub async fn get_git_committer(cx: &AsyncApp) -> GitCommitter { git_binary_path.unwrap_or(PathBuf::from("git")), paths::home_dir().clone(), cx.background_executor().clone(), + true, ); cx.background_spawn(async move { @@ -1103,14 +1114,12 @@ impl GitRepository for RealGitRepository { } fn show(&self, commit: String) -> BoxFuture<'_, Result> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = self.working_directory(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; - let output = new_command(git_binary_path) - .current_dir(&working_directory) - .args([ + let git = git_binary?; + let output = git + .build_command([ "--no-optional-locks", "show", "--no-patch", @@ -1141,15 +1150,14 @@ impl GitRepository for RealGitRepository { } fn load_commit(&self, commit: String, cx: AsyncApp) -> BoxFuture<'_, Result> { - let Some(working_directory) = self.repository.lock().workdir().map(ToOwned::to_owned) - else { + if self.repository.lock().workdir().is_none() { return future::ready(Err(anyhow!("no working directory"))).boxed(); - }; - let git_binary_path = self.any_git_binary_path.clone(); + } + let git_binary = self.git_binary(); cx.background_spawn(async move { - let show_output = util::command::new_command(&git_binary_path) - .current_dir(&working_directory) - .args([ + let git = git_binary?; + let show_output = git + .build_command([ "--no-optional-locks", "show", "--format=", @@ -1170,9 +1178,8 @@ impl GitRepository for RealGitRepository { let changes = parse_git_diff_name_status(&show_stdout); let parent_sha = format!("{}^", commit); - let mut cat_file_process = util::command::new_command(&git_binary_path) - .current_dir(&working_directory) - .args(["--no-optional-locks", "cat-file", "--batch=%(objectsize)"]) + let mut cat_file_process = git + .build_command(["--no-optional-locks", "cat-file", "--batch=%(objectsize)"]) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) @@ -1279,18 +1286,17 @@ impl GitRepository for RealGitRepository { mode: ResetMode, env: Arc>, ) -> BoxFuture<'_, Result<()>> { + let git_binary = self.git_binary(); async move { - let working_directory = self.working_directory(); - let mode_flag = match mode { ResetMode::Mixed => "--mixed", ResetMode::Soft => "--soft", }; - let output = new_command(&self.any_git_binary_path) + let git = git_binary?; + let output = git + .build_command(["reset", mode_flag, &commit]) .envs(env.iter()) - .current_dir(&working_directory?) - .args(["reset", mode_flag, &commit]) .output() .await?; anyhow::ensure!( @@ -1309,17 +1315,16 @@ impl GitRepository for RealGitRepository { paths: Vec, env: Arc>, ) -> BoxFuture<'_, Result<()>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); async move { if paths.is_empty() { return Ok(()); } - let output = new_command(&git_binary_path) - .current_dir(&working_directory?) + let git = git_binary?; + let output = git + .build_command(["checkout", &commit, "--"]) .envs(env.iter()) - .args(["checkout", &commit, "--"]) .args(paths.iter().map(|path| path.as_unix_str())) .output() .await?; @@ -1414,18 +1419,16 @@ impl GitRepository for RealGitRepository { env: Arc>, is_executable: bool, ) -> BoxFuture<'_, anyhow::Result<()>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; + let git = git_binary?; let mode = if is_executable { "100755" } else { "100644" }; if let Some(content) = content { - let mut child = new_command(&git_binary_path) - .current_dir(&working_directory) + let mut child = git + .build_command(["hash-object", "-w", "--stdin"]) .envs(env.iter()) - .args(["hash-object", "-w", "--stdin"]) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn()?; @@ -1438,10 +1441,9 @@ impl GitRepository for RealGitRepository { log::debug!("indexing SHA: {sha}, path {path:?}"); - let output = new_command(&git_binary_path) - .current_dir(&working_directory) + let output = git + .build_command(["update-index", "--add", "--cacheinfo", mode, sha]) .envs(env.iter()) - .args(["update-index", "--add", "--cacheinfo", mode, sha]) .arg(path.as_unix_str()) .output() .await?; @@ -1453,10 +1455,9 @@ impl GitRepository for RealGitRepository { ); } else { log::debug!("removing path {path:?} from the index"); - let output = new_command(&git_binary_path) - .current_dir(&working_directory) + let output = git + .build_command(["update-index", "--force-remove"]) .envs(env.iter()) - .args(["update-index", "--force-remove"]) .arg(path.as_unix_str()) .output() .await?; @@ -1485,14 +1486,12 @@ impl GitRepository for RealGitRepository { } fn revparse_batch(&self, revs: Vec) -> BoxFuture<'_, Result>>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; - let mut process = new_command(&git_binary_path) - .current_dir(&working_directory) - .args([ + let git = git_binary?; + let mut process = git + .build_command([ "--no-optional-locks", "cat-file", "--batch-check=%(objectname)", @@ -1545,19 +1544,14 @@ impl GitRepository for RealGitRepository { } fn status(&self, path_prefixes: &[RepoPath]) -> Task> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = match self.working_directory() { - Ok(working_directory) => working_directory, + let git = match self.git_binary() { + Ok(git) => git, Err(e) => return Task::ready(Err(e)), }; let args = git_status_args(path_prefixes); log::debug!("Checking for git status in {path_prefixes:?}"); self.executor.spawn(async move { - let output = new_command(&git_binary_path) - .current_dir(working_directory) - .args(args) - .output() - .await?; + let output = git.build_command(args).output().await?; if output.status.success() { let stdout = String::from_utf8_lossy(&output.stdout); stdout.parse() @@ -1569,9 +1563,8 @@ impl GitRepository for RealGitRepository { } fn diff_tree(&self, request: DiffTreeType) -> BoxFuture<'_, Result> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = match self.working_directory() { - Ok(working_directory) => working_directory, + let git = match self.git_binary() { + Ok(git) => git, Err(e) => return Task::ready(Err(e)).boxed(), }; @@ -1596,11 +1589,7 @@ impl GitRepository for RealGitRepository { self.executor .spawn(async move { - let output = new_command(&git_binary_path) - .current_dir(working_directory) - .args(args) - .output() - .await?; + let output = git.build_command(args).output().await?; if output.status.success() { let stdout = String::from_utf8_lossy(&output.stdout); stdout.parse() @@ -1613,13 +1602,12 @@ impl GitRepository for RealGitRepository { } fn stash_entries(&self) -> BoxFuture<'_, Result> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = self.working_directory(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let output = new_command(&git_binary_path) - .current_dir(working_directory?) - .args(&["stash", "list", "--pretty=format:%gd%x00%H%x00%ct%x00%s"]) + let git = git_binary?; + let output = git + .build_command(&["stash", "list", "--pretty=format:%gd%x00%H%x00%ct%x00%s"]) .output() .await?; if output.status.success() { @@ -1634,8 +1622,7 @@ impl GitRepository for RealGitRepository { } fn branches(&self) -> BoxFuture<'_, Result>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { let fields = [ @@ -1657,12 +1644,8 @@ impl GitRepository for RealGitRepository { "--format", &fields, ]; - let working_directory = working_directory?; - let output = new_command(&git_binary_path) - .current_dir(&working_directory) - .args(args) - .output() - .await?; + let git = git_binary?; + let output = git.build_command(args).output().await?; anyhow::ensure!( output.status.success(), @@ -1676,11 +1659,7 @@ impl GitRepository for RealGitRepository { if branches.is_empty() { let args = vec!["symbolic-ref", "--quiet", "HEAD"]; - let output = new_command(&git_binary_path) - .current_dir(&working_directory) - .args(args) - .output() - .await?; + let output = git.build_command(args).output().await?; // git symbolic-ref returns a non-0 exit code if HEAD points // to something other than a branch @@ -1702,13 +1681,12 @@ impl GitRepository for RealGitRepository { } fn worktrees(&self) -> BoxFuture<'_, Result>> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = self.working_directory(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let output = new_command(&git_binary_path) - .current_dir(working_directory?) - .args(&["--no-optional-locks", "worktree", "list", "--porcelain"]) + let git = git_binary?; + let output = git + .build_command(&["--no-optional-locks", "worktree", "list", "--porcelain"]) .output() .await?; if output.status.success() { @@ -1728,8 +1706,7 @@ impl GitRepository for RealGitRepository { directory: PathBuf, from_commit: Option, ) -> BoxFuture<'_, Result<()>> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = self.working_directory(); + let git_binary = self.git_binary(); let final_path = directory.join(&name); let mut args = vec![ OsString::from("--no-optional-locks"), @@ -1749,11 +1726,8 @@ impl GitRepository for RealGitRepository { self.executor .spawn(async move { std::fs::create_dir_all(final_path.parent().unwrap_or(&final_path))?; - let output = new_command(&git_binary_path) - .current_dir(working_directory?) - .args(args) - .output() - .await?; + let git = git_binary?; + let output = git.build_command(args).output().await?; if output.status.success() { Ok(()) } else { @@ -1765,9 +1739,7 @@ impl GitRepository for RealGitRepository { } fn remove_worktree(&self, path: PathBuf, force: bool) -> BoxFuture<'_, Result<()>> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = self.working_directory(); - let executor = self.executor.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { @@ -1781,18 +1753,14 @@ impl GitRepository for RealGitRepository { } args.push("--".into()); args.push(path.as_os_str().into()); - GitBinary::new(git_binary_path, working_directory?, executor) - .run(args) - .await?; + git_binary?.run(args).await?; anyhow::Ok(()) }) .boxed() } fn rename_worktree(&self, old_path: PathBuf, new_path: PathBuf) -> BoxFuture<'_, Result<()>> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = self.working_directory(); - let executor = self.executor.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { @@ -1804,9 +1772,7 @@ impl GitRepository for RealGitRepository { old_path.as_os_str().into(), new_path.as_os_str().into(), ]; - GitBinary::new(git_binary_path, working_directory?, executor) - .run(args) - .await?; + git_binary?.run(args).await?; anyhow::Ok(()) }) .boxed() @@ -1814,9 +1780,7 @@ impl GitRepository for RealGitRepository { fn change_branch(&self, name: String) -> BoxFuture<'_, Result<()>> { let repo = self.repository.clone(); - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); - let executor = self.executor.clone(); + let git_binary = self.git_binary(); let branch = self.executor.spawn(async move { let repo = repo.lock(); let branch = if let Ok(branch) = repo.find_branch(&name, BranchType::Local) { @@ -1851,9 +1815,7 @@ impl GitRepository for RealGitRepository { self.executor .spawn(async move { let branch = branch.await?; - GitBinary::new(git_binary_path, working_directory?, executor) - .run(&["checkout", &branch]) - .await?; + git_binary?.run(&["checkout", &branch]).await?; anyhow::Ok(()) }) .boxed() @@ -1864,9 +1826,7 @@ impl GitRepository for RealGitRepository { name: String, base_branch: Option, ) -> BoxFuture<'_, Result<()>> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = self.working_directory(); - let executor = self.executor.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { @@ -1877,22 +1837,18 @@ impl GitRepository for RealGitRepository { args.push(&base_branch_str); } - GitBinary::new(git_binary_path, working_directory?, executor) - .run(&args) - .await?; + git_binary?.run(&args).await?; anyhow::Ok(()) }) .boxed() } fn rename_branch(&self, branch: String, new_name: String) -> BoxFuture<'_, Result<()>> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = self.working_directory(); - let executor = self.executor.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - GitBinary::new(git_binary_path, working_directory?, executor) + git_binary? .run(&["branch", "-m", &branch, &new_name]) .await?; anyhow::Ok(()) @@ -1901,15 +1857,11 @@ impl GitRepository for RealGitRepository { } fn delete_branch(&self, name: String) -> BoxFuture<'_, Result<()>> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = self.working_directory(); - let executor = self.executor.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - GitBinary::new(git_binary_path, working_directory?, executor) - .run(&["branch", "-d", &name]) - .await?; + git_binary?.run(&["branch", "-d", &name]).await?; anyhow::Ok(()) }) .boxed() @@ -1921,20 +1873,11 @@ impl GitRepository for RealGitRepository { content: Rope, line_ending: LineEnding, ) -> BoxFuture<'_, Result> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); - let executor = self.executor.clone(); + let git = self.git_binary(); - executor + self.executor .spawn(async move { - crate::blame::Blame::for_path( - &git_binary_path, - &working_directory?, - &path, - &content, - line_ending, - ) - .await + crate::blame::Blame::for_path(&git?, &path, &content, line_ending).await }) .boxed() } @@ -1949,11 +1892,10 @@ impl GitRepository for RealGitRepository { skip: usize, limit: Option, ) -> BoxFuture<'_, Result> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; + let git = git_binary?; // Use a unique delimiter with a hardcoded UUID to separate commits // This essentially eliminates any chance of encountering the delimiter in actual commit data let commit_delimiter = @@ -1981,9 +1923,8 @@ impl GitRepository for RealGitRepository { args.push("--"); - let output = new_command(&git_binary_path) - .current_dir(&working_directory) - .args(&args) + let output = git + .build_command(&args) .arg(path.as_unix_str()) .output() .await?; @@ -2028,30 +1969,17 @@ impl GitRepository for RealGitRepository { } fn diff(&self, diff: DiffType) -> BoxFuture<'_, Result> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; + let git = git_binary?; let output = match diff { DiffType::HeadToIndex => { - new_command(&git_binary_path) - .current_dir(&working_directory) - .args(["diff", "--staged"]) - .output() - .await? - } - DiffType::HeadToWorktree => { - new_command(&git_binary_path) - .current_dir(&working_directory) - .args(["diff"]) - .output() - .await? + git.build_command(["diff", "--staged"]).output().await? } + DiffType::HeadToWorktree => git.build_command(["diff"]).output().await?, DiffType::MergeBase { base_ref } => { - new_command(&git_binary_path) - .current_dir(&working_directory) - .args(["diff", "--merge-base", base_ref.as_ref()]) + git.build_command(["diff", "--merge-base", base_ref.as_ref()]) .output() .await? } @@ -2071,38 +1999,29 @@ impl GitRepository for RealGitRepository { &self, diff: DiffType, ) -> BoxFuture<'_, Result>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; + let git = git_binary?; let output = match diff { DiffType::HeadToIndex => { - new_command(&git_binary_path) - .current_dir(&working_directory) - .args(["diff", "--numstat", "--staged"]) + git.build_command(["diff", "--numstat", "--staged"]) .output() .await? } DiffType::HeadToWorktree => { - new_command(&git_binary_path) - .current_dir(&working_directory) - .args(["diff", "--numstat"]) - .output() - .await? + git.build_command(["diff", "--numstat"]).output().await? } DiffType::MergeBase { base_ref } => { - new_command(&git_binary_path) - .current_dir(&working_directory) - .args([ - "diff", - "--numstat", - "--merge-base", - base_ref.as_ref(), - "HEAD", - ]) - .output() - .await? + git.build_command([ + "diff", + "--numstat", + "--merge-base", + base_ref.as_ref(), + "HEAD", + ]) + .output() + .await? } }; @@ -2123,15 +2042,14 @@ impl GitRepository for RealGitRepository { paths: Vec, env: Arc>, ) -> BoxFuture<'_, Result<()>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { if !paths.is_empty() { - let output = new_command(&git_binary_path) - .current_dir(&working_directory?) + let git = git_binary?; + let output = git + .build_command(["update-index", "--add", "--remove", "--"]) .envs(env.iter()) - .args(["update-index", "--add", "--remove", "--"]) .args(paths.iter().map(|p| p.as_unix_str())) .output() .await?; @@ -2151,16 +2069,15 @@ impl GitRepository for RealGitRepository { paths: Vec, env: Arc>, ) -> BoxFuture<'_, Result<()>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { if !paths.is_empty() { - let output = new_command(&git_binary_path) - .current_dir(&working_directory?) + let git = git_binary?; + let output = git + .build_command(["reset", "--quiet", "--"]) .envs(env.iter()) - .args(["reset", "--quiet", "--"]) .args(paths.iter().map(|p| p.as_std_path())) .output() .await?; @@ -2181,19 +2098,16 @@ impl GitRepository for RealGitRepository { paths: Vec, env: Arc>, ) -> BoxFuture<'_, Result<()>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let mut cmd = new_command(&git_binary_path); - cmd.current_dir(&working_directory?) + let git = git_binary?; + let output = git + .build_command(["stash", "push", "--quiet", "--include-untracked"]) .envs(env.iter()) - .args(["stash", "push", "--quiet"]) - .arg("--include-untracked"); - - cmd.args(paths.iter().map(|p| p.as_unix_str())); - - let output = cmd.output().await?; + .args(paths.iter().map(|p| p.as_unix_str())) + .output() + .await?; anyhow::ensure!( output.status.success(), @@ -2210,20 +2124,15 @@ impl GitRepository for RealGitRepository { index: Option, env: Arc>, ) -> BoxFuture<'_, Result<()>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let mut cmd = new_command(git_binary_path); + let git = git_binary?; let mut args = vec!["stash".to_string(), "pop".to_string()]; if let Some(index) = index { args.push(format!("stash@{{{}}}", index)); } - cmd.current_dir(&working_directory?) - .envs(env.iter()) - .args(args); - - let output = cmd.output().await?; + let output = git.build_command(&args).envs(env.iter()).output().await?; anyhow::ensure!( output.status.success(), @@ -2240,20 +2149,15 @@ impl GitRepository for RealGitRepository { index: Option, env: Arc>, ) -> BoxFuture<'_, Result<()>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let mut cmd = new_command(git_binary_path); + let git = git_binary?; let mut args = vec!["stash".to_string(), "apply".to_string()]; if let Some(index) = index { args.push(format!("stash@{{{}}}", index)); } - cmd.current_dir(&working_directory?) - .envs(env.iter()) - .args(args); - - let output = cmd.output().await?; + let output = git.build_command(&args).envs(env.iter()).output().await?; anyhow::ensure!( output.status.success(), @@ -2270,20 +2174,15 @@ impl GitRepository for RealGitRepository { index: Option, env: Arc>, ) -> BoxFuture<'_, Result<()>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let mut cmd = new_command(git_binary_path); + let git = git_binary?; let mut args = vec!["stash".to_string(), "drop".to_string()]; if let Some(index) = index { args.push(format!("stash@{{{}}}", index)); } - cmd.current_dir(&working_directory?) - .envs(env.iter()) - .args(args); - - let output = cmd.output().await?; + let output = git.build_command(&args).envs(env.iter()).output().await?; anyhow::ensure!( output.status.success(), @@ -2303,16 +2202,14 @@ impl GitRepository for RealGitRepository { ask_pass: AskPassDelegate, env: Arc>, ) -> BoxFuture<'_, Result<()>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); let executor = self.executor.clone(); // Note: Do not spawn this command on the background thread, it might pop open the credential helper // which we want to block on. async move { - let mut cmd = new_command(git_binary_path); - cmd.current_dir(&working_directory?) - .envs(env.iter()) - .args(["commit", "--quiet", "-m"]) + let git = git_binary?; + let mut cmd = git.build_command(["commit", "--quiet", "-m"]); + cmd.envs(env.iter()) .arg(&message.to_string()) .arg("--cleanup=strip") .arg("--no-verify") @@ -2351,16 +2248,21 @@ impl GitRepository for RealGitRepository { let working_directory = self.working_directory(); let executor = cx.background_executor().clone(); let git_binary_path = self.system_git_binary_path.clone(); + let is_trusted = self.is_trusted(); // Note: Do not spawn this command on the background thread, it might pop open the credential helper // which we want to block on. async move { let git_binary_path = git_binary_path.context("git not found on $PATH, can't push")?; let working_directory = working_directory?; - let mut command = new_command(git_binary_path); + let git = GitBinary::new( + git_binary_path, + working_directory, + executor.clone(), + is_trusted, + ); + let mut command = git.build_command(["push"]); command .envs(env.iter()) - .current_dir(&working_directory) - .args(["push"]) .args(options.map(|option| match option { PushOptions::SetUpstream => "--set-upstream", PushOptions::Force => "--force-with-lease", @@ -2388,15 +2290,20 @@ impl GitRepository for RealGitRepository { let working_directory = self.working_directory(); let executor = cx.background_executor().clone(); let git_binary_path = self.system_git_binary_path.clone(); + let is_trusted = self.is_trusted(); // Note: Do not spawn this command on the background thread, it might pop open the credential helper // which we want to block on. async move { let git_binary_path = git_binary_path.context("git not found on $PATH, can't pull")?; - let mut command = new_command(git_binary_path); - command - .envs(env.iter()) - .current_dir(&working_directory?) - .arg("pull"); + let working_directory = working_directory?; + let git = GitBinary::new( + git_binary_path, + working_directory, + executor.clone(), + is_trusted, + ); + let mut command = git.build_command(["pull"]); + command.envs(env.iter()); if rebase { command.arg("--rebase"); @@ -2424,15 +2331,21 @@ impl GitRepository for RealGitRepository { let remote_name = format!("{}", fetch_options); let git_binary_path = self.system_git_binary_path.clone(); let executor = cx.background_executor().clone(); + let is_trusted = self.is_trusted(); // Note: Do not spawn this command on the background thread, it might pop open the credential helper // which we want to block on. async move { let git_binary_path = git_binary_path.context("git not found on $PATH, can't fetch")?; - let mut command = new_command(git_binary_path); + let working_directory = working_directory?; + let git = GitBinary::new( + git_binary_path, + working_directory, + executor.clone(), + is_trusted, + ); + let mut command = git.build_command(["fetch", &remote_name]); command .envs(env.iter()) - .current_dir(&working_directory?) - .args(["fetch", &remote_name]) .stdout(Stdio::piped()) .stderr(Stdio::piped()); @@ -2442,14 +2355,12 @@ impl GitRepository for RealGitRepository { } fn get_push_remote(&self, branch: String) -> BoxFuture<'_, Result>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; - let output = new_command(&git_binary_path) - .current_dir(&working_directory) - .args(["rev-parse", "--abbrev-ref"]) + let git = git_binary?; + let output = git + .build_command(["rev-parse", "--abbrev-ref"]) .arg(format!("{branch}@{{push}}")) .output() .await?; @@ -2469,14 +2380,12 @@ impl GitRepository for RealGitRepository { } fn get_branch_remote(&self, branch: String) -> BoxFuture<'_, Result>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; - let output = new_command(&git_binary_path) - .current_dir(&working_directory) - .args(["config", "--get"]) + let git = git_binary?; + let output = git + .build_command(["config", "--get"]) .arg(format!("branch.{branch}.remote")) .output() .await?; @@ -2493,16 +2402,11 @@ impl GitRepository for RealGitRepository { } fn get_all_remotes(&self) -> BoxFuture<'_, Result>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; - let output = new_command(&git_binary_path) - .current_dir(&working_directory) - .args(["remote", "-v"]) - .output() - .await?; + let git = git_binary?; + let output = git.build_command(["remote", "-v"]).output().await?; anyhow::ensure!( output.status.success(), @@ -2551,17 +2455,12 @@ impl GitRepository for RealGitRepository { } fn check_for_pushed_commit(&self) -> BoxFuture<'_, Result>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; + let git = git_binary?; let git_cmd = async |args: &[&str]| -> Result { - let output = new_command(&git_binary_path) - .current_dir(&working_directory) - .args(args) - .output() - .await?; + let output = git.build_command(args).output().await?; anyhow::ensure!( output.status.success(), String::from_utf8_lossy(&output.stderr).to_string() @@ -2610,14 +2509,10 @@ impl GitRepository for RealGitRepository { } fn checkpoint(&self) -> BoxFuture<'static, Result> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); - let executor = self.executor.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; - let mut git = GitBinary::new(git_binary_path, working_directory.clone(), executor) - .envs(checkpoint_author_envs()); + let mut git = git_binary?.envs(checkpoint_author_envs()); git.with_temp_index(async |git| { let head_sha = git.run(&["rev-parse", "HEAD"]).await.ok(); let mut excludes = exclude_files(git).await?; @@ -2643,15 +2538,10 @@ impl GitRepository for RealGitRepository { } fn restore_checkpoint(&self, checkpoint: GitRepositoryCheckpoint) -> BoxFuture<'_, Result<()>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); - - let executor = self.executor.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; - - let git = GitBinary::new(git_binary_path, working_directory, executor); + let git = git_binary?; git.run(&[ "restore", "--source", @@ -2682,14 +2572,10 @@ impl GitRepository for RealGitRepository { left: GitRepositoryCheckpoint, right: GitRepositoryCheckpoint, ) -> BoxFuture<'_, Result> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); - - let executor = self.executor.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; - let git = GitBinary::new(git_binary_path, working_directory, executor); + let git = git_binary?; let result = git .run(&[ "diff-tree", @@ -2720,14 +2606,10 @@ impl GitRepository for RealGitRepository { base_checkpoint: GitRepositoryCheckpoint, target_checkpoint: GitRepositoryCheckpoint, ) -> BoxFuture<'_, Result> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); - - let executor = self.executor.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; - let git = GitBinary::new(git_binary_path, working_directory, executor); + let git = git_binary?; git.run(&[ "diff", "--find-renames", @@ -2744,14 +2626,10 @@ impl GitRepository for RealGitRepository { &self, include_remote_name: bool, ) -> BoxFuture<'_, Result>> { - let working_directory = self.working_directory(); - let git_binary_path = self.any_git_binary_path.clone(); - - let executor = self.executor.clone(); + let git_binary = self.git_binary(); self.executor .spawn(async move { - let working_directory = working_directory?; - let git = GitBinary::new(git_binary_path, working_directory, executor); + let git = git_binary?; let strip_prefix = if include_remote_name { "refs/remotes/" @@ -2801,15 +2679,19 @@ impl GitRepository for RealGitRepository { hook: RunHook, env: Arc>, ) -> BoxFuture<'_, Result<()>> { - let working_directory = self.working_directory(); + let git_binary = self.git_binary(); let repository = self.repository.clone(); - let git_binary_path = self.any_git_binary_path.clone(); - let executor = self.executor.clone(); let help_output = self.any_git_binary_help_output(); // Note: Do not spawn these commands on the background thread, as this causes some git hooks to hang. async move { - let working_directory = working_directory?; + let git = git_binary?; + + if !git.is_trusted { + bail!("Can't run git commit hooks in restrictive workspace"); + } + + let working_directory = git.working_directory.clone(); if !help_output .await .lines() @@ -2817,6 +2699,7 @@ impl GitRepository for RealGitRepository { { let hook_abs_path = repository.lock().path().join("hooks").join(hook.as_str()); if hook_abs_path.is_file() { + #[allow(clippy::disallowed_methods)] let output = new_command(&hook_abs_path) .envs(env.iter()) .current_dir(&working_directory) @@ -2836,8 +2719,7 @@ impl GitRepository for RealGitRepository { return Ok(()); } - let git = GitBinary::new(git_binary_path, working_directory, executor) - .envs(HashMap::clone(&env)); + let git = git.envs(HashMap::clone(&env)); git.run(&["hook", "run", "--ignore-missing", hook.as_str()]) .await?; Ok(()) @@ -2851,13 +2733,10 @@ impl GitRepository for RealGitRepository { log_order: LogOrder, request_tx: Sender>>, ) -> BoxFuture<'_, Result<()>> { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = self.working_directory(); - let executor = self.executor.clone(); + let git_binary = self.git_binary(); async move { - let working_directory = working_directory?; - let git = GitBinary::new(git_binary_path, working_directory, executor); + let git = git_binary?; let mut command = git.build_command([ "log", @@ -2911,19 +2790,12 @@ impl GitRepository for RealGitRepository { } fn commit_data_reader(&self) -> Result { - let git_binary_path = self.any_git_binary_path.clone(); - let working_directory = self - .working_directory() - .map_err(|_| anyhow!("no working directory"))?; - let executor = self.executor.clone(); + let git_binary = self.git_binary()?; let (request_tx, request_rx) = smol::channel::bounded::(64); let task = self.executor.spawn(async move { - if let Err(error) = - run_commit_data_reader(git_binary_path, working_directory, executor, request_rx) - .await - { + if let Err(error) = run_commit_data_reader(git_binary, request_rx).await { log::error!("commit data reader failed: {error:?}"); } }); @@ -2933,15 +2805,21 @@ impl GitRepository for RealGitRepository { _task: task, }) } + + fn set_trusted(&self, trusted: bool) { + self.is_trusted + .store(trusted, std::sync::atomic::Ordering::Release); + } + + fn is_trusted(&self) -> bool { + self.is_trusted.load(std::sync::atomic::Ordering::Acquire) + } } async fn run_commit_data_reader( - git_binary_path: PathBuf, - working_directory: PathBuf, - executor: BackgroundExecutor, + git: GitBinary, request_rx: smol::channel::Receiver, ) -> Result<()> { - let git = GitBinary::new(git_binary_path, working_directory, executor); let mut process = git .build_command(["--no-optional-locks", "cat-file", "--batch"]) .stdin(Stdio::piped()) @@ -3117,19 +2995,21 @@ async fn exclude_files(git: &GitBinary) -> Result { Ok(excludes) } -struct GitBinary { +pub(crate) struct GitBinary { git_binary_path: PathBuf, working_directory: PathBuf, executor: BackgroundExecutor, index_file_path: Option, envs: HashMap, + is_trusted: bool, } impl GitBinary { - fn new( + pub(crate) fn new( git_binary_path: PathBuf, working_directory: PathBuf, executor: BackgroundExecutor, + is_trusted: bool, ) -> Self { Self { git_binary_path, @@ -3137,6 +3017,7 @@ impl GitBinary { executor, index_file_path: None, envs: HashMap::default(), + is_trusted, } } @@ -3241,12 +3122,20 @@ impl GitBinary { Ok(String::from_utf8(output.stdout)?) } - fn build_command(&self, args: impl IntoIterator) -> util::command::Command + #[allow(clippy::disallowed_methods)] + pub(crate) fn build_command( + &self, + args: impl IntoIterator, + ) -> util::command::Command where S: AsRef, { let mut command = new_command(&self.git_binary_path); command.current_dir(&self.working_directory); + command.args(["-c", "core.fsmonitor=false"]); + if !self.is_trusted { + command.args(["-c", "core.hooksPath=/dev/null"]); + } command.args(args); if let Some(index_file_path) = self.index_file_path.as_ref() { command.env("GIT_INDEX_FILE", index_file_path); @@ -3506,6 +3395,102 @@ mod tests { } } + #[gpui::test] + async fn test_build_command_untrusted_includes_both_safety_args(cx: &mut TestAppContext) { + cx.executor().allow_parking(); + let dir = tempfile::tempdir().unwrap(); + let git = GitBinary::new( + PathBuf::from("git"), + dir.path().to_path_buf(), + cx.executor(), + false, + ); + let output = git + .build_command(["version"]) + .output() + .await + .expect("git version should succeed"); + assert!(output.status.success()); + + let git = GitBinary::new( + PathBuf::from("git"), + dir.path().to_path_buf(), + cx.executor(), + false, + ); + let output = git + .build_command(["config", "--get", "core.fsmonitor"]) + .output() + .await + .expect("git config should run"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!( + stdout.trim(), + "false", + "fsmonitor should be disabled for untrusted repos" + ); + + git2::Repository::init(dir.path()).unwrap(); + let git = GitBinary::new( + PathBuf::from("git"), + dir.path().to_path_buf(), + cx.executor(), + false, + ); + let output = git + .build_command(["config", "--get", "core.hooksPath"]) + .output() + .await + .expect("git config should run"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!( + stdout.trim(), + "/dev/null", + "hooksPath should be /dev/null for untrusted repos" + ); + } + + #[gpui::test] + async fn test_build_command_trusted_only_disables_fsmonitor(cx: &mut TestAppContext) { + cx.executor().allow_parking(); + let dir = tempfile::tempdir().unwrap(); + git2::Repository::init(dir.path()).unwrap(); + + let git = GitBinary::new( + PathBuf::from("git"), + dir.path().to_path_buf(), + cx.executor(), + true, + ); + let output = git + .build_command(["config", "--get", "core.fsmonitor"]) + .output() + .await + .expect("git config should run"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!( + stdout.trim(), + "false", + "fsmonitor should be disabled even for trusted repos" + ); + + let git = GitBinary::new( + PathBuf::from("git"), + dir.path().to_path_buf(), + cx.executor(), + true, + ); + let output = git + .build_command(["config", "--get", "core.hooksPath"]) + .output() + .await + .expect("git config should run"); + assert!( + !output.status.success(), + "hooksPath should NOT be overridden for trusted repos" + ); + } + #[gpui::test] async fn test_checkpoint_basic(cx: &mut TestAppContext) { disable_git_global_config(); @@ -4398,7 +4383,7 @@ mod tests { .spawn(async move { let git_binary_path = git_binary_path.clone(); let working_directory = working_directory?; - let git = GitBinary::new(git_binary_path, working_directory, executor); + let git = GitBinary::new(git_binary_path, working_directory, executor, true); git.run(&["gc", "--prune"]).await?; Ok(()) }) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 487e7f5f9699382ce4930141f7a0c7c50a1d23b8..b03c7d69ab05daf94254a9d47cb2ae23da3043d1 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -6,6 +6,9 @@ pub mod pending_op; use crate::{ ProjectEnvironment, ProjectItem, ProjectPath, buffer_store::{BufferStore, BufferStoreEvent}, + trusted_worktrees::{ + PathTrust, TrustedWorktrees, TrustedWorktreesEvent, TrustedWorktreesStore, + }, worktree_store::{WorktreeStore, WorktreeStoreEvent}, }; use anyhow::{Context as _, Result, anyhow, bail}; @@ -354,6 +357,7 @@ impl LocalRepositoryState { dot_git_abs_path: Arc, project_environment: WeakEntity, fs: Arc, + is_trusted: bool, cx: &mut AsyncApp, ) -> anyhow::Result { let environment = project_environment @@ -381,6 +385,7 @@ impl LocalRepositoryState { } }) .await?; + backend.set_trusted(is_trusted); Ok(LocalRepositoryState { backend, environment: Arc::new(environment), @@ -495,11 +500,15 @@ impl GitStore { state: GitStoreState, cx: &mut Context, ) -> Self { - let _subscriptions = vec![ + let mut _subscriptions = vec![ cx.subscribe(&worktree_store, Self::on_worktree_store_event), cx.subscribe(&buffer_store, Self::on_buffer_store_event), ]; + if let Some(trusted_worktrees) = TrustedWorktrees::try_get_global(cx) { + _subscriptions.push(cx.subscribe(&trusted_worktrees, Self::on_trusted_worktrees_event)); + } + GitStore { state, buffer_store, @@ -1517,6 +1526,13 @@ impl GitStore { let original_repo_abs_path: Arc = git::repository::original_repo_path_from_common_dir(common_dir_abs_path).into(); let id = RepositoryId(next_repository_id.fetch_add(1, atomic::Ordering::Release)); + let is_trusted = TrustedWorktrees::try_get_global(cx) + .map(|trusted_worktrees| { + trusted_worktrees.update(cx, |trusted_worktrees, cx| { + trusted_worktrees.can_trust(&self.worktree_store, worktree_id, cx) + }) + }) + .unwrap_or(false); let git_store = cx.weak_entity(); let repo = cx.new(|cx| { let mut repo = Repository::local( @@ -1526,6 +1542,7 @@ impl GitStore { dot_git_abs_path.clone(), project_environment.downgrade(), fs.clone(), + is_trusted, git_store, cx, ); @@ -1566,6 +1583,39 @@ impl GitStore { } } + fn on_trusted_worktrees_event( + &mut self, + _: Entity, + event: &TrustedWorktreesEvent, + cx: &mut Context, + ) { + if !matches!(self.state, GitStoreState::Local { .. }) { + return; + } + + let (is_trusted, event_paths) = match event { + TrustedWorktreesEvent::Trusted(_, trusted_paths) => (true, trusted_paths), + TrustedWorktreesEvent::Restricted(_, restricted_paths) => (false, restricted_paths), + }; + + for (repo_id, worktree_ids) in &self.worktree_ids { + if worktree_ids + .iter() + .any(|worktree_id| event_paths.contains(&PathTrust::Worktree(*worktree_id))) + { + if let Some(repo) = self.repositories.get(repo_id) { + let repository_state = repo.read(cx).repository_state.clone(); + cx.background_spawn(async move { + if let Ok(RepositoryState::Local(state)) = repository_state.await { + state.backend.set_trusted(is_trusted); + } + }) + .detach(); + } + } + } + } + fn on_buffer_store_event( &mut self, _: Entity, @@ -3763,6 +3813,13 @@ impl MergeDetails { } impl Repository { + pub fn is_trusted(&self) -> bool { + match self.repository_state.peek() { + Some(Ok(RepositoryState::Local(state))) => state.backend.is_trusted(), + _ => false, + } + } + pub fn snapshot(&self) -> RepositorySnapshot { self.snapshot.clone() } @@ -3788,6 +3845,7 @@ impl Repository { dot_git_abs_path: Arc, project_environment: WeakEntity, fs: Arc, + is_trusted: bool, git_store: WeakEntity, cx: &mut Context, ) -> Self { @@ -3804,6 +3862,7 @@ impl Repository { dot_git_abs_path, project_environment, fs, + is_trusted, cx, ) .await diff --git a/crates/project/tests/integration/git_store.rs b/crates/project/tests/integration/git_store.rs index 88614cec68b542b3d08de11cfe0c5f3781d6b379..82e92bc4f1cfb606fb09d5efd5d341ed2951c067 100644 --- a/crates/project/tests/integration/git_store.rs +++ b/crates/project/tests/integration/git_store.rs @@ -1293,3 +1293,208 @@ mod git_worktrees { use crate::Project; } + +mod trust_tests { + use collections::HashSet; + use fs::FakeFs; + use gpui::TestAppContext; + use project::trusted_worktrees::*; + + use serde_json::json; + use settings::SettingsStore; + use util::path; + + use crate::Project; + + fn init_test(cx: &mut TestAppContext) { + zlog::init_test(); + + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + }); + } + + #[gpui::test] + async fn test_repository_defaults_to_untrusted_without_trust_system(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree( + path!("/project"), + json!({ + ".git": {}, + "a.txt": "hello", + }), + ) + .await; + + // Create project without trust system — repos should default to untrusted. + let project = Project::test(fs, [path!("/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let repository = project.read_with(cx, |project, cx| { + project.repositories(cx).values().next().unwrap().clone() + }); + + repository.read_with(cx, |repo, _| { + assert!( + !repo.is_trusted(), + "repository should default to untrusted when no trust system is initialized" + ); + }); + } + + #[gpui::test] + async fn test_multiple_repos_trust_with_single_worktree(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree( + path!("/project"), + json!({ + ".git": {}, + "a.txt": "hello", + "sub": { + ".git": {}, + "b.txt": "world", + }, + }), + ) + .await; + + cx.update(|cx| { + init(DbTrustedPaths::default(), cx); + }); + + let project = + Project::test_with_worktree_trust(fs.clone(), [path!("/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let worktree_store = project.read_with(cx, |project, _| project.worktree_store()); + let worktree_id = worktree_store.read_with(cx, |store, cx| { + store.worktrees().next().unwrap().read(cx).id() + }); + + let repos = project.read_with(cx, |project, cx| { + project + .repositories(cx) + .values() + .cloned() + .collect::>() + }); + assert_eq!(repos.len(), 2, "should have two repositories"); + for repo in &repos { + repo.read_with(cx, |repo, _| { + assert!( + !repo.is_trusted(), + "all repos should be untrusted initially" + ); + }); + } + + let trusted_worktrees = cx + .update(|cx| TrustedWorktrees::try_get_global(cx).expect("trust global should be set")); + trusted_worktrees.update(cx, |store, cx| { + store.trust( + &worktree_store, + HashSet::from_iter([PathTrust::Worktree(worktree_id)]), + cx, + ); + }); + cx.executor().run_until_parked(); + + for repo in &repos { + repo.read_with(cx, |repo, _| { + assert!( + repo.is_trusted(), + "all repos should be trusted after worktree is trusted" + ); + }); + } + } + + #[gpui::test] + async fn test_repository_trust_restrict_trust_cycle(cx: &mut TestAppContext) { + init_test(cx); + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree( + path!("/project"), + json!({ + ".git": {}, + "a.txt": "hello", + }), + ) + .await; + + cx.update(|cx| { + project::trusted_worktrees::init(DbTrustedPaths::default(), cx); + }); + + let project = + Project::test_with_worktree_trust(fs.clone(), [path!("/project").as_ref()], cx).await; + cx.executor().run_until_parked(); + + let worktree_store = project.read_with(cx, |project, _| project.worktree_store()); + let worktree_id = worktree_store.read_with(cx, |store, cx| { + store.worktrees().next().unwrap().read(cx).id() + }); + + let repository = project.read_with(cx, |project, cx| { + project.repositories(cx).values().next().unwrap().clone() + }); + + repository.read_with(cx, |repo, _| { + assert!(!repo.is_trusted(), "repository should start untrusted"); + }); + + let trusted_worktrees = cx + .update(|cx| TrustedWorktrees::try_get_global(cx).expect("trust global should be set")); + + trusted_worktrees.update(cx, |store, cx| { + store.trust( + &worktree_store, + HashSet::from_iter([PathTrust::Worktree(worktree_id)]), + cx, + ); + }); + cx.executor().run_until_parked(); + + repository.read_with(cx, |repo, _| { + assert!( + repo.is_trusted(), + "repository should be trusted after worktree is trusted" + ); + }); + + trusted_worktrees.update(cx, |store, cx| { + store.restrict( + worktree_store.downgrade(), + HashSet::from_iter([PathTrust::Worktree(worktree_id)]), + cx, + ); + }); + cx.executor().run_until_parked(); + + repository.read_with(cx, |repo, _| { + assert!( + !repo.is_trusted(), + "repository should be untrusted after worktree is restricted" + ); + }); + + trusted_worktrees.update(cx, |store, cx| { + store.trust( + &worktree_store, + HashSet::from_iter([PathTrust::Worktree(worktree_id)]), + cx, + ); + }); + cx.executor().run_until_parked(); + + repository.read_with(cx, |repo, _| { + assert!( + repo.is_trusted(), + "repository should be trusted again after second trust" + ); + }); + } +}