Re: [PATCH v6] credential-store: warn instead of fatal for bogus lines from store
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 29, 2020, 23:47 UTC
- Message-ID
- <xmqqlfmdua5o.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20200429232322.68038-1-carenas@gmail.com>
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
Show 16 quoted lines
> 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?
Show 11 quoted lines
> 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?
Show 5 quoted lines
> +static int valid_credential(struct credential *entry)
> +{
> + return (entry->username && entry->password &&
> + ((entry->host && *entry->host) || entry->path));
> +}Show 21 quoted lines
> @@ -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.Show 11 quoted lines
> 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.
Show 24 quoted lines
> 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.
Show 8 quoted lines
> + 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.