refactoring and cleanups

Author: mxschmitt

packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts

@@ -54,44 +54,22 @@ class ALPNCache {
     const result = new ManualPromise<string>();
     this._cache.set(cacheKey, result);
     result.then(success);
-    if (!proxySocket) {
-      createTLSSocket({
-        host,
-        port,
-        servername: net.isIP(host) ? undefined : host,
-        ALPNProtocols: ['h2', 'http/1.1'],
-        rejectUnauthorized: false,
-        secureContext
-      }).then(socket => {
-        // The server may not respond with ALPN, in which case we default to http/1.1.
-        result.resolve(socket.alpnProtocol || 'http/1.1');
-        socket.end();
-      }).catch(error => {
-        debugLogger.log('client-certificates', `ALPN error: ${error.message}`);
-        result.resolve('http/1.1');
-      });
-    } else {
-      const socket = tls.connect({
-        socket: proxySocket,
-        port: port,
-        host: host,
-        ALPNProtocols: ['h2', 'http/1.1'],
-        rejectUnauthorized: false,
-        secureContext: secureContext,
-        servername: net.isIP(host) ? undefined : host
-      });
-      socket.on('secureConnect', () => {
-        result.resolve(socket.alpnProtocol || 'http/1.1');
-        socket.end();
-      });
-      socket.on('error', error => {
-        debugLogger.log('client-certificates', `ALPN error: ${error.message}`);
-        result.resolve('http/1.1');
-      });
-      socket.on('timeout', () => {
-        result.resolve('http/1.1');
-      });
-    }
+    createTLSSocket({
+      socket: proxySocket,
+      host,
+      port,
+      servername: net.isIP(host) ? undefined : host,
+      ALPNProtocols: ['h2', 'http/1.1'],
+      rejectUnauthorized: false,
+      secureContext,
+    }).then(socket => {
+      // The server may not respond with ALPN, in which case we default to http/1.1.
+      result.resolve(socket.alpnProtocol || 'http/1.1');
+      socket.end();
+    }).catch(error => {
+      debugLogger.log('client-certificates', `ALPN error: ${error.message}`);
+      result.resolve('http/1.1');
+    });
   }
 }
 
