trace2: redact URLs in error events

Transport errors can include a signed URL in the message passed to
error() or die(). Argument redaction does not cover these messages, so
a failed request can still record its password or signature in Trace2.

Have each target redact its formatted error payload. Keep the existing
va_list callback contract, including each target's use of va_copy(), so
the caller can still use the arguments when writing to stderr. Redact
the JSON target's separate format field as well, accounting for doubled
percent signs in literal query parameter names.

Use whitespace, double quotes, and angle brackets to delimit URLs in
error messages. Keep a trailing quote for a single-quoted token, but
treat other punctuation as part of the URL: an apostrophe inside a
signature is not a reliable delimiter. Continue scanning for adjacent
or nested URLs after redacting each match.

Signed-off-by: Daniel Reynaud <dreynaud@openai.com>
Signed-off-by: Taylor Blau <ttaylorr@openai.com>

Author: dreynaud-oai

t/t0212-trace2-event.sh

@@ -375,4 +375,36 @@ test_expect_success 'signature query parameters are redacted' '
 	done
 '
 
+test_expect_success 'error targets redact URLs without changing stderr' '
+	test_when_finished "rm trace.normal trace.perf trace.event stderr" &&
+	url="https://example.com/?sig=secret." &&
+	message="unable to access $SQ$url$SQ" &&
+	expect="unable to access ${SQ}https://example.com/?sig=<REDACTED>$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 &&
+	test_grep -F "$expect" trace.normal &&
+	test_grep -F "$expect" trace.perf &&
+	test_grep -F "$expect" trace.event &&
+	test_grep -F "\"fmt\":\"%s\"" trace.event &&
+	test_grep -F "$message" stderr
+'
+
+test_expect_success 'error redaction finds adjacent URLs' '
+	test_when_finished "rm trace.event stderr" &&
+	message="https://example.com/#fragment,https://example.com/?sig=secret" &&
+	GIT_TRACE2_EVENT="$(pwd)/trace.event" \
+		test-tool trace2 003error "$message" 2>stderr &&
+	test_grep "\"event\":\"error\".*sig=<REDACTED>" trace.event
+'
+
+test_expect_success 'URL signature redaction can be disabled' '
+	test_when_finished "rm trace.event stderr" &&
+	GIT_TRACE2_REDACT=0 GIT_TRACE2_EVENT="$(pwd)/trace.event" \
+		test-tool trace2 003error "https://example.com/?sig=secret" 2>stderr &&
+	test_grep "\"event\":\"start\".*sig=secret" trace.event &&
+	test_grep "\"event\":\"error\".*sig=secret" trace.event
+'
+
 test_done

trace2.c

@@ -250,18 +250,30 @@ int trace2_is_enabled(void)
 	return trace2_enabled;
 }
 
-static int is_signature_parameter(const char *parameter)
+static int is_signature_parameter(const char *parameter, int is_format)
 {
-	char *name = url_decode_parameter_name(&parameter);
-	char *p;
+	struct strbuf unescaped = STRBUF_INIT;
+	char *name, *p;
 	int ret;
 
+	/* A literal percent sign is doubled in a printf format string. */
+	if (is_format) {
+		while (*parameter && *parameter != '=') {
+			if (parameter[0] == '%' && parameter[1] == '%')
+				parameter++;
+			strbuf_addch(&unescaped, *parameter++);
+		}
+		parameter = unescaped.buf;
+	}
+
+	name = url_decode_parameter_name(&parameter);
 	for (p = name; *p; p++)
 		*p = tolower(*p);
 	ret = !strcmp(name, "sig") || !strcmp(name, "signature") ||
 	      ends_with(name, "-signature");
 
 	free(name);
+	strbuf_release(&unescaped);
 	return ret;
 }
 
@@ -271,7 +283,7 @@ static int is_signature_parameter(const char *parameter)
  * Returns the original if nothing needed to be redacted.
  * Returns a pointer that needs to be `free()`d otherwise.
  */
