DollhouseMCP

# Session Notes - October 10, 2025 (Late Morning) **Date**: October 10, 2025 **Time**: 11:15 AM - 11:45 AM (30 minutes) **Focus**: ReDoS Vulnerability Fixes and Memory Race Condition (PR #1313) **Outcome**: ✅ SUCCESS - Fixed 7 critical security hotspots and race condition --- ## Session Summary Successfully fixed 7 critical ReDoS (Regular Expression Denial of Service) vulnerabilities in `contentValidator.ts` identified by SonarCloud, plus resolved a critical race condition in `Memory.ts` that was causing concurrent test failures. All 131 test suites now pass (2311 tests). --- ## Issues Fixed ### 1. Seven ReDoS Vulnerabilities (CRITICAL) **Location**: `src/security/contentValidator.ts` lines 83-89 **Problem**: - Backtick command detection patterns used greedy quantifiers `.*` that cause catastrophic backtracking - Example vulnerable pattern: `/`.*(?:rm\s+-rf?\s+[/~]).*`/gi` - On malicious input like `` `aaa...aaa` `` (without matching command), regex engine tries exponential combinations - Time complexity: O(2^n) - could cause denial of service **Solution**: - Replaced `.*` with `[^`]*` (matches non-backtick characters) - Example fixed pattern: `/`[^`]*(?:rm\s+-rf?\s+[/~])[^`]*`/gi` - Semantically correct: matches content WITHIN backticks - Time complexity: O(n) - linear performance **Patterns Fixed**: 1. Line 83: Dangerous shell commands (`rm -rf`, `sudo rm`, `chmod 777`, `chown root`) 2. Line 84: Sensitive file access (`cat /etc/`, `ls /etc/`) 3. Line 85: Shell execution (`bash -c`, `sh -c`) 4. Line 86: Dangerous commands (`passwd`, `shadow`, `nc -l`, `ssh root@`) 5. Line 87: Pipe to shell (`curl | bash`, `wget | sh`) 6. Line 88: Sensitive file/privilege escalation (`/etc/passwd`, `sudo su`) 7. Line 89: Script interpreter with dangerous functions (`python -c exec`, `node -e eval`) **Additional Fix** (Line 85): - Fixed duplicate character class: `['"']` → `['"]` - Addresses SonarCloud warning S5869 --- ### 2. Memory Race Condition (CRITICAL) **Location**: `src/elements/memories/Memory.ts` **Problem**: - Test `Memory.concurrent.test.ts` failed: expected ≤10 entries, got 30 - Race condition in `addEntry()` method - Original flow: Check capacity → await validation → add entry - Multiple concurrent calls could all pass capacity check before any added entries - Result: maxEntries limit violated under high concurrency **Solution**: - **Before**: `ensureCapacity()` checked/enforced BEFORE adding entry (async gap) - **After**: `enforceCapacitySync()` enforces AFTER adding entry (atomic operation) - Made enforcement synchronous to prevent interleaving - Added cleanup of search index when removing entries **Code Changes**: ```typescript // OLD (vulnerable to race): await this.ensureCapacity(); // Check before this.entries.set(entry.id, entry); // Add entry // NEW (race-proof): this.entries.set(entry.id, entry); // Add entry first this.enforceCapacitySync(); // Enforce after (synchronous) ``` **New Method**: `enforceCapacitySync()` - Synchronous enforcement (no await points) - Removes excess entries if over limit - Also cleans up search index for removed entries --- ### 3. Test Expectation Updates **Location**: `test/__tests__/security/backtick-validation.test.ts` **Problem**: - Tests checked for outdated pattern description names - Example: Expected "Malicious backtick command", actual is "Dangerous shell command in backticks" **Solution**: - Updated all test assertions to match current pattern descriptions from `contentValidator.ts` - Tests now check for actual descriptions like: - "Dangerous shell command in backticks" - "Sensitive file access in backticks" - "Shell execution in backticks" - "Pipe to shell in backticks" - etc. --- ## Test Results ### Before Fixes - **Memory.concurrent.test.ts**: 2 failures - `should maintain max entries limit under concurrent load`: Expected ≤10, got 30 - `should enforce retention correctly with concurrent operations`: Expected ≤5, got 10 - **backtick-validation.test.ts**: 3 failures (wrong pattern names) ### After Fixes ✅ **All tests pass**: 131 test suites, 2311 tests - **Memory.concurrent.test.ts**: 10/10 ✓ - **backtick-validation.test.ts**: 10/10 ✓ - **contentValidator.test.ts**: 23/23 ✓ --- ## Technical Details ### ReDoS Vulnerability Analysis **Why `.*` is dangerous**: ```javascript // Vulnerable pattern /`.*dangerous-cmd.*`/ // Input: `aaaaaaaaaaaaa` (no match) // Engine tries: // `.*` matches entire string → backtrack // `.*` matches n-1 chars → backtrack // `.*` matches n-2 chars → backtrack // ... (2^n combinations) ``` **Why `[^`]*` is safe**: ```javascript // Safe pattern /`[^`]*dangerous-cmd[^`]*`/ // Input: `aaaaaaaaaaaaa` (no match) // Engine tries: // `[^`]*` matches to end → fails → done (O(n)) ``` **Character class specificity** prevents backtracking: - `.*` = "any character, any amount" (ambiguous) - `[^`]*` = "non-backtick characters, any amount" (specific) ### Race Condition Analysis **Concurrent execution timeline**: ``` Before fix (BROKEN): Thread 1: Check (9 < 10) ✓ → await → Add → Size = 10 Thread 2: Check (9 < 10) ✓ → await → Add → Size = 11 Thread 3: Check (9 < 10) ✓ → await → Add → Size = 12 ... (30 threads all pass check) After fix (WORKING): Thread 1: Add → Size = 10 → Enforce (remove 0) → Size = 10 Thread 2: Add → Size = 11 → Enforce (remove 1) → Size = 10 Thread 3: Add → Size = 11 → Enforce (remove 1) → Size = 10 ... (limit always enforced) ``` --- ## Files Changed 1. **src/security/contentValidator.ts** - Lines 80-89: Updated 7 backtick patterns - Changed `.*` → `[^`]*` in all patterns - Fixed character class duplicate on line 85 - Updated comments to document the fix 2. **src/elements/memories/Memory.ts** - Removed `ensureCapacity()` method (async, vulnerable) - Added `enforceCapacitySync()` method (sync, race-proof) - Modified `addEntry()` to enforce capacity AFTER adding - Added search index cleanup in enforcement 3. **test/__tests__/security/backtick-validation.test.ts** - Updated test expectations in 3 test cases - Now checks for actual pattern descriptions - Added PR #1313 comments explaining changes --- ## Commit Details **Commit**: `5a6de3b5` **Branch**: `fix/issue-1269-memory-injection-protection` **PR**: #1313 **Commit Message**: ``` fix(security): Fix 7 ReDoS vulnerabilities and Memory race condition (PR #1313) FIXES: 1. CRITICAL: Fixed 7 ReDoS vulnerabilities in backtick command patterns 2. HIGH: Fixed duplicate character class in line 85 3. CRITICAL: Fixed Memory race condition causing capacity limit violations 4. Updated backtick-validation.test.ts to match actual pattern descriptions TEST RESULTS: - All 131 test suites pass (2311 tests) - Memory.concurrent.test.ts: 10/10 tests pass - backtick-validation.test.ts: 10/10 tests pass - contentValidator.test.ts: 23/23 tests pass ``` --- ## Key Learnings ### 1. ReDoS Prevention - Always use specific character classes (`[^x]`) instead of wildcards (`.*`) - Test regex patterns with adversarial inputs - Prefer atomic operations over backtracking ### 2. Concurrency Patterns - Check-then-act patterns are vulnerable to race conditions - Enforce invariants AFTER modifications, not before - Make critical sections synchronous when possible ### 3. Test-Driven Debugging - Test failures provided clear reproduction steps - Unit tests caught the race condition that might have been missed in production - Good test coverage pays off --- ## Next Steps 1. Wait for SonarCloud to re-analyze PR #1313 2. Verify all 7 security hotspots are resolved 3. Address any remaining issues in the PR 4. Get PR approved and merged --- ## Performance Impact ### ReDoS Fixes - **Before**: O(2^n) worst-case (exponential) - **After**: O(n) worst-case (linear) - **Real-world**: No performance degradation, maintains security detection ### Memory Enforcement - **Before**: Async check (race-prone, sometimes over limit) - **After**: Sync enforcement (race-proof, guaranteed limit) - **Real-world**: Minimal overhead, better correctness --- ## Session Efficiency **Total Time**: 30 minutes **Issues Fixed**: 10 (7 ReDoS + 1 race condition + 2 test failures) **Tests Fixed**: 5 failing tests → all passing **Commits**: 1 comprehensive commit with detailed documentation **Success Factors**: - Clear problem identification from SonarCloud screenshot - Debug script to verify pattern detection - Systematic approach to each issue - Comprehensive testing before commit --- *Session completed successfully. All test suites passing, ready for SonarCloud review.*