Refactor proxy handling to require https_proxy

[johnstevenson]

Composer has always allowed a single http_proxy (or CGI_HTTP_PROXY) environment variable to be used for both HTTP and HTTPS requests. But many other tools and libraries require scheme-specific values.

The landscape is already complicated by the use of and need for upper and lower case values, so to bring matters inline with current practice https_proxy is now required for HTTPS requests.

The new proxy handler incorporates a transition mechanism, which allows http_proxy to be used for all requests when https_proxy is not set and provides a needsTransitionWarning method for the main application.

Moving to scheme-specific environment variables means that a user may set a single proxy for either HTTP or HTTPS requests. To accomodate this situation during the transition period, an https_proxy value can be set to an empty string which will prevent http_proxy being used for HTTPS requests.

Summary

• Everything works as before
• If a user has a single http_proxy and it is used for HTTPS requests, a warning message is shown after any other output:

Composer now requires separate proxy environment variables for HTTP and HTTPS requests.
Please set `https_proxy` in addition to your existing proxy environment variables.

Implementation
The refactoring was such that it was easier to replace the old ProxyManager class with a new ProxyHandler. In part this was because the original implementation was very rushed to meet the Composer 2 deadline and some unused artifacts from a last minute refactoring of that remained.

There are two small differences:

1. Previously a CGI_HTTP_PROXY value overwrote any http_proxy value when in a CLI environment. Now CGI_HTTP_PROXY (and also cgi_http_proxy which is something Ruby now uses) is only examined if no http_proxy was found. I cannot see this making a difference.

2. The previous getProxyForRequest method obtained the url scheme from parse_url($requestUrl, PHP_URL_SCHEME) ?: 'http', which had the unfortunate effect of turning a false value into http (like when Windows file urls are used in a couple of tests). The latest version uses (string) parse_url($requestUrl, PHP_URL_SCHEME). My concern is that valid request urls without a scheme are now not converted to http and will bypass any proxy. It is likely there are such urls?

The warning message is only shown if a command makes an HTTPS request, but it could easily be changed to show after every command.

This is a follow up to discussions on #11876

[johnstevenson]

Apologies, for the forced-push, but I've updated this to remove some bits that I thought might be needed for the Diagnose command, which it turns out were not. I will provide a separate PR for that later, to bring it up to date with these changes.

I've also fixed some code that was changed in CurlDownloader::initDownload() as part of some PHPStan improvements in 8600894. This changed setting CURLOPT_PROXY with an empty proxy url. However, this is needed because it prevents curl from reading any proxy environment variables (which we have already handled).

The CI tests are now failing on PHP 8.3 and below because StatusCommandTest::testLocallyModifiedPackages() fetches https://www.smarty.net/files/Smarty-3.1.7.zip and that domain name expired on 2024-04-05 is now parked.

[Seldaek]

Oh boy.. I thought it was going to be a reasonably small fix 😅 Thanks, but it will take me some time to review.

[johnstevenson]

I thought it was going to be a reasonably small fix

I did try multiple ways to keep the noise down in the diff, but to no avail.

Perhaps we could do it in the same way we reworked the installer script years ago, by breaking it down into a series of PRs that add functionality by stages?

As a first step, I could provide a new PR that simply adds the new ProxyHandler and ProxyItem classes without plumbing them in, which would make reviewing the functionality much easier.

I don't mind doing this, because I already have a some improvements on this PR yet to push, and in retrospect I think it would make your life easier.

it will take me some time to review.

There's no great rush here to add this change, but please let me know if you would prefer the above approach.

[Seldaek]

No i think it's fine, i can review those new files in isolation from the other changes anyway. Feel free to add whatever is needed here. I won't look until monday earliest

[johnstevenson]

5325ed9 changes how userinfo is set for curl by stripping it out of the proxy url and adding it with the CURLOPT_PROXYUSERPWD option. This is done because some curl versions will show userinfo in the error message when given a malformed url.

It also takes the choice of the curl proxy options out of CurlDownloader and lets RequestProxy provide them as an array.

[Seldaek]

So right now if I understood correctly it just behaves as it did before if only http_proxy is set, correct? If so I'd add this extra warning for the future, so nobody complains that we changed without notice (they'll still complain but that's life..).

Suggested change

	$io->writeError('<warning>Please set `https_proxy` in addition to your existing proxy environment variables.</warning>');
	$io->writeError('<warning>Please set `https_proxy` in addition to your existing proxy environment variables.</warning>');
	$io->writeError('<warning>This fallback (and warning) will be removed in Composer 2.8.0.</warning>');

[johnstevenson]

it just behaves as it did before if only http_proxy is set, correct?

Yes, that is correct. I'll add the additional warning.

Incidentally, I intend to submit a PR for a new how-to-use-composer-behind-a-proxy FAQ, which will detail the requirements and replace the info from the environment variables section (with a suitable reference). This could perhaps be added to the warning message later, if it is accepted.

[Seldaek]

Sounds good to have better docs on this 👍🏻

[Seldaek]

Ok done reviewing, beyond the few small code changes which IMO would improve readability (I struggled in a few places, but eventually grasped things, it's just code style mostly I think..) I think it looks good to me, thanks!

[Seldaek]

Looks good to me now, thanks! Anything else coming or good to merge for you as well?

[johnstevenson]

Yes, it's good to merge for me. I've got a Diagnose update and a docs update but I was going to PR them separately.

[johnstevenson]

Ah, one more thing. Do you want the warning message to be shown after any Composer command? At the moment it is only shown on commands that make requests.

[Seldaek]

Nah it's fine as is I think for the warning. And yes separate PRs later for the rest sounds good, also would appreciate a PR to clean up the transitional stuff for 2.8 but it's not urgent.

I'll merge this then, thanks again