Skip to content

Commit 89f028c

Browse files
geroplona-agent
geropl
and
ona-agent
committed
Validate OAuth returnTo URLs to prevent API access
- Add URL validation in authenticator to reject returnTo URLs pointing to /api paths or api. subdomains - Move isApiPath method into validateAuthorizeReturnToUrl for better encapsulation - Maintain existing prefix-based domain validation in getReturnToParamWithSafeBaseDomain - Add comprehensive tests for validation functions - Covers edge cases: case insensitivity, malformed URLs, query params Co-authored-by: Ona <no-reply@ona.com>
1 parent c0a7be3 commit 89f028c

File tree

6 files changed

+343
-14
lines changed

6 files changed

+343
-14
lines changed

‎components/server/src/auth/authenticator.spec.ts

Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,189 @@
1+
/**
2+
* Copyright (c) 2020 Gitpod GmbH. All rights reserved.
3+
* Licensed under the GNU Affero General Public License (AGPL).
4+
* See License.AGPL.txt in the project root for license information.
5+
*/
6+
7+
import * as chai from "chai";
8+
import { validateAuthorizeReturnToUrl } from "./authenticator";
9+
import { Config } from "../config";
10+
import { AuthProvider } from "./auth-provider";
11+
12+
const expect = chai.expect;
13+
14+
describe("authenticator", function () {
15+
describe("validateAuthorizeReturnToUrl", function () {
16+
const mockConfig = {
17+
hostUrl: {
18+
url: new URL("https://gitpod.io"),
19+
},
20+
} as Config;
21+
22+
const mockAuthProvider = {} as AuthProvider;
23+
24+
describe("valid URLs", function () {
25+
it("should return true for valid URLs from configured domain", function () {
26+
const result = validateAuthorizeReturnToUrl(
27+
"https://gitpod.io/dashboard",
28+
mockConfig,
29+
mockAuthProvider,
30+
);
31+
expect(result).to.be.true;
32+
});
33+
34+
it("should return true for valid URLs from www.gitpod.io", function () {
35+
const result = validateAuthorizeReturnToUrl(
36+
"https://www.gitpod.io/pricing",
37+
mockConfig,
38+
mockAuthProvider,
39+
);
40+
expect(result).to.be.true;
41+
});
42+
43+
it("should return false for URLs with subdomain that don't match prefix", function () {
44+
const result = validateAuthorizeReturnToUrl(
45+
"https://app.gitpod.io/workspaces",
46+
mockConfig,
47+
mockAuthProvider,
48+
);
49+
expect(result).to.be.false;
50+
});
51+
});
52+
53+
describe("invalid URLs - API paths", function () {
54+
it("should return false for URLs with /api path", function () {
55+
const result = validateAuthorizeReturnToUrl(
56+
"https://gitpod.io/api/authorize",
57+
mockConfig,
58+
mockAuthProvider,
59+
);
60+
expect(result).to.be.false;
61+
});
62+
63+
it("should return false for URLs with /api path", function () {
64+
const result = validateAuthorizeReturnToUrl(
65+
"https://gitpod.io/api/users",
66+
mockConfig,
67+
mockAuthProvider,
68+
);
69+
expect(result).to.be.false;
70+
});
71+
72+
it("should return false for URLs with /api/ path", function () {
73+
const result = validateAuthorizeReturnToUrl("https://gitpod.io/api/", mockConfig, mockAuthProvider);
74+
expect(result).to.be.false;
75+
});
76+
77+
it("should return false for URLs with /API path (case insensitive)", function () {
78+
const result = validateAuthorizeReturnToUrl(
79+
"https://gitpod.io/API/users",
80+
mockConfig,
81+
mockAuthProvider,
82+
);
83+
expect(result).to.be.false;
84+
});
85+
86+
it("should return false for URLs with /api/callback path", function () {
87+
const result = validateAuthorizeReturnToUrl(
88+
"https://gitpod.io/api/callback",
89+
mockConfig,
90+
mockAuthProvider,
91+
);
92+
expect(result).to.be.false;
93+
});
94+
95+
it("should return false for URLs with api. subdomain", function () {
96+
const result = validateAuthorizeReturnToUrl(
97+
"https://api.gitpod.io/users",
98+
mockConfig,
99+
mockAuthProvider,
100+
);
101+
expect(result).to.be.false;
102+
});
103+
104+
it("should return false for URLs with API. subdomain (case insensitive)", function () {
105+
const result = validateAuthorizeReturnToUrl(
106+
"https://API.gitpod.io/users",
107+
mockConfig,
108+
mockAuthProvider,
109+
);
110+
expect(result).to.be.false;
111+
});
112+
113+
it("should return false for URLs with api. subdomain and /callback path", function () {
114+
const result = validateAuthorizeReturnToUrl(
115+
"https://api.gitpod.io/callback",
116+
mockConfig,
117+
mockAuthProvider,
118+
);
119+
expect(result).to.be.false;
120+
});
121+
});
122+
123+
describe("invalid URLs - external domains", function () {
124+
it("should return false for URLs from external domains", function () {
125+
const result = validateAuthorizeReturnToUrl("https://evil.com/dashboard", mockConfig, mockAuthProvider);
126+
expect(result).to.be.false;
127+
});
128+
129+
it("should return false for URLs that look similar but are external", function () {
130+
const result = validateAuthorizeReturnToUrl(
131+
"https://gitpod.io.evil.com/dashboard",
132+
mockConfig,
133+
mockAuthProvider,
134+
);
135+
expect(result).to.be.false;
136+
});
137+
});
138+
139+
describe("malformed URLs", function () {
140+
it("should return false for malformed URLs", function () {
141+
const result = validateAuthorizeReturnToUrl("not-a-url", mockConfig, mockAuthProvider);
142+
expect(result).to.be.false;
143+
});
144+
145+
it("should return false for URLs with invalid protocol", function () {
146+
const result = validateAuthorizeReturnToUrl("javascript:alert('xss')", mockConfig, mockAuthProvider);
147+
expect(result).to.be.false;
148+
});
149+
});
150+
151+
describe("edge cases", function () {
152+
it("should return true for URLs with query parameters", function () {
153+
const result = validateAuthorizeReturnToUrl(
154+
"https://gitpod.io/dashboard?tab=workspaces",
155+
mockConfig,
156+
mockAuthProvider,
157+
);
158+
expect(result).to.be.true;
159+
});
160+
161+
it("should return true for URLs with fragments", function () {
162+
const result = validateAuthorizeReturnToUrl(
163+
"https://gitpod.io/dashboard#workspaces",
164+
mockConfig,
165+
mockAuthProvider,
166+
);
167+
expect(result).to.be.true;
168+
});
169+
170+
it("should return true for URLs with 'api' in query parameter but not in path", function () {
171+
const result = validateAuthorizeReturnToUrl(
172+
"https://gitpod.io/dashboard?redirect=/api",
173+
mockConfig,
174+
mockAuthProvider,
175+
);
176+
expect(result).to.be.true;
177+
});
178+
179+
it("should return true for URLs with 'api' in path but not starting with /api", function () {
180+
const result = validateAuthorizeReturnToUrl(
181+
"https://gitpod.io/graphql-api",
182+
mockConfig,
183+
mockAuthProvider,
184+
);
185+
expect(result).to.be.true;
186+
});
187+
});
188+
});
189+
});

