http,https: handle IPv6 with proxies

This simplifies the proxy configuration handling code,
 adds tests to make sure the proxy support works with IPv6
and throws correct errors for invalid proxy IPs.
Drive-by: remove useless properties from ProxyConfig

Author: joyeecheung

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' +