Contribution to fix multiple issues with proxy support

[mbargiel]

Describe the issue

This is not exactly a support question, but your PR bot is extremely aggressive and does not allow submitting a blank report, even one created from the New Issue flow.) On with the issue... 🤞

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch axios@0.24.0 for the project I'm working on.

At first I tried using some of the suggestions here like using https-proxy-agent, but it is severely limited in that it does not normal agent options (I hear it's a regression, and the lib is more or less abandoned). So I went for a deep-dive and I managed to isolate a certain number of flaws. I fixed them using patch-package but I'm willing to contribute them back and open separate PRs for each of these issues (along with tests), when time permits. Would you welcome this?

Issues I believe my changes can fix:

• HTTPS over HTTP Proxy Fails #3384 (this is a one-liner, axios doesn't actually allow HTTPS over HTTP due to a simple logic mixup)
• Requests to https through proxy #3903 (axios doesn't set the proxy protocol in setProxy, which is a problem when the proxy protocol and upstream protocol don't match)
• Axios Proxy Issues - 501 and SSL errors #3459 (might also be related to the same cause as Axios Proxy Issues - 501 and SSL errors #3459 or HTTPS over HTTP Proxy Fails #3384)
• Proxies are not working #4165 (likely a mix of the above, but also possibly related to relative redirects not properly supported due to a bug in follow-redirects

Other changes that I made in my patch:

• Enable re-evaluation of http_proxy, https_proxy and no_proxy (when not passing an explicit proxy config option) when following redirects (http_proxy and https_proxy could be configured differently)

The patch is fairly large and would probably be better conveyed as multiple PRs, but I'll share it below. (We have 160 tests in our project confirming the patch solves a number of issues, but I can't share them here; I would contribute with axios test coverage instead.)

Example Code

N/A

Expected behavior, if applicable

N/A

Environment

• Axios Version [0.24.0]
• Adapter [HTTP]
• Browser [(N/A)]
• Browser Version [(N/A)]
• Node.js Version [12.20.0]
• OS: [Windows 10, macOS Catalina]
• Additional Library Versions [follow-redirects@1.14.5]

Additional context/Screenshots

N/A

[mbargiel]

