Skip to content

Commit 9a5f415

Browse files
committed
[dashboard, server] Backport security fix from #20152
ENT-735
1 parent 8ae8366 commit 9a5f415

File tree

4 files changed

+54
-8
lines changed

4 files changed

+54
-8
lines changed

components/dashboard/src/OauthClientApproval.tsx

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
import gitpodIcon from "./icons/gitpod.svg";
88
import { getSafeURLRedirect } from "./provider-utils";
9+
import { isTrustedUrlOrPath } from "./utils";
910

1011
export default function OAuthClientApproval() {
1112
const params = new URLSearchParams(window.location.search);
@@ -18,8 +19,12 @@ export default function OAuthClientApproval() {
1819
const updateClientApproval = async (isApproved: boolean) => {
1920
if (redirectTo === "/") {
2021
window.location.replace(redirectTo);
22+
return;
23+
}
24+
const url = `${redirectTo}&approved=${isApproved ? "yes" : "no"}`;
25+
if (isTrustedUrlOrPath(url)) {
26+
window.location.replace(url);
2127
}
22-
window.location.replace(`${redirectTo}&approved=${isApproved ? "yes" : "no"}`);
2328
};
2429

2530
return (

components/dashboard/src/provider-utils.tsx

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import bitbucket from "./images/bitbucket.svg";
88
import github from "./images/github.svg";
99
import gitlab from "./images/gitlab.svg";
1010
import { gitpodHostUrl } from "./service/service";
11+
import { isTrustedUrlOrPath } from "./utils";
1112

1213
function iconForAuthProvider(type: string) {
1314
switch (type) {
@@ -121,11 +122,10 @@ const getSafeURLRedirect = (source?: string) => {
121122
const returnToURL: string | null = new URLSearchParams(source ? source : window.location.search).get("returnTo");
122123
if (returnToURL) {
123124
// Only allow oauth on the same host
124-
if (
125-
returnToURL
126-
.toLowerCase()
127-
.startsWith(`${window.location.protocol}//${window.location.host}/api/oauth/`.toLowerCase())
128-
) {
125+
const sameHost = returnToURL
126+
.toLowerCase()
127+
.startsWith(`${window.location.protocol}//${window.location.host}/api/oauth/`.toLowerCase());
128+
if (sameHost && isTrustedUrlOrPath(returnToURL)) {
129129
return returnToURL;
130130
}
131131
}

components/dashboard/src/utils.test.ts

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,10 @@
44
* See License-AGPL.txt in the project root for license information.
55
*/
66

7-
import { inResource } from "./utils";
7+
import { getURLHash } from "./App";
8+
import { inResource, isTrustedUrlOrPath } from "./utils";
89

910
test("inResource", () => {
10-
1111
// Given root path is a part of resources specified
1212
expect(inResource("/app", ["new", "app", "teams"])).toBe(true);
1313

@@ -25,5 +25,29 @@ test("inResource", () => {
2525

2626
// Both resources containing path with subdirectories
2727
expect(inResource("/admin/teams/someTeam/somePerson", ["/admin/teams"])).toBe(true);
28+
});
2829

30+
test("urlHash and isTrustedUrlOrPath", () => {
31+
global.window = Object.create(window);
32+
Object.defineProperty(window, "location", {
33+
value: {
34+
hash: "#https://example.org/user/repo",
35+
hostname: "example.org",
36+
},
37+
});
38+
39+
expect(getURLHash()).toBe("https://example.org/user/repo");
40+
41+
const isTrustedUrlOrPathCases: { location: string; trusted: boolean }[] = [
42+
{ location: "https://example.org/user/repo", trusted: true },
43+
{ location: "https://example.org/user", trusted: true },
44+
{ location: "https://example2.org/user", trusted: false },
45+
{ location: "/api/hello", trusted: true },
46+
{ location: "/", trusted: true },
47+
// eslint-disable-next-line no-script-url
48+
{ location: "javascript:alert(1)", trusted: false },
49+
];
50+
isTrustedUrlOrPathCases.forEach(({ location, trusted }) => {
51+
expect(isTrustedUrlOrPath(location)).toBe(trusted);
52+
});
2953
});

components/dashboard/src/utils.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,3 +79,20 @@ export function inResource(pathname: string, resources: string[]): boolean {
7979
// E.g. "api/userspace/resource" path is a part of resource "api/userspace"
8080
return resources.map((res) => trimmedResource.startsWith(trimResource(res))).some(Boolean);
8181
}
82+
83+
function parseUrl(url: string): URL | null {
84+
try {
85+
return new URL(url);
86+
} catch (_) {
87+
return null;
88+
}
89+
}
90+
91+
export function isTrustedUrlOrPath(urlOrPath: string) {
92+
const url = parseUrl(urlOrPath);
93+
const isTrusted = url ? window.location.hostname === url.hostname : urlOrPath.startsWith("/");
94+
if (!isTrusted) {
95+
console.warn("Untrusted URL", urlOrPath);
96+
}
97+
return isTrusted;
98+
}

0 commit comments

Comments
 (0)