diff --git a/CLAUDE.md b/CLAUDE.md index 8efb0a02ca0806..183b008b534c98 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,6 +27,7 @@ The `memory-bank/components/` directory contains detailed documentation for each 2. **Component-specific work** - Refer to the relevant component documentation in `memory-bank/components/` 3. **Architecture decisions** - Check `memory-bank/systemPatterns.md` for established patterns and conventions 4. **Current focus** - Review `memory-bank/activeContext.md` for ongoing work and priorities +5. **Git workflow** - **CRITICAL**: Always use feature branches with proper naming convention (see `memory-bank/systemPatterns.md` for branch naming rules) ## Key Characteristics diff --git a/components/server/src/auth/authenticator.spec.ts b/components/server/src/auth/authenticator.spec.ts new file mode 100644 index 00000000000000..2d13f3d3f44717 --- /dev/null +++ b/components/server/src/auth/authenticator.spec.ts @@ -0,0 +1,189 @@ +/** + * Copyright (c) 2020 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 * as chai from "chai"; +import { validateAuthorizeReturnToUrl } from "./authenticator"; +import { Config } from "../config"; +import { AuthProvider } from "./auth-provider"; + +const expect = chai.expect; + +describe("authenticator", function () { + describe("validateAuthorizeReturnToUrl", function () { + const mockConfig = { + hostUrl: { + url: new URL("https://gitpod.io"), + }, + } as Config; + + const mockAuthProvider = {} as AuthProvider; + + describe("valid URLs", function () { + it("should return true for valid URLs from configured domain", function () { + const result = validateAuthorizeReturnToUrl( + "https://gitpod.io/dashboard", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.true; + }); + + it("should return true for valid URLs from www.gitpod.io", function () { + const result = validateAuthorizeReturnToUrl( + "https://www.gitpod.io/pricing", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.true; + }); + + it("should return false for URLs with subdomain that don't match prefix", function () { + const result = validateAuthorizeReturnToUrl( + "https://app.gitpod.io/workspaces", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.false; + }); + }); + + describe("invalid URLs - API paths", function () { + it("should return false for URLs with /api path", function () { + const result = validateAuthorizeReturnToUrl( + "https://gitpod.io/api/authorize", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.false; + }); + + it("should return false for URLs with /api path", function () { + const result = validateAuthorizeReturnToUrl( + "https://gitpod.io/api/users", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.false; + }); + + it("should return false for URLs with /api/ path", function () { + const result = validateAuthorizeReturnToUrl("https://gitpod.io/api/", mockConfig, mockAuthProvider); + expect(result).to.be.false; + }); + + it("should return false for URLs with /API path (case insensitive)", function () { + const result = validateAuthorizeReturnToUrl( + "https://gitpod.io/API/users", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.false; + }); + + it("should return false for URLs with /api/callback path", function () { + const result = validateAuthorizeReturnToUrl( + "https://gitpod.io/api/callback", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.false; + }); + + it("should return false for URLs with api. subdomain", function () { + const result = validateAuthorizeReturnToUrl( + "https://api.gitpod.io/users", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.false; + }); + + it("should return false for URLs with API. subdomain (case insensitive)", function () { + const result = validateAuthorizeReturnToUrl( + "https://API.gitpod.io/users", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.false; + }); + + it("should return false for URLs with api. subdomain and /callback path", function () { + const result = validateAuthorizeReturnToUrl( + "https://api.gitpod.io/callback", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.false; + }); + }); + + describe("invalid URLs - external domains", function () { + it("should return false for URLs from external domains", function () { + const result = validateAuthorizeReturnToUrl("https://evil.com/dashboard", mockConfig, mockAuthProvider); + expect(result).to.be.false; + }); + + it("should return false for URLs that look similar but are external", function () { + const result = validateAuthorizeReturnToUrl( + "https://gitpod.io.evil.com/dashboard", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.false; + }); + }); + + describe("malformed URLs", function () { + it("should return false for malformed URLs", function () { + const result = validateAuthorizeReturnToUrl("not-a-url", mockConfig, mockAuthProvider); + expect(result).to.be.false; + }); + + it("should return false for URLs with invalid protocol", function () { + const result = validateAuthorizeReturnToUrl("javascript:alert('xss')", mockConfig, mockAuthProvider); + expect(result).to.be.false; + }); + }); + + describe("edge cases", function () { + it("should return true for URLs with query parameters", function () { + const result = validateAuthorizeReturnToUrl( + "https://gitpod.io/dashboard?tab=workspaces", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.true; + }); + + it("should return true for URLs with fragments", function () { + const result = validateAuthorizeReturnToUrl( + "https://gitpod.io/dashboard#workspaces", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.true; + }); + + it("should return true for URLs with 'api' in query parameter but not in path", function () { + const result = validateAuthorizeReturnToUrl( + "https://gitpod.io/dashboard?redirect=/api", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.true; + }); + + it("should return true for URLs with 'api' in path but not starting with /api", function () { + const result = validateAuthorizeReturnToUrl( + "https://gitpod.io/graphql-api", + mockConfig, + mockAuthProvider, + ); + expect(result).to.be.true; + }); + }); + }); +}); diff --git a/components/server/src/auth/authenticator.ts b/components/server/src/auth/authenticator.ts index eafd49e81af9f7..0ae2518e4ae11b 100644 --- a/components/server/src/auth/authenticator.ts +++ b/components/server/src/auth/authenticator.ts @@ -18,6 +18,8 @@ import { UserService } from "../user/user-service"; import { AuthFlow, AuthProvider } from "./auth-provider"; import { HostContextProvider } from "./host-context-provider"; import { SignInJWT } from "./jwt"; +import { getReturnToParamWithSafeBaseDomain } from "../express-util"; +import { getFeatureFlagEnforceAuthorizeStateValidation } from "../util/featureflags"; @injectable() export class Authenticator { @@ -239,6 +241,17 @@ export class Authenticator { return; } + // Validate returnTo URL if feature flag is enabled + const enforceValidation = await getFeatureFlagEnforceAuthorizeStateValidation(user.id); + if (enforceValidation) { + const valid = validateAuthorizeReturnToUrl(returnTo, this.config, authProvider); + if (!valid) { + log.info(`Bad request: invalid returnTo URL.`, { "authorize-flow": true }); + res.redirect(this.getSorryUrl(`Bad request: invalid returnTo URL.`)); + return; + } + } + // 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); @@ -322,3 +335,35 @@ export class Authenticator { return this.config.hostUrl.asSorry(message).toString(); } } + +// Whether the passed URL is a valid returnTo URL for OAuth authorization +export function validateAuthorizeReturnToUrl(returnTo: string, config: Config, authProvider: AuthProvider): boolean { + function isApiPath(url: string): boolean { + try { + const parsedUrl = new URL(url); + + // Check if hostname starts with "api." + if (parsedUrl.hostname.toLowerCase().startsWith("api.")) { + return true; + } + + // Check if path starts with "/api" + if (parsedUrl.pathname.toLowerCase().startsWith("/api")) { + return true; + } + + return false; + } catch { + // If URL parsing fails, consider it invalid + return true; + } + } + + // Reject URLs that point to API endpoints + if (isApiPath(returnTo)) { + return false; + } + + const validUrl = getReturnToParamWithSafeBaseDomain(returnTo, config.hostUrl.url); + return validUrl !== undefined; +} diff --git a/components/server/src/auth/generic-auth-provider.ts b/components/server/src/auth/generic-auth-provider.ts index 1295f3d96030b4..7ef03d15cc37e1 100644 --- a/components/server/src/auth/generic-auth-provider.ts +++ b/components/server/src/auth/generic-auth-provider.ts @@ -24,6 +24,8 @@ import { } from "../auth/errors"; import { Config } from "../config"; import { getRequestingClientInfo } from "../express-util"; +import { getFeatureFlagEnforceAuthorizeStateValidation } from "../util/featureflags"; +import { validateAuthorizeReturnToUrl } from "./authenticator"; import { TokenProvider } from "../user/token-provider"; import { UserAuthentication } from "../user/user-authentication"; import { AuthProviderService } from "./auth-provider-service"; @@ -302,6 +304,21 @@ export abstract class GenericAuthProvider implements AuthProvider { return; } + // Validate returnTo URL if feature flag is enabled + if (authFlow.returnTo) { + // Check feature flag - use existing user if logged in, otherwise default to false for new users + const userId = request.user?.id; + const enforceValidation = await getFeatureFlagEnforceAuthorizeStateValidation(userId || ""); + if (enforceValidation) { + const valid = validateAuthorizeReturnToUrl(authFlow.returnTo, this.config, this); + if (!valid) { + log.info(cxt, `(${strategyName}) Bad request: invalid returnTo URL.`, { clientInfo }); + response.redirect(this.getSorryUrl(`Bad request: invalid returnTo URL.`)); + 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 }); @@ -321,7 +338,6 @@ export abstract class GenericAuthProvider implements AuthProvider { return; } } - // assert additional infomation is attached to current session if (!authFlow) { // The auth flow state info is missing in the session: count as client error diff --git a/components/server/src/express-util.spec.ts b/components/server/src/express-util.spec.ts index ad259a86a1d0bb..9ae6f72b1dc983 100644 --- a/components/server/src/express-util.spec.ts +++ b/components/server/src/express-util.spec.ts @@ -5,7 +5,7 @@ */ import * as chai from "chai"; -import { isAllowedWebsocketDomain } from "./express-util"; +import { isAllowedWebsocketDomain, getReturnToParamWithSafeBaseDomain } from "./express-util"; const expect = chai.expect; describe("express-util", function () { @@ -47,4 +47,82 @@ describe("express-util", function () { expect(result).to.be.false; }); }); + + describe("getReturnToParamWithSafeBaseDomain", function () { + const configuredBaseDomain = new URL("https://gitpod.io"); + + describe("valid URLs", function () { + it("should allow URLs from configured base domain", function () { + const result = getReturnToParamWithSafeBaseDomain("https://gitpod.io/dashboard", configuredBaseDomain); + expect(result).to.equal("https://gitpod.io/dashboard"); + }); + + it("should allow URLs from www.gitpod.io", function () { + const result = getReturnToParamWithSafeBaseDomain( + "https://www.gitpod.io/pricing", + configuredBaseDomain, + ); + expect(result).to.equal("https://www.gitpod.io/pricing"); + }); + + it("should reject URLs with subdomain that don't match prefix", function () { + const result = getReturnToParamWithSafeBaseDomain( + "https://app.gitpod.io/workspaces", + configuredBaseDomain, + ); + expect(result).to.be.undefined; + }); + + it("should handle undefined returnToURL", function () { + const result = getReturnToParamWithSafeBaseDomain(undefined, configuredBaseDomain); + expect(result).to.be.undefined; + }); + + it("should handle empty returnToURL", function () { + const result = getReturnToParamWithSafeBaseDomain("", configuredBaseDomain); + expect(result).to.be.undefined; + }); + }); + + describe("invalid URLs - external domains", function () { + it("should reject URLs from external domains", function () { + const result = getReturnToParamWithSafeBaseDomain("https://evil.com/dashboard", configuredBaseDomain); + expect(result).to.be.undefined; + }); + + it("should reject URLs that look similar but are external", function () { + const result = getReturnToParamWithSafeBaseDomain( + "https://gitpod.io.evil.com/dashboard", + configuredBaseDomain, + ); + expect(result).to.be.undefined; + }); + }); + + describe("edge cases", function () { + it("should allow URLs with query parameters", function () { + const result = getReturnToParamWithSafeBaseDomain( + "https://gitpod.io/dashboard?tab=workspaces", + configuredBaseDomain, + ); + expect(result).to.equal("https://gitpod.io/dashboard?tab=workspaces"); + }); + + it("should allow URLs with fragments", function () { + const result = getReturnToParamWithSafeBaseDomain( + "https://gitpod.io/dashboard#workspaces", + configuredBaseDomain, + ); + expect(result).to.equal("https://gitpod.io/dashboard#workspaces"); + }); + + it("should allow URLs with 'api' in path but not starting with /api", function () { + const result = getReturnToParamWithSafeBaseDomain( + "https://gitpod.io/graphql-api", + configuredBaseDomain, + ); + expect(result).to.equal("https://gitpod.io/graphql-api"); + }); + }); + }); }); diff --git a/components/server/src/express-util.ts b/components/server/src/express-util.ts index 8cdd6eb5343ade..ca51e763c46cff 100644 --- a/components/server/src/express-util.ts +++ b/components/server/src/express-util.ts @@ -159,3 +159,26 @@ export function toClientHeaderFields(expressReq: express.Request): ClientHeaderF clientRegion: takeFirst(expressReq.headers["x-glb-client-region"]), }; } + +export function getReturnToParamWithSafeBaseDomain( + returnToURL: string | undefined, + configuredBaseDomain: URL, +): string | undefined { + function urlStartsWith(url: string, prefixUrl: string): boolean { + prefixUrl += prefixUrl.endsWith("/") ? "" : "/"; + return url.toLowerCase().startsWith(prefixUrl.toLowerCase()); + } + + if (!returnToURL) { + return undefined; + } + + if ( + urlStartsWith(returnToURL, configuredBaseDomain.toString()) || + urlStartsWith(returnToURL, "https://www.gitpod.io") + ) { + return returnToURL; + } + + return; +} diff --git a/components/server/src/user/user-controller.ts b/components/server/src/user/user-controller.ts index 558c9c86c87365..654d157aa4930e 100644 --- a/components/server/src/user/user-controller.ts +++ b/components/server/src/user/user-controller.ts @@ -17,7 +17,7 @@ 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, getReturnToParamWithSafeBaseDomain } from "../express-util"; import { GitpodToken, GitpodTokenType, User } from "@gitpod/gitpod-protocol"; import { HostContextProvider } from "../auth/host-context-provider"; import { reportJWTCookieIssued } from "../prometheus-metrics"; @@ -615,20 +615,12 @@ export class UserController { 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; + const result = getReturnToParamWithSafeBaseDomain(returnToURL, this.config.hostUrl.url); + if (!!returnToURL && !result) { + log.debug("The redirect URL does not match", { query: new TrustedValue(req.query).value }); } - - log.debug("The redirect URL does not match", { query: new TrustedValue(req.query).value }); - return; + return result; } private createGitpodServer(user: User, resourceGuard: ResourceAccessGuard) { diff --git a/components/server/src/util/featureflags.ts b/components/server/src/util/featureflags.ts index aeec45cfe39339..cc3cc3f9a8588e 100644 --- a/components/server/src/util/featureflags.ts +++ b/components/server/src/util/featureflags.ts @@ -17,3 +17,9 @@ export async function getFeatureFlagEnableContextEnvVarValidation(userId: string user: { id: userId }, }); } + +export async function getFeatureFlagEnforceAuthorizeStateValidation(userId: string): Promise { + return getExperimentsClientForBackend().getValueAsync("enforce_authorize_state_validation", false, { + user: { id: userId }, + }); +} diff --git a/memory-bank/systemPatterns.md b/memory-bank/systemPatterns.md index a76639d95b9394..23f3a4ddcbaa62 100644 --- a/memory-bank/systemPatterns.md +++ b/memory-bank/systemPatterns.md @@ -112,6 +112,27 @@ Gitpod follows a microservices architecture designed to be **Scalable**, **Resil ## Development Workflows +### Git Branch Management + +#### Feature Branch Naming Convention +**CRITICAL**: Always use feature branches from main for all work. Never work directly on main. + +**Naming Pattern**: `[initials]/[[issue-id]-][topic-shorthand]` + +**Rules**: +- **Maximum 24 characters total** +- **Initials**: Extract from `git config user.name` (first letter of each word) +- **Issue ID**: Numeric part only (e.g., "1592" from "CLC-1592") +- **Topic**: 2-3 words separated by dashes, describing the work +- **IMPORTANT**: Always run `git config user.name` first - never assume initials + +**Workflow**: +1. Run `git config user.name` to get actual name +2. Extract initials from the output +3. Create feature branch: `git checkout -b [initials]/[topic]` +4. Push work to feature branch +5. Create PR against main + ### PRD Workflow 1. **Requirements Gathering** (Plan): Understand problem, explore components 2. **PRD Creation** (Plan): Create document with standardized sections