{"thread":{"id":"53326","subject":"[PATCH] git-credential-store: skip empty lines and comments from store","startedAt":"2020-04-26T23:52:27Z","lastAt":"2020-05-03T10:06:52Z","messageCount":79,"participants":["Carlo Marcelo Arenas Belón","Eric Sunshine","Jeff King","Philip Oakley","Dirk","Junio C Hamano","Jonathan Nieder","Stefan Tauner","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"396308","messageId":"20200426234750.40418-1-carenas@gmail.com","threadId":"53326","inReplyTo":null,"subject":"[PATCH] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-26T23:47:50Z","receivedAt":"2020-04-26T23:52:27Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"with the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported as failing[1] to parse as valid credentials.\n\ninstead of passing every line to the matcher as read, trim them\nfrom spaces and skip the ones that will be otherwise empty or\nstart with \"#\" (assumed to be comments)\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n credential-store.c | 3 +++\n t/t0302-credential-store.sh | 17 +++++++++++++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..b2f160890d 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n+\t\tstrbuf_trim(&line);\n+\t\tif (line.len == 0 || *line.buf == '#')\n+\t\t\tcontinue;\n \t\tcredential_from_url(&entry, line.buf);\n \t\tif (entry.username && entry.password &&\n \t\t credential_match(c, &entry)) {\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..7245d2f449 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,4 +120,21 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_success 'get: allow for empty lines or comments in store file' '\n+\techo \"#this is a comment\" >\"$HOME/.git-credentials\" &&\n+\techo \"\" >>\"$HOME/.git-credentials\" &&\n+\techo \"https://user:pass@example.com\" >>\"$HOME/.git-credentials\" &&\n+\techo \" \" >>\"$HOME/.git-credentials\" &&\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\t--\n+\tEOF\n+'\n+\n test_done\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396310","messageId":"CAPig+cR8HKcbNxxw65ERz0iHvnO5aC6RXvF9NjvFTySXpcHCSQ@mail.gmail.com","threadId":"53326","inReplyTo":"20200426234750.40418-1-carenas@gmail.com","subject":"Re: [PATCH] git-credential-store: skip empty lines and comments from store","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-27T00:19:35Z","receivedAt":"2020-04-27T00:19:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[Cc:+Dirk -- so he knows his bug report[1] wasn't ignored]\n\nOn Sun, Apr 26, 2020 at 7:48 PM Carlo Marcelo Arenas Belón\n wrote:\n> with the added checks for invalid URLs in credentials, any locally\n> modified store files which might have empty lines or even comments\n> were reported as failing[1] to parse as valid credentials.\n>\n> instead of passing every line to the matcher as read, trim them\n> from spaces and skip the ones that will be otherwise empty or\n> start with \"#\" (assumed to be comments)\n>\n> Reported-by: Dirk \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> @@ -120,4 +120,21 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n> +test_expect_success 'get: allow for empty lines or comments in store file' '\n> + echo \"#this is a comment\" >\"$HOME/.git-credentials\" &&\n> + echo \"\" >>\"$HOME/.git-credentials\" &&\n> + echo \"https://user:pass@example.com\" >>\"$HOME/.git-credentials\" &&\n> + echo \" \" >>\"$HOME/.git-credentials\" &&\n\nIs there a reason you don't use a here-doc for the above (which would\nbe less noisy)? For instance:\n\n q_to_tab >\"$HOME/.git-credentials\" <<-\\EOF &&\n #this is a comment\n\n https://user:pass@example.com\n Q\n EOF\n\n[1]: https://lore.kernel.org/git/ad80aa0d-3a35-6d7e-7958-b3520e16c855@ed4u.de/\n"},{"id":"396311","messageId":"20200427004640.GA28818@Carlos-MBP","threadId":"53326","inReplyTo":"CAPig+cR8HKcbNxxw65ERz0iHvnO5aC6RXvF9NjvFTySXpcHCSQ@mail.gmail.com","subject":"Re: [PATCH] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-27T00:46:40Z","receivedAt":"2020-04-27T00:46:47Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sun, Apr 26, 2020 at 08:19:35PM -0400, Eric Sunshine wrote:\n> [Cc:+Dirk -- so he knows his bug report[1] wasn't ignored]\n> On Sun, Apr 26, 2020 at 7:48 PM Carlo Marcelo Arenas Belón\n> wrote:\n> > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> > @@ -120,4 +120,21 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n> > +test_expect_success 'get: allow for empty lines or comments in store file' '\n> > + echo \"#this is a comment\" >\"$HOME/.git-credentials\" &&\n> > + echo \"\" >>\"$HOME/.git-credentials\" &&\n> > + echo \"https://user:pass@example.com\" >>\"$HOME/.git-credentials\" &&\n> > + echo \" \" >>\"$HOME/.git-credentials\" &&\n> \n> Is there a reason you don't use a here-doc for the above (which would\n> be less noisy)? For instance:\n\nbecause I didn't knew it existed ;), thanks for your help and will include\nfor next version.\n\nCarlo\n"},{"id":"396319","messageId":"20200427084235.60798-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200426234750.40418-1-carenas@gmail.com","subject":"[PATCH v2] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-27T08:42:35Z","receivedAt":"2020-04-27T08:43:52Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"with the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported failing[1] to parse as valid credentials.\n\ninstead of passing every line to the matcher for processing, trim\nthem from spaces and skip the ones that will be otherwise empty or\nthat start with \"#\" (assumed to be comments)\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv2:\n* use a here-doc for clarity as suggested by Eric\n* improve commit message and include documentation\n\n Documentation/git-credential-store.txt | 7 +++++++\n credential-store.c | 3 +++\n t/t0302-credential-store.sh | 19 +++++++++++++++++++\n 3 files changed, 29 insertions(+)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 693dd9d9d7..7f7b53e4da 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -101,6 +101,13 @@ username (if we already have one) match, then the password is returned\n to Git. See the discussion of configuration in linkgit:gitcredentials[7]\n for more information.\n \n+Note that the file used is not a configuration file and should be ideally\n+managed only through git, as any manually introduced typos will compromise\n+the validation of credentials.\n+\n+The parser will ignore any lines starting with the '#' character during\n+the processing of credentials, though.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..b2f160890d 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n+\t\tstrbuf_trim(&line);\n+\t\tif (line.len == 0 || *line.buf == '#')\n+\t\t\tcontinue;\n \t\tcredential_from_url(&entry, line.buf);\n \t\tif (entry.username && entry.password &&\n \t\t credential_match(c, &entry)) {\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..0d13318255 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,4 +120,23 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_success 'get: allow for empty lines or comments in store file' '\n+\tq_to_cr >\"$HOME/.git-credentials\" <<-\\EOF &&\n+\t#this is a comment and the next line contains leading spaces\n+\t Q\n+\thttps://user:pass@example.com\n+\tQ\n+\tEOF\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\t--\n+\tEOF\n+'\n+\n test_done\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396325","messageId":"20200427115223.GA1718141@coredump.intra.peff.net","threadId":"53326","inReplyTo":"20200427084235.60798-1-carenas@gmail.com","subject":"Re: [PATCH v2] git-credential-store: skip empty lines and comments from store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-27T11:52:23Z","receivedAt":"2020-04-27T11:52:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 27, 2020 at 01:42:35AM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> with the added checks for invalid URLs in credentials, any locally\n> modified store files which might have empty lines or even comments\n> were reported failing[1] to parse as valid credentials.\n\nThose were never supposed to work. I'm mildly surprised that they did.\nIt looks like the username/password check here is what prevented us from\nmatching an empty line against a very broad pattern:\n\n while (strbuf_getline_lf(&line, fh) != EOF) {\n credential_from_url(&entry, line.buf);\n if (entry.username && entry.password &&\n credential_match(c, &entry)) {\n found_credential = 1;\n\nAnd there was no such thing as a comment character. E.g., a\n\"commented-out\" url like this:\n\n $ echo \"#https://user:pass@example.com/\" >creds\n\nwould still be matched:\n\n $ echo host=example.com | git credential-store --file creds get\n host=example.com\n username=user\n password=pass\n\nI guess I'm not really opposed to adding this as a feature, but I think\nthe justification should be \"because it is somehow useful\" and not\nbecause it's a bugfix.\n\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> index d6b54e8c65..0d13318255 100755\n> --- a/t/t0302-credential-store.sh\n> +++ b/t/t0302-credential-store.sh\n> @@ -120,4 +120,23 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n> \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n> '\n> \n> +test_expect_success 'get: allow for empty lines or comments in store file' '\n> +\tq_to_cr >\"$HOME/.git-credentials\" <<-\\EOF &&\n> +\t#this is a comment and the next line contains leading spaces\n> +\t Q\n> +\thttps://user:pass@example.com\n> +\tQ\n> +\tEOF\n\nq_to_cr is a little weird here, as we wouldn't expect there to be CRs in\nthe file. They do get removed by strbuf_trim(), even in non-comment\nlines. But perhaps \"sed s/Q//\" would accomplish the same thing (making\nthe whitespace more visible) without making anyone wonder whether the CR\nis an important part of the test?\n\n-Peff\n"},{"id":"396329","messageId":"20200427122520.GA61348@Carlos-MBP","threadId":"53326","inReplyTo":"20200427115223.GA1718141@coredump.intra.peff.net","subject":"Re: [PATCH v2] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-27T12:25:20Z","receivedAt":"2020-04-27T12:25:25Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Apr 27, 2020 at 07:52:23AM -0400, Jeff King wrote:\n> On Mon, Apr 27, 2020 at 01:42:35AM -0700, Carlo Marcelo Arenas Belón wrote:\n> \n> > with the added checks for invalid URLs in credentials, any locally\n> > modified store files which might have empty lines or even comments\n> > were reported failing[1] to parse as valid credentials.\n> \n> Those were never supposed to work. I'm mildly surprised that they did.\n\nagree on that, which is why I have to admit I couldn't find the right\nwording for the previous paragraph, and also why I didn't pinpoint a\nspecific commit as the one that introduced a bug.\n\nit was instead just meant to describe a \"current state of affairs\" and\nI was hoping the documentation entry will help guide future users to be\nmore careful with this file.\n\n> I guess I'm not really opposed to adding this as a feature, but I think\n> the justification should be \"because it is somehow useful\" and not\n> because it's a bugfix.\n\nI think that is a matter of semantics, because for some users it seems\nlike a regression on the last bugfix and therefore might be used (albeit\nIMHO incorrectly) as an excuse not to upgrade, which will be even worst.\n\nso yes, I am implementint this \"feature\" to prevent them to have an\nexcuse but it also means we would most likely need to fastrack this\ninto maint.\n\n> > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> > index d6b54e8c65..0d13318255 100755\n> > --- a/t/t0302-credential-store.sh\n> > +++ b/t/t0302-credential-store.sh\n> > @@ -120,4 +120,23 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n> > \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n> > '\n> > \n> > +test_expect_success 'get: allow for empty lines or comments in store file' '\n> > +\tq_to_cr >\"$HOME/.git-credentials\" <<-\\EOF &&\n> > +\t#this is a comment and the next line contains leading spaces\n> > +\t Q\n> > +\thttps://user:pass@example.com\n> > +\tQ\n> > +\tEOF\n> \n> q_to_cr is a little weird here, as we wouldn't expect there to be CRs in\n> the file. They do get removed by strbuf_trim(), even in non-comment\n> lines. But perhaps \"sed s/Q//\" would accomplish the same thing (making\n> the whitespace more visible) without making anyone wonder whether the CR\n> is an important part of the test?\n\nI know, but commiting a line with 1 tab and 4 empty spaces instead of\nusing Q seemed even worst and q_to_cr almost fits the bill and might\neven make this test \"windows compatible\" for once ;)\n\nwill rewrite then using test_write_lines and I hope it is still as\nreadable without having to resort to the originl ugly chain of echo\n\nCarlo\n"},{"id":"396331","messageId":"20200427125915.88667-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200427084235.60798-1-carenas@gmail.com","subject":"[PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-27T12:59:15Z","receivedAt":"2020-04-27T12:59:54Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"with the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\nusing the store file in this manner wasn't intended by the original\ncode and it had latent issues which the new code dutifully prevented\nbut since the strings used wouldn't had been valid credentials anyway\nwe could instead detect them and skip the matching logic and therefore\nformalize this \"feature\".\n\ntrim all lines as they are being read from the store file and skip the\nones that will be otherwise empty or that start with \"#\" (therefore\nassuming them to be comments)\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv3:\n* avoid using q_to_cr as suggested by Peff\n* a more verbose commit message and slightly more complete documentation \nv2:\n* use a here-doc for clarity as suggested by Eric\n* improve commit message and include documentation\n\n Documentation/git-credential-store.txt | 7 +++++++\n credential-store.c | 3 +++\n t/t0302-credential-store.sh | 15 +++++++++++++++\n 3 files changed, 25 insertions(+)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 693dd9d9d7..48ab4b13e5 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -101,6 +101,13 @@ username (if we already have one) match, then the password is returned\n to Git. See the discussion of configuration in linkgit:gitcredentials[7]\n for more information.\n \n+Note that the file used is not a configuration file and should be ideally\n+managed only through git, as any manually introduced typos will compromise\n+the validation of credentials.\n+\n+The parser will ignore any lines starting with the '#' character during\n+the processing of credentials for read, though.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..b2f160890d 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n+\t\tstrbuf_trim(&line);\n+\t\tif (line.len == 0 || *line.buf == '#')\n+\t\t\tcontinue;\n \t\tcredential_from_url(&entry, line.buf);\n \t\tif (entry.username && entry.password &&\n \t\t credential_match(c, &entry)) {\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..5e6ace3a06 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_success 'get: allow for empty lines or comments in store file' '\n+\ttest_write_lines \"#comment\" \" \" \"\" \\\n+\t\t https://user:pass@example.com >\"$HOME/.git-credentials\" &&\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\t--\n+\tEOF\n+'\n+\n test_done\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396333","messageId":"de8523c6-b8f0-b12d-20e4-b2418bbc18eb@iee.email","threadId":"53326","inReplyTo":"20200427125915.88667-1-carenas@gmail.com","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-04-27T13:48:05Z","receivedAt":"2020-04-27T13:48:13Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi Carlo,\nOn 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote:\n> with the added checks for invalid URLs in credentials, any locally\n> modified store files which might have empty lines or even comments\n> were reported[1] failing to parse as valid credentials.\n>\n> using the store file in this manner wasn't intended by the original\n> code and it had latent issues which the new code dutifully prevented\n> but since the strings used wouldn't had been valid credentials anyway\n> we could instead detect them and skip the matching logic and therefore\n> formalize this \"feature\".\n>\n> trim all lines as they are being read from the store file and skip the\n\nDoes trimming affect any credentials that may have spaces either end?\nI've not looked at the format.\n> ones that will be otherwise empty or that start with \"#\" (therefore\n> assuming them to be comments)\n>\n> [1] https://stackoverflow.com/a/61420852/5005936\n>\n> Reported-by: Dirk \n> Helped-by: Eric Sunshine \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> v3:\n> * avoid using q_to_cr as suggested by Peff\n> * a more verbose commit message and slightly more complete documentation \n> v2:\n> * use a here-doc for clarity as suggested by Eric\n> * improve commit message and include documentation\n>\n> Documentation/git-credential-store.txt | 7 +++++++\n> credential-store.c | 3 +++\n> t/t0302-credential-store.sh | 15 +++++++++++++++\n> 3 files changed, 25 insertions(+)\n>\n> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\n> index 693dd9d9d7..48ab4b13e5 100644\n> --- a/Documentation/git-credential-store.txt\n> +++ b/Documentation/git-credential-store.txt\n> @@ -101,6 +101,13 @@ username (if we already have one) match, then the password is returned\n> to Git. See the discussion of configuration in linkgit:gitcredentials[7]\n> for more information.\n> \n> +Note that the file used is not a configuration file and should be ideally\n> +managed only through git, as any manually introduced typos will compromise\n> +the validation of credentials.\n> +\n> +The parser will ignore any lines starting with the '#' character during\n> +the processing of credentials for read, though.\nShould the trimming of leading/trailing spaces be mentioned?\n\nAlso the git-credential page mentions that the credential must end with\na blank line (\"don't forget..\"). Should that be mentioned here, or have\nI misunderstood?\n> +\n> GIT\n> ---\n> Part of the linkgit:git[1] suite\n> diff --git a/credential-store.c b/credential-store.c\n> index c010497cb2..b2f160890d 100644\n> --- a/credential-store.c\n> +++ b/credential-store.c\n> @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn,\n> \t}\n> \n> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> +\t\tstrbuf_trim(&line);\n> +\t\tif (line.len == 0 || *line.buf == '#')\n> +\t\t\tcontinue;\n> \t\tcredential_from_url(&entry, line.buf);\n> \t\tif (entry.username && entry.password &&\n> \t\t credential_match(c, &entry)) {\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> index d6b54e8c65..5e6ace3a06 100755\n> --- a/t/t0302-credential-store.sh\n> +++ b/t/t0302-credential-store.sh\n> @@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n> \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n> '\n> \n> +test_expect_success 'get: allow for empty lines or comments in store file' '\n> +\ttest_write_lines \"#comment\" \" \" \"\" \\\n> +\t\t https://user:pass@example.com >\"$HOME/.git-credentials\" &&\n> +\tcheck fill store <<-\\EOF\n> +\tprotocol=https\n> +\thost=example.com\n> +\t--\n> +\tprotocol=https\n> +\thost=example.com\n> +\tusername=user\n> +\tpassword=pass\n> +\t--\n> +\tEOF\n> +'\n> +\n> test_done\nPhilip\n"},{"id":"396339","messageId":"CAPig+cRy9T43yXsuj-_vPJoROe8xYRMExxnCxRmG6kij+ykxKg@mail.gmail.com","threadId":"53326","inReplyTo":"20200427122520.GA61348@Carlos-MBP","subject":"Re: [PATCH v2] git-credential-store: skip empty lines and comments from store","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-27T14:43:01Z","receivedAt":"2020-04-27T14:43:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Apr 27, 2020 at 8:25 AM Carlo Marcelo Arenas Belón\n wrote:\n> On Mon, Apr 27, 2020 at 07:52:23AM -0400, Jeff King wrote:\n> > On Mon, Apr 27, 2020 at 01:42:35AM -0700, Carlo Marcelo Arenas Belón wrote:\n> > > +test_expect_success 'get: allow for empty lines or comments in store file' '\n> > > + q_to_cr >\"$HOME/.git-credentials\" <<-\\EOF &&\n> > > + #this is a comment and the next line contains leading spaces\n> > > + Q\n> > > + https://user:pass@example.com\n> > > + Q\n> > > + EOF\n> >\n> > q_to_cr is a little weird here, as we wouldn't expect there to be CRs in\n> > the file. They do get removed by strbuf_trim(), even in non-comment\n> > lines. But perhaps \"sed s/Q//\" would accomplish the same thing (making\n> > the whitespace more visible) without making anyone wonder whether the CR\n> > is an important part of the test?\n>\n> I know, but commiting a line with 1 tab and 4 empty spaces instead of\n> using Q seemed even worst and q_to_cr almost fits the bill and might\n> even make this test \"windows compatible\" for once ;)\n\nHmm? In the proposal[1], q_to_tab() was used, not q_to_cr(), and\nq_to_tab() seemed to fit quite well as a replacement for the original\n'echo' chain[2] which emitted one whitespace-only line. Substituting\nthat whitespace-only line with \"Q\" (which gets translated into a TAB)\nseems to fit much better than q_to_cr().\n\n> will rewrite then using test_write_lines and I hope it is still as\n> readable without having to resort to the originl ugly chain of echo\n\ntest_write_lines() doesn't do a very good job of representing -- at a\nglance -- what the \"expected\" output should be. The q_to_tab() block\nmade it much more clear.\n\n(Having said that, it's probably not worth another re-roll just to change that.)\n\n[1]: https://lore.kernel.org/git/CAPig+cR8HKcbNxxw65ERz0iHvnO5aC6RXvF9NjvFTySXpcHCSQ@mail.gmail.com/\n[2]: https://lore.kernel.org/git/20200426234750.40418-1-carenas@gmail.com/\n"},{"id":"396340","messageId":"9db4fc9c-94df-076d-0fe4-61bcfb73506e@ed4u.de","threadId":"53326","inReplyTo":"20200427125915.88667-1-carenas@gmail.com","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Dirk","fromEmail":"dirk@ed4u.de","sentAt":"2020-04-27T15:39:19Z","receivedAt":"2020-04-27T15:39:37Z","isPatch":true,"sender":{"key":"dirk@ed4u.de","avatar":null},"body":"Thank you very much. That's correct and ideal in my eyes.\n\nRegarding the comments, this is a new feature, of course. I think it's a worthless discussion about the question if the correct handling of empty lines are a bugfix or a new feature. In fact empty lines were handled correcty (ignored). So this might be considered a feature. But it wasn't documented, so it's a bugfix...\n\nAnyway. Thank you all.\n\nDirk\n\n\nAm 27.04.20 um 14:59 schrieb Carlo Marcelo Arenas Belón:\n> with the added checks for invalid URLs in credentials, any locally\n> modified store files which might have empty lines or even comments\n> were reported[1] failing to parse as valid credentials.\n>\n> using the store file in this manner wasn't intended by the original\n> code and it had latent issues which the new code dutifully prevented\n> but since the strings used wouldn't had been valid credentials anyway\n> we could instead detect them and skip the matching logic and therefore\n> formalize this \"feature\".\n>\n> trim all lines as they are being read from the store file and skip the\n> ones that will be otherwise empty or that start with \"#\" (therefore\n> assuming them to be comments)\n>\n> [1] https://stackoverflow.com/a/61420852/5005936\n>\n> Reported-by: Dirk \n> Helped-by: Eric Sunshine \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> v3:\n> * avoid using q_to_cr as suggested by Peff\n> * a more verbose commit message and slightly more complete documentation \n> v2:\n> * use a here-doc for clarity as suggested by Eric\n> * improve commit message and include documentation\n>\n> Documentation/git-credential-store.txt | 7 +++++++\n> credential-store.c | 3 +++\n> t/t0302-credential-store.sh | 15 +++++++++++++++\n> 3 files changed, 25 insertions(+)\n>\n> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\n> index 693dd9d9d7..48ab4b13e5 100644\n> --- a/Documentation/git-credential-store.txt\n> +++ b/Documentation/git-credential-store.txt\n> @@ -101,6 +101,13 @@ username (if we already have one) match, then the password is returned\n> to Git. See the discussion of configuration in linkgit:gitcredentials[7]\n> for more information.\n> \n> +Note that the file used is not a configuration file and should be ideally\n> +managed only through git, as any manually introduced typos will compromise\n> +the validation of credentials.\n> +\n> +The parser will ignore any lines starting with the '#' character during\n> +the processing of credentials for read, though.\n> +\n> GIT\n> ---\n> Part of the linkgit:git[1] suite\n> diff --git a/credential-store.c b/credential-store.c\n> index c010497cb2..b2f160890d 100644\n> --- a/credential-store.c\n> +++ b/credential-store.c\n> @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn,\n> \t}\n> \n> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> +\t\tstrbuf_trim(&line);\n> +\t\tif (line.len == 0 || *line.buf == '#')\n> +\t\t\tcontinue;\n> \t\tcredential_from_url(&entry, line.buf);\n> \t\tif (entry.username && entry.password &&\n> \t\t credential_match(c, &entry)) {\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> index d6b54e8c65..5e6ace3a06 100755\n> --- a/t/t0302-credential-store.sh\n> +++ b/t/t0302-credential-store.sh\n> @@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n> \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n> '\n> \n> +test_expect_success 'get: allow for empty lines or comments in store file' '\n> +\ttest_write_lines \"#comment\" \" \" \"\" \\\n> +\t\t https://user:pass@example.com >\"$HOME/.git-credentials\" &&\n> +\tcheck fill store <<-\\EOF\n> +\tprotocol=https\n> +\thost=example.com\n> +\t--\n> +\tprotocol=https\n> +\thost=example.com\n> +\tusername=user\n> +\tpassword=pass\n> +\t--\n> +\tEOF\n> +'\n> +\n> test_done\n"},{"id":"396360","messageId":"xmqqk12095yh.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200427115223.GA1718141@coredump.intra.peff.net","subject":"Re: [PATCH v2] git-credential-store: skip empty lines and comments from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-27T17:47:02Z","receivedAt":"2020-04-27T17:47:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Those were never supposed to work. I'm mildly surprised that they did.\n\nI know that feeling X-<. It probably was a mistake to pretend that\nthe file were editable with an editor, but it is too late to fix\nthat.\n\nThe next complainer may say that \"; comment\" no longer works. We\nprobably should draw a line somewhere, but I am not sure where.\n"},{"id":"396362","messageId":"xmqqftco94wx.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200427125915.88667-1-carenas@gmail.com","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-27T18:09:34Z","receivedAt":"2020-04-27T18:09:39Z","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> with the added checks for invalid URLs in credentials, any locally\n\ns/with/With/\n\n> modified store files which might have empty lines or even comments\n> were reported[1] failing to parse as valid credentials.\n\nThese files are not supposed to be viewed or edited without the help\nof the credential helpers. Do these blank lines and comments even\nsurvive when a new credential is approved, or do we just overwrite\nand lose them?\n\nI'd rather not to do either, if we did not have to, but if it were\nnecessary for us to do something, I am OK to ignore empty lines.\nBut I'd prefer not to mix the new \"# comment\" feature in, if we did\nnot have to.\n\nAlso, triming the lines that are not empty is unwarranted. IIUC,\nwhat the \"store\" action writes encodes whitespaces, so as soon as\nyou see whitespace on either end, (or anywhere on the line for that\nmatter), it is a hand-edited cruft in the file. If you ignore\ncomments, you probably should ignore those lines, too.\n\n"},{"id":"396366","messageId":"20200427190955.GA1728884@coredump.intra.peff.net","threadId":"53326","inReplyTo":"xmqqk12095yh.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] git-credential-store: skip empty lines and comments from store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-27T19:09:55Z","receivedAt":"2020-04-27T19:09:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 27, 2020 at 10:47:02AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Those were never supposed to work. I'm mildly surprised that they did.\n> \n> I know that feeling X-<. It probably was a mistake to pretend that\n> the file were editable with an editor, but it is too late to fix\n> that.\n\nI guess it is somewhat my fault for documenting the format at all\ngit-credential-store.txt. I probably should have said \"this is a black\nbox and is subject to change\".\n\n> The next complainer may say that \"; comment\" no longer works. We\n> probably should draw a line somewhere, but I am not sure where.\n\nIf we get a bug report for \"credential-store does not respect\ncore.commentChar\" I think my head would explode. :)\n\n-Peff\n"},{"id":"396367","messageId":"20200427191858.GB1728884@coredump.intra.peff.net","threadId":"53326","inReplyTo":"xmqqftco94wx.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-27T19:18:58Z","receivedAt":"2020-04-27T19:19:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote:\n\n> > modified store files which might have empty lines or even comments\n> > were reported[1] failing to parse as valid credentials.\n> \n> These files are not supposed to be viewed or edited without the help\n> of the credential helpers. Do these blank lines and comments even\n> survive when a new credential is approved, or do we just overwrite\n> and lose them?\n\nThat's a good question. In the older code we do save them, because\ncredential-store passes through lines which don't match the credential\nwe're operating on.\n\nBut in Carlo's patch, the immediate \"continue\" means we wouldn't ever\ncall \"other_cb\", which is what does that pass-through.\n\n> I'd rather not to do either, if we did not have to, but if it were\n> necessary for us to do something, I am OK to ignore empty lines.\n> But I'd prefer not to mix the new \"# comment\" feature in, if we did\n> not have to.\n> \n> Also, triming the lines that are not empty is unwarranted. IIUC,\n> what the \"store\" action writes encodes whitespaces, so as soon as\n> you see whitespace on either end, (or anywhere on the line for that\n> matter), it is a hand-edited cruft in the file. If you ignore\n> comments, you probably should ignore those lines, too.\n\nYeah, all of that seems quite sensible.\n\n-Peff\n"},{"id":"396386","messageId":"xmqqv9lk7j7p.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200427191858.GB1728884@coredump.intra.peff.net","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-27T20:43:38Z","receivedAt":"2020-04-27T20:43:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote:\n>\n>> > modified store files which might have empty lines or even comments\n>> > were reported[1] failing to parse as valid credentials.\n>> \n>> These files are not supposed to be viewed or edited without the help\n>> of the credential helpers. Do these blank lines and comments even\n>> survive when a new credential is approved, or do we just overwrite\n>> and lose them?\n>\n> That's a good question. In the older code we do save them, because\n> credential-store passes through lines which don't match the credential\n> we're operating on.\n>\n> But in Carlo's patch, the immediate \"continue\" means we wouldn't ever\n> call \"other_cb\", which is what does that pass-through.\n\nSo, does that mean the patch that started this thread will still help\nusers who wrote custom comments and blank lines in their credential\nstore by letting git-credential-store start up, but leaves a ticking\nbomb for them to lose these precious comments and blanks once they\nadd a new site, change password, etc., at which point the user realizes\nthat comments and blanks are not supported after all?\n\n>> I'd rather not to do either, if we did not have to, but if it were\n>> necessary for us to do something, I am OK to ignore empty lines.\n>> But I'd prefer not to mix the new \"# comment\" feature in, if we did\n>> not have to.\n>> \n>> Also, triming the lines that are not empty is unwarranted. IIUC,\n>> what the \"store\" action writes encodes whitespaces, so as soon as\n>> you see whitespace on either end, (or anywhere on the line for that\n>> matter), it is a hand-edited cruft in the file. If you ignore\n>> comments, you probably should ignore those lines, too.\n>\n> Yeah, all of that seems quite sensible.\n\nI think the first patch we need is a (belated) documentation patch,\nthat adds to the existing \"STORAGE FORMAT\". We already say \"Each\ncredential is stored on its own line as a URL\", but we do not say\nanything about allowing other cruft in the file. We probably\nshould. Adding a \"comment\" feature, if anybody feels like it, is OK\nand we can loosen the paragraph when that happens.\n\n-- >8 --\nSubject: credential-store: document the file format a bit more\n\nReading a malformed credential URL line and silently ignoring it\ndoes not mean that we promise to torelate and/or keep empty lines\nand \"# commented\" lines forever.\n\nSome people seem to take anything that is not explicitly forbidden\nas allowed, but the world does not work that way.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-credential-store.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 693dd9d9d7..76b0798856 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -94,6 +94,10 @@ stored on its own line as a URL like:\n https://user:pass@example.com\n ------------------------------\n \n+No other kinds of lines (e.g. empty lines or comment lines) are\n+allowed in the file, even though some may be silently ignored. Do\n+not view or edit the file with editors.\n+\n When Git needs authentication for a particular URL context,\n credential-store will consider that context a pattern to match against\n each entry in the credentials file. If the protocol, hostname, and\n"},{"id":"396392","messageId":"20200427211013.GB1732530@coredump.intra.peff.net","threadId":"53326","inReplyTo":"xmqqv9lk7j7p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-27T21:10:13Z","receivedAt":"2020-04-27T21:10:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote:\n\n> >> These files are not supposed to be viewed or edited without the help\n> >> of the credential helpers. Do these blank lines and comments even\n> >> survive when a new credential is approved, or do we just overwrite\n> >> and lose them?\n> >\n> > That's a good question. In the older code we do save them, because\n> > credential-store passes through lines which don't match the credential\n> > we're operating on.\n> >\n> > But in Carlo's patch, the immediate \"continue\" means we wouldn't ever\n> > call \"other_cb\", which is what does that pass-through.\n> \n> So, does that mean the patch that started this thread will still help\n> users who wrote custom comments and blank lines in their credential\n> store by letting git-credential-store start up, but leaves a ticking\n> bomb for them to lose these precious comments and blanks once they\n> add a new site, change password, etc., at which point the user realizes\n> that comments and blanks are not supported after all?\n\nYes, exactly. If I start with:\n\n cat >creds <<\\EOF\n # this is a comment\n https://user:pass@example.com/\n EOF\n\nthen:\n\n echo url=https://other:pass@other.example.com |\n git credential-store --file=creds store\n\nwith v2.26.0 results in:\n\n https://other:pass@other.example.com\n # this is a comment\n https://user:pass@example.com\n\nbut applying the patch on top:\n\n https://other:pass@other.example.com\n https://user:pass@example.com/\n\nThat raises another issue about comments, too: we make no promises about\nwhere we write new entries. They could be inserted between a comment\nline and one it is meant to annotate (that line could even be moved if\nwe reject and then re-approve a credential).\n\n> I think the first patch we need is a (belated) documentation patch,\n> that adds to the existing \"STORAGE FORMAT\". We already say \"Each\n> credential is stored on its own line as a URL\", but we do not say\n> anything about allowing other cruft in the file. We probably\n> should. Adding a \"comment\" feature, if anybody feels like it, is OK\n> and we can loosen the paragraph when that happens.\n\nThe more I look into it, the more negative I am on adding such a comment\nfeature.\n\n> -- >8 --\n> Subject: credential-store: document the file format a bit more\n\nYeah, this certainly clarifies things. I suppose one could ask why we'd\nbother documenting the format at all, then, though I do think it could\nbe helpful for debugging.\n\n> Reading a malformed credential URL line and silently ignoring it\n> does not mean that we promise to torelate and/or keep empty lines\n> and \"# commented\" lines forever.\n\ns/torelate/tolerate/\n\n-Peff\n"},{"id":"396410","messageId":"20200427234909.GC61348@Carlos-MBP","threadId":"53326","inReplyTo":"xmqqv9lk7j7p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-27T23:49:09Z","receivedAt":"2020-04-27T23:49:15Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote:\n> >\n> >> > modified store files which might have empty lines or even comments\n> >> > were reported[1] failing to parse as valid credentials.\n> >> \n> >> These files are not supposed to be viewed or edited without the help\n> >> of the credential helpers. Do these blank lines and comments even\n> >> survive when a new credential is approved, or do we just overwrite\n> >> and lose them?\n> >\n> > That's a good question. In the older code we do save them, because\n> > credential-store passes through lines which don't match the credential\n> > we're operating on.\n> >\n> > But in Carlo's patch, the immediate \"continue\" means we wouldn't ever\n> > call \"other_cb\", which is what does that pass-through.\n> \n> So, does that mean the patch that started this thread will still help\n> users who wrote custom comments and blank lines in their credential\n> store by letting git-credential-store start up, but leaves a ticking\n> bomb for them to lose these precious comments and blanks once they\n> add a new site, change password, etc., at which point the user realizes\n> that comments and blanks are not supported after all?\n\nyes, and it also helps users that might have added spaces or tabs\naround their credentials while editing them to still be able to use those\ninstead of just failing to match them.\n\nIMHO the only \"regression\" I was fixing was the fact that the current\ncode will get to throw a fatal error with an unhelpful message and prevent\naccess to valid credentials as shown by :\n\n$ /git credential fill\nprotocol=http\nhost=example.com\n\nwarning: url has no scheme:\nfatal: credential url cannot be parsed:\nUsername for 'http://example.com':\n\n> >> I'd rather not to do either, if we did not have to, but if it were\n> >> necessary for us to do something, I am OK to ignore empty lines.\n> >> But I'd prefer not to mix the new \"# comment\" feature in, if we did\n> >> not have to.\n> >> \n> >> Also, triming the lines that are not empty is unwarranted. IIUC,\n> >> what the \"store\" action writes encodes whitespaces, so as soon as\n> >> you see whitespace on either end, (or anywhere on the line for that\n> >> matter), it is a hand-edited cruft in the file. If you ignore\n> >> comments, you probably should ignore those lines, too.\n> >\n> > Yeah, all of that seems quite sensible.\n> \n> I think the first patch we need is a (belated) documentation patch,\n> that adds to the existing \"STORAGE FORMAT\". We already say \"Each\n> credential is stored on its own line as a URL\", but we do not say\n> anything about allowing other cruft in the file. We probably\n> should. Adding a \"comment\" feature, if anybody feels like it, is OK\n> and we can loosen the paragraph when that happens.\n> \n> -- >8 --\n> Subject: credential-store: document the file format a bit more\n> \n> Reading a malformed credential URL line and silently ignoring it\n> does not mean that we promise to torelate and/or keep empty lines\n> and \"# commented\" lines forever.\n> \n> Some people seem to take anything that is not explicitly forbidden\n> as allowed, but the world does not work that way.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/git-credential-store.txt | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\n> index 693dd9d9d7..76b0798856 100644\n> --- a/Documentation/git-credential-store.txt\n> +++ b/Documentation/git-credential-store.txt\n> @@ -94,6 +94,10 @@ stored on its own line as a URL like:\n> https://user:pass@example.com\n> ------------------------------\n> \n> +No other kinds of lines (e.g. empty lines or comment lines) are\n> +allowed in the file, even though some may be silently ignored. Do\n> +not view or edit the file with editors.\n\nview should be ok; mentioning that any typos or extraneous characters\nwill compromise the validation of credentials like I mentioned in my\nproposed documentation update probably worth doing here instead, too.\n\nCarlo\n"},{"id":"396424","messageId":"20200428013726.GD61348@Carlos-MBP","threadId":"53326","inReplyTo":"20200427211013.GB1732530@coredump.intra.peff.net","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T01:37:26Z","receivedAt":"2020-04-28T01:37:31Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Apr 27, 2020 at 05:10:13PM -0400, Jeff King wrote:\n> On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote:\n> > >> These files are not supposed to be viewed or edited without the help\n> > >> of the credential helpers. Do these blank lines and comments even\n> > >> survive when a new credential is approved, or do we just overwrite\n> > >> and lose them?\n> > >\n> > > That's a good question. In the older code we do save them, because\n> > > credential-store passes through lines which don't match the credential\n> > > we're operating on.\n> > >\n> > > But in Carlo's patch, the immediate \"continue\" means we wouldn't ever\n> > > call \"other_cb\", which is what does that pass-through.\n> > \n> > So, does that mean the patch that started this thread will still help\n> > users who wrote custom comments and blank lines in their credential\n> > store by letting git-credential-store start up, but leaves a ticking\n> > bomb for them to lose these precious comments and blanks once they\n> > add a new site, change password, etc., at which point the user realizes\n> > that comments and blanks are not supported after all?\n> \n> The more I look into it, the more negative I am on adding such a comment\n> feature.\n\nFWIW I am not interested on implementing a comment feature (regardless of\nwhat the subject says, and indeed was going to use quotes around it in my\noriginal patch), but it was already too long ;)\n\nmy objective was to allow people that were not upgrading because of this\n\"regression\" a path forward, but after all this discussion I am convinced\ntoo that my approach is misguided.\n\nif users are already manually editing this file, they should be able to\nfix the corrupted lines themselves but the error shown needs to be improved.\nand the failure mode corrected as well so they are not blocked.\n\nthis also means that my \"sneaky\" way to fix formatting issues is gone\nand the passthrough is called so nothing will be lost, but as you also\nsaid the location is not warranted to be kept so this comment \"feature\"\nis as useless as intended.\n\nwould something like the following (wouldn't pass the last test yet as\nit will need special handling to be able to handle the full path of the\nbroken file as shown in the warning sent to stderr) on top of Junio's\nmake more sense?\n\nit still \"silently\" fixes the formatting issues where a leading tab (test\nadded) or spaces were added to easy the transition, but probably should\nalso error out as Junio suggested originally.\n\nhad also updated the commit message and what is left of the documentation.\n\nCarlo\n-- >8 --\nSubject: [RFC PATCH v4] git-credential-store: skip empty lines and comments\n from store\n\nwith the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\nusing the store file in this manner wasn't intended by the original\ncode and it had latent issues which the new code dutifully prevented\nbut since the strings used wouldn't had been valid credentials anyway\nwe could instead detect them and skip the matching logic and therefore\nprevent a fatal failure.\n\ntrim all lines as they are being read from the store file and skip the\nones that will be otherwise empty or that start with \"#\" (therefore\nassuming them to be comments) but warn users so they will be make\naware of the issues introduced by their manual editing.\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Documentation/git-credential-store.txt | 4 ++++\n credential-store.c | 7 ++++++\n t/t0302-credential-store.sh | 31 ++++++++++++++++++++++++++\n 3 files changed, 42 insertions(+)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 693dd9d9d7..e9a1993021 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -101,6 +101,10 @@ username (if we already have one) match, then the password is returned\n to Git. See the discussion of configuration in linkgit:gitcredentials[7]\n for more information.\n \n+The parser will ignore any lines that are empty or starting with '#' character\n+during the processing of credentials for read, but warn the user so they can\n+be corrected.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..f1f016fec1 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -24,6 +24,13 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n+\t\tstrbuf_trim(&line);\n+\t\tif (line.len == 0 || *line.buf == '#') {\n+\t\t\twarning(\"ignoring corrupted credential entry \\\"%s\\\" in %s\", line.buf, fn);\n+\t\t\tif (other_cb)\n+\t\t\t\tother_cb(&line);\n+\t\t\tcontinue;\n+\t\t}\n \t\tcredential_from_url(&entry, line.buf);\n \t\tif (entry.username && entry.password &&\n \t\t credential_match(c, &entry)) {\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..12a158c136 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,4 +120,35 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_success 'get: attempt to workaround formatting isssues (ex: tab)' '\n+\tq_to_tab >\"$HOME/.git-credentials\" <<-\\EOF &&\n+\tQhttps://user:pass@example.com\n+\tEOF\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'get: skip empty lines or \"comments\" in store file' '\n+\ttest_write_lines \"#comment\" \" \" \"\" \\\n+\t\t https://user:pass@example.com >\"$HOME/.git-credentials\" &&\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\t--\n+\tEOF\n+'\n+\n test_done\n-- \n2.26.2.569.g1d74ac4d14\n\n\n"},{"id":"396425","messageId":"20200428014926.GE61348@Carlos-MBP","threadId":"53326","inReplyTo":"de8523c6-b8f0-b12d-20e4-b2418bbc18eb@iee.email","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T01:49:26Z","receivedAt":"2020-04-28T01:49:30Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Apr 27, 2020 at 02:48:05PM +0100, Philip Oakley wrote:\n> On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote:\n> > with the added checks for invalid URLs in credentials, any locally\n> > modified store files which might have empty lines or even comments\n> > were reported[1] failing to parse as valid credentials.\n> >\n> > using the store file in this manner wasn't intended by the original\n> > code and it had latent issues which the new code dutifully prevented\n> > but since the strings used wouldn't had been valid credentials anyway\n> > we could instead detect them and skip the matching logic and therefore\n> > formalize this \"feature\".\n> >\n> > trim all lines as they are being read from the store file and skip the\n> \n> Does trimming affect any credentials that may have spaces either end?\n\nall credentials are url encoded so the only spaces that are affected are\nthe ones that were added by careless editing of that file.\n\nas Eric pointed out, any tabs will be silently \"cleaned\" as well.\n\n> Should the trimming of leading/trailing spaces be mentioned?\n\nI wanted to keep that as an undocumented \"feature\" as it is just meant\nto help people to avoid a fatal error during the transition but didn't\nwant to encourage people thinking it is a supported part or even worse\nencourage people to start editing their files to add tabs and spaces.\n\nJunio's suggested documentation fix[1] makes that clearer that I could\n\n> Also the git-credential page mentions that the credential must end with\n> a blank line (\"don't forget..\"). Should that be mentioned here, or have\n> I misunderstood?\n\nthat is for the protocol part of it (which is used between git and the\ncredential helper), the file format for this specific helper doesn't\nrequire that.\n\nCarlo\n\n[1] https://lore.kernel.org/git/xmqqv9lk7j7p.fsf@gitster.c.googlers.com/\n"},{"id":"396434","messageId":"20200428052510.GA201501@google.com","threadId":"53326","inReplyTo":"xmqqv9lk7j7p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-28T05:25:10Z","receivedAt":"2020-04-28T05:25:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> -- >8 --\n> Subject: credential-store: document the file format a bit more\n>\n> Reading a malformed credential URL line and silently ignoring it\n> does not mean that we promise to torelate and/or keep empty lines\n> and \"# commented\" lines forever.\n>\n> Some people seem to take anything that is not explicitly forbidden\n> as allowed, but the world does not work that way.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/git-credential-store.txt | 4 ++++\n> 1 file changed, 4 insertions(+)\n>\n> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\n> index 693dd9d9d7..76b0798856 100644\n> --- a/Documentation/git-credential-store.txt\n> +++ b/Documentation/git-credential-store.txt\n> @@ -94,6 +94,10 @@ stored on its own line as a URL like:\n> https://user:pass@example.com\n> ------------------------------\n> \n> +No other kinds of lines (e.g. empty lines or comment lines) are\n> +allowed in the file, even though some may be silently ignored. Do\n> +not view or edit the file with editors.\n> +\n> When Git needs authentication for a particular URL context,\n\nI like this.\n\nI do suspect this is easy to run into accidentally. In $DAYJOB (in\nthe context of [1]) there was a service that accidentally wrote a \\n\\n\nat the end of a line that was used by git-credential-store. Once the\ncause was tracked down, it was straightforward to fix, but I don't\nlike the idea that others in a similar position may end up tempted to\njust not upgrade Git.\n\nIndependently, there is the thread we are replying to.\n\nIndependently, in Debian's bug tracking system, Stefan (cc-ed)\nreports[2]:\n\n| the vulnerability in CVE-2020-11008 is related to the handling\n| of credential helpers in git. In Buster this has been fixed in\n| 1:2.20.1-2+deb10u3. This broke my existing configuration where\n| repositories have credential.helper=store set. This is\n| documented in /usr/share/man/man1/git-credential-store.1.gz\n| and other files from git, git-doc etc.\n| I am unsure how to proceed... is this helper now unsupported?\n\n(Stefan, do you have more details? Did you manually populate your\ncredential store? What error message do you get?)\n\nI wonder if in addition to the above documentation change we may want\nsomething guaranteed to catch all cases where people would have\nexperienced a regression, like\n\ndiff --git i/credential-store.c w/credential-store.c\nindex c010497cb21..294e7716815 100644\n--- i/credential-store.c\n+++ w/credential-store.c\n@@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tcredential_from_url(&entry, line.buf);\n-\t\tif (entry.username && entry.password &&\n+\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n+\t\t entry.username && entry.password &&\n \t\t credential_match(c, &entry)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n\nAnd then we can tighten the handling of unrecognized lines to first\nwarn and then error out, as a controlled change that doesn't lead\npeople to regret updating git.\n\nThoughts?\n\nThanks,\nJonathan\n\n[1] https://cs.opensource.google/copybara/copybara/+/master:java/com/google/copybara/git/GitOptions.java;drc=bc79a0b1ffe18f79dea0b75ba3a24b641a50a9fc;l=46\n[2] https://bugs.debian.org/958929\n"},{"id":"396436","messageId":"20200428054155.GB2376380@coredump.intra.peff.net","threadId":"53326","inReplyTo":"20200428052510.GA201501@google.com","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-28T05:41:55Z","receivedAt":"2020-04-28T05:41:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 27, 2020 at 10:25:10PM -0700, Jonathan Nieder wrote:\n\n> I wonder if in addition to the above documentation change we may want\n> something guaranteed to catch all cases where people would have\n> experienced a regression, like\n> \n> diff --git i/credential-store.c w/credential-store.c\n> index c010497cb21..294e7716815 100644\n> --- i/credential-store.c\n> +++ w/credential-store.c\n> @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,\n> \t}\n> \n> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> -\t\tcredential_from_url(&entry, line.buf);\n> -\t\tif (entry.username && entry.password &&\n> +\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n> +\t\t entry.username && entry.password &&\n> \t\t credential_match(c, &entry)) {\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n> \n> And then we can tighten the handling of unrecognized lines to first\n> warn and then error out, as a controlled change that doesn't lead\n> people to regret updating git.\n\nI like that solution, as it mostly brings us back to the original\nbehavior, as weird or unexpected as it was.\n\nThe only I think would be different is:\n\n ://user:pass@example.com\n\nwhich I think the old code would have treated as matching any protocol\n(and the new code will reject as a bogus URL).\n\nI wondered if you'd need to free the partially-parsed struct fields on\nfailure, but the answer is no; we'd clear it the next time through the\nloop, or at the very end of the function.\n\n-Peff\n"},{"id":"396440","messageId":"20200428071802.GF61348@Carlos-MBP","threadId":"53326","inReplyTo":"20200428054155.GB2376380@coredump.intra.peff.net","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T07:18:02Z","receivedAt":"2020-04-28T07:18:08Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Apr 28, 2020 at 01:41:55AM -0400, Jeff King wrote:\n> On Mon, Apr 27, 2020 at 10:25:10PM -0700, Jonathan Nieder wrote:\n> \n> > I wonder if in addition to the above documentation change we may want\n> > something guaranteed to catch all cases where people would have\n> > experienced a regression, like\n> > \n> > diff --git i/credential-store.c w/credential-store.c\n> > index c010497cb21..294e7716815 100644\n> > --- i/credential-store.c\n> > +++ w/credential-store.c\n> > @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,\n> > \t}\n> > \n> > \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> > -\t\tcredential_from_url(&entry, line.buf);\n> > -\t\tif (entry.username && entry.password &&\n> > +\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n> > +\t\t entry.username && entry.password &&\n> > \t\t credential_match(c, &entry)) {\n> > \t\t\tfound_credential = 1;\n> > \t\t\tif (match_cb) {\n> > \n> > And then we can tighten the handling of unrecognized lines to first\n> > warn and then error out, as a controlled change that doesn't lead\n> > people to regret updating git.\n> \n> I like that solution, as it mostly brings us back to the original\n> behavior, as weird or unexpected as it was.\n\nI like this version better as well, and we could even reuse my test\ncase.\n\nit wouldn't cover cases where there were leading spaces/tabs around\nthe credential which I have to admit I liked just because it is\nmore robust to bad input, and there is no sane way now to tell the\nuser that there is invalid data anyway, but I am ok eitherway.\n\nCarlo\n"},{"id":"396444","messageId":"20200428081627.GB2381876@coredump.intra.peff.net","threadId":"53326","inReplyTo":"20200428071802.GF61348@Carlos-MBP","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-04-28T08:16:27Z","receivedAt":"2020-04-28T08:16:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 28, 2020 at 12:18:02AM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> > > \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> > > -\t\tcredential_from_url(&entry, line.buf);\n> > > -\t\tif (entry.username && entry.password &&\n> > > +\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n> > > +\t\t entry.username && entry.password &&\n> > > \t\t credential_match(c, &entry)) {\n> > > \t\t\tfound_credential = 1;\n> > > \t\t\tif (match_cb) {\n> > > \n> > > And then we can tighten the handling of unrecognized lines to first\n> > > warn and then error out, as a controlled change that doesn't lead\n> > > people to regret updating git.\n> > \n> > I like that solution, as it mostly brings us back to the original\n> > behavior, as weird or unexpected as it was.\n> \n> I like this version better as well, and we could even reuse my test\n> case.\n> \n> it wouldn't cover cases where there were leading spaces/tabs around\n> the credential which I have to admit I liked just because it is\n> more robust to bad input, and there is no sane way now to tell the\n> user that there is invalid data anyway, but I am ok eitherway.\n\nI think if we're discouraging people from hand-editing the file, then\nthat feature would be going in the wrong direction anyway.\n\nDid you or Jonathan want to wrap it up with the test and commit message?\n\n-Peff\n"},{"id":"396450","messageId":"20200428104858.28573-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200427125915.88667-1-carenas@gmail.com","subject":"[PATCH v4 0/4] credential-store: prevent fatal errors","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T10:48:58Z","receivedAt":"2020-04-28T10:49:36Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"with the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] as failing to parse as valid credentials.\n\ndocument better the format for the credential file and make sure\nany failures are still handled gently by the new code as a first\nstep to allow people to upgrade without having first to fix their\ncorrupted credential files.\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nCarlo Marcelo Arenas Belón (2):\n git-credential-store: skip empty lines and comments from store\n credential-store: make sure there is no regression with missing scheme\n\nJonathan Nieder (1):\n git-credential-store: fix (WIP)\n\nJunio C Hamano (1):\n credential-store: document the file format a bit more\n\n Documentation/git-credential-store.txt | 4 ++++\n credential-store.c | 4 ++--\n t/t0302-credential-store.sh | 32 ++++++++++++++++++++++++++\n 3 files changed, 38 insertions(+), 2 deletions(-)\n\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396451","messageId":"20200428105254.28658-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200428104858.28573-1-carenas@gmail.com","subject":"[PATCH v4 1/4] credential-store: document the file format a bit more","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T10:52:51Z","receivedAt":"2020-04-28T10:53:13Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: Junio C Hamano \n\nReading a malformed credential URL line and silently ignoring it\ndoes not mean that we promise to torelate and/or keep empty lines\nand \"# commented\" lines forever.\n\nSome people seem to take anything that is not explicitly forbidden\nas allowed, but the world does not work that way.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-credential-store.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 693dd9d9d7..76b0798856 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -94,6 +94,10 @@ stored on its own line as a URL like:\n https://user:pass@example.com\n ------------------------------\n \n+No other kinds of lines (e.g. empty lines or comment lines) are\n+allowed in the file, even though some may be silently ignored. Do\n+not view or edit the file with editors.\n+\n When Git needs authentication for a particular URL context,\n credential-store will consider that context a pattern to match against\n each entry in the credentials file. If the protocol, hostname, and\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396452","messageId":"20200428105254.28658-2-carenas@gmail.com","threadId":"53326","inReplyTo":"20200428105254.28658-1-carenas@gmail.com","subject":"[PATCH v4 2/4] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T10:52:52Z","receivedAt":"2020-04-28T10:53:15Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"with the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\nadd corresponding failing cases\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n t/t0302-credential-store.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..94cdcb9e56 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_failure 'get: store file can contain empty/bogus lines' '\n+\ttest_write_lines \"#comment\" \" \" \"\" \\\n+\t\t https://user:pass@example.com >\"$HOME/.git-credentials\" &&\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\t--\n+\tEOF\n+'\n+\n test_done\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396453","messageId":"20200428105254.28658-3-carenas@gmail.com","threadId":"53326","inReplyTo":"20200428105254.28658-1-carenas@gmail.com","subject":"[PATCH v4 3/4] git-credential-store: fix (WIP)","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T10:52:53Z","receivedAt":"2020-04-28T10:53:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"From: Jonathan Nieder \n\nHelped-by: Carlo Marcelo Arenas Belon \n---\n credential-store.c | 4 ++--\n t/t0302-credential-store.sh | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..294e771681 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tcredential_from_url(&entry, line.buf);\n-\t\tif (entry.username && entry.password &&\n+\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n+\t\t entry.username && entry.password &&\n \t\t credential_match(c, &entry)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex 94cdcb9e56..4e5a73cb99 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,7 +120,7 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n-test_expect_failure 'get: store file can contain empty/bogus lines' '\n+test_expect_success 'get: store file can contain empty/bogus lines' '\n \ttest_write_lines \"#comment\" \" \" \"\" \\\n \t\t https://user:pass@example.com >\"$HOME/.git-credentials\" &&\n \tcheck fill store <<-\\EOF\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396454","messageId":"20200428105254.28658-4-carenas@gmail.com","threadId":"53326","inReplyTo":"20200428105254.28658-1-carenas@gmail.com","subject":"[PATCH v4 4/4] credential-store: make sure there is no regression with missing scheme","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T10:52:54Z","receivedAt":"2020-04-28T10:53:24Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"c44088ecc4 (credential: treat URL without scheme as invalid, 2020-04-18)\nmade scheme (AKA protocol) mandatory so make sure that still fails.\n\nSuggested-by: Jeff King \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n t/t0302-credential-store.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex 4e5a73cb99..7c7a48303f 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -135,4 +135,21 @@ test_expect_success 'get: store file can contain empty/bogus lines' '\n \tEOF\n '\n \n+test_expect_success 'get: ignore credentials without scheme' '\n+\techo \"://user:pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\t--\n+\taskpass: Username for '\\''https://example.com'\\'':\n+\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n+\t--\n+\tEOF\n+'\n+\n test_done\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396455","messageId":"1M9Wuk-1jYti72AjM-005WZt@mail.gmx.com","threadId":"53326","inReplyTo":"20200428052510.GA201501@google.com","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Stefan Tauner","fromEmail":"stefan.tauner@gmx.at","sentAt":"2020-04-28T10:58:05Z","receivedAt":"2020-04-28T10:58:22Z","isPatch":true,"sender":{"key":"stefan.tauner@gmx.at","avatar":null},"body":"On Mon, 27 Apr 2020 22:25:10 -0700\nJonathan Nieder wrote:\n\n> Independently, in Debian's bug tracking system, Stefan (cc-ed)\n> reports[2]:\n> \n> | the vulnerability in CVE-2020-11008 is related to the handling\n> | of credential helpers in git. In Buster this has been fixed in\n> | 1:2.20.1-2+deb10u3. This broke my existing configuration where\n> | repositories have credential.helper=store set. This is\n> | documented in /usr/share/man/man1/git-credential-store.1.gz\n> | and other files from git, git-doc etc.\n> | I am unsure how to proceed... is this helper now unsupported?\n> \n> (Stefan, do you have more details? Did you manually populate your\n> credential store? What error message do you get?)\n\nI can't remember for sure - it's literally years ago - but I am pretty\nsure I did not *populate* it manually initially. However, I might have\nedited it to separate entries (https vs. smtp) and I might have added\nadditional entries by c&p & editing.\n\nI have replaced the one instance of \\n\\n used for separation with a\nnormal single line break between entries and how it works fine. Before\nthat I got the following output (that's obviously not very helpful if\nyou don't know the whole story :)\n\n> git pull\n> warning: url has no scheme: \n> fatal: credential url cannot be parsed: \n> Already up to date.\n> Current branch master is up to date.\n\n-- \nKind regards/Mit freundlichen Grüßen, Stefan Tauner\n"},{"id":"396456","messageId":"20200428112518.GA15981@Carlos-MBP","threadId":"53326","inReplyTo":"20200428081627.GB2381876@coredump.intra.peff.net","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T11:25:18Z","receivedAt":"2020-04-28T11:25:23Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Apr 28, 2020 at 04:16:27AM -0400, Jeff King wrote:\n> On Tue, Apr 28, 2020 at 12:18:02AM -0700, Carlo Marcelo Arenas Belón wrote:\n> > \n> > it wouldn't cover cases where there were leading spaces/tabs around\n> > the credential which I have to admit I liked just because it is\n> > more robust to bad input, and there is no sane way now to tell the\n> > user that there is invalid data anyway, but I am ok eitherway.\n> \n> I think if we're discouraging people from hand-editing the file, then\n> that feature would be going in the wrong direction anyway.\n\nI think it will actually encourage them to edit the file (and complain\nof a regression), since they can see the credential is correct, but\nsomehow git fails to work with it unlike their previous version.\n\nremember there is no warning that could explain why that happens and\nwith this pach there is no error either.\n\n> Did you or Jonathan want to wrap it up with the test and commit message?\n\nI was going to say Jonathan will have to do it, since he was the only one\nthat didn't provide a patch (and therefore a sign off) but guess I can\nsave everyone some time and practice my git-send-email skills (which \nobviously failed me)\n\nJonathan,\n\nmake sure your patch[1] is complete and correct and to resend it with\na sign off, feel free to add me as Tested-by if you feel like, but I\nadded myself as Helped-by anyway ;)\n\nJunio,\n\ndidn't fix the typo that Peff suggested fixed[2], because I forgot\nand I wasn't sure if that also means I should SOB it, but if you\ncould also consider my suggestion[3] then at you get my SOB warranted ;)\n\nCarlo\n\nPS. guess I should had send it as an RFC instead\n\n[1] https://lore.kernel.org/git/20200428105254.28658-3-carenas@gmail.com/\n[2] https://lore.kernel.org/git/20200427211013.GB1732530@coredump.intra.peff.net/\n[3] https://lore.kernel.org/git/20200427234909.GC61348@Carlos-MBP/\n"},{"id":"396468","messageId":"xmqqsggn4mxz.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200428052510.GA201501@google.com","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T16:03:36Z","receivedAt":"2020-04-28T16:03:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> I wonder if in addition to the above documentation change we may want\n> something guaranteed to catch all cases where people would have\n> experienced a regression, like\n>\n> diff --git i/credential-store.c w/credential-store.c\n> index c010497cb21..294e7716815 100644\n> --- i/credential-store.c\n> +++ w/credential-store.c\n> @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,\n> \t}\n> \n> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> -\t\tcredential_from_url(&entry, line.buf);\n> -\t\tif (entry.username && entry.password &&\n> +\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n> +\t\t entry.username && entry.password &&\n> \t\t credential_match(c, &entry)) {\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n\nHmph, so the idea is, instead of ignoring the potential error\ndetected by credential_from_url() and using credential when it is\navailable, we loudly attempt to parse and give warning on malformed\nentries when we discard a line?\n\nI think that is an excellent idea.\n\nIt would be nicer if we can somehow add where we found the offending\nline, e.g.\n\n /home/me/.gitcredential:396: url has no scheme: ://u:p@host/path\n\nDo we feel it a bit disturbing that u:p is shown in the error\nmessage, without redacting, by the way?\n\n"},{"id":"396469","messageId":"CAPig+cSJDHhUDQ3PGQRen32w9-8xVVFijDDoC5nqzioBHSL7Yw@mail.gmail.com","threadId":"53326","inReplyTo":"20200428105254.28658-1-carenas@gmail.com","subject":"Re: [PATCH v4 1/4] credential-store: document the file format a bit more","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-28T16:06:55Z","receivedAt":"2020-04-28T16:07:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón\n wrote:\n> Reading a malformed credential URL line and silently ignoring it\n> does not mean that we promise to torelate and/or keep empty lines\n> and \"# commented\" lines forever.\n>\n> Some people seem to take anything that is not explicitly forbidden\n> as allowed, but the world does not work that way.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\n> @@ -94,6 +94,10 @@ stored on its own line as a URL like:\n> +No other kinds of lines (e.g. empty lines or comment lines) are\n> +allowed in the file, even though some may be silently ignored. Do\n> +not view or edit the file with editors.\n\nI suggest dropping the \"even though some may be silently ignored\" bit\nsince it's both mysterious (providing no concrete information) and\nunnecessarily confusing since it flat out contradicts the earlier part\nof the sentence. The fact that the implementation has accidentally\nbeen loose in its parsing doesn't warrant introducing such ambiguity\ninto the (newly-added) documentation.\n"},{"id":"396470","messageId":"CAPig+cR8YGGXLcdiv1SHF5wQ305RgG1S0JhheVgF9fYstKxaQA@mail.gmail.com","threadId":"53326","inReplyTo":"20200428105254.28658-2-carenas@gmail.com","subject":"Re: [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-28T16:09:50Z","receivedAt":"2020-04-28T16:10:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón\n wrote:\n> git-credential-store: skip empty lines and comments from store\n\nI don't see anything in this patch which makes it skip anything at\nall; it only introduces a new test.\n\n> with the added checks for invalid URLs in credentials, any locally\n> modified store files which might have empty lines or even comments\n> were reported[1] failing to parse as valid credentials.\n>\n> add corresponding failing cases\n>\n> [1] https://stackoverflow.com/a/61420852/5005936\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n"},{"id":"396471","messageId":"CAPig+cTjZQ7csu78jFx-w1SfewptTrOeR68aEwst7F2qvowR4Q@mail.gmail.com","threadId":"53326","inReplyTo":"20200428105254.28658-3-carenas@gmail.com","subject":"Re: [PATCH v4 3/4] git-credential-store: fix (WIP)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-04-28T16:11:20Z","receivedAt":"2020-04-28T16:11:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón\n wrote:\n> From: Jonathan Nieder \n> Subject: git-credential-store: fix (WIP)\n\nUm, what? Did you forget to squash this into the previous patch?\n\n> Helped-by: Carlo Marcelo Arenas Belon \n\nNo sign-off? (Jonathan's)\n"},{"id":"396473","messageId":"20200428164204.GA56126@Carlos-MBP","threadId":"53326","inReplyTo":"CAPig+cR8YGGXLcdiv1SHF5wQ305RgG1S0JhheVgF9fYstKxaQA@mail.gmail.com","subject":"Re: [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T16:42:04Z","receivedAt":"2020-04-28T16:42:12Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Apr 28, 2020 at 12:09:50PM -0400, Eric Sunshine wrote:\n> On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón\n> wrote:\n> > git-credential-store: skip empty lines and comments from store\n> \n> I don't see anything in this patch which makes it skip anything at\n> all; it only introduces a new test.\n\nmy bad, forgot to fix the subject when rerolling; does something like:\ncredential-store: show \"regression\" when bogus credentials in file\nmake sense for the reroll?\n\nCarlo\n"},{"id":"396477","messageId":"20200428171408.GB56126@Carlos-MBP","threadId":"53326","inReplyTo":"CAPig+cTjZQ7csu78jFx-w1SfewptTrOeR68aEwst7F2qvowR4Q@mail.gmail.com","subject":"Re: [PATCH v4 3/4] git-credential-store: fix (WIP)","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T17:14:08Z","receivedAt":"2020-04-28T17:14:13Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Apr 28, 2020 at 12:11:20PM -0400, Eric Sunshine wrote:\n> On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón\n> wrote:\n> > From: Jonathan Nieder \n> > Subject: git-credential-store: fix (WIP)\n> \n> Um, what? Did you forget to squash this into the previous patch?\n\nno; was documenting his proposal from 20200428052510.GA201501@google.com[1]\nwith the hope (unsuccesfully) to save everyone time and easy testing by\nhaving a series ready to apply.\n\n> > Helped-by: Carlo Marcelo Arenas Belon \n> \n> No sign-off? (Jonathan's)\n\nreally my fault, but was expected as documented in:\n\n https://lore.kernel.org/git/20200428112518.GA15981@Carlos-MBP/\n\nsorry for the confussion and thanks for your review\n\nCarlo\n\n[1] https://lore.kernel.org/git/20200428052510.GA201501@google.com/\n"},{"id":"396485","messageId":"xmqqlfmf32a4.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200428105254.28658-1-carenas@gmail.com","subject":"Re: [PATCH v4 1/4] credential-store: document the file format a bit more","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T18:15:15Z","receivedAt":"2020-04-28T18:15:22Z","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> From: Junio C Hamano \n>\n> Reading a malformed credential URL line and silently ignoring it\n> does not mean ...\n\nI'd reuse the one I've already queued as 272281ef (credential-store:\ndocument the file format a bit more, 2020-04-27) for this step.\n\nThanks.\n\n-- >8 --\nSubject: [PATCH] credential-store: document the file format a bit more\n\nReading a malformed credential URL line and silently ignoring it\ndoes not mean that we support empty lines and/or \"# commented\" lines\nforever. We should document it to avoid confusion.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-credential-store.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 693dd9d9d7..76b0798856 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -94,6 +94,10 @@ stored on its own line as a URL like:\n https://user:pass@example.com\n ------------------------------\n \n+No other kinds of lines (e.g. empty lines or comment lines) are\n+allowed in the file, even though some may be silently ignored. Do\n+not view or edit the file with editors.\n+\n When Git needs authentication for a particular URL context,\n credential-store will consider that context a pattern to match against\n each entry in the credentials file. If the protocol, hostname, and\n-- \n2.26.2-266-ge870325ee8\n\n"},{"id":"396487","messageId":"xmqqh7x3325a.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"CAPig+cSJDHhUDQ3PGQRen32w9-8xVVFijDDoC5nqzioBHSL7Yw@mail.gmail.com","subject":"Re: [PATCH v4 1/4] credential-store: document the file format a bit more","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T18:18:09Z","receivedAt":"2020-04-28T18:18:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón\n> wrote:\n>> Reading a malformed credential URL line and silently ignoring it\n>> does not mean that we promise to torelate and/or keep empty lines\n>> and \"# commented\" lines forever.\n>>\n>> Some people seem to take anything that is not explicitly forbidden\n>> as allowed, but the world does not work that way.\n>>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\n>> @@ -94,6 +94,10 @@ stored on its own line as a URL like:\n>> +No other kinds of lines (e.g. empty lines or comment lines) are\n>> +allowed in the file, even though some may be silently ignored. Do\n>> +not view or edit the file with editors.\n>\n> I suggest dropping the \"even though some may be silently ignored\" bit\n> since it's both mysterious (providing no concrete information) and\n> unnecessarily confusing since it flat out contradicts the earlier part\n> of the sentence. The fact that the implementation has accidentally\n> been loose in its parsing doesn't warrant introducing such ambiguity\n> into the (newly-added) documentation.\n\nI do not think it is an ambiguity. The phrase is there just to\nremind the readers that they are not allowed to take a loose\nimplementation in the past as an excuse to throw in crufts and\nexpect the result to continue working.\n"},{"id":"396508","messageId":"20200428211407.GC56126@Carlos-MBP","threadId":"53326","inReplyTo":"xmqqsggn4mxz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-28T21:14:07Z","receivedAt":"2020-04-28T21:14:13Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Apr 28, 2020 at 09:03:36AM -0700, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n> \n> > I wonder if in addition to the above documentation change we may want\n> > something guaranteed to catch all cases where people would have\n> > experienced a regression, like\n> >\n> > diff --git i/credential-store.c w/credential-store.c\n> > index c010497cb21..294e7716815 100644\n> > --- i/credential-store.c\n> > +++ w/credential-store.c\n> > @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,\n> > \t}\n> > \n> > \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> > -\t\tcredential_from_url(&entry, line.buf);\n> > -\t\tif (entry.username && entry.password &&\n> > +\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n> > +\t\t entry.username && entry.password &&\n> > \t\t credential_match(c, &entry)) {\n> > \t\t\tfound_credential = 1;\n> > \t\t\tif (match_cb) {\n> \n> Hmph, so the idea is, instead of ignoring the potential error\n> detected by credential_from_url() and using credential when it is\n> available, we loudly attempt to parse and give warning on malformed\n> entries when we discard a line?\n\nthe idea was to silently ignore the line (notice quiet = 1), which\nis the \"original behaviour\" as Peff pointed out in his reply in\n20200428054155.GB2376380@coredump.intra.peff.net[1]\n\n> I think that is an excellent idea.\n> \n> It would be nicer if we can somehow add where we found the offending\n> line, e.g.\n> \n> /home/me/.gitcredential:396: url has no scheme: ://u:p@host/path\n> \n> Do we feel it a bit disturbing that u:p is shown in the error\n> message, without redacting, by the way?\n\nwe could do so with the following on top as a 5/4.\n\nmost of the test changes would go away in a reroll, and are similar\nto what would have been needed for my original suggested patch[2] with\nthe exception that in this series we won't support silently fixing\ncredentials which is something you mentioned a preference on.\n\nCarlo\n\n[1] https://lore.kernel.org/git/20200428054155.GB2376380@coredump.intra.peff.net/ \n[2] https://lore.kernel.org/git/20200428013726.GD61348@Carlos-MBP/\n-- >8 --\nSubject: [RFC PATCH] credential-store: SQUASH!!\n\nadd warnings\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n credential-store.c | 55 +++++++++++++++++++++++++++++++------\n t/t0302-credential-store.sh | 42 ++++++++++++++++------------\n 2 files changed, 70 insertions(+), 27 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex 294e771681..f76292df20 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -6,6 +6,32 @@\n \n static struct lock_file credential_lock;\n \n+static char *redact_credential(const struct strbuf *line)\n+{\n+\tstruct strbuf redacted_line = STRBUF_INIT;\n+\tchar *at = strchr(line->buf, '@');\n+\tchar *colon;\n+\tint redacted = 0;\n+\n+\tif (at) {\n+\t\tstrbuf_addf(&redacted_line, \"%.*s\",\n+\t\t\t(int)(at - line->buf), line->buf);\n+\t\tcolon = strrchr(redacted_line.buf, ':');\n+\t\tif (colon && *(colon + 1) != '/' && colon > redacted_line.buf) {\n+\t\t\tredacted_line.len = colon - redacted_line.buf + 1;\n+\t\t\tstrbuf_addstr(&redacted_line, \"\");\n+\t\t\tstrbuf_addstr(&redacted_line, at);\n+\t\t\tredacted = 1;\n+\t\t}\n+\t\telse\n+\t\t\tstrbuf_reset(&redacted_line);\n+\t}\n+\tif (!redacted)\n+\t\tstrbuf_addbuf(&redacted_line, line);\n+\n+\treturn redacted_line.buf;\n+}\n+\n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n \t\t\t\t void (*match_cb)(struct credential *),\n@@ -15,6 +41,7 @@ static int parse_credential_file(const char *fn,\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n \tint found_credential = 0;\n+\tint lineno = 0;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n@@ -24,17 +51,27 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n-\t\t entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n-\t\t\tfound_credential = 1;\n-\t\t\tif (match_cb) {\n-\t\t\t\tmatch_cb(&entry);\n-\t\t\t\tbreak;\n+\t\tlineno++;\n+\t\tif (!credential_from_url_gently(&entry, line.buf, 1)) {\n+\t\t\tif (entry.username && entry.password &&\n+\t\t\t\tcredential_match(c, &entry)) {\n+\t\t\t\tfound_credential = 1;\n+\t\t\t\tif (match_cb) {\n+\t\t\t\t\tmatch_cb(&entry);\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n \t\t\t}\n+\t\t\telse if (other_cb)\n+\t\t\t\tother_cb(&line);\n+\t\t}\n+\t\telse {\n+\t\t\tif (other_cb)\n+\t\t\t\tother_cb(&line);\n+\t\t\tchar *redacted = redact_credential(&line);\n+\t\t\twarning(\"%s:%d ignoring invalid credential: %s\",\n+\t\t\t\tfn, lineno, redacted);\n+\t\t\tfree(redacted);\n \t\t}\n-\t\telse if (other_cb)\n-\t\t\tother_cb(&line);\n \t}\n \n \tcredential_clear(&entry);\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex 7c7a48303f..597a75903a 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,36 +120,42 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n-test_expect_success 'get: store file can contain empty/bogus lines' '\n-\ttest_write_lines \"#comment\" \" \" \"\" \\\n-\t\t https://user:pass@example.com >\"$HOME/.git-credentials\" &&\n-\tcheck fill store <<-\\EOF\n+test_expect_success 'get: credentials without scheme are invalid' '\n+\techo \"://user:pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n \tprotocol=https\n \thost=example.com\n-\t--\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n \tprotocol=https\n \thost=example.com\n-\tusername=user\n-\tpassword=pass\n-\t--\n \tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n '\n \n-test_expect_success 'get: ignore credentials without scheme' '\n-\techo \"://user:pass@example.com\" >\"$HOME/.git-credentials\" &&\n-\tcheck fill store <<-\\EOF\n+test_expect_success 'get: store file can contain empty/bogus lines' '\n+\ttest_write_lines \"#comment\" \" \" \"\" \\\n+\t\t https://user:pass@example.com >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n \tprotocol=https\n \thost=example.com\n-\t--\n+\tusername=user\n+\tpassword=pass\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n \tprotocol=https\n \thost=example.com\n-\tusername=askpass-username\n-\tpassword=askpass-password\n-\t--\n-\taskpass: Username for '\\''https://example.com'\\'':\n-\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n-\t--\n \tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr &&\n+\ttest_line_count = 3 stderr\n '\n \n test_done\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396509","messageId":"xmqq4kt31fa8.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200428211407.GC56126@Carlos-MBP","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T21:17:19Z","receivedAt":"2020-04-28T21:17:25Z","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>> > +\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n>> > +\t\t entry.username && entry.password &&\n>> > \t\t credential_match(c, &entry)) {\n>> > \t\t\tfound_credential = 1;\n>> > \t\t\tif (match_cb) {\n>> \n>> Hmph, so the idea is, instead of ignoring the potential error\n>> detected by credential_from_url() and using credential when it is\n>> available, we loudly attempt to parse and give warning on malformed\n>> entries when we discard a line?\n>\n> the idea was to silently ignore the line (notice quiet = 1), which\n\nAh, sorry, I apparently did not read the patch carefully enough.\n\nThanks for a correction.\n"},{"id":"396521","messageId":"20200429003303.93583-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200428104858.28573-1-carenas@gmail.com","subject":"[PATCH v5] credential-store: warn instead of fatal for bogus lines from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-29T00:33:03Z","receivedAt":"2020-04-29T00:34:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"with the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\ninstead of doing a hard check for credentials, do a soft one and\nwarn the user so any invalid entries could be corrected.\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nBased-on-patch-by: Jonathan Nieder \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv5:\n* q_to_tab this round, with a single echo to make sure empty line\n is covered, as that seems to be a popular issue\n* rebase on top of jc/credential-store-file-format-doc\n* implement a redacter for credentials to use on errora to avoid\n leaking passwordss\nv4:\n* use credential_from_url_gently instead as shown by Jonathan\n* add documentation to clarify \"comments\" is not a supported feature\nv3:\n* avoid using q_to_cr as suggested by Peff\n* a more verbose commit message and slightly more complete documentation\nv2:\n* use a here-doc for clarity as suggested by Eric\n* improve commit message and include documentation\n\n Documentation/git-credential-store.txt | 12 +++++-\n credential-store.c | 51 ++++++++++++++++++++++----\n t/t0302-credential-store.sh | 43 ++++++++++++++++++++++\n 3 files changed, 96 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 76b0798856..30d498fe54 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -95,8 +95,16 @@ https://user:pass@example.com\n ------------------------------\n \n No other kinds of lines (e.g. empty lines or comment lines) are\n-allowed in the file, even though some may be silently ignored. Do\n-not view or edit the file with editors.\n+allowed in the file, even though historically the parser was very\n+lenient and some might had been silently ignored.\n+\n+Do not edit the file with editors as it could compromise the validity\n+of your credentials by sometimes subtle formatting issues (like spaces)\n+\n+In cases where those formatting issues are detected during parsing a\n+warning indicating the line found will be printed to stderr so it can\n+be corrected at your earliest convenience, but the remaining valid\n+credentials will be used to try to find a match as described below.\n \n When Git needs authentication for a particular URL context,\n credential-store will consider that context a pattern to match against\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..5324c56ce1 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -6,6 +6,32 @@\n \n static struct lock_file credential_lock;\n \n+static char *redact_credential(const struct strbuf *line)\n+{\n+\tstruct strbuf redacted_line = STRBUF_INIT;\n+\tchar *at = strchr(line->buf, '@');\n+\tchar *colon;\n+\tint redacted = 0;\n+\n+\tif (at) {\n+\t\tstrbuf_addf(&redacted_line, \"%.*s\",\n+\t\t\t(int)(at - line->buf), line->buf);\n+\t\tcolon = strrchr(redacted_line.buf, ':');\n+\t\tif (colon && *(colon + 1) != '/') {\n+\t\t\tredacted_line.len = colon - redacted_line.buf + 1;\n+\t\t\tstrbuf_addstr(&redacted_line, \"\");\n+\t\t\tstrbuf_addstr(&redacted_line, at);\n+\t\t\tredacted = 1;\n+\t\t}\n+\t\telse\n+\t\t\tstrbuf_reset(&redacted_line);\n+\t}\n+\tif (!redacted)\n+\t\tstrbuf_addbuf(&redacted_line, line);\n+\n+\treturn redacted_line.buf;\n+}\n+\n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n \t\t\t\t void (*match_cb)(struct credential *),\n@@ -15,6 +41,7 @@ static int parse_credential_file(const char *fn,\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n \tint found_credential = 0;\n+\tint lineno = 0;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n@@ -24,16 +51,24 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tcredential_from_url(&entry, line.buf);\n-\t\tif (entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n-\t\t\tfound_credential = 1;\n-\t\t\tif (match_cb) {\n-\t\t\t\tmatch_cb(&entry);\n-\t\t\t\tbreak;\n+\t\tlineno++;\n+\t\tif (!credential_from_url_gently(&entry, line.buf, 1)) {\n+\t\t\tif (entry.username && entry.password &&\n+\t\t\t\tcredential_match(c, &entry)) {\n+\t\t\t\tfound_credential = 1;\n+\t\t\t\tif (match_cb) {\n+\t\t\t\t\tmatch_cb(&entry);\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n-\t\telse if (other_cb)\n+\t\telse {\n+\t\t\tchar *redacted = redact_credential(&line);\n+\t\t\twarning(\"%s:%d %s: %s\", fn, lineno,\n+\t\t\t\t _(\"ignoring invalid credential\"), redacted);\n+\t\t\tfree(redacted);\n+\t\t}\n+\t\tif (!found_credential && other_cb)\n \t\t\tother_cb(&line);\n \t}\n \ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..68b59e6f98 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,4 +120,47 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_success 'get: credentials without scheme are invalid' '\n+\techo \"://user:secret@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr &&\n+\t! grep secret stderr\n+'\n+\n+test_expect_success 'get: store file can contain empty/bogus lines' '\n+\techo \"\" > \"$HOME/.git-credentials\" &&\n+\tq_to_tab <<-\\CONFIG >>\"$HOME/.git-credentials\" &&\n+\t#comment\n+\tQ\n+\thttps://user:pass@example.com\n+\tCONFIG\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr &&\n+\ttest_line_count = 3 stderr\n+'\n+\n test_done\n\nbase-commit: 272281efcc18fcedd248597b8859f343cae1c5a0\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396523","messageId":"xmqqd07qzz67.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200429003303.93583-1-carenas@gmail.com","subject":"Re: [PATCH v5] credential-store: warn instead of fatal for bogus lines from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-29T04:36:00Z","receivedAt":"2020-04-29T04:36:06Z","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> +static char *redact_credential(const struct strbuf *line)\n> +{\n> +\tstruct strbuf redacted_line = STRBUF_INIT;\n> +\tchar *at = strchr(line->buf, '@');\n> +\tchar *colon;\n> +\tint redacted = 0;\n> +\n> +\tif (at) {\n> +\t\tstrbuf_addf(&redacted_line, \"%.*s\",\n> +\t\t\t(int)(at - line->buf), line->buf);\n> +\t\tcolon = strrchr(redacted_line.buf, ':');\n\nJust showing my ignorance, but ...\n\n - Is the above strrchr() that forbids a colon in the password\n intended, or should it be strchr() that only forbids a colon in\n the username instead?\n\n - Would it hurt to redact both username and password as sensitive?\n If not, it would certainly make it simpler to unconditionally:\n\n int i;\n for (i = 0; i < redacted_line.len; i++) {\n if (redacted_line.buf[i] != ':')\n redacted_line.buf[i] = 'x';\n }\n\n"},{"id":"396526","messageId":"20200429073116.GA94208@Carlos-MBP","threadId":"53326","inReplyTo":"xmqqd07qzz67.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5] credential-store: warn instead of fatal for bogus lines from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-29T07:31:16Z","receivedAt":"2020-04-29T07:31:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Apr 28, 2020 at 09:36:00PM -0700, Junio C Hamano wrote:\n> Carlo Marcelo Arenas Belón writes:\n> > +static char *redact_credential(const struct strbuf *line)\n> > +{\n> > +\tstruct strbuf redacted_line = STRBUF_INIT;\n> > +\tchar *at = strchr(line->buf, '@');\n> > +\tchar *colon;\n> > +\tint redacted = 0;\n> > +\n> > +\tif (at) {\n> > +\t\tstrbuf_addf(&redacted_line, \"%.*s\",\n> > +\t\t\t(int)(at - line->buf), line->buf);\n> > +\t\tcolon = strrchr(redacted_line.buf, ':');\n> \n> Just showing my ignorance, but ...\n> \n> - Is the above strrchr() that forbids a colon in the password\n> intended, or should it be strchr() that only forbids a colon in\n> the username instead?\n\nneither; that \":\" is the separator we put in there between username\nand password (unless it was edited out), as any original \":\" in both\nis already urlencoded.\n\nstrrchr() was used in purpose to back only as much as it is needed\nfrom the string to hopefully find the beginning of the password.\n\nindeed, most of the uglyness of the code comes from the fact it tries\nreally hard to only redact the password so the rest of the credential\nis still useful for context and do so without changing the original\nline.\n\n> - Would it hurt to redact both username and password as sensitive?\n> If not, it would certainly make it simpler to unconditionally:\n> \n> int i;\n> for (i = 0; i < redacted_line.len; i++) {\n> if (redacted_line.buf[i] != ':')\n> redacted_line.buf[i] = 'x';\n> }\n\nthat would leak the length of the password which might also be considered\nsensitive\n\nIMHO if we are redacting both might as well not print anything and let\nthe user find the offending credential by line number instead ;) after\nall I got a feeling most of those entries are empty lines or \"comments\".\n\nfor example with the current code merged in pu we get :\n\n$ cat secretfile\n\n://u:p@example.com\nhttp://user:some%3apass@example.com\n\n$ git credential-store --file secretfile get\nprotocol=http\n\nwarning: secretfile:1 ignoring invalid credential:\nwarning: secretfile:2 ignoring invalid credential: ://u:@example.com\nusername=user\npassword=some:pass\n\nCarlo\n"},{"id":"396536","messageId":"c64b8f26-7f59-c701-20d2-7ca2e52f686d@iee.email","threadId":"53326","inReplyTo":"20200428014926.GE61348@Carlos-MBP","subject":"Re: [PATCH v3] git-credential-store: skip empty lines and comments from store","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-04-29T10:09:31Z","receivedAt":"2020-04-29T10:09:36Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi Carlo,\nThanks for the clarification.\nPhilip\n\nOn 28/04/2020 02:49, Carlo Marcelo Arenas Belón wrote:\n> On Mon, Apr 27, 2020 at 02:48:05PM +0100, Philip Oakley wrote:\n>> On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote:\n>>> with the added checks for invalid URLs in credentials, any locally\n>>> modified store files which might have empty lines or even comments\n>>> were reported[1] failing to parse as valid credentials.\n>>>\n>>> using the store file in this manner wasn't intended by the original\n>>> code and it had latent issues which the new code dutifully prevented\n>>> but since the strings used wouldn't had been valid credentials anyway\n>>> we could instead detect them and skip the matching logic and therefore\n>>> formalize this \"feature\".\n>>>\n>>> trim all lines as they are being read from the store file and skip the\n>> Does trimming affect any credentials that may have spaces either end?\n> all credentials are url encoded so the only spaces that are affected are\n> the ones that were added by careless editing of that file.\n>\n> as Eric pointed out, any tabs will be silently \"cleaned\" as well.\n>\n>> Should the trimming of leading/trailing spaces be mentioned?\n> I wanted to keep that as an undocumented \"feature\" as it is just meant\n> to help people to avoid a fatal error during the transition but didn't\n> want to encourage people thinking it is a supported part or even worse\n> encourage people to start editing their files to add tabs and spaces.\n>\n> Junio's suggested documentation fix[1] makes that clearer that I could\n>\n>> Also the git-credential page mentions that the credential must end with\n>> a blank line (\"don't forget..\"). Should that be mentioned here, or have\n>> I misunderstood?\n> that is for the protocol part of it (which is used between git and the\n> credential helper), the file format for this specific helper doesn't\n> require that.\n>\n> Carlo\n>\n> [1] https://lore.kernel.org/git/xmqqv9lk7j7p.fsf@gitster.c.googlers.com/\n\n"},{"id":"396569","messageId":"xmqqmu6uxms1.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200429073116.GA94208@Carlos-MBP","subject":"Re: [PATCH v5] credential-store: warn instead of fatal for bogus lines from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-29T16:46:38Z","receivedAt":"2020-04-29T16:46:44Z","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> IMHO if we are redacting both might as well not print anything and let\n> the user find the offending credential by line number instead ;) after\n> all I got a feeling most of those entries are empty lines or \"comments\".\n\nVery sensible.\n"},{"id":"396601","messageId":"20200429203546.56753-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200429003303.93583-1-carenas@gmail.com","subject":"[RFC PATCH v6 0/2] credential-store: prevent fatal errors","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-29T20:35:44Z","receivedAt":"2020-04-29T20:36:08Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"with the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] as failing to parse as valid credentials.\n\ndocument better the format for the credential file and make sure\nany failures are still handled gently by the new code to avoid\nregressions but make sure that any invalid credentials are flagged\nas such instead of being silently ignored.\n\nideally would like to add a test against the use of cert:// to make\nsure that is still well supported but don't know of any use case or\nif it is being used with store (AFAIK, it could and at least in theory\nit should still work fine with the added validations), hence sending\nthis as an RFC.\n\nalso, there is no real reason to have 2 patches but I am keeping them\nseparated because the first one should be AFAIK otherwise almost ready.\n\nbut considering that we are validating since v4 against a missing\nprotocol and printing a warning since v5, it seems we should then\nfully avoid all those silently ignored issues as well (as shown in\npatch 2) which I hope we could squash and release together (if agreed)\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nCarlo Marcelo Arenas Belón (2):\n credential-store: warn instead of fatal for bogus lines from store\n credential-store: warn for any incomplete credentials instead of using\n\n---\nv6:\n* get rid of redacter and only use line number for context\n* make validation more strict to also catch incomplete credentials\nv5:\n* q_to_tab this round, with a single echo to make sure empty line\n is covered, as that seems to be a popular issue\n* rebase on top of jc/credential-store-file-format-doc\n* implement a redacter for credentials to use on errors to avoid\n leaking passwordss\nv4:\n* use credential_from_url_gently instead as shown by Jonathan\n* add documentation to clarify \"comments\" is not a supported feature\nv3:\n* avoid using q_to_cr as suggested by Peff\n* a more verbose commit message and slightly more complete documentation\nv2:\n* use a here-doc for clarity as suggested by Eric\n* improve commit message and include documentation\n\n Documentation/git-credential-store.txt | 12 +++-\n credential-store.c | 23 +++++---\n t/t0302-credential-store.sh | 80 ++++++++++++++++++++++++++\n 3 files changed, 105 insertions(+), 10 deletions(-)\n\n\nbase-commit: 272281efcc18fcedd248597b8859f343cae1c5a0\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396602","messageId":"20200429203546.56753-2-carenas@gmail.com","threadId":"53326","inReplyTo":"20200429203546.56753-1-carenas@gmail.com","subject":"[RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-29T20:35:45Z","receivedAt":"2020-04-29T20:36:13Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"with the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\ninstead of doing a hard check for credentials, do a soft one and\nwarn the user so any invalid entries could be corrected.\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nBased-on-patch-by: Jonathan Nieder \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Documentation/git-credential-store.txt | 12 ++++++--\n credential-store.c | 22 +++++++++-----\n t/t0302-credential-store.sh | 42 ++++++++++++++++++++++++++\n 3 files changed, 66 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 76b0798856..30d498fe54 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -95,8 +95,16 @@ https://user:pass@example.com\n ------------------------------\n \n No other kinds of lines (e.g. empty lines or comment lines) are\n-allowed in the file, even though some may be silently ignored. Do\n-not view or edit the file with editors.\n+allowed in the file, even though historically the parser was very\n+lenient and some might had been silently ignored.\n+\n+Do not edit the file with editors as it could compromise the validity\n+of your credentials by sometimes subtle formatting issues (like spaces)\n+\n+In cases where those formatting issues are detected during parsing a\n+warning indicating the line found will be printed to stderr so it can\n+be corrected at your earliest convenience, but the remaining valid\n+credentials will be used to try to find a match as described below.\n \n When Git needs authentication for a particular URL context,\n credential-store will consider that context a pattern to match against\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..1cc5ca081a 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -15,6 +15,7 @@ static int parse_credential_file(const char *fn,\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n \tint found_credential = 0;\n+\tint lineno = 0;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n@@ -24,16 +25,21 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tcredential_from_url(&entry, line.buf);\n-\t\tif (entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n-\t\t\tfound_credential = 1;\n-\t\t\tif (match_cb) {\n-\t\t\t\tmatch_cb(&entry);\n-\t\t\t\tbreak;\n+\t\tlineno++;\n+\t\tif (!credential_from_url_gently(&entry, line.buf, 1)) {\n+\t\t\tif (entry.username && entry.password &&\n+\t\t\t\tcredential_match(c, &entry)) {\n+\t\t\t\tfound_credential = 1;\n+\t\t\t\tif (match_cb) {\n+\t\t\t\t\tmatch_cb(&entry);\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n-\t\telse if (other_cb)\n+\t\telse\n+\t\t\twarning(_(\"ignoring invalid credential in %s:%d\"),\n+\t\t\t\tfn, lineno);\n+\t\tif (!found_credential && other_cb)\n \t\t\tother_cb(&line);\n \t}\n \ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..801c1eb200 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,4 +120,46 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_success 'get: credentials without scheme are invalid' '\n+\techo \"://user:pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: store file can contain empty/bogus lines' '\n+\techo \"\" > \"$HOME/.git-credentials\" &&\n+\tq_to_tab <<-\\CONFIG >>\"$HOME/.git-credentials\" &&\n+\t#comment\n+\tQ\n+\thttps://user:pass@example.com\n+\tCONFIG\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr &&\n+\ttest_line_count = 3 stderr\n+'\n+\n test_done\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396603","messageId":"20200429203546.56753-3-carenas@gmail.com","threadId":"53326","inReplyTo":"20200429203546.56753-1-carenas@gmail.com","subject":"[RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of using","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-29T20:35:46Z","receivedAt":"2020-04-29T20:36:16Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"originally any credential found was tried for matching as far as it had\na username and password, but that resulted in fatal errors as the rules\nwere harden.\n\nnow that we have a way to report malformed credentials, use it to notify\nthe user when username/password was missing, instead of just silently\nskipping.\n\ndo the same for credentials that are missing host (or had one that is\nempty) or that are missing a path (for supporting cert://) as well.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n credential-store.c | 7 ++++---\n t/t0302-credential-store.sh | 38 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 3 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex 1cc5ca081a..53f77ff6f5 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -26,9 +26,10 @@ static int parse_credential_file(const char *fn,\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n \t\tlineno++;\n-\t\tif (!credential_from_url_gently(&entry, line.buf, 1)) {\n-\t\t\tif (entry.username && entry.password &&\n-\t\t\t\tcredential_match(c, &entry)) {\n+\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n+\t\t\t((entry.host && *entry.host) || entry.path) &&\n+\t\t\tentry.username && entry.password) {\n+\t\t\tif (credential_match(c, &entry)) {\n \t\t\t\tfound_credential = 1;\n \t\t\t\tif (match_cb) {\n \t\t\t\t\tmatch_cb(&entry);\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex 801c1eb200..3150f304cb 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -139,6 +139,44 @@ test_expect_success 'get: credentials without scheme are invalid' '\n \ttest_i18ngrep \"ignoring invalid credential\" stderr\n '\n \n+test_expect_success 'get: credentials without valid host/path are invalid' '\n+\techo \"https://user:pass@\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials without username/password are invalid' '\n+\techo \"https://pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n test_expect_success 'get: store file can contain empty/bogus lines' '\n \techo \"\" > \"$HOME/.git-credentials\" &&\n \tq_to_tab <<-\\CONFIG >>\"$HOME/.git-credentials\" &&\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396606","messageId":"xmqqo8rauhoh.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200429203546.56753-2-carenas@gmail.com","subject":"Re: [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-29T21:05:02Z","receivedAt":"2020-04-29T21:05:10Z","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> with the added checks for invalid URLs in credentials, any locally\n> modified store files which might have empty lines or even comments\n> were reported[1] failing to parse as valid credentials.\n>\n> instead of doing a hard check for credentials, do a soft one and\n> warn the user so any invalid entries could be corrected.\n\nYeah, this version looks be best so far. No need to worry about\nredacting anything; we are dealing with folks who have edited\nthe file, and they shouldn't have any trouble going to the line with\na problem given an exact line number.\n\n> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\n> index 76b0798856..30d498fe54 100644\n> --- a/Documentation/git-credential-store.txt\n> +++ b/Documentation/git-credential-store.txt\n> @@ -95,8 +95,16 @@ https://user:pass@example.com\n> ------------------------------\n> \n> No other kinds of lines (e.g. empty lines or comment lines) are\n> -allowed in the file, even though some may be silently ignored. Do\n> -not view or edit the file with editors.\n> +allowed in the file, even though historically the parser was very\n> +lenient and some might had been silently ignored.\n> +\n> +Do not edit the file with editors as it could compromise the validity\n> +of your credentials by sometimes subtle formatting issues (like spaces)\n\nI do not think dropping \"view or\" is warranted. There is no need to\ninvite the risk of opening with the intention to only view and then\nend up saving a modification. In other words, do not encourage use\nof an *editor* in any way.\n\n> +In cases where those formatting issues are detected during parsing a\n> +warning indicating the line found will be printed to stderr so it can\n> +be corrected at your earliest convenience, but the remaining valid\n> +credentials will be used to try to find a match as described below.\n\nI am often just as guilty, but the above four lines in a single\nsentence is hard to read, especially without minimum number of\ncomma. With a comma after \"during parsing\", and another after \"to\nstderr\", might make it more readable, but at that point, it would\nbecome far more readable if we split them into two sentences.\n\nPerhaps\n\n\tAn unparseable line is ignored, and a warning message points\n\tout the line number the problematic line appears in (you may\n\twant to delete them with an editor).\n\n> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> +\t\tlineno++;\n> +\t\tif (!credential_from_url_gently(&entry, line.buf, 1)) {\n> +\t\t\tif (entry.username && entry.password &&\n> +\t\t\t\tcredential_match(c, &entry)) {\n> +\t\t\t\tfound_credential = 1;\n> +\t\t\t\tif (match_cb) {\n> +\t\t\t\t\tmatch_cb(&entry);\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n> \t\t\t}\n> \t\t}\n> +\t\telse\n> +\t\t\twarning(_(\"ignoring invalid credential in %s:%d\"),\n> +\t\t\t\tfn, lineno);\n> +\t\tif (!found_credential && other_cb)\n> \t\t\tother_cb(&line);\n> \t}\n\nThe above is harder to follow than necessary.\n\n\twhile (... != EOF) {\n\t\tlineno++;\n\t\tif (credential is not well formed) {\n\t\t\twarning(_(\"ignoring...\"));\n\t\t} else if (the entry matches) {\n\t\t\tfound_credential = 1;\n\t\t\tif (match_cb) {\n\t\t\t\tmatch_cb(&entry);\n\t\t\t\tbreak; /* stop at the first match */\n\t\t\t}\n\t\t\tcontinue;\n\t\t}\n\n if (other_cb)\n\t\t\tother_cb(&line);\n\t}\n\nmay make the intention a bit clearer, with the loud \"continue\" inside.\n\n (1) we give warning for malformed one; and\n (2) we do not let other_cb touch a matching entry.\n\n"},{"id":"396607","messageId":"xmqqk11yuhca.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200429203546.56753-3-carenas@gmail.com","subject":"Re: [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of using","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-29T21:12:21Z","receivedAt":"2020-04-29T21:12:31Z","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> originally any credential found was tried for matching as far as it had\n> a username and password, but that resulted in fatal errors as the rules\n> were harden.\n\nharden -> hardened\n\n> now that we have a way to report malformed credentials, use it to notify\n> the user when username/password was missing, instead of just silently\n> skipping.\n\nSorry, but isn't that what happend already in the previous step?\nWhat are you ordering the codebase (after applying the previous\nstpe) do further? It already is \"using it to notify the user when\nusername and/or password is missing\".\n\n> do the same for credentials that are missing host (or had one that is\n> empty) or that are missing a path (for supporting cert://) as well.\n\nWhile the intention may be a good one ...\n\n\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> credential-store.c | 7 ++++---\n> t/t0302-credential-store.sh | 38 +++++++++++++++++++++++++++++++++++++\n> 2 files changed, 42 insertions(+), 3 deletions(-)\n>\n> diff --git a/credential-store.c b/credential-store.c\n> index 1cc5ca081a..53f77ff6f5 100644\n> --- a/credential-store.c\n> +++ b/credential-store.c\n> @@ -26,9 +26,10 @@ static int parse_credential_file(const char *fn,\n> \n> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> \t\tlineno++;\n> -\t\tif (!credential_from_url_gently(&entry, line.buf, 1)) {\n> -\t\t\tif (entry.username && entry.password &&\n> -\t\t\t\tcredential_match(c, &entry)) {\n> +\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n> +\t\t\t((entry.host && *entry.host) || entry.path) &&\n> +\t\t\tentry.username && entry.password) {\n> +\t\t\tif (credential_match(c, &entry)) {\n\n... this makes the code even harder to follow than it already was\nafter the previous step. In the previous step, at least it was\nclear that we'd catch *all* the well-formed/parseable input will\ncome into the first if () {...} block, but with the extra logic,\nthat is no longer true. Even a line that is well formed may be\nbypassed from matching and will be fed to \"else\" side.\n\n"},{"id":"396608","messageId":"xmqqftcmuh3a.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"xmqqo8rauhoh.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-29T21:17:45Z","receivedAt":"2020-04-29T21:17:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n>> +\t\tlineno++;\n>> +\t\tif (!credential_from_url_gently(&entry, line.buf, 1)) {\n>> +\t\t\tif (entry.username && entry.password &&\n>> +\t\t\t\tcredential_match(c, &entry)) {\n>> +\t\t\t\tfound_credential = 1;\n>> +\t\t\t\tif (match_cb) {\n>> +\t\t\t\t\tmatch_cb(&entry);\n>> +\t\t\t\t\tbreak;\n>> +\t\t\t\t}\n>> \t\t\t}\n>> \t\t}\n>> +\t\telse\n>> +\t\t\twarning(_(\"ignoring invalid credential in %s:%d\"),\n>> +\t\t\t\tfn, lineno);\n>> +\t\tif (!found_credential && other_cb)\n>> \t\t\tother_cb(&line);\n>> \t}\n>\n> The above is harder to follow than necessary.\n>\n> \twhile (... != EOF) {\n> \t\tlineno++;\n> \t\tif (credential is not well formed) {\n\nAfter reading your [2/2], I think this part deserves a bit of\nexplanation. By \"is not well formed\", what I mean is not just\ncredential_from_url_gently() returns non-zero, but also user/pass\nare specified. And your step [2/2] may make the condition for a\nline to be \"well formed\" even stricter by requiring at least one of\nhost or path, and that would also belong to this test.\n\n> \t\t\twarning(_(\"ignoring...\"));\n> \t\t} else if (the entry matches) {\n\nAnd the \"entry matches\" test here would only be \"what does\ncredential_match() say?\"\n\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n> \t\t\t\tmatch_cb(&entry);\n> \t\t\t\tbreak; /* stop at the first match */\n> \t\t\t}\n> \t\t\tcontinue;\n> \t\t}\n>\n> if (other_cb)\n> \t\t\tother_cb(&line);\n> \t}\n>\n> may make the intention a bit clearer, with the loud \"continue\" inside.\n>\n> (1) we give warning for malformed one; and\n> (2) we do not let other_cb touch a matching entry.\n"},{"id":"396612","messageId":"20200429214931.GB33264@Carlos-MBP","threadId":"53326","inReplyTo":"xmqqk11yuhca.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of usingy","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-29T21:49:31Z","receivedAt":"2020-04-29T21:49:36Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Apr 29, 2020 at 02:12:21PM -0700, Junio C Hamano wrote:\n> Carlo Marcelo Arenas Belón writes:\n> \n> > originally any credential found was tried for matching as far as it had\n> > a username and password, but that resulted in fatal errors as the rules\n> > were harden.\n> \n> harden -> hardened\n> \n> > now that we have a way to report malformed credentials, use it to notify\n> > the user when username/password was missing, instead of just silently\n> > skipping.\n> \n> Sorry, but isn't that what happend already in the previous step?\n> What are you ordering the codebase (after applying the previous\n> stpe) do further? It already is \"using it to notify the user when\n> username and/or password is missing\".\n\nnot sure I follow, but the current code (as well as the one after patch 1)\njust silently ignores any credential that was missing username and password\nsince the _gently version of the call will only really fail for a missing\nprotocol.\n\nso patch1 will notify ONLY(*) when the credential is so malformed that we can't\nfigure out which protocol to use (like empty lines and comments)\n\n(*) it will also fail if we have a '\\n' in one of the components but that\nis hopefully not relevant here.\n\n> > diff --git a/credential-store.c b/credential-store.c\n> > index 1cc5ca081a..53f77ff6f5 100644\n> > --- a/credential-store.c\n> > +++ b/credential-store.c\n> > @@ -26,9 +26,10 @@ static int parse_credential_file(const char *fn,\n> > \n> > \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> > \t\tlineno++;\n> > -\t\tif (!credential_from_url_gently(&entry, line.buf, 1)) {\n> > -\t\t\tif (entry.username && entry.password &&\n> > -\t\t\t\tcredential_match(c, &entry)) {\n> > +\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n> > +\t\t\t((entry.host && *entry.host) || entry.path) &&\n> > +\t\t\tentry.username && entry.password) {\n> > +\t\t\tif (credential_match(c, &entry)) {\n> \n> ... this makes the code even harder to follow than it already was\n> after the previous step. In the previous step, at least it was\n> clear that we'd catch *all* the well-formed/parseable input will\n> come into the first if () {...} block, but with the extra logic,\n> that is no longer true. Even a line that is well formed may be\n> bypassed from matching and will be fed to \"else\" side.\n\nI think it will be clearer with the reformatting you suggested and that I\nagree is needed, but wanted to make sure first that we agreed on the direction.\n\nFWIW the test cases show the format of the lines affected and while they are\nparseable enough that were processed they were obviously not valid, specially\nconsidering the updated rules.\n\nmaybe we should make the _gently version less gently instead?, so that callers\nwill be able to know they have an incomplete credential for matching after\ncalling it without having to do their own check?\n\nif the later, I am affraid this will be a far bigger change, which is why I\nwould rather duplicate the rule checking for at least the first iteration.\n\nCarlo\n"},{"id":"396614","messageId":"xmqq7dxyuexn.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200429214931.GB33264@Carlos-MBP","subject":"Re: [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of usingy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-29T22:04:20Z","receivedAt":"2020-04-29T22:04:28Z","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>> Sorry, but isn't that what happend already in the previous step?\n>> What are you ordering the codebase (after applying the previous\n>> stpe) do further? It already is \"using it to notify the user when\n>> username and/or password is missing\".\n>\n> not sure I follow, but the current code (as well as the one after patch 1)\n> just silently ignores any credential that was missing username and password\n> since the _gently version of the call will only really fail for a missing\n> protocol.\n\nAh, I misread the intention of [1/1]. While I was rewriting the\nif/else structure inside the loop, somehow I thought that we want to\nwarn when from_url_gently() fails or user/pass are missing, and in\nthis step, we are adding to the \"warning-worthy\" set an entry that\ndoes not have either host or path.\n\nSorry about the confusion.\n"},{"id":"396631","messageId":"20200429232322.68038-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200429203546.56753-1-carenas@gmail.com","subject":"[PATCH v6] credential-store: warn instead of fatal for bogus lines from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-29T23:23:22Z","receivedAt":"2020-04-29T23:23:42Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"with the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\ninstead of doing a hard check for credentials, do a soft one and\nwarn the user so any invalid entries could be corrected.\n\nmake sure that the credential to use is complete before calling\ncredential_match by confirming it has all fields set as expected\nby the updated rules.\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nBased-on-patch-by: Jonathan Nieder \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv6:\n* get rid of redacter and only use line number for context in warning\n* make validation more strict to also catch incomplete credentials\n* reorder check as suggested by Junio\nv5:\n* q_to_tab this round, with a single echo to make sure empty line\n is covered, as that seems to be a popular issue\n* rebase on top of jc/credential-store-file-format-doc\n* implement a redacter for credentials to use on errors to avoid\n leaking passwords\nv4:\n* use credential_from_url_gently instead as shown by Jonathan\n* add documentation to clarify \"comments\" is not a supported feature\nv3:\n* avoid using q_to_cr as suggested by Peff\n* a more verbose commit message and slightly more complete documentation\nv2:\n* use a here-doc for clarity as suggested by Eric\n* improve commit message and include documentation\n\n Documentation/git-credential-store.txt | 11 +++-\n credential-store.c | 26 +++++++--\n t/t0302-credential-store.sh | 80 ++++++++++++++++++++++++++\n 3 files changed, 111 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 76b0798856..122e238eb2 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -95,8 +95,15 @@ https://user:pass@example.com\n ------------------------------\n \n No other kinds of lines (e.g. empty lines or comment lines) are\n-allowed in the file, even though some may be silently ignored. Do\n-not view or edit the file with editors.\n+allowed in the file, even though historically the parser was very\n+lenient and some might had been silently ignored.\n+\n+Do not view or edit the file with editors as it could compromise the\n+validity of your credentials by sometimes subtle formatting issues,\n+like spaces, line wrapping or text encoding.\n+\n+An unparseable or otherwise invalid line is ignored, and a warning\n+message points out the problematic line number and file it appears in.\n \n When Git needs authentication for a particular URL context,\n credential-store will consider that context a pattern to match against\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..4d3c9e8000 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -6,6 +6,15 @@\n \n static struct lock_file credential_lock;\n \n+/*\n+ * entry->protocol comes validated from credential_from_url_gently\n+ */\n+static int valid_credential(struct credential *entry)\n+{\n+\treturn (entry->username && entry->password &&\n+\t\t((entry->host && *entry->host) || entry->path));\n+}\n+\n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n \t\t\t\t void (*match_cb)(struct credential *),\n@@ -15,6 +24,7 @@ static int parse_credential_file(const char *fn,\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n \tint found_credential = 0;\n+\tint lineno = 0;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n@@ -24,16 +34,24 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tcredential_from_url(&entry, line.buf);\n-\t\tif (entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n+\t\tlineno++;\n+\n+\t\tif (credential_from_url_gently(&entry, line.buf, 1) ||\n+\t\t\t!valid_credential(&entry)) {\n+\t\t\twarning(_(\"%s:%d: ignoring invalid credential\"),\n+\t\t\t\tfn, lineno);\n+\t\t}\n+\t\telse if (credential_match(c, &entry))\n+\t\t{\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n \t\t\t\tbreak;\n \t\t\t}\n+\t\t\tcontinue;\n \t\t}\n-\t\telse if (other_cb)\n+\n+\t\tif (other_cb)\n \t\t\tother_cb(&line);\n \t}\n \ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..3150f304cb 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,4 +120,84 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_success 'get: credentials without scheme are invalid' '\n+\techo \"://user:pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials without valid host/path are invalid' '\n+\techo \"https://user:pass@\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials without username/password are invalid' '\n+\techo \"https://pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: store file can contain empty/bogus lines' '\n+\techo \"\" > \"$HOME/.git-credentials\" &&\n+\tq_to_tab <<-\\CONFIG >>\"$HOME/.git-credentials\" &&\n+\t#comment\n+\tQ\n+\thttps://user:pass@example.com\n+\tCONFIG\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr &&\n+\ttest_line_count = 3 stderr\n+'\n+\n test_done\n\nbase-commit: 272281efcc18fcedd248597b8859f343cae1c5a0\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396632","messageId":"xmqqlfmdua5o.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200429232322.68038-1-carenas@gmail.com","subject":"Re: [PATCH v6] credential-store: warn instead of fatal for bogus lines from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-29T23:47:31Z","receivedAt":"2020-04-29T23:47:39Z","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> with the added checks for invalid URLs in credentials, any locally\n> modified store files which might have empty lines or even comments\n> were reported[1] failing to parse as valid credentials.\n\nwith -> With\n\n> instead of doing a hard check for credentials, do a soft one and\n> warn the user so any invalid entries could be corrected.\n\ninstead -> instead\n\n> make sure that the credential to use is complete before calling\n> credential_match by confirming it has all fields set as expected\n> by the updated rules.\n\nmake -> Make\n\n> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\n> index 76b0798856..122e238eb2 100644\n> --- a/Documentation/git-credential-store.txt\n> +++ b/Documentation/git-credential-store.txt\n> @@ -95,8 +95,15 @@ https://user:pass@example.com\n> ------------------------------\n> \n> No other kinds of lines (e.g. empty lines or comment lines) are\n> -allowed in the file, even though some may be silently ignored. Do\n> -not view or edit the file with editors.\n> +allowed in the file, even though historically the parser was very\n> +lenient and some might had been silently ignored.\n> +\n> +Do not view or edit the file with editors as it could compromise the\n> +validity of your credentials by sometimes subtle formatting issues,\n> +like spaces, line wrapping or text encoding.\n\nI do not think dropping \"view or\" is justifiable. There is no need\nto invite the risk of opening with the intention to only view and\nthen end up saving a modification. In other words, do not encourage\nuse of an *editor* in any way.\n\n> +\n> +An unparseable or otherwise invalid line is ignored, and a warning\n> +message points out the problematic line number and file it appears in.\n\nOK. You didn't want to tell them they can remove the problematic\nline as a whole with their editor?\n\n> diff --git a/credential-store.c b/credential-store.c\n> index c010497cb2..4d3c9e8000 100644\n> --- a/credential-store.c\n> +++ b/credential-store.c\n> @@ -6,6 +6,15 @@\n> \n> static struct lock_file credential_lock;\n> \n> +/*\n> + * entry->protocol comes validated from credential_from_url_gently\n> + */\n\nSure, but wouldn't it be simpler to do without such a comment and\ncheck it also here, just as a belt-and-suspender safety?\n\n> +static int valid_credential(struct credential *entry)\n> +{\n> +\treturn (entry->username && entry->password &&\n> +\t\t((entry->host && *entry->host) || entry->path));\n> +}\n\n> @@ -15,6 +24,7 @@ static int parse_credential_file(const char *fn,\n> \tstruct strbuf line = STRBUF_INIT;\n> \tstruct credential entry = CREDENTIAL_INIT;\n> \tint found_credential = 0;\n> +\tint lineno = 0;\n> \n> \tfh = fopen(fn, \"r\");\n> \tif (!fh) {\n> @@ -24,16 +34,24 @@ static int parse_credential_file(const char *fn,\n> \t}\n> \n> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> +\t\tlineno++;\n> +\n> +\t\tif (credential_from_url_gently(&entry, line.buf, 1) ||\n> +\t\t\t!valid_credential(&entry)) {\n> +\t\t\twarning(_(\"%s:%d: ignoring invalid credential\"),\n> +\t\t\t\tfn, lineno);\n> +\t\t}\n> +\t\telse if (credential_match(c, &entry))\n> +\t\t{\n\nStyle: \"} else if (...) {\" on a single line.\n\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n> \t\t\t\tmatch_cb(&entry);\n> \t\t\t\tbreak;\n> \t\t\t}\n> +\t\t\tcontinue;\n> \t\t}\n> +\n> +\t\tif (other_cb)\n> \t\t\tother_cb(&line);\n> \t}\n\nThis got vastly easier to read, thanks to the additional helper.\nLooking really good.\n\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> index d6b54e8c65..3150f304cb 100755\n> --- a/t/t0302-credential-store.sh\n> +++ b/t/t0302-credential-store.sh\n> @@ -120,4 +120,84 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n> \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n> '\n> \n> +test_expect_success 'get: credentials without scheme are invalid' '\n> +\techo \"://user:pass@example.com\" >\"$HOME/.git-credentials\" &&\n> +\tcat >expect-stdout <<-\\STDOUT &&\n> +\tprotocol=https\n> +\thost=example.com\n> +\tusername=askpass-username\n> +\tpassword=askpass-password\n> +\tSTDOUT\n> +\ttest_config credential.helper store &&\n> +\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n> +\tprotocol=https\n> +\thost=example.com\n> +\tEOF\n> +\ttest_cmp expect-stdout stdout &&\n> +\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n> +\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n\nThese messages are passed from credential.c::credential_ask_one() to\ngit_prompt() without any i18n, so use of a bare \"grep\" is good.\n\n> +\ttest_i18ngrep \"ignoring invalid credential\" stderr\n\nAnd this is new message inside _(), so i18ngrep is good.\n\nNice to see such an attention to the details.\n\n\n> +\ttest_config credential.helper store &&\n> +\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n> +\tprotocol=https\n> +\thost=example.com\n> +\tEOF\n> +\ttest_cmp expect-stdout stdout &&\n> +\ttest_i18ngrep \"ignoring invalid credential\" stderr &&\n> +\ttest_line_count = 3 stderr\n\nThe \"ignoring invalid credential\" message could be translated into\ntwo or more lines, but I think that is worrying too much about\ntheoretical possibility, so checking line count would probably be\nsufficient.\n\nThanks.\n"},{"id":"396634","messageId":"xmqqh7x1u9oz.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"xmqqlfmdua5o.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v6] credential-store: warn instead of fatal for bogus lines from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-29T23:57:32Z","receivedAt":"2020-04-29T23:57:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +Do not view or edit the file with editors as it could compromise the\n>> +validity of your credentials by sometimes subtle formatting issues,\n>> +like spaces, line wrapping or text encoding.\n>\n> I do not think dropping \"view or\" is justifiable....\n\nSorry, this comment is now stale.\n\n>> +An unparseable or otherwise invalid line is ignored, and a warning\n>> +message points out the problematic line number and file it appears in.\n>\n> OK. You didn't want to tell them they can remove the problematic\n> line as a whole with their editor?\n\nThis one I do not care too deeply either way. Probably it is\nobvious to the readers that they can remove the lines (otherwise\nthere is no good reason to give warnings in the first place).\n\n"},{"id":"396639","messageId":"20200430010024.GC33264@Carlos-MBP","threadId":"53326","inReplyTo":"xmqqlfmdua5o.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v6] credential-store: warn instead of fatal for bogus lines from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-30T01:00:24Z","receivedAt":"2020-04-30T01:00:31Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Apr 29, 2020 at 04:47:31PM -0700, Junio C Hamano wrote:\n> > instead of doing a hard check for credentials, do a soft one and\n> > warn the user so any invalid entries could be corrected.\n> \n> instead -> instead\n\n;)\n\n> I do not think dropping \"view or\" is justifiable. There is no need\n> to invite the risk of opening with the intention to only view and\n> then end up saving a modification. In other words, do not encourage\n> use of an *editor* in any way.\n> \n> > +An unparseable or otherwise invalid line is ignored, and a warning\n> > +message points out the problematic line number and file it appears in.\n> \n> OK. You didn't want to tell them they can remove the problematic\n> line as a whole with their editor?\n\nsomeone said \"do not encourage use of an *editor* in any way\" just a few\nlines before ;)\n\n> > +\ttest_config credential.helper store &&\n> > +\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n> > +\tprotocol=https\n> > +\thost=example.com\n> > +\tEOF\n> > +\ttest_cmp expect-stdout stdout &&\n> > +\ttest_i18ngrep \"ignoring invalid credential\" stderr &&\n> > +\ttest_line_count = 3 stderr\n> \n> The \"ignoring invalid credential\" message could be translated into\n> two or more lines, but I think that is worrying too much about\n> theoretical possibility, so checking line count would probably be\n> sufficient.\n\nyes, and specially considering that if I even ended up adding a -c\noption to test_i18ngrep as I intended originally we will still the\nsame issue.\n\nwhat is the preferred way to do something like this, or is it better\nto add a check for each line formatting issue this is handling?\n\nCarlo\n"},{"id":"396641","messageId":"20200430011959.70597-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200429232322.68038-1-carenas@gmail.com","subject":"[PATCH v7] credential-store: warn instead of fatal for bogus lines from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-30T01:19:59Z","receivedAt":"2020-04-30T01:20:23Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"With the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\nInstead of doing a hard check for credentials, do a soft one and\nwarn the user so any invalid entries could be corrected.\n\nMake sure that the credential to use is complete before calling\ncredential_match by confirming it has all fields set as expected\nby the updated rules.\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nHelped-by: Junio C Hamano \nBased-on-patch-by: Jonathan Nieder \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv7:\n* check for protocol in helper as suggested by Junio\nv6:\n* get rid of redacter and only use line number for context in warning\n* make validation more strict to also catch incomplete credentials\n* reorder check as suggested by Junio\nv5:\n* q_to_tab this round, with a single echo to make sure empty line\n is covered, as that seems to be a popular issue\n* rebase on top of jc/credential-store-file-format-doc\n* implement a redacter for credentials to use on errors to avoid\n leaking passwords\nv4:\n* use credential_from_url_gently instead as shown by Jonathan\n* add documentation to clarify \"comments\" is not a supported feature\nv3:\n* avoid using q_to_cr as suggested by Peff\n* a more verbose commit message and slightly more complete documentation\nv2:\n* use a here-doc for clarity as suggested by Eric\n* improve commit message and include documentation\n\n Documentation/git-credential-store.txt | 11 +++-\n credential-store.c | 22 +++++--\n t/t0302-credential-store.sh | 80 ++++++++++++++++++++++++++\n 3 files changed, 107 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 76b0798856..122e238eb2 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -95,8 +95,15 @@ https://user:pass@example.com\n ------------------------------\n \n No other kinds of lines (e.g. empty lines or comment lines) are\n-allowed in the file, even though some may be silently ignored. Do\n-not view or edit the file with editors.\n+allowed in the file, even though historically the parser was very\n+lenient and some might had been silently ignored.\n+\n+Do not view or edit the file with editors as it could compromise the\n+validity of your credentials by sometimes subtle formatting issues,\n+like spaces, line wrapping or text encoding.\n+\n+An unparseable or otherwise invalid line is ignored, and a warning\n+message points out the problematic line number and file it appears in.\n \n When Git needs authentication for a particular URL context,\n credential-store will consider that context a pattern to match against\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..2c5f9736fc 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -6,6 +6,13 @@\n \n static struct lock_file credential_lock;\n \n+static int valid_credential(struct credential *entry)\n+{\n+\treturn (entry->username && entry->password &&\n+\t\tentry->protocol && *entry->protocol &&\n+\t\t((entry->host && *entry->host) || entry->path));\n+}\n+\n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n \t\t\t\t void (*match_cb)(struct credential *),\n@@ -15,6 +22,7 @@ static int parse_credential_file(const char *fn,\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n \tint found_credential = 0;\n+\tint lineno = 0;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n@@ -24,16 +32,22 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tcredential_from_url(&entry, line.buf);\n-\t\tif (entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n+\t\tlineno++;\n+\n+\t\tif (credential_from_url_gently(&entry, line.buf, 1) ||\n+\t\t\t!valid_credential(&entry)) {\n+\t\t\twarning(_(\"%s:%d: ignoring invalid credential\"),\n+\t\t\t\tfn, lineno);\n+\t\t} else if (credential_match(c, &entry)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n \t\t\t\tbreak;\n \t\t\t}\n+\t\t\tcontinue;\n \t\t}\n-\t\telse if (other_cb)\n+\n+\t\tif (other_cb)\n \t\t\tother_cb(&line);\n \t}\n \ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..3150f304cb 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,4 +120,84 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_success 'get: credentials without scheme are invalid' '\n+\techo \"://user:pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials without valid host/path are invalid' '\n+\techo \"https://user:pass@\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials without username/password are invalid' '\n+\techo \"https://pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: store file can contain empty/bogus lines' '\n+\techo \"\" > \"$HOME/.git-credentials\" &&\n+\tq_to_tab <<-\\CONFIG >>\"$HOME/.git-credentials\" &&\n+\t#comment\n+\tQ\n+\thttps://user:pass@example.com\n+\tCONFIG\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr &&\n+\ttest_line_count = 3 stderr\n+'\n+\n test_done\n\nbase-commit: 272281efcc18fcedd248597b8859f343cae1c5a0\n-- \n2.26.2.569.g1d74ac4d14\n\n"},{"id":"396654","messageId":"20200430092939.3564-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200430011959.70597-1-carenas@gmail.com","subject":"[PATCH v8] credential-store: warn instead of fatal for bogus lines from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-30T09:29:39Z","receivedAt":"2020-04-30T09:30:46Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"With the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\nInstead of doing a hard check for credentials, do a soft one and\ntherefore avoid the reported fatal error.\n\nWarn the user indicating the filename and line number so any invalid\nentries could be corrected but continue parsing until a match is\nfound or all valid credentials are processed.\n\nMake sure that the credential that we will use to match is complete by\nconfirming it has all fields set as expected by the updated rules.\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nBased-on-patch-by: Jonathan Nieder \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv8:\n* only warn during get operations as otherwise line number might be\n incorrect\nv7:\n* check for protocol in helper as suggested by Junio\nv6:\n* get rid of redacter and only use line number for context in warning\n* make validation more strict to also catch incomplete credentials\n* reorder check as suggested by Junio\nv5:\n* q_to_tab this round, with a single echo to make sure empty line\n is covered, as that seems to be a popular issue\n* rebase on top of jc/credential-store-file-format-doc\n* implement a redacter for credentials to use on errors to avoid\n leaking passwords\nv4:\n* use credential_from_url_gently instead as shown by Jonathan\n* add documentation to clarify \"comments\" is not a supported feature\nv3:\n* avoid using q_to_cr as suggested by Peff\n* a more verbose commit message and slightly more complete documentation\nv2:\n* use a here-doc for clarity as suggested by Eric\n* improve commit message and include documentation\n\n Documentation/git-credential-store.txt | 11 +++-\n credential-store.c | 31 +++++++--\n t/t0302-credential-store.sh | 89 ++++++++++++++++++++++++++\n 3 files changed, 123 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 76b0798856..d5841cffad 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -95,8 +95,15 @@ https://user:pass@example.com\n ------------------------------\n \n No other kinds of lines (e.g. empty lines or comment lines) are\n-allowed in the file, even though some may be silently ignored. Do\n-not view or edit the file with editors.\n+allowed in the file, even though historically the parser was very\n+lenient and some might had been silently ignored.\n+\n+Do not view or edit the file with editors as it could compromise the\n+validity of your credentials by sometimes subtle formatting issues,\n+like spaces, line wrapping or text encoding.\n+\n+An unparseable or otherwise invalid line is ignored, and a warning\n+message points out the problematic file and line number it appears in.\n \n When Git needs authentication for a particular URL context,\n credential-store will consider that context a pattern to match against\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..9bd8ec9b67 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -4,10 +4,20 @@\n #include \"string-list.h\"\n #include \"parse-options.h\"\n \n+#define PARSE_VERBOSE 0x01\n+\n static struct lock_file credential_lock;\n \n+static int valid_credential(struct credential *entry)\n+{\n+\treturn (entry->username && entry->password &&\n+\t\tentry->protocol && *entry->protocol &&\n+\t\t((entry->host && *entry->host) || entry->path));\n+}\n+\n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n+\t\t\t\t int flags,\n \t\t\t\t void (*match_cb)(struct credential *),\n \t\t\t\t void (*other_cb)(struct strbuf *))\n {\n@@ -15,6 +25,7 @@ static int parse_credential_file(const char *fn,\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n \tint found_credential = 0;\n+\tint lineno = 0;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n@@ -24,16 +35,23 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tcredential_from_url(&entry, line.buf);\n-\t\tif (entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n+\t\tlineno++;\n+\n+\t\tif (credential_from_url_gently(&entry, line.buf, 1) ||\n+\t\t\t!valid_credential(&entry)) {\n+\t\t\tif (flags & PARSE_VERBOSE)\n+\t\t\t\twarning(_(\"%s:%d: ignoring invalid credential\"),\n+\t\t\t\t\tfn, lineno);\n+\t\t} else if (credential_match(c, &entry)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n \t\t\t\tbreak;\n \t\t\t}\n+\t\t\tcontinue;\n \t\t}\n-\t\telse if (other_cb)\n+\n+\t\tif (other_cb)\n \t\t\tother_cb(&line);\n \t}\n \n@@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\tdie_errno(\"unable to get credential storage lock\");\n \tif (extra)\n \t\tprint_line(extra);\n-\tparse_credential_file(fn, c, NULL, print_line);\n+\tparse_credential_file(fn, c, 0, NULL, print_line);\n \tif (commit_lock_file(&credential_lock) < 0)\n \t\tdie_errno(\"unable to write credential store\");\n }\n@@ -139,7 +157,8 @@ static void lookup_credential(const struct string_list *fns, struct credential *\n \tstruct string_list_item *fn;\n \n \tfor_each_string_list_item(fn, fns)\n-\t\tif (parse_credential_file(fn->string, c, print_entry, NULL))\n+\t\tif (parse_credential_file(fn->string, c, PARSE_VERBOSE,\n+\t\t\t\t\t\t print_entry, NULL))\n \t\t\treturn; /* Found credential */\n }\n \ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..8bc89b7efb 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,4 +120,93 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_success 'store: store succeeds silently in corrupted file' '\n+\techo \"#comment\" >\"$HOME/.git-credentials\" &&\n+\tcheck approve store <<-\\EOF &&\n+\turl=https://user:pass@example.com\n+\tEOF\n+\tgrep \"https://user:pass@example.com\" \"$HOME/.git-credentials\" &&\n+\ttest_must_be_empty stderr\n+'\n+\n+test_expect_success 'get: credentials without scheme are invalid' '\n+\techo \"://user:pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials without valid host/path are invalid' '\n+\techo \"https://user:pass@\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials without username/password are invalid' '\n+\techo \"https://pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: store file can contain empty/bogus lines' '\n+\techo \"\" > \"$HOME/.git-credentials\" &&\n+\tq_to_tab <<-\\CONFIG >>\"$HOME/.git-credentials\" &&\n+\t#comment\n+\tQ\n+\thttps://user:pass@example.com\n+\tCONFIG\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr &&\n+\ttest_line_count = 3 stderr\n+'\n+\n test_done\n\nbase-commit: 272281efcc18fcedd248597b8859f343cae1c5a0\n-- \n2.26.2.2.g9c647e0d3b.dirty\n\n"},{"id":"396668","messageId":"20200430160642.90096-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200430011959.70597-1-carenas@gmail.com","subject":"[PATCH v9] credential-store: warn instead of fatal for bogus lines from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-04-30T16:06:42Z","receivedAt":"2020-04-30T16:07:09Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"With the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\nInstead of doing a hard check for credentials, do a soft one and\ntherefore avoid the reported fatal error.\n\nWarn the user indicating the filename and line number so any invalid\nentries could be corrected but continue parsing until a match is\nfound or all valid credentials are processed.\n\nMake sure that the credential that we will use to match is complete by\nconfirming it has all fields set as expected by the updated rules.\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nBased-on-patch-by: Jonathan Nieder \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv9:\n* use strbuf_getline() instead to better handle files with CRLF\nv8:\n* only warn during get operations as otherwise line number might be\n incorrect\nv7:\n* check for protocol in helper as suggested by Junio\nv6:\n* get rid of redacter and only use line number for context in warning\n* make validation more strict to also catch incomplete credentials\n* reorder check as suggested by Junio\nv5:\n* q_to_tab this round, with a single echo to make sure empty line\n is covered, as that seems to be a popular issue\n* rebase on top of jc/credential-store-file-format-doc\n* implement a redacter for credentials to use on errors to avoid\n leaking passwords\nv4:\n* use credential_from_url_gently instead as shown by Jonathan\n* add documentation to clarify \"comments\" is not a supported feature\nv3:\n* avoid using q_to_cr as suggested by Peff\n* a more verbose commit message and slightly more complete documentation\nv2:\n* use a here-doc for clarity as suggested by Eric\n* improve commit message and include documentation\n\n Documentation/git-credential-store.txt | 11 +++-\n credential-store.c | 33 ++++++++--\n t/t0302-credential-store.sh | 90 +++++++++++++++++++++++++-\n 3 files changed, 124 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 76b0798856..d5841cffad 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -95,8 +95,15 @@ https://user:pass@example.com\n ------------------------------\n \n No other kinds of lines (e.g. empty lines or comment lines) are\n-allowed in the file, even though some may be silently ignored. Do\n-not view or edit the file with editors.\n+allowed in the file, even though historically the parser was very\n+lenient and some might had been silently ignored.\n+\n+Do not view or edit the file with editors as it could compromise the\n+validity of your credentials by sometimes subtle formatting issues,\n+like spaces, line wrapping or text encoding.\n+\n+An unparseable or otherwise invalid line is ignored, and a warning\n+message points out the problematic file and line number it appears in.\n \n When Git needs authentication for a particular URL context,\n credential-store will consider that context a pattern to match against\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..b1f5b2dea6 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -4,10 +4,20 @@\n #include \"string-list.h\"\n #include \"parse-options.h\"\n \n+#define PARSE_VERBOSE 0x01\n+\n static struct lock_file credential_lock;\n \n+static int valid_credential(struct credential *entry)\n+{\n+\treturn (entry->username && entry->password &&\n+\t\tentry->protocol && *entry->protocol &&\n+\t\t((entry->host && *entry->host) || entry->path));\n+}\n+\n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n+\t\t\t\t int flags,\n \t\t\t\t void (*match_cb)(struct credential *),\n \t\t\t\t void (*other_cb)(struct strbuf *))\n {\n@@ -15,6 +25,7 @@ static int parse_credential_file(const char *fn,\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n \tint found_credential = 0;\n+\tint lineno = 0;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n@@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn,\n \t\treturn found_credential;\n \t}\n \n-\twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tcredential_from_url(&entry, line.buf);\n-\t\tif (entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n+\twhile (strbuf_getline(&line, fh) != EOF) {\n+\t\tlineno++;\n+\n+\t\tif (credential_from_url_gently(&entry, line.buf, 1) ||\n+\t\t\t!valid_credential(&entry)) {\n+\t\t\tif (flags & PARSE_VERBOSE)\n+\t\t\t\twarning(_(\"%s:%d: ignoring invalid credential\"),\n+\t\t\t\t\tfn, lineno);\n+\t\t} else if (credential_match(c, &entry)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n \t\t\t\tbreak;\n \t\t\t}\n+\t\t\tcontinue;\n \t\t}\n-\t\telse if (other_cb)\n+\n+\t\tif (other_cb)\n \t\t\tother_cb(&line);\n \t}\n \n@@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\tdie_errno(\"unable to get credential storage lock\");\n \tif (extra)\n \t\tprint_line(extra);\n-\tparse_credential_file(fn, c, NULL, print_line);\n+\tparse_credential_file(fn, c, 0, NULL, print_line);\n \tif (commit_lock_file(&credential_lock) < 0)\n \t\tdie_errno(\"unable to write credential store\");\n }\n@@ -139,7 +157,8 @@ static void lookup_credential(const struct string_list *fns, struct credential *\n \tstruct string_list_item *fn;\n \n \tfor_each_string_list_item(fn, fns)\n-\t\tif (parse_credential_file(fn->string, c, print_entry, NULL))\n+\t\tif (parse_credential_file(fn->string, c, PARSE_VERBOSE,\n+\t\t\t\t\t\t print_entry, NULL))\n \t\t\treturn; /* Found credential */\n }\n \ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..05fd2785b9 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n-\n test_expect_success 'erase: erase matching credentials from both xdg and home files' '\n \techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n \tmkdir -p \"$HOME/.config/git\" &&\n@@ -120,4 +119,93 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_success 'get: credentials without scheme are invalid' '\n+\techo \"://user:pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials without valid host/path are invalid' '\n+\techo \"https://user:pass@\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials without username/password are invalid' '\n+\techo \"https://pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: store file can contain empty/bogus lines' '\n+\techo \"\" > \"$HOME/.git-credentials\" &&\n+\tq_to_tab <<-\\CONFIG >>\"$HOME/.git-credentials\" &&\n+\t#comment\n+\tQ\n+\thttps://user:pass@example.com\n+\tCONFIG\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr &&\n+\ttest_line_count = 3 stderr\n+'\n+\n+test_expect_success 'store: store succeeds silently in corrupted file' '\n+\techo \"#comment\" >\"$HOME/.git-credentials\" &&\n+\tcheck approve store <<-\\EOF &&\n+\turl=https://user:pass@example.com\n+\tEOF\n+\tgrep \"https://user:pass@example.com\" \"$HOME/.git-credentials\" &&\n+\ttest_must_be_empty stderr\n+'\n+\n test_done\n\nbase-commit: 272281efcc18fcedd248597b8859f343cae1c5a0\n-- \n2.26.2.2.gf219a08795\n\n"},{"id":"396694","messageId":"xmqq1ro4sp1p.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200430160642.90096-1-carenas@gmail.com","subject":"Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-30T20:21:06Z","receivedAt":"2020-04-30T20:21:17Z","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> With the added checks for invalid URLs in credentials, any locally\n> modified store files which might have empty lines or even comments\n> were reported[1] failing to parse as valid credentials.\n>\n> Instead of doing a hard check for credentials, do a soft one and\n> therefore avoid the reported fatal error.\n>\n> Warn the user indicating the filename and line number so any invalid\n> entries could be corrected but continue parsing until a match is\n> found or all valid credentials are processed.\n>\n> Make sure that the credential that we will use to match is complete by\n> confirming it has all fields set as expected by the updated rules.\n>\n> [1] https://stackoverflow.com/a/61420852/5005936\n>\n> Reported-by: Dirk \n> Helped-by: Eric Sunshine \n> Based-on-patch-by: Jonathan Nieder \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n\nLooks good.\n\n> diff --git a/credential-store.c b/credential-store.c\n> index c010497cb2..b1f5b2dea6 100644\n> --- a/credential-store.c\n> +++ b/credential-store.c\n> @@ -4,10 +4,20 @@\n> #include \"string-list.h\"\n> #include \"parse-options.h\"\n> \n> +#define PARSE_VERBOSE 0x01\n> +\n> static struct lock_file credential_lock;\n> \n> +static int valid_credential(struct credential *entry)\n> +{\n> +\treturn (entry->username && entry->password &&\n> +\t\tentry->protocol && *entry->protocol &&\n> +\t\t((entry->host && *entry->host) || entry->path));\n> +}\n\nOK.\n\n> static int parse_credential_file(const char *fn,\n> \t\t\t\t struct credential *c,\n> +\t\t\t\t int flags,\n> \t\t\t\t void (*match_cb)(struct credential *),\n> \t\t\t\t void (*other_cb)(struct strbuf *))\n> {\n> @@ -15,6 +25,7 @@ static int parse_credential_file(const char *fn,\n> \tstruct strbuf line = STRBUF_INIT;\n> \tstruct credential entry = CREDENTIAL_INIT;\n> \tint found_credential = 0;\n> +\tint lineno = 0;\n> \n> \tfh = fopen(fn, \"r\");\n> \tif (!fh) {\n> @@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn,\n> \t\treturn found_credential;\n> \t}\n> \n> -\twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> -\t\tcredential_from_url(&entry, line.buf);\n> -\t\tif (entry.username && entry.password &&\n> -\t\t credential_match(c, &entry)) {\n> +\twhile (strbuf_getline(&line, fh) != EOF) {\n\nHmph, I probably have missed some discussion that happened in the\nlast few days, but from the use of _lf() in the original, I sense\nthat it is very much deliberate that the file format is designed to\nbe LF delimited records, *not* a text file in which each line is\nterminated with some end-of-line marker that is platform dependent.\nIOW, an explicit use of _lf() shouts, at least to me, that we want a\nsequence of LF terminated records even on Windows and Macs.\n\nSo, I am not sure why this change is desirable.\n\n> +\t\tlineno++;\n> +\n> +\t\tif (credential_from_url_gently(&entry, line.buf, 1) ||\n> +\t\t\t!valid_credential(&entry)) {\n\nOK, so we read a line, fed it to the parser, and if we had trouble\nparsing or the line did not have enough credential material, we\ndiscard and warn (when told to).\n\n> +\t\t\tif (flags & PARSE_VERBOSE)\n> +\t\t\t\twarning(_(\"%s:%d: ignoring invalid credential\"),\n> +\t\t\t\t\tfn, lineno);\n> +\t\t} else if (credential_match(c, &entry)) {\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n> \t\t\t\tmatch_cb(&entry);\n> \t\t\t\tbreak;\n> \t\t\t}\n> +\t\t\tcontinue;\n\nAnd if the credential material on a good line matches, we remember\nwe saw a match. If there is match callback, we call it and leave\nthe loop to return from the function. Otherwise we go to the next\nline.\n> \t\t}\n> +\n> +\t\tif (other_cb)\n> \t\t\tother_cb(&line);\n\nA malformed/underspecified line and an unmatched line is fed to the\nother callback.\n\n> \t}\n> \n> @@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n> \t\tdie_errno(\"unable to get credential storage lock\");\n> \tif (extra)\n> \t\tprint_line(extra);\n> -\tparse_credential_file(fn, c, NULL, print_line);\n> +\tparse_credential_file(fn, c, 0, NULL, print_line);\n\nThis helper function is called from two places.\n\n - In store_credential_file, we write the credential we were asked\n to store to a strbuf, and then call this function. The function\n first writes that new credential and then calls the parse helper\n without PARSE_VERBOSE. We do not give any match callback, and\n other callback is to just print the line read from the credential\n file. So, the final output would be the new credential, followed\n by the current contents of the credential file except for the\n records that match the one we are storing. We do this without\n warning about malformed entries at all.\n\n - In remove_credential, we go over multiple files and copy the\n lines in each of them, except the ones that match the credential\n we are given. We also do this without warning about malformed\n entries at all.\n\n> \tif (commit_lock_file(&credential_lock) < 0)\n> \t\tdie_errno(\"unable to write credential store\");\n> }\n> @@ -139,7 +157,8 @@ static void lookup_credential(const struct string_list *fns, struct credential *\n> \tstruct string_list_item *fn;\n> \n> \tfor_each_string_list_item(fn, fns)\n> -\t\tif (parse_credential_file(fn->string, c, print_entry, NULL))\n> +\t\tif (parse_credential_file(fn->string, c, PARSE_VERBOSE,\n> +\t\t\t\t\t\t print_entry, NULL))\n> \t\t\treturn; /* Found credential */\n> }\n\nThis is triggered by the \"get\" operation. We read until we hit the\nentry we are looking for, at that point the match-callback will\nprint out \"username=... / password=...\" lines and we ignore the\nremainder of the file. While we look for the first matching\ncredential, we do warn about malformed or underspecified lines that\nwe are ignoring, but because we leave the loop once a matching line\nis found, bad lines that come after it won't be even looked at to be\nwarned.\n\nValidating and warning about bad entries is *not* the main purpose\nof the \"credential-store\" program, so I fully agree with the design\nof the \"get\" part. I am not so sure about the other two operations\n(i.e. \"store\" and \"erase\") that do scan all the entries and has\nchance to warn about bad ones, though (note: I am not saying that we\nshould parse verbosely---it is just that I do not know why you chose\nnot to and I am not convinced that it is a good idea not to warn).\n\nThe tests looked good.\n\nThanks.\n\n"},{"id":"396699","messageId":"xmqq7dxwr80o.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"xmqq1ro4sp1p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-30T21:14:15Z","receivedAt":"2020-04-30T21:14:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n[part of the commit log message]\n\n>> Warn the user indicating the filename and line number so any invalid\n>> entries could be corrected but continue parsing until a match is\n>> found or all valid credentials are processed.\n\nWe should say \"Do so only during the 'get' operation; give up giving\nany warning for 'erase' or 'store' operation, as keeping track of\nthe line number of the input file, while having to issue a warning\nthat has the line number of the output file, is too hard for us\" or\nsomething like that here.\n\nI suspect that it might not be \"too hard\", but we'd need to rename\nother_cb to a more specific name first and limit the possibility of\nrepurposing parse_credential_file(). \n\nFor example, other_cb can become \"copy_cb\", we declare that the\nfunction works in two (and no other) modes, one is to look-up (which\nis read-only so we report the input line number in our warning\nmessges) and the other is to copy-out-with-filtering (which will\nleave a file that is different from the input, so we report the\noutput line number).\n\nTo support the copy-out-wiht-filtering mode, we pass the starting\nline number into the function (i.e. 'store' may store a new line, so\nthe first line copied out to the file may be the second line in the\noutput), and count the output lines there:\n\n\tif (other_cb) {\n\t\tlines++;\n\t\tother_cb(&line);\n\t}\n\nand of course we won't increment lines++ when we saw a match in the\ncopy-out-with-filtering mode. In look-up mode (i.e. copy_cb == NULL),\nwe count the input lines just like your code.\n\nBut I suspect that it may not be worth doing. But if we decide not\nto do so, we should document why we chose (not) to in the commit log\nmessage.\n\n> Validating and warning about bad entries is *not* the main purpose\n> of the \"credential-store\" program, so I fully agree with the design\n> of the \"get\" part. I am not so sure about the other two operations\n> (i.e. \"store\" and \"erase\") that do scan all the entries and has\n> chance to warn about bad ones, though (note: I am not saying that we\n> should parse verbosely---it is just that I do not know why you chose\n> not to and I am not convinced that it is a good idea not to warn).\n\nI re-read the incremental log for v8 and found your explanation.\n\nThanks.\n"},{"id":"396706","messageId":"20200501003041.GD33264@Carlos-MBP","threadId":"53326","inReplyTo":"xmqq1ro4sp1p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-01T00:30:41Z","receivedAt":"2020-05-01T00:30:47Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Apr 30, 2020 at 01:21:06PM -0700, Junio C Hamano wrote:\n> Carlo Marcelo Arenas Belón writes:\n> > @@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn,\n> > \t\treturn found_credential;\n> > \t}\n> > \n> > -\twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> > -\t\tcredential_from_url(&entry, line.buf);\n> > -\t\tif (entry.username && entry.password &&\n> > -\t\t credential_match(c, &entry)) {\n> > +\twhile (strbuf_getline(&line, fh) != EOF) {\n> \n> Hmph, I probably have missed some discussion that happened in the\n> last few days, but from the use of _lf() in the original, I sense\n> that it is very much deliberate that the file format is designed to\n> be LF delimited records, *not* a text file in which each line is\n> terminated with some end-of-line marker that is platform dependent.\n> IOW, an explicit use of _lf() shouts, at least to me, that we want a\n> sequence of LF terminated records even on Windows and Macs.\n\napologize for the confusion, the only discussion was the one I had\nwith myself late at night when I realized that specific corruption\nwas not being detected, and might be related to issues that seemed\nto be common[1]\n\nthe problem is that practically speaking, if users in Windows and Macs\nedited the file they \"might\" had saved lines with the wrong line ending (*)\n(part of the reason I added a warning about \"encoding\" to the documentation),\nand those are difficult to \"see\" as invalid.\n\nusing the non _lf() version ensures any trailing '\\r' character would\nget removed from the credential and allow us to use them, instead of\nsilently failing.\n\noriginally I added a explicit check for it, which I assume you would prefer\nbut .. \n\n> So, I am not sure why this change is desirable.\n\nto give users immediate relief from what seems to be a commonly seen issue.\n\nIMHO after we get this released out and they see the updated documentation\nexplaining the risks, and some learn how to fix their credential files (\nand hopefully use an editor that lets them see the problem); we could then\nadd also a detection for this edge case and go back to _lf()\n\nI also had to admit I might had overreacted, as I was testing before v8 with\na very corrupted file and was seeing warnings twice on each operation and\nsomehow even got myself into the original fatal, which I had to admit I can't\nreplicate now after some needed rest.\n\nCarlo\n\n[1] https://github.com/desktop/desktop/issues/9597\n\n(*) textedit, which comes with macOS doesn't even write an EOF record when\n creating files, for example, and that behaviour seems to be common for\n most other native editors but a credential line WITHOUT line ending works\n"},{"id":"396708","messageId":"xmqqimhgph3z.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200501003041.GD33264@Carlos-MBP","subject":"Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-01T01:40:48Z","receivedAt":"2020-05-01T01:40:57Z","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> the problem is that practically speaking, if users in Windows and Macs\n> edited the file they \"might\" had saved lines with the wrong line ending (*)\n> (part of the reason I added a warning about \"encoding\" to the documentation),\n> and those are difficult to \"see\" as invalid.\n>\n> using the non _lf() version ensures any trailing '\\r' character would\n> get removed from the credential and allow us to use them, instead of\n> silently failing.\n\nYou are forgetting why we are fixing credential-store, aren't you?\n\nIt is primarily to help those who damaged their files by editing,\nand introducing cruft that cannot be parsed, from a stricter parsing\nintroduced recently in 2.17.4 and above. Without the fix, they\ncannot operate with the store they already have.\n\nNow, think about those users who saved their file, after adding CR\nat the end of each line, but didn't do any other edit (like adding a\nblank line or \"# comment\"). It may have happened 3 years ago. What\ndid they see from the system back then? It may have happened 3\nminutes ago. What would they see with the stricter parser now?\n\nWith or without the recent parser change, they would have seen that\nthese lines no longer match the URLs they wanted to match, but the\ncredential store does not die on them for malformed lines, no?\n\nIn other words, the stricter parsing did nothing to these users.\n\nIn fact, those users who added CR at the end of each line 3 years\nago may have even depended on the disappearance of these entries for\nall these years. Maybe lines that record their ancient passwords\nfor sites are still buried in the later parts of the file, with CR,\nbut doing no harm because these lines do not match anything. These\nusers may have changed their password since then and wrote new\nrecords with \"credential store\", and these new records are stored\nwithout CR at the end of the line, so they match the URLs.\n\nBy using the non _lf() variant, you are suddenly resurrecting these\nold records that the users thought are already gone and have been\ncausing no harm to them. Do we know that resurrecting these old\nrecords is a good thing to do? I don't. For example, once the user\ndecides to \"sort\" the file (after all, we are talking about users\nwho edit the file, so we cannot assume they won't do so), they would\nend up with duplicate records that record two passwords to the same\nsite and they cannot tell which one is current, as you even lost the\nCR at the end of line that would have told you which ones are\nbroken.\n\nIn short, you wouldn't know what ramification it has by suddenly\nusing non _lf() variant. And it has nothing to do with the fix we\nare trying to make.\n\nWhen fixing something, it is tempting to see if you can cover a bit\nmore area with the same change. But you should make it a habit to\nstick to the absolute minimum first for safety. When contemplating\nto step out of the absolute minimum, you need to think twice to see\nif that extra \"fix\" really fixes, or if it potentially harms.\n\nAnd I am not convinced that turning CRLF into LF in this case is a\ngood change. In any case, it certainly does not belong to the same\ncommit as the one that fixes the fallout from stricter parser\nintroduced in 2.17.4 and above.\n\nThanks.\n"},{"id":"396709","messageId":"CAPUEspiD10bYsCDoHM6PJdidyofNadJL0bexjiNNZCXNU+Ay_Q@mail.gmail.com","threadId":"53326","inReplyTo":"xmqqimhgph3z.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2020-05-01T02:24:14Z","receivedAt":"2020-05-01T02:26:58Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Apr 30, 2020 at 6:40 PM Junio C Hamano wrote:\n>\n> Carlo Marcelo Arenas Belón writes:\n>\n> > the problem is that practically speaking, if users in Windows and Macs\n> > edited the file they \"might\" had saved lines with the wrong line ending (*)\n> > (part of the reason I added a warning about \"encoding\" to the documentation),\n> > and those are difficult to \"see\" as invalid.\n> >\n> > using the non _lf() version ensures any trailing '\\r' character would\n> > get removed from the credential and allow us to use them, instead of\n> > silently failing.\n>\n> You are forgetting why we are fixing credential-store, aren't you?\n\nindeed, and thanks for the clarification.\n\nyou are correct this was a bad idea and is better to warn them instead\n(even if only\nduring get operations and for the lines that were processed) than my \"solution\".\n\nFWIW though, my change wasn't going to change the file on disk but only allow\nthe line to be processed.\n\ngave up already on sneakily \"fixing\" corruption issues after Peff\ncalled me on it\nand yes, another version you will never see had a PARSER_TRIM flag added ;)\n\nCarlo\n\nPS. should we really do the warn even in store/erase operations as a\nfollowup or is the warning not useful as is, and probably then all\noperations should be quiet (as Jonathan suggested originally?) and we\ncould do warn (and maybe fix) in a different change (maybe adding a\nfsck command of sorts)?\n\n> When fixing something, it is tempting to see if you can cover a bit\n> more area with the same change. But you should make it a habit to\n> stick to the absolute minimum first for safety. When contemplating\n> to step out of the absolute minimum, you need to think twice to see\n> if that extra \"fix\" really fixes, or if it potentially harms.\n"},{"id":"396710","messageId":"20200501032152.12160-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200430160642.90096-1-carenas@gmail.com","subject":"[RFC PATCH v10] credential-store: warn/ignore for bogus lines from store file","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-01T03:21:52Z","receivedAt":"2020-05-01T03:22:47Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"With the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\nInstead of doing a hard check for credentials, do a soft one and\ntherefore avoid the reported fatal error.\n\nWarn the user indicating the filename and line number during get,\nfor all entries that were processed, but continue parsing until\na match is found or all valid credentials are processed.\n\nWarning during store and erase operations has been suppressed\nfor now by adding a PARSER_VERBOSE flag that is only enabled\nduring get operations.\n\nMake sure that the credential that we will use to match is complete by\nconfirming it has all fields set as expected by the updated rules.\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nBased-on-patch-by: Jonathan Nieder \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv10:\n* insted of trying to use broken credentials in DOS format, make\n sure they are flagged as broken\nv9:\n* use strbuf_getline() instead to better handle files with CRLF\nv8:\n* only warn during get operations as otherwise line number might be\n incorrect\nv7:\n* check for protocol in helper as suggested by Junio\nv6:\n* get rid of redacter and only use line number for context in warning\n* make validation more strict to also catch incomplete credentials\n* reorder check as suggested by Junio\nv5:\n* q_to_tab this round, with a single echo to make sure empty line\n is covered, as that seems to be a popular issue\n* rebase on top of jc/credential-store-file-format-doc\n* implement a redacter for credentials to use on errors to avoid\n leaking passwords\nv4:\n* use credential_from_url_gently instead as shown by Jonathan\n* add documentation to clarify \"comments\" is not a supported feature\nv3:\n* avoid using q_to_cr as suggested by Peff\n* a more verbose commit message and slightly more complete documentation\nv2:\n* use a here-doc for clarity as suggested by Eric\n* improve commit message and include documentation\n\n Documentation/git-credential-store.txt | 18 +++-\n credential-store.c | 32 ++++++--\n t/t0302-credential-store.sh | 109 ++++++++++++++++++++++++-\n 3 files changed, 150 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 76b0798856..18ba8b6984 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -95,8 +95,22 @@ https://user:pass@example.com\n ------------------------------\n \n No other kinds of lines (e.g. empty lines or comment lines) are\n-allowed in the file, even though some may be silently ignored. Do\n-not view or edit the file with editors.\n+allowed in the file, even though historically the parser was very\n+lenient and some might had been silently ignored.\n+\n+Do not view or edit the file with editors as it could compromise the\n+validity of your credentials by sometimes subtle formatting issues,\n+like spaces, line wrapping or text encoding.\n+\n+An unparseable or otherwise invalid line is ignored, and a warning\n+message points out the problematic file and line number it appears in.\n+\n+Note that because of deficiencies of the current implementation, these\n+warnings will be only generated for the entries that were processed\n+while reading the credential files during a get operation, and so they\n+are not comprehensive, and will require you to use an editor (or\n+better a sed/awk/perl one liner) to edit the credential file and remove\n+them.\n \n When Git needs authentication for a particular URL context,\n credential-store will consider that context a pattern to match against\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..c2778eff8a 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -4,10 +4,20 @@\n #include \"string-list.h\"\n #include \"parse-options.h\"\n \n+#define PARSE_VERBOSE 0x01\n+\n static struct lock_file credential_lock;\n \n+static int valid_credential(struct credential *entry)\n+{\n+\treturn (entry->username && entry->password &&\n+\t\tentry->protocol && *entry->protocol &&\n+\t\t((entry->host && *entry->host) || entry->path));\n+}\n+\n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n+\t\t\t\t int flags,\n \t\t\t\t void (*match_cb)(struct credential *),\n \t\t\t\t void (*other_cb)(struct strbuf *))\n {\n@@ -15,6 +25,7 @@ static int parse_credential_file(const char *fn,\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n \tint found_credential = 0;\n+\tint lineno = 0;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n@@ -24,16 +35,24 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tcredential_from_url(&entry, line.buf);\n-\t\tif (entry.username && entry.password &&\n-\t\t credential_match(c, &entry)) {\n+\t\tlineno++;\n+\n+\t\tif (strchr(line.buf, '\\r') ||\n+\t\t\tcredential_from_url_gently(&entry, line.buf, 1) ||\n+\t\t\t!valid_credential(&entry)) {\n+\t\t\tif (flags & PARSE_VERBOSE)\n+\t\t\t\twarning(_(\"%s:%d: ignoring invalid credential\"),\n+\t\t\t\t\tfn, lineno);\n+\t\t} else if (credential_match(c, &entry)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\n \t\t\t\tmatch_cb(&entry);\n \t\t\t\tbreak;\n \t\t\t}\n+\t\t\tcontinue;\n \t\t}\n-\t\telse if (other_cb)\n+\n+\t\tif (other_cb)\n \t\t\tother_cb(&line);\n \t}\n \n@@ -62,7 +81,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\tdie_errno(\"unable to get credential storage lock\");\n \tif (extra)\n \t\tprint_line(extra);\n-\tparse_credential_file(fn, c, NULL, print_line);\n+\tparse_credential_file(fn, c, 0, NULL, print_line);\n \tif (commit_lock_file(&credential_lock) < 0)\n \t\tdie_errno(\"unable to write credential store\");\n }\n@@ -139,7 +158,8 @@ static void lookup_credential(const struct string_list *fns, struct credential *\n \tstruct string_list_item *fn;\n \n \tfor_each_string_list_item(fn, fns)\n-\t\tif (parse_credential_file(fn->string, c, print_entry, NULL))\n+\t\tif (parse_credential_file(fn->string, c, PARSE_VERBOSE,\n+\t\t\t\t\t\t print_entry, NULL))\n \t\t\treturn; /* Found credential */\n }\n \ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..00608e365a 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n-\n test_expect_success 'erase: erase matching credentials from both xdg and home files' '\n \techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n \tmkdir -p \"$HOME/.config/git\" &&\n@@ -120,4 +119,112 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+test_expect_success 'get: credentials without scheme are invalid' '\n+\techo \"://user:pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials without valid host/path are invalid' '\n+\techo \"https://user:pass@\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials without username/password are invalid' '\n+\techo \"https://pass@example.com\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: credentials with DOS line endings are invalid' '\n+\tprintf \"https://user:pass@example.com\\r\\n\" >\"$HOME/.git-credentials\" &&\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\tgrep \"askpass: Username for '\\''https://example.com'\\'':\" stderr &&\n+\tgrep \"askpass: Password for '\\''https://askpass-username@example.com'\\'':\" stderr &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'get: store file can contain empty/bogus lines' '\n+\techo \"\" > \"$HOME/.git-credentials\" &&\n+\tq_to_tab <<-\\CONFIG >>\"$HOME/.git-credentials\" &&\n+\t#comment\n+\tQ\n+\thttps://user:pass@example.com\n+\tCONFIG\n+\tcat >expect-stdout <<-\\STDOUT &&\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\tSTDOUT\n+\ttest_config credential.helper store &&\n+\tgit credential fill <<-\\EOF >stdout 2>stderr &&\n+\tprotocol=https\n+\thost=example.com\n+\tEOF\n+\ttest_cmp expect-stdout stdout &&\n+\ttest_i18ngrep \"ignoring invalid credential\" stderr &&\n+\ttest_line_count = 3 stderr\n+'\n+\n+test_expect_success 'store: store succeeds silently in corrupted file' '\n+\techo \"#comment\" >\"$HOME/.git-credentials\" &&\n+\tcheck approve store <<-\\EOF &&\n+\turl=https://user:pass@example.com\n+\tEOF\n+\tgrep \"https://user:pass@example.com\" \"$HOME/.git-credentials\" &&\n+\ttest_must_be_empty stderr\n+'\n+\n test_done\n\nbase-commit: 272281efcc18fcedd248597b8859f343cae1c5a0\n-- \n2.26.2.2.gf219a08795\n\n"},{"id":"396711","messageId":"20200501051847.31803-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200501032152.12160-1-carenas@gmail.com","subject":"[RFC PATCH v10 2/1] credential-store: warn also for store and erase","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-01T05:18:47Z","receivedAt":"2020-05-01T05:19:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"done in a hacky way and only as a POC, but at least works, if we\nare still thinking that warning should be at least comprehensive\nfor the first iteration.\n\ndidn't change the callback as you suggested but I think this\ndoesn't look that bad either.\n\nsquash for final v10 or ..?\n---\n Documentation/git-credential-store.txt | 7 -------\n credential-store.c | 19 +++++++++----------\n t/t0302-credential-store.sh | 20 +++++++++++++++++---\n 3 files changed, 26 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt\nindex 18ba8b6984..d5841cffad 100644\n--- a/Documentation/git-credential-store.txt\n+++ b/Documentation/git-credential-store.txt\n@@ -105,13 +105,6 @@ like spaces, line wrapping or text encoding.\n An unparseable or otherwise invalid line is ignored, and a warning\n message points out the problematic file and line number it appears in.\n \n-Note that because of deficiencies of the current implementation, these\n-warnings will be only generated for the entries that were processed\n-while reading the credential files during a get operation, and so they\n-are not comprehensive, and will require you to use an editor (or\n-better a sed/awk/perl one liner) to edit the credential file and remove\n-them.\n-\n When Git needs authentication for a particular URL context,\n credential-store will consider that context a pattern to match against\n each entry in the credentials file. If the protocol, hostname, and\ndiff --git a/credential-store.c b/credential-store.c\nindex c2778eff8a..236df8e7bf 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -4,8 +4,6 @@\n #include \"string-list.h\"\n #include \"parse-options.h\"\n \n-#define PARSE_VERBOSE 0x01\n-\n static struct lock_file credential_lock;\n \n static int valid_credential(struct credential *entry)\n@@ -17,7 +15,7 @@ static int valid_credential(struct credential *entry)\n \n static int parse_credential_file(const char *fn,\n \t\t\t\t struct credential *c,\n-\t\t\t\t int flags,\n+\t\t\t\t int initial_line,\n \t\t\t\t void (*match_cb)(struct credential *),\n \t\t\t\t void (*other_cb)(struct strbuf *))\n {\n@@ -25,7 +23,7 @@ static int parse_credential_file(const char *fn,\n \tstruct strbuf line = STRBUF_INIT;\n \tstruct credential entry = CREDENTIAL_INIT;\n \tint found_credential = 0;\n-\tint lineno = 0;\n+\tint lineno = initial_line;\n \n \tfh = fopen(fn, \"r\");\n \tif (!fh) {\n@@ -40,8 +38,7 @@ static int parse_credential_file(const char *fn,\n \t\tif (strchr(line.buf, '\\r') ||\n \t\t\tcredential_from_url_gently(&entry, line.buf, 1) ||\n \t\t\t!valid_credential(&entry)) {\n-\t\t\tif (flags & PARSE_VERBOSE)\n-\t\t\t\twarning(_(\"%s:%d: ignoring invalid credential\"),\n+\t\t\twarning(_(\"%s:%d: ignoring invalid credential\"),\n \t\t\t\t\tfn, lineno);\n \t\t} else if (credential_match(c, &entry)) {\n \t\t\tfound_credential = 1;\n@@ -77,11 +74,14 @@ static void print_line(struct strbuf *buf)\n static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\t\t\t struct strbuf *extra)\n {\n+\tint initial_line = 0;\n \tif (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)\n \t\tdie_errno(\"unable to get credential storage lock\");\n-\tif (extra)\n+\tif (extra) {\n \t\tprint_line(extra);\n-\tparse_credential_file(fn, c, 0, NULL, print_line);\n+\t\tinitial_line++;\n+\t}\n+\tparse_credential_file(fn, c, initial_line, NULL, print_line);\n \tif (commit_lock_file(&credential_lock) < 0)\n \t\tdie_errno(\"unable to write credential store\");\n }\n@@ -158,8 +158,7 @@ static void lookup_credential(const struct string_list *fns, struct credential *\n \tstruct string_list_item *fn;\n \n \tfor_each_string_list_item(fn, fns)\n-\t\tif (parse_credential_file(fn->string, c, PARSE_VERBOSE,\n-\t\t\t\t\t\t print_entry, NULL))\n+\t\tif (parse_credential_file(fn->string, c, 0, print_entry, NULL))\n \t\t\treturn; /* Found credential */\n }\n \ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex 00608e365a..78010590f4 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -218,13 +218,27 @@ test_expect_success 'get: store file can contain empty/bogus lines' '\n \ttest_line_count = 3 stderr\n '\n \n-test_expect_success 'store: store succeeds silently in corrupted file' '\n+test_expect_success 'store: succeeds in corrupted file but correctly warn' '\n \techo \"#comment\" >\"$HOME/.git-credentials\" &&\n-\tcheck approve store <<-\\EOF &&\n+\ttest_config credential.helper store &&\n+\tgit credential approve <<-\\EOF &&\n \turl=https://user:pass@example.com\n \tEOF\n \tgrep \"https://user:pass@example.com\" \"$HOME/.git-credentials\" &&\n-\ttest_must_be_empty stderr\n+\ttest_i18ngrep \"git-credentials:2: ignoring invalid credential\" stderr\n+'\n+\n+test_expect_success 'erase: succeeds in corrupted file but correctly warn' '\n+\tcat <<-\\CONFIG >\"$HOME/.git-credentials\" &&\n+\t#comment\n+\thttps://user:pass@example.com\n+\tCONFIG\n+\ttest_config credential.helper store &&\n+\tgit credential reject <<-\\EOF &&\n+\turl=https://example.com\n+\tEOF\n+\t! grep \"https://user:pass@example.com\" \"$HOME/.git-credentials\" &&\n+\ttest_i18ngrep \"git-credentials:1: ignoring invalid credential\" stderr\n '\n \n test_done\n-- \n2.26.2.3.g90bd2f7136\n\n"},{"id":"396712","messageId":"xmqqees4p6mb.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"CAPUEspiD10bYsCDoHM6PJdidyofNadJL0bexjiNNZCXNU+Ay_Q@mail.gmail.com","subject":"Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-01T05:27:24Z","receivedAt":"2020-05-01T05:27:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> PS. should we really do the warn even in store/erase operations as a\n> followup or is the warning not useful as is, and probably then all\n> operations should be quiet (as Jonathan suggested originally?) and we\n> could do warn (and maybe fix) in a different change (maybe adding a\n> fsck command of sorts)?\n\nYeah, I think I like the \"no warning, just ignore\" much better. The\nimplementation I suspect would become a lot simpler, right?\n\nThanks for thinking one extra step.\n"},{"id":"396713","messageId":"xmqqa72sp696.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200501051847.31803-1-carenas@gmail.com","subject":"Re: [RFC PATCH v10 2/1] credential-store: warn also for store and erase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-01T05:35:17Z","receivedAt":"2020-05-01T05:35:24Z","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> done in a hacky way and only as a POC, but at least works, if we\n> are still thinking that warning should be at least comprehensive\n> for the first iteration.\n\n...\n\n> @@ -25,7 +23,7 @@ static int parse_credential_file(const char *fn,\n> \tstruct strbuf line = STRBUF_INIT;\n> \tstruct credential entry = CREDENTIAL_INIT;\n> \tint found_credential = 0;\n> -\tint lineno = 0;\n> +\tint lineno = initial_line;\n> \n> \tfh = fopen(fn, \"r\");\n> \tif (!fh) {\n> @@ -40,8 +38,7 @@ static int parse_credential_file(const char *fn,\n> \t\tif (strchr(line.buf, '\\r') ||\n> \t\t\tcredential_from_url_gently(&entry, line.buf, 1) ||\n> \t\t\t!valid_credential(&entry)) {\n> -\t\t\tif (flags & PARSE_VERBOSE)\n> -\t\t\t\twarning(_(\"%s:%d: ignoring invalid credential\"),\n> +\t\t\twarning(_(\"%s:%d: ignoring invalid credential\"),\n> \t\t\t\t\tfn, lineno);\n> \t\t} else if (credential_match(c, &entry)) {\n> \t\t\tfound_credential = 1;\n\nI am not sure how this would correctly count when \"store\" and\n\"erase\" is used. During \"store\", the initial_line may let us\ncorrectly count the new line, and as long as we keep copying, our\noutput line number would stay to be correct, as we increment line by\none every time we read in, but as soon as we see the older version\nof the credential record this \"store\" is updating, i.e. when the\ncredential_match() reports a match, we will discard the line we read\nby hitting the \"continue\" and not calling other_cb(). We increment\nline, but we don't output a line when that happens, so any warning\nafter that point will be out of sync, no? And \"erase\" is the same\ndeal. It behaves the same way as \"store\" except that \"erase\" does\nnot emit a replacement record at the beginning. As soon as the\nmatching record from the input is dropped, our line number will be\nout of sync.\n\nIn any case, I think it probably is a good idea to start with a\nversion that ignores without warning, and I had an impression that\nyou were on the same page from reading your response to v9 review,\nso...\n"},{"id":"396746","messageId":"20200501135757.GF33264@Carlos-MBP","threadId":"53326","inReplyTo":"xmqqees4p6mb.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-01T13:57:57Z","receivedAt":"2020-05-01T13:58:03Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Apr 30, 2020 at 10:27:24PM -0700, Junio C Hamano wrote:\n> Carlo Arenas writes:\n> \n> > PS. should we really do the warn even in store/erase operations as a\n> > followup or is the warning not useful as is, and probably then all\n> > operations should be quiet (as Jonathan suggested originally?) and we\n> > could do warn (and maybe fix) in a different change (maybe adding a\n> > fsck command of sorts)?\n> \n> Yeah, I think I like the \"no warning, just ignore\" much better. The\n> implementation I suspect would become a lot simpler, right?\n\nsure, it will be basically the version that Jonathan suggested[1] on\ntop of yours and that I included in v4 but that was missing a SOB.\n\nJonathan,\n\nwould you want to submit it formally?, IMHO better if with base (from pu): \n272281efcc (credential-store: document the file format a bit more, 2020-04-27)\n\nit might be still worth including some of the test cases, so I could do it\ninstead too; but let me know what would be your preference otherwise, as they\nwill need to be wrapped, probably better than on what became my confusing\nattempt to save everyone's time with the v4 series[2], and that was made\nobsolete by Junio's creation of jc/credential-store-file-format-doc.\n\nCarlo\n\n[1] https://lore.kernel.org/git/20200428052510.GA201501@google.com/\n[2] https://lore.kernel.org/git/20200428104858.28573-1-carenas@gmail.com/\n\nCC: trim to avoid spam, probably long overdue\n"},{"id":"396778","messageId":"xmqqsggjlbvx.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200501135757.GF33264@Carlos-MBP","subject":"Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-01T18:59:30Z","receivedAt":"2020-05-01T18:59:37Z","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> attempt to save everyone's time with the v4 series[2], and that was made\n> obsolete by Junio's creation of jc/credential-store-file-format-doc.\n\nI actually discarded that \"file format only\" patch after you\nincluded in your series. One fewer patch I have to keep track of is\nalways better ;-)\n\nThanks.\n"},{"id":"396836","messageId":"20200502181643.38203-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200430160642.90096-1-carenas@gmail.com","subject":"[PATCH v10] credential-store: ignore bogus lines from store file","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-02T18:16:43Z","receivedAt":"2020-05-02T18:17:33Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"With the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\nInstead of doing a hard check for credentials, do a soft one and\ntherefore avoid the reported fatal error.\n\nAs a special case, flag files with CRLF endings as invalid early\nto prevent current problems in credential_from_url_gently() with\nhandling of '\\r' in the host.\n\nWhile at it add tests for all known corruptions that are currently\nignored to keep track of them and avoid the risk of regressions.\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nHelped-by: Junio C Hamano \nBased-on-patch-by: Jonathan Nieder \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv10:\n* go back to v4 but with better testing and commit message\n* make sure broken CR characters are ignored early\nv9:\n* use strbuf_getline() instead to better handle files with CRLF\nv8:\n* only warn during get operations as otherwise line number might be\n incorrect\nv7:\n* check for protocol in helper as suggested by Junio\nv6:\n* get rid of redacter and only use line number for context in warning\n* make validation more strict to also catch incomplete credentials\n* reorder check as suggested by Junio\nv5:\n* q_to_tab this round, with a single echo to make sure empty line\n is covered, as that seems to be a popular issue\n* rebase on top of jc/credential-store-file-format-doc\n* implement a redacter for credentials to use on errors to avoid\n leaking passwords\nv4:\n* use credential_from_url_gently instead as shown by Jonathan\n* add documentation to clarify \"comments\" is not a supported feature\nv3:\n* avoid using q_to_cr as suggested by Peff\n* a more verbose commit message and slightly more complete documentation\nv2:\n* use a here-doc for clarity as suggested by Eric\n* improve commit message and include documentation\n\n credential-store.c | 5 ++--\n t/t0302-credential-store.sh | 60 ++++++++++++++++++++++++++++++++++++-\n 2 files changed, 62 insertions(+), 3 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..fdfb81e632 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -24,8 +24,9 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tcredential_from_url(&entry, line.buf);\n-\t\tif (entry.username && entry.password &&\n+\t\tif (strchr(line.buf, '\\r') == NULL &&\n+\t\t !credential_from_url_gently(&entry, line.buf, 1) &&\n+\t\t entry.username && entry.password &&\n \t\t credential_match(c, &entry)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..9fd0aca55e 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n-\n test_expect_success 'erase: erase matching credentials from both xdg and home files' '\n \techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n \tmkdir -p \"$HOME/.config/git\" &&\n@@ -120,4 +119,63 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+invalid_credential_test() {\n+\ttest_expect_success 'get: ignore credentials without $1 as invalid' '\n+\t\techo \"$2\" >\"$HOME/.git-credentials\" &&\n+\t\tcheck fill store <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=askpass-username\n+\t\tpassword=askpass-password\n+\t\t--\n+\t\taskpass: Username for '\\''https://example.com'\\'':\n+\t\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n+\t\t--\n+\t\tEOF\n+\t'\n+}\n+\n+invalid_credential_test \"scheme\" ://user:pass@example.com\n+invalid_credential_test \"valid host/path\" https://user:pass@\n+invalid_credential_test \"username/password\" https://pass@example.com\n+\n+test_expect_success 'get: credentials with DOS line endings are invalid' '\n+\tprintf \"https://user:pass@example.com\\r\\n\" >\"$HOME/.git-credentials\" &&\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\t--\n+\taskpass: Username for '\\''https://example.com'\\'':\n+\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'get: store file can contain empty/bogus lines' '\n+\techo \"\" >\"$HOME/.git-credentials\" &&\n+\tq_to_tab <<-\\CREDENTIAL >>\"$HOME/.git-credentials\" &&\n+\t#comment\n+\tQ\n+\thttps://user:pass@example.com\n+\tCREDENTIAL\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\t--\n+\tEOF\n+'\n+\n test_done\n\nbase-commit: 49458fb91d61461938881559ce23abbb1a2f8c35\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"396846","messageId":"xmqqo8r6jc8i.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200502181643.38203-1-carenas@gmail.com","subject":"Re: [PATCH v10] credential-store: ignore bogus lines from store file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-02T20:47:09Z","receivedAt":"2020-05-02T20:48:48Z","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> With the added checks for invalid URLs in credentials, any locally\n> modified store files which might have empty lines or even comments\n> were reported[1] failing to parse as valid credentials.\n>\n> Instead of doing a hard check for credentials, do a soft one and\n> therefore avoid the reported fatal error.\n>\n> As a special case, flag files with CRLF endings as invalid early\n> to prevent current problems in credential_from_url_gently() with\n> handling of '\\r' in the host.\n\nI do not think it hurts to silently ignore a line that ends with CR,\nbut only because I do not think credential_from_url_gently() would\nnot match such a line when asked to match something without\ncomplaining. \n\nIn other words, isn't the new \"!strchr() &&\" in the condition a\nno-op?\n\n> diff --git a/credential-store.c b/credential-store.c\n> index c010497cb2..fdfb81e632 100644\n> --- a/credential-store.c\n> +++ b/credential-store.c\n> @@ -24,8 +24,9 @@ static int parse_credential_file(const char *fn,\n> \t}\n> \n> \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> -\t\tcredential_from_url(&entry, line.buf);\n> -\t\tif (entry.username && entry.password &&\n> +\t\tif (strchr(line.buf, '\\r') == NULL &&\n> +\t\t !credential_from_url_gently(&entry, line.buf, 1) &&\n> +\t\t entry.username && entry.password &&\n> \t\t credential_match(c, &entry)) {\n> \t\t\tfound_credential = 1;\n> \t\t\tif (match_cb) {\n\nIn any case, among the ones we discussed, this probably has the\nleast chance of unintended regression, I would think (with or\nwithout the added \"!strchr() &&\" check), so let's queue it and\nquickly merge it down thru 'next' to 'master'.\n\n> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\n> index d6b54e8c65..9fd0aca55e 100755\n> --- a/t/t0302-credential-store.sh\n> +++ b/t/t0302-credential-store.sh\n> @@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home\n> \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n> '\n> \n> -\n> test_expect_success 'erase: erase matching credentials from both xdg and home files' '\n> \techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n> \tmkdir -p \"$HOME/.config/git\" &&\n> @@ -120,4 +119,63 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n> \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n> '\n> \n> +invalid_credential_test() {\n> +\ttest_expect_success 'get: ignore credentials without $1 as invalid' '\n> +\t\techo \"$2\" >\"$HOME/.git-credentials\" &&\n> +\t\tcheck fill store <<-\\EOF\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\t--\n> +\t\tprotocol=https\n> +\t\thost=example.com\n> +\t\tusername=askpass-username\n> +\t\tpassword=askpass-password\n> +\t\t--\n> +\t\taskpass: Username for '\\''https://example.com'\\'':\n> +\t\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n> +\t\t--\n> +\t\tEOF\n> +\t'\n> +}\n> +\n> +invalid_credential_test \"scheme\" ://user:pass@example.com\n> +invalid_credential_test \"valid host/path\" https://user:pass@\n> +invalid_credential_test \"username/password\" https://pass@example.com\n\nThese are quite clear to see. Nicely done.\n\n> +test_expect_success 'get: credentials with DOS line endings are invalid' '\n> +\tprintf \"https://user:pass@example.com\\r\\n\" >\"$HOME/.git-credentials\" &&\n> +\tcheck fill store <<-\\EOF\n> +\tprotocol=https\n> +\thost=example.com\n> +\t--\n> +\tprotocol=https\n> +\thost=example.com\n> +\tusername=askpass-username\n> +\tpassword=askpass-password\n> +\t--\n> +\taskpass: Username for '\\''https://example.com'\\'':\n> +\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n> +\t--\n> +\tEOF\n> +'\n\nWhat I am curious is if anything breaks around this test if you lost\nthe extra \"!strchr() &&\" check. I suspect that this test will pass.\n\n> +test_expect_success 'get: store file can contain empty/bogus lines' '\n> +\techo \"\" >\"$HOME/.git-credentials\" &&\n> +\tq_to_tab <<-\\CREDENTIAL >>\"$HOME/.git-credentials\" &&\n> +\t#comment\n> +\tQ\n> +\thttps://user:pass@example.com\n> +\tCREDENTIAL\n> +\tcheck fill store <<-\\EOF\n> +\tprotocol=https\n> +\thost=example.com\n> +\t--\n> +\tprotocol=https\n> +\thost=example.com\n> +\tusername=user\n> +\tpassword=pass\n> +\t--\n> +\tEOF\n> +'\n> +\n> test_done\n>\n> base-commit: 49458fb91d61461938881559ce23abbb1a2f8c35\n"},{"id":"396847","messageId":"20200502210553.GA41113@Carlos-MBP","threadId":"53326","inReplyTo":"20200502181643.38203-1-carenas@gmail.com","subject":"Re: [PATCH v10] credential-store: ignore bogus lines from store file","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-02T21:05:53Z","receivedAt":"2020-05-02T21:06:05Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Dscho,\n\nthis was tested[1] in windows and was able to reproduce the bogus issues\nwith credential store in macOS using t0302.50\n\n[1] https://github.com/carenas/git/actions/runs/93858709\n\nJunio,\n\nwill followup with a more comprehensive fix for the problems with '\\r'\nwhich will obiously need also changes in credential.c, but that IMHO might\nbe more of a prerequisite for the next iteration (the one that warns)\n\nwould be very helpful if you squash the following, otherwise\n\nCarlo\n-- >8 --\nSubject: [PATCH] SQUASH!!!\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n t/t0302-credential-store.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex 9fd0aca55e..a05b64c8b3 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -120,7 +120,7 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n '\n \n invalid_credential_test() {\n-\ttest_expect_success 'get: ignore credentials without $1 as invalid' '\n+\ttest_expect_success \"get: ignore credentials without $1 as invalid\" '\n \t\techo \"$2\" >\"$HOME/.git-credentials\" &&\n \t\tcheck fill store <<-\\EOF\n \t\tprotocol=https\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"396849","messageId":"20200502212335.GB41113@Carlos-MBP","threadId":"53326","inReplyTo":"xmqqo8r6jc8i.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v10] credential-store: ignore bogus lines from store file","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-02T21:23:35Z","receivedAt":"2020-05-02T21:23:40Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, May 02, 2020 at 01:47:09PM -0700, Junio C Hamano wrote:\n> Carlo Marcelo Arenas Belón writes:\n> \n> > With the added checks for invalid URLs in credentials, any locally\n> > modified store files which might have empty lines or even comments\n> > were reported[1] failing to parse as valid credentials.\n> >\n> > Instead of doing a hard check for credentials, do a soft one and\n> > therefore avoid the reported fatal error.\n> >\n> > As a special case, flag files with CRLF endings as invalid early\n> > to prevent current problems in credential_from_url_gently() with\n> > handling of '\\r' in the host.\n> \n> I do not think it hurts to silently ignore a line that ends with CR,\n> but only because I do not think credential_from_url_gently() would\n> not match such a line when asked to match something without\n> complaining. \n\nfor a credential like the one in the testcase (meaning no url), it will\nappend \\r to the hostname, which would cause havoc if that credential\nis printed (meaning you will end up without a host line) and be back\nin the die() in credential_apply()\n\n> In other words, isn't the new \"!strchr() &&\" in the condition a\n> no-op?\n\nyou are correct that it will be unlikely (but not imposible) to get an\nembedded CR from the other side to match, which is what I want to\naddress in the next patchset.\n\nIMHO adding the proposed early check gives us space to fix the other\nissues at our own leasure and it is meant to be gone eventually. \n\n> > diff --git a/credential-store.c b/credential-store.c\n> > index c010497cb2..fdfb81e632 100644\n> > --- a/credential-store.c\n> > +++ b/credential-store.c\n> > @@ -24,8 +24,9 @@ static int parse_credential_file(const char *fn,\n> > \t}\n> > \n> > \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n> > -\t\tcredential_from_url(&entry, line.buf);\n> > -\t\tif (entry.username && entry.password &&\n> > +\t\tif (strchr(line.buf, '\\r') == NULL &&\n> > +\t\t !credential_from_url_gently(&entry, line.buf, 1) &&\n> > +\t\t entry.username && entry.password &&\n> > \t\t credential_match(c, &entry)) {\n> > \t\t\tfound_credential = 1;\n> > \t\t\tif (match_cb) {\n> \n> In any case, among the ones we discussed, this probably has the\n> least chance of unintended regression, I would think (with or\n> without the added \"!strchr() &&\" check), so let's queue it and\n> quickly merge it down thru 'next' to 'master'.\n\nconsidering the only line that I wrote was the strchr and the other one\nwas written by Jonathan and reviewed by Peff I definitly agree.\n\ndon't forget this is also a good candidate for maint (most likely all\nthe way to maint-2.17)\n\nCarlo\n"},{"id":"396850","messageId":"20200502215348.GC41113@Carlos-MBP","threadId":"53326","inReplyTo":"xmqqo8r6jc8i.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v10] credential-store: ignore bogus lines from store file","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-02T21:53:48Z","receivedAt":"2020-05-02T21:53:56Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, May 02, 2020 at 01:47:09PM -0700, Junio C Hamano wrote:\n> \n> What I am curious is if anything breaks around this test if you lost\n> the extra \"!strchr() &&\" check. I suspect that this test will pass.\n\ncorrect\n\nand with the strchr I am introducing a regression (compared against 2.26.0)\nin cases where the '\\r' gets appended to the path and therefore gets ignored\nfor matching (unless useHttpPath is true, which is not the default)\n\nwill add 2 more test cases to cover those, and guess we are going to 11 :(\n\nCarlo\n\nPS. thanks for all your patience and reviewing\n"},{"id":"396852","messageId":"20200502223447.61478-1-carenas@gmail.com","threadId":"53326","inReplyTo":"20200502181643.38203-1-carenas@gmail.com","subject":"[PATCH v11] credential-store: ignore bogus lines from store file","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-02T22:34:47Z","receivedAt":"2020-05-02T22:38:16Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"With the added checks for invalid URLs in credentials, any locally\nmodified store files which might have empty lines or even comments\nwere reported[1] failing to parse as valid credentials.\n\nInstead of doing a hard check for credentials, do a soft one and\ntherefore avoid the reported fatal error.\n\nWhile at it add tests for all known corruptions that are currently\nignored to keep track of them and avoid the risk of regressions.\n\n[1] https://stackoverflow.com/a/61420852/5005936\n\nReported-by: Dirk \nHelped-by: Eric Sunshine \nHelped-by: Junio C Hamano \nBased-on-patch-by: Jonathan Nieder \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv11:\n* remove bogus early check for CR characters\nv10:\n* go back to v4 but with better testing and commit message\n* make sure broken CR characters are ignored early\nv9:\n* use strbuf_getline() instead to better handle files with CRLF\nv8:\n* only warn during get operations as otherwise line number might be\n incorrect\nv7:\n* check for protocol in helper as suggested by Junio\nv6:\n* get rid of redacter and only use line number for context in warning\n* make validation more strict to also catch incomplete credentials\n* reorder check as suggested by Junio\nv5:\n* q_to_tab this round, with a single echo to make sure empty line\n is covered, as that seems to be a popular issue\n* rebase on top of jc/credential-store-file-format-doc\n* implement a redacter for credentials to use on errors to avoid\n leaking passwords\nv4:\n* use credential_from_url_gently instead as shown by Jonathan\n* add documentation to clarify \"comments\" is not a supported feature\nv3:\n* avoid using q_to_cr as suggested by Peff\n* a more verbose commit message and slightly more complete documentation\nv2:\n* use a here-doc for clarity as suggested by Eric\n* improve commit message and include documentation\n\n credential-store.c | 4 +-\n t/t0302-credential-store.sh | 91 ++++++++++++++++++++++++++++++++++++-\n 2 files changed, 92 insertions(+), 3 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex c010497cb2..294e771681 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,\n \t}\n \n \twhile (strbuf_getline_lf(&line, fh) != EOF) {\n-\t\tcredential_from_url(&entry, line.buf);\n-\t\tif (entry.username && entry.password &&\n+\t\tif (!credential_from_url_gently(&entry, line.buf, 1) &&\n+\t\t entry.username && entry.password &&\n \t\t credential_match(c, &entry)) {\n \t\t\tfound_credential = 1;\n \t\t\tif (match_cb) {\ndiff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh\nindex d6b54e8c65..716bf1af9f 100755\n--- a/t/t0302-credential-store.sh\n+++ b/t/t0302-credential-store.sh\n@@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n-\n test_expect_success 'erase: erase matching credentials from both xdg and home files' '\n \techo \"https://home-user:home-pass@example.com\" >\"$HOME/.git-credentials\" &&\n \tmkdir -p \"$HOME/.config/git\" &&\n@@ -120,4 +119,94 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi\n \ttest_must_be_empty \"$HOME/.config/git/credentials\"\n '\n \n+invalid_credential_test() {\n+\ttest_expect_success \"get: ignore credentials without $1 as invalid\" '\n+\t\techo \"$2\" >\"$HOME/.git-credentials\" &&\n+\t\tcheck fill store <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=example.com\n+\t\tusername=askpass-username\n+\t\tpassword=askpass-password\n+\t\t--\n+\t\taskpass: Username for '\\''https://example.com'\\'':\n+\t\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n+\t\t--\n+\t\tEOF\n+\t'\n+}\n+\n+invalid_credential_test \"scheme\" ://user:pass@example.com\n+invalid_credential_test \"valid host/path\" https://user:pass@\n+invalid_credential_test \"username/password\" https://pass@example.com\n+\n+test_expect_success 'get: credentials with DOS line endings are invalid' '\n+\tprintf \"https://user:pass@example.com\\r\\n\" >\"$HOME/.git-credentials\" &&\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\t--\n+\taskpass: Username for '\\''https://example.com'\\'':\n+\taskpass: Password for '\\''https://askpass-username@example.com'\\'':\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'get: credentials with path and DOS line endings are valid' '\n+\tprintf \"https://user:pass@example.com/repo.git\\r\\n\" >\"$HOME/.git-credentials\" &&\n+\tcheck fill store <<-\\EOF\n+\turl=https://example.com/repo.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' '\n+\tprintf \"https://user:pass@example.com/repo.git\\r\\n\" >\"$HOME/.git-credentials\" &&\n+\ttest_config credential.useHttpPath true &&\n+\tcheck fill store <<-\\EOF\n+\turl=https://example.com/repo.git\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tpath=repo.git\n+\tusername=askpass-username\n+\tpassword=askpass-password\n+\t--\n+\taskpass: Username for '\\''https://example.com/repo.git'\\'':\n+\taskpass: Password for '\\''https://askpass-username@example.com/repo.git'\\'':\n+\t--\n+\tEOF\n+'\n+\n+test_expect_success 'get: store file can contain empty/bogus lines' '\n+\techo \"\" >\"$HOME/.git-credentials\" &&\n+\tq_to_tab <<-\\CREDENTIAL >>\"$HOME/.git-credentials\" &&\n+\t#comment\n+\tQ\n+\thttps://user:pass@example.com\n+\tCREDENTIAL\n+\tcheck fill store <<-\\EOF\n+\tprotocol=https\n+\thost=example.com\n+\t--\n+\tprotocol=https\n+\thost=example.com\n+\tusername=user\n+\tpassword=pass\n+\t--\n+\tEOF\n+'\n+\n test_done\n\nbase-commit: 49458fb91d61461938881559ce23abbb1a2f8c35\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"396853","messageId":"xmqqk11tkft4.fsf@gitster.c.googlers.com","threadId":"53326","inReplyTo":"20200502215348.GC41113@Carlos-MBP","subject":"Re: [PATCH v10] credential-store: ignore bogus lines from store file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-03T00:44:39Z","receivedAt":"2020-05-03T00:44:44Z","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> and with the strchr I am introducing a regression (compared against 2.26.0)\n> in cases where the '\\r' gets appended to the path and therefore gets ignored\n> for matching (unless useHttpPath is true, which is not the default)\n>\n> will add 2 more test cases to cover those, and guess we are going to 11 :(\n>\n> Carlo\n>\n> PS. thanks for all your patience and reviewing\n\nAmong those 11, I also am responsible for a few while we ere\npursuing the approach to warn X-<.\n\nThanks for _your_ patience.\n\n"},{"id":"396867","messageId":"20200503100627.GA171296@coredump.intra.peff.net","threadId":"53326","inReplyTo":"xmqqo8r6jc8i.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v10] credential-store: ignore bogus lines from store file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-03T10:06:27Z","receivedAt":"2020-05-03T10:06:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 02, 2020 at 01:47:09PM -0700, Junio C Hamano wrote:\n\n> > As a special case, flag files with CRLF endings as invalid early\n> > to prevent current problems in credential_from_url_gently() with\n> > handling of '\\r' in the host.\n> \n> I do not think it hurts to silently ignore a line that ends with CR,\n> but only because I do not think credential_from_url_gently() would\n> not match such a line when asked to match something without\n> complaining.\n\nI wondered if we might hit a case where the CR ends up in the path,\nlike:\n\n $ printf 'https://user:pass@example.com/\\r\\n' >creds\n $ echo url=https://example.com/ |\n git credential-store --file=creds get\n username=user\n password=pass\n\nbecause the path is parsed as missing in the incoming pattern (and thus\nwe match any path, even \"\\r\").\n\nBut credential-store would never write such a path in the first place.\nEven with the trailing slash on an incoming URL, it will write:\n\n https://example.com\n\nwithout a slash at all (and thus any inserted CR would be part of the\nhostname). So somebody would have to have inserted it themselves, or\nhave turned useHTTPPath on (in which case we _would_ complain on the\nmatching side, too, because we'd try matching the path with a CR in it).\n\nI think it's reasonable to assume that any CR would have been a problem\neven in older versions.\n\n-Peff\n"}]}