Re: [PATCH 1/1] http: reject unsupported proxy URL schemes

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

aminnimaj@gmail.com writes:

> +static int is_socks_proxy_protocol(const char *protocol)
> +{
> +	return protocol &&
> +		(!strcmp(protocol, "socks") ||
> +		 !strcmp(protocol, "socks4") ||
> +		 !strcmp(protocol, "socks4a") ||
> +		 !strcmp(protocol, "socks5") ||
> +		 !strcmp(protocol, "socks5h"));
> +}
> +
> +static int set_curl_proxy_type(CURL *result, const char *protocol)
> +{
> +	if (!protocol || !strcmp(protocol, "http"))
> +		return 0;
> +
> +	if (!strcmp(protocol, "socks5h"))
> +		curl_easy_setopt(result, CURLOPT_PROXYTYPE,
> +				 (long)CURLPROXY_SOCKS5_HOSTNAME);
> +	else if (!strcmp(protocol, "socks5"))
> +		curl_easy_setopt(result, CURLOPT_PROXYTYPE,
> +				 (long)CURLPROXY_SOCKS5);
> +	else if (!strcmp(protocol, "socks4a"))
> +		curl_easy_setopt(result, CURLOPT_PROXYTYPE,
> +				 (long)CURLPROXY_SOCKS4A);
> +	else if (!strcmp(protocol, "socks") ||
> +		 !strcmp(protocol, "socks4"))
> +		curl_easy_setopt(result, CURLOPT_PROXYTYPE,
> +				 (long)CURLPROXY_SOCKS4);
> +	else if (!strcmp(protocol, "https")) {
> +		curl_easy_setopt(result, CURLOPT_PROXYTYPE, (long)CURLPROXY_HTTPS);
> +
> +		if (http_proxy_ssl_cert)
> +			curl_easy_setopt(result, CURLOPT_PROXY_SSLCERT,
> +					 http_proxy_ssl_cert);
> +
> +		if (http_proxy_ssl_key)
> +			curl_easy_setopt(result, CURLOPT_PROXY_SSLKEY,
> +					 http_proxy_ssl_key);
> +
> +		if (has_proxy_cert_password())
> +			curl_easy_setopt(result, CURLOPT_PROXY_KEYPASSWD,
> +					 proxy_cert_auth.password);
> +	} else {
> +		return -1;
> +	}
> +
> +	return 0;
> +}

Can these two be rewritten to be more table driven?  I.e.,

static struct socks_proxy_type {
	const char *name;
	long curlsym;
} socks_proxy_type[] = {
	{ "socks", CURLPROXY_SOCKS4_HOSTNAME },
	...
	{ "socks5h", CURLPROXY_SOCKS5_HOSTNAME },
};

static bool is_socks_proxy_protocol(const char *protocol)
{
	if (!protocol)
		return false;
	for (int i = 0; i < ARRAY_SIZE(socks_proxy_type); i++)
        	if (!strcmp(socks_proxy_type[i].name, protocol))
                	return true;
	return false;
}

static int set_curl_proxy_type(...)
{
	for (int i = 0; i < ARRAY_SIZE(socks_proxy_type); i++) {
        	if (!strcmp(socks_proxy_type[i].name, protocol)) {
			curl_easy_setopt(result, CURLOPT_PROXYTYPE, 
					socks_proxy_type[i].curlsym);
			return 0;
		}
	}
	/* otherwise ... */
        if (!strcmp(protocol, "https")) {
		...
	}
}

> diff --git a/t/t5564-http-proxy.sh b/t/t5564-http-proxy.sh
> index 3bcbdef409..db69aa2295 100755
> --- a/t/t5564-http-proxy.sh
> +++ b/t/t5564-http-proxy.sh
> @@ -95,4 +95,9 @@ test_expect_success 'Unix socket requires localhost' - <<\EOT
>  	}
>  EOT
>  
> +test_expect_success 'unknown proxy scheme is rejected' '
> +	! git clone -c http.proxy=htpp://127.0.0.1 https://example.com/repo.git 2>err &&

Use test_must_fail to tell between uncontrolled failures like
crashes and controlled die()s.

> +	grep -Fx "fatal: Invalid proxy URL '\''htpp://127.0.0.1'\'': unsupported proxy scheme '\''htpp'\''" err
> +'

Avoid insisting the exact match with such a long line and stick to
the essential part, like "unsupported proxy scheme '...'".

Also use test_grep for better debuggability.