Ensure all API endpoints are safe against uncaught promise rejections

davidje13 · Dec 19, 2024 · 406793a · 406793a
1 parent 2debac1
commit 406793a
Show file tree

Hide file tree

Showing 7 changed files with 179 additions and 148 deletions.
diff --git a/backend/src/helpers/routeHelpers.ts b/backend/src/helpers/routeHelpers.ts
@@ -1,11 +1,15 @@
 import type { RequestHandler } from 'express';
 
-export function safe<H extends RequestHandler>(handler: H): H {
-  return (async (req, res, next) => {
+// express 4.x does not handle asynchronous errors - if an asynchronous operation fails, it will crash the server
+// we defend against this by wrapping all async handlers in this function, which mimics the behaviour of express 5.x
+export function safe<P = unknown>(
+  handler: RequestHandler<P>,
+): RequestHandler<P> {
+  return async (req, res, next) => {
     try {
       await handler(req, res, next);
     } catch (e) {
       next(e);
     }
-  }) as H;
+  };
 }
diff --git a/backend/src/routers/ApiAuthRouter.ts b/backend/src/routers/ApiAuthRouter.ts
@@ -2,6 +2,7 @@ import { WebSocketExpress, Router, type JWTPayload } from 'websocket-express';
 import { type RetroAuthService } from '../services/RetroAuthService';
 import { type UserAuthService } from '../services/UserAuthService';
 import { type RetroService } from '../services/RetroService';
+import { safe } from '../helpers/routeHelpers';
 
 const JSON_BODY = WebSocketExpress.json({ limit: 4 * 1024 });
 
@@ -18,41 +19,49 @@ export class ApiAuthRouter extends Router {
       (token): JWTPayload | null => userAuthService.readAndVerifyToken(token),
     );
 
-    this.get('/tokens/:retroId/user', userAuthMiddleware, async (req, res) => {
-      const userId = WebSocketExpress.getAuthData(res).sub!;
-      const { retroId } = req.params;
-
-      if (
-        !retroId ||
-        !(await retroService.isRetroOwnedByUser(retroId, userId))
-      ) {
-        res.status(403).json({ error: 'not retro owner' });
-        return;
-      }
-
-      const retroToken = await retroAuthService.grantOwnerToken(retroId);
-      if (!retroToken) {
-        res.status(500).json({ error: 'retro not found' });
-        return;
-      }
-
-      res.status(200).json({ retroToken });
-    });
-
-    this.post('/tokens/:retroId', JSON_BODY, async (req, res) => {
-      const { retroId } = req.params;
-      const { password } = req.body;
-
-      const retroToken = await retroAuthService.grantForPassword(
-        retroId,
-        password,
-      );
-      if (!retroToken) {
-        res.status(400).json({ error: 'incorrect password' });
-        return;
-      }
-
-      res.status(200).json({ retroToken });
-    });
+    this.get(
+      '/tokens/:retroId/user',
+      userAuthMiddleware,
+      safe<{ retroId: string }>(async (req, res) => {
+        const userId = WebSocketExpress.getAuthData(res).sub!;
+        const { retroId } = req.params;
+
+        if (
+          !retroId ||
+          !(await retroService.isRetroOwnedByUser(retroId, userId))
+        ) {
+          res.status(403).json({ error: 'not retro owner' });
+          return;
+        }
+
+        const retroToken = await retroAuthService.grantOwnerToken(retroId);
+        if (!retroToken) {
+          res.status(500).json({ error: 'retro not found' });
+          return;
+        }
+
+        res.status(200).json({ retroToken });
+      }),
+    );
+
+    this.post(
+      '/tokens/:retroId',
+      JSON_BODY,
+      safe<{ retroId: string }>(async (req, res) => {
+        const { retroId } = req.params;
+        const { password } = req.body;
+
+        const retroToken = await retroAuthService.grantForPassword(
+          retroId,
+          password,
+        );
+        if (!retroToken) {
+          res.status(400).json({ error: 'incorrect password' });
+          return;
+        }
+
+        res.status(200).json({ retroToken });
+      }),
+    );
   }
 }
diff --git a/backend/src/routers/ApiGiphyRouter.ts b/backend/src/routers/ApiGiphyRouter.ts
@@ -1,32 +1,36 @@
 import { Router } from 'websocket-express';
 import { type GiphyService } from '../services/GiphyService';
 import { logError } from '../log';
