Skip to content

Remove superfluous security feature flags and always enable protections #21007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

geropl
Copy link
Member

@geropl geropl commented Aug 13, 2025

Description

This PR removes three superfluous security feature flags and ensures the underlying security protections are permanently enabled:

  • context_env_var_validation - Environment variable validation now always enabled
  • enable_nonce_validation - CSRF protection with nonce validation now always enabled
  • enable_strict_authorize_return_to - Strict OAuth returnTo validation now always enabled

Related Issue(s)

Fixes CLC-1618

Security Impact

These changes enhance security by ensuring critical protections cannot be accidentally disabled:

  1. Environment Variable Injection Protection (CLC-1591) - Always validates context env vars to prevent code execution attacks
  2. CSRF Protection (CLC-1592, CLC-1643, CLC-1642) - Always validates nonces in OAuth flows to prevent unauthorized requests
  3. OAuth Redirect Validation - Always validates returnTo URLs to prevent redirect attacks

Changes Made

Code Simplification

  • Removed feature flag functions from featureflags.ts
  • Removed conditional security checks in authenticator.ts and user-controller.ts
  • Updated comments to reflect permanent security measures

Test Updates

  • Removed feature flag mocking from test files
  • Updated test descriptions to reflect always-enabled security
  • Consolidated security validation tests

Testing

  • ✅ All relevant unit tests pass (28/28 for affected components)
  • ✅ Environment variable validation tests pass
  • ✅ OAuth returnTo validation tests pass
  • ✅ Code formatting and linting applied

Benefits

  • Enhanced Security: Critical security features permanently active
  • Reduced Attack Surface: No way to accidentally disable protections
  • Simplified Codebase: Removed conditional security logic (-159 lines, +46 lines)
  • Consistent Protection: All users benefit from security measures

How to test

  1. Environment Variable Validation:

    • Create workspace with dangerous env vars (e.g., BASH_ENV=$(curl|sh)) - should be blocked
    • Create workspace with safe env vars (e.g., VERSION=1.2.3) - should work
  2. OAuth Flows:

    • Login/logout with different SCM providers - should work with CSRF protection
    • Bind additional SCM providers - should work with nonce validation
    • Test returnTo URL validation on authorize endpoint - should enforce strict patterns

Documentation

No documentation changes needed - these were internal feature flags not exposed to users.

- 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]>
Remove unused Experiments import that was causing TypeScript compilation error.

Co-authored-by: Ona <[email protected]>
@geropl
Copy link
Member Author

geropl commented Aug 14, 2025

Reviewed the code myself and it does LGTM ✔️
Let's wait a couple for days (next Wednesday) before dropping, no rush here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants