From 560f06b51cdf3ab113f93f0be3e7c50961ebdbac Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Thu, 11 Jul 2024 23:37:44 +0200 Subject: [PATCH] review feedback --- .../playwright-core/src/server/browser.ts | 10 ++-------- .../src/server/browserContext.ts | 20 +++++++++++++++---- .../playwright-core/src/server/browserType.ts | 12 ++--------- packages/playwright-core/src/server/fetch.ts | 3 ++- tests/library/client-certificates.spec.ts | 4 +--- 5 files changed, 23 insertions(+), 26 deletions(-) diff --git a/packages/playwright-core/src/server/browser.ts b/packages/playwright-core/src/server/browser.ts index d37d7d7335..1b77c4b49f 100644 --- a/packages/playwright-core/src/server/browser.ts +++ b/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 { 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) diff --git a/packages/playwright-core/src/server/browserContext.ts b/packages/playwright-core/src/server/browserContext.ts index 388a1027bb..f4090ca9b8 100644 --- a/packages/playwright-core/src/server/browserContext.ts +++ b/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[] = []; 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'); } } diff --git a/packages/playwright-core/src/server/browserType.ts b/packages/playwright-core/src/server/browserType.ts index e13ce20508..413f212831 100644 --- a/packages/playwright-core/src/server/browserType.ts +++ b/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 \' 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 { 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) diff --git a/packages/playwright-core/src/server/fetch.ts b/packages/playwright-core/src/server/fetch.ts index 6dafff956a..f5dad1d656 100644 --- a/packages/playwright-core/src/server/fetch.ts +++ b/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; diff --git a/tests/library/client-certificates.spec.ts b/tests/library/client-certificates.spec.ts index 75162757f9..fc964116d1 100644 --- a/tests/library/client-certificates.spec.ts +++ b/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(f => server.listen(0, 'localhost', () => f())); await use(`https://localhost:${(server.address() as net.AddressInfo).port}/`); await new Promise(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'],