{"thread":{"id":"53308","subject":"[PATCH] push: anonymize URLs in error messages and warnings","startedAt":"2020-04-24T14:20:13Z","lastAt":"2020-04-24T21:22:41Z","messageCount":6,"participants":["Johannes Schindelin via GitGitGadget","Taylor Blau","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"396131","messageId":"pull.618.git.1587738008248.gitgitgadget@gmail.com","threadId":"53308","inReplyTo":null,"subject":"[PATCH] push: anonymize URLs in error messages and warnings","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-24T14:20:08Z","receivedAt":"2020-04-24T14:20:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nJust like 47abd85ba0 (fetch: Strip usernames from url's before storing\nthem, 2009-04-17) and later 882d49ca5c (push: anonymize URL in status\noutput, 2016-07-13), and even later c1284b21f243 (curl: anonymize URLs\nin error messages and warnings, 2019-03-04) this change anonymizes URLs\n(read: strips them of user names and especially passwords) in\nuser-facing error messages and warnings.\n\nSigned-off-by: Johannes Schindelin \n---\n push: anonymize URLs in error messages and warnings\n \n A token used by GitGitGadget was leaked by this bug. Thankfully, it\n seems nobody noticed, and I installed a patched Git on the self-hosted\n build agent so that this won't happen anymore.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-618%2Fdscho%2Fanonymize-push-url-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-618/dscho/anonymize-push-url-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/618\n\n builtin/push.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 6dbf0f0bb71..bd2a2cbfbd7 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -340,6 +340,7 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n {\n \tint err;\n \tunsigned int reject_reasons;\n+\tchar *anon_url = transport_anonymize_url(transport->url);\n \n \ttransport_set_verbosity(transport, verbosity, progress);\n \ttransport->family = family;\n@@ -364,11 +365,12 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n \ttrace2_region_leave(\"push\", \"transport_push\", the_repository);\n \tif (err != 0) {\n \t\tfprintf(stderr, \"%s\", push_get_color(PUSH_COLOR_ERROR));\n-\t\terror(_(\"failed to push some refs to '%s'\"), transport->url);\n+\t\terror(_(\"failed to push some refs to '%s'\"), anon_url);\n \t\tfprintf(stderr, \"%s\", push_get_color(PUSH_COLOR_RESET));\n \t}\n \n \terr |= transport_disconnect(transport);\n+\tfree(anon_url);\n \tif (!err)\n \t\treturn 0;\n \n\nbase-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec\n-- \ngitgitgadget\n"},{"id":"396148","messageId":"20200424165042.GA58621@syl.local","threadId":"53308","inReplyTo":"pull.618.git.1587738008248.gitgitgadget@gmail.com","subject":"Re: [PATCH] push: anonymize URLs in error messages and warnings","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-04-24T16:50:42Z","receivedAt":"2020-04-24T16:50:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Apr 24, 2020 at 02:20:08PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n>\n> Just like 47abd85ba0 (fetch: Strip usernames from url's before storing\n> them, 2009-04-17) and later 882d49ca5c (push: anonymize URL in status\n> output, 2016-07-13), and even later c1284b21f243 (curl: anonymize URLs\n> in error messages and warnings, 2019-03-04) this change anonymizes URLs\n> (read: strips them of user names and especially passwords) in\n> user-facing error messages and warnings.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> push: anonymize URLs in error messages and warnings\n>\n> A token used by GitGitGadget was leaked by this bug. Thankfully, it\n> seems nobody noticed, and I installed a patched Git on the self-hosted\n> build agent so that this won't happen anymore.\n\nAll looks good to me, thanks.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"396176","messageId":"xmqqy2qkbpa4.fsf@gitster.c.googlers.com","threadId":"53308","inReplyTo":"20200424165042.GA58621@syl.local","subject":"Re: [PATCH] push: anonymize URLs in error messages and warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T20:29:55Z","receivedAt":"2020-04-24T20:30:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Fri, Apr 24, 2020 at 02:20:08PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>> From: Johannes Schindelin \n>>\n>> Just like 47abd85ba0 (fetch: Strip usernames from url's before storing\n>> them, 2009-04-17) and later 882d49ca5c (push: anonymize URL in status\n>> output, 2016-07-13), and even later c1284b21f243 (curl: anonymize URLs\n>> in error messages and warnings, 2019-03-04) this change anonymizes URLs\n>> (read: strips them of user names and especially passwords) in\n>> user-facing error messages and warnings.\n>>\n>> Signed-off-by: Johannes Schindelin \n>> ---\n>> push: anonymize URLs in error messages and warnings\n>>\n>> A token used by GitGitGadget was leaked by this bug. Thankfully, it\n>> seems nobody noticed, and I installed a patched Git on the self-hosted\n>> build agent so that this won't happen anymore.\n>\n> All looks good to me, thanks.\n>\n> Reviewed-by: Taylor Blau \n\nThanks for the patch and a quick review.\n\nWill queue.\n"},{"id":"396179","messageId":"xmqqtv18bov7.fsf@gitster.c.googlers.com","threadId":"53308","inReplyTo":"pull.618.git.1587738008248.gitgitgadget@gmail.com","subject":"Re: [PATCH] push: anonymize URLs in error messages and warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T20:38:52Z","receivedAt":"2020-04-24T20:38:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> builtin/push.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n\nIs this something we can protect with a test? perhaps like 882d49ca\n(push: anonymize URL in status output, 2016-07-13) did?\n\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 6dbf0f0bb71..bd2a2cbfbd7 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -340,6 +340,7 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n> {\n> \tint err;\n> \tunsigned int reject_reasons;\n> +\tchar *anon_url = transport_anonymize_url(transport->url);\n\nDo we need to anonymize this early, way before we know we'd fail the\npush? It wouldn't be that transport->url is going to be munged\nbefore we realize that we have to issue an error message---otherwise\nyou'd not be writing a patch to replace transport->url in the error\nmessage. So this placement of anonymize call sounds like taking the\nerror path as the main flow of control.\n\nIn other words, would it break to squash the following change in?\n\n builtin/push.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex bd2a2cbfbd..b88948b07e 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -340,7 +340,6 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n {\n \tint err;\n \tunsigned int reject_reasons;\n-\tchar *anon_url = transport_anonymize_url(transport->url);\n \n \ttransport_set_verbosity(transport, verbosity, progress);\n \ttransport->family = family;\n@@ -364,13 +363,14 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n \t\t\t rs, flags, &reject_reasons);\n \ttrace2_region_leave(\"push\", \"transport_push\", the_repository);\n \tif (err != 0) {\n+\t\tchar *anon_url = transport_anonymize_url(transport->url);\n \t\tfprintf(stderr, \"%s\", push_get_color(PUSH_COLOR_ERROR));\n \t\terror(_(\"failed to push some refs to '%s'\"), anon_url);\n \t\tfprintf(stderr, \"%s\", push_get_color(PUSH_COLOR_RESET));\n+\t\tfree(anon_url);\n \t}\n \n \terr |= transport_disconnect(transport);\n-\tfree(anon_url);\n \tif (!err)\n \t\treturn 0;\n \n"},{"id":"396182","messageId":"nycvar.QRO.7.76.6.2004242259520.18039@tvgsbejvaqbjf.bet","threadId":"53308","inReplyTo":"xmqqtv18bov7.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] push: anonymize URLs in error messages and warnings","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-24T21:04:38Z","receivedAt":"2020-04-24T21:04:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 24 Apr 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > builtin/push.c | 4 +++-\n> > 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> Is this something we can protect with a test? perhaps like 882d49ca\n> (push: anonymize URL in status output, 2016-07-13) did?\n>\n> > diff --git a/builtin/push.c b/builtin/push.c\n> > index 6dbf0f0bb71..bd2a2cbfbd7 100644\n> > --- a/builtin/push.c\n> > +++ b/builtin/push.c\n> > @@ -340,6 +340,7 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n> > {\n> > \tint err;\n> > \tunsigned int reject_reasons;\n> > +\tchar *anon_url = transport_anonymize_url(transport->url);\n>\n> Do we need to anonymize this early, way before we know we'd fail the\n> push? It wouldn't be that transport->url is going to be munged\n> before we realize that we have to issue an error message---otherwise\n> you'd not be writing a patch to replace transport->url in the error\n> message. So this placement of anonymize call sounds like taking the\n> error path as the main flow of control.\n>\n> In other words, would it break to squash the following change in?\n\nIt would break it _if I hadn't forgotten to include this_:\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex bd2a2cbfbd73..59c8acb55680 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -358,7 +358,7 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n \t}\n\n \tif (verbosity > 0)\n-\t\tfprintf(stderr, _(\"Pushing to %s\\n\"), transport->url);\n+\t\tfprintf(stderr, _(\"Pushing to %s\\n\"), anon_url);\n \ttrace2_region_enter(\"push\", \"transport_push\", the_repository);\n \terr = transport_push(the_repository, transport,\n \t\t\t rs, flags, &reject_reasons);\n\nAnd it's not like we change the URL in `push_with_options()`: it is\nexpected to remain unmodified throughout this function.\n\nDo you want me to send a v2 with above diff squashed in, or can you apply\nlocally (unless more reviews trickle in that require changes)?\n\nCiao,\nDscho\n\n>\n> builtin/push.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/push.c b/builtin/push.c\n> index bd2a2cbfbd..b88948b07e 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -340,7 +340,6 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n> {\n> \tint err;\n> \tunsigned int reject_reasons;\n> -\tchar *anon_url = transport_anonymize_url(transport->url);\n>\n> \ttransport_set_verbosity(transport, verbosity, progress);\n> \ttransport->family = family;\n> @@ -364,13 +363,14 @@ static int push_with_options(struct transport *transport, struct refspec *rs,\n> \t\t\t rs, flags, &reject_reasons);\n> \ttrace2_region_leave(\"push\", \"transport_push\", the_repository);\n> \tif (err != 0) {\n> +\t\tchar *anon_url = transport_anonymize_url(transport->url);\n> \t\tfprintf(stderr, \"%s\", push_get_color(PUSH_COLOR_ERROR));\n> \t\terror(_(\"failed to push some refs to '%s'\"), anon_url);\n> \t\tfprintf(stderr, \"%s\", push_get_color(PUSH_COLOR_RESET));\n> +\t\tfree(anon_url);\n> \t}\n>\n> \terr |= transport_disconnect(transport);\n> -\tfree(anon_url);\n> \tif (!err)\n> \t\treturn 0;\n>\n>\n>\n"},{"id":"396186","messageId":"xmqqh7x8bmub.fsf@gitster.c.googlers.com","threadId":"53308","inReplyTo":"nycvar.QRO.7.76.6.2004242259520.18039@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] push: anonymize URLs in error messages and warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T21:22:36Z","receivedAt":"2020-04-24T21:22:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> \tif (verbosity > 0)\n> -\t\tfprintf(stderr, _(\"Pushing to %s\\n\"), transport->url);\n> +\t\tfprintf(stderr, _(\"Pushing to %s\\n\"), anon_url);\n\nHeh, both of us did not see this? We must be tired.\n\nWill replace the squash one with this one liner and wait until dust\nsettles.\n\nThanks for a quick turnaround.\n"}]}