Re: [PATCH v2 04/10] promisor-remote: reject empty name or URL in advertised remote

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> In parse_one_advertised_remote(), we check for a NULL remote name and
> remote URL, but not for empty ones. An empty URL seems possible as
> url_percent_decode("") doesn't return NULL.
>
> In promisor_config_info_list(), we ignore remotes with empty URLs, so a
> Git server should not advertise remotes with empty URLs. It's possible
> that a buggy or malicious server would do it though.
>
> So let's tighten the check in parse_one_advertised_remote() to also
> reject empty strings at parse time.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  promisor-remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Personally, I am not enthused to see "NULL or empty", primarily
because it is an entry into a slippery slope.

Sure, an empty string is implausible, but would a single letter URL
a lot more plausible?  Not at all.  How about three letters?  Would
it be now a bit more plausible than an empty string?  Drawing the
line there between "" and "x" does not sound sensible.

Don't we have a code that _uses_ these URL strings to protect it
against a malformed or otherwise unusable URL already?  Can't we
rely on that working correctly to omit this check?

> diff --git a/promisor-remote.c b/promisor-remote.c
> index 8e062ec160..8322349ae8 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -722,7 +722,7 @@ static struct promisor_info *parse_one_advertised_remote(const char *remote_info
>  
>  	string_list_clear(&elem_list, 0);
>  
> -	if (!info->name || !info->url) {
> +	if (!info->name || !*info->name || !info->url || !*info->url) {
>  		warning(_("server advertised a promisor remote without a name or URL: '%s', "
>  			  "ignoring this remote"), remote_info);
>  		promisor_info_free(info);