Revert "Reapply "fix(har timing): record connect timing for proxied connections" (#32855) (#33003)" (#34535)

Author: dgozman

packages/playwright-core/ThirdPartyNotices.txt

@@ -15,7 +15,7 @@
     "diff": "^7.0.0",
     "dotenv": "^16.4.5",
     "graceful-fs": "4.2.10",
-    "https-proxy-agent": "7.0.6",
+    "https-proxy-agent": "5.0.1",
     "jpeg-js": "0.4.4",
     "mime": "^3.0.0",
     "minimatch": "^3.1.2",
@@ -25,7 +25,7 @@
     "proxy-from-env": "1.1.0",
     "retry": "0.12.0",
     "signal-exit": "3.0.7",
-    "socks-proxy-agent": "8.0.5",
+    "socks-proxy-agent": "6.1.1",
     "stack-utils": "2.0.5",
     "ws": "8.17.1",
     "yaml": "^2.6.0"

packages/playwright-core/bundles/utils/package-lock.json

@@ -20,6 +20,7 @@ import http from 'http';
 import https from 'https';
 import type { Readable, TransformCallback } from 'stream';
 import { pipeline, Transform } from 'stream';
+import url from 'url';
 import zlib from 'zlib';
 import type { HTTPCredentials } from '../../types/types';
 import { TimeoutSettings } from '../common/timeoutSettings';
@@ -499,12 +500,12 @@ export abstract class APIRequestContext extends SdkObject {
         // happy eyeballs don't emit lookup and connect events, so we use our custom ones
         const happyEyeBallsTimings = timingForSocket(socket);
         dnsLookupAt = happyEyeBallsTimings.dnsLookupAt;
-        tcpConnectionAt ??= happyEyeBallsTimings.tcpConnectionAt;
+        tcpConnectionAt = happyEyeBallsTimings.tcpConnectionAt;
 
         // non-happy-eyeballs sockets
         listeners.push(
             eventsHelper.addEventListener(socket, 'lookup', () => { dnsLookupAt = monotonicTime(); }),
-            eventsHelper.addEventListener(socket, 'connect', () => { tcpConnectionAt ??= monotonicTime(); }),
+            eventsHelper.addEventListener(socket, 'connect', () => { tcpConnectionAt = monotonicTime(); }),
             eventsHelper.addEventListener(socket, 'secureConnect', () => {
               tlsHandshakeAt = monotonicTime();
 
@@ -521,21 +522,11 @@ export abstract class APIRequestContext extends SdkObject {
             }),
         );
 
-        // when using socks proxy, having the socket means the connection got established
-        if (agent instanceof SocksProxyAgent)
-          tcpConnectionAt ??= monotonicTime();
-
         serverIPAddress = socket.remoteAddress;
         serverPort = socket.remotePort;
       });
       request.on('finish', () => { requestFinishAt = monotonicTime(); });
 
-      // http proxy
-      request.on('proxyConnect', () => {
-        tcpConnectionAt ??= monotonicTime();
-      });
-
-
       progress.log(`→ ${options.method} ${url.toString()}`);
       if (options.headers) {
         for (const [name, value] of Object.entries(options.headers))
@@ -702,16 +693,17 @@ export class GlobalAPIRequestContext extends APIRequestContext {
 }
 
 export function createProxyAgent(proxy: types.ProxySettings) {
-  const proxyURL = new URL(proxy.server);
-  if (proxyURL.protocol?.startsWith('socks'))
-    return new SocksProxyAgent(proxyURL);
-
+  const proxyOpts = url.parse(proxy.server);
+  if (proxyOpts.protocol?.startsWith('socks')) {
+    return new SocksProxyAgent({
+      host: proxyOpts.hostname,
+      port: proxyOpts.port || undefined,
+    });
+  }
   if (proxy.username)
-    proxyURL.username = proxy.username;
-  if (proxy.password)
-    proxyURL.password = proxy.password;
-  // TODO: We should use HttpProxyAgent conditional on proxyURL.protocol instead of always using CONNECT method.
-  return new HttpsProxyAgent(proxyURL);
+    proxyOpts.auth = `${proxy.username}:${proxy.password || ''}`;
+  // TODO: We should use HttpProxyAgent conditional on proxyOpts.protocol instead of always using CONNECT method.
+  return new HttpsProxyAgent(proxyOpts);
 }
 
 function toHeadersArray(rawHeaders: string[]): types.HeadersArray {

packages/playwright-core/bundles/utils/package.json

@@ -98,7 +98,7 @@ class SocksProxyConnection {
 
   async connect() {
     if (this.socksProxy.proxyAgentFromOptions)
-      this.target = await this.socksProxy.proxyAgentFromOptions.connect(new EventEmitter() as any, { host: rewriteToLocalhostIfNeeded(this.host), port: this.port, secureEndpoint: false });
+      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);
 

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

@@ -50,7 +50,7 @@ export function httpRequest(params: HTTPRequestParams, onResponse: (r: http.Inco
 
   const proxyURL = getProxyForUrl(params.url);
   if (proxyURL) {
-    const parsedProxyURL = new URL(proxyURL);
+    const parsedProxyURL = url.parse(proxyURL);
     if (params.url.startsWith('http:')) {
       options = {
         path: parsedUrl.href,

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

@@ -138,7 +138,7 @@ export async function setupSocksForwardingServer({
   const socksProxy = new SocksProxy();
   socksProxy.setPattern('*');
   socksProxy.addListener(SocksProxy.Events.SocksRequested, async (payload: SocksSocketRequestedPayload) => {
-    if (!['127.0.0.1', '0:0:0:0:0:0:0:1', 'fake-localhost-127-0-0-1.nip.io', 'localhost'].includes(payload.host) || payload.port !== allowedTargetPort) {
+    if (!['127.0.0.1', 'fake-localhost-127-0-0-1.nip.io', 'localhost'].includes(payload.host) || payload.port !== allowedTargetPort) {
       socksProxy.sendSocketError({ uid: payload.uid, error: 'ECONNREFUSED' });
       return;
     }

packages/playwright-core/src/utils/network.ts

@@ -844,7 +844,6 @@ for (const kind of ['launchServer', 'run-server'] as const) {
       });
 
       test('should proxy requests from fetch api', async ({ startRemoteServer, server, browserName, connect, channel, platform, dummyServerPort }, workerInfo) => {
-        test.fixme(true, 'broken because of socks proxy agent error: Socks5 proxy rejected connection - ConnectionRefused');
         test.skip(browserName === 'webkit' && platform === 'darwin', 'no localhost proxying');
 
         let reachedOriginalTarget = false;

tests/config/proxy.ts

@@ -390,7 +390,7 @@ test.describe('browser', () => {
     });
     expect(connectHosts).toEqual([]);
     await page.goto(serverURL);
-    const host = browserName === 'webkit' && isMac ? '0:0:0:0:0:0:0:1' : '127.0.0.1';
+    const host = browserName === 'webkit' && isMac ? 'localhost' : '127.0.0.1';
     expect(connectHosts).toEqual([`${host}:${serverPort}`]);
     await expect(page.getByTestId('message')).toHaveText('Hello Alice, your certificate was issued by localhost!');
     await page.close();

tests/library/browsertype-connect.spec.ts

@@ -24,9 +24,9 @@ import type { Log } from '../../packages/trace/src/har';
 import { parseHar } from '../config/utils';
 const { createHttp2Server } = require('../../packages/playwright-core/lib/utils');
 
-async function pageWithHar(contextFactory: (options?: BrowserContextOptions) => Promise<BrowserContext>, testInfo: any, options: { outputPath?: string, proxy?: BrowserContextOptions['proxy'] } & Partial<Pick<BrowserContextOptions['recordHar'], 'content' | 'omitContent' | 'mode'>> = {}) {
+async function pageWithHar(contextFactory: (options?: BrowserContextOptions) => Promise<BrowserContext>, testInfo: any, options: { outputPath?: string } & Partial<Pick<BrowserContextOptions['recordHar'], 'content' | 'omitContent' | 'mode'>> = {}) {
   const harPath = testInfo.outputPath(options.outputPath || 'test.har');
-  const context = await contextFactory({ recordHar: { path: harPath, ...options }, ignoreHTTPSErrors: true, proxy: options.proxy });
+  const context = await contextFactory({ recordHar: { path: harPath, ...options }, ignoreHTTPSErrors: true });
   const page = await context.newPage();
   return {
     page,
@@ -861,38 +861,6 @@ it('should respect minimal mode for API Requests', async ({ contextFactory, serv
   expect(entry.response.bodySize).toBe(-1);
 });
 
-it('should include timings when using http proxy', async ({ contextFactory, server, proxyServer }, testInfo) => {
-  proxyServer.forwardTo(server.PORT, { allowConnectRequests: true });
-  const { page, getLog } = await pageWithHar(contextFactory, testInfo, { proxy: { server: `localhost:${proxyServer.PORT}` } });
-  const response = await page.request.get(server.EMPTY_PAGE);
-  expect(proxyServer.connectHosts).toEqual([`localhost:${server.PORT}`]);
-  await expect(response).toBeOK();
-  const log = await getLog();
-  expect(log.entries[0].timings.connect).toBeGreaterThan(0);
-});
-
-it('should include timings when using socks proxy', async ({ contextFactory, server, socksPort }, testInfo) => {
-  const { page, getLog } = await pageWithHar(contextFactory, testInfo, { proxy: { server: `socks5://localhost:${socksPort}` } });
-  const response = await page.request.get(server.EMPTY_PAGE);
-  expect(await response.text()).toContain('Served by the SOCKS proxy');
-  await expect(response).toBeOK();
-  const log = await getLog();
-  expect(log.entries[0].timings.connect).toBeGreaterThan(0);
-});
-
-it('should not have connect and dns timings when socket is reused', async ({ contextFactory, server }, testInfo) => {
-  const { page, getLog } = await pageWithHar(contextFactory, testInfo);
-  await page.request.get(server.EMPTY_PAGE);
-  await page.request.get(server.EMPTY_PAGE);
-
-  const log = await getLog();
-  expect(log.entries).toHaveLength(2);
-  const request2 = log.entries[1];
-  expect.soft(request2.timings.connect).toBe(-1);
-  expect.soft(request2.timings.dns).toBe(-1);
-  expect.soft(request2.timings.blocked).toBeGreaterThan(0);
-});
-
 it('should include redirects from API request', async ({ contextFactory, server }, testInfo) => {
   server.setRedirect('/redirect-me', '/simple.json');
   const { page, getLog } = await pageWithHar(contextFactory, testInfo);