Re: [PATCH v6 1/2] fetch-pack: redact packfile urls in traces
flat view
From: Jonathan Tan <hidden>
Date: 2021-11-08 23:01:16
quoted hunk ↗ jump to hunk
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).
quoted hunk ↗ jump to hunk
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/ (local)
quoted hunk ↗ jump to hunk
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]
quoted hunk ↗ jump to hunk
+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.