{"thread":{"id":"58495","subject":"[PATCH 0/3] Correct credential helper discrepancies handling input","startedAt":"2022-09-22T16:59:45Z","lastAt":"2022-09-22T22:11:55Z","messageCount":8,"participants":["Matthew John Cheetham via GitGitGadget","Derrick Stolee","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"463452","messageId":"pull.1363.git.1663865974.gitgitgadget@gmail.com","threadId":"58495","inReplyTo":null,"subject":"[PATCH 0/3] Correct credential helper discrepancies handling input","fromName":"Matthew John Cheetham via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-22T16:59:31Z","receivedAt":"2022-09-22T16:59:45Z","isPatch":true,"sender":{"key":"mjcheetham@outlook.com","avatar":"https://avatars.githubusercontent.com/u/5658207?v=4"},"body":"Whilst working in the credential helper and auth space I happened to notice\nthat the behaviour of some of the credential helpers in contrib/credential/\nis not consistent.\n\nSpecifically both the git-credential-wincred and git-credential-netrc\nhelpers die when they receive unknown credential property keys, which is\ncontrary to the behaviour of all the other in-tree helpers including:\ngit-credential-cache, -store, -libsecret, -gnome-keyring, -osxkeychain.\n\nAlso update the git-credential-osxkeychain helper to include a comment\nmaking it's behaviour explicit in ignoring unknown keys, as per other\nhelpers.\n\nMatthew John Cheetham (3):\n wincred: ignore unknown lines (do not die)\n netrc: ignore unknown lines (do not die)\n osxkeychain: clarify that we ignore unknown lines\n\n contrib/credential/netrc/git-credential-netrc.perl | 5 ++++-\n .../credential/osxkeychain/git-credential-osxkeychain.c | 5 +++++\n contrib/credential/wincred/git-credential-wincred.c | 7 +++++--\n 3 files changed, 14 insertions(+), 3 deletions(-)\n\n\nbase-commit: dd3f6c4cae7e3b15ce984dce8593ff7569650e24\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1363%2Fmjcheetham%2Fcredhelper-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1363/mjcheetham/credhelper-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1363\n-- \ngitgitgadget\n"},{"id":"463453","messageId":"6426f9c3954866b3fd9259d1a58d2c41dc42e17f.1663865974.git.gitgitgadget@gmail.com","threadId":"58495","inReplyTo":"pull.1363.git.1663865974.gitgitgadget@gmail.com","subject":"[PATCH 1/3] wincred: ignore unknown lines (do not die)","fromName":"Matthew John Cheetham via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-22T16:59:32Z","receivedAt":"2022-09-22T16:59:50Z","isPatch":true,"sender":{"key":"mjcheetham@outlook.com","avatar":"https://avatars.githubusercontent.com/u/5658207?v=4"},"body":"From: Matthew John Cheetham \n\nIt is the expectation that credential helpers be liberal in what they\naccept and conservative in what they return, to allow for future growth\nand evolution of the protocol/interaction.\n\nAll of the other helpers (store, cache, osxkeychain, libsecret,\ngnome-keyring) except `netrc` currently ignore any credential lines\nthat are not recognised, whereas the Windows helper (wincred) instead\ndies.\n\nFix the discrepancy and ignore unknown lines in the wincred helper.\n\nSigned-off-by: Matthew John Cheetham \n---\n contrib/credential/wincred/git-credential-wincred.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c\nindex 5091048f9c6..ead6e267c78 100644\n--- a/contrib/credential/wincred/git-credential-wincred.c\n+++ b/contrib/credential/wincred/git-credential-wincred.c\n@@ -278,8 +278,11 @@ static void read_credential(void)\n \t\t\twusername = utf8_to_utf16_dup(v);\n \t\t} else if (!strcmp(buf, \"password\"))\n \t\t\tpassword = utf8_to_utf16_dup(v);\n-\t\telse\n-\t\t\tdie(\"unrecognized input\");\n+\t\t/*\n+\t\t * Ignore other lines; we don't know what they mean, but\n+\t\t * this future-proofs us when later versions of git do\n+\t\t * learn new lines, and the helpers are updated to match.\n+\t\t */\n \t}\n }\n \n-- \ngitgitgadget\n\n"},{"id":"463454","messageId":"ae5c1bfc092e98b810757e752efd7cfde48a3809.1663865974.git.gitgitgadget@gmail.com","threadId":"58495","inReplyTo":"pull.1363.git.1663865974.gitgitgadget@gmail.com","subject":"[PATCH 2/3] netrc: ignore unknown lines (do not die)","fromName":"Matthew John Cheetham via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-22T16:59:33Z","receivedAt":"2022-09-22T16:59:59Z","isPatch":true,"sender":{"key":"mjcheetham@outlook.com","avatar":"https://avatars.githubusercontent.com/u/5658207?v=4"},"body":"From: Matthew John Cheetham \n\nContrary to the documentation on credential helpers, as well as the help\ntext for git-credential-netrc itself, this helper will `die` when\npresented with an unknown property/attribute/token.\n\nCorrect the behaviour here by skipping and ignoring any tokens that are\nunknown. This means all helpers in the tree are consistent and ignore\nany unknown credential properties/attributes.\n\nSigned-off-by: Matthew John Cheetham \n---\n contrib/credential/netrc/git-credential-netrc.perl | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl\nindex bc57cc65884..9fb998ae090 100755\n--- a/contrib/credential/netrc/git-credential-netrc.perl\n+++ b/contrib/credential/netrc/git-credential-netrc.perl\n@@ -356,7 +356,10 @@ sub read_credential_data_from_stdin {\n \t\tnext unless m/^([^=]+)=(.+)/;\n \n \t\tmy ($token, $value) = ($1, $2);\n-\t\tdie \"Unknown search token $token\" unless exists $q{$token};\n+\n+\t\t# skip any unknown tokens\n+\t\tnext unless exists $q{$token};\n+\n \t\t$q{$token} = $value;\n \t\tlog_debug(\"We were given search token $token and value $value\");\n \t}\n-- \ngitgitgadget\n\n"},{"id":"463455","messageId":"2ece562a5952b5752de5040b17ec36076164c72f.1663865974.git.gitgitgadget@gmail.com","threadId":"58495","inReplyTo":"pull.1363.git.1663865974.gitgitgadget@gmail.com","subject":"[PATCH 3/3] osxkeychain: clarify that we ignore unknown lines","fromName":"Matthew John Cheetham via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-22T16:59:34Z","receivedAt":"2022-09-22T17:00:01Z","isPatch":true,"sender":{"key":"mjcheetham@outlook.com","avatar":"https://avatars.githubusercontent.com/u/5658207?v=4"},"body":"From: Matthew John Cheetham \n\nLike in all the other credential helpers, the osxkeychain helper\nignores unknown credential lines.\n\nAdd a comment (a la the other helpers) to make it clear and explicit\nthat this is the desired behaviour.\n\nSigned-off-by: Matthew John Cheetham \n---\n contrib/credential/osxkeychain/git-credential-osxkeychain.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\nindex bf77748d602..e29cc28779d 100644\n--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c\n@@ -159,6 +159,11 @@ static void read_credential(void)\n \t\t\tusername = xstrdup(v);\n \t\telse if (!strcmp(buf, \"password\"))\n \t\t\tpassword = xstrdup(v);\n+\t\t/*\n+\t\t * Ignore other lines; we don't know what they mean, but\n+\t\t * this future-proofs us when later versions of git do\n+\t\t * learn new lines, and the helpers are updated to match.\n+\t\t */\n \t}\n }\n \n-- \ngitgitgadget\n"},{"id":"463457","messageId":"0e3bafd2-b331-96af-b379-f00a27909da6@github.com","threadId":"58495","inReplyTo":"pull.1363.git.1663865974.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Correct credential helper discrepancies handling input","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-22T18:31:16Z","receivedAt":"2022-09-22T18:33:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/22/2022 12:59 PM, Matthew John Cheetham via GitGitGadget wrote:\n> Whilst working in the credential helper and auth space I happened to notice\n> that the behaviour of some of the credential helpers in contrib/credential/\n> is not consistent.\n> \n> Specifically both the git-credential-wincred and git-credential-netrc\n> helpers die when they receive unknown credential property keys, which is\n> contrary to the behaviour of all the other in-tree helpers including:\n> git-credential-cache, -store, -libsecret, -gnome-keyring, -osxkeychain.\n> \n> Also update the git-credential-osxkeychain helper to include a comment\n> making it's behaviour explicit in ignoring unknown keys, as per other\n> helpers.\n\nThanks for sending this patches separate from your earlier RFC. I\nthink these patches speak for themselves and are good to go.\n\nThanks,\n-Stolee\n\n"},{"id":"463476","messageId":"xmqqbkr7xg28.fsf@gitster.g","threadId":"58495","inReplyTo":"6426f9c3954866b3fd9259d1a58d2c41dc42e17f.1663865974.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] wincred: ignore unknown lines (do not die)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-22T21:19:43Z","receivedAt":"2022-09-22T21:19:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matthew John Cheetham via GitGitGadget\" \nwrites:\n\n> From: Matthew John Cheetham \n>\n> It is the expectation that credential helpers be liberal in what they\n> accept and conservative in what they return, to allow for future growth\n> and evolution of the protocol/interaction.\n\nThat is nice in principle, and the updated code below may work well\nwith existing \"other side of the connection\" (codepaths in \"git\"\nthat asks credential API to talk to the helpers), but I am not sure\nif this is always a safe thing to do.\n\nWhen we gain a new \"command\" in the protocol, if we just read it\nwithout understanding it, would we open ourselves to a risk of\nbreaking the protocol communication, worst of which may be to\ndeadlock? A new command, when received by a more recent helper that\nunderstands how to react to it, may _require_ it to write more than\n\"username\" and \"password\" back to \"git\" from get_credential(), for\nexample, but the helper with this patch alone, while not complaining\nabout seeing such a new and unknown command, would not know how to\ncompute and write that third thing other than \"username\" and\n\"password\"---would the other side who issued that new command get\nstuck waiting for us to return the third thing? Worse yet, the new\ncommand may expect us to read further in get_credential()\n(e.g. maybe they will give us a challenge, which may need to be used\nwhen yielding the \"username\" and \"password\" things), but because we\ndo not even know we need to read, their attempt to write to us may\nget them stuck, and that is when we are expecting to write to them,\neasily leading to a deadlock, no?\n\n> All of the other helpers (store, cache, osxkeychain, libsecret,\n> gnome-keyring) except `netrc` currently ignore any credential lines\n> that are not recognised, whereas the Windows helper (wincred) instead\n> dies.\n\nIs that different from saying \"everybody other than netrc and win\nignore unknown\"?\n\n> Fix the discrepancy and ignore unknown lines in the wincred helper.\n\nOK. As long as everybody consistently ignores, and possibly opens\nthemselves up consistently to a protocol mismatch issue, it is OK.\nWe will know if that can be a real problem or does not happen in\npractice.\n\n> Signed-off-by: Matthew John Cheetham \n> ---\n> contrib/credential/wincred/git-credential-wincred.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c\n> index 5091048f9c6..ead6e267c78 100644\n> --- a/contrib/credential/wincred/git-credential-wincred.c\n> +++ b/contrib/credential/wincred/git-credential-wincred.c\n> @@ -278,8 +278,11 @@ static void read_credential(void)\n> \t\t\twusername = utf8_to_utf16_dup(v);\n> \t\t} else if (!strcmp(buf, \"password\"))\n> \t\t\tpassword = utf8_to_utf16_dup(v);\n> -\t\telse\n> -\t\t\tdie(\"unrecognized input\");\n> +\t\t/*\n> +\t\t * Ignore other lines; we don't know what they mean, but\n> +\t\t * this future-proofs us when later versions of git do\n> +\t\t * learn new lines, and the helpers are updated to match.\n> +\t\t */\n> \t}\n> }\n"},{"id":"463477","messageId":"YyzdFBaiWgCJzMWX@coredump.intra.peff.net","threadId":"58495","inReplyTo":"xmqqbkr7xg28.fsf@gitster.g","subject":"Re: [PATCH 1/3] wincred: ignore unknown lines (do not die)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-22T22:09:24Z","receivedAt":"2022-09-22T22:09:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 22, 2022 at 02:19:43PM -0700, Junio C Hamano wrote:\n\n> > It is the expectation that credential helpers be liberal in what they\n> > accept and conservative in what they return, to allow for future growth\n> > and evolution of the protocol/interaction.\n> \n> That is nice in principle, and the updated code below may work well\n> with existing \"other side of the connection\" (codepaths in \"git\"\n> that asks credential API to talk to the helpers), but I am not sure\n> if this is always a safe thing to do.\n> \n> When we gain a new \"command\" in the protocol, if we just read it\n> without understanding it, would we open ourselves to a risk of\n> breaking the protocol communication, worst of which may be to\n> deadlock? A new command, when received by a more recent helper that\n> understands how to react to it, may _require_ it to write more than\n> \"username\" and \"password\" back to \"git\" from get_credential(), for\n> example, but the helper with this patch alone, while not complaining\n> about seeing such a new and unknown command, would not know how to\n> compute and write that third thing other than \"username\" and\n> \"password\"---would the other side who issued that new command get\n> stuck waiting for us to return the third thing? Worse yet, the new\n> command may expect us to read further in get_credential()\n> (e.g. maybe they will give us a challenge, which may need to be used\n> when yielding the \"username\" and \"password\" things), but because we\n> do not even know we need to read, their attempt to write to us may\n> get them stuck, and that is when we are expecting to write to them,\n> easily leading to a deadlock, no?\n\nThis open-endedness was an intentional part of the original\ncredential-helper design. Helpers are always \"best effort\", and it is OK\nif they ignore a request, or return a partial result. If the sending\nside really wants to extend the protocol in a way that the other side\ndoesn't act at all (say, they \"username\" to be used _only_ if the helper\nalso understands a new \"foobar\" key), then they should be adding the new\nfields as an atomic unit. I.e., \"foobar\", \"foobar-userame\", and so on.\nAnd then existing helpers which don't understand the new feature will\njust ignore it totally.\n\nIn practice, I think it's not that big a deal either way, because the\nsetup of these helpers is under the control of the user. So if you\nreally want to use \"foobar\" but a helper doesn't support it, then you'd\njust remove that helper from your config.\n\n-Peff\n"},{"id":"463479","messageId":"Yyzdp21kNuLqhM0X@coredump.intra.peff.net","threadId":"58495","inReplyTo":"pull.1363.git.1663865974.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Correct credential helper discrepancies handling input","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-22T22:11:51Z","receivedAt":"2022-09-22T22:11:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 22, 2022 at 04:59:31PM +0000, Matthew John Cheetham via GitGitGadget wrote:\n\n> Whilst working in the credential helper and auth space I happened to notice\n> that the behaviour of some of the credential helpers in contrib/credential/\n> is not consistent.\n> \n> Specifically both the git-credential-wincred and git-credential-netrc\n> helpers die when they receive unknown credential property keys, which is\n> contrary to the behaviour of all the other in-tree helpers including:\n> git-credential-cache, -store, -libsecret, -gnome-keyring, -osxkeychain.\n> \n> Also update the git-credential-osxkeychain helper to include a comment\n> making it's behaviour explicit in ignoring unknown keys, as per other\n> helpers.\n\nThanks, I think these are all sensible changes. The new behavior matches\nthe original vision of the helper protocol. But more importantly, it\nbrings them in line with all of the _other_ existing helpers. Which is\nno coincidence, since the designer of the protocol wrote most of them. ;)\n\nI haven't had a chance to look carefully at the rest of your larger\nseries yet, but I think these are a nice standalone improvement\nregardless of what happens there.\n\n-Peff\n"}]}