* [BUG] url schemes should be case-insensitive @ 2018-06-24 8:56 Jeff King 2018-06-25 18:19 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2018-06-24 8:56 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] url schemes should be case-insensitive 2018-06-24 8:56 [BUG] url schemes should be case-insensitive Jeff King @ 2018-06-25 18:19 ` Junio C Hamano 2018-06-26 12:21 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2018-06-25 18:19 UTC (permalink / raw) To: Jeff King; +Cc: git 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] url schemes should be case-insensitive 2018-06-25 18:19 ` Junio C Hamano @ 2018-06-26 12:21 ` Jeff King 2018-06-26 17:09 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2018-06-26 12:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] url schemes should be case-insensitive 2018-06-26 12:21 ` Jeff King @ 2018-06-26 17:09 ` Junio C Hamano 2018-06-26 18:27 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2018-06-26 17:09 UTC (permalink / raw) To: Jeff King; +Cc: git 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] url schemes should be case-insensitive 2018-06-26 17:09 ` Junio C Hamano @ 2018-06-26 18:27 ` Jeff King 2018-06-26 20:03 ` Jeff King 0 siblings, 1 reply; 6+ messages in thread From: Jeff King @ 2018-06-26 18:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] url schemes should be case-insensitive 2018-06-26 18:27 ` Jeff King @ 2018-06-26 20:03 ` Jeff King 0 siblings, 0 replies; 6+ messages in thread From: Jeff King @ 2018-06-26 20:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-26 20:03 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-24 8:56 [BUG] url schemes should be case-insensitive Jeff King 2018-06-25 18:19 ` Junio C Hamano 2018-06-26 12:21 ` Jeff King 2018-06-26 17:09 ` Junio C Hamano 2018-06-26 18:27 ` Jeff King 2018-06-26 20:03 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).