Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor proxy handling to require https_proxy #11915

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

johnstevenson
Copy link
Member

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

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.
@johnstevenson
Copy link
Member Author

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
Copy link
Member

Seldaek commented Apr 12, 2024

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

@johnstevenson
Copy link
Member Author

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
Copy link
Member

Seldaek commented Apr 13, 2024

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
Copy link
Member Author

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 Seldaek added this to the 2.8 milestone Apr 15, 2024
if (ProxyHandler::getInstance()->needsTransitionWarning()) {
$io->writeError('');
$io->writeError('<warning>Composer now requires separate proxy environment variables for HTTP and HTTPS requests.</warning>');
$io->writeError('<warning>Please set `https_proxy` in addition to your existing proxy environment variables.</warning>');
Copy link
Member

@Seldaek Seldaek Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>');

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to have better docs on this 👍🏻

Copy link
Member

@Seldaek Seldaek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Seldaek modified the milestones: 2.8, 2.7 Apr 17, 2024
Copy link
Member

@Seldaek Seldaek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@johnstevenson
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

Seldaek commented Apr 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants