search

graasp / graasp-plugin-websockets

A websockets extension for Graasp exposed through a Fastify plugin
GNU Affero General Public License v3.0
0 stars 0 forks source link

Fix a bug where connections to the ws endpoint without a valid session would crash the server #3

Closed codeofmochi closed 3 years ago

codeofmochi commented 3 years ago

When running the graasp server with the graasp-websockets plugin, accessing the websocket endpoint crashes the server if accessed using a websocket connection without a valid sesion.

If accessed through HTTP, no error is raised and the server doesn't crash. If accessed through WS, the whole process crashes with error:

graasp-websockets: an error occured: Error: no valid session
[Node]  Destroying connection [object Object]...
[Node] events.js:353
[Node]       throw er; // Unhandled 'error' event
[Node]       ^
[Node] 
[Node] Error: no valid session
[Node]     at Object.<anonymous> (/home/momochan/dev/epfl/graasp/dist/plugins/auth/auth.js:45:23)
[Node]     at Generator.next (<anonymous>)
[Node]     at /home/momochan/dev/epfl/graasp/dist/plugins/auth/auth.js:8:71
[Node]     at new Promise (<anonymous>)
[Node]     at __awaiter (/home/momochan/dev/epfl/graasp/dist/plugins/auth/auth.js:4:12)
[Node]     at Object.validateSession (/home/momochan/dev/epfl/graasp/dist/plugins/auth/auth.js:40:16)
[Node]     at hookIterator (/home/momochan/dev/epfl/graasp/node_modules/fastify/lib/hooks.js:237:10)
[Node]     at next (/home/momochan/dev/epfl/graasp/node_modules/fastify/lib/hooks.js:164:16)
[Node]     at hookRunner (/home/momochan/dev/epfl/graasp/node_modules/fastify/lib/hooks.js:187:3)
[Node]     at preValidationCallback (/home/momochan/dev/epfl/graasp/node_modules/fastify/lib/handleRequest.js:99:5)
[Node]     at handler (/home/momochan/dev/epfl/graasp/node_modules/fastify/lib/handleRequest.js:70:7)
[Node]     at handleRequest (/home/momochan/dev/epfl/graasp/node_modules/fastify/lib/handleRequest.js:18:5)
[Node]     at runPreParsing (/home/momochan/dev/epfl/graasp/node_modules/fastify/lib/route.js:421:5)
[Node]     at next (/home/momochan/dev/epfl/graasp/node_modules/fastify/lib/hooks.js:158:7)
[Node]     at internalNext (/home/momochan/dev/epfl/graasp/node_modules/helmet/dist/index.js:86:6)
[Node]     at xXssProtectionMiddleware (/home/momochan/dev/epfl/graasp/node_modules/helmet/dist/middlewares/x-xss-protection/index.js:6:3)
[Node] Emitted 'error' event on Duplex instance at:
[Node]     at Duplex.duplexOnError (/home/momochan/dev/epfl/graasp/node_modules/ws/lib/stream.js:37:10)
[Node]     at Duplex.emit (events.js:376:20)
[Node]     at Duplex.emit (domain.js:470:12)
[Node]     at emitErrorNT (internal/streams/destroy.js:106:8)
[Node]     at emitErrorCloseNT (internal/streams/destroy.js:74:3)
[Node]     at processTicksAndRejections (internal/process/task_queues.js:82:21)

Thus the error is probably in the errorHandler passed here: https://github.com/graasp/graasp-websockets/blob/4a55f781d29f49c0ff5766052d7f27f8fcc53f81/src/service-api.ts#L53-L58

codeofmochi commented 3 years ago

Indeed, the bug was caused by conn.destroy(error).

The Stream.Readable API (https://nodejs.org/api/stream.html#stream_readable_destroy_error) raises the passed error optionally. We don't need the error to be processed by the stream itself, so we should just destroy the connection without passing the error.

Hence fix is https://github.com/graasp/graasp-websockets/commit/16670d7339a19ec407367955a6c668548b950344