git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Oct 28, 2021, 16:39 UTC
Message-ID
<211028.86sfwlw10o.gmgdl@evledraar.gmail.com>
In-Reply-To
<c7f0977cabd4ba7311b8045bc57e9e30198651fd.1635288599.git.gitgitgadget@gmail.com>
On Tue, Oct 26 2021, Ivan Frade via GitGitGadget wrote:
Show 39 quoted lines
> 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)>

Show 6 quoted lines
> +			} 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, )
Previous: Ivan Frade via GitGitGadgetNext: Eric Sunshine
Message 18 of 43 in “fetch-pack: redact packfile urls in traces”
  1. 0/2 fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Oct 8, 2021
  2. 1/2 fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Oct 8, 2021
  3. Ævar Arnfjörð BjarmasonOct 8, 2021
  4. Ivan FradeOct 8, 2021
  5. 2/2 Documentation: packfile-uri hash can be longer than 40 hex charsIvan Frade via GitGitGadget, Oct 8, 2021
  6. Ævar Arnfjörð BjarmasonOct 8, 2021
  7. fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Oct 9, 2021
  8. Junio C HamanoOct 11, 2021
  9. Ivan FradeOct 26, 2021
  10. fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Oct 19, 2021
  11. Ævar Arnfjörð BjarmasonOct 20, 2021
  12. 0/2 fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Oct 26, 2021
  13. 1/2 fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Oct 26, 2021
  14. Junio C HamanoOct 28, 2021
  15. Ivan FradeOct 28, 2021
  16. Junio C HamanoOct 28, 2021
  17. 2/2 http-fetch: redact url on die() messageIvan Frade via GitGitGadget, Oct 26, 2021
  18. Ævar Arnfjörð BjarmasonOct 28, 2021
  19. Eric SunshineOct 28, 2021
  20. Ivan FradeOct 28, 2021
  21. Ivan FradeOct 28, 2021
  22. Junio C HamanoOct 29, 2021
  23. Ævar Arnfjörð BjarmasonNov 9, 2021
  24. 0/2 fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Oct 28, 2021
  25. 1/2 fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Oct 28, 2021
  26. Junio C HamanoOct 28, 2021
  27. Ivan FradeOct 29, 2021
  28. Junio C HamanoOct 29, 2021
  29. Jonathan TanNov 8, 2021
  30. 2/2 http-fetch: redact url on die() messageIvan Frade via GitGitGadget, Oct 28, 2021
  31. 0/2 fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Oct 29, 2021
  32. 1/2 fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Oct 29, 2021
  33. Jonathan TanNov 8, 2021
  34. Ævar Arnfjörð BjarmasonNov 9, 2021
  35. Ivan FradeNov 10, 2021
  36. Ævar Arnfjörð BjarmasonNov 11, 2021
  37. Ivan FradeNov 10, 2021
  38. 2/2 http-fetch: redact url on die() messageIvan Frade via GitGitGadget, Oct 29, 2021
  39. Jonathan TanNov 8, 2021
  40. 0/2 fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Nov 10, 2021
  41. 1/2 fetch-pack: redact packfile urls in tracesIvan Frade via GitGitGadget, Nov 10, 2021
  42. 2/2 http-fetch: redact url on die() messageIvan Frade via GitGitGadget, Nov 10, 2021
  43. Junio C HamanoNov 12, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.