From b38e8a393f19dfcf14148b6eee35b3c5adc82f6d Mon Sep 17 00:00:00 2001 From: Yoshiya Hinosawa Date: Fri, 17 Jan 2025 12:47:23 +0900 Subject: [PATCH 1/4] fix(ext/node): tls.connect regression --- ext/node/polyfills/_tls_wrap.ts | 8 ++------ ext/node/polyfills/net.ts | 8 +++++++- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/ext/node/polyfills/_tls_wrap.ts b/ext/node/polyfills/_tls_wrap.ts index dc7dac77ac4bb7..31d95995d18577 100644 --- a/ext/node/polyfills/_tls_wrap.ts +++ b/ext/node/polyfills/_tls_wrap.ts @@ -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; @@ -202,7 +199,6 @@ export class TLSSocket extends net.Socket { // https://github.com/szmarczak/http2-wrapper/blob/51eeaf59ff9344fb192b092241bfda8506983620/source/utils/js-stream-socket.js#L6 handle._parent = handle; handle._parentWrap = wrap; - return handle; } } diff --git a/ext/node/polyfills/net.ts b/ext/node/polyfills/net.ts index 2d57507c1bd277..4e7cb3e4ec6d72 100644 --- a/ext/node/polyfills/net.ts +++ b/ext/node/polyfills/net.ts @@ -337,7 +337,7 @@ export function _normalizeArgs(args: unknown[]): NormalizedArgs { return arr; } -function _afterConnect( +async function _afterConnect( status: number, // deno-lint-ignore no-explicit-any handle: any, @@ -356,6 +356,12 @@ function _afterConnect( return; } + // 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") { + await handle.afterConnectTls(); + } + debug("afterConnect"); assert(socket.connecting); From 090760db8782919455fa8a316b555e22b17937cc Mon Sep 17 00:00:00 2001 From: Yoshiya Hinosawa Date: Fri, 17 Jan 2025 15:15:19 +0900 Subject: [PATCH 2/4] add test case --- tests/unit_node/tls_test.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/unit_node/tls_test.ts b/tests/unit_node/tls_test.ts index f34d9efb5b7857..754ad7ef2545f5 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,16 @@ 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), "Example Domain"); +}) + // https://github.com/denoland/deno/pull/20120 Deno.test("tls.connect mid-read tcp->tls upgrade", async () => { const { promise, resolve } = Promise.withResolvers(); @@ -264,10 +275,12 @@ Deno.test("tls connect upgrade tcp", async () => { const socket = new net.Socket(); socket.connect(443, "google.com"); socket.on("connect", () => { + console.log("hi") const secure = tls.connect({ socket }); secure.on("secureConnect", () => resolve()); }); + console.log("hi") await promise; socket.destroy(); }); From 6f228fc271b90ca0403ea3d69e5397346e4059db Mon Sep 17 00:00:00 2001 From: Yoshiya Hinosawa Date: Fri, 17 Jan 2025 15:54:48 +0900 Subject: [PATCH 3/4] fix --- ext/node/polyfills/_tls_wrap.ts | 3 ++- ext/node/polyfills/net.ts | 12 ++++++------ tests/unit_node/tls_test.ts | 10 +++++----- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/ext/node/polyfills/_tls_wrap.ts b/ext/node/polyfills/_tls_wrap.ts index 31d95995d18577..6697bc97accf89 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) { @@ -199,6 +199,7 @@ export class TLSSocket extends net.Socket { // https://github.com/szmarczak/http2-wrapper/blob/51eeaf59ff9344fb192b092241bfda8506983620/source/utils/js-stream-socket.js#L6 handle._parent = handle; handle._parentWrap = wrap; + return handle; } } diff --git a/ext/node/polyfills/net.ts b/ext/node/polyfills/net.ts index 4e7cb3e4ec6d72..45689be1d255b6 100644 --- a/ext/node/polyfills/net.ts +++ b/ext/node/polyfills/net.ts @@ -356,12 +356,6 @@ async function _afterConnect( return; } - // 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") { - await handle.afterConnectTls(); - } - debug("afterConnect"); assert(socket.connecting); @@ -384,6 +378,12 @@ async 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") { + await 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 754ad7ef2545f5..97d753e4f89597 100644 --- a/tests/unit_node/tls_test.ts +++ b/tests/unit_node/tls_test.ts @@ -12,7 +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 { text } from "node:stream/consumers"; import { execCode } from "../unit/test_util.ts"; const tlsTestdataDir = fromFileUrl( @@ -104,9 +104,11 @@ Deno.test("tls.connect makes tls connection to example.com", async () => { 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"); + socket.write( + "GET / HTTP/1.1\r\nHost: example.com\r\nConnection: close\r\n\r\n", + ); assertStringIncludes(await text(socket), "Example Domain"); -}) +}); // https://github.com/denoland/deno/pull/20120 Deno.test("tls.connect mid-read tcp->tls upgrade", async () => { @@ -275,12 +277,10 @@ Deno.test("tls connect upgrade tcp", async () => { const socket = new net.Socket(); socket.connect(443, "google.com"); socket.on("connect", () => { - console.log("hi") const secure = tls.connect({ socket }); secure.on("secureConnect", () => resolve()); }); - console.log("hi") await promise; socket.destroy(); }); From 17fc0c749071e5093ba9240de9bef769de2f8ce5 Mon Sep 17 00:00:00 2001 From: Yoshiya Hinosawa Date: Fri, 17 Jan 2025 16:53:18 +0900 Subject: [PATCH 4/4] do not use async function --- ext/node/polyfills/net.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/node/polyfills/net.ts b/ext/node/polyfills/net.ts index 45689be1d255b6..8fde8eac1e4747 100644 --- a/ext/node/polyfills/net.ts +++ b/ext/node/polyfills/net.ts @@ -337,7 +337,7 @@ export function _normalizeArgs(args: unknown[]): NormalizedArgs { return arr; } -async function _afterConnect( +function _afterConnect( status: number, // deno-lint-ignore no-explicit-any handle: any, @@ -381,7 +381,7 @@ async function _afterConnect( // 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") { - await handle.afterConnectTls(); + handle.afterConnectTls(); } // Start the first read, or get an immediate EOF.