Fix SOCKS handling of IPs with SNI passthrough & transforms

Previously, a SOCKS request to an IP (not DNS) resulted in that IP still
appearing in a few places, notably including transforms/callbacks &
upstream SNI when doing passthrough. This was a bug, as the effective
hostname (from Host header or inbound SNI) should be used instead, with
the IP only used for the actual TCP connection destination.

Author: pimterry

src/rules/passthrough-handling.ts

@@ -2,11 +2,13 @@ import { Buffer } from 'buffer';
 import * as fs from 'fs/promises';
 import * as tls from 'tls';
 import * as url from 'url';
+import type * as net from 'net';
 
 import * as _ from 'lodash';
 import { oneLine } from 'common-tags';
 import CacheableLookup from 'cacheable-lookup';
 import * as semver from 'semver';
+import { ErrorLike, unreachableCheck } from '@httptoolkit/util';
 
 import { CompletedBody, Headers, RawHeaders } from '../types';
 import { byteLength } from '../util/util';
@@ -15,8 +17,9 @@ import { isIP, isLocalhostAddress, normalizeIP } from '../util/ip-utils';
 import { CachedDns, dnsLookup, DnsLookupFunction } from '../util/dns';
 import { isMockttpBody, encodeBodyBuffer } from '../util/request-utils';
 import { areFFDHECurvesSupported } from '../util/openssl-compat';
-import { ErrorLike, unreachableCheck } from '@httptoolkit/util';
 import { findRawHeaderIndex, getHeaderValue } from '../util/header-utils';
