git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v8] credential-store: warn instead of fatal for bogus lines from store

From
Carlo Marcelo Arenas Belón <carenas@gmail.com>
Date
Apr 30, 2020, 09:29 UTC
Message-ID
<20200430092939.3564-1-carenas@gmail.com>
In-Reply-To
<20200430011959.70597-1-carenas@gmail.com>

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.

Instead of doing a hard check for credentials, do a soft one and therefore avoid the reported fatal error.

Warn the user indicating the filename and line number so any invalid entries could be corrected but continue parsing until a match is found or all valid credentials are processed.

Make sure that the credential that we will use to match is complete by confirming it has all fields set as expected by the updated rules.

[1] https://stackoverflow.com/a/61420852/5005936
Reported-by: Dirk <dirk@ed4u.de>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Based-on-patch-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
v8:
* only warn during get operations as otherwise line number might be
  incorrect
v7:
* check for protocol in helper as suggested by Junio
v6:
* get rid of redacter and only use line number for context in warning
* make validation more strict to also catch incomplete credentials
* reorder check as suggested by Junio
v5:
* q_to_tab this round, with a single echo to make sure empty line
  is covered, as that seems to be a popular issue
* rebase on top of jc/credential-store-file-format-doc
* implement a redacter for credentials to use on errors to avoid
  leaking passwords
v4:
* use credential_from_url_gently instead as shown by Jonathan
* add documentation to clarify "comments" is not a supported feature
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 | 11 +++-
 credential-store.c                     | 31 +++++++--
 t/t0302-credential-store.sh            | 89 ++++++++++++++++++++++++++
 3 files changed, 123 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 76b0798856..d5841cffad 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.
+
+An unparseable or otherwise invalid line is ignored, and a warning
+message points out the problematic file and line number it appears in.
 
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
diff --git a/credential-store.c b/credential-store.c
index c010497cb2..9bd8ec9b67 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -4,10 +4,20 @@
 #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)
+{
+	return (entry->username && entry->password &&
+		entry->protocol && *entry->protocol &&
+		((entry->host && *entry->host) || entry->path));
+}
+
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
+				  int flags,
 				  void (*match_cb)(struct credential *),
 				  void (*other_cb)(struct strbuf *))
 {
@@ -15,6 +25,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 +35,23 @@ 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 &&
-		    credential_match(c, &entry)) {
+		lineno++;
+
+		if (credential_from_url_gently(&entry, line.buf, 1) ||
+			!valid_credential(&entry)) {
+			if (flags & PARSE_VERBOSE)
+				warning(_("%s:%d: ignoring invalid credential"),
+					fn, lineno);
+		} else if (credential_match(c, &entry)) {
 			found_credential = 1;
 			if (match_cb) {
 				match_cb(&entry);
 				break;
 			}
+			continue;
 		}
-		else if (other_cb)
+
+		if (other_cb)
 			other_cb(&line);
 	}
 
@@ -62,7 +80,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,
 		die_errno("unable to get credential storage lock");
 	if (extra)
 		print_line(extra);
-	parse_credential_file(fn, c, NULL, print_line);
+	parse_credential_file(fn, c, 0, NULL, print_line);
 	if (commit_lock_file(&credential_lock) < 0)
 		die_errno("unable to write credential store");
 }
@@ -139,7 +157,8 @@ 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, print_entry, NULL))
+		if (parse_credential_file(fn->string, c, PARSE_VERBOSE,
+						 print_entry, NULL))
 			return; /* Found credential */
 }
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index d6b54e8c65..8bc89b7efb 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -120,4 +120,93 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
 	test_must_be_empty "$HOME/.config/git/credentials"
 '
 
+test_expect_success 'store: store succeeds silently in corrupted file' '
+	echo "#comment" >"$HOME/.git-credentials" &&
+	check approve store <<-\EOF &&
+	url=https://user:pass@example.com
+	EOF
+	grep "https://user:pass@example.com" "$HOME/.git-credentials" &&
+	test_must_be_empty stderr
+'
+
+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 &&
+	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" &&
+	#comment
+	Q
+	https://user:pass@example.com
+	CONFIG
+	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
+	EOF
+	test_cmp expect-stdout stdout &&
+	test_i18ngrep "ignoring invalid credential" stderr &&
+	test_line_count = 3 stderr
+'
+
 test_done

base-commit: 272281efcc18fcedd248597b8859f343cae1c5a0

