diff --git a/t/t0212-trace2-event.sh b/t/t0212-trace2-event.sh index f5358a1dd44991..74fd5ca3264754 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,68 @@ 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 && + 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 scheme input expect + do + : >trace.event && + GIT_TRACE2_EVENT="$(pwd)/trace.event" \ + test-tool trace2 303redact_def_param url "$scheme://example.com/pack$input" && + grep "\"event\":\"def_param\"" trace.event >actual && + test_grep -F "\"value\":\"$scheme://example.com/pack$expect\"" actual || return 1 + done 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 c23c0a227b7032..3f40c195810429 100644 --- a/trace2.c +++ b/trace2.c @@ -17,6 +17,7 @@ #include "trace2/tr2_tgt.h" #include "trace2/tr2_tls.h" #include "trace2/tr2_tmr.h" +#include "url.h" static int trace2_enabled; static int trace2_redact = 1; @@ -249,9 +250,19 @@ int trace2_is_enabled(void) return trace2_enabled; } +static int is_signature_parameter(const char *parameter, size_t len) +{ + char *name = url_decode_mem(parameter, len); + const char *suffix = strrchr(name, '-'); + int ret = !strcasecmp(name, "sig") || + !strcasecmp(suffix ? suffix + 1 : name, "signature"); + + free(name); + return ret; +} + /* - * Redacts an argument, i.e. ensures that no password in - * https://user:password@host/-style URLs is logged. + * Redact passwords and signature query parameters in HTTP(S) URLs. * * Returns the original if nothing needed to be redacted. * Returns a pointer that needs to be `free()`d otherwise. @@ -259,22 +270,83 @@ int trace2_is_enabled(void) static const char *redact_arg(const char *arg) { const char *p, *colon; + const char *unredacted = arg; + struct strbuf buf = STRBUF_INIT; size_t at; if (!trace2_redact || - (!skip_prefix(arg, "https://", &p) && - !skip_prefix(arg, "http://", &p))) + (!skip_iprefix(arg, "https://", &p) && + !skip_iprefix(arg, "http://", &p))) return arg; - at = strcspn(p, "@/"); - if (p[at] != '@') - return arg; + at = strcspn(p, "@/?#"); + if (p[at] == '@' && (colon = memchr(p, ':', at))) { + strbuf_add(&buf, arg, colon + 1 - arg); + strbuf_addstr(&buf, ""); + unredacted = p + at; + } + + 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; + } + } - colon = memchr(p, ':', at); - if (!colon) + if (!buf.len) return arg; + strbuf_addstr(&buf, unredacted); + return strbuf_detach(&buf, NULL); +} - return xstrfmt("%.*s:%s", (int)(colon - arg), arg, p + at); +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 = strcasestr(p, "http"))) { + const char *end; + const char *redacted; + char *url; + + if (!skip_iprefix(p, "http://", &end) && + !skip_iprefix(p, "https://", &end)) { + 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); } /* @@ -380,18 +452,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,