Re: [PATCH v4 2/2] http-fetch: redact url on die() message

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Oct 26 2021, Ivan Frade via GitGitGadget wrote:

> From: Ivan Frade <ifrade@google.com>
>
> http-fetch prints the URL after failing to fetch it. This can be
> confusing to users (they cannot really do anything with it) but even
> worse, 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.
>
> Signed-off-by: Ivan Frade <ifrade@google.com>
> ---
>  http-fetch.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/http-fetch.c b/http-fetch.c
> index fa642462a9e..bbe09a6ad9f 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,18 @@ 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 (!git_env_bool("GIT_TRACE_REDACT", 1) || !nurl) {
> +				die("Unable to get pack file %s\n%s", preq->url,
> +				    curl_errorstr);

small nit: arrange if's from "if (cheap || expensive)", i.e. no need for
getenv() if !nurl, but maybe compilers are smart enough for that...

nit: die() messages should start with lower-case (in CodingGuidelines), and I think it's better to quote both, so:

    die("unable to get pack '%s': '%s'", ...)

Or maybe without the second '%s', as in 3e8084f1884 (http: check
CURLE_SSL_PINNEDPUBKEYNOTMATCH when emitting errors, 2021-09-24) (which
I authored, but just copy/pasted the convention in the surrounding
code)>

> +			} else {
> +				char *schema = xstrndup(url.url, url.scheme_len);
> +				char *host = xstrndup(&url.url[url.host_off], url.host_len);
> +				die("failed to get '%s' url from '%s' "
> +				    "(full URL redacted due to GIT_TRACE_REDACT setting)\n%s",
> +				    schema, host, curl_errorstr);

Hrm, I haven't tested, but aren't both of those xstrndup's redundant to
using %*s instead of %s for the printf format? I.e.:

    die("failed to get '%*s'[...]", url.schema_len, url.url, )