[PATCH] git-credential-store: skip empty lines and comments from store

[PATCH] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

with the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported as failing[1] to parse as valid credentials.

instead of passing every line to the matcher as read, trim them
from spaces and skip the ones that will be otherwise empty or
start with "#" (assumed to be comments)

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 credential-store.c          |  3 +++
 t/t0302-credential-store.sh | 17 +++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/credential-store.c b/credential-store.c
index c010497cb2..b2f160890d 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
+		strbuf_trim(&line);
+		if (line.len == 0 || *line.buf == '#')
+			continue;
 		credential_from_url(&entry, line.buf);
 		if (entry.username && entry.password &&
 		    credential_match(c, &entry)) {
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..7245d2f449 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,21 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'get: allow for empty lines or comments in store file' '
+	echo "#this is a comment" >"$HOME/.git-credentials" &&
+	echo "" >>"$HOME/.git-credentials" &&
+	echo "https://user:pass@example.com" >>"$HOME/.git-credentials" &&
+	echo "    " >>"$HOME/.git-credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	--
+	EOF
+'
+
 test_done
-- 
2.26.2.569.g1d74ac4d14

Re: [PATCH] git-credential-store: skip empty lines and comments from store

[Eric Sunshine]

[Cc:+Dirk -- so he knows his bug report[1] wasn't ignored]

On Sun, Apr 26, 2020 at 7:48 PM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> with the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported as failing[1] to parse as valid credentials.
>
> instead of passing every line to the matcher as read, trim them
> from spaces and skip the ones that will be otherwise empty or
> start with "#" (assumed to be comments)
>
> Reported-by: Dirk <dirk@ed4u.de>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> @@ -120,4 +120,21 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
> +test_expect_success 'get: allow for empty lines or comments in store file' '
> +       echo "#this is a comment" >"$HOME/.git-credentials" &&
> +       echo "" >>"$HOME/.git-credentials" &&
> +       echo "https://user:pass@example.com" >>"$HOME/.git-credentials" &&
> +       echo "    " >>"$HOME/.git-credentials" &&

Is there a reason you don't use a here-doc for the above (which would
be less noisy)? For instance:

    q_to_tab >"$HOME/.git-credentials" <<-\EOF &&
    #this is a comment

    https://user:pass@example.com
    Q
    EOF

[1]: https://lore.kernel.org/git/ad80aa0d-3a35-6d7e-7958-b3520e16c855@ed4u.de/

Re: [PATCH] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

On Sun, Apr 26, 2020 at 08:19:35PM -0400, Eric Sunshine wrote:
> [Cc:+Dirk -- so he knows his bug report[1] wasn't ignored]
> On Sun, Apr 26, 2020 at 7:48 PM Carlo Marcelo Arenas Belón
> <carenas@gmail.com> wrote:
> > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> > @@ -120,4 +120,21 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
> > +test_expect_success 'get: allow for empty lines or comments in store file' '
> > +       echo "#this is a comment" >"$HOME/.git-credentials" &&
> > +       echo "" >>"$HOME/.git-credentials" &&
> > +       echo "https://user:pass@example.com" >>"$HOME/.git-credentials" &&
> > +       echo "    " >>"$HOME/.git-credentials" &&
> 
> Is there a reason you don't use a here-doc for the above (which would
> be less noisy)? For instance:

because I didn't knew it existed ;), thanks for your help and will include
for next version.

Carlo

[PATCH v2] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

with the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported failing[1] to parse as valid credentials.

instead of passing every line to the matcher for processing, trim
them from spaces and skip the ones that will be otherwise empty or
that start with "#" (assumed to be comments)

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 Documentation/git-credential-store.txt |  7 +++++++
 credential-store.c                     |  3 +++
 t/t0302-credential-store.sh            | 19 +++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 693dd9d9d7..7f7b53e4da 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -101,6 +101,13 @@ username (if we already have one) match, then the password is returned
 to Git. See the discussion of configuration in linkgit:gitcredentials[7]
 for more information.
 
+Note that the file used is not a configuration file and should be ideally
+managed only through git, as any manually introduced typos will compromise
+the validation of credentials.
+
+The parser will ignore any lines starting with the '#' character during
+the processing of credentials, though.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..b2f160890d 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
+		strbuf_trim(&line);
+		if (line.len == 0 || *line.buf == '#')
+			continue;
 		credential_from_url(&entry, line.buf);
 		if (entry.username && entry.password &&
 		    credential_match(c, &entry)) {
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..0d13318255 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,23 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'get: allow for empty lines or comments in store file' '
+	q_to_cr >"$HOME/.git-credentials" <<-\EOF &&
+	#this is a comment and the next line contains leading spaces
+	    Q
+	https://user:pass@example.com
+	Q
+	EOF
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	--
+	EOF
+'
+
 test_done
-- 
2.26.2.569.g1d74ac4d14

Re: [PATCH v2] git-credential-store: skip empty lines and comments from store

[Jeff King]

On Mon, Apr 27, 2020 at 01:42:35AM -0700, Carlo Marcelo Arenas Belón wrote:

> with the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported failing[1] to parse as valid credentials.

Those were never supposed to work. I'm mildly surprised that they did.
It looks like the username/password check here is what prevented us from
matching an empty line against a very broad pattern:

          while (strbuf_getline_lf(&line, fh) != EOF) {
                  credential_from_url(&entry, line.buf);
                  if (entry.username && entry.password &&
                      credential_match(c, &entry)) {
                          found_credential = 1;

And there was no such thing as a comment character. E.g., a
"commented-out" url like this:

  $ echo "#https://user:pass@example.com/" >creds

would still be matched:

  $ echo host=example.com | git credential-store --file creds get
  host=example.com
  username=user
  password=pass

I guess I'm not really opposed to adding this as a feature, but I think
the justification should be "because it is somehow useful" and not
because it's a bugfix.

> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index d6b54e8c65..0d13318255 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -120,4 +120,23 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
>  	test_must_be_empty "$HOME/.config/git/credentials"
>  '
>  
> +test_expect_success 'get: allow for empty lines or comments in store file' '
> +	q_to_cr >"$HOME/.git-credentials" <<-\EOF &&
> +	#this is a comment and the next line contains leading spaces
> +	    Q
> +	https://user:pass@example.com
> +	Q
> +	EOF

q_to_cr is a little weird here, as we wouldn't expect there to be CRs in
the file. They do get removed by strbuf_trim(), even in non-comment
lines. But perhaps "sed s/Q//" would accomplish the same thing (making
the whitespace more visible) without making anyone wonder whether the CR
is an important part of the test?

-Peff

Re: [PATCH v2] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

On Mon, Apr 27, 2020 at 07:52:23AM -0400, Jeff King wrote:
> On Mon, Apr 27, 2020 at 01:42:35AM -0700, Carlo Marcelo Arenas Belón wrote:
> 
> > with the added checks for invalid URLs in credentials, any locally
> > modified store files which might have empty lines or even comments
> > were reported failing[1] to parse as valid credentials.
> 
> Those were never supposed to work. I'm mildly surprised that they did.

agree on that, which is why I have to admit I couldn't find the right
wording for the previous paragraph, and also why I didn't pinpoint a
specific commit as the one that introduced a bug.

it was instead just meant to describe a "current state of affairs" and
I was hoping the documentation entry will help guide future users to be
more careful with this file.

> I guess I'm not really opposed to adding this as a feature, but I think
> the justification should be "because it is somehow useful" and not
> because it's a bugfix.

I think that is a matter of semantics, because for some users it seems
like a regression on the last bugfix and therefore might be used (albeit
IMHO incorrectly) as an excuse not to upgrade, which will be even worst.

so yes, I am implementint this "feature" to prevent them to have an
excuse but it also means we would most likely need to fastrack this
into maint.

> > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> > index d6b54e8c65..0d13318255 100755
> > --- a/t/t0302-credential-store.sh
> > +++ b/t/t0302-credential-store.sh
> > @@ -120,4 +120,23 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
> >  	test_must_be_empty "$HOME/.config/git/credentials"
> >  '
> >  
> > +test_expect_success 'get: allow for empty lines or comments in store file' '
> > +	q_to_cr >"$HOME/.git-credentials" <<-\EOF &&
> > +	#this is a comment and the next line contains leading spaces
> > +	    Q
> > +	https://user:pass@example.com
> > +	Q
> > +	EOF
> 
> q_to_cr is a little weird here, as we wouldn't expect there to be CRs in
> the file. They do get removed by strbuf_trim(), even in non-comment
> lines. But perhaps "sed s/Q//" would accomplish the same thing (making
> the whitespace more visible) without making anyone wonder whether the CR
> is an important part of the test?

I know, but commiting a line with 1 tab and 4 empty spaces instead of
using Q seemed even worst and q_to_cr almost fits the bill and might
even make this test "windows compatible" for once ;)

will rewrite then using test_write_lines and I hope it is still as
readable without having to resort to the originl ugly chain of echo

Carlo

Re: [PATCH v2] git-credential-store: skip empty lines and comments from store

[Eric Sunshine]

On Mon, Apr 27, 2020 at 8:25 AM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> On Mon, Apr 27, 2020 at 07:52:23AM -0400, Jeff King wrote:
> > On Mon, Apr 27, 2020 at 01:42:35AM -0700, Carlo Marcelo Arenas Belón wrote:
> > > +test_expect_success 'get: allow for empty lines or comments in store file' '
> > > +   q_to_cr >"$HOME/.git-credentials" <<-\EOF &&
> > > +   #this is a comment and the next line contains leading spaces
> > > +       Q
> > > +   https://user:pass@example.com
> > > +   Q
> > > +   EOF
> >
> > q_to_cr is a little weird here, as we wouldn't expect there to be CRs in
> > the file. They do get removed by strbuf_trim(), even in non-comment
> > lines. But perhaps "sed s/Q//" would accomplish the same thing (making
> > the whitespace more visible) without making anyone wonder whether the CR
> > is an important part of the test?
>
> I know, but commiting a line with 1 tab and 4 empty spaces instead of
> using Q seemed even worst and q_to_cr almost fits the bill and might
> even make this test "windows compatible" for once ;)

Hmm? In the proposal[1], q_to_tab() was used, not q_to_cr(), and
q_to_tab() seemed to fit quite well as a replacement for the original
'echo' chain[2] which emitted one whitespace-only line. Substituting
that whitespace-only line with "Q" (which gets translated into a TAB)
seems to fit much better than q_to_cr().

> will rewrite then using test_write_lines and I hope it is still as
> readable without having to resort to the originl ugly chain of echo

test_write_lines() doesn't do a very good job of representing -- at a
glance -- what the "expected" output should be. The q_to_tab() block
made it much  more clear.

(Having said that, it's probably not worth another re-roll just to change that.)

[1]: https://lore.kernel.org/git/CAPig+cR8HKcbNxxw65ERz0iHvnO5aC6RXvF9NjvFTySXpcHCSQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/20200426234750.40418-1-carenas@gmail.com/

Re: [PATCH v2] git-credential-store: skip empty lines and comments from store

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Those were never supposed to work. I'm mildly surprised that they did.

I know that feeling X-<.  It probably was a mistake to pretend that
the file were editable with an editor, but it is too late to fix
that.

The next complainer may say that "; comment" no longer works.  We
probably should draw a line somewhere, but I am not sure where.

Re: [PATCH v2] git-credential-store: skip empty lines and comments from store

[Jeff King]

On Mon, Apr 27, 2020 at 10:47:02AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Those were never supposed to work. I'm mildly surprised that they did.
> 
> I know that feeling X-<.  It probably was a mistake to pretend that
> the file were editable with an editor, but it is too late to fix
> that.

I guess it is somewhat my fault for documenting the format at all
git-credential-store.txt. I probably should have said "this is a black
box and is subject to change".

> The next complainer may say that "; comment" no longer works.  We
> probably should draw a line somewhere, but I am not sure where.

If we get a bug report for "credential-store does not respect
core.commentChar" I think my head would explode. :)

-Peff

[PATCH v3] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

with the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

using the store file in this manner wasn't intended by the original
code and it had latent issues which the new code dutifully prevented
but since the strings used wouldn't had been valid credentials anyway
we could instead detect them and skip the matching logic and therefore
formalize this "feature".

trim all lines as they are being read from the store file and skip the
ones that will be otherwise empty or that start with "#" (therefore
assuming them to be comments)

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v3:
* avoid using q_to_cr as suggested by Peff
* a more verbose commit message and slightly more complete documentation 
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 Documentation/git-credential-store.txt |  7 +++++++
 credential-store.c                     |  3 +++
 t/t0302-credential-store.sh            | 15 +++++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 693dd9d9d7..48ab4b13e5 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -101,6 +101,13 @@ username (if we already have one) match, then the password is returned
 to Git. See the discussion of configuration in linkgit:gitcredentials[7]
 for more information.
 
+Note that the file used is not a configuration file and should be ideally
+managed only through git, as any manually introduced typos will compromise
+the validation of credentials.
+
+The parser will ignore any lines starting with the '#' character during
+the processing of credentials for read, though.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..b2f160890d 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
+		strbuf_trim(&line);
+		if (line.len == 0 || *line.buf == '#')
+			continue;
 		credential_from_url(&entry, line.buf);
 		if (entry.username && entry.password &&
 		    credential_match(c, &entry)) {
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..5e6ace3a06 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'get: allow for empty lines or comments in store file' '
+	test_write_lines "#comment" " " "" \
+		 https://user:pass@example.com >"$HOME/.git-credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	--
+	EOF
+'
+
 test_done
-- 
2.26.2.569.g1d74ac4d14

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Philip Oakley]

Hi Carlo,
On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote:
> with the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported[1] failing to parse as valid credentials.
>
> using the store file in this manner wasn't intended by the original
> code and it had latent issues which the new code dutifully prevented
> but since the strings used wouldn't had been valid credentials anyway
> we could instead detect them and skip the matching logic and therefore
> formalize this "feature".
>
> trim all lines as they are being read from the store file and skip the

Does trimming affect any credentials that may have spaces either end?
I've not looked at the format.
> ones that will be otherwise empty or that start with "#" (therefore
> assuming them to be comments)
>
> [1] https://stackoverflow.com/a/61420852/5005936
>
> Reported-by: Dirk <dirk@ed4u.de>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> v3:
> * avoid using q_to_cr as suggested by Peff
> * a more verbose commit message and slightly more complete documentation 
> v2:
> * use a here-doc for clarity as suggested by Eric
> * improve commit message and include documentation
>
>  Documentation/git-credential-store.txt |  7 +++++++
>  credential-store.c                     |  3 +++
>  t/t0302-credential-store.sh            | 15 +++++++++++++++
>  3 files changed, 25 insertions(+)
>
> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> index 693dd9d9d7..48ab4b13e5 100644
> --- a/Documentation/git-credential-store.txt
> +++ b/Documentation/git-credential-store.txt
> @@ -101,6 +101,13 @@ username (if we already have one) match, then the password is returned
>  to Git. See the discussion of configuration in linkgit:gitcredentials[7]
>  for more information.
>  
> +Note that the file used is not a configuration file and should be ideally
> +managed only through git, as any manually introduced typos will compromise
> +the validation of credentials.
> +
> +The parser will ignore any lines starting with the '#' character during
> +the processing of credentials for read, though.
Should the trimming of leading/trailing spaces be mentioned?

Also the git-credential page mentions that the credential must end with
a blank line ("don't forget.."). Should that be mentioned here, or have
I misunderstood?
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> diff --git a/credential-store.c b/credential-store.c
> index c010497cb2..b2f160890d 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn,
>  	}
>  
>  	while (strbuf_getline_lf(&line, fh) != EOF) {
> +		strbuf_trim(&line);
> +		if (line.len == 0 || *line.buf == '#')
> +			continue;
>  		credential_from_url(&entry, line.buf);
>  		if (entry.username && entry.password &&
>  		    credential_match(c, &entry)) {
> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index d6b54e8c65..5e6ace3a06 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
>  	test_must_be_empty "$HOME/.config/git/credentials"
>  '
>  
> +test_expect_success 'get: allow for empty lines or comments in store file' '
> +	test_write_lines "#comment" " " "" \
> +		 https://user:pass@example.com >"$HOME/.git-credentials" &&
> +	check fill store <<-\EOF
> +	protocol=https
> +	host=example.com
> +	--
> +	protocol=https
> +	host=example.com
> +	username=user
> +	password=pass
> +	--
> +	EOF
> +'
> +
>  test_done
Philip

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

On Mon, Apr 27, 2020 at 02:48:05PM +0100, Philip Oakley wrote:
> On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote:
> > with the added checks for invalid URLs in credentials, any locally
> > modified store files which might have empty lines or even comments
> > were reported[1] failing to parse as valid credentials.
> >
> > using the store file in this manner wasn't intended by the original
> > code and it had latent issues which the new code dutifully prevented
> > but since the strings used wouldn't had been valid credentials anyway
> > we could instead detect them and skip the matching logic and therefore
> > formalize this "feature".
> >
> > trim all lines as they are being read from the store file and skip the
> 
> Does trimming affect any credentials that may have spaces either end?

all credentials are url encoded so the only spaces that are affected are
the ones that were added by careless editing of that file.

as Eric pointed out, any tabs will be silently "cleaned" as well.

> Should the trimming of leading/trailing spaces be mentioned?

I wanted to keep that as an undocumented "feature" as it is just meant
to help people to avoid a fatal error during the transition but didn't
want to encourage people thinking it is a supported part or even worse
encourage people to start editing their files to add tabs and spaces.

Junio's suggested documentation fix[1] makes that clearer that I could

> Also the git-credential page mentions that the credential must end with
> a blank line ("don't forget.."). Should that be mentioned here, or have
> I misunderstood?

that is for the protocol part of it (which is used between git and the
credential helper), the file format for this specific helper doesn't
require that.

Carlo

[1] https://lore.kernel.org/git/xmqqv9lk7j7p.fsf@gitster.c.googlers.com/

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Philip Oakley]

Hi Carlo,
Thanks for the clarification.
Philip

On 28/04/2020 02:49, Carlo Marcelo Arenas Belón wrote:
> On Mon, Apr 27, 2020 at 02:48:05PM +0100, Philip Oakley wrote:
>> On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote:
>>> with the added checks for invalid URLs in credentials, any locally
>>> modified store files which might have empty lines or even comments
>>> were reported[1] failing to parse as valid credentials.
>>>
>>> using the store file in this manner wasn't intended by the original
>>> code and it had latent issues which the new code dutifully prevented
>>> but since the strings used wouldn't had been valid credentials anyway
>>> we could instead detect them and skip the matching logic and therefore
>>> formalize this "feature".
>>>
>>> trim all lines as they are being read from the store file and skip the
>> Does trimming affect any credentials that may have spaces either end?
> all credentials are url encoded so the only spaces that are affected are
> the ones that were added by careless editing of that file.
>
> as Eric pointed out, any tabs will be silently "cleaned" as well.
>
>> Should the trimming of leading/trailing spaces be mentioned?
> I wanted to keep that as an undocumented "feature" as it is just meant
> to help people to avoid a fatal error during the transition but didn't
> want to encourage people thinking it is a supported part or even worse
> encourage people to start editing their files to add tabs and spaces.
>
> Junio's suggested documentation fix[1] makes that clearer that I could
>
>> Also the git-credential page mentions that the credential must end with
>> a blank line ("don't forget.."). Should that be mentioned here, or have
>> I misunderstood?
> that is for the protocol part of it (which is used between git and the
> credential helper), the file format for this specific helper doesn't
> require that.
>
> Carlo
>
> [1] https://lore.kernel.org/git/xmqqv9lk7j7p.fsf@gitster.c.googlers.com/

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Dirk]

Thank you very much. That's correct and ideal in my eyes.

Regarding 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...

Anyway. Thank you all.

Dirk


Am 27.04.20 um 14:59 schrieb Carlo Marcelo Arenas Belón:
> with the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported[1] failing to parse as valid credentials.
>
> using the store file in this manner wasn't intended by the original
> code and it had latent issues which the new code dutifully prevented
> but since the strings used wouldn't had been valid credentials anyway
> we could instead detect them and skip the matching logic and therefore
> formalize this "feature".
>
> trim all lines as they are being read from the store file and skip the
> ones that will be otherwise empty or that start with "#" (therefore
> assuming them to be comments)
>
> [1] https://stackoverflow.com/a/61420852/5005936
>
> Reported-by: Dirk <dirk@ed4u.de>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> v3:
> * avoid using q_to_cr as suggested by Peff
> * a more verbose commit message and slightly more complete documentation 
> v2:
> * use a here-doc for clarity as suggested by Eric
> * improve commit message and include documentation
>
>  Documentation/git-credential-store.txt |  7 +++++++
>  credential-store.c                     |  3 +++
>  t/t0302-credential-store.sh            | 15 +++++++++++++++
>  3 files changed, 25 insertions(+)
>
> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> index 693dd9d9d7..48ab4b13e5 100644
> --- a/Documentation/git-credential-store.txt
> +++ b/Documentation/git-credential-store.txt
> @@ -101,6 +101,13 @@ username (if we already have one) match, then the password is returned
>  to Git. See the discussion of configuration in linkgit:gitcredentials[7]
>  for more information.
>  
> +Note that the file used is not a configuration file and should be ideally
> +managed only through git, as any manually introduced typos will compromise
> +the validation of credentials.
> +
> +The parser will ignore any lines starting with the '#' character during
> +the processing of credentials for read, though.
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> diff --git a/credential-store.c b/credential-store.c
> index c010497cb2..b2f160890d 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn,
>  	}
>  
>  	while (strbuf_getline_lf(&line, fh) != EOF) {
> +		strbuf_trim(&line);
> +		if (line.len == 0 || *line.buf == '#')
> +			continue;
>  		credential_from_url(&entry, line.buf);
>  		if (entry.username && entry.password &&
>  		    credential_match(c, &entry)) {
> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index d6b54e8c65..5e6ace3a06 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
>  	test_must_be_empty "$HOME/.config/git/credentials"
>  '
>  
> +test_expect_success 'get: allow for empty lines or comments in store file' '
> +	test_write_lines "#comment" " " "" \
> +		 https://user:pass@example.com >"$HOME/.git-credentials" &&
> +	check fill store <<-\EOF
> +	protocol=https
> +	host=example.com
> +	--
> +	protocol=https
> +	host=example.com
> +	username=user
> +	password=pass
> +	--
> +	EOF
> +'
> +
>  test_done

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> with the added checks for invalid URLs in credentials, any locally

s/with/With/

> modified store files which might have empty lines or even comments
> were reported[1] failing to parse as valid credentials.

These files are not supposed to be viewed or edited without the help
of the credential helpers.  Do these blank lines and comments even
survive when a new credential is approved, or do we just overwrite
and lose them?

I'd rather not to do either, if we did not have to, but if it were
necessary for us to do something, I am OK to ignore empty lines.
But I'd prefer not to mix the new "# comment" feature in, if we did
not have to.

Also, triming the lines that are not empty is unwarranted.  IIUC,
what the "store" action writes encodes whitespaces, so as soon as
you see whitespace on either end, (or anywhere on the line for that
matter), it is a hand-edited cruft in the file.  If you ignore
comments, you probably should ignore those lines, too.

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Jeff King]

On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote:

> > modified store files which might have empty lines or even comments
> > were reported[1] failing to parse as valid credentials.
> 
> These files are not supposed to be viewed or edited without the help
> of the credential helpers.  Do these blank lines and comments even
> survive when a new credential is approved, or do we just overwrite
> and lose them?

That's a good question. In the older code we do save them, because
credential-store passes through lines which don't match the credential
we're operating on.

But in Carlo's patch, the immediate "continue" means we wouldn't ever
call "other_cb", which is what does that pass-through.

> I'd rather not to do either, if we did not have to, but if it were
> necessary for us to do something, I am OK to ignore empty lines.
> But I'd prefer not to mix the new "# comment" feature in, if we did
> not have to.
> 
> Also, triming the lines that are not empty is unwarranted.  IIUC,
> what the "store" action writes encodes whitespaces, so as soon as
> you see whitespace on either end, (or anywhere on the line for that
> matter), it is a hand-edited cruft in the file.  If you ignore
> comments, you probably should ignore those lines, too.

Yeah, all of that seems quite sensible.

-Peff

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote:
>
>> > modified store files which might have empty lines or even comments
>> > were reported[1] failing to parse as valid credentials.
>> 
>> These files are not supposed to be viewed or edited without the help
>> of the credential helpers.  Do these blank lines and comments even
>> survive when a new credential is approved, or do we just overwrite
>> and lose them?
>
> That's a good question. In the older code we do save them, because
> credential-store passes through lines which don't match the credential
> we're operating on.
>
> But in Carlo's patch, the immediate "continue" means we wouldn't ever
> call "other_cb", which is what does that pass-through.

So, does that mean the patch that started this thread will still help
users who wrote custom comments and blank lines in their credential
store by letting git-credential-store start up, but leaves a ticking
bomb for them to lose these precious comments and blanks once they
add a new site, change password, etc., at which point the user realizes
that comments and blanks are not supported after all?

>> I'd rather not to do either, if we did not have to, but if it were
>> necessary for us to do something, I am OK to ignore empty lines.
>> But I'd prefer not to mix the new "# comment" feature in, if we did
>> not have to.
>> 
>> Also, triming the lines that are not empty is unwarranted.  IIUC,
>> what the "store" action writes encodes whitespaces, so as soon as
>> you see whitespace on either end, (or anywhere on the line for that
>> matter), it is a hand-edited cruft in the file.  If you ignore
>> comments, you probably should ignore those lines, too.
>
> Yeah, all of that seems quite sensible.

I think the first patch we need is a (belated) documentation patch,
that adds to the existing "STORAGE FORMAT".  We already say "Each
credential is stored on its own line as a URL", but we do not say
anything about allowing other cruft in the file.  We probably
should.  Adding a "comment" feature, if anybody feels like it, is OK
and we can loosen the paragraph when that happens.

-- >8 --
Subject: credential-store: document the file format a bit more

Reading a malformed credential URL line and silently ignoring it
does not mean that we promise to torelate and/or keep empty lines
and "# commented" lines forever.

Some people seem to take anything that is not explicitly forbidden
as allowed, but the world does not work that way.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-credential-store.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 693dd9d9d7..76b0798856 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -94,6 +94,10 @@ stored on its own line as a URL like:
 https://user:pass@example.com
 ------------------------------
 
+No other kinds of lines (e.g. empty lines or comment lines) are
+allowed in the file, even though some may be silently ignored. Do
+not view or edit the file with editors.
+
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
 each entry in the credentials file.  If the protocol, hostname, and

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Jeff King]

On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote:

> >> These files are not supposed to be viewed or edited without the help
> >> of the credential helpers.  Do these blank lines and comments even
> >> survive when a new credential is approved, or do we just overwrite
> >> and lose them?
> >
> > That's a good question. In the older code we do save them, because
> > credential-store passes through lines which don't match the credential
> > we're operating on.
> >
> > But in Carlo's patch, the immediate "continue" means we wouldn't ever
> > call "other_cb", which is what does that pass-through.
> 
> So, does that mean the patch that started this thread will still help
> users who wrote custom comments and blank lines in their credential
> store by letting git-credential-store start up, but leaves a ticking
> bomb for them to lose these precious comments and blanks once they
> add a new site, change password, etc., at which point the user realizes
> that comments and blanks are not supported after all?

Yes, exactly. If I start with:

  cat >creds <<\EOF
  # this is a comment
  https://user:pass@example.com/
  EOF

then:

  echo url=https://other:pass@other.example.com |
  git credential-store --file=creds store

with v2.26.0 results in:

  https://other:pass@other.example.com
  # this is a comment
  https://user:pass@example.com

but applying the patch on top:

  https://other:pass@other.example.com
  https://user:pass@example.com/

That raises another issue about comments, too: we make no promises about
where we write new entries. They could be inserted between a comment
line and one it is meant to annotate (that line could even be moved if
we reject and then re-approve a credential).

> I think the first patch we need is a (belated) documentation patch,
> that adds to the existing "STORAGE FORMAT".  We already say "Each
> credential is stored on its own line as a URL", but we do not say
> anything about allowing other cruft in the file.  We probably
> should.  Adding a "comment" feature, if anybody feels like it, is OK
> and we can loosen the paragraph when that happens.

The more I look into it, the more negative I am on adding such a comment
feature.

> -- >8 --
> Subject: credential-store: document the file format a bit more

Yeah, this certainly clarifies things. I suppose one could ask why we'd
bother documenting the format at all, then, though I do think it could
be helpful for debugging.

> Reading a malformed credential URL line and silently ignoring it
> does not mean that we promise to torelate and/or keep empty lines
> and "# commented" lines forever.

s/torelate/tolerate/

-Peff

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

On Mon, Apr 27, 2020 at 05:10:13PM -0400, Jeff King wrote:
> On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote:
> > >> These files are not supposed to be viewed or edited without the help
> > >> of the credential helpers.  Do these blank lines and comments even
> > >> survive when a new credential is approved, or do we just overwrite
> > >> and lose them?
> > >
> > > That's a good question. In the older code we do save them, because
> > > credential-store passes through lines which don't match the credential
> > > we're operating on.
> > >
> > > But in Carlo's patch, the immediate "continue" means we wouldn't ever
> > > call "other_cb", which is what does that pass-through.
> > 
> > So, does that mean the patch that started this thread will still help
> > users who wrote custom comments and blank lines in their credential
> > store by letting git-credential-store start up, but leaves a ticking
> > bomb for them to lose these precious comments and blanks once they
> > add a new site, change password, etc., at which point the user realizes
> > that comments and blanks are not supported after all?
> 
> The more I look into it, the more negative I am on adding such a comment
> feature.

FWIW I am not interested on implementing a comment feature (regardless of
what the subject says, and indeed was going to use quotes around it in my
original patch), but it was already too long ;)

