Fix bug where including auth params in a proxy URL broke all connections

This happened because http-proxy-agent attempts to make a single
(non-CONNECT) proxy request, including proxy-auth there, and that
resulted in a setHeader() call on the request after the initial request
had been created with raw headers (which permanently sets the headers),
which threw HTTP_HEADERS_ALREADY_SENT.

To fix this, we now drop proxy-agent to implement our own proxy logic,
and always use https-proxy-agent for HTTP(S), so we *always* do a
CONNECT in these cases. This allows the proxy-agent to set the header
for the CONNECT independently of the raw headers set later on. It also
lets us implement our own stricter caching for proxy agents, and it's
going to let us doing better custom configuration for proxy-agents in
other ways too (imminently).

Author: pimterry

custom-typings/proxy-agent-modules.d.ts

@@ -0,0 +1,5 @@
+// Each of these breaks because they use synthetic default imports internally for
+// core node packages. Easiest to skip their types entirely:
+declare module 'https-proxy-agent';
+declare module 'socks-proxy-agent';
+declare module 'pac-proxy-agent';

custom-typings/proxy-agent.d.ts

@@ -129,8 +129,6 @@
     "form-data-encoder": "^1.7.2",
     "formdata-node": "^4.3.2",
     "fs-extra": "^8.1.0",
-    "http-proxy-agent": "^2.0.0",
-    "https-proxy-agent": "^2.2.1",
     "karma": "^6.3.2",
     "karma-chai": "^0.1.0",
     "karma-chrome-launcher": "^2.2.0",
@@ -168,7 +166,6 @@
     "@graphql-tools/schema": "^8.5.0",
     "@graphql-tools/utils": "^8.8.0",
     "@httptoolkit/httpolyglot": "^2.0.1",
-    "@httptoolkit/proxy-agent": "^5.0.1-socks-lookup-fix.0",
     "@httptoolkit/subscriptions-transport-ws": "^0.11.2",
     "@httptoolkit/websocket-stream": "^6.0.1",
     "@types/cors": "^2.8.6",
@@ -188,13 +185,17 @@
     "graphql-tag": "^2.12.6",
     "http-encoding": "^1.5.1",
     "http2-wrapper": "2.0.5",
+    "https-proxy-agent": "^5.0.1",
     "isomorphic-ws": "^4.0.1",
     "lodash": "^4.16.4",
+    "lru-cache": "^7.14.0",
     "native-duplexpair": "^1.0.0",
     "node-forge": "^1.2.1",
+    "pac-proxy-agent": "^5.0.0",
     "parse-multipart-data": "^1.4.0",
     "performance-now": "^2.1.0",
     "portfinder": "1.0.28",
+    "socks-proxy-agent": "^7.0.0",
     "typed-error": "^3.0.2",
     "uuid": "^8.3.2",
     "ws": "^8.8.0"

package.json

@@ -1,7 +1,14 @@
 import * as _ from 'lodash';
+import * as url from 'url';
 import * as http from 'http';
 import * as https from 'https';
-import ProxyAgent = require('@httptoolkit/proxy-agent');
+
+import * as LRU from 'lru-cache';
+
+import getHttpsProxyAgent = require('https-proxy-agent');
+import getPacProxyAgent = require('pac-proxy-agent');
+import { SocksProxyAgent } from 'socks-proxy-agent';
+const getSocksProxyAgent = (opts: any) => new SocksProxyAgent(opts);
 
 import { isNode } from "../util/util";
 import { getProxySetting, matchesNoProxy, ProxySettingSource } from './proxy-config';
@@ -16,6 +23,31 @@ const KeepAliveAgents = isNode
         })
     } : {};
 
