[BUG] url schemes should be case-insensitive

[BUG] url schemes should be case-insensitive

[Jeff King]

We seem to match url schemes case-sensitively:

  $ git clone SSH://example.com/repo.git
  Cloning into 'repo'...
  fatal: Unable to find remote helper for 'SSH'

whereas rfc3986 is clear that the scheme portion is case-insensitive.
We probably ought to match at least our internal ones with strcasecmp.
Possibly we should also normalize external helpers (so "FOO://bar" would
always call git-remote-foo, never git-remote-FOO).

We could probably also give an advise() message in the above output,
suggesting that the problem is likely one of:

  1. They misspelled the scheme.

  2. They need to install the appropriate helper.

This may be a good topic for somebody looking for low-hanging fruit to
get involved in development (I'd maybe call it a #leftoverbits, but
since I didn't start on it, I'm not sure if it counts as "left over" ;)).

-Peff

Re: [BUG] url schemes should be case-insensitive

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We seem to match url schemes case-sensitively:
>
>   $ git clone SSH://example.com/repo.git
>   Cloning into 'repo'...
>   fatal: Unable to find remote helper for 'SSH'
>
> whereas rfc3986 is clear that the scheme portion is case-insensitive.
> We probably ought to match at least our internal ones with strcasecmp.

That may break if somebody at DevToolGroup@$BIGCOMPANY got cute and
named their custom remote helper SSH:// that builds on top of the
normal ssh:// protocol with something extra and gave it to their
developers (and they named the http counterpart that has the same
extra HTTP://, of course).  If we probe for git-remote-SSH first and
then fall back to git-remote-ssh, then we won't break these people,
though.  I agree that it may be a good bite-sized #leftoverbit
material.

> Possibly we should also normalize external helpers (so "FOO://bar" would
> always call git-remote-foo, never git-remote-FOO).

> We could probably also give an advise() message in the above output,
> suggesting that the problem is likely one of:
>
>   1. They misspelled the scheme.
>
>   2. They need to install the appropriate helper.
>
> This may be a good topic for somebody looking for low-hanging fruit to
> get involved in development (I'd maybe call it a #leftoverbits, but
> since I didn't start on it, I'm not sure if it counts as "left over" ;)).

Well, noticing an issue, analysing and discussing potential
improvements and their ramifications is already half the work, so it
does count as left-over, I would say.

It may probably be a good idea to do an advice, but I'd think
"Untable to find remote helper for 'SSH'" may be clear enough.  If
anything, perhaps saying "remote helper for 'SSH' protocol" would
make it even clear?  I dunno.

Re: [BUG] url schemes should be case-insensitive

[Jeff King]

On Mon, Jun 25, 2018 at 11:19:51AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > We seem to match url schemes case-sensitively:
> >
> >   $ git clone SSH://example.com/repo.git
> >   Cloning into 'repo'...
> >   fatal: Unable to find remote helper for 'SSH'
> >
> > whereas rfc3986 is clear that the scheme portion is case-insensitive.
> > We probably ought to match at least our internal ones with strcasecmp.
> 
> That may break if somebody at DevToolGroup@$BIGCOMPANY got cute and
> named their custom remote helper SSH:// that builds on top of the
> normal ssh:// protocol with something extra and gave it to their
> developers (and they named the http counterpart that has the same
> extra HTTP://, of course).

True, though I am on the fence whether that is a property worth
maintaining. AFAIK it was not planned and is just a "this is how it
happened to work" case that is (IMHO) doing the wrong thing.

c.f. https://xkcd.com/1172/

Is this a hypothetical, or do you know of a $BIGCOMPANY that uses this?
The only similar case I know of is contrib/persistent-https, where we
realized the better way to do it is:

  [url "my-custom-ssh://"]
  insteadOf = ssh://

> If we probe for git-remote-SSH first and
> then fall back to git-remote-ssh, then we won't break these people,
> though.  I agree that it may be a good bite-sized #leftoverbit
> material.

This is probably a little tricky to implement, since there is no
git-remote-ssh, and we handle it internally in a totally separate
code path. So "probe for git-remote-SSH" is probably not "try to run
it", but really "search the PATH for something plausible". That may be
good enough.

It may also interact in a funny way with our allowed-protocol code, if
"SSH" gets a pass as "ssh" under the default config, but actually runs
the otherwise-disallowed git-remote-SSH (though one would _hope_ if you
have such a git-remote-SSH that it behaves just like an ssh remote).

> > We could probably also give an advise() message in the above output,
> > suggesting that the problem is likely one of:
> >
> >   1. They misspelled the scheme.
> >
> >   2. They need to install the appropriate helper.
> >
> > This may be a good topic for somebody looking for low-hanging fruit to
> > get involved in development (I'd maybe call it a #leftoverbits, but
> > since I didn't start on it, I'm not sure if it counts as "left over" ;)).
> [..]
> It may probably be a good idea to do an advice, but I'd think
> "Untable to find remote helper for 'SSH'" may be clear enough.  If
> anything, perhaps saying "remote helper for 'SSH' protocol" would
> make it even clear?  I dunno.

