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

From: Derrick Stolee <derrickstolee@github.com>
Date: May 27, 2022, 14:43 UTC
Message-ID: <0cf130d2-464b-8661-d53a-723041f32fc6@github.com>
In-Reply-To: <220527.86fskv2g50.gmgdl@evledraar.gmail.com>

On 5/27/2022 10:22 AM, Ævar Arnfjörð Bjarmason wrote:

Show 22 quoted lines

> 
> 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):
> 
>> +	/*
>> +	 * 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.

The name means "Do what needs to be done when creds are detected in a URL".

If we decide to change the response to creds in a URL, then we would change the implementation without changing the name.

Show 6 quoted lines

>>  			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,

Unfortunately, at this point 'url' is not equal to 'orig_url' and 'colon_ptr' isn't even within the 'orig_url' string. It's been copied and mutated. It was my first instinct to share as much as possible, which is why 'schema' is reused.

Thanks, -Stolee