{"thread":{"id":"53281","subject":"Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version","startedAt":"2020-04-21T22:33:50Z","lastAt":"2020-04-27T18:44:13Z","messageCount":20,"participants":["Ilya Tretyakov","Jeff King","brian m. carlson","Jonathan Nieder","Johannes Schindelin","Carlo Marcelo Arenas Belón","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"395869","messageId":"CAOrRacVviJP3w98-=QpFKYp630cN3gZQYnvAWZXeKqZRk2UDXg@mail.gmail.com","threadId":"53281","inReplyTo":null,"subject":"Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version","fromName":"Ilya Tretyakov","fromEmail":"it@it3xl.ru","sentAt":"2020-04-21T22:31:46Z","receivedAt":"2020-04-21T22:33:50Z","isPatch":false,"sender":{"key":"it@it3xl.ru","avatar":null},"body":"Credential helpers are no longer invoked in case of having sub-folder\nparts in a repository URL.\n\nFor example, if we have a \"/my-proj/\" part in the repository URL.\nThe following configuration doesn't invoke a credential helper script\nin 2.26.1 version of Git but invokes in 2.24.1.2.\n\n[credential \"https://git.exaple.com/my-proj/my-repo.git\"]\n    helper = !'/c/some-path/bash-git-credential-helper/git-cred.sh'\nprovide  repo_b\n\n\nA workaround here is to cut the URL to a root view.\nFor example, we can use https://git.exaple.com/\ninstead of https://git.exaple.com/my-proj/ or\nhttps://git.exaple.com/my-proj/my-repo.git\nas below.\n\n[credential \"https://git.exaple.com/\"]\n    helper = !'/c/some-path/bash-git-credential-helper/git-cred.sh'\nprovide  repo_b\n\nBTW, the following configuration still works in all Git versions.\n\n[credential \"https://foo.bar/my-repo.git\"]\n    helper = !'/c/some-path/bash-git-credential-helper/git-cred.sh'\nprovide  repo_a\n\nIf this would be possible, please let me know if this could be fixed\nin future Git versions.\nOr that I have to adopt my tools, described below, to this new\nconfiguration limitation.\n\nJFYI,\nI have a tool to synchronize remote Git repositories. This is when two\nremote repositories on different servers behave as a single\nrepository.\n - https://github.com/it3xl/git-repo-sync\nI often use this tool with my own bash credential helper on different\nautomation servers.\n - https://github.com/it3xl/bash-git-credential-helper\nAnd now, my Git-credential helper isn't invoked for repositories that\nhave additional parts in their URLs.\n\nOf course, I've found a workaround but some people may want to\nconfigure more specific credentials for specific URL-s on the same\ndomain.\n\nI am aware of the latest vulnerability discovered in the\nGit-credential helper functionality.\nI hope this was the cause of the trouble I described here. ))\n\nKind Regards,\nIlya\nit3xl.ru\n"},{"id":"395870","messageId":"20200421225837.GB3515235@coredump.intra.peff.net","threadId":"53281","inReplyTo":"CAOrRacVviJP3w98-=QpFKYp630cN3gZQYnvAWZXeKqZRk2UDXg@mail.gmail.com","subject":"Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-21T22:58:37Z","receivedAt":"2020-04-21T22:58:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2020 at 01:31:46AM +0300, Ilya Tretyakov wrote:\n\n> Credential helpers are no longer invoked in case of having sub-folder\n> parts in a repository URL.\n> \n> For example, if we have a \"/my-proj/\" part in the repository URL.\n> The following configuration doesn't invoke a credential helper script\n> in 2.26.1 version of Git but invokes in 2.24.1.2.\n> \n> [credential \"https://git.exaple.com/my-proj/my-repo.git\"]\n>     helper = !'/c/some-path/bash-git-credential-helper/git-cred.sh'\n> provide  repo_b\n\nThis is unrelated to the recent helper fixes in v2.26.x. Here's a simple\nreproduction:\n\n  url=https://git.example.com/my-proj/my-repo.git\n  echo url=$url |\n  GIT_TERMINAL_PROMPT=0 \\\n  ./git \\\n    -c credential.helper= \\\n    -c credential.$url.helper='!echo username=foo; echo password=bar;:' \\\n    credential fill\n\nwhich should print a filled credential (with \"foo/bar\"), but will fail\nwith recent versions. It bisects to brian's 46fd7b3900 (credential:\nallow wildcard patterns when matching config, 2020-02-20).\n\n-Peff\n"},{"id":"395874","messageId":"20200422010943.GD6465@camp.crustytoothpaste.net","threadId":"53281","inReplyTo":"20200421225837.GB3515235@coredump.intra.peff.net","subject":"Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-04-22T01:09:43Z","receivedAt":"2020-04-22T01:10:21Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-04-21 at 22:58:37, Jeff King wrote:\n> On Wed, Apr 22, 2020 at 01:31:46AM +0300, Ilya Tretyakov wrote:\n> \n> > Credential helpers are no longer invoked in case of having sub-folder\n> > parts in a repository URL.\n> > \n> > For example, if we have a \"/my-proj/\" part in the repository URL.\n> > The following configuration doesn't invoke a credential helper script\n> > in 2.26.1 version of Git but invokes in 2.24.1.2.\n> > \n> > [credential \"https://git.exaple.com/my-proj/my-repo.git\"]\n> >     helper = !'/c/some-path/bash-git-credential-helper/git-cred.sh'\n> > provide  repo_b\n> \n> This is unrelated to the recent helper fixes in v2.26.x. Here's a simple\n> reproduction:\n> \n>   url=https://git.example.com/my-proj/my-repo.git\n>   echo url=$url |\n>   GIT_TERMINAL_PROMPT=0 \\\n>   ./git \\\n>     -c credential.helper= \\\n>     -c credential.$url.helper='!echo username=foo; echo password=bar;:' \\\n>     credential fill\n> \n> which should print a filled credential (with \"foo/bar\"), but will fail\n> with recent versions. It bisects to brian's 46fd7b3900 (credential:\n> allow wildcard patterns when matching config, 2020-02-20).\n\nYeah, I can reproduce this.  It looks like what's happening is that\nwe're percent-encoding the slash in the paths as %2f, which of course\nisn't going to match in the urlmatch code.  We probably need to tell the\npercent encoding function not to encode slashes in this case.\n\nI'm testing a patch now and hope to have it on the list a little later\nthis evening.  Thanks for reporting and bisecting, and sorry for the\nbreakage.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"395875","messageId":"20200422012344.2051103-1-sandals@crustytoothpaste.net","threadId":"53281","inReplyTo":"20200421225837.GB3515235@coredump.intra.peff.net","subject":"[PATCH] credential: fix matching URLs with multiple levels in path","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-04-22T01:23:44Z","receivedAt":"2020-04-22T01:24:29Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"46fd7b3900 (\"credential: allow wildcard patterns when matching config\",\n2020-02-20) introduced support for matching credential helpers using\nurlmatch.  In doing so, it introduced code to percent-encode the paths\nwe get from the credential helper so that they could be effectively\nmatched by the urlmatch code.\n\nUnfortunately, that code had a bug: it percent-encoded the slashes in\nthe path, resulting in any URL path that contained multiple levels\n(i.e., a directory component) not matching.\n\nWe are currently the only caller of the percent-encoding code and could\nsimply change it not to encode slashes.  However, this would be\nsurprising to other potential users who might want to use it and might\nresult in unwanted slashes appearing in the encoded value.\n\nSo instead, let's add a flag to skip encoding slashes, which is the\nbehavior we want here, and use it when calling the code in this case.\nAdd a test for credential helper URLs using multiple slashes in the\npath, which our test suite previously lacked.\n\nReported-by: Ilya Tretyakov <it@it3xl.ru>\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n credential.c           |  4 ++--\n strbuf.c               |  8 +++++---\n strbuf.h               |  6 +++++-\n t/t0300-credentials.sh | 18 ++++++++++++++++++\n 4 files changed, 30 insertions(+), 6 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 108d9e183a..f0e55a27ac 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -136,14 +136,14 @@ static void credential_format(struct credential *c, struct strbuf *out)\n \t\treturn;\n \tstrbuf_addf(out, \"%s://\", c->protocol);\n \tif (c->username && *c->username) {\n-\t\tstrbuf_add_percentencode(out, c->username);\n+\t\tstrbuf_add_percentencode(out, c->username, STRBUF_PERCENTENCODE_PATH);\n \t\tstrbuf_addch(out, '@');\n \t}\n \tif (c->host)\n \t\tstrbuf_addstr(out, c->host);\n \tif (c->path) {\n \t\tstrbuf_addch(out, '/');\n-\t\tstrbuf_add_percentencode(out, c->path);\n+\t\tstrbuf_add_percentencode(out, c->path, STRBUF_PERCENTENCODE_PATH);\n \t}\n }\n \ndiff --git a/strbuf.c b/strbuf.c\nindex bb0065ccaf..da5a1c9ca4 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -479,15 +479,17 @@ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)\n \t}\n }\n \n-#define URL_UNSAFE_CHARS \" <>\\\"%{}|\\\\^`:/?#[]@!$&'()*+,;=\"\n+#define URL_UNSAFE_CHARS \" <>\\\"%{}|\\\\^`:?#[]@!$&'()*+,;=\"\n \n-void strbuf_add_percentencode(struct strbuf *dst, const char *src)\n+void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags)\n {\n \tsize_t i, len = strlen(src);\n \n \tfor (i = 0; i < len; i++) {\n \t\tunsigned char ch = src[i];\n-\t\tif (ch <= 0x1F || ch >= 0x7F || strchr(URL_UNSAFE_CHARS, ch))\n+\t\tif (ch <= 0x1F || ch >= 0x7F ||\n+\t\t    (ch == '/' && !(flags & STRBUF_PERCENTENCODE_PATH)) ||\n+\t\t    strchr(URL_UNSAFE_CHARS, ch))\n \t\t\tstrbuf_addf(dst, \"%%%02X\", (unsigned char)ch);\n \t\telse\n \t\t\tstrbuf_addch(dst, ch);\ndiff --git a/strbuf.h b/strbuf.h\nindex ce8e49c0b2..651aedff57 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -378,11 +378,15 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb,\n  */\n void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n \n+#define STRBUF_PERCENTENCODE_PATH 1\n+\n /**\n  * Append the contents of a string to a strbuf, percent-encoding any characters\n  * that are needed to be encoded for a URL.\n+ *\n+ * If STRBUF_PERCENTENCODE_PATH is set in flags, don't percent-encode slashes.\n  */\n-void strbuf_add_percentencode(struct strbuf *dst, const char *src);\n+void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags);\n \n /**\n  * Append the given byte size as a human-readable string (i.e. 12.23 KiB,\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 5555a1524f..15eeef1dfd 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -510,6 +510,24 @@ test_expect_success 'helpers can abort the process' '\n \ttest_i18ncmp expect stderr\n '\n \n+test_expect_success 'helpers can fetch with multiple path components' '\n+\ttest_unconfig credential.helper &&\n+\ttest_config credential.https://example.com/foo/repo.git.helper \"verbatim foo bar\" &&\n+\techo url=https://example.com/foo/repo.git | git credential fill &&\n+\tcheck fill <<-\\EOF\n+\turl=https://example.com/foo/repo.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\t--\n+\tverbatim: get\n+\tverbatim: protocol=https\n+\tverbatim: host=example.com\n+\tEOF\n+'\n+\n test_expect_success 'empty helper spec resets helper list' '\n \ttest_config credential.helper \"verbatim file file\" &&\n \tcheck fill \"\" \"verbatim cmdline cmdline\" <<-\\EOF\n"},{"id":"395876","messageId":"20200422012817.GD103469@google.com","threadId":"53281","inReplyTo":"20200422010943.GD6465@camp.crustytoothpaste.net","subject":"Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-22T01:28:17Z","receivedAt":"2020-04-22T01:28:21Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"brian m. carlson wrote:\n> On 2020-04-21 at 22:58:37, Jeff King wrote:\n\n>> This is unrelated to the recent helper fixes in v2.26.x. Here's a simple\n>> reproduction:\n>>\n>>   url=https://git.example.com/my-proj/my-repo.git\n>>   echo url=$url |\n>>   GIT_TERMINAL_PROMPT=0 \\\n>>   ./git \\\n>>     -c credential.helper= \\\n>>     -c credential.$url.helper='!echo username=foo; echo password=bar;:' \\\n>>     credential fill\n>> \n>> which should print a filled credential (with \"foo/bar\"), but will fail\n>> with recent versions. It bisects to brian's 46fd7b3900 (credential:\n>> allow wildcard patterns when matching config, 2020-02-20).\n>\n> Yeah, I can reproduce this.  It looks like what's happening is that\n> we're percent-encoding the slash in the paths as %2f, which of course\n> isn't going to match in the urlmatch code.  We probably need to tell the\n> percent encoding function not to encode slashes in this case.\n>\n> I'm testing a patch now and hope to have it on the list a little later\n> this evening.  Thanks for reporting and bisecting, and sorry for the\n> breakage.\n\nThanks.  Here's another (though I haven't tried bisecting yet):\n\n\techo url='https://github.com/git/git' |\n\tGIT_TERMINAL_PROMPT=0 \\\n\tgit -c credential.helper= \\\n\t\t-c credential.github.com.helper='!echo username=foo; echo password=bar;:' \\\n\t\tcredential fill\n\nproduces\n\n\tfatal: could not read Username for 'https://github.com': terminal prompts disabled\n\nJonathan\n"},{"id":"395878","messageId":"20200422013629.GA3559880@coredump.intra.peff.net","threadId":"53281","inReplyTo":"20200422012817.GD103469@google.com","subject":"Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-22T01:36:29Z","receivedAt":"2020-04-22T01:36:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 21, 2020 at 06:28:17PM -0700, Jonathan Nieder wrote:\n\n> Thanks.  Here's another (though I haven't tried bisecting yet):\n> \n> \techo url='https://github.com/git/git' |\n> \tGIT_TERMINAL_PROMPT=0 \\\n> \tgit -c credential.helper= \\\n> \t\t-c credential.github.com.helper='!echo username=foo; echo password=bar;:' \\\n> \t\tcredential fill\n> \n> produces\n> \n> \tfatal: could not read Username for 'https://github.com': terminal prompts disabled\n\nIt's almost certainly the same commit. The credential_match() function\nwould have said \"aha, there is no protocol in the pattern, so match any\nprotocol\". But now we are trying to match full URLs.\n\nTBH, I'm not sure if the original was actually sane (especially in light\nof all of the recent confusion around missing protocols).\n\n-Peff\n"},{"id":"395879","messageId":"20200422022020.GF6465@camp.crustytoothpaste.net","threadId":"53281","inReplyTo":"20200422012817.GD103469@google.com","subject":"Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-04-22T02:20:20Z","receivedAt":"2020-04-22T02:20:57Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-04-22 at 01:28:17, Jonathan Nieder wrote:\n> brian m. carlson wrote:\n> > On 2020-04-21 at 22:58:37, Jeff King wrote:\n> \n> >> This is unrelated to the recent helper fixes in v2.26.x. Here's a simple\n> >> reproduction:\n> >>\n> >>   url=https://git.example.com/my-proj/my-repo.git\n> >>   echo url=$url |\n> >>   GIT_TERMINAL_PROMPT=0 \\\n> >>   ./git \\\n> >>     -c credential.helper= \\\n> >>     -c credential.$url.helper='!echo username=foo; echo password=bar;:' \\\n> >>     credential fill\n> >> \n> >> which should print a filled credential (with \"foo/bar\"), but will fail\n> >> with recent versions. It bisects to brian's 46fd7b3900 (credential:\n> >> allow wildcard patterns when matching config, 2020-02-20).\n> >\n> > Yeah, I can reproduce this.  It looks like what's happening is that\n> > we're percent-encoding the slash in the paths as %2f, which of course\n> > isn't going to match in the urlmatch code.  We probably need to tell the\n> > percent encoding function not to encode slashes in this case.\n> >\n> > I'm testing a patch now and hope to have it on the list a little later\n> > this evening.  Thanks for reporting and bisecting, and sorry for the\n> > breakage.\n> \n> Thanks.  Here's another (though I haven't tried bisecting yet):\n> \n> \techo url='https://github.com/git/git' |\n> \tGIT_TERMINAL_PROMPT=0 \\\n> \tgit -c credential.helper= \\\n> \t\t-c credential.github.com.helper='!echo username=foo; echo password=bar;:' \\\n> \t\tcredential fill\n\ngitcredentials(7) says the following:\n\n  Git considers each credential to have a context defined by a URL.\n  This context is used to look up context-specific configuration, and is\n  passed to any helpers, which may use it as an index into secure\n  storage.\n\nI'm not sure a hostname qualifies as a URL in this case.  So while my\npatch did break this, I don't believe it's ever been documented to\nactually work and was an artifact of our implementation (along with\n\"credential./git/git.helper\" and \"credential.https://.helper\").  I've\nalso never seen this syntax used in the wild, but maybe I'm not looking\nin the right places.\n\nI don't think we can shoehorn it into urlmatch, since that would break\ncompatibility with the `http.*` config options, so I think we'd have to\nrevert the entire feature if we want to preserve it.  I think I'd prefer\nto leave things as it is since it seems uncommon and there are easy\nalternatives, but if folks prefer, I can send a patch to revert the\nurlmatch feature.\n\nI will likely not be online tomorrow (Wednesday), so if folks decide\nthey want a revert, I can send that out Thursday afternoon (GMT-05:00).\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"395882","messageId":"20200422040644.GC3559880@coredump.intra.peff.net","threadId":"53281","inReplyTo":"20200422022020.GF6465@camp.crustytoothpaste.net","subject":"Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-22T04:06:44Z","receivedAt":"2020-04-22T04:06:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2020 at 02:20:20AM +0000, brian m. carlson wrote:\n\n> > Thanks.  Here's another (though I haven't tried bisecting yet):\n> > \n> > \techo url='https://github.com/git/git' |\n> > \tGIT_TERMINAL_PROMPT=0 \\\n> > \tgit -c credential.helper= \\\n> > \t\t-c credential.github.com.helper='!echo username=foo; echo password=bar;:' \\\n> > \t\tcredential fill\n> \n> gitcredentials(7) says the following:\n> \n>   Git considers each credential to have a context defined by a URL.\n>   This context is used to look up context-specific configuration, and is\n>   passed to any helpers, which may use it as an index into secure\n>   storage.\n> \n> I'm not sure a hostname qualifies as a URL in this case.  So while my\n> patch did break this, I don't believe it's ever been documented to\n> actually work and was an artifact of our implementation (along with\n> \"credential./git/git.helper\" and \"credential.https://.helper\").  I've\n> also never seen this syntax used in the wild, but maybe I'm not looking\n> in the right places.\n\nI'm pretty sure it was an intended use case, though it is a natural\noutcome of the credential_match() strategy of \"unspecified things match\nanything\". I'd suspect that anybody relying on it is doing so\nunintentionally, and just forgot to put the protocol field in. Though I\nsuppose doing so would let you cover http/https in a single block.\n\nAt any rate, even in versions _without_ your patch, that became a hard\nerror in this week's release. In v2.24.3, for example:\n\n  $ echo url=https://anyhost.example.com |\n    git -c credential.example.com.username=foo credential fill\n  warning: url has no scheme: example.com\n  fatal: credential url cannot be parsed: example.com\n\nbecause we're relying there on credential_from_url() to parse the config\ncredentials, too. After your patch, we use the http-config machinery,\nwhich simply doesn't match.\n\n> I don't think we can shoehorn it into urlmatch, since that would break\n> compatibility with the `http.*` config options, so I think we'd have to\n> revert the entire feature if we want to preserve it.  I think I'd prefer\n> to leave things as it is since it seems uncommon and there are easy\n> alternatives, but if folks prefer, I can send a patch to revert the\n> urlmatch feature.\n\nI agree that we should leave it. Aside from the dual http/https thing\n(which _hopefully_ is rare these days as https become more of a\nstandard), I don't think it has a legitimate use case. And I think we\nshould be pushing users to be a bit more careful with their url config.\n\n-Peff\n"},{"id":"395883","messageId":"20200422041614.GD3559880@coredump.intra.peff.net","threadId":"53281","inReplyTo":"20200422012344.2051103-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] credential: fix matching URLs with multiple levels in path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-22T04:16:14Z","receivedAt":"2020-04-22T04:16:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2020 at 01:23:44AM +0000, brian m. carlson wrote:\n\n> 46fd7b3900 (\"credential: allow wildcard patterns when matching config\",\n> 2020-02-20) introduced support for matching credential helpers using\n> urlmatch.  In doing so, it introduced code to percent-encode the paths\n> we get from the credential helper so that they could be effectively\n> matched by the urlmatch code.\n> \n> Unfortunately, that code had a bug: it percent-encoded the slashes in\n> the path, resulting in any URL path that contained multiple levels\n> (i.e., a directory component) not matching.\n> \n> We are currently the only caller of the percent-encoding code and could\n> simply change it not to encode slashes.  However, this would be\n> surprising to other potential users who might want to use it and might\n> result in unwanted slashes appearing in the encoded value.\n> \n> So instead, let's add a flag to skip encoding slashes, which is the\n> behavior we want here, and use it when calling the code in this case.\n> Add a test for credential helper URLs using multiple slashes in the\n> path, which our test suite previously lacked.\n\nThanks for the quick turnaround. The explanation makes sense.\n\nThe patch leaves me with one question, though...\n\n> diff --git a/credential.c b/credential.c\n> index 108d9e183a..f0e55a27ac 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -136,14 +136,14 @@ static void credential_format(struct credential *c, struct strbuf *out)\n>  \t\treturn;\n>  \tstrbuf_addf(out, \"%s://\", c->protocol);\n>  \tif (c->username && *c->username) {\n> -\t\tstrbuf_add_percentencode(out, c->username);\n> +\t\tstrbuf_add_percentencode(out, c->username, STRBUF_PERCENTENCODE_PATH);\n>  \t\tstrbuf_addch(out, '@');\n\nWouldn't we want to keep encoding slashes in the username?\n\n>  \tif (c->path) {\n>  \t\tstrbuf_addch(out, '/');\n> -\t\tstrbuf_add_percentencode(out, c->path);\n> +\t\tstrbuf_add_percentencode(out, c->path, STRBUF_PERCENTENCODE_PATH);\n>  \t}\n\nThis hunk is the one I expected.\n\n> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> index 5555a1524f..15eeef1dfd 100755\n> --- a/t/t0300-credentials.sh\n> +++ b/t/t0300-credentials.sh\n> @@ -510,6 +510,24 @@ test_expect_success 'helpers can abort the process' '\n>  \ttest_i18ncmp expect stderr\n>  '\n>  \n> +test_expect_success 'helpers can fetch with multiple path components' '\n> +\ttest_unconfig credential.helper &&\n> +\ttest_config credential.https://example.com/foo/repo.git.helper \"verbatim foo bar\" &&\n\nOK, you can't just use an argument to \"check\" because you want to set a\nspecific config option, not just credential.helper. Would this test make\nsense a little higher in the file, below \"match percent-encoded values\"\nperhaps?\n\n> +\techo url=https://example.com/foo/repo.git | git credential fill &&\n\nWhat's this line doing? It will just do the same \"check fill\" as\nbelow, but without the stdout checking. Is it leftover debugging cruft?\n\n> +\tcheck fill <<-\\EOF\n> +\turl=https://example.com/foo/repo.git\n> +\t--\n> +\tprotocol=https\n> +\thost=example.com\n> +\tusername=foo\n> +\tpassword=bar\n> +\t--\n> +\tverbatim: get\n> +\tverbatim: protocol=https\n> +\tverbatim: host=example.com\n\nAnd here we confirm that we got values from the \"verbatim\" helper. Good.\n\n-Peff\n"},{"id":"395925","messageId":"20200422184549.GH6465@camp.crustytoothpaste.net","threadId":"53281","inReplyTo":"20200422041614.GD3559880@coredump.intra.peff.net","subject":"Re: [PATCH] credential: fix matching URLs with multiple levels in path","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-04-22T18:45:49Z","receivedAt":"2020-04-22T18:45:58Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-04-22 at 04:16:14, Jeff King wrote:\n> On Wed, Apr 22, 2020 at 01:23:44AM +0000, brian m. carlson wrote:\n> \n> > 46fd7b3900 (\"credential: allow wildcard patterns when matching config\",\n> > 2020-02-20) introduced support for matching credential helpers using\n> > urlmatch.  In doing so, it introduced code to percent-encode the paths\n> > we get from the credential helper so that they could be effectively\n> > matched by the urlmatch code.\n> > \n> > Unfortunately, that code had a bug: it percent-encoded the slashes in\n> > the path, resulting in any URL path that contained multiple levels\n> > (i.e., a directory component) not matching.\n> > \n> > We are currently the only caller of the percent-encoding code and could\n> > simply change it not to encode slashes.  However, this would be\n> > surprising to other potential users who might want to use it and might\n> > result in unwanted slashes appearing in the encoded value.\n> > \n> > So instead, let's add a flag to skip encoding slashes, which is the\n> > behavior we want here, and use it when calling the code in this case.\n> > Add a test for credential helper URLs using multiple slashes in the\n> > path, which our test suite previously lacked.\n> \n> Thanks for the quick turnaround. The explanation makes sense.\n> \n> The patch leaves me with one question, though...\n> \n> > diff --git a/credential.c b/credential.c\n> > index 108d9e183a..f0e55a27ac 100644\n> > --- a/credential.c\n> > +++ b/credential.c\n> > @@ -136,14 +136,14 @@ static void credential_format(struct credential *c, struct strbuf *out)\n> >  \t\treturn;\n> >  \tstrbuf_addf(out, \"%s://\", c->protocol);\n> >  \tif (c->username && *c->username) {\n> > -\t\tstrbuf_add_percentencode(out, c->username);\n> > +\t\tstrbuf_add_percentencode(out, c->username, STRBUF_PERCENTENCODE_PATH);\n> >  \t\tstrbuf_addch(out, '@');\n> \n> Wouldn't we want to keep encoding slashes in the username?\n\nOh, you're right, we would.  That makes my commit message shorter, even.\n\n> > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> > index 5555a1524f..15eeef1dfd 100755\n> > --- a/t/t0300-credentials.sh\n> > +++ b/t/t0300-credentials.sh\n> > @@ -510,6 +510,24 @@ test_expect_success 'helpers can abort the process' '\n> >  \ttest_i18ncmp expect stderr\n> >  '\n> >  \n> > +test_expect_success 'helpers can fetch with multiple path components' '\n> > +\ttest_unconfig credential.helper &&\n> > +\ttest_config credential.https://example.com/foo/repo.git.helper \"verbatim foo bar\" &&\n> \n> OK, you can't just use an argument to \"check\" because you want to set a\n> specific config option, not just credential.helper. Would this test make\n> sense a little higher in the file, below \"match percent-encoded values\"\n> perhaps?\n\nI can hoist it, sure.\n\n> > +\techo url=https://example.com/foo/repo.git | git credential fill &&\n> \n> What's this line doing? It will just do the same \"check fill\" as\n> below, but without the stdout checking. Is it leftover debugging cruft?\n\nYeah, it is.  I'll rip it out in v2.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"395932","messageId":"nycvar.QRO.7.76.6.2004222117400.18039@tvgsbejvaqbjf.bet","threadId":"53281","inReplyTo":"20200422040644.GC3559880@coredump.intra.peff.net","subject":"Re: Credential helpers are no longer invoked in case of having sub-folder parts in a repository URL. Since 2.26.1 version","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-22T19:20:22Z","receivedAt":"2020-04-22T19:20:40Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 22 Apr 2020, Jeff King wrote:\n\n> On Wed, Apr 22, 2020 at 02:20:20AM +0000, brian m. carlson wrote:\n>\n> > > Thanks.  Here's another (though I haven't tried bisecting yet):\n> > >\n> > > \techo url='https://github.com/git/git' |\n> > > \tGIT_TERMINAL_PROMPT=0 \\\n> > > \tgit -c credential.helper= \\\n> > > \t\t-c credential.github.com.helper='!echo username=foo; echo password=bar;:' \\\n> > > \t\tcredential fill\n> >\n> > gitcredentials(7) says the following:\n> >\n> >   Git considers each credential to have a context defined by a URL.\n> >   This context is used to look up context-specific configuration, and is\n> >   passed to any helpers, which may use it as an index into secure\n> >   storage.\n> >\n> > I'm not sure a hostname qualifies as a URL in this case.  So while my\n> > patch did break this, I don't believe it's ever been documented to\n> > actually work and was an artifact of our implementation (along with\n> > \"credential./git/git.helper\" and \"credential.https://.helper\").  I've\n> > also never seen this syntax used in the wild, but maybe I'm not looking\n> > in the right places.\n>\n> I'm pretty sure it was an intended use case, though it is a natural\n> outcome of the credential_match() strategy of \"unspecified things match\n> anything\". I'd suspect that anybody relying on it is doing so\n> unintentionally, and just forgot to put the protocol field in. Though I\n> suppose doing so would let you cover http/https in a single block.\n>\n> At any rate, even in versions _without_ your patch, that became a hard\n> error in this week's release. In v2.24.3, for example:\n>\n>   $ echo url=https://anyhost.example.com |\n>     git -c credential.example.com.username=foo credential fill\n>   warning: url has no scheme: example.com\n>   fatal: credential url cannot be parsed: example.com\n>\n> because we're relying there on credential_from_url() to parse the config\n> credentials, too. After your patch, we use the http-config machinery,\n> which simply doesn't match.\n\nThis affects also pre-v2.26.* versions. One fallout is that some GitHub\nDesktop users reported that they cannot fetch anymore:\nhttps://github.com/desktop/desktop/issues/9597\n\nI _think_ I have a good fix for this, and am only waiting for the PR\nbuilds at https://github.com/gitgitgadget/git/pull/615 to finish before I\nwill submit that patch series for review.\n\nIdeally, I will integrate these patches into some MinGit backports\ntonight, still, so that GitHub Desktop can roll out another release to\navoid many more reports. Therefore, I hope for quick reviews ;-)\n\nCiao,\nDscho\n\n>\n> > I don't think we can shoehorn it into urlmatch, since that would break\n> > compatibility with the `http.*` config options, so I think we'd have to\n> > revert the entire feature if we want to preserve it.  I think I'd prefer\n> > to leave things as it is since it seems uncommon and there are easy\n> > alternatives, but if folks prefer, I can send a patch to revert the\n> > urlmatch feature.\n>\n> I agree that we should leave it. Aside from the dual http/https thing\n> (which _hopefully_ is rare these days as https become more of a\n> standard), I don't think it has a legitimate use case. And I think we\n> should be pushing users to be a bit more careful with their url config.\n>\n> -Peff\n>\n>\n"},{"id":"395937","messageId":"20200422195109.2224035-1-sandals@crustytoothpaste.net","threadId":"53281","inReplyTo":"20200421225837.GB3515235@coredump.intra.peff.net","subject":"[PATCH v2] credential: fix matching URLs with multiple levels in path","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-04-22T19:51:09Z","receivedAt":"2020-04-22T19:51:28Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"46fd7b3900 (\"credential: allow wildcard patterns when matching config\",\n2020-02-20) introduced support for matching credential helpers using\nurlmatch.  In doing so, it introduced code to percent-encode the paths\nwe get from the credential helper so that they could be effectively\nmatched by the urlmatch code.\n\nUnfortunately, that code had a bug: it percent-encoded the slashes in\nthe path, resulting in any URL path that contained multiple levels\n(i.e., a directory component) not matching.\n\nWe are currently the only caller of the percent-encoding code and could\nsimply change it not to encode slashes.  However, we still want to\nencode slashes in the username component, so we need to have both\nbehaviors available.\n\nSo instead, let's add a flag to skip encoding slashes, which is the\nbehavior we want here, and use it when calling the code in this case.\nAdd a test for credential helper URLs using multiple slashes in the\npath, which our test suite previously lacked, as well as one ensuring\nthat we handle usernames with slashes gracefully.\n\nReported-by: Ilya Tretyakov <it@it3xl.ru>\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\nChanges from v1:\n* Continue to encode slashes in usernames.\n* Add a test for encoding slashes in usernames.\n* Hoist existing tests near the other percent-encoding tests.\n* Update commit message.\n* Remove debugging information.\n\n credential.c           |  4 ++--\n strbuf.c               |  8 +++++---\n strbuf.h               |  6 +++++-\n t/t0300-credentials.sh | 30 ++++++++++++++++++++++++++++++\n 4 files changed, 42 insertions(+), 6 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 108d9e183a..6658cd0860 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -136,14 +136,14 @@ static void credential_format(struct credential *c, struct strbuf *out)\n \t\treturn;\n \tstrbuf_addf(out, \"%s://\", c->protocol);\n \tif (c->username && *c->username) {\n-\t\tstrbuf_add_percentencode(out, c->username);\n+\t\tstrbuf_add_percentencode(out, c->username, 0);\n \t\tstrbuf_addch(out, '@');\n \t}\n \tif (c->host)\n \t\tstrbuf_addstr(out, c->host);\n \tif (c->path) {\n \t\tstrbuf_addch(out, '/');\n-\t\tstrbuf_add_percentencode(out, c->path);\n+\t\tstrbuf_add_percentencode(out, c->path, STRBUF_PERCENTENCODE_PATH);\n \t}\n }\n \ndiff --git a/strbuf.c b/strbuf.c\nindex bb0065ccaf..da5a1c9ca4 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -479,15 +479,17 @@ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)\n \t}\n }\n \n-#define URL_UNSAFE_CHARS \" <>\\\"%{}|\\\\^`:/?#[]@!$&'()*+,;=\"\n+#define URL_UNSAFE_CHARS \" <>\\\"%{}|\\\\^`:?#[]@!$&'()*+,;=\"\n \n-void strbuf_add_percentencode(struct strbuf *dst, const char *src)\n+void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags)\n {\n \tsize_t i, len = strlen(src);\n \n \tfor (i = 0; i < len; i++) {\n \t\tunsigned char ch = src[i];\n-\t\tif (ch <= 0x1F || ch >= 0x7F || strchr(URL_UNSAFE_CHARS, ch))\n+\t\tif (ch <= 0x1F || ch >= 0x7F ||\n+\t\t    (ch == '/' && !(flags & STRBUF_PERCENTENCODE_PATH)) ||\n+\t\t    strchr(URL_UNSAFE_CHARS, ch))\n \t\t\tstrbuf_addf(dst, \"%%%02X\", (unsigned char)ch);\n \t\telse\n \t\t\tstrbuf_addch(dst, ch);\ndiff --git a/strbuf.h b/strbuf.h\nindex ce8e49c0b2..651aedff57 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -378,11 +378,15 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb,\n  */\n void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n \n+#define STRBUF_PERCENTENCODE_PATH 1\n+\n /**\n  * Append the contents of a string to a strbuf, percent-encoding any characters\n  * that are needed to be encoded for a URL.\n+ *\n+ * If STRBUF_PERCENTENCODE_PATH is set in flags, don't percent-encode slashes.\n  */\n-void strbuf_add_percentencode(struct strbuf *dst, const char *src);\n+void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags);\n \n /**\n  * Append the given byte size as a human-readable string (i.e. 12.23 KiB,\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 5555a1524f..87267a7aac 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -366,6 +366,36 @@ test_expect_success 'match percent-encoded values' '\n \tEOF\n '\n \n+test_expect_success 'match percent-encoded values in username' '\n+\ttest_config credential.https://user%2fname@example.com/foo/bar.git.helper \"$HELPER\" &&\n+\tcheck fill <<-\\EOF\n+\turl=https://user%2fname@example.com/foo/bar.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'fetch with multiple path components' '\n+\ttest_unconfig credential.helper &&\n+\ttest_config credential.https://example.com/foo/repo.git.helper \"verbatim foo bar\" &&\n+\tcheck fill <<-\\EOF\n+\turl=https://example.com/foo/repo.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\t--\n+\tverbatim: get\n+\tverbatim: protocol=https\n+\tverbatim: host=example.com\n+\tEOF\n+'\n+\n test_expect_success 'pull username from config' '\n \ttest_config credential.https://example.com.username foo &&\n \tcheck fill <<-\\EOF\n"},{"id":"395939","messageId":"20200422200424.GF558336@coredump.intra.peff.net","threadId":"53281","inReplyTo":"20200422195109.2224035-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2] credential: fix matching URLs with multiple levels in path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-22T20:04:24Z","receivedAt":"2020-04-22T20:04:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2020 at 07:51:09PM +0000, brian m. carlson wrote:\n\n> Changes from v1:\n> * Continue to encode slashes in usernames.\n> * Add a test for encoding slashes in usernames.\n> * Hoist existing tests near the other percent-encoding tests.\n> * Update commit message.\n> * Remove debugging information.\n\nThank for an easy-to-read explanation and patch (as usual). This version\nlooks good to me.\n\nI think there's still an open question on:\n\n  [credential \"no-scheme.example.com\"]\n\nconfig sections. I feel like that's something we never really intended\nto support and should discourage, but it seems as though it may be in\nwide use. But it's definitely a separate patch, and it sounds like Dscho\nis working on it.\n\n-Peff\n"},{"id":"396106","messageId":"20200424045030.GA44448@Carlos-MBP","threadId":"53281","inReplyTo":"20200422195109.2224035-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2] credential: fix matching URLs with multiple levels in path","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-24T04:50:30Z","receivedAt":"2020-04-24T04:50:34Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Apr 22, 2020 at 07:51:09PM +0000, brian m. carlson wrote:\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -378,11 +378,15 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb,\n>   */\n>  void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n>  \n> +#define STRBUF_PERCENTENCODE_PATH 1\n> +\n>  /**\n>   * Append the contents of a string to a strbuf, percent-encoding any characters\n>   * that are needed to be encoded for a URL.\n> + *\n> + * If STRBUF_PERCENTENCODE_PATH is set in flags, don't percent-encode slashes.\n>   */\n\nwouldn't it be better to call this STRBUF_PERCENTENCODE_SLASH instead?, since\nit is actually not applied to path?\n\nCarlo\n"},{"id":"396174","messageId":"xmqqeescd4a2.fsf@gitster.c.googlers.com","threadId":"53281","inReplyTo":"20200424045030.GA44448@Carlos-MBP","subject":"Re: [PATCH v2] credential: fix matching URLs with multiple levels in path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T20:20:37Z","receivedAt":"2020-04-24T20:20:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:\n\n> On Wed, Apr 22, 2020 at 07:51:09PM +0000, brian m. carlson wrote:\n>> --- a/strbuf.h\n>> +++ b/strbuf.h\n>> @@ -378,11 +378,15 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb,\n>>   */\n>>  void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n>>  \n>> +#define STRBUF_PERCENTENCODE_PATH 1\n>> +\n>>  /**\n>>   * Append the contents of a string to a strbuf, percent-encoding any characters\n>>   * that are needed to be encoded for a URL.\n>> + *\n>> + * If STRBUF_PERCENTENCODE_PATH is set in flags, don't percent-encode slashes.\n>>   */\n>\n> wouldn't it be better to call this STRBUF_PERCENTENCODE_SLASH instead?, since\n> it is actually not applied to path?\n\nI was about to say the same thing.  Can we phrase it a bit shorter\nthan PERCENTENCODE, by the way?\n"},{"id":"396270","messageId":"20200425213215.71674-1-sandals@crustytoothpaste.net","threadId":"53281","inReplyTo":"20200421225837.GB3515235@coredump.intra.peff.net","subject":"[PATCH v3] redential: fix matching URLs with multiple levels in path","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-04-25T21:32:15Z","receivedAt":"2020-04-25T21:34:10Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"46fd7b3900 (\"credential: allow wildcard patterns when matching config\",\n2020-02-20) introduced support for matching credential helpers using\nurlmatch.  In doing so, it introduced code to percent-encode the paths\nwe get from the credential helper so that they could be effectively\nmatched by the urlmatch code.\n\nUnfortunately, that code had a bug: it percent-encoded the slashes in\nthe path, resulting in any URL path that contained multiple levels\n(i.e., a directory component) not matching.\n\nWe are currently the only caller of the percent-encoding code and could\nsimply change it not to encode slashes.  However, we still want to\nencode slashes in the username component, so we need to have both\nbehaviors available.\n\nSo instead, let's add a flag to control encoding slashes, which is the\nbehavior we want here, and use it when calling the code in this case.\nAdd a test for credential helper URLs using multiple slashes in the\npath, which our test suite previously lacked, as well as one ensuring\nthat we handle usernames with slashes gracefully.\n\nReported-by: Ilya Tretyakov <it@it3xl.ru>\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\nChanges from v2:\n* Invert the sense of the flag.\n* Shorten the flag name.\n\nChanges from v1:\n* Continue to encode slashes in usernames.\n* Add a test for encoding slashes in usernames.\n* Hoist existing tests near the other percent-encoding tests.\n* Update commit message.\n* Remove debugging information.\n\n credential.c           |  4 ++--\n strbuf.c               |  8 +++++---\n strbuf.h               |  7 ++++++-\n t/t0300-credentials.sh | 30 ++++++++++++++++++++++++++++++\n 4 files changed, 43 insertions(+), 6 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 108d9e183a..f5327d3d9e 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -136,14 +136,14 @@ static void credential_format(struct credential *c, struct strbuf *out)\n \t\treturn;\n \tstrbuf_addf(out, \"%s://\", c->protocol);\n \tif (c->username && *c->username) {\n-\t\tstrbuf_add_percentencode(out, c->username);\n+\t\tstrbuf_add_percentencode(out, c->username, STRBUF_ENCODE_SLASH);\n \t\tstrbuf_addch(out, '@');\n \t}\n \tif (c->host)\n \t\tstrbuf_addstr(out, c->host);\n \tif (c->path) {\n \t\tstrbuf_addch(out, '/');\n-\t\tstrbuf_add_percentencode(out, c->path);\n+\t\tstrbuf_add_percentencode(out, c->path, 0);\n \t}\n }\n \ndiff --git a/strbuf.c b/strbuf.c\nindex bb0065ccaf..51c83aae2d 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -479,15 +479,17 @@ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)\n \t}\n }\n \n-#define URL_UNSAFE_CHARS \" <>\\\"%{}|\\\\^`:/?#[]@!$&'()*+,;=\"\n+#define URL_UNSAFE_CHARS \" <>\\\"%{}|\\\\^`:?#[]@!$&'()*+,;=\"\n \n-void strbuf_add_percentencode(struct strbuf *dst, const char *src)\n+void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags)\n {\n \tsize_t i, len = strlen(src);\n \n \tfor (i = 0; i < len; i++) {\n \t\tunsigned char ch = src[i];\n-\t\tif (ch <= 0x1F || ch >= 0x7F || strchr(URL_UNSAFE_CHARS, ch))\n+\t\tif (ch <= 0x1F || ch >= 0x7F ||\n+\t\t    (ch == '/' && (flags & STRBUF_ENCODE_SLASH)) ||\n+\t\t    strchr(URL_UNSAFE_CHARS, ch))\n \t\t\tstrbuf_addf(dst, \"%%%02X\", (unsigned char)ch);\n \t\telse\n \t\t\tstrbuf_addch(dst, ch);\ndiff --git a/strbuf.h b/strbuf.h\nindex ce8e49c0b2..e524c52e01 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -378,11 +378,16 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb,\n  */\n void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n \n+#define STRBUF_ENCODE_SLASH 1\n+\n /**\n  * Append the contents of a string to a strbuf, percent-encoding any characters\n  * that are needed to be encoded for a URL.\n+ *\n+ * If STRBUF_ENCODE_SLASH is set in flags, percent-encode slashes.  Otherwise,\n+ * slashes are not percent-encoded.\n  */\n-void strbuf_add_percentencode(struct strbuf *dst, const char *src);\n+void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags);\n \n /**\n  * Append the given byte size as a human-readable string (i.e. 12.23 KiB,\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 5555a1524f..87267a7aac 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -366,6 +366,36 @@ test_expect_success 'match percent-encoded values' '\n \tEOF\n '\n \n+test_expect_success 'match percent-encoded values in username' '\n+\ttest_config credential.https://user%2fname@example.com/foo/bar.git.helper \"$HELPER\" &&\n+\tcheck fill <<-\\EOF\n+\turl=https://user%2fname@example.com/foo/bar.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'fetch with multiple path components' '\n+\ttest_unconfig credential.helper &&\n+\ttest_config credential.https://example.com/foo/repo.git.helper \"verbatim foo bar\" &&\n+\tcheck fill <<-\\EOF\n+\turl=https://example.com/foo/repo.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\t--\n+\tverbatim: get\n+\tverbatim: protocol=https\n+\tverbatim: host=example.com\n+\tEOF\n+'\n+\n test_expect_success 'pull username from config' '\n \ttest_config credential.https://example.com.username foo &&\n \tcheck fill <<-\\EOF\n"},{"id":"396281","messageId":"CAPig+cQW1qLRgQg=yR2Uv5T9vQG-8zWyfBLzTh6=YU943Ayb+Q@mail.gmail.com","threadId":"53281","inReplyTo":"20200425213215.71674-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3] redential: fix matching URLs with multiple levels in path","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-26T01:51:19Z","receivedAt":"2020-04-26T01:51:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 25, 2020 at 5:34 PM brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n> redential: fix matching URLs with multiple levels in path\n\nWhat's a \"redential\"?\n"},{"id":"396293","messageId":"20200426172659.118503-1-sandals@crustytoothpaste.net","threadId":"53281","inReplyTo":"20200421225837.GB3515235@coredump.intra.peff.net","subject":"[PATCH v4] credential: fix matching URLs with multiple levels in path","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-04-26T17:26:59Z","receivedAt":"2020-04-26T17:27:13Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"46fd7b3900 (\"credential: allow wildcard patterns when matching config\",\n2020-02-20) introduced support for matching credential helpers using\nurlmatch.  In doing so, it introduced code to percent-encode the paths\nwe get from the credential helper so that they could be effectively\nmatched by the urlmatch code.\n\nUnfortunately, that code had a bug: it percent-encoded the slashes in\nthe path, resulting in any URL path that contained multiple levels\n(i.e., a directory component) not matching.\n\nWe are currently the only caller of the percent-encoding code and could\nsimply change it not to encode slashes.  However, we still want to\nencode slashes in the username component, so we need to have both\nbehaviors available.\n\nSo instead, let's add a flag to control encoding slashes, which is the\nbehavior we want here, and use it when calling the code in this case.\nAdd a test for credential helper URLs using multiple slashes in the\npath, which our test suite previously lacked, as well as one ensuring\nthat we handle usernames with slashes gracefully.\n\nReported-by: Ilya Tretyakov <it@it3xl.ru>\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\nChanges from v3:\n* Don't delete the first character of the summary.\n\nChanges from v2:\n* Invert the sense of the flag.\n* Shorten the flag name.\n\nChanges from v1:\n* Continue to encode slashes in usernames.\n* Add a test for encoding slashes in usernames.\n* Hoist existing tests near the other percent-encoding tests.\n* Update commit message.\n* Remove debugging information.\n\n credential.c           |  4 ++--\n strbuf.c               |  8 +++++---\n strbuf.h               |  7 ++++++-\n t/t0300-credentials.sh | 30 ++++++++++++++++++++++++++++++\n 4 files changed, 43 insertions(+), 6 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 108d9e183a..f5327d3d9e 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -136,14 +136,14 @@ static void credential_format(struct credential *c, struct strbuf *out)\n \t\treturn;\n \tstrbuf_addf(out, \"%s://\", c->protocol);\n \tif (c->username && *c->username) {\n-\t\tstrbuf_add_percentencode(out, c->username);\n+\t\tstrbuf_add_percentencode(out, c->username, STRBUF_ENCODE_SLASH);\n \t\tstrbuf_addch(out, '@');\n \t}\n \tif (c->host)\n \t\tstrbuf_addstr(out, c->host);\n \tif (c->path) {\n \t\tstrbuf_addch(out, '/');\n-\t\tstrbuf_add_percentencode(out, c->path);\n+\t\tstrbuf_add_percentencode(out, c->path, 0);\n \t}\n }\n \ndiff --git a/strbuf.c b/strbuf.c\nindex bb0065ccaf..51c83aae2d 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -479,15 +479,17 @@ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)\n \t}\n }\n \n-#define URL_UNSAFE_CHARS \" <>\\\"%{}|\\\\^`:/?#[]@!$&'()*+,;=\"\n+#define URL_UNSAFE_CHARS \" <>\\\"%{}|\\\\^`:?#[]@!$&'()*+,;=\"\n \n-void strbuf_add_percentencode(struct strbuf *dst, const char *src)\n+void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags)\n {\n \tsize_t i, len = strlen(src);\n \n \tfor (i = 0; i < len; i++) {\n \t\tunsigned char ch = src[i];\n-\t\tif (ch <= 0x1F || ch >= 0x7F || strchr(URL_UNSAFE_CHARS, ch))\n+\t\tif (ch <= 0x1F || ch >= 0x7F ||\n+\t\t    (ch == '/' && (flags & STRBUF_ENCODE_SLASH)) ||\n+\t\t    strchr(URL_UNSAFE_CHARS, ch))\n \t\t\tstrbuf_addf(dst, \"%%%02X\", (unsigned char)ch);\n \t\telse\n \t\t\tstrbuf_addch(dst, ch);\ndiff --git a/strbuf.h b/strbuf.h\nindex ce8e49c0b2..e524c52e01 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -378,11 +378,16 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb,\n  */\n void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n \n+#define STRBUF_ENCODE_SLASH 1\n+\n /**\n  * Append the contents of a string to a strbuf, percent-encoding any characters\n  * that are needed to be encoded for a URL.\n+ *\n+ * If STRBUF_ENCODE_SLASH is set in flags, percent-encode slashes.  Otherwise,\n+ * slashes are not percent-encoded.\n  */\n-void strbuf_add_percentencode(struct strbuf *dst, const char *src);\n+void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags);\n \n /**\n  * Append the given byte size as a human-readable string (i.e. 12.23 KiB,\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 5555a1524f..87267a7aac 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -366,6 +366,36 @@ test_expect_success 'match percent-encoded values' '\n \tEOF\n '\n \n+test_expect_success 'match percent-encoded values in username' '\n+\ttest_config credential.https://user%2fname@example.com/foo/bar.git.helper \"$HELPER\" &&\n+\tcheck fill <<-\\EOF\n+\turl=https://user%2fname@example.com/foo/bar.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'fetch with multiple path components' '\n+\ttest_unconfig credential.helper &&\n+\ttest_config credential.https://example.com/foo/repo.git.helper \"verbatim foo bar\" &&\n+\tcheck fill <<-\\EOF\n+\turl=https://example.com/foo/repo.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\t--\n+\tverbatim: get\n+\tverbatim: protocol=https\n+\tverbatim: host=example.com\n+\tEOF\n+'\n+\n test_expect_success 'pull username from config' '\n \ttest_config credential.https://example.com.username foo &&\n \tcheck fill <<-\\EOF\n"},{"id":"396313","messageId":"20200427011809.2494377-1-sandals@crustytoothpaste.net","threadId":"53281","inReplyTo":"20200421225837.GB3515235@coredump.intra.peff.net","subject":"[PATCH v5] credential: fix matching URLs with multiple levels in path","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-04-27T01:18:08Z","receivedAt":"2020-04-27T01:18:21Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"46fd7b3900 (\"credential: allow wildcard patterns when matching config\",\n2020-02-20) introduced support for matching credential helpers using\nurlmatch.  In doing so, it introduced code to percent-encode the paths\nwe get from the credential helper so that they could be effectively\nmatched by the urlmatch code.\n\nUnfortunately, that code had a bug: it percent-encoded the slashes in\nthe path, resulting in any URL path that contained multiple levels\n(i.e., a directory component) not matching.\n\nWe are currently the only caller of the percent-encoding code and could\nsimply change it not to encode slashes.  However, we still want to\nencode slashes in the username component, so we need to have both\nbehaviors available.\n\nSo instead, let's add a flag to control encoding slashes, which is the\nbehavior we want here, and use it when calling the code in this case.\n\nAdd a test for credential helper URLs using multiple slashes in the\npath, which our test suite previously lacked, as well as one ensuring\nthat we handle usernames with slashes gracefully.  Since we're testing\nother percent-encoding handling, let's add one for non-ASCII UTF-8\ncharacters as well.\n\nReported-by: Ilya Tretyakov <it@it3xl.ru>\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\nI talked to Carlo off-list because he was concerned about the potential\nsecurity implications of the patch, and as part of that discussion he\nsuggested an additional test for our UTF-8 handling.  Since adding some\nadditional tests for that case seemed like a good idea, I squashed in a\npatch of his to handle that case.\n\nThat's the only change between v4 and v5.\n\n credential.c           |  4 ++--\n strbuf.c               |  8 +++++---\n strbuf.h               |  7 ++++++-\n t/t0300-credentials.sh | 45 ++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 58 insertions(+), 6 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 064e25e5d5..d72e2ed0d8 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -136,14 +136,14 @@ static void credential_format(struct credential *c, struct strbuf *out)\n \t\treturn;\n \tstrbuf_addf(out, \"%s://\", c->protocol);\n \tif (c->username && *c->username) {\n-\t\tstrbuf_add_percentencode(out, c->username);\n+\t\tstrbuf_add_percentencode(out, c->username, STRBUF_ENCODE_SLASH);\n \t\tstrbuf_addch(out, '@');\n \t}\n \tif (c->host)\n \t\tstrbuf_addstr(out, c->host);\n \tif (c->path) {\n \t\tstrbuf_addch(out, '/');\n-\t\tstrbuf_add_percentencode(out, c->path);\n+\t\tstrbuf_add_percentencode(out, c->path, 0);\n \t}\n }\n \ndiff --git a/strbuf.c b/strbuf.c\nindex bb0065ccaf..51c83aae2d 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -479,15 +479,17 @@ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)\n \t}\n }\n \n-#define URL_UNSAFE_CHARS \" <>\\\"%{}|\\\\^`:/?#[]@!$&'()*+,;=\"\n+#define URL_UNSAFE_CHARS \" <>\\\"%{}|\\\\^`:?#[]@!$&'()*+,;=\"\n \n-void strbuf_add_percentencode(struct strbuf *dst, const char *src)\n+void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags)\n {\n \tsize_t i, len = strlen(src);\n \n \tfor (i = 0; i < len; i++) {\n \t\tunsigned char ch = src[i];\n-\t\tif (ch <= 0x1F || ch >= 0x7F || strchr(URL_UNSAFE_CHARS, ch))\n+\t\tif (ch <= 0x1F || ch >= 0x7F ||\n+\t\t    (ch == '/' && (flags & STRBUF_ENCODE_SLASH)) ||\n+\t\t    strchr(URL_UNSAFE_CHARS, ch))\n \t\t\tstrbuf_addf(dst, \"%%%02X\", (unsigned char)ch);\n \t\telse\n \t\t\tstrbuf_addch(dst, ch);\ndiff --git a/strbuf.h b/strbuf.h\nindex ce8e49c0b2..e524c52e01 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -378,11 +378,16 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb,\n  */\n void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n \n+#define STRBUF_ENCODE_SLASH 1\n+\n /**\n  * Append the contents of a string to a strbuf, percent-encoding any characters\n  * that are needed to be encoded for a URL.\n+ *\n+ * If STRBUF_ENCODE_SLASH is set in flags, percent-encode slashes.  Otherwise,\n+ * slashes are not percent-encoded.\n  */\n-void strbuf_add_percentencode(struct strbuf *dst, const char *src);\n+void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags);\n \n /**\n  * Append the given byte size as a human-readable string (i.e. 12.23 KiB,\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 48484cbcf6..91a007fbfa 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -366,6 +366,51 @@ test_expect_success 'match percent-encoded values' '\n \tEOF\n '\n \n+test_expect_success 'match percent-encoded UTF-8 values in path' '\n+\ttest_config credential.https://example.com.useHttpPath true &&\n+\ttest_config credential.https://example.com/perú.git.helper \"$HELPER\" &&\n+\tcheck fill <<-\\EOF\n+\turl=https://example.com/per%C3%BA.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tpath=perú.git\n+\tusername=foo\n+\tpassword=bar\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'match percent-encoded values in username' '\n+\ttest_config credential.https://user%2fname@example.com/foo/bar.git.helper \"$HELPER\" &&\n+\tcheck fill <<-\\EOF\n+\turl=https://user%2fname@example.com/foo/bar.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'fetch with multiple path components' '\n+\ttest_unconfig credential.helper &&\n+\ttest_config credential.https://example.com/foo/repo.git.helper \"verbatim foo bar\" &&\n+\tcheck fill <<-\\EOF\n+\turl=https://example.com/foo/repo.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=foo\n+\tpassword=bar\n+\t--\n+\tverbatim: get\n+\tverbatim: protocol=https\n+\tverbatim: host=example.com\n+\tEOF\n+'\n+\n test_expect_success 'pull username from config' '\n \ttest_config credential.https://example.com.username foo &&\n \tcheck fill <<-\\EOF\n"},{"id":"396363","messageId":"xmqqblnc93bd.fsf@gitster.c.googlers.com","threadId":"53281","inReplyTo":"20200427011809.2494377-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v5] credential: fix matching URLs with multiple levels in path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-27T18:44:06Z","receivedAt":"2020-04-27T18:44:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> I talked to Carlo off-list because he was concerned about the potential\n> security implications of the patch, and as part of that discussion he\n> suggested an additional test for our UTF-8 handling.  Since adding some\n> additional tests for that case seemed like a good idea, I squashed in a\n> patch of his to handle that case.\n>\n> That's the only change between v4 and v5.\n\nWith the polarity flip of the option done in v4, the resulting code\nis quite easy to follow.  Thanks, will queue.\n"}]}