I think it doesn't help much if the user does not know what a remote
helper is, or why Git is looking for one. Though at least it gives a
term to start searching for, I guess. The original motivation for my
mail was the real-world report at:

  https://github.com/git/git-scm.com/issues/1219

-Peff

Re: [BUG] url schemes should be case-insensitive

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > We seem to match url schemes case-sensitively:
>> >
>> >   $ git clone SSH://example.com/repo.git
>> >   Cloning into 'repo'...
>> >   fatal: Unable to find remote helper for 'SSH'
>> >
>> > whereas rfc3986 is clear that the scheme portion is case-insensitive.
>> > We probably ought to match at least our internal ones with strcasecmp.
>> 
>> That may break if somebody at DevToolGroup@$BIGCOMPANY got cute and
>> named their custom remote helper SSH:// that builds on top of the
>> normal ssh:// protocol with something extra and gave it to their
>> developers (and they named the http counterpart that has the same
>> extra HTTP://, of course).
>
> True, though I am on the fence whether that is a property worth
> maintaining. AFAIK it was not planned and is just a "this is how it
> happened to work" case that is (IMHO) doing the wrong thing.

FWIW, I fully agree with the assessment; sorry for not saying that
together with the devil's advocate comment to save a round-tip.

> It may also interact in a funny way with our allowed-protocol code, if
> "SSH" gets a pass as "ssh" under the default config, but actually runs
> the otherwise-disallowed git-remote-SSH (though one would _hope_ if you
> have such a git-remote-SSH that it behaves just like an ssh remote).

True.  I did not offhand recall how protocol whitelist matches the
protocol name with config, but transport.c::get_protocol_config()
seems to say that the <name> part of "protocol.<name>.allow" is case
sensitive, and we match known-safe (and known-unsafe "ext::")
protocols with strcmp() not strcasecmp().  We need to figure out the
implications of allowing SSH:// not to error out but pretending as
if it were ssh:// on those who have protocol.ssh.allow defined.

>> > We could probably also give an advise() message in the above output,
>> > suggesting that the problem is likely one of:
>> >
>> >   1. They misspelled the scheme.
>> >
>> >   2. They need to install the appropriate helper.
>> >
>> > This may be a good topic for somebody looking for low-hanging fruit to
>> > get involved in development (I'd maybe call it a #leftoverbits, but
>> > since I didn't start on it, I'm not sure if it counts as "left over" ;)).
>> [..]
>> It may probably be a good idea to do an advice, but I'd think
>> "Untable to find remote helper for 'SSH'" may be clear enough.  If
>> anything, perhaps saying "remote helper for 'SSH' protocol" would
>> make it even clear?  I dunno.
>
> I think it doesn't help much if the user does not know what a remote
> helper is, or why Git is looking for one.

True.  

	$ git clone SSH://example.com/repo.git
	fatal: unable to handle URL that begins with SSH://

would be clear enough, perhaps?  At least this line of change is a
small first step that would improve the situation without potential
to break anybody who has been abusing the case sensitivity loophole.

