From b6c2db780b3ec0cb3f06b4baa9a8f65364d84f47 Mon Sep 17 00:00:00 2001 From: mustard Date: Wed, 28 Aug 2024 00:45:40 +0800 Subject: [PATCH] Validate redirect url (#20152) * Validate redirect url * Address feedback * feedback 2 --- components/dashboard/src/Login.tsx | 4 ++-- .../dashboard/src/OauthClientApproval.tsx | 7 ++++++- components/dashboard/src/utils.test.ts | 18 ++++++++++++++++-- components/dashboard/src/utils.ts | 17 +++++++++++++++++ .../src/oauth-server/oauth-controller.ts | 4 ++-- 5 files changed, 43 insertions(+), 7 deletions(-) diff --git a/components/dashboard/src/Login.tsx b/components/dashboard/src/Login.tsx index d1d477a088905b..ebd72f6af7976c 100644 --- a/components/dashboard/src/Login.tsx +++ b/components/dashboard/src/Login.tsx @@ -10,7 +10,7 @@ import { UserContext } from "./user-context"; import { getGitpodService } from "./service/service"; import { iconForAuthProvider, openAuthorizeWindow, simplifyProviderName } from "./provider-utils"; import exclamation from "./images/exclamation.svg"; -import { getURLHash } from "./utils"; +import { getURLHash, isTrustedUrlOrPath } from "./utils"; import ErrorMessage from "./components/ErrorMessage"; import { Heading1, Heading2, Subheading } from "./components/typography/headings"; import { SSOLoginForm } from "./login/SSOLoginForm"; @@ -84,7 +84,7 @@ export const Login: FC = ({ onLoggedIn }) => { const returnToPath = new URLSearchParams(window.location.search).get("returnToPath"); if (returnToPath) { const isAbsoluteURL = /^https?:\/\//i.test(returnToPath); - if (!isAbsoluteURL) { + if (!isAbsoluteURL && isTrustedUrlOrPath(returnToPath)) { window.location.replace(returnToPath); } } diff --git a/components/dashboard/src/OauthClientApproval.tsx b/components/dashboard/src/OauthClientApproval.tsx index 62c1b2bbead5cc..66f2000dcd42e6 100644 --- a/components/dashboard/src/OauthClientApproval.tsx +++ b/components/dashboard/src/OauthClientApproval.tsx @@ -7,6 +7,7 @@ import { Button } from "@podkit/buttons/Button"; import gitpodIcon from "./icons/gitpod.svg"; import { Heading1, Subheading } from "@podkit/typography/Headings"; +import { isTrustedUrlOrPath } from "./utils"; export default function OAuthClientApproval() { const params = new URLSearchParams(window.location.search); @@ -23,8 +24,12 @@ export default function OAuthClientApproval() { const updateClientApproval = async (isApproved: boolean) => { if (redirectTo === "/") { window.location.replace(redirectTo); + return; + } + const url = `${redirectTo}&approved=${isApproved ? "yes" : "no"}`; + if (isTrustedUrlOrPath(url)) { + window.location.replace(url); } - window.location.replace(`${redirectTo}&approved=${isApproved ? "yes" : "no"}`); }; return ( diff --git a/components/dashboard/src/utils.test.ts b/components/dashboard/src/utils.test.ts index 735582de1fd63a..6de77f411f7b6c 100644 --- a/components/dashboard/src/utils.test.ts +++ b/components/dashboard/src/utils.test.ts @@ -4,7 +4,7 @@ * See License.AGPL.txt in the project root for license information. */ -import { inResource, getURLHash } from "./utils"; +import { inResource, getURLHash, isTrustedUrlOrPath } from "./utils"; test("inResource", () => { // Given root path is a part of resources specified @@ -26,13 +26,27 @@ test("inResource", () => { expect(inResource("/admin/teams/someTeam/somePerson", ["/admin/teams"])).toBe(true); }); -test("urlHash", () => { +test("urlHash and isTrustedUrlOrPath", () => { global.window = Object.create(window); Object.defineProperty(window, "location", { value: { hash: "#https://example.org/user/repo", + hostname: "example.org", }, }); expect(getURLHash()).toBe("https://example.org/user/repo"); + + const isTrustedUrlOrPathCases: { location: string; trusted: boolean }[] = [ + { location: "https://example.org/user/repo", trusted: true }, + { location: "https://example.org/user", trusted: true }, + { location: "https://example2.org/user", trusted: false }, + { location: "/api/hello", trusted: true }, + { location: "/", trusted: true }, + // eslint-disable-next-line no-script-url + { location: "javascript:alert(1)", trusted: false }, + ]; + isTrustedUrlOrPathCases.forEach(({ location, trusted }) => { + expect(isTrustedUrlOrPath(location)).toBe(trusted); + }); }); diff --git a/components/dashboard/src/utils.ts b/components/dashboard/src/utils.ts index 489dbc6fd16a1d..4787e6fbd446d1 100644 --- a/components/dashboard/src/utils.ts +++ b/components/dashboard/src/utils.ts @@ -213,3 +213,20 @@ export class ReplayableEventEmitter extends EventEm return this.reachedEnd; } } + +function parseUrl(url: string): URL | null { + try { + return new URL(url); + } catch (_) { + return null; + } +} + +export function isTrustedUrlOrPath(urlOrPath: string) { + const url = parseUrl(urlOrPath); + const isTrusted = url ? window.location.hostname === url.hostname : urlOrPath.startsWith("/"); + if (!isTrusted) { + console.warn("Untrusted URL", urlOrPath); + } + return isTrusted; +} diff --git a/components/server/src/oauth-server/oauth-controller.ts b/components/server/src/oauth-server/oauth-controller.ts index b837c39e81ac59..408ed570254674 100644 --- a/components/server/src/oauth-server/oauth-controller.ts +++ b/components/server/src/oauth-server/oauth-controller.ts @@ -26,7 +26,7 @@ export class OAuthController { private getValidUser(req: express.Request, res: express.Response): User | null { if (!req.isAuthenticated() || !User.is(req.user)) { - const returnToPath = encodeURIComponent(`api${req.originalUrl}`); + const returnToPath = encodeURIComponent(`/api${req.originalUrl}`); const redirectTo = `${this.config.hostUrl}login?returnToPath=${returnToPath}`; res.redirect(redirectTo); return null; @@ -102,7 +102,7 @@ export class OAuthController { if (!oauthClientsApproved || !oauthClientsApproved[clientID]) { const client = await clientRepository.getByIdentifier(clientID); if (client) { - const returnToPath = encodeURIComponent(`api${req.originalUrl}`); + const returnToPath = encodeURIComponent(`/api${req.originalUrl}`); const redirectTo = `${this.config.hostUrl}oauth-approval?clientID=${client.id}&clientName=${client.name}&returnToPath=${returnToPath}`; res.redirect(redirectTo); return false;