{"thread":{"id":"46658","subject":"[PATCH/RFC] push: anonymize URL in error output","startedAt":"2017-08-23T09:50:22Z","lastAt":"2017-08-26T19:01:03Z","messageCount":7,"participants":["Ivan Vyshnevskyi","Lars Schneider","Jeff King","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"327014","messageId":"20170823094929.13541-1-sainaen@gmail.com","threadId":"46658","inReplyTo":null,"subject":"[PATCH/RFC] push: anonymize URL in error output","fromName":"Ivan Vyshnevskyi","fromEmail":"sainaen@gmail.com","sentAt":"2017-08-23T09:49:29Z","receivedAt":"2017-08-23T09:50:22Z","isPatch":true,"sender":{"key":"sainaen@gmail.com","avatar":"https://gravatar.com/avatar/88c765029ffb3b0f5de502399851496bce084c76b70da2ef8f1e21a1cb377963?d=mp&s=160"},"body":"Commits 47abd85 (fetch: Strip usernames from url's before storing them,\n2009-04-17) and later 882d49c (push: anonymize URL in status output,\n2016-07-14) made fetch and push strip the authentication part of the\nremote URLs when used in the merge-commit messages or status outputs.\nThe URLs that are part of the error messages were not anonymized.\n\nA commonly used pattern for storing artifacts from a build server in a\nremote repository utilizes a \"secure\" environment variable with\ncredentials to embed them in the URL and execute a push. Given enough\nruns, an intermittent network failure will cause a push to fail, leaving\na non-anonymized URL in the build log.\n\nTo prevent that, reuse the same anonymizing function to scrub\ncredentials from URL in the push error output.\n\nSigned-off-by: Ivan Vyshnevskyi <sainaen@gmail.com>\n---\n\nThis is my first attempt to propose a patch, sorry if I did something wrong!\n\nI've tested my changes on Travis CI, and the build is green [1].\n\nNot sure how much of the background should be included in the commit message.\nThe \"commonly used pattern\" I mention could be found in the myriad of\nonline tutorials and looks something like this:\n\n    git push -fq https://$GIT_CREDS@github.com/$REPO_SLUG\n\nNote, that a lot of developers mistakenly assume that '--quiet' ('-q') will\nsuppress all output. Sometimes, they would redirect standard output to\n/dev/null, without realizing that errors are printed out to stderr.\n\nI didn't mention this in the commit, but another typical offender is a tool that\ncalls 'git push' as part of its execution. This is a spectrum that starts with\nshell scripts, includes simple one-task apps in Python or Ruby, and ends with\nthe plugins for JavaScript, Java, Groovy, and Scala build tools. (I'd like to\navoid shaming their authors publicly, but could send you a few examples\nprivately.)\n\nI gathered the data by going through build logs of popular open source projects\n(and projects of their contributors) hosted on GitHub and built by Travis CI.\nI found about 2.3k unique credentials (but only about nine hundred were active\nat the time), and more than a half of those were exposed by a failed push. See\nthe advisory from Travis CI [2] for results of their own scan.\n\nWhile the issue is public for several months now and addressed on Travis CI,\nI keep finding build logs with credentials on the internet. So I think it's\nworth fixing in the git itself.\n\n[1]: https://travis-ci.org/sainaen/git/builds/267180560\n[2]: https://blog.travis-ci.com/2017-05-08-security-advisory\n\n builtin/push.c             |  2 +-\n t/t5541-http-push-smart.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 03846e837..59f3bc975 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -336,7 +336,7 @@ static int push_with_options(struct transport *transport, int flags)\n \terr = transport_push(transport, refspec_nr, refspec, flags,\n \t\t\t     &reject_reasons);\n \tif (err != 0)\n-\t\terror(_(\"failed to push some refs to '%s'\"), transport->url);\n+\t\terror(_(\"failed to push some refs to '%s'\"), transport_anonymize_url(transport->url));\n \n \terr |= transport_disconnect(transport);\n \tif (!err)\ndiff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\nindex d38bf3247..0b6fb6252 100755\n--- a/t/t5541-http-push-smart.sh\n+++ b/t/t5541-http-push-smart.sh\n@@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs password' '\n \tgrep \"^To $HTTPD_URL/smart/test_repo.git\" status\n '\n \n+cat >\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" <<EOF\n+#!/bin/sh\n+exit 1\n+EOF\n+chmod a+x \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n+\n+cat >exp <<EOF\n+error: failed to push some refs to '$HTTPD_URL/smart/test_repo.git'\n+EOF\n+\n+test_expect_success 'failed push status output scrubs password' '\n+\tcd \"$ROOT_PATH\"/test_repo_clone &&\n+\ttest_must_fail git push \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" +HEAD:scrub_err 2>stderr &&\n+\tgrep \"^error: failed to push some refs\" stderr >act &&\n+\ttest_i18ncmp exp act\n+'\n+rm -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n+\n stop_httpd\n test_done\n-- \n2.14.1\n\n"},{"id":"327018","messageId":"698B5636-BE6B-47EF-BE4B-1DB590E0028F@gmail.com","threadId":"46658","inReplyTo":"20170823094929.13541-1-sainaen@gmail.com","subject":"Re: [PATCH/RFC] push: anonymize URL in error output","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-08-23T10:57:27Z","receivedAt":"2017-08-23T10:57:37Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 23 Aug 2017, at 11:49, Ivan Vyshnevskyi <sainaen@gmail.com> wrote:\n> \n> Commits 47abd85 (fetch: Strip usernames from url's before storing them,\n> 2009-04-17) and later 882d49c (push: anonymize URL in status output,\n> 2016-07-14) made fetch and push strip the authentication part of the\n> remote URLs when used in the merge-commit messages or status outputs.\n> The URLs that are part of the error messages were not anonymized.\n> \n> A commonly used pattern for storing artifacts from a build server in a\n> remote repository utilizes a \"secure\" environment variable with\n> credentials to embed them in the URL and execute a push. Given enough\n> runs, an intermittent network failure will cause a push to fail, leaving\n> a non-anonymized URL in the build log.\n> \n> To prevent that, reuse the same anonymizing function to scrub\n> credentials from URL in the push error output.\n> \n> Signed-off-by: Ivan Vyshnevskyi <sainaen@gmail.com>\n> ---\n> \n> This is my first attempt to propose a patch, sorry if I did something wrong!\n> \n> I've tested my changes on Travis CI, and the build is green [1].\n> \n> Not sure how much of the background should be included in the commit message.\n> The \"commonly used pattern\" I mention could be found in the myriad of\n> online tutorials and looks something like this:\n> \n>    git push -fq https://$GIT_CREDS@github.com/$REPO_SLUG\n> \n> Note, that a lot of developers mistakenly assume that '--quiet' ('-q') will\n> suppress all output. Sometimes, they would redirect standard output to\n> /dev/null, without realizing that errors are printed out to stderr.\n> \n> I didn't mention this in the commit, but another typical offender is a tool that\n> calls 'git push' as part of its execution. This is a spectrum that starts with\n> shell scripts, includes simple one-task apps in Python or Ruby, and ends with\n> the plugins for JavaScript, Java, Groovy, and Scala build tools. (I'd like to\n> avoid shaming their authors publicly, but could send you a few examples\n> privately.)\n> \n> I gathered the data by going through build logs of popular open source projects\n> (and projects of their contributors) hosted on GitHub and built by Travis CI.\n> I found about 2.3k unique credentials (but only about nine hundred were active\n> at the time), and more than a half of those were exposed by a failed push. See\n> the advisory from Travis CI [2] for results of their own scan.\n> \n> While the issue is public for several months now and addressed on Travis CI,\n> I keep finding build logs with credentials on the internet. So I think it's\n> worth fixing in the git itself.\n> \n> [1]: https://travis-ci.org/sainaen/git/builds/267180560\n> [2]: https://blog.travis-ci.com/2017-05-08-security-advisory\n> \n\nThis sounds very reasonable to me! Thanks for the contribution!\n\nI wonder if we should even extend this. Consider this case:\n\n  $ git push https://lars:secret@server/org/repo1\n  remote: Invalid username or password.\n  fatal: Authentication failed for 'https://lars:secret@server/org/repo1/'\n\nI might not have valid credentials for repo1 but my credentials could\nvery well be valid for repo2.\n\n- Lars\n\n\n> builtin/push.c             |  2 +-\n> t/t5541-http-push-smart.sh | 18 ++++++++++++++++++\n> 2 files changed, 19 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/push.c b/builtin/push.c\n> index 03846e837..59f3bc975 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -336,7 +336,7 @@ static int push_with_options(struct transport *transport, int flags)\n> \terr = transport_push(transport, refspec_nr, refspec, flags,\n> \t\t\t     &reject_reasons);\n> \tif (err != 0)\n> -\t\terror(_(\"failed to push some refs to '%s'\"), transport->url);\n> +\t\terror(_(\"failed to push some refs to '%s'\"), transport_anonymize_url(transport->url));\n> \n> \terr |= transport_disconnect(transport);\n> \tif (!err)\n> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\n> index d38bf3247..0b6fb6252 100755\n> --- a/t/t5541-http-push-smart.sh\n> +++ b/t/t5541-http-push-smart.sh\n> @@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs password' '\n> \tgrep \"^To $HTTPD_URL/smart/test_repo.git\" status\n> '\n> \n> +cat >\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" <<EOF\n> +#!/bin/sh\n> +exit 1\n> +EOF\n> +chmod a+x \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n> +\n> +cat >exp <<EOF\n> +error: failed to push some refs to '$HTTPD_URL/smart/test_repo.git'\n> +EOF\n> +\n> +test_expect_success 'failed push status output scrubs password' '\n> +\tcd \"$ROOT_PATH\"/test_repo_clone &&\n> +\ttest_must_fail git push \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" +HEAD:scrub_err 2>stderr &&\n> +\tgrep \"^error: failed to push some refs\" stderr >act &&\n> +\ttest_i18ncmp exp act\n> +'\n> +rm -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n> +\n> stop_httpd\n> test_done\n> -- \n> 2.14.1\n> \n\n"},{"id":"327047","messageId":"20170823155826.m4s5y55x2esfoass@sigill.intra.peff.net","threadId":"46658","inReplyTo":"20170823094929.13541-1-sainaen@gmail.com","subject":"Re: [PATCH/RFC] push: anonymize URL in error output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-23T15:58:26Z","receivedAt":"2017-08-23T15:58:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 23, 2017 at 12:49:29PM +0300, Ivan Vyshnevskyi wrote:\n\n> Commits 47abd85 (fetch: Strip usernames from url's before storing them,\n> 2009-04-17) and later 882d49c (push: anonymize URL in status output,\n> 2016-07-14) made fetch and push strip the authentication part of the\n> remote URLs when used in the merge-commit messages or status outputs.\n> The URLs that are part of the error messages were not anonymized.\n> \n> A commonly used pattern for storing artifacts from a build server in a\n> remote repository utilizes a \"secure\" environment variable with\n> credentials to embed them in the URL and execute a push. Given enough\n> runs, an intermittent network failure will cause a push to fail, leaving\n> a non-anonymized URL in the build log.\n> \n> To prevent that, reuse the same anonymizing function to scrub\n> credentials from URL in the push error output.\n\nThis makes sense. I suspect that most errors we output should be using\nthe anonymized URL. Did you poke around for other calls?\n\nThe general structure of the patch looks good, but I have a few minor\ncomments below.\n\n> Not sure how much of the background should be included in the commit message.\n> The \"commonly used pattern\" I mention could be found in the myriad of\n> online tutorials and looks something like this:\n\nMy knee-jerk reaction is if it's worth writing after the dashes, it's\nworth putting in the commit message.\n\nHowever, in the case I think it is OK as-is (the motivation of \"we\nalready avoid leaking auth info to stdout, so we should do the same for\nerror messages\" seems self-contained and reasonable).\n\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 03846e837..59f3bc975 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -336,7 +336,7 @@ static int push_with_options(struct transport *transport, int flags)\n>  \terr = transport_push(transport, refspec_nr, refspec, flags,\n>  \t\t\t     &reject_reasons);\n>  \tif (err != 0)\n> -\t\terror(_(\"failed to push some refs to '%s'\"), transport->url);\n> +\t\terror(_(\"failed to push some refs to '%s'\"), transport_anonymize_url(transport->url));\n\nThis leaks the return value. That's probably not a _huge_ deal since the\nprogram is likely to exit, but it's a bad pattern. I wonder if we should\nbe setting up transport->anonymous_url preemptively, and just let its\nmemory belong to the transport struct.\n\n> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\n> index d38bf3247..0b6fb6252 100755\n> --- a/t/t5541-http-push-smart.sh\n> +++ b/t/t5541-http-push-smart.sh\n> @@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs password' '\n>  \tgrep \"^To $HTTPD_URL/smart/test_repo.git\" status\n>  '\n>  \n> +cat >\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" <<EOF\n> +#!/bin/sh\n> +exit 1\n> +EOF\n> +chmod a+x \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n> +\n> +cat >exp <<EOF\n> +error: failed to push some refs to '$HTTPD_URL/smart/test_repo.git'\n> +EOF\n\nI know the t5541 script, which is old and messy, led you into these bad\nconstructs. But usually in modern tests we:\n\n 1. Try to keep all commands inside test_expect blocks to catch\n    unexpected failures or unwanted output.\n\n 2. Use write_script for writing scripts, like:\n\n      write_script \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" <<-\\EOF\n      exit 1\n      EOF\n\n 3. Backslash our here-doc delimiter to suppress interpolation.\n\n> +test_expect_success 'failed push status output scrubs password' '\n> +\tcd \"$ROOT_PATH\"/test_repo_clone &&\n> +\ttest_must_fail git push \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" +HEAD:scrub_err 2>stderr &&\n> +\tgrep \"^error: failed to push some refs\" stderr >act &&\n> +\ttest_i18ncmp exp act\n> +'\n> +rm -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n\nSimilarly, this \"rm\" should probably be a test_when_finished in the\nblock with the write_script (unless you really need to carry it over\nseveral test_expect blocks, in which case there should be an explicit\ntest_expect cleaning it up).\n\nInstead of grepping for the exact error, should we instead grep for the\npassword to make sure it is not present on _any_ line?\n\n-Peff\n"},{"id":"327143","messageId":"4a1afb31-e240-fda5-416d-b85c968819f4@gmail.com","threadId":"46658","inReplyTo":"698B5636-BE6B-47EF-BE4B-1DB590E0028F@gmail.com","subject":"Re: [PATCH/RFC] push: anonymize URL in error output","fromName":"Ivan Vyshnevskyi","fromEmail":"sainaen@gmail.com","sentAt":"2017-08-24T18:58:57Z","receivedAt":"2017-08-24T18:59:06Z","isPatch":true,"sender":{"key":"sainaen@gmail.com","avatar":"https://gravatar.com/avatar/88c765029ffb3b0f5de502399851496bce084c76b70da2ef8f1e21a1cb377963?d=mp&s=160"},"body":"On 23/08/17 13:57, Lars Schneider wrote:\n> \n>> On 23 Aug 2017, at 11:49, Ivan Vyshnevskyi <sainaen@gmail.com> wrote:\n>>\n>> Commits 47abd85 (fetch: Strip usernames from url's before storing them,\n>> 2009-04-17) and later 882d49c (push: anonymize URL in status output,\n>> 2016-07-14) made fetch and push strip the authentication part of the\n>> remote URLs when used in the merge-commit messages or status outputs.\n>> The URLs that are part of the error messages were not anonymized.\n>>\n>> A commonly used pattern for storing artifacts from a build server in a\n>> remote repository utilizes a \"secure\" environment variable with\n>> credentials to embed them in the URL and execute a push. Given enough\n>> runs, an intermittent network failure will cause a push to fail, leaving\n>> a non-anonymized URL in the build log.\n>>\n>> To prevent that, reuse the same anonymizing function to scrub\n>> credentials from URL in the push error output.\n>>\n>> Signed-off-by: Ivan Vyshnevskyi <sainaen@gmail.com>\n>> ---\n>>\n>> This is my first attempt to propose a patch, sorry if I did something wrong!\n>>\n>> I've tested my changes on Travis CI, and the build is green [1].\n>>\n>> Not sure how much of the background should be included in the commit message.\n>> The \"commonly used pattern\" I mention could be found in the myriad of\n>> online tutorials and looks something like this:\n>>\n>>    git push -fq https://$GIT_CREDS@github.com/$REPO_SLUG\n>>\n>> Note, that a lot of developers mistakenly assume that '--quiet' ('-q') will\n>> suppress all output. Sometimes, they would redirect standard output to\n>> /dev/null, without realizing that errors are printed out to stderr.\n>>\n>> I didn't mention this in the commit, but another typical offender is a tool that\n>> calls 'git push' as part of its execution. This is a spectrum that starts with\n>> shell scripts, includes simple one-task apps in Python or Ruby, and ends with\n>> the plugins for JavaScript, Java, Groovy, and Scala build tools. (I'd like to\n>> avoid shaming their authors publicly, but could send you a few examples\n>> privately.)\n>>\n>> I gathered the data by going through build logs of popular open source projects\n>> (and projects of their contributors) hosted on GitHub and built by Travis CI.\n>> I found about 2.3k unique credentials (but only about nine hundred were active\n>> at the time), and more than a half of those were exposed by a failed push. See\n>> the advisory from Travis CI [2] for results of their own scan.\n>>\n>> While the issue is public for several months now and addressed on Travis CI,\n>> I keep finding build logs with credentials on the internet. So I think it's\n>> worth fixing in the git itself.\n>>\n>> [1]: https://travis-ci.org/sainaen/git/builds/267180560\n>> [2]: https://blog.travis-ci.com/2017-05-08-security-advisory\n>>\n> \n> This sounds very reasonable to me! Thanks for the contribution!>\nThank you!\n\n> I wonder if we should even extend this. Consider this case:\n> \n>   $ git push https://lars:secret@server/org/repo1\n>   remote: Invalid username or password.\n>   fatal: Authentication failed for 'https://lars:secret@server/org/repo1/'\n> \n> I might not have valid credentials for repo1 but my credentials could\n> very well be valid for repo2.\n> \n> - Lars\n> \nYeah, you're right. In fact, there was a similar scenario:\n1. maintainer creates a \"<something>-bot\" GitHub account to use for\npushing from CI back to the repository, but forgets to add this new\naccount to the project's organization\n2. then they update the build to do the push with new credentials\n3. auto-triggered build fails because the \"*-bot\" has no access yet\n\nAfter that, they'd typically revoke exposed token and create a new one,\nbut sometimes they forget, and so the active token stays in the build log.\n\nI found the place where this and couple of other errors seem to be\nemitted ('discover_refs()' in remote-curl.c), but, to be honest, it just\nseemed harder to figure out for the first patch: do I include the\ntransport.h here just to use this one function or should I copy it over?\nOr move it to url.c (I guess?) and replace other usages?\n\nThough, with some help, I'd be happy to tackle harder cases too. :)\n\n> \n>> builtin/push.c             |  2 +-\n>> t/t5541-http-push-smart.sh | 18 ++++++++++++++++++\n>> 2 files changed, 19 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/push.c b/builtin/push.c\n>> index 03846e837..59f3bc975 100644\n>> --- a/builtin/push.c\n>> +++ b/builtin/push.c\n>> @@ -336,7 +336,7 @@ static int push_with_options(struct transport *transport, int flags)\n>> \terr = transport_push(transport, refspec_nr, refspec, flags,\n>> \t\t\t     &reject_reasons);\n>> \tif (err != 0)\n>> -\t\terror(_(\"failed to push some refs to '%s'\"), transport->url);\n>> +\t\terror(_(\"failed to push some refs to '%s'\"), transport_anonymize_url(transport->url));\n>>\n>> \terr |= transport_disconnect(transport);\n>> \tif (!err)\n>> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\n>> index d38bf3247..0b6fb6252 100755\n>> --- a/t/t5541-http-push-smart.sh\n>> +++ b/t/t5541-http-push-smart.sh\n>> @@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs password' '\n>> \tgrep \"^To $HTTPD_URL/smart/test_repo.git\" status\n>> '\n>>\n>> +cat >\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" <<EOF\n>> +#!/bin/sh\n>> +exit 1\n>> +EOF\n>> +chmod a+x \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n>> +\n>> +cat >exp <<EOF\n>> +error: failed to push some refs to '$HTTPD_URL/smart/test_repo.git'\n>> +EOF\n>> +\n>> +test_expect_success 'failed push status output scrubs password' '\n>> +\tcd \"$ROOT_PATH\"/test_repo_clone &&\n>> +\ttest_must_fail git push \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" +HEAD:scrub_err 2>stderr &&\n>> +\tgrep \"^error: failed to push some refs\" stderr >act &&\n>> +\ttest_i18ncmp exp act\n>> +'\n>> +rm -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n>> +\n>> stop_httpd\n>> test_done\n>> -- \n>> 2.14.1\n>>\n"},{"id":"327144","messageId":"82741094-19a6-e071-227d-f92b3b077a69@gmail.com","threadId":"46658","inReplyTo":"20170823155826.m4s5y55x2esfoass@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] push: anonymize URL in error output","fromName":"Ivan Vyshnevskyi","fromEmail":"sainaen@gmail.com","sentAt":"2017-08-24T19:01:50Z","receivedAt":"2017-08-24T19:01:59Z","isPatch":true,"sender":{"key":"sainaen@gmail.com","avatar":"https://gravatar.com/avatar/88c765029ffb3b0f5de502399851496bce084c76b70da2ef8f1e21a1cb377963?d=mp&s=160"},"body":"On 23/08/17 18:58, Jeff King wrote:\n> On Wed, Aug 23, 2017 at 12:49:29PM +0300, Ivan Vyshnevskyi wrote:\n> \n>> Commits 47abd85 (fetch: Strip usernames from url's before storing them,\n>> 2009-04-17) and later 882d49c (push: anonymize URL in status output,\n>> 2016-07-14) made fetch and push strip the authentication part of the\n>> remote URLs when used in the merge-commit messages or status outputs.\n>> The URLs that are part of the error messages were not anonymized.\n>>\n>> A commonly used pattern for storing artifacts from a build server in a\n>> remote repository utilizes a \"secure\" environment variable with\n>> credentials to embed them in the URL and execute a push. Given enough\n>> runs, an intermittent network failure will cause a push to fail, leaving\n>> a non-anonymized URL in the build log.\n>>\n>> To prevent that, reuse the same anonymizing function to scrub\n>> credentials from URL in the push error output.\n> \n> This makes sense. I suspect that most errors we output should be using\n> the anonymized URL. Did you poke around for other calls?\nYes, I tried to check and unfortunately there are couple of places with\npossible leaks:\n* 'discover_refs()' in remote-curl.c when there's a HTTP error (see a\nreal-life scenario with an authz error in my response to Lars) -- is it\nok to include transport.h just to use one function or is there a cleaner\nway?\n* 'setup_push_upstream()' in push.c when a command doesn't have a branch\nnames (haven't saw problems with this in the wild, but could occur\nduring the CI setup) -- for this one, probably anonymization should\nhappen when the 'remote->name' field is set in the 'make_remote()'; same\nquestion though, is it ok to include transport.h here?\n\nAlso there's an case of verbose output: I'm not sure I should change it,\nbut it does print out the non-anonymized URLs at least during push.\n> \n> The general structure of the patch looks good, but I have a few minor\n> comments below.\n> \n>> Not sure how much of the background should be included in the commit message.\n>> The \"commonly used pattern\" I mention could be found in the myriad of\n>> online tutorials and looks something like this:\n> \n> My knee-jerk reaction is if it's worth writing after the dashes, it's\n> worth putting in the commit message.\n> \n> However, in the case I think it is OK as-is (the motivation of \"we\n> already avoid leaking auth info to stdout, so we should do the same for\n> error messages\" seems self-contained and reasonable)\nWell, I tend to be wordy, and most of the commit messages I saw were\nrather short, so decided to split. Wonder, if maybe example command\nshould be included without the rest of it. Would it be useful?\n> \n>> diff --git a/builtin/push.c b/builtin/push.c\n>> index 03846e837..59f3bc975 100644\n>> --- a/builtin/push.c\n>> +++ b/builtin/push.c\n>> @@ -336,7 +336,7 @@ static int push_with_options(struct transport *transport, int flags)\n>>  \terr = transport_push(transport, refspec_nr, refspec, flags,\n>>  \t\t\t     &reject_reasons);\n>>  \tif (err != 0)\n>> -\t\terror(_(\"failed to push some refs to '%s'\"), transport->url);\n>> +\t\terror(_(\"failed to push some refs to '%s'\"), transport_anonymize_url(transport->url));\n> \n> This leaks the return value. That's probably not a _huge_ deal since the\n> program is likely to exit, but it's a bad pattern. I wonder if we should\n> be setting up transport->anonymous_url preemptively, and just let its\n> memory belong to the transport struct.\nAh. Thanks! I knew I'd fail in the memory management even with the\none-line patch. :)\n\nAbout 'transport->anonymous_url': not sure if it's worth it. There are\nfour calls to 'transport_anonymize_url' right now and it looks like the\nnew one in my patch is the first that has a transport struct instance\nnear by. The next likely candidate for update 'discover_refs()' also\ngets the url as an argument.\n> \n>> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\n>> index d38bf3247..0b6fb6252 100755\n>> --- a/t/t5541-http-push-smart.sh\n>> +++ b/t/t5541-http-push-smart.sh\n>> @@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs password' '\n>>  \tgrep \"^To $HTTPD_URL/smart/test_repo.git\" status\n>>  '\n>>  \n>> +cat >\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" <<EOF\n>> +#!/bin/sh\n>> +exit 1\n>> +EOF\n>> +chmod a+x \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n>> +\n>> +cat >exp <<EOF\n>> +error: failed to push some refs to '$HTTPD_URL/smart/test_repo.git'\n>> +EOF\n> \n> I know the t5541 script, which is old and messy, led you into these bad\n> constructs. But usually in modern tests we:\n> \n>  1. Try to keep all commands inside test_expect blocks to catch\n>     unexpected failures or unwanted output.\n> \n>  2. Use write_script for writing scripts, like:\n> \n>       write_script \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" <<-\\EOF\n>       exit 1\n>       EOF\n> \n>  3. Backslash our here-doc delimiter to suppress interpolation.\n> \n>> +test_expect_success 'failed push status output scrubs password' '\n>> +\tcd \"$ROOT_PATH\"/test_repo_clone &&\n>> +\ttest_must_fail git push \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" +HEAD:scrub_err 2>stderr &&\n>> +\tgrep \"^error: failed to push some refs\" stderr >act &&\n>> +\ttest_i18ncmp exp act\n>> +'\n>> +rm -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n> \n> Similarly, this \"rm\" should probably be a test_when_finished in the\n> block with the write_script (unless you really need to carry it over\n> several test_expect blocks, in which case there should be an explicit\n> test_expect cleaning it up).\nThanks! You're right. I just followed examples in the file.\nUpdated [1], will send with the next patch version.\n\n> \n> Instead of grepping for the exact error, should we instead grep for the\n> password to make sure it is not present on _any_ line?\n> \n> -Peff\n> \nOne possible issue I see is that this will make it overlap with the\n'push status output scrubs password' case above. But if it's not a\nproblem, I can replace last two lines with just a 'test_i18ngrep !'\n\n[1]:\nhttps://github.com/sainaen/git/blob/af17713/t/t5541-http-push-smart.sh#L380-L392\n"},{"id":"327217","messageId":"20170825193743.GD103659@google.com","threadId":"46658","inReplyTo":"82741094-19a6-e071-227d-f92b3b077a69@gmail.com","subject":"Re: [PATCH/RFC] push: anonymize URL in error output","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-08-25T19:37:43Z","receivedAt":"2017-08-25T19:37:52Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/24, Ivan Vyshnevskyi wrote:\n> On 23/08/17 18:58, Jeff King wrote:\n> > On Wed, Aug 23, 2017 at 12:49:29PM +0300, Ivan Vyshnevskyi wrote:\n> > \n> >> Commits 47abd85 (fetch: Strip usernames from url's before storing them,\n> >> 2009-04-17) and later 882d49c (push: anonymize URL in status output,\n> >> 2016-07-14) made fetch and push strip the authentication part of the\n> >> remote URLs when used in the merge-commit messages or status outputs.\n> >> The URLs that are part of the error messages were not anonymized.\n> >>\n> >> A commonly used pattern for storing artifacts from a build server in a\n> >> remote repository utilizes a \"secure\" environment variable with\n> >> credentials to embed them in the URL and execute a push. Given enough\n> >> runs, an intermittent network failure will cause a push to fail, leaving\n> >> a non-anonymized URL in the build log.\n> >>\n> >> To prevent that, reuse the same anonymizing function to scrub\n> >> credentials from URL in the push error output.\n> > \n> > This makes sense. I suspect that most errors we output should be using\n> > the anonymized URL. Did you poke around for other calls?\n> Yes, I tried to check and unfortunately there are couple of places with\n> possible leaks:\n> * 'discover_refs()' in remote-curl.c when there's a HTTP error (see a\n> real-life scenario with an authz error in my response to Lars) -- is it\n> ok to include transport.h just to use one function or is there a cleaner\n> way?\n> * 'setup_push_upstream()' in push.c when a command doesn't have a branch\n> names (haven't saw problems with this in the wild, but could occur\n> during the CI setup) -- for this one, probably anonymization should\n> happen when the 'remote->name' field is set in the 'make_remote()'; same\n> question though, is it ok to include transport.h here?\n> \n> Also there's an case of verbose output: I'm not sure I should change it,\n> but it does print out the non-anonymized URLs at least during push.\n> > \n> > The general structure of the patch looks good, but I have a few minor\n> > comments below.\n> > \n> >> Not sure how much of the background should be included in the commit message.\n> >> The \"commonly used pattern\" I mention could be found in the myriad of\n> >> online tutorials and looks something like this:\n> > \n> > My knee-jerk reaction is if it's worth writing after the dashes, it's\n> > worth putting in the commit message.\n> > \n> > However, in the case I think it is OK as-is (the motivation of \"we\n> > already avoid leaking auth info to stdout, so we should do the same for\n> > error messages\" seems self-contained and reasonable)\n> Well, I tend to be wordy, and most of the commit messages I saw were\n> rather short, so decided to split. Wonder, if maybe example command\n> should be included without the rest of it. Would it be useful?\n\nI'm guilty of writing short commit messages (something I need to work\non) but when looking through logs I much prefer to see longer messages\nexplaining rationals and trade-offs.\n\n> > \n> >> diff --git a/builtin/push.c b/builtin/push.c\n> >> index 03846e837..59f3bc975 100644\n> >> --- a/builtin/push.c\n> >> +++ b/builtin/push.c\n> >> @@ -336,7 +336,7 @@ static int push_with_options(struct transport *transport, int flags)\n> >>  \terr = transport_push(transport, refspec_nr, refspec, flags,\n> >>  \t\t\t     &reject_reasons);\n> >>  \tif (err != 0)\n> >> -\t\terror(_(\"failed to push some refs to '%s'\"), transport->url);\n> >> +\t\terror(_(\"failed to push some refs to '%s'\"), transport_anonymize_url(transport->url));\n> > \n> > This leaks the return value. That's probably not a _huge_ deal since the\n> > program is likely to exit, but it's a bad pattern. I wonder if we should\n> > be setting up transport->anonymous_url preemptively, and just let its\n> > memory belong to the transport struct.\n> Ah. Thanks! I knew I'd fail in the memory management even with the\n> one-line patch. :)\n> \n> About 'transport->anonymous_url': not sure if it's worth it. There are\n> four calls to 'transport_anonymize_url' right now and it looks like the\n> new one in my patch is the first that has a transport struct instance\n> near by. The next likely candidate for update 'discover_refs()' also\n> gets the url as an argument.\n> > \n> >> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh\n> >> index d38bf3247..0b6fb6252 100755\n> >> --- a/t/t5541-http-push-smart.sh\n> >> +++ b/t/t5541-http-push-smart.sh\n> >> @@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs password' '\n> >>  \tgrep \"^To $HTTPD_URL/smart/test_repo.git\" status\n> >>  '\n> >>  \n> >> +cat >\"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" <<EOF\n> >> +#!/bin/sh\n> >> +exit 1\n> >> +EOF\n> >> +chmod a+x \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n> >> +\n> >> +cat >exp <<EOF\n> >> +error: failed to push some refs to '$HTTPD_URL/smart/test_repo.git'\n> >> +EOF\n> > \n> > I know the t5541 script, which is old and messy, led you into these bad\n> > constructs. But usually in modern tests we:\n> > \n> >  1. Try to keep all commands inside test_expect blocks to catch\n> >     unexpected failures or unwanted output.\n> > \n> >  2. Use write_script for writing scripts, like:\n> > \n> >       write_script \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\" <<-\\EOF\n> >       exit 1\n> >       EOF\n> > \n> >  3. Backslash our here-doc delimiter to suppress interpolation.\n> > \n> >> +test_expect_success 'failed push status output scrubs password' '\n> >> +\tcd \"$ROOT_PATH\"/test_repo_clone &&\n> >> +\ttest_must_fail git push \"$HTTPD_URL_USER_PASS/smart/test_repo.git\" +HEAD:scrub_err 2>stderr &&\n> >> +\tgrep \"^error: failed to push some refs\" stderr >act &&\n> >> +\ttest_i18ncmp exp act\n> >> +'\n> >> +rm -f \"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update\"\n> > \n> > Similarly, this \"rm\" should probably be a test_when_finished in the\n> > block with the write_script (unless you really need to carry it over\n> > several test_expect blocks, in which case there should be an explicit\n> > test_expect cleaning it up).\n> Thanks! You're right. I just followed examples in the file.\n> Updated [1], will send with the next patch version.\n> \n> > \n> > Instead of grepping for the exact error, should we instead grep for the\n> > password to make sure it is not present on _any_ line?\n> > \n> > -Peff\n> > \n> One possible issue I see is that this will make it overlap with the\n> 'push status output scrubs password' case above. But if it's not a\n> problem, I can replace last two lines with just a 'test_i18ngrep !'\n> \n> [1]:\n> https://github.com/sainaen/git/blob/af17713/t/t5541-http-push-smart.sh#L380-L392\n\n-- \nBrandon Williams\n"},{"id":"327249","messageId":"20170826190056.f53htagkoz6ydboe@sigill.intra.peff.net","threadId":"46658","inReplyTo":"20170825193743.GD103659@google.com","subject":"Re: [PATCH/RFC] push: anonymize URL in error output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-08-26T19:00:57Z","receivedAt":"2017-08-26T19:01:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 25, 2017 at 12:37:43PM -0700, Brandon Williams wrote:\n\n> > > My knee-jerk reaction is if it's worth writing after the dashes, it's\n> > > worth putting in the commit message.\n> > > \n> > > However, in the case I think it is OK as-is (the motivation of \"we\n> > > already avoid leaking auth info to stdout, so we should do the same for\n> > > error messages\" seems self-contained and reasonable)\n> > Well, I tend to be wordy, and most of the commit messages I saw were\n> > rather short, so decided to split. Wonder, if maybe example command\n> > should be included without the rest of it. Would it be useful?\n> \n> I'm guilty of writing short commit messages (something I need to work\n> on) but when looking through logs I much prefer to see longer messages\n> explaining rationals and trade-offs.\n\nI think as with all writing, there is both \"too short\" and \"too long\".\nI like having those extra bits in the commit message, too, but you have\nto make sure they don't drown out the main points.\n\nI find that when a message gets long, it often benefits from revising\nand re-ordering. E.g., having an intro paragraph that explains the\nmotivation and solution in one sentence, and _then_ go into the details\nfor people who are really digging into the details. Good organization\nlets readers get just the level they need and skip the rest.\n\nEasier said than done, of course. :)\n\n-Peff\n"}]}