review feedback

packages/playwright-core/bin/socks-certs/README.md

@@ -0,0 +1,12 @@
+# Certfificates for Socks Proxy
+
+These certificates are used when client certificates are used with
+Playwright. Playwright then creates a Socks proxy, which sits between
+the browser and the actual target server. The Socks proxy then does the
+TLS upgrade using the certficiates in this directory (locally) while
+maintaining the client certificates to the target server.
+The certificates are generated via:
+
+```bash
+openssl req -new -newkey rsa:2048 -days 3650 -nodes -x509 -keyout key.pem -out cert.pem -subj "/CN=localhost"
+```

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

@@ -25,7 +25,7 @@ import type { RecentLogsCollector } from '../utils/debugLogger';
 import type { CallMetadata } from './instrumentation';
 import { SdkObject } from './instrumentation';
 import { Artifact } from './artifact';
-import { ClientCertificatesProxy, shouldUseMitmSocksProxy } from './socksClientCertificatesInterceptor';
+import { ClientCertificatesProxy } from './socksClientCertificatesInterceptor';
 
 export interface BrowserProcess {
   onclose?: ((exitCode: number | null, signal: string | null) => void);
@@ -85,13 +85,13 @@ export abstract class Browser extends SdkObject {
 
   async newContext(metadata: CallMetadata, options: channels.BrowserNewContextParams): Promise<BrowserContext> {
     validateBrowserContextOptions(options, this.options);
-    let socksServer: ClientCertificatesProxy | undefined;
+    let clientCertificatesProxy: ClientCertificatesProxy | undefined;
-    if (shouldUseMitmSocksProxy(options)) {
+    if (options.clientCertificates?.length) {
-      socksServer = new ClientCertificatesProxy(options);
+      clientCertificatesProxy = new ClientCertificatesProxy(options);
-      options.proxy = { server: await socksServer.listen() };
+      options.proxy = { server: await clientCertificatesProxy.listen() };
     }
     const context = await this.doCreateNewContext(options);
-    context._socksServer = socksServer;
+    context._clientCertificatesProxy = clientCertificatesProxy;
     if (options.storageState)
       await context.setStorageState(metadata, options.storageState);
     return context;

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

@@ -43,7 +43,7 @@ import * as consoleApiSource from '../generated/consoleApiSource';
 import { BrowserContextAPIRequestContext } from './fetch';
 import type { Artifact } from './artifact';
 import { Clock } from './clock';
-import { shouldUseMitmSocksProxy, type ClientCertificatesProxy } from './socksClientCertificatesInterceptor';
+import { type ClientCertificatesProxy } from './socksClientCertificatesInterceptor';
 
 export abstract class BrowserContext extends SdkObject {
   static Events = {
@@ -91,7 +91,7 @@ export abstract class BrowserContext extends SdkObject {
   private _debugger!: Debugger;
   _closeReason: string | undefined;
   readonly clock: Clock;
-  _socksServer: ClientCertificatesProxy | undefined;
+  _clientCertificatesProxy: ClientCertificatesProxy | undefined;
 
   constructor(browser: Browser, options: channels.BrowserNewContextParams, browserContextId: string | undefined) {
     super(browser, 'browser-context');
@@ -449,7 +449,7 @@ export abstract class BrowserContext extends SdkObject {
         await harRecorder.flush();
       await this.tracing.flush();
 
-      await this._socksServer?.close();
+      await this._clientCertificatesProxy?.close();
 
       // Cleanup.
       const promises: Promise<void>[] = [];
@@ -691,7 +691,7 @@ export function validateBrowserContextOptions(options: channels.BrowserNewContex
       throw new Error(`Browser needs to be launched with the global proxy. If all contexts override the proxy, global proxy will be never used and can be any string, for example "launch({ proxy: { server: 'http://per-context' } })"`);
     options.proxy = normalizeProxySettings(options.proxy);
   }
-  if (shouldUseMitmSocksProxy(options))
+  if (options.clientCertificates?.length)
     options.ignoreHTTPSErrors = true;
   verifyGeolocation(options.geolocation);
 }

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

@@ -21,7 +21,7 @@ import fs from 'fs';
 import tls from 'tls';
 import stream from 'stream';
 import { createSocket } from '../utils/happy-eyeballs';
-import { assert, globToRegex, isUnderTest } from '../utils';
+import { globToRegex, isUnderTest } from '../utils';
 import type { SocksSocketClosedPayload, SocksSocketDataPayload, SocksSocketRequestedPayload } from '../common/socksProxy';
 import { SocksProxy } from '../common/socksProxy';
 import type * as channels from '@protocol/channels';
@@ -38,20 +38,24 @@ class SocksConnectionDuplex extends stream.Duplex {
 }
 
 class SocksProxyConnection {
+  private readonly socksProxy: ClientCertificatesProxy;
+  private readonly uid: string;
+  private readonly host: string;
+  private readonly port: number;
   firstPackageReceived: boolean = false;
-  isTLS: boolean = false;
-
   target!: net.Socket;
-
   // In case of http, we just pipe data to the target socket and they are |undefined|.
   internal: stream.Duplex | undefined;
   internalTLS: tls.TLSSocket | undefined;
 
-  constructor(private readonly socksProxy: ClientCertificatesProxy, private readonly uid: string, private readonly host: string, private readonly port: number) {}
+  constructor(socksProxy: ClientCertificatesProxy, uid: string, host: string, port: number) {
+    this.socksProxy = socksProxy;
+    this.uid = uid;
+    this.host = host;
+    this.port = port;
+  }
 
   async connect() {
-    // WebKit on Darwin doesn't proxy localhost requests through the given proxy.
-    // Workaround: Rewrite to localhost during tests.
     this.target = await createSocket(isUnderTest() ? 'localhost' : this.host, this.port);
     this.target.on('close', () => this.socksProxy._socksProxy.sendSocketEnd({ uid: this.uid }));
     this.target.on('error', error => this.socksProxy._socksProxy.sendSocketError({ uid: this.uid, error: error.message }));
@@ -73,21 +77,18 @@ class SocksProxyConnection {
     if (!this.firstPackageReceived) {
       this.firstPackageReceived = true;
       // 0x16 is SSLv3/TLS "handshake" content type: https://en.wikipedia.org/wiki/Transport_Layer_Security#TLS_record
-      this.isTLS = data[0] === 0x16;
+      if (data[0] === 0x16)
-      if (this.isTLS)
         this._attachTLSListeners();
       else
         this.target.on('data', data => this.socksProxy._socksProxy.sendSocketData({ uid: this.uid, data }));
     }
-    if (this.isTLS)
+    if (this.internal)
-      this.internal!.push(data);
+      this.internal.push(data);
     else
       this.target.write(data);
   }
 
   private _attachTLSListeners() {
-    assert(this.isTLS);
-
     this.internal = new SocksConnectionDuplex(data => this.socksProxy._socksProxy.sendSocketData({ uid: this.uid, data }));
     this.internalTLS = new tls.TLSSocket(this.internal, {
       isServer: true,
@@ -160,7 +161,8 @@ export class ClientCertificatesProxy {
   }
 
   public async listen(): Promise<string> {
-    return `socks5://127.0.0.1:${await this._socksProxy.listen(0)}`;
+    const port = await this._socksProxy.listen(0, '127.0.0.1');
+    return `socks5://127.0.0.1:${port}`;
   }
 
   public async close() {
@@ -193,20 +195,12 @@ export function clientCertificatesToTLSOptions(
   for (const { certs } of matchingCerts) {
     for (const cert of certs) {
       if (cert.cert)
-        requestOptions.cert.push(cert.cert);
+        tlsOptions.cert.push(cert.cert);
       if (cert.key)
-        requestOptions.key.push({ pem: cert.key, passphrase: cert.passphrase });
+        tlsOptions.key.push({ pem: cert.key, passphrase: cert.passphrase });
       if (cert.pfx)
-        requestOptions.pfx.push({ buf: cert.pfx, passphrase: cert.passphrase });
+        tlsOptions.pfx.push({ buf: cert.pfx, passphrase: cert.passphrase });
     }
   }
-  return requestOptions;
+  return tlsOptions;
-}
-
-export function shouldUseMitmSocksProxy(options: {
-  clientCertificates?: channels.BrowserNewContextOptions['clientCertificates'];
-}) {
-  if (options.clientCertificates && options.clientCertificates.length > 0)
-    return true;
-  return false;
 }