From eeef44455face28c3896678cbf3318eccd348c65 Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Fri, 12 Jul 2024 10:17:58 +0200 Subject: [PATCH] feedback --- .../playwright-core/src/server/browser.ts | 2 +- .../src/server/browserContext.ts | 8 +--- .../playwright-core/src/server/browserType.ts | 32 ++++++++++--- packages/playwright-core/types/types.d.ts | 48 +++++++++---------- packages/playwright/types/test.d.ts | 12 ++--- 5 files changed, 59 insertions(+), 43 deletions(-) diff --git a/packages/playwright-core/src/server/browser.ts b/packages/playwright-core/src/server/browser.ts index 1b77c4b49f..5f7b85fa13 100644 --- a/packages/playwright-core/src/server/browser.ts +++ b/packages/playwright-core/src/server/browser.ts @@ -84,7 +84,7 @@ export abstract class Browser extends SdkObject { async newContext(metadata: CallMetadata, options: channels.BrowserNewContextParams): Promise { validateBrowserContextOptions(options, this.options); - const clientCertificatesProxy = await createClientCertificatesProxyIfNeeded(options); + const clientCertificatesProxy = await createClientCertificatesProxyIfNeeded(options, this.options); const context = await this.doCreateNewContext(options); context._clientCertificatesProxy = clientCertificatesProxy; if (options.storageState) diff --git a/packages/playwright-core/src/server/browserContext.ts b/packages/playwright-core/src/server/browserContext.ts index f4090ca9b8..49283b7aa9 100644 --- a/packages/playwright-core/src/server/browserContext.ts +++ b/packages/playwright-core/src/server/browserContext.ts @@ -654,10 +654,10 @@ export function assertBrowserContextIsNotOwned(context: BrowserContext) { } } -export async function createClientCertificatesProxyIfNeeded(options: channels.BrowserNewContextOptions) { +export async function createClientCertificatesProxyIfNeeded(options: channels.BrowserNewContextOptions, browserOptions?: BrowserOptions) { if (!options.clientCertificates?.length) return; - if (options.proxy?.server) + if (options.proxy?.server || browserOptions?.proxy?.server) throw new Error('Cannot specify both proxy and clientCertificates'); const clientCertificatesProxy = new ClientCertificatesProxy(options); options.proxy = { server: await clientCertificatesProxy.listen() }; @@ -699,12 +699,8 @@ export function validateBrowserContextOptions(options: channels.BrowserNewContex if (options.proxy) { if (!browserOptions.proxy && browserOptions.isChromium && os.platform() === 'win32') 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' } })"`); - if (!browserOptions.persistent && options.clientCertificates) - 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); } diff --git a/packages/playwright-core/src/server/browserType.ts b/packages/playwright-core/src/server/browserType.ts index 413f212831..d0258358f9 100644 --- a/packages/playwright-core/src/server/browserType.ts +++ b/packages/playwright-core/src/server/browserType.ts @@ -40,6 +40,7 @@ import type { CallMetadata } from './instrumentation'; import { SdkObject } from './instrumentation'; import { ManualPromise } from '../utils/manualPromise'; import { type ProtocolError, isProtocolError } from './protocolError'; +import type { 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 \' before running Playwright.\n\n<3 Playwright Team'; @@ -77,7 +78,7 @@ export abstract class BrowserType extends SdkObject { async launchPersistentContext(metadata: CallMetadata, userDataDir: string, options: channels.BrowserTypeLaunchPersistentContextOptions & { useWebSocket?: boolean }): Promise { options = this._validateLaunchOptions(options); const controller = new ProgressController(metadata, this); - const persistent: channels.BrowserNewContextParams = options; + const persistent: channels.BrowserNewContextParams = { ...options }; controller.setLogName('browser'); const browser = await controller.run(progress => { return this._innerLaunchWithRetries(progress, options, persistent, helper.debugProtocolLogger(), userDataDir).catch(e => { throw this._rewriteStartupLog(e); }); @@ -101,9 +102,22 @@ export abstract class BrowserType extends SdkObject { async _innerLaunch(progress: Progress, options: types.LaunchOptions, persistent: channels.BrowserNewContextParams | undefined, protocolLogger: types.ProtocolLogger, maybeUserDataDir?: string): Promise { options.proxy = options.proxy ? normalizeProxySettings(options.proxy) : undefined; - const clientCertificatesProxy = await createClientCertificatesProxyIfNeeded(persistent ?? {}); + let clientCertificatesProxy: ClientCertificatesProxy | undefined; + if (persistent) { + // Note: Any initial requests done by browser will not be intercepted by the client certificates proxy. + // We rely when the Page/Frames to ignoreHTTPSErrors to bypass the certificate errors. + clientCertificatesProxy = await createClientCertificatesProxyIfNeeded(persistent); + options.proxy = persistent.proxy; + } const browserLogsCollector = new RecentLogsCollector(); - const { browserProcess, userDataDir, artifactsDir, transport } = await this._launchProcess(progress, options, !!persistent, browserLogsCollector, maybeUserDataDir); + let launchInfo; + try { + launchInfo = await this._launchProcess(progress, options, !!persistent, browserLogsCollector, maybeUserDataDir); + } catch (error) { + await clientCertificatesProxy?.close(); + throw error; + } + const { browserProcess, userDataDir, artifactsDir, transport } = launchInfo; if ((options as any).__testHookBeforeCreateBrowser) await (options as any).__testHookBeforeCreateBrowser(); const browserOptions: BrowserOptions = { @@ -127,13 +141,19 @@ export abstract class BrowserType extends SdkObject { if (persistent) validateBrowserContextOptions(persistent, browserOptions); copyTestHooks(options, browserOptions); - const browser = await this._connectToTransport(transport, browserOptions); + let browser; + try { + browser = await this._connectToTransport(transport, browserOptions); + } catch (error) { + await clientCertificatesProxy?.close(); + throw error; + } (browser as any)._userDataDirForTest = userDataDir; + if (persistent) + browser._defaultContext!._clientCertificatesProxy = clientCertificatesProxy; // We assume no control when using custom arguments, and do not prepare the default context in that case. if (persistent && !options.ignoreAllDefaultArgs) await browser._defaultContext!._loadDefaultContext(progress); - if (persistent) - browser._defaultContext!._clientCertificatesProxy = clientCertificatesProxy; return browser; } diff --git a/packages/playwright-core/types/types.d.ts b/packages/playwright-core/types/types.d.ts index 89b27a118a..2164aa12a3 100644 --- a/packages/playwright-core/types/types.d.ts +++ b/packages/playwright-core/types/types.d.ts @@ -13222,15 +13222,15 @@ export interface BrowserType { chromiumSandbox?: boolean; /** - * An array of client certificates to be used. Each certificate object must have `cert` and `key` or `pfx` to load the - * client certificate. Optionally, `passphrase` property should be provided if the private key is encrypted. If the - * certificate is valid only for specific URLs, the `url` property should be provided with a glob pattern to match the - * URLs that the certificate is valid for. + * An array of client certificates to be used. Each certificate object must have both `certPath` and `keyPath` or a + * single `pfxPath` to load the client certificate. Optionally, `passphrase` property should be provided if the + * private key is encrypted. If the certificate is valid only for specific URLs, the `url` property should be provided + * with a glob pattern to match the URLs that the certificate is valid for. * * **NOTE** Using Client Certificates in combination with Proxy Servers is not supported. * - * **NOTE** When using WebKit on macOS, accessing `localhost` might not work as expected. Instead, use - * `playwright.local` as the hostname. + * **NOTE** When using WebKit on macOS, accessing `localhost` will not pick up client certificates. You can make it + * work by replacing `localhost` with `local.playwright`. Instead, use `playwright.local` as the hostname. */ clientCertificates?: Array<{ /** @@ -15634,15 +15634,15 @@ export interface APIRequest { baseURL?: string; /** - * An array of client certificates to be used. Each certificate object must have `cert` and `key` or `pfx` to load the - * client certificate. Optionally, `passphrase` property should be provided if the private key is encrypted. If the - * certificate is valid only for specific URLs, the `url` property should be provided with a glob pattern to match the - * URLs that the certificate is valid for. + * An array of client certificates to be used. Each certificate object must have both `certPath` and `keyPath` or a + * single `pfxPath` to load the client certificate. Optionally, `passphrase` property should be provided if the + * private key is encrypted. If the certificate is valid only for specific URLs, the `url` property should be provided + * with a glob pattern to match the URLs that the certificate is valid for. * * **NOTE** Using Client Certificates in combination with Proxy Servers is not supported. * - * **NOTE** When using WebKit on macOS, accessing `localhost` might not work as expected. Instead, use - * `playwright.local` as the hostname. + * **NOTE** When using WebKit on macOS, accessing `localhost` will not pick up client certificates. You can make it + * work by replacing `localhost` with `local.playwright`. Instead, use `playwright.local` as the hostname. */ clientCertificates?: Array<{ /** @@ -16828,15 +16828,15 @@ export interface Browser extends EventEmitter { bypassCSP?: boolean; /** - * An array of client certificates to be used. Each certificate object must have `cert` and `key` or `pfx` to load the - * client certificate. Optionally, `passphrase` property should be provided if the private key is encrypted. If the - * certificate is valid only for specific URLs, the `url` property should be provided with a glob pattern to match the - * URLs that the certificate is valid for. + * An array of client certificates to be used. Each certificate object must have both `certPath` and `keyPath` or a + * single `pfxPath` to load the client certificate. Optionally, `passphrase` property should be provided if the + * private key is encrypted. If the certificate is valid only for specific URLs, the `url` property should be provided + * with a glob pattern to match the URLs that the certificate is valid for. * * **NOTE** Using Client Certificates in combination with Proxy Servers is not supported. * - * **NOTE** When using WebKit on macOS, accessing `localhost` might not work as expected. Instead, use - * `playwright.local` as the hostname. + * **NOTE** When using WebKit on macOS, accessing `localhost` will not pick up client certificates. You can make it + * work by replacing `localhost` with `local.playwright`. Instead, use `playwright.local` as the hostname. */ clientCertificates?: Array<{ /** @@ -20303,15 +20303,15 @@ export interface BrowserContextOptions { bypassCSP?: boolean; /** - * An array of client certificates to be used. Each certificate object must have `cert` and `key` or `pfx` to load the - * client certificate. Optionally, `passphrase` property should be provided if the private key is encrypted. If the - * certificate is valid only for specific URLs, the `url` property should be provided with a glob pattern to match the - * URLs that the certificate is valid for. + * An array of client certificates to be used. Each certificate object must have both `certPath` and `keyPath` or a + * single `pfxPath` to load the client certificate. Optionally, `passphrase` property should be provided if the + * private key is encrypted. If the certificate is valid only for specific URLs, the `url` property should be provided + * with a glob pattern to match the URLs that the certificate is valid for. * * **NOTE** Using Client Certificates in combination with Proxy Servers is not supported. * - * **NOTE** When using WebKit on macOS, accessing `localhost` might not work as expected. Instead, use - * `playwright.local` as the hostname. + * **NOTE** When using WebKit on macOS, accessing `localhost` will not pick up client certificates. You can make it + * work by replacing `localhost` with `local.playwright`. Instead, use `playwright.local` as the hostname. */ clientCertificates?: Array<{ /** diff --git a/packages/playwright/types/test.d.ts b/packages/playwright/types/test.d.ts index 2fa3f60b44..7725121d12 100644 --- a/packages/playwright/types/test.d.ts +++ b/packages/playwright/types/test.d.ts @@ -5202,15 +5202,15 @@ export interface PlaywrightTestOptions { */ colorScheme: ColorScheme; /** - * An array of client certificates to be used. Each certificate object must have `cert` and `key` or `pfx` to load the - * client certificate. Optionally, `passphrase` property should be provided if the private key is encrypted. If the - * certificate is valid only for specific URLs, the `url` property should be provided with a glob pattern to match the - * URLs that the certificate is valid for. + * An array of client certificates to be used. Each certificate object must have both `certPath` and `keyPath` or a + * single `pfxPath` to load the client certificate. Optionally, `passphrase` property should be provided if the + * private key is encrypted. If the certificate is valid only for specific URLs, the `url` property should be provided + * with a glob pattern to match the URLs that the certificate is valid for. * * **NOTE** Using Client Certificates in combination with Proxy Servers is not supported. * - * **NOTE** When using WebKit on macOS, accessing `localhost` might not work as expected. Instead, use - * `playwright.local` as the hostname. + * **NOTE** When using WebKit on macOS, accessing `localhost` will not pick up client certificates. You can make it + * work by replacing `localhost` with `local.playwright`. Instead, use `playwright.local` as the hostname. * * **Usage** *