Honor disallow_shell in SSH client feature check
#1814
Conversation
When running an SSH client, the `disallow_shell` option determines whether the client command, before arguments, is to be run directly or if it is to be run by a shell. (One example of when it is run directly is if it comes from the `GIT_SSH` environment variable, while one example of when it is run by a shell is if it comes from the `GIT_SSH_COMMAND` environment variable.) When invoking the client in the most central and common case of actually attempting to connect to a remote server, `disallow_shell` was already followed. However, in some cases we are not sure what kind of SSH client program we have, and so to find that out (so we know how to run it to connect to a server), we run a test command, to see if it recognizes `-G` as OpenSSH clients do. Often we can tell what kind of client program we have without needing to do that. But if we do need to do it, we pre-run the client to check. In this use, the `disallow_shell` option was not followed, and instead the use of a shell was unconditionally treated as allowed. This fixes that by setting `prepare.use_shell = false` on a constructed `gix_command::Prepare` instance, which seems to be the prevailing style for achieving this elsewhere in `gix-transport`.
This creates two helper functions and moves the logic for the SSH client feature check (determining the `ProgramKind`) to them. Further refactoring is possible, especially since some of the use of mutability might be replaceable by early return with no loss of readability. But for now this maintains the preexisting structure of the code covering the case where it is not necessary to pre-run the SSH client.
EliahKagan
force-pushed
the
run-ci/ssh-kind
branch
3 times, most recently
from
4c8a77a to
94626fe
Compare
This is with the idea that they're likely to be able to be inlined, since they each have exactly one call site.
EliahKagan
force-pushed
the
run-ci/ssh-kind
branch
from
94626fe to
c0cbc19
Compare
There was a problem hiding this comment.
Thanks so much! This is a huge improvement in overall code-readability as well.
After doing that, I figured that having an issue might help it advance, even if I couldn't immediately make it as complete as I would like. But then I thought, even better than an incomplete issue would be an incomplete pull request. This is that incomplete pull request. The main and possibly only way it is incomplete is that it has no tests. However, if this is judged not to require tests, then maybe this is complete after all.
Even though having tests would always be better, doing so is quite expensive (after all, to truly test it, one has to test it from gix as well as it feeds all configuration), so it's probably more efficient to reserve such tests for cases where a regression would mean a exposing the downstream to security risks.
When running an SSH client, the
disallow_shelloption determines whether the client command, before arguments, is to be run directly or if it is to be run by a shell.(One example of when it is run directly is if it comes from the
GIT_SSHenvironment variable, while one example of when it is run by a shell is if it comes from theGIT_SSH_COMMANDenvironment variable.)When invoking the client in the most central and common case of actually attempting to connect to a remote server,
disallow_shellwas already followed. However, in some cases we are not sure what kind of SSH client program we have, and so to find that out (so we know how to run it to connect to a server), we run a test command, to see if it recognizes-Gas OpenSSH clients do. Often we can tell what kind of client program we have without needing to do that. But if we do need to do it, we pre-run the client to check. In this use, thedisallow_shelloption was not followed, and instead the use of a shell was unconditionally treated as allowed.This fixes that by setting
prepare.use_shell = falseon a constructedgix_command::Prepareinstance, which seems to be the prevailing style for achieving this elsewhere ingix-transport.The fix itself is in a445b72, but by itself this makes the
connectfunction larger and more complicated, so 9255ccc and 9599895 refactor to address that.I posted #1800 (comment) as a comment in a related (but not extremely tightly related) review thread, where it came up. I posted it there rather than as an issue, on the grounds that I was not done investigating it and could not, at the moment, confidently supply instructions to demonstrate the bug or a test case that would fail due to it.
After doing that, I figured that having an issue might help it advance, even if I couldn't immediately make it as complete as I would like. But then I thought, even better than an incomplete issue would be an incomplete pull request. This is that incomplete pull request. The main and possibly only way it is incomplete is that it has no tests. However, if this is judged not to require tests, then maybe this is complete after all.
While the goal of the refactoring in 9255ccc was just to avoid making the code any harder to read and understand, it might lead to making it easier to understand, since being contained in a function allows the logic to be refactored in more ways. In #1800 (comment), I had said:
That might become clearer with further refactoring, which should now be easier to do. However, I haven't included that in this pull request. Instead doing only as much refactoring as seemed necessary to me to avoid making the code harder to read. (Also, perhaps you just know exactly what's going on there.)
Please note that, in addition to being possibly incomplete, this pull request is narrowly scoped. Its goal is only to fix the place in
gix-transportwhere thedisallow_shelloption is not honored. The matter of how backslashes are common and seem to get in the way of avoiding using shells is not addressed here: this does not modifygix-command.