Skip to content

Commit b6c2db7

Browse files
authored
Validate redirect url (#20152)
* Validate redirect url * Address feedback * feedback 2
1 parent 97ba0a7 commit b6c2db7

File tree

5 files changed

+43
-7
lines changed

5 files changed

+43
-7
lines changed

components/dashboard/src/Login.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { UserContext } from "./user-context";
1010
import { getGitpodService } from "./service/service";
1111
import { iconForAuthProvider, openAuthorizeWindow, simplifyProviderName } from "./provider-utils";
1212
import exclamation from "./images/exclamation.svg";
13-
import { getURLHash } from "./utils";
13+
import { getURLHash, isTrustedUrlOrPath } from "./utils";
1414
import ErrorMessage from "./components/ErrorMessage";
1515
import { Heading1, Heading2, Subheading } from "./components/typography/headings";
1616
import { SSOLoginForm } from "./login/SSOLoginForm";
@@ -84,7 +84,7 @@ export const Login: FC<LoginProps> = ({ onLoggedIn }) => {
8484
const returnToPath = new URLSearchParams(window.location.search).get("returnToPath");
8585
if (returnToPath) {
8686
const isAbsoluteURL = /^https?:\/\//i.test(returnToPath);
87-
if (!isAbsoluteURL) {
87+
if (!isAbsoluteURL && isTrustedUrlOrPath(returnToPath)) {
8888
window.location.replace(returnToPath);
8989
}
9090
}

components/dashboard/src/OauthClientApproval.tsx

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import { Button } from "@podkit/buttons/Button";
88
import gitpodIcon from "./icons/gitpod.svg";
99
import { Heading1, Subheading } from "@podkit/typography/Headings";
10+
import { isTrustedUrlOrPath } from "./utils";
1011

1112
export default function OAuthClientApproval() {
1213
const params = new URLSearchParams(window.location.search);
@@ -23,8 +24,12 @@ export default function OAuthClientApproval() {
2324
const updateClientApproval = async (isApproved: boolean) => {
2425
if (redirectTo === "/") {
2526
window.location.replace(redirectTo);
27+
return;
28+
}
29+
const url = `${redirectTo}&approved=${isApproved ? "yes" : "no"}`;
30+
if (isTrustedUrlOrPath(url)) {
31+
window.location.replace(url);
2632
}
27-
window.location.replace(`${redirectTo}&approved=${isApproved ? "yes" : "no"}`);
2833
};
2934

3035
return (

components/dashboard/src/utils.test.ts

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

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

99
test("inResource", () => {
1010
// Given root path is a part of resources specified
@@ -26,13 +26,27 @@ test("inResource", () => {
2626
expect(inResource("/admin/teams/someTeam/somePerson", ["/admin/teams"])).toBe(true);
2727
});
2828

29-
test("urlHash", () => {
29+
test("urlHash and isTrustedUrlOrPath", () => {
3030
global.window = Object.create(window);
3131
Object.defineProperty(window, "location", {
3232
value: {
3333
hash: "#https://example.org/user/repo",
34+
hostname: "example.org",
3435
},
3536
});
3637

3738
expect(getURLHash()).toBe("https://example.org/user/repo");
39+
40+
const isTrustedUrlOrPathCases: { location: string; trusted: boolean }[] = [
41+
{ location: "https://example.org/user/repo", trusted: true },
42+
{ location: "https://example.org/user", trusted: true },
43+
{ location: "https://example2.org/user", trusted: false },
44+
{ location: "/api/hello", trusted: true },
45+
{ location: "/", trusted: true },
46+
// eslint-disable-next-line no-script-url
47+
{ location: "javascript:alert(1)", trusted: false },
48+
];
49+
isTrustedUrlOrPathCases.forEach(({ location, trusted }) => {
50+
expect(isTrustedUrlOrPath(location)).toBe(trusted);
51+
});
3852
});

components/dashboard/src/utils.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,3 +213,20 @@ export class ReplayableEventEmitter<EventTypes extends EventMap> extends EventEm
213213
return this.reachedEnd;
214214
}
215215
}
216+
217+
function parseUrl(url: string): URL | null {
218+
try {
219+
return new URL(url);
220+
} catch (_) {
221+
return null;
222+
}
223+
}
224+
225+
export function isTrustedUrlOrPath(urlOrPath: string) {
226+
const url = parseUrl(urlOrPath);
227+
const isTrusted = url ? window.location.hostname === url.hostname : urlOrPath.startsWith("/");
228+
if (!isTrusted) {
229+
console.warn("Untrusted URL", urlOrPath);
230+
}
231+
return isTrusted;
232+
}

components/server/src/oauth-server/oauth-controller.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ export class OAuthController {
2626

2727
private getValidUser(req: express.Request, res: express.Response): User | null {
2828
if (!req.isAuthenticated() || !User.is(req.user)) {
29-
const returnToPath = encodeURIComponent(`api${req.originalUrl}`);
29+
const returnToPath = encodeURIComponent(`/api${req.originalUrl}`);
3030
const redirectTo = `${this.config.hostUrl}login?returnToPath=${returnToPath}`;
3131
res.redirect(redirectTo);
3232
return null;
@@ -102,7 +102,7 @@ export class OAuthController {
102102
if (!oauthClientsApproved || !oauthClientsApproved[clientID]) {
103103
const client = await clientRepository.getByIdentifier(clientID);
104104
if (client) {
105-
const returnToPath = encodeURIComponent(`api${req.originalUrl}`);
105+
const returnToPath = encodeURIComponent(`/api${req.originalUrl}`);
106106
const redirectTo = `${this.config.hostUrl}oauth-approval?clientID=${client.id}&clientName=${client.name}&returnToPath=${returnToPath}`;
107107
res.redirect(redirectTo);
108108
return false;

0 commit comments

Comments
 (0)