fix: make certain git-credential helper implementations work from cmd-prompts on Windows (#1103)

On prompts, typically only `git.exe` is available, but no shell (i.e. `sh`).
Thus, we have to prefer going through `git.exe` to launch credential helpers and possibly split
simple arguments ourselves.

Author: Byron

Cargo.lock

@@ -28,6 +28,7 @@ gix-trace = { version = "^0.1.3", path = "../gix-trace" }
 thiserror = "1.0.32"
 serde = { version = "1.0.114", optional = true, default-features = false, features = ["derive"] }
 bstr = { version = "1.3.0", default-features = false, features = ["std"]}
+shell-words = "1.0"
 
 
 

gix-credentials/Cargo.toml

@@ -12,7 +12,8 @@ pub enum Kind {
     /// A custom credentials helper, as identified just by the name with optional arguments
     ExternalName {
         /// The name like `foo` along with optional args, like `foo --arg --bar="a b"`, with arguments using `sh` shell quoting rules.
-        /// The program executed will be `git-credential-foo` if `name_and_args` starts with `foo`.
+        /// The program executed will be `git-credential-foo [args]` if `name_and_args` starts with `foo [args]`.
+        /// Note that a shell is only used if it's needed.
         name_and_args: BString,
     },
     /// A custom credentials helper, as identified just by the absolute path to the program and optional arguments. The program is executed through a shell.
@@ -53,7 +54,6 @@ impl Program {
                 if gix_path::is_absolute(path) {
                     Kind::ExternalPath { path_and_args: input }
                 } else {
-                    input.insert_str(0, "git credential-");
                     Kind::ExternalName { name_and_args: input }
                 }
             };
@@ -65,33 +65,41 @@ impl Program {
         }
         from_custom_definition_inner(input.into())
     }
-}
-
-/// Builder
-impl Program {
-    /// By default `stderr` of programs is inherited and typically displayed in the terminal.
-    pub fn suppress_stderr(mut self) -> Self {
-        self.stderr = false;
-        self
-    }
-}
 