-static const char *redact_arg(const char *arg)
+static const char *redact_url(const char *arg, int is_format)
 {
 	const char *p, *colon;
 	const char *unredacted = arg;
@@ -303,7 +315,7 @@ static const char *redact_arg(const char *arg)
 			const char *equals = memchr(p, '=', end - p);
 
 			if (equals && equals + 1 < end &&
-			    is_signature_parameter(p)) {
+			    is_signature_parameter(p, is_format)) {
 				strbuf_add(&buf, unredacted,
 					   equals + 1 - unredacted);
 				strbuf_addstr(&buf, "<REDACTED>");
@@ -319,6 +331,55 @@ static const char *redact_arg(const char *arg)
 	return strbuf_detach(&buf, NULL);
 }
 
+static const char *redact_arg(const char *arg)
+{
+	return redact_url(arg, 0);
+}
+
+void tr2_redact_error(struct strbuf *buf, int is_format)
+{
+	size_t pos = 0;
+
+	if (!trace2_redact)
+		return;
+
+	while (pos < buf->len) {
+		const char *p = strcasestr(buf->buf + pos, "http");
+		const char *end, *scheme_end;
+		const char *redacted;
+		char *url;
+		size_t start;
+
+		if (!p)
+			break;
+		start = p - buf->buf;
+		pos = start + 4;
+		if (!skip_iprefix(p, "http://", &scheme_end) &&
+		    !skip_iprefix(p, "https://", &scheme_end))
+			continue;
+
+		/*
+		 * Whitespace, double quotes and angle brackets delimit URLs.
+		 * Other punctuation can belong to a signature, so preserve a
+		 * single quote only at the end of a single-quoted token.
+		 */
+		end = p + strcspn(p, " \t\r\n\v\f\"<>");
+		if (start && p[-1] == '\'' && end[-1] == '\'')
+			end--;
+
+		/* Also examine any further URLs in the unredacted text. */
+		pos = scheme_end - buf->buf;
+		url = xmemdupz(p, end - p);
+		redacted = redact_url(url, is_format);
+		if (redacted != url) {
+			strbuf_splice(buf, start, end - p,
+				      redacted, strlen(redacted));
+			free((char *)redacted);
+		}
+		free(url);
+	}
+}
+
 /*
  * Redacts arguments in an argument list.
  *

trace2/tr2_tgt.h

@@ -3,6 +3,7 @@
 
 struct child_process;
 struct repository;
+struct strbuf;
 struct json_writer;
 struct tr2_timer_metadata;
 struct tr2_timer;
@@ -157,4 +158,10 @@ extern struct tr2_tgt tr2_tgt_event;
 extern struct tr2_tgt tr2_tgt_normal;
 extern struct tr2_tgt tr2_tgt_perf;
 
+/*
+ * Redact URLs in an error payload before writing it. Set is_format when
+ * redacting a printf format string, where literal '%' characters are doubled.
+ */
+void tr2_redact_error(struct strbuf *buf, int is_format);
+
 #endif /* TR2_TGT_H */

trace2/tr2_tgt_event.c

@@ -238,15 +238,31 @@ static void fn_error_va_fl(const char *file, int line, const char *fmt,
 
 	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) {
+		struct strbuf message = STRBUF_INIT;
+		va_list copy_ap;
+
+		va_copy(copy_ap, ap);
+		strbuf_vaddf(&message, fmt, copy_ap);
+		va_end(copy_ap);
+		tr2_redact_error(&message, 0);
+		jw_object_string(&jw, "msg", message.buf);
+		strbuf_release(&message);
+	}
 	/*
 	 * Also emit the format string as a field in case
 	 * post-processors want to aggregate common error
 	 * messages by type without argument fields (such
 	 * as pathnames or branch names) cluttering it up.
 	 */
-	if (fmt && *fmt)
-		jw_object_string(&jw, "fmt", fmt);
+	if (fmt && *fmt) {
+		struct strbuf format = STRBUF_INIT;
+
+		strbuf_addstr(&format, fmt);
+		tr2_redact_error(&format, 1);
+		jw_object_string(&jw, "fmt", format.buf);
+		strbuf_release(&format);
+	}
 	jw_end(&jw);
 
 	tr2_dst_write_line(&tr2dst_event, &jw.json);

trace2/tr2_tgt_normal.c

@@ -152,6 +152,7 @@ static void fn_error_va_fl(const char *file, int line, const char *fmt,
 		strbuf_addch(&buf_payload, ' ');
 		maybe_append_string_va(&buf_payload, fmt, ap);
 	}
+	tr2_redact_error(&buf_payload, 0);
 	normal_io_write_fl(file, line, &buf_payload);
 	strbuf_release(&buf_payload);
 }

trace2/tr2_tgt_perf.c

@@ -241,6 +241,7 @@ static void fn_error_va_fl(const char *file, int line, const char *fmt,
 	struct strbuf buf_payload = STRBUF_INIT;
 
 	maybe_append_string_va(&buf_payload, fmt, ap);
+	tr2_redact_error(&buf_payload, 0);
 
 	perf_io_write_fl(file, line, event_name, NULL, NULL, NULL, NULL,
 			 &buf_payload);