| Message ID | 20200429203546.56753-3-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | credential-store: prevent fatal errors | expand |
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.
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
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.
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" &&
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(-)