feat: client certificates fixes (#34873) and URL pattern support #34939

mmacvicar · 2025-02-27T04:46:32Z

Bug Fixes

Fix for #34873

Makes ALPN use a proxy if available and the secureContext for the origin.
Enable lookup fixtures in places that were missing, like the client certificates socks proxy.
Private domains now work if available thorugh a proxy and using client certificates.

Support for patterns in clientCertificates

Backward compatible, current origin with schema are valid patterns.
Follows chromium enterprise policy URL patterns, also known as content settings patterns
https://chromeenterprise.google/policies/url-patterns/
https://source.chromium.org/chromium/chromium/src/+/main:components/content_settings/core/common/content_settings_pattern.h;l=248;drc=20799f4c32d950ce93d495f44eec648400f38a19

For example:

https://[*.]domain.com
https://*/
https://..domain.com/
https://www.domain.com:*/

vitalets · 2025-02-27T10:24:47Z

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

@@ -357,3 +431,181 @@ export function rewriteOpenSSLErrorIfNeeded(error: Error): Error {
    'You could probably modernize the certificate by following the steps at https://github.com/nodejs/node/issues/40672#issuecomment-1243648223',
  ].join('\n'));
 }
+
+/*
+  Pattern is a pattern that matches a URL. Based on the Chromium


Maybe consider using URLPattern as it becomes a standard.

Yes, also it solves the parsing which I really wanted to avoid. I tried it and ended up discarding it, there was something I was missing I cannot remember now. Let me check that and get back.

So! URLPattern would do the job nicely for this and most cases where playwright needs to do url matching. However, it has been available in Node.js current (23.x) only since last month, on the browser is also early and experimental, it is still missing in firefox and people is discussing APIs to match url lists due to performance concerns.

Said that, there is an alternative which is ditching [.] in favor of {.}?, which would make it work as a subset of URLPattern instead of following the chromium patterns. That would make sense if the project is looking forward to adopt URLPattern broadly, in the meantime I opted for the chromium patterns as 1) it was the only I knew was used for this, it pops out when people search about auto selecting client certs, 2) it is fairly simple to implement.

For reference: #34915

Please stay away from URLPattern.

mmacvicar · 2025-02-27T14:44:24Z

@microsoft-github-policy-service agree

- ALPNCache uses proxy and secure context. - Allow using resolve fixtures for client cert proxy and ALPN cache.

github-actions · 2025-03-01T03:48:45Z

Test results for "tests 1"

5 flaky

⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-output.spec.ts:80:5 › should show console messages for test @macos-latest-node18-1
⚠️ [playwright-test] › tests/ui-mode-trace.spec.ts:341:5 › should work behind reverse proxy @ubuntu-latest-node18-1
⚠️ [webkit-page] › tests/page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

38672 passed, 793 skipped
✔️✔️✔️

Merge workflow run.

pavelfeldman · 2025-03-04T00:41:50Z

docs/src/api/params.md

@@ -549,7 +549,7 @@ Does not enforce fixed viewport, allows resizing window in the headed mode.

 ## context-option-clientCertificates
 - `clientCertificates` <[Array]<[Object]>>
-  - `origin` <[string]> Exact origin that the certificate is valid for. Origin includes `https` protocol, a hostname and optionally a port.
+  - `origin` <[string]> Exact origin or chromium enterprise policy style URL pattern that the certificate is valid for. Origin includes `https` protocol, a hostname and optionally a port.


Adding yet another pattern will definitely slow things down. If you absolutely need a pattern, use regex.

pavelfeldman · 2025-03-04T00:44:44Z

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

@@ -39,10 +39,15 @@ function loadDummyServerCertsIfNeeded() {
  dummyServerTlsOptions = { key, cert };
 }

+type ALPNCacheOptions = {
+  socket?: stream.Duplex | undefined;


nit: we would drop | undefined and use ? notation for both properties unless this would be a refactoring that needs to update all the call sites.

pavelfeldman · 2025-03-04T00:48:06Z

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

  }
 }

+// Only used for fixtures
+type SocksProxyConnectionOptions = {


You are in the server non-user-facing code, you can introduce a property fooForTest if you need it.

pavelfeldman · 2025-03-04T00:49:46Z

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

    this.internal = new stream.Duplex({
      read: () => {},
      write: (data, encoding, callback) => {
        this.socksProxy._socksProxy.sendSocketData({ uid: this.uid, data });
        callback();
      }
    });
-    this.socksProxy.alpnCache.get(rewriteToLocalhostIfNeeded(this.host), this.port, alpnProtocolChosenByServer => {
+    const secureContext = this.socksProxy.secureContextForOrigin(new URL(`https://${this.host}:${this.port}`).origin);


We don't do more than one thing in a single PR. Your changes look reasonable, but reviewing them together is a bit hard, especially since they touch user-facing API. Please split it into fixes for those!

There are essentially 2 things here + things that were needed just to test them. First, there is the fix to #34873 which straightens the ALPN behavior, this one can be indeed isolated. Although, it does fix more than one thing, for example, apart from not using the proxy to figure the protocol, it did not used the secure context either. As they touched the same parts and required some changes to be tested, if separated I could make a couple of dependants PRs.

Then there is the patterns, that one is very isolated with the only problem that in order to be tested it required changes done in the previous part. I just tried to avoid having dependant PRs.

Would that split make sense of you would go even smaller? I could do one addressing all of the ALPNCache issues, just ensuring it reaches out hosts in the same way the client would and another one for the patterns depending on the first.

if separated I could make a couple of dependants PRs.

I'd very much prefer that.

Would that split make sense of you would go even smaller?

I need to understand the PR in order to review it. Making it about one thing helps keep track of things. I don't have a question of "Ok, this line changed, which of the 4 things this PR does did it change for?". Does not need to be a PR per line, but proper scoping would minimize the amount of questions and friction during the review.

pavelfeldman · 2025-03-04T00:50:06Z

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

@@ -357,3 +431,181 @@ export function rewriteOpenSSLErrorIfNeeded(error: Error): Error {
    'You could probably modernize the certificate by following the steps at https://github.com/nodejs/node/issues/40672#issuecomment-1243648223',
  ].join('\n'));
 }
+
+/*
+  Pattern is a pattern that matches a URL. Based on the Chromium


Please stay away from URLPattern.

mmacvicar mentioned this pull request Feb 27, 2025

[Bug]: Timeout if host is only reacheable via a proxy and client certificates settings is set #34873

Open

mmacvicar changed the title ~~Client certificates fixes (#34873) and URL pattern support~~ feat: client certificates fixes (#34873) and URL pattern support Feb 27, 2025

This comment has been minimized.

Sign in to view

vitalets reviewed Feb 27, 2025

View reviewed changes

mmacvicar force-pushed the fix_client_certs_private_domains branch from 1da0b4e to bfe6e65 Compare February 27, 2025 14:48

This comment has been minimized.

Sign in to view

yury-s requested a review from mxschmitt February 28, 2025 21:56

mmacvicar added 10 commits February 28, 2025 19:49

Allow extra domains in test socks proxy

4863ba7

Fix use of client certificates on hosts behind proxies.

c0197e4

- ALPNCache uses proxy and secure context. - Allow using resolve fixtures for client cert proxy and ALPN cache.

Add lookup fixtures are passed to createSocket

275857f

Add pattern can be used to select client certificates

2657eb5

Add example with literal IP addresses.

6948762

Add explanation and source for Pattern implementation

0b76c01

Add client certificate patterns to the doc

c752493

Remove no longer used function

745aa27

Fix lint issue

e7044dc

Update declarations

87c3cb9

mmacvicar force-pushed the fix_client_certs_private_domains branch from 0223cc7 to 87c3cb9 Compare March 1, 2025 03:03

pavelfeldman reviewed Mar 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: client certificates fixes (#34873) and URL pattern support #34939

feat: client certificates fixes (#34873) and URL pattern support #34939

mmacvicar commented Feb 27, 2025

This comment has been minimized.

vitalets Feb 27, 2025

mmacvicar Feb 27, 2025

mmacvicar Feb 27, 2025 •

edited

Loading

vitalets Feb 27, 2025

pavelfeldman Mar 4, 2025

mmacvicar commented Feb 27, 2025

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Mar 1, 2025

pavelfeldman Mar 4, 2025

pavelfeldman Mar 4, 2025

pavelfeldman Mar 4, 2025

pavelfeldman Mar 4, 2025

mmacvicar Mar 5, 2025 •

edited

Loading

pavelfeldman Mar 7, 2025 •

edited

Loading

pavelfeldman Mar 4, 2025

feat: client certificates fixes (#34873) and URL pattern support #34939

Are you sure you want to change the base?

feat: client certificates fixes (#34873) and URL pattern support #34939

Conversation

mmacvicar commented Feb 27, 2025

Bug Fixes

Support for patterns in clientCertificates

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmacvicar Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmacvicar commented Feb 27, 2025

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Mar 1, 2025

Test results for "tests 1"

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmacvicar Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

pavelfeldman Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmacvicar Feb 27, 2025 •

edited

Loading

mmacvicar Mar 5, 2025 •

edited

Loading

pavelfeldman Mar 7, 2025 •

edited

Loading