Repository: nodejs/undici

Problem statement:
websocket: fix ByteParser.run and parseCloseBody
ParseCloseBody can return null. In that case this.#info.closeInfo would be null and the following line would result in a crash.

https://github.com/nodejs/undici/blob/5d5454380b72889706146a9c4d5f65225a99640f/lib/web/websocket/receiver.js#L111


Also reduces the amount of Buffers created.



Hints:
## [Codecov](https://app.codecov.io/gh/nodejs/undici/pull/3080?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nodejs) Report
Attention: Patch coverage is `87.50000%` with `2 lines` in your changes are missing coverage. Please review.
> Project coverage is 94.19%. Comparing base [(`48af032`)](https://app.codecov.io/gh/nodejs/undici/commit/48af0320a35214316b4ecdfaed0194af5d1d13d5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nodejs) to head [(`3def0c3`)](https://app.codecov.io/gh/nodejs/undici/pull/3080?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nodejs).
> Report is 76 commits behind head on main.

| [Files](https://app.codecov.io/gh/nodejs/undici/pull/3080?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nodejs) | Patch % | Lines |
|---|---|---|
| [lib/web/websocket/receiver.js](https://app.codecov.io/gh/nodejs/undici/pull/3080?src=pr&el=tree&filepath=lib%2Fweb%2Fwebsocket%2Freceiver.js&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nodejs#diff-bGliL3dlYi93ZWJzb2NrZXQvcmVjZWl2ZXIuanM=) | 87.50% | [2 Missing :warning: ](https://app.codecov.io/gh/nodejs/undici/pull/3080?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nodejs) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #3080      +/-   ##
==========================================
+ Coverage   94.00%   94.19%   +0.18%     
==========================================
  Files          89       90       +1     
  Lines       24314    24436     +122     
==========================================
+ Hits        22856    23017     +161     
+ Misses       1458     1419      -39     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/nodejs/undici/pull/3080?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nodejs).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nodejs).

tests are rightfully failing, the PR assumes that having no closing code is invalid when it's optional
@KhafraDev 
PTAL
I think there is a bug in the original logic. 
`parseCloseBody` is returning in two places null. This will actually result in code breaking. 
@KhafraDev 

What do you think now about this?
I'm going to go ahead and close this. The main change is creating a singular less Buffer in a very cold case. As lpinca said in my original PR implementing websocket (paraphrasing): "the performance of the opening handshake doesn't matter because it rarely happens". It's the same for closing the connection.
I disagree. At first it was about a performance improvement. Then I found a bug when I implemented this PR, which could lead to a crash.
There isn't a bug being fixed without a test to cover it. The test that you added passes in main. The logic is mostly flawed as I pointed out.
well, lets see if i can write a rogue websocket server.
@KhafraDev 
added unit test ;)
This diff makes that test pass. There is quite a lot of extraneous stuff here.

```diff
diff --git a/lib/web/websocket/receiver.js b/lib/web/websocket/receiver.js
index 4b35ceb5..b1ef4720 100644
--- a/lib/web/websocket/receiver.js
+++ b/lib/web/websocket/receiver.js
@@ -316,7 +316,7 @@ class ByteParser extends Writable {
     try {
       reason = utf8Decode(reason)
     } catch {
-      return null
+      reason = undefined
     }

     return { code, reason }
```
I'm not sure if ignoring the reason is correct though. Maybe failing the connection (basically, force closing it) is a better idea. In that case, nothing would need to be changed in parseCloseBody.

