Skip to content

Commit 59eb3f1

Browse files
geroplona-agent
andcommitted
refactor: consolidate envvar security validation and use feature flags
Refactors the environment variable injection security fix based on feedback: 1. Consolidates EnvVarSecurity into EnvvarSanitization namespace within envvar-prefix-context-parser.ts instead of standalone module 2. Replaces config-based feature flag with workspace feature flag 'envvar_context_validation' in user.featureFlags.permanentWSFeatureFlags 3. Consolidates all tests into single test file with two test suites: - TestEnvvarPrefixParser (integration tests) - TestEnvvarSanitization (unit tests) Changes: - Added 'envvar_context_validation' to WorkspaceFeatureFlags in protocol.ts - Moved security validation logic into EnvvarSanitization namespace - Updated parser to check user feature flags instead of config - Removed standalone envvar-security.ts and envvar-security.spec.ts files - Removed envvarContextValidation from config.ts - Updated design document to reflect refactored architecture The security functionality remains identical - three-layer validation with backward compatibility when feature flag is disabled. Co-authored-by: Ona <[email protected]>
1 parent 97d2fe3 commit 59eb3f1

File tree

7 files changed

+616
-616
lines changed

7 files changed

+616
-616
lines changed

DESIGN_ENV_VAR_SECURITY.md

Lines changed: 25 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
**Issue**: CLC-1591 - Environment Variable Injection Leads to Private Repo Content Exfiltration
44
**CVSS**: 8.0 (High) - Cross-origin attackers can inject environment variables leading to command execution
55
****Date**: 2025-01-22
6-
**Status**: Phase 1 Implementation Complete
6+
**Status**: Phase 1 Refactored - Feature Flag Implementation
77

88
## Problem Statement
99

@@ -384,23 +384,28 @@ Verify legitimate use cases continue to work:
384384

385385
## Implementation Progress
386386

387-
### Phase 1: Core Implementation ✅ COMPLETE
388-
**Status**: Implemented and ready for testing
389-
**Files Created/Modified**:
390-
-`components/server/src/workspace/envvar-security.ts` - Core security validation module
391-
-`components/server/src/config.ts` - Added feature flag configuration
392-
-`components/server/src/workspace/envvar-prefix-context-parser.ts` - Integrated security validation
393-
-`components/server/src/workspace/envvar-security.spec.ts` - Comprehensive unit tests
394-
-`components/server/src/workspace/envvar-prefix-context-parser.spec.ts` - Enhanced integration tests
387+
### Phase 1: Core Implementation ✅ COMPLETE (Refactored)
388+
**Status**: Refactored to use workspace feature flags and consolidated architecture
389+
**Files Modified**:
390+
-`components/gitpod-protocol/src/protocol.ts` - Added `envvar_context_validation` feature flag
391+
-`components/server/src/workspace/envvar-prefix-context-parser.ts` - Integrated `EnvvarSanitization` namespace
392+
-`components/server/src/workspace/envvar-prefix-context-parser.spec.ts` - Consolidated all tests
393+
-`components/server/src/config.ts` - Removed config-based feature flag
394+
395+
**Refactoring Changes**:
396+
- **Consolidated Architecture**: Moved security validation into `EnvvarSanitization` namespace within the main parser file
397+
- **Feature Flag Implementation**: Replaced config-based flag with workspace feature flag `envvar_context_validation`
398+
- **Test Consolidation**: Combined all tests into single test file with two test suites
399+
- **Simplified Dependencies**: Removed standalone security module and config dependency
395400

396401
**Implementation Details**:
397-
- **Three-layer security validation** implemented as designed:
402+
- **Three-layer security validation** implemented in `EnvvarSanitization` namespace:
398403
1. Variable name blacklist (auto-executing variables)
399404
2. Character whitelist for values `[A-Za-z0-9_\-\.?=]`
400405
3. Injection pattern detection (command substitution, chaining, etc.)
401-
- **Feature flag support** with backward compatibility (disabled by default)
406+
- **Workspace feature flag**: `envvar_context_validation` in user's `permanentWSFeatureFlags`
407+
- **Backward compatibility**: Feature disabled by default, only enabled when user has the feature flag
402408
- **Comprehensive logging** for blocked variables with reason codes
403-
- **Dependency injection** properly configured with Config service
404409
- **URL decoding** handled before validation to prevent bypass attempts
405410

406411
**Security Coverage**:
@@ -412,19 +417,20 @@ Verify legitimate use cases continue to work:
412417
- ✅ Command injection patterns detected: `$(...)`, `|cmd`, `;cmd`, `&&cmd`
413418

414419
**Testing Coverage**:
415-
-Unit tests for all security validation layers
416-
-Integration tests with feature flag scenarios
420+
-`TestEnvvarPrefixParser` suite: Integration tests with feature flag scenarios
421+
-`TestEnvvarSanitization` suite: Unit tests for all security validation layers
417422
- ✅ Attack vector validation tests
418423
- ✅ Legitimate use case preservation tests
419424
- ✅ URL encoding/decoding edge cases
425+
- ✅ Feature flag enabled/disabled behavior tests
420426

421427
### Next Steps: Phase 2 - Testing & Validation
422428
**Planned Activities**:
423429
1. Build and run test suite in proper environment
424430
2. Validate all attack vectors are blocked
425431
3. Confirm legitimate use cases work
426432
4. Performance testing of validation overhead
427-
5. Prepare for gradual rollout
433+
5. Configure feature flag rollout strategy
428434

429435
## Security Review
430436

@@ -438,7 +444,9 @@ The solution is conservative by design, prioritizing security over convenience f
438444

439445
**Phase 1 Implementation Validation**:
440446
- ✅ All security layers implemented as specified
441-
-Feature flag system properly integrated
442-
- ✅ Comprehensive test coverage created
447+
-Workspace feature flag system properly integrated
448+
- ✅ Comprehensive test coverage created and consolidated
443449
- ✅ Backward compatibility maintained
444450
- ✅ Logging and monitoring integrated
451+
- ✅ Architecture simplified and consolidated
452+
- ✅ Dependencies reduced (no standalone security module)

components/gitpod-protocol/src/protocol.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,7 @@ export const WorkspaceFeatureFlags = {
224224
workspace_connection_limiting: undefined,
225225
workspace_psi: undefined,
226226
ssh_ca: undefined,
227+
envvar_context_validation: undefined,
227228
};
228229
export type NamedWorkspaceFeatureFlag = keyof typeof WorkspaceFeatureFlags;
229230
export namespace NamedWorkspaceFeatureFlag {

components/server/src/config.ts

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -274,14 +274,6 @@ export interface ConfigSerialized {
274274

275275
/** true if this is a Dedicated */
276276
isDedicatedInstallation: boolean;
277-
278-
/**
279-
* Environment variable context validation configuration
280-
* Controls security validation of environment variables set via context URLs
281-
*/
282-
envvarContextValidation?: {
283-
enabled: boolean;
284-
};
285277
}
286278

287279
export interface CookieConfig {

0 commit comments

Comments
 (0)