Re: [PATCH] clone/fetch: anonymize URLs in the reflog

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 2, 2020, 16:55 UTC
Message-ID: <xmqqimg9tnkk.fsf@gitster.c.googlers.com>
In-Reply-To: <20200601214715.GB3309882@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> On Mon, Jun 01, 2020 at 07:20:02PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> 
>> Even if we strongly discourage putting credentials into the URLs passed
>> via the command-line, there _is_ support for that, and users _do_ do
>> that.
>> 
>> Let's scrub them before writing them to the reflog.
>
> Good idea.
>
>>     This came up in an internal audit, but we do not consider this to be a
>>     big deal: the reflog is local and not really shared with anybody.
>
> Agreed.

Nice.

Show 36 quoted lines

>>  builtin/clone.c            | 10 ++++++----
>>  builtin/fetch.c            |  9 +++++++--
>>  t/t5541-http-push-smart.sh | 15 +++++++++++++++
>
> The patch itself looks very neatly done.
>
>> @@ -993,11 +993,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>  
>>  	path = get_repo_path(repo_name, &is_bundle);
>>  	if (path)
>> -		repo = absolute_pathdup(repo_name);
>> +		display_repo = repo = absolute_pathdup(repo_name);
>>  	else if (!strchr(repo_name, ':'))
>>  		die(_("repository '%s' does not exist"), repo_name);
>> -	else
>> +	else {
>>  		repo = repo_name;
>> +		display_repo = transport_anonymize_url(repo);
>> +	}
>
> Not introduced by your patch, but I had to read this a few times to make
> sure we always end up with repo and display_repo set. IMHO it would be
> easier to read as:
>
>   if (this) {
>      repo = ...;
>      display_repo = ...;
>   } else if (that) {
>      repo = ...;
>      display_repo = ...;
>   } else {
>      die(...);
>   }
>
> instead of sticking the die() in the middle.  Maybe just personal
> preference, though. :)

For a if/elseif cascade of few-liner blocks each, I do not think it would matter, but if a block were larger, having the die() case at the beginning or at the end would indeed make it easier to spot any anomalies.

Show 7 quoted lines

>> +	# should have been scrubbed down to vanilla URL
>> +	git log -g master >reflog &&
>> +	grep "$HTTPD_URL" reflog &&
>> +	! grep "$HTTPD_URL_USER_PASS" reflog
>> +'
>
> And you make sure we retain the username. Nice.