git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Ivan Vyshnevskyi <sainaen@gmail.com>
To: git@vger.kernel.org
Cc: Brandon Williams <bmwill@google.com>, Jeff King <peff@peff.net>
Subject: [PATCH/RFC] push: anonymize URL in error output
Date: Wed, 23 Aug 2017 12:49:29 +0300	[thread overview]
Message-ID: <20170823094929.13541-1-sainaen@gmail.com> (raw)

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@gmail.com>
---

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@github.com/$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

 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


             reply	other threads:[~2017-08-23  9:50 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-23  9:49 Ivan Vyshnevskyi [this message]
2017-08-23 10:57 ` [PATCH/RFC] push: anonymize URL in error output Lars Schneider
2017-08-24 18:58   ` Ivan Vyshnevskyi
2017-08-23 15:58 ` Jeff King
2017-08-24 19:01   ` Ivan Vyshnevskyi
2017-08-25 19:37     ` Brandon Williams
2017-08-26 19:00       ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170823094929.13541-1-sainaen@gmail.com \
    --to=sainaen@gmail.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).