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

From: Derrick Stolee <derrickstolee@github.com>
Date: May 24, 2022, 20:14 UTC
Message-ID: <9318a400-479f-4029-c10b-3bfe498fcbb7@github.com>
In-Reply-To: <nycvar.QRO.7.76.6.2205241343390.352@tvgsbejvaqbjf.bet>

On 5/24/2022 7:46 AM, Johannes Schindelin wrote:

Show 27 quoted lines

> Hi Stolee,
> 
> On Mon, 23 May 2022, Derrick Stolee wrote:
> 
>> On 5/23/2022 3:06 PM, Junio C Hamano wrote:
>>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>
>>>> +static void detected_credentials_in_url(const char *url, size_t scheme_len)
>>>> +{
>>>> +	char *value = NULL;
>>>> +	const char *at_ptr;
>>>> +	const char *colon_ptr;
>>>> +	struct strbuf anonymized = STRBUF_INIT;
>>>> +
>>>> +	/* "ignore" is the default behavior. */
>>>> +	if (git_config_get_string("fetch.credentialsinurl", &value) ||
>>>> +	    !strcasecmp("ignore", value))
>>>> +		goto cleanup;
>>>> +
>>>> +	at_ptr = strchr(url, '@');
>>>> +	colon_ptr = strchr(url + scheme_len + 3, ':');
>>>
>>> We expect that at_ptr would come after colon_ptr (i.e. in
>>> "scheme://<u>:<p>@host", no @ exists in <u> or <p> part) and the
>>> while() loop below assumes that for redacting.
> 
> Careful here. https://me@there.com:9999/ _is_ a valid URL, too.

Thanks for checking. The method should not be called unless the password region was already detected. I'll add a BUG() statement and a comment to prevent future callers from providing incorrect URLs.

I can also add a test to show that this warning is not output when
the only colon is for the port number.

Show 10 quoted lines

>>> Are we better off if we assert it here, or has the calling parser
>>> already rejected such cases?
>>
>> This computation of at_ptr matches the one in url_normalize_1(),
>> so it at least agrees about where the "username[:password]" section
>> could be. That does mean that the password cannot contain an "@"
>> symbol (unless it is special-cased somehow?).
> 
> The password cannot contain a literal `@`, and neither can the user name.
> They have to be URL-encoded, via `%40`.

Thanks! -Stolee