fix(devtools-proxy-support): work around more issues with proxy-agent (#451)

Work around more issues identified while adding testing for proxy
support to mongosh.

Admittedly, at this point the original choice to use `proxy-agent`
might not seem like the wisest anymore :) We’ll see how receptive
their maintainers are to our patches – so far, none of the issues
discovered should be fundamentally in conflict with the packages’
design philosophy.

Author: addaleax

package-lock.json

@@ -24,14 +24,22 @@
   "license": "Apache-2.0",
   "main": "dist/index.js",
   "exports": {
-    "require": "./dist/index.js",
-    "import": "./dist/.esm-wrapper.mjs"
+    ".": {
+      "require": "./dist/index.js",
+      "import": "./dist/.esm-wrapper.mjs",
+      "types": "./dist/index.d.ts"
+    },
+    "./proxy-options": {
+      "require": "./dist/proxy-options-public.js",
+      "import": "./dist/.esm-wrapper-po.mjs",
+      "types": "./dist/proxy-options-public.d.ts"
+    }
   },
   "types": "./dist/index.d.ts",
   "scripts": {
     "bootstrap": "npm run compile",
     "prepublishOnly": "npm run compile",
-    "compile": "tsc -p tsconfig.json && gen-esm-wrapper . ./dist/.esm-wrapper.mjs",
+    "compile": "tsc -p tsconfig.json && gen-esm-wrapper . ./dist/.esm-wrapper.mjs && gen-esm-wrapper ./dist/proxy-options-public ./dist/.esm-wrapper-po.mjs",
     "typecheck": "tsc --noEmit",
     "eslint": "eslint",
     "prettier": "prettier",
@@ -48,9 +56,10 @@
   "dependencies": {
     "@mongodb-js/socksv5": "^0.0.10",
     "agent-base": "^7.1.1",
+    "debug": "^4.3.6",
     "lru-cache": "^11.0.0",
     "node-fetch": "^3.3.2",
-    "pac-proxy-agent": "7.0.2",
+    "pac-proxy-agent": "^7.0.2",
     "http-proxy-agent": "^7.0.2",
     "https-proxy-agent": "^7.0.5",
     "socks-proxy-agent": "^8.0.4",

packages/devtools-proxy-support/package.json

@@ -145,10 +145,20 @@ export function createAgent(
 
 export function useOrCreateAgent(
   proxyOptions: DevtoolsProxyOptions | AgentWithInitialize,
-  target?: string
+  target?: string,
+  useTargetRegardlessOfExistingAgent = false
 ): AgentWithInitialize | undefined {
   if ('createConnection' in proxyOptions) {
-    return proxyOptions as AgentWithInitialize;
+    const agent = proxyOptions as AgentWithInitialize;
+    if (
+      useTargetRegardlessOfExistingAgent &&
+      target !== undefined &&
+      agent.proxyOptions &&
+      !proxyForUrl(agent.proxyOptions, target)
+    ) {
+      return undefined;
+    }
+    return agent;
   } else {
     if (
       target !== undefined &&

packages/devtools-proxy-support/src/agent.ts

@@ -1,10 +1,4 @@
-export {
-  DevtoolsProxyOptions,
-  DevtoolsProxyOptionsSecrets,
-  translateToElectronProxyConfig,
-  extractProxySecrets,
-  mergeProxySecrets,
-} from './proxy-options';
+export * from './proxy-options-public';
 export { Tunnel, TunnelOptions, createSocks5Tunnel } from './socks5';
 export { createAgent, useOrCreateAgent, AgentWithInitialize } from './agent';
 export {

packages/devtools-proxy-support/src/index.ts

@@ -41,6 +41,7 @@ import type { HttpsProxyAgentOptions } from 'https-proxy-agent';
 import type { HttpsProxyAgent } from 'https-proxy-agent';
 import type { SocksProxyAgentOptions } from 'socks-proxy-agent';
 import type { SocksProxyAgent } from 'socks-proxy-agent';
+import { createRequire } from 'module';
 
 const debug = createDebug('proxy-agent');
 
@@ -180,6 +181,10 @@ export class ProxyAgent extends Agent {
     debug('Request URL: %o', url);
     debug('Proxy URL: %o', proxy);
 
+    if (proxy.startsWith('pac+')) {
+      installPacHttpsHack();
+    }
+
     // attempt to get a cached `http.Agent` instance first
     const cacheKey = `${protocol}+${proxy}`;
     let agent = this.cache.get(cacheKey);
@@ -208,3 +213,51 @@ export class ProxyAgent extends Agent {
     super.destroy();
   }
 }
+
+declare const __webpack_require__: unknown;
+
+// Work around https://github.com/TooTallNate/proxy-agents/pull/329
+// While the proxy-agent package implementation in this file,
+// and in the original, properly check whether an 'upgrade' header
+// is present and set to 'websocket', the pac-proxy-agent performs
+// a similar 'CONNECT vs regular HTTP proxy' selection and doesn't
+// account for this. We monkey-patch in this behavior ourselves.
+function installPacHttpsHack() {
+  // eslint-disable-next-line @typescript-eslint/consistent-type-imports
+  let HttpProxyAgent: typeof import('http-proxy-agent').HttpProxyAgent;
+  // eslint-disable-next-line @typescript-eslint/consistent-type-imports
+  let HttpsProxyAgent: typeof import('https-proxy-agent').HttpsProxyAgent;
+  if (typeof __webpack_require__ === 'undefined') {
+    const pacProxyAgentPath = require.resolve('pac-proxy-agent');
+    const pacRequire = createRequire(pacProxyAgentPath);
+    HttpProxyAgent = pacRequire('http-proxy-agent').HttpProxyAgent;
+    HttpsProxyAgent = pacRequire('https-proxy-agent').HttpsProxyAgent;
+  } else {
+    // No such thing as require.resolve() in webpack, just need to assume
+    // that everything is hoisted :(
+    // eslint-disable-next-line @typescript-eslint/no-var-requires
+    HttpProxyAgent = require('http-proxy-agent').HttpProxyAgent;
+    // eslint-disable-next-line @typescript-eslint/no-var-requires
+    HttpsProxyAgent = require('https-proxy-agent').HttpsProxyAgent;
+  }
+
+  const kCompanionHttpsProxyAgent = Symbol('kCompanionHttpsProxyAgent');
+  // eslint-disable-next-line @typescript-eslint/unbound-method
+  const originalConnect = HttpProxyAgent.prototype.connect;
+  HttpProxyAgent.prototype.connect = function (req, ...args) {
+    if (req.getHeader('upgrade') === 'websocket') {
+      let companionHttpsAgent: HttpsProxyAgent<string> = (this as any)[
+        kCompanionHttpsProxyAgent
+      ];
+      if (!companionHttpsAgent) {
+        companionHttpsAgent = new HttpsProxyAgent(
+          this.proxy.href,
+          this.options
+        );
+        (this as any)[kCompanionHttpsProxyAgent] = companionHttpsAgent;
+      }
+      return companionHttpsAgent.connect(req, ...args);
+    }
+    return originalConnect.call(this, req, ...args);
+  };
+}

packages/devtools-proxy-support/src/proxy-agent.ts

@@ -0,0 +1,9 @@
+// Seperate exports that can be loaded in a browser-only
+// environment (e.g. compass-web).
+export {
+  DevtoolsProxyOptions,
+  DevtoolsProxyOptionsSecrets,
+  translateToElectronProxyConfig,
+  extractProxySecrets,
+  mergeProxySecrets,
+} from './proxy-options';

packages/devtools-proxy-support/src/proxy-options-public.ts

@@ -1,3 +1,4 @@
+// Only use browser-compatible imports or type imports here
 import type { ConnectionOptions } from 'tls';
 import type { TunnelOptions } from './socks5';
 import type { ClientRequest } from 'http';

packages/devtools-proxy-support/src/proxy-options.ts

@@ -1,10 +1,16 @@
 import sinon from 'sinon';
 import { HTTPServerProxyTestSetup } from '../test/helpers';
 import type { Tunnel, TunnelOptions } from './socks5';
-import { createSocks5Tunnel } from './socks5';
+import { connectThroughAgent, createSocks5Tunnel } from './socks5';
 import { expect } from 'chai';
 import { createFetch } from './fetch';
 import type { DevtoolsProxyOptions } from './proxy-options';
+import { createAgent } from './agent';
+import type { AddressInfo, Server } from 'net';
+import { createConnection, createServer } from 'net';
+import { once } from 'events';
+import type { IncomingMessage } from 'http';
+import type { Duplex } from 'stream';
 
 describe('createSocks5Tunnel', function () {
   let setup: HTTPServerProxyTestSetup;
@@ -201,4 +207,65 @@ describe('createSocks5Tunnel', function () {
     const response = await fetch('http://example.com/hello');
     expect(await response.text()).to.equal('OK /hello');
   });
+
+  context('with a non-HTTP target', function () {
+    let netServer: Server;
+    beforeEach(async function () {
+      netServer = createServer((sock) =>
+        sock.once('data', (chk) => sock.end('hello, ' + chk.toString() + '!'))
+      );
+      netServer.listen(0);
+      await once(netServer, 'listening');
+    });
+
+    afterEach(async function () {
+      netServer.close();
+      await once(netServer, 'close');
+    });
+
+    // This simulates a number of aspects of using a PAC proxy with an actual MongoDB server
+    it('can be used with a PAC proxy and a non-HTTP target', async function () {
+      setup.pacFile = () => {
+        return `function FindProxyForURL() { return 'HTTP 127.0.0.1:${setup.httpProxyPort}'; }`;
+      };
+      setup.httpProxyServer.removeAllListeners('connect');
+      setup.httpProxyServer.on(
+        'connect',
+        (req: IncomingMessage, socket: Duplex, head: Buffer) => {
+          socket.unshift(head);
+          const [host, port] = req.url!.split(':');
+          const outgoing = createConnection(+port, host);
+          socket.write('HTTP/1.0 200 OK\r\n\r\n');
+          socket.pipe(outgoing).pipe(socket);
+        }
+      );
+      tunnel = await setupSocks5Tunnel(
+        {
+          useEnvironmentVariableProxies: true,
+          env: {
+            MONGODB_PROXY: `pac+http://foo:bar@127.0.0.1:${setup.httpServerPort}/pac`,
+          },
+        },
+        {},
+        'mongodb://'
+      );
+      if (!tunnel) {
+        // regular conditional instead of assertion so that TS can follow it
+        expect.fail('failed to create Socks5 tunnel');
+      }
+
+      const agent = createAgent({
+        proxy: `socks5://127.0.0.1:${tunnel.config.proxyPort}`,
+      });
+      const socket = await connectThroughAgent({
+        dstAddr: 'localhost',
+        dstPort: (netServer.address() as AddressInfo).port,
+        agent,
+      });
+      socket.write('world');
+      let received = '';
+      for await (const chunk of socket.setEncoding('utf8')) received += chunk;
+      expect(received).to.equal('hello, world!');
+    });
+  });
 });

packages/devtools-proxy-support/src/socks5.spec.ts

@@ -69,11 +69,61 @@ function createFakeHttpClientRequest(
       getHeader(name: string) {
         return headers[name];
       },
+      setHeader(name: string, value: string) {
+        headers[name] = value;
+      },
+      _implicitHeader() {
+        // Even some internal/non-public properties like this are required by http-proxy-agent:
+        // https://github.com/TooTallNate/proxy-agents/blob/5555794b6d9e4b0a36fac80a2d3acea876a8f7dc/packages/http-proxy-agent/src/index.ts#L36
+      },
       overrideProtocol,
     }
   );
 }
 
+export async function connectThroughAgent({
+  dstAddr,
+  dstPort,
+  agent,
+  overrideProtocol,
+}: {
+  dstAddr: string;
+  dstPort: number;
+  agent: AgentWithInitialize;
+  overrideProtocol?: string | undefined;
+}): Promise<Duplex> {
+  const channel = await new Promise<Duplex | undefined>((resolve, reject) => {
+    const req = createFakeHttpClientRequest(dstAddr, dstPort, overrideProtocol);
+    req.onSocket = (sock) => {
+      if (sock) resolve(sock);
+    };
+    agent.createSocket(
+      req,
+      {
+        host: dstAddr,
+        port: dstPort,
+      },
+      (err, sock) => {
+        // Ideally, we would always be using this callback for retrieving the `sock`
+        // instance. However, agent-base does not call the callback at all if
+        // the agent resolved to another agent (as is the case for e.g. `ProxyAgent`).
+        if (err) reject(err);
+        else if (sock) resolve(sock);
+        else
+          reject(
+            new Error(
+              'Received neither error object nor socket from agent.createSocket()'
+            )
+          );
+      }
+    );
+  });
+
+  if (!channel)
+    throw new Error(`Could not create channel to ${dstAddr}:${dstPort}`);
+  return channel;
+}
+
 // The original version of this code was largely taken from
 // https://github.com/mongodb-js/compass/tree/55a5a608713d7316d158dc66febeb6b114d8b40d/packages/ssh-tunnel/src
 class Socks5Server extends EventEmitter implements Tunnel {
@@ -237,40 +287,12 @@ class Socks5Server extends EventEmitter implements Tunnel {
   }
 
   private async forwardOut(dstAddr: string, dstPort: number): Promise<Duplex> {
-    const channel = await new Promise<Duplex>((resolve, reject) => {
-      const req = createFakeHttpClientRequest(
-        dstAddr,
-        dstPort,
-        this.overrideProtocol
-      );
-      req.onSocket = (sock) => {
-        if (sock) resolve(sock);
-      };
-      this.agent.createSocket(
-        req,
-        {
-          host: dstAddr,
-          port: dstPort,
-        },
-        (err, sock) => {
-          // Ideally, we would always be using this callback for retrieving the `sock`
-          // instance. However, agent-base does not call the callback at all if
-          // the agent resolved to another agent (as is the case for e.g. `ProxyAgent`).
-          if (err) reject(err);
-          else if (sock) resolve(sock);
-          else
-            reject(
-              new Error(
-                'Received neither error object nor socket from agent.createSocket()'
-              )
-            );
-        }
-      );
+    return await connectThroughAgent({
+      dstAddr,
+      dstPort,
+      agent: this.agent,
+      overrideProtocol: this.overrideProtocol,
     });
-
-    if (!channel)
-      throw new Error(`Could not create channel to ${dstAddr}:${dstPort}`);
-    return channel;
   }
 
   private async socks5Request(
@@ -309,9 +331,13 @@ class Socks5Server extends EventEmitter implements Tunnel {
       socket.on('error', forwardingErrorHandler);
 
       socket.once('close', () => {
+        if (!channel?.destroyed) channel.destroy();
         this.logger.emit('socks5:forwarded-socket-closed', { ...logMetadata });
         this.connections.delete(socket as Socket);
       });
+      channel.once('close', () => {
+        if (!socket?.destroyed) socket?.destroy();
+      });
 
       socket.pipe(channel).pipe(socket);
     } catch (err) {
@@ -370,7 +396,7 @@ export function createSocks5Tunnel(
     return new ExistingTunnel(socks5OnlyProxyOptions);
   }
 
-  const agent = useOrCreateAgent(proxyOptions, target);
+  const agent = useOrCreateAgent(proxyOptions, target, true);
   if (!agent) return undefined;
 
   let generateCredentials = false;

packages/devtools-proxy-support/src/socks5.ts

@@ -223,12 +223,12 @@ export class HTTPServerProxyTestSetup {
     await Promise.all(closePromises);
   }
 
-  pacFile() {
+  pacFile = () => {
     return `function FindProxyForURL(url, host) {
       if (host === 'pac-invalidproxy') {
         return 'SOCKS5 127.0.0.1:1';
       }
       return 'SOCKS5 127.0.0.1:${this.socks5ProxyPort}';
     }`;
-  }
+  };
 }