+import { getDefaultPort } from '../util/url';
+import { TlsMetadata } from '../util/socket-extensions';
 
 import {
     CallbackRequestResult,
@@ -28,7 +31,6 @@ import {
     PassThroughInitialTransforms,
     PassThroughLookupOptions
 } from './passthrough-handling-definitions';
-import { getDefaultPort } from '../util/url';
 import { applyMatchReplace } from './match-replace';
 
 // TLS settings for proxied connections, intended to avoid TLS fingerprint blocking
@@ -41,7 +43,13 @@ const SSL_OP_NO_ENCRYPT_THEN_MAC = 1 << 19;
 
 // All settings are designed to exactly match Firefox v103, since that's a good baseline
 // that seems to be widely accepted and is easy to emulate from Node.js.
-export const getUpstreamTlsOptions = (strictChecks: boolean): tls.SecureContextOptions => ({
+export const getUpstreamTlsOptions = ({ strictHttpsChecks, serverName }: {
+    strictHttpsChecks: boolean,
+    serverName?: string
+}): tls.ConnectionOptions => ({
+    servername: serverName && !isIP(serverName)
+        ? serverName
+        : undefined, // Can't send IPs in SNI
     ecdhCurve: [
         'X25519',
         'prime256v1', // N.B. Equivalent to secp256r1
@@ -88,12 +96,12 @@ export const getUpstreamTlsOptions = (strictChecks: boolean): tls.SecureContextO
 
         // This magic cipher is the very obtuse way that OpenSSL downgrades the overall
         // security level to allow various legacy settings, protocols & ciphers:
-        ...(!strictChecks
+        ...(!strictHttpsChecks
             ? ['@SECLEVEL=0']
             : []
         )
     ].join(':'),
-    secureOptions: strictChecks
+    secureOptions: strictHttpsChecks
         ? SSL_OP_TLSEXT_PADDING | SSL_OP_NO_ENCRYPT_THEN_MAC
         : SSL_OP_TLSEXT_PADDING | SSL_OP_NO_ENCRYPT_THEN_MAC | SSL_OP_LEGACY_SERVER_CONNECT,
     ...({
@@ -107,10 +115,10 @@ export const getUpstreamTlsOptions = (strictChecks: boolean): tls.SecureContextO
     allowPartialTrustChain: semver.satisfies(process.version, '>=22.9.0'),
 
     // Allow TLSv1, if !strict:
-    minVersion: strictChecks ? tls.DEFAULT_MIN_VERSION : 'TLSv1',
+    minVersion: strictHttpsChecks ? tls.DEFAULT_MIN_VERSION : 'TLSv1',
 
     // Skip certificate validation entirely, if not strict:
-    rejectUnauthorized: strictChecks,
+    rejectUnauthorized: strictHttpsChecks,
 });
 
 export async function getTrustedCAs(
@@ -182,21 +190,23 @@ export async function buildOverriddenBody(
 }
 
 /**
- * Effectively match the slightly-different-context logic in MockttpServer for showing a
- * request's destination within the URL. We prioritise domain names over IPs, and
- * derive the most appropriate name available. In this case, we drop the port, since that's
- * always specified elsewhere.
+ * Effectively match the slightly-different-context logic in MockttpServer for generating a 'name'
+ * for a request's destination (e.g. in the URL). We prioritise domain names over IPs, and
+ * derive the most appropriate name available. In this method we consider only hostnames, so we
+ * drop the port, as that's always specified elsewhere.
  */
-export function getUrlHostname(
+export function getEffectiveHostname(
     destinationHostname: string | null,
+    socket: net.Socket,
     rawHeaders: RawHeaders
 ) {
     return destinationHostname && !isIP(destinationHostname)
         ? destinationHostname
         : ( // Use header info rather than raw IPs, if we can:
             getHeaderValue(rawHeaders, ':authority') ??
             getHeaderValue(rawHeaders, 'host') ??
-            destinationHostname ?? // Use destination if it's a bare IP, if we have nothing else
+            socket[TlsMetadata]?.sniHostname ??
+            destinationHostname ?? // Use bare IP destination if we have nothing else
             'localhost'
         ).replace(/:\d+$/, '');
 }
@@ -223,8 +233,8 @@ function deriveUrlLinkedHeader(
             // existing value, we accept it but print a warning. This would be easy to
             // do if you mutate the existing headers, for example, and ignore the host.
             console.warn(oneLine`
-                Passthrough callback overrode the URL and the ${headerName} header
-                with mismatched values, which may be a mistake. The URL implies
+                Passthrough callback set the URL and the ${headerName} header
+                to mismatched values, which may be a mistake. The URL implies
                 ${expectedValue}, whilst the header was set to ${replacementValue}.
             `);
         }

src/rules/requests/request-step-impls.ts

@@ -86,7 +86,7 @@ import {
     getDnsLookupFunction,
     getTrustedCAs,
     buildUpstreamErrorTags,
-    getUrlHostname,
+    getEffectiveHostname,
     applyDestinationTransforms
 } from '../passthrough-handling';
 
@@ -422,11 +422,17 @@ export class PassThroughStepImpl extends PassThroughStep {
         // Capture raw request data:
         let { method, url: reqUrl, rawHeaders, destination } = clientReq as OngoingRequest;
         let { protocol, pathname, search: query } = url.parse(reqUrl);
-        let hostname: string = destination.hostname;
+        const clientSocket = (clientReq as any).socket as net.Socket;
+
+        // Actual IP address or hostname
+        let hostAddress = destination.hostname;
+        // Same as hostAddress, unless it's an IP, in which case it's our best guess of the
+        // functional 'name' for the host (from Host header or SNI).
+        let hostname: string = getEffectiveHostname(hostAddress, clientSocket, rawHeaders);
         let port: string | null | undefined = destination.port.toString();
 
         // Check if this request is a request loop:
-        if (isSocketLoop(this.outgoingSockets, (clientReq as any).socket)) {
+        if (isSocketLoop(this.outgoingSockets, clientSocket)) {
             throw new Error(oneLine`
                 Passthrough loop detected. This probably means you're sending a request directly
                 to a passthrough endpoint, which is forwarding it to the target URL, which is a
@@ -444,8 +450,8 @@ export class PassThroughStepImpl extends PassThroughStep {
 
         const isH2Downstream = isHttp2(clientReq);
 
-        hostname = await getClientRelativeHostname(
-            hostname,
+        hostAddress = await getClientRelativeHostname(
+            hostAddress,
             clientReq.remoteIpAddress,
             getDnsLookupFunction(this.lookupOptions)
         );
@@ -466,6 +472,8 @@ export class PassThroughStepImpl extends PassThroughStep {
                 matchReplaceBody
             } = this.transformRequest;
 
+            const originalHostname = hostname;
+
             ({
                 reqUrl,
                 protocol,
@@ -484,6 +492,12 @@ export class PassThroughStepImpl extends PassThroughStep {
                  query
             }));
 
+            // If you modify the hostname, we also treat that as modifying the
+            // resulting destination in turn:
+            if (hostname !== originalHostname) {
+                hostAddress = hostname;
+            }
+
             if (replaceMethod) {
                 method = replaceMethod;
             }
@@ -579,8 +593,7 @@ export class PassThroughStepImpl extends PassThroughStep {
 
             if (modifiedReq?.response) {
                 if (modifiedReq.response === 'close') {
-                    const socket: net.Socket = (clientReq as any).socket;
-                    socket.end();
+                    clientSocket.end();
                     throw new AbortError('Connection closed intentionally by rule', 'E_RULE_BREQ_CLOSE');
                 } else if (modifiedReq.response === 'reset') {
                     requireSocketResetSupport();
@@ -594,18 +607,27 @@ export class PassThroughStepImpl extends PassThroughStep {
             }
 
             method = modifiedReq?.method || method;
-            reqUrl = modifiedReq?.url || reqUrl;
+
+            // Reparse the new URL, if necessary
+            if (modifiedReq?.url) {
+                if (!isAbsoluteUrl(modifiedReq?.url)) throw new Error("Overridden request URLs must be absolute");
+
+                reqUrl = modifiedReq.url;
+
+                const parsedUrl = url.parse(reqUrl);
+                ({ protocol, port, pathname, search: query } = parsedUrl);
+                hostname = parsedUrl.hostname!;
+                hostAddress = hostname;
+            }
 
             let headers = modifiedReq?.headers || clientHeaders;
 
             // We need to make sure the Host/:authority header is updated correctly - following the user's returned value if
             // they provided one, but updating it if not to match the effective target URL of the request:
-            const expectedTargetUrl = modifiedReq?.url ?? reqUrl;
-
             Object.assign(headers,
                 isH2Downstream
-                    ? getH2HeadersAfterModification(expectedTargetUrl, clientHeaders, modifiedReq?.headers)
-                    : { 'host': getHostAfterModification(expectedTargetUrl, clientHeaders, modifiedReq?.headers) }
+                    ? getH2HeadersAfterModification(reqUrl, clientHeaders, modifiedReq?.headers)
+                    : { 'host': getHostAfterModification(reqUrl, clientHeaders, modifiedReq?.headers) }
             );
 
             validateCustomHeaders(
@@ -630,14 +652,6 @@ export class PassThroughStepImpl extends PassThroughStep {
                 }
             }
 
-            // Reparse the new URL, if necessary
-            if (modifiedReq?.url) {
-                if (!isAbsoluteUrl(modifiedReq?.url)) throw new Error("Overridden request URLs must be absolute");
-                const parsedUrl = url.parse(reqUrl);
-                ({ protocol, port, pathname, search: query } = parsedUrl);
-                hostname = parsedUrl.hostname!;
-            }
-
             rawHeaders = objectHeadersToRaw(headers);
         }
 
@@ -726,7 +740,7 @@ export class PassThroughStepImpl extends PassThroughStep {
             serverReq = await makeRequest({
                 protocol,
                 method,
-                hostname,
+                hostname: hostAddress,
                 port,
                 family,
                 path: `${pathname || '/'}${query || ''}`,
@@ -739,7 +753,7 @@ export class PassThroughStepImpl extends PassThroughStep {
                 agent,
 
                 // TLS options:
-                ...getUpstreamTlsOptions(strictHttpsChecks),
+                ...getUpstreamTlsOptions({ strictHttpsChecks, serverName: hostname }),
                 ...clientCert,
                 ...caConfig
             }, (serverRes) => (async () => {
@@ -944,7 +958,7 @@ export class PassThroughStepImpl extends PassThroughStep {
                         }
 
                         if (modifiedRes === 'close') {
-                            (clientReq as any).socket.end();
+                            clientSocket.end();
                         } else if (modifiedRes === 'reset') {
                             requireSocketResetSupport();
                             resetOrDestroy(clientReq);
@@ -1151,12 +1165,10 @@ export class PassThroughStepImpl extends PassThroughStep {
             // Fire rule events, to allow in-depth debugging of upstream traffic & modifications,
             // so anybody interested can see _exactly_ what we're sending upstream here:
             if (options.emitEventCallback) {
-                const urlHost = getUrlHostname(hostname, rawHeaders);
-
                 options.emitEventCallback('passthrough-request-head', {
                     method,
                     protocol: protocol!.replace(/:$/, ''),
-                    hostname: urlHost,
+                    hostname,
                     port,
                     path: `${pathname || '/'}${query || ''}`,
                     rawHeaders