From b55451b178b941dcd15a7139e208e38b176b9452 Mon Sep 17 00:00:00 2001
From: Yoshiya Hinosawa <stibium121@gmail.com>
Date: Sat, 18 Jan 2025 00:10:26 +0900
Subject: [PATCH] fix(ext/node): tls.connect regression (#27707)

The TLS start sequence has been broken since #26661 because of the way
how we wrap TCP handle to create TLS handle.

#26661 introduced happy-eyeballs algorithm and some connection could be
dropped because of happy-eyeball attempt timeout. The current
implementation doesn't consider that case and it could start TLS
handshake with timed out TCP connection. That caused #27652 .

This PR fixes it by changing the initialization steps. Now `wrapHandle`
of TLSSocket set up `afterConnectTls` callback in TCP handle, and
`afterConnect` of TCP handle calls it at `connect` event timing if it
exists. This avoids starting TLS session with timed out connection.

closes #27652
---
 ext/node/polyfills/_tls_wrap.ts |  9 +++------
 ext/node/polyfills/net.ts       |  6 ++++++
 tests/unit_node/tls_test.ts     | 13 +++++++++++++
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/ext/node/polyfills/_tls_wrap.ts b/ext/node/polyfills/_tls_wrap.ts
index dc7dac77ac..6697bc97ac 100644
--- a/ext/node/polyfills/_tls_wrap.ts
+++ b/ext/node/polyfills/_tls_wrap.ts
@@ -79,7 +79,7 @@ export class TLSSocket extends net.Socket {
   ssl: any;
 
   _start() {
-    this[kHandle].afterConnect();
+    this[kHandle].afterConnectTls();
   }
 
   constructor(socket: any, opts: any = kEmptyObject) {
@@ -150,16 +150,14 @@ export class TLSSocket extends net.Socket {
 
       const { promise, resolve } = Promise.withResolvers();
 
-      // Patches `afterConnect` hook to replace TCP conn with TLS conn
-      const afterConnect = handle.afterConnect;
-      handle.afterConnect = async (req: any, status: number) => {
+      // Set `afterConnectTls` hook. This is called in the `afterConnect` method of net.Socket
+      handle.afterConnectTls = async () => {
         options.hostname ??= undefined; // coerce to undefined if null, startTls expects hostname to be undefined
         if (tlssock._needsSockInitWorkaround) {
           // skips the TLS handshake for @npmcli/agent as it's handled by
           // onSocket handler of ClientRequest object.
           tlssock.emit("secure");
           tlssock.removeListener("end", onConnectEnd);
-          return afterConnect.call(handle, req, status);
         }
 
         try {
@@ -190,7 +188,6 @@ export class TLSSocket extends net.Socket {
         } catch {
           // TODO(kt3k): Handle this
         }
-        return afterConnect.call(handle, req, status);
       };
 
       handle.upgrading = promise;
diff --git a/ext/node/polyfills/net.ts b/ext/node/polyfills/net.ts
index 2d57507c1b..8fde8eac1e 100644
--- a/ext/node/polyfills/net.ts
+++ b/ext/node/polyfills/net.ts
@@ -378,6 +378,12 @@ function _afterConnect(
     socket.emit("connect");
     socket.emit("ready");
 
+    // Deno specific: run tls handshake if it's from a tls socket
+    // This swaps the handle[kStreamBaseField] from TcpConn to TlsConn
+    if (typeof handle.afterConnectTls === "function") {
+      handle.afterConnectTls();
+    }
+
     // Start the first read, or get an immediate EOF.
     // this doesn't actually consume any bytes, because len=0.
     if (readable && !socket.isPaused()) {
diff --git a/tests/unit_node/tls_test.ts b/tests/unit_node/tls_test.ts
index f34d9efb5b..97d753e4f8 100644
--- a/tests/unit_node/tls_test.ts
+++ b/tests/unit_node/tls_test.ts
@@ -12,6 +12,7 @@ import { fromFileUrl, join } from "@std/path";
 import * as tls from "node:tls";
 import * as net from "node:net";
 import * as stream from "node:stream";
+import { text } from "node:stream/consumers";
 import { execCode } from "../unit/test_util.ts";
 
 const tlsTestdataDir = fromFileUrl(
@@ -97,6 +98,18 @@ Connection: close
   assertEquals(bodyText, "hello");
 });
 
+// Regression at https://github.com/denoland/deno/issues/27652
+Deno.test("tls.connect makes tls connection to example.com", async () => {
+  const socket = tls.connect(443, "example.com");
+  await new Promise((resolve) => {
+    socket.on("secureConnect", resolve);
+  });
+  socket.write(
+    "GET / HTTP/1.1\r\nHost: example.com\r\nConnection: close\r\n\r\n",
+  );
+  assertStringIncludes(await text(socket), "<title>Example Domain</title>");
+});
+
 // https://github.com/denoland/deno/pull/20120
 Deno.test("tls.connect mid-read tcp->tls upgrade", async () => {
   const { promise, resolve } = Promise.withResolvers<void>();