Hi Junio,
On Sun, 8 Jan 2017, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > I suspect that this will break when GIT_SSH_COMMAND, which is meant
> > to be processed by the shell, hence the user can write anything,
> > begins with a one-shot environment variable assignment, e.g.
> >
> > [core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink
> >
> > One possible unintended side effect of this patch is when VAL1 ends
> > with /plink (or /tortoiseplink) and the command is not either of
> > these, in which case the "tortoiseplink" and "putty" variables will
> > tweak the command line for an SSH implementation that does not want
> > such a tweak to be made. There may be other unintended fallouts.
>
> Thinking about this further, as the sshcommand (or GIT_SSH_COMMAND)
> interface takes any shell-interpretable commands, the first "token"
> you see is not limited to a one-shot environment assignment. It
> could even be "cmd1 && cmd2 && ..." or even:
>
> if test -f ${TPLINK:=//path/to/tortoiseplink}
> then
> exec "$TPLINK" "$@"
> elif test -f ${PLINK:=//path/to/plink}
> exec "$PLINK" "$@"
> else
> echo >&2 no usable ssh on this host
> fi
Sure, it could be all of that. It could even blast through the environment
limits in some environments on some platforms, but not on others. It could
automatically transfer bitcoins whenever a connection to github.com is
attempted. It could start the camera and build a time-lapse of "every time
I pushed or fetched a repository", as an art project. It could shut down
the computer.
It could do all of that.
In practice, however, what users realistically do is to use
GIT_SSH_COMMAND whenever they need to override the ssh command with
options.
This is the common case, and that is what we must make less cumbersome to
use.
If you feel strongly about your contrived examples possibly being affected
by this patch, we could easily make this conditional on
1) no '&&' or '||' being found on the command-line, and
2) argv[0] not containing an '='
Another approach would be to verify that argv[i] starts with '-' for
non-zero i.
But do we really need to do that?
Let's have a very brief look at the amount of work required to come up
with a false positive (I am not so much concerned about any power user
writing elaborate shell expressions that may call plink.exe: those power
users will simply have to continue to use their own -P => -p and -batch
hacks):
The logic kicks in if the split command-line's first component has a
basename `plink` or `tortoiseplink` (possibly with `.exe` suffix). The
only way this logic can kick in by mistake is if the first component is
*not* the command to call *and* if the first component *still* has a
basename of `plink` or `tortoiseplink`.
That means that the user has to specify something like
HAHAHA_IT_IS_NOT=/plink.exe ssh
as GIT_SSH_COMMAND.
Now, let's take a *very* brief look at the question how likely it is to
have a basename of `plink` or `tortoiseplink`. Remember, there are two
ways that the basename can be a specific term: either the original string
is already equal to that term, or it ends in a slash followed by the term.
That is, either the first component refers to plink.exe/tortoiseplink.exe, i.e.
it would be a *true* positive. Or the first component would end in
"/plink" or "/tortoiseplink" (possibly with the `.exe` suffix) *and not be
a command*. But how likely is it to specify a non-command (such as a
per-process variable assignment) that specifically mentions plink or
tortoiseplink?
Is it even realistic to expect users to specify values for GIT_SSH_COMMAND
that contain "plink" as a substring when they do *not* want to call plink
at all?
After thinking a bit longer than I had originally planned about it, my
answer is: no. No, I do not think it is realistic. I fully expect *no*
user to have a GIT_SSH_COMMAND containing even a substring "plink"
*anywhere*, unless they do, in fact, want to call plink or tortoiseplink.
My main aim with Git for Windows is to improve the user experience, and
the patch in question does do that. Of course, I also try to avoid
breaking existing setups while improving the user experience, and
I believe that it is safe to assume that the patch in question in all
likelihood does not break any existing setup.
Ciao,
Dscho