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

From: Jeff King <peff@peff.net>
Date: Aug 1, 2024, 06:04 UTC
Message-ID: <20240801060418.GB621899@coredump.intra.peff.net>
In-Reply-To: <pull.1767.v4.git.1722489776279.gitgitgadget@gmail.com>

On Thu, Aug 01, 2024 at 05:22:56AM +0000, Ryan Hendrickson via GitGitGadget wrote:

Show 11 quoted lines

> From: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>
> 
> The documentation for `http.proxy` describes that option, and the
> environment variables it overrides, as supporting "the syntax understood
> by curl". curl allows SOCKS proxies to use a path to a Unix domain
> socket, like `socks5h://localhost/path/to/socket.sock`. Git should
> therefore include, if present, the path part of the proxy URL in what it
> passes to libcurl.
> 
> Co-authored-by: Jeff King <peff@peff.net>
> Signed-off-by: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>

Thanks for crediting me. I'll add my:

 Signed-off-by: Jeff King <peff@peff.net>

to be explicit that the proxy script is under the DCO.

Show 18 quoted lines

> -		curl_easy_setopt(result, CURLOPT_PROXY, proxy_auth.host);
> +		strbuf_addstr(&proxy, proxy_auth.host);
> +		if (proxy_auth.path) {
> +			curl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);
> +
> +			if (ver->version_num < 0x075400)
> +				die("libcurl 7.84 or later is required to support paths in proxy URLs");
> +
> +			if (!starts_with(proxy_auth.protocol, "socks"))
> +				die("Invalid proxy URL '%s': only SOCKS proxies support paths",
> +				    curl_http_proxy);
> +
> +			if (strcasecmp(proxy_auth.host, "localhost"))
> +				die("Invalid proxy URL '%s': host must be localhost if a path is present",
> +				    curl_http_proxy);
> +
> +			strbuf_addch(&proxy, '/');
> +			strbuf_add_percentencode(&proxy, proxy_auth.path, 0);

This all looks good to me. I wondered briefly whether proxy_auth.protocol could ever be NULL, but I think we refuse to parse such a URL.

Show 13 quoted lines

> +start_socks() {
> +	mkfifo socks_output &&
> +	{
> +		"$PERL_PATH" "$TEST_DIRECTORY/socks4-proxy.pl" "$1" >socks_output &
> +		echo $! > "$TRASH_DIRECTORY/socks.pid"
> +	} &&
> +	read line <socks_output &&
> +	test "$line" = ready
> +}
> +
> +# The %30 tests that the correct amount of percent-encoding is applied to the
> +# proxy string passed to curl.
> +test_lazy_prereq SOCKS_PROXY 'test_have_prereq PERL && start_socks "$TRASH_DIRECTORY/%30.sock"'

OK, I see you figured out that the lazy prereq requires giving the full path to the socket. :) I had forgotten that we also run the prereq in a subshell to avoid side effects, but you caught that, as well.

All of this to me is good evidence that the non-lazy version you had originally is a better approach. But I don't think it's worth spending time fighting over, so I'm OK either way.

> +test_atexit 'test ! -e "$TRASH_DIRECTORY/socks.pid" || kill "$(cat "$TRASH_DIRECTORY/socks.pid")"'

Ah, good. I had meant to mention cleaning up at the end (as we do for git-daemon and apache), but forgot. I'm glad you included it here.

Show 10 quoted lines

> +test_expect_success SOCKS_PROXY 'clone via Unix socket' '
> +	test_when_finished "rm -rf clone" &&
> +	test_config_global http.proxy "socks4://localhost$PWD/%2530.sock" && {
> +		{
> +			GIT_TRACE_CURL=$PWD/trace git clone "$HTTPD_URL/smart/repo.git" clone 2>err &&
> +			grep -i "SOCKS4 request granted." trace
> +		} ||
> +		grep "^fatal: libcurl 7\.84 or later" err
> +	}
> +'

Looks good. Usually I do not bother with escaping "." in grep messages, as it is already a loose match and it keeps the test easier to read. But it is OK to do so.

We do have a test_grep wrapper which gives nicer output when the match fails, but maybe that is a bad choice here, since we accept either of the two messages. (Likewise for using test_cmp, I suppose).

The use of "||" in the command-chaining is unusual enough that it's probably worth calling out either in a comment or in the commit message.

Show 14 quoted lines

> +test_expect_success 'Unix socket requires socks*:' '
> +	! git clone -c http.proxy=localhost/path https://example.com/repo.git 2>err && {
> +		grep "^fatal: Invalid proxy URL '\''localhost/path'\'': only SOCKS proxies support paths" err ||
> +		grep "^fatal: libcurl 7\.84 or later" err
> +	}
> +'
> +
> +test_expect_success 'Unix socket requires localhost' '
> +	! git clone -c http.proxy=socks4://127.0.0.1/path https://example.com/repo.git 2>err && {
> +		grep "^fatal: Invalid proxy URL '\''socks4://127\.0\.0\.1/path'\'': host must be localhost if a path is present" err ||
> +		grep "^fatal: libcurl 7\.84 or later" err
> +	}
> +'
> +

Likewise here, I'd probably just match the single-quotes with "." for readability (but it's OK to write it as you did). You can (since a week or so ago) also use a here-doc body like:

  test_expect_success 'Unix socket requires socks*:' - <<\EOT
	...
	test_grep "^fatal: Invalid proxy URL 'localhost/path': only SOCKS proxies support paths" err ||
	...
  EOT

but I'm OK with it either way. The same "||" comment applies.

-Peff