On 5/24/2022 7:46 AM, Johannes Schindelin wrote:
> 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@xxxxxxxxx> 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@xxxxxxxxx: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.
>>> 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