my objective was to allow people that were not upgrading because of this
"regression" a path forward, but after all this discussion I am convinced
too that my approach is misguided.

if users are already manually editing this file, they should be able to
fix the corrupted lines themselves but the error shown needs to be improved.
and the failure mode corrected as well so they are not blocked.

this also means that my "sneaky" way to fix formatting issues is gone
and the passthrough is called so nothing will be lost, but as you also
said the location is not warranted to be kept so this comment "feature"
is as useless as intended.

would something like the following (wouldn't pass the last test yet as
it will need special handling to be able to handle the full path of the
broken file as shown in the warning sent to stderr) on top of Junio's
make more sense?

it still "silently" fixes the formatting issues where a leading tab (test
added) or spaces were added to easy the transition, but probably should
also error out as Junio suggested originally.

had also updated the commit message and what is left of the documentation.

Carlo
-- >8 --
Subject: [RFC PATCH v4] git-credential-store: skip empty lines and comments
 from store

with the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

using the store file in this manner wasn't intended by the original
code and it had latent issues which the new code dutifully prevented
but since the strings used wouldn't had been valid credentials anyway
we could instead detect them and skip the matching logic and therefore
prevent a fatal failure.

trim all lines as they are being read from the store file and skip the
ones that will be otherwise empty or that start with "#" (therefore
assuming them to be comments) but warn users so they will be make
aware of the issues introduced by their manual editing.

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 Documentation/git-credential-store.txt |  4 ++++
 credential-store.c                     |  7 ++++++
 t/t0302-credential-store.sh            | 31 ++++++++++++++++++++++++++
 3 files changed, 42 insertions(+)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 693dd9d9d7..e9a1993021 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -101,6 +101,10 @@ username (if we already have one) match, then the password is returned
 to Git. See the discussion of configuration in linkgit:gitcredentials[7]
 for more information.
 
+The parser will ignore any lines that are empty or starting with '#' character
+during the processing of credentials for read, but warn the user so they can
+be corrected.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..f1f016fec1 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -24,6 +24,13 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
+		strbuf_trim(&line);
+		if (line.len == 0 || *line.buf == '#') {
+			warning("ignoring corrupted credential entry \"%s\" in %s", line.buf, fn);
+			if (other_cb)
+				other_cb(&line);
+			continue;
+		}
 		credential_from_url(&entry, line.buf);
 		if (entry.username && entry.password &&
 		    credential_match(c, &entry)) {
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..12a158c136 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,35 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'get: attempt to workaround formatting isssues (ex: tab)' '
+	q_to_tab >"$HOME/.git-credentials" <<-\EOF &&
+	Qhttps://user:pass@example.com
+	EOF
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	--
+	EOF
+'
+
+test_expect_success 'get: skip empty lines or "comments" in store file' '
+	test_write_lines "#comment" " " "" \
+		 https://user:pass@example.com >"$HOME/.git-credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	--
+	EOF
+'
+
 test_done
-- 
2.26.2.569.g1d74ac4d14

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote:
> >
> >> > modified store files which might have empty lines or even comments
> >> > were reported[1] failing to parse as valid credentials.
> >> 
> >> These files are not supposed to be viewed or edited without the help
> >> of the credential helpers.  Do these blank lines and comments even
> >> survive when a new credential is approved, or do we just overwrite
> >> and lose them?
> >
> > That's a good question. In the older code we do save them, because
> > credential-store passes through lines which don't match the credential
> > we're operating on.
> >
> > But in Carlo's patch, the immediate "continue" means we wouldn't ever
> > call "other_cb", which is what does that pass-through.
> 
> So, does that mean the patch that started this thread will still help
> users who wrote custom comments and blank lines in their credential
> store by letting git-credential-store start up, but leaves a ticking
> bomb for them to lose these precious comments and blanks once they
> add a new site, change password, etc., at which point the user realizes
> that comments and blanks are not supported after all?

yes, and it also helps users that might have added spaces or tabs
around their credentials while editing them to still be able to use those
instead of just failing to match them.

IMHO the only "regression" I was fixing was the fact that the current
code will get to throw a fatal error with an unhelpful message and prevent
access to valid credentials as shown by :

$ /git credential fill
protocol=http
host=example.com

warning: url has no scheme:
fatal: credential url cannot be parsed:
Username for 'http://example.com':

> >> I'd rather not to do either, if we did not have to, but if it were
> >> necessary for us to do something, I am OK to ignore empty lines.
> >> But I'd prefer not to mix the new "# comment" feature in, if we did
> >> not have to.
> >> 
> >> Also, triming the lines that are not empty is unwarranted.  IIUC,
> >> what the "store" action writes encodes whitespaces, so as soon as
> >> you see whitespace on either end, (or anywhere on the line for that
> >> matter), it is a hand-edited cruft in the file.  If you ignore
> >> comments, you probably should ignore those lines, too.
> >
> > Yeah, all of that seems quite sensible.
> 
> I think the first patch we need is a (belated) documentation patch,
> that adds to the existing "STORAGE FORMAT".  We already say "Each
> credential is stored on its own line as a URL", but we do not say
> anything about allowing other cruft in the file.  We probably
> should.  Adding a "comment" feature, if anybody feels like it, is OK
> and we can loosen the paragraph when that happens.
> 
> -- >8 --
> Subject: credential-store: document the file format a bit more
> 
> Reading a malformed credential URL line and silently ignoring it
> does not mean that we promise to torelate and/or keep empty lines
> and "# commented" lines forever.
> 
> Some people seem to take anything that is not explicitly forbidden
> as allowed, but the world does not work that way.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git-credential-store.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> index 693dd9d9d7..76b0798856 100644
> --- a/Documentation/git-credential-store.txt
> +++ b/Documentation/git-credential-store.txt
> @@ -94,6 +94,10 @@ stored on its own line as a URL like:
>  https://user:pass@example.com
>  ------------------------------
>  
> +No other kinds of lines (e.g. empty lines or comment lines) are
> +allowed in the file, even though some may be silently ignored. Do
> +not view or edit the file with editors.

view should be ok; mentioning that any typos or extraneous characters
will compromise the validation of credentials like I mentioned in my
proposed documentation update probably worth doing here instead, too.

Carlo

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Jonathan Nieder]

Junio C Hamano wrote:

> -- >8 --
> Subject: credential-store: document the file format a bit more
>
> Reading a malformed credential URL line and silently ignoring it
> does not mean that we promise to torelate and/or keep empty lines
> and "# commented" lines forever.
>
> Some people seem to take anything that is not explicitly forbidden
> as allowed, but the world does not work that way.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git-credential-store.txt | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> index 693dd9d9d7..76b0798856 100644
> --- a/Documentation/git-credential-store.txt
> +++ b/Documentation/git-credential-store.txt
> @@ -94,6 +94,10 @@ stored on its own line as a URL like:
>  https://user:pass@example.com
>  ------------------------------
> 
> +No other kinds of lines (e.g. empty lines or comment lines) are
> +allowed in the file, even though some may be silently ignored. Do
> +not view or edit the file with editors.
> +
>  When Git needs authentication for a particular URL context,

I like this.

I do suspect this is easy to run into accidentally.  In $DAYJOB (in
the context of [1]) there was a service that accidentally wrote a \n\n
at the end of a line that was used by git-credential-store.  Once the
cause was tracked down, it was straightforward to fix, but I don't
like the idea that others in a similar position may end up tempted to
just not upgrade Git.

Independently, there is the thread we are replying to.

Independently, in Debian's bug tracking system, Stefan (cc-ed)
reports[2]:

| the vulnerability in CVE-2020-11008 is related to the handling
| of credential helpers in git. In Buster this has been fixed in
| 1:2.20.1-2+deb10u3. This broke my existing configuration where
| repositories have credential.helper=store set. This is
| documented in /usr/share/man/man1/git-credential-store.1.gz
| and other files from git, git-doc etc.
| I am unsure how to proceed... is this helper now unsupported?

(Stefan, do you have more details?  Did you manually populate your
credential store?  What error message do you get?)

I wonder if in addition to the above documentation change we may want
something guaranteed to catch all cases where people would have
experienced a regression, like

