Hocuspocus can crash on invalid WS payload (WS_ERR_INVALID_UTF8 or WS_ERR_INVALID_OPCODE)

[jamesopti]

Description
We've started to see this error occasionally again in production that causes the Hocuspocus server to crash on a websocket error. Specifically, the error is:

/app/node_modules/@hocuspocus/server/node_modules/ws/lib/receiver.js:519
return error(
^
Error: Invalid WebSocket frame: invalid UTF-8 sequence
    at Receiver.dataMessage (/app/node_modules/@hocuspocus/server/node_modules/ws/lib/receiver.js:519:18)
    at Receiver.getData (/app/node_modules/@hocuspocus/server/node_modules/ws/lib/receiver.js:446:17)
    at Receiver.startLoop (/app/node_modules/@hocuspocus/server/node_modules/ws/lib/receiver.js:148:22)
    at Receiver._write (/app/node_modules/@hocuspocus/server/node_modules/ws/lib/receiver.js:83:10)
    at writeOrBuffer (node:internal/streams/writable:389:12)
    at _write (node:internal/streams/writable:330:10)
    at Receiver.Writable.write (node:internal/streams/writable:334:10)
    at Socket.socketOnData (/app/node_modules/@hocuspocus/server/node_modules/ws/lib/websocket.js:1231:35)
    at Socket.emit (node:events:390:28)
    at Socket.emit (node:domain:475:12) {
  code: 'WS_ERR_INVALID_UTF8',
  [Symbol(status-code)]: 1007
}

#393 Attempted to fix this but it appears to still be possible. It's unclear to me how to catch this with certainty.

Steps to reproduce the bug
Unfortunately I'm not able to reproduce this yet. I tried sending some invalid websocket payloads to Hocuspocus but those appear to be filtered correctly.

Expected behavior
I would not expect ANY invalid websocket message from a single client to crash the server but rather log an error and close the offending connection.

Environment?

• operating system: NodeJS 16
• hocuspocus version: @hocuspocus/server@npm:1.0.0-beta.2

[janthurau]

hey @jamesopti, I've spent some time here and managed to identify a possible issue:

• We're registering the 'error' handler when setting up the connection, which is after authentication has been cleared (either because it's inactive, or because the appropriate token has been received).
• When, while 'onAuthenticate' hook is running, an invalid message is received the server crashes (because the connection hasn't been set up yet, so the handler is not bound).

I'm working on a fix, I don't have a solution in mind yet but wanted to keep you updated on progress already. To make it more obvious, here's how you can reproduce it:

test('does not crash when utf-8 sequence is sent', async t => {
  await new Promise(resolve => {
    const server = newHocuspocus({
      async onAuthenticate(data: onAuthenticatePayload) {
        return new Promise(resolve => {
          setTimeout(resolve, 2000)
        })
      },
    })

    const provider = newHocuspocusProvider(server, {
      token: 'test123',
      onClose({ event }) {
        t.is(event.code, 1002)
        provider.destroy()
      },
      onDestroy() {
        t.pass()
        resolve(true)
      },
    })

    setInterval(() => {
      // @ts-ignore
      provider.webSocket?._socket.write(Buffer.from([0x00, 0x00])) // eslint-disable-line
    }, 500)

  })
})

// Update: I'd assume it's also possible while onLoadDocument is running (just by looking at our code)

[jamesopti]

@janthurau Thanks for sharing! This does seem like a good find/improvement.

I'd have to thoroughly comb through our logs to see if this makes sense as the culprit for the instance where we saw the crash but nevertheless, this change feels like a good one.

[janthurau]

I've merged this, if you're still seeing this issue once you updated (release will go out soon), feel free to re-open!

Thanks again or reporting this 😎