Skip to content

Commit 2508f23

Browse files
geroplgeropl
committed
[server] minor refactor/renames
1 parent 11354aa commit 2508f23

File tree

10 files changed

+114
-121
lines changed

10 files changed

+114
-121
lines changed

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

Lines changed: 34 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import { HostContextProvider } from "./host-context-provider";
2020
import { SignInJWT } from "./jwt";
2121
import { NonceService } from "./nonce-service";
2222
import { getFeatureFlagEnableNonceValidation, getFeatureFlagEnableStrictAuthorizeReturnTo } from "../util/featureflags";
23-
import { validateLoginReturnToUrl, validateAuthorizeReturnToUrl, safeRedirect } from "../express-util";
23+
import { validateLoginReturnToUrl, validateAuthorizeReturnToUrl, safeFragmentRedirect } from "../express-util";
2424

2525
@injectable()
2626
export class Authenticator {
@@ -93,7 +93,7 @@ export class Authenticator {
9393
pathname: req.path,
9494
search: new URL(req.url, this.config.hostUrl.url).search,
9595
});
96-
safeRedirect(res, baseUrl.toString());
96+
safeFragmentRedirect(res, baseUrl.toString());
9797
return;
9898
}
9999

@@ -185,12 +185,14 @@ export class Authenticator {
185185
let returnToParam: string | undefined = req.query.returnTo?.toString();
186186
if (returnToParam) {
187187
log.info(`Stored returnTo URL: ${returnToParam}`, { "login-flow": true });
188-
189-
// Validate returnTo URL against allowlist for login API
190-
if (!validateLoginReturnToUrl(returnToParam, this.config.hostUrl)) {
191-
log.warn(`Invalid returnTo URL rejected for login: ${returnToParam}`, { "login-flow": true });
192-
safeRedirect(res, this.getSorryUrl(`Invalid return URL.`));
193-
return;
188+
const isStrictAuthorizeValidationEnabled = await getFeatureFlagEnableStrictAuthorizeReturnTo();
189+
if (isStrictAuthorizeValidationEnabled) {
190+
// Validate returnTo URL against allowlist for login API
191+
if (!validateLoginReturnToUrl(returnToParam, this.config.hostUrl)) {
192+
log.warn(`Invalid returnTo URL rejected for login: ${returnToParam}`, { "login-flow": true });
193+
safeFragmentRedirect(res, this.getSorryUrl(`Invalid return URL.`));
194+
return;
195+
}
194196
}
195197
}
196198
// returnTo defaults to workspaces url
@@ -202,7 +204,7 @@ export class Authenticator {
202204
const authProvider = host && (await this.getAuthProviderForHost(host));
203205
if (!host || !authProvider) {
204206
log.info(`Bad request: missing parameters.`, { "login-flow": true });
205-
safeRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`));
207+
safeFragmentRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`));
206208
return;
207209
}
208210
// Logins with organizational Git Auth is not permitted
@@ -211,12 +213,12 @@ export class Authenticator {
211213
"authorize-flow": true,
212214
ap: authProvider.info,
213215
});
214-
safeRedirect(res, this.getSorryUrl(`Login with "${host}" is not permitted.`));
216+
safeFragmentRedirect(res, this.getSorryUrl(`Login with "${host}" is not permitted.`));
215217
return;
216218
}
217219
if (this.config.disableDynamicAuthProviderLogin && !authProvider.params.builtin) {
218220
log.info(`Auth Provider is not allowed.`, { ap: authProvider.info });
219-
safeRedirect(res, this.getSorryUrl(`Login with ${authProvider.params.host} is not allowed.`));
221+
safeFragmentRedirect(res, this.getSorryUrl(`Login with ${authProvider.params.host} is not allowed.`));
220222
return;
221223
}
222224

@@ -226,7 +228,7 @@ export class Authenticator {
226228
"login-flow": true,
227229
ap: authProvider.info,
228230
});
229-
safeRedirect(res, this.getSorryUrl(`Login with "${host}" is not permitted.`));
231+
safeFragmentRedirect(res, this.getSorryUrl(`Login with "${host}" is not permitted.`));
230232
return;
231233
}
232234

