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

From: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>
Date: Jul 31, 2024, 15:33 UTC
Message-ID: <3aff1e44-f6eb-b7d7-2bcc-00be8256fcfd@alum.mit.edu>
In-Reply-To: <20240729200953.GB23714@coredump.intra.peff.net>

At 2024-07-29 16:09-0400, Jeff King <peff@peff.net> sent:

Show 10 quoted lines

> Looking into the curl support, I did notice two things:
>
>  - unix sockets are only supported for socks proxies. Is it meaningful
>    to have a path on an http proxy? I don't think so (there is no place
>    to send it in the "GET" line). But perhaps we should limit the new
>    code only to socks.
>
>  - curl says you should put "localhost" in the host field for this,
>    though obviously it is not otherwise used. Should we likewise
>    require that to kick in the code?

Sounds good, I've added checks and tests for both of these cases.

Show 17 quoted lines

>> @@ -1265,7 +1266,24 @@ 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);
>> +		strbuf_addstr(&proxy, proxy_auth.host);
>> +		if (proxy_auth.path) {
>
> There's one gotcha here I wondered about: we usually throw away the path
> element, unless credential.useHTTPPath is set. That only happens after
> we load the per-credential config, though, via credential_apply_config(),
> which in turn is triggered by a call to credential_fill(), etc.
>
> I think this is OK here, since we have just called credential_from_url()
> ourselves, and the earliest credential_fill() we'd hit is from
> init_curl_proxy_auth(), which is called right after the code you're
> touching.

Yes, good lookout but I don't think this is a problem either.

Show 10 quoted lines

>> +			int path_is_supported = 0;
>> +			/* curl_version_info was added in curl 7.10 */
>> +#if LIBCURL_VERSION_NUM >= 0x070a00
>> +			curl_version_info_data *ver = curl_version_info(CURLVERSION_NOW);
>> +			path_is_supported = ver->version_num >= 0x075400;
>> +#endif
>
> We've been trying to keep our curl version #ifdefs in git-curl-compat.h,
> with human-readable names. But in this case, I think we can ditch it
> entirely, as we require curl 7.21.3 or later already.

Ah, okay, that is good to know! I'll remove the #if.

Show 8 quoted lines

> This will be the first time we check curl_version_info() instead of a
> build-time option. I think that makes sense here. Most features require
> extra information at build-time (e.g., CURLOPT_* constants), but in this
> case it is purely a check on the runtime behavior.
>
> We perhaps could get away with just letting an older curl quietly ignore
> the path field, but what you have here is probably a bit friendlier for
> users.

Yes, curl has a nasty tendency to quietly ignore a lot of things. I didn't want users to be uncertain about whether they were using the feature incorrectly if an older curl was the actual issue.

> What if we switched to socks4, which drops all of the auth bits and
> supports only IPs, not hostnames (that came in socks4a). This is the
> shortest I could come up with (only lightly tested):

Ah, many thanks! I like this much better, and I'm not proficient enough with Perl to have written it myself.

Show 9 quoted lines

> I can see two paths forward:
>
>  a. There's been recent discussion about adding an option for Git to
>     report the run-time curl version. That doesn't exist yet, though
>     "git version --build-options" will report the build-time version.
>     That might be good enough for the purposes of testing a build.
>
>  b. Write the test such that it expects the request to work _or_ we see
>     the "version too old" failure.

Got it, I'll go with option b.

> If you usually see success, try running the test script with "--stress"
> to create extra load, and you should see failures.

Huh. I never saw any race issues on my machine even with --stress, but your approach is safer so I'm running with it.

Thank you!