moah changes

Author: geropl

components/server/src/auth/authenticator.ts

@@ -185,14 +185,11 @@ export class Authenticator {
         let returnToParam: string | undefined = req.query.returnTo?.toString();
         if (returnToParam) {
             log.info(`Stored returnTo URL: ${returnToParam}`, { "login-flow": true });
-            const isStrictAuthorizeValidationEnabled = await getFeatureFlagEnableStrictAuthorizeReturnTo();
-            if (isStrictAuthorizeValidationEnabled) {
-                // Validate returnTo URL against allowlist for login API
-                if (!validateLoginReturnToUrl(returnToParam, this.config.hostUrl)) {
-                    log.warn(`Invalid returnTo URL rejected for login: ${returnToParam}`, { "login-flow": true });
-                    safeFragmentRedirect(res, this.getSorryUrl(`Invalid return URL.`));
-                    return;
-                }
+            // Validate returnTo URL against allowlist for login API
+            if (!validateLoginReturnToUrl(returnToParam, this.config.hostUrl)) {
+                log.warn(`Invalid returnTo URL rejected for login: ${returnToParam}`, { "login-flow": true });
+                safeFragmentRedirect(res, this.getSorryUrl(`Invalid return URL.`));
+                return;
             }
         }
         // returnTo defaults to workspaces url

components/server/src/express-util.ts

@@ -242,14 +242,14 @@ export function validateAuthorizeReturnToUrl(returnTo: string, hostUrl: GitpodHo
     return validateReturnToUrlWithPatterns(returnTo, hostUrl, allowedPatterns);
 }
 
-export function getSafeReturnToParam(req: express.Request, validator: (url: string) => boolean): string | undefined {
+export function getSafeReturnToParam(req: express.Request, validator?: (url: string) => boolean): string | undefined {
     // @ts-ignore Type 'ParsedQs' is not assignable
     const returnToURL: string | undefined = req.query.redirect || req.query.returnTo;
     if (!returnToURL) {
         return;
     }
 
-    if (!validator(returnToURL)) {
+    if (validator && !validator(returnToURL)) {
         log.debug("The redirect URL does not match allowed patterns", { query: new TrustedValue(req.query).value });
         return;
     }

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

@@ -43,6 +43,7 @@ import { UserService } from "./user-service";
 import { WorkspaceService } from "../workspace/workspace-service";
 import { runWithSubjectId } from "../util/request-context";
 import { SubjectId } from "../auth/subject-id";
+import { getFeatureFlagEnableStrictAuthorizeReturnTo } from "../util/featureflags";
 
 export const ServerFactory = Symbol("ServerFactory");
 export type ServerFactory = () => GitpodServerImpl;
@@ -240,8 +241,16 @@ export class UserController {
                 res.sendStatus(403);
                 return;
             }
-            this.ensureSafeReturnToParamForAuthorize(req);
-            this.authenticator.authorize(req, res, next).catch((err) => log.error("authenticator.authorize", err));
+            this.ensureSafeReturnToParam(req);
+            this.ensureSafeReturnToParamForAuthorize(req)
+                .then(() => {
+                    this.authenticator
+                        .authorize(req, res, next)
+                        .catch((err) => log.error("authenticator.authorize", err));
+                })
+                .catch((err) => {
+                    log.error("authenticator.authorize", err);
+                });
         });
         router.get("/deauthorize", (req: express.Request, res: express.Response, next: express.NextFunction) => {
             if (!User.is(req.user)) {
@@ -252,8 +261,16 @@ export class UserController {
                 res.sendStatus(403);
                 return;
             }
-            this.ensureSafeReturnToParamForAuthorize(req);
-            this.authenticator.deauthorize(req, res, next).catch((err) => log.error("authenticator.deauthorize", err));
+            this.ensureSafeReturnToParam(req);
+            this.ensureSafeReturnToParamForAuthorize(req)
+                .then(() => {
+                    this.authenticator
+                        .deauthorize(req, res, next)
+                        .catch((err) => log.error("authenticator.deauthorize", err));
+                })
+                .catch((err) => {
+                    log.error("authenticator.deauthorize", err);
+                });
         });
         router.get("/logout", async (req: express.Request, res: express.Response, next: express.NextFunction) => {
             const logContext = LogContext.from({ user: req.user, request: req });
@@ -614,8 +631,20 @@ export class UserController {
         return returnTo;
     }
 
-    protected ensureSafeReturnToParamForAuthorize(req: express.Request): string | undefined {
-        const returnTo = getSafeReturnToParam(req, (url) => validateAuthorizeReturnToUrl(url, this.config.hostUrl));
+    // TODO(gpl): Once we drop the feature flag, turn into the same form as above method.
+    protected async ensureSafeReturnToParamForAuthorize(req: express.Request): Promise<string | undefined> {
+        let returnTo = getSafeReturnToParam(req);
+        if (returnTo) {
+            const isStrictAuthorizeValidationEnabled = await getFeatureFlagEnableStrictAuthorizeReturnTo();
+            if (isStrictAuthorizeValidationEnabled) {
+                // Validate returnTo URL against allowlist for authorize API
+                if (!validateAuthorizeReturnToUrl(returnTo, this.config.hostUrl)) {
+                    log.warn(`Invalid returnTo URL rejected for authorize: ${returnTo}`, { "login-flow": true });
+                    returnTo = undefined;
+                }
+            }
+        }
+
         req.query.returnTo = returnTo;
         return returnTo;
     }