diff --git i/credential-store.c w/credential-store.c
index c010497cb21..294e7716815 100644
--- i/credential-store.c
+++ w/credential-store.c
@@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
+		if (!credential_from_url_gently(&entry, line.buf, 1) &&
+		    entry.username && entry.password &&
 		    credential_match(c, &entry)) {
 			found_credential = 1;
 			if (match_cb) {

And then we can tighten the handling of unrecognized lines to first
warn and then error out, as a controlled change that doesn't lead
people to regret updating git.

Thoughts?

Thanks,
Jonathan

[1] https://cs.opensource.google/copybara/copybara/+/master:java/com/google/copybara/git/GitOptions.java;drc=bc79a0b1ffe18f79dea0b75ba3a24b641a50a9fc;l=46
[2] https://bugs.debian.org/958929

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Jeff King]

On Mon, Apr 27, 2020 at 10:25:10PM -0700, Jonathan Nieder wrote:

> I wonder if in addition to the above documentation change we may want
> something guaranteed to catch all cases where people would have
> experienced a regression, like
> 
> diff --git i/credential-store.c w/credential-store.c
> index c010497cb21..294e7716815 100644
> --- i/credential-store.c
> +++ w/credential-store.c
> @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,
>  	}
>  
>  	while (strbuf_getline_lf(&line, fh) != EOF) {
> -		credential_from_url(&entry, line.buf);
> -		if (entry.username && entry.password &&
> +		if (!credential_from_url_gently(&entry, line.buf, 1) &&
> +		    entry.username && entry.password &&
>  		    credential_match(c, &entry)) {
>  			found_credential = 1;
>  			if (match_cb) {
> 
> And then we can tighten the handling of unrecognized lines to first
> warn and then error out, as a controlled change that doesn't lead
> people to regret updating git.

I like that solution, as it mostly brings us back to the original
behavior, as weird or unexpected as it was.

The only I think would be different is:

  ://user:pass@example.com

which I think the old code would have treated as matching any protocol
(and the new code will reject as a bogus URL).

I wondered if you'd need to free the partially-parsed struct fields on
failure, but the answer is no; we'd clear it the next time through the
loop, or at the very end of the function.

-Peff

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

On Tue, Apr 28, 2020 at 01:41:55AM -0400, Jeff King wrote:
> On Mon, Apr 27, 2020 at 10:25:10PM -0700, Jonathan Nieder wrote:
> 
> > I wonder if in addition to the above documentation change we may want
> > something guaranteed to catch all cases where people would have
> > experienced a regression, like
> > 
> > diff --git i/credential-store.c w/credential-store.c
> > index c010497cb21..294e7716815 100644
> > --- i/credential-store.c
> > +++ w/credential-store.c
> > @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,
> >  	}
> >  
> >  	while (strbuf_getline_lf(&line, fh) != EOF) {
> > -		credential_from_url(&entry, line.buf);
> > -		if (entry.username && entry.password &&
> > +		if (!credential_from_url_gently(&entry, line.buf, 1) &&
> > +		    entry.username && entry.password &&
> >  		    credential_match(c, &entry)) {
> >  			found_credential = 1;
> >  			if (match_cb) {
> > 
> > And then we can tighten the handling of unrecognized lines to first
> > warn and then error out, as a controlled change that doesn't lead
> > people to regret updating git.
> 
> I like that solution, as it mostly brings us back to the original
> behavior, as weird or unexpected as it was.

I like this version better as well, and we could even reuse my test
case.

it wouldn't cover cases where there were leading spaces/tabs around
the credential which I have to admit I liked just because it is
more robust to bad input, and there is no sane way now to tell the
user that there is invalid data anyway, but I am ok eitherway.

Carlo

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Jeff King]

On Tue, Apr 28, 2020 at 12:18:02AM -0700, Carlo Marcelo Arenas Belón wrote:

> > >  	while (strbuf_getline_lf(&line, fh) != EOF) {
> > > -		credential_from_url(&entry, line.buf);
> > > -		if (entry.username && entry.password &&
> > > +		if (!credential_from_url_gently(&entry, line.buf, 1) &&
> > > +		    entry.username && entry.password &&
> > >  		    credential_match(c, &entry)) {
> > >  			found_credential = 1;
> > >  			if (match_cb) {
> > > 
> > > And then we can tighten the handling of unrecognized lines to first
> > > warn and then error out, as a controlled change that doesn't lead
> > > people to regret updating git.
> > 
> > I like that solution, as it mostly brings us back to the original
> > behavior, as weird or unexpected as it was.
> 
> I like this version better as well, and we could even reuse my test
> case.
> 
> it wouldn't cover cases where there were leading spaces/tabs around
> the credential which I have to admit I liked just because it is
> more robust to bad input, and there is no sane way now to tell the
> user that there is invalid data anyway, but I am ok eitherway.

I think if we're discouraging people from hand-editing the file, then
that feature would be going in the wrong direction anyway.

Did you or Jonathan want to wrap it up with the test and commit message?

-Peff

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

On Tue, Apr 28, 2020 at 04:16:27AM -0400, Jeff King wrote:
> On Tue, Apr 28, 2020 at 12:18:02AM -0700, Carlo Marcelo Arenas Belón wrote:
> > 
> > it wouldn't cover cases where there were leading spaces/tabs around
> > the credential which I have to admit I liked just because it is
> > more robust to bad input, and there is no sane way now to tell the
> > user that there is invalid data anyway, but I am ok eitherway.
> 
> I think if we're discouraging people from hand-editing the file, then
> that feature would be going in the wrong direction anyway.

I think it will actually encourage them to edit the file (and complain
of a regression), since they can see the credential is correct, but
somehow git fails to work with it unlike their previous version.

remember there is no warning that could explain why that happens and
with this pach there is no error either.

> Did you or Jonathan want to wrap it up with the test and commit message?

I was going to say Jonathan will have to do it, since he was the only one
that didn't provide a patch (and therefore a sign off) but guess I can
save everyone some time and practice my git-send-email skills (which 
obviously failed me)

Jonathan,

make sure your patch[1] is complete and correct and to resend it with
a sign off, feel free to add me as Tested-by if you feel like, but I
added myself as Helped-by anyway ;)

Junio,

didn't fix the typo that Peff suggested fixed[2], because I forgot
and I wasn't sure if that also means I should SOB it, but if you
could also consider my suggestion[3] then at you get my SOB warranted ;)

Carlo

PS. guess I should had send it as an RFC instead

[1] https://lore.kernel.org/git/20200428105254.28658-3-carenas@gmail.com/
[2] https://lore.kernel.org/git/20200427211013.GB1732530@coredump.intra.peff.net/
[3] https://lore.kernel.org/git/20200427234909.GC61348@Carlos-MBP/

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Stefan Tauner]

On Mon, 27 Apr 2020 22:25:10 -0700
Jonathan Nieder <jrnieder@gmail.com> wrote:

> Independently, in Debian's bug tracking system, Stefan (cc-ed)
> reports[2]:
> 
> | the vulnerability in CVE-2020-11008 is related to the handling
> | of credential helpers in git. In Buster this has been fixed in
> | 1:2.20.1-2+deb10u3. This broke my existing configuration where
> | repositories have credential.helper=store set. This is
> | documented in /usr/share/man/man1/git-credential-store.1.gz
> | and other files from git, git-doc etc.
> | I am unsure how to proceed... is this helper now unsupported?
> 
> (Stefan, do you have more details?  Did you manually populate your
> credential store?  What error message do you get?)

I can't remember for sure - it's literally years ago - but I am pretty
sure I did not *populate* it manually initially. However, I might have
edited it to separate entries (https vs. smtp) and I might have added
additional entries by c&p & editing.

I have replaced the one instance of \n\n used for separation with a
normal single line break between entries and how it works fine. Before
that I got the following output (that's obviously not very helpful if
you don't know the whole story :)

> git pull
> warning: url has no scheme: 
> fatal: credential url cannot be parsed: 
> Already up to date.
> Current branch master is up to date.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I wonder if in addition to the above documentation change we may want
> something guaranteed to catch all cases where people would have
> experienced a regression, like
>
> diff --git i/credential-store.c w/credential-store.c
> index c010497cb21..294e7716815 100644
> --- i/credential-store.c
> +++ w/credential-store.c
> @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,
>  	}
>  
>  	while (strbuf_getline_lf(&line, fh) != EOF) {
> -		credential_from_url(&entry, line.buf);
> -		if (entry.username && entry.password &&
> +		if (!credential_from_url_gently(&entry, line.buf, 1) &&
> +		    entry.username && entry.password &&
>  		    credential_match(c, &entry)) {
>  			found_credential = 1;
>  			if (match_cb) {

Hmph, so the idea is, instead of ignoring the potential error
detected by credential_from_url() and using credential when it is
available, we loudly attempt to parse and give warning on malformed
entries when we discard a line?

I think that is an excellent idea.

It would be nicer if we can somehow add where we found the offending
line, e.g.

    /home/me/.gitcredential:396: url has no scheme: ://u:p@host/path

Do we feel it a bit disturbing that u:p is shown in the error
message, without redacting, by the way?

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

On Tue, Apr 28, 2020 at 09:03:36AM -0700, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > I wonder if in addition to the above documentation change we may want
> > something guaranteed to catch all cases where people would have
> > experienced a regression, like
> >
> > diff --git i/credential-store.c w/credential-store.c
> > index c010497cb21..294e7716815 100644
> > --- i/credential-store.c
> > +++ w/credential-store.c
> > @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,
> >  	}
> >  
> >  	while (strbuf_getline_lf(&line, fh) != EOF) {
> > -		credential_from_url(&entry, line.buf);
> > -		if (entry.username && entry.password &&
> > +		if (!credential_from_url_gently(&entry, line.buf, 1) &&
> > +		    entry.username && entry.password &&
> >  		    credential_match(c, &entry)) {
> >  			found_credential = 1;
> >  			if (match_cb) {
> 
> Hmph, so the idea is, instead of ignoring the potential error
> detected by credential_from_url() and using credential when it is
> available, we loudly attempt to parse and give warning on malformed
> entries when we discard a line?

the idea was to silently ignore the line (notice quiet = 1), which
is the "original behaviour" as Peff pointed out in his reply in
20200428054155.GB2376380@coredump.intra.peff.net[1]

> I think that is an excellent idea.
> 
> It would be nicer if we can somehow add where we found the offending
> line, e.g.
> 
>     /home/me/.gitcredential:396: url has no scheme: ://u:p@host/path
> 
> Do we feel it a bit disturbing that u:p is shown in the error
> message, without redacting, by the way?

we could do so with the following on top as a 5/4.

most of the test changes would go away in a reroll, and are similar
to what would have been needed for my original suggested patch[2] with
the exception that in this series we won't support silently fixing
credentials which is something you mentioned a preference on.

Carlo

[1] https://lore.kernel.org/git/20200428054155.GB2376380@coredump.intra.peff.net/ 
[2] https://lore.kernel.org/git/20200428013726.GD61348@Carlos-MBP/
-- >8 --
Subject: [RFC PATCH] credential-store: SQUASH!!

add warnings

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 credential-store.c          | 55 +++++++++++++++++++++++++++++++------
 t/t0302-credential-store.sh | 42 ++++++++++++++++------------
 2 files changed, 70 insertions(+), 27 deletions(-)

diff --git a/credential-store.c b/credential-store.c
index 294e771681..f76292df20 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -6,6 +6,32 @@
 
 static struct lock_file credential_lock;
 
+static char *redact_credential(const struct strbuf *line)
+{
+	struct strbuf redacted_line = STRBUF_INIT;
+	char *at = strchr(line->buf, '@');
+	char *colon;
+	int redacted = 0;
+
+	if (at) {
+		strbuf_addf(&redacted_line, "%.*s",
+			(int)(at - line->buf), line->buf);
+		colon = strrchr(redacted_line.buf, ':');
+		if (colon && *(colon + 1) != '/' && colon > redacted_line.buf) {
+			redacted_line.len = colon - redacted_line.buf + 1;
+			strbuf_addstr(&redacted_line, "<redacted>");
+			strbuf_addstr(&redacted_line, at);
+			redacted = 1;
+		}
+		else
+			strbuf_reset(&redacted_line);
+	}
+	if (!redacted)
+		strbuf_addbuf(&redacted_line, line);
+
+	return redacted_line.buf;
+}
+
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
 				  void (*match_cb)(struct credential *),
@@ -15,6 +41,7 @@ static int parse_credential_file(const char *fn,
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
 	int found_credential = 0;
+	int lineno = 0;
 
 	fh = fopen(fn, "r");
 	if (!fh) {
@@ -24,17 +51,27 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		if (!credential_from_url_gently(&entry, line.buf, 1) &&
-		    entry.username && entry.password &&
-		    credential_match(c, &entry)) {
-			found_credential = 1;
-			if (match_cb) {
-				match_cb(&entry);
-				break;
+		lineno++;
+		if (!credential_from_url_gently(&entry, line.buf, 1)) {
+			if (entry.username && entry.password &&
+				credential_match(c, &entry)) {
+				found_credential = 1;
+				if (match_cb) {
+					match_cb(&entry);
+					break;
+				}
 			}
+			else if (other_cb)
+				other_cb(&line);
+		}
+		else {
+			if (other_cb)
+				other_cb(&line);
+			char *redacted = redact_credential(&line);
+			warning("%s:%d ignoring invalid credential: %s",
+				fn, lineno, redacted);
+			free(redacted);
 		}
-		else if (other_cb)
-			other_cb(&line);
 	}
 
 	credential_clear(&entry);
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 7c7a48303f..597a75903a 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,36 +120,42 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
-test_expect_success 'get: store file can contain empty/bogus lines' '
-	test_write_lines "#comment" " " "" \
-		 https://user:pass@example.com >"$HOME/.git-credentials" &&
-	check fill store <<-\EOF
+test_expect_success 'get: credentials without scheme are invalid' '
+	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
 	protocol=https
 	host=example.com
-	--
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
 	protocol=https
 	host=example.com
-	username=user
-	password=pass
-	--
 	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
 '
 
-test_expect_success 'get: ignore credentials without scheme' '
-	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
-	check fill store <<-\EOF
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	test_write_lines "#comment" " " "" \
+		 https://user:pass@example.com >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
 	protocol=https
 	host=example.com
-	--
+	username=user
+	password=pass
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
 	protocol=https
 	host=example.com
-	username=askpass-username
-	password=askpass-password
-	--
-	askpass: Username for '\''https://example.com'\'':
-	askpass: Password for '\''https://askpass-username@example.com'\'':
-	--
 	EOF
+	test_cmp expect-stdout stdout &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	test_line_count = 3 stderr
 '
 
 test_done
-- 
2.26.2.569.g1d74ac4d14

Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

>> > +		if (!credential_from_url_gently(&entry, line.buf, 1) &&
>> > +		    entry.username && entry.password &&
>> >  		    credential_match(c, &entry)) {
>> >  			found_credential = 1;
>> >  			if (match_cb) {
>> 
>> Hmph, so the idea is, instead of ignoring the potential error
>> detected by credential_from_url() and using credential when it is
>> available, we loudly attempt to parse and give warning on malformed
>> entries when we discard a line?
>
> the idea was to silently ignore the line (notice quiet = 1), which

Ah, sorry, I apparently did not read the patch carefully enough.

Thanks for a correction.

[PATCH v4 0/4] credential-store: prevent fatal errors

[Carlo Marcelo Arenas Belón]

with the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] as failing to parse as valid credentials.

document better the format for the credential file and make sure
any failures are still handled gently by the new code as a first
step to allow people to upgrade without having first to fix their
corrupted credential files.

[1] https://stackoverflow.com/a/61420852/5005936

Carlo Marcelo Arenas Belón (2):
  git-credential-store: skip empty lines and comments from store
  credential-store: make sure there is no regression with missing scheme

Jonathan Nieder (1):
  git-credential-store: fix (WIP)

Junio C Hamano (1):
  credential-store: document the file format a bit more

 Documentation/git-credential-store.txt |  4 ++++
 credential-store.c                     |  4 ++--
 t/t0302-credential-store.sh            | 32 ++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 2 deletions(-)

-- 
2.26.2.569.g1d74ac4d14

[PATCH v4 1/4] credential-store: document the file format a bit more

[Carlo Marcelo Arenas Belón]

From: Junio C Hamano <gitster@pobox.com>

Reading a malformed credential URL line and silently ignoring it
does not mean that we promise to torelate and/or keep empty lines
and "# commented" lines forever.

Some people seem to take anything that is not explicitly forbidden
as allowed, but the world does not work that way.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-credential-store.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 693dd9d9d7..76b0798856 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -94,6 +94,10 @@ stored on its own line as a URL like:
 https://user:pass@example.com
 ------------------------------
 
+No other kinds of lines (e.g. empty lines or comment lines) are
+allowed in the file, even though some may be silently ignored. Do
+not view or edit the file with editors.
+
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
 each entry in the credentials file.  If the protocol, hostname, and
-- 
2.26.2.569.g1d74ac4d14

[PATCH v4 2/4] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

with the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

add corresponding failing cases

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/t0302-credential-store.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..94cdcb9e56 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_failure 'get: store file can contain empty/bogus lines' '
+	test_write_lines "#comment" " " "" \
+		 https://user:pass@example.com >"$HOME/.git-credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	--
+	EOF
+'
+
 test_done
-- 
2.26.2.569.g1d74ac4d14

Re: [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store

[Eric Sunshine]

On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> git-credential-store: skip empty lines and comments from store

I don't see anything in this patch which makes it skip anything at
all; it only introduces a new test.

> with the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported[1] failing to parse as valid credentials.
>
> add corresponding failing cases
>
> [1] https://stackoverflow.com/a/61420852/5005936
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>

Re: [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store

[Carlo Marcelo Arenas Belón]

On Tue, Apr 28, 2020 at 12:09:50PM -0400, Eric Sunshine wrote:
> On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón
> <carenas@gmail.com> wrote:
> > git-credential-store: skip empty lines and comments from store
> 
> I don't see anything in this patch which makes it skip anything at
> all; it only introduces a new test.

my bad, forgot to fix the subject when rerolling; does something like:
credential-store: show "regression" when bogus credentials in file
make sense for the reroll?

Carlo

[PATCH v4 3/4] git-credential-store: fix (WIP)

[Carlo Marcelo Arenas Belón]

From: Jonathan Nieder <jrnieder@gmail.com>

Helped-by: Carlo Marcelo Arenas Belon <carenas@gmail.com>
---
 credential-store.c          | 4 ++--
 t/t0302-credential-store.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/credential-store.c b/credential-store.c
index c010497cb2..294e771681 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
+		if (!credential_from_url_gently(&entry, line.buf, 1) &&
+		    entry.username && entry.password &&
 		    credential_match(c, &entry)) {
 			found_credential = 1;
 			if (match_cb) {
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 94cdcb9e56..4e5a73cb99 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,7 +120,7 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
-test_expect_failure 'get: store file can contain empty/bogus lines' '
+test_expect_success 'get: store file can contain empty/bogus lines' '
 	test_write_lines "#comment" " " "" \
 		 https://user:pass@example.com >"$HOME/.git-credentials" &&
 	check fill store <<-\EOF
-- 
2.26.2.569.g1d74ac4d14

Re: [PATCH v4 3/4] git-credential-store: fix (WIP)

[Eric Sunshine]

On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> From: Jonathan Nieder <jrnieder@gmail.com>
> Subject: git-credential-store: fix (WIP)

Um, what? Did you forget to squash this into the previous patch?

> Helped-by: Carlo Marcelo Arenas Belon <carenas@gmail.com>

No sign-off? (Jonathan's)

Re: [PATCH v4 3/4] git-credential-store: fix (WIP)

[Carlo Marcelo Arenas Belón]

On Tue, Apr 28, 2020 at 12:11:20PM -0400, Eric Sunshine wrote:
> On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón
> <carenas@gmail.com> wrote:
> > From: Jonathan Nieder <jrnieder@gmail.com>
> > Subject: git-credential-store: fix (WIP)
> 
> Um, what? Did you forget to squash this into the previous patch?

no; was documenting his proposal from 20200428052510.GA201501@google.com[1]
with the hope (unsuccesfully) to save everyone time and easy testing by
having a series ready to apply.

> > Helped-by: Carlo Marcelo Arenas Belon <carenas@gmail.com>
> 
> No sign-off? (Jonathan's)

really my fault, but was expected as documented in:

  https://lore.kernel.org/git/20200428112518.GA15981@Carlos-MBP/

sorry for the confussion and thanks for your review

Carlo

[1] https://lore.kernel.org/git/20200428052510.GA201501@google.com/

[PATCH v4 4/4] credential-store: make sure there is no regression with missing scheme

[Carlo Marcelo Arenas Belón]

c44088ecc4 (credential: treat URL without scheme as invalid, 2020-04-18)
made scheme (AKA protocol) mandatory so make sure that still fails.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/t0302-credential-store.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 4e5a73cb99..7c7a48303f 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -135,4 +135,21 @@ test_expect_success 'get: store file can contain empty/bogus lines' '
 	EOF
 '
 
+test_expect_success 'get: ignore credentials without scheme' '
+	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	--
+	askpass: Username for '\''https://example.com'\'':
+	askpass: Password for '\''https://askpass-username@example.com'\'':
+	--
+	EOF
+'
+
 test_done
-- 
2.26.2.569.g1d74ac4d14

Re: [PATCH v4 1/4] credential-store: document the file format a bit more

[Eric Sunshine]

On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> Reading a malformed credential URL line and silently ignoring it
> does not mean that we promise to torelate and/or keep empty lines
> and "# commented" lines forever.
>
> Some people seem to take anything that is not explicitly forbidden
> as allowed, but the world does not work that way.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> @@ -94,6 +94,10 @@ stored on its own line as a URL like:
> +No other kinds of lines (e.g. empty lines or comment lines) are
> +allowed in the file, even though some may be silently ignored. Do
> +not view or edit the file with editors.

I suggest dropping the "even though some may be silently ignored" bit
since it's both mysterious (providing no concrete information) and
unnecessarily confusing since it flat out contradicts the earlier part
of the sentence. The fact that the implementation has accidentally
been loose in its parsing doesn't warrant introducing such ambiguity
into the (newly-added) documentation.

Re: [PATCH v4 1/4] credential-store: document the file format a bit more

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Apr 28, 2020 at 6:53 AM Carlo Marcelo Arenas Belón
> <carenas@gmail.com> wrote:
>> Reading a malformed credential URL line and silently ignoring it
>> does not mean that we promise to torelate and/or keep empty lines
>> and "# commented" lines forever.
>>
>> Some people seem to take anything that is not explicitly forbidden
>> as allowed, but the world does not work that way.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
>> @@ -94,6 +94,10 @@ stored on its own line as a URL like:
>> +No other kinds of lines (e.g. empty lines or comment lines) are
>> +allowed in the file, even though some may be silently ignored. Do
>> +not view or edit the file with editors.
>
> I suggest dropping the "even though some may be silently ignored" bit
> since it's both mysterious (providing no concrete information) and
> unnecessarily confusing since it flat out contradicts the earlier part
> of the sentence. The fact that the implementation has accidentally
> been loose in its parsing doesn't warrant introducing such ambiguity
> into the (newly-added) documentation.

I do not think it is an ambiguity.  The phrase is there just to
remind the readers that they are not allowed to take a loose
implementation in the past as an excuse to throw in crufts and
expect the result to continue working.

Re: [PATCH v4 1/4] credential-store: document the file format a bit more

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> From: Junio C Hamano <gitster@pobox.com>
>
> Reading a malformed credential URL line and silently ignoring it
> does not mean ...

I'd reuse the one I've already queued as 272281ef (credential-store:
document the file format a bit more, 2020-04-27) for this step.

Thanks.

-- >8 --
Subject: [PATCH] credential-store: document the file format a bit more

Reading a malformed credential URL line and silently ignoring it
does not mean that we support empty lines and/or "# commented" lines
forever.  We should document it to avoid confusion.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-credential-store.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 693dd9d9d7..76b0798856 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -94,6 +94,10 @@ stored on its own line as a URL like:
 https://user:pass@example.com
 ------------------------------
 
+No other kinds of lines (e.g. empty lines or comment lines) are
+allowed in the file, even though some may be silently ignored. Do
+not view or edit the file with editors.
+
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
 each entry in the credentials file.  If the protocol, hostname, and
-- 
2.26.2-266-ge870325ee8

[PATCH v5] credential-store: warn instead of fatal for bogus lines from store

[Carlo Marcelo Arenas Belón]

with the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

instead of doing a hard check for credentials, do a soft one and
warn the user so any invalid entries could be corrected.

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v5:
* q_to_tab this round, with a single echo to make sure empty line
  is covered, as that seems to be a popular issue
* rebase on top of jc/credential-store-file-format-doc
* implement a redacter for credentials to use on errora to avoid
  leaking passwordss
v4:
* use credential_from_url_gently instead as shown by Jonathan
* add documentation to clarify "comments" is not a supported feature
v3:
* avoid using q_to_cr as suggested by Peff
* a more verbose commit message and slightly more complete documentation
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 Documentation/git-credential-store.txt | 12 +++++-
 credential-store.c                     | 51 ++++++++++++++++++++++----
 t/t0302-credential-store.sh            | 43 ++++++++++++++++++++++
 3 files changed, 96 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 76b0798856..30d498fe54 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -95,8 +95,16 @@ https://user:pass@example.com
 ------------------------------
 
 No other kinds of lines (e.g. empty lines or comment lines) are
-allowed in the file, even though some may be silently ignored. Do
-not view or edit the file with editors.
+allowed in the file, even though historically the parser was very
+lenient and some might had been silently ignored.
+
+Do not edit the file with editors as it could compromise the validity
+of your credentials by sometimes subtle formatting issues (like spaces)
+
+In cases where those formatting issues are detected during parsing a
+warning indicating the line found will be printed to stderr so it can
+be corrected at your earliest convenience, but the remaining valid
+credentials will be used to try to find a match as described below.
 
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..5324c56ce1 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -6,6 +6,32 @@
 
 static struct lock_file credential_lock;
 
+static char *redact_credential(const struct strbuf *line)
+{
+	struct strbuf redacted_line = STRBUF_INIT;
+	char *at = strchr(line->buf, '@');
+	char *colon;
+	int redacted = 0;
+
+	if (at) {
+		strbuf_addf(&redacted_line, "%.*s",
+			(int)(at - line->buf), line->buf);
+		colon = strrchr(redacted_line.buf, ':');
+		if (colon && *(colon + 1) != '/') {
+			redacted_line.len = colon - redacted_line.buf + 1;
+			strbuf_addstr(&redacted_line, "<redacted>");
+			strbuf_addstr(&redacted_line, at);
+			redacted = 1;
+		}
+		else
+			strbuf_reset(&redacted_line);
+	}
+	if (!redacted)
+		strbuf_addbuf(&redacted_line, line);
+
+	return redacted_line.buf;
+}
+
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
 				  void (*match_cb)(struct credential *),
@@ -15,6 +41,7 @@ static int parse_credential_file(const char *fn,
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
 	int found_credential = 0;
+	int lineno = 0;
 
 	fh = fopen(fn, "r");
 	if (!fh) {
@@ -24,16 +51,24 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
-		    credential_match(c, &entry)) {
-			found_credential = 1;
-			if (match_cb) {
-				match_cb(&entry);
-				break;
+		lineno++;
+		if (!credential_from_url_gently(&entry, line.buf, 1)) {
+			if (entry.username && entry.password &&
+				credential_match(c, &entry)) {
+				found_credential = 1;
+				if (match_cb) {
+					match_cb(&entry);
+					break;
+				}
 			}
 		}
-		else if (other_cb)
+		else {
+			char *redacted = redact_credential(&line);
+			warning("%s:%d %s: %s", fn, lineno,
+				 _("ignoring invalid credential"), redacted);
+			free(redacted);
+		}
+		if (!found_credential && other_cb)
 			other_cb(&line);
 	}
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..68b59e6f98 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,47 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'get: credentials without scheme are invalid' '
+	echo "://user:secret@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	! grep secret stderr
+'
+
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	echo "" > "$HOME/.git-credentials" &&
+	q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CONFIG
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	test_line_count = 3 stderr
+'
+
 test_done

base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0
-- 
2.26.2.569.g1d74ac4d14

Re: [PATCH v5] credential-store: warn instead of fatal for bogus lines from store

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> +static char *redact_credential(const struct strbuf *line)
> +{
> +	struct strbuf redacted_line = STRBUF_INIT;
> +	char *at = strchr(line->buf, '@');
> +	char *colon;
> +	int redacted = 0;
> +
> +	if (at) {
> +		strbuf_addf(&redacted_line, "%.*s",
> +			(int)(at - line->buf), line->buf);
> +		colon = strrchr(redacted_line.buf, ':');

Just showing my ignorance, but ...

 - Is the above strrchr() that forbids a colon in the password
   intended, or should it be strchr() that only forbids a colon in
   the username instead?

 - Would it hurt to redact both username and password as sensitive?
   If not, it would certainly make it simpler to unconditionally:

                int i;
                for (i = 0; i < redacted_line.len; i++) {
                        if (redacted_line.buf[i] != ':')
                                redacted_line.buf[i] = 'x';
                }

Re: [PATCH v5] credential-store: warn instead of fatal for bogus lines from store

[Carlo Marcelo Arenas Belón]

On Tue, Apr 28, 2020 at 09:36:00PM -0700, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> > +static char *redact_credential(const struct strbuf *line)
> > +{
> > +	struct strbuf redacted_line = STRBUF_INIT;
> > +	char *at = strchr(line->buf, '@');
> > +	char *colon;
> > +	int redacted = 0;
> > +
> > +	if (at) {
> > +		strbuf_addf(&redacted_line, "%.*s",
> > +			(int)(at - line->buf), line->buf);
> > +		colon = strrchr(redacted_line.buf, ':');
> 
> Just showing my ignorance, but ...
> 
>  - Is the above strrchr() that forbids a colon in the password
>    intended, or should it be strchr() that only forbids a colon in
>    the username instead?

neither; that ":" is the separator we put in there between username
and password (unless it was edited out), as any original ":" in both
is already urlencoded.

strrchr() was used in purpose to back only as much as it is needed
from the string to hopefully find the beginning of the password.

indeed, most of the uglyness of the code comes from the fact it tries
really hard to only redact the password so the rest of the credential
is still useful for context and do so without changing the original
line.

>  - Would it hurt to redact both username and password as sensitive?
>    If not, it would certainly make it simpler to unconditionally:
> 
>                 int i;
>                 for (i = 0; i < redacted_line.len; i++) {
>                         if (redacted_line.buf[i] != ':')
>                                 redacted_line.buf[i] = 'x';
>                 }

that would leak the length of the password which might also be considered
sensitive

IMHO if we are redacting both might as well not print anything and let
the user find the offending credential by line number instead ;) after
all I got a feeling most of those entries are empty lines or "comments".

for example with the current code merged in pu we get :

$ cat secretfile

://u:p@example.com
http://user:some%3apass@example.com

$ git credential-store --file secretfile get
protocol=http

warning: secretfile:1 ignoring invalid credential:
warning: secretfile:2 ignoring invalid credential: ://u:<redacted>@example.com
username=user
password=some:pass

Carlo

Re: [PATCH v5] credential-store: warn instead of fatal for bogus lines from store

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> IMHO if we are redacting both might as well not print anything and let
> the user find the offending credential by line number instead ;) after
> all I got a feeling most of those entries are empty lines or "comments".

Very sensible.

[RFC PATCH v6 0/2] credential-store: prevent fatal errors

[Carlo Marcelo Arenas Belón]

with the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] as failing to parse as valid credentials.

document better the format for the credential file and make sure
any failures are still handled gently by the new code to avoid
regressions but make sure that any invalid credentials are flagged
as such instead of being silently ignored.

ideally would like to add a test against the use of cert:// to make
sure that is still well supported but don't know of any use case or
if it is being used with store (AFAIK, it could and at least in theory
it should still work fine with the added validations), hence sending
this as an RFC.

also, there is no real reason to have 2 patches but I am keeping them
separated because the first one should be AFAIK otherwise almost ready.

but considering that we are validating since v4 against a missing
protocol and printing a warning since v5, it seems we should then
fully avoid all those silently ignored issues as well (as shown in
patch 2) which I hope we could squash and release together (if agreed)

[1] https://stackoverflow.com/a/61420852/5005936

Carlo Marcelo Arenas Belón (2):
  credential-store: warn instead of fatal for bogus lines from store
  credential-store: warn for any incomplete credentials instead of using

---
v6:
* get rid of redacter and only use line number for context
* make validation more strict to also catch incomplete credentials
v5:
* q_to_tab this round, with a single echo to make sure empty line
  is covered, as that seems to be a popular issue
* rebase on top of jc/credential-store-file-format-doc
* implement a redacter for credentials to use on errors to avoid
  leaking passwordss
v4:
* use credential_from_url_gently instead as shown by Jonathan
* add documentation to clarify "comments" is not a supported feature
v3:
* avoid using q_to_cr as suggested by Peff
* a more verbose commit message and slightly more complete documentation
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 Documentation/git-credential-store.txt | 12 +++-
 credential-store.c                     | 23 +++++---
 t/t0302-credential-store.sh            | 80 ++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 10 deletions(-)


base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0
-- 
2.26.2.569.g1d74ac4d14

[RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store

[Carlo Marcelo Arenas Belón]

with the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

instead of doing a hard check for credentials, do a soft one and
warn the user so any invalid entries could be corrected.

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 Documentation/git-credential-store.txt | 12 ++++++--
 credential-store.c                     | 22 +++++++++-----
 t/t0302-credential-store.sh            | 42 ++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 76b0798856..30d498fe54 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -95,8 +95,16 @@ https://user:pass@example.com
 ------------------------------
 
 No other kinds of lines (e.g. empty lines or comment lines) are
-allowed in the file, even though some may be silently ignored. Do
-not view or edit the file with editors.
+allowed in the file, even though historically the parser was very
+lenient and some might had been silently ignored.
+
+Do not edit the file with editors as it could compromise the validity
+of your credentials by sometimes subtle formatting issues (like spaces)
+
+In cases where those formatting issues are detected during parsing a
+warning indicating the line found will be printed to stderr so it can
+be corrected at your earliest convenience, but the remaining valid
+credentials will be used to try to find a match as described below.
 
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..1cc5ca081a 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -15,6 +15,7 @@ static int parse_credential_file(const char *fn,
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
 	int found_credential = 0;
+	int lineno = 0;
 
 	fh = fopen(fn, "r");
 	if (!fh) {
@@ -24,16 +25,21 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
-		    credential_match(c, &entry)) {
-			found_credential = 1;
-			if (match_cb) {
-				match_cb(&entry);
-				break;
+		lineno++;
+		if (!credential_from_url_gently(&entry, line.buf, 1)) {
+			if (entry.username && entry.password &&
+				credential_match(c, &entry)) {
+				found_credential = 1;
+				if (match_cb) {
+					match_cb(&entry);
+					break;
+				}
 			}
 		}
-		else if (other_cb)
+		else
+			warning(_("ignoring invalid credential in %s:%d"),
+				fn, lineno);
+		if (!found_credential && other_cb)
 			other_cb(&line);
 	}
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..801c1eb200 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,46 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'get: credentials without scheme are invalid' '
+	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	echo "" > "$HOME/.git-credentials" &&
+	q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CONFIG
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	test_line_count = 3 stderr
+'
+
 test_done
-- 
2.26.2.569.g1d74ac4d14

Re: [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> with the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported[1] failing to parse as valid credentials.
>
> instead of doing a hard check for credentials, do a soft one and
> warn the user so any invalid entries could be corrected.

Yeah, this version looks be best so far.  No need to worry about
redacting anything; we are dealing with folks who have edited
the file, and they shouldn't have any trouble going to the line with
a problem given an exact line number.

> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> index 76b0798856..30d498fe54 100644
> --- a/Documentation/git-credential-store.txt
> +++ b/Documentation/git-credential-store.txt
> @@ -95,8 +95,16 @@ https://user:pass@example.com
>  ------------------------------
>  
>  No other kinds of lines (e.g. empty lines or comment lines) are
> -allowed in the file, even though some may be silently ignored. Do
> -not view or edit the file with editors.
> +allowed in the file, even though historically the parser was very
> +lenient and some might had been silently ignored.
> +
> +Do not edit the file with editors as it could compromise the validity
> +of your credentials by sometimes subtle formatting issues (like spaces)

I do not think dropping "view or" is warranted.  There is no need to
invite the risk of opening with the intention to only view and then
end up saving a modification.  In other words, do not encourage use
of an *editor* in any way.

> +In cases where those formatting issues are detected during parsing a
> +warning indicating the line found will be printed to stderr so it can
> +be corrected at your earliest convenience, but the remaining valid
> +credentials will be used to try to find a match as described below.

I am often just as guilty, but the above four lines in a single
sentence is hard to read, especially without minimum number of
comma.  With a comma after "during parsing", and another after "to
stderr", might make it more readable, but at that point, it would
become far more readable if we split them into two sentences.

Perhaps

	An unparseable line is ignored, and a warning message points
	out the line number the problematic line appears in (you may
	want to delete them with an editor).

>  	while (strbuf_getline_lf(&line, fh) != EOF) {
> +		lineno++;
> +		if (!credential_from_url_gently(&entry, line.buf, 1)) {
> +			if (entry.username && entry.password &&
> +				credential_match(c, &entry)) {
> +				found_credential = 1;
> +				if (match_cb) {
> +					match_cb(&entry);
> +					break;
> +				}
>  			}
>  		}
> +		else
> +			warning(_("ignoring invalid credential in %s:%d"),
> +				fn, lineno);
> +		if (!found_credential && other_cb)
>  			other_cb(&line);
>  	}

The above is harder to follow than necessary.

	while (... != EOF) {
		lineno++;
		if (credential is not well formed) {
			warning(_("ignoring..."));
		} else if (the entry matches) {
			found_credential = 1;
			if (match_cb) {
				match_cb(&entry);
				break; /* stop at the first match */
			}
			continue;
		}

                if (other_cb)
			other_cb(&line);
	}

may make the intention a bit clearer, with the loud "continue" inside.

 (1) we give warning for malformed one; and
 (2) we do not let other_cb touch a matching entry.

Re: [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>  	while (strbuf_getline_lf(&line, fh) != EOF) {
>> +		lineno++;
>> +		if (!credential_from_url_gently(&entry, line.buf, 1)) {
>> +			if (entry.username && entry.password &&
>> +				credential_match(c, &entry)) {
>> +				found_credential = 1;
>> +				if (match_cb) {
>> +					match_cb(&entry);
>> +					break;
>> +				}
>>  			}
>>  		}
>> +		else
>> +			warning(_("ignoring invalid credential in %s:%d"),
>> +				fn, lineno);
>> +		if (!found_credential && other_cb)
>>  			other_cb(&line);
>>  	}
>
> The above is harder to follow than necessary.
>
> 	while (... != EOF) {
> 		lineno++;
> 		if (credential is not well formed) {

After reading your [2/2], I think this part deserves a bit of
explanation.  By "is not well formed", what I mean is not just
credential_from_url_gently() returns non-zero, but also user/pass
are specified.  And your step [2/2] may make the condition for a
line to be "well formed" even stricter by requiring at least one of
host or path, and that would also belong to this test.

> 			warning(_("ignoring..."));
> 		} else if (the entry matches) {

And the "entry matches" test here would only be "what does
credential_match() say?"

> 			found_credential = 1;
> 			if (match_cb) {
> 				match_cb(&entry);
> 				break; /* stop at the first match */
> 			}
> 			continue;
> 		}
>
>                 if (other_cb)
> 			other_cb(&line);
> 	}
>
> may make the intention a bit clearer, with the loud "continue" inside.
>
>  (1) we give warning for malformed one; and
>  (2) we do not let other_cb touch a matching entry.

[RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of using

[Carlo Marcelo Arenas Belón]

originally any credential found was tried for matching as far as it had
a username and password, but that resulted in fatal errors as the rules
were harden.

now that we have a way to report malformed credentials, use it to notify
the user when username/password was missing, instead of just silently
skipping.

do the same for credentials that are missing host (or had one that is
empty) or that are missing a path (for supporting cert://) as well.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 credential-store.c          |  7 ++++---
 t/t0302-credential-store.sh | 38 +++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/credential-store.c b/credential-store.c
index 1cc5ca081a..53f77ff6f5 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -26,9 +26,10 @@ static int parse_credential_file(const char *fn,
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
 		lineno++;
-		if (!credential_from_url_gently(&entry, line.buf, 1)) {
-			if (entry.username && entry.password &&
-				credential_match(c, &entry)) {
+		if (!credential_from_url_gently(&entry, line.buf, 1) &&
+			((entry.host && *entry.host) || entry.path) &&
+			entry.username && entry.password) {
+			if (credential_match(c, &entry)) {
 				found_credential = 1;
 				if (match_cb) {
 					match_cb(&entry);
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 801c1eb200..3150f304cb 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -139,6 +139,44 @@ test_expect_success 'get: credentials without scheme are invalid' '
 	test_i18ngrep "ignoring invalid credential" stderr
 '
 
+test_expect_success 'get: credentials without valid host/path are invalid' '
+	echo "https://user:pass@" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials without username/password are invalid' '
+	echo "https://pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
 test_expect_success 'get: store file can contain empty/bogus lines' '
 	echo "" > "$HOME/.git-credentials" &&
 	q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" &&
-- 
2.26.2.569.g1d74ac4d14

Re: [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of using

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> originally any credential found was tried for matching as far as it had
> a username and password, but that resulted in fatal errors as the rules
> were harden.

harden -> hardened

> now that we have a way to report malformed credentials, use it to notify
> the user when username/password was missing, instead of just silently
> skipping.

Sorry, but isn't that what happend already in the previous step?
What are you ordering the codebase (after applying the previous
stpe) do further?  It already is "using it to notify the user when
username and/or password is missing".

> do the same for credentials that are missing host (or had one that is
> empty) or that are missing a path (for supporting cert://) as well.

While the intention may be a good one ...


> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  credential-store.c          |  7 ++++---
>  t/t0302-credential-store.sh | 38 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/credential-store.c b/credential-store.c
> index 1cc5ca081a..53f77ff6f5 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -26,9 +26,10 @@ static int parse_credential_file(const char *fn,
>  
>  	while (strbuf_getline_lf(&line, fh) != EOF) {
>  		lineno++;
> -		if (!credential_from_url_gently(&entry, line.buf, 1)) {
> -			if (entry.username && entry.password &&
> -				credential_match(c, &entry)) {
> +		if (!credential_from_url_gently(&entry, line.buf, 1) &&
> +			((entry.host && *entry.host) || entry.path) &&
> +			entry.username && entry.password) {
> +			if (credential_match(c, &entry)) {

... this makes the code even harder to follow than it already was
after the previous step.  In the previous step, at least it was
clear that we'd catch *all* the well-formed/parseable input will
come into the first if () {...} block, but with the extra logic,
that is no longer true.  Even a line that is well formed may be
bypassed from matching and will be fed to "else" side.

Re: [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of usingy

[Carlo Marcelo Arenas Belón]

On Wed, Apr 29, 2020 at 02:12:21PM -0700, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> 
> > originally any credential found was tried for matching as far as it had
> > a username and password, but that resulted in fatal errors as the rules
> > were harden.
> 
> harden -> hardened
> 
> > now that we have a way to report malformed credentials, use it to notify
> > the user when username/password was missing, instead of just silently
> > skipping.
> 
> Sorry, but isn't that what happend already in the previous step?
> What are you ordering the codebase (after applying the previous
> stpe) do further?  It already is "using it to notify the user when
> username and/or password is missing".

not sure I follow, but the current code (as well as the one after patch 1)
just silently ignores any credential that was missing username and password
since the _gently version of the call will only really fail for a missing
protocol.

so patch1 will notify ONLY(*) when the credential is so malformed that we can't
figure out which protocol to use (like empty lines and comments)

(*) it will also fail if we have a '\n' in one of the components but that
is hopefully not relevant here.

> > diff --git a/credential-store.c b/credential-store.c
> > index 1cc5ca081a..53f77ff6f5 100644
> > --- a/credential-store.c
> > +++ b/credential-store.c
> > @@ -26,9 +26,10 @@ static int parse_credential_file(const char *fn,
> >  
> >  	while (strbuf_getline_lf(&line, fh) != EOF) {
> >  		lineno++;
> > -		if (!credential_from_url_gently(&entry, line.buf, 1)) {
> > -			if (entry.username && entry.password &&
> > -				credential_match(c, &entry)) {
> > +		if (!credential_from_url_gently(&entry, line.buf, 1) &&
> > +			((entry.host && *entry.host) || entry.path) &&
> > +			entry.username && entry.password) {
> > +			if (credential_match(c, &entry)) {
> 
> ... this makes the code even harder to follow than it already was
> after the previous step.  In the previous step, at least it was
> clear that we'd catch *all* the well-formed/parseable input will
> come into the first if () {...} block, but with the extra logic,
> that is no longer true.  Even a line that is well formed may be
> bypassed from matching and will be fed to "else" side.

I think it will be clearer with the reformatting you suggested and that I
agree is needed, but wanted to make sure first that we agreed on the direction.

FWIW the test cases show the format of the lines affected and while they are
parseable enough that were processed they were obviously not valid, specially
considering the updated rules.

maybe we should make the _gently version less gently instead?, so that callers
will be able to know they have an incomplete credential for matching after
calling it without having to do their own check?

if the later, I am affraid this will be a far bigger change, which is why I
would rather duplicate the rule checking for at least the first iteration.

Carlo

Re: [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of usingy

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

>> Sorry, but isn't that what happend already in the previous step?
>> What are you ordering the codebase (after applying the previous
>> stpe) do further?  It already is "using it to notify the user when
>> username and/or password is missing".
>
> not sure I follow, but the current code (as well as the one after patch 1)
> just silently ignores any credential that was missing username and password
> since the _gently version of the call will only really fail for a missing
> protocol.

Ah, I misread the intention of [1/1].  While I was rewriting the
if/else structure inside the loop, somehow I thought that we want to
warn when from_url_gently() fails or user/pass are missing, and in
this step, we are adding to the "warning-worthy" set an entry that
does not have either host or path.

Sorry about the confusion.

[PATCH v6] credential-store: warn instead of fatal for bogus lines from store

[Carlo Marcelo Arenas Belón]

with the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

instead of doing a hard check for credentials, do a soft one and
warn the user so any invalid entries could be corrected.

make sure that the credential to use is complete before calling
credential_match by confirming it has all fields set as expected
by the updated rules.

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v6:
* get rid of redacter and only use line number for context in warning
* make validation more strict to also catch incomplete credentials
* reorder check as suggested by Junio
v5:
* q_to_tab this round, with a single echo to make sure empty line
  is covered, as that seems to be a popular issue
* rebase on top of jc/credential-store-file-format-doc
* implement a redacter for credentials to use on errors to avoid
  leaking passwords
v4:
* use credential_from_url_gently instead as shown by Jonathan
* add documentation to clarify "comments" is not a supported feature
v3:
* avoid using q_to_cr as suggested by Peff
* a more verbose commit message and slightly more complete documentation
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 Documentation/git-credential-store.txt | 11 +++-
 credential-store.c                     | 26 +++++++--
 t/t0302-credential-store.sh            | 80 ++++++++++++++++++++++++++
 3 files changed, 111 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 76b0798856..122e238eb2 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -95,8 +95,15 @@ https://user:pass@example.com
 ------------------------------
 
 No other kinds of lines (e.g. empty lines or comment lines) are
-allowed in the file, even though some may be silently ignored. Do
-not view or edit the file with editors.
+allowed in the file, even though historically the parser was very
+lenient and some might had been silently ignored.
+
+Do not view or edit the file with editors as it could compromise the
+validity of your credentials by sometimes subtle formatting issues,
+like spaces, line wrapping or text encoding.
+
+An unparseable or otherwise invalid line is ignored, and a warning
+message points out the problematic line number and file it appears in.
 
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..4d3c9e8000 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -6,6 +6,15 @@
 
 static struct lock_file credential_lock;
 
+/*
+ * entry->protocol comes validated from credential_from_url_gently
+ */
+static int valid_credential(struct credential *entry)
+{
+	return (entry->username && entry->password &&
+		((entry->host && *entry->host) || entry->path));
+}
+
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
 				  void (*match_cb)(struct credential *),
@@ -15,6 +24,7 @@ static int parse_credential_file(const char *fn,
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
 	int found_credential = 0;
+	int lineno = 0;
 
 	fh = fopen(fn, "r");
 	if (!fh) {
@@ -24,16 +34,24 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
-		    credential_match(c, &entry)) {
+		lineno++;
+
+		if (credential_from_url_gently(&entry, line.buf, 1) ||
+			!valid_credential(&entry)) {
+			warning(_("%s:%d: ignoring invalid credential"),
+				fn, lineno);
+		}
+		else if (credential_match(c, &entry))
+		{
 			found_credential = 1;
 			if (match_cb) {
 				match_cb(&entry);
 				break;
 			}
+			continue;
 		}
-		else if (other_cb)
+
+		if (other_cb)
 			other_cb(&line);
 	}
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..3150f304cb 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,84 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'get: credentials without scheme are invalid' '
+	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials without valid host/path are invalid' '
+	echo "https://user:pass@" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials without username/password are invalid' '
+	echo "https://pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	echo "" > "$HOME/.git-credentials" &&
+	q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CONFIG
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	test_line_count = 3 stderr
+'
+
 test_done

base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0
-- 
2.26.2.569.g1d74ac4d14

Re: [PATCH v6] credential-store: warn instead of fatal for bogus lines from store

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> with the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported[1] failing to parse as valid credentials.

with -> With

> instead of doing a hard check for credentials, do a soft one and
> warn the user so any invalid entries could be corrected.

instead -> instead

> make sure that the credential to use is complete before calling
> credential_match by confirming it has all fields set as expected
> by the updated rules.

make -> Make

> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> index 76b0798856..122e238eb2 100644
> --- a/Documentation/git-credential-store.txt
> +++ b/Documentation/git-credential-store.txt
> @@ -95,8 +95,15 @@ https://user:pass@example.com
>  ------------------------------
>  
>  No other kinds of lines (e.g. empty lines or comment lines) are
> -allowed in the file, even though some may be silently ignored. Do
> -not view or edit the file with editors.
> +allowed in the file, even though historically the parser was very
> +lenient and some might had been silently ignored.
> +
> +Do not view or edit the file with editors as it could compromise the
> +validity of your credentials by sometimes subtle formatting issues,
> +like spaces, line wrapping or text encoding.

I do not think dropping "view or" is justifiable.  There is no need
to invite the risk of opening with the intention to only view and
then end up saving a modification.  In other words, do not encourage
use of an *editor* in any way.

> +
> +An unparseable or otherwise invalid line is ignored, and a warning
> +message points out the problematic line number and file it appears in.

OK.  You didn't want to tell them they can remove the problematic
line as a whole with their editor?

> diff --git a/credential-store.c b/credential-store.c
> index c010497cb2..4d3c9e8000 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -6,6 +6,15 @@
>  
>  static struct lock_file credential_lock;
>  
> +/*
> + * entry->protocol comes validated from credential_from_url_gently
> + */

Sure, but wouldn't it be simpler to do without such a comment and
check it also here, just as a belt-and-suspender safety?

> +static int valid_credential(struct credential *entry)
> +{
> +	return (entry->username && entry->password &&
> +		((entry->host && *entry->host) || entry->path));
> +}

> @@ -15,6 +24,7 @@ static int parse_credential_file(const char *fn,
>  	struct strbuf line = STRBUF_INIT;
>  	struct credential entry = CREDENTIAL_INIT;
>  	int found_credential = 0;
> +	int lineno = 0;
>  
>  	fh = fopen(fn, "r");
>  	if (!fh) {
> @@ -24,16 +34,24 @@ static int parse_credential_file(const char *fn,
>  	}
>  
>  	while (strbuf_getline_lf(&line, fh) != EOF) {
> +		lineno++;
> +
> +		if (credential_from_url_gently(&entry, line.buf, 1) ||
> +			!valid_credential(&entry)) {
> +			warning(_("%s:%d: ignoring invalid credential"),
> +				fn, lineno);
> +		}
> +		else if (credential_match(c, &entry))
> +		{

Style:  "} else if (...) {" on a single line.

>  			found_credential = 1;
>  			if (match_cb) {
>  				match_cb(&entry);
>  				break;
>  			}
> +			continue;
>  		}
> +
> +		if (other_cb)
>  			other_cb(&line);
>  	}

This got vastly easier to read, thanks to the additional helper.
Looking really good.

> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index d6b54e8c65..3150f304cb 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -120,4 +120,84 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
>  	test_must_be_empty "$HOME/.config/git/credentials"
>  '
>  
> +test_expect_success 'get: credentials without scheme are invalid' '
> +	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
> +	cat >expect-stdout <<-\STDOUT &&
> +	protocol=https
> +	host=example.com
> +	username=askpass-username
> +	password=askpass-password
> +	STDOUT
> +	test_config credential.helper store &&
> +	git credential fill <<-\EOF >stdout 2>stderr &&
> +	protocol=https
> +	host=example.com
> +	EOF
> +	test_cmp expect-stdout stdout &&
> +	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
> +	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&

These messages are passed from credential.c::credential_ask_one() to
git_prompt() without any i18n, so use of a bare "grep" is good.

> +	test_i18ngrep "ignoring invalid credential" stderr

And this is new message inside _(), so i18ngrep is good.

Nice to see such an attention to the details.


> +	test_config credential.helper store &&
> +	git credential fill <<-\EOF >stdout 2>stderr &&
> +	protocol=https
> +	host=example.com
> +	EOF
> +	test_cmp expect-stdout stdout &&
> +	test_i18ngrep "ignoring invalid credential" stderr &&
> +	test_line_count = 3 stderr

The "ignoring invalid credential" message could be translated into
two or more lines, but I think that is worrying too much about
theoretical possibility, so checking line count would probably be
sufficient.

Thanks.

Re: [PATCH v6] credential-store: warn instead of fatal for bogus lines from store

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +Do not view or edit the file with editors as it could compromise the
>> +validity of your credentials by sometimes subtle formatting issues,
>> +like spaces, line wrapping or text encoding.
>
> I do not think dropping "view or" is justifiable....

Sorry, this comment is now stale.

>> +An unparseable or otherwise invalid line is ignored, and a warning
>> +message points out the problematic line number and file it appears in.
>
> OK.  You didn't want to tell them they can remove the problematic
> line as a whole with their editor?

This one I do not care too deeply either way.  Probably it is
obvious to the readers that they can remove the lines (otherwise
there is no good reason to give warnings in the first place).

Re: [PATCH v6] credential-store: warn instead of fatal for bogus lines from store

[Carlo Marcelo Arenas Belón]

On Wed, Apr 29, 2020 at 04:47:31PM -0700, Junio C Hamano wrote:
> > instead of doing a hard check for credentials, do a soft one and
> > warn the user so any invalid entries could be corrected.
> 
> instead -> instead

;)

> I do not think dropping "view or" is justifiable.  There is no need
> to invite the risk of opening with the intention to only view and
> then end up saving a modification.  In other words, do not encourage
> use of an *editor* in any way.
> 
> > +An unparseable or otherwise invalid line is ignored, and a warning
> > +message points out the problematic line number and file it appears in.
> 
> OK.  You didn't want to tell them they can remove the problematic
> line as a whole with their editor?

someone said "do not encourage use of an *editor* in any way" just a few
lines before ;)

> > +	test_config credential.helper store &&
> > +	git credential fill <<-\EOF >stdout 2>stderr &&
> > +	protocol=https
> > +	host=example.com
> > +	EOF
> > +	test_cmp expect-stdout stdout &&
> > +	test_i18ngrep "ignoring invalid credential" stderr &&
> > +	test_line_count = 3 stderr
> 
> The "ignoring invalid credential" message could be translated into
> two or more lines, but I think that is worrying too much about
> theoretical possibility, so checking line count would probably be
> sufficient.

yes, and specially considering that if I even ended up adding a -c
option to test_i18ngrep as I intended originally we will still the
same issue.

what is the preferred way to do something like this, or is it better
to add a check for each line formatting issue this is handling?

Carlo

[PATCH v7] credential-store: warn instead of fatal for bogus lines from store

[Carlo Marcelo Arenas Belón]

With the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

Instead of doing a hard check for credentials, do a soft one and
warn the user so any invalid entries could be corrected.

Make sure that the credential to use is complete before calling
credential_match by confirming it has all fields set as expected
by the updated rules.

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v7:
* check for protocol in helper as suggested by Junio
v6:
* get rid of redacter and only use line number for context in warning
* make validation more strict to also catch incomplete credentials
* reorder check as suggested by Junio
v5:
* q_to_tab this round, with a single echo to make sure empty line
  is covered, as that seems to be a popular issue
* rebase on top of jc/credential-store-file-format-doc
* implement a redacter for credentials to use on errors to avoid
  leaking passwords
v4:
* use credential_from_url_gently instead as shown by Jonathan
* add documentation to clarify "comments" is not a supported feature
v3:
* avoid using q_to_cr as suggested by Peff
* a more verbose commit message and slightly more complete documentation
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 Documentation/git-credential-store.txt | 11 +++-
 credential-store.c                     | 22 +++++--
 t/t0302-credential-store.sh            | 80 ++++++++++++++++++++++++++
 3 files changed, 107 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 76b0798856..122e238eb2 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -95,8 +95,15 @@ https://user:pass@example.com
 ------------------------------
 
 No other kinds of lines (e.g. empty lines or comment lines) are
-allowed in the file, even though some may be silently ignored. Do
-not view or edit the file with editors.
+allowed in the file, even though historically the parser was very
+lenient and some might had been silently ignored.
+
+Do not view or edit the file with editors as it could compromise the
+validity of your credentials by sometimes subtle formatting issues,
+like spaces, line wrapping or text encoding.
+
+An unparseable or otherwise invalid line is ignored, and a warning
+message points out the problematic line number and file it appears in.
 
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..2c5f9736fc 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -6,6 +6,13 @@
 
 static struct lock_file credential_lock;
 
+static int valid_credential(struct credential *entry)
+{
+	return (entry->username && entry->password &&
+		entry->protocol && *entry->protocol &&
+		((entry->host && *entry->host) || entry->path));
+}
+
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
 				  void (*match_cb)(struct credential *),
@@ -15,6 +22,7 @@ static int parse_credential_file(const char *fn,
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
 	int found_credential = 0;
+	int lineno = 0;
 
 	fh = fopen(fn, "r");
 	if (!fh) {
@@ -24,16 +32,22 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
-		    credential_match(c, &entry)) {
+		lineno++;
+
+		if (credential_from_url_gently(&entry, line.buf, 1) ||
+			!valid_credential(&entry)) {
+			warning(_("%s:%d: ignoring invalid credential"),
+				fn, lineno);
+		} else if (credential_match(c, &entry)) {
 			found_credential = 1;
 			if (match_cb) {
 				match_cb(&entry);
 				break;
 			}
+			continue;
 		}
-		else if (other_cb)
+
+		if (other_cb)
 			other_cb(&line);
 	}
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..3150f304cb 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,84 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'get: credentials without scheme are invalid' '
+	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials without valid host/path are invalid' '
+	echo "https://user:pass@" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials without username/password are invalid' '
+	echo "https://pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	echo "" > "$HOME/.git-credentials" &&
+	q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CONFIG
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	test_line_count = 3 stderr
+'
+
 test_done

base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0
-- 
2.26.2.569.g1d74ac4d14

[PATCH v8] credential-store: warn instead of fatal for bogus lines from store

[Carlo Marcelo Arenas Belón]

With the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

Instead of doing a hard check for credentials, do a soft one and
therefore avoid the reported fatal error.

Warn the user indicating the filename and line number so any invalid
entries could be corrected but continue parsing until a match is
found or all valid credentials are processed.

Make sure that the credential that we will use to match is complete by
confirming it has all fields set as expected by the updated rules.

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v8:
* only warn during get operations as otherwise line number might be
  incorrect
v7:
* check for protocol in helper as suggested by Junio
v6:
* get rid of redacter and only use line number for context in warning
* make validation more strict to also catch incomplete credentials
* reorder check as suggested by Junio
v5:
* q_to_tab this round, with a single echo to make sure empty line
  is covered, as that seems to be a popular issue
* rebase on top of jc/credential-store-file-format-doc
* implement a redacter for credentials to use on errors to avoid
  leaking passwords
v4:
* use credential_from_url_gently instead as shown by Jonathan
* add documentation to clarify "comments" is not a supported feature
v3:
* avoid using q_to_cr as suggested by Peff
* a more verbose commit message and slightly more complete documentation
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 Documentation/git-credential-store.txt | 11 +++-
 credential-store.c                     | 31 +++++++--
 t/t0302-credential-store.sh            | 89 ++++++++++++++++++++++++++
 3 files changed, 123 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 76b0798856..d5841cffad 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -95,8 +95,15 @@ https://user:pass@example.com
 ------------------------------
 
 No other kinds of lines (e.g. empty lines or comment lines) are
-allowed in the file, even though some may be silently ignored. Do
-not view or edit the file with editors.
+allowed in the file, even though historically the parser was very
+lenient and some might had been silently ignored.
+
+Do not view or edit the file with editors as it could compromise the
+validity of your credentials by sometimes subtle formatting issues,
+like spaces, line wrapping or text encoding.
+
+An unparseable or otherwise invalid line is ignored, and a warning
+message points out the problematic file and line number it appears in.
 
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..9bd8ec9b67 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -4,10 +4,20 @@
 #include "string-list.h"
 #include "parse-options.h"
 
+#define PARSE_VERBOSE 0x01
+
 static struct lock_file credential_lock;
 
+static int valid_credential(struct credential *entry)
+{
+	return (entry->username && entry->password &&
+		entry->protocol && *entry->protocol &&
+		((entry->host && *entry->host) || entry->path));
+}
+
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
+				  int flags,
 				  void (*match_cb)(struct credential *),
 				  void (*other_cb)(struct strbuf *))
 {
@@ -15,6 +25,7 @@ static int parse_credential_file(const char *fn,
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
 	int found_credential = 0;
+	int lineno = 0;
 
 	fh = fopen(fn, "r");
 	if (!fh) {
@@ -24,16 +35,23 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
-		    credential_match(c, &entry)) {
+		lineno++;
+
+		if (credential_from_url_gently(&entry, line.buf, 1) ||
+			!valid_credential(&entry)) {
+			if (flags & PARSE_VERBOSE)
+				warning(_("%s:%d: ignoring invalid credential"),
+					fn, lineno);
+		} else if (credential_match(c, &entry)) {
 			found_credential = 1;
 			if (match_cb) {
 				match_cb(&entry);
 				break;
 			}
+			continue;
 		}
-		else if (other_cb)
+
+		if (other_cb)
 			other_cb(&line);
 	}
 
@@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
 		die_errno("unable to get credential storage lock");
 	if (extra)
 		print_line(extra);
-	parse_credential_file(fn, c, NULL, print_line);
+	parse_credential_file(fn, c, 0, NULL, print_line);
 	if (commit_lock_file(&credential_lock) < 0)
 		die_errno("unable to write credential store");
 }
@@ -139,7 +157,8 @@ static void lookup_credential(const struct string_list *fns, struct credential *
 	struct string_list_item *fn;
 
 	for_each_string_list_item(fn, fns)
-		if (parse_credential_file(fn->string, c, print_entry, NULL))
+		if (parse_credential_file(fn->string, c, PARSE_VERBOSE,
+						 print_entry, NULL))
 			return; /* Found credential */
 }
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..8bc89b7efb 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,93 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'store: store succeeds silently in corrupted file' '
+	echo "#comment" >"$HOME/.git-credentials" &&
+	check approve store <<-\EOF &&
+	url=https://user:pass@example.com
+	EOF
+	grep "https://user:pass@example.com" "$HOME/.git-credentials" &&
+	test_must_be_empty stderr
+'
+
+test_expect_success 'get: credentials without scheme are invalid' '
+	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials without valid host/path are invalid' '
+	echo "https://user:pass@" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials without username/password are invalid' '
+	echo "https://pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	echo "" > "$HOME/.git-credentials" &&
+	q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CONFIG
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	test_line_count = 3 stderr
+'
+
 test_done

base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0
-- 
2.26.2.2.g9c647e0d3b.dirty

[PATCH v9] credential-store: warn instead of fatal for bogus lines from store

[Carlo Marcelo Arenas Belón]

With the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

Instead of doing a hard check for credentials, do a soft one and
therefore avoid the reported fatal error.

Warn the user indicating the filename and line number so any invalid
entries could be corrected but continue parsing until a match is
found or all valid credentials are processed.

Make sure that the credential that we will use to match is complete by
confirming it has all fields set as expected by the updated rules.

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v9:
* use strbuf_getline() instead to better handle files with CRLF
v8:
* only warn during get operations as otherwise line number might be
  incorrect
v7:
* check for protocol in helper as suggested by Junio
v6:
* get rid of redacter and only use line number for context in warning
* make validation more strict to also catch incomplete credentials
* reorder check as suggested by Junio
v5:
* q_to_tab this round, with a single echo to make sure empty line
  is covered, as that seems to be a popular issue
* rebase on top of jc/credential-store-file-format-doc
* implement a redacter for credentials to use on errors to avoid
  leaking passwords
v4:
* use credential_from_url_gently instead as shown by Jonathan
* add documentation to clarify "comments" is not a supported feature
v3:
* avoid using q_to_cr as suggested by Peff
* a more verbose commit message and slightly more complete documentation
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 Documentation/git-credential-store.txt | 11 +++-
 credential-store.c                     | 33 ++++++++--
 t/t0302-credential-store.sh            | 90 +++++++++++++++++++++++++-
 3 files changed, 124 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 76b0798856..d5841cffad 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -95,8 +95,15 @@ https://user:pass@example.com
 ------------------------------
 
 No other kinds of lines (e.g. empty lines or comment lines) are
-allowed in the file, even though some may be silently ignored. Do
-not view or edit the file with editors.
+allowed in the file, even though historically the parser was very
+lenient and some might had been silently ignored.
+
+Do not view or edit the file with editors as it could compromise the
+validity of your credentials by sometimes subtle formatting issues,
+like spaces, line wrapping or text encoding.
+
+An unparseable or otherwise invalid line is ignored, and a warning
+message points out the problematic file and line number it appears in.
 
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..b1f5b2dea6 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -4,10 +4,20 @@
 #include "string-list.h"
 #include "parse-options.h"
 
+#define PARSE_VERBOSE 0x01
+
 static struct lock_file credential_lock;
 
+static int valid_credential(struct credential *entry)
+{
+	return (entry->username && entry->password &&
+		entry->protocol && *entry->protocol &&
+		((entry->host && *entry->host) || entry->path));
+}
+
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
+				  int flags,
 				  void (*match_cb)(struct credential *),
 				  void (*other_cb)(struct strbuf *))
 {
@@ -15,6 +25,7 @@ static int parse_credential_file(const char *fn,
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
 	int found_credential = 0;
+	int lineno = 0;
 
 	fh = fopen(fn, "r");
 	if (!fh) {
@@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn,
 		return found_credential;
 	}
 
-	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
-		    credential_match(c, &entry)) {
+	while (strbuf_getline(&line, fh) != EOF) {
+		lineno++;
+
+		if (credential_from_url_gently(&entry, line.buf, 1) ||
+			!valid_credential(&entry)) {
+			if (flags & PARSE_VERBOSE)
+				warning(_("%s:%d: ignoring invalid credential"),
+					fn, lineno);
+		} else if (credential_match(c, &entry)) {
 			found_credential = 1;
 			if (match_cb) {
 				match_cb(&entry);
 				break;
 			}
+			continue;
 		}
-		else if (other_cb)
+
+		if (other_cb)
 			other_cb(&line);
 	}
 
@@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
 		die_errno("unable to get credential storage lock");
 	if (extra)
 		print_line(extra);
-	parse_credential_file(fn, c, NULL, print_line);
+	parse_credential_file(fn, c, 0, NULL, print_line);
 	if (commit_lock_file(&credential_lock) < 0)
 		die_errno("unable to write credential store");
 }
@@ -139,7 +157,8 @@ static void lookup_credential(const struct string_list *fns, struct credential *
 	struct string_list_item *fn;
 
 	for_each_string_list_item(fn, fns)
-		if (parse_credential_file(fn->string, c, print_entry, NULL))
+		if (parse_credential_file(fn->string, c, PARSE_VERBOSE,
+						 print_entry, NULL))
 			return; /* Found credential */
 }
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..05fd2785b9 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
-
 test_expect_success 'erase: erase matching credentials from both xdg and home files' '
 	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
 	mkdir -p "$HOME/.config/git" &&
@@ -120,4 +119,93 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'get: credentials without scheme are invalid' '
+	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials without valid host/path are invalid' '
+	echo "https://user:pass@" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials without username/password are invalid' '
+	echo "https://pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	echo "" > "$HOME/.git-credentials" &&
+	q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CONFIG
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	test_line_count = 3 stderr
+'
+
+test_expect_success 'store: store succeeds silently in corrupted file' '
+	echo "#comment" >"$HOME/.git-credentials" &&
+	check approve store <<-\EOF &&
+	url=https://user:pass@example.com
+	EOF
+	grep "https://user:pass@example.com" "$HOME/.git-credentials" &&
+	test_must_be_empty stderr
+'
+
 test_done

base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0
-- 
2.26.2.2.gf219a08795

Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> With the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported[1] failing to parse as valid credentials.
>
> Instead of doing a hard check for credentials, do a soft one and
> therefore avoid the reported fatal error.
>
> Warn the user indicating the filename and line number so any invalid
> entries could be corrected but continue parsing until a match is
> found or all valid credentials are processed.
>
> Make sure that the credential that we will use to match is complete by
> confirming it has all fields set as expected by the updated rules.
>
> [1] https://stackoverflow.com/a/61420852/5005936
>
> Reported-by: Dirk <dirk@ed4u.de>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---

Looks good.

> diff --git a/credential-store.c b/credential-store.c
> index c010497cb2..b1f5b2dea6 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -4,10 +4,20 @@
>  #include "string-list.h"
>  #include "parse-options.h"
>  
> +#define PARSE_VERBOSE 0x01
> +
>  static struct lock_file credential_lock;
>  
> +static int valid_credential(struct credential *entry)
> +{
> +	return (entry->username && entry->password &&
> +		entry->protocol && *entry->protocol &&
> +		((entry->host && *entry->host) || entry->path));
> +}

OK.

>  static int parse_credential_file(const char *fn,
>  				  struct credential *c,
> +				  int flags,
>  				  void (*match_cb)(struct credential *),
>  				  void (*other_cb)(struct strbuf *))
>  {
> @@ -15,6 +25,7 @@ static int parse_credential_file(const char *fn,
>  	struct strbuf line = STRBUF_INIT;
>  	struct credential entry = CREDENTIAL_INIT;
>  	int found_credential = 0;
> +	int lineno = 0;
>  
>  	fh = fopen(fn, "r");
>  	if (!fh) {
> @@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn,
>  		return found_credential;
>  	}
>  
> -	while (strbuf_getline_lf(&line, fh) != EOF) {
> -		credential_from_url(&entry, line.buf);
> -		if (entry.username && entry.password &&
> -		    credential_match(c, &entry)) {
> +	while (strbuf_getline(&line, fh) != EOF) {

Hmph, I probably have missed some discussion that happened in the
last few days, but from the use of _lf() in the original, I sense
that it is very much deliberate that the file format is designed to
be LF delimited records, *not* a text file in which each line is
terminated with some end-of-line marker that is platform dependent.
IOW, an explicit use of _lf() shouts, at least to me, that we want a
sequence of LF terminated records even on Windows and Macs.

So, I am not sure why this change is desirable.

> +		lineno++;
> +
> +		if (credential_from_url_gently(&entry, line.buf, 1) ||
> +			!valid_credential(&entry)) {

OK, so we read a line, fed it to the parser, and if we had trouble
parsing or the line did not have enough credential material, we
discard and warn (when told to).

> +			if (flags & PARSE_VERBOSE)
> +				warning(_("%s:%d: ignoring invalid credential"),
> +					fn, lineno);
> +		} else if (credential_match(c, &entry)) {
>  			found_credential = 1;
>  			if (match_cb) {
>  				match_cb(&entry);
>  				break;
>  			}
> +			continue;

And if the credential material on a good line matches, we remember
we saw a match.  If there is match callback, we call it and leave
the loop to return from the function.  Otherwise we go to the next
line.
>  		}
> +
> +		if (other_cb)
>  			other_cb(&line);

A malformed/underspecified line and an unmatched line is fed to the
other callback.

>  	}
>  
> @@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
>  		die_errno("unable to get credential storage lock");
>  	if (extra)
>  		print_line(extra);
> -	parse_credential_file(fn, c, NULL, print_line);
> +	parse_credential_file(fn, c, 0, NULL, print_line);

This helper function is called from two places.

 - In store_credential_file, we write the credential we were asked
   to store to a strbuf, and then call this function.  The function
   first writes that new credential and then calls the parse helper
   without PARSE_VERBOSE.  We do not give any match callback, and
   other callback is to just print the line read from the credential
   file.  So, the final output would be the new credential, followed
   by the current contents of the credential file except for the
   records that match the one we are storing.  We do this without
   warning about malformed entries at all.

 - In remove_credential, we go over multiple files and copy the
   lines in each of them, except the ones that match the credential
   we are given.  We also do this without warning about malformed
   entries at all.

>  	if (commit_lock_file(&credential_lock) < 0)
>  		die_errno("unable to write credential store");
>  }
> @@ -139,7 +157,8 @@ static void lookup_credential(const struct string_list *fns, struct credential *
>  	struct string_list_item *fn;
>  
>  	for_each_string_list_item(fn, fns)
> -		if (parse_credential_file(fn->string, c, print_entry, NULL))
> +		if (parse_credential_file(fn->string, c, PARSE_VERBOSE,
> +						 print_entry, NULL))
>  			return; /* Found credential */
>  }

This is triggered by the "get" operation.  We read until we hit the
entry we are looking for, at that point the match-callback will
print out "username=... / password=..." lines and we ignore the
remainder of the file.  While we look for the first matching
credential, we do warn about malformed or underspecified lines that
we are ignoring, but because we leave the loop once a matching line
is found, bad lines that come after it won't be even looked at to be
warned.

Validating and warning about bad entries is *not* the main purpose
of the "credential-store" program, so I fully agree with the design
of the "get" part.  I am not so sure about the other two operations
(i.e. "store" and "erase") that do scan all the entries and has
chance to warn about bad ones, though (note: I am not saying that we
should parse verbosely---it is just that I do not know why you chose
not to and I am not convinced that it is a good idea not to warn).

The tests looked good.

Thanks.

Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

[part of the commit log message]

>> Warn the user indicating the filename and line number so any invalid
>> entries could be corrected but continue parsing until a match is
>> found or all valid credentials are processed.

We should say "Do so only during the 'get' operation; give up giving
any warning for 'erase' or 'store' operation, as keeping track of
the line number of the input file, while having to issue a warning
that has the line number of the output file, is too hard for us" or
something like that here.

I suspect that it might not be "too hard", but we'd need to rename
other_cb to a more specific name first and limit the possibility of
repurposing parse_credential_file().  

For example, other_cb can become "copy_cb", we declare that the
function works in two (and no other) modes, one is to look-up (which
is read-only so we report the input line number in our warning
messges) and the other is to copy-out-with-filtering (which will
leave a file that is different from the input, so we report the
output line number).

To support the copy-out-wiht-filtering mode, we pass the starting
line number into the function (i.e. 'store' may store a new line, so
the first line copied out to the file may be the second line in the
output), and count the output lines there:

	if (other_cb) {
		lines++;
		other_cb(&line);
	}

and of course we won't increment lines++ when we saw a match in the
copy-out-with-filtering mode.  In look-up mode (i.e. copy_cb == NULL),
we count the input lines just like your code.

But I suspect that it may not be worth doing.  But if we decide not
to do so, we should document why we chose (not) to in the commit log
message.

> Validating and warning about bad entries is *not* the main purpose
> of the "credential-store" program, so I fully agree with the design
> of the "get" part.  I am not so sure about the other two operations
> (i.e. "store" and "erase") that do scan all the entries and has
> chance to warn about bad ones, though (note: I am not saying that we
> should parse verbosely---it is just that I do not know why you chose
> not to and I am not convinced that it is a good idea not to warn).

I re-read the incremental log for v8 and found your explanation.

Thanks.

Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store

[Carlo Marcelo Arenas Belón]

On Thu, Apr 30, 2020 at 01:21:06PM -0700, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> > @@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn,
> >  		return found_credential;
> >  	}
> >  
> > -	while (strbuf_getline_lf(&line, fh) != EOF) {
> > -		credential_from_url(&entry, line.buf);
> > -		if (entry.username && entry.password &&
> > -		    credential_match(c, &entry)) {
> > +	while (strbuf_getline(&line, fh) != EOF) {
> 
> Hmph, I probably have missed some discussion that happened in the
> last few days, but from the use of _lf() in the original, I sense
> that it is very much deliberate that the file format is designed to
> be LF delimited records, *not* a text file in which each line is
> terminated with some end-of-line marker that is platform dependent.
> IOW, an explicit use of _lf() shouts, at least to me, that we want a
> sequence of LF terminated records even on Windows and Macs.

apologize for the confusion, the only discussion was the one I had
with myself late at night when I realized that specific corruption
was not being detected, and might be related to issues that seemed
to be common[1]

the problem is that practically speaking, if users in Windows and Macs
edited the file they "might" had saved lines with the wrong line ending (*)
(part of the reason I added a warning about "encoding" to the documentation),
and those are difficult to "see" as invalid.

using the non _lf() version ensures any trailing '\r' character would
get removed from the credential and allow us to use them, instead of
silently failing.

originally I added a explicit check for it, which I assume you would prefer
but .. 

> So, I am not sure why this change is desirable.

to give users immediate relief from what seems to be a commonly seen issue.

IMHO after we get this released out and they see the updated documentation
explaining the risks, and some learn how to fix their credential files (
and hopefully use an editor that lets them see the problem); we could then
add also a detection for this edge case and go back to _lf()

I also had to admit I might had overreacted, as I was testing before v8 with
a very corrupted file and was seeing warnings twice on each operation and
somehow even got myself into the original fatal, which I had to admit I can't
replicate now after some needed rest.

Carlo

[1] https://github.com/desktop/desktop/issues/9597

(*) textedit, which comes with macOS doesn't even write an EOF record when
    creating files, for example, and that behaviour seems to be common for
    most other native editors but a credential line WITHOUT line ending works

Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> the problem is that practically speaking, if users in Windows and Macs
> edited the file they "might" had saved lines with the wrong line ending (*)
> (part of the reason I added a warning about "encoding" to the documentation),
> and those are difficult to "see" as invalid.
>
> using the non _lf() version ensures any trailing '\r' character would
> get removed from the credential and allow us to use them, instead of
> silently failing.

You are forgetting why we are fixing credential-store, aren't you?

It is primarily to help those who damaged their files by editing,
and introducing cruft that cannot be parsed, from a stricter parsing
introduced recently in 2.17.4 and above.  Without the fix, they
cannot operate with the store they already have.

Now, think about those users who saved their file, after adding CR
at the end of each line, but didn't do any other edit (like adding a
blank line or "# comment").  It may have happened 3 years ago.  What
did they see from the system back then?  It may have happened 3
minutes ago.  What would they see with the stricter parser now?

With or without the recent parser change, they would have seen that
these lines no longer match the URLs they wanted to match, but the
credential store does not die on them for malformed lines, no?

In other words, the stricter parsing did nothing to these users.

In fact, those users who added CR at the end of each line 3 years
ago may have even depended on the disappearance of these entries for
all these years.  Maybe lines that record their ancient passwords
for sites are still buried in the later parts of the file, with CR,
but doing no harm because these lines do not match anything.  These
users may have changed their password since then and wrote new
records with "credential store", and these new records are stored
without CR at the end of the line, so they match the URLs.

By using the non _lf() variant, you are suddenly resurrecting these
old records that the users thought are already gone and have been
causing no harm to them.  Do we know that resurrecting these old
records is a good thing to do?  I don't.  For example, once the user
decides to "sort" the file (after all, we are talking about users
who edit the file, so we cannot assume they won't do so), they would
end up with duplicate records that record two passwords to the same
site and they cannot tell which one is current, as you even lost the
CR at the end of line that would have told you which ones are
broken.

In short, you wouldn't know what ramification it has by suddenly
using non _lf() variant.  And it has nothing to do with the fix we
are trying to make.

When fixing something, it is tempting to see if you can cover a bit
more area with the same change.  But you should make it a habit to
stick to the absolute minimum first for safety.  When contemplating
to step out of the absolute minimum, you need to think twice to see
if that extra "fix" really fixes, or if it potentially harms.

And I am not convinced that turning CRLF into LF in this case is a
good change.  In any case, it certainly does not belong to the same
commit as the one that fixes the fallout from stricter parser
introduced in 2.17.4 and above.

Thanks.

Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store

[Carlo Arenas]

On Thu, Apr 30, 2020 at 6:40 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
>
> > the problem is that practically speaking, if users in Windows and Macs
> > edited the file they "might" had saved lines with the wrong line ending (*)
> > (part of the reason I added a warning about "encoding" to the documentation),
> > and those are difficult to "see" as invalid.
> >
> > using the non _lf() version ensures any trailing '\r' character would
> > get removed from the credential and allow us to use them, instead of
> > silently failing.
>
> You are forgetting why we are fixing credential-store, aren't you?

indeed, and thanks for the clarification.

you are correct this was a bad idea and is better to warn them instead
(even if only
during get operations and for the lines that were processed) than my "solution".

FWIW though, my change wasn't going to change the file on disk but only allow
the line to be processed.

gave up already on sneakily "fixing" corruption issues after Peff
called me on it
and yes, another version you will never see had a PARSER_TRIM flag added ;)

Carlo

PS. should we really do the warn even in store/erase operations as a
followup or is the warning not useful as is, and probably then all
operations should be quiet (as Jonathan suggested originally?) and we
could do warn (and maybe fix) in a different change (maybe adding a
fsck command of sorts)?

> When fixing something, it is tempting to see if you can cover a bit
> more area with the same change.  But you should make it a habit to
> stick to the absolute minimum first for safety.  When contemplating
> to step out of the absolute minimum, you need to think twice to see
> if that extra "fix" really fixes, or if it potentially harms.

Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> PS. should we really do the warn even in store/erase operations as a
> followup or is the warning not useful as is, and probably then all
> operations should be quiet (as Jonathan suggested originally?) and we
> could do warn (and maybe fix) in a different change (maybe adding a
> fsck command of sorts)?

Yeah, I think I like the "no warning, just ignore" much better.  The
implementation I suspect would become a lot simpler, right?

Thanks for thinking one extra step.

Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store

[Carlo Marcelo Arenas Belón]

On Thu, Apr 30, 2020 at 10:27:24PM -0700, Junio C Hamano wrote:
> Carlo Arenas <carenas@gmail.com> writes:
> 
> > PS. should we really do the warn even in store/erase operations as a
> > followup or is the warning not useful as is, and probably then all
> > operations should be quiet (as Jonathan suggested originally?) and we
> > could do warn (and maybe fix) in a different change (maybe adding a
> > fsck command of sorts)?
> 
> Yeah, I think I like the "no warning, just ignore" much better.  The
> implementation I suspect would become a lot simpler, right?

sure, it will be basically the version that Jonathan suggested[1] on
top of yours and that I included in v4 but that was missing a SOB.

Jonathan,

would you want to submit it formally?, IMHO better if with base (from pu): 
272281efcc (credential-store: document the file format a bit more, 2020-04-27)

it might be still worth including some of the test cases, so I could do it
instead too; but let me know what would be your preference otherwise, as they
will need to be wrapped, probably better than on what became my confusing
attempt to save everyone's time with the v4 series[2], and that was made
obsolete by Junio's creation of jc/credential-store-file-format-doc.

Carlo

[1] https://lore.kernel.org/git/20200428052510.GA201501@google.com/
[2] https://lore.kernel.org/git/20200428104858.28573-1-carenas@gmail.com/

CC: trim to avoid spam, probably long overdue

Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> attempt to save everyone's time with the v4 series[2], and that was made
> obsolete by Junio's creation of jc/credential-store-file-format-doc.

I actually discarded that "file format only" patch after you
included in your series.  One fewer patch I have to keep track of is
always better ;-)

Thanks.

[RFC PATCH v10] credential-store: warn/ignore for bogus lines from store file

[Carlo Marcelo Arenas Belón]

With the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

Instead of doing a hard check for credentials, do a soft one and
therefore avoid the reported fatal error.

Warn the user indicating the filename and line number during get,
for all entries that were processed, but continue parsing until
a match is found or all valid credentials are processed.

Warning during store and erase operations has been suppressed
for now by adding a PARSER_VERBOSE flag that is only enabled
during get operations.

Make sure that the credential that we will use to match is complete by
confirming it has all fields set as expected by the updated rules.

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v10:
* insted of trying to use broken credentials in DOS format, make
  sure they are flagged as broken
v9:
* use strbuf_getline() instead to better handle files with CRLF
v8:
* only warn during get operations as otherwise line number might be
  incorrect
v7:
* check for protocol in helper as suggested by Junio
v6:
* get rid of redacter and only use line number for context in warning
* make validation more strict to also catch incomplete credentials
* reorder check as suggested by Junio
v5:
* q_to_tab this round, with a single echo to make sure empty line
  is covered, as that seems to be a popular issue
* rebase on top of jc/credential-store-file-format-doc
* implement a redacter for credentials to use on errors to avoid
  leaking passwords
v4:
* use credential_from_url_gently instead as shown by Jonathan
* add documentation to clarify "comments" is not a supported feature
v3:
* avoid using q_to_cr as suggested by Peff
* a more verbose commit message and slightly more complete documentation
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 Documentation/git-credential-store.txt |  18 +++-
 credential-store.c                     |  32 ++++++--
 t/t0302-credential-store.sh            | 109 ++++++++++++++++++++++++-
 3 files changed, 150 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 76b0798856..18ba8b6984 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -95,8 +95,22 @@ https://user:pass@example.com
 ------------------------------
 
 No other kinds of lines (e.g. empty lines or comment lines) are
-allowed in the file, even though some may be silently ignored. Do
-not view or edit the file with editors.
+allowed in the file, even though historically the parser was very
+lenient and some might had been silently ignored.
+
+Do not view or edit the file with editors as it could compromise the
+validity of your credentials by sometimes subtle formatting issues,
+like spaces, line wrapping or text encoding.
+
+An unparseable or otherwise invalid line is ignored, and a warning
+message points out the problematic file and line number it appears in.
+
+Note that because of deficiencies of the current implementation, these
+warnings will be only generated for the entries that were processed
+while reading the credential files during a get operation, and so they
+are not comprehensive, and will require you to use an editor (or
+better a sed/awk/perl one liner) to edit the credential file and remove
+them.
 
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..c2778eff8a 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -4,10 +4,20 @@
 #include "string-list.h"
 #include "parse-options.h"
 
+#define PARSE_VERBOSE 0x01
+
 static struct lock_file credential_lock;
 
+static int valid_credential(struct credential *entry)
+{
+	return (entry->username && entry->password &&
+		entry->protocol && *entry->protocol &&
+		((entry->host && *entry->host) || entry->path));
+}
+
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
+				  int flags,
 				  void (*match_cb)(struct credential *),
 				  void (*other_cb)(struct strbuf *))
 {
@@ -15,6 +25,7 @@ static int parse_credential_file(const char *fn,
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
 	int found_credential = 0;
+	int lineno = 0;
 
 	fh = fopen(fn, "r");
 	if (!fh) {
@@ -24,16 +35,24 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
-		    credential_match(c, &entry)) {
+		lineno++;
+
+		if (strchr(line.buf, '\r') ||
+			credential_from_url_gently(&entry, line.buf, 1) ||
+			!valid_credential(&entry)) {
+			if (flags & PARSE_VERBOSE)
+				warning(_("%s:%d: ignoring invalid credential"),
+					fn, lineno);
+		} else if (credential_match(c, &entry)) {
 			found_credential = 1;
 			if (match_cb) {
 				match_cb(&entry);
 				break;
 			}
+			continue;
 		}
-		else if (other_cb)
+
+		if (other_cb)
 			other_cb(&line);
 	}
 
@@ -62,7 +81,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
 		die_errno("unable to get credential storage lock");
 	if (extra)
 		print_line(extra);
-	parse_credential_file(fn, c, NULL, print_line);
+	parse_credential_file(fn, c, 0, NULL, print_line);
 	if (commit_lock_file(&credential_lock) < 0)
 		die_errno("unable to write credential store");
 }
@@ -139,7 +158,8 @@ static void lookup_credential(const struct string_list *fns, struct credential *
 	struct string_list_item *fn;
 
 	for_each_string_list_item(fn, fns)
-		if (parse_credential_file(fn->string, c, print_entry, NULL))
+		if (parse_credential_file(fn->string, c, PARSE_VERBOSE,
+						 print_entry, NULL))
 			return; /* Found credential */
 }
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..00608e365a 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
-
 test_expect_success 'erase: erase matching credentials from both xdg and home files' '
 	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
 	mkdir -p "$HOME/.config/git" &&
@@ -120,4 +119,112 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'get: credentials without scheme are invalid' '
+	echo "://user:pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials without valid host/path are invalid' '
+	echo "https://user:pass@" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials without username/password are invalid' '
+	echo "https://pass@example.com" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: credentials with DOS line endings are invalid' '
+	printf "https://user:pass@example.com\r\n" >"$HOME/.git-credentials" &&
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	grep "askpass: Username for '\''https://example.com'\'':" stderr &&
+	grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr &&
+	test_i18ngrep "ignoring invalid credential" stderr
+'
+
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	echo "" > "$HOME/.git-credentials" &&
+	q_to_tab <<-\CONFIG >>"$HOME/.git-credentials" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CONFIG
+	cat >expect-stdout <<-\STDOUT &&
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	STDOUT
+	test_config credential.helper store &&
+	git credential fill <<-\EOF >stdout 2>stderr &&
+	protocol=https
+	host=example.com
+	EOF
+	test_cmp expect-stdout stdout &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	test_line_count = 3 stderr
+'
+
+test_expect_success 'store: store succeeds silently in corrupted file' '
+	echo "#comment" >"$HOME/.git-credentials" &&
+	check approve store <<-\EOF &&
+	url=https://user:pass@example.com
+	EOF
+	grep "https://user:pass@example.com" "$HOME/.git-credentials" &&
+	test_must_be_empty stderr
+'
+
 test_done

base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0
-- 
2.26.2.2.gf219a08795

[RFC PATCH v10 2/1] credential-store: warn also for store and erase

[Carlo Marcelo Arenas Belón]

done in a hacky way and only as a POC, but at least works, if we
are still thinking that warning should be at least comprehensive
for the first iteration.

didn't change the callback as you suggested but I think this
doesn't look that bad either.

squash for final v10 or ..?
---
 Documentation/git-credential-store.txt |  7 -------
 credential-store.c                     | 19 +++++++++----------
 t/t0302-credential-store.sh            | 20 +++++++++++++++++---
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 18ba8b6984..d5841cffad 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -105,13 +105,6 @@ like spaces, line wrapping or text encoding.
 An unparseable or otherwise invalid line is ignored, and a warning
 message points out the problematic file and line number it appears in.
 
-Note that because of deficiencies of the current implementation, these
-warnings will be only generated for the entries that were processed
-while reading the credential files during a get operation, and so they
-are not comprehensive, and will require you to use an editor (or
-better a sed/awk/perl one liner) to edit the credential file and remove
-them.
-
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
 each entry in the credentials file.  If the protocol, hostname, and
diff --git a/credential-store.c b/credential-store.c
index c2778eff8a..236df8e7bf 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -4,8 +4,6 @@
 #include "string-list.h"
 #include "parse-options.h"
 
-#define PARSE_VERBOSE 0x01
-
 static struct lock_file credential_lock;
 
 static int valid_credential(struct credential *entry)
@@ -17,7 +15,7 @@ static int valid_credential(struct credential *entry)
 
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
-				  int flags,
+				  int initial_line,
 				  void (*match_cb)(struct credential *),
 				  void (*other_cb)(struct strbuf *))
 {
@@ -25,7 +23,7 @@ static int parse_credential_file(const char *fn,
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
 	int found_credential = 0;
-	int lineno = 0;
+	int lineno = initial_line;
 
 	fh = fopen(fn, "r");
 	if (!fh) {
@@ -40,8 +38,7 @@ static int parse_credential_file(const char *fn,
 		if (strchr(line.buf, '\r') ||
 			credential_from_url_gently(&entry, line.buf, 1) ||
 			!valid_credential(&entry)) {
-			if (flags & PARSE_VERBOSE)
-				warning(_("%s:%d: ignoring invalid credential"),
+			warning(_("%s:%d: ignoring invalid credential"),
 					fn, lineno);
 		} else if (credential_match(c, &entry)) {
 			found_credential = 1;
@@ -77,11 +74,14 @@ static void print_line(struct strbuf *buf)
 static void rewrite_credential_file(const char *fn, struct credential *c,
 				    struct strbuf *extra)
 {
+	int initial_line = 0;
 	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
 		die_errno("unable to get credential storage lock");
-	if (extra)
+	if (extra) {
 		print_line(extra);
-	parse_credential_file(fn, c, 0, NULL, print_line);
+		initial_line++;
+	}
+	parse_credential_file(fn, c, initial_line, NULL, print_line);
 	if (commit_lock_file(&credential_lock) < 0)
 		die_errno("unable to write credential store");
 }
@@ -158,8 +158,7 @@ static void lookup_credential(const struct string_list *fns, struct credential *
 	struct string_list_item *fn;
 
 	for_each_string_list_item(fn, fns)
-		if (parse_credential_file(fn->string, c, PARSE_VERBOSE,
-						 print_entry, NULL))
+		if (parse_credential_file(fn->string, c, 0, print_entry, NULL))
 			return; /* Found credential */
 }
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 00608e365a..78010590f4 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -218,13 +218,27 @@ test_expect_success 'get: store file can contain empty/bogus lines' '
 	test_line_count = 3 stderr
 '
 
-test_expect_success 'store: store succeeds silently in corrupted file' '
+test_expect_success 'store: succeeds in corrupted file but correctly warn' '
 	echo "#comment" >"$HOME/.git-credentials" &&
-	check approve store <<-\EOF &&
+	test_config credential.helper store &&
+	git credential approve <<-\EOF &&
 	url=https://user:pass@example.com
 	EOF
 	grep "https://user:pass@example.com" "$HOME/.git-credentials" &&
-	test_must_be_empty stderr
+	test_i18ngrep "git-credentials:2: ignoring invalid credential" stderr
+'
+
+test_expect_success 'erase: succeeds in corrupted file but correctly warn' '
+	cat <<-\CONFIG >"$HOME/.git-credentials" &&
+	#comment
+	https://user:pass@example.com
+	CONFIG
+	test_config credential.helper store &&
+	git credential reject <<-\EOF &&
+	url=https://example.com
+	EOF
+	! grep "https://user:pass@example.com" "$HOME/.git-credentials" &&
+	test_i18ngrep "git-credentials:1: ignoring invalid credential" stderr
 '
 
 test_done
-- 
2.26.2.3.g90bd2f7136

Re: [RFC PATCH v10 2/1] credential-store: warn also for store and erase

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> done in a hacky way and only as a POC, but at least works, if we
> are still thinking that warning should be at least comprehensive
> for the first iteration.

...

> @@ -25,7 +23,7 @@ static int parse_credential_file(const char *fn,
>  	struct strbuf line = STRBUF_INIT;
>  	struct credential entry = CREDENTIAL_INIT;
>  	int found_credential = 0;
> -	int lineno = 0;
> +	int lineno = initial_line;
>  
>  	fh = fopen(fn, "r");
>  	if (!fh) {
> @@ -40,8 +38,7 @@ static int parse_credential_file(const char *fn,
>  		if (strchr(line.buf, '\r') ||
>  			credential_from_url_gently(&entry, line.buf, 1) ||
>  			!valid_credential(&entry)) {
> -			if (flags & PARSE_VERBOSE)
> -				warning(_("%s:%d: ignoring invalid credential"),
> +			warning(_("%s:%d: ignoring invalid credential"),
>  					fn, lineno);
>  		} else if (credential_match(c, &entry)) {
>  			found_credential = 1;

I am not sure how this would correctly count when "store" and
"erase" is used.  During "store", the initial_line may let us
correctly count the new line, and as long as we keep copying, our
output line number would stay to be correct, as we increment line by
one every time we read in, but as soon as we see the older version
of the credential record this "store" is updating, i.e. when the
credential_match() reports a match, we will discard the line we read
by hitting the "continue" and not calling other_cb().  We increment
line, but we don't output a line when that happens, so any warning
after that point will be out of sync, no?  And "erase" is the same
deal.  It behaves the same way as "store" except that "erase" does
not emit a replacement record at the beginning.  As soon as the
matching record from the input is dropped, our line number will be
out of sync.

In any case, I think it probably is a good idea to start with a
version that ignores without warning, and I had an impression that
you were on the same page from reading your response to v9 review,
so...

[PATCH v10] credential-store: ignore bogus lines from store file

[Carlo Marcelo Arenas Belón]

With the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

Instead of doing a hard check for credentials, do a soft one and
therefore avoid the reported fatal error.

As a special case, flag files with CRLF endings as invalid early
to prevent current problems in credential_from_url_gently() with
handling of '\r' in the host.

While at it add tests for all known corruptions that are currently
ignored to keep track of them and avoid the risk of regressions.

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v10:
* go back to v4 but with better testing and commit message
* make sure broken CR characters are ignored early
v9:
* use strbuf_getline() instead to better handle files with CRLF
v8:
* only warn during get operations as otherwise line number might be
  incorrect
v7:
* check for protocol in helper as suggested by Junio
v6:
* get rid of redacter and only use line number for context in warning
* make validation more strict to also catch incomplete credentials
* reorder check as suggested by Junio
v5:
* q_to_tab this round, with a single echo to make sure empty line
  is covered, as that seems to be a popular issue
* rebase on top of jc/credential-store-file-format-doc
* implement a redacter for credentials to use on errors to avoid
  leaking passwords
v4:
* use credential_from_url_gently instead as shown by Jonathan
* add documentation to clarify "comments" is not a supported feature
v3:
* avoid using q_to_cr as suggested by Peff
* a more verbose commit message and slightly more complete documentation
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 credential-store.c          |  5 ++--
 t/t0302-credential-store.sh | 60 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/credential-store.c b/credential-store.c
index c010497cb2..fdfb81e632 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -24,8 +24,9 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
+		if (strchr(line.buf, '\r') == NULL &&
+		    !credential_from_url_gently(&entry, line.buf, 1) &&
+		    entry.username && entry.password &&
 		    credential_match(c, &entry)) {
 			found_credential = 1;
 			if (match_cb) {
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..9fd0aca55e 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
-
 test_expect_success 'erase: erase matching credentials from both xdg and home files' '
 	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
 	mkdir -p "$HOME/.config/git" &&
@@ -120,4 +119,63 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+invalid_credential_test() {
+	test_expect_success 'get: ignore credentials without $1 as invalid' '
+		echo "$2" >"$HOME/.git-credentials" &&
+		check fill store <<-\EOF
+		protocol=https
+		host=example.com
+		--
+		protocol=https
+		host=example.com
+		username=askpass-username
+		password=askpass-password
+		--
+		askpass: Username for '\''https://example.com'\'':
+		askpass: Password for '\''https://askpass-username@example.com'\'':
+		--
+		EOF
+	'
+}
+
+invalid_credential_test "scheme" ://user:pass@example.com
+invalid_credential_test "valid host/path" https://user:pass@
+invalid_credential_test "username/password" https://pass@example.com
+
+test_expect_success 'get: credentials with DOS line endings are invalid' '
+	printf "https://user:pass@example.com\r\n" >"$HOME/.git-credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	--
+	askpass: Username for '\''https://example.com'\'':
+	askpass: Password for '\''https://askpass-username@example.com'\'':
+	--
+	EOF
+'
+
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	echo "" >"$HOME/.git-credentials" &&
+	q_to_tab <<-\CREDENTIAL >>"$HOME/.git-credentials" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CREDENTIAL
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	--
+	EOF
+'
+
 test_done

base-commit: 49458fb91d61461938881559ce23abbb1a2f8c35
-- 
2.26.2.686.gfaf46a9ccd

Re: [PATCH v10] credential-store: ignore bogus lines from store file

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> With the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported[1] failing to parse as valid credentials.
>
> Instead of doing a hard check for credentials, do a soft one and
> therefore avoid the reported fatal error.
>
> As a special case, flag files with CRLF endings as invalid early
> to prevent current problems in credential_from_url_gently() with
> handling of '\r' in the host.

I do not think it hurts to silently ignore a line that ends with CR,
but only because I do not think credential_from_url_gently() would
not match such a line when asked to match something without
complaining.  

In other words, isn't the new "!strchr() &&" in the condition a
no-op?

> diff --git a/credential-store.c b/credential-store.c
> index c010497cb2..fdfb81e632 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -24,8 +24,9 @@ static int parse_credential_file(const char *fn,
>  	}
>  
>  	while (strbuf_getline_lf(&line, fh) != EOF) {
> -		credential_from_url(&entry, line.buf);
> -		if (entry.username && entry.password &&
> +		if (strchr(line.buf, '\r') == NULL &&
> +		    !credential_from_url_gently(&entry, line.buf, 1) &&
> +		    entry.username && entry.password &&
>  		    credential_match(c, &entry)) {
>  			found_credential = 1;
>  			if (match_cb) {

In any case, among the ones we discussed, this probably has the
least chance of unintended regression, I would think (with or
without the added "!strchr() &&" check), so let's queue it and
quickly merge it down thru 'next' to 'master'.

> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index d6b54e8c65..9fd0aca55e 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home
>  	test_must_be_empty "$HOME/.config/git/credentials"
>  '
>  
> -
>  test_expect_success 'erase: erase matching credentials from both xdg and home files' '
>  	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
>  	mkdir -p "$HOME/.config/git" &&
> @@ -120,4 +119,63 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
>  	test_must_be_empty "$HOME/.config/git/credentials"
>  '
>  
> +invalid_credential_test() {
> +	test_expect_success 'get: ignore credentials without $1 as invalid' '
> +		echo "$2" >"$HOME/.git-credentials" &&
> +		check fill store <<-\EOF
> +		protocol=https
> +		host=example.com
> +		--
> +		protocol=https
> +		host=example.com
> +		username=askpass-username
> +		password=askpass-password
> +		--
> +		askpass: Username for '\''https://example.com'\'':
> +		askpass: Password for '\''https://askpass-username@example.com'\'':
> +		--
> +		EOF
> +	'
> +}
> +
> +invalid_credential_test "scheme" ://user:pass@example.com
> +invalid_credential_test "valid host/path" https://user:pass@
> +invalid_credential_test "username/password" https://pass@example.com

These are quite clear to see.  Nicely done.

> +test_expect_success 'get: credentials with DOS line endings are invalid' '
> +	printf "https://user:pass@example.com\r\n" >"$HOME/.git-credentials" &&
> +	check fill store <<-\EOF
> +	protocol=https
> +	host=example.com
> +	--
> +	protocol=https
> +	host=example.com
> +	username=askpass-username
> +	password=askpass-password
> +	--
> +	askpass: Username for '\''https://example.com'\'':
> +	askpass: Password for '\''https://askpass-username@example.com'\'':
> +	--
> +	EOF
> +'

What I am curious is if anything breaks around this test if you lost
the extra "!strchr() &&" check.  I suspect that this test will pass.

> +test_expect_success 'get: store file can contain empty/bogus lines' '
> +	echo "" >"$HOME/.git-credentials" &&
> +	q_to_tab <<-\CREDENTIAL >>"$HOME/.git-credentials" &&
> +	#comment
> +	Q
> +	https://user:pass@example.com
> +	CREDENTIAL
> +	check fill store <<-\EOF
> +	protocol=https
> +	host=example.com
> +	--
> +	protocol=https
> +	host=example.com
> +	username=user
> +	password=pass
> +	--
> +	EOF
> +'
> +
>  test_done
>
> base-commit: 49458fb91d61461938881559ce23abbb1a2f8c35

Re: [PATCH v10] credential-store: ignore bogus lines from store file

[Carlo Marcelo Arenas Belón]

On Sat, May 02, 2020 at 01:47:09PM -0700, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> 
> > With the added checks for invalid URLs in credentials, any locally
> > modified store files which might have empty lines or even comments
> > were reported[1] failing to parse as valid credentials.
> >
> > Instead of doing a hard check for credentials, do a soft one and
> > therefore avoid the reported fatal error.
> >
> > As a special case, flag files with CRLF endings as invalid early
> > to prevent current problems in credential_from_url_gently() with
> > handling of '\r' in the host.
> 
> I do not think it hurts to silently ignore a line that ends with CR,
> but only because I do not think credential_from_url_gently() would
> not match such a line when asked to match something without
> complaining.  

for a credential like the one in the testcase (meaning no url), it will
append \r to the hostname, which would cause havoc if that credential
is printed (meaning you will end up without a host line) and be back
in the die() in credential_apply()

> In other words, isn't the new "!strchr() &&" in the condition a
> no-op?

you are correct that it will be unlikely (but not imposible) to get an
embedded CR from the other side to match, which is what I want to
address in the next patchset.

IMHO adding the proposed early check gives us space to fix the other
issues at our own leasure and it is meant to be gone eventually. 

> > diff --git a/credential-store.c b/credential-store.c
> > index c010497cb2..fdfb81e632 100644
> > --- a/credential-store.c
> > +++ b/credential-store.c
> > @@ -24,8 +24,9 @@ static int parse_credential_file(const char *fn,
> >  	}
> >  
> >  	while (strbuf_getline_lf(&line, fh) != EOF) {
> > -		credential_from_url(&entry, line.buf);
> > -		if (entry.username && entry.password &&
> > +		if (strchr(line.buf, '\r') == NULL &&
> > +		    !credential_from_url_gently(&entry, line.buf, 1) &&
> > +		    entry.username && entry.password &&
> >  		    credential_match(c, &entry)) {
> >  			found_credential = 1;
> >  			if (match_cb) {
> 
> In any case, among the ones we discussed, this probably has the
> least chance of unintended regression, I would think (with or
> without the added "!strchr() &&" check), so let's queue it and
> quickly merge it down thru 'next' to 'master'.

considering the only line that I wrote was the strchr and the other one
was written by Jonathan and reviewed by Peff I definitly agree.

don't forget this is also a good candidate for maint (most likely all
the way to maint-2.17)

Carlo

Re: [PATCH v10] credential-store: ignore bogus lines from store file

[Carlo Marcelo Arenas Belón]

On Sat, May 02, 2020 at 01:47:09PM -0700, Junio C Hamano wrote:
> 
> What I am curious is if anything breaks around this test if you lost
> the extra "!strchr() &&" check.  I suspect that this test will pass.

correct

and with the strchr I am introducing a regression (compared against 2.26.0)
in cases where the '\r' gets appended to the path and therefore gets ignored
for matching (unless useHttpPath is true, which is not the default)

will add 2 more test cases to cover those, and guess we are going to 11 :(

Carlo

PS. thanks for all your patience and reviewing

Re: [PATCH v10] credential-store: ignore bogus lines from store file

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> and with the strchr I am introducing a regression (compared against 2.26.0)
> in cases where the '\r' gets appended to the path and therefore gets ignored
> for matching (unless useHttpPath is true, which is not the default)
>
> will add 2 more test cases to cover those, and guess we are going to 11 :(
>
> Carlo
>
> PS. thanks for all your patience and reviewing

Among those 11, I also am responsible for a few while we ere
pursuing the approach to warn X-<.

Thanks for _your_ patience.

Re: [PATCH v10] credential-store: ignore bogus lines from store file

[Jeff King]

On Sat, May 02, 2020 at 01:47:09PM -0700, Junio C Hamano wrote:

> > As a special case, flag files with CRLF endings as invalid early
> > to prevent current problems in credential_from_url_gently() with
> > handling of '\r' in the host.
> 
> I do not think it hurts to silently ignore a line that ends with CR,
> but only because I do not think credential_from_url_gently() would
> not match such a line when asked to match something without
> complaining.

I wondered if we might hit a case where the CR ends up in the path,
like:

  $ printf 'https://user:pass@example.com/\r\n' >creds
  $ echo url=https://example.com/ |
    git credential-store --file=creds get
  username=user
  password=pass

because the path is parsed as missing in the incoming pattern (and thus
we match any path, even "\r").

But credential-store would never write such a path in the first place.
Even with the trailing slash on an incoming URL, it will write:

  https://example.com

without a slash at all (and thus any inserted CR would be part of the
hostname). So somebody would have to have inserted it themselves, or
have turned useHTTPPath on (in which case we _would_ complain on the
matching side, too, because we'd try matching the path with a CR in it).

I think it's reasonable to assume that any CR would have been a problem
even in older versions.

-Peff

Re: [PATCH v10] credential-store: ignore bogus lines from store file

[Carlo Marcelo Arenas Belón]

Dscho,

this was tested[1] in windows and was able to reproduce the bogus issues
with credential store in macOS using t0302.50

[1] https://github.com/carenas/git/actions/runs/93858709

Junio,

will followup with a more comprehensive fix for the problems with '\r'
which will obiously need also changes in credential.c, but that IMHO might
be more of a prerequisite for the next iteration (the one that warns)

would be very helpful if you squash the following, otherwise

Carlo
-- >8 --
Subject: [PATCH] SQUASH!!!

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 t/t0302-credential-store.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 9fd0aca55e..a05b64c8b3 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,7 +120,7 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 '
 
 invalid_credential_test() {
-	test_expect_success 'get: ignore credentials without $1 as invalid' '
+	test_expect_success "get: ignore credentials without $1 as invalid" '
 		echo "$2" >"$HOME/.git-credentials" &&
 		check fill store <<-\EOF
 		protocol=https
-- 
2.26.2.686.gfaf46a9ccd

[PATCH v11] credential-store: ignore bogus lines from store file

[Carlo Marcelo Arenas Belón]

With the added checks for invalid URLs in credentials, any locally
modified store files which might have empty lines or even comments
were reported[1] failing to parse as valid credentials.

Instead of doing a hard check for credentials, do a soft one and
therefore avoid the reported fatal error.

While at it add tests for all known corruptions that are currently
ignored to keep track of them and avoid the risk of regressions.

[1] https://stackoverflow.com/a/61420852/5005936

Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v11:
* remove bogus early check for CR characters
v10:
* go back to v4 but with better testing and commit message
* make sure broken CR characters are ignored early
v9:
* use strbuf_getline() instead to better handle files with CRLF
v8:
* only warn during get operations as otherwise line number might be
  incorrect
v7:
* check for protocol in helper as suggested by Junio
v6:
* get rid of redacter and only use line number for context in warning
* make validation more strict to also catch incomplete credentials
* reorder check as suggested by Junio
v5:
* q_to_tab this round, with a single echo to make sure empty line
  is covered, as that seems to be a popular issue
* rebase on top of jc/credential-store-file-format-doc
* implement a redacter for credentials to use on errors to avoid
  leaking passwords
v4:
* use credential_from_url_gently instead as shown by Jonathan
* add documentation to clarify "comments" is not a supported feature
v3:
* avoid using q_to_cr as suggested by Peff
* a more verbose commit message and slightly more complete documentation
v2:
* use a here-doc for clarity as suggested by Eric
* improve commit message and include documentation

 credential-store.c          |  4 +-
 t/t0302-credential-store.sh | 91 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 92 insertions(+), 3 deletions(-)

diff --git a/credential-store.c b/credential-store.c
index c010497cb2..294e771681 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,
 	}
 
 	while (strbuf_getline_lf(&line, fh) != EOF) {
-		credential_from_url(&entry, line.buf);
-		if (entry.username && entry.password &&
+		if (!credential_from_url_gently(&entry, line.buf, 1) &&
+		    entry.username && entry.password &&
 		    credential_match(c, &entry)) {
 			found_credential = 1;
 			if (match_cb) {
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..716bf1af9f 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -107,7 +107,6 @@ test_expect_success 'store: if both xdg and home files exist, only store in home
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
-
 test_expect_success 'erase: erase matching credentials from both xdg and home files' '
 	echo "https://home-user:home-pass@example.com" >"$HOME/.git-credentials" &&
 	mkdir -p "$HOME/.config/git" &&
@@ -120,4 +119,94 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+invalid_credential_test() {
+	test_expect_success "get: ignore credentials without $1 as invalid" '
+		echo "$2" >"$HOME/.git-credentials" &&
+		check fill store <<-\EOF
+		protocol=https
+		host=example.com
+		--
+		protocol=https
+		host=example.com
+		username=askpass-username
+		password=askpass-password
+		--
+		askpass: Username for '\''https://example.com'\'':
+		askpass: Password for '\''https://askpass-username@example.com'\'':
+		--
+		EOF
+	'
+}
+
+invalid_credential_test "scheme" ://user:pass@example.com
+invalid_credential_test "valid host/path" https://user:pass@
+invalid_credential_test "username/password" https://pass@example.com
+
+test_expect_success 'get: credentials with DOS line endings are invalid' '
+	printf "https://user:pass@example.com\r\n" >"$HOME/.git-credentials" &&
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=askpass-username
+	password=askpass-password
+	--
+	askpass: Username for '\''https://example.com'\'':
+	askpass: Password for '\''https://askpass-username@example.com'\'':
+	--
+	EOF
+'
+
+test_expect_success 'get: credentials with path and DOS line endings are valid' '
+	printf "https://user:pass@example.com/repo.git\r\n" >"$HOME/.git-credentials" &&
+	check fill store <<-\EOF
+	url=https://example.com/repo.git
+	--
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	--
+	EOF
+'
+
+test_expect_success 'get: credentials with DOS line endings are invalid if path is relevant' '
+	printf "https://user:pass@example.com/repo.git\r\n" >"$HOME/.git-credentials" &&
+	test_config credential.useHttpPath true &&
+	check fill store <<-\EOF
+	url=https://example.com/repo.git
+	--
+	protocol=https
+	host=example.com
+	path=repo.git
+	username=askpass-username
+	password=askpass-password
+	--
+	askpass: Username for '\''https://example.com/repo.git'\'':
+	askpass: Password for '\''https://askpass-username@example.com/repo.git'\'':
+	--
+	EOF
+'
+
+test_expect_success 'get: store file can contain empty/bogus lines' '
+	echo "" >"$HOME/.git-credentials" &&
+	q_to_tab <<-\CREDENTIAL >>"$HOME/.git-credentials" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CREDENTIAL
+	check fill store <<-\EOF
+	protocol=https
+	host=example.com
+	--
+	protocol=https
+	host=example.com
+	username=user
+	password=pass
+	--
+	EOF
+'
+
 test_done

base-commit: 49458fb91d61461938881559ce23abbb1a2f8c35
-- 
2.26.2.686.gfaf46a9ccd