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

[Dirk <dirk@ed4u.de>]

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