-impl Program {
-    pub(crate) fn start(
-        &mut self,
-        action: &helper::Action,
-    ) -> std::io::Result<(std::process::ChildStdin, Option<std::process::ChildStdout>)> {
-        assert!(self.child.is_none(), "BUG: must not call `start()` twice");
+    /// Convert the program into the respective command, suitable to invoke `action`.
+    pub fn to_command(&self, action: &helper::Action) -> std::process::Command {
+        let git_program = cfg!(windows).then(|| "git.exe").unwrap_or("git");
         let mut cmd = match &self.kind {
             Kind::Builtin => {
-                let mut cmd = Command::new(cfg!(windows).then(|| "git.exe").unwrap_or("git"));
+                let mut cmd = Command::new(git_program);
                 cmd.arg("credential").arg(action.as_arg(false));
                 cmd
             }
-            Kind::ExternalShellScript(for_shell)
-            | Kind::ExternalName {
-                name_and_args: for_shell,
+            Kind::ExternalName { name_and_args } => {
+                let mut args = name_and_args.clone();
+                args.insert_str(0, "credential-");
+                args.insert_str(0, " ");
+                args.insert_str(0, git_program);
+                let split_args = if args.find_byteset(b"\\|&;<>()$`\n*?[#~%").is_none() {
+                    args.to_str()
+                        .ok()
+                        .and_then(|args| shell_words::split(args).ok().map(Vec::into_iter))
+                } else {
+                    None
+                };
+                match split_args {
+                    Some(mut args) => {
+                        let mut cmd = Command::new(args.next().expect("non-empty input"));
+                        cmd.args(args).arg(action.as_arg(true));
+                        cmd
+                    }
+                    None => gix_command::prepare(gix_path::from_bstr(args.as_ref()).into_owned())
+                        .arg(action.as_arg(true))
+                        .with_shell()
+                        .into(),
+                }
             }
+            Kind::ExternalShellScript(for_shell)
             | Kind::ExternalPath {
                 path_and_args: for_shell,
             } => gix_command::prepare(gix_path::from_bstr(for_shell.as_bstr()).as_ref())
@@ -106,6 +114,26 @@ impl Program {
                 Stdio::null()
             })
             .stderr(if self.stderr { Stdio::inherit() } else { Stdio::null() });
+        cmd
+    }
+}
+
+/// Builder
+impl Program {
+    /// By default `stderr` of programs is inherited and typically displayed in the terminal.
+    pub fn suppress_stderr(mut self) -> Self {
+        self.stderr = false;
+        self
+    }
+}
+
+impl Program {
+    pub(crate) fn start(
+        &mut self,
+        action: &helper::Action,
+    ) -> std::io::Result<(std::process::ChildStdin, Option<std::process::ChildStdout>)> {
+        assert!(self.child.is_none(), "BUG: must not call `start()` twice");
+        let mut cmd = self.to_command(action);
         gix_trace::debug!(cmd = ?cmd, "launching credential helper");
         let mut child = cmd.spawn()?;
         let stdin = child.stdin.take().expect("stdin to be configured");

gix-credentials/src/program/mod.rs

@@ -1,42 +1,102 @@
-use gix_credentials::{program::Kind, Program};
+use gix_credentials::{helper, program::Kind, Program};
+
+#[cfg(windows)]
+const GIT: &str = "git.exe";
+#[cfg(not(windows))]
+const GIT: &str = "git";
+
+#[cfg(windows)]
+const SH: &str = "sh";
+#[cfg(not(windows))]
+const SH: &str = "/bin/sh";
 
 #[test]
-fn script() {
-    assert!(
-        matches!(Program::from_custom_definition("!exe").kind, Kind::ExternalShellScript(script) if script == "exe")
+fn empty() {
+    let prog = Program::from_custom_definition("");
+    assert!(matches!(&prog.kind, Kind::ExternalName { name_and_args } if name_and_args == ""));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{GIT}" "credential-" "store""#),
+        "not useful, but allowed, would have to be caught elsewhere"
+    );
+}
+
+#[test]
+fn simple_script_in_path() {
+    let prog = Program::from_custom_definition("!exe");
+    assert!(matches!(&prog.kind, Kind::ExternalShellScript(script) if script == "exe"));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        r#""exe" "store""#,
+        "it didn't detect anything shell-scripty, and thus doesn't use a shell"
     );
 }
 
 #[test]
 fn name_with_args() {
     let input = "name --arg --bar=\"a b\"";
-    let expected = "git credential-name --arg --bar=\"a b\"";
-    assert!(
-        matches!(Program::from_custom_definition(input).kind, Kind::ExternalName{name_and_args} if name_and_args == expected)
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalName{name_and_args} if name_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{GIT}" "credential-name" "--arg" "--bar=a b" "store""#)
+    );
+}
+
+#[test]
+fn name_with_special_args() {
+    let input = "name --arg --bar=~/folder/in/home";
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalName{name_and_args} if name_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{SH}" "-c" "{GIT} credential-name --arg --bar=~/folder/in/home \"$@\"" "--" "store""#)
     );
 }
 
 #[test]
 fn name() {
     let input = "name";
-    let expected = "git credential-name";
-    assert!(
-        matches!(Program::from_custom_definition(input).kind, Kind::ExternalName{name_and_args} if name_and_args == expected)
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalName{name_and_args} if name_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{GIT}" "credential-name" "store""#),
+        "we detect that this can run without shell, which is also more portable on windows"
     );
 }
 
 #[test]
-fn path_with_args() {
+fn path_with_args_that_definitely_need_shell() {
     let input = "/abs/name --arg --bar=\"a b\"";
-    assert!(
-        matches!(Program::from_custom_definition(input).kind, Kind::ExternalPath{path_and_args} if path_and_args == input)
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{SH}" "-c" "/abs/name --arg --bar=\"a b\" \"$@\"" "--" "store""#)
     );
 }
 
 #[test]
-fn path() {
+fn path_without_args() {
     let input = "/abs/name";
-    assert!(
-        matches!(Program::from_custom_definition(input).kind, Kind::ExternalPath{path_and_args} if path_and_args == input)
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        r#""/abs/name" "store""#,
+        "no shell is used"
+    );
+}
+
+#[test]
+fn path_with_simple_args() {
+    let input = "/abs/name a b";
+    let prog = Program::from_custom_definition(input);
+    assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input));
+    assert_eq!(
+        format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))),
+        format!(r#""{SH}" "-c" "/abs/name a b \"$@\"" "--" "store""#),
+        "a shell is used as well because there are arguments, and we don't do splitting ourselves. On windows, this can be a problem."
     );
 }