Re: [BUG] url schemes should be case-insensitive

[Jeff King]

On Tue, Jun 26, 2018 at 10:09:58AM -0700, Junio C Hamano wrote:

> > It may also interact in a funny way with our allowed-protocol code, if
> > "SSH" gets a pass as "ssh" under the default config, but actually runs
> > the otherwise-disallowed git-remote-SSH (though one would _hope_ if you
> > have such a git-remote-SSH that it behaves just like an ssh remote).
> 
> True.  I did not offhand recall how protocol whitelist matches the
> protocol name with config, but transport.c::get_protocol_config()
> seems to say that the <name> part of "protocol.<name>.allow" is case
> sensitive, and we match known-safe (and known-unsafe "ext::")
> protocols with strcmp() not strcasecmp().  We need to figure out the
> implications of allowing SSH:// not to error out but pretending as
> if it were ssh:// on those who have protocol.ssh.allow defined.

That function is actually a little tricky, because we feed it mostly
from string literals (so we end up in the ssh code path, and then feed
it "ssh"). But I think for remote-helpers we feed it literally from the
URL we got fed.

So yeah, we would not want to allow EXT::"rm -rf /" to slip past the
known-unsafe match. Any normalization should happen before then
(probably right in transport_helper_init).

Come to think of it, that's already sort-of an issue now. If you have a
case-insensitive filesystem, then EXT:: is going to pass this check, but
still run git-remote-ext. We're saved there somewhat by the fact that
the default is to reject unknown helpers in submodules (otherwise, we'd
have that horrible submodule bug all over again).

That goes beyond just cases, too. On HFS+ I wonder if I could ask for
"\u{0200}ext::" and run git-remote-ext.

> > I think it doesn't help much if the user does not know what a remote
> > helper is, or why Git is looking for one.
> 
> True.  
> 
> 	$ git clone SSH://example.com/repo.git
> 	fatal: unable to handle URL that begins with SSH://
> 
> would be clear enough, perhaps?  At least this line of change is a
> small first step that would improve the situation without potential
> to break anybody who has been abusing the case sensitivity loophole.

Yeah, certainly the advice is orthogonal to any behavior changes. The
original report complained of:

  $ git clone SSH://...
  fatal: 'remote-SSH' is not a git command. See 'git --help'.

but since 6b02de3b9d in 2010 we say:

  fatal: Unable to find remote helper for 'SSH'

So I actually wonder if there is something else going on. I find it hard
to believe the OP is using something older than Git v1.7.0. They did
appear to be on Windows, though. Is it possible our ENOENT detection
from start_command() is not accurate on Windows?

-Peff

Re: [BUG] url schemes should be case-insensitive

[Jeff King]

On Tue, Jun 26, 2018 at 02:27:39PM -0400, Jeff King wrote:

> So yeah, we would not want to allow EXT::"rm -rf /" to slip past the
> known-unsafe match. Any normalization should happen before then
> (probably right in transport_helper_init).
> 
> Come to think of it, that's already sort-of an issue now. If you have a
> case-insensitive filesystem, then EXT:: is going to pass this check, but
> still run git-remote-ext. We're saved there somewhat by the fact that
> the default is to reject unknown helpers in submodules (otherwise, we'd
> have that horrible submodule bug all over again).
> 
> That goes beyond just cases, too. On HFS+ I wonder if I could ask for
> "\u{0200}ext::" and run git-remote-ext.

That should be \u{200c}, of course, to get the correct sneaky character.
And the good news is that no, it doesn't work. We are protected by this
code in transport_get():

          /* maybe it is a foreign URL? */
          if (url) {
                  const char *p = url;

                  while (is_urlschemechar(p == url, *p))
                          p++;     
                  if (starts_with(p, "::"))
                          helper = xstrndup(url, p - url);
          }

So we'll only allow remote-helper names with valid url characters, which
are basically [A-Za-z0-9+.-]. So I think we probably only have to worry
about true case issues, and not any kind of weird filesystem-specific
behaviors.

-Peff