Resolving proxy from env on redirect

[mbargiel]

The current http adapter implementation evaluates the proxy environment variables only once, when the request is initiated. When redirects are followed, the proxy is not re-resolved, which can be problematic in some cases. For instance:

• With http_proxy and https_proxy pointing to two different server hosts/ports, an http: request redirecting to an https: endpoint (or vice versa... go figure why) would result in the wrong proxy configuration used. (curl properly re-evaluates the environment variables on redirect and supports this use case.)
• With no_proxy set to one or more hosts, a request from one of the excluded hosts redirecting to an endpoint served by a non-excluded host would not use a proxy (when it should); likewise, a request from a non-excluded host redirecting to an endpoint served by an excluded host would use a proxy (when it shouldn't)

This PR implements a change in the setProxy code to always re-evaluate the proxy from environment, but only when the original request configuration did not include a proxy (ie. not changing the rules where "resolve from environment" kicks in).

Also as part of this PR is a minor fix to allow using e.g. http_proxy=https://proxyhost:proxyport, so this PR should fix #3903. Basic HTTP proxying should be do-able over an encrypted (TLS) connection to the proxy, for instance when proxy credentials are used. Arguably, that's a strange use case, but there's no reason to not support it, especially when it's trivial to do so (curl does support using http endpoints over an https proxy.)

Ref: proposed contribution 1) mentioned in my comment on issue #4318.

[mbargiel]

Review pointers

[mbargiel]

@jasonsaayman As requested, here's a PR that's fixing some protocol/proxy resolution issues. I thought it would be easier to review this first, then the HTTP CONNECT (https-proxy-agent) PR second. If you would rather have it all in a single "Fix proxy support" PR, let me know.

[hrimhari]

Hi @jasonsaayman , is there anything we can do to help this move forward?

[mbargiel]

I checked the conflicts, and they are trivial to solve, although one of them is worrying: I commented on the PR introducing the change (exposing beforeRedirect as a user option 👍) because it can break proxy support on redirection (👎). I could solve the problem in this PR, by making sure we register a callback that in turns calls either a callback re-applying setProxy (when proxy is used), either the user callback (when options.beforeRedirect is specified), or both (when both are present/used).

[11tonytony11]

When will this fix be integrated into AXIOS?

[jasonsaayman]

I will get to this one as soon as possible, I am trying to wrap up a v1 so that we can move forward with some large upgrades

[mbargiel]

@jasonsaayman Thanks for the update! I'll sit tight until then. Let me know when is a good time to merge into my branch and fix conflicts. Also, how you want me to address the beforeRedirect option proxy regression I mentioned. I still have a follow-up PR, to actually integrate HTTP CONNECT into Axios, but I wanted to get this preliminary/orthogonal work out of the way first.

[jasonsaayman]

@mbargiel please cna you do the merge conflict fixes, I will wait for this and then merge. This is a great and helpful contribution thanks 💯 🥇

[mbargiel]

@jasonsaayman Yes, certainly! I'll try to fit this in my schedule today.

[mbargiel]

@jasonsaayman I merged the main branch and added an extra commit to fix the regression introduced by the config.beforeRedirect support - see 57befc3.

I added a test with which I confirmed the suspected regression and validated the fix.

[mbargiel]

If you prefer the latter beforeRedirect fix to be in its own, follow-up PR, I will happily oblige 😄

[mbargiel]

Once this is merged, I'll get to work preparing the second half of my proxy fix contribution: supporting secure HTTPS proxying (over a TCP tunnel established using HTTP CONNECT). Note that I've been using https-proxy-agent but there are a few problems with it and I had to implement dirty workarounds - not fun. It's been suggested to me that hpagent might be a better contender so I'll look into that instead, but it might take me a whilie. Or I can share what I have that works with https-proxy-agent, and provide an hpagent replacement PR later.

[mbargiel]

@jasonsaayman I'm not sure my beforeRedirect fix is good. My test only covers a single redirection, but I should verify the same case for more than 1 redirect (I suspect my code will append the beforeRedirect callback multiple times and/or the config.beforeRedirect will not be re-registered). If you don't mind, I'll pull it out of this PR and include it as a separate PR + Issue.

[mbargiel]

See Issue #4703. PR to fix it will follow after this one is merged.

[jasonsaayman]

Thanks very much for this 👍