-- 
2.26.2.2.g9c647e0d3b.dirty
Previous: Carlo Marcelo Arenas BelónNext: Carlo Marcelo Arenas Belón
Message 59 of 79 in “git-credential-store: skip empty lines and comments from store”
  1. git-credential-store: skip empty lines and comments from storeCarlo Marcelo Arenas Belón, Apr 26, 2020
  2. Eric SunshineApr 27, 2020
  3. Carlo Marcelo Arenas BelónApr 27, 2020
  4. git-credential-store: skip empty lines and comments from storeCarlo Marcelo Arenas Belón, Apr 27, 2020
  5. Jeff KingApr 27, 2020
  6. Carlo Marcelo Arenas BelónApr 27, 2020
  7. Eric SunshineApr 27, 2020
  8. Junio C HamanoApr 27, 2020
  9. Jeff KingApr 27, 2020
  10. git-credential-store: skip empty lines and comments from storeCarlo Marcelo Arenas Belón, Apr 27, 2020
  11. Philip OakleyApr 27, 2020
  12. Carlo Marcelo Arenas BelónApr 28, 2020
  13. Philip OakleyApr 29, 2020
  14. DirkApr 27, 2020
  15. Junio C HamanoApr 27, 2020
  16. Jeff KingApr 27, 2020
  17. Junio C HamanoApr 27, 2020
  18. Jeff KingApr 27, 2020
  19. Carlo Marcelo Arenas BelónApr 28, 2020
  20. Carlo Marcelo Arenas BelónApr 27, 2020
  21. Jonathan NiederApr 28, 2020
  22. Jeff KingApr 28, 2020
  23. Carlo Marcelo Arenas BelónApr 28, 2020
  24. Jeff KingApr 28, 2020
  25. Carlo Marcelo Arenas BelónApr 28, 2020
  26. Stefan TaunerApr 28, 2020
  27. Junio C HamanoApr 28, 2020
  28. Carlo Marcelo Arenas BelónApr 28, 2020
  29. Junio C HamanoApr 28, 2020
  30. 0/4 credential-store: prevent fatal errorsCarlo Marcelo Arenas Belón, Apr 28, 2020
  31. 1/4 credential-store: document the file format a bit moreCarlo Marcelo Arenas Belón, Apr 28, 2020
  32. 2/4 git-credential-store: skip empty lines and comments from storeCarlo Marcelo Arenas Belón, Apr 28, 2020
  33. Eric SunshineApr 28, 2020
  34. Carlo Marcelo Arenas BelónApr 28, 2020
  35. 3/4 git-credential-store: fix (WIP)Carlo Marcelo Arenas Belón, Apr 28, 2020
  36. Eric SunshineApr 28, 2020
  37. Carlo Marcelo Arenas BelónApr 28, 2020
  38. 4/4 credential-store: make sure there is no regression with missing schemeCarlo Marcelo Arenas Belón, Apr 28, 2020
  39. Eric SunshineApr 28, 2020
  40. Junio C HamanoApr 28, 2020
  41. Junio C HamanoApr 28, 2020
  42. credential-store: warn instead of fatal for bogus lines from storeCarlo Marcelo Arenas Belón, Apr 29, 2020
  43. Junio C HamanoApr 29, 2020
  44. Carlo Marcelo Arenas BelónApr 29, 2020
  45. Junio C HamanoApr 29, 2020
  46. 0/2 credential-store: prevent fatal errorsCarlo Marcelo Arenas Belón, Apr 29, 2020
  47. 1/2 credential-store: warn instead of fatal for bogus lines from storeCarlo Marcelo Arenas Belón, Apr 29, 2020
  48. Junio C HamanoApr 29, 2020
  49. Junio C HamanoApr 29, 2020
  50. 2/2 credential-store: warn for any incomplete credentials instead of usingCarlo Marcelo Arenas Belón, Apr 29, 2020
  51. Junio C HamanoApr 29, 2020
  52. Carlo Marcelo Arenas BelónApr 29, 2020
  53. Junio C HamanoApr 29, 2020
  54. credential-store: warn instead of fatal for bogus lines from storeCarlo Marcelo Arenas Belón, Apr 29, 2020
  55. Junio C HamanoApr 29, 2020
  56. Junio C HamanoApr 29, 2020
  57. Carlo Marcelo Arenas BelónApr 30, 2020
  58. credential-store: warn instead of fatal for bogus lines from storeCarlo Marcelo Arenas Belón, Apr 30, 2020
  59. credential-store: warn instead of fatal for bogus lines from storeCarlo Marcelo Arenas Belón, Apr 30, 2020
  60. credential-store: warn instead of fatal for bogus lines from storeCarlo Marcelo Arenas Belón, Apr 30, 2020
  61. Junio C HamanoApr 30, 2020
  62. Junio C HamanoApr 30, 2020
  63. Carlo Marcelo Arenas BelónMay 1, 2020
  64. Junio C HamanoMay 1, 2020
  65. Carlo ArenasMay 1, 2020
  66. Junio C HamanoMay 1, 2020
  67. Carlo Marcelo Arenas BelónMay 1, 2020
  68. Junio C HamanoMay 1, 2020
  69. credential-store: warn/ignore for bogus lines from store fileCarlo Marcelo Arenas Belón, May 1, 2020
  70. 2/1 credential-store: warn also for store and eraseCarlo Marcelo Arenas Belón, May 1, 2020
  71. Junio C HamanoMay 1, 2020
  72. credential-store: ignore bogus lines from store fileCarlo Marcelo Arenas Belón, May 2, 2020
  73. Junio C HamanoMay 2, 2020
  74. Carlo Marcelo Arenas BelónMay 2, 2020
  75. Carlo Marcelo Arenas BelónMay 2, 2020
  76. Junio C HamanoMay 3, 2020
  77. Jeff KingMay 3, 2020
  78. Carlo Marcelo Arenas BelónMay 2, 2020
  79. credential-store: ignore bogus lines from store fileCarlo Marcelo Arenas Belón, May 2, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.