> On 23 Aug 2017, at 11:49, Ivan Vyshnevskyi <sainaen@xxxxxxxxx> wrote: > > Commits 47abd85 (fetch: Strip usernames from url's before storing them, > 2009-04-17) and later 882d49c (push: anonymize URL in status output, > 2016-07-14) made fetch and push strip the authentication part of the > remote URLs when used in the merge-commit messages or status outputs. > The URLs that are part of the error messages were not anonymized. > > A commonly used pattern for storing artifacts from a build server in a > remote repository utilizes a "secure" environment variable with > credentials to embed them in the URL and execute a push. Given enough > runs, an intermittent network failure will cause a push to fail, leaving > a non-anonymized URL in the build log. > > To prevent that, reuse the same anonymizing function to scrub > credentials from URL in the push error output. > > Signed-off-by: Ivan Vyshnevskyi <sainaen@xxxxxxxxx> > --- > > This is my first attempt to propose a patch, sorry if I did something wrong! > > I've tested my changes on Travis CI, and the build is green [1]. > > Not sure how much of the background should be included in the commit message. > The "commonly used pattern" I mention could be found in the myriad of > online tutorials and looks something like this: > > git push -fq https://$GIT_CREDS@xxxxxxxxxx/$REPO_SLUG > > Note, that a lot of developers mistakenly assume that '--quiet' ('-q') will > suppress all output. Sometimes, they would redirect standard output to > /dev/null, without realizing that errors are printed out to stderr. > > I didn't mention this in the commit, but another typical offender is a tool that > calls 'git push' as part of its execution. This is a spectrum that starts with > shell scripts, includes simple one-task apps in Python or Ruby, and ends with > the plugins for JavaScript, Java, Groovy, and Scala build tools. (I'd like to > avoid shaming their authors publicly, but could send you a few examples > privately.) > > I gathered the data by going through build logs of popular open source projects > (and projects of their contributors) hosted on GitHub and built by Travis CI. > I found about 2.3k unique credentials (but only about nine hundred were active > at the time), and more than a half of those were exposed by a failed push. See > the advisory from Travis CI [2] for results of their own scan. > > While the issue is public for several months now and addressed on Travis CI, > I keep finding build logs with credentials on the internet. So I think it's > worth fixing in the git itself. > > [1]: https://travis-ci.org/sainaen/git/builds/267180560 > [2]: https://blog.travis-ci.com/2017-05-08-security-advisory > This sounds very reasonable to me! Thanks for the contribution! I wonder if we should even extend this. Consider this case: $ git push https://lars:secret@server/org/repo1 remote: Invalid username or password. fatal: Authentication failed for 'https://lars:secret@server/org/repo1/' I might not have valid credentials for repo1 but my credentials could very well be valid for repo2. - Lars > builtin/push.c | 2 +- > t/t5541-http-push-smart.sh | 18 ++++++++++++++++++ > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/builtin/push.c b/builtin/push.c > index 03846e837..59f3bc975 100644 > --- a/builtin/push.c > +++ b/builtin/push.c > @@ -336,7 +336,7 @@ static int push_with_options(struct transport *transport, int flags) > err = transport_push(transport, refspec_nr, refspec, flags, > &reject_reasons); > if (err != 0) > - error(_("failed to push some refs to '%s'"), transport->url); > + error(_("failed to push some refs to '%s'"), transport_anonymize_url(transport->url)); > > err |= transport_disconnect(transport); > if (!err) > diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh > index d38bf3247..0b6fb6252 100755 > --- a/t/t5541-http-push-smart.sh > +++ b/t/t5541-http-push-smart.sh > @@ -377,5 +377,23 @@ test_expect_success 'push status output scrubs password' ' > grep "^To $HTTPD_URL/smart/test_repo.git" status > ' > > +cat >"$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update" <<EOF > +#!/bin/sh > +exit 1 > +EOF > +chmod a+x "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update" > + > +cat >exp <<EOF > +error: failed to push some refs to '$HTTPD_URL/smart/test_repo.git' > +EOF > + > +test_expect_success 'failed push status output scrubs password' ' > + cd "$ROOT_PATH"/test_repo_clone && > + test_must_fail git push "$HTTPD_URL_USER_PASS/smart/test_repo.git" +HEAD:scrub_err 2>stderr && > + grep "^error: failed to push some refs" stderr >act && > + test_i18ncmp exp act > +' > +rm -f "$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git/hooks/update" > + > stop_httpd > test_done > -- > 2.14.1 >
