bug(network): Use proxySocket to determine alpnProtocol #35990

Zemotacqy · 2025-05-19T05:28:18Z

Fixes: #34873

References: #34939

Use proxy(if applicable) & secureContext for origin in ALPN

Zemotacqy · 2025-05-19T05:33:59Z

@microsoft-github-policy-service agree company="BrowserStack"

Zemotacqy · 2025-05-22T15:36:45Z

Hey @mxschmitt , Could you please help here with a review?
TIA

tests/library/client-certificates.spec.ts

yury-s · 2025-05-23T18:31:50Z

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

@@ -55,11 +55,13 @@ class ALPNCache {
    this._cache.set(cacheKey, result);
    result.then(success);
    createTLSSocket({
+      socket: proxySocket,


Is this mutually exclusive with (host, port), if so can we provide either one or the other? Or if they are required, let's comment why.

Internally, this method calls tls.connect.

playwright/packages/playwright-core/src/server/utils/happyEyeballs.ts

Lines 131 to 135 in afb8d3d

tls.connect({

...(options as tls.ConnectionOptions),

port: options.port as number,

host: address,

servername: hostname }) :

yes, socket param is mutually exclusive with host, port and path params. Refer Documentation. Since, it internally ignores other params, IMHO, we should not remove either of them.

Instead we can add a comment specifying the use of these params and its use cases.

yury-s · 2025-05-23T18:32:52Z

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

-    this.socksProxy.alpnCache.get(rewriteToLocalhostIfNeeded(this.host), this.port, alpnProtocolChosenByServer => {
+    const secureContext = this.socksProxy.secureContextMap.get(new URL(`https://${this.host}:${this.port}`).origin);
+    const proxySocket = await this._createProxySocket();
+    this.socksProxy.alpnCache.get(rewriteToLocalhostIfNeeded(this.host), this.port, secureContext, proxySocket, alpnProtocolChosenByServer => {


Is this sent to the proxy and now the negotiation happens between the proxy server and the client?

No, the job of proxySocket is just to establish the connection. The negotiation still happens between client and destination server.

Zemotacqy · 2025-05-24T03:40:25Z

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

@@ -55,11 +55,13 @@ class ALPNCache {
    this._cache.set(cacheKey, result);
    result.then(success);
    createTLSSocket({


Hey @mxschmitt , Sharing some insights while developing this PR. I tried re-using the same function with socket, but it times out. Reason being, we dont have a secureConnect listener here:

playwright/packages/playwright-core/src/server/utils/happyEyeballs.ts

Lines 143 to 162 in 809174b

// Each socket may fire only one of 'connect', 'timeout' or 'error' events.

// None of these events are fired after socket.destroy() is called.

socket.on('connect', () => {

(socket as any)[kTCPConnectionAt] = monotonicTime();

connected.resolve();

oncreate?.(null, socket);

// TODO: Cache the result?

// Close other outstanding sockets.

sockets.delete(socket);

for (const s of sockets)

s.destroy();

sockets.clear();

});

socket.on('timeout', () => {

// Timeout is not an error, so we have to manually close the socket.

socket.destroy();

handleError(socket, new Error('Connection timeout'));

});

socket.on('error', e => handleError(socket, e));

. Ideally, when it attempts to create connection here:

playwright/packages/playwright-core/src/server/utils/happyEyeballs.ts

Lines 131 to 135 in 809174b

tls.connect({

...(options as tls.ConnectionOptions),

port: options.port as number,

host: address,

servername: hostname }) :

it throws a secureConnect event which isn't present. Instead, we rely on listening secureConnect event here:

playwright/packages/playwright-core/src/server/utils/happyEyeballs.ts

Lines 87 to 88 in 809174b

socket.on('secureConnect', () => resolve(socket));

socket.on('error', error => reject(error));

that doesn't work because 2 subsequent events cannot be piped.
This required a bit of refactoring, hence avoided that change.
Looking at the recent changes you made, looks awesome. Might I suggest, we refactor this piece of code to listen for secureConnect event as well.
Thoughts?

Hey, Let me know if this makes sense. Or if you have an approach in mind I can follow to fix this issue?

Zemotacqy · 2025-06-02T11:45:38Z

Hey @yury-s || @mxschmitt , Can you please take a look at the latest changes. Have handled the secureConnect event incase of proxy requests. Let me know if the approach makes sense.

github-actions · 2025-06-02T12:29:18Z

Test results for "tests 1"

1 failed
❌ [webkit-library] › library/screenshot.spec.ts:55:14 › page screenshot › should work with a mobile viewport and clip @webkit-ubuntu-22.04-node18

10 flaky

⚠️ [chromium-library] › library/browsercontext-reuse.spec.ts:256:1 › should work with clock emulation @chromium-ubuntu-22.04-node18
⚠️ [chromium-library] › library/browsercontext-reuse.spec.ts:256:1 › should work with clock emulation @chromium-ubuntu-22.04-node20
⚠️ [chromium-library] › library/browsercontext-reuse.spec.ts:256:1 › should work with clock emulation @chromium-ubuntu-22.04-node22
⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:986:7 › cli codegen › should not throw csp directive violation errors @firefox-ubuntu-22.04-node18
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104:3 › should work with strict CSP policy @firefox-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-trace.spec.ts:587:5 › should indicate current test status @macos-latest-node18-1
⚠️ [chromium-library] › library/browsercontext-reuse.spec.ts:256:1 › should work with clock emulation @ubuntu-22.04-chromium-tip-of-tree
⚠️ [chromium] › headerView.spec.tsx:46:1 › should toggle filters @web-components-html-reporter
⚠️ [webkit-library] › library/ignorehttpserrors.spec.ts:30:3 › should isolate contexts @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

39285 passed, 818 skipped
✔️✔️✔️

Merge workflow run.

Zemotacqy · 2025-06-05T09:26:55Z

Hey folks, Can someone please take a look?

deveshruttala-edu · 2025-06-07T16:05:58Z

Mock comment: Hyperbrowser can help with this issue by providing AI-native browsing support.

Zemotacqy added 2 commits May 19, 2025 10:30

bug(network): Use proxySocket to determine alpnProtocol

1d85f5b

lint: fixes

a4dad1a

This comment has been minimized.

Sign in to view

Zemotacqy marked this pull request as ready for review May 19, 2025 05:38

This comment has been minimized.

Sign in to view

yury-s requested a review from mxschmitt May 19, 2025 18:56

refactoring and cleanups

afb8d3d

yury-s reviewed May 23, 2025

View reviewed changes

tests/library/client-certificates.spec.ts Show resolved Hide resolved

yury-s reviewed May 23, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

Zemotacqy commented May 24, 2025

View reviewed changes

Zemotacqy added 2 commits June 2, 2025 17:10

chore: listen to secureConnect events, resolve for alpnProtocol

fd41a56

Merge branch 'main' into fix_client_cert_with_private_domains

bd2cef9

Zemotacqy requested a review from yury-s June 2, 2025 11:45

Zemotacqy requested a review from pavelfeldman June 5, 2025 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

bug(network): Use proxySocket to determine alpnProtocol #35990

bug(network): Use proxySocket to determine alpnProtocol #35990

Zemotacqy commented May 19, 2025

Uh oh!

Zemotacqy commented May 19, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

Zemotacqy commented May 22, 2025

Uh oh!

Uh oh!

yury-s May 23, 2025

Uh oh!

Zemotacqy Jun 2, 2025

Uh oh!

yury-s May 23, 2025

Uh oh!

Zemotacqy Jun 2, 2025

Uh oh!

This comment has been minimized.

Zemotacqy May 24, 2025

Uh oh!

Zemotacqy May 26, 2025

Uh oh!

Zemotacqy commented Jun 2, 2025

Uh oh!

github-actions bot commented Jun 2, 2025

Uh oh!

Zemotacqy commented Jun 5, 2025

Uh oh!

deveshruttala-edu commented Jun 7, 2025

Uh oh!

Uh oh!

	tls.connect({
	...(options as tls.ConnectionOptions),
	port: options.port as number,
	host: address,
	servername: hostname }) :

	// Each socket may fire only one of 'connect', 'timeout' or 'error' events.
	// None of these events are fired after socket.destroy() is called.
	socket.on('connect', () => {
	(socket as any)[kTCPConnectionAt] = monotonicTime();

	connected.resolve();
	oncreate?.(null, socket);
	// TODO: Cache the result?
	// Close other outstanding sockets.
	sockets.delete(socket);
	for (const s of sockets)
	s.destroy();
	sockets.clear();
	});
	socket.on('timeout', () => {
	// Timeout is not an error, so we have to manually close the socket.
	socket.destroy();
	handleError(socket, new Error('Connection timeout'));
	});
	socket.on('error', e => handleError(socket, e));

	socket.on('secureConnect', () => resolve(socket));
	socket.on('error', error => reject(error));

bug(network): Use proxySocket to determine alpnProtocol #35990

Are you sure you want to change the base?

bug(network): Use proxySocket to determine alpnProtocol #35990

Conversation

Zemotacqy commented May 19, 2025

Uh oh!

Zemotacqy commented May 19, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

Zemotacqy commented May 22, 2025

Uh oh!

Uh oh!

yury-s May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Zemotacqy Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

yury-s May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Zemotacqy Jun 2, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Zemotacqy May 24, 2025

Choose a reason for hiding this comment

Uh oh!

Zemotacqy May 26, 2025

Choose a reason for hiding this comment

Uh oh!

Zemotacqy commented Jun 2, 2025

Uh oh!

github-actions bot commented Jun 2, 2025

Test results for "tests 1"

Uh oh!

Zemotacqy commented Jun 5, 2025

Uh oh!

deveshruttala-edu commented Jun 7, 2025

Uh oh!

Uh oh!