fix: auto-detect HTTP proxy tunneling (#5116)

* fix: auto-detect HTTP proxy tunneling

Signed-off-by: Matteo Collina <hello@matteocollina.com>

* test: enable proxy tunneling in bundle test

Signed-off-by: Matteo Collina <hello@matteocollina.com>

* fix: forward over HTTPS proxy per RFC 9112 §3.2.2

Tunnel decision now depends on the request protocol only. HTTP requests
through an HTTPS proxy use absolute-form request-target over TLS to the
proxy instead of CONNECT, matching Node's built-in http.request and
RFC 9112. HTTPS requests still tunnel via CONNECT.

Http1ProxyWrapper pins the proxy SNI so the inner Client does not derive
it from the rewritten Host header, and wraps ERR_TLS_CERT_ALTNAME_INVALID
into SecureProxyConnectionError for parity with the tunneling path.

Signed-off-by: Matteo Collina <hello@matteocollina.com>

* ci: disable shared-builtin job pending nodejs/node sync

The forwarding fix from #5116 is in undici main but the embedded undici
in nodejs/node still expects the old semantics, so the shared-builtin
build fails until Node.js 26 ships a release embedding the updated
undici. Comment is left in place to re-enable for Node.js 26 at that
point. Node.js 24 stays off as the change is not being backported.

Signed-off-by: Matteo Collina <hello@matteocollina.com>

* fix: force HTTP/1.1 for proxy forwarding

* test: force tunneling for CONNECT regression

---------

Signed-off-by: Matteo Collina <hello@matteocollina.com>

Author: mcollina

.github/workflows/ci.yml

@@ -262,21 +262,24 @@ jobs:
       - name: Run fuzzing tests
         run: npm run test:fuzzing
 
-  test-shared-builtin:
-    name: Test with Node.js ${{ matrix.node-version }} compiled --shared-builtin-undici/undici-path
-    uses: ./.github/workflows/nodejs-shared.yml
-    strategy:
-      fail-fast: false
-      max-parallel: 0
-      matrix:
-        # Node.js 20 and 22 are intentionally excluded here because
-        # --shared-builtin-undici/undici-path still hits upstream Node.js issues there.
-        # Keep validating supported/current majors, and start exercising 26
-        # automatically once a release is available.
-        node-version: ['24', '26']
-        runs-on: ['ubuntu-latest']
-    with:
-      node-version: ${{ matrix.node-version }}
+  # Disabled while the absolute-form proxy forwarding fix from
+  # https://github.com/nodejs/undici/pull/5116 lives only in undici main:
+  # nodejs/node still embeds the previous behavior, so the shared-builtin
+  # build picks up undici from this checkout but runs the bundled Node.js
+  # tests, which expect the old semantics. Re-add Node.js 26 to the matrix
+  # once a Node.js 26 release embeds an undici that includes #5116.
+  # Node.js 24 stays off this job because the change is not being backported.
+  # test-shared-builtin:
+  #   name: Test with Node.js ${{ matrix.node-version }} compiled --shared-builtin-undici/undici-path
+  #   uses: ./.github/workflows/nodejs-shared.yml
+  #   strategy:
+  #     fail-fast: false
+  #     max-parallel: 0
+  #     matrix:
+  #       node-version: ['26']
+  #       runs-on: ['ubuntu-latest']
+  #   with:
+  #     node-version: ${{ matrix.node-version }}
 
   test-types:
     name: Test TypeScript types

docs/docs/api/ProxyAgent.md

@@ -62,12 +62,19 @@ added: v4.8.2
     * `origin` {URL} The proxy origin.
     * `opts` {Object} The resolved options for the dispatcher.
     * Returns: {Dispatcher}
-  * `proxyTunnel` {boolean} Forces tunneling through the proxy. When `false`,
-    requests where both the proxy and the endpoint use the insecure `http:`
-    protocol are sent directly to the proxy with the absolute request URI rather
-    than through a `CONNECT` tunnel, matching `curl` behavior. Secure
-    connections always use a tunnel regardless of this option. **Default:**
-    `true`.
+  * `proxyTunnel` {boolean} Forces tunneling through the proxy. By default,
+    Undici detects tunneling based on the request protocol. If the target
+    endpoint uses HTTPS, Undici establishes a `CONNECT` tunnel through the proxy
+    (after the TLS handshake to the proxy itself when the proxy URL is HTTPS).
+    If the target endpoint uses plain HTTP, Undici forwards the request to the
+    proxy using an HTTP/1.1 absolute-form request target (over TLS when the
+    proxy URL is HTTPS), as required by
+    [RFC 9112 §3.2.2](https://www.rfc-editor.org/rfc/rfc9112.html#name-absolute-form).
+    This non-tunneled forwarding path does not negotiate HTTP/2 with the proxy.
+    Set `proxyTunnel` to `true` to force tunneling for plain HTTP requests as
+    well. Currently, there is no way to facilitate HTTP/1.1 IP tunneling as
+    described in
+    [RFC 9484](https://www.rfc-editor.org/rfc/rfc9484.html#name-http-11-request).
 
 Throws an {InvalidArgumentError} when no proxy URI is provided, when
 `clientFactory` is not a function, or when both `auth` and `token` are supplied.

lib/dispatcher/proxy-agent.js

@@ -37,17 +37,23 @@ function defaultAgentFactory (origin, opts) {
   return new Pool(origin, opts)
 }
 
+function shouldProxyTunnel (requestProtocol, proxyTunnel) {
+  return proxyTunnel === true || requestProtocol !== 'http:'
+}
+
 class Http1ProxyWrapper extends DispatcherBase {
   #client
+  #proxyServername
 
-  constructor (proxyUrl, { headers = {}, connect, factory }) {
+  constructor (proxyUrl, { headers = {}, connect, factory, proxyServername }) {
     if (!proxyUrl) {
       throw new InvalidArgumentError('Proxy URL is mandatory')
     }
 
     super()
 
     this[kProxyHeaders] = headers
+    this.#proxyServername = proxyServername
     if (factory) {
       this.#client = factory(proxyUrl, { connect })
     } else {
@@ -82,6 +88,13 @@ class Http1ProxyWrapper extends DispatcherBase {
     }
     opts.headers = { ...this[kProxyHeaders], ...headers }
 
+    // Pin the SNI/cert hostname to the proxy. Without this the underlying
+    // Client would derive it from the (rewritten) Host header, which points
+    // at the target — wrong for the TLS handshake to the proxy itself.
+    if (this.#proxyServername != null) {
+      opts.servername = this.#proxyServername
+    }
+
     return this.#client[kDispatch](opts, handler)
   }
 
@@ -105,7 +118,7 @@ class ProxyAgent extends DispatcherBase {
       throw new InvalidArgumentError('Proxy opts.clientFactory must be a function.')
     }
 
-    const { proxyTunnel = true, connectTimeout } = opts
+    const { proxyTunnel, connectTimeout } = opts
 
     super()
 
@@ -132,6 +145,7 @@ class ProxyAgent extends DispatcherBase {
     }
 
     const connect = buildConnector({ timeout: connectTimeout, ...opts.proxyTls })
+    const connectHTTP1 = buildConnector({ timeout: connectTimeout, ...opts.proxyTls, allowH2: false })
     this[kConnectEndpoint] = buildConnector({ timeout: connectTimeout, ...opts.requestTls })
     this[kConnectEndpointHTTP1] = buildConnector({ timeout: connectTimeout, ...opts.requestTls, allowH2: false })
 
@@ -152,11 +166,23 @@ class ProxyAgent extends DispatcherBase {
         })
       }
 
-      if (!this[kTunnelProxy] && protocol === 'http:' && this[kProxy].protocol === 'http:') {
+      if (!shouldProxyTunnel(protocol, this[kTunnelProxy])) {
+        const forwardConnect = this[kProxy].protocol === 'https:'
+          ? (opts, cb) => connectHTTP1(opts, (err, socket) => {
+              if (err && err.code === 'ERR_TLS_CERT_ALTNAME_INVALID') {
+                cb(new SecureProxyConnectionError(err))
+              } else {
+                cb(err, socket)
+              }
+            })
+          : connectHTTP1
         return new Http1ProxyWrapper(this[kProxy].uri, {
           headers: this[kProxyHeaders],
-          connect,
-          factory: agentFactory
+          connect: forwardConnect,
+          factory: agentFactory,
+          proxyServername: this[kProxy].protocol === 'https:'
+            ? (this[kProxyTls]?.servername || proxyHostname)
+            : undefined
         })
       }
       return agentFactory(origin, options)

test/env-http-proxy-agent-nodejs-bundle.js

@@ -20,7 +20,7 @@ describe('EnvHttpProxyAgent and setGlobalDispatcher', () => {
     process.env = { ...env }
   })
 
-  test('should work with undici fetch from index-fetch', async (t) => {
+  test('should work with undici fetch from index-fetch with tunneling enabled', async (t) => {
     const { strictEqual } = tspl(t, { plan: 3 })
 
     // Instead of using mocks, start a real server and a minimal proxy server
@@ -73,7 +73,7 @@ describe('EnvHttpProxyAgent and setGlobalDispatcher', () => {
     const proxyAddress = `http://localhost:${proxy.address().port}`
     const serverAddress = `http://localhost:${server.address().port}`
     process.env.http_proxy = proxyAddress
-    setGlobalDispatcher(new EnvHttpProxyAgent())
+    setGlobalDispatcher(new EnvHttpProxyAgent({ proxyTunnel: true }))
 
     const res = await undiciFetch(serverAddress)
     strictEqual(await res.text(), 'Hello world')

test/env-http-proxy-agent.js

@@ -4,6 +4,8 @@ const { tspl } = require('@matteo.collina/tspl')
 const { test, describe, after, beforeEach } = require('node:test')
 const { EnvHttpProxyAgent, ProxyAgent, Agent, fetch, MockAgent } = require('..')
 const { kNoProxyAgent, kHttpProxyAgent, kHttpsProxyAgent, kClosed, kDestroyed, kProxy } = require('../lib/core/symbols')
+const { createServer } = require('node:http')
+const { createProxy } = require('proxy')
 
 const env = { ...process.env }
 
@@ -158,6 +160,54 @@ test('destroys all agents', async (t) => {
   t.ok(dispatcher[kHttpsProxyAgent][kDestroyed])
 })
 
+test('defaults to non-tunneled HTTP proxying for HTTP endpoints - #5093', async (t) => {
+  t = tspl(t, { plan: 3 })
+
+  const server = await buildServer()
+  const proxy = await buildProxy()
+
+  process.env.http_proxy = `http://localhost:${proxy.address().port}`
+
+  const dispatcher = new EnvHttpProxyAgent()
+  const serverUrl = `http://localhost:${server.address().port}`
+
+  try {
+    proxy.on('connect', () => {
+      t.fail('should not tunnel plain HTTP over an HTTP proxy by default')
+    })
+
+    proxy.on('request', (req) => {
+      t.strictEqual(req.url, `${serverUrl}/`)
+    })
+
+    server.on('request', (req, res) => {
+      t.strictEqual(req.url, '/')
+      res.end('ok')
+    })
+
+    const response = await fetch(serverUrl, { dispatcher })
+    t.strictEqual(await response.text(), 'ok')
+  } finally {
+    await new Promise((resolve) => proxy.close(resolve))
+    await new Promise((resolve) => server.close(resolve))
+    await dispatcher.close()
+  }
+})
+
+function buildServer () {
+  return new Promise((resolve) => {
+    const server = createServer({ joinDuplicateHeaders: true })
+    server.listen(0, () => resolve(server))
+  })
+}
+
+function buildProxy () {
+  return new Promise((resolve) => {
+    const server = createProxy(createServer({ joinDuplicateHeaders: true }))
+    server.listen(0, () => resolve(server))
+  })
+}
+
 const createEnvHttpProxyAgentWithMocks = (plan = 1, opts = {}) => {
   const factory = (origin) => {
     const mockAgent = new MockAgent()
@@ -171,7 +221,7 @@ const createEnvHttpProxyAgentWithMocks = (plan = 1, opts = {}) => {
   }
   process.env.http_proxy = 'http://localhost:8080'
   process.env.https_proxy = 'http://localhost:8443'
-  const dispatcher = new EnvHttpProxyAgent({ ...opts, factory })
+  const dispatcher = new EnvHttpProxyAgent({ proxyTunnel: true, ...opts, factory })
   const agentSymbols = [kNoProxyAgent, kHttpProxyAgent, kHttpsProxyAgent]
   agentSymbols.forEach((agentSymbol) => {
     const originalDispatch = dispatcher[agentSymbol].dispatch

test/issue-3897.js

@@ -33,7 +33,7 @@ test('a proxy that drops the CONNECT tunnel fails the request instead of looping
   await once(proxy, 'listening')
 
   const proxyUrl = `http://127.0.0.1:${proxy.address().port}`
-  const proxyAgent = new ProxyAgent(proxyUrl)
+  const proxyAgent = new ProxyAgent({ uri: proxyUrl, proxyTunnel: true })
 
   after(async () => {
     await proxyAgent.close()