Re: [PATCH 1/6] http: try http_proxy env var when http.proxy config option is not set

From: Jeff King <peff@peff.net>
Date: May 4, 2012, 07:08 UTC
Message-ID: <20120504070802.GA21895@sigill.intra.peff.net>
In-Reply-To: <4FA2B4D3.90809@seap.minhap.es>

On Thu, May 03, 2012 at 06:39:47PM +0200, Nelson Benitez Leon wrote:

Show 17 quoted lines

> +static const char *read_prot_proxy_env(const char *protocol)
> +{
> +	const char *env_proxy;
> +	struct strbuf var = STRBUF_INIT;
> +
> +	strbuf_addf(&var, "%s_proxy", protocol);
> +	env_proxy = getenv(var.buf);
> +	if (!env_proxy && strcmp("http_proxy", var.buf)) {
> +		char *p;
> +		for (p = var.buf; *p; p++)
> +			*p = toupper(*p);
> +		env_proxy = getenv(var.buf);
> +	}
> +	strbuf_release(&var);
> +	
> +	return env_proxy;
> +}

Thanks, this is way more readable than the previous iteration.

Show 12 quoted lines

> +static int host_allowed_by_noproxy_env (const char *host)
> +{
> +	const char *no_proxy = getenv("no_proxy");
> +	if (!no_proxy)
> +		no_proxy = getenv("NO_PROXY");
> +	if (!no_proxy ||
> +	    (strcmp("*", no_proxy) &&
> +	     !strstr(no_proxy, host)))
> +		return 1;
> +	
> +	return 0;
> +}

This simplified parsing misses a lot of corner cases. Three I can see right off the bat:

  1. If your NO_PROXY is "no-proxy.com", and your host is
     "proxy.com", your code will consider that a match, but curl does
     not.

  2. If your NO_PROXY contains "no-proxy.com", but your host is
     "www.no-proxy.com", curl will consider that a match, but your code
     does not.

  3. If your NO_PROXY contains "no-proxy.com", but your host is
     "no-proxy.com:80", curl will consider that a match, but your code
     does not.

I don't see any way around it besides implementing curl's full tokenizing and matching algorithm, which is about a page of code. I'd really prefer not to re-implement bits of curl (especially because they may change later), but AFAIK there is no way to ask curl "is there a proxy configured, and if so, what is it?".

The rest of this patch looks OK to me, though.

-Peff