push: anonymize URLs in error messages and warnings

From: Johannes Schindelin <[email protected]>

Just like 47abd85ba0 (fetch: Strip usernames from url's before storing them, 2009-04-17) and later 882d49ca5c (push: anonymize URL in status output, 2016-07-13), and even later c1284b21f243 (curl: anonymize URLs in error messages and warnings, 2019-03-04) this change anonymizes URLs (read: strips them of user names and especially passwords) in user-facing error messages and warnings.

Signed-off-by Johannes Schindelin <[email protected]>

---
push: anonymize URLs in error messages and warnings

A token used by GitGitGadget was leaked by this bug. Thankfully, it seems nobody noticed, and I installed a patched Git on the self-hosted build agent so that this won't happen anymore.

Quoted 13 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Just like 47abd85ba0 (fetch: Strip usernames from url's before storing them, 2009-04-17) and later 882d49ca5c (push: anonymize URL in status output, 2016-07-13), and even later c1284b21f243 (curl: anonymize URLs in error messages and warnings, 2019-03-04) this change anonymizes URLs (read: strips them of user names and especially passwords) in user-facing error messages and warnings.

Signed-off-by Johannes Schindelin <[email protected]>

---
push: anonymize URLs in error messages and warnings

Quoted 3 lines

A token used by GitGitGadget was leaked by this bug. Thankfully, it seems nobody noticed, and I installed a patched Git on the self-hosted build agent so that this won't happen anymore.

All looks good to me, thanks.

Reviewed-by Taylor Blau <[email protected]>

 builtin/push.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 2 lines · writes:

 builtin/push.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Is this something we can protect with a test? perhaps like 882d49ca (push: anonymize URL in status output, 2016-07-13) did?

builtin/push.c static int push_with_options(struct transport *transport, struct refspec *rs, · line 343

Quoted 9 lines

builtin/push.c +1 -0

index 6dbf0f0bb71..bd2a2cbfbd7 100644
@@ -340,6 +340,7 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
 {
 	int err;
 	unsigned int reject_reasons;
+	char *anon_url = transport_anonymize_url(transport->url);

Do we need to anonymize this early, way before we know we'd fail the push? It wouldn't be that transport->url is going to be munged before we realize that we have to issue an error message---otherwise you'd not be writing a patch to replace transport->url in the error message. So this placement of anonymize call sounds like taking the error path as the main flow of control.

In other words, would it break to squash the following change in?

 builtin/push.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

builtin/push.c +2 -2

index bd2a2cbfbd..b88948b07e 100644
@@ -340,7 +340,6 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
 {
 	int err;
 	unsigned int reject_reasons;
-	char *anon_url = transport_anonymize_url(transport->url);
 
 	transport_set_verbosity(transport, verbosity, progress);
 	transport->family = family;

@@ -364,13 +363,14 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
 			     rs, flags, &reject_reasons);
 	trace2_region_leave("push", "transport_push", the_repository);
 	if (err != 0) {
+		char *anon_url = transport_anonymize_url(transport->url);
 		fprintf(stderr, "%s", push_get_color(PUSH_COLOR_ERROR));
 		error(_("failed to push some refs to '%s'"), anon_url);
 		fprintf(stderr, "%s", push_get_color(PUSH_COLOR_RESET));
+		free(anon_url);
 	}
 
 	err |= transport_disconnect(transport);
-	free(anon_url);
 	if (!err)
 		return 0;