Skip to content

Commit b33555f

Browse files
iQQBotona-agent
andcommitted
feat: add feature flags for nonce validation and strict authorize returnTo
Add two feature flags to control security features with safe defaults: **Feature Flag 1: enable_nonce_validation (default: false)** - Controls CSRF nonce validation in OAuth flows - When disabled: Nonce is generated but not validated (future compatibility) - When enabled: Full CSRF protection with nonce and origin validation - Nonce cookies are always generated and cleared for consistency **Feature Flag 2: enable_strict_authorize_return_to (default: false)** - Controls returnTo validation strictness for /api/authorize endpoint - When disabled: Falls back to login validation (broader patterns) - When enabled: Uses strict authorize validation (limited to specific paths) - /api/login always uses login validation regardless of flag **Implementation Details:** - Always generate nonce for consistency and future compatibility - Only validate nonce when feature flag is enabled - Always clear nonce cookies regardless of validation state - Authorize endpoint checks flag and falls back gracefully - Comprehensive logging for debugging and monitoring **Backward Compatibility:** - Default false ensures no breaking changes - Gradual rollout possible via feature flag configuration - Existing authentication flows continue to work - Safe fallback behavior when flags are disabled Co-authored-by: Ona <[email protected]>
1 parent df392be commit b33555f

File tree

3 files changed

+110
-29
lines changed

3 files changed

+110
-29
lines changed

components/server/src/auth/authenticator.ts

Lines changed: 42 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import { HostContextProvider } from "./host-context-provider";
2121
import { SignInJWT } from "./jwt";
2222
import { NonceService } from "./nonce-service";
2323
import { ensureUrlHasFragment } from "./fragment-utils";
24+
import { getFeatureFlagEnableNonceValidation, getFeatureFlagEnableStrictAuthorizeReturnTo } from "../util/featureflags";
2425

