From 69bc8872d5170239e2987d12945ce2cbb0e087c3 Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Mon, 16 Sep 2024 17:29:29 +0200 Subject: [PATCH] review follow-ups --- packages/playwright-core/src/server/browser.ts | 1 + packages/playwright-core/src/server/browserType.ts | 13 +++++++------ packages/playwright-core/src/server/fetch.ts | 5 ++--- .../server/socksClientCertificatesInterceptor.ts | 9 ++++----- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/playwright-core/src/server/browser.ts b/packages/playwright-core/src/server/browser.ts index 9a7df8bf6c..252443bc44 100644 --- a/packages/playwright-core/src/server/browser.ts +++ b/packages/playwright-core/src/server/browser.ts @@ -86,6 +86,7 @@ export abstract class Browser extends SdkObject { let clientCertificatesProxy: ClientCertificatesProxy | undefined; if (options.clientCertificates?.length) { clientCertificatesProxy = new ClientCertificatesProxy(options); + options = { ...options }; options.proxyOverride = await clientCertificatesProxy.listen(); options.internalIgnoreHTTPSErrors = true; } diff --git a/packages/playwright-core/src/server/browserType.ts b/packages/playwright-core/src/server/browserType.ts index 05c600da26..abc15a20f4 100644 --- a/packages/playwright-core/src/server/browserType.ts +++ b/packages/playwright-core/src/server/browserType.ts @@ -93,8 +93,8 @@ export abstract class BrowserType extends SdkObject { return browser; } - async launchPersistentContext(metadata: CallMetadata, userDataDir: string, persistentContextOptions: channels.BrowserTypeLaunchPersistentContextOptions & { useWebSocket?: boolean, internalIgnoreHTTPSErrors?: boolean }): Promise { - const launchOptions = this._validateLaunchOptions(persistentContextOptions); + async launchPersistentContext(metadata: CallMetadata, userDataDir: string, options: channels.BrowserTypeLaunchPersistentContextOptions & { useWebSocket?: boolean, internalIgnoreHTTPSErrors?: boolean }): Promise { + const launchOptions = this._validateLaunchOptions(options); if (this._useBidi) launchOptions.useWebSocket = true; const controller = new ProgressController(metadata, this); @@ -102,13 +102,14 @@ export abstract class BrowserType extends SdkObject { const browser = await controller.run(async progress => { // Note: Any initial TLS requests will fail since we rely on the Page/Frames initialize which sets ignoreHTTPSErrors. let clientCertificatesProxy: ClientCertificatesProxy | undefined; - if (persistentContextOptions.clientCertificates?.length) { - clientCertificatesProxy = new ClientCertificatesProxy(persistentContextOptions); + if (options.clientCertificates?.length) { + clientCertificatesProxy = new ClientCertificatesProxy(options); launchOptions.proxyOverride = await clientCertificatesProxy?.listen(); - persistentContextOptions.internalIgnoreHTTPSErrors = true; + options = { ...options }; + options.internalIgnoreHTTPSErrors = true; } progress.cleanupWhenAborted(() => clientCertificatesProxy?.close()); - const browser = await this._innerLaunchWithRetries(progress, launchOptions, persistentContextOptions, helper.debugProtocolLogger(), userDataDir).catch(e => { throw this._rewriteStartupLog(e); }); + const browser = await this._innerLaunchWithRetries(progress, launchOptions, options, helper.debugProtocolLogger(), userDataDir).catch(e => { throw this._rewriteStartupLog(e); }); browser._defaultContext!._clientCertificatesProxy = clientCertificatesProxy; return browser; }, TimeoutSettings.launchTimeout(launchOptions)); diff --git a/packages/playwright-core/src/server/fetch.ts b/packages/playwright-core/src/server/fetch.ts index 16d73e8bac..bd9b01c285 100644 --- a/packages/playwright-core/src/server/fetch.ts +++ b/packages/playwright-core/src/server/fetch.ts @@ -169,9 +169,8 @@ export abstract class APIRequestContext extends SdkObject { const method = params.method?.toUpperCase() || 'GET'; const proxy = defaults.proxy; let agent; - // When `clientCertificates` is present, we set the `proxy` property to our own socks proxy - // for the browser to use. However, we don't need it here, because we already respect - // `clientCertificates` when fetching from Node.js. + // We skip 'per-context' in order to not break existing users. 'per-context' was previously used to + // workaround an upstream Chromium bug. Can be removed in the future. if (proxy && proxy.server !== 'per-context' && !shouldBypassProxy(requestUrl, proxy.bypass)) agent = createProxyAgent(proxy); diff --git a/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts b/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts index b7831889f2..08aaa4f25a 100644 --- a/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts +++ b/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts @@ -97,9 +97,8 @@ class SocksProxyConnection { } async connect() { - if (this.socksProxy.agent) - // @ts-expect-error - this.target = await this.socksProxy.agent.callback(new EventEmitter(), { host: rewriteToLocalhostIfNeeded(this.host), port: this.port }); + 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); @@ -241,7 +240,7 @@ export class ClientCertificatesProxy { ignoreHTTPSErrors: boolean | undefined; secureContextMap: Map = new Map(); alpnCache: ALPNCache; - agent: ReturnType | undefined; + proxyAgentFromOptions: ReturnType | undefined; constructor( contextOptions: Pick @@ -249,7 +248,7 @@ export class ClientCertificatesProxy { verifyClientCertificates(contextOptions.clientCertificates); this.alpnCache = new ALPNCache(); this.ignoreHTTPSErrors = contextOptions.ignoreHTTPSErrors; - this.agent = contextOptions.proxy ? createProxyAgent(contextOptions.proxy) : undefined; + this.proxyAgentFromOptions = contextOptions.proxy ? createProxyAgent(contextOptions.proxy) : undefined; this._initSecureContexts(contextOptions.clientCertificates); this._socksProxy = new SocksProxy(); this._socksProxy.setPattern('*');