Re: [PATCH] http: do not ignore proxy path

From: Jeff King <peff@peff.net>
Date: Jul 26, 2024, 21:11 UTC
Message-ID: <20240726211156.GA714986@coredump.intra.peff.net>
In-Reply-To: <pull.1767.git.1722009170590.gitgitgadget@gmail.com>

On Fri, Jul 26, 2024 at 03:52:50PM +0000, Ryan Hendrickson via GitGitGadget wrote:

>      * Tests: I could use a pointer on how best to add a test for this.
>        Adding a case to t5564-http-proxy.sh seems straightforward but I
>        don't think httpd can be configured to listen to domain sockets; can
>        I use netcat?

I don't offhand know of a way to test this without a custom program like netcat. If it's the only option, it's OK to use tools that might not be available everywhere as long as the tests are marked as optional with the appropriate prerequisite. You can find prior art by looking for test_lazy_prereq calls (e.g., the ones for GZIP or ZIPINFO are pretty straight-forward).

I would warn that there are several not-quite-compatible variants of netcat floating around, which can create headaches. You might be better off with a short perl script using IO::Socket::UNIX or similar.

Show 16 quoted lines

> diff --git a/http.c b/http.c
> index 623ed234891..0cd75986a6b 100644
> --- a/http.c
> +++ b/http.c
> @@ -1265,7 +1265,13 @@ static CURL *get_curl_handle(void)
>  		if (!proxy_auth.host)
>  			die("Invalid proxy URL '%s'", curl_http_proxy);
>  
> -		curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
> +		if (proxy_auth.path) {
> +			struct strbuf proxy = STRBUF_INIT;
> +			strbuf_addf(&proxy, "%s/%s", proxy_auth.host, proxy_auth.path);
> +			curl_easy_setopt(result, CURLOPT_PROXY, proxy.buf);
> +			strbuf_release(&proxy);
> +		} else
> +			curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);

The fields in the proxy_auth struct have been parsed from the url, with any url encoding removed. But then we paste them back together into a pseudo-url without doing any further encoding. Is that correct?

I doubt that the host contains a "/", but if you had a path that contained a "%", then the URL form of that is going to be %25. Which is curl expecting to get here?

I say "pseudo-url" because it is weird to slap a path on the end of the hostname but leave off the scheme, etc. Which kind of makes me wonder why we pass proxy_auth.host in the first place, and not simply the original curl_http_proxy. It looks like a weird interaction between 372370f167 (http: use credential API to handle proxy authentication, 2016-01-26) and 57415089bd (http: honor empty http.proxy option to bypass proxy, 2017-04-11). The former added a _second_ CURLOPT_PROXY call, and the latter removed the first one.

I wonder if we could go back to passing the string straight to curl (as we did prior to 2016), and keeping the proxy_auth struct purely as a mechanism for gathering credentials. That would presumably fix your use case. And this is also perhaps an interesting data point for Junio's question about regressions (if people were passing paths, it used to work and was broken in 2016).

-Peff