threads / patch / 53308

patchpush: anonymize URLs in error messages and warnings

Subject: [PATCH] push: anonymize URLs in error messages and warnings

## tl;dr

6 messages between Apr 24, 2020 and Apr 24, 2020. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Johannes Schindelin via GitGitGadget· Apr 24, 2020, 14:20 UTC · lore
From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
    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.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-618%2Fdscho%2Fanonymize-push-url-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-618/dscho/anonymize-push-url-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/618
 builtin/push.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to builtin/push.c +3 −1
diff --git a/builtin/push.c b/builtin/push.c
index 6dbf0f0bb71..bd2a2cbfbd7 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -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);
 
 	transport_set_verbosity(transport, verbosity, progress);
 	transport->family = family;
@@ -364,11 +365,12 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
 	trace2_region_leave("push", "transport_push", the_repository);
 	if (err != 0) {
 		fprintf(stderr, "%s", push_get_color(PUSH_COLOR_ERROR));
-		error(_("failed to push some refs to '%s'"), transport->url);
+		error(_("failed to push some refs to '%s'"), anon_url);
 		fprintf(stderr, "%s", push_get_color(PUSH_COLOR_RESET));
 	}
 
 	err |= transport_disconnect(transport);
+	free(anon_url);
 	if (!err)
 		return 0;
 

base-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec

-- 
gitgitgadget
Taylor Blau· Apr 24, 2020, 16:50 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH] push: anonymize URLs in error messages and warnings

On Fri, Apr 24, 2020 at 02:20:08PM +0000, Johannes Schindelin via GitGitGadget wrote:
Show 16 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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 <johannes.schindelin@gmx.de>
> ---
>     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.
All looks good to me, thanks.
  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks, Taylor

Junio C Hamano· Apr 24, 2020, 20:29 UTC · re: Taylor Blau · lore

Re: [PATCH] push: anonymize URLs in error messages and warnings

Taylor Blau <me@ttaylorr.com> writes:
Show 21 quoted lines
> On Fri, Apr 24, 2020 at 02:20:08PM +0000, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> 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 <johannes.schindelin@gmx.de>
>> ---
>>     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.
>
> All looks good to me, thanks.
>
>   Reviewed-by: Taylor Blau <me@ttaylorr.com>
Thanks for the patch and a quick review.
Will queue.
Junio C Hamano· Apr 24, 2020, 20:38 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH] push: anonymize URLs in error messages and warnings

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> 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?

Show 9 quoted lines
> diff --git a/builtin/push.c b/builtin/push.c
> index 6dbf0f0bb71..bd2a2cbfbd7 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -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(-)
Show changes to builtin/push.c +2 −2
diff --git a/builtin/push.c b/builtin/push.c
index bd2a2cbfbd..b88948b07e 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -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;
Johannes Schindelin· Apr 24, 2020, 21:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] push: anonymize URLs in error messages and warnings

Hi Junio,
On Fri, 24 Apr 2020, Junio C Hamano wrote:
Show 27 quoted lines
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> 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?
>
> > diff --git a/builtin/push.c b/builtin/push.c
> > index 6dbf0f0bb71..bd2a2cbfbd7 100644
> > --- a/builtin/push.c
> > +++ b/builtin/push.c
> > @@ -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?
It would break it _if I hadn't forgotten to include this_:
Show changes to builtin/push.c +1 −1
diff --git a/builtin/push.c b/builtin/push.c
index bd2a2cbfbd73..59c8acb55680 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -358,7 +358,7 @@ static int push_with_options(struct transport *transport, struct refspec *rs,
 	}

 	if (verbosity > 0)
-		fprintf(stderr, _("Pushing to %s\n"), transport->url);
+		fprintf(stderr, _("Pushing to %s\n"), anon_url);
 	trace2_region_enter("push", "transport_push", the_repository);
 	err = transport_push(the_repository, transport,
 			     rs, flags, &reject_reasons);

And it's not like we change the URL in `push_with_options()`: it is
expected to remain unmodified throughout this function.

Do you want me to send a v2 with above diff squashed in, or can you apply
locally (unless more reviews trickle in that require changes)?

Ciao,
Dscho

>
>  builtin/push.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index bd2a2cbfbd..b88948b07e 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -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;
>
>
>
Junio C Hamano· Apr 24, 2020, 21:22 UTC · re: Johannes Schindelin · lore

Re: [PATCH] push: anonymize URLs in error messages and warnings

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>  	if (verbosity > 0)
> -		fprintf(stderr, _("Pushing to %s\n"), transport->url);
> +		fprintf(stderr, _("Pushing to %s\n"), anon_url);
Heh, both of us did not see this?  We must be tired.

Will replace the squash one with this one liner and wait until dust settles.

Thanks for a quick turnaround.

← back to recent threads