[v6,2/2] http-fetch: redact url on die() message

Commit Message

Ivan Frade Oct. 29, 2021, 6:42 p.m. UTC

From: Ivan Frade <[e-mail]>

http-fetch prints the URL after failing to fetch it. This can be
confusing to users (they cannot really do anything with it), and they
can share by accident a sensitive URL (e.g. with credentials) while
looking for help.

Redact the URL unless the GIT_TRACE_REDACT variable is set to false. This
mimics the redaction of other sensitive information in git, like the
Authorization header in HTTP.

Fix also capitalization of previous die() message (must start in
lowercase).

Signed-off-by: Ivan Frade <[e-mail]>
---
 http-fetch.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Jonathan Tan Nov. 8, 2021, 11:06 p.m. UTC | #1

> @@ -63,8 +64,17 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
>  	if (start_active_slot(preq->slot)) {
>  		run_active_slot(preq->slot);
>  		if (results.curl_result != CURLE_OK) {
> -			die("Unable to get pack file %s\n%s", preq->url,
> -			    curl_errorstr);
> +			struct url_info url;
> +			char *nurl = url_normalize(preq->url, &url);
> +			if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
> +				die("unable to get pack file '%s'\n%s", preq->url,
> +				    curl_errorstr);
> +			} else {
> +				die("failed to get '%.*s' url from '%.*s' "
> +				    "(full URL redacted due to GIT_TRACE_REDACT setting)\n%s",
> +				    (int)url.scheme_len, url.url,
> +				    (int)url.host_len, &url.url[url.host_off], curl_errorstr);
> +			}

I was confused why nurl was set but never used in "else", but I see that
it's because url_normalize() also sets that value in the urlinfo struct.
This patch looks good (and patch 1 too, with my suggested changes).

Patch

diff --git a/http-fetch.c b/http-fetch.c
index fa642462a9e..c7c7d391ac5 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -4,6 +4,7 @@ 
 #include "http.h"
 #include "walker.h"
 #include "strvec.h"
+#include "urlmatch.h"
 
 static const char http_fetch_usage[] = "git http-fetch "
 "[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin | --packfile=hash | commit-id] url";
@@ -63,8 +64,17 @@  static void fetch_single_packfile(struct object_id *packfile_hash,
 	if (start_active_slot(preq->slot)) {
 		run_active_slot(preq->slot);
 		if (results.curl_result != CURLE_OK) {
-			die("Unable to get pack file %s\n%s", preq->url,
-			    curl_errorstr);
+			struct url_info url;
+			char *nurl = url_normalize(preq->url, &url);
+			if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
+				die("unable to get pack file '%s'\n%s", preq->url,
+				    curl_errorstr);
+			} else {
+				die("failed to get '%.*s' url from '%.*s' "
+				    "(full URL redacted due to GIT_TRACE_REDACT setting)\n%s",
+				    (int)url.scheme_len, url.url,
+				    (int)url.host_len, &url.url[url.host_off], curl_errorstr);
+			}
 		}
 	} else {
 		die("Unable to start request");