Fix #6013#6028
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@Byron is attempting to deploy a commit to the GitButler Team on Vercel. A member of the Team first needs to authorize it. |
…he signing command. We run it in a shell, specifically the Git shell, on Windows so more programs are available. On Unix it sholdn't hurt even though it doesn't have a specific purpose there. The quoting is needed on Windows to prevent backslashes to mean escaping.
There was a problem hiding this comment.
If it is ever possible for the strings being single-quoted to contain single quotes, then their single quotes should also be neutralized by replacing each original occurrence of ' with '\''.
(This works in sh, other shells behaving POSIX-compatibly, and in practice in Bourne-style shells in general. It looks like it would always work here. It is not guaranteed to work in non-Bourne-style shells. But in most non-Bourne-style shells, single-quoting is not as strong, so pasting them on the outside would be insufficient for other reasons.)
For quoting command when it is, in its entirety, the path to a program, it seems like this is something gitoxide might reasonably be able to do, as an opt-in feature when using gix_command::Prepare. I've opened GitoxideLabs/gitoxide#1799 about this and other aspects of ambiguity in how the command and argument strings are interpreted.
Please note that, at least as proposed, it looks like that would not cover any of the quoting done in this PR, which seems to affect only arguments. But I think existing gix_command functionality could, if desired, neutralize that by using args for everything but the command name.
(If done here, that change would reach all the way up to the let mut cmd_string line. But it should work just as well to neutralize existing quotes when passing quotes the outside of arguments or, if it is certain that no ' will ever appear, to do nothing further.)
|
Thanks for researching this. I'd also think that it would be better not to build command-strings here and instead pass everything as arguments officially. That way everything regarding escaping should be taken care of properly. |
The path to the command is the main part I would be worried about, since it might potentially be in an unusually named directory. (I don't know if Windows will ever automatically name a user profile directory with a |
|
While I am working on this, do you think that it's appropriate to forcefully change an ssh key to have usable permissions? gitbutler/crates/gitbutler-repo/src/repository_ext.rs Lines 444 to 450 in bc482aa |
|
I'm not sure and I think I would need to understand more about the context. 0o600 are the usual permissions for it. I can think of two possible concerns:
But I emphasize I haven't looked into the details of how this is being used, nor as of yet thought through the full implications of either of those scenarios. |
|
Thank you! Then I will leave it as it clearly is a convenience measure. I'd think that if an attacker can create SSH keys there, it would probably also be able to set their permissions accordingly. Probably SSH also checks ownership, which isn't changed here, and which might prevent a 'key-swap' attack of sorts. Edit: here is the adjustment: #6060 |
At least when the key is in the usual location of the user's |
3 participants
Make sure paths are quoted when passing them to the shell that runs the signing command.
We run it in a shell, specifically the Git shell, on Windows so more programs are available.
On Unix it sholdn't hurt even though it doesn't have a specific purpose there.
The quoting is needed on Windows to prevent backslashes to mean escaping.
Fixes #6013 .