(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

@@ -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);
+            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;
+          }
         }
       });
 

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<boolean>;
 }
 
@@ -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<void>) {
+      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));
   }

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