diff mbox series

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

Message ID 20200501051847.31803-1-carenas@gmail.com (mailing list archive)
State New, archived
Headers show
Series [RFC,v10] credential-store: warn/ignore for bogus lines from store file | expand

Commit Message

Carlo Marcelo Arenas Belón May 1, 2020, 5:18 a.m. UTC 
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(-)

Comments

Junio C Hamano May 1, 2020, 5:35 a.m. UTC | #1 
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...
diff mbox series

Patch

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