From 807668babf10c7f92b920fd05e509a20dbbd04aa Mon Sep 17 00:00:00 2001 From: Daniel Reynaud Date: Mon, 24 Aug 2026 15:03:20 -0700 Subject: [PATCH 1/3] trace2: redact signatures from presigned blob storage URLs Trace2 redacts passwords in HTTP(S) URLs, but leaves signatures in their query strings. Presigned blob storage URLs can therefore expose access credentials in trace output. Redact values for sig, signature, and query names ending in -signature. Match names case-insensitively after percent decoding, without checking the host. Preserve the rest of the URL and the existing redaction opt-out. Extend the existing event tests to cover signatures alongside passwords, encoded and repeated query parameters, and unchanged URL fields. Signed-off-by: Daniel Reynaud --- t/t0212-trace2-event.sh | 56 +++++++++++++++++++++++++++++++++-------- trace2.c | 51 ++++++++++++++++++++++++++++++------- 2 files changed, 87 insertions(+), 20 deletions(-) diff --git a/t/t0212-trace2-event.sh b/t/t0212-trace2-event.sh index f5358a1dd44991..1aad27e8639346 100755 --- a/t/t0212-trace2-event.sh +++ b/t/t0212-trace2-event.sh @@ -322,17 +322,17 @@ test_expect_success 'discard traces when there are too many files' ' head -n2 trace_target_dir/git-trace2-discard | tail -n1 | grep \"event\":\"too_many_files\" ' -# In the following "...redact..." tests, skip testing the GIT_TRACE2_REDACT=0 -# case because we would need to exactly model the full JSON event stream like -# we did in the basic tests above and I do not think it is worth it. +url="https://user:pwd@example.com/?sig=secret&expires=123" +redacted_url="https://user:@example.com/?sig=&expires=123" test_expect_success 'unsafe URLs are redacted by default in cmd_start events' ' test_when_finished \ "rm -r trace.event" && GIT_TRACE2_EVENT="$(pwd)/trace.event" \ - test-tool trace2 300redact_start git clone https://user:pwd@example.com/ clone2 && - test_grep ! user:pwd trace.event + test-tool trace2 300redact_start git clone "$url" clone2 && + test_grep ! -E "user:pwd|secret" trace.event && + test_grep -F "$redacted_url" trace.event ' test_expect_success 'unsafe URLs are redacted by default in child_start events' ' @@ -340,8 +340,9 @@ test_expect_success 'unsafe URLs are redacted by default in child_start events' "rm -r trace.event" && GIT_TRACE2_EVENT="$(pwd)/trace.event" \ - test-tool trace2 301redact_child_start git clone https://user:pwd@example.com/ clone2 && - test_grep ! user:pwd trace.event + test-tool trace2 301redact_child_start git clone "$url" clone2 && + test_grep ! -E "user:pwd|secret" trace.event && + test_grep -F "$redacted_url" trace.event ' test_expect_success 'unsafe URLs are redacted by default in exec events' ' @@ -349,8 +350,9 @@ test_expect_success 'unsafe URLs are redacted by default in exec events' ' "rm -r trace.event" && GIT_TRACE2_EVENT="$(pwd)/trace.event" \ - test-tool trace2 302redact_exec git clone https://user:pwd@example.com/ clone2 && - test_grep ! user:pwd trace.event + test-tool trace2 302redact_exec git clone "$url" clone2 && + test_grep ! -E "user:pwd|secret" trace.event && + test_grep -F "$redacted_url" trace.event ' test_expect_success 'unsafe URLs are redacted by default in def_param events' ' @@ -358,8 +360,40 @@ test_expect_success 'unsafe URLs are redacted by default in def_param events' ' "rm -r trace.event" && GIT_TRACE2_EVENT="$(pwd)/trace.event" \ - test-tool trace2 303redact_def_param url https://user:pwd@example.com/ && - test_grep ! user:pwd trace.event + test-tool trace2 303redact_def_param url "$url" && + test_grep ! -E "user:pwd|secret" trace.event && + test_grep -F "$redacted_url" trace.event +' + +test_expect_success 'redact signature query values without changing the rest of the URL' ' + test_when_finished "rm -f trace.event actual cases" && + cat >cases <<-\EOF && + ?sig=secret ?sig= + ?expires=123&Signature=secret ?expires=123&Signature= + ?X-Blob-SIGNATURE=secret&expires=123 ?X-Blob-SIGNATURE=&expires=123 + ?%73ig=secret%2Bvalue%3D&sig=other ?%73ig=&sig= + ?SIG=secret#fragment ?SIG=#fragment + ?sig=&signature ?sig=&signature + ?sig=sig=secret&expires=123 ?sig=&expires=123 + ?design=value&signature-extra=value ?design=value&signature-extra=value + ?value=sig=public ?value=sig=public + #fragment?sig=public #fragment?sig=public + EOF + while read input expect + do + : >trace.event && + GIT_TRACE2_EVENT="$(pwd)/trace.event" \ + test-tool trace2 303redact_def_param url "https://example.com/pack$input" && + grep "\"event\":\"def_param\"" trace.event >actual && + test_grep -F "\"value\":\"https://example.com/pack$expect\"" actual || return 1 + done "); + unredacted = p + at; + } - colon = memchr(p, ':', at); - if (!colon) - return arg; + p += strcspn(p, "?#"); + if (*p == '?') { + p++; + while (*p && *p != '#') { + const char *end = p + strcspn(p, "&#"); + const char *equals = memchr(p, '=', end - p); + + if (equals && equals + 1 < end && + is_signature_parameter(p, equals - p)) { + strbuf_add(&buf, unredacted, + equals + 1 - unredacted); + strbuf_addstr(&buf, ""); + unredacted = end; + } + p = *end == '&' ? end + 1 : end; + } + } - return xstrfmt("%.*s:%s", (int)(colon - arg), arg, p + at); + if (!buf.len) + return arg; + strbuf_addstr(&buf, unredacted); + return strbuf_detach(&buf, NULL); } /* From c1e270459c4de31c750b1ac39ba1d83135388d22 Mon Sep 17 00:00:00 2001 From: Daniel Reynaud Date: Mon, 24 Aug 2026 16:47:42 -0700 Subject: [PATCH 2/3] trace2: redact URL signatures in error events Transport errors can embed a presigned URL in a message passed to die(). Trace2 records that message without the argument redaction applied to start and child_start events, so a failed request can still expose its signature. Format error messages once and redact embedded URLs before passing them to the output targets. Keep the separate message and format fields, and redact URLs in either field. Leave stderr and request arguments unchanged. Test all three targets with quoted URLs and multiple URLs in one error, and check that stderr and the redaction opt-out retain their behavior. Signed-off-by: Daniel Reynaud --- t/t0212-trace2-event.sh | 28 ++++++++++++++++++ trace2.c | 65 +++++++++++++++++++++++++++++++++++++---- trace2/tr2_tgt.h | 7 +++-- trace2/tr2_tgt_event.c | 9 +++--- trace2/tr2_tgt_normal.c | 8 ++--- trace2/tr2_tgt_perf.c | 8 ++--- 6 files changed, 104 insertions(+), 21 deletions(-) diff --git a/t/t0212-trace2-event.sh b/t/t0212-trace2-event.sh index 1aad27e8639346..5a0f2d57489a64 100755 --- a/t/t0212-trace2-event.sh +++ b/t/t0212-trace2-event.sh @@ -396,4 +396,32 @@ test_expect_success 'URL signature redaction can be disabled' ' test_grep -F "$url" trace.event ' +test_expect_success 'redact URLs in error events without changing stderr' ' + test_when_finished "rm trace.normal trace.perf trace.event errors stderr" && + message="unable to access $SQ$url$SQ: try ${SQ}https://example.com/o${SQ}brien?signature=other$SQ." && + expected="unable to access $SQ$redacted_url$SQ: try ${SQ}https://example.com/o${SQ}brien?signature=$SQ." && + GIT_TRACE2="$(pwd)/trace.normal" \ + GIT_TRACE2_PERF="$(pwd)/trace.perf" \ + GIT_TRACE2_EVENT="$(pwd)/trace.event" \ + test-tool trace2 003error "$message" 2>stderr && + grep " error " trace.normal >errors && + grep "| error " trace.perf >>errors && + grep "\"event\":\"error\"" trace.event >>errors && + test_grep ! -E "user:pwd|secret|signature=other" errors && + test_grep -F "$expected" trace.normal && + test_grep -F "$expected" trace.perf && + test_grep -F "$expected" trace.event && + test_grep -F "\"fmt\":\"%s\"" errors && + test_grep -F "$message" stderr +' + +test_expect_success 'URL signature redaction can be disabled in error events' ' + test_when_finished "rm trace.event errors stderr" && + message="unable to access $SQ$url$SQ" && + GIT_TRACE2_REDACT=0 GIT_TRACE2_EVENT="$(pwd)/trace.event" \ + test-tool trace2 003error "$message" 2>stderr && + grep "\"event\":\"error\"" trace.event >errors && + test_grep -F "$message" errors +' + test_done diff --git a/trace2.c b/trace2.c index d61456a8939223..ca3d8b2b6bfbf3 100644 --- a/trace2.c +++ b/trace2.c @@ -310,6 +310,44 @@ static const char *redact_arg(const char *arg) return strbuf_detach(&buf, NULL); } +static const char *redact_error_message(const char *message) +{ + struct strbuf buf = STRBUF_INIT; + const char *p = message, *unredacted = message; + + if (!trace2_redact || !message) + return message; + + while ((p = strstr(p, "http"))) { + const char *end; + const char *redacted; + char *url; + + if (!starts_with(p, "http://") && !starts_with(p, "https://")) { + p += 4; + continue; + } + end = p + strcspn(p, " \t\r\n\v\f"); + /* Keep punctuation surrounding URLs in diagnostics. */ + while (end > p && strchr("'\".,:;)]}>", end[-1])) + end--; + url = xmemdupz(p, end - p); + redacted = redact_arg(url); + if (redacted != url) { + strbuf_add(&buf, unredacted, p - unredacted); + strbuf_addstr(&buf, redacted); + unredacted = end; + free((char *)redacted); + } + free(url); + p = end; + } + if (!buf.len) + return message; + strbuf_addstr(&buf, unredacted); + return strbuf_detach(&buf, NULL); +} + /* * Redacts arguments in an argument list. * @@ -413,18 +451,33 @@ void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt, va_list ap) { struct tr2_tgt *tgt_j; + struct strbuf message = STRBUF_INIT; + const char *redacted_fmt, *redacted_message; int j; if (!trace2_enabled) return; - /* - * We expect each target function to treat 'ap' as constant - * and use va_copy (because an 'ap' can only be walked once). - */ + if (fmt && *fmt) { + va_list copy_ap; + + va_copy(copy_ap, ap); + strbuf_vaddf(&message, fmt, copy_ap); + va_end(copy_ap); + } + redacted_fmt = redact_error_message(fmt); + redacted_message = redact_error_message(message.buf); + for_each_wanted_builtin (j, tgt_j) - if (tgt_j->pfn_error_va_fl) - tgt_j->pfn_error_va_fl(file, line, fmt, ap); + if (tgt_j->pfn_error_fl) + tgt_j->pfn_error_fl(file, line, redacted_fmt, + redacted_message); + + if (redacted_fmt != fmt) + free((char *)redacted_fmt); + if (redacted_message != message.buf) + free((char *)redacted_message); + strbuf_release(&message); } void trace2_cmd_path_fl(const char *file, int line, const char *pathname) diff --git a/trace2/tr2_tgt.h b/trace2/tr2_tgt.h index 1f626cffea0fc0..f2b473e2ad30e3 100644 --- a/trace2/tr2_tgt.h +++ b/trace2/tr2_tgt.h @@ -28,8 +28,9 @@ typedef void(tr2_tgt_evt_exit_fl_t)(const char *file, int line, typedef void(tr2_tgt_evt_signal_t)(uint64_t us_elapsed_absolute, int signo); typedef void(tr2_tgt_evt_atexit_t)(uint64_t us_elapsed_absolute, int code); -typedef void(tr2_tgt_evt_error_va_fl_t)(const char *file, int line, - const char *fmt, va_list ap); +/* Error messages and format strings are redacted before dispatch. */ +typedef void(tr2_tgt_evt_error_fl_t)(const char *file, int line, + const char *fmt, const char *message); typedef void(tr2_tgt_evt_command_path_fl_t)(const char *file, int line, const char *command_path); @@ -128,7 +129,7 @@ struct tr2_tgt { tr2_tgt_evt_exit_fl_t *pfn_exit_fl; tr2_tgt_evt_signal_t *pfn_signal; tr2_tgt_evt_atexit_t *pfn_atexit; - tr2_tgt_evt_error_va_fl_t *pfn_error_va_fl; + tr2_tgt_evt_error_fl_t *pfn_error_fl; tr2_tgt_evt_command_path_fl_t *pfn_command_path_fl; tr2_tgt_evt_command_ancestry_fl_t *pfn_command_ancestry_fl; tr2_tgt_evt_command_name_fl_t *pfn_command_name_fl; diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c index 5a0381791f7eb4..f64ec41bac1800 100644 --- a/trace2/tr2_tgt_event.c +++ b/trace2/tr2_tgt_event.c @@ -230,15 +230,16 @@ static void maybe_add_string_va(struct json_writer *jw, const char *field_name, } } -static void fn_error_va_fl(const char *file, int line, const char *fmt, - va_list ap) +static void fn_error_fl(const char *file, int line, const char *fmt, + const char *message) { const char *event_name = "error"; struct json_writer jw = JSON_WRITER_INIT; jw_object_begin(&jw, 0); event_fmt_prepare(event_name, file, line, NULL, &jw); - maybe_add_string_va(&jw, "msg", fmt, ap); + if (fmt && *fmt) + jw_object_string(&jw, "msg", message); /* * Also emit the format string as a field in case * post-processors want to aggregate common error @@ -696,7 +697,7 @@ struct tr2_tgt tr2_tgt_event = { .pfn_exit_fl = fn_exit_fl, .pfn_signal = fn_signal, .pfn_atexit = fn_atexit, - .pfn_error_va_fl = fn_error_va_fl, + .pfn_error_fl = fn_error_fl, .pfn_command_path_fl = fn_command_path_fl, .pfn_command_ancestry_fl = fn_command_ancestry_fl, .pfn_command_name_fl = fn_command_name_fl, diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c index 924736ab36093b..e3936815e333eb 100644 --- a/trace2/tr2_tgt_normal.c +++ b/trace2/tr2_tgt_normal.c @@ -142,15 +142,15 @@ static void maybe_append_string_va(struct strbuf *buf, const char *fmt, } } -static void fn_error_va_fl(const char *file, int line, const char *fmt, - va_list ap) +static void fn_error_fl(const char *file, int line, const char *fmt, + const char *message) { struct strbuf buf_payload = STRBUF_INIT; strbuf_addstr(&buf_payload, "error"); if (fmt && *fmt) { strbuf_addch(&buf_payload, ' '); - maybe_append_string_va(&buf_payload, fmt, ap); + strbuf_addstr(&buf_payload, message); } normal_io_write_fl(file, line, &buf_payload); strbuf_release(&buf_payload); @@ -384,7 +384,7 @@ struct tr2_tgt tr2_tgt_normal = { .pfn_exit_fl = fn_exit_fl, .pfn_signal = fn_signal, .pfn_atexit = fn_atexit, - .pfn_error_va_fl = fn_error_va_fl, + .pfn_error_fl = fn_error_fl, .pfn_command_path_fl = fn_command_path_fl, .pfn_command_ancestry_fl = fn_command_ancestry_fl, .pfn_command_name_fl = fn_command_name_fl, diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c index 4eb9289f950505..e7e7d47903f80d 100644 --- a/trace2/tr2_tgt_perf.c +++ b/trace2/tr2_tgt_perf.c @@ -234,13 +234,13 @@ static void maybe_append_string_va(struct strbuf *buf, const char *fmt, } } -static void fn_error_va_fl(const char *file, int line, const char *fmt, - va_list ap) +static void fn_error_fl(const char *file, int line, const char *fmt UNUSED, + const char *message) { const char *event_name = "error"; struct strbuf buf_payload = STRBUF_INIT; - maybe_append_string_va(&buf_payload, fmt, ap); + strbuf_addstr(&buf_payload, message); perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL, &buf_payload); @@ -609,7 +609,7 @@ struct tr2_tgt tr2_tgt_perf = { .pfn_exit_fl = fn_exit_fl, .pfn_signal = fn_signal, .pfn_atexit = fn_atexit, - .pfn_error_va_fl = fn_error_va_fl, + .pfn_error_fl = fn_error_fl, .pfn_command_path_fl = fn_command_path_fl, .pfn_command_ancestry_fl = fn_command_ancestry_fl, .pfn_command_name_fl = fn_command_name_fl, From 83c4b516ebe962e9abed7b04e577bda8060bbd7a Mon Sep 17 00:00:00 2001 From: Daniel Reynaud Date: Mon, 24 Aug 2026 17:07:28 -0700 Subject: [PATCH 3/3] trace2: match URL schemes case-insensitively URL schemes are case-insensitive, but the redactor only recognized lowercase HTTP(S) prefixes. Uppercase and mixed-case schemes could therefore leave signatures in trace output. Use case-insensitive matching for argument URLs and URLs embedded in error messages. Preserve the original URL spelling and cover these schemes in the existing redaction tests. Signed-off-by: Daniel Reynaud --- t/t0212-trace2-event.sh | 34 +++++++++++++++++----------------- trace2.c | 9 +++++---- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/t/t0212-trace2-event.sh b/t/t0212-trace2-event.sh index 5a0f2d57489a64..74fd5ca3264754 100755 --- a/t/t0212-trace2-event.sh +++ b/t/t0212-trace2-event.sh @@ -322,8 +322,8 @@ test_expect_success 'discard traces when there are too many files' ' head -n2 trace_target_dir/git-trace2-discard | tail -n1 | grep \"event\":\"too_many_files\" ' -url="https://user:pwd@example.com/?sig=secret&expires=123" -redacted_url="https://user:@example.com/?sig=&expires=123" +url="HtTpS://user:pwd@example.com/?sig=secret&expires=123" +redacted_url="HtTpS://user:@example.com/?sig=&expires=123" test_expect_success 'unsafe URLs are redacted by default in cmd_start events' ' test_when_finished \ @@ -368,24 +368,24 @@ test_expect_success 'unsafe URLs are redacted by default in def_param events' ' test_expect_success 'redact signature query values without changing the rest of the URL' ' test_when_finished "rm -f trace.event actual cases" && cat >cases <<-\EOF && - ?sig=secret ?sig= - ?expires=123&Signature=secret ?expires=123&Signature= - ?X-Blob-SIGNATURE=secret&expires=123 ?X-Blob-SIGNATURE=&expires=123 - ?%73ig=secret%2Bvalue%3D&sig=other ?%73ig=&sig= - ?SIG=secret#fragment ?SIG=#fragment - ?sig=&signature ?sig=&signature - ?sig=sig=secret&expires=123 ?sig=&expires=123 - ?design=value&signature-extra=value ?design=value&signature-extra=value - ?value=sig=public ?value=sig=public - #fragment?sig=public #fragment?sig=public + http ?sig=secret ?sig= + HTTPS ?expires=123&Signature=secret ?expires=123&Signature= + hTtP ?X-Blob-SIGNATURE=secret&expires=123 ?X-Blob-SIGNATURE=&expires=123 + https ?%73ig=secret%2Bvalue%3D&sig=other ?%73ig=&sig= + HTTP ?SIG=secret#fragment ?SIG=#fragment + https ?sig=&signature ?sig=&signature + https ?sig=sig=secret&expires=123 ?sig=&expires=123 + https ?design=value&signature-extra=value ?design=value&signature-extra=value + https ?value=sig=public ?value=sig=public + https #fragment?sig=public #fragment?sig=public EOF - while read input expect + while read scheme input expect do : >trace.event && GIT_TRACE2_EVENT="$(pwd)/trace.event" \ - test-tool trace2 303redact_def_param url "https://example.com/pack$input" && + test-tool trace2 303redact_def_param url "$scheme://example.com/pack$input" && grep "\"event\":\"def_param\"" trace.event >actual && - test_grep -F "\"value\":\"https://example.com/pack$expect\"" actual || return 1 + test_grep -F "\"value\":\"$scheme://example.com/pack$expect\"" actual || return 1 done