From 26811a6bce3b3ef4a2a1fc414c1aeef35f0f8518 Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Tue, 15 Sep 2026 08:53:41 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(y-provider)=20prevent=20crash=20on?= =?UTF-8?q?=20malformed=20frames=20from=20rejected=20websockets?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 1 + .../__tests__/collaborationWSHandler.test.ts | 24 --------- .../y-provider/__tests__/hocuspocusWS.test.ts | 49 ++++++++++++++++++- .../src/handlers/collaborationWSHandler.ts | 4 -- .../y-provider/src/servers/appServer.ts | 15 +++++- 5 files changed, 63 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fd49314f..b753d1799 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/frontend/servers/y-provider/__tests__/collaborationWSHandler.test.ts b/src/frontend/servers/y-provider/__tests__/collaborationWSHandler.test.ts index f046c4aed..f70df0a7b 100644 --- a/src/frontend/servers/y-provider/__tests__/collaborationWSHandler.test.ts +++ b/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') diff --git a/src/frontend/servers/y-provider/__tests__/hocuspocusWS.test.ts b/src/frontend/servers/y-provider/__tests__/hocuspocusWS.test.ts index 16d6c929d..ce0b6da17 100644 --- a/src/frontend/servers/y-provider/__tests__/hocuspocusWS.test.ts +++ b/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(); diff --git a/src/frontend/servers/y-provider/src/handlers/collaborationWSHandler.ts b/src/frontend/servers/y-provider/src/handlers/collaborationWSHandler.ts index 633e6be25..8890ad0b4 100644 --- a/src/frontend/servers/y-provider/src/handlers/collaborationWSHandler.ts +++ b/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) { diff --git a/src/frontend/servers/y-provider/src/servers/appServer.ts b/src/frontend/servers/y-provider/src/servers/appServer.ts index 87334cec7..8c23fe2bd 100644 --- a/src/frontend/servers/y-provider/src/servers/appServer.ts +++ b/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);