From ef4180c8da8e21835760917f37acea0be3a6b326 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Thu, 15 Aug 2024 13:50:32 -0400 Subject: [PATCH] (core) Fix unhandledRejection caused by exception from verifyClient. Summary: This includes two fixes: one to ensure that any exception from websocket upgrade handlers are handled (by destroying the socket). A test case is added for this. The other is to ensure verifyClient returns false instead of failing; this should lead to a better error to the client (Forbidden, rather than just socket close). This is only tested manually with a curl request. Test Plan: Added a test case for the more sensitive half of the fix. Reviewers: georgegevoian Reviewed By: georgegevoian Subscribers: georgegevoian Differential Revision: https://phab.getgrist.com/D4323 --- app/server/lib/Comm.ts | 23 +++++++++++++-------- app/server/lib/GristSocketServer.ts | 19 +++++++++++++---- test/server/lib/GristSockets.ts | 32 +++++++++++++++++++++++++---- 3 files changed, 58 insertions(+), 16 deletions(-) diff --git a/app/server/lib/Comm.ts b/app/server/lib/Comm.ts index eb13c632..b414661a 100644 --- a/app/server/lib/Comm.ts +++ b/app/server/lib/Comm.ts @@ -247,15 +247,22 @@ export class Comm extends EventEmitter { for (const server of servers) { const wssi = new GristSocketServer(server, { verifyClient: async (req: http.IncomingMessage) => { - if (this._options.hosts) { - // DocWorker ID (/dw/) and version tag (/v/) may be present in this request but are not - // needed. addOrgInfo assumes req.url starts with /o/ if present. - req.url = parseFirstUrlPart('dw', req.url!).path; - req.url = parseFirstUrlPart('v', req.url).path; - await this._options.hosts.addOrgInfo(req); + try { + if (this._options.hosts) { + // DocWorker ID (/dw/) and version tag (/v/) may be present in this request but are not + // needed. addOrgInfo assumes req.url starts with /o/ if present. + req.url = parseFirstUrlPart('dw', req.url!).path; + req.url = parseFirstUrlPart('v', req.url).path; + await this._options.hosts.addOrgInfo(req); + } + + return trustOrigin(req); + } catch (err) { + // Consider exceptions (e.g. in parsing unexpected hostname) as failures to verify. + // In practice, we only see this happening for spammy/illegitimate traffic; there is + // no particular reason to log these. + return false; } - - return trustOrigin(req); } }); diff --git a/app/server/lib/GristSocketServer.ts b/app/server/lib/GristSocketServer.ts index 5a098f06..b39fc274 100644 --- a/app/server/lib/GristSocketServer.ts +++ b/app/server/lib/GristSocketServer.ts @@ -3,10 +3,13 @@ import * as WS from 'ws'; import * as EIO from 'engine.io'; import {GristServerSocket, GristServerSocketEIO, GristServerSocketWS} from './GristServerSocket'; import * as net from 'net'; +import * as stream from 'stream'; const MAX_PAYLOAD = 100e6; export interface GristSocketServerOptions { + // Check if this request should be accepted. To produce a valid response (perhaps a rejection), + // this callback should not throw. verifyClient?: (request: http.IncomingMessage) => Promise; } @@ -64,7 +67,15 @@ export class GristSocketServer { private _attach(server: http.Server) { // Forward all WebSocket upgrade requests to WS - server.on('upgrade', async (request, socket, head) => { + + // Wrapper for server event handlers that catches rejected promises, which would otherwise + // lead to "unhandledRejection" and process exit. Instead we abort the connection, which helps + // in testing this scenario. This is a fallback; in reality, handlers should never throw. + function destroyOnRejection(socket: stream.Duplex, func: () => Promise) { + func().catch(e => { socket.destroy(); }); + } + + server.on('upgrade', (request, socket, head) => destroyOnRejection(socket, async () => { if (this._options?.verifyClient && !await this._options.verifyClient(request)) { // Because we are handling an "upgrade" event, we don't have access to // a "response" object, just the raw socket. We can still construct @@ -76,14 +87,14 @@ export class GristSocketServer { this._wsServer.handleUpgrade(request, socket as net.Socket, head, (client) => { this._connectionHandler?.(new GristServerSocketWS(client), request); }); - }); + })); // At this point an Express app is installed as the handler for the server's // "request" event. We need to install our own listener instead, to intercept // requests that are meant for the Engine.IO polling implementation. const listeners = [...server.listeners("request")]; server.removeAllListeners("request"); - server.on("request", async (req, res) => { + server.on("request", (req, res) => destroyOnRejection(req.socket, async() => { // Intercept requests that have transport=polling in their querystring if (/[&?]transport=polling(&|$)/.test(req.url ?? '')) { if (this._options?.verifyClient && !await this._options.verifyClient(req)) { @@ -98,7 +109,7 @@ export class GristSocketServer { listener.call(server, req, res); } } - }); + })); server.on("close", this.close.bind(this)); } diff --git a/test/server/lib/GristSockets.ts b/test/server/lib/GristSockets.ts index f08b8214..f1e8725d 100644 --- a/test/server/lib/GristSockets.ts +++ b/test/server/lib/GristSockets.ts @@ -1,7 +1,7 @@ import { assert } from 'chai'; import * as http from 'http'; import { GristClientSocket } from 'app/client/components/GristClientSocket'; -import { GristSocketServer } from 'app/server/lib/GristSocketServer'; +import { GristSocketServer, GristSocketServerOptions } from 'app/server/lib/GristSocketServer'; import { fromCallback, listenPromise } from 'app/server/lib/serverUtils'; import { AddressInfo } from 'net'; import httpProxy from 'http-proxy'; @@ -29,9 +29,9 @@ describe(`GristSockets`, function () { await stopSocketServer(); }); - async function startSocketServer() { + async function startSocketServer(options?: GristSocketServerOptions) { server = http.createServer((req, res) => res.writeHead(404).end()); - socketServer = new GristSocketServer(server); + socketServer = new GristSocketServer(server, options); await listenPromise(server.listen(0, 'localhost')); serverPort = (server.address() as AddressInfo).port; } @@ -165,6 +165,30 @@ describe(`GristSockets`, function () { await closePromise; }); + it("should fail gracefully if verifyClient throws exception", async function () { + + // Restart servers with a failing verifyClient method. + await stopProxyServer(); + await stopSocketServer(); + await startSocketServer({verifyClient: () => { throw new Error("Test error from verifyClient"); }}); + await startProxyServer(); + + // Check whether we are getting an unhandledRejection. + let rejection: unknown = null; + const onUnhandledRejection = (err: unknown) => { rejection = err; }; + process.on('unhandledRejection', onUnhandledRejection); + + try { + // The "poll error" comes from the fallback to polling. + await assert.isRejected(connectClient(wsAddress), /poll error/); + } finally { + // Typings for process.removeListener are broken, possibly by electron's presence + // (https://github.com/electron/electron/issues/9626). + process.removeListener('unhandledRejection' as any, onUnhandledRejection); + } + // The important thing is that we don't get unhandledRejection. + assert.equal(rejection, null); + }); }); } -}); \ No newline at end of file +});