Skip to content

Commit 8a7443e

Browse files
iQQBotiQQBot
committed
use safeRedirect for redirect
1 parent 6d200db commit 8a7443e

File tree

10 files changed

+87
-57
lines changed

10 files changed

+87
-57
lines changed

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

Lines changed: 23 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,8 @@ import { AuthFlow, AuthProvider } from "./auth-provider";
1919
import { HostContextProvider } from "./host-context-provider";
2020
import { SignInJWT } from "./jwt";
2121
import { NonceService } from "./nonce-service";
22-
import { ensureUrlHasFragment } from "./fragment-utils";
2322
import { getFeatureFlagEnableNonceValidation, getFeatureFlagEnableStrictAuthorizeReturnTo } from "../util/featureflags";
24-
import { validateLoginReturnToUrl, validateAuthorizeReturnToUrl } from "../express-util";
23+
import { validateLoginReturnToUrl, validateAuthorizeReturnToUrl, safeRedirect } from "../express-util";
2524

2625
@injectable()
2726
export class Authenticator {
@@ -94,7 +93,7 @@ export class Authenticator {
9493
pathname: req.path,
9594
search: new URL(req.url, this.config.hostUrl.url).search,
9695
});
97-
res.redirect(baseUrl.toString());
96+
safeRedirect(res, baseUrl.toString());
9897
return;
9998
}
10099

@@ -190,21 +189,20 @@ export class Authenticator {
190189
// Validate returnTo URL against allowlist for login API
191190
if (!validateLoginReturnToUrl(returnToParam, this.config.hostUrl)) {
192191
log.warn(`Invalid returnTo URL rejected for login: ${returnToParam}`, { "login-flow": true });
193-
res.redirect(this.getSorryUrl(`Invalid return URL.`));
192+
safeRedirect(res, this.getSorryUrl(`Invalid return URL.`));
194193
return;
195194
}
196195
}
197196
// returnTo defaults to workspaces url
198197
const workspaceUrl = this.config.hostUrl.asDashboard().toString();
199198
returnToParam = returnToParam || workspaceUrl;
200-
// Ensure returnTo URL has a fragment to prevent OAuth token inheritance attacks
201-
const returnTo = ensureUrlHasFragment(returnToParam);
199+
const returnTo = returnToParam;
202200

203201
const host: string = req.query.host?.toString() || "";
204202
const authProvider = host && (await this.getAuthProviderForHost(host));
205203
if (!host || !authProvider) {
206204
log.info(`Bad request: missing parameters.`, { "login-flow": true });
207-
res.redirect(this.getSorryUrl(`Bad request: missing parameters.`));
205+
safeRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`));
208206
return;
209207
}
210208
// Logins with organizational Git Auth is not permitted
@@ -213,12 +211,12 @@ export class Authenticator {
213211
"authorize-flow": true,
214212
ap: authProvider.info,
215213
});
216-
res.redirect(this.getSorryUrl(`Login with "${host}" is not permitted.`));
214+
safeRedirect(res, this.getSorryUrl(`Login with "${host}" is not permitted.`));
217215
return;
218216
}
219217
if (this.config.disableDynamicAuthProviderLogin && !authProvider.params.builtin) {
220218
log.info(`Auth Provider is not allowed.`, { ap: authProvider.info });
221-
res.redirect(this.getSorryUrl(`Login with ${authProvider.params.host} is not allowed.`));
219+
safeRedirect(res, this.getSorryUrl(`Login with ${authProvider.params.host} is not allowed.`));
222220
return;
223221
}
224222

@@ -228,7 +226,7 @@ export class Authenticator {
228226
"login-flow": true,
229227
ap: authProvider.info,
230228
});
231-
res.redirect(this.getSorryUrl(`Login with "${host}" is not permitted.`));
229+
safeRedirect(res, this.getSorryUrl(`Login with "${host}" is not permitted.`));
232230
return;
233231
}
234232

@@ -250,7 +248,7 @@ export class Authenticator {
250248
const user = req.user;
251249
if (!req.isAuthenticated() || !User.is(user)) {
252250
log.info(`User is not authenticated.`);
253-
res.redirect(this.getSorryUrl(`Not authenticated. Please login.`));
251+
safeRedirect(res, this.getSorryUrl(`Not authenticated. Please login.`));
254252
return;
255253
}
256254
const returnTo: string = req.query.returnTo?.toString() || this.config.hostUrl.asDashboard().toString();
@@ -260,20 +258,21 @@ export class Authenticator {
260258

261259
if (!host || !authProvider) {
262260
log.warn(`Bad request: missing parameters.`);
263-
res.redirect(this.getSorryUrl(`Bad request: missing parameters.`));
261+
safeRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`));
264262
return;
265263
}
266264

