Merge pull request #3969 from gitbutlerapp/handle-sign-failure

handle sign failure

app/src/lib/backend/ipc.ts

@@ -6,7 +6,8 @@ export enum Code {
 	Unknown = 'errors.unknown',
 	Validation = 'errors.validation',
 	ProjectsGitAuth = 'errors.projects.git.auth',
-	DefaultTargetNotFound = 'errors.projects.default_target.not_found'
+	DefaultTargetNotFound = 'errors.projects.default_target.not_found',
+	CommitSigningFailed = 'errors.commit.signing_failed'
 }
 
 export class UserError extends Error {

app/src/lib/vbranches/branchController.ts

@@ -64,7 +64,20 @@ export class BranchController {
 			});
 			posthog.capture('Commit Successful');
 		} catch (err: any) {
-			showError('Failed to commit changes', err);
+			if (err.code === 'errors.commit.signing_failed') {
+				showToast({
+					title: 'Failed to commit due to signing error',
+					message: `
+You can disable commit signing in the project settings or review the signing setup within your git configuration.
+
+Please check our [documentation](https://docs.gitbutler.com/features/virtual-branches/verifying-commits) on setting up commit signing and verification.
+					`,
+					errorMessage: err.message,
+					style: 'error'
+				});
+			} else {
+				showError('Failed to commit changes', err);
+			}
 			posthog.capture('Commit Failed', err);
 			throw err;
 		}

crates/gitbutler-core/src/config/git.rs

@@ -2,7 +2,7 @@ use crate::projects::Project;
 use anyhow::Result;
 use git2::ConfigLevel;
 
-const CFG_SIGN_COMMITS: &str = "gitbutler.signCommits";
+use super::CFG_SIGN_COMMITS;
 
 impl Project {
     pub fn set_sign_commits(&self, val: bool) -> Result<()> {

crates/gitbutler-core/src/config/mod.rs

@@ -1 +1,3 @@
 pub mod git;
+
+pub const CFG_SIGN_COMMITS: &str = "gitbutler.signCommits";

crates/gitbutler-core/src/error.rs

@@ -131,6 +131,7 @@ pub enum Code {
     Validation,
     ProjectGitAuth,
     DefaultTargetNotFound,
+    CommitSigningFailed,
 }
 
 impl std::fmt::Display for Code {
@@ -140,6 +141,7 @@ impl std::fmt::Display for Code {
             Code::Validation => "errors.validation",
             Code::ProjectGitAuth => "errors.projects.git.auth",
             Code::DefaultTargetNotFound => "errors.projects.default_target.not_found",
+            Code::CommitSigningFailed => "errors.commit.signing_failed",
         };
         f.write_str(code)
     }

crates/gitbutler-core/src/git/repository_ext.rs

@@ -1,8 +1,10 @@
-use anyhow::{bail, Context, Result};
+use anyhow::{anyhow, bail, Context, Result};
 use git2::{BlameOptions, Repository, Tree};
 use std::{path::Path, process::Stdio, str};
 use tracing::instrument;
 
+use crate::{config::CFG_SIGN_COMMITS, error::Code};
+
 use super::Refname;
 use std::io::Write;
 #[cfg(unix)]
@@ -79,8 +81,16 @@ impl RepositoryExt for Repository {
 
         let commit_buffer = inject_change_id(&commit_buffer, change_id)?;
 
-        let oid = do_commit_buffer(self, commit_buffer)?;
+        let should_sign = self.config()?.get_bool(CFG_SIGN_COMMITS).unwrap_or(false);
 
+        let oid = if should_sign {
+            let signature = sign_buffer(self, &commit_buffer)
+                .map_err(|e| anyhow!(e).context(Code::CommitSigningFailed))?;
+            self.commit_signed(&commit_buffer, &signature, None)?
+        } else {
+            self.odb()?
+                .write(git2::ObjectType::Commit, commit_buffer.as_bytes())?
+        };
         // update reference
         if let Some(refname) = update_ref {
             self.reference(&refname.to_string(), oid, true, message)?;
@@ -106,123 +116,107 @@ impl RepositoryExt for Repository {
     }
 }
 
-/// takes raw commit data and commits it to the repository
+/// Signs the buffer with the configured gpg key, returning the signature.
-/// - if the git config commit.gpgSign is set, it will sign the commit
+fn sign_buffer(repo: &git2::Repository, buffer: &String) -> Result<String> {
-/// returns an oid of the new commit object
-fn do_commit_buffer(repo: &git2::Repository, buffer: String) -> Result<git2::Oid> {
     // check git config for gpg.signingkey
-    let should_sign = repo.config()?.get_bool("commit.gpgSign").unwrap_or(false);
+    // TODO: support gpg.ssh.defaultKeyCommand to get the signing key if this value doesn't exist
-    if should_sign {
+    let signing_key = repo.config()?.get_string("user.signingkey");
-        // TODO: support gpg.ssh.defaultKeyCommand to get the signing key if this value doesn't exist
+    if let Ok(signing_key) = signing_key {
-        let signing_key = repo.config()?.get_string("user.signingkey");
+        let sign_format = repo.config()?.get_string("gpg.format");
-        if let Ok(signing_key) = signing_key {
+        let is_ssh = if let Ok(sign_format) = sign_format {
-            let sign_format = repo.config()?.get_string("gpg.format");
+            sign_format == "ssh"
-            let is_ssh = if let Ok(sign_format) = sign_format {
+        } else {
-                sign_format == "ssh"
+            false
+        };
+
+        if is_ssh {
+            // write commit data to a temp file so we can sign it
+            let mut signature_storage = tempfile::NamedTempFile::new()?;
+            signature_storage.write_all(buffer.as_ref())?;
+            let buffer_file_to_sign_path = signature_storage.into_temp_path();
+
+            let gpg_program = repo.config()?.get_string("gpg.ssh.program");
+            let mut cmd =
+                std::process::Command::new(gpg_program.unwrap_or("ssh-keygen".to_string()));
+            cmd.args(["-Y", "sign", "-n", "git", "-f"]);
+
+            #[cfg(windows)]
+            cmd.creation_flags(0x08000000); // CREATE_NO_WINDOW
+
+            let output;
+            // support literal ssh key
+            if let (true, signing_key) = is_literal_ssh_key(&signing_key) {
+                // write the key to a temp file
+                let mut key_storage = tempfile::NamedTempFile::new()?;
+                key_storage.write_all(signing_key.as_bytes())?;
+
+                // if on unix
+                #[cfg(unix)]
+                {
+                    // make sure the tempfile permissions are acceptable for a private ssh key
+                    let mut permissions = key_storage.as_file().metadata()?.permissions();
+                    permissions.set_mode(0o600);
+                    key_storage.as_file().set_permissions(permissions)?;
+                }
+
+                let key_file_path = key_storage.into_temp_path();
+
+                cmd.arg(&key_file_path);
+                cmd.arg("-U");
+                cmd.arg(&buffer_file_to_sign_path);
+                cmd.stdout(Stdio::piped());
+                cmd.stdin(Stdio::null());
+
+                let child = cmd.spawn()?;
+                output = child.wait_with_output()?;
             } else {
-                false
+                cmd.arg(signing_key);
-            };
+                cmd.arg(&buffer_file_to_sign_path);
+                cmd.stdout(Stdio::piped());
+                cmd.stdin(Stdio::null());
 
-            if is_ssh {
+                let child = cmd.spawn()?;
-                // write commit data to a temp file so we can sign it
+                output = child.wait_with_output()?;
-                let mut signature_storage = tempfile::NamedTempFile::new()?;
+            }
-                signature_storage.write_all(buffer.as_ref())?;
-                let buffer_file_to_sign_path = signature_storage.into_temp_path();
 
-                let gpg_program = repo.config()?.get_string("gpg.ssh.program");
+            if output.status.success() {
-                let mut cmd =
+                // read signed_storage path plus .sig
-                    std::process::Command::new(gpg_program.unwrap_or("ssh-keygen".to_string()));
+                let signature_path = buffer_file_to_sign_path.with_extension("sig");
-                cmd.args(["-Y", "sign", "-n", "git", "-f"]);
+                let sig_data = std::fs::read(signature_path)?;
+                let signature = String::from_utf8_lossy(&sig_data).into_owned();
+                return Ok(signature);
+            }
+        } else {
+            // is gpg
+            let gpg_program = repo.config()?.get_string("gpg.program");
+            let mut cmd = std::process::Command::new(gpg_program.unwrap_or("gpg".to_string()));
+            cmd.args(["--status-fd=2", "-bsau", &signing_key])
+                //.arg(&signed_storage)
+                .arg("-")
+                .stdout(Stdio::piped())
+                .stdin(Stdio::piped());
 
-                #[cfg(windows)]
+            #[cfg(windows)]
-                cmd.creation_flags(0x08000000); // CREATE_NO_WINDOW
+            cmd.creation_flags(0x08000000); // CREATE_NO_WINDOW
 
-                let output;
+            let mut child = cmd
-                // support literal ssh key
+                .spawn()
-                if let (true, signing_key) = is_literal_ssh_key(&signing_key) {
+                .context(anyhow::format_err!("failed to spawn {:?}", cmd))?;
-                    // write the key to a temp file
+            child
-                    let mut key_storage = tempfile::NamedTempFile::new()?;
+                .stdin
-                    key_storage.write_all(signing_key.as_bytes())?;
+                .take()
+                .expect("configured")
+                .write_all(buffer.to_string().as_ref())?;
 
-                    // if on unix
+            let output = child.wait_with_output()?;
-                    #[cfg(unix)]
+            if output.status.success() {
-                    {
+                // read stdout
-                        // make sure the tempfile permissions are acceptable for a private ssh key
+                let signature = String::from_utf8_lossy(&output.stdout).into_owned();
-                        let mut permissions = key_storage.as_file().metadata()?.permissions();
+                return Ok(signature);
-                        permissions.set_mode(0o600);
-                        key_storage.as_file().set_permissions(permissions)?;
-                    }
-
-                    let key_file_path = key_storage.into_temp_path();
-
-                    cmd.arg(&key_file_path);
-                    cmd.arg("-U");
-                    cmd.arg(&buffer_file_to_sign_path);
-                    cmd.stdout(Stdio::piped());
-                    cmd.stdin(Stdio::null());
-
-                    let child = cmd.spawn()?;
-                    output = child.wait_with_output()?;
-                } else {
-                    cmd.arg(signing_key);
-                    cmd.arg(&buffer_file_to_sign_path);
-                    cmd.stdout(Stdio::piped());
-                    cmd.stdin(Stdio::null());
-
-                    let child = cmd.spawn()?;
-                    output = child.wait_with_output()?;
-                }
-
-                if output.status.success() {
-                    // read signed_storage path plus .sig
-                    let signature_path = buffer_file_to_sign_path.with_extension("sig");
-                    let sig_data = std::fs::read(signature_path)?;
-                    let signature = String::from_utf8_lossy(&sig_data);
-                    let oid = repo
-                        .commit_signed(&buffer, &signature, None)
-                        .map(Into::into)
-                        .map_err(Into::into);
-                    return oid;
-                }
-            } else {
-                // is gpg
-                let gpg_program = repo.config()?.get_string("gpg.program");
-                let mut cmd = std::process::Command::new(gpg_program.unwrap_or("gpg".to_string()));
-                cmd.args(["--status-fd=2", "-bsau", &signing_key])
-                    //.arg(&signed_storage)
-                    .arg("-")
-                    .stdout(Stdio::piped())
-                    .stdin(Stdio::piped());
-
-                #[cfg(windows)]
-                cmd.creation_flags(0x08000000); // CREATE_NO_WINDOW
-
-                let mut child = cmd.spawn()?;
-                child
-                    .stdin
-                    .take()
-                    .expect("configured")
-                    .write_all(buffer.to_string().as_ref())?;
-
-                let output = child.wait_with_output()?;
-                if output.status.success() {
-                    // read stdout
-                    let signature = String::from_utf8_lossy(&output.stdout);
-                    let oid = repo
-                        .commit_signed(&buffer, &signature, None)
-                        .map(Into::into)
-                        .map_err(Into::into);
-                    return oid;
-                }
             }
         }
     }
-
+    Err(anyhow::anyhow!("Unsupported commit signing method"))
-    let oid = repo
-        .odb()?
-        .write(git2::ObjectType::Commit, buffer.as_bytes())?;
-
-    Ok(oid)
 }
 
 fn is_literal_ssh_key(string: &str) -> (bool, &str) {
@@ -244,7 +238,7 @@ fn inject_change_id(commit_buffer: &[u8], change_id: Option<&str>) -> Result<Str
         .unwrap_or_else(|| format!("{}", uuid::Uuid::new_v4()));
 
     let commit_ends_in_newline = commit_buffer.ends_with(b"\n");
-    let commit_buffer = str::from_utf8(commit_buffer).unwrap();
+    let commit_buffer = str::from_utf8(commit_buffer)?;
     let lines = commit_buffer.lines();
     let mut new_buffer = String::new();
     let mut found = false;