From: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jonathan Nieder <jrnieder@gmail.com>, Jeff King <peff@peff.net>,
git@vger.kernel.org, dirk@ed4u.de, sunshine@sunshineco.com,
Stefan Tauner <stefan.tauner@gmx.at>
Subject: Re: [PATCH v3] git-credential-store: skip empty lines and comments from store
Date: Tue, 28 Apr 2020 14:14:07 -0700 [thread overview]
Message-ID: <20200428211407.GC56126@Carlos-MBP> (raw)
In-Reply-To: <xmqqsggn4mxz.fsf@gitster.c.googlers.com>
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
next prev parent reply other threads:[~2020-04-28 21:14 UTC|newest]
Thread overview: 79+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-26 23:47 [PATCH] git-credential-store: skip empty lines and comments from store Carlo Marcelo Arenas Belón
2020-04-27 0:19 ` Eric Sunshine
2020-04-27 0:46 ` Carlo Marcelo Arenas Belón
2020-04-27 8:42 ` [PATCH v2] " Carlo Marcelo Arenas Belón
2020-04-27 11:52 ` Jeff King
2020-04-27 12:25 ` Carlo Marcelo Arenas Belón
2020-04-27 14:43 ` Eric Sunshine
2020-04-27 17:47 ` Junio C Hamano
2020-04-27 19:09 ` Jeff King
2020-04-27 12:59 ` [PATCH v3] " Carlo Marcelo Arenas Belón
2020-04-27 13:48 ` Philip Oakley
2020-04-28 1:49 ` Carlo Marcelo Arenas Belón
2020-04-29 10:09 ` Philip Oakley
2020-04-27 15:39 ` Dirk
2020-04-27 18:09 ` Junio C Hamano
2020-04-27 19:18 ` Jeff King
2020-04-27 20:43 ` Junio C Hamano
2020-04-27 21:10 ` Jeff King
2020-04-28 1:37 ` Carlo Marcelo Arenas Belón
2020-04-27 23:49 ` Carlo Marcelo Arenas Belón
2020-04-28 5:25 ` Jonathan Nieder
2020-04-28 5:41 ` Jeff King
2020-04-28 7:18 ` Carlo Marcelo Arenas Belón
2020-04-28 8:16 ` Jeff King
2020-04-28 11:25 ` Carlo Marcelo Arenas Belón
2020-04-28 10:58 ` Stefan Tauner
2020-04-28 16:03 ` Junio C Hamano
2020-04-28 21:14 ` Carlo Marcelo Arenas Belón [this message]
2020-04-28 21:17 ` Junio C Hamano
2020-04-28 10:48 ` [PATCH v4 0/4] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón
2020-04-28 10:52 ` [PATCH v4 1/4] credential-store: document the file format a bit more Carlo Marcelo Arenas Belón
2020-04-28 10:52 ` [PATCH v4 2/4] git-credential-store: skip empty lines and comments from store Carlo Marcelo Arenas Belón
2020-04-28 16:09 ` Eric Sunshine
2020-04-28 16:42 ` Carlo Marcelo Arenas Belón
2020-04-28 10:52 ` [PATCH v4 3/4] git-credential-store: fix (WIP) Carlo Marcelo Arenas Belón
2020-04-28 16:11 ` Eric Sunshine
2020-04-28 17:14 ` Carlo Marcelo Arenas Belón
2020-04-28 10:52 ` [PATCH v4 4/4] credential-store: make sure there is no regression with missing scheme Carlo Marcelo Arenas Belón
2020-04-28 16:06 ` [PATCH v4 1/4] credential-store: document the file format a bit more Eric Sunshine
2020-04-28 18:18 ` Junio C Hamano
2020-04-28 18:15 ` Junio C Hamano
2020-04-29 0:33 ` [PATCH v5] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón
2020-04-29 4:36 ` Junio C Hamano
2020-04-29 7:31 ` Carlo Marcelo Arenas Belón
2020-04-29 16:46 ` Junio C Hamano
2020-04-29 20:35 ` [RFC PATCH v6 0/2] credential-store: prevent fatal errors Carlo Marcelo Arenas Belón
2020-04-29 20:35 ` [RFC PATCH v6 1/2] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón
2020-04-29 21:05 ` Junio C Hamano
2020-04-29 21:17 ` Junio C Hamano
2020-04-29 20:35 ` [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of using Carlo Marcelo Arenas Belón
2020-04-29 21:12 ` Junio C Hamano
2020-04-29 21:49 ` [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of usingy Carlo Marcelo Arenas Belón
2020-04-29 22:04 ` Junio C Hamano
2020-04-29 23:23 ` [PATCH v6] credential-store: warn instead of fatal for bogus lines from store Carlo Marcelo Arenas Belón
2020-04-29 23:47 ` Junio C Hamano
2020-04-29 23:57 ` Junio C Hamano
2020-04-30 1:00 ` Carlo Marcelo Arenas Belón
2020-04-30 1:19 ` [PATCH v7] " Carlo Marcelo Arenas Belón
2020-04-30 9:29 ` [PATCH v8] " Carlo Marcelo Arenas Belón
2020-04-30 16:06 ` [PATCH v9] " Carlo Marcelo Arenas Belón
2020-04-30 20:21 ` Junio C Hamano
2020-04-30 21:14 ` Junio C Hamano
2020-05-01 0:30 ` Carlo Marcelo Arenas Belón
2020-05-01 1:40 ` Junio C Hamano
2020-05-01 2:24 ` Carlo Arenas
2020-05-01 5:27 ` Junio C Hamano
2020-05-01 13:57 ` Carlo Marcelo Arenas Belón
2020-05-01 18:59 ` Junio C Hamano
2020-05-01 3:21 ` [RFC PATCH v10] credential-store: warn/ignore for bogus lines from store file Carlo Marcelo Arenas Belón
2020-05-01 5:18 ` [RFC PATCH v10 2/1] credential-store: warn also for store and erase Carlo Marcelo Arenas Belón
2020-05-01 5:35 ` Junio C Hamano
2020-05-02 18:16 ` [PATCH v10] credential-store: ignore bogus lines from store file Carlo Marcelo Arenas Belón
2020-05-02 20:47 ` Junio C Hamano
2020-05-02 21:23 ` Carlo Marcelo Arenas Belón
2020-05-02 21:53 ` Carlo Marcelo Arenas Belón
2020-05-03 0:44 ` Junio C Hamano
2020-05-03 10:06 ` Jeff King
2020-05-02 21:05 ` Carlo Marcelo Arenas Belón
2020-05-02 22:34 ` [PATCH v11] " Carlo Marcelo Arenas Belón
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200428211407.GC56126@Carlos-MBP \
--to=carenas@gmail.com \
--cc=dirk@ed4u.de \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=peff@peff.net \
--cc=stefan.tauner@gmx.at \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).