Skip to content

Commit a876d61

Browse files
dzirafdziraf
committed
fix: fix security issues caused by withProtectedRoutes middleware
1 parent 153db26 commit a876d61

3 files changed

Lines changed: 101 additions & 111 deletions

File tree

‎src/authentication/protected-routes.handler.ts‎

Lines changed: 13 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -1,71 +1,21 @@
1-
import AdminJS, { Router as AdminRouter } from "adminjs";
2-
import { Router } from "express";
3-
import { convertToExpressRoute } from "../convertRoutes";
4-
import { pathToRegexp } from "path-to-regexp";
5-
6-
const doesNotRequireAuthentication = (
7-
url: string,
8-
{ loginPath, logoutPath }
9-
) => {
10-
return (
11-
isAdminAsset(url) || url.startsWith(loginPath) || url.startsWith(logoutPath)
12-
);
13-
};
1+
import AdminJS from "adminjs";
2+
import { Router, RequestHandler } from "express";
143

154
export const withProtectedRoutesHandler = (
165
router: Router,
176
admin: AdminJS
187
): void => {
19-
const { rootPath, loginPath, logoutPath } = admin.options;
20-
21-
router.use((req, res, next) => {
22-
if (
23-
doesNotRequireAuthentication(req.originalUrl, { loginPath, logoutPath })
24-
) {
25-
return next();
26-
}
27-
28-
if (isAdminRoute(req.originalUrl, rootPath)) {
29-
if (!!req.session.adminUser) {
30-
return next();
31-
}
32-
return res.redirect(loginPath);
33-
}
34-
35-
return next(); // custom routes in admin router
36-
});
37-
};
38-
39-
export const isAdminRoute = (
40-
originalUrl: string,
41-
adminRootPath: string
42-
): boolean => {
43-
const adminRoutes = AdminRouter.routes
44-
.map((route) => convertToExpressRoute(route.path))
45-
.filter((route) => route !== "");
46-
47-
let urlWithoutAdminRootPath = originalUrl.split("?")[0];
48-
if (adminRootPath !== "/") {
49-
urlWithoutAdminRootPath = urlWithoutAdminRootPath.replace(
50-
adminRootPath,
51-
""
52-
);
53-
if (!urlWithoutAdminRootPath.startsWith("/")) {
54-
urlWithoutAdminRootPath = `/${urlWithoutAdminRootPath}`;
8+
const { loginPath } = admin.options;
9+
const authorizedRoutesMiddleware: RequestHandler = (
10+
request,
11+
response,
12+
next
13+
) => {
14+
if (!request.session || !request.session.adminUser) {
15+
return response.redirect(loginPath);
5516
}
56-
}
57-
58-
const isAdminRootUrl = originalUrl === adminRootPath;
59-
const isUrlUnderRootPath = originalUrl.startsWith(adminRootPath);
17+
return next();
18+
};
6019

61-
return (
62-
isAdminRootUrl ||
63-
(adminRoutes.some((route) =>
64-
pathToRegexp(route).test(urlWithoutAdminRootPath)
65-
) &&
66-
isUrlUnderRootPath)
67-
);
20+
router.use(authorizedRoutesMiddleware);
6821
};
69-
70-
const isAdminAsset = (url: string) =>
71-
AdminRouter.assets.find((asset) => url.match(asset.path));

‎src/buildAuthenticatedRouter.ts‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
1-
import AdminJS from "adminjs";
1+
import AdminJS, { Router as AdminRouter } from "adminjs";
22
import express, { Router } from "express";
33
import formidableMiddleware from "express-formidable";
44
import session from "express-session";
55
import { withLogin } from "./authentication/login.handler";
66
import { withLogout } from "./authentication/logout.handler";
77
import { withProtectedRoutesHandler } from "./authentication/protected-routes.handler";
8-
import { buildRouter } from "./buildRouter";
8+
import { buildAssets, buildRoutes, initializeAdmin } from "./buildRouter";
99
import { OldBodyParserUsedError } from "./errors";
1010
import { AuthenticationOptions, FormidableOptions } from "./types";
1111

@@ -52,6 +52,9 @@ export const buildAuthenticatedRouter = (
5252
sessionOptions?: session.SessionOptions,
5353
formidableOptions?: FormidableOptions
5454
): Router => {
55+
initializeAdmin(admin);
56+
57+
const { routes, assets } = AdminRouter;
5558
const router = predefinedRouter || express.Router();
5659

5760
router.use((req, _, next) => {
@@ -70,9 +73,12 @@ export const buildAuthenticatedRouter = (
7073
);
7174
router.use(formidableMiddleware(formidableOptions));
7275

73-
withProtectedRoutesHandler(router, admin);
7476
withLogin(router, admin, auth);
7577
withLogout(router, admin);
78+
buildAssets({ assets, router });
79+
80+
withProtectedRoutesHandler(router, admin);
81+
buildRoutes({ admin, routes, router });
7682

77-
return buildRouter(admin, router, formidableOptions);
83+
return router;
7884
};

‎src/buildRouter.ts‎

Lines changed: 78 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -10,74 +10,108 @@ import { convertToExpressRoute } from "./convertRoutes";
1010
const INVALID_ADMINJS_INSTANCE =
1111
"You have to pass an instance of AdminJS to the buildRouter() function";
1212

13-
export const buildRouter = (
14-
admin: AdminJS,
15-
predefinedRouter?: Router | null,
16-
formidableOptions?: FormidableOptions
17-
): Router => {
13+
export type RouteHandlerArgs = {
14+
admin: AdminJS;
15+
route: typeof AdminRouter["routes"][0];
16+
};
17+
18+
export type BuildRoutesArgs = {
19+
admin: AdminJS;
20+
routes: typeof AdminRouter["routes"];
21+
router: Router;
22+
};
23+
24+
export type BuildAssetsArgs = {
25+
assets: typeof AdminRouter["assets"];
26+
router: Router;
27+
};
28+
29+
export const initializeAdmin = (admin: AdminJS): void => {
1830
if (admin?.constructor?.name !== "AdminJS") {
1931
throw new WrongArgumentError(INVALID_ADMINJS_INSTANCE);
2032
}
2133

2234
admin.initialize().then(() => {
2335
log.debug("AdminJS: bundle ready");
2436
});
37+
};
2538

26-
const { routes, assets } = AdminRouter;
27-
const router = predefinedRouter ?? Router();
28-
router.use(formidableMiddleware(formidableOptions));
39+
export const routeHandler = ({
40+
admin,
41+
route,
42+
}: RouteHandlerArgs): RequestHandler => async (req, res, next) => {
43+
try {
44+
const controller = new route.Controller(
45+
{ admin },
46+
req.session && req.session.adminUser
47+
);
48+
const { params, query } = req;
49+
const method = req.method.toLowerCase();
50+
const payload = {
51+
...(req.fields || {}),
52+
...(req.files || {}),
53+
};
54+
const html = await controller[route.action](
55+
{
56+
...req,
57+
params,
58+
query,
59+
payload,
60+
method,
61+
},
62+
res
63+
);
64+
if (route.contentType) {
65+
res.set({ "Content-Type": route.contentType });
66+
}
67+
if (html) {
68+
res.send(html);
69+
}
70+
} catch (e) {
71+
next(e);
72+
}
73+
};
2974

75+
export const buildRoutes = ({
76+
admin,
77+
routes,
78+
router,
79+
}: BuildRoutesArgs): void => {
3080
routes.forEach((route) => {
3181
// we have to change routes defined in AdminJS from {recordId} to :recordId
3282
const expressPath = convertToExpressRoute(route.path);
3383

34-
const handler: RequestHandler = async (req, res, next) => {
35-
try {
36-
const controller = new route.Controller(
37-
{ admin },
38-
req.session && req.session.adminUser
39-
);
40-
const { params, query } = req;
41-
const method = req.method.toLowerCase();
42-
const payload = {
43-
...(req.fields || {}),
44-
...(req.files || {}),
45-
};
46-
const html = await controller[route.action](
47-
{
48-
...req,
49-
params,
50-
query,
51-
payload,
52-
method,
53-
},
54-
res
55-
);
56-
if (route.contentType) {
57-
res.set({ "Content-Type": route.contentType });
58-
}
59-
if (html) {
60-
res.send(html);
61-
}
62-
} catch (e) {
63-
next(e);
64-
}
65-
};
66-
6784
if (route.method === "GET") {
68-
router.get(expressPath, handler);
85+
router.get(expressPath, routeHandler({ admin, route }));
6986
}
7087

7188
if (route.method === "POST") {
72-
router.post(expressPath, handler);
89+
router.post(expressPath, routeHandler({ admin, route }));
7390
}
7491
});
92+
};
7593

94+
export const buildAssets = ({ assets, router }: BuildAssetsArgs): void => {
7695
assets.forEach((asset) => {
77-
router.get(asset.path, async (req, res) => {
96+
router.get(asset.path, async (_req, res) => {
7897
res.sendFile(path.resolve(asset.src));
7998
});
8099
});
100+
};
101+
102+
export const buildRouter = (
103+
admin: AdminJS,
104+
predefinedRouter?: Router | null,
105+
formidableOptions?: FormidableOptions
106+
): Router => {
107+
initializeAdmin(admin);
108+
109+
const { routes, assets } = AdminRouter;
110+
const router = predefinedRouter ?? Router();
111+
router.use(formidableMiddleware(formidableOptions));
112+
113+
buildRoutes({ admin, routes, router });
114+
buildAssets({ assets, router });
81115

82116
return router;
83117
};

0 commit comments

Comments
 (0)