@@ -123,11 +101,7 @@ class SocksProxyConnection {
   }
 
   async connect() {
-    if (this.socksProxy.proxyAgentFromOptions)
-      this.target = await this.socksProxy.proxyAgentFromOptions.callback(new EventEmitter() as any, { host: rewriteToLocalhostIfNeeded(this.host), port: this.port, secureEndpoint: false });
-    else
-      this.target = await createSocket(rewriteToLocalhostIfNeeded(this.host), this.port);
-
+    this.target = await this._createProxySocket() ?? await createSocket(rewriteToLocalhostIfNeeded(this.host), this.port);
     this.target.once('close', this._targetCloseEventListener);
     this.target.once('error', error => this.socksProxy._socksProxy.sendSocketError({ uid: this.uid, error: error.message }));
     if (this._closed) {
@@ -166,19 +140,21 @@ class SocksProxyConnection {
       this.target.write(data);
   }
 
+  private async _createProxySocket() {
+    if (this.socksProxy.proxyAgentFromOptions)
+      return await this.socksProxy.proxyAgentFromOptions.callback(new EventEmitter() as any, { host: rewriteToLocalhostIfNeeded(this.host), port: this.port, secureEndpoint: false });
+  }
+
   private async _attachTLSListeners() {
     this.internal = new stream.Duplex({
-      read: () => {},
+      read: () => { },
       write: (data, encoding, callback) => {
         this.socksProxy._socksProxy.sendSocketData({ uid: this.uid, data });
         callback();
       }
     });
     const secureContext = this.socksProxy.secureContextMap.get(new URL(`https://${this.host}:${this.port}`).origin);
-    let proxySocket: stream.Duplex | undefined = undefined;
-    if (this.socksProxy.proxyAgentFromOptions)
-      proxySocket = await this.socksProxy.proxyAgentFromOptions.callback(new EventEmitter() as any, { host: rewriteToLocalhostIfNeeded(this.host), port: this.port, secureEndpoint: false });
-
+    const proxySocket = await this._createProxySocket();
     this.socksProxy.alpnCache.get(rewriteToLocalhostIfNeeded(this.host), this.port, secureContext, proxySocket, alpnProtocolChosenByServer => {
       proxySocket?.destroy();
       debugLogger.log('client-certificates', `Proxy->Target ${this.host}:${this.port} chooses ALPN ${alpnProtocolChosenByServer}`);
@@ -251,7 +227,7 @@ class SocksProxyConnection {
           rejectUnauthorized: !this.socksProxy.ignoreHTTPSErrors,
           ALPNProtocols: [internalTLS.alpnProtocol || 'http/1.1'],
           servername: !net.isIP(this.host) ? this.host : undefined,
-          secureContext: secureContext,
+          secureContext,
         });
 
         targetTLS.once('secureConnect', () => {

tests/config/proxy.ts

@@ -133,16 +133,16 @@ export class TestProxy {
 }
 
 export async function setupSocksForwardingServer({
-  port, forwardPort, allowedTargetPort
+  port, forwardPort, allowedTargetPort, additionalAllowedHosts = []
 }: {
-  port: number, forwardPort: number, allowedTargetPort: number
+  port: number, forwardPort: number, allowedTargetPort: number, additionalAllowedHosts?: string[]
 }) {
   const connectHosts = [];
   const connections = new Map<string, net.Socket>();
   const socksProxy = new SocksProxy();
   socksProxy.setPattern('*');
   socksProxy.addListener(SocksProxy.Events.SocksRequested, async (payload: SocksSocketRequestedPayload) => {
-    if (!['127.0.0.1', 'fake-localhost-127-0-0-1.nip.io', 'localhost'].includes(payload.host) || payload.port !== allowedTargetPort) {
+    if (!['127.0.0.1', 'fake-localhost-127-0-0-1.nip.io', 'localhost', ...additionalAllowedHosts].includes(payload.host) || payload.port !== allowedTargetPort) {
       socksProxy.sendSocketError({ uid: payload.uid, error: 'ECONNREFUSED' });
       return;
     }

tests/library/client-certificates.spec.ts

@@ -24,6 +24,7 @@ import { expect, playwrightTest as base } from '../config/browserTest';
 import type net from 'net';
 import type { BrowserContextOptions } from 'packages/playwright-test';
 import { setupSocksForwardingServer } from '../config/proxy';
+import type { LookupAddress } from 'dns';
 const { createHttpsServer, createHttp2Server } = require('../../packages/playwright-core/lib/utils');
 
 type TestOptions = {
@@ -391,6 +392,55 @@ test.describe('browser', () => {
     await page.close();
   });
 
+  test('should pass with matching certificates and when a socks proxy is used on an otherwise unreachable server', async ({ browser, startCCServer, asset, browserName, isMac }) => {
+    const serverURL = await startCCServer({ useFakeLocalhost: browserName === 'webkit' && isMac });
+    const serverPort = parseInt(new URL(serverURL).port, 10);
+    const privateDomain = `private.playwright.test`;
+    const { proxyServerAddr, closeProxyServer, connectHosts } = await setupSocksForwardingServer({
+      port: test.info().workerIndex + 2048 + 2,
+      forwardPort: serverPort,
+      allowedTargetPort: serverPort,
+      additionalAllowedHosts: [privateDomain],
+    });
+
+    // make private domain resolve to unreachable server 192.0.2.0
+    // any attempt to connect will timeout
+    let interceptedHostnameLookup: string | undefined;
+    const __testHookLookup = (hostname: string): LookupAddress[] => {
+      if (hostname === privateDomain) {
+        interceptedHostnameLookup = hostname;
+        return [
+          { address: '192.0.2.0', family: 4 },
+        ];
+      }
+      return [];
+    };
+
+    const context = await browser.newContext({
+      ignoreHTTPSErrors: true,
+      clientCertificates: [{
+        origin: new URL(serverURL).origin.replace('127.0.0.1', privateDomain),
+        certPath: asset('client-certificates/client/trusted/cert.pem'),
+        keyPath: asset('client-certificates/client/trusted/key.pem'),
+      }],
+      proxy: { server: proxyServerAddr },
+      ...
+      { __testHookLookup } as any
+    });
+    const page = await context.newPage();
+    expect(connectHosts).toEqual([]);
+    const requestURL = serverURL.replace('127.0.0.1', privateDomain);
+    await page.goto(requestURL);
+
+    // only the proxy server should have tried to resolve the private domain
+    // and the test proxy server does not resolve domains
+    expect(interceptedHostnameLookup).toBe(undefined);
+    expect(connectHosts).toEqual([`${privateDomain}:${serverPort}`]);
+    await expect(page.getByTestId('message')).toHaveText('Hello Alice, your certificate was issued by localhost!');
+    await page.close();
+    await closeProxyServer();
+  });
+
   test('should pass with matching certificates and when a socks proxy is used', async ({ browser, startCCServer, asset, browserName, isMac }) => {
     const serverURL = await startCCServer({ useFakeLocalhost: browserName === 'webkit' && isMac });
     const serverPort = parseInt(new URL(serverURL).port, 10);
@@ -791,7 +841,7 @@ test.describe('browser', () => {
     const page = await context.newPage();
 
     // This was triggering an unhandled error before.
-    await page.goto(serverUrl).catch(() => {});
+    await page.goto(serverUrl).catch(() => { });
 
     await context.close();
     await new Promise<void>(resolve => server.close(() => resolve()));