Re: [PATCH 1/2] fetch-pack: redact packfile urls in traces

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 8, 2021, 19:36 UTC
Message-ID: <87zgrjmhgd.fsf@evledraar.gmail.com>
In-Reply-To: <b473f145a87a22db99734c6a21395f0d24c3da3c.1633708986.git.gitgitgadget@gmail.com>

On Fri, Oct 08 2021, Ivan Frade via GitGitGadget wrote:

Show 15 quoted lines

> diff --git a/http-fetch.c b/http-fetch.c
> index fa642462a9e..d35e33e4f65 100644
> --- a/http-fetch.c
> +++ b/http-fetch.c
> @@ -63,7 +63,9 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
>  	if (start_active_slot(preq->slot)) {
>  		run_active_slot(preq->slot);
>  		if (results.curl_result != CURLE_OK) {
> -			die("Unable to get pack file %s\n%s", preq->url,
> +			int showUrl = git_env_bool("GIT_TRACE_REDACT", 1);
> +			die("Unable to get offloaded pack file %s\n%s",
> +			    showUrl ? preq->url : "<redacted>",
>  			    curl_errorstr);
>  		}
>  	} else {

Your CL and commit message just talk about traes, but this is a die() message.

Perhaps it makes sense to redact it there too for some reason, but that seems to be a thing to separately argue for.

This message is shown interactively to users, and I could see it be annoying to not have the URL that failed in your terminal output, even if it has some one-time token.

Which is presumably different from the use-cases you're thinking of, I'm assuming some logging of detached processes, or central logging of user actions.

Show 19 quoted lines

> +test_expect_success 'packfile-uri redacted in trace' '
> +	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
> +	rm -rf "$P" http_child log &&
> +
> +	git init "$P" &&
> +	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
> +
> +	echo my-blob >"$P/my-blob" &&
> +	git -C "$P" add my-blob &&
> +	git -C "$P" commit -m x &&
> +
> +	configure_exclusion "$P" my-blob >h &&
> +
> +	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
> +	git -c protocol.version=2 \
> +		-c fetch.uriprotocols=http,https \
> +		clone "$HTTPD_URL/smart/http_parent" http_child &&
> +
> +	grep -A1 "clone<\ ..packfile-uris" log | grep "clone<\ <redacted>"

We don't rely on GNU options like those for the test suite, it'll break on various supported platformrs.

In this case the whole LHS of the pipe looks like it could be dropped, why not grep for "^clone< <redacted>"?

Also you don't need to quote the space character in regexes, it's not a metacharacter.