Make encrypt_after_connect support work by cbiesinger · Pull Request #40709 · web-platform-tests/wpt

cbiesinger · 2023-06-22T18:50:12Z

And also enable it for wptrunner.

There were a few bugs here:

close_connection was not set to False in handle_connect. As a result, the actual request coming over the now-encrypted connection was not handled.
When the server is used as a proxy, for HTTP requests the client will send a full URL in the request line. This is always http because https will use the CONNECT method. So use http instead of using the server's scheme.
For some reason, even with close_connection=True, the socket does not seem to actually get closed. Because many handlers do not send a content length header, this leads to the request timing out. Directly call shutdown() to ensure the socket gets closed.

This PR also sets encrypt_after_connect to True when SSL is not turned on.

WeizhongX · 2023-07-26T18:35:36Z


            _host, self.port = self.httpd.socket.getsockname()
-        except Exception:
+        except Exception as e:


unit test failed due to this line:

./wptserve/wptserve/server.py:846:9: F841 local variable 'e' is assigned to but never used

Thanks, fixed now

WeizhongX · 2023-07-26T20:29:08Z

-                                     use_ssl=False,
-                                     key_file=None,
-                                     certificate=None,
+                                     use_ssl=True,


Seems when use_ssl is true, the scheme will be https. Is this what we want? (A bit of background might help.)

Yeah, hm, I think you're right that it shouldn't. Fixed.

WeizhongX · 2023-07-26T20:30:38Z

+        # We can hardcore http here instead of using scheme because this only
+        # happens when the server is used as a proxy server, and that never
+        # sends an HTTPS URL (because those use CONNECT)
+        if self.request_path.startswith("http://"):


So now line 294 is only for connecting to a proxy server's case. This does not seem to be the case previously.

Why do you say that? In which non-proxy cases would a user agent send a URL with scheme in the request?

That said, given the other change this is no longer needed so I reverted it.

cbiesinger

Thanks for the comments! Please take another look, @WeizhongX

cbiesinger · 2023-10-25T19:55:28Z

+        # We can hardcore http here instead of using scheme because this only
+        # happens when the server is used as a proxy server, and that never
+        # sends an HTTPS URL (because those use CONNECT)
+        if self.request_path.startswith("http://"):


Why do you say that? In which non-proxy cases would a user agent send a URL with scheme in the request?

That said, given the other change this is no longer needed so I reverted it.

cbiesinger · 2023-10-25T19:58:35Z

-                                     use_ssl=False,
-                                     key_file=None,
-                                     certificate=None,
+                                     use_ssl=True,


Yeah, hm, I think you're right that it shouldn't. Fixed.

WeizhongX · 2023-10-25T20:37:11Z

The test failed with below message. Is this expected?

/infrastructure/server/test-pac.html
FAIL test that PAC metadata is respected - promise_test: Unhandled rejection with value: object "TypeError: NetworkError when attempting to fetch resource."

cbiesinger · 2023-11-08T18:59:13Z

FYI this is not ready for merge, it breaks wpt serve and I don't know yet why

cbiesinger · 2026-10-01T21:36:02Z

OK, I think this is now ready to land. Sorry for the very long delay.

This will be useful for FedCM and EVP tests.

cbiesinger · 2026-10-01T21:36:27Z

The MacOS failure looks unrelated (unexpected passes in unrelated files)

jonathan-j-lee

Given how old this PR is, I re-reviewed it and it still LGTM.

Enable encrypt_after_connect on the plain HTTP server when SSL is available, so that a PAC file can route https:// requests through it. Always compute an SSL config when certificates are available, even if the SSL environment is not otherwise enabled. Also pass the CA certificate path for the pregenerated certificates so that an SSL config is produced by default, and fall back to plain HTTP in start_http_server when there is no SSL config. Extend infrastructure/server/test-pac to cover an https:// request through the proxy. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

Close TLS connections set up after a CONNECT request in finish(), sending a close_notify first. socketserver only closes the socket it accepted, which wrap_socket() detaches, so the TLS socket was left open. This replaces shutting down every connection that is marked to be closed, which ended TLS connections without a close_notify. Track the scheme per connection, so that requests sent through a CONNECT tunnel have an https URL instead of the HTTP server's scheme. Explain why the connection is kept open after a CONNECT request. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

wpt-pr-bot added infra serve wptserve labels Jun 22, 2023

wpt-pr-bot assigned DanielRyanSmith Jun 22, 2023

wpt-pr-bot requested review from DanielRyanSmith, foolip and jgraham June 22, 2023 18:50

WeizhongX reviewed Jul 26, 2023

View reviewed changes

DanielRyanSmith closed this Oct 5, 2023

DanielRyanSmith reopened this Oct 5, 2023

WeizhongX reviewed Oct 5, 2023

View reviewed changes

cbiesinger force-pushed the proxysupport branch from 9ad004b to 667f18a Compare October 25, 2023 20:04

cbiesinger commented Oct 25, 2023

View reviewed changes

WeizhongX approved these changes Oct 25, 2023

View reviewed changes

web-platform-tests deleted a comment from ribeiromarilene Feb 8, 2024

cbiesinger force-pushed the proxysupport branch from e54cee8 to 2e589e4 Compare October 1, 2026 15:15

wpt-pr-bot requested review from jcscottiii and jonathan-j-lee October 1, 2026 15:15

cbiesinger force-pushed the proxysupport branch from 2e589e4 to dc522a3 Compare October 1, 2026 15:29

jonathan-j-lee approved these changes Oct 8, 2026

View reviewed changes

Comment thread infrastructure/server/test-pac.sub.html Resolved

cbiesinger and others added 2 commits October 8, 2026 17:21

cbiesinger force-pushed the proxysupport branch from dc522a3 to 7d3ec72 Compare October 8, 2026 21:25

cbiesinger enabled auto-merge (rebase) October 9, 2026 14:33

DanielRyanSmith disabled auto-merge October 9, 2026 17:50

DanielRyanSmith merged commit 3b8aed1 into web-platform-tests:master Oct 9, 2026
51 of 56 checks passed

cbiesinger deleted the proxysupport branch October 9, 2026 17:56

panva mentioned this pull request Nov 3, 2026

test(wpt): fix startup checks and failure logging nodejs/undici#6152

Merged

Conversation

cbiesinger commented Jun 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbiesinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WeizhongX commented Oct 25, 2023

Uh oh!

cbiesinger commented Nov 8, 2023

Uh oh!

cbiesinger commented Oct 1, 2026

Uh oh!

cbiesinger commented Oct 1, 2026

Uh oh!

jonathan-j-lee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants