Skip to content

fix: escape GIT_SSH before promoting to GIT_SSH_COMMAND#13239

Closed
slarse wants to merge 1 commit intomasterfrom
GB-1286/fix-git-ssh
Closed

fix: escape GIT_SSH before promoting to GIT_SSH_COMMAND#13239
slarse wants to merge 1 commit intomasterfrom
GB-1286/fix-git-ssh

Conversation

@slarse
Copy link
Copy Markdown
Contributor

@slarse slarse commented Apr 9, 2026

🧢 Changes

Title pretty much sums it up.

🎫 Affected issues

Fixes: #13088

@vercel
Copy link
Copy Markdown

vercel bot commented Apr 9, 2026 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
gitbutler-web Ignored Ignored Preview Apr 9, 2026 11:28am

Request Review

@github-actions github-actions bot added the rust Pull requests that update Rust code label Apr 9, 2026
@slarse slarse force-pushed the GB-1286/fix-git-ssh branch from 3922c99 to 2d99a1e Compare April 9, 2026 11:28
@slarse
Copy link
Copy Markdown
Contributor Author

slarse commented Apr 9, 2026 •
edited
Loading

Okay, so after toying around with just escaping GIT_SSH, I've found that this whole thing of taking the user's configuration and haphazardly modifying it is just a plain bad idea.

Escaping hassle aside, if the user has set any of GIT_SSH, GIT_SSH_COMMAND or core.sshCommand, it's not safe for us to just add options to it (which is the entire purpose of promoting GIT_SSH to GIT_SSH_COMMAND).

For example, the user in #13088 is using plink. Even if we escape the path properly, plink still doesn't accept the additional OpenSSH-specific options we append (-o StrictHostKeyChecking=accept-new -o KbdInteractiveAuthentication=no), so it errors out even when the executable is correctly resolved.

So my take is that we should only set GIT_SSH_COMMAND to our preferred default if the user has configured none of GIT_SSH, GIT_SSH_COMMAND or core.sshCommand.

I'll leave this PR here for reference and start over on a new fix.

Byron
Byron reviewed Apr 10, 2026
View reviewed changes
crates/gitbutler-git/src/repository.rs
// Escape the path with single quotes so it's safe to execute by a POSIX shell.
//
// Even on Windows, Git uses a POSIX(ish) shell to execute GIT_SSH_COMMAND, see https://github.com/git/git/blob/b15384c06f77bc2d34d0d3623a8a58218313a561/run-command.c#L277-L286
let escaped_git_ssh = git_ssh.as_ref().replace("'", r"'\''");
Copy link
Copy Markdown
Collaborator

@Byron Byron Apr 10, 2026

Choose a reason for hiding this comment

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

You might be able to use https://docs.rs/gix-quote/latest/gix_quote/fn.single.html .

Copy link
Copy Markdown
Contributor Author

@slarse slarse Apr 10, 2026

Choose a reason for hiding this comment

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

Thanks! Good to know that's there.

@slarse slarse mentioned this pull request Apr 10, 2026
Open
@slarse slarse closed this Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Byron Byron Byron left review comments

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

rust Pull requests that update Rust code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Failed to fetch (from private GitHub remotes, git/SSH protocol)

2 participants

@slarse @Byron