From 5fbcd0360e6beca08146a7ca346ab9fee14f0b24 Mon Sep 17 00:00:00 2001 From: Hugo Torres Date: Thu, 21 Aug 2025 09:29:21 -0400 Subject: [PATCH 1/4] fix: deadlocks when clients wait for secureConnect before writing --- .../ClientRequest/MockHttpSocket.ts | 51 +++- ...-socket-secure-connect-passthrough.test.ts | 229 ++++++++++++++++++ 2 files changed, 272 insertions(+), 8 deletions(-) create mode 100644 test/modules/http/regressions/http-socket-secure-connect-passthrough.test.ts diff --git a/src/interceptors/ClientRequest/MockHttpSocket.ts b/src/interceptors/ClientRequest/MockHttpSocket.ts index c4fe832c4..3da2ce3a8 100644 --- a/src/interceptors/ClientRequest/MockHttpSocket.ts +++ b/src/interceptors/ClientRequest/MockHttpSocket.ts @@ -63,6 +63,7 @@ export class MockHttpSocket extends MockSocket { private shouldKeepAlive?: boolean private socketState: 'unknown' | 'mock' | 'passthrough' = 'unknown' + private hasEarlyConnect = false private responseParser: HTTPParser<1> private responseStream?: Readable private originalSocket?: net.Socket @@ -160,6 +161,36 @@ export class MockHttpSocket extends MockSocket { return emitEvent() } + /** + * Override the 'once' method to detect when clients are waiting for connection events. + * This prevents deadlock when clients wait for 'secureConnect' before writing data. + */ + public once(event: string | symbol, listener: Function): this { + // Handle connection events to prevent deadlock + if (this.connecting && this.socketState === 'unknown') { + if (event === 'secureConnect') { + // Schedule the emission for the next tick to allow the listener to be attached first + setImmediate(() => { + if (this.connecting && this.socketState === 'unknown' && !this.hasEarlyConnect) { + this.hasEarlyConnect = true + this.emit('secureConnect') + } + }) + } + + if (event === 'connect') { + // Schedule the emission for the next tick to allow the listener to be attached first + setImmediate(() => { + if (this.connecting && this.socketState === 'unknown' && !this.hasEarlyConnect) { + this.hasEarlyConnect = true + this.emit('connect') + } + }) + } + } + return super.once(event as any, listener as any) + } + public destroy(error?: Error | undefined): this { // Destroy the response parser when the socket gets destroyed. // Normally, we should listen to the "close" event but it @@ -202,7 +233,7 @@ export class MockHttpSocket extends MockSocket { } // If the developer destroys the socket, destroy the original connection. - this.once('error', (error) => { + this.once('error', (error: Error) => { socket.destroy(error) }) @@ -275,9 +306,13 @@ export class MockHttpSocket extends MockSocket { .on('lookup', (...args) => this.emit('lookup', ...args)) .on('connect', () => { this.connecting = socket.connecting - this.emit('connect') + + // Don't re-emit 'connect' - already emitted in mockConnect() + if (!this.hasEarlyConnect) { + this.emit('connect') + } }) - .on('secureConnect', () => this.emit('secureConnect')) + .on('secureConnect', () => this.hasEarlyConnect ? {} : this.emit('secureConnect')) .on('secure', () => this.emit('secure')) .on('session', (session) => this.emit('session', session)) .on('ready', () => this.emit('ready')) @@ -490,8 +525,8 @@ export class MockHttpSocket extends MockSocket { } private onRequestStart: RequestHeadersCompleteCallback = ( - versionMajor, - versionMinor, + _versionMajor, + _versionMinor, rawHeaders, _, path, @@ -612,10 +647,10 @@ export class MockHttpSocket extends MockSocket { } private onResponseStart: ResponseHeadersCompleteCallback = ( - versionMajor, - versionMinor, + _versionMajor, + _versionMinor, rawHeaders, - method, + _method, url, status, statusText diff --git a/test/modules/http/regressions/http-socket-secure-connect-passthrough.test.ts b/test/modules/http/regressions/http-socket-secure-connect-passthrough.test.ts new file mode 100644 index 000000000..efa7cd659 --- /dev/null +++ b/test/modules/http/regressions/http-socket-secure-connect-passthrough.test.ts @@ -0,0 +1,229 @@ +/** + * @vitest-environment node + * @see https://github.com/mswjs/interceptors/issues/XXX + */ +import { vi, it, expect, beforeAll, afterAll, afterEach } from 'vitest' +import http from 'node:http' +import https from 'node:https' +import type { TLSSocket } from 'node:tls' +import { HttpServer } from '@open-draft/test-server/http' +import { ClientRequestInterceptor } from '../../../../src/interceptors/ClientRequest/index' + +const httpServer = new HttpServer((app) => { + app.get('/', (_req, res) => { + res.status(200).send('original-response') + }) + app.post('/echo', (req, res) => { + let body = '' + req.on('data', chunk => body += chunk) + req.on('end', () => { + res.status(200).json({ received: body }) + }) + }) +}) + +const interceptor = new ClientRequestInterceptor() + +beforeAll(async () => { + interceptor.apply() + await httpServer.listen() +}) + +afterEach(() => { + interceptor.removeAllListeners() +}) + +afterAll(async () => { + interceptor.dispose() + await httpServer.close() +}) + +it('emits "secureConnect" event for passthrough HTTPS requests when client waits for it before writing', async () => { + // This test reproduces the Stripe SDK pattern where the client waits + // for secureConnect before writing data to ensure TLS handshake is complete. + // The issue is that when a request is NOT mocked (passthrough), the MockHttpSocket + // doesn't emit secureConnect at the right time, causing clients to hang. + + const secureConnectListener = vi.fn() + const responseListener = vi.fn() + const errorListener = vi.fn() + + const requestData = JSON.stringify({ test: 'data' }) + + const request = https.request(httpServer.https.url('/echo'), { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Content-Length': Buffer.byteLength(requestData) + }, + rejectUnauthorized: false // Allow self-signed cert from test server + }) + + request.on('error', errorListener) + + request.on('response', (res) => { + responseListener() + + let body = '' + res.on('data', chunk => body += chunk) + res.on('end', () => { + expect(JSON.parse(body)).toEqual({ received: requestData }) + }) + }) + + // This is the critical pattern from Stripe SDK: + // Wait for the socket and then wait for secureConnect before writing + request.once('socket', (socket) => { + console.log('HTTPS test - socket received, connecting:', socket.connecting) + expect(socket).toBeDefined() + + // The socket should indicate it's a TLS socket + const tlsSocket = socket as TLSSocket + expect(tlsSocket.encrypted).toBe(true) + + if (socket.connecting) { + console.log('HTTPS test - socket is connecting, waiting for secureConnect') + // Client waits for secureConnect before writing data + // This is where the bug occurs - secureConnect never fires for passthrough + socket.once('secureConnect', () => { + console.log('HTTPS test - secureConnect received') + secureConnectListener() + request.write(requestData) + request.end() + }) + } else { + console.log('HTTPS test - socket already connected') + // Socket is already connected + secureConnectListener() + request.write(requestData) + request.end() + } + }) + + // Wait for the request to complete + // This will timeout if secureConnect is never emitted + await vi.waitFor(() => { + expect(responseListener).toHaveBeenCalledTimes(1) + }, { timeout: 5000 }) + + // Verify the secureConnect event was handled + expect(secureConnectListener).toHaveBeenCalledTimes(1) + expect(errorListener).not.toHaveBeenCalled() +}) + +it('emits "connect" event for passthrough HTTP requests when client waits for it before writing', async () => { + // Similar test for HTTP to ensure connect event works correctly + + const connectListener = vi.fn() + const responseListener = vi.fn() + const errorListener = vi.fn() + + const requestData = JSON.stringify({ test: 'data' }) + + const request = http.request(httpServer.http.url('/echo'), { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Content-Length': Buffer.byteLength(requestData) + } + }) + + request.on('error', errorListener) + + request.on('response', (res) => { + responseListener() + + let body = '' + res.on('data', chunk => body += chunk) + res.on('end', () => { + expect(JSON.parse(body)).toEqual({ received: requestData }) + }) + }) + + // Pattern similar to Stripe SDK but for HTTP + request.once('socket', (socket) => { + expect(socket).toBeDefined() + + if (socket.connecting) { + // Client waits for connect before writing data + socket.once('connect', () => { + connectListener() + request.write(requestData) + request.end() + }) + } else { + // Socket is already connected + connectListener() + request.write(requestData) + request.end() + } + }) + + // Wait for the request to complete + await vi.waitFor(() => { + expect(responseListener).toHaveBeenCalledTimes(1) + }, { timeout: 5000 }) + + // Verify the connect event was handled + expect(connectListener).toHaveBeenCalledTimes(1) + expect(errorListener).not.toHaveBeenCalled() +}) + +it('emits "secureConnect" for mocked HTTPS requests when client waits for it before writing', async () => { + // Verify that mocked responses work correctly (this should already work) + + interceptor.on('request', ({ request, controller }) => { + if (request.url.includes('/mocked')) { + controller.respondWith(new Response(JSON.stringify({ mocked: true }), { + status: 200, + headers: { 'Content-Type': 'application/json' } + })) + } + }) + + const secureConnectListener = vi.fn() + const responseListener = vi.fn() + + const requestData = JSON.stringify({ test: 'data' }) + + const request = https.request(httpServer.https.url('/mocked'), { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'Content-Length': Buffer.byteLength(requestData) + }, + rejectUnauthorized: false + }) + + request.on('response', (res) => { + responseListener() + + let body = '' + res.on('data', chunk => body += chunk) + res.on('end', () => { + expect(JSON.parse(body)).toEqual({ mocked: true }) + }) + }) + + request.once('socket', (socket) => { + if (socket.connecting) { + socket.once('secureConnect', () => { + secureConnectListener() + request.write(requestData) + request.end() + }) + } else { + secureConnectListener() + request.write(requestData) + request.end() + } + }) + + // Wait for the request to complete + await vi.waitFor(() => { + expect(responseListener).toHaveBeenCalledTimes(1) + }, { timeout: 5000 }) + + // This should work for mocked responses + expect(secureConnectListener).toHaveBeenCalledTimes(1) +}) \ No newline at end of file From b0e57e83ecf9418ca06354c18f947eaf4f1d77de Mon Sep 17 00:00:00 2001 From: Hugo Torres Date: Thu, 21 Aug 2025 15:57:01 -0400 Subject: [PATCH 2/4] fix: some regressions from our secureConnect change --- .../ClientRequest/MockHttpSocket.ts | 86 ++++++++++++++----- ...-socket-secure-connect-passthrough.test.ts | 4 - 2 files changed, 63 insertions(+), 27 deletions(-) diff --git a/src/interceptors/ClientRequest/MockHttpSocket.ts b/src/interceptors/ClientRequest/MockHttpSocket.ts index 3da2ce3a8..f6fc5ad22 100644 --- a/src/interceptors/ClientRequest/MockHttpSocket.ts +++ b/src/interceptors/ClientRequest/MockHttpSocket.ts @@ -63,7 +63,8 @@ export class MockHttpSocket extends MockSocket { private shouldKeepAlive?: boolean private socketState: 'unknown' | 'mock' | 'passthrough' = 'unknown' - private hasEarlyConnect = false + private hasEmittedEarlyConnect = false + private hasEmittedEarlySecureConnect = false private responseParser: HTTPParser<1> private responseStream?: Readable private originalSocket?: net.Socket @@ -160,32 +161,53 @@ export class MockHttpSocket extends MockSocket { return emitEvent() } + /** * Override the 'once' method to detect when clients are waiting for connection events. * This prevents deadlock when clients wait for 'secureConnect' before writing data. */ public once(event: string | symbol, listener: Function): this { - // Handle connection events to prevent deadlock + // Handle connection events to prevent deadlock when clients wait for + // 'connect' or 'secureConnect' before writing data if (this.connecting && this.socketState === 'unknown') { - if (event === 'secureConnect') { - // Schedule the emission for the next tick to allow the listener to be attached first - setImmediate(() => { - if (this.connecting && this.socketState === 'unknown' && !this.hasEarlyConnect) { - this.hasEarlyConnect = true - this.emit('secureConnect') - } - }) - } - - if (event === 'connect') { - // Schedule the emission for the next tick to allow the listener to be attached first - setImmediate(() => { - if (this.connecting && this.socketState === 'unknown' && !this.hasEarlyConnect) { - this.hasEarlyConnect = true - this.emit('connect') - } - }) + if (event === 'secureConnect' && this.baseUrl.protocol === 'https:' && !this.hasEmittedEarlySecureConnect) { + // Check if this is being called from Node.js internals (specifically Socket._read) + const stack = new Error().stack || '' + const isInternalCall = stack.includes('Socket._read') + + // Only set the flag and emit early if this is NOT an internal Node.js call + if (!isInternalCall) { + this.hasEmittedEarlySecureConnect = true + // Schedule the emission for the next tick to allow the listener to be attached first + setImmediate(() => { + if (this.socketState === 'unknown') { + this.connecting = false // Mark as connected + // Emit connect first if not already emitted + if (!this.hasEmittedEarlyConnect) { + this.hasEmittedEarlyConnect = true + this.emit('connect') + } + this.emit('secureConnect') + } + }) + } + } else if (event === 'connect' && !this.hasEmittedEarlyConnect) { + // Check if this is being called from Node.js internals (specifically Socket._read) + const stack = new Error().stack || '' + const isInternalCall = stack.includes('Socket._read') + + // Only set the flag and emit early if this is NOT an internal Node.js call + if (!isInternalCall) { + this.hasEmittedEarlyConnect = true + // Schedule the emission for the next tick to allow the listener to be attached first + setImmediate(() => { + if (this.socketState === 'unknown') { + this.connecting = false // Mark as connected + this.emit('connect') + } + }) + } } } return super.once(event as any, listener as any) @@ -209,6 +231,13 @@ export class MockHttpSocket extends MockSocket { * its data/events through this Socket. */ public passthrough(): void { + // If we scheduled an early connect event but haven't emitted it yet, + // emit it now before going to passthrough mode so Node.js can apply setTimeout + if (this.hasEmittedEarlyConnect && this.connecting) { + this.connecting = false + this.emit('connect') + } + this.socketState = 'passthrough' if (this.destroyed) { @@ -217,6 +246,11 @@ export class MockHttpSocket extends MockSocket { const socket = this.createConnection() this.originalSocket = socket + + // If a timeout was set on this socket before passthrough, apply it to the original socket + if (this.timeout !== undefined && this.timeout > 0) { + socket.setTimeout(this.timeout) + } /** * @note Inherit the original socket's connection handle. @@ -305,14 +339,20 @@ export class MockHttpSocket extends MockSocket { socket .on('lookup', (...args) => this.emit('lookup', ...args)) .on('connect', () => { + this.connecting = socket.connecting - // Don't re-emit 'connect' - already emitted in mockConnect() - if (!this.hasEarlyConnect) { + // Don't re-emit 'connect' if already emitted early + if (!this.hasEmittedEarlyConnect) { this.emit('connect') } }) - .on('secureConnect', () => this.hasEarlyConnect ? {} : this.emit('secureConnect')) + .on('secureConnect', () => { + // Don't re-emit 'secureConnect' if already emitted early + if (!this.hasEmittedEarlySecureConnect) { + this.emit('secureConnect') + } + }) .on('secure', () => this.emit('secure')) .on('session', (session) => this.emit('session', session)) .on('ready', () => this.emit('ready')) diff --git a/test/modules/http/regressions/http-socket-secure-connect-passthrough.test.ts b/test/modules/http/regressions/http-socket-secure-connect-passthrough.test.ts index efa7cd659..28ced338f 100644 --- a/test/modules/http/regressions/http-socket-secure-connect-passthrough.test.ts +++ b/test/modules/http/regressions/http-socket-secure-connect-passthrough.test.ts @@ -74,7 +74,6 @@ it('emits "secureConnect" event for passthrough HTTPS requests when client waits // This is the critical pattern from Stripe SDK: // Wait for the socket and then wait for secureConnect before writing request.once('socket', (socket) => { - console.log('HTTPS test - socket received, connecting:', socket.connecting) expect(socket).toBeDefined() // The socket should indicate it's a TLS socket @@ -82,17 +81,14 @@ it('emits "secureConnect" event for passthrough HTTPS requests when client waits expect(tlsSocket.encrypted).toBe(true) if (socket.connecting) { - console.log('HTTPS test - socket is connecting, waiting for secureConnect') // Client waits for secureConnect before writing data // This is where the bug occurs - secureConnect never fires for passthrough socket.once('secureConnect', () => { - console.log('HTTPS test - secureConnect received') secureConnectListener() request.write(requestData) request.end() }) } else { - console.log('HTTPS test - socket already connected') // Socket is already connected secureConnectListener() request.write(requestData) From f26a1ce4ef55ccc2a58cc061611c7537fe4e1a44 Mon Sep 17 00:00:00 2001 From: Hugo Torres Date: Thu, 21 Aug 2025 16:05:39 -0400 Subject: [PATCH 3/4] fix: address implementation in MockHttpSocket --- .../ClientRequest/MockHttpSocket.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/interceptors/ClientRequest/MockHttpSocket.ts b/src/interceptors/ClientRequest/MockHttpSocket.ts index f6fc5ad22..657873cda 100644 --- a/src/interceptors/ClientRequest/MockHttpSocket.ts +++ b/src/interceptors/ClientRequest/MockHttpSocket.ts @@ -271,8 +271,6 @@ export class MockHttpSocket extends MockSocket { socket.destroy(error) }) - this.address = socket.address.bind(socket) - // Flush the buffered "socket.write()" calls onto // the original socket instance (i.e. write request body). // Exhaust the "requestBuffer" in case this Socket @@ -375,6 +373,22 @@ export class MockHttpSocket extends MockSocket { .on('end', () => this.emit('end')) } + // If address is called on a passthrough socket before the original socket is set, it won't + // return anything. This can happen because sockets fire `connect` early to avoid a causing + // a deadlock with some HTTP clients that like to wait for `connect` or `secureConnect` + // before calling `write`. + public address(): net.AddressInfo | {} { + if (this.originalSocket) { + return this.originalSocket.address() + } + + return { + address: this.connectionOptions.hostname || this.connectionOptions.host || '127.0.0.1', + family: this.connectionOptions.family === 6 ? 'IPv6' : 'IPv4', + port: this.connectionOptions.port || 80 + } + } + /** * Convert the given Fetch API `Response` instance to an * HTTP message and push it to the socket. From 1e9d3d7d281844c47de106e70ef9608bc507c9c0 Mon Sep 17 00:00:00 2001 From: Hugo Torres Date: Thu, 21 Aug 2025 19:15:52 -0400 Subject: [PATCH 4/4] refactor: track connect listeners set up by _read better --- .../ClientRequest/MockHttpSocket.ts | 63 ++++++++++--------- 1 file changed, 35 insertions(+), 28 deletions(-) diff --git a/src/interceptors/ClientRequest/MockHttpSocket.ts b/src/interceptors/ClientRequest/MockHttpSocket.ts index 657873cda..c52a8875d 100644 --- a/src/interceptors/ClientRequest/MockHttpSocket.ts +++ b/src/interceptors/ClientRequest/MockHttpSocket.ts @@ -65,6 +65,7 @@ export class MockHttpSocket extends MockSocket { private socketState: 'unknown' | 'mock' | 'passthrough' = 'unknown' private hasEmittedEarlyConnect = false private hasEmittedEarlySecureConnect = false + private isInternalCall = false private responseParser: HTTPParser<1> private responseStream?: Readable private originalSocket?: net.Socket @@ -151,6 +152,23 @@ export class MockHttpSocket extends MockSocket { } } + /** + * Override _read to track when Node.js internals are making calls. + * Without this, calls to `_read` on the socket will set up listeners + * to `connect` events which will never actually get called because + * `hasEmittedEarlyConnect` will skip emitting `connect` once we've + * detected that a user has tried listening to `connect` events before + * the socket was ready for it. + */ + public _read(size: number): void { + this.isInternalCall = true + try { + super._read(size) + } finally { + this.isInternalCall = false + } + } + public emit(event: string | symbol, ...args: any[]): boolean { const emitEvent = super.emit.bind(this, event as any, ...args) @@ -165,19 +183,16 @@ export class MockHttpSocket extends MockSocket { /** * Override the 'once' method to detect when clients are waiting for connection events. - * This prevents deadlock when clients wait for 'secureConnect' before writing data. + * This prevents deadlock when clients wait for 'secureConnect' before writing data. Some + * HTTP clients like the Stripe SDK like to do this. Since the interceptor waits for + * `write` to be called before it knows if something is to be mocked or bypassed, this + * causes a deadlock because it'll never emit `secureConnect` until it knows. */ - public once(event: string | symbol, listener: Function): this { - // Handle connection events to prevent deadlock when clients wait for - // 'connect' or 'secureConnect' before writing data - if (this.connecting && this.socketState === 'unknown') { + public once(event: string, listener: (...args: any[]) => void): this { + // Only emit early connection events for user code, not Node.js internals + // Node.js calls once('connect') from Socket._read which sets isInternalCall flag + if (!this.isInternalCall && this.connecting && this.socketState === 'unknown') { if (event === 'secureConnect' && this.baseUrl.protocol === 'https:' && !this.hasEmittedEarlySecureConnect) { - // Check if this is being called from Node.js internals (specifically Socket._read) - const stack = new Error().stack || '' - const isInternalCall = stack.includes('Socket._read') - - // Only set the flag and emit early if this is NOT an internal Node.js call - if (!isInternalCall) { this.hasEmittedEarlySecureConnect = true // Schedule the emission for the next tick to allow the listener to be attached first setImmediate(() => { @@ -191,23 +206,15 @@ export class MockHttpSocket extends MockSocket { this.emit('secureConnect') } }) - } } else if (event === 'connect' && !this.hasEmittedEarlyConnect) { - // Check if this is being called from Node.js internals (specifically Socket._read) - const stack = new Error().stack || '' - const isInternalCall = stack.includes('Socket._read') - - // Only set the flag and emit early if this is NOT an internal Node.js call - if (!isInternalCall) { - this.hasEmittedEarlyConnect = true - // Schedule the emission for the next tick to allow the listener to be attached first - setImmediate(() => { - if (this.socketState === 'unknown') { - this.connecting = false // Mark as connected - this.emit('connect') - } - }) - } + this.hasEmittedEarlyConnect = true + // Schedule the emission for the next tick to allow the listener to be attached first + setImmediate(() => { + if (this.socketState === 'unknown') { + this.connecting = false // Mark as connected + this.emit('connect') + } + }) } } return super.once(event as any, listener as any) @@ -237,7 +244,7 @@ export class MockHttpSocket extends MockSocket { this.connecting = false this.emit('connect') } - + this.socketState = 'passthrough' if (this.destroyed) {