🐛(y-provider) prevent crash on malformed frames from rejected websockets

When a WebSocket connection is rejected for a missing Origin or cookies,
or hits a route with no matching handler, express-ws still completes the
upgrade handshake and only closes the socket afterwards. The `ws`
library keeps parsing incoming frames during the close handshake, which
can take up to 30 seconds, and no 'error' listener was attached to these
sockets in the meantime. A single malformed frame (e.g. reserved bits
set) made the parser emit an unhandled 'error' event, crashing the whole
process and taking down realtime collaboration for every connected user.

We now attach the error listener on every WebSocket as soon as it is
created, before any routing or middleware runs, so malformed frames are
logged instead of crashing the server.

CHANGELOG.md

@@ -26,6 +26,7 @@ and this project adheres to
 - 🐛(frontend) scroll to the linked block in read-only documents #2663
 - 🐛(frontend) hide the selection highlight on presenter images #2665
 - 🐛(y-provider) prevent process crash on malformed websocket frames #2673
+- 🐛(y-provider) prevent crash on malformed frames from rejected websockets
 - 🐛(frontend) keep commented text sharp when printing to PDF #2674
 - 🐛(docker) pull minio images from quay.io #2675
 - ♿️(frontend) restore presenter focus trapping after share links #2533

src/frontend/servers/y-provider/__tests__/collaborationWSHandler.test.ts

@@ -37,30 +37,6 @@ describe('collaborationWSHandler', () => {
     expect(handleConnectionMock).toHaveBeenCalledWith(ws, req);
   });
 
-  test('does not crash the process when the socket emits an unexpected "error" event', () => {
-    const consoleErrorMock = vi
-      .spyOn(console, 'error')
-      .mockImplementation(() => undefined);
-    const { ws } = createFakeWs();
-
-    collaborationWSHandler(ws, {} as Request);
-
-    const wsError = Object.assign(new Error('Invalid WebSocket frame'), {
-      code: 'WS_ERR_UNEXPECTED_RSV_2_3',
-    });
-
-    // Without an 'error' listener, EventEmitter would throw here and crash
-    // the process - this call must not throw.
-    expect(() => ws.emit('error', wsError)).not.toThrow();
-
-    expect(consoleErrorMock).toHaveBeenCalledWith(
-      'WebSocket connection error:',
-      wsError,
-    );
-
-    consoleErrorMock.mockRestore();
-  });
-
   test('closes the socket and logs if handleConnection throws synchronously', () => {
     const consoleErrorMock = vi
       .spyOn(console, 'error')

src/frontend/servers/y-provider/__tests__/hocuspocusWS.test.ts

@@ -1,4 +1,4 @@
-import { Server } from 'node:net';
+import { Server, Socket } from 'node:net';
 
 import {
   HocuspocusProvider,
@@ -40,6 +40,7 @@ console.log = vi.fn();
 import * as CollaborationBackend from '@/api/collaborationBackend';
 import { COLLABORATION_SERVER_ORIGIN as origin, PORT as port } from '@/env';
 import { promiseDone } from '@/helpers';
+import { routes } from '@/routes';
 import { hocuspocusServer, initApp } from '@/servers';
 
 describe('Server Tests', () => {
@@ -94,6 +95,52 @@ describe('Server Tests', () => {
     return promise;
   });
 
+  [
+    {
+      title: 'rejected for a bad origin',
+      path: routes.COLLABORATION_WS,
+      headers: { Origin: 'http://bad-origin.com' },
+    },
+    {
+      title: 'rejected for missing cookies',
+      path: routes.COLLABORATION_WS,
+      headers: { Origin: origin },
+    },
+    {
+      title: 'on an unknown route',
+      path: '/unknown-route/',
+      headers: { Origin: origin, Cookie: 'docs_sessionid=abc' },
+    },
+  ].forEach(({ title, path, headers }) => {
+    test(`Malformed frame on a WebSocket ${title} does not crash the server`, () => {
+      const { promise, done } = promiseDone();
+      const ws = new WebSocket(
+        `ws://localhost:${port}${path}?room=${uuidv4()}`,
+        { headers },
+      );
+
+      ws.onopen = () => {
+        // Masked text frame with the reserved RSV2 bit set: the server has
+        // already started closing the socket but still reads this frame.
+        (ws as unknown as { _socket: Socket })._socket.write(
+          Buffer.from([0xa1, 0x80, 0x00, 0x00, 0x00, 0x00]),
+        );
+      };
+
+      ws.onclose = () => {
+        expect(console.error).toHaveBeenCalledWith(
+          'WebSocket connection error:',
+          expect.objectContaining({
+            message: expect.stringContaining('RSV2 and RSV3 must be clear'),
+          }),
+        );
+        done();
+      };
+
+      return promise;
+    });
+  });
+
   test('WebSocket connection not allowed if room not matching provider name', () => {
     const { promise, done } = promiseDone();
     const room = uuidv4();

src/frontend/servers/y-provider/src/handlers/collaborationWSHandler.ts

@@ -4,10 +4,6 @@ import * as ws from 'ws';
 import { hocuspocusServer } from '@/servers/hocuspocusServer';
 
 export const collaborationWSHandler = (ws: ws.WebSocket, req: Request) => {
-  ws.on('error', (error) => {
-    console.error('WebSocket connection error:', error);
-  });
-
   try {
     hocuspocusServer.hocuspocus.handleConnection(ws, req);
   } catch (error) {

src/frontend/servers/y-provider/src/servers/appServer.ts

@@ -1,6 +1,7 @@
 import * as Sentry from '@sentry/node';
 import express from 'express';
 import expressWebsockets from 'express-ws';
+import * as ws from 'ws';
 
 import { CONVERSION_FILE_MAX_SIZE } from '@/env';
 import {
@@ -19,7 +20,19 @@ import { logger } from '@/utils';
  * @returns An object containing the Express app, Hocuspocus server, and HTTP server instance.
  */
 export const initApp = () => {
-  const { app } = expressWebsockets(express());
+  const { app, getWss } = expressWebsockets(express());
+
+  /**
+   * Handle socket errors on every WebSocket, before any middleware runs.
+   * Sockets rejected by `wsSecurity` or by express-ws (unknown route) are
+   * closed with a closing handshake and keep reading frames meanwhile: an
+   * unhandled 'error' from a malformed frame would crash the process.
+   */
+  getWss().prependListener('connection', (socket: ws.WebSocket) => {
+    socket.on('error', (error) => {
+      console.error('WebSocket connection error:', error);
+    });
+  });
 
   app.use(corsMiddleware);