267265
try {
268266
await this.userAuthentication.deauthorize(user, authProvider.authProviderId);
269-
res.redirect(returnTo);
267+
safeRedirect(res, returnTo);
270268
} catch (error) {
271269
next(error);
272270
log.error(`Failed to disconnect a provider.`, error, {
273271
host,
274272
userId: user.id,
275273
});
276-
res.redirect(
274+
safeRedirect(
275+
res,
277276
this.getSorryUrl(
278277
`Failed to disconnect a provider: ${error && error.message ? error.message : "unknown reason"}`,
279278
),
@@ -285,12 +284,13 @@ export class Authenticator {
285284
const user = req.user;
286285
if (!req.isAuthenticated() || !User.is(user)) {
287286
log.info(`User is not authenticated.`, { "authorize-flow": true });
288-
res.redirect(this.getSorryUrl(`Not authenticated. Please login.`));
287+
safeRedirect(res, this.getSorryUrl(`Not authenticated. Please login.`));
289288
return;
290289
}
291290
if (user.id === BUILTIN_INSTLLATION_ADMIN_USER_ID) {
292291
log.info(`Authorization is not permitted for admin user.`);
293-
res.redirect(
292+
safeRedirect(
293+
res,
294294
this.getSorryUrl(`Authorization is not permitted for admin user. Please login with a user account.`),
295295
);
296296
return;
@@ -303,7 +303,7 @@ export class Authenticator {
303303

304304
if (!returnToParam || !host || !authProvider) {
305305
log.info(`Bad request: missing parameters.`, { "authorize-flow": true });
306-
res.redirect(this.getSorryUrl(`Bad request: missing parameters.`));
306+
safeRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`));
307307
return;
308308
}
309309

@@ -319,12 +319,11 @@ export class Authenticator {
319319
"authorize-flow": true,
320320
strictValidation: isStrictAuthorizeValidationEnabled,
321321
});
322-
res.redirect(this.getSorryUrl(`Invalid return URL.`));
322+
safeRedirect(res, this.getSorryUrl(`Invalid return URL.`));
323323
return;
324324
}
325325

326-
// Ensure returnTo URL has a fragment to prevent OAuth token inheritance attacks
327-
const returnTo = ensureUrlHasFragment(returnToParam);
326+
const returnTo = returnToParam;
328327

329328
// For non-verified org auth provider, ensure user is an owner of the org
330329
if (!authProvider.info.verified && authProvider.info.organizationId) {
@@ -334,7 +333,7 @@ export class Authenticator {
334333
"authorize-flow": true,
335334
ap: authProvider.info,
336335
});
337-
res.redirect(this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
336+
safeRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
338337
return;
339338
}
340339
}
@@ -345,7 +344,7 @@ export class Authenticator {
345344
"authorize-flow": true,
346345
ap: authProvider.info,
347346
});
348-
res.redirect(this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
347+
safeRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
349348
return;
350349
}
351350

@@ -357,7 +356,7 @@ export class Authenticator {
357356
"authorize-flow": true,
358357
ap: authProvider.info,
359358
});
360-
res.redirect(this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
359+
safeRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
361360
return;
362361
}
363362
}
@@ -411,6 +410,6 @@ export class Authenticator {
411410
return [];
412411
}
413412
private getSorryUrl(message: string) {
414-
return ensureUrlHasFragment(this.config.hostUrl.asSorry(message).toString());
413+
return this.config.hostUrl.asSorry(message).toString();
415414
}
416415
}

‎components/server/src/auth/generic-auth-provider.ts‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import {
2323
UnconfirmedUserException,
2424
} from "../auth/errors";
2525
import { Config } from "../config";
26-
import { getRequestingClientInfo } from "../express-util";
26+
import { getRequestingClientInfo, safeRedirect } from "../express-util";
2727
import { TokenProvider } from "../user/token-provider";
2828
import { UserAuthentication } from "../user/user-authentication";
2929
import { AuthProviderService } from "./auth-provider-service";
@@ -287,7 +287,8 @@ export abstract class GenericAuthProvider implements AuthProvider {
287287
const state = request.query.state;
288288
if (!state) {
289289
log.error(cxt, `(${strategyName}) No state present on callback request.`, { clientInfo });
290-
response.redirect(
290+
safeRedirect(
291+
response,
291292
this.getSorryUrl(`No state was present on the authentication callback. Please try again.`),
292293
);
293294
return;
@@ -298,7 +299,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
298299
log.error(`(${strategyName}) Auth flow state is missing.`);
299300

300301
reportLoginCompleted("failed", "git");
301-
response.redirect(this.getSorryUrl(`Auth flow state is missing.`));
302+
safeRedirect(response, this.getSorryUrl(`Auth flow state is missing.`));
302303
return;
303304
}
304305

@@ -309,7 +310,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
309310
`(${strategyName}) User is already logged in. No auth info provided. Redirecting to dashboard.`,
310311
{ clientInfo },
311312
);
312-
response.redirect(this.config.hostUrl.asDashboard().toString());
313+
safeRedirect(response, this.config.hostUrl.asDashboard().toString());
313314
return;
314315
}
315316
}
@@ -320,15 +321,15 @@ export abstract class GenericAuthProvider implements AuthProvider {
320321
reportLoginCompleted("failed_client", "git");
321322

322323
log.error(cxt, `(${strategyName}) No session found during auth callback.`, { clientInfo });
323-
response.redirect(this.getSorryUrl(`Please allow Cookies in your browser and try to log in again.`));
324+
safeRedirect(response, this.getSorryUrl(`Please allow Cookies in your browser and try to log in again.`));
324325
return;
325326
}
326327

327328
if (authFlow.host !== this.host) {
328329
reportLoginCompleted("failed", "git");
329330

330331
log.error(cxt, `(${strategyName}) Host does not match.`, { clientInfo });
331-
response.redirect(this.getSorryUrl(`Host does not match.`));
332+
safeRedirect(response, this.getSorryUrl(`Host does not match.`));
332333
return;
333334
}
334335

@@ -359,7 +360,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
359360
authenticate(request, response, next);
360361
});
361362
} catch (error) {
362-
response.redirect(this.getSorryUrl(`OAuth2 error. (${error})`));
363+
safeRedirect(response, this.getSorryUrl(`OAuth2 error. (${error})`));
363364
return;
364365
}
365366
const [err, userOrIdentity, flowContext] = result;
@@ -471,7 +472,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
471472
);
472473

473474
const { returnTo } = authFlow;
474-
response.redirect(returnTo);
475+
safeRedirect(response, returnTo);
475476
return;
476477
} else {
477478
// Complete login into an existing account
@@ -536,7 +537,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
536537
search: "message=error:" + Buffer.from(JSON.stringify(error), "utf-8").toString("base64"),
537538
})
538539
.toString();
539-
response.redirect(url);
540+
safeRedirect(response, url);
540541
}
541542

542543
/**

‎components/server/src/auth/login-completion-handler.ts‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics";
1616
import { trackLogin } from "../analytics";
1717
import { SessionHandler } from "../session-handler";
1818
import { AuthJWT } from "./jwt";
19-
import { ensureUrlHasFragment } from "./fragment-utils";
19+
import { safeRedirect } from "../express-util";
2020

2121
/**
2222
* The login completion handler pulls the strings between the OAuth2 flow, the ToS flow, and the session management.
@@ -50,15 +50,13 @@ export class LoginCompletionHandler {
5050
} catch (err) {
5151
reportLoginCompleted("failed", "git");
5252
log.error(logContext, `Failed to login user. Redirecting to /sorry on login.`, err);
53-
response.redirect(this.config.hostUrl.asSorry("Oops! Something went wrong during login.").toString());
53+
safeRedirect(response, this.config.hostUrl.asSorry("Oops! Something went wrong during login.").toString());
5454
return;
5555
}
5656

5757
// Update session info
5858
const returnToParam = returnToUrl || this.config.hostUrl.asDashboard().toString();
59-
60-
// Ensure returnTo URL has a fragment to prevent OAuth token inheritance attacks
61-
let returnTo = ensureUrlHasFragment(returnToParam);
59+
let returnTo = returnToParam;
6260

6361
if (elevateScopes) {
6462
const elevateScopesUrl = this.config.hostUrl
@@ -91,7 +89,7 @@ export class LoginCompletionHandler {
9189

9290
log.info(logContext, `User is logged in successfully. Redirect to: ${returnTo}`);
9391
reportLoginCompleted("succeeded", "git");
94-
response.redirect(returnTo);
92+
safeRedirect(response, returnTo);
9593
}
9694

9795
public async updateAuthProviderAsVerified(hostname: string, user: User) {

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import express from "express";
99
import * as crypto from "crypto";
1010
import { IncomingHttpHeaders } from "http";
1111
import { GitpodHostUrl } from "@gitpod/gitpod-protocol/lib/util/gitpod-host-url";
12+
import { ensureUrlHasFragment } from "./auth/fragment-utils";
1213

1314
export const query = (...tuples: [string, string][]) => {
1415
if (tuples.length === 0) {
@@ -236,3 +237,24 @@ export function validateAuthorizeReturnToUrl(returnTo: string, hostUrl: GitpodHo
236237

237238
return validateReturnToUrlWithPatterns(returnTo, hostUrl, allowedPatterns);
238239
}
240+
241+
/**
242+
* Safe redirect wrapper that automatically ensures URLs have fragments to prevent
243+
* OAuth token inheritance attacks.
244+
*
245+
* When OAuth providers redirect with tokens in URL fragments, browsers inherit
246+
* fragments from the current page if the target URL doesn't have one. This wrapper
247+
* automatically applies fragment protection to all redirects.
248+
*
249+
* @param res Express response object
250+
* @param url URL to redirect to
251+
* @param status Optional HTTP status code (default: 302)
252+
*/
253+
export function safeRedirect(res: express.Response, url: string, status?: number): void {
254+
const protectedUrl = ensureUrlHasFragment(url);
255+
if (status) {
256+
res.redirect(status, protectedUrl);
257+
} else {
258+
res.redirect(protectedUrl);
259+
}
260+
}

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import express from "express";
1414
import { inject, injectable } from "inversify";
1515
import { URL } from "url";
1616
import { Config } from "../config";
17+
import { safeRedirect } from "../express-util";
1718
import { clientRepository, createAuthorizationServer } from "./oauth-authorization-server";
1819
import { inMemoryDatabase, toolboxClient } from "./db";
1920
import { getFeatureFlagEnableExperimentalJBTB } from "../util/featureflags";
@@ -28,7 +29,7 @@ export class OAuthController {
2829
if (!req.isAuthenticated() || !User.is(req.user)) {
2930
const returnToPath = encodeURIComponent(`/api${req.originalUrl}`);
3031
const redirectTo = `${this.config.hostUrl}login?returnToPath=${returnToPath}`;
31-
res.redirect(redirectTo);
32+
safeRedirect(res, redirectTo);
3233
return null;
3334
}
3435
const user = req.user as User;
@@ -88,7 +89,7 @@ export class OAuthController {
8889

8990
const redirectUri = new URL(req.query.redirect_uri);
9091
redirectUri.searchParams.append("approved", "no");
91-
res.redirect(redirectUri.toString());
92+
safeRedirect(res, redirectUri.toString());
9293
return false;
9394
} else if (wasApproved == "yes") {
9495
const additionalData = (user.additionalData = user.additionalData || {});
@@ -104,7 +105,7 @@ export class OAuthController {
104105
if (client) {
105106
const returnToPath = encodeURIComponent(`/api${req.originalUrl}`);
106107
const redirectTo = `${this.config.hostUrl}oauth-approval?clientID=${client.id}&clientName=${client.name}&returnToPath=${returnToPath}`;
107-
res.redirect(redirectTo);
108+
safeRedirect(res, redirectTo);
108109
return false;
109110
} else {
110111
log.error(`/oauth/authorize unknown client id: "${clientID}"`);

0 commit comments

Comments
 (0)