+import { safe } from '../helpers/routeHelpers';
 
 export class ApiGiphyRouter extends Router {
   public constructor(service: GiphyService) {
     super();
 
-    this.get('/search', async (req, res) => {
-      const { q, lang = 'en' } = req.query;
+    this.get(
+      '/search',
+      safe(async (req, res) => {
+        const { q, lang = 'en' } = req.query;
 
-      if (typeof q !== 'string' || !q) {
-        res.status(400).json({ error: 'Bad request' });
-        return;
-      }
+        if (typeof q !== 'string' || !q) {
+          res.status(400).json({ error: 'Bad request' });
+          return;
+        }
 
-      if (typeof lang !== 'string') {
-        res.status(400).json({ error: 'Bad request' });
-        return;
-      }
+        if (typeof lang !== 'string') {
+          res.status(400).json({ error: 'Bad request' });
+          return;
+        }
 
-      try {
-        const gifs = await service.search(q, 10, lang);
+        try {
+          const gifs = await service.search(q, 10, lang);
 
-        res.json({ gifs });
-      } catch (err) {
-        logError('Giphy proxy error', err);
-        res.status(500).json({ error: 'Proxy error' });
-      }
-    });
+          res.json({ gifs });
+        } catch (err) {
+          logError('Giphy proxy error', err);
+          res.status(500).json({ error: 'Proxy error' });
+        }
+      }),
+    );
   }
 }
diff --git a/backend/src/routers/ApiPasswordCheckRouter.ts b/backend/src/routers/ApiPasswordCheckRouter.ts
@@ -1,6 +1,7 @@
 import { Router } from 'websocket-express';
 import { type PasswordCheckService } from '../services/PasswordCheckService';
 import { logError } from '../log';
+import { safe } from '../helpers/routeHelpers';
 
 const VALID_RANGE = /^[0-9A-Z]{5}$/;
 
@@ -15,32 +16,35 @@ export class ApiPasswordCheckRouter extends Router {
   public constructor(service: PasswordCheckService) {
     super();
 
-    this.get('/:range', async (req, res) => {
-      const { range } = req.params;
+    this.get(
+      '/:range',
+      safe<{ range: string }>(async (req, res) => {
+        const { range } = req.params;
 
-      if (!VALID_RANGE.test(range)) {
-        res.status(400).end();
-      }
-
-      try {
-        const data = await service.getBreachesRange(range);
-        res.header('cache-control', CACHE_CONTROL);
-        res.removeHeader('expires');
-        res.removeHeader('pragma');
-        res.end(data);
-      } catch (err) {
-        if (err instanceof Error && err.message === 'Invalid range prefix') {
+        if (!VALID_RANGE.test(range)) {
           res.status(400).end();
-        } else if (
-          err instanceof Error &&
-          err.message === 'Service unavailable'
-        ) {
-          res.status(503).end();
-        } else {
-          logError('Password breaches lookup error', err);
-          res.status(500).end();
         }
-      }
-    });
+
+        try {
+          const data = await service.getBreachesRange(range);
+          res.header('cache-control', CACHE_CONTROL);
+          res.removeHeader('expires');
+          res.removeHeader('pragma');
+          res.end(data);
+        } catch (err) {
+          if (err instanceof Error && err.message === 'Invalid range prefix') {
+            res.status(400).end();
+          } else if (
+            err instanceof Error &&
+            err.message === 'Service unavailable'
+          ) {
+            res.status(503).end();
+          } else {
+            logError('Password breaches lookup error', err);
+            res.status(500).end();
+          }
+        }
+      }),
+    );
   }
 }
diff --git a/backend/src/routers/ApiRetroArchivesRouter.ts b/backend/src/routers/ApiRetroArchivesRouter.ts
@@ -2,6 +2,7 @@ import { WebSocketExpress, Router } from 'websocket-express';
 import { type RetroArchiveService } from '../services/RetroArchiveService';
 import { extractRetroData } from '../helpers/jsonParsers';
 import { logError } from '../log';
+import { safe } from '../helpers/routeHelpers';
 
 const JSON_BODY = WebSocketExpress.json({ limit: 512 * 1024 });
 
@@ -12,20 +13,20 @@ export class ApiRetroArchivesRouter extends Router {
     this.get(
       '/',
       WebSocketExpress.requireAuthScope('readArchives'),
-      async (req, res) => {
+      safe<{ retroId: string }>(async (req, res) => {
         const { retroId } = req.params;
 
         const archives =
           await retroArchiveService.getRetroArchiveSummaries(retroId);
         res.json({ archives });
-      },
+      }),
     );
 
     this.post(
       '/',
       WebSocketExpress.requireAuthScope('write'),
       JSON_BODY,
-      async (req, res) => {
+      safe<{ retroId: string }>(async (req, res) => {
         try {
           const { retroId } = req.params;
           const data = extractRetroData(req.body);
@@ -46,13 +47,13 @@ export class ApiRetroArchivesRouter extends Router {
             res.status(400).json({ error: e.message });
           }
         }
-      },
+      }),
     );
 
     this.get(
       '/:archiveId',
       WebSocketExpress.requireAuthScope('readArchives'),
-      async (req, res) => {
+      safe<{ retroId: string; archiveId: string }>(async (req, res) => {
         const { retroId, archiveId } = req.params;
 
         const archive = await retroArchiveService.getRetroArchive(
@@ -65,7 +66,7 @@ export class ApiRetroArchivesRouter extends Router {
         } else {
           res.status(404).end();
         }
-      },
+      }),
     );
   }
 }