fix: fix security issues caused by withProtectedRoutes middleware

src/authentication/protected-routes.handler.ts

@@ -1,71 +1,21 @@
-import AdminJS, { Router as AdminRouter } from "adminjs";
-import { Router } from "express";
-import { convertToExpressRoute } from "../convertRoutes";
-import { pathToRegexp } from "path-to-regexp";
-
-const doesNotRequireAuthentication = (
-  url: string,
-  { loginPath, logoutPath }
-) => {
-  return (
-    isAdminAsset(url) || url.startsWith(loginPath) || url.startsWith(logoutPath)
-  );
-};
+import AdminJS from "adminjs";
+import { Router, RequestHandler } from "express";
 
 export const withProtectedRoutesHandler = (
   router: Router,
   admin: AdminJS
 ): void => {
-  const { rootPath, loginPath, logoutPath } = admin.options;
-
-  router.use((req, res, next) => {
-    if (
-      doesNotRequireAuthentication(req.originalUrl, { loginPath, logoutPath })
-    ) {
-      return next();
-    }
-
-    if (isAdminRoute(req.originalUrl, rootPath)) {
-      if (!!req.session.adminUser) {
-        return next();
-      }
-      return res.redirect(loginPath);
-    }
-
-    return next(); // custom routes in admin router
-  });
-};
-
-export const isAdminRoute = (
-  originalUrl: string,
-  adminRootPath: string
-): boolean => {
-  const adminRoutes = AdminRouter.routes
-    .map((route) => convertToExpressRoute(route.path))
-    .filter((route) => route !== "");
-
-  let urlWithoutAdminRootPath = originalUrl.split("?")[0];
-  if (adminRootPath !== "/") {
-    urlWithoutAdminRootPath = urlWithoutAdminRootPath.replace(
-      adminRootPath,
-      ""
-    );
-    if (!urlWithoutAdminRootPath.startsWith("/")) {
-      urlWithoutAdminRootPath = `/${urlWithoutAdminRootPath}`;
+  const { loginPath } = admin.options;
+  const authorizedRoutesMiddleware: RequestHandler = (
+    request,
+    response,
+    next
+  ) => {
+    if (!request.session || !request.session.adminUser) {
+      return response.redirect(loginPath);
     }
-  }
-
-  const isAdminRootUrl = originalUrl === adminRootPath;
-  const isUrlUnderRootPath = originalUrl.startsWith(adminRootPath);
+    return next();
+  };
 
-  return (
-    isAdminRootUrl ||
-    (adminRoutes.some((route) =>
-      pathToRegexp(route).test(urlWithoutAdminRootPath)
-    ) &&
-      isUrlUnderRootPath)
-  );
+  router.use(authorizedRoutesMiddleware);
 };
-
-const isAdminAsset = (url: string) =>
-  AdminRouter.assets.find((asset) => url.match(asset.path));

src/buildAuthenticatedRouter.ts

@@ -1,11 +1,11 @@
-import AdminJS from "adminjs";
+import AdminJS, { Router as AdminRouter } from "adminjs";
 import express, { Router } from "express";
 import formidableMiddleware from "express-formidable";
 import session from "express-session";
 import { withLogin } from "./authentication/login.handler";
 import { withLogout } from "./authentication/logout.handler";
 import { withProtectedRoutesHandler } from "./authentication/protected-routes.handler";
-import { buildRouter } from "./buildRouter";
+import { buildAssets, buildRoutes, initializeAdmin } from "./buildRouter";
 import { OldBodyParserUsedError } from "./errors";
 import { AuthenticationOptions, FormidableOptions } from "./types";
 
@@ -52,6 +52,9 @@ export const buildAuthenticatedRouter = (
   sessionOptions?: session.SessionOptions,
   formidableOptions?: FormidableOptions
 ): Router => {
+  initializeAdmin(admin);
+
+  const { routes, assets } = AdminRouter;
   const router = predefinedRouter || express.Router();
 
   router.use((req, _, next) => {
@@ -70,9 +73,12 @@ export const buildAuthenticatedRouter = (
   );
   router.use(formidableMiddleware(formidableOptions));
 
-  withProtectedRoutesHandler(router, admin);
   withLogin(router, admin, auth);
   withLogout(router, admin);
+  buildAssets({ assets, router });
+
+  withProtectedRoutesHandler(router, admin);
+  buildRoutes({ admin, routes, router });
 
-  return buildRouter(admin, router, formidableOptions);
+  return router;
 };

src/buildRouter.ts

@@ -10,74 +10,108 @@ import { convertToExpressRoute } from "./convertRoutes";
 const INVALID_ADMINJS_INSTANCE =
   "You have to pass an instance of AdminJS to the buildRouter() function";
 
-export const buildRouter = (
-  admin: AdminJS,
-  predefinedRouter?: Router | null,
-  formidableOptions?: FormidableOptions
-): Router => {
+export type RouteHandlerArgs = {
+  admin: AdminJS;
+  route: typeof AdminRouter["routes"][0];
+};
+
+export type BuildRoutesArgs = {
+  admin: AdminJS;
+  routes: typeof AdminRouter["routes"];
+  router: Router;
+};
+
+export type BuildAssetsArgs = {
+  assets: typeof AdminRouter["assets"];
+  router: Router;
+};
+
+export const initializeAdmin = (admin: AdminJS): void => {
   if (admin?.constructor?.name !== "AdminJS") {
     throw new WrongArgumentError(INVALID_ADMINJS_INSTANCE);
   }
 
   admin.initialize().then(() => {
     log.debug("AdminJS: bundle ready");
   });
+};
 
-  const { routes, assets } = AdminRouter;
-  const router = predefinedRouter ?? Router();
-  router.use(formidableMiddleware(formidableOptions));
+export const routeHandler = ({
+  admin,
+  route,
+}: RouteHandlerArgs): RequestHandler => async (req, res, next) => {
+  try {
+    const controller = new route.Controller(
+      { admin },
+      req.session && req.session.adminUser
+    );
+    const { params, query } = req;
+    const method = req.method.toLowerCase();
+    const payload = {
+      ...(req.fields || {}),
+      ...(req.files || {}),
+    };
+    const html = await controller[route.action](
+      {
+        ...req,
+        params,
+        query,
+        payload,
+        method,
+      },
+      res
+    );
+    if (route.contentType) {
+      res.set({ "Content-Type": route.contentType });
+    }
+    if (html) {
+      res.send(html);
+    }
+  } catch (e) {
+    next(e);
+  }
+};
 
+export const buildRoutes = ({
+  admin,
+  routes,
+  router,
+}: BuildRoutesArgs): void => {
   routes.forEach((route) => {
     // we have to change routes defined in AdminJS from {recordId} to :recordId
     const expressPath = convertToExpressRoute(route.path);
 
-    const handler: RequestHandler = async (req, res, next) => {
-      try {
-        const controller = new route.Controller(
-          { admin },
-          req.session && req.session.adminUser
-        );
-        const { params, query } = req;
-        const method = req.method.toLowerCase();
-        const payload = {
-          ...(req.fields || {}),
-          ...(req.files || {}),
-        };
-        const html = await controller[route.action](
-          {
-            ...req,
-            params,
-            query,
-            payload,
-            method,
-          },
-          res
-        );
-        if (route.contentType) {
-          res.set({ "Content-Type": route.contentType });
-        }
-        if (html) {
-          res.send(html);
-        }
-      } catch (e) {
-        next(e);
-      }
-    };
-
     if (route.method === "GET") {
-      router.get(expressPath, handler);
+      router.get(expressPath, routeHandler({ admin, route }));
     }
 
     if (route.method === "POST") {
-      router.post(expressPath, handler);
+      router.post(expressPath, routeHandler({ admin, route }));
     }
   });
+};
 
+export const buildAssets = ({ assets, router }: BuildAssetsArgs): void => {
   assets.forEach((asset) => {
-    router.get(asset.path, async (req, res) => {
+    router.get(asset.path, async (_req, res) => {
       res.sendFile(path.resolve(asset.src));
     });
   });
+};
+
+export const buildRouter = (
+  admin: AdminJS,
+  predefinedRouter?: Router | null,
+  formidableOptions?: FormidableOptions
+): Router => {
+  initializeAdmin(admin);
+
+  const { routes, assets } = AdminRouter;
+  const router = predefinedRouter ?? Router();
+  router.use(formidableMiddleware(formidableOptions));
+
+  buildRoutes({ admin, routes, router });
+  buildAssets({ assets, router });
 
   return router;
 };