Dino created
When on a untrusted project, if one was to try and use the commit
generation functionality, the command would fail because of the `-c
diff.external` configuration provided in `GitBinary::build_command`, as
git would interpret this as `""` and try to run that command.
This `-c diff.external` is a good safeguard to have on untrusted
repositories because it prevents random commands, configured in
`.git/config` from being run. For example, if one uses `git config
diff.external "touch bananas.txt"` and then run `git diff`, a new
`bananas.txt` file would be created.
However, it was still possible to bypass this safeguard using the
following strategy:
1. Specify a custom diff for a specific file format. For example, for
markdown files, with `printf '*.md diff=pwned\n' > .gitattributes`
2. Update the command run by the `pwned` diff, for example, `git config
diff.pwned.command "touch bananas.txt"`
3. Open Zed and attempt to generate a commit message in an untrusted
repository and check that a new `bananas.txt` file was created
This is only prevented by using the `--no-ext-diff` flag on the `diff`
command, so a new `GitBinary::build_diff_command` has been introduced
which simply wraps `GitBinary::build_command` and adds the
`--no-ext-diff` flag, if necessary.
As a side-effect, this also makes it so that generating a commit message
in an untrusted repository works again, which was accidentally broken on
https://github.com/zed-industries/zed/pull/50649 .
Before you mark this PR as ready for review, make sure that you have:
- [X] Added a solid test coverage and/or screenshots from doing manual
testing
- [X] Done a self-review taking into account security and performance
aspects
- [X] Aligned any UI changes with the [UI
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
Release Notes:
- Fixed commit message generation in untrusted repositoriescrates/git/src/blame.rs | 2
crates/git/src/commit.rs | 2
crates/git/src/repository.rs | 98 ++++++++++++++++++++-----------------
3 files changed, 55 insertions(+), 47 deletions(-)
@@ -58,7 +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();
- git.build_command(["blame", "--incremental", "--contents", "-"])
+ git.build_command(&["blame", "--incremental", "--contents", "-"])
.arg(path.as_unix_str())
.stdin(Stdio::piped())
.stdout(Stdio::piped())
@@ -81,7 +81,7 @@ pub(crate) async fn get_messages(git: &GitBinary, shas: &[Oid]) -> Result<HashMa
async fn get_messages_impl(git: &GitBinary, shas: &[Oid]) -> Result<Vec<String>> {
const MARKER: &str = "<MARKER>";
let output = git
- .build_command(["show"])
+ .build_command(&["show"])
.arg("-s")
.arg(format!("--format=%B{}", MARKER))
.args(shas.iter().map(ToString::to_string))
@@ -1039,7 +1039,7 @@ impl RealGitRepository {
let git_binary = self.git_binary();
let output: SharedString = self
.executor
- .spawn(async move { git_binary?.run(["help", "-a"]).await })
+ .spawn(async move { git_binary?.run(&["help", "-a"]).await })
.await
.unwrap_or_default()
.into();
@@ -1086,9 +1086,12 @@ pub async fn get_git_committer(cx: &AsyncApp) -> GitCommitter {
);
cx.background_spawn(async move {
- let name = git.run(["config", "--global", "user.name"]).await.log_err();
+ let name = git
+ .run(&["config", "--global", "user.name"])
+ .await
+ .log_err();
let email = git
- .run(["config", "--global", "user.email"])
+ .run(&["config", "--global", "user.email"])
.await
.log_err();
GitCommitter { name, email }
@@ -1119,7 +1122,7 @@ impl GitRepository for RealGitRepository {
.spawn(async move {
let git = git_binary?;
let output = git
- .build_command([
+ .build_command(&[
"--no-optional-locks",
"show",
"--no-patch",
@@ -1157,7 +1160,7 @@ impl GitRepository for RealGitRepository {
cx.background_spawn(async move {
let git = git_binary?;
let show_output = git
- .build_command([
+ .build_command(&[
"--no-optional-locks",
"show",
"--format=",
@@ -1179,7 +1182,7 @@ impl GitRepository for RealGitRepository {
let parent_sha = format!("{}^", commit);
let mut cat_file_process = git
- .build_command(["--no-optional-locks", "cat-file", "--batch=%(objectsize)"])
+ .build_command(&["--no-optional-locks", "cat-file", "--batch=%(objectsize)"])
.stdin(Stdio::piped())
.stdout(Stdio::piped())
.stderr(Stdio::piped())
@@ -1295,7 +1298,7 @@ impl GitRepository for RealGitRepository {
let git = git_binary?;
let output = git
- .build_command(["reset", mode_flag, &commit])
+ .build_command(&["reset", mode_flag, &commit])
.envs(env.iter())
.output()
.await?;
@@ -1323,7 +1326,7 @@ impl GitRepository for RealGitRepository {
let git = git_binary?;
let output = git
- .build_command(["checkout", &commit, "--"])
+ .build_command(&["checkout", &commit, "--"])
.envs(env.iter())
.args(paths.iter().map(|path| path.as_unix_str()))
.output()
@@ -1427,7 +1430,7 @@ impl GitRepository for RealGitRepository {
if let Some(content) = content {
let mut child = git
- .build_command(["hash-object", "-w", "--stdin"])
+ .build_command(&["hash-object", "-w", "--stdin"])
.envs(env.iter())
.stdin(Stdio::piped())
.stdout(Stdio::piped())
@@ -1442,7 +1445,7 @@ impl GitRepository for RealGitRepository {
log::debug!("indexing SHA: {sha}, path {path:?}");
let output = git
- .build_command(["update-index", "--add", "--cacheinfo", mode, sha])
+ .build_command(&["update-index", "--add", "--cacheinfo", mode, sha])
.envs(env.iter())
.arg(path.as_unix_str())
.output()
@@ -1456,7 +1459,7 @@ impl GitRepository for RealGitRepository {
} else {
log::debug!("removing path {path:?} from the index");
let output = git
- .build_command(["update-index", "--force-remove"])
+ .build_command(&["update-index", "--force-remove"])
.envs(env.iter())
.arg(path.as_unix_str())
.output()
@@ -1491,7 +1494,7 @@ impl GitRepository for RealGitRepository {
.spawn(async move {
let git = git_binary?;
let mut process = git
- .build_command([
+ .build_command(&[
"--no-optional-locks",
"cat-file",
"--batch-check=%(objectname)",
@@ -1551,7 +1554,7 @@ impl GitRepository for RealGitRepository {
let args = git_status_args(path_prefixes);
log::debug!("Checking for git status in {path_prefixes:?}");
self.executor.spawn(async move {
- let output = git.build_command(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()
@@ -1589,7 +1592,7 @@ impl GitRepository for RealGitRepository {
self.executor
.spawn(async move {
- let output = git.build_command(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()
@@ -1645,7 +1648,7 @@ impl GitRepository for RealGitRepository {
&fields,
];
let git = git_binary?;
- let output = git.build_command(args).output().await?;
+ let output = git.build_command(&args).output().await?;
anyhow::ensure!(
output.status.success(),
@@ -1659,7 +1662,7 @@ impl GitRepository for RealGitRepository {
if branches.is_empty() {
let args = vec!["symbolic-ref", "--quiet", "HEAD"];
- let output = git.build_command(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
@@ -1727,7 +1730,7 @@ impl GitRepository for RealGitRepository {
.spawn(async move {
std::fs::create_dir_all(final_path.parent().unwrap_or(&final_path))?;
let git = git_binary?;
- let output = git.build_command(args).output().await?;
+ let output = git.build_command(&args).output().await?;
if output.status.success() {
Ok(())
} else {
@@ -1753,7 +1756,7 @@ impl GitRepository for RealGitRepository {
}
args.push("--".into());
args.push(path.as_os_str().into());
- git_binary?.run(args).await?;
+ git_binary?.run(&args).await?;
anyhow::Ok(())
})
.boxed()
@@ -1772,7 +1775,7 @@ impl GitRepository for RealGitRepository {
old_path.as_os_str().into(),
new_path.as_os_str().into(),
];
- git_binary?.run(args).await?;
+ git_binary?.run(&args).await?;
anyhow::Ok(())
})
.boxed()
@@ -1975,11 +1978,11 @@ impl GitRepository for RealGitRepository {
let git = git_binary?;
let output = match diff {
DiffType::HeadToIndex => {
- git.build_command(["diff", "--staged"]).output().await?
+ git.build_command(&["diff", "--staged"]).output().await?
}
- DiffType::HeadToWorktree => git.build_command(["diff"]).output().await?,
+ DiffType::HeadToWorktree => git.build_command(&["diff"]).output().await?,
DiffType::MergeBase { base_ref } => {
- git.build_command(["diff", "--merge-base", base_ref.as_ref()])
+ git.build_command(&["diff", "--merge-base", base_ref.as_ref()])
.output()
.await?
}
@@ -2036,7 +2039,7 @@ impl GitRepository for RealGitRepository {
if !paths.is_empty() {
let git = git_binary?;
let output = git
- .build_command(["update-index", "--add", "--remove", "--"])
+ .build_command(&["update-index", "--add", "--remove", "--"])
.envs(env.iter())
.args(paths.iter().map(|p| p.as_unix_str()))
.output()
@@ -2064,7 +2067,7 @@ impl GitRepository for RealGitRepository {
if !paths.is_empty() {
let git = git_binary?;
let output = git
- .build_command(["reset", "--quiet", "--"])
+ .build_command(&["reset", "--quiet", "--"])
.envs(env.iter())
.args(paths.iter().map(|p| p.as_std_path()))
.output()
@@ -2091,7 +2094,7 @@ impl GitRepository for RealGitRepository {
.spawn(async move {
let git = git_binary?;
let output = git
- .build_command(["stash", "push", "--quiet", "--include-untracked"])
+ .build_command(&["stash", "push", "--quiet", "--include-untracked"])
.envs(env.iter())
.args(paths.iter().map(|p| p.as_unix_str()))
.output()
@@ -2196,7 +2199,7 @@ impl GitRepository for RealGitRepository {
// which we want to block on.
async move {
let git = git_binary?;
- let mut cmd = git.build_command(["commit", "--quiet", "-m"]);
+ let mut cmd = git.build_command(&["commit", "--quiet", "-m"]);
cmd.envs(env.iter())
.arg(&message.to_string())
.arg("--cleanup=strip")
@@ -2248,7 +2251,7 @@ impl GitRepository for RealGitRepository {
executor.clone(),
is_trusted,
);
- let mut command = git.build_command(["push"]);
+ let mut command = git.build_command(&["push"]);
command
.envs(env.iter())
.args(options.map(|option| match option {
@@ -2290,7 +2293,7 @@ impl GitRepository for RealGitRepository {
executor.clone(),
is_trusted,
);
- let mut command = git.build_command(["pull"]);
+ let mut command = git.build_command(&["pull"]);
command.envs(env.iter());
if rebase {
@@ -2331,7 +2334,7 @@ impl GitRepository for RealGitRepository {
executor.clone(),
is_trusted,
);
- let mut command = git.build_command(["fetch", &remote_name]);
+ let mut command = git.build_command(&["fetch", &remote_name]);
command
.envs(env.iter())
.stdout(Stdio::piped())
@@ -2348,7 +2351,7 @@ impl GitRepository for RealGitRepository {
.spawn(async move {
let git = git_binary?;
let output = git
- .build_command(["rev-parse", "--abbrev-ref"])
+ .build_command(&["rev-parse", "--abbrev-ref"])
.arg(format!("{branch}@{{push}}"))
.output()
.await?;
@@ -2373,7 +2376,7 @@ impl GitRepository for RealGitRepository {
.spawn(async move {
let git = git_binary?;
let output = git
- .build_command(["config", "--get"])
+ .build_command(&["config", "--get"])
.arg(format!("branch.{branch}.remote"))
.output()
.await?;
@@ -2394,7 +2397,7 @@ impl GitRepository for RealGitRepository {
self.executor
.spawn(async move {
let git = git_binary?;
- let output = git.build_command(["remote", "-v"]).output().await?;
+ let output = git.build_command(&["remote", "-v"]).output().await?;
anyhow::ensure!(
output.status.success(),
@@ -2725,7 +2728,7 @@ impl GitRepository for RealGitRepository {
async move {
let git = git_binary?;
- let mut command = git.build_command([
+ let mut command = git.build_command(&[
"log",
GRAPH_COMMIT_FORMAT,
log_order.as_arg(),
@@ -2808,7 +2811,7 @@ async fn run_commit_data_reader(
request_rx: smol::channel::Receiver<CommitDataRequest>,
) -> Result<()> {
let mut process = git
- .build_command(["--no-optional-locks", "cat-file", "--batch"])
+ .build_command(&["--no-optional-locks", "cat-file", "--batch"])
.stdin(Stdio::piped())
.stdout(Stdio::piped())
.stderr(Stdio::piped())
@@ -3075,7 +3078,7 @@ impl GitBinary {
.join(format!("index-{}.tmp", id))
}
- pub async fn run<S>(&self, args: impl IntoIterator<Item = S>) -> Result<String>
+ pub async fn run<S>(&self, args: &[S]) -> Result<String>
where
S: AsRef<OsStr>,
{
@@ -3087,7 +3090,7 @@ impl GitBinary {
}
/// Returns the result of the command without trimming the trailing newline.
- pub async fn run_raw<S>(&self, args: impl IntoIterator<Item = S>) -> Result<String>
+ pub async fn run_raw<S>(&self, args: &[S]) -> Result<String>
where
S: AsRef<OsStr>,
{
@@ -3105,10 +3108,7 @@ impl GitBinary {
}
#[allow(clippy::disallowed_methods)]
- pub(crate) fn build_command<S>(
- &self,
- args: impl IntoIterator<Item = S>,
- ) -> util::command::Command
+ pub(crate) fn build_command<S>(&self, args: &[S]) -> util::command::Command
where
S: AsRef<OsStr>,
{
@@ -3125,6 +3125,14 @@ impl GitBinary {
command.args(["-c", "diff.external="]);
}
command.args(args);
+
+ // If the `diff` command is being used, we'll want to add the
+ // `--no-ext-diff` flag when working on an untrusted repository,
+ // preventing any external diff programs from being invoked.
+ if !self.is_trusted && args.iter().any(|arg| arg.as_ref() == "diff") {
+ command.arg("--no-ext-diff");
+ }
+
if let Some(index_file_path) = self.index_file_path.as_ref() {
command.env("GIT_INDEX_FILE", index_file_path);
}
@@ -3394,7 +3402,7 @@ mod tests {
false,
);
let output = git
- .build_command(["version"])
+ .build_command(&["version"])
.output()
.await
.expect("git version should succeed");
@@ -3407,7 +3415,7 @@ mod tests {
false,
);
let output = git
- .build_command(["config", "--get", "core.fsmonitor"])
+ .build_command(&["config", "--get", "core.fsmonitor"])
.output()
.await
.expect("git config should run");
@@ -3426,7 +3434,7 @@ mod tests {
false,
);
let output = git
- .build_command(["config", "--get", "core.hooksPath"])
+ .build_command(&["config", "--get", "core.hooksPath"])
.output()
.await
.expect("git config should run");
@@ -3451,7 +3459,7 @@ mod tests {
true,
);
let output = git
- .build_command(["config", "--get", "core.fsmonitor"])
+ .build_command(&["config", "--get", "core.fsmonitor"])
.output()
.await
.expect("git config should run");
@@ -3469,7 +3477,7 @@ mod tests {
true,
);
let output = git
- .build_command(["config", "--get", "core.hooksPath"])
+ .build_command(&["config", "--get", "core.hooksPath"])
.output()
.await
.expect("git config should run");