edit: Yeah, I don't think the test case makes sense. We're receiving an invalid frame, why are we handling it gracefully?
same test against main:
![image](https://github.com/nodejs/undici/assets/5059100/2b42025c-6f75-44a1-8a3b-7aacb56fb051)
I understand, but the way this PR is fixing this is wrong.
```
1007

      1007 indicates that an endpoint is terminating the connection
      because it has received data within a message that was not
      consistent with the type of the message (e.g., non-UTF-8 [[RFC3629]]
      data within a text message).
```


Patch:
diff --git a/lib/web/websocket/connection.js b/lib/web/websocket/connection.js
--- a/lib/web/websocket/connection.js
+++ b/lib/web/websocket/connection.js
@@ -1,13 +1,14 @@
 'use strict'
 
-const { uid, states, sentCloseFrameState } = require('./constants')
+const { uid, states, sentCloseFrameState, emptyBuffer, opcodes } = require('./constants')
 const {
   kReadyState,
   kSentClose,
   kByteParser,
-  kReceivedClose
+  kReceivedClose,
+  kResponse
 } = require('./symbols')
-const { fireEvent, failWebsocketConnection } = require('./util')
+const { fireEvent, failWebsocketConnection, isClosing, isClosed, isEstablished } = require('./util')
 const { channels } = require('../../core/diagnostics')
 const { CloseEvent } = require('./events')
 const { makeRequest } = require('../fetch/request')
@@ -15,6 +16,7 @@ const { fetching } = require('../fetch/index')
 const { Headers } = require('../fetch/headers')
 const { getDecodeSplit } = require('../fetch/util')
 const { kHeadersList } = require('../../core/symbols')
+const { WebsocketFrameSend } = require('./frame')
 
 /** @type {import('crypto')} */
 let crypto
@@ -211,6 +213,72 @@ function establishWebSocketConnection (url, protocols, ws, onEstablish, options)
   return controller
 }
 
+function closeWebSocketConnection (ws, code, reason, reasonByteLength) {
+  if (isClosing(ws) || isClosed(ws)) {
+    // If this's ready state is CLOSING (2) or CLOSED (3)
+    // Do nothing.
+  } else if (!isEstablished(ws)) {
+    // If the WebSocket connection is not yet established
+    // Fail the WebSocket connection and set this's ready state
+    // to CLOSING (2).
+    failWebsocketConnection(ws, 'Connection was closed before it was established.')
+    ws[kReadyState] = states.CLOSING
+  } else if (ws[kSentClose] === sentCloseFrameState.NOT_SENT) {
+    // If the WebSocket closing handshake has not yet been started
+    // Start the WebSocket closing handshake and set this's ready
+    // state to CLOSING (2).
+    // - If neither code nor reason is present, the WebSocket Close
+    //   message must not have a body.
+    // - If code is present, then the status code to use in the
+    //   WebSocket Close message must be the integer given by code.
+    // - If reason is also present, then reasonBytes must be
+    //   provided in the Close message after the status code.
+
+    ws[kSentClose] = sentCloseFrameState.PROCESSING
+
+    const frame = new WebsocketFrameSend()
+
+    // If neither code nor reason is present, the WebSocket Close
+    // message must not have a body.
+
+    // If code is present, then the status code to use in the
+    // WebSocket Close message must be the integer given by code.
+    if (code !== undefined && reason === undefined) {
+      frame.frameData = Buffer.allocUnsafe(2)
+      frame.frameData.writeUInt16BE(code, 0)
+    } else if (code !== undefined && reason !== undefined) {
+      // If reason is also present, then reasonBytes must be
+      // provided in the Close message after the status code.
+      frame.frameData = Buffer.allocUnsafe(2 + reasonByteLength)
+      frame.frameData.writeUInt16BE(code, 0)
+      // the body MAY contain UTF-8-encoded data with value /reason/
+      frame.frameData.write(reason, 2, 'utf-8')
+    } else {
+      frame.frameData = emptyBuffer
+    }
+
+    /** @type {import('stream').Duplex} */
+    const socket = ws[kResponse].socket
+
+    socket.write(frame.createFrame(opcodes.CLOSE), (err) => {
+      if (!err) {
+        ws[kSentClose] = sentCloseFrameState.SENT
+      }
+    })
+
+    ws[kSentClose] = sentCloseFrameState.PROCESSING
+
+    // Upon either sending or receiving a Close control frame, it is said
+    // that _The WebSocket Closing Handshake is Started_ and that the
+    // WebSocket connection is in the CLOSING state.
+    ws[kReadyState] = states.CLOSING
+  } else {
+    // Otherwise
+    // Set this's ready state to CLOSING (2).
+    ws[kReadyState] = states.CLOSING
+  }
+}
+
 /**
  * @param {Buffer} chunk
  */