‎components/server/src/auth/authenticator.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import { UserService } from "../user/user-service";
1818
import { AuthFlow, AuthProvider } from "./auth-provider";
1919
import { HostContextProvider } from "./host-context-provider";
2020
import { SignInJWT } from "./jwt";
21+
import { getReturnToParamWithSafeBaseDomain } from "../express-util";
2122

2223
@injectable()
2324
export class Authenticator {
@@ -239,6 +240,14 @@ export class Authenticator {
239240
return;
240241
}
241242

243+
// Validate returnTo URL
244+
const valid = validateAuthorizeReturnToUrl(returnTo, this.config, authProvider);
245+
if (!valid) {
246+
log.info(`Bad request: invalid returnTo URL.`, { "authorize-flow": true });
247+
res.redirect(this.getSorryUrl(`Bad request: invalid returnTo URL.`));
248+
return;
249+
}
250+
242251
// For non-verified org auth provider, ensure user is an owner of the org
243252
if (!authProvider.info.verified && authProvider.info.organizationId) {
244253
const member = await this.teamDb.findTeamMembership(user.id, authProvider.info.organizationId);
@@ -322,3 +331,35 @@ export class Authenticator {
322331
return this.config.hostUrl.asSorry(message).toString();
323332
}
324333
}
334+
335+
// Whether the passed URL is a valid returnTo URL for OAuth authorization
336+
export function validateAuthorizeReturnToUrl(returnTo: string, config: Config, authProvider: AuthProvider): boolean {
337+
function isApiPath(url: string): boolean {
338+
try {
339+
const parsedUrl = new URL(url);
340+
341+
// Check if hostname starts with "api."
342+
if (parsedUrl.hostname.toLowerCase().startsWith("api.")) {
343+
return true;
344+
}
345+
346+
// Check if path starts with "/api"
347+
if (parsedUrl.pathname.toLowerCase().startsWith("/api")) {
348+
return true;
349+
}
350+
351+
return false;
352+
} catch {
353+
// If URL parsing fails, consider it invalid
354+
return true;
355+
}
356+
}
357+
358+
// Reject URLs that point to API endpoints
359+
if (isApiPath(returnTo)) {
360+
return false;
361+
}
362+
363+
const validUrl = getReturnToParamWithSafeBaseDomain(returnTo, config.hostUrl.url);
364+
return validUrl !== undefined;
365+
}

‎components/server/src/express-util.spec.ts

Lines changed: 79 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
*/
66

77
import * as chai from "chai";
8-
import { isAllowedWebsocketDomain } from "./express-util";
8+
import { isAllowedWebsocketDomain, getReturnToParamWithSafeBaseDomain } from "./express-util";
99
const expect = chai.expect;
1010

1111
describe("express-util", function () {
@@ -47,4 +47,82 @@ describe("express-util", function () {
4747
expect(result).to.be.false;
4848
});
4949
});
50+
51+
describe("getReturnToParamWithSafeBaseDomain", function () {
52+
const configuredBaseDomain = new URL("https://gitpod.io");
53+
54+
describe("valid URLs", function () {
55+
it("should allow URLs from configured base domain", function () {
56+
const result = getReturnToParamWithSafeBaseDomain("https://gitpod.io/dashboard", configuredBaseDomain);
57+
expect(result).to.equal("https://gitpod.io/dashboard");
58+
});
59+
60+
it("should allow URLs from www.gitpod.io", function () {
61+
const result = getReturnToParamWithSafeBaseDomain(
62+
"https://www.gitpod.io/pricing",
63+
configuredBaseDomain,
64+
);
65+
expect(result).to.equal("https://www.gitpod.io/pricing");
66+
});
67+
68+
it("should reject URLs with subdomain that don't match prefix", function () {
69+
const result = getReturnToParamWithSafeBaseDomain(
70+
"https://app.gitpod.io/workspaces",
71+
configuredBaseDomain,
72+
);
73+
expect(result).to.be.undefined;
74+
});
75+
76+
it("should handle undefined returnToURL", function () {
77+
const result = getReturnToParamWithSafeBaseDomain(undefined, configuredBaseDomain);
78+
expect(result).to.be.undefined;
79+
});
80+
81+
it("should handle empty returnToURL", function () {
82+
const result = getReturnToParamWithSafeBaseDomain("", configuredBaseDomain);
83+
expect(result).to.be.undefined;
84+
});
85+
});
86+
87+
describe("invalid URLs - external domains", function () {
88+
it("should reject URLs from external domains", function () {
89+
const result = getReturnToParamWithSafeBaseDomain("https://evil.com/dashboard", configuredBaseDomain);
90+
expect(result).to.be.undefined;
91+
});
92+
93+
it("should reject URLs that look similar but are external", function () {
94+
const result = getReturnToParamWithSafeBaseDomain(
95+
"https://gitpod.io.evil.com/dashboard",
96+
configuredBaseDomain,
97+
);
98+
expect(result).to.be.undefined;
99+
});
100+
});
101+
102+
describe("edge cases", function () {
103+
it("should allow URLs with query parameters", function () {
104+
const result = getReturnToParamWithSafeBaseDomain(
105+
"https://gitpod.io/dashboard?tab=workspaces",
106+
configuredBaseDomain,
107+
);
108+
expect(result).to.equal("https://gitpod.io/dashboard?tab=workspaces");
109+
});
110+
111+
it("should allow URLs with fragments", function () {
112+
const result = getReturnToParamWithSafeBaseDomain(
113+
"https://gitpod.io/dashboard#workspaces",
114+
configuredBaseDomain,
115+
);
116+
expect(result).to.equal("https://gitpod.io/dashboard#workspaces");
117+
});
118+
119+
it("should allow URLs with 'api' in path but not starting with /api", function () {
120+
const result = getReturnToParamWithSafeBaseDomain(
121+
"https://gitpod.io/graphql-api",
122+
configuredBaseDomain,
123+
);
124+
expect(result).to.equal("https://gitpod.io/graphql-api");
125+
});
126+
});
127+
});
50128
});

‎components/server/src/express-util.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,3 +159,26 @@ export function toClientHeaderFields(expressReq: express.Request): ClientHeaderF
159159
clientRegion: takeFirst(expressReq.headers["x-glb-client-region"]),
160160
};
161161
}
162+
163+
export function getReturnToParamWithSafeBaseDomain(
164+
returnToURL: string | undefined,
165+
configuredBaseDomain: URL,
166+
): string | undefined {
167+
function urlStartsWith(url: string, prefixUrl: string): boolean {
168+
prefixUrl += prefixUrl.endsWith("/") ? "" : "/";
169+
return url.toLowerCase().startsWith(prefixUrl.toLowerCase());
170+
}
171+
172+
if (!returnToURL) {
173+
return undefined;
174+
}
175+
176+
if (
177+
urlStartsWith(returnToURL, configuredBaseDomain.toString()) ||
178+
urlStartsWith(returnToURL, "https://www.gitpod.io")
179+
) {
180+
return returnToURL;
181+
}
182+
183+
return;
184+
}

0 commit comments

Comments
 (0)