From 842ac984d57676aec610d54eaba73d88262ae931 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Thu, 10 Jul 2025 20:38:51 -0400 Subject: [PATCH] git: Intercept signing prompt from GPG when committing (#34096) Closes #30111 - [x] basic implementation - [x] implementation for remote projects - [x] surface error output from GPG if signing fails - [ ] ~~Windows~~ Release Notes: - git: Passphrase prompts from GPG to unlock commit signing keys are now shown in Zed. --- Cargo.lock | 1 + crates/askpass/Cargo.toml | 1 + crates/askpass/src/askpass.rs | 59 +++++++++++++++ crates/fs/src/fake_git_repo.rs | 4 +- crates/git/Cargo.toml | 4 +- crates/git/src/repository.rs | 124 ++++++++++++++++++++++---------- crates/git_ui/src/git_panel.rs | 20 ++++-- crates/project/src/git_store.rs | 30 ++++++-- crates/proto/proto/git.proto | 1 + 9 files changed, 193 insertions(+), 51 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 163cba778a..bc6783ce92 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -607,6 +607,7 @@ dependencies = [ "parking_lot", "smol", "tempfile", + "unindent", "util", "workspace-hack", ] diff --git a/crates/askpass/Cargo.toml b/crates/askpass/Cargo.toml index 0527399af8..3241061dc6 100644 --- a/crates/askpass/Cargo.toml +++ b/crates/askpass/Cargo.toml @@ -19,5 +19,6 @@ net.workspace = true parking_lot.workspace = true smol.workspace = true tempfile.workspace = true +unindent.workspace = true util.workspace = true workspace-hack.workspace = true diff --git a/crates/askpass/src/askpass.rs b/crates/askpass/src/askpass.rs index f085a2be72..8a91e748ed 100644 --- a/crates/askpass/src/askpass.rs +++ b/crates/askpass/src/askpass.rs @@ -40,11 +40,21 @@ impl AskPassDelegate { self.tx.send((prompt, tx)).await?; Ok(rx.await?) } + + pub fn new_always_failing() -> Self { + let (tx, _rx) = mpsc::unbounded::<(String, oneshot::Sender)>(); + Self { + tx, + _task: Task::ready(()), + } + } } pub struct AskPassSession { #[cfg(not(target_os = "windows"))] script_path: std::path::PathBuf, + #[cfg(not(target_os = "windows"))] + gpg_script_path: std::path::PathBuf, #[cfg(target_os = "windows")] askpass_helper: String, #[cfg(target_os = "windows")] @@ -59,6 +69,9 @@ const ASKPASS_SCRIPT_NAME: &str = "askpass.sh"; #[cfg(target_os = "windows")] const ASKPASS_SCRIPT_NAME: &str = "askpass.ps1"; +#[cfg(not(target_os = "windows"))] +const GPG_SCRIPT_NAME: &str = "gpg.sh"; + impl AskPassSession { /// This will create a new AskPassSession. /// You must retain this session until the master process exits. @@ -72,6 +85,8 @@ impl AskPassSession { let temp_dir = tempfile::Builder::new().prefix("zed-askpass").tempdir()?; let askpass_socket = temp_dir.path().join("askpass.sock"); let askpass_script_path = temp_dir.path().join(ASKPASS_SCRIPT_NAME); + #[cfg(not(target_os = "windows"))] + let gpg_script_path = temp_dir.path().join(GPG_SCRIPT_NAME); let (askpass_opened_tx, askpass_opened_rx) = oneshot::channel::<()>(); let listener = UnixListener::bind(&askpass_socket).context("creating askpass socket")?; #[cfg(not(target_os = "windows"))] @@ -135,9 +150,20 @@ impl AskPassSession { askpass_script_path.display() ); + #[cfg(not(target_os = "windows"))] + { + let gpg_script = generate_gpg_script(); + fs::write(&gpg_script_path, gpg_script) + .await + .with_context(|| format!("creating gpg wrapper script at {gpg_script_path:?}"))?; + make_file_executable(&gpg_script_path).await?; + } + Ok(Self { #[cfg(not(target_os = "windows"))] script_path: askpass_script_path, + #[cfg(not(target_os = "windows"))] + gpg_script_path, #[cfg(target_os = "windows")] secret, @@ -160,6 +186,19 @@ impl AskPassSession { &self.askpass_helper } + #[cfg(not(target_os = "windows"))] + pub fn gpg_script_path(&self) -> Option> { + Some(&self.gpg_script_path) + } + + #[cfg(target_os = "windows")] + pub fn gpg_script_path(&self) -> Option> { + // TODO implement wrapping GPG on Windows. This is more difficult than on Unix + // because we can't use --passphrase-fd with a nonstandard FD, and both --passphrase + // and --passphrase-file are insecure. + None:: + } + // This will run the askpass task forever, resolving as many authentication requests as needed. // The caller is responsible for examining the result of their own commands and cancelling this // future when this is no longer needed. Note that this can only be called once, but due to the @@ -263,3 +302,23 @@ fn generate_askpass_script(zed_path: &std::path::Path, askpass_socket: &std::pat askpass_socket = askpass_socket.display(), ) } + +#[inline] +#[cfg(not(target_os = "windows"))] +fn generate_gpg_script() -> String { + use unindent::Unindent as _; + + r#" + #!/bin/sh + set -eu + + unset GIT_CONFIG_PARAMETERS + GPG_PROGRAM=$(git config gpg.program || echo 'gpg') + PROMPT="Enter passphrase to unlock GPG key:" + PASSPHRASE=$(${GIT_ASKPASS} "${PROMPT}") + + exec "${GPG_PROGRAM}" --batch --no-tty --yes --passphrase-fd 3 --pinentry-mode loopback "$@" 3<, _options: CommitOptions, + _ask_pass: AskPassDelegate, _env: Arc>, - ) -> BoxFuture<'_, Result<()>> { + _cx: AsyncApp, + ) -> BoxFuture<'static, Result<()>> { unimplemented!() } diff --git a/crates/git/Cargo.toml b/crates/git/Cargo.toml index ab2210094d..3591e815a4 100644 --- a/crates/git/Cargo.toml +++ b/crates/git/Cargo.toml @@ -41,9 +41,9 @@ futures.workspace = true workspace-hack.workspace = true [dev-dependencies] +gpui = { workspace = true, features = ["test-support"] } pretty_assertions.workspace = true serde_json.workspace = true +tempfile.workspace = true text = { workspace = true, features = ["test-support"] } unindent.workspace = true -gpui = { workspace = true, features = ["test-support"] } -tempfile.workspace = true diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 2ecd4bb894..a704ba1482 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -391,8 +391,12 @@ pub trait GitRepository: Send + Sync { message: SharedString, name_and_email: Option<(SharedString, SharedString)>, options: CommitOptions, + askpass: AskPassDelegate, env: Arc>, - ) -> BoxFuture<'_, Result<()>>; + // This method takes an AsyncApp to ensure it's invoked on the main thread, + // otherwise git-credentials-manager won't work. + cx: AsyncApp, + ) -> BoxFuture<'static, Result<()>>; fn push( &self, @@ -1193,36 +1197,68 @@ impl GitRepository for RealGitRepository { message: SharedString, name_and_email: Option<(SharedString, SharedString)>, options: CommitOptions, + ask_pass: AskPassDelegate, env: Arc>, - ) -> BoxFuture<'_, Result<()>> { + cx: AsyncApp, + ) -> BoxFuture<'static, Result<()>> { let working_directory = self.working_directory(); - self.executor - .spawn(async move { - let mut cmd = new_smol_command("git"); - cmd.current_dir(&working_directory?) - .envs(env.iter()) - .args(["commit", "--quiet", "-m"]) - .arg(&message.to_string()) - .arg("--cleanup=strip"); + let executor = cx.background_executor().clone(); + async move { + let working_directory = working_directory?; + let have_user_git_askpass = env.contains_key("GIT_ASKPASS"); + let mut command = new_smol_command("git"); + command.current_dir(&working_directory).envs(env.iter()); - if options.amend { - cmd.arg("--amend"); - } + let ask_pass = if have_user_git_askpass { + None + } else { + Some(AskPassSession::new(&executor, ask_pass).await?) + }; - if let Some((name, email)) = name_and_email { - cmd.arg("--author").arg(&format!("{name} <{email}>")); - } + if let Some(program) = ask_pass + .as_ref() + .and_then(|ask_pass| ask_pass.gpg_script_path()) + { + command.arg("-c").arg(format!( + "gpg.program={}", + program.as_ref().to_string_lossy() + )); + } - let output = cmd.output().await?; + command + .args(["commit", "-m"]) + .arg(message.to_string()) + .arg("--cleanup=strip") + .stdin(smol::process::Stdio::null()) + .stdout(smol::process::Stdio::piped()) + .stderr(smol::process::Stdio::piped()); + if options.amend { + command.arg("--amend"); + } + + if let Some((name, email)) = name_and_email { + command.arg("--author").arg(&format!("{name} <{email}>")); + } + + if let Some(ask_pass) = ask_pass { + command.env("GIT_ASKPASS", ask_pass.script_path()); + let git_process = command.spawn()?; + + run_askpass_command(ask_pass, git_process).await?; + Ok(()) + } else { + let git_process = command.spawn()?; + let output = git_process.output().await?; anyhow::ensure!( output.status.success(), - "Failed to commit:\n{}", + "{}", String::from_utf8_lossy(&output.stderr) ); Ok(()) - }) - .boxed() + } + } + .boxed() } fn push( @@ -2046,12 +2082,16 @@ mod tests { ) .await .unwrap(); - repo.commit( - "Initial commit".into(), - None, - CommitOptions::default(), - Arc::new(checkpoint_author_envs()), - ) + cx.spawn(|cx| { + repo.commit( + "Initial commit".into(), + None, + CommitOptions::default(), + AskPassDelegate::new_always_failing(), + Arc::new(checkpoint_author_envs()), + cx, + ) + }) .await .unwrap(); @@ -2075,12 +2115,16 @@ mod tests { ) .await .unwrap(); - repo.commit( - "Commit after checkpoint".into(), - None, - CommitOptions::default(), - Arc::new(checkpoint_author_envs()), - ) + cx.spawn(|cx| { + repo.commit( + "Commit after checkpoint".into(), + None, + CommitOptions::default(), + AskPassDelegate::new_always_failing(), + Arc::new(checkpoint_author_envs()), + cx, + ) + }) .await .unwrap(); @@ -2213,12 +2257,16 @@ mod tests { ) .await .unwrap(); - repo.commit( - "Initial commit".into(), - None, - CommitOptions::default(), - Arc::new(checkpoint_author_envs()), - ) + cx.spawn(|cx| { + repo.commit( + "Initial commit".into(), + None, + CommitOptions::default(), + AskPassDelegate::new_always_failing(), + Arc::new(checkpoint_author_envs()), + cx, + ) + }) .await .unwrap(); diff --git a/crates/git_ui/src/git_panel.rs b/crates/git_ui/src/git_panel.rs index c50e2f8912..cc378af832 100644 --- a/crates/git_ui/src/git_panel.rs +++ b/crates/git_ui/src/git_panel.rs @@ -1574,10 +1574,15 @@ impl GitPanel { let task = if self.has_staged_changes() { // Repository serializes all git operations, so we can just send a commit immediately - let commit_task = active_repository.update(cx, |repo, cx| { - repo.commit(message.into(), None, options, cx) - }); - cx.background_spawn(async move { commit_task.await? }) + cx.spawn_in(window, async move |this, cx| { + let askpass_delegate = this.update_in(cx, |this, window, cx| { + this.askpass_delegate("git commit", window, cx) + })?; + let commit_task = active_repository.update(cx, |repo, cx| { + repo.commit(message.into(), None, options, askpass_delegate, cx) + })?; + commit_task.await? + }) } else { let changed_files = self .entries @@ -1594,10 +1599,13 @@ impl GitPanel { let stage_task = active_repository.update(cx, |repo, cx| repo.stage_entries(changed_files, cx)); - cx.spawn(async move |_, cx| { + cx.spawn_in(window, async move |this, cx| { stage_task.await?; + let askpass_delegate = this.update_in(cx, |this, window, cx| { + this.askpass_delegate("git commit".to_string(), window, cx) + })?; let commit_task = active_repository.update(cx, |repo, cx| { - repo.commit(message.into(), None, options, cx) + repo.commit(message.into(), None, options, askpass_delegate, cx) })?; commit_task.await? }) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 9ff3823e0f..69fe58aadb 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -1726,6 +1726,18 @@ impl GitStore { let repository_id = RepositoryId::from_proto(envelope.payload.repository_id); let repository_handle = Self::repository_for_request(&this, repository_id, &mut cx)?; + let askpass = if let Some(askpass_id) = envelope.payload.askpass_id { + make_remote_delegate( + this, + envelope.payload.project_id, + repository_id, + askpass_id, + &mut cx, + ) + } else { + AskPassDelegate::new_always_failing() + }; + let message = SharedString::from(envelope.payload.message); let name = envelope.payload.name.map(SharedString::from); let email = envelope.payload.email.map(SharedString::from); @@ -1739,6 +1751,7 @@ impl GitStore { CommitOptions { amend: options.amend, }, + askpass, cx, ) })? @@ -3462,11 +3475,14 @@ impl Repository { message: SharedString, name_and_email: Option<(SharedString, SharedString)>, options: CommitOptions, + askpass: AskPassDelegate, _cx: &mut App, ) -> oneshot::Receiver> { let id = self.id; + let askpass_delegates = self.askpass_delegates.clone(); + let askpass_id = util::post_inc(&mut self.latest_askpass_id); - self.send_job(Some("git commit".into()), move |git_repo, _cx| async move { + self.send_job(Some("git commit".into()), move |git_repo, cx| async move { match git_repo { RepositoryState::Local { backend, @@ -3474,10 +3490,16 @@ impl Repository { .. } => { backend - .commit(message, name_and_email, options, environment) + .commit(message, name_and_email, options, askpass, environment, cx) .await } RepositoryState::Remote { project_id, client } => { + askpass_delegates.lock().insert(askpass_id, askpass); + let _defer = util::defer(|| { + let askpass_delegate = askpass_delegates.lock().remove(&askpass_id); + debug_assert!(askpass_delegate.is_some()); + }); + let (name, email) = name_and_email.unzip(); client .request(proto::Commit { @@ -3489,9 +3511,9 @@ impl Repository { options: Some(proto::commit::CommitOptions { amend: options.amend, }), + askpass_id: Some(askpass_id), }) - .await - .context("sending commit request")?; + .await?; Ok(()) } diff --git a/crates/proto/proto/git.proto b/crates/proto/proto/git.proto index 1fdef2eea6..6645e91de7 100644 --- a/crates/proto/proto/git.proto +++ b/crates/proto/proto/git.proto @@ -294,6 +294,7 @@ message Commit { optional string email = 5; string message = 6; optional CommitOptions options = 7; + optional uint64 askpass_id = 8; message CommitOptions { bool amend = 1;