review feedback

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

@@ -16,7 +16,7 @@
 
 import type * as types from './types';
 import type * as channels from '@protocol/channels';
-import { BrowserContext, validateBrowserContextOptions } from './browserContext';
+import { BrowserContext, createClientCertificatesProxyIfNeeded, validateBrowserContextOptions } from './browserContext';
 import { Page } from './page';
 import { Download } from './download';
 import type { ProxySettings } from './types';
@@ -25,7 +25,6 @@ import type { RecentLogsCollector } from '../utils/debugLogger';
 import type { CallMetadata } from './instrumentation';
 import { SdkObject } from './instrumentation';
 import { Artifact } from './artifact';
-import { ClientCertificatesProxy } from './socksClientCertificatesInterceptor';
 
 export interface BrowserProcess {
   onclose?: ((exitCode: number | null, signal: string | null) => void);
@@ -85,12 +84,7 @@ export abstract class Browser extends SdkObject {
 
   async newContext(metadata: CallMetadata, options: channels.BrowserNewContextParams): Promise<BrowserContext> {
     validateBrowserContextOptions(options, this.options);
-    let clientCertificatesProxy: ClientCertificatesProxy | undefined;
-    if (options.clientCertificates?.length) {
-      clientCertificatesProxy = new ClientCertificatesProxy(options);
-      options.proxy = { server: await clientCertificatesProxy.listen() };
-      options.ignoreHTTPSErrors = true;
-    }
+    const clientCertificatesProxy = await createClientCertificatesProxyIfNeeded(options);
     const context = await this.doCreateNewContext(options);
     context._clientCertificatesProxy = clientCertificatesProxy;
     if (options.storageState)

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 { type ClientCertificatesProxy } from './socksClientCertificatesInterceptor';
+import { ClientCertificatesProxy } from './socksClientCertificatesInterceptor';
 
 export abstract class BrowserContext extends SdkObject {
   static Events = {
@@ -247,6 +247,7 @@ export abstract class BrowserContext extends SdkObject {
       // at the same time.
       return;
     }
+    this._clientCertificatesProxy?.close().catch(() => {});
     this.tracing.abort();
     if (this._isPersistentContext)
       this.onClosePersistent();
@@ -449,8 +450,6 @@ export abstract class BrowserContext extends SdkObject {
         await harRecorder.flush();
       await this.tracing.flush();
 
-      await this._clientCertificatesProxy?.close();
-
       // Cleanup.
       const promises: Promise<void>[] = [];
       for (const { context, artifact } of this._browser._idToVideo.values()) {
@@ -655,6 +654,17 @@ export function assertBrowserContextIsNotOwned(context: BrowserContext) {
   }
 }
 
+export async function createClientCertificatesProxyIfNeeded(options: channels.BrowserNewContextOptions) {
+  if (!options.clientCertificates?.length)
+    return;
+  if (options.proxy?.server)
+    throw new Error('Cannot specify both proxy and clientCertificates');
+  const clientCertificatesProxy = new ClientCertificatesProxy(options);
+  options.proxy = { server: await clientCertificatesProxy.listen() };
+  options.ignoreHTTPSErrors = true;
+  return clientCertificatesProxy;
+}
+
 export function validateBrowserContextOptions(options: channels.BrowserNewContextParams, browserOptions: BrowserOptions) {
   if (options.noDefaultViewport && options.deviceScaleFactor !== undefined)
     throw new Error(`"deviceScaleFactor" option is not supported with null "viewport"`);
@@ -693,6 +703,8 @@ export function validateBrowserContextOptions(options: channels.BrowserNewContex
       throw new Error('Cannot specify both proxy and clientCertificates');
     options.proxy = normalizeProxySettings(options.proxy);
   }
+  if (!browserOptions.persistent && browserOptions.proxy?.server && options.clientCertificates)
+    throw new Error('Cannot specify both proxy and clientCertificates');
   verifyGeolocation(options.geolocation);
   verifyClientCertificates(options.clientCertificates);
 }
@@ -725,7 +737,7 @@ export function verifyClientCertificates(clientCertificates?: channels.BrowserNe
         throw new Error('cert is specified without key');
       if (!cert.cert && cert.key)
         throw new Error('key is specified without cert');
-      if (cert.pfx && (cert.cert || cert.key || cert.passphrase))
+      if (cert.pfx && (cert.cert || cert.key))
         throw new Error('pfx is specified together with cert, key or passphrase');
     }
   }

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

@@ -18,7 +18,7 @@ import fs from 'fs';
 import * as os from 'os';
 import path from 'path';
 import type { BrowserContext } from './browserContext';
-import { normalizeProxySettings, validateBrowserContextOptions } from './browserContext';
+import { createClientCertificatesProxyIfNeeded, normalizeProxySettings, validateBrowserContextOptions } from './browserContext';
 import type { BrowserName } from './registry';
 import { registry } from './registry';
 import type { ConnectionTransport } from './transport';
@@ -40,7 +40,6 @@ import type { CallMetadata } from './instrumentation';
 import { SdkObject } from './instrumentation';
 import { ManualPromise } from '../utils/manualPromise';
 import { type ProtocolError, isProtocolError } from './protocolError';
-import { ClientCertificatesProxy } from './socksClientCertificatesInterceptor';
 
 export const kNoXServerRunningError = 'Looks like you launched a headed browser without having a XServer running.\n' +
   'Set either \'headless: true\' or use \'xvfb-run <your-playwright-app>\' before running Playwright.\n\n<3 Playwright Team';
@@ -102,14 +101,7 @@ export abstract class BrowserType extends SdkObject {
 
   async _innerLaunch(progress: Progress, options: types.LaunchOptions, persistent: channels.BrowserNewContextParams | undefined, protocolLogger: types.ProtocolLogger, maybeUserDataDir?: string): Promise<Browser> {
     options.proxy = options.proxy ? normalizeProxySettings(options.proxy) : undefined;
-    let clientCertificatesProxy: ClientCertificatesProxy | undefined;
-    if (persistent?.clientCertificates?.length) {
-      if (persistent.proxy?.server)
-        throw new Error('Cannot specify both proxy and clientCertificates');
-      clientCertificatesProxy = new ClientCertificatesProxy(persistent);
-      options.proxy = { server: await clientCertificatesProxy.listen() };
-      persistent.ignoreHTTPSErrors = true;
-    }
+    const clientCertificatesProxy = await createClientCertificatesProxyIfNeeded(persistent ?? {});
     const browserLogsCollector = new RecentLogsCollector();
     const { browserProcess, userDataDir, artifactsDir, transport } = await this._launchProcess(progress, options, !!persistent, browserLogsCollector, maybeUserDataDir);
     if ((options as any).__testHookBeforeCreateBrowser)

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

@@ -193,10 +193,11 @@ export abstract class APIRequestContext extends SdkObject {
       maxRedirects: params.maxRedirects === 0 ? -1 : params.maxRedirects === undefined ? 20 : params.maxRedirects,
       timeout,
       deadline,
-      ...(process.env.PW_DO_NOT_USE_EXTRA_CA_CERTS ? { ca: [fs.readFileSync(process.env.PW_DO_NOT_USE_EXTRA_CA_CERTS)] } : {}),
       ...clientCertificatesToTLSOptions(this._defaultOptions().clientCertificates, requestUrl.toString()),
       __testHookLookup: (params as any).__testHookLookup,
     };
+    if (process.env.PWTEST_UNSUPPORTED_CUSTOM_CA)
+      options.ca = [fs.readFileSync(process.env.PWTEST_UNSUPPORTED_CUSTOM_CA)];
     // rejectUnauthorized = undefined is treated as true in Node.js 12.
     if (params.ignoreHTTPSErrors || defaults.ignoreHTTPSErrors)
       options.rejectUnauthorized = false;

tests/library/client-certificates.spec.ts

@@ -43,7 +43,7 @@ const test = base.extend<{ serverURL: string, serverURLRewrittenToLocalhost: str
         res.end(`Sorry, but you need to provide a client certificate to continue.`);
       }
     });
-    process.env.PW_DO_NOT_USE_EXTRA_CA_CERTS = asset('client-certificates/server/server_cert.pem');
+    process.env.PWTEST_UNSUPPORTED_CUSTOM_CA = asset('client-certificates/server/server_cert.pem');
     await new Promise<void>(f => server.listen(0, 'localhost', () => f()));
     await use(`https://localhost:${(server.address() as net.AddressInfo).port}/`);
     await new Promise<void>(resolve => server.close(() => resolve()));
@@ -80,8 +80,6 @@ const kValidationSubTests: [BrowserContextOptions, string][] = [
       certs: [{
         certPath: kDummyFileName,
         keyPath: kDummyFileName,
-        pfxPath: kDummyFileName,
-        passphrase: kDummyFileName,
       }]
     }]
   }, 'Cannot specify both proxy and clientCertificates'],