Re: [PATCH v6 1/2] fetch-pack: redact packfile urls in traces
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Nov 8, 2021, 23:01 UTC
- Message-ID
- <20211108230111.1101434-1-jonathantanmy@google.com>
- In-Reply-To
- <a6098f98946bd9cc1186ab9c83d917566c78b805.1635532975.git.gitgitgadget@gmail.com>
Show 13 quoted lines
> 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).
Show 20 quoted lines
> 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/
Show 26 quoted lines
> 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]
Show 18 quoted lines
> +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.