http,https: handle IPv6 with proxies

lib/https.js

@@ -68,7 +68,7 @@ let debug = require('internal/util/debuglog').debuglog('https', (fn) => {
 const net = require('net');
 const { URL, urlToHttpOptions, isURL } = require('internal/url');
 const { validateObject } = require('internal/validators');
-const { isIP, isIPv6 } = require('internal/net');
+const { isIP } = require('internal/net');
 const assert = require('internal/assert');
 const { getOptionValue } = require('internal/options');
 
@@ -171,7 +171,11 @@ function getTunnelConfigForProxiedHttps(agent, reqOptions) {
   }
   const { auth, href } = agent[kProxyConfig];
   // The request is a HTTPS request, assemble the payload for establishing the tunnel.
-  const requestHost = isIPv6(reqOptions.host) ? `[${reqOptions.host}]` : reqOptions.host;
+  const ipType = isIP(reqOptions.host);
+  // The request target must put IPv6 address in square brackets.
+  // Here reqOptions is already processed by urlToHttpOptions so we'll add them back if necessary.
+  // See https://www.rfc-editor.org/rfc/rfc3986#section-3.2.2
+  const requestHost = ipType === 6 ? `[${reqOptions.host}]` : reqOptions.host;
   const requestPort = reqOptions.port || agent.defaultPort;
   const endpoint = `${requestHost}:${requestPort}`;
   // The ClientRequest constructor should already have validated the host and the port.
@@ -198,7 +202,7 @@ function getTunnelConfigForProxiedHttps(agent, reqOptions) {
     proxyTunnelPayload: payload,
     requestOptions: {  // Options used for the request sent after the tunnel is established.
       __proto__: null,
-      servername: reqOptions.servername || (isIP(reqOptions.host) ? undefined : reqOptions.host),
+      servername: reqOptions.servername || ipType ? undefined : reqOptions.host,
       ...reqOptions,
     },
   };

lib/internal/http.js

@@ -2,6 +2,7 @@
 
 const {
   Date,
+  Number,
   NumberParseInt,
   Symbol,
   decodeURIComponent,
@@ -99,24 +100,23 @@ function ipToInt(ip) {
  * Represents the proxy configuration for an agent. The built-in http and https agent
  * implementation have one of this when they are configured to use a proxy.
  * @property {string} href - Full URL of the proxy server.
- * @property {string} host - Full host including port, e.g. 'localhost:8080'.
- * @property {string} hostname - Hostname without brackets for IPv6 addresses.
- * @property {number} port - Port number of the proxy server.
- * @property {string} protocol - Protocol of the proxy server, e.g. 'http:' or 'https:'.
+ * @property {string} protocol - Proxy protocol used to talk to the proxy server.
  * @property {string|undefined} auth - proxy-authorization header value, if username or password is provided.
  * @property {Array<string>} bypassList - List of hosts to bypass the proxy.
  * @property {object} proxyConnectionOptions - Options for connecting to the proxy server.
  */
 class ProxyConfig {
   constructor(proxyUrl, keepAlive, noProxyList) {
-    const { host, hostname, port, protocol, username, password } = new URL(proxyUrl);
-    this.href = proxyUrl; // Full URL of the proxy server.
-    this.host = host; // Full host including port, e.g. 'localhost:8080'.
-    // Trim off the brackets from IPv6 addresses. As it's parsed from a valid URL, an opening
-    // "[" Must already have a matching "]" at the end.
-    this.hostname = hostname[0] === '[' ? hostname.slice(1, -1) : hostname;
-    this.port = port ? NumberParseInt(port, 10) : (protocol === 'https:' ? 443 : 80);
-    this.protocol = protocol; // Protocol of the proxy server, e.g. 'http:' or 'https:'.
+    let parsedURL;
+    try {
+      parsedURL = new URL(proxyUrl);
+    } catch {
+      throw new ERR_PROXY_INVALID_CONFIG(`Invalid proxy URL: ${proxyUrl}`);
+    }
+    const { hostname, port, protocol, username, password } = parsedURL;
+
+    this.href = proxyUrl;
+    this.protocol = protocol;
 
     if (username || password) {
       // If username or password is provided, prepare the proxy-authorization header.
@@ -128,9 +128,13 @@ class ProxyConfig {
     } else {
       this.bypassList = []; // No bypass list provided.
     }
+
     this.proxyConnectionOptions = {
-      host: this.hostname,
-      port: this.port,
+      // The host name comes from parsed URL so if it starts with '[' it must be an IPv6 address
+      // ending with ']'. Remove the brackets for net.connect().
+      host: hostname[0] === '[' ? hostname.slice(1, -1) : hostname,
+      // The port comes from parsed URL so it is either '' or a valid number string.
+      port: port ? Number(port) : (protocol === 'https:' ? 443 : 80),
     };
   }
 

test/client-proxy/test-http-proxy-request-invalid-proxy.mjs

@@ -0,0 +1,36 @@
+// This tests that constructing agents with invalid proxy URLs throws ERR_PROXY_INVALID_CONFIG.
+import '../common/index.mjs';
+import assert from 'node:assert';
+import http from 'node:http';
+
+const testCases = [
+  {
+    name: 'invalid IPv4 address',
+    proxyUrl: 'http://256.256.256.256:8080',
+  },
+  {
+    name: 'invalid IPv6 address',
+    proxyUrl: 'http://::1:8080',
+  },
+  {
+    name: 'missing host',
+    proxyUrl: 'http://:8080',
+  },
+  {
+    name: 'non-numeric port',
+    proxyUrl: 'http://proxy.example.com:port',
+  },
+];
+
+for (const testCase of testCases) {
+  assert.throws(() => {
+    new http.Agent({
+      proxyEnv: {
+        HTTP_PROXY: testCase.proxyUrl,
+      },
+    });
+  }, {
+    code: 'ERR_PROXY_INVALID_CONFIG',
+    message: `Invalid proxy URL: ${testCase.proxyUrl}`,
+  });
+}

test/client-proxy/test-http-proxy-request-ipv6.mjs

@@ -0,0 +1,51 @@
+// This tests making HTTP requests through an HTTP proxy using IPv6 addresses.
+
+import * as common from '../common/index.mjs';
+import assert from 'node:assert';
+import http from 'node:http';
+import { once } from 'events';
+import { createProxyServer, runProxiedRequest } from '../common/proxy-server.js';
+
+if (!common.hasIPv6) {
+  common.skip('missing IPv6 support');
+}
+
+// Start a server to process the final request.
+const server = http.createServer(common.mustCall((req, res) => {
+  res.end('Hello world');
+}));
+server.on('error', common.mustNotCall((err) => { console.error('Server error', err); }));
+server.listen(0);
+await once(server, 'listening');
+
+// Start a minimal proxy server.
+const { proxy, logs } = createProxyServer();
+proxy.listen(0);
+await once(proxy, 'listening');
+
+{
+  const serverHost = `[::1]:${server.address().port}`;
+  const requestUrl = `http://${serverHost}/test`;
+  const expectedLogs = [{
+    method: 'GET',
+    url: requestUrl,
+    headers: {
+      'connection': 'keep-alive',
+      'proxy-connection': 'keep-alive',
+      'host': serverHost,
+    },
+  }];
+
+  const { code, signal, stdout } = await runProxiedRequest({
+    NODE_USE_ENV_PROXY: 1,
+    REQUEST_URL: requestUrl,
+    HTTP_PROXY: `http://[::1]:${proxy.address().port}`,
+  });
+  assert.deepStrictEqual(logs, expectedLogs);
+  assert.match(stdout, /Hello world/);
+  assert.strictEqual(code, 0);
+  assert.strictEqual(signal, null);
+}
+
+proxy.close();
+server.close();

test/client-proxy/test-https-proxy-request-ipv6.mjs

@@ -0,0 +1,90 @@
+// This tests making HTTPS requests through an HTTP proxy using IPv6 addresses.
+
+import * as common from '../common/index.mjs';
+import fixtures from '../common/fixtures.js';
+import assert from 'node:assert';
+import { once } from 'events';
+import { createProxyServer, runProxiedRequest } from '../common/proxy-server.js';
+
+if (!common.hasIPv6) {
+  common.skip('missing IPv6 support');
+}
+
+if (!common.hasCrypto) {
+  common.skip('missing crypto');
+}
+
+// https must be dynamically imported so that builds without crypto support
+// can skip it.
+const { default: https } = await import('node:https');
+
+// Start a server to process the final request.
+const server = https.createServer({
+  cert: fixtures.readKey('agent8-cert.pem'),
+  key: fixtures.readKey('agent8-key.pem'),
+}, common.mustCall((req, res) => {
+  res.end('Hello world');
+}, 2));
+server.on('error', common.mustNotCall((err) => { console.error('Server error', err); }));
+server.listen(0);
+await once(server, 'listening');
+
+// Start a minimal proxy server.
+const { proxy, logs } = createProxyServer();
+proxy.listen(0);
+await once(proxy, 'listening');
+
+{
+  const serverHost = `localhost:${server.address().port}`;
+  const requestUrl = `https://${serverHost}/test`;
+  const expectedLogs = [{
+    method: 'CONNECT',
+    url: serverHost,
+    headers: {
+      'proxy-connection': 'keep-alive',
+      'host': serverHost,
+    },
+  }];
+
+  const { code, signal, stdout } = await runProxiedRequest({
+    NODE_USE_ENV_PROXY: 1,
+    REQUEST_URL: requestUrl,
+    HTTPS_PROXY: `http://[::1]:${proxy.address().port}`,
+    NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'fake-startcom-root-cert.pem'),
+  });
+  assert.deepStrictEqual(logs, expectedLogs);
+  assert.match(stdout, /Hello world/);
+  assert.strictEqual(code, 0);
+  assert.strictEqual(signal, null);
+}
+
+// Test with IPv6 address in the request URL.
+{
+  logs.splice(0, logs.length); // Clear the logs.
+  const serverHost = `[::1]:${server.address().port}`;
+  const requestUrl = `https://${serverHost}/test`;
+  const expectedLogs = [{
+    method: 'CONNECT',
+    url: serverHost,
+    headers: {
+      'proxy-connection': 'keep-alive',
+      'host': serverHost,
+    },
+  }];
+
+  const { code, signal, stdout } = await runProxiedRequest({
+    NODE_USE_ENV_PROXY: 1,
+    REQUEST_URL: requestUrl,
+    HTTPS_PROXY: `http://[::1]:${proxy.address().port}`,
+    // Disable certificate verification for this request, for we don't have
+    // a certificate for [::1].
+    NODE_TLS_REJECT_UNAUTHORIZED: '0',
+  });
+  assert.deepStrictEqual(logs, expectedLogs);
+  assert.match(stdout, /Hello world/);
+  assert.strictEqual(code, 0);
+  assert.strictEqual(signal, null);
+}
+
+proxy.close();
+server.close();

test/common/proxy-server.js

@@ -32,12 +32,12 @@ exports.createProxyServer = function(options = {}) {
   }
   proxy.on('request', (req, res) => {
     logRequest(logs, req);
-    const [hostname, port] = req.headers.host.split(':');
+    const { hostname, port } = new URL(`http://${req.headers.host}`);
     const targetPort = port || 80;
 
     const url = new URL(req.url);
     const options = {
-      hostname: hostname,
+      hostname: hostname.startsWith('[') ? hostname.slice(1, -1) : hostname,
       port: targetPort,
       path: url.pathname + url.search,  // Convert back to relative URL.
       method: req.method,
@@ -72,13 +72,15 @@ exports.createProxyServer = function(options = {}) {
   proxy.on('connect', (req, res, head) => {
     logRequest(logs, req);
 
-    const [hostname, port] = req.url.split(':');
+    const { hostname, port } = new URL(`https://${req.url}`);
 
     res.on('error', (err) => {
       logs.push({ error: err, source: 'proxy response' });
     });
 
-    const proxyReq = net.connect(port, hostname, () => {
+    const normalizedHostname = hostname.startsWith('[') && hostname.endsWith(']') ?
+      hostname.slice(1, -1) : hostname;
+    const proxyReq = net.connect(port, normalizedHostname, () => {
       res.write(
         'HTTP/1.1 200 Connection Established\r\n' +
         'Proxy-agent: Node.js-Proxy\r\n' +