@@ -248,7 +250,7 @@ export class Authenticator {
248250
const user = req.user;
249251
if (!req.isAuthenticated() || !User.is(user)) {
250252
log.info(`User is not authenticated.`);
251-
safeRedirect(res, this.getSorryUrl(`Not authenticated. Please login.`));
253+
safeFragmentRedirect(res, this.getSorryUrl(`Not authenticated. Please login.`));
252254
return;
253255
}
254256
const returnTo: string = req.query.returnTo?.toString() || this.config.hostUrl.asDashboard().toString();
@@ -258,20 +260,20 @@ export class Authenticator {
258260

259261
if (!host || !authProvider) {
260262
log.warn(`Bad request: missing parameters.`);
261-
safeRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`));
263+
safeFragmentRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`));
262264
return;
263265
}
264266

265267
try {
266268
await this.userAuthentication.deauthorize(user, authProvider.authProviderId);
267-
safeRedirect(res, returnTo);
269+
safeFragmentRedirect(res, returnTo);
268270
} catch (error) {
269271
next(error);
270272
log.error(`Failed to disconnect a provider.`, error, {
271273
host,
272274
userId: user.id,
273275
});
274-
safeRedirect(
276+
safeFragmentRedirect(
275277
res,
276278
this.getSorryUrl(
277279
`Failed to disconnect a provider: ${error && error.message ? error.message : "unknown reason"}`,
@@ -284,12 +286,12 @@ export class Authenticator {
284286
const user = req.user;
285287
if (!req.isAuthenticated() || !User.is(user)) {
286288
log.info(`User is not authenticated.`, { "authorize-flow": true });
287-
safeRedirect(res, this.getSorryUrl(`Not authenticated. Please login.`));
289+
safeFragmentRedirect(res, this.getSorryUrl(`Not authenticated. Please login.`));
288290
return;
289291
}
290292
if (user.id === BUILTIN_INSTLLATION_ADMIN_USER_ID) {
291293
log.info(`Authorization is not permitted for admin user.`);
292-
safeRedirect(
294+
safeFragmentRedirect(
293295
res,
294296
this.getSorryUrl(`Authorization is not permitted for admin user. Please login with a user account.`),
295297
);
@@ -303,24 +305,22 @@ export class Authenticator {
303305

304306
if (!returnToParam || !host || !authProvider) {
305307
log.info(`Bad request: missing parameters.`, { "authorize-flow": true });
306-
safeRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`));
308+
safeFragmentRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`));
307309
return;
308310
}
309311

310312
// Validate returnTo URL against allowlist for authorize API
311313
const isStrictAuthorizeValidationEnabled = await getFeatureFlagEnableStrictAuthorizeReturnTo();
312-
const isValidReturnTo = isStrictAuthorizeValidationEnabled
313-
? validateAuthorizeReturnToUrl(returnToParam, this.config.hostUrl)
314-
: validateLoginReturnToUrl(returnToParam, this.config.hostUrl);
315-
316-
if (!isValidReturnTo) {
317-
const validationType = isStrictAuthorizeValidationEnabled ? "strict authorize" : "login fallback";
318-
log.warn(`Invalid returnTo URL rejected for authorize (${validationType}): ${returnToParam}`, {
319-
"authorize-flow": true,
320-
strictValidation: isStrictAuthorizeValidationEnabled,
321-
});
322-
safeRedirect(res, this.getSorryUrl(`Invalid return URL.`));
323-
return;
314+
if (isStrictAuthorizeValidationEnabled) {
315+
const isValidReturnTo = validateAuthorizeReturnToUrl(returnToParam, this.config.hostUrl);
316+
if (!isValidReturnTo) {
317+
log.warn(`Invalid returnTo URL rejected for authorize`, {
318+
"authorize-flow": true,
319+
returnToParam,
320+
});
321+
safeFragmentRedirect(res, this.getSorryUrl(`Invalid return URL.`));
322+
return;
323+
}
324324
}
325325

326326
const returnTo = returnToParam;
@@ -333,7 +333,7 @@ export class Authenticator {
333333
"authorize-flow": true,
334334
ap: authProvider.info,
335335
});
336-
safeRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
336+
safeFragmentRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
337337
return;
338338
}
339339
}
@@ -344,7 +344,7 @@ export class Authenticator {
344344
"authorize-flow": true,
345345
ap: authProvider.info,
346346
});
347-
safeRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
347+
safeFragmentRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
348348
return;
349349
}
350350

@@ -356,7 +356,7 @@ export class Authenticator {
356356
"authorize-flow": true,
357357
ap: authProvider.info,
358358
});
359-
safeRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
359+
safeFragmentRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`));
360360
return;
361361
}
362362
}

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

Lines changed: 12 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, safeRedirect } from "../express-util";
26+
import { getRequestingClientInfo, safeFragmentRedirect } 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,7 @@ 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-
safeRedirect(
290+
safeFragmentRedirect(
291291
response,
292292
this.getSorryUrl(`No state was present on the authentication callback. Please try again.`),
293293
);
@@ -299,7 +299,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
299299
log.error(`(${strategyName}) Auth flow state is missing.`);
300300

301301
reportLoginCompleted("failed", "git");
302-
safeRedirect(response, this.getSorryUrl(`Auth flow state is missing.`));
302+
safeFragmentRedirect(response, this.getSorryUrl(`Auth flow state is missing.`));
303303
return;
304304
}
305305

@@ -310,7 +310,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
310310
`(${strategyName}) User is already logged in. No auth info provided. Redirecting to dashboard.`,
311311
{ clientInfo },
312312
);
313-
safeRedirect(response, this.config.hostUrl.asDashboard().toString());
313+
safeFragmentRedirect(response, this.config.hostUrl.asDashboard().toString());
314314
return;
315315
}
316316
}
@@ -321,15 +321,18 @@ export abstract class GenericAuthProvider implements AuthProvider {
321321
reportLoginCompleted("failed_client", "git");
322322

323323
log.error(cxt, `(${strategyName}) No session found during auth callback.`, { clientInfo });
324-
safeRedirect(response, this.getSorryUrl(`Please allow Cookies in your browser and try to log in again.`));
324+
safeFragmentRedirect(
325+
response,
326+
this.getSorryUrl(`Please allow Cookies in your browser and try to log in again.`),
327+
);
325328
return;
326329
}
327330

328331
if (authFlow.host !== this.host) {
329332
reportLoginCompleted("failed", "git");
330333

331334
log.error(cxt, `(${strategyName}) Host does not match.`, { clientInfo });
332-
safeRedirect(response, this.getSorryUrl(`Host does not match.`));
335+
safeFragmentRedirect(response, this.getSorryUrl(`Host does not match.`));
333336
return;
334337
}
335338

@@ -360,7 +363,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
360363
authenticate(request, response, next);
361364
});
362365
} catch (error) {
363-
safeRedirect(response, this.getSorryUrl(`OAuth2 error. (${error})`));
366+
safeFragmentRedirect(response, this.getSorryUrl(`OAuth2 error. (${error})`));
364367
return;
365368
}
366369
const [err, userOrIdentity, flowContext] = result;
@@ -472,7 +475,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
472475
);
473476

474477
const { returnTo } = authFlow;
475-
safeRedirect(response, returnTo);
478+
safeFragmentRedirect(response, returnTo);
476479
return;
477480
} else {
478481
// Complete login into an existing account
@@ -537,7 +540,7 @@ export abstract class GenericAuthProvider implements AuthProvider {
537540
search: "message=error:" + Buffer.from(JSON.stringify(error), "utf-8").toString("base64"),
538541
})
539542
.toString();
540-
safeRedirect(response, url);
543+
safeFragmentRedirect(response, url);
541544
}
542545

543546
/**

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

Lines changed: 6 additions & 3 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 { safeRedirect } from "../express-util";
19+
import { safeFragmentRedirect } 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,7 +50,10 @@ 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-
safeRedirect(response, this.config.hostUrl.asSorry("Oops! Something went wrong during login.").toString());
53+
safeFragmentRedirect(
54+
response,
55+
this.config.hostUrl.asSorry("Oops! Something went wrong during login.").toString(),
56+
);
5457
return;
5558
}
5659

@@ -89,7 +92,7 @@ export class LoginCompletionHandler {
8992

9093
log.info(logContext, `User is logged in successfully. Redirect to: ${returnTo}`);
9194
reportLoginCompleted("succeeded", "git");
92-
safeRedirect(response, returnTo);
95+
safeFragmentRedirect(response, returnTo);
9396
}
9497

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

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

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import * as crypto from "crypto";
1010
import { IncomingHttpHeaders } from "http";
1111
import { GitpodHostUrl } from "@gitpod/gitpod-protocol/lib/util/gitpod-host-url";
1212
import { ensureUrlHasFragment } from "./auth/fragment-utils";
13+
import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
14+
import { TrustedValue } from "@gitpod/gitpod-protocol/lib/util/scrubbing";
1315

1416
export const query = (...tuples: [string, string][]) => {
1517
if (tuples.length === 0) {
@@ -169,14 +171,14 @@ export function toClientHeaderFields(expressReq: express.Request): ClientHeaderF
169171
* @param allowedPatterns Array of regex patterns that are allowed for the pathname
170172
* @returns true if the URL is valid, false otherwise
171173
*/
172-
export function validateReturnToUrlWithPatterns(
174+
function validateReturnToUrlWithPatterns(
173175
returnTo: string,
174-
hostUrl: GitpodHostUrl,
175-
allowedPatterns: RegExp[],
176+
allowedBaseUrl: GitpodHostUrl,
177+
allowedPatterns?: RegExp[],
176178
): boolean {
177179
try {
178180
const url = new URL(returnTo);
179-
const baseUrl = hostUrl.url;
181+
const baseUrl = allowedBaseUrl.url;
180182

181183
// Must be same origin OR www.gitpod.io exception
182184
const isSameOrigin = url.origin === baseUrl.origin;
@@ -191,7 +193,7 @@ export function validateReturnToUrlWithPatterns(
191193
return url.pathname === "/";
192194
}
193195

194-
if (allowedPatterns && allowedPatterns.length != 0) {
196+
if (allowedPatterns !== undefined) {
195197
// Check if pathname matches any allowed pattern
196198
const isAllowedPath = allowedPatterns.some((pattern) => pattern.test(url.pathname));
197199
if (!isAllowedPath) {
@@ -218,8 +220,8 @@ export function validateReturnToUrlWithPatterns(
218220
* Login API allows broader navigation after authentication.
219221
*/
220222
export function validateLoginReturnToUrl(returnTo: string, hostUrl: GitpodHostUrl): boolean {
221-
// We have already verified the domain above, and we do not restrict the redirect location for loginReturnToUrl.
222-
return validateReturnToUrlWithPatterns(returnTo, hostUrl, []);
223+
// We just verify the domain
224+
return validateReturnToUrlWithPatterns(returnTo, hostUrl, undefined);
223225
}
224226

225227
/**
@@ -240,6 +242,21 @@ export function validateAuthorizeReturnToUrl(returnTo: string, hostUrl: GitpodHo
240242
return validateReturnToUrlWithPatterns(returnTo, hostUrl, allowedPatterns);
241243
}
242244

245+
export function getSafeReturnToParam(req: express.Request, validator: (url: string) => boolean): string | undefined {
246+
// @ts-ignore Type 'ParsedQs' is not assignable
247+
const returnToURL: string | undefined = req.query.redirect || req.query.returnTo;
248+
if (!returnToURL) {
249+
return;
250+
}
251+
252+
if (!validator(returnToURL)) {
253+
log.debug("The redirect URL does not match allowed patterns", { query: new TrustedValue(req.query).value });
254+
return;
255+
}
256+
257+
return returnToURL;
258+
}
259+
243260
/**
244261
* Safe redirect wrapper that automatically ensures URLs have fragments to prevent
245262
* OAuth token inheritance attacks.
@@ -252,7 +269,7 @@ export function validateAuthorizeReturnToUrl(returnTo: string, hostUrl: GitpodHo
252269
* @param url URL to redirect to
253270
* @param status Optional HTTP status code (default: 302)
254271
*/
255-
export function safeRedirect(res: express.Response, url: string, status?: number): void {
272+
export function safeFragmentRedirect(res: express.Response, url: string, status?: number): void {
256273
const protectedUrl = ensureUrlHasFragment(url);
257274
if (status) {
258275
res.redirect(status, protectedUrl);

0 commit comments

Comments
 (0)