2526
/**
2627
* Common validation logic for returnTo URLs.
@@ -173,32 +174,35 @@ export class Authenticator {
173174
return;
174175
}
175176

176-
// Validate nonce for CSRF protection
177-
const stateNonce = flowState.nonce;
178-
const cookieNonce = this.nonceService.getNonceFromCookie(req);
179-
180-
if (!this.nonceService.validateNonce(stateNonce, cookieNonce)) {
181-
log.error(`CSRF protection: Nonce validation failed`, {
182-
url: req.url,
183-
hasStateNonce: !!stateNonce,
184-
hasCookieNonce: !!cookieNonce,
185-
});
186-
res.status(403).send("Authentication failed");
187-
return;
177+
// Validate nonce for CSRF protection (if feature flag is enabled)
178+
const isNonceValidationEnabled = await getFeatureFlagEnableNonceValidation();
179+
if (isNonceValidationEnabled) {
180+
const stateNonce = flowState.nonce;
181+
const cookieNonce = this.nonceService.getNonceFromCookie(req);
182+
183+
if (!this.nonceService.validateNonce(stateNonce, cookieNonce)) {
184+
log.error(`CSRF protection: Nonce validation failed`, {
185+
url: req.url,
186+
hasStateNonce: !!stateNonce,
187+
hasCookieNonce: !!cookieNonce,
188+
});
189+
res.status(403).send("Authentication failed");
190+
return;
191+
}
192+
193+
// Validate origin for additional CSRF protection
194+
if (!this.nonceService.validateOrigin(req)) {
195+
log.error(`CSRF protection: Origin validation failed`, {
196+
url: req.url,
197+
origin: req.get("Origin"),
198+
referer: req.get("Referer"),
199+
});
200+
res.status(403).send("Invalid request");
201+
return;
202+
}
188203
}
189204

190-
// Validate origin for additional CSRF protection
191-
if (!this.nonceService.validateOrigin(req)) {
192-
log.error(`CSRF protection: Origin validation failed`, {
193-
url: req.url,
194-
origin: req.get("Origin"),
195-
referer: req.get("Referer"),
196-
});
197-
res.status(403).send("Invalid request");
198-
return;
199-
}
200-
201-
// Clear the nonce cookie after successful validation
205+
// Always clear the nonce cookie
202206
this.nonceService.clearNonceCookie(res);
203207

204208
const hostContext = this.hostContextProvider.get(host);
@@ -213,7 +217,7 @@ export class Authenticator {
213217
await hostContext.authProvider.callback(req, res, next);
214218
} catch (error) {
215219
log.error(`Failed to handle callback.`, error, { url: req.url });
216-
// Clear nonce cookie on error as well
220+
// Always clear nonce cookie on error
217221
this.nonceService.clearNonceCookie(res);
218222
}
219223
} else {
@@ -315,7 +319,7 @@ export class Authenticator {
315319
return;
316320
}
317321

318-
// Generate nonce for CSRF protection
322+
// Always generate nonce for CSRF protection (validation controlled by feature flag)
319323
const nonce = this.nonceService.generateNonce();
320324
this.nonceService.setNonceCookie(res, nonce);
321325

@@ -391,8 +395,17 @@ export class Authenticator {
391395
}
392396

393397
// Validate returnTo URL against allowlist for authorize API
394-
if (!validateAuthorizeReturnToUrl(returnToParam, this.config.hostUrl)) {
395-
log.warn(`Invalid returnTo URL rejected for authorize: ${returnToParam}`, { "authorize-flow": true });
398+
const isStrictAuthorizeValidationEnabled = await getFeatureFlagEnableStrictAuthorizeReturnTo();
399+
const isValidReturnTo = isStrictAuthorizeValidationEnabled
400+
? validateAuthorizeReturnToUrl(returnToParam, this.config.hostUrl)
401+
: validateLoginReturnToUrl(returnToParam, this.config.hostUrl);
402+
403+
if (!isValidReturnTo) {
404+
const validationType = isStrictAuthorizeValidationEnabled ? "strict authorize" : "login fallback";
405+
log.warn(`Invalid returnTo URL rejected for authorize (${validationType}): ${returnToParam}`, {
406+
"authorize-flow": true,
407+
strictValidation: isStrictAuthorizeValidationEnabled,
408+
});
396409
res.redirect(this.getSorryUrl(`Invalid return URL.`));
397410
return;
398411
}
@@ -459,7 +472,7 @@ export class Authenticator {
459472
// authorize Gitpod
460473
log.info(`(doAuthorize) wanted scopes (${override ? "overriding" : "merging"}): ${wantedScopes.join(",")}`);
461474

462-
// Generate nonce for CSRF protection
475+
// Always generate nonce for CSRF protection (validation controlled by feature flag)
463476
const nonce = this.nonceService.generateNonce();
464477
this.nonceService.setNonceCookie(res, nonce);
465478

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,58 @@ describe("ReturnTo URL Validation", () => {
8888
expect(result).to.equal(false, `Should reject www.gitpod.io non-root: ${url}`);
8989
});
9090
});
91+
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:
99+
// 1. Nonce is always generated and stored in cookie
100+
// 2. Nonce is always included in JWT state
101+
// 3. Nonce validation only occurs when feature flag is enabled
102+
// 4. Cookie is always cleared after callback processing
103+
104+
expect(true).to.equal(true); // Documentation test
105+
});
106+
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
117+
118+
expect(true).to.equal(true); // Documentation test
119+
});
120+
121+
it("should show difference between strict and fallback validation", () => {
122+
const testUrls = [
123+
{ url: "https://gitpod.io/workspaces", strictAllowed: false, fallbackAllowed: true },
124+
{ url: "https://gitpod.io/settings", strictAllowed: false, fallbackAllowed: true },
125+
{ url: "https://gitpod.io/new", strictAllowed: true, fallbackAllowed: true },
126+
{ url: "https://gitpod.io/quickstart", strictAllowed: true, fallbackAllowed: true },
127+
{
128+
url: "https://gitpod.io/complete-auth?message=success",
129+
strictAllowed: true,
130+
fallbackAllowed: true,
131+
},
132+
];
133+
134+
testUrls.forEach(({ url, strictAllowed, fallbackAllowed }) => {
135+
const strictResult = validateAuthorizeReturnToUrl(url, hostUrl);
136+
const fallbackResult = validateLoginReturnToUrl(url, hostUrl);
137+
138+
expect(strictResult).to.equal(strictAllowed, `Strict validation failed for: ${url}`);
139+
expect(fallbackResult).to.equal(fallbackAllowed, `Fallback validation failed for: ${url}`);
140+
});
141+
});
142+
});
91143
});
92144

93145
describe("validateAuthorizeReturnToUrl", () => {

components/server/src/util/featureflags.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,19 @@ export async function getFeatureFlagEnableContextEnvVarValidation(userId: string
1717
user: { id: userId },
1818
});
1919
}
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+
}

0 commit comments

Comments
 (0)