fix potential open redirect

2022-09-22 11:35:47 -04:00 · 2022-09-22 11:35:47 -04:00 · eaa28441ac
parent 3140039d5c
commit eaa28441ac
7 changed files with 88 additions and 75 deletions
--- a/app.js
+++ b/app.js
@ -24,6 +24,7 @@ import { version } from "./version.js";
 import * as databaseInitializer from "./helpers/initializer.database.js";
 import debug from "debug";
 import { apiGetHandler } from "./handlers/permissions.js";
+import { getSafeRedirectURL } from "./helpers/functions.authentication.js";
 const debugApp = debug("lot-occupancy-system:app");
 databaseInitializer.initializeDatabase();
 const __dirname = ".";
@ -91,7 +92,8 @@ const sessionChecker = (request, response, next) => {
    if (request.session.user && request.cookies[sessionCookieName]) {
        return next();
    }
-    return response.redirect(`${urlPrefix}/login?redirect=${request.originalUrl}`);
+    const redirectUrl = getSafeRedirectURL(request.originalUrl);
+    return response.redirect(`${urlPrefix}/login?redirect=${redirectUrl}`);
 };
 app.use((request, response, next) => {
    response.locals.buildNumber = version;
--- a/app.ts
+++ b/app.ts
@ -31,6 +31,7 @@ import * as databaseInitializer from "./helpers/initializer.database.js";

 import debug from "debug";
 import { apiGetHandler } from "./handlers/permissions.js";
+import { getSafeRedirectURL } from "./helpers/functions.authentication.js";
 const debugApp = debug("lot-occupancy-system:app");

 /*
@ -177,8 +178,10 @@ const sessionChecker = (
        return next();
    }

+    const redirectUrl = getSafeRedirectURL(request.originalUrl);
+
    return response.redirect(
-        `${urlPrefix}/login?redirect=${request.originalUrl}`
+        `${urlPrefix}/login?redirect=${redirectUrl}`
    );
 };

--- a/helpers/functions.authentication.d.ts
+++ b/helpers/functions.authentication.d.ts
@ -1 +1,2 @@
 export declare const authenticate: (userName: string, password: string) => Promise<boolean>;
+export declare const getSafeRedirectURL: (possibleRedirectURL?: string) => string;
--- a/helpers/functions.authentication.js
+++ b/helpers/functions.authentication.js
@ -24,3 +24,36 @@ export const authenticate = async (userName, password) => {
    }
    return await authenticateViaActiveDirectory(userName, password);
 };
+const safeRedirects = new Set([
+    "/admin/cleanup",
+    "/admin/fees",
+    "/admin/occupancytypes",
+    "/admin/tables",
+    "/lotoccupancies",
+    "/lotoccupancies/new",
+    "/lots",
+    "/lots/new",
+    "/maps",
+    "/maps/new",
+    "/workorders",
+    "/workorders/new",
+    "/workorders/milestonecalendar",
+    "/workorders/outlook",
+    "/reports"
+]);
+export const getSafeRedirectURL = (possibleRedirectURL = "") => {
+    const urlPrefix = configFunctions.getProperty("reverseProxy.urlPrefix");
+    if (typeof possibleRedirectURL === "string") {
+        const urlToCheck = (possibleRedirectURL.startsWith(urlPrefix)
+            ? possibleRedirectURL.slice(urlPrefix.length)
+            : possibleRedirectURL).toLowerCase();
+        if (safeRedirects.has(urlToCheck) ||
+            /^(\/maps\/)\d+(\/edit)?$/.test(urlToCheck) ||
+            /^(\/lots\/)\d+(\/edit)?$/.test(urlToCheck) ||
+            /^(\/lotoccupancies\/)\d+(\/edit)?$/.test(urlToCheck) ||
+            /^(\/workorders\/)\d+(\/edit)?$/.test(urlToCheck)) {
+            return urlPrefix + urlToCheck;
+        }
+    }
+    return urlPrefix + "/dashboard";
+};
--- a/helpers/functions.authentication.ts
+++ b/helpers/functions.authentication.ts
@ -41,3 +41,46 @@ export const authenticate = async (

    return await authenticateViaActiveDirectory(userName, password);
 };
+
+
+const safeRedirects = new Set([
+    "/admin/cleanup",
+    "/admin/fees",
+    "/admin/occupancytypes",
+    "/admin/tables",
+    "/lotoccupancies",
+    "/lotoccupancies/new",
+    "/lots",
+    "/lots/new",
+    "/maps",
+    "/maps/new",
+    "/workorders",
+    "/workorders/new",
+    "/workorders/milestonecalendar",
+    "/workorders/outlook",
+    "/reports"
+]);
+
+export const getSafeRedirectURL = (possibleRedirectURL = "") => {
+    const urlPrefix = configFunctions.getProperty("reverseProxy.urlPrefix");
+
+    if (typeof possibleRedirectURL === "string") {
+        const urlToCheck = (
+            possibleRedirectURL.startsWith(urlPrefix)
+                ? possibleRedirectURL.slice(urlPrefix.length)
+                : possibleRedirectURL
+        ).toLowerCase();
+
+        if (
+            safeRedirects.has(urlToCheck) ||
+            /^(\/maps\/)\d+(\/edit)?$/.test(urlToCheck) ||
+            /^(\/lots\/)\d+(\/edit)?$/.test(urlToCheck) ||
+            /^(\/lotoccupancies\/)\d+(\/edit)?$/.test(urlToCheck) ||
+            /^(\/workorders\/)\d+(\/edit)?$/.test(urlToCheck)
+        ) {
+            return urlPrefix + urlToCheck;
+        }
+    }
+
+    return urlPrefix + "/dashboard";
+};
--- a/routes/login.js
+++ b/routes/login.js
@ -6,42 +6,12 @@ import { getApiKey } from "../helpers/functions.api.js";
 import Debug from "debug";
 const debug = Debug("lot-occupancy-system:login");
 export const router = Router();
-const safeRedirects = new Set([
-    "/admin/fees",
-    "/admin/occupancytypes",
-    "/admin/tables",
-    "/lotoccupancies",
-    "/lotoccupancies/new",
-    "/lots",
-    "/lots/new",
-    "/maps",
-    "/maps/new",
-    "/workorders",
-    "/workorders/new",
-    "/reports"
-]);
-const getSafeRedirectURL = (possibleRedirectURL = "") => {
-    const urlPrefix = configFunctions.getProperty("reverseProxy.urlPrefix");
-    if (typeof possibleRedirectURL === "string") {
-        const urlToCheck = (possibleRedirectURL.startsWith(urlPrefix)
-            ? possibleRedirectURL.slice(urlPrefix.length)
-            : possibleRedirectURL).toLowerCase();
-        if (safeRedirects.has(urlToCheck) ||
-            /^(\/maps\/)\d+(\/edit)?$/.test(urlToCheck) ||
-            /^(\/lots\/)\d+(\/edit)?$/.test(urlToCheck) ||
-            /^(\/lotoccupancies\/)\d+(\/edit)?$/.test(urlToCheck) ||
-            /^(\/workorders\/)\d+(\/edit)?$/.test(urlToCheck)) {
-            return urlPrefix + urlToCheck;
-        }
-    }
-    return urlPrefix + "/dashboard";
-};
 router
    .route("/")
    .get((request, response) => {
    const sessionCookieName = configFunctions.getProperty("session.cookieName");
    if (request.session.user && request.cookies[sessionCookieName]) {
-        const redirectURL = getSafeRedirectURL((request.query.redirect || ""));
+        const redirectURL = authenticationFunctions.getSafeRedirectURL((request.query.redirect || ""));
        response.redirect(redirectURL);
    }
    else {
@ -57,7 +27,7 @@ router
    const userName = request.body.userName;
    const passwordPlain = request.body.password;
    const unsafeRedirectURL = request.body.redirect;
-    const redirectURL = getSafeRedirectURL(typeof unsafeRedirectURL === "string" ? unsafeRedirectURL : "");
+    const redirectURL = authenticationFunctions.getSafeRedirectURL(typeof unsafeRedirectURL === "string" ? unsafeRedirectURL : "");
    let isAuthenticated = false;
    if (userName.charAt(0) === "*") {
        if (useTestDatabases && userName === passwordPlain) {
--- a/routes/login.ts
+++ b/routes/login.ts
@ -16,45 +16,6 @@ const debug = Debug("lot-occupancy-system:login");

 export const router = Router();

-const safeRedirects = new Set([
-    "/admin/fees",
-    "/admin/occupancytypes",
-    "/admin/tables",
-    "/lotoccupancies",
-    "/lotoccupancies/new",
-    "/lots",
-    "/lots/new",
-    "/maps",
-    "/maps/new",
-    "/workorders",
-    "/workorders/new",
-    "/reports"
-]);
-
-const getSafeRedirectURL = (possibleRedirectURL = "") => {
-    const urlPrefix = configFunctions.getProperty("reverseProxy.urlPrefix");
-
-    if (typeof possibleRedirectURL === "string") {
-        const urlToCheck = (
-            possibleRedirectURL.startsWith(urlPrefix)
-                ? possibleRedirectURL.slice(urlPrefix.length)
-                : possibleRedirectURL
-        ).toLowerCase();
-
-        if (
-            safeRedirects.has(urlToCheck) ||
-            /^(\/maps\/)\d+(\/edit)?$/.test(urlToCheck) ||
-            /^(\/lots\/)\d+(\/edit)?$/.test(urlToCheck) ||
-            /^(\/lotoccupancies\/)\d+(\/edit)?$/.test(urlToCheck) ||
-            /^(\/workorders\/)\d+(\/edit)?$/.test(urlToCheck)
-        ) {
-            return urlPrefix + urlToCheck;
-        }
-    }
-
-    return urlPrefix + "/dashboard";
-};
-
 router
    .route("/")
    .get((request, response) => {
@ -62,7 +23,7 @@ router
            configFunctions.getProperty("session.cookieName");

        if (request.session.user && request.cookies[sessionCookieName]) {
-            const redirectURL = getSafeRedirectURL(
+            const redirectURL = authenticationFunctions.getSafeRedirectURL(
                (request.query.redirect || "") as string
            );

@ -82,7 +43,7 @@ router

        const unsafeRedirectURL = request.body.redirect;

-        const redirectURL = getSafeRedirectURL(
+        const redirectURL = authenticationFunctions.getSafeRedirectURL(
            typeof unsafeRedirectURL === "string" ? unsafeRedirectURL : ""
        );