From 80040e11b609d1e1f8daaeb16c4473b2da1c0d34 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 14 Nov 2023 08:09:22 +0100 Subject: [PATCH] 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. --- Cargo.lock | 7 ++ gix-credentials/Cargo.toml | 1 + gix-credentials/src/program/mod.rs | 72 ++++++++++----- .../tests/program/from_custom_definition.rs | 92 +++++++++++++++---- 4 files changed, 134 insertions(+), 38 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bd526b0380e..9c617f3e81d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1485,6 +1485,7 @@ dependencies = [ "gix-trace 0.1.3", "gix-url", "serde", + "shell-words", "thiserror", ] @@ -4106,6 +4107,12 @@ dependencies = [ "lazy_static", ] +[[package]] +name = "shell-words" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "24188a676b6ae68c3b2cb3a01be17fbf7240ce009799bb56d5b1409051e78fde" + [[package]] name = "signal-hook" version = "0.3.17" diff --git a/gix-credentials/Cargo.toml b/gix-credentials/Cargo.toml index ceefa995efc..237816dbd39 100644 --- a/gix-credentials/Cargo.toml +++ b/gix-credentials/Cargo.toml @@ -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" diff --git a/gix-credentials/src/program/mod.rs b/gix-credentials/src/program/mod.rs index 66346d2b06a..f2c341ace5c 100644 --- a/gix-credentials/src/program/mod.rs +++ b/gix-credentials/src/program/mod.rs @@ -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)> { - 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)> { + 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"); diff --git a/gix-credentials/tests/program/from_custom_definition.rs b/gix-credentials/tests/program/from_custom_definition.rs index 1b9d8a0561d..15d4d7a4d1d 100644 --- a/gix-credentials/tests/program/from_custom_definition.rs +++ b/gix-credentials/tests/program/from_custom_definition.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." ); }