| Message ID | pull.618.git.1587738008248.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | push: anonymize URLs in error messages and warnings | expand |
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
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.
"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;
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; > > >
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.
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;