DollhouseMCP

# Session Notes - Todd Dibble Audit Verification and Relabeling **Date**: October 9, 2025 **Time**: Morning Session **Focus**: Verify unconfirmed audit findings, properly categorize issues based on local threat model **Outcome**: ✅ Complete - All 18 findings verified and properly categorized --- ## Session Summary Completed comprehensive verification of Todd Dibble's audit findings. Successfully investigated all 5 previously unverified issues, created GitHub issues #1300-1304, and fundamentally reframed the audit response to reflect DollhouseMCP's actual threat model as a local-only MCP server. **Key Insight**: Only 2 out of 18 findings are actual security issues in the local context. The rest are reliability, performance, architecture, or code quality concerns. --- ## Work Completed ### 1. Verified All 5 Outstanding Issues ✅ Activated Debug Detective, Security Analyst, and Audio Summarizer personas for systematic investigation: #### Issue #12 → GitHub #1301 (Random Sample Algorithm) - **Finding**: CONFIRMED at `EnhancedIndexManager.ts:2168-2171` - **Problem**: Uses `.sort(() => 0.5 - Math.random())` which is O(n log n) - **Should Use**: Fisher-Yates shuffle for O(k) performance - **Impact**: ~1000x slower for large arrays - **Category**: Performance optimization (NOT security) #### Issue #13 → GitHub #1302 (Collection Search) - **Finding**: CONFIRMED at `CollectionSearch.ts:345-365` - **Problem**: O(n) linear filtering, no inverted index - **Mitigation**: Heavy caching makes this acceptable currently - **Category**: Performance/scalability (NOT security) #### Issue #15 → GitHub #1303 (Lock Queue Unbounded) - **Finding**: PARTIALLY CONFIRMED at `fileLockManager.ts:19` - **Problem**: `Map<string, Promise<any>>` has no max depth - **Protection**: 10-second timeout + automatic cleanup - **Risk**: Burst traffic within timeout window - **Category**: Reliability (NOT security in local context) #### Issue #17 → GitHub #1300 (Circular Dependencies) - **Finding**: CRITICALLY CONFIRMED - **Locations**: - `EnhancedIndexManager.ts:30, 233` - `VerbTriggerManager.ts:16-17, 115` - **Problem**: Runtime circular dependency via singleton `getInstance()` calls - **Impact**: Initialization deadlocks, partially initialized singletons - **Category**: CRITICAL architectural flaw (reliability, not security) #### Issue #18 → GitHub #1304 (Observer Pattern) - **Finding**: CONFIRMED as feature request - **Type**: Enhancement, not a bug - **Use Cases**: Audit logging, analytics, plugin system - **Category**: Feature request (NOT security) --- ### 2. Threat Model Analysis 🎯 **Critical Question Raised**: "Who's going to be attacking this? It runs locally on someone's machine." **Threat Model for DollhouseMCP**: - ✅ Local-only execution - ✅ Single-user system (user has full filesystem access) - ✅ Not internet-accessible - ✅ No remote attackers **Primary Security Risk**: Malicious community content (personas, skills, templates) - Path traversal to read sensitive files - YAML bombs for local DoS - Content injection attacks **NOT Security Risks in Local Context**: - Memory leaks → User's own bug - Unbounded caches → User's own resource exhaustion - "DoS" issues → User DoS'ing themselves (reliability issue) - Token encryption → If attacker has filesystem access, they already have everything --- ### 3. Issues Properly Relabeled **GitHub Label Updates**: - ✅ Removed `area: security` from #1300 (circular dependencies) - ✅ Removed `area: security` from #1303 (lock queue) - ✅ Updated #1300 title to remove misleading "CRITICAL:" prefix **Document Renamed**: - ✅ `TODD_DIBBLE_SECURITY_AUDIT_RESPONSE.md` → `TODD_DIBBLE_AUDIT_RESPONSE.md` --- ### 4. Document Reorganized with Proper Categories **New Structure**: #### Actual Security Issues (2 out of 18) 1. **#1290** - Path traversal via symlinks (CRITICAL) 2. **#1298** - YAML bomb detection threshold (MEDIUM) #### Reliability & Resource Management (3) 3. **#1291** - Memory leak (setInterval cleanup) 4. **#1292** - APICache unbounded growth 5. **#1303** - Lock queue unbounded depth #### Defense-in-Depth (1) 6. **#1293** - Token encryption hardening (nice-to-have) #### Performance (2) 7. **#1301** - Random sample algorithm O(n log n) 8. **#1302** - Collection search inverted index #### Architecture & Code Quality (6) 9. **#1300** - Circular dependencies (CRITICAL architectural) 10. **#1294** - Extract BaseElementManager 11. **#1295** - Reduce singleton usage 12. **#1296** - Replace 'as any' assertions (89x) 13. **#1297** - Replace Record<string, any> (53x) 14. **#1299** - Refactor EnhancedIndexManager #### Feature Request (1) 15. **#1304** - Observer pattern lifecycle events #### Already Tracked (1) 16. **#881/#512** - index.ts refactoring (6,028 lines) #### False Positives (2) 17. Git clone command injection - already protected 18. Backtick command patterns - working as designed --- ## Key Learnings ### 1. Threat Modeling Matters Traditional "security audit" thinking doesn't apply to local-only systems. Context is everything: - Web service: "DoS" = security vulnerability - Local tool: "DoS" = resource management bug ### 2. Proper Categorization Labels and categories should reflect actual risk, not just surface appearance: - Something in `security/` folder ≠ security issue - Circular dependency in production code = reliability, not security - Memory leak = quality/reliability, not security (unless targeted attack vector) ### 3. Malicious Content is the Real Attack Vector For DollhouseMCP, the primary security concern is **supply chain security**: - Community collection personas/skills/templates - Path traversal in content validation - Code injection via YAML/template processing - DoS via resource exhaustion (YAML bombs) --- ## GitHub Issues Created **Total Created**: 5 new issues (#1300-1304) - All properly categorized and labeled - All credit Todd Dibble - All include code locations, recommendations, and estimated fix times **Complete Audit Status**: - 15 confirmed issues - 2 false positives - 1 already tracked - **0 need verification** (all verified) --- ## Next Session Priorities ### CRITICAL (Address Immediately) #### 1. Fix #1290 - Path Traversal via Symlinks **File**: `src/security/pathValidator.ts:35-62` **Problem**: Uses `path.resolve()` which doesn't follow symlinks **Threat**: Malicious persona could bypass path restrictions and read sensitive files **Fix**: Use `fs.realpath()` to resolve symlinks before validation **Estimated Time**: 10 minutes **Priority**: CRITICAL SECURITY - **Do this first** ```typescript // Current (vulnerable): const resolved = path.resolve(basePath, filePath); // Fixed: const resolved = await fs.promises.realpath(path.resolve(basePath, filePath)); // Then validate resolved path is within allowed boundaries ``` #### 2. Fix #1298 - YAML Bomb Detection Threshold **File**: `src/security/contentValidator.ts:297` **Problem**: 10:1 amplification ratio too permissive **Threat**: Malicious element could cause local DoS **Fix**: Change threshold from 10:1 to 5:1 **Estimated Time**: 5 minutes **Priority**: MEDIUM SECURITY - **Quick win** ```typescript // Current: const YAML_BOMB_THRESHOLD = 10; // Fixed: const YAML_BOMB_THRESHOLD = 5; ``` ### HIGH (Next Sprint) 3. **#1300** - Circular dependency architectural fix (4-8 hours) 4. **#1291** - Memory leak cleanup (15 minutes) 5. **#1292** - Cache size limits (30 minutes) ### MEDIUM (Backlog) 6. **#1301** - Fisher-Yates shuffle (15 minutes - easy win) 7. **#1303** - Lock queue depth limit (1-2 hours) 8. **#1302** - Inverted index (4-6 hours) 9. **#1293-1299** - Code quality improvements --- ## Files Modified ### Created - `docs/development/SESSION_NOTES_2025-10-09-TODD-DIBBLE-AUDIT-VERIFICATION.md` ### Renamed - `TODD_DIBBLE_SECURITY_AUDIT_RESPONSE.md` → `TODD_DIBBLE_AUDIT_RESPONSE.md` ### Updated - `TODD_DIBBLE_AUDIT_RESPONSE.md` - Complete reorganization with threat model context ### GitHub Issues - Created: #1300, #1301, #1302, #1303, #1304 - Updated labels: #1300, #1303 --- ## Investigation Methodology **Tools Used**: - Debug Detective persona for systematic investigation - Security Analyst persona for threat assessment - Audio Summarizer skill for quick status updates - Grep/Read tools for code investigation - GitHub CLI for issue management **Process**: 1. Search for code patterns (`.sort()`, `Math.random()`, `getInstance()`) 2. Read relevant files to understand context 3. Verify actual behavior vs Todd's concerns 4. Categorize based on threat model 5. Create properly labeled GitHub issues 6. Update documentation --- ## Statistics **Time Spent**: ~2 hours **Issues Investigated**: 5 **Issues Created**: 5 **Labels Updated**: 2 **Documents Updated**: 1 **Documents Renamed**: 1 **Code Locations Verified**: 12+ **Grep Searches**: 15+ **File Reads**: 10+ --- ## Quotes & Insights > "Are they actual security issues or are they more like design issues or inefficiencies?" > - Mick, challenging the "security audit" framing > "Since this thing runs locally on somebody's machine, who is going to be trying to break the security?" > - Mick, identifying the actual threat model **Key Realization**: Todd's audit was labeled "security audit" but was really a comprehensive code quality review. Only 2/18 findings were actual security issues in the local-only threat model. --- ## Recommendations for Future Audits 1. **Define Threat Model First** - Who are the attackers? - What are they trying to accomplish? - What access do they have? 2. **Categorize by Actual Impact** - Security = protects against adversarial exploitation - Reliability = prevents system failures - Performance = improves user experience - Quality = maintainability and correctness 3. **Context Matters** - Local vs hosted - Single-user vs multi-tenant - Trusted vs untrusted content - Internet-accessible vs isolated --- **Session End**: October 9, 2025 **Status**: ✅ Complete - All findings verified, properly categorized, and documented **Next Action**: Fix #1290 (path traversal) and #1298 (YAML bomb threshold) in next session