{"thread":{"id":"53289","subject":"[PATCH 0/3] credential: handle partial URLs in config settings again","startedAt":"2020-04-22T20:51:09Z","lastAt":"2020-05-01T20:01:20Z","messageCount":48,"participants":["Johannes Schindelin via GitGitGadget","Jeff King","Johannes Schindelin","Junio C Hamano","Jonathan Nieder","Carlo Arenas","Carlo Marcelo Arenas Belón"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"395943","messageId":"pull.615.git.1587588665.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":null,"subject":"[PATCH 0/3] credential: handle partial URLs in config settings again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-22T20:51:02Z","receivedAt":"2020-04-22T20:51:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This fixes the problem illustrated by Peff's example\n[https://lore.kernel.org/git/20200422040644.GC3559880@coredump.intra.peff.net/]\n, in maint-2.17:\n\n $ echo url=https://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\nThe fix is necessarily different than what was proposed by brian\n[https://lore.kernel.org/git/20200422012344.2051103-1-sandals@crustytoothpaste.net/] \nbecause that fix targets v2.26.x which has 46fd7b390034 (credential: allow\nwildcard patterns when matching config, 2020-02-20).\n\nThis patch series targets maint-2.17 instead (and might actually not be able\nto fix maint due to that wildcard pattern patch; I haven't had the time to\ncheck yet).\n\nPlease note that Git v2.17.4 will not do what we would expect here: if any\nhost name (without protocol) is specified, e.g. -c\ncredential.golli.wog.username = boo, it will actually ignore the host name.\nThat is, this will populate the username:\n\n $ echo url=https://example.com |\n git -c credential.totally.bog.us.username=foo credential fill\n\nObviously, this is unexpected, as a Git config like this would leave the\nlast specified user name as \"winner\":\n\n[credential \"first.com\"]\n username = Whos On\n[credential \"second.com\"]\n username = Who\n\nThis patch series fixes this. The quoted part of [credential \"\"] will\nbe interpreted as a partial URL:\n\n * It can start with a protocol followed by ://, but does not have to.\n * If it starts with a protocol, the host name will always be set (if the \n :// is followed immediately by yet another slash, then it will be set to\n the empty string).\n * If it starts without a protocol, it is treated as a path if the value\n starts with a slash (and the host will be left unset).\n * If it starts without a protocol and the first character is not a slash,\n it will be treated as a host name, optionally followed by a slash and the\n path.\n\nJohannes Schindelin (3):\n credential: fix grammar\n credential: teach `credential_from_url()` a non-strict mode\n credential: handle `credential..` again\n\n credential.c | 21 ++++++++++++++-------\n credential.h | 8 ++++++--\n fsck.c | 2 +-\n t/t0300-credentials.sh | 13 +++++++++++++\n 4 files changed, 34 insertions(+), 10 deletions(-)\n\n\nbase-commit: df5be6dc3fd18c294ec93a9af0321334e3f92c9c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-615%2Fdscho%2Fcredential-config-partial-url-maint-2.17-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-615/dscho/credential-config-partial-url-maint-2.17-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/615\n-- \ngitgitgadget\n"},{"id":"395944","messageId":"2bf781081d9e43f38f2141de023fe918eedcbf5d.1587588665.git.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":"pull.615.git.1587588665.gitgitgadget@gmail.com","subject":"[PATCH 1/3] credential: fix grammar","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-22T20:51:03Z","receivedAt":"2020-04-22T20:51:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThere was a lot going on behind the scenes when the vulnerability and\npossible solutions were discussed. Grammar was not a primary focus,\nthat's why this slipped in.\n\nSigned-off-by: Johannes Schindelin \n---\n credential.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential.h b/credential.h\nindex 122a23cd2f1..5a86502d95c 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -32,7 +32,7 @@ void credential_write(const struct credential *, FILE *);\n /*\n * Parse a url into a credential struct, replacing any existing contents.\n *\n- * Ifthe url can't be parsed (e.g., a missing \"proto://\" component), the\n+ * If the url can't be parsed (e.g., a missing \"proto://\" component), the\n * resulting credential will be empty but we'll still return success from the\n * \"gently\" form.\n *\n-- \ngitgitgadget\n\n"},{"id":"395945","messageId":"66823c735b1d5ea2047a29656e82fa6fe895f6f1.1587588665.git.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":"pull.615.git.1587588665.gitgitgadget@gmail.com","subject":"[PATCH 3/3] credential: handle `credential..` again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-22T20:51:05Z","receivedAt":"2020-04-22T20:51:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the patches for CVE-2020-11008, the ability to specify credential\nsettings in the config for partial URLs got lost. For example, it used\nto be possible to specify a credential helper for a specific protocol:\n\n\t[credential \"https://\"]\n\t\thelper = my-https-helper\n\nLikewise, it used to be possible to configure settings for a specific\nhost, e.g.:\n\n\t[credential \"dev.azure.com\"]\n\t\tuseHTTPPath = true\n\nLet's reinstate this behavior.\n\nOriginal-test-case-by: Jeff King \nSigned-off-by: Johannes Schindelin \n---\n credential.c | 7 ++++++-\n t/t0300-credentials.sh | 13 +++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/credential.c b/credential.c\nindex c73260ac40f..b9e8daa5406 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -53,7 +53,12 @@ static int credential_config_callback(const char *var, const char *value,\n \t\tchar *url = xmemdupz(key, dot - key);\n \t\tint matched;\n \n-\t\tcredential_from_url(&want, url);\n+\t\tif (credential_from_url_gently(&want, url, 0, 0) < 0) {\n+\t\t\twarning(_(\"skipping credential lookup for url: %s\"), url);\n+\t\t\tcredential_clear(c);\n+\t\t\tfree(url);\n+\t\t\treturn 0;\n+\t\t}\n \t\tmatched = credential_match(&want, c);\n \n \t\tcredential_clear(&want);\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex efed3ea2955..9dcba6a7ad9 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -448,4 +448,17 @@ test_expect_success 'credential system refuses to work with missing protocol' '\n \ttest_i18ncmp expect stderr\n '\n \n+test_expect_success 'credential config accepts partial URLs' '\n+\techo url=https://example.com |\n+\tgit -c credential.example.com.username=boo \\\n+\t\tcredential fill >actual &&\n+\tcat >expect <<-EOF &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=boo\n+\tpassword=askpass-password\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"395946","messageId":"1081841b16de31693473e72ff817bed5f0064dda.1587588665.git.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":"pull.615.git.1587588665.gitgitgadget@gmail.com","subject":"[PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-22T20:51:04Z","receivedAt":"2020-04-22T20:51:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nPrior to the fixes for CVE-2020-11008, we were _very_ lenient in what we\nrequired from a URL in order to parse it into a `struct credential`.\nThat led to serious vulnerabilities.\n\nThere was one call site, though, that really needed that leniency: when\nparsing config settings a la `credential.dev.azure.com.useHTTPPath`.\n\nIn preparation for fixing that regression, let's add a parameter called\n`strict` to the `credential_from_url()` function and convert the\nexisting callers to enforce that strict mode.\n\nSigned-off-by: Johannes Schindelin \n---\n credential.c | 14 ++++++++------\n credential.h | 6 +++++-\n fsck.c | 2 +-\n 3 files changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 64a841eddca..c73260ac40f 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -344,7 +344,7 @@ static int check_url_component(const char *url, int quiet,\n }\n \n int credential_from_url_gently(struct credential *c, const char *url,\n-\t\t\t int quiet)\n+\t\t\t int strict, int quiet)\n {\n \tconst char *at, *colon, *cp, *slash, *host, *proto_end;\n \n@@ -357,12 +357,12 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \t * (3) proto://:@/...\n \t */\n \tproto_end = strstr(url, \"://\");\n-\tif (!proto_end || proto_end == url) {\n+\tif (strict && (!proto_end || proto_end == url)) {\n \t\tif (!quiet)\n \t\t\twarning(_(\"url has no scheme: %s\"), url);\n \t\treturn -1;\n \t}\n-\tcp = proto_end + 3;\n+\tcp = proto_end ? proto_end + 3 : url;\n \tat = strchr(cp, '@');\n \tcolon = strchr(cp, ':');\n \tslash = strchrnul(cp, '/');\n@@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \t\thost = at + 1;\n \t}\n \n-\tc->protocol = xmemdupz(url, proto_end - url);\n-\tc->host = url_decode_mem(host, slash - host);\n+\tif (proto_end && proto_end - url > 0)\n+\t\tc->protocol = xmemdupz(url, proto_end - url);\n+\tif (slash - url > 0)\n+\t\tc->host = url_decode_mem(host, slash - host);\n \t/* Trim leading and trailing slashes from path */\n \twhile (*slash == '/')\n \t\tslash++;\n@@ -407,6 +409,6 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \n void credential_from_url(struct credential *c, const char *url)\n {\n-\tif (credential_from_url_gently(c, url, 0) < 0)\n+\tif (credential_from_url_gently(c, url, 1, 0) < 0)\n \t\tdie(_(\"credential url cannot be parsed: %s\"), url);\n }\ndiff --git a/credential.h b/credential.h\nindex 5a86502d95c..823ec2caf35 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -41,9 +41,13 @@ void credential_write(const struct credential *, FILE *);\n * an error but leave the broken state in the credential object for further\n * examination. The non-gentle form will issue a warning to stderr and return\n * an empty credential.\n+ *\n+ * In strict mode, an empty protocol or an empty host name are not allowed.\n+ * The credential_from_url() function enforces strict mode.\n */\n void credential_from_url(struct credential *, const char *url);\n-int credential_from_url_gently(struct credential *, const char *url, int quiet);\n+int credential_from_url_gently(struct credential *, const char *url,\n+\t\t\t int strict, int quiet);\n \n int credential_match(const struct credential *have,\n \t\t const struct credential *want);\ndiff --git a/fsck.c b/fsck.c\nindex 31b5be05f54..aa66dc1e742 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1076,7 +1076,7 @@ static int check_submodule_url(const char *url)\n \telse if (url_to_curl_url(url, &curl_url)) {\n \t\tstruct credential c = CREDENTIAL_INIT;\n \t\tint ret = 0;\n-\t\tif (credential_from_url_gently(&c, curl_url, 1) ||\n+\t\tif (credential_from_url_gently(&c, curl_url, 1, 1) ||\n \t\t !*c.host)\n \t\t\tret = -1;\n \t\tcredential_clear(&c);\n-- \ngitgitgadget\n\n"},{"id":"395950","messageId":"20200422221334.GA571740@coredump.intra.peff.net","threadId":"53289","inReplyTo":"pull.615.git.1587588665.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] credential: handle partial URLs in config settings again","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-22T22:13:34Z","receivedAt":"2020-04-22T22:13:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 22, 2020 at 08:51:02PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> This fixes the problem illustrated by Peff's example\n> [https://lore.kernel.org/git/20200422040644.GC3559880@coredump.intra.peff.net/]\n> , in maint-2.17:\n> \n> $ echo url=https://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> The fix is necessarily different than what was proposed by brian\n> [https://lore.kernel.org/git/20200422012344.2051103-1-sandals@crustytoothpaste.net/] \n> because that fix targets v2.26.x which has 46fd7b390034 (credential: allow\n> wildcard patterns when matching config, 2020-02-20).\n> \n> This patch series targets maint-2.17 instead (and might actually not be able\n> to fix maint due to that wildcard pattern patch; I haven't had the time to\n> check yet).\n\nYes, this will remove the die() in all versions, but in v2.26.0 and up,\nthat config would be silently ignored (and your new test will fail).\n\n> Please note that Git v2.17.4 will not do what we would expect here: if any\n> host name (without protocol) is specified, e.g. -c\n> credential.golli.wog.username = boo, it will actually ignore the host name.\n> That is, this will populate the username:\n> \n> $ echo url=https://example.com |\n> git -c credential.totally.bog.us.username=foo credential fill\n\nThat seems scary. What if it is not .username, but:\n\n [credential \"example.com\"]\n username = foo\n helper = \"!echo password=bar\"\n\n? (Or you can imagine a helper that is pulling from a read-only store,\nlike \"pass\"). That would send the credential to the wrong host.\n\nI think any fix we do here needs to make sure we are not any\nreintroducing wrong-host problems (though I do admit that the usage like\nmy example above is probably way less common than vanilla helpers that\ndo the host-selection themselves).\n\n-Peff\n"},{"id":"395951","messageId":"nycvar.QRO.7.76.6.2004230019170.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"20200422221334.GA571740@coredump.intra.peff.net","subject":"Re: [PATCH 0/3] credential: handle partial URLs in config settings again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-22T22:26:57Z","receivedAt":"2020-04-22T22:27:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 22 Apr 2020, Jeff King wrote:\n\n> On Wed, Apr 22, 2020 at 08:51:02PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > This fixes the problem illustrated by Peff's example\n> > [https://lore.kernel.org/git/20200422040644.GC3559880@coredump.intra.peff.net/]\n> > , in maint-2.17:\n> >\n> > $ echo url=https://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> > The fix is necessarily different than what was proposed by brian\n> > [https://lore.kernel.org/git/20200422012344.2051103-1-sandals@crustytoothpaste.net/]\n> > because that fix targets v2.26.x which has 46fd7b390034 (credential: allow\n> > wildcard patterns when matching config, 2020-02-20).\n> >\n> > This patch series targets maint-2.17 instead (and might actually not be able\n> > to fix maint due to that wildcard pattern patch; I haven't had the time to\n> > check yet).\n>\n> Yes, this will remove the die() in all versions, but in v2.26.0 and up,\n> that config would be silently ignored (and your new test will fail).\n\nThanks for testing!\n\n> > Please note that Git v2.17.4 will not do what we would expect here: if any\n> > host name (without protocol) is specified, e.g. -c\n> > credential.golli.wog.username = boo, it will actually ignore the host name.\n> > That is, this will populate the username:\n> >\n> > $ echo url=https://example.com |\n> > git -c credential.totally.bog.us.username=foo credential fill\n>\n> That seems scary. What if it is not .username, but:\n>\n> [credential \"example.com\"]\n> username = foo\n> helper = \"!echo password=bar\"\n>\n> ? (Or you can imagine a helper that is pulling from a read-only store,\n> like \"pass\"). That would send the credential to the wrong host.\n\nIt would. But I am not aware of any implications regarding `.gitmodules`\n(for some reason I now expect every bug to open an attack vector via\nsubmodules, I wonder why that is), so that's at least good.\n\nThere do seem to be a few projects that set a\n`credential..useHTTPPath` as part of their build, but I doubt\nthat that could be exploited.\n\n> I think any fix we do here needs to make sure we are not any\n> reintroducing wrong-host problems (though I do admit that the usage like\n> my example above is probably way less common than vanilla helpers that\n> do the host-selection themselves).\n\nYes. For the record, I tried to be very careful here. The _only_ code path\nthat is affected by this change is the config reading. Reading\n`.gitmodules` (also in `fsck.c`) should behave identically to the way\nv2.17.5 behaves: they should not allow empty protocol or hostname. If you\nspot any difference in behavior in this regard, please do let me know:\nthat would be a bug.\n\nAnd I do not think that these patches could re-introduce the problems you\nfixed: the `credential_match()` tries to work against a `struct\ncredential` that has been sent to `credential_apply_config()` and as per\nyour fixes, this function now verifies that neither protocol nor hostname\nare unset.\n\nBut again, I would love another pair of eyes on this, to confirm my\nanalysis.\n\nThanks,\nDscho\n"},{"id":"395952","messageId":"xmqqh7xbi28f.fsf@gitster.c.googlers.com","threadId":"53289","inReplyTo":"1081841b16de31693473e72ff817bed5f0064dda.1587588665.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-22T22:29:04Z","receivedAt":"2020-04-22T22:29:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> -\tif (!proto_end || proto_end == url) {\n> +\tif (strict && (!proto_end || proto_end == url)) {\n> \t\tif (!quiet)\n> \t\t\twarning(_(\"url has no scheme: %s\"), url);\n> \t\treturn -1;\n> \t}\n> -\tcp = proto_end + 3;\n> +\tcp = proto_end ? proto_end + 3 : url;\n> \tat = strchr(cp, '@');\n> \tcolon = strchr(cp, ':');\n> \tslash = strchrnul(cp, '/');\n> @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n> \t\thost = at + 1;\n> \t}\n> \n> -\tc->protocol = xmemdupz(url, proto_end - url);\n> -\tc->host = url_decode_mem(host, slash - host);\n> +\tif (proto_end && proto_end - url > 0)\n> +\t\tc->protocol = xmemdupz(url, proto_end - url);\n\nMissing \"proto://\" under non-strict mode would leave c->protocol\nNULL (not \"\") here, as described in [0/3].\n\nHere, slash would be pointing at one of \"/?#\" at the end of the host\nand url would be pointing at...? E.g. for \"http:///path\", URL\npoints at 'h' at the beginning, proto_end points at ':', cp points\nat the last '/' before \"path\" and slash is the same as cp. host\npoints at cp as there is no '@' at sign.\n\n> +\tif (slash - url > 0)\n> +\t\tc->host = url_decode_mem(host, slash - host);\n\nThis wants to make c->host NULL when host is missing, as described\nin [0/3].\n\nShouldn't the condition based on \"slash - host\", though?\n\nOther than that, it looks sensible.\n"},{"id":"395953","messageId":"20200422224711.GC140314@google.com","threadId":"53289","inReplyTo":"nycvar.QRO.7.76.6.2004230019170.18039@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/3] credential: handle partial URLs in config settings again","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-22T22:47:11Z","receivedAt":"2020-04-22T22:47:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJohannes Schindelin wrote:\n> On Wed, 22 Apr 2020, Jeff King wrote:\n>> On Wed, Apr 22, 2020 at 08:51:02PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n>>> Please note that Git v2.17.4 will not do what we would expect here: if any\n>>> host name (without protocol) is specified, e.g. -c\n>>> credential.golli.wog.username = boo, it will actually ignore the host name.\n>>> That is, this will populate the username:\n>>>\n>>> $ echo url=https://example.com |\n>>> git -c credential.totally.bog.us.username=foo credential fill\n>>\n>> That seems scary. What if it is not .username, but:\n>>\n>> [credential \"example.com\"]\n>> username = foo\n>> helper = \"!echo password=bar\"\n>>\n>> ? (Or you can imagine a helper that is pulling from a read-only store,\n>> like \"pass\"). That would send the credential to the wrong host.\n>\n> It would. But I am not aware of any implications regarding `.gitmodules`\n> (for some reason I now expect every bug to open an attack vector via\n> submodules, I wonder why that is), so that's at least good.\n\nSubmodules are only one of many ways that people end up cloning from\nan attacker-controlled URL. In submodules we're careful not to use\n--recurse-submodules by default at clone time. So I'll mentally\nsubsitute \"attacker-controlled URLs\" where you say \"submodules\". ;-)\n\nI agree with Peff's concern about the unpatched state: since there are\npeople who use `[credential \"host.example.com\"] helper` and there are\ncredential helpers that ignore the host passed in, the combination can\nhurt people. (Fortunately, there aren't many credential helpers in\nthat category that are commonly used.)\n\n[...]\n> Yes. For the record, I tried to be very careful here. The _only_ code path\n> that is affected by this change is the config reading.\n[...]\n> But again, I would love another pair of eyes on this, to confirm my\n> analysis.\n\nAs mentioned above, the config reading is sensitive, too. That said,\nI suspect you got it to do basically the right thing.\n\nReading through the patches. Thank you.\n\nJonathan\n"},{"id":"395954","messageId":"nycvar.QRO.7.76.6.2004230042370.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"xmqqh7xbi28f.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-22T22:50:37Z","receivedAt":"2020-04-22T22:51:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 22 Apr 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > -\tif (!proto_end || proto_end == url) {\n> > +\tif (strict && (!proto_end || proto_end == url)) {\n> > \t\tif (!quiet)\n> > \t\t\twarning(_(\"url has no scheme: %s\"), url);\n> > \t\treturn -1;\n> > \t}\n> > -\tcp = proto_end + 3;\n> > +\tcp = proto_end ? proto_end + 3 : url;\n> > \tat = strchr(cp, '@');\n> > \tcolon = strchr(cp, ':');\n> > \tslash = strchrnul(cp, '/');\n> > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n> > \t\thost = at + 1;\n> > \t}\n> >\n> > -\tc->protocol = xmemdupz(url, proto_end - url);\n> > -\tc->host = url_decode_mem(host, slash - host);\n> > +\tif (proto_end && proto_end - url > 0)\n> > +\t\tc->protocol = xmemdupz(url, proto_end - url);\n>\n> Missing \"proto://\" under non-strict mode would leave c->protocol\n> NULL (not \"\") here, as described in [0/3].\n\nRight. I debated whether to set it to `NULL` explicitly, or whether to\nleave it alone. At the end, I settled with the version in v2.17.4.\n\n> Here, slash would be pointing at one of \"/?#\" at the end of the host\n> and url would be pointing at...? E.g. for \"http:///path\", URL\n> points at 'h' at the beginning, proto_end points at ':', cp points\n> at the last '/' before \"path\" and slash is the same as cp. host\n> points at cp as there is no '@' at sign.\n\nIt is not obvious from the diff: `url` is never changed. It would still\npoint at the first `h`, as you said.\n\n> > +\tif (slash - url > 0)\n> > +\t\tc->host = url_decode_mem(host, slash - host);\n>\n> This wants to make c->host NULL when host is missing, as described\n> in [0/3].\n\nI did not describe well enough in 0/3, my apologies.\n\nAgain, I tried to go with the version from v2.17.4 here, i.e. to set\n`c->host` if we get a value, but not set it to `NULL` otherwise (instead\nleave as-is).\n\n> Shouldn't the condition based on \"slash - host\", though?\n\nThat would have been my preferred solution, too. But there is some\nsubtlety at work: for a `url` like `/this/is/my/path`, we want to leave\n`c->host` as-is, but for `cert:///...` we want to set it to \"\" (t0300.23\nand t7416.12 test for this explicitly).\n\nI guess a better condition would be `if (proto_end || slash - host > 0)`.\nWhat do you think?\n\n> Other than that, it looks sensible.\n\nThanks,\nDscho\n"},{"id":"395955","messageId":"xmqqd07zi0p6.fsf@gitster.c.googlers.com","threadId":"53289","inReplyTo":"nycvar.QRO.7.76.6.2004230042370.18039@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-22T23:02:13Z","receivedAt":"2020-04-22T23:02:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> It is not obvious from the diff: `url` is never changed. It would still\n> point at the first `h`, as you said.\n>\n>> > +\tif (slash - url > 0)\n>> > +\t\tc->host = url_decode_mem(host, slash - host);\n> ...\n> I guess a better condition would be `if (proto_end || slash - host > 0)`.\n\nYeah, that would probably be more intuitive to read. What triggered\nmy reaction was the mismatch between \"slash - url\" in the condition\nand \"slash - host\" that specifies the length of the memory region.\n\n\n"},{"id":"395962","messageId":"20200422232912.GD140314@google.com","threadId":"53289","inReplyTo":"2bf781081d9e43f38f2141de023fe918eedcbf5d.1587588665.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] credential: fix grammar","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-22T23:29:12Z","receivedAt":"2020-04-22T23:29:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Schindelin wrote:\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> credential.h | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"395963","messageId":"20200422233854.GE140314@google.com","threadId":"53289","inReplyTo":"1081841b16de31693473e72ff817bed5f0064dda.1587588665.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-22T23:38:54Z","receivedAt":"2020-04-22T23:39:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Schindelin wrote:\n\n> There was one call site, though, that really needed that leniency: when\n> parsing config settings a la `credential.dev.azure.com.useHTTPPath`.\n\nThanks for tackling this.\n\nCan the commit message say a little more about the semantics and when\nsomeone would use this?\n\nIs it a shortcut for\n\n\t[credential \"http://dev.azure.com\"]\n\t\tuseHttpPath = true\n\n\t[credential \"https://dev.azure.com\"]\n\t\tuseHttpPath = true\n\n?\n\n> In preparation for fixing that regression, let's add a parameter called\n> `strict` to the `credential_from_url()` function and convert the\n> existing callers to enforce that strict mode.\n\nI suspect this would be easier to read squashed with patch 3. That\nwould also mean that the functionality and test coverage come at the\nsame time.\n\n[...]\n> diff --git a/credential.c b/credential.c\n> index 64a841eddca..c73260ac40f 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -344,7 +344,7 @@ static int check_url_component(const char *url, int quiet,\n> }\n> \n> int credential_from_url_gently(struct credential *c, const char *url,\n> -\t\t\t int quiet)\n> +\t\t\t int strict, int quiet)\n\nThe collection of flags makes me wonder whether it's time to use a\nsingle \"flags\" parameter with flags that are |ed together. That way,\ncall sites are easier to read without requiring cross-reference\nassistance to see which option each boolean parameter represents.\n\nAlternatively, could the non-strict form be a separate public function\nthat uses the same static helper that takes two boolean args? That is,\nsomething like\n\n\tint credential_from_url_gently(struct credential *c, const char *url,\n\t\t\t\t int quiet)\n\t{\n\t\treturn parse_credential_url(c, url, 1, quiet);\n\t}\n\n\tint credential_from_url_nonstrict(struct credential *c, const char *url,\n\t\t\t\t\t int quiet)\n\t{\n\t\treturn parse_credential_url(c, url, 0, quiet);\n\t}\n\n[...]\n> @@ -357,12 +357,12 @@ int credential_from_url_gently(struct credential *c, const char *url,\n> \t * (3) proto://:@/...\n> \t */\n> \tproto_end = strstr(url, \"://\");\n> -\tif (!proto_end || proto_end == url) {\n> +\tif (strict && (!proto_end || proto_end == url)) {\n> \t\tif (!quiet)\n> \t\t\twarning(_(\"url has no scheme: %s\"), url);\n> \t\treturn -1;\n> \t}\n\nWhen !strict, this means we are not requiring a protocol. No other\ndifference appears to be intended.\n\n[...]\n> @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n> \t\thost = at + 1;\n> \t}\n> \n> -\tc->protocol = xmemdupz(url, proto_end - url);\n> -\tc->host = url_decode_mem(host, slash - host);\n> +\tif (proto_end && proto_end - url > 0)\n> +\t\tc->protocol = xmemdupz(url, proto_end - url);\n\nWhat should happen when the protocol isn't present? Does this mean\ncallers will need to be audited to make sure they handle NULL?\n\n> +\tif (slash - url > 0)\n> +\t\tc->host = url_decode_mem(host, slash - host);\n\nWhat should happen the URL starts with a slash?\n\nThanks,\nJonathan\n"},{"id":"395965","messageId":"20200422235708.GF140314@google.com","threadId":"53289","inReplyTo":"66823c735b1d5ea2047a29656e82fa6fe895f6f1.1587588665.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] credential: handle `credential..` again","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-22T23:57:08Z","receivedAt":"2020-04-22T23:57:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Schindelin wrote:\n\n> In the patches for CVE-2020-11008, the ability to specify credential\n> settings in the config for partial URLs got lost. For example, it used\n> to be possible to specify a credential helper for a specific protocol:\n>\n> \t[credential \"https://\"]\n> \t\thelper = my-https-helper\n>\n> Likewise, it used to be possible to configure settings for a specific\n> host, e.g.:\n>\n> \t[credential \"dev.azure.com\"]\n> \t\tuseHTTPPath = true\n\nAh, I see.\n\n[...]\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -53,7 +53,12 @@ static int credential_config_callback(const char *var, const char *value,\n> \t\tchar *url = xmemdupz(key, dot - key);\n> \t\tint matched;\n> \n> -\t\tcredential_from_url(&want, url);\n> +\t\tif (credential_from_url_gently(&want, url, 0, 0) < 0) {\n> +\t\t\twarning(_(\"skipping credential lookup for url: %s\"), url);\n> +\t\t\tcredential_clear(c);\n\nHm, the error message doesn't seem right here, since `url` is a config\nkey instead of URL whose credential's are being looked up.\n\nShould the error message include the entirety of `var` to make\ndebugging easier?\n\n[...]\n> --- a/t/t0300-credentials.sh\n> +++ b/t/t0300-credentials.sh\n> @@ -448,4 +448,17 @@ test_expect_success 'credential system refuses to work with missing protocol' '\n> \ttest_i18ncmp expect stderr\n> '\n> \n> +test_expect_success 'credential config accepts partial URLs' '\n> +\techo url=https://example.com |\n> +\tgit -c credential.example.com.username=boo \\\n> +\t\tcredential fill >actual &&\n\nCan the tests also check the behavior with bad URLs (that we are\nappropriately tolerating and warning about them?\n\nStepping back: one thing I like about the code in \"master\" that uses\nurlmatch_config_entry is that it is not treating these config keys\nin the same way as URLs that Git would fetch from. For example, if\none of the config keys contains %0a, then that's perfectly fine ---\nwe are not going to write them to a credential helper or to libcurl.\n\nThe only problem is that the pattern matching syntax doesn't match\nthe behavior that users historically expected. (Keeping in mind\nthat we never actually provided the behavior that users expected.\n`credential.example.com.helper` settings applied to all hosts.)\n\nWould it make sense for parsing these config url patterns to share\n*less* code with credential_from_url? Ramifications:\n\n- unless we add specific support for it, we'd lose support for\n patterns that are specific to a user (e.g.\n \"credential.https://user@example.com.helper\").\n\n- likewise, we'd lose support for\n \"credential.https://user:pass@example.com.helper\".\n\n- we could control what \"credential.https:///repo.git.helper\"\n means, for example by rejecting it. When we reject it, the\n error message could be specific to this use case instead of\n shared with other URL handling.\n\n- we wouldn't suport \"credential.example.com/repo.git.helper\"\n by mistake.\n\n- to sum up, we could specifically define exactly what cases we want\n to support:\n\n\t[credential \"example.com\"]\n\t\t# settings for the host\n\t\t...\n\n\t[credential \"user@example.com\"] # maybe\n\t\t# settings for the host/user combination\n\t\t...\n\n\t[credential \"https://\"]\n\t\t# settings for the scheme\n\t\t...\n\n\t[credential \"https://example.com\"]\n\t\t# settings for the host/scheme combination\n\t\t...\n\n\t[credential \"https://example.com/\"]\n\t\t# likewise\n\t\t...\n\n\t[credential \"https://user@example.com\"] # maybe\n\t\t# settings for the host/scheme/user combination\n\t\t...\n\n\t[credential \"https://user@example.com/\"] # maybe\n\t\t# likewise\n\t\t...\n\n\t[credential \"https://example.com/repo.git\"]\n\t\t# settings for the host/scheme/path combination\n\t\t...\n\n\t[credential \"https://user@example.com/repo.git\"] # maybe\n\t\t# settings for the host/scheme/user/path combination\n\t\t...\n\n without accidentally promising support for anything else\n\nWhat do you think?\n\nThanks,\nJonathan\n"},{"id":"395966","messageId":"CAPUEspgJvN6f6Wjo-yjYj-x+bYtC3vdSvwUtrF=MbJDjwYUTdA@mail.gmail.com","threadId":"53289","inReplyTo":"20200422233854.GE140314@google.com","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2020-04-23T00:02:26Z","receivedAt":"2020-04-23T00:02:45Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Apr 22, 2020 at 4:41 PM Jonathan Nieder wrote:\n> Johannes Schindelin wrote:\n> > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n> > host = at + 1;\n> > }\n> >\n> > - c->protocol = xmemdupz(url, proto_end - url);\n> > - c->host = url_decode_mem(host, slash - host);\n> > + if (proto_end && proto_end - url > 0)\n> > + c->protocol = xmemdupz(url, proto_end - url);\n>\n> What should happen when the protocol isn't present? Does this mean\n> callers will need to be audited to make sure they handle NULL?\n\nthe previous code was ensuring protocol was always at least \"\" (albeit it\nmight had been easier to understand with a comment)\n\nremoving the proto_end - url check would have the same effect, but then\nwe will need to also add a explicit xmemdupz(\"\") for the nonstrict part\nor audit (and with certainty add) checks to prevent a NULL protocol to\nintroduce regressions\n\nCarlo\n"},{"id":"396001","messageId":"nycvar.QRO.7.76.6.2004231433060.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"CAPUEspgJvN6f6Wjo-yjYj-x+bYtC3vdSvwUtrF=MbJDjwYUTdA@mail.gmail.com","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-23T13:28:13Z","receivedAt":"2020-04-23T13:28:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Wed, 22 Apr 2020, Carlo Arenas wrote:\n\n> On Wed, Apr 22, 2020 at 4:41 PM Jonathan Nieder wrote:\n> > Johannes Schindelin wrote:\n> > > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n> > > host = at + 1;\n> > > }\n> > >\n> > > - c->protocol = xmemdupz(url, proto_end - url);\n> > > - c->host = url_decode_mem(host, slash - host);\n> > > + if (proto_end && proto_end - url > 0)\n> > > + c->protocol = xmemdupz(url, proto_end - url);\n> >\n> > What should happen when the protocol isn't present? Does this mean\n> > callers will need to be audited to make sure they handle NULL?\n>\n> the previous code was ensuring protocol was always at least \"\" (albeit it\n> might had been easier to understand with a comment)\n\nIf you mean v2.17.5 by \"the previous code\", then yes.\n\nHowever, what I wanted to reinstate was the behavior of v2.17.4, at least\nthe behavior that was obviously intended by this code:\n\n\tint credential_match(const struct credential *want,\n\t\t\t const struct credential *have)\n\t{\n\t#define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))\n\t\treturn CHECK(protocol) &&\n\t\t CHECK(host) &&\n\t\t CHECK(path) &&\n\t\t CHECK(username);\n\t#undef CHECK\n\t}\n\nWhat speaks loudly to me is that _any_ of these can be `NULL` in `want`.\nAny of them. Even protocol.\n\nRemember, the call that I modified here to be more lenient populates that\n`want`, not that `have`. The `have` still uses the strict mode, and that\nis very much by design.\n\nI also saw reports where users try to set `useHTTPPath` for hosts, not for\nURLs, and it kind of makes sense. So I wanted to make that work, too.\n\nExcept that I uncovered the bug during my investigation that v2.17.4\nhandled `credential..useHTTPPath` as if no `` was\nprovided at all!\n\nSo I wanted to fix that bug as well.\n\nWhat I do _not_ want is to enforce `protocol` to be set (falling back to\nan empty string if not specified). See below as to why.\n\n> removing the proto_end - url check would have the same effect, but then\n> we will need to also add a explicit xmemdupz(\"\") for the nonstrict part\n> or audit (and with certainty add) checks to prevent a NULL protocol to\n> introduce regressions\n\nI fear that my patches did not make it clear that the lenient mode is\n_only_ used in the config parsing, in which case we very much do not want\nto have the unspecified parts be interpreted as empty strings.\n\nFor example, if I specify\n\n\t[credential \"http://\"]\n\t\thelper = prevent\n\nto catch _all_ http:// URLs, I definitely do not want that to be used only\nfor URLs with an empty host name!\n\nLikewise, if I specify\n\n\t[credential \"dev.azure.com\"]\n\t\tuseHTTPPath = true\n\nI positively want to use the path part of the URL to specify what\ncredential to use _when accessing http://dev.azure.com/... _or_\nhttps://dev.azure.com/... URLs, I do not expect that setting to be used\nonly for URLs with an empty protocol (which are now forbidden, anyway,\nIIUC)!\n\nSo no, that `xmemdupz(\"\")` would introduce very much undesirable behavior,\nI believe.\n\nCiao,\nDscho\n"},{"id":"396041","messageId":"20200423212212.GA20669@Carlos-MBP","threadId":"53289","inReplyTo":"nycvar.QRO.7.76.6.2004231433060.18039@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-23T21:22:12Z","receivedAt":"2020-04-23T21:22:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Apr 23, 2020 at 03:28:13PM +0200, Johannes Schindelin wrote:\n> On Wed, 22 Apr 2020, Carlo Arenas wrote:\n> > On Wed, Apr 22, 2020 at 4:41 PM Jonathan Nieder wrote:\n> > > Johannes Schindelin wrote:\n> > > > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n> > > > host = at + 1;\n> > > > }\n> > > >\n> > > > - c->protocol = xmemdupz(url, proto_end - url);\n> > > > - c->host = url_decode_mem(host, slash - host);\n> > > > + if (proto_end && proto_end - url > 0)\n> > > > + c->protocol = xmemdupz(url, proto_end - url);\n> > >\n> > > What should happen when the protocol isn't present? Does this mean\n> > > callers will need to be audited to make sure they handle NULL?\n> >\n> > the previous code was ensuring protocol was always at least \"\" (albeit it\n> > might had been easier to understand with a comment)\n> \n> I fear that my patches did not make it clear that the lenient mode is\n> _only_ used in the config parsing, in which case we very much do not want\n> to have the unspecified parts be interpreted as empty strings.\n\nI think the concern raised was that since we are using the same function\nin both cases there might be unintended consequences on changing the\nsemantics for the other case.\n\nthe argument made by Jonathan to use something else for configuration\n(as is done in master) will help in that direction, and might be needed\nanyway as your code it otherwise broken for current maint and master, but\nif not possible (maybe later?) something like this could probably help :\n\ndiff --git a/credential.c b/credential.c\nindex 88612e583c..f972fcc895 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -389,8 +389,9 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \n \tif (proto_end && proto_end - url > 0)\n \t\tc->protocol = xmemdupz(url, proto_end - url);\n-\tif (slash - url > 0)\n+\tif (strict || slash > url)\n \t\tc->host = url_decode_mem(host, slash - host);\n+\n \t/* Trim leading and trailing slashes from path */\n \twhile (*slash == '/')\n \t\tslash++;\n\nchanging the condition there as you suggested to Junio would be a plus IMHO\nas well as getting some test that would excercise the new warning that was\nintroduced in credential.c:57\n\nCarlo\n"},{"id":"396054","messageId":"nycvar.QRO.7.76.6.2004232359120.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"20200423212212.GA20669@Carlos-MBP","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-23T22:03:17Z","receivedAt":"2020-04-23T22:03:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:\n\n> On Thu, Apr 23, 2020 at 03:28:13PM +0200, Johannes Schindelin wrote:\n> > On Wed, 22 Apr 2020, Carlo Arenas wrote:\n> > > On Wed, Apr 22, 2020 at 4:41 PM Jonathan Nieder wrote:\n> > > > Johannes Schindelin wrote:\n> > > > > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n> > > > > host = at + 1;\n> > > > > }\n> > > > >\n> > > > > - c->protocol = xmemdupz(url, proto_end - url);\n> > > > > - c->host = url_decode_mem(host, slash - host);\n> > > > > + if (proto_end && proto_end - url > 0)\n> > > > > + c->protocol = xmemdupz(url, proto_end - url);\n> > > >\n> > > > What should happen when the protocol isn't present? Does this mean\n> > > > callers will need to be audited to make sure they handle NULL?\n> > >\n> > > the previous code was ensuring protocol was always at least \"\" (albeit it\n> > > might had been easier to understand with a comment)\n> >\n> > I fear that my patches did not make it clear that the lenient mode is\n> > _only_ used in the config parsing, in which case we very much do not want\n> > to have the unspecified parts be interpreted as empty strings.\n>\n> I think the concern raised was that since we are using the same function\n> in both cases there might be unintended consequences on changing the\n> semantics for the other case.\n\nIndeed, I share that concern. That's why I wanted to be extra careful\nthere to make sure that introducing this lenient mode does _not_ change\nthe non-lenient mode in the least, i.e. it is the reason why I kept 2/3\nseparate from 3/3.\n\n> the argument made by Jonathan to use something else for configuration\n> (as is done in master) will help in that direction, and might be needed\n> anyway as your code it otherwise broken for current maint and master,\n\nI am in general not a fan of the idea to have two separate parsers for\nessentially the same thing. In this instance, the difference between the\nlenient mode and the non-lenient mode is so obvious that I'd rather reuse\nthe same code (think: Don't Repeat Yourself).\n\n> but if not possible (maybe later?) something like this could probably\n> help :\n>\n> diff --git a/credential.c b/credential.c\n> index 88612e583c..f972fcc895 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -389,8 +389,9 @@ int credential_from_url_gently(struct credential *c, const char *url,\n>\n> \tif (proto_end && proto_end - url > 0)\n> \t\tc->protocol = xmemdupz(url, proto_end - url);\n> -\tif (slash - url > 0)\n> +\tif (strict || slash > url)\n> \t\tc->host = url_decode_mem(host, slash - host);\n> +\n> \t/* Trim leading and trailing slashes from path */\n> \twhile (*slash == '/')\n> \t\tslash++;\n>\n> changing the condition there as you suggested to Junio would be a plus IMHO\n> as well as getting some test that would excercise the new warning that was\n> introduced in credential.c:57\n\nYes (modulo doing \"greater than\" comparison on pointers which is IIRC not\npermitted in C in general).\n\nCiao,\nDscho\n"},{"id":"396056","messageId":"xmqqy2qlette.fsf@gitster.c.googlers.com","threadId":"53289","inReplyTo":"nycvar.QRO.7.76.6.2004232359120.18039@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-23T22:11:25Z","receivedAt":"2020-04-23T22:11:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Yes (modulo doing \"greater than\" comparison on pointers which is IIRC not\n> permitted in C in general).\n\nOf course, people write a loop like this\n\n\tchar *cp, *ep = strchr(string, '\\n');\n\n\tfor (cp = string; cp < ep; cp++)\n\t\t...\n\nall the time, and forbidding pointer comparison would make the\nlanguage impossible to use ;-)\n\nI think you are confused with a different rule---what is not kosher\nis to compare two pointers that do not point into elements of the\nsame array. Whether the comparison is done in (ptr1 < ptr2) way, or\n(ptr2 - ptr1 < 0) way, does not change the equation.\n"},{"id":"396057","messageId":"nycvar.QRO.7.76.6.2004240006510.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"20200422224711.GC140314@google.com","subject":"Re: [PATCH 0/3] credential: handle partial URLs in config settings again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-23T22:11:54Z","receivedAt":"2020-04-23T22:12:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Wed, 22 Apr 2020, Jonathan Nieder wrote:\n\n> Johannes Schindelin wrote:\n> > On Wed, 22 Apr 2020, Jeff King wrote:\n> >> On Wed, Apr 22, 2020 at 08:51:02PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> >>> Please note that Git v2.17.4 will not do what we would expect here: if any\n> >>> host name (without protocol) is specified, e.g. -c\n> >>> credential.golli.wog.username = boo, it will actually ignore the host name.\n> >>> That is, this will populate the username:\n> >>>\n> >>> $ echo url=https://example.com |\n> >>> git -c credential.totally.bog.us.username=foo credential fill\n> >>\n> >> That seems scary. What if it is not .username, but:\n> >>\n> >> [credential \"example.com\"]\n> >> username = foo\n> >> helper = \"!echo password=bar\"\n> >>\n> >> ? (Or you can imagine a helper that is pulling from a read-only store,\n> >> like \"pass\"). That would send the credential to the wrong host.\n> >\n> > It would. But I am not aware of any implications regarding `.gitmodules`\n> > (for some reason I now expect every bug to open an attack vector via\n> > submodules, I wonder why that is), so that's at least good.\n>\n> Submodules are only one of many ways that people end up cloning from\n> an attacker-controlled URL.\n\nYet the vast majority of the vulnerabilities in Git that we fixed over the\nlast years has involved submodules in their attack vector.\n\n> In submodules we're careful not to use --recurse-submodules by default\n> at clone time. So I'll mentally subsitute \"attacker-controlled URLs\"\n> where you say \"submodules\". ;-)\n\nSay what you will about this, but the practical reality seems to be that\nmost of the security bugs in Git affected submodule users.\n\nSomebody even muttered the suggestion to me that submodules should be\ndeprecated already.\n\n> I agree with Peff's concern about the unpatched state: since there are\n> people who use `[credential \"host.example.com\"] helper` and there are\n> credential helpers that ignore the host passed in, the combination can\n> hurt people. (Fortunately, there aren't many credential helpers in\n> that category that are commonly used.)\n\nYes. That's why I want to make sure that the URL parser we use here is\nstrict by default, and lenient _only_ when parsing the config (because\nthen, it is more like a URL *pattern* used for matching, the parsed URL is\nnever used directly).\n\n> [...]\n> > Yes. For the record, I tried to be very careful here. The _only_ code path\n> > that is affected by this change is the config reading.\n> [...]\n> > But again, I would love another pair of eyes on this, to confirm my\n> > analysis.\n>\n> As mentioned above, the config reading is sensitive, too. That said,\n> I suspect you got it to do basically the right thing.\n>\n> Reading through the patches. Thank you.\n\nThank you very much!\nDscho\n"},{"id":"396059","messageId":"xmqqtv19etkx.fsf@gitster.c.googlers.com","threadId":"53289","inReplyTo":"xmqqy2qlette.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-23T22:16:30Z","receivedAt":"2020-04-23T22:16:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> Yes (modulo doing \"greater than\" comparison on pointers which is IIRC not\n>> permitted in C in general).\n>\n> Of course, people write a loop like this\n>\n> \tchar *cp, *ep = strchr(string, '\\n');\n>\n> \tfor (cp = string; cp < ep; cp++)\n> \t\t...\n>\n> all the time, and forbidding pointer comparison would make the\n> language impossible to use ;-)\n>\n> I think you are confused with a different rule---what is not kosher\n> is to compare two pointers that do not point into elements of the\n> same array. Whether the comparison is done in (ptr1 < ptr2) way, or\n> (ptr2 - ptr1 < 0) way, does not change the equation.\n\nHaving said that, between\n\n1.\tif (strict || slash - url > 0)\n2.\tif (strict || slash > url)\n \t\tc->host = url_decode_mem(host, slash - host);\n\nI think the former is moderately easier to read. It still has the\nsame \"Huh?\" factor that a comparison between slash and URL guards\nthe size of the region being decoded, which is slash - host, and\nmakes the reader wonder how these two variables, URL and host,\nrelate to each other at this point in the code, though, either way\nthe comparison is spelled.\n\nThanks.\n"},{"id":"396061","messageId":"nycvar.QRO.7.76.6.2004240032520.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"20200422233854.GE140314@google.com","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-23T22:50:48Z","receivedAt":"2020-04-23T22:51:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Wed, 22 Apr 2020, Jonathan Nieder wrote:\n\n> Johannes Schindelin wrote:\n>\n> > There was one call site, though, that really needed that leniency: when\n> > parsing config settings a la `credential.dev.azure.com.useHTTPPath`.\n>\n> Thanks for tackling this.\n>\n> Can the commit message say a little more about the semantics and when\n> someone would use this?\n>\n> Is it a shortcut for\n>\n> \t[credential \"http://dev.azure.com\"]\n> \t\tuseHttpPath = true\n>\n> \t[credential \"https://dev.azure.com\"]\n> \t\tuseHttpPath = true\n>\n> ?\n\nI don't really want to be too verbose about this in _this_ commit message.\nBut I do see your point. This is my current version of the commit message:\n\n credential: optionally allow partial URLs in credential_from_url_gently()\n\n Prior to the fixes for CVE-2020-11008, we were _very_ lenient in what we\n required from a URL in order to parse it into a `struct credential`.\n That led to serious vulnerabilities.\n\n There was one call site, though, that really needed that leniency: when\n parsing config settings a la `credential.dev.azure.com.useHTTPPath`.\n Settings like this might be desired when users want to use, say, a given\n user name on a given host, regardless of the protocol to be used.\n\n In preparation for fixing that bug, let's add a parameter called\n `allow_partial_url` to the `credential_from_url_gently()` function and\n convert the existing callers to set that parameter to 0, i.e. do not\n change the existing behavior, just add the option to be more lenient.\n\n Please note that this patch does more than just reinstating a way to\n imitate the behavior before those CVE-2020-11008 fixes: Before that, we\n would simply ignore URLs without a protocol. In other words,\n misleadingly, the following setting would be applied to _all_ URLs:\n\n [credential \"example.com\"]\n username = that-me\n\n The obvious intention is to match the host name only. With this patch,\n we allow precisely that: when parsing the URL with non-zero\n `allow_partial_url`, we do not simply return success if there was no\n protocol, but we simply leave the protocol unset and continue parsing\n the URL.\n\nAs you see, I added substantially more information there.\n\n> > In preparation for fixing that regression, let's add a parameter\n> > called `strict` to the `credential_from_url()` function and convert\n> > the existing callers to enforce that strict mode.\n>\n> I suspect this would be easier to read squashed with patch 3. That\n> would also mean that the functionality and test coverage come at the\n> same time.\n\nOn the contrary, I _really_ want them to be separate, so that it is easier\nto review. I know that _I_ had an easier time looking over the patch that\nintroduces the non-strict mode, to make sure that it does not change a\nthing in strict mode (and yes, v1 made that harder than necessary by _not_\nusing `strict ||` in the condition that guards the `c->host` assignment).\n\nIn short: it is my intention to keep the two patches separate, with the\nmain goal to make sure that I don't introduce any of those regressions\nPeff is worried about.\n\n> [...]\n> > diff --git a/credential.c b/credential.c\n> > index 64a841eddca..c73260ac40f 100644\n> > --- a/credential.c\n> > +++ b/credential.c\n> > @@ -344,7 +344,7 @@ static int check_url_component(const char *url, int quiet,\n> > }\n> >\n> > int credential_from_url_gently(struct credential *c, const char *url,\n> > -\t\t\t int quiet)\n> > +\t\t\t int strict, int quiet)\n>\n> The collection of flags makes me wonder whether it's time to use a\n> single \"flags\" parameter with flags that are |ed together. That way,\n> call sites are easier to read without requiring cross-reference\n> assistance to see which option each boolean parameter represents.\n\nI resisted the temptation because I don't want this to be a big patch\nseries, but a fast one. The reports keep coming in, and the current plan\nseems to be for GitHub Desktop to release another version on Monday, for\nwhich I need to release a MinGit backport probably tomorrow, otherwise it\nwon't work timewise.\n\nMeaning: I don't really want to do \"nice to have\" work like this flag.\n\n> Alternatively, could the non-strict form be a separate public function\n> that uses the same static helper that takes two boolean args? That is,\n> something like\n>\n> \tint credential_from_url_gently(struct credential *c, const char *url,\n> \t\t\t\t int quiet)\n> \t{\n> \t\treturn parse_credential_url(c, url, 1, quiet);\n> \t}\n>\n> \tint credential_from_url_nonstrict(struct credential *c, const char *url,\n> \t\t\t\t\t int quiet)\n> \t{\n> \t\treturn parse_credential_url(c, url, 0, quiet);\n> \t}\n\nLooks good, but there are only three callers (and I don't expect more).\nAnd the only caller interested in the lenient mode (read: the only\nnow-complicated call) lives in the very same file as the called function.\nSo it strikes me as quite a bit of an overkill to introduce two new\nfunctions.\n\n> [...]\n> > @@ -357,12 +357,12 @@ int credential_from_url_gently(struct credential *c, const char *url,\n> > \t * (3) proto://:@/...\n> > \t */\n> > \tproto_end = strstr(url, \"://\");\n> > -\tif (!proto_end || proto_end == url) {\n> > +\tif (strict && (!proto_end || proto_end == url)) {\n> > \t\tif (!quiet)\n> > \t\t\twarning(_(\"url has no scheme: %s\"), url);\n> > \t\treturn -1;\n> > \t}\n>\n> When !strict, this means we are not requiring a protocol. No other\n> difference appears to be intended.\n\nAlmost. My intention was to handle missing host names, too: `/repo.git`\nshould also match `https://example.com/repo.git`.\n\nThe user name is optional already, anyway.\n\nBTW I realized (while working on these patches) that it would probably be\na good idea to warn about passwords in these `credential..`\nsettings: the password will be ignored as far as `credential_match()` is\nconcerned, and they should probably not live in the config.\n\nBut for aforementioned reasons, I decided against including a patch that\nmakes that happen.\n\n> [...]\n> > @@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n> > \t\thost = at + 1;\n> > \t}\n> >\n> > -\tc->protocol = xmemdupz(url, proto_end - url);\n> > -\tc->host = url_decode_mem(host, slash - host);\n> > +\tif (proto_end && proto_end - url > 0)\n> > +\t\tc->protocol = xmemdupz(url, proto_end - url);\n>\n> What should happen when the protocol isn't present? Does this mean\n> callers will need to be audited to make sure they handle NULL?\n\nAgain, the sole caller of this lenient mode is the config parser which\nthen wants to match the (partial) URL against the actual URL, using this\nfunction:\n\n\tint credential_match(const struct credential *want,\n\t\t\t const struct credential *have)\n\t{\n\t#define CHECK(x) (!want->x || (have->x && !strcmp(want->x, have->x)))\n\t\treturn CHECK(protocol) &&\n\t\t CHECK(host) &&\n\t\t CHECK(path) &&\n\t\t CHECK(username);\n\t#undef CHECK\n\t}\n\nThe lenient parser is supposed to populate the `want` of this function. In\nother words, the code _expects_ `protocol` (or for that matter, _any_\nattribute of `want`) to be optional.\n\n> > +\tif (slash - url > 0)\n> > +\t\tc->host = url_decode_mem(host, slash - host);\n>\n> What should happen the URL starts with a slash?\n\nOh... I thought it was obvious that a partial URL starting with a slash\nwould refer to the path part only... Does that not make sense?\n\nCiao,\nDscho\n"},{"id":"396063","messageId":"nycvar.QRO.7.76.6.2004240057220.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"20200422235708.GF140314@google.com","subject":"Re: [PATCH 3/3] credential: handle `credential..` again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-23T23:19:34Z","receivedAt":"2020-04-23T23:19:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Wed, 22 Apr 2020, Jonathan Nieder wrote:\n\n> Johannes Schindelin wrote:\n>\n> [...]\n> > --- a/credential.c\n> > +++ b/credential.c\n> > @@ -53,7 +53,12 @@ static int credential_config_callback(const char *var, const char *value,\n> > \t\tchar *url = xmemdupz(key, dot - key);\n> > \t\tint matched;\n> >\n> > -\t\tcredential_from_url(&want, url);\n> > +\t\tif (credential_from_url_gently(&want, url, 0, 0) < 0) {\n> > +\t\t\twarning(_(\"skipping credential lookup for url: %s\"), url);\n> > +\t\t\tcredential_clear(c);\n>\n> Hm, the error message doesn't seem right here, since `url` is a config\n> key instead of URL whose credential's are being looked up.\n>\n> Should the error message include the entirety of `var` to make\n> debugging easier?\n\nI suppose you're right.\n\nBTW I just realized a much worse issue: `credential_clear(c);`. This\nclears the wrong struct. It should clear `want`, not `c`.\n\n> [...]\n> > --- a/t/t0300-credentials.sh\n> > +++ b/t/t0300-credentials.sh\n> > @@ -448,4 +448,17 @@ test_expect_success 'credential system refuses to work with missing protocol' '\n> > \ttest_i18ncmp expect stderr\n> > '\n> >\n> > +test_expect_success 'credential config accepts partial URLs' '\n> > +\techo url=https://example.com |\n> > +\tgit -c credential.example.com.username=boo \\\n> > +\t\tcredential fill >actual &&\n>\n> Can the tests also check the behavior with bad URLs (that we are\n> appropriately tolerating and warning about them?\n\nYes, my bad! The next iteration will have a test for that.\n\n> Stepping back: one thing I like about the code in \"master\" that uses\n> urlmatch_config_entry is that it is not treating these config keys\n> in the same way as URLs that Git would fetch from. For example, if\n> one of the config keys contains %0a, then that's perfectly fine ---\n> we are not going to write them to a credential helper or to libcurl.\n\nThat is actually not what the code does, at least not in `maint-2.17`. It\nvery much warns about `%0a` in config keys. The test I am adding to verify\nthat the warning above is exercised correctly looks like this:\n\n\tgit -c credential.$partial.helper=yep \\\n -c credential.with%0anewline.username=uh-oh \\\n credential fill >output 2>error &&\n test_i18ngrep \"skipping credential lookup for url\" error\n\nThat is literally the only way I get `credential_from_url_gently()` to\nreturn `-1` in the lenient mode.\n\n> The only problem is that the pattern matching syntax doesn't match\n> the behavior that users historically expected. (Keeping in mind\n> that we never actually provided the behavior that users expected.\n> `credential.example.com.helper` settings applied to all hosts.)\n\nYes, I fix that, too. It is a bad usability bug, in my eyes, and I think\nit is better to fix it while I'm in the space anyway.\n\n> Would it make sense for parsing these config url patterns to share\n> *less* code with credential_from_url? Ramifications:\n>\n> - unless we add specific support for it, we'd lose support for\n> patterns that are specific to a user (e.g.\n> \"credential.https://user@example.com.helper\").\n>\n> - likewise, we'd lose support for\n> \"credential.https://user:pass@example.com.helper\".\n>\n> - we could control what \"credential.https:///repo.git.helper\"\n> means, for example by rejecting it. When we reject it, the\n> error message could be specific to this use case instead of\n> shared with other URL handling.\n>\n> - we wouldn't suport \"credential.example.com/repo.git.helper\"\n> by mistake.\n\nI think we can have _all_ of this _without_ violating the DRY principle.\n\nRemember: my main motivation for keeping 2/3 apart from 3/3 was so that it\nis really easy to verify that 2/3 does not break the callers that _need_\nthe strict mode.\n\nAnd since that is the case, we can then enjoy the benefit of the shared\ncode for the one caller that wants to match also partial URLs.\n\n> - to sum up, we could specifically define exactly what cases we want\n> to support:\n>\n> \t[credential \"example.com\"]\n> \t\t# settings for the host\n> \t\t...\n>\n> \t[credential \"user@example.com\"] # maybe\n> \t\t# settings for the host/user combination\n> \t\t...\n>\n> \t[credential \"https://\"]\n> \t\t# settings for the scheme\n> \t\t...\n>\n> \t[credential \"https://example.com\"]\n> \t\t# settings for the host/scheme combination\n> \t\t...\n>\n> \t[credential \"https://example.com/\"]\n> \t\t# likewise\n> \t\t...\n>\n> \t[credential \"https://user@example.com\"] # maybe\n> \t\t# settings for the host/scheme/user combination\n> \t\t...\n>\n> \t[credential \"https://user@example.com/\"] # maybe\n> \t\t# likewise\n> \t\t...\n>\n> \t[credential \"https://example.com/repo.git\"]\n> \t\t# settings for the host/scheme/path combination\n> \t\t...\n>\n> \t[credential \"https://user@example.com/repo.git\"] # maybe\n> \t\t# settings for the host/scheme/user/path combination\n> \t\t...\n>\n> without accidentally promising support for anything else\n>\n> What do you think?\n\nI added tests for all of those. They all work as a naive user (like me)\nwould expect them to.\n\nI threw in another test, too:\n\n\t[credential \"/repo.git\"]\n\t\t...\n\nAnd I also threw in negative tests, to verify that non-matching protocol\nor host name or path mean that the config setting is ignored.\n\nThank you for your thorough review, it really helps me,\nDscho\n"},{"id":"396064","messageId":"nycvar.QRO.7.76.6.2004240121180.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"xmqqtv19etkx.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/3] credential: teach `credential_from_url()` a non-strict mode","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-23T23:22:38Z","receivedAt":"2020-04-23T23:22:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 23 Apr 2020, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> > Johannes Schindelin writes:\n> >\n> >> Yes (modulo doing \"greater than\" comparison on pointers which is IIRC not\n> >> permitted in C in general).\n> >\n> > Of course, people write a loop like this\n> >\n> > \tchar *cp, *ep = strchr(string, '\\n');\n> >\n> > \tfor (cp = string; cp < ep; cp++)\n> > \t\t...\n> >\n> > all the time, and forbidding pointer comparison would make the\n> > language impossible to use ;-)\n> >\n> > I think you are confused with a different rule---what is not kosher\n> > is to compare two pointers that do not point into elements of the\n> > same array. Whether the comparison is done in (ptr1 < ptr2) way, or\n> > (ptr2 - ptr1 < 0) way, does not change the equation.\n\nYep, that's my confusion all right.\n\n> Having said that, between\n>\n> 1.\tif (strict || slash - url > 0)\n> 2.\tif (strict || slash > url)\n> \t\tc->host = url_decode_mem(host, slash - host);\n>\n> I think the former is moderately easier to read. It still has the\n> same \"Huh?\" factor that a comparison between slash and URL guards\n> the size of the region being decoded, which is slash - host, and\n> makes the reader wonder how these two variables, URL and host,\n> relate to each other at this point in the code, though, either way\n> the comparison is spelled.\n\nI fully agree! That's why I use `strict || slash - host > 0` in my next\niteration (actually, I decided to rename `strict`, but that's beside the\npoint).\n\nCiao,\nDscho\n"},{"id":"396066","messageId":"fc772d21b7472afc6ad862ff30ec4be91f0e8d80.1587685397.git.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":"pull.615.v2.git.1587685397.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] credential: optionally allow partial URLs in credential_from_url_gently()","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-23T23:43:16Z","receivedAt":"2020-04-23T23:43:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nPrior to the fixes for CVE-2020-11008, we were _very_ lenient in what we\nrequired from a URL in order to parse it into a `struct credential`.\nThat led to serious vulnerabilities.\n\nThere was one call site, though, that really needed that leniency: when\nparsing config settings a la `credential.dev.azure.com.useHTTPPath`.\nSettings like this might be desired when users want to use, say, a given\nuser name on a given host, regardless of the protocol to be used.\n\nIn preparation for fixing that bug, let's add a parameter called\n`allow_partial_url` to the `credential_from_url_gently()` function and\nconvert the existing callers to set that parameter to 0, i.e. do not\nchange the existing behavior, just add the option to be more lenient.\n\nPlease note that this patch does more than just reinstating a way to\nimitate the behavior before those CVE-2020-11008 fixes: Before that, we\nwould simply ignore URLs without a protocol. In other words,\nmisleadingly, the following setting would be applied to _all_ URLs:\n\n\t[credential \"example.com\"]\n\t\tusername = that-me\n\nThe obvious intention is to match the host name only. With this patch,\nwe allow precisely that: when parsing the URL with non-zero\n`allow_partial_url`, we do not simply return success if there was no\nprotocol, but we simply leave the protocol unset and continue parsing\nthe URL.\n\nSigned-off-by: Johannes Schindelin \n---\n credential.c | 14 ++++++++------\n credential.h | 26 +++++++++++++++++++++++++-\n fsck.c | 2 +-\n 3 files changed, 34 insertions(+), 8 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 64a841eddca..52965a5122c 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -344,7 +344,7 @@ static int check_url_component(const char *url, int quiet,\n }\n \n int credential_from_url_gently(struct credential *c, const char *url,\n-\t\t\t int quiet)\n+\t\t\t int allow_partial_url, int quiet)\n {\n \tconst char *at, *colon, *cp, *slash, *host, *proto_end;\n \n@@ -357,12 +357,12 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \t * (3) proto://:@/...\n \t */\n \tproto_end = strstr(url, \"://\");\n-\tif (!proto_end || proto_end == url) {\n+\tif (!allow_partial_url && (!proto_end || proto_end == url)) {\n \t\tif (!quiet)\n \t\t\twarning(_(\"url has no scheme: %s\"), url);\n \t\treturn -1;\n \t}\n-\tcp = proto_end + 3;\n+\tcp = proto_end ? proto_end + 3 : url;\n \tat = strchr(cp, '@');\n \tcolon = strchr(cp, ':');\n \tslash = strchrnul(cp, '/');\n@@ -382,8 +382,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \t\thost = at + 1;\n \t}\n \n-\tc->protocol = xmemdupz(url, proto_end - url);\n-\tc->host = url_decode_mem(host, slash - host);\n+\tif (proto_end && proto_end - url > 0)\n+\t\tc->protocol = xmemdupz(url, proto_end - url);\n+\tif (!allow_partial_url || slash - host > 0)\n+\t\tc->host = url_decode_mem(host, slash - host);\n \t/* Trim leading and trailing slashes from path */\n \twhile (*slash == '/')\n \t\tslash++;\n@@ -407,6 +409,6 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \n void credential_from_url(struct credential *c, const char *url)\n {\n-\tif (credential_from_url_gently(c, url, 0) < 0)\n+\tif (credential_from_url_gently(c, url, 0, 0) < 0)\n \t\tdie(_(\"credential url cannot be parsed: %s\"), url);\n }\ndiff --git a/credential.h b/credential.h\nindex 5a86502d95c..5abc865b95d 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -41,9 +41,33 @@ void credential_write(const struct credential *, FILE *);\n * an error but leave the broken state in the credential object for further\n * examination. The non-gentle form will issue a warning to stderr and return\n * an empty credential.\n+ *\n+ * If allow_partial_url is non-zero, partial URLs are allowed, i.e. it can, but\n+ * does not have to, contain\n+ *\n+ * - a protocol (or scheme) of the form \"://\"\n+ *\n+ * - a host name (the part after the protocol and before the first slash after\n+ * that, if any)\n+ *\n+ * - a user name and potentially a password (as \"[:]@\" part of\n+ * the host name)\n+ *\n+ * - a path (the part after the host name, if any, starting with the slash)\n+ *\n+ * Missing parts will be left unset in `struct credential`. Thus, `https://`\n+ * will have only the `protocol` set, `example.com` only the host name, and\n+ * `/git` only the path.\n+ *\n+ * Note that an empty host name in an otherwise fully-qualified URL will be\n+ * treated as unset when allow_partial_url is non-zero (and only then,\n+ * otherwise it is treated as the empty string).\n+ *\n+ * The credential_from_url() function does not allow partial URLs.\n */\n void credential_from_url(struct credential *, const char *url);\n-int credential_from_url_gently(struct credential *, const char *url, int quiet);\n+int credential_from_url_gently(struct credential *, const char *url,\n+\t\t\t int allow_partial_url, int quiet);\n \n int credential_match(const struct credential *have,\n \t\t const struct credential *want);\ndiff --git a/fsck.c b/fsck.c\nindex 31b5be05f54..cec97b63901 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1076,7 +1076,7 @@ static int check_submodule_url(const char *url)\n \telse if (url_to_curl_url(url, &curl_url)) {\n \t\tstruct credential c = CREDENTIAL_INIT;\n \t\tint ret = 0;\n-\t\tif (credential_from_url_gently(&c, curl_url, 1) ||\n+\t\tif (credential_from_url_gently(&c, curl_url, 0, 1) ||\n \t\t !*c.host)\n \t\t\tret = -1;\n \t\tcredential_clear(&c);\n-- \ngitgitgadget\n\n"},{"id":"396067","messageId":"pull.615.v2.git.1587685397.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":"pull.615.git.1587588665.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] credential: handle partial URLs in config settings again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-23T23:43:14Z","receivedAt":"2020-04-23T23:43:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This fixes the problem illustrated by Peff's example\n[https://lore.kernel.org/git/20200422040644.GC3559880@coredump.intra.peff.net/]\n, in maint-2.17:\n\n $ echo url=https://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\nThe fix is necessarily different than what was proposed by brian\n[https://lore.kernel.org/git/20200422012344.2051103-1-sandals@crustytoothpaste.net/] \nbecause that fix targets v2.26.x which has 46fd7b390034 (credential: allow\nwildcard patterns when matching config, 2020-02-20).\n\nThis patch series targets maint-2.17 instead (and might actually not be able\nto fix maint due to that wildcard pattern patch; I haven't had the time to\ncheck yet).\n\nPlease note that Git v2.17.4 will not do what we would expect here: if any\nhost name (without protocol) is specified, e.g. -c\ncredential.golli.wog.username = boo, it will actually ignore the host name.\nThat is, this will populate the username:\n\n $ echo url=https://example.com |\n git -c credential.totally.bog.us.username=foo credential fill\n\nObviously, this is unexpected, as a Git config like this would leave the\nlast specified user name as \"winner\":\n\n[credential \"first.com\"]\n username = Whos On\n[credential \"second.com\"]\n username = Who\n\nThis patch series fixes this. The quoted part of [credential \"\"] will\nbe interpreted as a partial URL:\n\n * It can start with a protocol followed by ://, but does not have to.\n * If it starts with a protocol, the host name will always be set (if the \n :// is followed immediately by yet another slash, then it will be set to\n the empty string).\n * If it starts without a protocol, it is treated as a path if the value\n starts with a slash (and the host will be left unset).\n * If it starts without a protocol and the first character is not a slash,\n it will be treated as a host name, optionally followed by a slash and the\n path.\n\nChanges since v1:\n\n * The condition when the c->host field is set was made more intuitive.\n * The \"fix grammar\" commit now has Jonathan's \"reviewed-by\" footer.\n * Inverted the meaning of the parameter strict and renamed it to \n allow_partial_urls, to clarify its role.\n * Enhanced the commit message of the second patch to illustrate the\n motivation for the lenient mode a bit better.\n * [Not a change] I did leave the second and the third patch separate from\n one another. This makes it a lot easier to follow the iteration and to\n keep the reviews straight: it separates the \"how do we make URL parts\n optional?\" from the \"where do we need URL parts to be optional?\"\n * A partial URL https:// is now correctly interpreted as having only the\n protocol set, but not host name nor path.\n * The lenient mode is now explained a lot more verbosely in the code\n comment in credential.h.\n * When skipping a config setting, we now show the config key, not just the\n URL (which might be incomplete, i.e. not actually a URL).\n * When skipping a config setting, the correct struct credential is cleared\n (i.e. the just-parsed one, not the one against which we wanted to match\n the just-parsed one).\n * Added a ton more partial URLs to the test, and the test now also verifies\n that the warning comes out all right.\n\nJohannes Schindelin (3):\n credential: fix grammar\n credential: optionally allow partial URLs in\n credential_from_url_gently()\n credential: handle `credential..` again\n\n credential.c | 22 +++++++++++++++-------\n credential.h | 28 ++++++++++++++++++++++++++--\n fsck.c | 2 +-\n t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 4 files changed, 81 insertions(+), 10 deletions(-)\n\n\nbase-commit: df5be6dc3fd18c294ec93a9af0321334e3f92c9c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-615%2Fdscho%2Fcredential-config-partial-url-maint-2.17-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-615/dscho/credential-config-partial-url-maint-2.17-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/615\n\nRange-diff vs v1:\n\n 1: 2bf781081d9 ! 1: 2c1c0ae91eb credential: fix grammar\n @@ Commit message\n possible solutions were discussed. Grammar was not a primary focus,\n that's why this slipped in.\n \n + Reviewed-by: Jonathan Nieder \n Signed-off-by: Johannes Schindelin \n \n ## credential.h ##\n 2: 1081841b16d ! 2: fc772d21b74 credential: teach `credential_from_url()` a non-strict mode\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - credential: teach `credential_from_url()` a non-strict mode\n + credential: optionally allow partial URLs in credential_from_url_gently()\n \n Prior to the fixes for CVE-2020-11008, we were _very_ lenient in what we\n required from a URL in order to parse it into a `struct credential`.\n @@ Commit message\n \n There was one call site, though, that really needed that leniency: when\n parsing config settings a la `credential.dev.azure.com.useHTTPPath`.\n + Settings like this might be desired when users want to use, say, a given\n + user name on a given host, regardless of the protocol to be used.\n \n - In preparation for fixing that regression, let's add a parameter called\n - `strict` to the `credential_from_url()` function and convert the\n - existing callers to enforce that strict mode.\n + In preparation for fixing that bug, let's add a parameter called\n + `allow_partial_url` to the `credential_from_url_gently()` function and\n + convert the existing callers to set that parameter to 0, i.e. do not\n + change the existing behavior, just add the option to be more lenient.\n +\n + Please note that this patch does more than just reinstating a way to\n + imitate the behavior before those CVE-2020-11008 fixes: Before that, we\n + would simply ignore URLs without a protocol. In other words,\n + misleadingly, the following setting would be applied to _all_ URLs:\n +\n + [credential \"example.com\"]\n + username = that-me\n +\n + The obvious intention is to match the host name only. With this patch,\n + we allow precisely that: when parsing the URL with non-zero\n + `allow_partial_url`, we do not simply return success if there was no\n + protocol, but we simply leave the protocol unset and continue parsing\n + the URL.\n \n Signed-off-by: Johannes Schindelin \n \n @@ credential.c: static int check_url_component(const char *url, int quiet,\n \n int credential_from_url_gently(struct credential *c, const char *url,\n -\t\t\t int quiet)\n -+\t\t\t int strict, int quiet)\n ++\t\t\t int allow_partial_url, int quiet)\n {\n \tconst char *at, *colon, *cp, *slash, *host, *proto_end;\n \n @@ credential.c: int credential_from_url_gently(struct credential *c, const char *u\n \t */\n \tproto_end = strstr(url, \"://\");\n -\tif (!proto_end || proto_end == url) {\n -+\tif (strict && (!proto_end || proto_end == url)) {\n ++\tif (!allow_partial_url && (!proto_end || proto_end == url)) {\n \t\tif (!quiet)\n \t\t\twarning(_(\"url has no scheme: %s\"), url);\n \t\treturn -1;\n @@ credential.c: int credential_from_url_gently(struct credential *c, const char *u\n -\tc->host = url_decode_mem(host, slash - host);\n +\tif (proto_end && proto_end - url > 0)\n +\t\tc->protocol = xmemdupz(url, proto_end - url);\n -+\tif (slash - url > 0)\n ++\tif (!allow_partial_url || slash - host > 0)\n +\t\tc->host = url_decode_mem(host, slash - host);\n \t/* Trim leading and trailing slashes from path */\n \twhile (*slash == '/')\n @@ credential.c: int credential_from_url_gently(struct credential *c, const char *u\n void credential_from_url(struct credential *c, const char *url)\n {\n -\tif (credential_from_url_gently(c, url, 0) < 0)\n -+\tif (credential_from_url_gently(c, url, 1, 0) < 0)\n ++\tif (credential_from_url_gently(c, url, 0, 0) < 0)\n \t\tdie(_(\"credential url cannot be parsed: %s\"), url);\n }\n \n @@ credential.h: void credential_write(const struct credential *, FILE *);\n * examination. The non-gentle form will issue a warning to stderr and return\n * an empty credential.\n + *\n -+ * In strict mode, an empty protocol or an empty host name are not allowed.\n -+ * The credential_from_url() function enforces strict mode.\n ++ * If allow_partial_url is non-zero, partial URLs are allowed, i.e. it can, but\n ++ * does not have to, contain\n ++ *\n ++ * - a protocol (or scheme) of the form \"://\"\n ++ *\n ++ * - a host name (the part after the protocol and before the first slash after\n ++ * that, if any)\n ++ *\n ++ * - a user name and potentially a password (as \"[:]@\" part of\n ++ * the host name)\n ++ *\n ++ * - a path (the part after the host name, if any, starting with the slash)\n ++ *\n ++ * Missing parts will be left unset in `struct credential`. Thus, `https://`\n ++ * will have only the `protocol` set, `example.com` only the host name, and\n ++ * `/git` only the path.\n ++ *\n ++ * Note that an empty host name in an otherwise fully-qualified URL will be\n ++ * treated as unset when allow_partial_url is non-zero (and only then,\n ++ * otherwise it is treated as the empty string).\n ++ *\n ++ * The credential_from_url() function does not allow partial URLs.\n */\n void credential_from_url(struct credential *, const char *url);\n -int credential_from_url_gently(struct credential *, const char *url, int quiet);\n +int credential_from_url_gently(struct credential *, const char *url,\n -+\t\t\t int strict, int quiet);\n ++\t\t\t int allow_partial_url, int quiet);\n \n int credential_match(const struct credential *have,\n \t\t const struct credential *want);\n @@ fsck.c: static int check_submodule_url(const char *url)\n \t\tstruct credential c = CREDENTIAL_INIT;\n \t\tint ret = 0;\n -\t\tif (credential_from_url_gently(&c, curl_url, 1) ||\n -+\t\tif (credential_from_url_gently(&c, curl_url, 1, 1) ||\n ++\t\tif (credential_from_url_gently(&c, curl_url, 0, 1) ||\n \t\t !*c.host)\n \t\t\tret = -1;\n \t\tcredential_clear(&c);\n 3: 66823c735b1 ! 3: daedaffe960 credential: handle `credential..` again\n @@ Commit message\n \n Let's reinstate this behavior.\n \n - Original-test-case-by: Jeff King \n + While at it, increase the test coverage to document and verify the\n + behavior with a couple other categories of partial URLs.\n +\n Signed-off-by: Johannes Schindelin \n \n ## credential.c ##\n @@ credential.c: static int credential_config_callback(const char *var, const char\n \t\tint matched;\n \n -\t\tcredential_from_url(&want, url);\n -+\t\tif (credential_from_url_gently(&want, url, 0, 0) < 0) {\n -+\t\t\twarning(_(\"skipping credential lookup for url: %s\"), url);\n -+\t\t\tcredential_clear(c);\n ++\t\tif (credential_from_url_gently(&want, url, 1, 0) < 0) {\n ++\t\t\twarning(_(\"skipping credential lookup for key: %s\"),\n ++\t\t\t\tvar);\n ++\t\t\tcredential_clear(&want);\n +\t\t\tfree(url);\n +\t\t\treturn 0;\n +\t\t}\n @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w\n \ttest_i18ncmp expect stderr\n '\n \n -+test_expect_success 'credential config accepts partial URLs' '\n -+\techo url=https://example.com |\n -+\tgit -c credential.example.com.username=boo \\\n -+\t\tcredential fill >actual &&\n -+\tcat >expect <<-EOF &&\n -+\tprotocol=https\n -+\thost=example.com\n -+\tusername=boo\n -+\tpassword=askpass-password\n -+\tEOF\n -+\ttest_cmp expect actual\n ++test_expect_success 'credential config with partial URLs' '\n ++\techo \"echo password=yep\" | write_script git-credential-yep &&\n ++\ttest_write_lines url=https://user@example.com/repo.git >input &&\n ++\tfor partial in \\\n ++\t\texample.com \\\n ++\t\tuser@example.com \\\n ++\t\thttps:// \\\n ++\t\thttps://example.com \\\n ++\t\thttps://example.com/ \\\n ++\t\thttps://user@example.com \\\n ++\t\thttps://user@example.com/ \\\n ++\t\thttps://example.com/repo.git \\\n ++\t\thttps://user@example.com/repo.git \\\n ++\t\t/repo.git\n ++\tdo\n ++\t\tgit -c credential.$partial.helper=yep \\\n ++\t\t\tcredential fill output &&\n ++\t\tgrep yep output ||\n ++\t\treturn 1\n ++\tdone &&\n ++\n ++\tfor partial in \\\n ++\t\tdont.use.this \\\n ++\t\thttp:// \\\n ++\t\t/repo\n ++\tdo\n ++\t\tgit -c credential.$partial.helper=yep \\\n ++\t\t\tcredential fill output &&\n ++\t\t! grep yep output ||\n ++\t\treturn 1\n ++\tdone &&\n ++\n ++\tgit -c credential.$partial.helper=yep \\\n ++\t\t-c credential.with%0anewline.username=uh-oh \\\n ++\t\tcredential fill output 2>error &&\n ++\ttest_i18ngrep \"skipping credential lookup for key\" error\n ++\n +'\n +\n test_done\n\n-- \ngitgitgadget\n"},{"id":"396068","messageId":"daedaffe960581733c25383a2a1b30056a415594.1587685397.git.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":"pull.615.v2.git.1587685397.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] credential: handle `credential..` again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-23T23:43:17Z","receivedAt":"2020-04-23T23:43:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the patches for CVE-2020-11008, the ability to specify credential\nsettings in the config for partial URLs got lost. For example, it used\nto be possible to specify a credential helper for a specific protocol:\n\n\t[credential \"https://\"]\n\t\thelper = my-https-helper\n\nLikewise, it used to be possible to configure settings for a specific\nhost, e.g.:\n\n\t[credential \"dev.azure.com\"]\n\t\tuseHTTPPath = true\n\nLet's reinstate this behavior.\n\nWhile at it, increase the test coverage to document and verify the\nbehavior with a couple other categories of partial URLs.\n\nSigned-off-by: Johannes Schindelin \n---\n credential.c | 8 +++++++-\n t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+), 1 deletion(-)\n\ndiff --git a/credential.c b/credential.c\nindex 52965a5122c..3505f6356d8 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -53,7 +53,13 @@ static int credential_config_callback(const char *var, const char *value,\n \t\tchar *url = xmemdupz(key, dot - key);\n \t\tint matched;\n \n-\t\tcredential_from_url(&want, url);\n+\t\tif (credential_from_url_gently(&want, url, 1, 0) < 0) {\n+\t\t\twarning(_(\"skipping credential lookup for key: %s\"),\n+\t\t\t\tvar);\n+\t\t\tcredential_clear(&want);\n+\t\t\tfree(url);\n+\t\t\treturn 0;\n+\t\t}\n \t\tmatched = credential_match(&want, c);\n \n \t\tcredential_clear(&want);\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex efed3ea2955..6fff76cb932 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -448,4 +448,43 @@ test_expect_success 'credential system refuses to work with missing protocol' '\n \ttest_i18ncmp expect stderr\n '\n \n+test_expect_success 'credential config with partial URLs' '\n+\techo \"echo password=yep\" | write_script git-credential-yep &&\n+\ttest_write_lines url=https://user@example.com/repo.git >input &&\n+\tfor partial in \\\n+\t\texample.com \\\n+\t\tuser@example.com \\\n+\t\thttps:// \\\n+\t\thttps://example.com \\\n+\t\thttps://example.com/ \\\n+\t\thttps://user@example.com \\\n+\t\thttps://user@example.com/ \\\n+\t\thttps://example.com/repo.git \\\n+\t\thttps://user@example.com/repo.git \\\n+\t\t/repo.git\n+\tdo\n+\t\tgit -c credential.$partial.helper=yep \\\n+\t\t\tcredential fill output &&\n+\t\tgrep yep output ||\n+\t\treturn 1\n+\tdone &&\n+\n+\tfor partial in \\\n+\t\tdont.use.this \\\n+\t\thttp:// \\\n+\t\t/repo\n+\tdo\n+\t\tgit -c credential.$partial.helper=yep \\\n+\t\t\tcredential fill output &&\n+\t\t! grep yep output ||\n+\t\treturn 1\n+\tdone &&\n+\n+\tgit -c credential.$partial.helper=yep \\\n+\t\t-c credential.with%0anewline.username=uh-oh \\\n+\t\tcredential fill output 2>error &&\n+\ttest_i18ngrep \"skipping credential lookup for key\" error\n+\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"396069","messageId":"2c1c0ae91eba2092b24c392786ca2e7c81ded334.1587685397.git.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":"pull.615.v2.git.1587685397.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] credential: fix grammar","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-23T23:43:15Z","receivedAt":"2020-04-23T23:43:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThere was a lot going on behind the scenes when the vulnerability and\npossible solutions were discussed. Grammar was not a primary focus,\nthat's why this slipped in.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Johannes Schindelin \n---\n credential.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential.h b/credential.h\nindex 122a23cd2f1..5a86502d95c 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -32,7 +32,7 @@ void credential_write(const struct credential *, FILE *);\n /*\n * Parse a url into a credential struct, replacing any existing contents.\n *\n- * Ifthe url can't be parsed (e.g., a missing \"proto://\" component), the\n+ * If the url can't be parsed (e.g., a missing \"proto://\" component), the\n * resulting credential will be empty but we'll still return success from the\n * \"gently\" form.\n *\n-- \ngitgitgadget\n\n"},{"id":"396074","messageId":"20200424000558.GB20669@Carlos-MBP","threadId":"53289","inReplyTo":"daedaffe960581733c25383a2a1b30056a415594.1587685397.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] credential: handle `credential..` again","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-24T00:05:58Z","receivedAt":"2020-04-24T00:06:02Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> diff --git a/credential.c b/credential.c\n> index 52965a5122c..3505f6356d8 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -53,7 +53,13 @@ static int credential_config_callback(const char *var, const char *value,\n> \t\tchar *url = xmemdupz(key, dot - key);\n> \t\tint matched;\n> \n> -\t\tcredential_from_url(&want, url);\n> +\t\tif (credential_from_url_gently(&want, url, 1, 0) < 0) {\n\ndefinitely not worth a reroll, but just wondering if would make sense to call\ncredential_from_url_gently(!quiet) here, just for consistency?\n\nother than that this series is looking great, under the assumption that there\nis going to be some more followup with non essential changes.\n\nwill chip in with an test helper for that series so we can hopefully keep our\nsanity next time someone touches that function again.\n\nCarlo\n"},{"id":"396076","messageId":"nycvar.QRO.7.76.6.2004240215100.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"20200424000558.GB20669@Carlos-MBP","subject":"Re: [PATCH v2 3/3] credential: handle `credential..` again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-24T00:16:45Z","receivedAt":"2020-04-24T00:17:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:\n\n> On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > diff --git a/credential.c b/credential.c\n> > index 52965a5122c..3505f6356d8 100644\n> > --- a/credential.c\n> > +++ b/credential.c\n> > @@ -53,7 +53,13 @@ static int credential_config_callback(const char *var, const char *value,\n> > \t\tchar *url = xmemdupz(key, dot - key);\n> > \t\tint matched;\n> >\n> > -\t\tcredential_from_url(&want, url);\n> > +\t\tif (credential_from_url_gently(&want, url, 1, 0) < 0) {\n>\n> definitely not worth a reroll, but just wondering if would make sense to call\n> credential_from_url_gently(!quiet) here, just for consistency?\n\nWe don't have any `quiet` variable here.\n\n> other than that this series is looking great, under the assumption that there\n> is going to be some more followup with non essential changes.\n\nI am sure I don't follow. What non-essential changes are you assuming will\nfollow up?\n\n> will chip in with an test helper for that series so we can hopefully keep our\n> sanity next time someone touches that function again.\n\nAre the tests I added to t0300 not enough? Or do you think it will need a\nnative test helper that is included in `t/helper/test-tool` and exercised\nin the test suite somehow?\n\nCiao,\nDscho\n"},{"id":"396078","messageId":"xmqqpnbxen6h.fsf@gitster.c.googlers.com","threadId":"53289","inReplyTo":"20200424000558.GB20669@Carlos-MBP","subject":"Re: [PATCH v2 3/3] credential: handle `credential..` again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T00:34:46Z","receivedAt":"2020-04-24T00:34:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>> diff --git a/credential.c b/credential.c\n>> index 52965a5122c..3505f6356d8 100644\n>> --- a/credential.c\n>> +++ b/credential.c\n>> @@ -53,7 +53,13 @@ static int credential_config_callback(const char *var, const char *value,\n>> \t\tchar *url = xmemdupz(key, dot - key);\n>> \t\tint matched;\n>> \n>> -\t\tcredential_from_url(&want, url);\n>> +\t\tif (credential_from_url_gently(&want, url, 1, 0) < 0) {\n>\n> definitely not worth a reroll, but just wondering if would make sense to call\n> credential_from_url_gently(!quiet) here, just for consistency?\n\nSpeaking of which, it is not clear which one of \"...url, 1, 0)\" is\nthe \"quiet\" bit. I somehow thought that somebody suggested to roll\nthese two into a flags word and give quiet and the other bit a name,\nand after seeing this line, I tend to agree that would be great for\nreadability.\n\n> other than that this series is looking great, under the assumption that there\n> is going to be some more followup with non essential changes.\n>\n> will chip in with an test helper for that series so we can hopefully keep our\n> sanity next time someone touches that function again.\n\nThanks, everybody, for help polishing this topic.\n\n"},{"id":"396079","messageId":"20200424003926.GC20669@Carlos-MBP","threadId":"53289","inReplyTo":"nycvar.QRO.7.76.6.2004240215100.18039@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 3/3] credential: handle `credential..` again","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-24T00:39:26Z","receivedAt":"2020-04-24T00:39:31Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, Apr 24, 2020 at 02:16:45AM +0200, Johannes Schindelin wrote:\n> On Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:\n> > On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > > diff --git a/credential.c b/credential.c\n> > > index 52965a5122c..3505f6356d8 100644\n> > > --- a/credential.c\n> > > +++ b/credential.c\n> > > @@ -53,7 +53,13 @@ static int credential_config_callback(const char *var, const char *value,\n> > > \t\tchar *url = xmemdupz(key, dot - key);\n> > > \t\tint matched;\n> > >\n> > > -\t\tcredential_from_url(&want, url);\n> > > +\t\tif (credential_from_url_gently(&want, url, 1, 0) < 0) {\n> >\n> > definitely not worth a reroll, but just wondering if would make sense to call\n> > credential_from_url_gently(!quiet) here, just for consistency?\n> \n> We don't have any `quiet` variable here.\n\nmy bad, should had been more explicit as it is confusing with all those\nbooleans at the end without a flags parameter.\n\nI mean the call should be instead :\n\n if (credential_from_url_gently(&want, url, 1, 1) < 0) {\n\nsince we want to warn if the configuration is not supported just like is\ndone after this check.\n\n> \n> > other than that this series is looking great, under the assumption that there\n> > is going to be some more followup with non essential changes.\n> \n> I am sure I don't follow. What non-essential changes are you assuming will\n> follow up?\n\nthe ones that were discussed with Jonathan in a differen thread :\n\n* using a flags parameter instead of two ints\n* whatever will be needed so it also works in master and maint\n\n> > will chip in with an test helper for that series so we can hopefully keep our\n> > sanity next time someone touches that function again.\n> \n> Are the tests I added to t0300 not enough? Or do you think it will need a\n> native test helper that is included in `t/helper/test-tool` and exercised\n> in the test suite somehow?\n\nI think they are enough, it is only that is easier to quickly iterate with\npossible bad input with a helper. which is what I was offering for the next\nthread since its need is orthogonal to what you are trying to accomplish.\n\nCarlo\n"},{"id":"396081","messageId":"xmqqlfmlemxk.fsf@gitster.c.googlers.com","threadId":"53289","inReplyTo":"xmqqpnbxen6h.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/3] credential: handle `credential..` again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T00:40:07Z","receivedAt":"2020-04-24T00:40:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Carlo Marcelo Arenas Belón writes:\n>\n>> On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>>> diff --git a/credential.c b/credential.c\n>>> index 52965a5122c..3505f6356d8 100644\n>>> --- a/credential.c\n>>> +++ b/credential.c\n>>> @@ -53,7 +53,13 @@ static int credential_config_callback(const char *var, const char *value,\n>>> \t\tchar *url = xmemdupz(key, dot - key);\n>>> \t\tint matched;\n>>> \n>>> -\t\tcredential_from_url(&want, url);\n>>> +\t\tif (credential_from_url_gently(&want, url, 1, 0) < 0) {\n>>\n>> definitely not worth a reroll, but just wondering if would make sense to call\n>> credential_from_url_gently(!quiet) here, just for consistency?\n>\n> Speaking of which, it is not clear which one of \"...url, 1, 0)\" is\n> the \"quiet\" bit. I somehow thought that somebody suggested to roll\n> these two into a flags word and give quiet and the other bit a name,\n> and after seeing this line, I tend to agree that would be great for\n> readability.\n\nAh, I should have checked before opening my mouth. It was this\nmessage <20200422233854.GE140314@google.com> from Jonathan Nieder.\n\nI also am OK with his \"two thin wrappers around the underlying\nhelper that takes two separate arguments\", if that makes the\nresulting code easier to follow. I have a feeling that the caller\nknows (from the context, or the reason why it calls the\ncredential-from-url code) if it wants strict or nonstrict variant\nand that is not something the caller is told from its caller. And\nif that is the case, \"two thin wrappers\" approach does make a lot of\nsense.\n\nThanks.\n"},{"id":"396083","messageId":"20200424004937.GD20669@Carlos-MBP","threadId":"53289","inReplyTo":"pull.615.v2.git.1587685397.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] credential: handle partial URLs in config settings again","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-24T00:49:37Z","receivedAt":"2020-04-24T00:49:42Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Reviewed-by: Carlo Marcelo Arenas Belón \n\nfor the whole series\n"},{"id":"396122","messageId":"nycvar.QRO.7.76.6.2004241331180.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"20200424003926.GC20669@Carlos-MBP","subject":"Re: [PATCH v2 3/3] credential: handle `credential..` again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-24T11:34:15Z","receivedAt":"2020-04-24T11:34:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:\n\n> On Fri, Apr 24, 2020 at 02:16:45AM +0200, Johannes Schindelin wrote:\n> > On Thu, 23 Apr 2020, Carlo Marcelo Arenas Belón wrote:\n> > > On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > > > diff --git a/credential.c b/credential.c\n> > > > index 52965a5122c..3505f6356d8 100644\n> > > > --- a/credential.c\n> > > > +++ b/credential.c\n> > > > @@ -53,7 +53,13 @@ static int credential_config_callback(const char *var, const char *value,\n> > > > \t\tchar *url = xmemdupz(key, dot - key);\n> > > > \t\tint matched;\n> > > >\n> > > > -\t\tcredential_from_url(&want, url);\n> > > > +\t\tif (credential_from_url_gently(&want, url, 1, 0) < 0) {\n> > >\n> > > definitely not worth a reroll, but just wondering if would make sense to call\n> > > credential_from_url_gently(!quiet) here, just for consistency?\n> >\n> > We don't have any `quiet` variable here.\n>\n> my bad, should had been more explicit as it is confusing with all those\n> booleans at the end without a flags parameter.\n>\n> I mean the call should be instead :\n>\n> if (credential_from_url_gently(&want, url, 1, 1) < 0) {\n>\n> since we want to warn if the configuration is not supported just like is\n> done after this check.\n\nSince Junio expressed support for Jonathan's idea of using separate\nfunctions that wrap one helper function, I went with that instead.\n\n> > > other than that this series is looking great, under the assumption that there\n> > > is going to be some more followup with non essential changes.\n> >\n> > I am sure I don't follow. What non-essential changes are you assuming will\n> > follow up?\n>\n> the ones that were discussed with Jonathan in a differen thread :\n>\n> * using a flags parameter instead of two ints\n> * whatever will be needed so it also works in master and maint\n\nOy, I am still under water just trying to get the MinGit for Windows\nbackports updated so that users can upgrade instead of complaining on bug\ntrackers and on Twitter...\n\nBut yes, that will be the next step. Now that I have a comprehensive test\ncase, it should be relatively easy.\n\n> > > will chip in with an test helper for that series so we can hopefully keep our\n> > > sanity next time someone touches that function again.\n> >\n> > Are the tests I added to t0300 not enough? Or do you think it will need a\n> > native test helper that is included in `t/helper/test-tool` and exercised\n> > in the test suite somehow?\n>\n> I think they are enough, it is only that is easier to quickly iterate with\n> possible bad input with a helper. which is what I was offering for the next\n> thread since its need is orthogonal to what you are trying to accomplish.\n\nUsually I would agree with you. It's quicker to iterate, and a ton faster\nto run (especially on Windows).\n\nIn this instance, I am going for a regression test case rather than a unit\ntest, though, because I really need to ensure that what end users are\ntrying to do on their machines will work (and continue to work).\n\nCiao,\nDscho\n"},{"id":"396123","messageId":"nycvar.QRO.7.76.6.2004241336170.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"xmqqlfmlemxk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/3] credential: handle `credential..` again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-24T11:36:33Z","receivedAt":"2020-04-24T11:36:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 23 Apr 2020, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n> > Carlo Marcelo Arenas Belón writes:\n> >\n> >> On Thu, Apr 23, 2020 at 11:43:17PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> >>> diff --git a/credential.c b/credential.c\n> >>> index 52965a5122c..3505f6356d8 100644\n> >>> --- a/credential.c\n> >>> +++ b/credential.c\n> >>> @@ -53,7 +53,13 @@ static int credential_config_callback(const char *var, const char *value,\n> >>> \t\tchar *url = xmemdupz(key, dot - key);\n> >>> \t\tint matched;\n> >>>\n> >>> -\t\tcredential_from_url(&want, url);\n> >>> +\t\tif (credential_from_url_gently(&want, url, 1, 0) < 0) {\n> >>\n> >> definitely not worth a reroll, but just wondering if would make sense to call\n> >> credential_from_url_gently(!quiet) here, just for consistency?\n> >\n> > Speaking of which, it is not clear which one of \"...url, 1, 0)\" is\n> > the \"quiet\" bit. I somehow thought that somebody suggested to roll\n> > these two into a flags word and give quiet and the other bit a name,\n> > and after seeing this line, I tend to agree that would be great for\n> > readability.\n>\n> Ah, I should have checked before opening my mouth. It was this\n> message <20200422233854.GE140314@google.com> from Jonathan Nieder.\n>\n> I also am OK with his \"two thin wrappers around the underlying\n> helper that takes two separate arguments\", if that makes the\n> resulting code easier to follow. I have a feeling that the caller\n> knows (from the context, or the reason why it calls the\n> credential-from-url code) if it wants strict or nonstrict variant\n> and that is not something the caller is told from its caller. And\n> if that is the case, \"two thin wrappers\" approach does make a lot of\n> sense.\n\nAll right, two wrapper functions it is.\n\nCiao,\nDscho\n"},{"id":"396125","messageId":"pull.615.v3.git.1587728992.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":"pull.615.v2.git.1587685397.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] credential: handle partial URLs in config settings again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-24T11:49:49Z","receivedAt":"2020-04-24T11:49:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This fixes the problem illustrated by Peff's example\n[https://lore.kernel.org/git/20200422040644.GC3559880@coredump.intra.peff.net/]\n, in maint-2.17:\n\n $ echo url=https://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\nThe fix is necessarily different than what was proposed by brian\n[https://lore.kernel.org/git/20200422012344.2051103-1-sandals@crustytoothpaste.net/] \nbecause that fix targets v2.26.x which has 46fd7b390034 (credential: allow\nwildcard patterns when matching config, 2020-02-20).\n\nThis patch series targets maint-2.17 instead (and might actually not be able\nto fix maint due to that wildcard pattern patch; I haven't had the time to\ncheck yet).\n\nPlease note that Git v2.17.4 will not do what we would expect here: if any\nhost name (without protocol) is specified, e.g. -c\ncredential.golli.wog.username = boo, it will actually ignore the host name.\nThat is, this will populate the username:\n\n $ echo url=https://example.com |\n git -c credential.totally.bog.us.username=foo credential fill\n\nObviously, this is unexpected, as a Git config like this would leave the\nlast specified user name as \"winner\":\n\n[credential \"first.com\"]\n username = Whos On\n[credential \"second.com\"]\n username = Who\n\nThis patch series fixes this. The quoted part of [credential \"\"] will\nbe interpreted as a partial URL:\n\n * It can start with a protocol followed by ://, but does not have to.\n * If it starts with a protocol, the host name will always be set (if the \n :// is followed immediately by yet another slash, then it will be set to\n the empty string).\n * If it starts without a protocol, it is treated as a path if the value\n starts with a slash (and the host will be left unset).\n * If it starts without a protocol and the first character is not a slash,\n it will be treated as a host name, optionally followed by a slash and the\n path.\n\nChanges since v2:\n\n * Refactored out the credential_from_potentially_partial_url() function so\n that existing callers (except for the one we want to change) stay\n untouched.\n * When encountering an invalid URL while parsing the config, we no longer\n suppress the parser's warning.\n * The test now uses the file name convention stdin/stdout/stderr of \n t/lib-credential.sh.\n\nChanges since v1:\n\n * The condition when the c->host field is set was made more intuitive.\n * The \"fix grammar\" commit now has Jonathan's \"reviewed-by\" footer.\n * Inverted the meaning of the parameter strict and renamed it to \n allow_partial_urls, to clarify its role.\n * Enhanced the commit message of the second patch to illustrate the\n motivation for the lenient mode a bit better.\n * [Not a change] I did leave the second and the third patch separate from\n one another. This makes it a lot easier to follow the iteration and to\n keep the reviews straight: it separates the \"how do we make URL parts\n optional?\" from the \"where do we need URL parts to be optional?\"\n * A partial URL https:// is now correctly interpreted as having only the\n protocol set, but not host name nor path.\n * The lenient mode is now explained a lot more verbosely in the code\n comment in credential.h.\n * When skipping a config setting, we now show the config key, not just the\n URL (which might be incomplete, i.e. not actually a URL).\n * When skipping a config setting, the correct struct credential is cleared\n (i.e. the just-parsed one, not the one against which we wanted to match\n the just-parsed one).\n * Added a ton more partial URLs to the test, and the test now also verifies\n that the warning comes out all right.\n\nJohannes Schindelin (3):\n credential: fix grammar\n credential: optionally allow partial URLs in\n credential_from_url_gently()\n credential: handle `credential..` again\n\n credential.c | 60 +++++++++++++++++++++++++++++++++++++-----\n credential.h | 2 +-\n t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++\n 3 files changed, 93 insertions(+), 8 deletions(-)\n\n\nbase-commit: df5be6dc3fd18c294ec93a9af0321334e3f92c9c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-615%2Fdscho%2Fcredential-config-partial-url-maint-2.17-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-615/dscho/credential-config-partial-url-maint-2.17-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/615\n\nRange-diff vs v2:\n\n 1: 2c1c0ae91eb = 1: 2c1c0ae91eb credential: fix grammar\n 2: fc772d21b74 ! 2: e92f139dfc7 credential: optionally allow partial URLs in credential_from_url_gently()\n @@ Commit message\n Settings like this might be desired when users want to use, say, a given\n user name on a given host, regardless of the protocol to be used.\n \n - In preparation for fixing that bug, let's add a parameter called\n - `allow_partial_url` to the `credential_from_url_gently()` function and\n - convert the existing callers to set that parameter to 0, i.e. do not\n - change the existing behavior, just add the option to be more lenient.\n + In preparation for fixing that bug, let's refactor the code to\n + optionally allow for partial URLs. For the moment, this functionality is\n + only exposed via the now-renamed function `credential_from_url_1()`, but\n + it is not used. The intention is to make it easier to verify that this\n + commit does not change the existing behavior unless explicitly allowing\n + for partial URLs.\n \n Please note that this patch does more than just reinstating a way to\n imitate the behavior before those CVE-2020-11008 fixes: Before that, we\n @@ Commit message\n \n ## credential.c ##\n @@ credential.c: static int check_url_component(const char *url, int quiet,\n + \treturn -1;\n }\n \n - int credential_from_url_gently(struct credential *c, const char *url,\n +-int credential_from_url_gently(struct credential *c, const char *url,\n -\t\t\t int quiet)\n -+\t\t\t int allow_partial_url, int quiet)\n ++/*\n ++ * Potentially-partial URLs can, but do not have to, contain\n ++ *\n ++ * - a protocol (or scheme) of the form \"://\"\n ++ *\n ++ * - a host name (the part after the protocol and before the first slash after\n ++ * that, if any)\n ++ *\n ++ * - a user name and potentially a password (as \"[:]@\" part of\n ++ * the host name)\n ++ *\n ++ * - a path (the part after the host name, if any, starting with the slash)\n ++ *\n ++ * Missing parts will be left unset in `struct credential`. Thus, `https://`\n ++ * will have only the `protocol` set, `example.com` only the host name, and\n ++ * `/git` only the path.\n ++ *\n ++ * Note that an empty host name in an otherwise fully-qualified URL (e.g.\n ++ * `cert:///path/to/cert.pem`) will be treated as unset if we expect the URL to\n ++ * be potentially partial, and only then (otherwise, the empty string is used).\n ++ *\n ++ * The credential_from_url() function does not allow partial URLs.\n ++ */\n ++static int credential_from_url_1(struct credential *c, const char *url,\n ++\t\t\t\t int allow_partial_url, int quiet)\n {\n \tconst char *at, *colon, *cp, *slash, *host, *proto_end;\n \n @@ credential.c: int credential_from_url_gently(struct credential *c, const char *u\n \twhile (*slash == '/')\n \t\tslash++;\n @@ credential.c: int credential_from_url_gently(struct credential *c, const char *url,\n + \treturn 0;\n + }\n \n ++int credential_from_url_gently(struct credential *c, const char *url, int quiet)\n ++{\n ++\treturn credential_from_url_1(c, url, 0, quiet);\n ++}\n ++\n void credential_from_url(struct credential *c, const char *url)\n {\n --\tif (credential_from_url_gently(c, url, 0) < 0)\n -+\tif (credential_from_url_gently(c, url, 0, 0) < 0)\n - \t\tdie(_(\"credential url cannot be parsed: %s\"), url);\n - }\n -\n - ## credential.h ##\n -@@ credential.h: void credential_write(const struct credential *, FILE *);\n - * an error but leave the broken state in the credential object for further\n - * examination. The non-gentle form will issue a warning to stderr and return\n - * an empty credential.\n -+ *\n -+ * If allow_partial_url is non-zero, partial URLs are allowed, i.e. it can, but\n -+ * does not have to, contain\n -+ *\n -+ * - a protocol (or scheme) of the form \"://\"\n -+ *\n -+ * - a host name (the part after the protocol and before the first slash after\n -+ * that, if any)\n -+ *\n -+ * - a user name and potentially a password (as \"[:]@\" part of\n -+ * the host name)\n -+ *\n -+ * - a path (the part after the host name, if any, starting with the slash)\n -+ *\n -+ * Missing parts will be left unset in `struct credential`. Thus, `https://`\n -+ * will have only the `protocol` set, `example.com` only the host name, and\n -+ * `/git` only the path.\n -+ *\n -+ * Note that an empty host name in an otherwise fully-qualified URL will be\n -+ * treated as unset when allow_partial_url is non-zero (and only then,\n -+ * otherwise it is treated as the empty string).\n -+ *\n -+ * The credential_from_url() function does not allow partial URLs.\n - */\n - void credential_from_url(struct credential *, const char *url);\n --int credential_from_url_gently(struct credential *, const char *url, int quiet);\n -+int credential_from_url_gently(struct credential *, const char *url,\n -+\t\t\t int allow_partial_url, int quiet);\n - \n - int credential_match(const struct credential *have,\n - \t\t const struct credential *want);\n -\n - ## fsck.c ##\n -@@ fsck.c: static int check_submodule_url(const char *url)\n - \telse if (url_to_curl_url(url, &curl_url)) {\n - \t\tstruct credential c = CREDENTIAL_INIT;\n - \t\tint ret = 0;\n --\t\tif (credential_from_url_gently(&c, curl_url, 1) ||\n -+\t\tif (credential_from_url_gently(&c, curl_url, 0, 1) ||\n - \t\t !*c.host)\n - \t\t\tret = -1;\n - \t\tcredential_clear(&c);\n + \tif (credential_from_url_gently(c, url, 0) < 0)\n 3: daedaffe960 ! 3: 0535908dd7e credential: handle `credential..` again\n @@ Commit message\n Signed-off-by: Johannes Schindelin \n \n ## credential.c ##\n +@@ credential.c: int credential_match(const struct credential *want,\n + #undef CHECK\n + }\n + \n ++\n ++static int credential_from_potentially_partial_url(struct credential *c,\n ++\t\t\t\t\t\t const char *url);\n ++\n + static int credential_config_callback(const char *var, const char *value,\n + \t\t\t\t void *data)\n + {\n @@ credential.c: static int credential_config_callback(const char *var, const char *value,\n \t\tchar *url = xmemdupz(key, dot - key);\n \t\tint matched;\n \n -\t\tcredential_from_url(&want, url);\n -+\t\tif (credential_from_url_gently(&want, url, 1, 0) < 0) {\n ++\t\tif (credential_from_potentially_partial_url(&want, url) < 0) {\n +\t\t\twarning(_(\"skipping credential lookup for key: %s\"),\n +\t\t\t\tvar);\n +\t\t\tcredential_clear(&want);\n @@ credential.c: static int credential_config_callback(const char *var, const char\n \t\tmatched = credential_match(&want, c);\n \n \t\tcredential_clear(&want);\n +@@ credential.c: static int credential_from_url_1(struct credential *c, const char *url,\n + \treturn 0;\n + }\n + \n ++static int credential_from_potentially_partial_url(struct credential *c,\n ++\t\t\t\t\t\t const char *url)\n ++{\n ++\treturn credential_from_url_1(c, url, 1, 0);\n ++}\n ++\n + int credential_from_url_gently(struct credential *c, const char *url, int quiet)\n + {\n + \treturn credential_from_url_1(c, url, 0, quiet);\n \n ## t/t0300-credentials.sh ##\n @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work with missing protocol' '\n @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w\n \n +test_expect_success 'credential config with partial URLs' '\n +\techo \"echo password=yep\" | write_script git-credential-yep &&\n -+\ttest_write_lines url=https://user@example.com/repo.git >input &&\n ++\ttest_write_lines url=https://user@example.com/repo.git >stdin &&\n +\tfor partial in \\\n +\t\texample.com \\\n +\t\tuser@example.com \\\n @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w\n +\t\t/repo.git\n +\tdo\n +\t\tgit -c credential.$partial.helper=yep \\\n -+\t\t\tcredential fill output &&\n -+\t\tgrep yep output ||\n ++\t\t\tcredential fill stdout &&\n ++\t\tgrep yep stdout ||\n +\t\treturn 1\n +\tdone &&\n +\n @@ t/t0300-credentials.sh: test_expect_success 'credential system refuses to work w\n +\t\t/repo\n +\tdo\n +\t\tgit -c credential.$partial.helper=yep \\\n -+\t\t\tcredential fill output &&\n -+\t\t! grep yep output ||\n ++\t\t\tcredential fill stdout &&\n ++\t\t! grep yep stdout ||\n +\t\treturn 1\n +\tdone &&\n +\n +\tgit -c credential.$partial.helper=yep \\\n +\t\t-c credential.with%0anewline.username=uh-oh \\\n -+\t\tcredential fill output 2>error &&\n -+\ttest_i18ngrep \"skipping credential lookup for key\" error\n ++\t\tcredential fill stdout 2>stderr &&\n ++\ttest_i18ngrep \"skipping credential lookup for key\" stderr\n +\n +'\n +\n\n-- \ngitgitgadget\n"},{"id":"396126","messageId":"2c1c0ae91eba2092b24c392786ca2e7c81ded334.1587728992.git.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":"pull.615.v3.git.1587728992.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] credential: fix grammar","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-24T11:49:50Z","receivedAt":"2020-04-24T11:50:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThere was a lot going on behind the scenes when the vulnerability and\npossible solutions were discussed. Grammar was not a primary focus,\nthat's why this slipped in.\n\nReviewed-by: Jonathan Nieder \nSigned-off-by: Johannes Schindelin \n---\n credential.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential.h b/credential.h\nindex 122a23cd2f1..5a86502d95c 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -32,7 +32,7 @@ void credential_write(const struct credential *, FILE *);\n /*\n * Parse a url into a credential struct, replacing any existing contents.\n *\n- * Ifthe url can't be parsed (e.g., a missing \"proto://\" component), the\n+ * If the url can't be parsed (e.g., a missing \"proto://\" component), the\n * resulting credential will be empty but we'll still return success from the\n * \"gently\" form.\n *\n-- \ngitgitgadget\n\n"},{"id":"396127","messageId":"0535908dd7ea4487b342c0f86182579279c57b34.1587728992.git.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":"pull.615.v3.git.1587728992.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] credential: handle `credential..` again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-24T11:49:52Z","receivedAt":"2020-04-24T11:50:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the patches for CVE-2020-11008, the ability to specify credential\nsettings in the config for partial URLs got lost. For example, it used\nto be possible to specify a credential helper for a specific protocol:\n\n\t[credential \"https://\"]\n\t\thelper = my-https-helper\n\nLikewise, it used to be possible to configure settings for a specific\nhost, e.g.:\n\n\t[credential \"dev.azure.com\"]\n\t\tuseHTTPPath = true\n\nLet's reinstate this behavior.\n\nWhile at it, increase the test coverage to document and verify the\nbehavior with a couple other categories of partial URLs.\n\nSigned-off-by: Johannes Schindelin \n---\n credential.c | 18 +++++++++++++++++-\n t/t0300-credentials.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 56 insertions(+), 1 deletion(-)\n\ndiff --git a/credential.c b/credential.c\nindex 7dbbf26f174..c1a9ca4e485 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -35,6 +35,10 @@ int credential_match(const struct credential *want,\n #undef CHECK\n }\n \n+\n+static int credential_from_potentially_partial_url(struct credential *c,\n+\t\t\t\t\t\t const char *url);\n+\n static int credential_config_callback(const char *var, const char *value,\n \t\t\t\t void *data)\n {\n@@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value,\n \t\tchar *url = xmemdupz(key, dot - key);\n \t\tint matched;\n \n-\t\tcredential_from_url(&want, url);\n+\t\tif (credential_from_potentially_partial_url(&want, url) < 0) {\n+\t\t\twarning(_(\"skipping credential lookup for key: %s\"),\n+\t\t\t\tvar);\n+\t\t\tcredential_clear(&want);\n+\t\t\tfree(url);\n+\t\t\treturn 0;\n+\t\t}\n \t\tmatched = credential_match(&want, c);\n \n \t\tcredential_clear(&want);\n@@ -430,6 +440,12 @@ static int credential_from_url_1(struct credential *c, const char *url,\n \treturn 0;\n }\n \n+static int credential_from_potentially_partial_url(struct credential *c,\n+\t\t\t\t\t\t const char *url)\n+{\n+\treturn credential_from_url_1(c, url, 1, 0);\n+}\n+\n int credential_from_url_gently(struct credential *c, const char *url, int quiet)\n {\n \treturn credential_from_url_1(c, url, 0, quiet);\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex efed3ea2955..f796bbfd48b 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -448,4 +448,43 @@ test_expect_success 'credential system refuses to work with missing protocol' '\n \ttest_i18ncmp expect stderr\n '\n \n+test_expect_success 'credential config with partial URLs' '\n+\techo \"echo password=yep\" | write_script git-credential-yep &&\n+\ttest_write_lines url=https://user@example.com/repo.git >stdin &&\n+\tfor partial in \\\n+\t\texample.com \\\n+\t\tuser@example.com \\\n+\t\thttps:// \\\n+\t\thttps://example.com \\\n+\t\thttps://example.com/ \\\n+\t\thttps://user@example.com \\\n+\t\thttps://user@example.com/ \\\n+\t\thttps://example.com/repo.git \\\n+\t\thttps://user@example.com/repo.git \\\n+\t\t/repo.git\n+\tdo\n+\t\tgit -c credential.$partial.helper=yep \\\n+\t\t\tcredential fill stdout &&\n+\t\tgrep yep stdout ||\n+\t\treturn 1\n+\tdone &&\n+\n+\tfor partial in \\\n+\t\tdont.use.this \\\n+\t\thttp:// \\\n+\t\t/repo\n+\tdo\n+\t\tgit -c credential.$partial.helper=yep \\\n+\t\t\tcredential fill stdout &&\n+\t\t! grep yep stdout ||\n+\t\treturn 1\n+\tdone &&\n+\n+\tgit -c credential.$partial.helper=yep \\\n+\t\t-c credential.with%0anewline.username=uh-oh \\\n+\t\tcredential fill stdout 2>stderr &&\n+\ttest_i18ngrep \"skipping credential lookup for key\" stderr\n+\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"396128","messageId":"e92f139dfc732720f5819a954c54f2e3c5f7d1c9.1587728992.git.gitgitgadget@gmail.com","threadId":"53289","inReplyTo":"pull.615.v3.git.1587728992.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] credential: optionally allow partial URLs in credential_from_url_gently()","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-24T11:49:51Z","receivedAt":"2020-04-24T11:50:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nPrior to the fixes for CVE-2020-11008, we were _very_ lenient in what we\nrequired from a URL in order to parse it into a `struct credential`.\nThat led to serious vulnerabilities.\n\nThere was one call site, though, that really needed that leniency: when\nparsing config settings a la `credential.dev.azure.com.useHTTPPath`.\nSettings like this might be desired when users want to use, say, a given\nuser name on a given host, regardless of the protocol to be used.\n\nIn preparation for fixing that bug, let's refactor the code to\noptionally allow for partial URLs. For the moment, this functionality is\nonly exposed via the now-renamed function `credential_from_url_1()`, but\nit is not used. The intention is to make it easier to verify that this\ncommit does not change the existing behavior unless explicitly allowing\nfor partial URLs.\n\nPlease note that this patch does more than just reinstating a way to\nimitate the behavior before those CVE-2020-11008 fixes: Before that, we\nwould simply ignore URLs without a protocol. In other words,\nmisleadingly, the following setting would be applied to _all_ URLs:\n\n\t[credential \"example.com\"]\n\t\tusername = that-me\n\nThe obvious intention is to match the host name only. With this patch,\nwe allow precisely that: when parsing the URL with non-zero\n`allow_partial_url`, we do not simply return success if there was no\nprotocol, but we simply leave the protocol unset and continue parsing\nthe URL.\n\nSigned-off-by: Johannes Schindelin \n---\n credential.c | 42 ++++++++++++++++++++++++++++++++++++------\n 1 file changed, 36 insertions(+), 6 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 64a841eddca..7dbbf26f174 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -343,8 +343,31 @@ static int check_url_component(const char *url, int quiet,\n \treturn -1;\n }\n \n-int credential_from_url_gently(struct credential *c, const char *url,\n-\t\t\t int quiet)\n+/*\n+ * Potentially-partial URLs can, but do not have to, contain\n+ *\n+ * - a protocol (or scheme) of the form \"://\"\n+ *\n+ * - a host name (the part after the protocol and before the first slash after\n+ * that, if any)\n+ *\n+ * - a user name and potentially a password (as \"[:]@\" part of\n+ * the host name)\n+ *\n+ * - a path (the part after the host name, if any, starting with the slash)\n+ *\n+ * Missing parts will be left unset in `struct credential`. Thus, `https://`\n+ * will have only the `protocol` set, `example.com` only the host name, and\n+ * `/git` only the path.\n+ *\n+ * Note that an empty host name in an otherwise fully-qualified URL (e.g.\n+ * `cert:///path/to/cert.pem`) will be treated as unset if we expect the URL to\n+ * be potentially partial, and only then (otherwise, the empty string is used).\n+ *\n+ * The credential_from_url() function does not allow partial URLs.\n+ */\n+static int credential_from_url_1(struct credential *c, const char *url,\n+\t\t\t\t int allow_partial_url, int quiet)\n {\n \tconst char *at, *colon, *cp, *slash, *host, *proto_end;\n \n@@ -357,12 +380,12 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \t * (3) proto://:@/...\n \t */\n \tproto_end = strstr(url, \"://\");\n-\tif (!proto_end || proto_end == url) {\n+\tif (!allow_partial_url && (!proto_end || proto_end == url)) {\n \t\tif (!quiet)\n \t\t\twarning(_(\"url has no scheme: %s\"), url);\n \t\treturn -1;\n \t}\n-\tcp = proto_end + 3;\n+\tcp = proto_end ? proto_end + 3 : url;\n \tat = strchr(cp, '@');\n \tcolon = strchr(cp, ':');\n \tslash = strchrnul(cp, '/');\n@@ -382,8 +405,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \t\thost = at + 1;\n \t}\n \n-\tc->protocol = xmemdupz(url, proto_end - url);\n-\tc->host = url_decode_mem(host, slash - host);\n+\tif (proto_end && proto_end - url > 0)\n+\t\tc->protocol = xmemdupz(url, proto_end - url);\n+\tif (!allow_partial_url || slash - host > 0)\n+\t\tc->host = url_decode_mem(host, slash - host);\n \t/* Trim leading and trailing slashes from path */\n \twhile (*slash == '/')\n \t\tslash++;\n@@ -405,6 +430,11 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \treturn 0;\n }\n \n+int credential_from_url_gently(struct credential *c, const char *url, int quiet)\n+{\n+\treturn credential_from_url_1(c, url, 0, quiet);\n+}\n+\n void credential_from_url(struct credential *c, const char *url)\n {\n \tif (credential_from_url_gently(c, url, 0) < 0)\n-- \ngitgitgadget\n\n"},{"id":"396144","messageId":"20200424152307.GA85188@Carlos-MBP","threadId":"53289","inReplyTo":"0535908dd7ea4487b342c0f86182579279c57b34.1587728992.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/3] credential: handle `credential..` again","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-24T15:23:07Z","receivedAt":"2020-04-24T15:23:12Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, Apr 24, 2020 at 11:49:52AM +0000, Johannes Schindelin via GitGitGadget wrote:\n> diff --git a/credential.c b/credential.c\n> index 7dbbf26f174..c1a9ca4e485 100644\n> --- a/credential.c\n> +++ b/credential.c\n> @@ -35,6 +35,10 @@ int credential_match(const struct credential *want,\n> #undef CHECK\n> }\n> \n> +\n> +static int credential_from_potentially_partial_url(struct credential *c,\n> +\t\t\t\t\t\t const char *url);\n> +\n> static int credential_config_callback(const char *var, const char *value,\n> \t\t\t\t void *data)\n> {\n\nsomething like credential_from_url_partial might had been more grep friendly\nbut this would work as well.\n\n> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> index efed3ea2955..f796bbfd48b 100755\n> --- a/t/t0300-credentials.sh\n> +++ b/t/t0300-credentials.sh\n> @@ -448,4 +448,43 @@ test_expect_success 'credential system refuses to work with missing protocol' '\n> \ttest_i18ncmp expect stderr\n> '\n> \n> +test_expect_success 'credential config with partial URLs' '\n> +\techo \"echo password=yep\" | write_script git-credential-yep &&\n> +\ttest_write_lines url=https://user@example.com/repo.git >stdin &&\n> +\tfor partial in \\\n> +\t\texample.com \\\n> +\t\tuser@example.com \\\n\neven if it works, configurations with a username might not be worth the\ntrouble to support IMHO\n\nmaybe better not to include them in the tests then either?\n\nother than that, like the previous version (which is functionally equivalent)\nshould be IMHO good to go.\n\nCarlo\n"},{"id":"396196","messageId":"xmqq4kt8bk5r.fsf@gitster.c.googlers.com","threadId":"53289","inReplyTo":"0535908dd7ea4487b342c0f86182579279c57b34.1587728992.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/3] credential: handle `credential..` again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T22:20:32Z","receivedAt":"2020-04-24T22:20:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value,\n> \t\tchar *url = xmemdupz(key, dot - key);\n> \t\tint matched;\n> \n> -\t\tcredential_from_url(&want, url);\n> +\t\tif (credential_from_potentially_partial_url(&want, url) < 0) {\n> +\t\t\twarning(_(\"skipping credential lookup for key: %s\"),\n> +\t\t\t\tvar);\n> +\t\t\tcredential_clear(&want);\n> +\t\t\tfree(url);\n> +\t\t\treturn 0;\n> +\t\t}\n> \t\tmatched = credential_match(&want, c);\n\nUnfortunately, the whole section of this code that is being patched\nhere goes away with 46fd7b39 (credential: allow wildcard patterns\nwhen matching config, 2020-02-20), that delegates large part of the\nlogic to urlmatch.\n\nDscho and Brian, how do we want to proceed? As the conflicting\nchange has already been in 'next' for more than a month and a half,\nwe'd need this \"partial URL\" logic build to work well with it.\n\nThanks.\n"},{"id":"396197","messageId":"nycvar.QRO.7.76.6.2004250027550.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"xmqq4kt8bk5r.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/3] credential: handle `credential..` again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-24T22:29:51Z","receivedAt":"2020-04-24T22:30:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 24 Apr 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value,\n> > \t\tchar *url = xmemdupz(key, dot - key);\n> > \t\tint matched;\n> >\n> > -\t\tcredential_from_url(&want, url);\n> > +\t\tif (credential_from_potentially_partial_url(&want, url) < 0) {\n> > +\t\t\twarning(_(\"skipping credential lookup for key: %s\"),\n> > +\t\t\t\tvar);\n> > +\t\t\tcredential_clear(&want);\n> > +\t\t\tfree(url);\n> > +\t\t\treturn 0;\n> > +\t\t}\n> > \t\tmatched = credential_match(&want, c);\n>\n> Unfortunately, the whole section of this code that is being patched\n> here goes away with 46fd7b39 (credential: allow wildcard patterns\n> when matching config, 2020-02-20), that delegates large part of the\n> logic to urlmatch.\n>\n> Dscho and Brian, how do we want to proceed? As the conflicting\n> change has already been in 'next' for more than a month and a half,\n> we'd need this \"partial URL\" logic build to work well with it.\n\nI prepared a PR: https://github.com/gitgitgadget/git/pull/620. I do not\nnecessarily feel 100% comfortable with that approach yet, but at least it\nlets the new test case of t0300 pass.\n\nCiao,\nDscho\n\n\n"},{"id":"396241","messageId":"nycvar.QRO.7.76.6.2004251238590.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"20200424152307.GA85188@Carlos-MBP","subject":"Re: [PATCH v3 3/3] credential: handle `credential..` again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-25T10:43:21Z","receivedAt":"2020-04-25T10:43:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Fri, 24 Apr 2020, Carlo Marcelo Arenas Belón wrote:\n\n> On Fri, Apr 24, 2020 at 11:49:52AM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > diff --git a/credential.c b/credential.c\n> > index 7dbbf26f174..c1a9ca4e485 100644\n> > --- a/credential.c\n> > +++ b/credential.c\n> > @@ -35,6 +35,10 @@ int credential_match(const struct credential *want,\n> > #undef CHECK\n> > }\n> >\n> > +\n> > +static int credential_from_potentially_partial_url(struct credential *c,\n> > +\t\t\t\t\t\t const char *url);\n> > +\n> > static int credential_config_callback(const char *var, const char *value,\n> > \t\t\t\t void *data)\n> > {\n>\n> something like credential_from_url_partial might had been more grep friendly\n> but this would work as well.\n\nIt might be more grep'able, but it sounds really awkward to me, that's why\nI did not use that name (it was my first candidate).\n\n> > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\n> > index efed3ea2955..f796bbfd48b 100755\n> > --- a/t/t0300-credentials.sh\n> > +++ b/t/t0300-credentials.sh\n> > @@ -448,4 +448,43 @@ test_expect_success 'credential system refuses to work with missing protocol' '\n> > \ttest_i18ncmp expect stderr\n> > '\n> >\n> > +test_expect_success 'credential config with partial URLs' '\n> > +\techo \"echo password=yep\" | write_script git-credential-yep &&\n> > +\ttest_write_lines url=https://user@example.com/repo.git >stdin &&\n> > +\tfor partial in \\\n> > +\t\texample.com \\\n> > +\t\tuser@example.com \\\n>\n> even if it works, configurations with a username might not be worth the\n> trouble to support IMHO\n>\n> maybe better not to include them in the tests then either?\n\nLet me counter this:\n\n- It would take extra code _to prevent_ the username from being used, and\n\n- There is precedent where the user name _does_ matter: it is relatively\n normal to access different orgs' repositories at\n https://dev.azure.com///_git via different accounts.\n\nTogether, those points convince me that special-casing the username _and\nexplicitly ignoring it_ would not make sense.\n\n> other than that, like the previous version (which is functionally equivalent)\n> should be IMHO good to go.\n\nThank you for reviewing it!\n\nCiao,\nDscho\n"},{"id":"396516","messageId":"xmqqr1w7yzjc.fsf@gitster.c.googlers.com","threadId":"53289","inReplyTo":"nycvar.QRO.7.76.6.2004250027550.18039@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 3/3] credential: handle `credential..` again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T23:13:27Z","receivedAt":"2020-04-28T23:13:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Fri, 24 Apr 2020, Junio C Hamano wrote:\n>\n>> \"Johannes Schindelin via GitGitGadget\" \n>> writes:\n>>\n>> > @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value,\n>> > \t\tchar *url = xmemdupz(key, dot - key);\n>> > \t\tint matched;\n>> >\n>> > -\t\tcredential_from_url(&want, url);\n>> > +\t\tif (credential_from_potentially_partial_url(&want, url) < 0) {\n>> > +\t\t\twarning(_(\"skipping credential lookup for key: %s\"),\n>> > +\t\t\t\tvar);\n>> > +\t\t\tcredential_clear(&want);\n>> > +\t\t\tfree(url);\n>> > +\t\t\treturn 0;\n>> > +\t\t}\n>> > \t\tmatched = credential_match(&want, c);\n>>\n>> Unfortunately, the whole section of this code that is being patched\n>> here goes away with 46fd7b39 (credential: allow wildcard patterns\n>> when matching config, 2020-02-20), that delegates large part of the\n>> logic to urlmatch.\n>>\n>> Dscho and Brian, how do we want to proceed? As the conflicting\n>> change has already been in 'next' for more than a month and a half,\n>> we'd need this \"partial URL\" logic build to work well with it.\n>\n> I prepared a PR: https://github.com/gitgitgadget/git/pull/620. I do not\n> necessarily feel 100% comfortable with that approach yet, but at least it\n> lets the new test case of t0300 pass.\n\nThe -2.17 patch series (your [v3 3/3]) ends t0300 like so\n\n +\tdone &&\n +\n +\tgit -c credential.$partial.helper=yep \\\n +\t\t-c credential.with%0anewline.username=uh-oh \\\n +\t\tcredential fill stdout 2>stderr &&\n +\ttest_i18ngrep \"skipping credential lookup for key\" stderr\n +\n +'\n +\n test_done\n\nwhile the other branch lacks the extra blank line just before the\nsingle quote is closed. I queued 850ef627 (SQUASH??? lose excess\nblank line to match the other side of the eventual merge,\n2020-04-24) on top so that the \"-s ours\" merge would be without\nunnecessary evil-merge noise. You probably would want to squash it\nin.\n\nThanks.\n"},{"id":"396557","messageId":"nycvar.QRO.7.76.6.2004291657220.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"xmqqr1w7yzjc.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/3] credential: handle `credential..` again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-29T14:58:05Z","receivedAt":"2020-04-29T14:58:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 28 Apr 2020, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > Hi Junio,\n> >\n> > On Fri, 24 Apr 2020, Junio C Hamano wrote:\n> >\n> >> \"Johannes Schindelin via GitGitGadget\" \n> >> writes:\n> >>\n> >> > @@ -53,7 +57,13 @@ static int credential_config_callback(const char *var, const char *value,\n> >> > \t\tchar *url = xmemdupz(key, dot - key);\n> >> > \t\tint matched;\n> >> >\n> >> > -\t\tcredential_from_url(&want, url);\n> >> > +\t\tif (credential_from_potentially_partial_url(&want, url) < 0) {\n> >> > +\t\t\twarning(_(\"skipping credential lookup for key: %s\"),\n> >> > +\t\t\t\tvar);\n> >> > +\t\t\tcredential_clear(&want);\n> >> > +\t\t\tfree(url);\n> >> > +\t\t\treturn 0;\n> >> > +\t\t}\n> >> > \t\tmatched = credential_match(&want, c);\n> >>\n> >> Unfortunately, the whole section of this code that is being patched\n> >> here goes away with 46fd7b39 (credential: allow wildcard patterns\n> >> when matching config, 2020-02-20), that delegates large part of the\n> >> logic to urlmatch.\n> >>\n> >> Dscho and Brian, how do we want to proceed? As the conflicting\n> >> change has already been in 'next' for more than a month and a half,\n> >> we'd need this \"partial URL\" logic build to work well with it.\n> >\n> > I prepared a PR: https://github.com/gitgitgadget/git/pull/620. I do not\n> > necessarily feel 100% comfortable with that approach yet, but at least it\n> > lets the new test case of t0300 pass.\n>\n> The -2.17 patch series (your [v3 3/3]) ends t0300 like so\n>\n> +\tdone &&\n> +\n> +\tgit -c credential.$partial.helper=yep \\\n> +\t\t-c credential.with%0anewline.username=uh-oh \\\n> +\t\tcredential fill stdout 2>stderr &&\n> +\ttest_i18ngrep \"skipping credential lookup for key\" stderr\n> +\n> +'\n> +\n> test_done\n>\n> while the other branch lacks the extra blank line just before the\n> single quote is closed. I queued 850ef627 (SQUASH??? lose excess\n> blank line to match the other side of the eventual merge,\n> 2020-04-24) on top so that the \"-s ours\" merge would be without\n> unnecessary evil-merge noise. You probably would want to squash it\n> in.\n\nThank you for pointing that out. I already did that:\nhttps://github.com/gitgitgadget/git/compare/0535908dd7ea4487b342c0f86182579279c57b34..d59738ecf741a5fddd70f133eaaf69768e245bcc\n\nDo you want me to send another iteration, for completeness' sake?\n\nCiao,\nDscho\n"},{"id":"396558","messageId":"xmqq8siez4m2.fsf@gitster.c.googlers.com","threadId":"53289","inReplyTo":"nycvar.QRO.7.76.6.2004291657220.18039@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 3/3] credential: handle `credential..` again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-29T15:36:05Z","receivedAt":"2020-04-29T15:36:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> while the other branch lacks the extra blank line just before the\n>> single quote is closed. I queued 850ef627 (SQUASH??? ...\n>\n> Thank you for pointing that out. I already did that: ...\n> Do you want me to send another iteration, for completeness' sake?\n\nThen if I squash 850ef627 on my end, we'd be in sync, so unless\nthere is any other change, no need to resend it. Have we got enough\neyeballs that looked at the patches already?\n\nThanks.\n\n"},{"id":"396780","messageId":"nycvar.QRO.7.76.6.2005012156220.18039@tvgsbejvaqbjf.bet","threadId":"53289","inReplyTo":"xmqq8siez4m2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 3/3] credential: handle `credential..` again","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-01T19:58:49Z","receivedAt":"2020-05-01T19:59:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 29 Apr 2020, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> >> while the other branch lacks the extra blank line just before the\n> >> single quote is closed. I queued 850ef627 (SQUASH??? ...\n> >\n> > Thank you for pointing that out. I already did that: ...\n> > Do you want me to send another iteration, for completeness' sake?\n>\n> Then if I squash 850ef627 on my end, we'd be in sync, so unless\n> there is any other change, no need to resend it.\n\nPerfect, thanks!\n\n> Have we got enough eyeballs that looked at the patches already?\n\nI would obviously prefer more reviews.\n\nHaving said that, GitHub Desktop rolled out a new release this past Monday\nwith the MinGit backport I prepared with these here patches, and the only\nadditional report at https://github.com/desktop/desktop/issues/9597 talks\nabout macOS (and the MinGit backport is Windows-only). Which I take as\ngood news for the robustness and correctness of the fixes.\n\nCiao,\nDscho\n"},{"id":"396781","messageId":"xmqqo8r7l914.fsf@gitster.c.googlers.com","threadId":"53289","inReplyTo":"nycvar.QRO.7.76.6.2005012156220.18039@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v3 3/3] credential: handle `credential..` again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-01T20:01:11Z","receivedAt":"2020-05-01T20:01:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I would obviously prefer more reviews.\n>\n> Having said that, GitHub Desktop rolled out a new release this past Monday\n> with the MinGit backport I prepared with these here patches, and the only\n> additional report at https://github.com/desktop/desktop/issues/9597 talks\n> about macOS (and the MinGit backport is Windows-only). Which I take as\n> good news for the robustness and correctness of the fixes.\n\nGood. My preference actually is to fast-track this down to 'master'\nwithout the usual 'at least about a week in next' gestation period.\n"}]}