{"thread":{"id":"48770","subject":"[BUG] url schemes should be case-insensitive","startedAt":"2018-06-24T08:56:29Z","lastAt":"2018-06-26T20:03:24Z","messageCount":6,"participants":["Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"350792","messageId":"20180624085622.GA28575@sigill.intra.peff.net","threadId":"48770","inReplyTo":null,"subject":"[BUG] url schemes should be case-insensitive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-24T08:56:23Z","receivedAt":"2018-06-24T08:56:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We seem to match url schemes case-sensitively:\n\n $ git clone SSH://example.com/repo.git\n Cloning into 'repo'...\n fatal: Unable to find remote helper for 'SSH'\n\nwhereas rfc3986 is clear that the scheme portion is case-insensitive.\nWe probably ought to match at least our internal ones with strcasecmp.\nPossibly we should also normalize external helpers (so \"FOO://bar\" would\nalways call git-remote-foo, never git-remote-FOO).\n\nWe could probably also give an advise() message in the above output,\nsuggesting that the problem is likely one of:\n\n 1. They misspelled the scheme.\n\n 2. They need to install the appropriate helper.\n\nThis may be a good topic for somebody looking for low-hanging fruit to\nget involved in development (I'd maybe call it a #leftoverbits, but\nsince I didn't start on it, I'm not sure if it counts as \"left over\" ;)).\n\n-Peff\n"},{"id":"350874","messageId":"xmqqvaa6ohiw.fsf@gitster-ct.c.googlers.com","threadId":"48770","inReplyTo":"20180624085622.GA28575@sigill.intra.peff.net","subject":"Re: [BUG] url schemes should be case-insensitive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-25T18:19:51Z","receivedAt":"2018-06-25T18:19:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We seem to match url schemes case-sensitively:\n>\n> $ git clone SSH://example.com/repo.git\n> Cloning into 'repo'...\n> fatal: Unable to find remote helper for 'SSH'\n>\n> whereas rfc3986 is clear that the scheme portion is case-insensitive.\n> We probably ought to match at least our internal ones with strcasecmp.\n\nThat may break if somebody at DevToolGroup@$BIGCOMPANY got cute and\nnamed their custom remote helper SSH:// that builds on top of the\nnormal ssh:// protocol with something extra and gave it to their\ndevelopers (and they named the http counterpart that has the same\nextra HTTP://, of course). If we probe for git-remote-SSH first and\nthen fall back to git-remote-ssh, then we won't break these people,\nthough. I agree that it may be a good bite-sized #leftoverbit\nmaterial.\n\n> Possibly we should also normalize external helpers (so \"FOO://bar\" would\n> always call git-remote-foo, never git-remote-FOO).\n\n> We could probably also give an advise() message in the above output,\n> suggesting that the problem is likely one of:\n>\n> 1. They misspelled the scheme.\n>\n> 2. They need to install the appropriate helper.\n>\n> This may be a good topic for somebody looking for low-hanging fruit to\n> get involved in development (I'd maybe call it a #leftoverbits, but\n> since I didn't start on it, I'm not sure if it counts as \"left over\" ;)).\n\nWell, noticing an issue, analysing and discussing potential\nimprovements and their ramifications is already half the work, so it\ndoes count as left-over, I would say.\n\nIt may probably be a good idea to do an advice, but I'd think\n\"Untable to find remote helper for 'SSH'\" may be clear enough. If\nanything, perhaps saying \"remote helper for 'SSH' protocol\" would\nmake it even clear? I dunno.\n"},{"id":"350965","messageId":"20180626122143.GA14052@sigill.intra.peff.net","threadId":"48770","inReplyTo":"xmqqvaa6ohiw.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] url schemes should be case-insensitive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-26T12:21:43Z","receivedAt":"2018-06-26T12:21:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 25, 2018 at 11:19:51AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > We seem to match url schemes case-sensitively:\n> >\n> > $ git clone SSH://example.com/repo.git\n> > Cloning into 'repo'...\n> > fatal: Unable to find remote helper for 'SSH'\n> >\n> > whereas rfc3986 is clear that the scheme portion is case-insensitive.\n> > We probably ought to match at least our internal ones with strcasecmp.\n> \n> That may break if somebody at DevToolGroup@$BIGCOMPANY got cute and\n> named their custom remote helper SSH:// that builds on top of the\n> normal ssh:// protocol with something extra and gave it to their\n> developers (and they named the http counterpart that has the same\n> extra HTTP://, of course).\n\nTrue, though I am on the fence whether that is a property worth\nmaintaining. AFAIK it was not planned and is just a \"this is how it\nhappened to work\" case that is (IMHO) doing the wrong thing.\n\nc.f. https://xkcd.com/1172/\n\nIs this a hypothetical, or do you know of a $BIGCOMPANY that uses this?\nThe only similar case I know of is contrib/persistent-https, where we\nrealized the better way to do it is:\n\n [url \"my-custom-ssh://\"]\n insteadOf = ssh://\n\n> If we probe for git-remote-SSH first and\n> then fall back to git-remote-ssh, then we won't break these people,\n> though. I agree that it may be a good bite-sized #leftoverbit\n> material.\n\nThis is probably a little tricky to implement, since there is no\ngit-remote-ssh, and we handle it internally in a totally separate\ncode path. So \"probe for git-remote-SSH\" is probably not \"try to run\nit\", but really \"search the PATH for something plausible\". That may be\ngood enough.\n\nIt may also interact in a funny way with our allowed-protocol code, if\n\"SSH\" gets a pass as \"ssh\" under the default config, but actually runs\nthe otherwise-disallowed git-remote-SSH (though one would _hope_ if you\nhave such a git-remote-SSH that it behaves just like an ssh remote).\n\n> > We could probably also give an advise() message in the above output,\n> > suggesting that the problem is likely one of:\n> >\n> > 1. They misspelled the scheme.\n> >\n> > 2. They need to install the appropriate helper.\n> >\n> > This may be a good topic for somebody looking for low-hanging fruit to\n> > get involved in development (I'd maybe call it a #leftoverbits, but\n> > since I didn't start on it, I'm not sure if it counts as \"left over\" ;)).\n> [..]\n> It may probably be a good idea to do an advice, but I'd think\n> \"Untable to find remote helper for 'SSH'\" may be clear enough. If\n> anything, perhaps saying \"remote helper for 'SSH' protocol\" would\n> make it even clear? I dunno.\n\nI think it doesn't help much if the user does not know what a remote\nhelper is, or why Git is looking for one. Though at least it gives a\nterm to start searching for, I guess. The original motivation for my\nmail was the real-world report at:\n\n https://github.com/git/git-scm.com/issues/1219\n\n-Peff\n"},{"id":"350983","messageId":"xmqq1sctmq3d.fsf@gitster-ct.c.googlers.com","threadId":"48770","inReplyTo":"20180626122143.GA14052@sigill.intra.peff.net","subject":"Re: [BUG] url schemes should be case-insensitive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-26T17:09:58Z","receivedAt":"2018-06-26T17:10:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > We seem to match url schemes case-sensitively:\n>> >\n>> > $ git clone SSH://example.com/repo.git\n>> > Cloning into 'repo'...\n>> > fatal: Unable to find remote helper for 'SSH'\n>> >\n>> > whereas rfc3986 is clear that the scheme portion is case-insensitive.\n>> > We probably ought to match at least our internal ones with strcasecmp.\n>> \n>> That may break if somebody at DevToolGroup@$BIGCOMPANY got cute and\n>> named their custom remote helper SSH:// that builds on top of the\n>> normal ssh:// protocol with something extra and gave it to their\n>> developers (and they named the http counterpart that has the same\n>> extra HTTP://, of course).\n>\n> True, though I am on the fence whether that is a property worth\n> maintaining. AFAIK it was not planned and is just a \"this is how it\n> happened to work\" case that is (IMHO) doing the wrong thing.\n\nFWIW, I fully agree with the assessment; sorry for not saying that\ntogether with the devil's advocate comment to save a round-tip.\n\n> It may also interact in a funny way with our allowed-protocol code, if\n> \"SSH\" gets a pass as \"ssh\" under the default config, but actually runs\n> the otherwise-disallowed git-remote-SSH (though one would _hope_ if you\n> have such a git-remote-SSH that it behaves just like an ssh remote).\n\nTrue. I did not offhand recall how protocol whitelist matches the\nprotocol name with config, but transport.c::get_protocol_config()\nseems to say that the part of \"protocol..allow\" is case\nsensitive, and we match known-safe (and known-unsafe \"ext::\")\nprotocols with strcmp() not strcasecmp(). We need to figure out the\nimplications of allowing SSH:// not to error out but pretending as\nif it were ssh:// on those who have protocol.ssh.allow defined.\n\n>> > We could probably also give an advise() message in the above output,\n>> > suggesting that the problem is likely one of:\n>> >\n>> > 1. They misspelled the scheme.\n>> >\n>> > 2. They need to install the appropriate helper.\n>> >\n>> > This may be a good topic for somebody looking for low-hanging fruit to\n>> > get involved in development (I'd maybe call it a #leftoverbits, but\n>> > since I didn't start on it, I'm not sure if it counts as \"left over\" ;)).\n>> [..]\n>> It may probably be a good idea to do an advice, but I'd think\n>> \"Untable to find remote helper for 'SSH'\" may be clear enough. If\n>> anything, perhaps saying \"remote helper for 'SSH' protocol\" would\n>> make it even clear? I dunno.\n>\n> I think it doesn't help much if the user does not know what a remote\n> helper is, or why Git is looking for one.\n\nTrue. \n\n\t$ git clone SSH://example.com/repo.git\n\tfatal: unable to handle URL that begins with SSH://\n\nwould be clear enough, perhaps? At least this line of change is a\nsmall first step that would improve the situation without potential\nto break anybody who has been abusing the case sensitivity loophole.\n\n"},{"id":"350991","messageId":"20180626182739.GA28358@sigill.intra.peff.net","threadId":"48770","inReplyTo":"xmqq1sctmq3d.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] url schemes should be case-insensitive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-26T18:27:40Z","receivedAt":"2018-06-26T18:27:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 26, 2018 at 10:09:58AM -0700, Junio C Hamano wrote:\n\n> > It may also interact in a funny way with our allowed-protocol code, if\n> > \"SSH\" gets a pass as \"ssh\" under the default config, but actually runs\n> > the otherwise-disallowed git-remote-SSH (though one would _hope_ if you\n> > have such a git-remote-SSH that it behaves just like an ssh remote).\n> \n> True. I did not offhand recall how protocol whitelist matches the\n> protocol name with config, but transport.c::get_protocol_config()\n> seems to say that the part of \"protocol..allow\" is case\n> sensitive, and we match known-safe (and known-unsafe \"ext::\")\n> protocols with strcmp() not strcasecmp(). We need to figure out the\n> implications of allowing SSH:// not to error out but pretending as\n> if it were ssh:// on those who have protocol.ssh.allow defined.\n\nThat function is actually a little tricky, because we feed it mostly\nfrom string literals (so we end up in the ssh code path, and then feed\nit \"ssh\"). But I think for remote-helpers we feed it literally from the\nURL we got fed.\n\nSo yeah, we would not want to allow EXT::\"rm -rf /\" to slip past the\nknown-unsafe match. Any normalization should happen before then\n(probably right in transport_helper_init).\n\nCome to think of it, that's already sort-of an issue now. If you have a\ncase-insensitive filesystem, then EXT:: is going to pass this check, but\nstill run git-remote-ext. We're saved there somewhat by the fact that\nthe default is to reject unknown helpers in submodules (otherwise, we'd\nhave that horrible submodule bug all over again).\n\nThat goes beyond just cases, too. On HFS+ I wonder if I could ask for\n\"\\u{0200}ext::\" and run git-remote-ext.\n\n> > I think it doesn't help much if the user does not know what a remote\n> > helper is, or why Git is looking for one.\n> \n> True. \n> \n> \t$ git clone SSH://example.com/repo.git\n> \tfatal: unable to handle URL that begins with SSH://\n> \n> would be clear enough, perhaps? At least this line of change is a\n> small first step that would improve the situation without potential\n> to break anybody who has been abusing the case sensitivity loophole.\n\nYeah, certainly the advice is orthogonal to any behavior changes. The\noriginal report complained of:\n\n $ git clone SSH://...\n fatal: 'remote-SSH' is not a git command. See 'git --help'.\n\nbut since 6b02de3b9d in 2010 we say:\n\n fatal: Unable to find remote helper for 'SSH'\n\nSo I actually wonder if there is something else going on. I find it hard\nto believe the OP is using something older than Git v1.7.0. They did\nappear to be on Windows, though. Is it possible our ENOENT detection\nfrom start_command() is not accurate on Windows?\n\n-Peff\n"},{"id":"351001","messageId":"20180626200319.GA2230@sigill.intra.peff.net","threadId":"48770","inReplyTo":"20180626182739.GA28358@sigill.intra.peff.net","subject":"Re: [BUG] url schemes should be case-insensitive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-26T20:03:19Z","receivedAt":"2018-06-26T20:03:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 26, 2018 at 02:27:39PM -0400, Jeff King wrote:\n\n> So yeah, we would not want to allow EXT::\"rm -rf /\" to slip past the\n> known-unsafe match. Any normalization should happen before then\n> (probably right in transport_helper_init).\n> \n> Come to think of it, that's already sort-of an issue now. If you have a\n> case-insensitive filesystem, then EXT:: is going to pass this check, but\n> still run git-remote-ext. We're saved there somewhat by the fact that\n> the default is to reject unknown helpers in submodules (otherwise, we'd\n> have that horrible submodule bug all over again).\n> \n> That goes beyond just cases, too. On HFS+ I wonder if I could ask for\n> \"\\u{0200}ext::\" and run git-remote-ext.\n\nThat should be \\u{200c}, of course, to get the correct sneaky character.\nAnd the good news is that no, it doesn't work. We are protected by this\ncode in transport_get():\n\n /* maybe it is a foreign URL? */\n if (url) {\n const char *p = url;\n\n while (is_urlschemechar(p == url, *p))\n p++; \n if (starts_with(p, \"::\"))\n helper = xstrndup(url, p - url);\n }\n\nSo we'll only allow remote-helper names with valid url characters, which\nare basically [A-Za-z0-9+.-]. So I think we probably only have to worry\nabout true case issues, and not any kind of weird filesystem-specific\nbehaviors.\n\n-Peff\n"}]}