Resolving proxy from env on redirect#4436
Conversation
Redirections can target different hosts or change the protocol from http to https or vice versa. When the proxy option is inferred from the environment, it should be recomputed when the protocol or host changes because the proxy host can differ or even whether to proxy or not can differ.
1) setProxy now changes request options protocol when using a proxy with explicit protocol. 2) As a result, selection of the correct transport can be simplified. 3) Legacy agent selection needs to be moved done accordingly. (Is 'agent' option even still used?)
The proxy-from-env library is a popular, lightweight library that is very easy to use and covers a few more cases, not to mention it has extensive test coverage.
|
@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. |
|
Hi @jasonsaayman , is there anything we can do to help this move forward? |
|
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 |
|
When will this fix be integrated into AXIOS? |
|
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 |
|
@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 |
|
@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 💯 🥇 |
|
@jasonsaayman Yes, certainly! I'll try to fit this in my schedule today. |
|
@jasonsaayman I merged the main branch and added an extra commit to fix the regression introduced by the I added a test with which I confirmed the suspected regression and validated the fix. |
|
If you prefer the latter |
|
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
force-pushed
the
feature/fix-proxy/resolve-from-env-on-redirect
branch
2 times, most recently
from
824be7b to
ffd1b14
Compare
mbargiel
force-pushed
the
feature/fix-proxy/resolve-from-env-on-redirect
branch
from
ffd1b14 to
2de3cab
Compare
|
@jasonsaayman I'm not sure my |
|
See Issue #4703. PR to fix it will follow after this one is merged. |
|
Thanks very much for this 👍 |
* Fixing http adapter to recompute proxy on redirect Redirections can target different hosts or change the protocol from http to https or vice versa. When the proxy option is inferred from the environment, it should be recomputed when the protocol or host changes because the proxy host can differ or even whether to proxy or not can differ. * Fixing proxy protocol handling 1) setProxy now changes request options protocol when using a proxy with explicit protocol. 2) As a result, selection of the correct transport can be simplified. 3) Legacy agent selection needs to be moved done accordingly. (Is 'agent' option even still used?) * Using proxy-from-env library to handle proxy env vars The proxy-from-env library is a popular, lightweight library that is very easy to use and covers a few more cases, not to mention it has extensive test coverage. * Fixing proxy auth handling * Adding test proving env vars are re-resolved on redirect * Revert unnecessary change * Fixing proxy beforeRedirect regression * Fixing lint errors * Revert "Fixing lint errors" This reverts commit 2de3cab. * Revert "Fixing proxy beforeRedirect regression" This reverts commit 57befc3.
* Fixing http adapter to recompute proxy on redirect Redirections can target different hosts or change the protocol from http to https or vice versa. When the proxy option is inferred from the environment, it should be recomputed when the protocol or host changes because the proxy host can differ or even whether to proxy or not can differ. * Fixing proxy protocol handling 1) setProxy now changes request options protocol when using a proxy with explicit protocol. 2) As a result, selection of the correct transport can be simplified. 3) Legacy agent selection needs to be moved done accordingly. (Is 'agent' option even still used?) * Using proxy-from-env library to handle proxy env vars The proxy-from-env library is a popular, lightweight library that is very easy to use and covers a few more cases, not to mention it has extensive test coverage. * Fixing proxy auth handling * Adding test proving env vars are re-resolved on redirect * Revert unnecessary change * Fixing proxy beforeRedirect regression * Fixing lint errors * Revert "Fixing lint errors" This reverts commit 2de3cab. * Revert "Fixing proxy beforeRedirect regression" This reverts commit 57befc3.
* Fixing http adapter to recompute proxy on redirect Redirections can target different hosts or change the protocol from http to https or vice versa. When the proxy option is inferred from the environment, it should be recomputed when the protocol or host changes because the proxy host can differ or even whether to proxy or not can differ. * Fixing proxy protocol handling 1) setProxy now changes request options protocol when using a proxy with explicit protocol. 2) As a result, selection of the correct transport can be simplified. 3) Legacy agent selection needs to be moved done accordingly. (Is 'agent' option even still used?) * Using proxy-from-env library to handle proxy env vars The proxy-from-env library is a popular, lightweight library that is very easy to use and covers a few more cases, not to mention it has extensive test coverage. * Fixing proxy auth handling * Adding test proving env vars are re-resolved on redirect * Revert unnecessary change * Fixing proxy beforeRedirect regression * Fixing lint errors * Revert "Fixing lint errors" This reverts commit 2de3cab. * Revert "Fixing proxy beforeRedirect regression" This reverts commit 57befc3.
* Fixing http adapter to recompute proxy on redirect Redirections can target different hosts or change the protocol from http to https or vice versa. When the proxy option is inferred from the environment, it should be recomputed when the protocol or host changes because the proxy host can differ or even whether to proxy or not can differ. * Fixing proxy protocol handling 1) setProxy now changes request options protocol when using a proxy with explicit protocol. 2) As a result, selection of the correct transport can be simplified. 3) Legacy agent selection needs to be moved done accordingly. (Is 'agent' option even still used?) * Using proxy-from-env library to handle proxy env vars The proxy-from-env library is a popular, lightweight library that is very easy to use and covers a few more cases, not to mention it has extensive test coverage. * Fixing proxy auth handling * Adding test proving env vars are re-resolved on redirect * Revert unnecessary change * Fixing proxy beforeRedirect regression * Fixing lint errors * Revert "Fixing lint errors" This reverts commit 2de3cab. * Revert "Fixing proxy beforeRedirect regression" This reverts commit 57befc3.
* Fixing http adapter to recompute proxy on redirect Redirections can target different hosts or change the protocol from http to https or vice versa. When the proxy option is inferred from the environment, it should be recomputed when the protocol or host changes because the proxy host can differ or even whether to proxy or not can differ. * Fixing proxy protocol handling 1) setProxy now changes request options protocol when using a proxy with explicit protocol. 2) As a result, selection of the correct transport can be simplified. 3) Legacy agent selection needs to be moved done accordingly. (Is 'agent' option even still used?) * Using proxy-from-env library to handle proxy env vars The proxy-from-env library is a popular, lightweight library that is very easy to use and covers a few more cases, not to mention it has extensive test coverage. * Fixing proxy auth handling * Adding test proving env vars are re-resolved on redirect * Revert unnecessary change * Fixing proxy beforeRedirect regression * Fixing lint errors * Revert "Fixing lint errors" This reverts commit 2de3cab. * Revert "Fixing proxy beforeRedirect regression" This reverts commit 57befc3.
5 participants
The current
httpadapter 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:http_proxyandhttps_proxypointing to two different server hosts/ports, anhttp:request redirecting to anhttps:endpoint (or vice versa... go figure why) would result in the wrong proxy configuration used. (curlproperly re-evaluates the environment variables on redirect and supports this use case.)no_proxyset 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
setProxycode 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 (curldoes support usinghttpendpoints over anhttpsproxy.)Ref: proposed contribution 1) mentioned in my comment on issue #4318.