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
PR-URL: #59894
Refs: #57872
Reviewed-By: Aditi Singh <aditisingh1400@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Author: joyeecheung

lib/https.js

@@ -68,7 +68,7 @@ let debug = require('internal/util/debuglog').debuglog('https', (fn) => {
 constnet=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');
 constassert=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.
-constrequestHost=isIPv6(reqOptions.host) ? `[${reqOptions.host}]` : reqOptions.host;
+constipType=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
+constrequestHost=ipType===6 ? `[${reqOptions.host}]` : reqOptions.host;
 constrequestPort=reqOptions.port||agent.defaultPort;
 constendpoint=`${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.
  */
 classProxyConfig{
 constructor(proxyUrl,keepAlive,noProxyList){
-const{ host, hostname, port, protocol, username, password }=newURL(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:'.
+letparsedURL;
+try{
+parsedURL=newURL(proxyUrl);
+}catch{
+thrownewERR_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';
+importassertfrom'node:assert';
+importhttpfrom'node:http';
+
+consttestCases=[
+{
+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(consttestCaseoftestCases){
+assert.throws(()=>{
+newhttp.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*ascommonfrom'../common/index.mjs';
+importassertfrom'node:assert';
+importhttpfrom'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.
+constserver=http.createServer(common.mustCall((req,res)=>{
+res.end('Hello world');
+}));
+server.on('error',common.mustNotCall((err)=>{console.error('Server error',err);}));
+server.listen(0);
+awaitonce(server,'listening');
+
+// Start a minimal proxy server.
+const{ proxy, logs }=createProxyServer();
+proxy.listen(0);
+awaitonce(proxy,'listening');
+
+{
+constserverHost=`[::1]:${server.address().port}`;
+constrequestUrl=`http://${serverHost}/test`;
+constexpectedLogs=[{
+method: 'GET',
+url: requestUrl,
+headers: {
+'connection': 'keep-alive',
+'proxy-connection': 'keep-alive',
+'host': serverHost,
+},
+}];
+
+const{ code, signal, stdout }=awaitrunProxiedRequest({
+NODE_USE_ENV_PROXY: 1,
+REQUEST_URL: requestUrl,
+HTTP_PROXY: `http://[::1]:${proxy.address().port}`,
+});
+assert.deepStrictEqual(logs,expectedLogs);
+assert.match(stdout,/Helloworld/);
+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*ascommonfrom'../common/index.mjs';
+importfixturesfrom'../common/fixtures.js';
+importassertfrom'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}=awaitimport('node:https');
+
+// Start a server to process the final request.
+constserver=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);
+awaitonce(server,'listening');
+
+// Start a minimal proxy server.
+const{ proxy, logs }=createProxyServer();
+proxy.listen(0);
+awaitonce(proxy,'listening');
+
+{
+constserverHost=`localhost:${server.address().port}`;
+constrequestUrl=`https://${serverHost}/test`;
+constexpectedLogs=[{
+method: 'CONNECT',
+url: serverHost,
+headers: {
+'proxy-connection': 'keep-alive',
+'host': serverHost,
+},
+}];
+
+const{ code, signal, stdout }=awaitrunProxiedRequest({
+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,/Helloworld/);
+assert.strictEqual(code,0);
+assert.strictEqual(signal,null);
+}
+
+// Test with IPv6 address in the request URL.
+{
+logs.splice(0,logs.length);// Clear the logs.
+constserverHost=`[::1]:${server.address().port}`;
+constrequestUrl=`https://${serverHost}/test`;
+constexpectedLogs=[{
+method: 'CONNECT',
+url: serverHost,
+headers: {
+'proxy-connection': 'keep-alive',
+'host': serverHost,
+},
+}];
+
+const{ code, signal, stdout }=awaitrunProxiedRequest({
+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,/Helloworld/);
+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}=newURL(`http://${req.headers.host}`);
 consttargetPort=port||80;
 
 consturl=newURL(req.url);
 constoptions={
-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}=newURL(`https://${req.url}`);
 
 res.on('error',(err)=>{
 logs.push({error: err,source: 'client response for connect'});
 });
 
-constproxyReq=net.connect(port,hostname,()=>{
+constnormalizedHostname=hostname.startsWith('[')&&hostname.endsWith(']') ?
+hostname.slice(1,-1) : hostname;
+constproxyReq=net.connect(port,normalizedHostname,()=>{
 res.write(
 'HTTP/1.1 200 Connection Established\r\n'+
 'Proxy-agent: Node.js-Proxy\r\n'+