From b90ede079d3671ea2c1a2a54cd133f01bf889c74 Mon Sep 17 00:00:00 2001 From: Ike <137194738+ike-kottlowski@users.noreply.github.com> Date: Fri, 11 Apr 2025 22:55:02 -0400 Subject: [PATCH] [PM-18888] Fix duo redirect URL checks (#14174) * fix(PM-18888) : Create more strict checking of redirectURL to protect against open redirect attacks using regex. * fix : modify comments and check for embedded credentials. * feat : add testability to duo-redirect connector * fix : fixing strict typing; Removed styling from duo-redirect.ts which allows us to test without adding additional files and configurations for jest. * fix : remove duo-redirect.scss --- apps/web/src/connectors/duo-redirect.scss | 1 - apps/web/src/connectors/duo-redirect.spec.ts | 51 +++++++++++++ apps/web/src/connectors/duo-redirect.ts | 76 ++++++++++++++------ apps/web/webpack.config.js | 2 +- 4 files changed, 105 insertions(+), 25 deletions(-) delete mode 100644 apps/web/src/connectors/duo-redirect.scss create mode 100644 apps/web/src/connectors/duo-redirect.spec.ts diff --git a/apps/web/src/connectors/duo-redirect.scss b/apps/web/src/connectors/duo-redirect.scss deleted file mode 100644 index a4c7f9b25b7..00000000000 --- a/apps/web/src/connectors/duo-redirect.scss +++ /dev/null @@ -1 +0,0 @@ -@import "../scss/styles.scss"; diff --git a/apps/web/src/connectors/duo-redirect.spec.ts b/apps/web/src/connectors/duo-redirect.spec.ts new file mode 100644 index 00000000000..c0498861ba0 --- /dev/null +++ b/apps/web/src/connectors/duo-redirect.spec.ts @@ -0,0 +1,51 @@ +import { redirectToDuoFrameless } from "./duo-redirect"; + +describe("duo-redirect", () => { + describe("redirectToDuoFrameless", () => { + beforeEach(() => { + Object.defineProperty(window, "location", { + value: { href: "" }, + writable: true, + }); + }); + + it("should redirect to a valid Duo URL", () => { + const validUrl = "https://api-123.duosecurity.com/auth"; + redirectToDuoFrameless(validUrl); + expect(window.location.href).toBe(validUrl); + }); + + it("should redirect to a valid Duo Federal URL", () => { + const validUrl = "https://api-123.duofederal.com/auth"; + redirectToDuoFrameless(validUrl); + expect(window.location.href).toBe(validUrl); + }); + + it("should throw an error for an invalid URL", () => { + const invalidUrl = "https://malicious-site.com"; + expect(() => redirectToDuoFrameless(invalidUrl)).toThrow("Invalid redirect URL"); + }); + + it("should throw an error for an malicious URL with valid redirect embedded", () => { + const invalidUrl = "https://malicious-site.com\\@api-123.duosecurity.com/auth"; + expect(() => redirectToDuoFrameless(invalidUrl)).toThrow("Invalid redirect URL"); + }); + + it("should throw an error for a non-HTTPS URL", () => { + const nonHttpsUrl = "http://api-123.duosecurity.com/auth"; + expect(() => redirectToDuoFrameless(nonHttpsUrl)).toThrow("Invalid redirect URL"); + }); + + it("should throw an error for a URL with an invalid hostname", () => { + const invalidHostnameUrl = "https://api-123.invalid.com"; + expect(() => redirectToDuoFrameless(invalidHostnameUrl)).toThrow("Invalid redirect URL"); + }); + + it("should throw an error for a URL with credentials", () => { + const UrlWithCredentials = "https://api-123.duosecurity.com:password@evil/attack"; + expect(() => redirectToDuoFrameless(UrlWithCredentials)).toThrow( + "Invalid redirect URL: embedded credentials not allowed", + ); + }); + }); +}); diff --git a/apps/web/src/connectors/duo-redirect.ts b/apps/web/src/connectors/duo-redirect.ts index c19e056d306..d1841247962 100644 --- a/apps/web/src/connectors/duo-redirect.ts +++ b/apps/web/src/connectors/duo-redirect.ts @@ -1,14 +1,8 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import { getQsParam } from "./common"; import { TranslationService } from "./translation.service"; -// FIXME: Remove when updating file. Eslint update -// eslint-disable-next-line @typescript-eslint/no-require-imports -require("./duo-redirect.scss"); - const mobileDesktopCallback = "bitwarden://duo-callback"; -let localeService: TranslationService = null; +let localeService: TranslationService | null = null; window.addEventListener("load", async () => { const redirectUrl = getQsParam("duoFramelessUrl"); @@ -18,9 +12,18 @@ window.addEventListener("load", async () => { return; } - const client = getQsParam("client"); - const code = getQsParam("code"); - const state = getQsParam("state"); + const client: string | null = getQsParam("client"); + const code: string | null = getQsParam("code"); + const state: string | null = getQsParam("state"); + if (!client) { + throw new Error("client is null"); + } + if (!code) { + throw new Error("code is null"); + } + if (!state) { + throw new Error("state is null"); + } localeService = new TranslationService(navigator.language, "locales"); await localeService.init(); @@ -53,16 +56,28 @@ window.addEventListener("load", async () => { * validate the Duo AuthUrl and redirect to it. * @param redirectUrl the duo auth url */ -function redirectToDuoFrameless(redirectUrl: string) { - const validateUrl = new URL(redirectUrl); - const validDuoUrl = - validateUrl.protocol === "https:" && - (validateUrl.hostname.endsWith(".duosecurity.com") || - validateUrl.hostname.endsWith(".duofederal.com")); - - if (!validDuoUrl) { +export function redirectToDuoFrameless(redirectUrl: string) { + // Regex to match a valid duo redirect URL + /** + * This regex checks for the following: + * The string must start with "https://api-" + * Followed by a subdomain that can contain letters, numbers + * Followed by either "duosecurity.com" or "duofederal.com" + * This ensures that the redirect does not contain any malicious content + * and is a valid Duo URL. + * */ + const duoRedirectUrlRegex = /^https:\/\/api-[a-zA-Z0-9]+\.(duosecurity|duofederal)\.com/; + // Check if the redirect URL matches the regex + if (!duoRedirectUrlRegex.test(redirectUrl)) { throw new Error("Invalid redirect URL"); } + // At this point we know the URL to be valid, but we need to check for embedded credentials + const validateUrl = new URL(redirectUrl); + // URLs should not contain + // Check that no embedded credentials are present + if (validateUrl.username || validateUrl.password) { + throw new Error("Invalid redirect URL: embedded credentials not allowed"); + } window.location.href = decodeURIComponent(redirectUrl); } @@ -72,17 +87,23 @@ function redirectToDuoFrameless(redirectUrl: string) { * so browser, desktop, and mobile are not able to take advantage of the countdown timer or close button. */ function displayHandoffMessage(client: string) { - const content = document.getElementById("content"); + const content: HTMLElement | null = document.getElementById("content"); + if (!content) { + throw new Error("content element not found"); + } content.className = "text-center"; content.innerHTML = ""; const h1 = document.createElement("h1"); - const p = document.createElement("p"); + const p: HTMLElement = document.createElement("p"); + if (!localeService) { + throw new Error("localeService is not initialized"); + } h1.textContent = localeService.t("youSuccessfullyLoggedIn"); p.textContent = client == "web" - ? (p.textContent = localeService.t("thisWindowWillCloseIn5Seconds")) + ? localeService.t("thisWindowWillCloseIn5Seconds") : localeService.t("youMayCloseThisWindow"); h1.className = "font-weight-semibold"; @@ -102,11 +123,20 @@ function displayHandoffMessage(client: string) { }); content.appendChild(button); - // Countdown timer (closes tab upon completion) - let num = Number(p.textContent.match(/\d+/)[0]); + if (p.textContent === null) { + throw new Error("count down container is null"); + } + const counterString: string | null = p.textContent.match(/\d+/)?.[0] || null; + if (!counterString) { + throw new Error("count down time cannot be null"); + } + let num: number = Number(counterString); const interval = setInterval(() => { if (num > 1) { + if (p.textContent === null) { + throw new Error("count down container is null"); + } p.textContent = p.textContent.replace(String(num), String(num - 1)); num--; } else { diff --git a/apps/web/webpack.config.js b/apps/web/webpack.config.js index d172ea95c71..9ccccee21bf 100644 --- a/apps/web/webpack.config.js +++ b/apps/web/webpack.config.js @@ -142,7 +142,7 @@ const plugins = [ new HtmlWebpackPlugin({ template: "./src/connectors/duo-redirect.html", filename: "duo-redirect-connector.html", - chunks: ["connectors/duo-redirect"], + chunks: ["connectors/duo-redirect", "styles"], }), new HtmlWebpackPlugin({ template: "./src/404.html",