The patch (includes fix for multiple of the above (except follow-redirects; see follow-redirects/follow-redirects#179 for that one).

diff --git a/node_modules/axios/lib/adapters/http.js b/node_modules/axios/lib/adapters/http.js
index 85f3061..55c1b29 100755
--- a/node_modules/axios/lib/adapters/http.js
+++ b/node_modules/axios/lib/adapters/http.js
@@ -18,29 +18,89 @@ var Cancel = require('../cancel/Cancel');
 
 var isHttps = /https:?/;
 
+function resolveProxyFromEnv(protocol, hostname) {
+  var proxy
+  var proxyEnv = protocol.slice(0, -1) + '_proxy';
+  var proxyUrl = process.env[proxyEnv] || process.env[proxyEnv.toUpperCase()];
+  if (proxyUrl) {
+    var parsedProxyUrl = url.parse(proxyUrl);
+    var noProxyEnv = process.env.no_proxy || process.env.NO_PROXY;
+    var shouldProxy = true;
+
+    if (noProxyEnv) {
+      var noProxy = noProxyEnv.split(',').map(function trim(s) {
+        return s.trim();
+      });
+
+      shouldProxy = !noProxy.some(function proxyMatch(proxyElement) {
+        if (!proxyElement) {
+          return false;
+        }
+        if (proxyElement === '*') {
+          return true;
+        }
+        if (proxyElement[0] === '.' &&
+            hostname.substr(hostname.length - proxyElement.length) === proxyElement) {
+          return true;
+        }
+
+        return hostname === proxyElement;
+      });
+    }
+
+    if (shouldProxy) {
+      proxy = {
+        host: parsedProxyUrl.hostname,
+        port: parsedProxyUrl.port,
+        protocol: parsedProxyUrl.protocol,
+      };
+
+      if (parsedProxyUrl.auth) {
+        var proxyUrlAuth = parsedProxyUrl.auth.split(':');
+        proxy.auth = {
+          username: proxyUrlAuth[0],
+          password: proxyUrlAuth[1]
+        };
+      }
+    }
+  }
+  return proxy;
+}
+
 /**
  *
  * @param {http.ClientRequestArgs} options
  * @param {AxiosProxyConfig} proxy
  * @param {string} location
  */
-function setProxy(options, proxy, location) {
-  options.hostname = proxy.host;
-  options.host = proxy.host;
-  options.port = proxy.port;
-  options.path = location;
-
-  // Basic proxy authorization
-  if (proxy.auth) {
-    var base64 = Buffer.from(proxy.auth.username + ':' + proxy.auth.password, 'utf8').toString('base64');
-    options.headers['Proxy-Authorization'] = 'Basic ' + base64;
+function setProxy(options, configProxy, location) {
+  var proxy = configProxy
+  if (!proxy && proxy !== false) {
+    proxy = resolveProxyFromEnv(options.protocol, options.hostname);
+  }
+  if (proxy) {
+    options.headers.host = options.hostname + (options.port ? ':' + options.port : '');
+    options.hostname = proxy.host;
+    options.host = proxy.host;
+    options.port = proxy.port;
+    options.protocol = proxy.protocol;
+    options.path = location;
+
+    // Basic proxy authorization
+    if (proxy.auth) {
+      var base64 = Buffer.from(proxy.auth.username + ':' + proxy.auth.password, 'utf8').toString('base64');
+      options.headers['Proxy-Authorization'] = 'Basic ' + base64;
+    }
   }
 
-  // If a proxy is used, any redirects must also pass through the proxy
+  // Any redirects must pass through the appropriate proxy (configured or provided with env vars)
   options.beforeRedirect = function beforeRedirect(redirection) {
     redirection.headers.host = redirection.host;
-    setProxy(redirection, proxy, redirection.href);
+    // Pass the original config proxy, proxy will be recomputed from env vars if applicable
+    setProxy(redirection, configProxy, redirection.href);
   };
+
+  return proxy;
 }
 
 /*eslint consistent-return:0*/
@@ -138,80 +198,30 @@ module.exports = function httpAdapter(config) {
       headers: headers,
       agent: agent,
       agents: { http: config.httpAgent, https: config.httpsAgent },
-      auth: auth
+      auth: auth,
+      protocol: protocol,
     };
 
+    var proxy
     if (config.socketPath) {
       options.socketPath = config.socketPath;
     } else {
       options.hostname = parsed.hostname;
       options.port = parsed.port;
-    }
-
-    var proxy = config.proxy;
-    if (!proxy && proxy !== false) {
-      var proxyEnv = protocol.slice(0, -1) + '_proxy';
-      var proxyUrl = process.env[proxyEnv] || process.env[proxyEnv.toUpperCase()];
-      if (proxyUrl) {
-        var parsedProxyUrl = url.parse(proxyUrl);
-        var noProxyEnv = process.env.no_proxy || process.env.NO_PROXY;
-        var shouldProxy = true;
-
-        if (noProxyEnv) {
-          var noProxy = noProxyEnv.split(',').map(function trim(s) {
-            return s.trim();
-          });
-
-          shouldProxy = !noProxy.some(function proxyMatch(proxyElement) {
-            if (!proxyElement) {
-              return false;
-            }
-            if (proxyElement === '*') {
-              return true;
-            }
-            if (proxyElement[0] === '.' &&
-                parsed.hostname.substr(parsed.hostname.length - proxyElement.length) === proxyElement) {
-              return true;
-            }
-
-            return parsed.hostname === proxyElement;
-          });
-        }
-
-        if (shouldProxy) {
-          proxy = {
-            host: parsedProxyUrl.hostname,
-            port: parsedProxyUrl.port,
-            protocol: parsedProxyUrl.protocol
-          };
-
-          if (parsedProxyUrl.auth) {
-            var proxyUrlAuth = parsedProxyUrl.auth.split(':');
-            proxy.auth = {
-              username: proxyUrlAuth[0],
-              password: proxyUrlAuth[1]
-            };
-          }
-        }
-      }
-    }
-
-    if (proxy) {
-      options.headers.host = parsed.hostname + (parsed.port ? ':' + parsed.port : '');
-      setProxy(options, proxy, protocol + '//' + parsed.hostname + (parsed.port ? ':' + parsed.port : '') + options.path);
+      proxy = setProxy(options, config.proxy, protocol + '//' + parsed.hostname + (parsed.port ? ':' + parsed.port : '') + options.path);
     }
 
     var transport;
-    var isHttpsProxy = isHttpsRequest && (proxy ? isHttps.test(proxy.protocol) : true);
+    var useHttpsTransport = (proxy ? isHttps.test(proxy.protocol) : isHttpsRequest);
     if (config.transport) {
       transport = config.transport;
     } else if (config.maxRedirects === 0) {
-      transport = isHttpsProxy ? https : http;
+      transport = useHttpsTransport ? https : http;
     } else {
       if (config.maxRedirects) {
         options.maxRedirects = config.maxRedirects;
       }
-      transport = isHttpsProxy ? httpsFollow : httpFollow;
+      transport = useHttpsTransport ? httpsFollow : httpFollow;
     }
 
     if (config.maxBodyLength > -1) {

To apply this patch, download and install patch-package, copy this patch to patches/axios+0.24.0.patch in your repo, then run npx patch-package axios.

Fixes:

• HTTPS over HTTP
• Different http_proxy/https_proxy configurations (including one set and not the other)
• Redirections switching HTTP/HTTPS protocols

[Amorites]

@mbargiel would you like to create a Pull Request?

[jasonsaayman]

@mbargiel please can you put this in a pull request and tag me? If you cant I will

[mbargiel]

@Amorites @jasonsaayman I definitely want to contribute this as a PR, when time permits. I was planning on splitting it into 3 actually, since it's addressing 3 different issues.

I also discovered since then that Axios does not have support for HTTP CONNECT, meaning that when using a proxy with HTTP (unencrypted) to reach out to HTTPS endpoint URLs, the connection is not encrypted end to end, but actually unencrypted between the client and the proxy. If HTTPS is used to connect to the proxy as well, then it only works if the proxy is able to generate an SSL certificate on the fly and play Man-in-the-Middle (ie intercept the connection). In any case, in this modern age, it is not unreasonable to expect proxy clients to implement use HTTP CONNECT when accessing for HTTPS over HTTP(S) proxies and enable end-to-end encryption. Unfortunately, fixing this in Axios is somewhat trickier (with respect to supporting user-provided agents or agent configurations...); I tried creating a patch to use the unsupported tunnel library, but that would be a kind of bandaid, especially considering tunnel has not been active for almost 4 years now.

[mbargiel]

OK so I went further and implemented an HTTP CONNECT fix since we needed it for our own project. I revisited and simplified patch at the expense of introducing a dependency (which has a small footprint - https://www.npmjs.com/package/proxy-from-env).

I would like to know if the axios maintainers would appreciate one or more PRs to introduce the fixes. I included revisiting tests in my branch (including fixing handling errors in various tests - that would be a separate PR to make tests more robust).

[mbargiel]

To summarize, here's what I would like to contribute, potentially all as separate PRs:

1. Resolve process.env-based proxy when following redirects because redirects can target different hosts or protocols, and http_proxy/https_proxy or no_proxy could apply differently.
   (I used proxy-from-env, but I could revert this part to keep Axios's own http_proxy/https_proxy/no_proxy implementation. IIRC I found some limitations/errors in the Axios implementation which aren't part of proxy-from-env, but I can't remember on top of my head.)
2. Add HTTP CONNECT support for HTTPS requests so they can be encrypted end-to-end.
   (I used https-proxy-agent for this. I tried implementing my own but it ended up feeling more robust to re-use something that lots of people had successfully used, in particular something that was tested on its own.)
3. Fix assertion failure handling in various tests to avoid test runner timeouts.
   (Not proxy-related but it fixed a couple of issues I had while testing.)

[mbargiel]

Sorry @jasonsaayman my brain hadn't connected together the facts that you asked for a PR and you were the maintainer 😆 I'll get on it for all 3 items in the short list above. It will be easier to discuss the specifics of each in their own PR.

[mbargiel]

The 3rd PR is dependent on the first two (well, mostly the "Resolving proxy from env on redirect" one) so I would suggest looking at those first before I open the last PR.

[jasonsaayman]

Ok cool great thanks, I will look at this tonight

[manuelmeister]

Thank you! This is amazing

[jasonsaayman]

I have not forgotten this, i will get to it asap as i am going through all the open pull requests to finish cutting a v1

[jasonsaayman]

This has been merged 🥳

[mbargiel]

@jasonsaayman Actually, there's still one last thing to fix for me to consider this complete 😅 In my Axios fork, I integrated https-proxy-agent to implement HTTPS proxying over TCP tunnels established with HTTP CONNECT (as per HTTP spec), but there's a couple of issues with that library, one of which being that it's inactive. I also had luck getting this working with hpagent, a simpler library that actually extends (rather than replace) the http(s).Agent classes and is a better fit, but it has two small issues that I'm waiting to hear more from.

So I'm not enclined to contribute the https-proxy-agent-based version of my fork, unless you don't mind adding that, then (possibly) switching to hpagent later. At the very least I could open a Draft PR so you can see what it's about and see why I hadn't done so yet.