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

Resolving proxy from env on redirect #4436

Conversation

mbargiel
Copy link
Contributor

@mbargiel mbargiel commented Jan 31, 2022

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 added 6 commits Jan 31, 2022
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.
Copy link
Contributor Author

@mbargiel mbargiel left a comment

Review pointers

lib/adapters/http.js Outdated Show resolved Hide resolved
lib/adapters/http.js Show resolved Hide resolved
lib/adapters/http.js Outdated Show resolved Hide resolved
lib/adapters/http.js Show resolved Hide resolved
test/unit/adapters/http.js Show resolved Hide resolved
test/unit/adapters/http.js Show resolved Hide resolved
lib/adapters/http.js Show resolved Hide resolved
@mbargiel
Copy link
Contributor Author

mbargiel commented Jan 31, 2022

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

hrimhari commented Mar 16, 2022

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

@mbargiel
Copy link
Contributor Author

mbargiel commented Mar 16, 2022

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

11tonytony11 commented Apr 12, 2022

When will this fix be integrated into AXIOS?

@jasonsaayman
Copy link
Member

jasonsaayman commented Apr 21, 2022

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

mbargiel commented Apr 21, 2022

@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 jasonsaayman added follow-up Issue or pull request requires a follow. and removed follow-up Issue or pull request requires a follow. labels May 4, 2022
@jasonsaayman
Copy link
Member

jasonsaayman commented May 9, 2022

@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 jasonsaayman added priority:high follow-up Issue or pull request requires a follow. labels May 9, 2022
@mbargiel
Copy link
Contributor Author

mbargiel commented May 9, 2022

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

@mbargiel
Copy link
Contributor Author

mbargiel commented May 10, 2022

@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
Copy link
Contributor Author

mbargiel commented May 10, 2022

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

@mbargiel
Copy link
Contributor Author

mbargiel commented May 10, 2022

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 mbargiel force-pushed the feature/fix-proxy/resolve-from-env-on-redirect branch 2 times, most recently from 824be7b to ffd1b14 Compare May 10, 2022
@mbargiel mbargiel force-pushed the feature/fix-proxy/resolve-from-env-on-redirect branch from ffd1b14 to 2de3cab Compare May 10, 2022
@mbargiel
Copy link
Contributor Author

mbargiel commented May 10, 2022

@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
Copy link
Contributor Author

mbargiel commented May 10, 2022

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

@jasonsaayman jasonsaayman merged commit 495d5fb into axios:master May 11, 2022
3 checks passed
@jasonsaayman
Copy link
Member

jasonsaayman commented May 11, 2022

Thanks very much for this 👍

@mbargiel mbargiel deleted the feature/fix-proxy/resolve-from-env-on-redirect branch May 11, 2022
@mbargiel mbargiel restored the feature/fix-proxy/resolve-from-env-on-redirect branch May 11, 2022
@mbargiel mbargiel deleted the feature/fix-proxy/resolve-from-env-on-redirect branch May 11, 2022
@mbargiel mbargiel restored the feature/fix-proxy/resolve-from-env-on-redirect branch May 11, 2022
@mbargiel mbargiel deleted the feature/fix-proxy/resolve-from-env-on-redirect branch May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
follow-up Issue or pull request requires a follow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Requests to https through proxy
5 participants