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

[Jonathan Tan <jonathantanmy@google.com>]

> diff --git a/fetch-pack.c b/fetch-pack.c
> index a9604f35a3e..62ea90541c5 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1653,8 +1653,12 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  				receive_wanted_refs(&reader, sought, nr_sought);
>  
>  			/* get the pack(s) */
> +			if (git_env_bool("GIT_TRACE_REDACT", 1))
> +				reader.options |= PACKET_READ_REDACT_URI_PATH;
>  			if (process_section_header(&reader, "packfile-uris", 1))
>  				receive_packfile_uris(&reader, &packfile_uris);
> +			reader.options &= ~PACKET_READ_REDACT_URI_PATH;

Probably worth commenting why you're resetting the flag (avoid the
relatively expensive URI check when we don't need it).

> diff --git a/pkt-line.c b/pkt-line.c
> index 2dc8ac274bd..5a69ddc2e77 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -370,6 +370,31 @@ int packet_length(const char lenbuf_hex[4])
>  	return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
>  }
>  
> +static char *find_packfile_uri_path(const char *buffer)
> +{
> +	const char *URI_MARK = "://";
> +	char *path;
> +	int len;
> +
> +	/* First char is sideband mark */
> +	buffer += 1;
> +
> +	len = strspn(buffer, "0123456789abcdefABCDEF");
> +	if (len != (int)the_hash_algo->hexsz || buffer[len] != ' ')
> +		return NULL; /* required "<hash>SP" not seen */

Optional: As I said in my reply (just sent out), checking for both SHA-1
and SHA-256 lengths is reasonable too.

[1] https://lore.kernel.org/git/20211108224335.569596-1-jonathantanmy@google.com/

> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index d527cf6c49f..f01af2f2ed3 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -1107,6 +1107,57 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul
>  	test_i18ngrep "disallowed submodule name" err
>  '
>  
> +test_expect_success 'packfile-uri path 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 &&
> +
> +	git -C "$P" hash-object my-blob >objh &&
> +	git -C "$P" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
> +	git -C "$P" config --add \
> +		"uploadpack.blobpackfileuri" \
> +		"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
> +
> +	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \

No need for GIT_TRACE=1 since you're not checking stdout. Also I don't
think GIT_TEST_SIDEBAND_ALL=1 is needed - we should check that it works
even without a test variable (and I've checked and it seems to work).

[snip]

> +test_expect_success 'packfile-uri path not redacted in trace when GIT_TRACE_REDACT=0' '
> +	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 &&
> +
> +	git -C "$P" hash-object my-blob >objh &&
> +	git -C "$P" pack-objects "$HTTPD_DOCUMENT_ROOT_PATH/mypack" <objh >packh &&
> +	git -C "$P" config --add \
> +		"uploadpack.blobpackfileuri" \
> +		"$(cat objh) $(cat packh) $HTTPD_URL/dumb/mypack-$(cat packh).pack" &&
> +
> +	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \

Same comment here.