fix: validate EOF for chunked h1 responses

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.')
     }

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()

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)')
+  }
+})

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)
 })