@@ -237,10 +305,10 @@ function onSocketClose () {
 
   const result = ws[kByteParser].closingInfo
 
-  if (result) {
+  if (result && !result.error) {
     code = result.code ?? 1005
     reason = result.reason
-  } else if (ws[kSentClose] !== sentCloseFrameState.SENT) {
+  } else if (!ws[kReceivedClose]) {
     // If _The WebSocket
     // Connection is Closed_ and no Close control frame was received by the
     // endpoint (such as could occur if the underlying transport connection
@@ -293,5 +361,6 @@ function onSocketError (error) {
 }
 
 module.exports = {
-  establishWebSocketConnection
+  establishWebSocketConnection,
+  closeWebSocketConnection
 }
diff --git a/lib/web/websocket/receiver.js b/lib/web/websocket/receiver.js
--- a/lib/web/websocket/receiver.js
+++ b/lib/web/websocket/receiver.js
@@ -6,6 +6,7 @@ const { kReadyState, kSentClose, kResponse, kReceivedClose } = require('./symbol
 const { channels } = require('../../core/diagnostics')
 const { isValidStatusCode, failWebsocketConnection, websocketMessageReceived, utf8Decode } = require('./util')
 const { WebsocketFrameSend } = require('./frame')
+const { CloseEvent } = require('./events')
 
 // This code was influenced by ws released under the MIT license.
 // Copyright (c) 2011 Einar Otto Stangvik <einaros@gmail.com>
@@ -55,6 +56,12 @@ class ByteParser extends Writable {
 
         this.#info.fin = (buffer[0] & 0x80) !== 0
         this.#info.opcode = buffer[0] & 0x0F
+        this.#info.masked = (buffer[1] & 0x80) === 0x80
+
+        if (this.#info.masked) {
+          failWebsocketConnection(this.ws, 'Frame cannot be masked')
+          return callback()
+        }
 
         // If we receive a fragmented message, we use the type of the first
         // frame to parse the full message as binary/text, when it's terminated
@@ -102,6 +109,13 @@ class ByteParser extends Writable {
 
           this.#info.closeInfo = this.parseCloseBody(body)
 
+          if (this.#info.closeInfo.error) {
+            const { code, reason } = this.#info.closeInfo
+
+            callback(new CloseEvent('close', { wasClean: false, reason, code }))
+            return
+          }
+
           if (this.ws[kSentClose] !== sentCloseFrameState.SENT) {
             // If an endpoint receives a Close frame and did not previously send a
             // Close frame, the endpoint MUST send a Close frame in response.  (When
@@ -310,16 +324,16 @@ class ByteParser extends Writable {
     }
 
     if (code !== undefined && !isValidStatusCode(code)) {
-      return null
+      return { code: 1002, reason: 'Invalid status code', error: true }
     }
 
     try {
       reason = utf8Decode(reason)
     } catch {
-      return null
+      return { code: 1007, reason: 'Invalid UTF-8', error: true }
     }
 
-    return { code, reason }
+    return { code, reason, error: false }
   }
 
   get closingInfo () {
diff --git a/lib/web/websocket/util.js b/lib/web/websocket/util.js
--- a/lib/web/websocket/util.js
+++ b/lib/web/websocket/util.js
@@ -197,7 +197,8 @@ function failWebsocketConnection (ws, reason) {
   if (reason) {
     // TODO: process.nextTick
     fireEvent('error', ws, (type, init) => new ErrorEvent(type, init), {
-      error: new Error(reason)
+      error: new Error(reason),
+      message: reason
     })
   }
 }
diff --git a/lib/web/websocket/websocket.js b/lib/web/websocket/websocket.js
--- a/lib/web/websocket/websocket.js
+++ b/lib/web/websocket/websocket.js
@@ -3,7 +3,7 @@
 const { webidl } = require('../fetch/webidl')
 const { URLSerializer } = require('../fetch/data-url')
 const { getGlobalOrigin } = require('../fetch/global')
-const { staticPropertyDescriptors, states, sentCloseFrameState, opcodes, emptyBuffer } = require('./constants')
+const { staticPropertyDescriptors, states, sentCloseFrameState, opcodes } = require('./constants')
 const {
   kWebSocketURL,
   kReadyState,
@@ -16,18 +16,17 @@ const {
 const {
   isConnecting,
   isEstablished,
-  isClosed,
   isClosing,
   isValidSubprotocol,
-  failWebsocketConnection,
   fireEvent
 } = require('./util')
-const { establishWebSocketConnection } = require('./connection')
+const { establishWebSocketConnection, closeWebSocketConnection } = require('./connection')
 const { WebsocketFrameSend } = require('./frame')
 const { ByteParser } = require('./receiver')
 const { kEnumerableProperty, isBlobLike } = require('../../core/util')
 const { getGlobalDispatcher } = require('../../global')
 const { types } = require('node:util')
+const { ErrorEvent } = require('./events')
 
 let experimentalWarned = false
 
@@ -197,67 +196,7 @@ class WebSocket extends EventTarget {
     }
 
     // 3. Run the first matching steps from the following list:
-    if (isClosing(this) || isClosed(this)) {
-      // If this's ready state is CLOSING (2) or CLOSED (3)
-      // Do nothing.
-    } else if (!isEstablished(this)) {
-      // If the WebSocket connection is not yet established
-      // Fail the WebSocket connection and set this's ready state
-      // to CLOSING (2).
-      failWebsocketConnection(this, 'Connection was closed before it was established.')
-      this[kReadyState] = WebSocket.CLOSING
-    } else if (this[kSentClose] === sentCloseFrameState.NOT_SENT) {
-      // If the WebSocket closing handshake has not yet been started
-      // Start the WebSocket closing handshake and set this's ready
-      // state to CLOSING (2).
-      // - If neither code nor reason is present, the WebSocket Close
-      //   message must not have a body.
-      // - If code is present, then the status code to use in the
-      //   WebSocket Close message must be the integer given by code.
-      // - If reason is also present, then reasonBytes must be
-      //   provided in the Close message after the status code.
-
-      this[kSentClose] = sentCloseFrameState.PROCESSING
-
-      const frame = new WebsocketFrameSend()
-
-      // If neither code nor reason is present, the WebSocket Close
-      // message must not have a body.
-
-      // If code is present, then the status code to use in the
-      // WebSocket Close message must be the integer given by code.
-      if (code !== undefined && reason === undefined) {
-        frame.frameData = Buffer.allocUnsafe(2)
-        frame.frameData.writeUInt16BE(code, 0)
-      } else if (code !== undefined && reason !== undefined) {
-        // If reason is also present, then reasonBytes must be
-        // provided in the Close message after the status code.
-        frame.frameData = Buffer.allocUnsafe(2 + reasonByteLength)
-        frame.frameData.writeUInt16BE(code, 0)
-        // the body MAY contain UTF-8-encoded data with value /reason/
-        frame.frameData.write(reason, 2, 'utf-8')
-      } else {
-        frame.frameData = emptyBuffer
-      }
-
-      /** @type {import('stream').Duplex} */
-      const socket = this[kResponse].socket
-
-      socket.write(frame.createFrame(opcodes.CLOSE), (err) => {
-        if (!err) {
-          this[kSentClose] = sentCloseFrameState.SENT
-        }
-      })
-
-      // Upon either sending or receiving a Close control frame, it is said
-      // that _The WebSocket Closing Handshake is Started_ and that the
-      // WebSocket connection is in the CLOSING state.
-      this[kReadyState] = states.CLOSING
-    } else {
-      // Otherwise
-      // Set this's ready state to CLOSING (2).
-      this[kReadyState] = WebSocket.CLOSING
-    }
+    closeWebSocketConnection(this, code, reason, reasonByteLength)
   }
 
   /**
@@ -521,9 +460,8 @@ class WebSocket extends EventTarget {
     this[kResponse] = response
 
     const parser = new ByteParser(this)
-    parser.on('drain', function onParserDrain () {
-      this.ws[kResponse].socket.resume()
-    })
+    parser.on('drain', onParserDrain)
+    parser.on('error', onParserError.bind(this))
 
     response.socket.ws = this
     this[kByteParser] = parser
@@ -647,6 +585,16 @@ webidl.converters.WebSocketSendData = function (V) {
   return webidl.converters.USVString(V)
 }
 
+function onParserDrain () {
+  this.ws[kResponse].socket.resume()
+}
+
+function onParserError (err) {
+  fireEvent('error', this, () => new ErrorEvent('error', { error: err, message: err.reason }))
+
+  closeWebSocketConnection(this, err.code)
+}
+
 module.exports = {
   WebSocket
 }


Test patch:
diff --git a/test/websocket/client-received-masked-frame.js b/test/websocket/client-received-masked-frame.js
new file mode 100644
--- /dev/null
+++ b/test/websocket/client-received-masked-frame.js
@@ -0,0 +1,45 @@
+'use strict'
+
+const { test } = require('node:test')
+const { once } = require('node:events')
+const { WebSocketServer } = require('ws')
+const { WebSocket } = require('../..')
+const { tspl } = require('@matteo.collina/tspl')
+const { WebsocketFrameSend } = require('../../lib/web/websocket/frame')
+
+test('Client fails the connection if receiving a masked frame', async (t) => {
+  const assert = tspl(t, { plan: 2 })
+
+  const body = Buffer.allocUnsafe(2)
+  body.writeUInt16BE(1006, 0)
+
+  const frame = new WebsocketFrameSend(body)
+  const buffer = frame.createFrame(0x8)
+
+  const server = new WebSocketServer({ port: 0 })
+
+  server.on('connection', (ws) => {
+    const socket = ws._socket
+
+    socket.write(buffer, () => ws.close())
+  })
+
+  const ws = new WebSocket(`ws://localhost:${server.address().port}`)
+
+  ws.addEventListener('close', (e) => {
+    assert.deepStrictEqual(e.code, 1006)
+  })
+
+  ws.addEventListener('error', () => {
+    assert.ok(true)
+  })
+
+  t.after(() => {
+    server.close()
+    ws.close()
+  })
+
+  await once(ws, 'close')
+
+  await assert.completed
+})
diff --git a/test/websocket/close-invalid-status-code.js b/test/websocket/close-invalid-status-code.js
new file mode 100644
--- /dev/null
+++ b/test/websocket/close-invalid-status-code.js
@@ -0,0 +1,39 @@
+'use strict'
+
+const { test } = require('node:test')
+const { once } = require('node:events')
+const { WebSocketServer } = require('ws')
+const { WebSocket } = require('../..')
+const { tspl } = require('@matteo.collina/tspl')
+
+test('Client fails the connection if receiving a masked frame', async (t) => {
+  const assert = tspl(t, { plan: 2 })
+
+  const server = new WebSocketServer({ port: 0 })
+
+  server.on('connection', (ws) => {
+    const socket = ws._socket
+
+    // 1006 status code
+    socket.write(Buffer.from([0x88, 0x02, 0x03, 0xee]), () => ws.close())
+  })
+
+  const ws = new WebSocket(`ws://localhost:${server.address().port}`)
+
+  ws.addEventListener('close', (e) => {
+    assert.deepStrictEqual(e.code, 1006)
+  })
+
+  ws.addEventListener('error', () => {
+    assert.ok(true)
+  })
+
+  t.after(() => {
+    server.close()
+    ws.close()
+  })
+
+  await once(ws, 'close')
+
+  await assert.completed
+})
diff --git a/test/websocket/close-invalid-utf-8.js b/test/websocket/close-invalid-utf-8.js
new file mode 100644
--- /dev/null
+++ b/test/websocket/close-invalid-utf-8.js
@@ -0,0 +1,49 @@
+'use strict'
+
+const { test } = require('node:test')
+const { once } = require('node:events')
+const { WebSocketServer } = require('ws')
+const { WebSocket } = require('../..')
+const { tspl } = require('@matteo.collina/tspl')
+
+test('Receiving a close frame with invalid utf-8', async (t) => {
+  const assert = tspl(t, { plan: 2 })
+
+  const server = new WebSocketServer({ port: 0 })
+
+  server.on('connection', (ws) => {
+    ws.close(1000, Buffer.from([0xFF, 0xFE]))
+
+    ws.on('close', (code) => {
+      assert.equal(code, 1007)
+    })
+  })
+
+  const events = []
+  const ws = new WebSocket(`ws://localhost:${server.address().port}`)
+
+  ws.addEventListener('close', (e) => {
+    events.push({ type: 'close', code: e.code })
+  })
+
+  ws.addEventListener('error', () => {
+    events.push({ type: 'error' })
+  })
+
+  t.after(() => {
+    server.close()
+    ws.close()
+  })
+
+  await once(ws, 'close')
+
+  // An error event should be propagated immediately, then we should receive
+  // a close event with a 1006 code. The code is 1006, and not 1007 (as we send
+  // the server) because the connection is closed before the server responds.
+  assert.deepStrictEqual(events, [
+    { type: 'error' },
+    { type: 'close', code: 1006 }
+  ])
+
+  await assert.completed
+})