Re: [PATCH v3 2/3] remote: do not show credentials when relative_url() fails

[Derrick Stolee <stolee@gmail.com>]

On 11/5/2026 10:28 AM, Aniket Gupta wrote:
> When relative_url() cannot resolve a URL, it names the URL in its error
> message. If the URL contains a user name and password, both end up in
> the message, and from there in terminal output and logs.
> 
> Pass the URL through transport_anonymize_url() first. Also name the
> URL that was passed in rather than what is left of it after some
> components have already been stripped off, which is what a user would
> recognize.

I'm less convinced on this factor. I understand anonymizing the URL if
we are sending it to a location that is perhaps "less trusted" (like
trace logs) but the user asked us to operate on a given URL that caused
problems, so they should know the string that caused problems.

In environments that store stdout/stderr logs, those systems already
need to be careful about such strings in the commands themselves, let
alone the output strings.

And back to the bundle-URI application: if the bundle list uses such
URIs for its child bundles, then that's not a safe mechanism for
communicating those credentials. This isn't going to protect secrets
like that from being exposed.

This is just my opinion on the necessity of this change. If we _do_
want this change, then I continued my review: the patch accomplishes
its goal.
> @@ -3078,9 +3078,12 @@ char *relative_url_gently(const char *remote_url, const char *url,
>  			url += 3;
>  			chopped = chop_last_dir(&remoteurl, is_relative);
>  			if (chopped < 0) {
> +				char *anon = transport_anonymize_url(remote_url);
> +
>  				strbuf_addf(err,
>  					    _("cannot strip one component off url '%s'"),
> -					    remoteurl);
> +					    anon);
> +				free(anon);
>  				free(remoteurl);

This code looks good, assuming taht we want to make this change.

> +test_expect_success 'relative_url() does not show credentials when it fails' '
> +	cat >expect <<-\EOF &&
> +	fatal: cannot strip one component off url '\''https://host.example/a'\''
> +	EOF
> +	test_must_fail test-tool submodule resolve-relative-url "" \
> +		"https://user:secret@host.example/a" ../../../../../../x 2>actual &&
> +	test_cmp expect actual
> +'
Thank you for including a test that demonstrates this change.

Thanks,
-Stolee