From 1cd51122cc4f6e3969f7fe91aaa1778cf99ab1b4 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Sat, 18 Oct 2025 18:06:58 +0200 Subject: [PATCH] fix: prevent connection leak when redirect='error' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes issue where SSE and other long-lived connections could leak when using fetch with redirect: 'error' option. The spec optimization of reusing fetchParams when window is 'no-window' and redirect is 'error' was causing connection lifecycle issues. In Node.js, request.window is always set to 'no-window', so this optimization was being applied in all cases where redirect: 'error' was used. This caused problems with connection cleanup, particularly for EventSource and SSE use cases where the connection needs to be properly aborted. The fix disables this optimization and always clones the request and copies fetchParams to ensure proper connection lifecycle management. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Fixes: #4627 Signed-off-by: Matteo Collina --- lib/web/fetch/index.js | 34 ++-- test/node-test/fetch-redirect-error-abort.js | 186 +++++++++++++++++++ 2 files changed, 204 insertions(+), 16 deletions(-) create mode 100644 test/node-test/fetch-redirect-error-abort.js diff --git a/lib/web/fetch/index.js b/lib/web/fetch/index.js index 439d78e1c11..9e9f953e03c 100644 --- a/lib/web/fetch/index.js +++ b/lib/web/fetch/index.js @@ -1380,24 +1380,26 @@ async function httpNetworkOrCacheFetch ( // 8. Run these steps, but abort when the ongoing fetch is terminated: - // 1. If request’s window is "no-window" and request’s redirect mode is + // 1. If request's window is "no-window" and request's redirect mode is // "error", then set httpFetchParams to fetchParams and httpRequest to // request. - if (request.window === 'no-window' && request.redirect === 'error') { - httpFetchParams = fetchParams - httpRequest = request - } else { - // Otherwise: - - // 1. Set httpRequest to a clone of request. - httpRequest = cloneRequest(request) - - // 2. Set httpFetchParams to a copy of fetchParams. - httpFetchParams = { ...fetchParams } - - // 3. Set httpFetchParams’s request to httpRequest. - httpFetchParams.request = httpRequest - } + // Note: This optimization is disabled to prevent connection lifecycle issues. + // See https://github.com/nodejs/undici/issues/4627 + // if (request.window === 'no-window' && request.redirect === 'error') { + // httpFetchParams = fetchParams + // httpRequest = request + // } else { + // Otherwise: + + // 1. Set httpRequest to a clone of request. + httpRequest = cloneRequest(request) + + // 2. Set httpFetchParams to a copy of fetchParams. + httpFetchParams = { ...fetchParams } + + // 3. Set httpFetchParams's request to httpRequest. + httpFetchParams.request = httpRequest + // } // 3. Let includeCredentials be true if one of const includeCredentials = diff --git a/test/node-test/fetch-redirect-error-abort.js b/test/node-test/fetch-redirect-error-abort.js new file mode 100644 index 00000000000..b231b61f48d --- /dev/null +++ b/test/node-test/fetch-redirect-error-abort.js @@ -0,0 +1,186 @@ +'use strict' + +const { test } = require('node:test') +const assert = require('node:assert') +const { fetch } = require('../..') +const { createServer } = require('node:http') +const { once } = require('node:events') + +// https://github.com/nodejs/undici/issues/4627 +test('fetch with redirect: error can be properly aborted', async (t) => { + let connectionClosed = false + + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + // SSE-like response + res.writeHead(200, { + 'Content-Type': 'text/event-stream', + 'Cache-Control': 'no-cache', + Connection: 'keep-alive' + }) + + // Send data periodically + const interval = setInterval(() => { + if (!res.destroyed) { + res.write('data: test\n\n') + } + }, 50) + + req.on('close', () => { + connectionClosed = true + clearInterval(interval) + }) + }) + + await once(server.listen(0), 'listening') + + try { + const controller = new AbortController() + + const response = await fetch(`http://localhost:${server.address().port}/sse`, { + signal: controller.signal, + redirect: 'error' + }) + + assert.strictEqual(response.status, 200) + + // Start consuming the stream + const reader = response.body.getReader() + const readPromise = reader.read() + + // Abort after a short delay + await new Promise(resolve => setTimeout(resolve, 100)) + controller.abort() + + // Verify the abort propagated + try { + await readPromise + assert.fail('Expected read to be aborted') + } catch (err) { + assert.ok(err.name === 'AbortError' || err.message.includes('aborted') || err.message.includes('terminated'), 'Read was aborted') + } + + // Give the server time to detect the closed connection + await new Promise(resolve => setTimeout(resolve, 100)) + + // Verify the connection was actually closed on the server side + assert.ok(connectionClosed, 'Connection should be closed on server side') + } finally { + server.close() + } +}) + +test('fetch with redirect: error and window: no-window can be properly aborted', async (t) => { + let connectionClosed = false + + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + res.writeHead(200, { + 'Content-Type': 'text/event-stream', + 'Cache-Control': 'no-cache', + Connection: 'keep-alive' + }) + + const interval = setInterval(() => { + if (!res.destroyed) { + res.write('data: test\n\n') + } + }, 50) + + req.on('close', () => { + connectionClosed = true + clearInterval(interval) + }) + }) + + await once(server.listen(0), 'listening') + + try { + const controller = new AbortController() + + // This mimics how EventSource polyfill might call fetch + const response = await fetch(`http://localhost:${server.address().port}/sse`, { + signal: controller.signal, + redirect: 'error', + window: null // This makes request.window become 'no-window' + }) + + assert.strictEqual(response.status, 200) + + const reader = response.body.getReader() + const readPromise = reader.read() + + await new Promise(resolve => setTimeout(resolve, 100)) + controller.abort() + + try { + await readPromise + assert.fail('Expected read to be aborted') + } catch (err) { + assert.ok(err.name === 'AbortError' || err.message.includes('aborted') || err.message.includes('terminated'), 'Read was aborted') + } + + await new Promise(resolve => setTimeout(resolve, 100)) + + assert.ok(connectionClosed, 'Connection should be closed on server side') + } finally { + server.close() + } +}) + +test('multiple sequential fetches with redirect: error are properly cleaned up', async (t) => { + const connections = [] + + const server = createServer({ joinDuplicateHeaders: true }, (req, res) => { + const conn = { closed: false } + connections.push(conn) + + res.writeHead(200, { + 'Content-Type': 'text/event-stream', + 'Cache-Control': 'no-cache', + Connection: 'keep-alive' + }) + + const interval = setInterval(() => { + if (!res.destroyed) { + res.write('data: test\n\n') + } + }, 50) + + req.on('close', () => { + conn.closed = true + clearInterval(interval) + }) + }) + + await once(server.listen(0), 'listening') + + try { + // Create multiple connections + for (let i = 0; i < 3; i++) { + const controller = new AbortController() + + const response = await fetch(`http://localhost:${server.address().port}/sse`, { + signal: controller.signal, + redirect: 'error' + }) + + assert.strictEqual(response.status, 200) + + // Start reading + const reader = response.body.getReader() + reader.read().catch(() => {}) // Ignore abort errors + + // Abort after a short delay + await new Promise(resolve => setTimeout(resolve, 100)) + controller.abort() + } + + // Give time for all connections to close + await new Promise(resolve => setTimeout(resolve, 100)) + + // All connections should be closed + assert.strictEqual(connections.length, 3, 'Should have 3 connections') + assert.ok(connections.every(c => c.closed), 'All connections should be closed') + } finally { + server.close() + } +})