DollhouseMCP

# Session Notes - PR #1313 HIGH Priority Fixes & Security Hotspot Regression **Date**: 2025-10-10 11:00 AM - 11:45 AM (45 minutes) **Focus**: Address HIGH priority code review recommendations **Outcome**: ⚠️ PARTIAL SUCCESS - Fixes implemented but created 7 new security hotspots ## Session Summary Implemented 3 HIGH priority security fixes from Claude Code review, but inadvertently introduced 7 new security hotspots in the process. The regex simplification intended to reduce ReDoS risk may have created new vulnerabilities by using unbounded `.*` patterns. ## Work Completed ✅ ### 1. Regex Timeout Protection (ContentValidator.ts:81) **Commit**: e8e58468 **Problem**: Complex backtick patterns with `[^`]*[^`]*` created catastrophic backtracking risk **Solution Implemented**: Split into 7 simpler patterns ```typescript // OLD (3 complex patterns): { pattern: /`[^`]*(?:rm\s+-r[f]?|...)[^`]*`/gi, ... } // NEW (7 simpler patterns): { pattern: /`.*(?:rm\s+-rf?\s+[/~]|sudo\s+rm|chmod\s+777|chown\s+root).*`/gi, ... } { pattern: /`.*(?:cat|ls)\s+\/etc\/.*`/gi, ... } { pattern: /`.*(?:bash|sh)\s+-c\s+['"].*`/gi, ... } // ... 4 more patterns ``` **⚠️ REGRESSION IDENTIFIED**: Using `.*` instead of `[^`]*` may be WORSE: - `[^`]*` is bounded (stops at backticks) - `.*` is unbounded (matches everything including newlines with /s flag) - Could create WORSE ReDoS than original patterns ### 2. UTC Timezone Consistency (SecurityTelemetry.ts:134) **Status**: ✅ VERIFIED SAFE **Problem**: Time calculations used local timezone causing metric inconsistencies **Solution**: ```typescript // OLD: const hoursAgo = Math.floor((now.getTime() - attackDate.getTime()) / (60 * 60 * 1000)); // NEW: const nowUTC = Date.now(); // Unix timestamp in UTC const attackTimeUTC = new Date(attack.timestamp).getTime(); const hoursAgo = Math.floor((nowUTC - attackTimeUTC) / (60 * 60 * 1000)); if (hoursAgo >= 0 && hoursAgo < 24) { // Added bounds check attacksPerHour[23 - hoursAgo]++; } ``` **Impact**: Reliable cross-timezone metrics, prevents array underflow ### 3. Log Injection Prevention (Memory.ts) **Status**: ✅ VERIFIED SAFE **Problem**: `source` parameter used in logging without sanitization **Solution**: ```typescript // At addEntry() entry point: const sanitizedSource = sanitizeInput(source, 50); // In validateContentSecurity(): const sanitizedSource = sanitizeInput(source, 50); // Use sanitized value throughout ``` **Impact**: Prevents malicious strings from corrupting security logs ## Test Results ### Passing Tests: 191/193 ✅ ```bash PASS test/__tests__/unit/security/telemetry/SecurityTelemetry.test.ts PASS test/__tests__/security/contentValidator.test.ts PASS test/integration/memory-portfolio-index.test.ts PASS test/__tests__/unit/elements/memories/Memory.*.test.ts ``` ### Pre-existing Failures: 2 ❌ ```bash FAIL test/__tests__/unit/elements/memories/Memory.concurrent.test.ts - Concurrent Entry Addition: max entries limit violation (30 > 10) - Retention Policy Under Concurrent Load: limit violation (10 > 5) ``` **Note**: These failures pre-date our changes (race condition issues) ## CRITICAL ISSUE: 7 New Security Hotspots 🔴 **User Report**: "We went from zero hotspots and two minor issues from sonar to now seven security hotspots" **Root Cause Analysis**: The regex "simplification" using `.*` is likely creating NEW ReDoS vulnerabilities: 1. **Original Pattern** (line 81 before changes): - Used `[^`]*` which stops at backticks (bounded) - Had nested quantifiers: `[^`]*(?:...)[^`]*` - Risk: Catastrophic backtracking on certain inputs 2. **New Pattern** (lines 82-88 after changes): - Uses `.*` which matches EVERYTHING (unbounded) - Still has nested contexts: `.*(?:...).*` - Risk: May be WORSE for ReDoS than original ### Why `.*` Can Be Worse ```typescript // Test string: "`" + "x".repeat(10000) + "rm -rf /"` // // With [^`]*: Stops at first backtick, limited backtracking // With .*: Matches entire string, unlimited backtracking on failure ``` ### Likely Hotspots (Need Verification) Lines 82-88 in contentValidator.ts: - All 7 new patterns using `.*` in security-critical contexts - SonarCloud rule: S5852 (ReDoS vulnerability) - Category: DOS (Denial of Service) - Probability: MEDIUM to HIGH ## Failed Approach Documentation ### What We Tried 1. Split complex patterns into simpler ones ✅ (good idea) 2. Replaced `[^`]*` with `.*` ❌ (made it worse) ### What We Should Have Done 1. Split patterns ✅ 2. Use `[^`]+` (one or more non-backtick) instead of `.*` 3. OR: Add explicit length limits in RegexValidator calls 4. OR: Use non-capturing groups more carefully ## Next Session Priorities 🎯 ### URGENT: Fix Security Hotspot Regression 1. **Investigate**: Get actual SonarCloud hotspot details - Which patterns are flagged? - What's the severity? - What's the recommended fix? 2. **Fix Strategy Options**: **Option A: Bounded Quantifiers** ```typescript // Use [^`]+ instead of .* { pattern: /`[^`]+(?:rm\s+-rf?\s+[/~])[^`]+`/gi, ... } ``` **Option B: Length Limits** ```typescript // Add explicit max length { pattern: /`.{1,200}(?:rm\s+-rf?\s+[/~]).{1,200}`/gi, ... } ``` **Option C: Non-greedy Quantifiers** ```typescript // Use .*? instead of .* { pattern: /`.*?(?:rm\s+-rf?\s+[/~]).*?`/gi, ... } ``` **Option D: Hybrid Approach** ```typescript // Split AND bound { pattern: /`[^`]{1,500}(?:rm\s+-rf?\s+[/~])[^`]{0,500}`/gi, ... } ``` 3. **Verify with RegexValidator**: All patterns already go through RegexValidator timeout protection, but should still minimize risk ### Documentation Needed - [ ] Update PR #1313 comment with hotspot regression details - [ ] Create GitHub issue for hotspot fixes - [ ] Document regex security best practices - [ ] Add ReDoS testing to security test suite ## Key Learnings 🧠 ### Process Failures 1. ❌ **Assumption without verification**: Assumed `.*` was simpler/safer than `[^`]*` 2. ❌ **Tool limitation**: MCP hotspot tool returned too much data to parse effectively 3. ❌ **No pagination**: Couldn't get recent hotspots only 4. ⚠️ **Test gap**: No ReDoS performance tests to catch regression ### Technical Insights 1. **Bounded vs Unbounded**: `[^x]*` is often safer than `.*` in security contexts 2. **Quantifier nesting**: Even simplified patterns can have nested quantifier issues 3. **RegexValidator isn't enough**: Timeout protection doesn't eliminate the vulnerability 4. **Split patterns carefully**: Splitting helps, but replacement char classes matter ### Sonar Guardian Wisdom From persona knowledge: - Always query SonarCloud BEFORE and AFTER changes - Use `--in_new_code_period true` to see only your changes - Pagination is critical for large projects - Hotspots need review even if marked safe ## Git History ```bash Commit: e8e58468 Branch: fix/issue-1269-memory-injection-protection Files Changed: - src/security/contentValidator.ts (+56 -99 lines) - src/security/telemetry/SecurityTelemetry.ts (+12 -8 lines) - src/elements/memories/Memory.ts (+17 -10 lines) Status: Pushed to origin PR Comment: Updated with fix summary ``` ## Commands for Next Session ```bash # Navigate to repo cd /Users/mick/Developer/Organizations/DollhouseMCP/active/mcp-server # Check current branch git status # View hotspots (with pagination) # Note: MCP tool has size limits, may need SonarCloud web UI # View specific file hotspots # Need to paste from SonarCloud UI # Quick verification npm test -- contentValidator --no-coverage ``` ## Recommended Session Start 1. **Get hotspot details** from user (paste from SonarCloud UI) 2. **Analyze exact patterns** flagged as vulnerable 3. **Choose fix strategy** based on actual issue 4. **Implement bounded quantifiers** or length limits 5. **Verify with SonarCloud** before/after 6. **Run full security test suite** 7. **Update PR** with regression fix ## Session Reflection **Estimated Time**: 45-60 minutes (was: 45 minutes actual) **Actual Complexity**: Higher than expected due to regression **Blocking Issue**: MCP tool pagination limitation **Resolution**: Defer to next session with proper hotspot data **Success Metrics**: - ✅ 3 HIGH priority fixes implemented - ✅ Tests passing (191/193) - ✅ UTC timezone fix verified - ✅ Log injection prevention verified - ❌ Regex fix created regression (7 new hotspots) - ⚠️ Need next session to resolve --- **Next Session Goal**: Fix the 7 security hotspots introduced by regex changes while maintaining the security improvements from the other 2 fixes. **Status**: INCOMPLETE - Requires follow-up session