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