push: anonymize URLs in error messages and warnings #618

dscho · 2020-04-24T14:07:42Z

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.

Changes since v1:

The forgotten message (in the verbose output) now also anonymizes the URL.

dscho · 2020-04-24T14:19:14Z

/submit

gitgitgadget · 2020-04-24T14:20:28Z

Submitted as pull.618.git.1587738008248.gitgitgadget@gmail.com

gitgitgadget · 2020-04-24T16:53:12Z

On the Git mailing list, Taylor Blau wrote (reply to this):

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,
Taylor

gitgitgadget · 2020-04-24T20:33:04Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Taylor Blau <me@ttaylorr.com> writes:

> 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.

gitgitgadget · 2020-04-24T20:41:48Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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?

 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;

Just like 47abd85 (fetch: Strip usernames from url's before storing them, 2009-04-17) and later 882d49c (push: anonymize URL in status output, 2016-07-13), and even later c1284b2 (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>

gitgitgadget · 2020-04-24T21:05:54Z

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

On Fri, 24 Apr 2020, Junio C Hamano wrote:

> "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_:

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;
>
>
>

gitgitgadget · 2020-04-24T21:24:06Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

gitgitgadget · 2020-04-24T23:52:49Z

This patch series was integrated into pu via git@640bad4.

gitgitgadget · 2020-04-28T00:58:13Z

This patch series was integrated into pu via git@ae99c50.

gitgitgadget · 2020-04-28T21:55:00Z

This patch series was integrated into pu via git@59fbea3.

gitgitgadget · 2020-04-29T00:30:24Z

This branch is now known as js/anonymise-push-url-in-errors.

gitgitgadget · 2020-04-29T00:30:25Z

This patch series was integrated into pu via git@fd471a1.

gitgitgadget · 2020-04-29T00:30:27Z

This patch series was integrated into next via git@49539cf.

gitgitgadget · 2020-04-30T00:52:53Z

This patch series was integrated into pu via git@d7d5b7c.

gitgitgadget · 2020-05-01T22:32:44Z

This patch series was integrated into pu via git@2c42fb7.

gitgitgadget · 2020-05-01T22:32:45Z

This patch series was integrated into next via git@2c42fb7.

gitgitgadget · 2020-05-01T22:32:47Z

This patch series was integrated into master via git@2c42fb7.

gitgitgadget · 2020-05-01T22:32:50Z

Closed via 2c42fb7.

dscho mentioned this pull request Apr 24, 2020

Hide GitHub tokens from the PR comments gitgitgadget/gitgitgadget#226

Merged

dscho force-pushed the anonymize-push-url branch from 2358458 to e0afc6f Compare April 24, 2020 21:05

gitgitgadget bot added the pu label Apr 24, 2020

dscho added the ready to submit Has commits that have not been submitted yet label Apr 25, 2020

gitgitgadget bot added the next label Apr 29, 2020

gitgitgadget bot added the master label May 1, 2020

gitgitgadget bot closed this May 1, 2020

dscho deleted the anonymize-push-url branch May 2, 2020 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

push: anonymize URLs in error messages and warnings #618

push: anonymize URLs in error messages and warnings #618

dscho commented Apr 24, 2020 •

edited

dscho commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 28, 2020

gitgitgadget bot commented Apr 28, 2020

gitgitgadget bot commented Apr 29, 2020

gitgitgadget bot commented Apr 29, 2020

gitgitgadget bot commented Apr 29, 2020

gitgitgadget bot commented Apr 30, 2020

gitgitgadget bot commented May 1, 2020

gitgitgadget bot commented May 1, 2020

gitgitgadget bot commented May 1, 2020

gitgitgadget bot commented May 1, 2020

push: anonymize URLs in error messages and warnings #618

push: anonymize URLs in error messages and warnings #618

Conversation

dscho commented Apr 24, 2020 • edited

dscho commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 24, 2020

gitgitgadget bot commented Apr 28, 2020

gitgitgadget bot commented Apr 28, 2020

gitgitgadget bot commented Apr 29, 2020

gitgitgadget bot commented Apr 29, 2020

gitgitgadget bot commented Apr 29, 2020

gitgitgadget bot commented Apr 30, 2020

gitgitgadget bot commented May 1, 2020

gitgitgadget bot commented May 1, 2020

gitgitgadget bot commented May 1, 2020

gitgitgadget bot commented May 1, 2020

dscho commented Apr 24, 2020 •

edited