diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 72cd13c2bc5..1615b48a568 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/docs/docs/api/ProxyAgent.md b/docs/docs/api/ProxyAgent.md index 8db7221a362..d922fe71b1d 100644 --- a/docs/docs/api/ProxyAgent.md +++ b/docs/docs/api/ProxyAgent.md @@ -27,7 +27,7 @@ For detailed information on the parsing process and potential validation errors, * **clientFactory** `(origin: URL, opts: Object) => Dispatcher` (optional) - Default: `(origin, opts) => new Pool(origin, opts)` * **requestTls** `BuildOptions` (optional) - Options object passed when creating the underlying socket via the connector builder for the request. It extends from [`Client#ConnectOptions`](/docs/docs/api/Client.md#parameter-connectoptions). * **proxyTls** `BuildOptions` (optional) - Options object passed when creating the underlying socket via the connector builder for the proxy server. It extends from [`Client#ConnectOptions`](/docs/docs/api/Client.md#parameter-connectoptions). -* **proxyTunnel** `boolean` (optional) - For connections involving secure protocols, Undici will always establish a tunnel via the HTTP2 CONNECT extension. If proxyTunnel is set to true, this will occur for unsecured proxy/endpoint connections as well. Currently, there is no way to facilitate HTTP1 IP tunneling as described in https://www.rfc-editor.org/rfc/rfc9484.html#name-http-11-request. If proxyTunnel is set to false (the default), ProxyAgent connections where both the Proxy and Endpoint are unsecured will issue all requests to the Proxy, and prefix the endpoint request path with the endpoint origin address. +* **proxyTunnel** `boolean` (optional) - Undici automatically detects when proxy tunneling is required based on the request protocol. If the target endpoint uses HTTPS, Undici establishes a CONNECT tunnel through the proxy (TLS handshaking with the proxy first if the proxy URL is also HTTPS). If the target endpoint uses plain HTTP, Undici forwards the request to the proxy with the target origin prefixed in the request path (over TLS if the proxy URL is HTTPS), as required by [RFC 9112 §3.2.2](https://www.rfc-editor.org/rfc/rfc9112.html#name-absolute-form). Set `proxyTunnel` to `true` to force tunneling for plain HTTP requests as well. Currently, there is no way to facilitate HTTP1 IP tunneling as described in https://www.rfc-editor.org/rfc/rfc9484.html#name-http-11-request. Examples: diff --git a/lib/dispatcher/proxy-agent.js b/lib/dispatcher/proxy-agent.js index 8522bdd586d..349394eaf76 100644 --- a/lib/dispatcher/proxy-agent.js +++ b/lib/dispatcher/proxy-agent.js @@ -37,10 +37,15 @@ 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') } @@ -48,6 +53,7 @@ class Http1ProxyWrapper extends DispatcherBase { 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() @@ -151,11 +164,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) => connect(opts, (err, socket) => { + if (err && err.code === 'ERR_TLS_CERT_ALTNAME_INVALID') { + cb(new SecureProxyConnectionError(err)) + } else { + cb(err, socket) + } + }) + : connect 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) diff --git a/test/env-http-proxy-agent-nodejs-bundle.js b/test/env-http-proxy-agent-nodejs-bundle.js index 4cf238e9f6b..42f5c692f48 100644 --- a/test/env-http-proxy-agent-nodejs-bundle.js +++ b/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') diff --git a/test/env-http-proxy-agent.js b/test/env-http-proxy-agent.js index 97969cfaf20..e31bf5aac74 100644 --- a/test/env-http-proxy-agent.js +++ b/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 diff --git a/test/proxy-agent.js b/test/proxy-agent.js index fdf9024b407..e452c4f7648 100644 --- a/test/proxy-agent.js +++ b/test/proxy-agent.js @@ -901,7 +901,7 @@ test('use proxy-agent with setGlobalDispatcher', async (t) => { const serverUrl = `http://localhost:${server.address().port}` const proxyUrl = `http://localhost:${proxy.address().port}` - const proxyAgent = new ProxyAgent({ uri: proxyUrl, proxyTunnel: false }) + const proxyAgent = new ProxyAgent({ uri: proxyUrl }) const parsedOrigin = new URL(serverUrl) setGlobalDispatcher(proxyAgent) @@ -985,7 +985,7 @@ test('ProxyAgent correctly sends headers when using fetch - #1355, #1623', async const serverUrl = `http://localhost:${server.address().port}` const proxyUrl = `http://localhost:${proxy.address().port}` - const proxyAgent = new ProxyAgent({ uri: proxyUrl, proxyTunnel: false }) + const proxyAgent = new ProxyAgent({ uri: proxyUrl }) setGlobalDispatcher(proxyAgent) after(() => setGlobalDispatcher(defaultDispatcher)) @@ -1095,7 +1095,7 @@ test('should throw when proxy does not return 200', async (t) => { return false } - const proxyAgent = new ProxyAgent({ uri: proxyUrl, proxyTunnel: false }) + const proxyAgent = new ProxyAgent({ uri: proxyUrl }) try { await request(serverUrl, { dispatcher: proxyAgent }) t.fail() @@ -1273,6 +1273,61 @@ test('Proxy via HTTPS to HTTPS endpoint', async (t) => { proxyAgent.close() }) +test('Proxy via HTTPS to HTTP endpoint forwards without tunneling by default', async (t) => { + t = tspl(t, { plan: 4 }) + const server = await buildServer() + const proxy = await buildSSLProxy() + + const serverUrl = `http://localhost:${server.address().port}` + const proxyUrl = `https://localhost:${proxy.address().port}` + const proxyAgent = new ProxyAgent({ + uri: proxyUrl, + proxyTls: { + ca: [ + certs.root.crt + ], + servername: 'proxy' + } + }) + + server.on('request', function (req, res) { + t.ok(!req.connection.encrypted) + const headers = { host: req.headers.host, connection: req.headers.connection } + res.end(JSON.stringify(headers)) + }) + + server.on('secureConnection', () => { + t.fail('server is http') + }) + + proxy.on('secureConnection', () => { + t.ok(true, 'TLS handshake to the proxy must still happen') + }) + + proxy.on('connect', () => { + t.fail('should not tunnel plain HTTP through an HTTPS proxy by default') + }) + + proxy.on('request', function (req) { + const bits = { method: req.method, url: req.url } + t.deepStrictEqual(bits, { + method: 'GET', + url: `${serverUrl}/` + }) + }) + + const data = await request(serverUrl, { dispatcher: proxyAgent }) + const json = await data.body.json() + t.deepStrictEqual(json, { + host: `localhost:${server.address().port}`, + connection: 'keep-alive' + }) + + server.close() + proxy.close() + proxyAgent.close() +}) + test('Proxy via HTTPS to HTTP endpoint with tunneling enabled', async (t) => { t = tspl(t, { plan: 3 }) const server = await buildServer() diff --git a/types/proxy-agent.d.ts b/types/proxy-agent.d.ts index 05c7e95dcbf..53fdb352922 100644 --- a/types/proxy-agent.d.ts +++ b/types/proxy-agent.d.ts @@ -24,6 +24,12 @@ declare namespace ProxyAgent { requestTls?: buildConnector.BuildOptions; proxyTls?: buildConnector.BuildOptions; clientFactory?(origin: URL, opts: object): Dispatcher; + /** + * Undici tunnels via CONNECT when the target endpoint uses HTTPS, and + * forwards (absolute-form request-target, per RFC 9112 §3.2.2) otherwise, + * regardless of whether the proxy URL is HTTP or HTTPS. Set to true to + * force tunneling for plain HTTP requests as well. + */ proxyTunnel?: boolean; } }