diff --git a/lib/dispatcher/agent.js b/lib/dispatcher/agent.js index db2f817d0fe..90b46fe3aeb 100644 --- a/lib/dispatcher/agent.js +++ b/lib/dispatcher/agent.js @@ -24,7 +24,6 @@ function defaultFactory (origin, opts) { class Agent extends DispatcherBase { constructor ({ factory = defaultFactory, maxRedirections = 0, connect, ...options } = {}) { - if (typeof factory !== 'function') { throw new InvalidArgumentError('factory must be a function.') } diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index 2b8fa05da29..ef3d38ea4f2 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -279,29 +279,71 @@ class Parser { const offset = llhttp.llhttp_get_error_pos(this.ptr) - currentBufferPtr - if (ret === constants.ERROR.PAUSED_UPGRADE) { - this.onUpgrade(data.slice(offset)) - } else if (ret === constants.ERROR.PAUSED) { - this.paused = true - socket.unshift(data.slice(offset)) - } else if (ret !== constants.ERROR.OK) { - const ptr = llhttp.llhttp_get_error_reason(this.ptr) - let message = '' - /* istanbul ignore else: difficult to make a test case for */ - if (ptr) { - const len = new Uint8Array(llhttp.memory.buffer, ptr).indexOf(0) - message = - 'Response does not match the HTTP/1.1 protocol (' + - Buffer.from(llhttp.memory.buffer, ptr, len).toString() + - ')' + if (ret !== constants.ERROR.OK) { + const body = data.subarray(offset) + + if (ret === constants.ERROR.PAUSED_UPGRADE) { + this.onUpgrade(body) + } else if (ret === constants.ERROR.PAUSED) { + this.paused = true + socket.unshift(body) + } else { + throw this.createError(ret, body) } - throw new HTTPParserError(message, constants.ERROR[ret], data.slice(offset)) } } catch (err) { util.destroy(socket, err) } } + finish () { + assert(currentParser === null) + assert(this.ptr != null) + assert(!this.paused) + + const { llhttp } = this + + let ret + + try { + currentParser = this + ret = llhttp.llhttp_finish(this.ptr) + } finally { + currentParser = null + } + + if (ret === constants.ERROR.OK) { + return null + } + + if (ret === constants.ERROR.PAUSED || ret === constants.ERROR.PAUSED_UPGRADE) { + this.paused = true + return null + } + + return this.createError(ret, EMPTY_BUF) + } + + createError (ret, data) { + const { llhttp, contentLength, bytesRead } = this + + if (contentLength && bytesRead !== parseInt(contentLength, 10)) { + return new ResponseContentLengthMismatchError() + } + + const ptr = llhttp.llhttp_get_error_reason(this.ptr) + let message = '' + if (ptr) { + const len = new Uint8Array(llhttp.memory.buffer, ptr).indexOf(0) + message = + 'Response does not match the HTTP/1.1 protocol (' + + Buffer.from(llhttp.memory.buffer, ptr, len).toString() + + ')' + } + + return new HTTPParserError(message, constants.ERROR[ret], data) + } + destroy () { assert(this.ptr != null) assert(currentParser == null) @@ -673,8 +715,11 @@ async function connectH1 (client, socket) { // On Mac OS, we get an ECONNRESET even if there is a full body to be forwarded // to the user. if (err.code === 'ECONNRESET' && parser.statusCode && !parser.shouldKeepAlive) { - // We treat all incoming data so for as a valid response. - parser.onMessageComplete() + const parserErr = parser.finish() + if (parserErr) { + this[kError] = parserErr + this[kClient][kOnError](parserErr) + } return } @@ -693,8 +738,10 @@ async function connectH1 (client, socket) { const parser = this[kParser] if (parser.statusCode && !parser.shouldKeepAlive) { - // We treat all incoming data so far as a valid response. - parser.onMessageComplete() + const parserErr = parser.finish() + if (parserErr) { + util.destroy(this, parserErr) + } return } @@ -706,8 +753,7 @@ async function connectH1 (client, socket) { if (parser) { if (!this[kError] && parser.statusCode && !parser.shouldKeepAlive) { - // We treat all incoming data so far as a valid response. - parser.onMessageComplete() + this[kError] = parser.finish() || this[kError] } this[kParser].destroy() diff --git a/test/parser-issues.js b/test/parser-issues.js index 2d9f04628de..8ffde294f13 100644 --- a/test/parser-issues.js +++ b/test/parser-issues.js @@ -3,7 +3,16 @@ const { tspl } = require('@matteo.collina/tspl') const { test, after } = require('node:test') const net = require('node:net') -const { Client, errors } = require('..') +const { Client, errors, fetch } = require('..') + +const truncatedChunkedResponse = Buffer.from( + 'HTTP/1.1 200 OK\r\n' + + 'Transfer-Encoding: chunked\r\n' + + 'Connection: close\r\n' + + '\r\n' + + '3\r\n' + + 'hel\r\n' +) test('https://github.com/mcollina/undici/issues/268', async (t) => { t = tspl(t, { plan: 2 }) @@ -123,3 +132,57 @@ test('split header value', async (t) => { await t.completed }) + +test('truncated chunked responses terminated by EOF error the response body', async (t) => { + t = tspl(t, { plan: 3 }) + + const server = net.createServer((socket) => { + socket.end(truncatedChunkedResponse) + }) + after(() => server.close()) + + await new Promise(resolve => server.listen(0, resolve)) + + const client = new Client(`http://localhost:${server.address().port}`) + after(() => client.destroy()) + + client.request({ + method: 'GET', + path: '/' + }, (err, { body } = {}) => { + t.ifError(err) + body + .on('end', () => { + t.fail('expected the truncated chunked body to fail') + }) + .on('error', (err) => { + t.strictEqual(err.name, 'HTTPParserError') + t.strictEqual(err.message, 'Response does not match the HTTP/1.1 protocol (Invalid EOF state)') + }) + .resume() + }) + + await t.completed +}) + +test('fetch rejects truncated chunked responses terminated by EOF', async (t) => { + t = tspl(t, { plan: 3 }) + + const server = net.createServer((socket) => { + socket.end(truncatedChunkedResponse) + }) + after(() => server.close()) + + await new Promise(resolve => server.listen(0, resolve)) + + const res = await fetch(`http://localhost:${server.address().port}`) + t.strictEqual(res.status, 200) + + try { + await res.text() + t.fail('expected fetch to reject the truncated chunked body') + } catch (err) { + t.strictEqual(err.name, 'TypeError') + t.strictEqual(err.cause?.message, 'Response does not match the HTTP/1.1 protocol (Invalid EOF state)') + } +}) diff --git a/test/websocket/permessage-deflate-config.js b/test/websocket/permessage-deflate-config.js index 4a3cc5c4d4c..f3a2e160190 100644 --- a/test/websocket/permessage-deflate-config.js +++ b/test/websocket/permessage-deflate-config.js @@ -1,5 +1,6 @@ 'use strict' +const assert = require('node:assert') const { test } = require('node:test') const { once } = require('node:events') const { WebSocketServer } = require('ws') @@ -16,7 +17,7 @@ test('Agent webSocketOptions.maxPayloadSize is read correctly', async (t) => { t.after(() => agent.close()) // Verify the option is stored and retrievable - t.assert.strictEqual(agent.webSocketOptions.maxPayloadSize, customLimit) + assert.strictEqual(agent.webSocketOptions.maxPayloadSize, customLimit) }) test('Agent with default webSocketOptions uses 128 MB limit', async (t) => { @@ -25,7 +26,7 @@ test('Agent with default webSocketOptions uses 128 MB limit', async (t) => { t.after(() => agent.close()) // Default should be 128 MB - t.assert.strictEqual(agent.webSocketOptions.maxPayloadSize, 128 * 1024 * 1024) + assert.strictEqual(agent.webSocketOptions.maxPayloadSize, 128 * 1024 * 1024) }) test('Custom maxPayloadSize allows messages under limit', async (t) => { @@ -55,7 +56,7 @@ test('Custom maxPayloadSize allows messages under limit', async (t) => { const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`, { dispatcher: agent }) const [event] = await once(client, 'message') - t.assert.strictEqual(event.data.size, dataSize, 'Message under limit should be received') + assert.strictEqual(event.data.size, dataSize, 'Message under limit should be received') client.close() }) @@ -84,7 +85,7 @@ test('Messages at exactly the limit succeed', async (t) => { const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`, { dispatcher: agent }) const [event] = await once(client, 'message') - t.assert.strictEqual(event.data.size, limit, 'Message at exactly the limit should succeed') + assert.strictEqual(event.data.size, limit, 'Message at exactly the limit should succeed') client.close() }) @@ -99,7 +100,7 @@ test('Client webSocketOptions.maxPayloadSize is read correctly', async (t) => { t.after(() => client.close()) // Verify the option is stored and retrievable - t.assert.strictEqual(client.webSocketOptions.maxPayloadSize, customLimit) + assert.strictEqual(client.webSocketOptions.maxPayloadSize, customLimit) }) test('Pool webSocketOptions.maxPayloadSize is read correctly', async (t) => { @@ -113,5 +114,5 @@ test('Pool webSocketOptions.maxPayloadSize is read correctly', async (t) => { t.after(() => pool.close()) // Verify the option is stored and retrievable - t.assert.strictEqual(pool.webSocketOptions.maxPayloadSize, customLimit) + assert.strictEqual(pool.webSocketOptions.maxPayloadSize, customLimit) }) diff --git a/test/websocket/permessage-deflate-limit.js b/test/websocket/permessage-deflate-limit.js index 30c89866842..45cdea4b77d 100644 --- a/test/websocket/permessage-deflate-limit.js +++ b/test/websocket/permessage-deflate-limit.js @@ -1,5 +1,6 @@ 'use strict' +const assert = require('node:assert') const { test } = require('node:test') const { once } = require('node:events') const { randomFillSync } = require('node:crypto') @@ -26,7 +27,7 @@ test('Compressed message under limit decompresses successfully', async (t) => { const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`) const [event] = await once(client, 'message') - t.assert.strictEqual(event.data.size, 1024) + assert.strictEqual(event.data.size, 1024) client.close() }) @@ -41,7 +42,7 @@ test('Agent webSocketOptions.maxPayloadSize is read correctly', async (t) => { t.after(() => agent.close()) // Verify the option is stored and retrievable - t.assert.strictEqual(agent.webSocketOptions.maxPayloadSize, customLimit) + assert.strictEqual(agent.webSocketOptions.maxPayloadSize, customLimit) }) test('Agent with default webSocketOptions uses 128 MB limit', async (t) => { @@ -50,7 +51,7 @@ test('Agent with default webSocketOptions uses 128 MB limit', async (t) => { t.after(() => agent.close()) // Default should be 128 MB - t.assert.strictEqual(agent.webSocketOptions.maxPayloadSize, 128 * 1024 * 1024) + assert.strictEqual(agent.webSocketOptions.maxPayloadSize, 128 * 1024 * 1024) }) test('Custom maxPayloadSize allows messages under limit', async (t) => { @@ -80,7 +81,7 @@ test('Custom maxPayloadSize allows messages under limit', async (t) => { const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`, { dispatcher: agent }) const [event] = await once(client, 'message') - t.assert.strictEqual(event.data.size, dataSize, 'Message under limit should be received') + assert.strictEqual(event.data.size, dataSize, 'Message under limit should be received') client.close() }) @@ -109,7 +110,7 @@ test('Messages at exactly the limit succeed', async (t) => { const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`, { dispatcher: agent }) const [event] = await once(client, 'message') - t.assert.strictEqual(event.data.size, limit, 'Message at exactly the limit should succeed') + assert.strictEqual(event.data.size, limit, 'Message at exactly the limit should succeed') client.close() }) @@ -132,7 +133,7 @@ test('Compressed frame payload over wire-size limit is rejected', async (t) => { } } - t.assert.ok(payload, 'Expected incompressible payload with compressed wire size over the limit') + assert.ok(payload, 'Expected incompressible payload with compressed wire size over the limit') let messageReceived = false @@ -159,8 +160,8 @@ test('Compressed frame payload over wire-size limit is rejected', async (t) => { await Promise.race([closePromise, timeoutPromise]) - t.assert.strictEqual(messageReceived, false, 'Compressed frame over wire-size limit should be rejected') - t.assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') + assert.strictEqual(messageReceived, false, 'Compressed frame over wire-size limit should be rejected') + assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') }) test('Messages over the limit are rejected', async (t) => { @@ -206,9 +207,9 @@ test('Messages over the limit are rejected', async (t) => { await Promise.race([closePromise, timeoutPromise]) - t.assert.strictEqual(messageReceived, false, 'Message over limit should be rejected') - t.assert.ok(closeEvent !== null, 'Close event should have been emitted') - t.assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') + assert.strictEqual(messageReceived, false, 'Message over limit should be rejected') + assert.ok(closeEvent !== null, 'Close event should have been emitted') + assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') }) test('Limit can be disabled by setting maxPayloadSize to 0', async (t) => { @@ -220,7 +221,9 @@ test('Limit can be disabled by setting maxPayloadSize to 0', async (t) => { t.after(() => server.close()) await once(server, 'listening') - const dataSize = 100 * 1024 * 1024 // 100 MB + // Keep this comfortably above the smaller limits used elsewhere in this file, + // while avoiding the 100 MB transfer that can be slow on CI. + const dataSize = 2 * 1024 * 1024 // 2 MB server.on('connection', (ws) => { ws.send(Buffer.alloc(dataSize, 0x41), { binary: true }) @@ -236,19 +239,16 @@ test('Limit can be disabled by setting maxPayloadSize to 0', async (t) => { t.after(() => agent.close()) const client = new WebSocket(`ws://127.0.0.1:${server.address().port}`, { dispatcher: agent }) + const timeout = Symbol('timeout') - // Use Promise.race with timeout since large message takes time - const messagePromise = once(client, 'message') - const timeoutPromise = sleep(10000) + const result = await Promise.race([ + once(client, 'message'), + sleep(10000, timeout) + ]) - const result = await Promise.race([messagePromise, timeoutPromise]) - - if (result) { - t.assert.strictEqual(result[0].data.size, dataSize, 'Large message should be received when limit is disabled') - client.close() - } else { - t.fail('Test timed out waiting for large message') - } + assert.notStrictEqual(result, timeout, 'Test timed out waiting for large message') + assert.strictEqual(result[0].data.size, dataSize, 'Large message should be received when limit is disabled') + client.close() }) test('Fragmented compressed payload over total limit is rejected', async (t) => { @@ -297,8 +297,8 @@ test('Fragmented compressed payload over total limit is rejected', async (t) => await Promise.race([closePromise, timeoutPromise]) - t.assert.strictEqual(messageReceived, false, 'Fragmented compressed message over total limit should be rejected') - t.assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') + assert.strictEqual(messageReceived, false, 'Fragmented compressed message over total limit should be rejected') + assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') }) test('Raw uncompressed payload over immediate limit is rejected', async (t) => { @@ -337,8 +337,8 @@ test('Raw uncompressed payload over immediate limit is rejected', async (t) => { await Promise.race([closePromise, timeoutPromise]) - t.assert.strictEqual(messageReceived, false, 'Raw uncompressed message over limit should be rejected') - t.assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') + assert.strictEqual(messageReceived, false, 'Raw uncompressed message over limit should be rejected') + assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') }) test('Raw uncompressed payload over 16-bit extended limit is rejected', async (t) => { @@ -377,8 +377,8 @@ test('Raw uncompressed payload over 16-bit extended limit is rejected', async (t await Promise.race([closePromise, timeoutPromise]) - t.assert.strictEqual(messageReceived, false, 'Raw uncompressed message over limit should be rejected') - t.assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') + assert.strictEqual(messageReceived, false, 'Raw uncompressed message over limit should be rejected') + assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') }) test('Raw uncompressed payload over 64-bit extended limit is rejected', async (t) => { @@ -417,6 +417,6 @@ test('Raw uncompressed payload over 64-bit extended limit is rejected', async (t await Promise.race([closePromise, timeoutPromise]) - t.assert.strictEqual(messageReceived, false, 'Raw uncompressed message over limit should be rejected') - t.assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') + assert.strictEqual(messageReceived, false, 'Raw uncompressed message over limit should be rejected') + assert.strictEqual(client.readyState, WebSocket.CLOSED, 'Connection should be closed after exceeding limit') })