Skip to content

fix: use bash -c to get interpretation of tilde and env vars in path #5756

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 11, 2024

Conversation

ndom91
Copy link
Contributor

@ndom91 ndom91 commented Dec 6, 2024

☕️ Reasoning

  • Allow users to pass relative paths in signing key path input

🧢 Changes

  • Use bash -c ... to execute our &gpg_program command
  • Detects the users system default $SHELL first, then falls back to sh if it cant be found

Copy link

vercel bot commented Dec 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
gitbutler-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 11, 2024 10:45am
gitbutler-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 11, 2024 10:45am

Copy link
Collaborator

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something you could try is the to use gix-command for this. It does the no-window thing automatically, and has a lot of tricks to execute correctly even on platforms that aren't really made for executing non-gui programs.

This could get you started…

let gpg_in_sh: std::process::Command = gix::command::prepare(gpg_cmd).with_shell().into()

…to then finalize the command configuration the traditional way.

This is how Git executes things as well.

@ndom91
Copy link
Contributor Author

ndom91 commented Dec 9, 2024

Something you could try is the to use gix-command for this. It does the no-window thing automatically, and has a lot of tricks to execute correctly even on platforms that aren't really made for executing non-gui programs.

This could get you started…

let gpg_in_sh: std::process::Command = gix::command::prepare(gpg_cmd).with_shell().into()

…to then finalize the command configuration the traditional way.

This is how Git executes things as well.

Okay trying this now. I'm basiclaly passing the finished Command into the gix::command prepare fn so we end up with this:

// cmd: "ssh-keygen" "-Y" "sign" "-n" "git" "-f" "$HOME/.ssh/id_ndo4.pub /tmp/.tmpQDYPg7"
let mut gpg_cmd: std::process::Command =
    gix::command::prepare(format!("{:?}", cmd))
        .with_shell()
        .into();

However, this seems to both escape the pre-existing " characters and append an additional -- onto the end of the command as if its expecting more arguments that are designed to be passed to its child. i.e.

gpg_cmd: "/bin/sh" "-c" "\"ssh-keygen\" \"-Y\" \"sign\" \"-n\" \"git\" \"-f\" \"$HOME/.ssh/id_ndo4.pub /tmp/.tmpQDYPg7\"" "--"

Any way to avoid the additional escaping and appending --? @Byron

@Byron
Copy link
Collaborator

Byron commented Dec 9, 2024

Any way to avoid the additional escaping and appending --? @Byron

Is this a problem for execution? I tried to execute it and it appeared to work fine, except that it can't find the files as expected.

The -- makes it possible to pass additional arguments to the script, which can be used by hooks and callbacks, even though it's not needed here, but it's also nothing that is in the way I think.

Probably I am missing something though, and hope you could clarify.
Thanks again.

@ndom91
Copy link
Contributor Author

ndom91 commented Dec 9, 2024

Yeah no so the -- isn't an issue for execution the way I've got it working now. However it looks like the sh shell it's using by default doesn't interpret the tilde correctly. $HOME does work though.

There doesn't seem to be a way to customize the shell that with_shell() is using, is that correct?

@Byron
Copy link
Collaborator

Byron commented Dec 9, 2024

There doesn't seem to be a way to customize the shell that with_shell() is using, is that correct?

That is correct. It's easy to add though and I am working on it already, but in the middle of that I wondered why ~ doesn't seem to be replaced even though it worked in my tests with sh v.3.2.

Maybe you tried it on Windows? There it performs argument splitting itself if it can do so safely, completely bypassing the shell by default. However, it would refrain from doing so if it finds any of \\|&;<>()$``\n*?[#~%.
I guess I am still puzzled why sh didn't expand ~ like it should.

@ndom91
Copy link
Contributor Author

ndom91 commented Dec 10, 2024

Hmm yeah good point, just tried using the sh shell in a terminal emulator locally and it resolved ~ just fine.

A quick triple check on this PR though does confirm:

  • /home/ndo/.ssh/key works
  • $HOME/.ssh/key works
  • ~/.ssh/key doesnt work :(
    • errors out with failed to sign ssh: couldn't load public key ~/.ssh/key.pub: no such file or directory

@Byron
Copy link
Collaborator

Byron commented Dec 10, 2024

tl;dr: The problem seems to be the extra quotes - can these be removed? I also noticed that there seem to be two files within one quote: // cmd: "ssh-keygen" "-Y" "sign" "-n" "git" "-f" "$HOME/.ssh/id_ndo4.pub /tmp/.tmpQDYPg7" (right here at the end) which seems strange.


I cannot reproduce this on MacOS. In the testsuite, there was this test that I modified to see what happens:

        let mut cmd = std::process::Command::from(gix_command::prepare("~/bin/exe --foo \"a b\"").with_shell());
        assert_eq!(
            format!("{cmd:?}"),
            format!(r#""{SH}" "-c" "~/bin/exe --foo \"a b\"" "--""#),
            "this always needs a shell as we need tilde expansion"
        );
        dbg!(cmd.output().unwrap().stdout.as_bstr());

And this is the output: --: /Users/byron/bin/exe: No such file or directory - it definitely resolved the tilde.
nn
However, putting the tilde in argument position…

        let mut cmd = std::process::Command::from(gix_command::prepare("echo \"~/bin/hello\"").with_shell());
        dbg!(cmd.output().unwrap().stdout.as_bstr());

… doesn't resolve: "~/bin/hello\n".

The same is true when using bash:

        let mut cmd = std::process::Command::from(
            gix_command::prepare("echo \"~/bin/hello\"")
                .with_shell()
                .with_shell_program("bash"),
        );
        dbg!(&cmd);
        dbg!(cmd.output().unwrap().stdout.as_bstr());

Which resolves to:

[gix-command/tests/command.rs:345:9] &cmd = Command {
    program: "bash",
    args: [
        "bash",
        "-c",
        "echo \"~/bin/hello\"",
        "--",
    ],
    stdin: Some(
        Null,
    ),
    stdout: Some(
        MakePipe,
    ),
    stderr: Some(
        Inherit,
    ),
}
[gix-command/tests/command.rs:346:9] cmd.output().unwrap().stdout.as_bstr() = "~/bin/hello\n"

However, it seems to be about the extra quotes on bash, so removing them…

        let mut cmd = std::process::Command::from(
            gix_command::prepare("echo ~/bin/hello")
                .with_shell()
                .with_shell_program("bash"),
        );

…works: "/Users/byron/bin/hello\n"

And fortunately the same is true with sh:

        let mut cmd = std::process::Command::from(gix_command::prepare("echo ~/bin/hello").with_shell());
        dbg!(&cmd);
        dbg!(cmd.output().unwrap().stdout.as_bstr());
[gix-command/tests/command.rs:341:9] &cmd = Command {
    program: "/bin/sh",
    args: [
        "/bin/sh",
        "-c",
        "echo ~/bin/hello",
        "--",
    ],
    stdin: Some(
        Null,
    ),
    stdout: Some(
        MakePipe,
    ),
    stderr: Some(
        Inherit,
    ),
}
[gix-command/tests/command.rs:342:9] cmd.output().unwrap().stdout.as_bstr() = "/Users/byron/bin/hello\n"

The problem seems to be the extra quotes - can these be removed? I also noticed that there seem to be two files within one quote: // cmd: "ssh-keygen" "-Y" "sign" "-n" "git" "-f" "$HOME/.ssh/id_ndo4.pub /tmp/.tmpQDYPg7" (right here at the end) which seems strange.

@ndom91
Copy link
Contributor Author

ndom91 commented Dec 10, 2024

Thanks for the deep-dive!

So the extra quotes seem to be by default from the rust std::process::Command output, when you append args to a command. Maybe we can just replace() them all out of the resulting string version of the command?

Regarding the final two paths being wrapped in 1 set of quotes - that's been fixed in the last 1 or 2 commits in this branch already. So the currently, the cmd being passed to gix::command::prepare() is:

// 1. CMD::
"ssh-keygen" "-Y" "sign" "-n" "git" "-f" "~/.ssh/id_ndo4.pub" "/tmp/.tmpdoUZgL"

And the output of the gix prepare with_shell() is

// 2. GPG_CMD:
"/bin/sh" "-c" "\"ssh-keygen\" \"-Y\" \"sign\" \"-n\" \"git\" \"-f\" \"~/.ssh/id_ndo4.pub\" \"/tmp/.tmpwjuXH3\"" "--"

For context, those come from here:

println!("1. CMD: {:?}", cmd);
let mut gpg_cmd: std::process::Command =
    gix::command::prepare(format!("{:?}", cmd))
        .with_shell()
        .into();

println!("2. GPG_CMD: {:?}", gpg_cmd);
gpg_cmd.stderr(Stdio::piped());
gpg_cmd.stdout(Stdio::piped());
gpg_cmd.stdin(Stdio::null());

let child = gpg_cmd.spawn()?;
output = child.wait_with_output()?;

@ndom91
Copy link
Contributor Author

ndom91 commented Dec 10, 2024

I've got an alternative here which seems to work well with both ~ and $HOME 🙏

let cmd_str = cmd
    .get_args()
    .map(|arg| arg.to_str().unwrap_or(""))
    .collect::<Vec<&str>>()
    .join(" ");
let full_cmd =
    format!("{} {}", cmd.get_program().to_str().unwrap_or(""), cmd_str);

let mut gpg_cmd: std::process::Command =
    gix::command::prepare(full_cmd).with_shell().into();

println!("GPG_CMD3: {:?}", gpg_cmd);

Which results in:

GPG_CMD3: "/bin/sh" "-c" "ssh-keygen -Y sign -n git -f ~/.ssh/id_ndo4.pub /tmp/.tmpsMoprG" "--"

@Byron
Copy link
Collaborator

Byron commented Dec 10, 2024

Good to hear it works now!

Somehow I can't shake the feeling that there is no need at all to go through std::process::Command to get the gpg command going - can't it just all be substituted into a string? String is also fine here as we know the paths will only be plain ASCII, and so will the program name.

@ndom91
Copy link
Contributor Author

ndom91 commented Dec 10, 2024

Thanks for the extra push haha, that was a bit convoluted. Works now with both ~ and $HOME with the "just strings" variant thats in this PR now as well 🥳

cmd_string: "ssh-keygen -Y sign -n git -f $HOME/.ssh/id_ndo4.pub /tmp/.tmpZqo4Lf"
GPG_CMD2: "/bin/sh" "-c" "ssh-keygen -Y sign -n git -f $HOME/.ssh/id_ndo4.pub /tmp/.tmpZqo4Lf" "--"

@ndom91 ndom91 force-pushed the use-bash-to-interpret-tilde-etc branch from c857c4f to 814c099 Compare December 11, 2024 10:43
@ndom91 ndom91 enabled auto-merge (rebase) December 11, 2024 10:43
@ndom91 ndom91 merged commit 9279eba into master Dec 11, 2024
21 checks passed
@ndom91 ndom91 deleted the use-bash-to-interpret-tilde-etc branch December 11, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants