diff --git a/components/server/src/auth/api-subdomain-redirect.spec.ts b/components/server/src/auth/api-subdomain-redirect.spec.ts new file mode 100644 index 00000000000000..26817f0a11680c --- /dev/null +++ b/components/server/src/auth/api-subdomain-redirect.spec.ts @@ -0,0 +1,82 @@ +/** + * Copyright (c) 2024 Gitpod GmbH. All rights reserved. + * Licensed under the GNU Affero General Public License (AGPL). + * See License.AGPL.txt in the project root for license information. + */ + +import { expect } from "chai"; + +describe("API Subdomain Redirect Logic", () => { + // Test the core logic without complex dependency injection + function isApiSubdomainOfConfiguredHost(hostname: string, configuredHost: string): boolean { + return hostname === `api.${configuredHost}`; + } + + describe("isApiSubdomainOfConfiguredHost", () => { + it("should detect api subdomain of configured host", () => { + const configuredHost = "gitpod.io"; + const testCases = [ + { hostname: "api.gitpod.io", expected: true }, + { hostname: "api.preview.gitpod-dev.com", expected: false }, // Different configured host + { hostname: "gitpod.io", expected: false }, // Main domain + { hostname: "workspace-123.gitpod.io", expected: false }, // Other subdomain + { hostname: "api.evil.com", expected: false }, // Different domain + ]; + + testCases.forEach(({ hostname, expected }) => { + const result = isApiSubdomainOfConfiguredHost(hostname, configuredHost); + expect(result).to.equal(expected, `Failed for hostname: ${hostname}`); + }); + }); + + it("should handle GitHub OAuth edge case correctly", () => { + // This is the specific case mentioned in the login completion handler + const configuredHost = "gitpod.io"; + const apiSubdomain = `api.${configuredHost}`; + + const result = isApiSubdomainOfConfiguredHost(apiSubdomain, configuredHost); + expect(result).to.be.true; + }); + + it("should handle preview environment correctly", () => { + const configuredHost = "preview.gitpod-dev.com"; + const apiSubdomain = `api.${configuredHost}`; + + const result = isApiSubdomainOfConfiguredHost(apiSubdomain, configuredHost); + expect(result).to.be.true; + }); + }); + + describe("OAuth callback flow scenarios", () => { + it("should identify redirect scenarios correctly", () => { + const scenarios = [ + { + name: "GitHub OAuth Callback on API Subdomain", + hostname: "api.gitpod.io", + configuredHost: "gitpod.io", + shouldRedirect: true, + }, + { + name: "Regular Login on Main Domain", + hostname: "gitpod.io", + configuredHost: "gitpod.io", + shouldRedirect: false, + }, + { + name: "Workspace Port (Should Not Redirect)", + hostname: "3000-gitpod.io", + configuredHost: "gitpod.io", + shouldRedirect: false, + }, + ]; + + scenarios.forEach((scenario) => { + const result = isApiSubdomainOfConfiguredHost(scenario.hostname, scenario.configuredHost); + expect(result).to.equal( + scenario.shouldRedirect, + `${scenario.name}: Expected ${scenario.shouldRedirect} for ${scenario.hostname}`, + ); + }); + }); + }); +}); diff --git a/components/server/src/auth/auth-provider.ts b/components/server/src/auth/auth-provider.ts index 76259d6dc4562d..5d2929012e18cc 100644 --- a/components/server/src/auth/auth-provider.ts +++ b/components/server/src/auth/auth-provider.ts @@ -97,6 +97,7 @@ export interface AuthFlow { readonly host: string; readonly returnTo: string; readonly overrideScopes?: boolean; + readonly nonce?: string; } export namespace AuthFlow { export function is(obj: any): obj is AuthFlow { diff --git a/components/server/src/auth/authenticator.ts b/components/server/src/auth/authenticator.ts index eafd49e81af9f7..2467a4f26fcffa 100644 --- a/components/server/src/auth/authenticator.ts +++ b/components/server/src/auth/authenticator.ts @@ -18,6 +18,9 @@ import { UserService } from "../user/user-service"; import { AuthFlow, AuthProvider } from "./auth-provider"; import { HostContextProvider } from "./host-context-provider"; import { SignInJWT } from "./jwt"; +import { NonceService } from "./nonce-service"; +import { getFeatureFlagEnableNonceValidation, getFeatureFlagEnableStrictAuthorizeReturnTo } from "../util/featureflags"; +import { validateLoginReturnToUrl, validateAuthorizeReturnToUrl, safeFragmentRedirect } from "../express-util"; @injectable() export class Authenticator { @@ -30,6 +33,7 @@ export class Authenticator { @inject(TokenProvider) protected readonly tokenProvider: TokenProvider; @inject(UserAuthentication) protected readonly userAuthentication: UserAuthentication; @inject(SignInJWT) protected readonly signInJWT: SignInJWT; + @inject(NonceService) protected readonly nonceService: NonceService; @postConstruct() protected setup() { @@ -77,6 +81,42 @@ export class Authenticator { if (!host) { throw new Error("Auth flow state is missing 'host' attribute."); } + + // Handle GitHub OAuth edge case: redirect from api.* subdomain to base domain + // This allows nonce validation to work since cookies are accessible on base domain + if (this.isApiSubdomainOfConfiguredHost(req.hostname)) { + log.info(`OAuth callback on api subdomain, redirecting to base domain for nonce validation`, { + hostname: req.hostname, + configuredHost: this.config.hostUrl.url.hostname, + }); + const baseUrl = this.config.hostUrl.with({ + pathname: req.path, + search: new URL(req.url, this.config.hostUrl.url).search, + }); + safeFragmentRedirect(res, baseUrl.toString()); + return; + } + + // Validate nonce for CSRF protection (if feature flag is enabled) + const isNonceValidationEnabled = await getFeatureFlagEnableNonceValidation(); + if (isNonceValidationEnabled) { + const stateNonce = flowState.nonce; + const cookieNonce = this.nonceService.getNonceFromCookie(req); + + if (!this.nonceService.validateNonce(stateNonce, cookieNonce)) { + log.error(`CSRF protection: Nonce validation failed`, { + url: req.url, + hasStateNonce: !!stateNonce, + hasCookieNonce: !!cookieNonce, + }); + res.status(403).send("Authentication failed"); + return; + } + } + + // Always clear the nonce cookie + this.nonceService.clearNonceCookie(res); + const hostContext = this.hostContextProvider.get(host); if (!hostContext) { throw new Error("No host context found."); @@ -89,6 +129,8 @@ export class Authenticator { await hostContext.authProvider.callback(req, res, next); } catch (error) { log.error(`Failed to handle callback.`, error, { url: req.url }); + // Always clear nonce cookie on error + this.nonceService.clearNonceCookie(res); } } else { // Otherwise proceed with other handlers @@ -121,6 +163,15 @@ export class Authenticator { return state; } + /** + * Checks if the current hostname is api.{configured-domain}. + * This handles the GitHub OAuth edge case where callbacks may come to api.* subdomain. + */ + private isApiSubdomainOfConfiguredHost(hostname: string): boolean { + const configuredHost = this.config.hostUrl.url.hostname; + return hostname === `api.${configuredHost}`; + } + protected async getAuthProviderForHost(host: string): Promise { const hostContext = this.hostContextProvider.get(host); return hostContext && hostContext.authProvider; @@ -131,18 +182,26 @@ export class Authenticator { log.info(`User is already authenticated. Continue.`, { "login-flow": true }); return next(); } - let returnTo: string | undefined = req.query.returnTo?.toString(); - if (returnTo) { - log.info(`Stored returnTo URL: ${returnTo}`, { "login-flow": true }); + let returnToParam: string | undefined = req.query.returnTo?.toString(); + if (returnToParam) { + log.info(`Stored returnTo URL: ${returnToParam}`, { "login-flow": true }); + // 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 const workspaceUrl = this.config.hostUrl.asDashboard().toString(); - returnTo = returnTo || workspaceUrl; + returnToParam = returnToParam || workspaceUrl; + const returnTo = returnToParam; + const host: string = req.query.host?.toString() || ""; const authProvider = host && (await this.getAuthProviderForHost(host)); if (!host || !authProvider) { log.info(`Bad request: missing parameters.`, { "login-flow": true }); - res.redirect(this.getSorryUrl(`Bad request: missing parameters.`)); + safeFragmentRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`)); return; } // Logins with organizational Git Auth is not permitted @@ -151,12 +210,12 @@ export class Authenticator { "authorize-flow": true, ap: authProvider.info, }); - res.redirect(this.getSorryUrl(`Login with "${host}" is not permitted.`)); + safeFragmentRedirect(res, this.getSorryUrl(`Login with "${host}" is not permitted.`)); return; } if (this.config.disableDynamicAuthProviderLogin && !authProvider.params.builtin) { log.info(`Auth Provider is not allowed.`, { ap: authProvider.info }); - res.redirect(this.getSorryUrl(`Login with ${authProvider.params.host} is not allowed.`)); + safeFragmentRedirect(res, this.getSorryUrl(`Login with ${authProvider.params.host} is not allowed.`)); return; } @@ -166,13 +225,18 @@ export class Authenticator { "login-flow": true, ap: authProvider.info, }); - res.redirect(this.getSorryUrl(`Login with "${host}" is not permitted.`)); + safeFragmentRedirect(res, this.getSorryUrl(`Login with "${host}" is not permitted.`)); return; } + // Always generate nonce for CSRF protection (validation controlled by feature flag) + const nonce = this.nonceService.generateNonce(); + this.nonceService.setNonceCookie(res, nonce); + const state = await this.signInJWT.sign({ host, returnTo, + nonce, }); // authenticate user @@ -183,7 +247,7 @@ export class Authenticator { const user = req.user; if (!req.isAuthenticated() || !User.is(user)) { log.info(`User is not authenticated.`); - res.redirect(this.getSorryUrl(`Not authenticated. Please login.`)); + safeFragmentRedirect(res, this.getSorryUrl(`Not authenticated. Please login.`)); return; } const returnTo: string = req.query.returnTo?.toString() || this.config.hostUrl.asDashboard().toString(); @@ -193,20 +257,21 @@ export class Authenticator { if (!host || !authProvider) { log.warn(`Bad request: missing parameters.`); - res.redirect(this.getSorryUrl(`Bad request: missing parameters.`)); + safeFragmentRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`)); return; } try { await this.userAuthentication.deauthorize(user, authProvider.authProviderId); - res.redirect(returnTo); + safeFragmentRedirect(res, returnTo); } catch (error) { next(error); log.error(`Failed to disconnect a provider.`, error, { host, userId: user.id, }); - res.redirect( + safeFragmentRedirect( + res, this.getSorryUrl( `Failed to disconnect a provider: ${error && error.message ? error.message : "unknown reason"}`, ), @@ -218,27 +283,45 @@ export class Authenticator { const user = req.user; if (!req.isAuthenticated() || !User.is(user)) { log.info(`User is not authenticated.`, { "authorize-flow": true }); - res.redirect(this.getSorryUrl(`Not authenticated. Please login.`)); + safeFragmentRedirect(res, this.getSorryUrl(`Not authenticated. Please login.`)); return; } if (user.id === BUILTIN_INSTLLATION_ADMIN_USER_ID) { log.info(`Authorization is not permitted for admin user.`); - res.redirect( + safeFragmentRedirect( + res, this.getSorryUrl(`Authorization is not permitted for admin user. Please login with a user account.`), ); return; } - const returnTo: string | undefined = req.query.returnTo?.toString(); + const returnToParam: string | undefined = req.query.returnTo?.toString(); const host: string | undefined = req.query.host?.toString(); const scopes: string = req.query.scopes?.toString() || ""; const override = req.query.override === "true"; const authProvider = host && (await this.getAuthProviderForHost(host)); - if (!returnTo || !host || !authProvider) { + + if (!returnToParam || !host || !authProvider) { log.info(`Bad request: missing parameters.`, { "authorize-flow": true }); - res.redirect(this.getSorryUrl(`Bad request: missing parameters.`)); + safeFragmentRedirect(res, this.getSorryUrl(`Bad request: missing parameters.`)); return; } + // Validate returnTo URL against allowlist for authorize API + const isStrictAuthorizeValidationEnabled = await getFeatureFlagEnableStrictAuthorizeReturnTo(); + if (isStrictAuthorizeValidationEnabled) { + const isValidReturnTo = validateAuthorizeReturnToUrl(returnToParam, this.config.hostUrl); + if (!isValidReturnTo) { + log.warn(`Invalid returnTo URL rejected for authorize`, { + "authorize-flow": true, + returnToParam, + }); + safeFragmentRedirect(res, this.getSorryUrl(`Invalid return URL.`)); + return; + } + } + + const returnTo = returnToParam; + // For non-verified org auth provider, ensure user is an owner of the org if (!authProvider.info.verified && authProvider.info.organizationId) { const member = await this.teamDb.findTeamMembership(user.id, authProvider.info.organizationId); @@ -247,7 +330,7 @@ export class Authenticator { "authorize-flow": true, ap: authProvider.info, }); - res.redirect(this.getSorryUrl(`Authorization with "${host}" is not permitted.`)); + safeFragmentRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`)); return; } } @@ -258,7 +341,7 @@ export class Authenticator { "authorize-flow": true, ap: authProvider.info, }); - res.redirect(this.getSorryUrl(`Authorization with "${host}" is not permitted.`)); + safeFragmentRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`)); return; } @@ -270,7 +353,7 @@ export class Authenticator { "authorize-flow": true, ap: authProvider.info, }); - res.redirect(this.getSorryUrl(`Authorization with "${host}" is not permitted.`)); + safeFragmentRedirect(res, this.getSorryUrl(`Authorization with "${host}" is not permitted.`)); return; } } @@ -297,7 +380,12 @@ export class Authenticator { } // authorize Gitpod log.info(`(doAuthorize) wanted scopes (${override ? "overriding" : "merging"}): ${wantedScopes.join(",")}`); - const state = await this.signInJWT.sign({ host, returnTo, overrideScopes: override }); + + // Always generate nonce for CSRF protection (validation controlled by feature flag) + const nonce = this.nonceService.generateNonce(); + this.nonceService.setNonceCookie(res, nonce); + + const state = await this.signInJWT.sign({ host, returnTo, overrideScopes: override, nonce }); authProvider.authorize(req, res, next, this.deriveAuthState(state), wantedScopes); } private mergeScopes(a: string[], b: string[]) { diff --git a/components/server/src/auth/fragment-protection.spec.ts b/components/server/src/auth/fragment-protection.spec.ts new file mode 100644 index 00000000000000..3f22f5172914be --- /dev/null +++ b/components/server/src/auth/fragment-protection.spec.ts @@ -0,0 +1,52 @@ +/** + * Copyright (c) 2024 Gitpod GmbH. All rights reserved. + * Licensed under the GNU Affero General Public License (AGPL). + * See License.AGPL.txt in the project root for license information. + */ + +import { expect } from "chai"; +import { ensureUrlHasFragment } from "./fragment-utils"; + +describe("Fragment Protection", () => { + describe("ensureUrlHasFragment", () => { + it("should add empty fragment to URL without fragment", () => { + const url = "https://gitpod.io/workspaces"; + const result = ensureUrlHasFragment(url); + + expect(result).to.equal("https://gitpod.io/workspaces#"); + }); + + it("should preserve existing fragment", () => { + const url = "https://gitpod.io/workspaces#existing"; + const result = ensureUrlHasFragment(url); + + expect(result).to.equal(url); + }); + + it("should handle URLs with query parameters", () => { + const url = "https://gitpod.io/workspaces?tab=recent"; + const result = ensureUrlHasFragment(url); + + expect(result).to.equal("https://gitpod.io/workspaces?tab=recent#"); + }); + + it("should handle invalid URLs gracefully", () => { + const url = "not-a-valid-url"; + const result = ensureUrlHasFragment(url); + + expect(result).to.equal("not-a-valid-url#"); + }); + + it("should prevent OAuth token inheritance attack", () => { + // Scenario: OAuth provider redirects with token in fragment + // If returnTo URL has no fragment, browser inherits the token fragment + const returnToWithoutFragment = "https://gitpod.io/workspaces"; + const protectedUrl = ensureUrlHasFragment(returnToWithoutFragment); + + // Now when OAuth provider redirects to: protectedUrl + token fragment + // The existing fragment prevents inheritance + expect(protectedUrl).to.include("#"); + expect(protectedUrl).to.not.equal(returnToWithoutFragment); + }); + }); +}); diff --git a/components/server/src/auth/fragment-utils.ts b/components/server/src/auth/fragment-utils.ts new file mode 100644 index 00000000000000..16c538c0403a11 --- /dev/null +++ b/components/server/src/auth/fragment-utils.ts @@ -0,0 +1,32 @@ +/** + * Copyright (c) 2024 Gitpod GmbH. All rights reserved. + * Licensed under the GNU Affero General Public License (AGPL). + * See License.AGPL.txt in the project root for license information. + */ + +/** + * Ensures a returnTo URL has a fragment to prevent OAuth token inheritance attacks. + * + * When OAuth providers use response_type=token, they redirect with access tokens + * in URL fragments. If the returnTo URL doesn't have a fragment, browsers inherit + * the current page's fragment, potentially exposing tokens to malicious sites. + * + * Uses an empty fragment (#) to prevent inheritance without interfering with + * Gitpod's context provider resolution. + */ +export function ensureUrlHasFragment(url: string): string { + try { + const parsedUrl = new URL(url); + // If URL already has a fragment, return as-is + if (parsedUrl.hash) { + return url; + } + // Add empty fragment to prevent inheritance + // Using just "#" to avoid interfering with context provider resolution that + // treats fragments as git URLs + return url + "#"; + } catch (error) { + // If URL is invalid, add fragment anyway + return url + "#"; + } +} diff --git a/components/server/src/auth/generic-auth-provider.ts b/components/server/src/auth/generic-auth-provider.ts index 1295f3d96030b4..22cf40dd013108 100644 --- a/components/server/src/auth/generic-auth-provider.ts +++ b/components/server/src/auth/generic-auth-provider.ts @@ -23,7 +23,7 @@ import { UnconfirmedUserException, } from "../auth/errors"; import { Config } from "../config"; -import { getRequestingClientInfo } from "../express-util"; +import { getRequestingClientInfo, safeFragmentRedirect } from "../express-util"; import { TokenProvider } from "../user/token-provider"; import { UserAuthentication } from "../user/user-authentication"; import { AuthProviderService } from "./auth-provider-service"; @@ -287,7 +287,8 @@ export abstract class GenericAuthProvider implements AuthProvider { const state = request.query.state; if (!state) { log.error(cxt, `(${strategyName}) No state present on callback request.`, { clientInfo }); - response.redirect( + safeFragmentRedirect( + response, this.getSorryUrl(`No state was present on the authentication callback. Please try again.`), ); return; @@ -298,15 +299,7 @@ export abstract class GenericAuthProvider implements AuthProvider { log.error(`(${strategyName}) Auth flow state is missing.`); reportLoginCompleted("failed", "git"); - response.redirect(this.getSorryUrl(`Auth flow state is missing.`)); - return; - } - - if (!this.loginCompletionHandler.isBaseDomain(request)) { - // For auth requests that are not targetting the base domain, we redirect to the base domain, so they come with our cookie. - log.info(`(${strategyName}) Auth request on subdomain, redirecting to base domain`, { clientInfo }); - const target = new URL(request.url, this.config.hostUrl.url.toString()).toString(); - response.redirect(target); + safeFragmentRedirect(response, this.getSorryUrl(`Auth flow state is missing.`)); return; } @@ -317,7 +310,7 @@ export abstract class GenericAuthProvider implements AuthProvider { `(${strategyName}) User is already logged in. No auth info provided. Redirecting to dashboard.`, { clientInfo }, ); - response.redirect(this.config.hostUrl.asDashboard().toString()); + safeFragmentRedirect(response, this.config.hostUrl.asDashboard().toString()); return; } } @@ -328,7 +321,10 @@ export abstract class GenericAuthProvider implements AuthProvider { reportLoginCompleted("failed_client", "git"); log.error(cxt, `(${strategyName}) No session found during auth callback.`, { clientInfo }); - response.redirect(this.getSorryUrl(`Please allow Cookies in your browser and try to log in again.`)); + safeFragmentRedirect( + response, + this.getSorryUrl(`Please allow Cookies in your browser and try to log in again.`), + ); return; } @@ -336,7 +332,7 @@ export abstract class GenericAuthProvider implements AuthProvider { reportLoginCompleted("failed", "git"); log.error(cxt, `(${strategyName}) Host does not match.`, { clientInfo }); - response.redirect(this.getSorryUrl(`Host does not match.`)); + safeFragmentRedirect(response, this.getSorryUrl(`Host does not match.`)); return; } @@ -367,7 +363,7 @@ export abstract class GenericAuthProvider implements AuthProvider { authenticate(request, response, next); }); } catch (error) { - response.redirect(this.getSorryUrl(`OAuth2 error. (${error})`)); + safeFragmentRedirect(response, this.getSorryUrl(`OAuth2 error. (${error})`)); return; } const [err, userOrIdentity, flowContext] = result; @@ -479,7 +475,7 @@ export abstract class GenericAuthProvider implements AuthProvider { ); const { returnTo } = authFlow; - response.redirect(returnTo); + safeFragmentRedirect(response, returnTo); return; } else { // Complete login into an existing account @@ -544,7 +540,7 @@ export abstract class GenericAuthProvider implements AuthProvider { search: "message=error:" + Buffer.from(JSON.stringify(error), "utf-8").toString("base64"), }) .toString(); - response.redirect(url); + safeFragmentRedirect(response, url); } /** diff --git a/components/server/src/auth/login-completion-handler.ts b/components/server/src/auth/login-completion-handler.ts index 3345100df9783c..d4978b74ac7980 100644 --- a/components/server/src/auth/login-completion-handler.ts +++ b/components/server/src/auth/login-completion-handler.ts @@ -6,7 +6,6 @@ import { inject, injectable } from "inversify"; import express from "express"; -import * as crypto from "crypto"; import { User } from "@gitpod/gitpod-protocol"; import { log, LogContext } from "@gitpod/gitpod-protocol/lib/util/logging"; import { Config } from "../config"; @@ -17,7 +16,7 @@ import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics"; import { trackLogin } from "../analytics"; import { SessionHandler } from "../session-handler"; import { AuthJWT } from "./jwt"; -import { OneTimeSecretServer } from "../one-time-secret-server"; +import { safeFragmentRedirect } from "../express-util"; /** * The login completion handler pulls the strings between the OAuth2 flow, the ToS flow, and the session management. @@ -30,7 +29,6 @@ export class LoginCompletionHandler { @inject(AuthProviderService) protected readonly authProviderService: AuthProviderService; @inject(AuthJWT) protected readonly authJWT: AuthJWT; @inject(SessionHandler) protected readonly session: SessionHandler; - @inject(OneTimeSecretServer) private readonly otsServer: OneTimeSecretServer; async complete( request: express.Request, @@ -52,12 +50,17 @@ export class LoginCompletionHandler { } catch (err) { reportLoginCompleted("failed", "git"); log.error(logContext, `Failed to login user. Redirecting to /sorry on login.`, err); - response.redirect(this.config.hostUrl.asSorry("Oops! Something went wrong during login.").toString()); + safeFragmentRedirect( + response, + this.config.hostUrl.asSorry("Oops! Something went wrong during login.").toString(), + ); return; } // Update session info - let returnTo = returnToUrl || this.config.hostUrl.asDashboard().toString(); + const returnToParam = returnToUrl || this.config.hostUrl.asDashboard().toString(); + let returnTo = returnToParam; + if (elevateScopes) { const elevateScopesUrl = this.config.hostUrl .withApi({ @@ -81,25 +84,6 @@ export class LoginCompletionHandler { ); } - if (!this.isBaseDomain(request)) { - // (GitHub edge case) If we got redirected here onto a sub-domain (e.g. api.gitpod.io), we need to redirect to the base domain in order to Set-Cookie properly. - const secret = crypto - .createHash("sha256") - .update(user.id + this.config.session.secret) - .digest("hex"); - const expirationDate = new Date(Date.now() + 1000 * 60); // 1 minutes - const token = await this.otsServer.serveToken({}, secret, expirationDate); - - reportLoginCompleted("succeeded_via_ots", "git"); - log.info( - logContext, - `User will be logged in via OTS on the base domain. (Indirect) redirect to: ${returnTo}`, - ); - const baseDomainRedirect = this.config.hostUrl.asLoginWithOTS(user.id, token.token, returnTo).toString(); - response.redirect(baseDomainRedirect); - return; - } - // (default case) If we got redirected here onto the base domain of the Gitpod installation, we can just issue the cookie right away. const cookie = await this.session.createJWTSessionCookie(user.id); response.cookie(cookie.name, cookie.value, cookie.opts); @@ -108,11 +92,7 @@ export class LoginCompletionHandler { log.info(logContext, `User is logged in successfully. Redirect to: ${returnTo}`); reportLoginCompleted("succeeded", "git"); - response.redirect(returnTo); - } - - public isBaseDomain(req: express.Request): boolean { - return req.hostname === this.config.hostUrl.url.hostname; + safeFragmentRedirect(response, returnTo); } public async updateAuthProviderAsVerified(hostname: string, user: User) { diff --git a/components/server/src/auth/nonce-service.spec.ts b/components/server/src/auth/nonce-service.spec.ts new file mode 100644 index 00000000000000..fd60e975a0e4fe --- /dev/null +++ b/components/server/src/auth/nonce-service.spec.ts @@ -0,0 +1,67 @@ +/** + * Copyright (c) 2024 Gitpod GmbH. All rights reserved. + * Licensed under the GNU Affero General Public License (AGPL). + * See License.AGPL.txt in the project root for license information. + */ + +import { expect } from "chai"; +import { Container } from "inversify"; +import { Config } from "../config"; +import { NonceService } from "./nonce-service"; + +describe("NonceService", () => { + let container: Container; + let nonceService: NonceService; + + beforeEach(() => { + container = new Container(); + container.bind(Config).toConstantValue({ + hostUrl: { + url: new URL("https://gitpod.io"), + }, + auth: { + session: { + cookie: { + secure: true, + }, + }, + }, + } as any); + container.bind(NonceService).toSelf(); + nonceService = container.get(NonceService); + }); + + describe("generateNonce", () => { + it("should generate unique nonces", () => { + const nonce1 = nonceService.generateNonce(); + const nonce2 = nonceService.generateNonce(); + + expect(nonce1).to.be.a("string"); + expect(nonce2).to.be.a("string"); + expect(nonce1).to.not.equal(nonce2); + expect(nonce1.length).to.be.greaterThan(40); // base64url encoded 32 bytes + }); + }); + + describe("validateNonce", () => { + it("should validate matching nonces", () => { + const nonce = nonceService.generateNonce(); + const isValid = nonceService.validateNonce(nonce, nonce); + expect(isValid).to.be.true; + }); + + it("should reject different nonces", () => { + const nonce1 = nonceService.generateNonce(); + const nonce2 = nonceService.generateNonce(); + const isValid = nonceService.validateNonce(nonce1, nonce2); + expect(isValid).to.be.false; + }); + + it("should reject undefined nonces", () => { + const nonce = nonceService.generateNonce(); + expect(nonceService.validateNonce(nonce, undefined)).to.be.false; + expect(nonceService.validateNonce(undefined, nonce)).to.be.false; + expect(nonceService.validateNonce(undefined, undefined)).to.be.false; + }); + }); +}); diff --git a/components/server/src/auth/nonce-service.ts b/components/server/src/auth/nonce-service.ts new file mode 100644 index 00000000000000..b4ac005c3f9c29 --- /dev/null +++ b/components/server/src/auth/nonce-service.ts @@ -0,0 +1,80 @@ +/** + * Copyright (c) 2024 Gitpod GmbH. All rights reserved. + * Licensed under the GNU Affero General Public License (AGPL). + * See License.AGPL.txt in the project root for license information. + */ + +import { injectable, inject } from "inversify"; +import express from "express"; +import * as crypto from "crypto"; +import { Config } from "../config"; + +const NONCE_COOKIE_NAME = "__Host-_gitpod_oauth_nonce_"; +const NONCE_LENGTH = 32; // 32 bytes = 256 bits + +@injectable() +export class NonceService { + @inject(Config) protected readonly config: Config; + + /** + * Generates a cryptographically secure random nonce + */ + generateNonce(): string { + return crypto.randomBytes(NONCE_LENGTH).toString("base64url"); + } + + /** + * Sets the nonce cookie in the response + */ + setNonceCookie(res: express.Response, nonce: string): void { + res.cookie(NONCE_COOKIE_NAME, nonce, { + httpOnly: true, + secure: this.config.auth.session.cookie.secure, + sameSite: "lax", + maxAge: 5 * 60 * 1000, // 5 minutes (same as JWT state expiry) + path: "/", + }); + } + + /** + * Gets the nonce from the request cookies + */ + getNonceFromCookie(req: express.Request): string | undefined { + return req.cookies?.[NONCE_COOKIE_NAME]; + } + + /** + * Clears the nonce cookie + */ + clearNonceCookie(res: express.Response): void { + res.clearCookie(NONCE_COOKIE_NAME, { + httpOnly: true, + secure: this.config.auth.session.cookie.secure, + sameSite: "lax", + path: "/", + }); + } + + /** + * Validates that the nonce from the state matches the nonce from the cookie + */ + validateNonce(stateNonce: string | undefined, cookieNonce: string | undefined): boolean { + if (!stateNonce || !cookieNonce) { + return false; + } + + // Use crypto.timingSafeEqual to prevent timing attacks + if (stateNonce.length !== cookieNonce.length) { + return false; + } + + const stateBuffer = Buffer.from(stateNonce, "base64url"); + const cookieBuffer = Buffer.from(cookieNonce, "base64url"); + + if (stateBuffer.length !== cookieBuffer.length) { + return false; + } + + return crypto.timingSafeEqual(stateBuffer, cookieBuffer); + } +} diff --git a/components/server/src/auth/return-to-validation.spec.ts b/components/server/src/auth/return-to-validation.spec.ts new file mode 100644 index 00000000000000..e1eea9555f5532 --- /dev/null +++ b/components/server/src/auth/return-to-validation.spec.ts @@ -0,0 +1,307 @@ +/** + * Copyright (c) 2024 Gitpod GmbH. All rights reserved. + * Licensed under the GNU Affero General Public License (AGPL). + * See License.AGPL.txt in the project root for license information. + */ + +import { expect } from "chai"; +import { GitpodHostUrl } from "@gitpod/gitpod-protocol/lib/util/gitpod-host-url"; +import { validateLoginReturnToUrl, validateAuthorizeReturnToUrl } from "../express-util"; + +describe("ReturnTo URL Validation", () => { + const hostUrl = new GitpodHostUrl("https://gitpod.io"); + + describe("validateLoginReturnToUrl", () => { + it("should accept any valid path after domain validation", () => { + const validUrls = [ + "https://gitpod.io/", + "https://gitpod.io/workspaces", + "https://gitpod.io/workspaces/123", + "https://gitpod.io/settings", + "https://gitpod.io/settings/integrations", + "https://gitpod.io/user-settings", + "https://gitpod.io/user-settings/account", + "https://gitpod.io/integrations", + "https://gitpod.io/integrations/github", + "https://gitpod.io/repositories", + "https://gitpod.io/repositories/123", + "https://gitpod.io/prebuilds", + "https://gitpod.io/prebuilds/456", + "https://gitpod.io/members", + "https://gitpod.io/billing", + "https://gitpod.io/usage", + "https://gitpod.io/insights", + "https://gitpod.io/new", + "https://gitpod.io/login", + "https://gitpod.io/login/org-slug", + "https://gitpod.io/quickstart", + "https://gitpod.io/admin", // Now allowed since login doesn't restrict paths + "https://gitpod.io/api/workspace", // Now allowed + "https://gitpod.io/any-path", // Any path is allowed + ]; + + validUrls.forEach((url) => { + const result = validateLoginReturnToUrl(url, hostUrl); + expect(result).to.equal(true, `Should accept valid login URL: ${url}`); + }); + }); + + it("should accept complete-auth with ONLY message parameter", () => { + const validCompleteAuthUrls = [ + "https://gitpod.io/complete-auth?message=success:123", + "https://gitpod.io/complete-auth?message=success", + ]; + + validCompleteAuthUrls.forEach((url) => { + const result = validateLoginReturnToUrl(url, hostUrl); + expect(result).to.equal(true, `Should accept complete-auth with only message: ${url}`); + }); + }); + + it("should reject complete-auth with additional parameters", () => { + const invalidCompleteAuthUrls = [ + "https://gitpod.io/complete-auth?message=success&other=param", + "https://gitpod.io/complete-auth?other=param&message=success", + "https://gitpod.io/complete-auth", + "https://gitpod.io/complete-auth?other=param", + "https://gitpod.io/complete-auth?msg=success", // Wrong parameter name + ]; + + invalidCompleteAuthUrls.forEach((url) => { + const result = validateLoginReturnToUrl(url, hostUrl); + expect(result).to.equal(false, `Should reject complete-auth with extra params: ${url}`); + }); + }); + + it("should accept www.gitpod.io root only", () => { + const result = validateLoginReturnToUrl("https://www.gitpod.io/", hostUrl); + expect(result).to.equal(true, "Should accept www.gitpod.io root"); + + const invalidWwwUrls = [ + "https://www.gitpod.io/workspaces", + "https://www.gitpod.io/login", + "http://www.gitpod.io/", // Wrong protocol + ]; + + invalidWwwUrls.forEach((url) => { + const result = validateLoginReturnToUrl(url, hostUrl); + expect(result).to.equal(false, `Should reject www.gitpod.io non-root: ${url}`); + }); + }); + + describe("Feature Flag Integration", () => { + it("should document nonce validation feature flag behavior", () => { + // Feature flag: enable_nonce_validation (default: false) + // When enabled: Full CSRF protection with nonce validation + // When disabled: Nonce is generated but not validated (for future compatibility) + + // This test documents the expected behavior: + // 1. Nonce is always generated and stored in cookie + // 2. Nonce is always included in JWT state + // 3. Nonce validation only occurs when feature flag is enabled + // 4. Cookie is always cleared after callback processing + + expect(true).to.equal(true); // Documentation test + }); + + it("should document strict authorize returnTo validation feature flag behavior", () => { + // Feature flag: enable_strict_authorize_return_to (default: false) + // When enabled: Uses validateAuthorizeReturnToUrl (strict patterns) + // When disabled: Falls back to validateLoginReturnToUrl (broader patterns) + + // This test documents the expected behavior: + // 1. /api/authorize endpoint checks the feature flag + // 2. If enabled: Only allows complete-auth, root, /new, /quickstart + // 3. If disabled: Falls back to login validation (broader patterns) + // 4. /api/login endpoint always uses login validation + + expect(true).to.equal(true); // Documentation test + }); + + it("should show difference between strict and fallback validation", () => { + const testUrls = [ + { url: "https://gitpod.io/workspaces", strictAllowed: false, fallbackAllowed: true }, + { url: "https://gitpod.io/settings", strictAllowed: false, fallbackAllowed: true }, + { url: "https://gitpod.io/new", strictAllowed: true, fallbackAllowed: true }, + { url: "https://gitpod.io/quickstart", strictAllowed: true, fallbackAllowed: true }, + { + url: "https://gitpod.io/complete-auth?message=success", + strictAllowed: true, + fallbackAllowed: true, + }, + ]; + + testUrls.forEach(({ url, strictAllowed, fallbackAllowed }) => { + const strictResult = validateAuthorizeReturnToUrl(url, hostUrl); + const fallbackResult = validateLoginReturnToUrl(url, hostUrl); + + expect(strictResult).to.equal(strictAllowed, `Strict validation failed for: ${url}`); + expect(fallbackResult).to.equal(fallbackAllowed, `Fallback validation failed for: ${url}`); + }); + }); + }); + }); + + describe("validateAuthorizeReturnToUrl", () => { + it("should accept only specific allowlisted paths", () => { + const validUrls = [ + "https://gitpod.io/", // Root + "https://gitpod.io/new", // Create workspace page + "https://gitpod.io/quickstart", // Quickstart page + ]; + + validUrls.forEach((url) => { + const result = validateAuthorizeReturnToUrl(url, hostUrl); + expect(result).to.equal(true, `Should accept valid authorize URL: ${url}`); + }); + }); + + it("should accept complete-auth with ONLY message parameter", () => { + const validCompleteAuthUrls = [ + "https://gitpod.io/complete-auth?message=success:123", + "https://gitpod.io/complete-auth?message=success", + ]; + + validCompleteAuthUrls.forEach((url) => { + const result = validateAuthorizeReturnToUrl(url, hostUrl); + expect(result).to.equal(true, `Should accept complete-auth with only message: ${url}`); + }); + }); + + it("should reject paths not in authorize allowlist", () => { + const rejectedUrls = [ + "https://gitpod.io/workspaces", + "https://gitpod.io/workspaces/123", + "https://gitpod.io/user-settings", + "https://gitpod.io/integrations", + "https://gitpod.io/prebuilds", + "https://gitpod.io/members", + "https://gitpod.io/billing", + "https://gitpod.io/usage", + "https://gitpod.io/insights", + "https://gitpod.io/login", + "https://gitpod.io/settings", + "https://gitpod.io/repositories", + "https://gitpod.io/admin", + "https://gitpod.io/api/workspace", + ]; + + rejectedUrls.forEach((url) => { + const result = validateAuthorizeReturnToUrl(url, hostUrl); + expect(result).to.equal(false, `Should reject authorize URL: ${url}`); + }); + }); + + it("should accept www.gitpod.io root only", () => { + const result = validateAuthorizeReturnToUrl("https://www.gitpod.io/", hostUrl); + expect(result).to.equal(true, "Should accept www.gitpod.io root"); + }); + }); + + describe("Common validation tests", () => { + it("should reject different origins", () => { + const invalidOriginUrls = [ + "https://evil.com/workspaces", + "http://gitpod.io/workspaces", // Different protocol + "https://gitpod.io:8080/workspaces", // Different port + "https://subdomain.gitpod.io/workspaces", // Different subdomain + ]; + + invalidOriginUrls.forEach((url) => { + expect(validateLoginReturnToUrl(url, hostUrl)).to.equal(false, `Login should reject: ${url}`); + expect(validateAuthorizeReturnToUrl(url, hostUrl)).to.equal(false, `Authorize should reject: ${url}`); + }); + }); + + it("should have different path restrictions for login vs authorize", () => { + const pathsAllowedInLoginOnly = [ + "https://gitpod.io/admin", + "https://gitpod.io/workspace-123", + "https://gitpod.io/api/workspace", + "https://gitpod.io/workspaces", + "https://gitpod.io/settings", + "https://gitpod.io/any-arbitrary-path", + ]; + + pathsAllowedInLoginOnly.forEach((url) => { + expect(validateLoginReturnToUrl(url, hostUrl)).to.equal(true, `Login should allow: ${url}`); + expect(validateAuthorizeReturnToUrl(url, hostUrl)).to.equal(false, `Authorize should reject: ${url}`); + }); + }); + + it("should reject invalid URLs", () => { + const invalidUrls = [ + "not-a-url", + "javascript:alert('xss')", + "data:text/html,", + "", + "//evil.com/workspaces", + "ftp://gitpod.io/workspaces", + ]; + + invalidUrls.forEach((url) => { + expect(validateLoginReturnToUrl(url, hostUrl)).to.equal(false, `Login should reject: ${url}`); + expect(validateAuthorizeReturnToUrl(url, hostUrl)).to.equal(false, `Authorize should reject: ${url}`); + }); + }); + + it("should work with different host configurations", () => { + const previewHostUrl = new GitpodHostUrl("https://preview.gitpod-dev.com"); + + // Login should accept any path on same origin + const validLoginUrls = [ + "https://preview.gitpod-dev.com/workspaces", + "https://preview.gitpod-dev.com/complete-auth?message=success", + "https://preview.gitpod-dev.com/any-path", + ]; + + validLoginUrls.forEach((url) => { + expect(validateLoginReturnToUrl(url, previewHostUrl)).to.equal(true, `Login should accept: ${url}`); + }); + + // Authorize should only accept allowlisted paths + const validAuthorizeUrls = [ + "https://preview.gitpod-dev.com/", // Root + "https://preview.gitpod-dev.com/new", // Create workspace page + "https://preview.gitpod-dev.com/quickstart", // Quickstart page + "https://preview.gitpod-dev.com/complete-auth?message=success", + ]; + + validAuthorizeUrls.forEach((url) => { + expect(validateAuthorizeReturnToUrl(url, previewHostUrl)).to.equal( + true, + `Authorize should accept: ${url}`, + ); + }); + + // Should reject /workspaces for authorize since it's not in allowlist + expect(validateAuthorizeReturnToUrl("https://preview.gitpod-dev.com/workspaces", previewHostUrl)).to.equal( + false, + ); + + // Should reject URLs for different hosts + expect(validateLoginReturnToUrl("https://gitpod.io/workspaces", previewHostUrl)).to.equal(false); + expect(validateAuthorizeReturnToUrl("https://gitpod.io/workspaces", previewHostUrl)).to.equal(false); + }); + + it("should validate www.gitpod.io specifically", () => { + // Test with production gitpod.io + const prodHostUrl = new GitpodHostUrl("https://gitpod.io"); + expect(validateLoginReturnToUrl("https://www.gitpod.io/", prodHostUrl)).to.equal(true); + expect(validateAuthorizeReturnToUrl("https://www.gitpod.io/", prodHostUrl)).to.equal(true); + + // Test with different deployment - www.gitpod.io should still work as it's hardcoded + const customHostUrl = new GitpodHostUrl("https://gitpod.example.com"); + expect(validateLoginReturnToUrl("https://www.gitpod.io/", customHostUrl)).to.equal(true); + expect(validateAuthorizeReturnToUrl("https://www.gitpod.io/", customHostUrl)).to.equal(true); + + // Test that other www subdomains don't work + expect(validateLoginReturnToUrl("https://www.gitpod.example.com/", prodHostUrl)).to.equal(false); + expect(validateAuthorizeReturnToUrl("https://www.gitpod.example.com/", prodHostUrl)).to.equal(false); + + // Test domain injection prevention + expect(validateLoginReturnToUrl("https://www.gitpod.io.evil.com/", prodHostUrl)).to.equal(false); + expect(validateAuthorizeReturnToUrl("https://www.gitpod.io.evil.com/", prodHostUrl)).to.equal(false); + }); + }); +}); diff --git a/components/server/src/container-module.ts b/components/server/src/container-module.ts index 5164b779a1529c..40a1a05be788a1 100644 --- a/components/server/src/container-module.ts +++ b/components/server/src/container-module.ts @@ -45,6 +45,7 @@ import { HostContainerMapping } from "./auth/host-container-mapping"; import { HostContextProvider, HostContextProviderFactory } from "./auth/host-context-provider"; import { HostContextProviderImpl } from "./auth/host-context-provider-impl"; import { AuthJWT, SignInJWT } from "./auth/jwt"; +import { NonceService } from "./auth/nonce-service"; import { LoginCompletionHandler } from "./auth/login-completion-handler"; import { VerificationService } from "./auth/verification-service"; import { InstallationService } from "./auth/installation-service"; @@ -366,6 +367,7 @@ export const productionContainerModule = new ContainerModule( bind(AuthJWT).toSelf().inSingletonScope(); bind(SignInJWT).toSelf().inSingletonScope(); + bind(NonceService).toSelf().inSingletonScope(); bind(PrebuildManager).toSelf().inSingletonScope(); bind(LazyPrebuildManager).toFactory((ctx) => { diff --git a/components/server/src/express-util.ts b/components/server/src/express-util.ts index 8cdd6eb5343ade..f3f96984cc513b 100644 --- a/components/server/src/express-util.ts +++ b/components/server/src/express-util.ts @@ -8,6 +8,10 @@ import { URL } from "url"; import express from "express"; import * as crypto from "crypto"; import { IncomingHttpHeaders } from "http"; +import { GitpodHostUrl } from "@gitpod/gitpod-protocol/lib/util/gitpod-host-url"; +import { ensureUrlHasFragment } from "./auth/fragment-utils"; +import { log } from "@gitpod/gitpod-protocol/lib/util/logging"; +import { TrustedValue } from "@gitpod/gitpod-protocol/lib/util/scrubbing"; export const query = (...tuples: [string, string][]) => { if (tuples.length === 0) { @@ -159,3 +163,117 @@ export function toClientHeaderFields(expressReq: express.Request): ClientHeaderF clientRegion: takeFirst(expressReq.headers["x-glb-client-region"]), }; } + +/** + * Common validation logic for returnTo URLs. + * @param returnTo The URL to validate + * @param hostUrl The host URL configuration + * @param allowedPatterns Array of regex patterns that are allowed for the pathname + * @returns true if the URL is valid, false otherwise + */ +function validateReturnToUrlWithPatterns( + returnTo: string, + allowedBaseUrl: GitpodHostUrl, + allowedPatterns?: RegExp[], +): boolean { + try { + const url = new URL(returnTo); + const baseUrl = allowedBaseUrl.url; + + // Must be same origin OR www.gitpod.io exception + const isSameOrigin = url.origin === baseUrl.origin; + const isGitpodWebsite = url.protocol === "https:" && url.hostname === "www.gitpod.io"; + + if (!isSameOrigin && !isGitpodWebsite) { + return false; + } + + // For www.gitpod.io, only allow root path + if (isGitpodWebsite) { + return url.pathname === "/"; + } + + if (allowedPatterns !== undefined) { + // Check if pathname matches any allowed pattern + const isAllowedPath = allowedPatterns.some((pattern) => pattern.test(url.pathname)); + if (!isAllowedPath) { + return false; + } + } + + // For complete-auth, require ONLY message parameter (used by OAuth flows) + if (url.pathname === "/complete-auth") { + const searchParams = new URLSearchParams(url.search); + const paramKeys = Array.from(searchParams.keys()); + return paramKeys.length === 1 && paramKeys[0] === "message" && searchParams.has("message"); + } + + return true; + } catch (error) { + // Invalid URL + return false; + } +} + +/** + * Validates returnTo URLs for login API endpoints. + * Login API allows broader navigation after authentication. + */ +export function validateLoginReturnToUrl(returnTo: string, hostUrl: GitpodHostUrl): boolean { + // We just verify the domain + return validateReturnToUrlWithPatterns(returnTo, hostUrl, undefined); +} + +/** + * Validates returnTo URLs for authorize API endpoints. + * Authorize API allows complete-auth callbacks and dashboard pages for scope elevation. + */ +export function validateAuthorizeReturnToUrl(returnTo: string, hostUrl: GitpodHostUrl): boolean { + const allowedPatterns = [ + // 1. complete-auth callback for OAuth popup windows + /^\/complete-auth$/, + + // 2. Dashboard pages (for scope elevation flows) + /^\/$/, // Root + /^\/new\/?$/, // Create workspace page + /^\/quickstart\/?$/, // Quickstart page + ]; + + return validateReturnToUrlWithPatterns(returnTo, hostUrl, allowedPatterns); +} + +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 && !validator(returnToURL)) { + log.debug("The redirect URL does not match allowed patterns", { query: new TrustedValue(req.query).value }); + return; + } + + return returnToURL; +} + +/** + * Safe redirect wrapper that automatically ensures URLs have fragments to prevent + * OAuth token inheritance attacks. + * + * When OAuth providers redirect with tokens in URL fragments, browsers inherit + * fragments from the current page if the target URL doesn't have one. This wrapper + * automatically applies fragment protection to all redirects. + * + * @param res Express response object + * @param url URL to redirect to + * @param status Optional HTTP status code (default: 302) + */ +export function safeFragmentRedirect(res: express.Response, url: string, status?: number): void { + const protectedUrl = ensureUrlHasFragment(url); + if (status) { + res.redirect(status, protectedUrl); + } else { + res.redirect(protectedUrl); + } +} diff --git a/components/server/src/oauth-server/oauth-controller.ts b/components/server/src/oauth-server/oauth-controller.ts index 408ed570254674..33bb32f8fc0309 100644 --- a/components/server/src/oauth-server/oauth-controller.ts +++ b/components/server/src/oauth-server/oauth-controller.ts @@ -14,6 +14,7 @@ import express from "express"; import { inject, injectable } from "inversify"; import { URL } from "url"; import { Config } from "../config"; +import { safeFragmentRedirect } from "../express-util"; import { clientRepository, createAuthorizationServer } from "./oauth-authorization-server"; import { inMemoryDatabase, toolboxClient } from "./db"; import { getFeatureFlagEnableExperimentalJBTB } from "../util/featureflags"; @@ -28,7 +29,7 @@ export class OAuthController { if (!req.isAuthenticated() || !User.is(req.user)) { const returnToPath = encodeURIComponent(`/api${req.originalUrl}`); const redirectTo = `${this.config.hostUrl}login?returnToPath=${returnToPath}`; - res.redirect(redirectTo); + safeFragmentRedirect(res, redirectTo); return null; } const user = req.user as User; @@ -88,7 +89,7 @@ export class OAuthController { const redirectUri = new URL(req.query.redirect_uri); redirectUri.searchParams.append("approved", "no"); - res.redirect(redirectUri.toString()); + safeFragmentRedirect(res, redirectUri.toString()); return false; } else if (wasApproved == "yes") { const additionalData = (user.additionalData = user.additionalData || {}); @@ -104,7 +105,7 @@ export class OAuthController { if (client) { const returnToPath = encodeURIComponent(`/api${req.originalUrl}`); const redirectTo = `${this.config.hostUrl}oauth-approval?clientID=${client.id}&clientName=${client.name}&returnToPath=${returnToPath}`; - res.redirect(redirectTo); + safeFragmentRedirect(res, redirectTo); return false; } else { log.error(`/oauth/authorize unknown client id: "${clientID}"`); diff --git a/components/server/src/prebuilds/github-app.ts b/components/server/src/prebuilds/github-app.ts index 6ca30d52feb5e5..60d7b7edf9b90c 100644 --- a/components/server/src/prebuilds/github-app.ts +++ b/components/server/src/prebuilds/github-app.ts @@ -21,6 +21,7 @@ import { } from "@gitpod/gitpod-db/lib"; import express from "express"; import { log, LogContext, LogrusLogLevel } from "@gitpod/gitpod-protocol/lib/util/logging"; +import { safeFragmentRedirect } from "../express-util"; import { WorkspaceConfig, User, @@ -111,7 +112,7 @@ export class GithubApp { options .getRouter("/pbs") .get("/*", (req: express.Request, res: express.Response, next: express.NextFunction) => { - res.redirect(301, this.getBadgeImageURL()); + safeFragmentRedirect(res, this.getBadgeImageURL(), 301); }); app.on("installation.created", (ctx: Context<"installation.created">) => { @@ -190,7 +191,7 @@ export class GithubApp { const slug = data.data.slug; const state = req.query.state; - res.redirect(`https://github.com/apps/${slug}/installations/new?state=${state}`); + safeFragmentRedirect(res, `https://github.com/apps/${slug}/installations/new?state=${state}`); } catch (error) { console.error(error, { error }); res.status(500).send("GitHub App is not configured."); @@ -213,12 +214,12 @@ export class GithubApp { "message=payload:" + Buffer.from(JSON.stringify(payload), "utf-8").toString("base64"), }) .toString(); - res.redirect(url); + safeFragmentRedirect(res, url); } else { const url = this.config.hostUrl .with({ pathname: "install-github-app", search: `installation_id=${installationId}` }) .toString(); - res.redirect(url); + safeFragmentRedirect(res, url); } }); } diff --git a/components/server/src/user/newsletter-subscription-controller.ts b/components/server/src/user/newsletter-subscription-controller.ts index 4eaeae7792c453..9cb1adad808858 100644 --- a/components/server/src/user/newsletter-subscription-controller.ts +++ b/components/server/src/user/newsletter-subscription-controller.ts @@ -8,6 +8,7 @@ import express from "express"; import { inject, injectable } from "inversify"; import { UserDB } from "@gitpod/gitpod-db/lib"; import { IAnalyticsWriter } from "@gitpod/gitpod-protocol/lib/analytics"; +import { safeFragmentRedirect } from "../express-util"; @injectable() export class NewsletterSubscriptionController { @@ -72,7 +73,7 @@ export class NewsletterSubscriptionController { [newsletterProperties[newsletterType].property]: true, }, }); - res.redirect(successPageUrl); + safeFragmentRedirect(res, successPageUrl); } else { this.analytics.identify({ userId: email, @@ -80,7 +81,7 @@ export class NewsletterSubscriptionController { [newsletterProperties[newsletterType].property]: true, }, }); - res.redirect(successPageUrl); + safeFragmentRedirect(res, successPageUrl); } } catch (error) { res.send({ diff --git a/components/server/src/user/user-controller.ts b/components/server/src/user/user-controller.ts index 558c9c86c87365..8ded6409126ee1 100644 --- a/components/server/src/user/user-controller.ts +++ b/components/server/src/user/user-controller.ts @@ -17,7 +17,13 @@ import { Permission } from "@gitpod/gitpod-protocol/lib/permission"; import { parseWorkspaceIdFromHostname } from "@gitpod/gitpod-protocol/lib/util/parse-workspace-id"; import { SessionHandler } from "../session-handler"; import { URL } from "url"; -import { getRequestingClientInfo } from "../express-util"; +import { + getRequestingClientInfo, + validateAuthorizeReturnToUrl, + validateLoginReturnToUrl, + safeFragmentRedirect, + getSafeReturnToParam, +} from "../express-util"; import { GitpodToken, GitpodTokenType, User } from "@gitpod/gitpod-protocol"; import { HostContextProvider } from "../auth/host-context-provider"; import { reportJWTCookieIssued } from "../prometheus-metrics"; @@ -37,7 +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 { TrustedValue } from "@gitpod/gitpod-protocol/lib/util/scrubbing"; +import { getFeatureFlagEnableStrictAuthorizeReturnTo } from "../util/featureflags"; export const ServerFactory = Symbol("ServerFactory"); export type ServerFactory = () => GitpodServerImpl; @@ -65,8 +71,8 @@ export class UserController { if (req.isAuthenticated()) { log.info("(Auth) User is already authenticated.", { "login-flow": true }); // redirect immediately - const redirectTo = this.getSafeReturnToParam(req) || this.config.hostUrl.asDashboard().toString(); - res.redirect(redirectTo); + const redirectTo = this.ensureSafeReturnToParam(req) || this.config.hostUrl.asDashboard().toString(); + safeFragmentRedirect(res, redirectTo); return; } const clientInfo = getRequestingClientInfo(req); @@ -81,11 +87,11 @@ export class UserController { // If there is no known auth host, we need to ask the user const redirectToLoginPage = !req.query.host; if (redirectToLoginPage) { - const returnTo = this.getSafeReturnToParam(req); + const returnTo = this.ensureSafeReturnToParam(req); const search = returnTo ? `returnTo=${returnTo}` : ""; const loginPageUrl = this.config.hostUrl.asLogin().with({ search }).toString(); log.info(`Redirecting to login ${loginPageUrl}`); - res.redirect(loginPageUrl); + safeFragmentRedirect(res, loginPageUrl); return; } @@ -185,12 +191,12 @@ export class UserController { // Redirect the admin-user to the Org Settings page. // The dashboard is expected to render the Onboading flow instead of the regular view, // but if the browser is reloaded after completion of the flow, it should be fine to see the settings. - res.redirect("/settings", 307); + safeFragmentRedirect(res, "/settings", 307); } catch (e) { log.error("Failed to sign-in as admin with OTS Token", e); // Always redirect to an expired token page if there's an error - res.redirect("/error/expired-ots", 307); + safeFragmentRedirect(res, "/error/expired-ots", 307); return; } }); @@ -215,10 +221,10 @@ export class UserController { reportJWTCookieIssued(); // If returnTo was passed and it's safe, redirect to it - const returnTo = this.getSafeReturnToParam(req); + const returnTo = this.ensureSafeReturnToParam(req); if (returnTo) { log.info(`Redirecting after OTS login ${returnTo}`); - res.redirect(returnTo); + safeFragmentRedirect(res, returnTo); return; } @@ -236,7 +242,15 @@ export class UserController { return; } this.ensureSafeReturnToParam(req); - this.authenticator.authorize(req, res, next).catch((err) => log.error("authenticator.authorize", err)); + 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)) { @@ -248,7 +262,15 @@ export class UserController { return; } this.ensureSafeReturnToParam(req); - this.authenticator.deauthorize(req, res, next).catch((err) => log.error("authenticator.deauthorize", err)); + 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 }); @@ -271,7 +293,7 @@ export class UserController { }); } - const redirectToUrl = this.getSafeReturnToParam(req) || this.config.hostUrl.toString(); + const redirectToUrl = this.ensureSafeReturnToParam(req) || this.config.hostUrl.toString(); if (req.isAuthenticated()) { req.logout(); @@ -282,7 +304,7 @@ export class UserController { // then redirect log.info(logContext, "(Logout) Redirecting...", { redirectToUrl, ...logPayload }); - res.redirect(redirectToUrl); + safeFragmentRedirect(res, redirectToUrl); }); router.get("/auth/jwt-cookie", this.sessionHandler.jwtSessionConvertor()); @@ -504,7 +526,7 @@ export class UserController { otsExpirationTime.setMinutes(otsExpirationTime.getMinutes() + 2); const ots = await this.otsServer.serve({}, token, otsExpirationTime); - res.redirect(`http://${rt}/?ots=${encodeURI(ots.url)}`); + safeFragmentRedirect(res, `http://${rt}/?ots=${encodeURI(ots.url)}`); }, ); } @@ -561,7 +583,7 @@ export class UserController { } protected async augmentLoginRequest(req: express.Request) { - const returnToURL = this.getSafeReturnToParam(req); + const returnToURL = this.ensureSafeReturnToParam(req); if (req.query.host) { // This login request points already to an auth host return; @@ -603,32 +625,28 @@ export class UserController { } } - protected ensureSafeReturnToParam(req: express.Request) { - req.query.returnTo = this.getSafeReturnToParam(req); - } - - protected urlStartsWith(url: string, prefixUrl: string): boolean { - prefixUrl += prefixUrl.endsWith("/") ? "" : "/"; - return url.toLowerCase().startsWith(prefixUrl.toLowerCase()); + protected ensureSafeReturnToParam(req: express.Request): string | undefined { + const returnTo = getSafeReturnToParam(req, (url) => validateLoginReturnToUrl(url, this.config.hostUrl)); + req.query.returnTo = returnTo; + return returnTo; } - protected getSafeReturnToParam(req: express.Request) { - // @ts-ignore Type 'ParsedQs' is not assignable - const returnToURL: string | undefined = req.query.redirect || req.query.returnTo; - if (!returnToURL) { - log.debug("Empty redirect URL"); - return; - } - - if ( - this.urlStartsWith(returnToURL, this.config.hostUrl.toString()) || - this.urlStartsWith(returnToURL, "https://www.gitpod.io") - ) { - return returnToURL; + // TODO(gpl): Once we drop the feature flag, turn into the same form as above method. + protected async ensureSafeReturnToParamForAuthorize(req: express.Request): Promise { + 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; + } + } } - log.debug("The redirect URL does not match", { query: new TrustedValue(req.query).value }); - return; + req.query.returnTo = returnTo; + return returnTo; } private createGitpodServer(user: User, resourceGuard: ResourceAccessGuard) { diff --git a/components/server/src/util/featureflags.ts b/components/server/src/util/featureflags.ts index aeec45cfe39339..d1264c7115a0c1 100644 --- a/components/server/src/util/featureflags.ts +++ b/components/server/src/util/featureflags.ts @@ -17,3 +17,19 @@ export async function getFeatureFlagEnableContextEnvVarValidation(userId: string user: { id: userId }, }); } + +/** + * Feature flag for enabling nonce validation in OAuth flows. + * Default: false (disabled) + */ +export async function getFeatureFlagEnableNonceValidation(): Promise { + return getExperimentsClientForBackend().getValueAsync("enable_nonce_validation", false, {}); +} + +/** + * Feature flag for enabling strict returnTo validation for /api/authorize endpoint. + * Default: false (disabled, falls back to login validation) + */ +export async function getFeatureFlagEnableStrictAuthorizeReturnTo(): Promise { + return getExperimentsClientForBackend().getValueAsync("enable_strict_authorize_return_to", false, {}); +} diff --git a/components/server/src/workspace/headless-log-controller.ts b/components/server/src/workspace/headless-log-controller.ts index b6aab3455caa98..a02a3f9e18e52a 100644 --- a/components/server/src/workspace/headless-log-controller.ts +++ b/components/server/src/workspace/headless-log-controller.ts @@ -15,6 +15,7 @@ import { WorkspaceInstance, } from "@gitpod/gitpod-protocol"; import { log, LogContext } from "@gitpod/gitpod-protocol/lib/util/logging"; +import { safeFragmentRedirect } from "../express-util"; import { CompositeResourceAccessGuard, OwnerResourceGuard, @@ -252,7 +253,7 @@ export class HeadlessLogController { ); }); if (redirect) { - res.redirect(302, redirect.taskUrl); + safeFragmentRedirect(res, redirect.taskUrl, 302); return; } diff --git a/memory-bank/components/server.md b/memory-bank/components/server.md index 24fdc5cb548b47..e154aa145ddf19 100644 --- a/memory-bank/components/server.md +++ b/memory-bank/components/server.md @@ -117,6 +117,7 @@ The Server integrates with: - Implements proper error handling and logging - Uses HTTPS for secure communication - Manages sensitive data securely +- Uses `safeFragmentRedirect()` for all HTTP redirects to prevent OAuth token inheritance attacks ## Common Usage Patterns