+const ProxyAgentFactoryMap = {
+    'http:': getHttpsProxyAgent, // HTTPS here really means 'CONNECT-tunnelled' - it can do either
+    'https:': getHttpsProxyAgent,
+
+    'pac+http:': getPacProxyAgent,
+    'pac+https:': getPacProxyAgent,
+
+    'socks:': getSocksProxyAgent,
+    'socks4:': getSocksProxyAgent,
+    'socks4a:': getSocksProxyAgent,
+    'socks5:': getSocksProxyAgent,
+    'socks5h:': getSocksProxyAgent
+} as const;
+
+const proxyAgentCache = new LRU<string, http.Agent>({
+    max: 20,
+
+    ttl: 1000 * 60 * 5, // Drop refs to unused agents after 5 minutes
+    ttlResolution: 1000 * 60, // Check for expiry once every minute maximum
+    ttlAutopurge: true, // Actively drop expired agents
+    updateAgeOnGet: true // Don't drop agents while they're in use
+});
+
+const getCacheKey = (options: {}) => JSON.stringify(options);
+
 export async function getAgent({
     protocol, hostname, port, tryHttp2, keepAlive, proxySettingSource
 }: {
@@ -35,7 +67,24 @@ export async function getAgent({
         if (!matchesNoProxy(hostname, port, proxySetting.noProxy)) {
             // We notably ignore HTTP/2 upstream in this case: it's complicated to mix that up with proxying
             // so for now we ignore it entirely.
-            return new ProxyAgent(proxySetting.proxyUrl) as http.Agent;
+
+            const cacheKey = getCacheKey({
+                url: proxySetting?.proxyUrl
+            });
+
+            if (!proxyAgentCache.has(cacheKey)) {
+                const { protocol, auth, hostname, port } = url.parse(proxySetting?.proxyUrl);
+                const buildProxyAgent = ProxyAgentFactoryMap[protocol as keyof typeof ProxyAgentFactoryMap];
+
+                proxyAgentCache.set(cacheKey, buildProxyAgent({
+                    protocol,
+                    auth,
+                    hostname,
+                    port
+                }));
+            }
+
+            return proxyAgentCache.get(cacheKey);
         }
     }
 

src/rules/http-agents.ts

@@ -2669,6 +2669,30 @@ nodeOnly(() => {
                 expect((await proxyEndpoint.getSeenRequests()).length).to.equal(1);
             });
 
+            it("should support authenticating to the remote proxy", async () => {
+                // Remote server sends fixed response on this one URL:
+                await remoteServer.forGet('/test-url').thenReply(200, "Remote server says hi!");
+
+                // Mockttp forwards requests via our intermediate proxy
+                await server.forAnyRequest().thenPassThrough({
+                    proxyConfig: {
+                        proxyUrl: intermediateProxy.url
+                            .replace('://', '://username:password@')
+                    }
+                });
+
+                const response = await request.get(remoteServer.urlFor("/test-url"));
+
+                // We get a successful response
+                expect(response).to.equal("Remote server says hi!");
+                // And it went via the intermediate proxy
+                expect((await proxyEndpoint.getSeenRequests()).length).to.equal(1);
+
+                // N.B: we don't actually check that the auth params are used here, only that the request with
+                // them in the URL sends OK. We can't, unfortunately, since they only exist in the CONNECT
+                // and that's always unwrapped and never exposed. Visible in Wireshark though.
+            });
+
             it("should skip the proxy if the target is in the no-proxy list", async () => {
                 // Remote server sends fixed response on this one URL:
                 await remoteServer.forGet('/test-url').thenReply(200, "Remote server says hi!");

test/integration/proxy.spec.ts

@@ -12,7 +12,9 @@
             "scripthost"
         ],
         "paths": {
-            "@httptoolkit/proxy-agent": ["./custom-typings/proxy-agent.d.ts"]
+            "https-proxy-agent": ["./custom-typings/proxy-agent-modules.d.ts"],
+            "socks-proxy-agent": ["./custom-typings/proxy-agent-modules.d.ts"],
+            "pac-proxy-agent": ["./custom-typings/proxy-agent-modules.d.ts"]
         }
     },
     "compileOnSave": true,