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

[RFC PATCH] fetch: stop emitting duplicate transfer.credentialsInUrl=warn warnings

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Oct 31, 2022, 20:47 UTC
Message-ID
<RFC-patch-1.1-0266485bc6c-20221031T204149Z-avarab@gmail.com>
In-Reply-To
<pull.1399.git.1667245638.gitgitgadget@gmail.com>

Improve on 6dcbdc0d661 (remote: create fetch.credentialsInUrl config, 2022-06-06) by fixing the cases where we emit duplicate warnings, which were:

 * In the same process, as we'd get the same "struct remote *"
   again. We could probably save ourselves more work in those scenarios,
   but adding a flag to the "struct remote" indicating that we've validated
   the URLs will fix those issues.
 * When we invoke e.g. "git-remote-https" (aka. "git-remote-curl")
   from "git fetch". For those cases let's pass down the equivalent of a
   "-c transfer.credentialsInUrl=allow", since we know that we've already
   inspected our remotes in the parent process.
   See 7390f05a3c6 (fetch: after refetch, encourage auto gc repacking,
   2022-03-28) for prior use of git_config_push_parameter() for this
   purpose, i.e. to push config parameters before invoking a
   sub-process.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

[Grabbing a quote from https://lore.kernel.org/git/98fa5270cb720f2f038c4bb9571c7d306ff5d759.1667245639.git.gitgitgadget@gmail.com/ for a reply]

Show 5 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> It is unclear as to _why_, but under certain circumstances the warning
> about credentials being passed as part of the URL seems to be swallowed
> by the `git remote-https` helper in the Windows jobs of Git's CI builds.

I think this should fix the root cause of the issue you're seeing. I have a larger local branch to fix various issues with this credentialsInUrl facility that I hadn't gotten around to submitting:

	https://github.com/git/git/compare/master...avar:avar/fix-cred-in-url-warnings-2

This is a cherry-pick of 7/* of that (the first were doc changes, missing test coverage etc).

> Since it is not actually important how many times Git prints the
> warning/error message, as long as it prints it at least once, let's just
> make the test a bit more lenient and test for the latter instead of the
> former, which works around these CI issues.

That being said your change is obviously smaller, so if we'd prefer that in first as a band-aid I'm fine with that.

But I'd really like to object to the "it is not actually important how many...", it's crappy UX to spew duplicate output at the user, and we should avoid it.

So it does matter, and we get it wrong not just in this but also some other cases.

 builtin/clone.c       |  5 +++-
 builtin/fetch.c       |  4 ++++
 builtin/push.c        |  6 ++++-
 remote.c              | 53 ++++++++++++++++++++++++++++++++-----------
 remote.h              | 14 ++++++++++++
 t/t5516-fetch-push.sh |  2 +-
 t/t5601-clone.sh      |  2 +-
 7 files changed, 69 insertions(+), 17 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 547d6464b3c..da219b74e43 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -903,12 +903,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	int err = 0, complete_refs_before_fetch = 1;
 	int submodule_progress;
 	int filter_submodules = 0;
-
+	enum credentials_in_url cred_in_url_cfg = get_credentials_in_url();
 	struct transport_ls_refs_options transport_ls_refs_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
 
 	packet_trace_identity("clone");
 
+	if (cred_in_url_cfg == CRED_IN_URL_WARN)
+		git_config_push_parameter("transfer.credentialsInUrl=allow");
+
 	git_config(git_clone_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b06e454cbdd..34a10e1927f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -2110,9 +2110,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	struct remote *remote = NULL;
 	int result = 0;
 	int prune_tags_ok = 1;
+	enum credentials_in_url cred_in_url_cfg = get_credentials_in_url();
 
 	packet_trace_identity("fetch");
 
+	if (cred_in_url_cfg == CRED_IN_URL_WARN)
+		git_config_push_parameter("transfer.credentialsInUrl=allow");
+
 	/* Record the command line for the reflog */
 	strbuf_addstr(&default_rla, "fetch");
 	for (i = 1; i < argc; i++) {
diff --git a/builtin/push.c b/builtin/push.c
index f0329c62a2d..a4890b1586d 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -576,7 +576,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	struct string_list *push_options;
 	const struct string_list_item *item;
 	struct remote *remote;
-
+	enum credentials_in_url cred_in_url_cfg = get_credentials_in_url();
 	struct option options[] = {
 		OPT__VERBOSITY(&verbosity),
 		OPT_STRING( 0 , "repo", &repo, N_("repository"), N_("repository")),
@@ -619,6 +619,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 	};
 
 	packet_trace_identity("push");
+
+	if (cred_in_url_cfg == CRED_IN_URL_WARN)
+		git_config_push_parameter("transfer.credentialsInUrl=allow");
+
 	git_config(git_push_config, &flags);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
 	push_options = (push_options_cmdline.nr
diff --git a/remote.c b/remote.c
index 60869beebe7..a35da349629 100644
--- a/remote.c
+++ b/remote.c
@@ -615,24 +615,42 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)
 	return NULL;
 }
 
-static void validate_remote_url(struct remote *remote)
+static enum credentials_in_url cred_in_url = CRED_IN_URL_UNKNOWN;
+enum credentials_in_url get_credentials_in_url(void)
 {
-	int i;
+	enum credentials_in_url new = CRED_IN_URL_ALLOW;
 	const char *value;
-	struct strbuf redacted = STRBUF_INIT;
-	int warn_not_die;
+
+	if (cred_in_url != CRED_IN_URL_UNKNOWN)
+		return cred_in_url;
 
 	if (git_config_get_string_tmp("transfer.credentialsinurl", &value))
-		return;
+		goto done;
 
 	if (!strcmp("warn", value))
-		warn_not_die = 1;
+		new = CRED_IN_URL_WARN;
 	else if (!strcmp("die", value))
-		warn_not_die = 0;
+		new = CRED_IN_URL_DIE;
 	else if (!strcmp("allow", value))
-		return;
+		goto done;
 	else
-		die(_("unrecognized value transfer.credentialsInUrl: '%s'"), value);
+		die(_("unrecognized value transfer.credentialsInURL: '%s'"), value);
+
+done:
+	cred_in_url = new;
+	return cred_in_url;
+}
+
+static void validate_remote_url(struct remote *remote)
+{
+	int i;
+	struct strbuf redacted = STRBUF_INIT;
+	enum credentials_in_url cfg = get_credentials_in_url();
+
+	if (remote->validated_urls)
+		goto done;
+	if (cfg == CRED_IN_URL_ALLOW)
+		goto done;
 
 	for (i = 0; i < remote->url_nr; i++) {
 		struct url_info url_info = { 0 };
@@ -647,16 +665,25 @@ static void validate_remote_url(struct remote *remote)
 		strbuf_addstr(&redacted,
 			      url_info.url + url_info.passwd_off + url_info.passwd_len);
 
-		if (warn_not_die)
+		switch (cfg) {
+		case CRED_IN_URL_WARN:
 			warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
-		else
+			break;
+		case CRED_IN_URL_DIE:
 			die(_("URL '%s' uses plaintext credentials"), redacted.buf);
-
-loop_cleanup:
+			break;
+		case CRED_IN_URL_ALLOW:
+		case CRED_IN_URL_UNKNOWN:
+			BUG("unreachable");
+			break;
+		}
+	loop_cleanup:
 		free(url_info.url);
 	}
 
 	strbuf_release(&redacted);
+done:
+	remote->validated_urls = 1;
 }
 
 static struct remote *
diff --git a/remote.h b/remote.h
index 1c4621b414b..5a2da13b4cb 100644
--- a/remote.h
+++ b/remote.h
@@ -98,6 +98,8 @@ struct remote {
 	int prune;
 	int prune_tags;
 
+	int validated_urls;
+
 	/**
 	 * The configured helper programs to run on the remote side, for
 	 * Git-native protocols.
@@ -445,4 +447,16 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);
 char *relative_url(const char *remote_url, const char *url,
 		   const char *up_path);
 
+enum credentials_in_url {
+	CRED_IN_URL_UNKNOWN,
+	CRED_IN_URL_ALLOW,
+	CRED_IN_URL_WARN,
+	CRED_IN_URL_DIE,
+};
+
+/**
+ * Get the transfer.credentialsInUrl config setting as an "enum
+ * credentials_in_url".
+ */
+enum credentials_in_url get_credentials_in_url(void);
 #endif
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 79dc470c014..d01f5cd349f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1860,7 +1860,7 @@ test_expect_success LIBCURL 'fetch warns or fails when using username:password'
 
 	test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&
 	grep "warning: $message" err >warnings &&
-	test_line_count = 3 warnings &&
+	test_line_count = 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err &&
 	grep "fatal: $message" err >warnings &&
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 45f0803ed4d..37adfd9f83b 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -78,7 +78,7 @@ test_expect_success LIBCURL 'clone warns or fails when using username:password'
 
 	test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&
 	grep "warning: $message" err >warnings &&
-	test_line_count = 2 warnings &&
+	test_line_count = 1 warnings &&
 
 	test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&
 	grep "fatal: $message" err >warnings &&

-- 
2.38.0.1280.g8136eb6fab2
Previous: Jeff KingNext: Taylor Blau
Message 26 of 45 in “t5516/t5601: avoid using localhost for failing HTTPS requests”
  1. 0/2 t5516/t5601: avoid using localhost for failing HTTPS requestsJohannes Schindelin via GitGitGadget, Oct 31, 2022
  2. 1/2 t5516/t5601: avoid using `localhost` for failing HTTPS requestsJohannes Schindelin via GitGitGadget, Oct 31, 2022
  3. Ævar Arnfjörð BjarmasonOct 31, 2022
  4. Jeff KingOct 31, 2022
  5. Taylor BlauNov 1, 2022
  6. Jeff KingNov 1, 2022
  7. Jeff KingNov 1, 2022
  8. Jeff KingNov 1, 2022
  9. 1/2 t5516: move plaintext-password tests from t5601 and t5516Jeff King, Nov 1, 2022
  10. Ævar Arnfjörð BjarmasonNov 1, 2022
  11. Jeff KingNov 1, 2022
  12. Taylor BlauNov 1, 2022
  13. 2/2 t5516/t5601: be less strict about the number of credential warningsJeff King, Nov 1, 2022
  14. Ævar Arnfjörð BjarmasonNov 1, 2022
  15. Jeff KingNov 1, 2022
  16. Ævar Arnfjörð BjarmasonNov 1, 2022
  17. Jeff KingNov 1, 2022
  18. Ævar Arnfjörð BjarmasonNov 1, 2022
  19. Junio C HamanoNov 1, 2022
  20. Jeff KingNov 1, 2022
  21. Taylor BlauNov 1, 2022
  22. 2/2 t5516/t5601: be less strict about the number of credential warningsJohannes Schindelin via GitGitGadget, Oct 31, 2022
  23. Jeff KingOct 31, 2022
  24. Taylor BlauNov 1, 2022
  25. Jeff KingNov 1, 2022
  26. fetch: stop emitting duplicate transfer.credentialsInUrl=warn warningsÆvar Arnfjörð Bjarmason, Oct 31, 2022
  27. Taylor BlauNov 1, 2022
  28. Jeff KingNov 1, 2022
  29. Ævar Arnfjörð BjarmasonNov 1, 2022
  30. Taylor BlauNov 1, 2022
  31. Ævar Arnfjörð BjarmasonNov 1, 2022
  32. Taylor BlauNov 2, 2022
  33. 2/2 t5551: be less strict about the number of credential warningsJeff King, Nov 2, 2022
  34. Eric SunshineNov 2, 2022
  35. Jeff KingNov 2, 2022
  36. Eric SunshineNov 2, 2022
  37. Jeff KingNov 2, 2022
  38. Taylor BlauNov 3, 2022
  39. Jeff KingNov 1, 2022
  40. Ævar Arnfjörð BjarmasonNov 1, 2022
  41. Taylor BlauNov 1, 2022
  42. Ævar Arnfjörð BjarmasonNov 1, 2022
  43. Jeff KingNov 2, 2022
  44. Ævar Arnfjörð BjarmasonNov 4, 2022
  45. Jeff KingNov 4, 2022

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.