Re: [PATCH v2] urlmatch: create fetch.credentialsInUrl config

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 27, 2022, 14:22 UTC
Message-ID: <220527.86fskv2g50.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1237.v2.git.1653658034086.gitgitgadget@gmail.com>

On Fri, May 27 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>

Just real quick, I hadn't taken notice of this before (the rest looks good at a glance):

Show 10 quoted lines

> +	/*
> +	 * Let's do some defensive programming to ensure the given
> +	 * URL is of the proper format.
> +	 */
> +	if (!colon_ptr)
> +		BUG("failed to find colon in url '%s' with scheme_len %"PRIuMAX,
> +		    url, (uintmax_t) scheme_len);
> +	if (colon_ptr > at_ptr)
> +		BUG("input url '%s' does not include credentials",
> +		    url);

So the function is renamed to detected_credentials_in_url(), so as a nit I'd expect some verb like "strip", "redact" or whatever inthe name or whatever, to make it clear what we're doing.

But since the only caller here below...

Show 31 quoted lines

> +
> +	/* Include the colon when creating the redacted URL. */
> +	colon_ptr++;
> +	strbuf_addstr(&redacted, url);
> +	strbuf_splice(&redacted, colon_ptr - url, at_ptr - colon_ptr,
> +		      "<redacted>", 10);
> +
> +	if (!strcmp("warn", value))
> +		warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
> +	if (!strcmp("die", value))
> +		die(_("URL '%s' uses plaintext credentials"), redacted.buf);
> +
> +	strbuf_release(&redacted);
> +}
> +
>  static char *url_normalize_1(const char *url, struct url_info *out_info, char allow_globs)
>  {
>  	/*
> @@ -144,6 +198,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al
>  	 */
>  
>  	size_t url_len = strlen(url);
> +	const char *orig_url = url;
>  	struct strbuf norm;
>  	size_t spanned;
>  	size_t scheme_len, user_off=0, user_len=0, passwd_off=0, passwd_len=0;
> @@ -191,6 +246,7 @@ static char *url_normalize_1(const char *url, struct url_info *out_info, char al
>  			}
>  			colon_ptr = strchr(norm.buf + scheme_len + 3, ':');
>  			if (colon_ptr) {
> +				detected_credentials_in_url(orig_url, scheme_len);

Has already done the work of finding the colon_ptr (and at_ptr) why re-do that paranoia since we have a static function, we could just pass the two pointers we found already to strbuf_splice().

This also seems really close to something we could just add to strbuf.c
as e.g a strbuf_splice_to(). I.e. just:
	
	int strbuf_splice_to(const struct strbuf *in, struct strbuf *sb,
			     size_t pos, size_t len,
			     const void *data, size_t data_len);
	
Which would be used as:
	
	struct strbuf sb = STRBUF_INIT;
	if (!strbuf_splice_to(url, &redacted, /* same as strbuf_splice(...) */))
		warn("oh noes a password in %s", sb.buf);
	else
		warn("have no password in %s, no replacement done", url->buf);

Which re earlier talk of sharing an implementation with the other <redacted> code looks like it could be dropped into the relevant part of pkt-line.c.

But maybe that's all going overboard :)