[BUG] maxSockets option not respected for proxied connections

[silverwind]

What / Why

The maxSocket option does not appear to be respected in case a HTTPS proxy is in use, e.g. on a typical npm install, a proxy may be bombarded with hundrets of parallel requests, potentially overloading it.

I think the issue is that this module passes maxSockets to https-proxy-agent via the constructor, but that module does not support such a constructor option. It may be possible to set it via agent.maxSockets after agent instantiation, see TooTallNate/proxy-agents#105.

When

When a HTTPS_PROXY environment variable is set to a proxy, meaning https-proxy-agent is being used.

Where

On npm install

How

Current Behavior

Proxy is being bombarded with unbound number of parallel requests

Steps to Reproduce

• Configure a local HTTPS proxy on port 3128
• Run HTTPS_PROXY=https://localhost:3128 npm install on a sizeable project
• Observer the proxy's log

Expected Behavior

Parallel requests to the proxy limited to 15 (the current default)

[nlf]

after some investigation, setting the maxSockets outside of the constructor has no effect. agent-base, the module all three of http-proxy-agent, https-proxy-agent and socks-proxy-agent are built on does not respect the setting at all.

[huangyq23]

hpagent's approach of extending Node's own http and https agent module seems to be a more reasonable implementation than http-proxy-agent/https-proxy-agent/socket-proxy-agent's approach of a separate agent-base.

It does not seem the maintainer of http-proxy-agent/https-proxy-agent/socket-proxy-agent plan to add keep alive support anytime soon.
TooTallNate/node-agent-base#5
TooTallNate/node-socks-proxy-agent#68
TooTallNate/node-http-proxy-agent#23

Proxy server are heavily used in corporate networking environment. Without KeepAlive and connection pooling, it is almost always going to create a DoS.

make-fetch-happen/lib/agent.js

Lines 172 to 174 in ea71110

	const HttpProxyAgent = require('http-proxy-agent')
	const HttpsProxyAgent = require('https-proxy-agent')
	const { SocksProxyAgent } = require('socks-proxy-agent')

I wonder if make-fetch-happen should switch to hpagent or try to make the underlying agent for proxy configurable so that we can plug in alternative implementation.

[silverwind]

I think it's worth switching to hpagent for HTTP and HTTPS. I don't think it can do Socks, so socks-proxy-agent will probably have to remain. As an additional bonus, hpagent's API is easier to use as well as it just takes a proxy server URL instead of parts.

[lukekarrys]

This should be fixed by the latest version of @npmcli/agent@2.1.0. That package still uses https-proxy-agent but only as a way to get a socket connection. There is only a single agent (and HttpsAgent) supplied by @npmcli/agent which respect maxSockets.

@silverwind if you are still seeing issues, please open a new issue (or i can transfer this one and reopen it) over at https://github.com/npm/agent/issues/new/choose