DollhouseMCP

# Todd Dibble Audit Response **Date**: October 9, 2025 **Audit Performed By**: Todd Dibble **Audit Type**: Comprehensive code quality audit (security, reliability, architecture, performance) **Total Findings**: 18 issues identified **Response Summary**: 15 confirmed (issues created), 2 false positives, 1 already tracked, 0 need verification (all verified) ## Threat Model Context DollhouseMCP is a **local-only MCP server** that runs on the user's own machine. The threat model differs significantly from hosted web services: - ✅ **Single-user system** - User has full filesystem access already - ✅ **Not internet-accessible** - No remote attackers - ✅ **Local execution only** - No multi-tenancy concerns **Primary Security Risk**: Malicious community content (personas, skills, templates) could exploit vulnerabilities via: - Path traversal to read sensitive files - YAML bombs for local DoS - Other content-injection attacks **Not Security Risks in Local Context**: Memory leaks, unbounded caches, and "DoS" issues that only affect the user's own machine are **reliability/quality issues**, not security vulnerabilities. --- ## Issues Created from Audit ### Actual Security Issues (Protect Against Malicious Content) #### #1290 - Path traversal via symlinks in pathValidator - **Severity**: CRITICAL SECURITY - **Threat**: Malicious community content could read sensitive files - **Location**: `src/security/pathValidator.ts:35-62` - **Issue**: Uses `path.resolve()` which doesn't follow symlinks, allowing bypass - **Fix Time**: 10 minutes - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1290 #### #1298 - Tighten YAML bomb detection threshold - **Severity**: MEDIUM SECURITY - **Threat**: Malicious elements could cause local DoS - **Location**: `src/security/contentValidator.ts:297` - **Issue**: Current 10:1 amplification ratio could be more conservative (recommend 5:1) - **Fix Time**: 5 minutes - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1298 --- ### Reliability & Resource Management Issues #### #1291 - Memory leak in NLPScoringManager from uncleaned setInterval - **Severity**: HIGH (Reliability) - **Impact**: ~60KB/hour memory leak per instance, eventual crashes - **Location**: `src/portfolio/NLPScoringManager.ts:86` - **Issue**: `setInterval` with no cleanup method - **Fix Time**: 15 minutes - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1291 #### #1292 - APICache has unbounded growth within TTL window - **Severity**: MEDIUM (Reliability) - **Impact**: User's own operations could exhaust memory - **Location**: `src/cache/APICache.ts` - **Issue**: No size limits, only TTL expiry - **Fix Time**: 30 minutes - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1292 #### #1303 - Add max queue depth limit to FileLockManager - **Severity**: MEDIUM (Reliability) - **Impact**: Burst traffic could exhaust user's own memory - **Location**: `src/security/fileLockManager.ts:19` - **Issue**: `Map<string, Promise<any>>` has no maximum depth limit - **Current Protections**: 10-second timeout per lock + automatic cleanup - **Fix Time**: 1-2 hours - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1303 --- ### Defense-in-Depth Improvements #### #1293 - Enhance token encryption passphrase with hardware-specific entropy - **Severity**: LOW (Defense-in-Depth) - **Location**: `src/security/tokenManager.ts:409-416` - **Issue**: Could add hardware entropy (CPU, MAC address) for additional protection - **Current Status**: Already uses SHA256, not trivially predictable - **Note**: In local context, filesystem access already compromises all secrets - **Fix Time**: 1 hour - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1293 --- ### Performance Optimization Issues #### #1301 - Replace inefficient O(n log n) random sample with Fisher-Yates shuffle - **Severity**: MEDIUM (Performance) - **Location**: `src/portfolio/EnhancedIndexManager.ts:2168-2171` - **Issue**: Uses `.sort(() => 0.5 - Math.random())` which is O(n log n) instead of O(k) - **Impact**: ~1000x slower than optimal for large arrays - **Fix Time**: 15 minutes - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1301 #### #1302 - Optimize collection search with inverted index - **Severity**: MEDIUM (Performance) - **Location**: `src/collection/CollectionSearch.ts:345-365` - **Issue**: O(n) linear search, could use inverted index for O(1) lookups - **Current Mitigations**: Heavy caching makes this acceptable for now - **Fix Time**: 4-6 hours - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1302 --- ### Architecture & Code Quality Issues #### #1300 - Runtime circular dependency between EnhancedIndexManager and VerbTriggerManager - **Severity**: CRITICAL (Architectural Reliability) - **Location**: `src/portfolio/EnhancedIndexManager.ts:30, 233` and `src/portfolio/VerbTriggerManager.ts:16-17, 115` - **Issue**: Runtime initialization circular dependency via singleton `getInstance()` calls - **Impact**: Can cause initialization deadlocks, partially initialized singletons, unpredictable behavior - **Fix Time**: 4-8 hours - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1300 #### #1294 - Extract BaseElementManager to eliminate code duplication - **Severity**: MEDIUM (Maintainability) - **Location**: `src/elements/skills/SkillManager.ts` (544 lines), `src/elements/templates/TemplateManager.ts` (666 lines) - **Issue**: ~70% code similarity across element managers - **Fix Time**: 4-6 hours - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1294 #### #1295 - Reduce singleton usage through dependency injection - **Severity**: MEDIUM (Testability) - **Issue**: 35+ singleton classes make unit testing difficult - **Fix Time**: 40+ hours (major architectural refactor) - **Priority**: LOW - long-term improvement - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1295 #### #1296 - Type safety: Replace 89 'as any' assertions with specific types - **Severity**: MEDIUM (Type Safety) - **Issue**: 89 occurrences across 25 files bypassing TypeScript checking - **Fix Time**: 6-8 hours - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1296 #### #1297 - Type safety: Replace 53 'Record<string, any>' with 'Record<string, unknown>' - **Severity**: LOW (Type Safety) - **Issue**: 53 occurrences that should use `unknown` for better type checking - **Fix Time**: 2-3 hours - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1297 #### #1299 - Refactor EnhancedIndexManager: Extract concerns into separate modules - **Severity**: MEDIUM (Maintainability) - **Location**: `src/portfolio/EnhancedIndexManager.ts` (2,338 lines) - **Issue**: Multiple concerns mixed together in large file - **Fix Time**: 8-12 hours - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1299 --- ## Existing Issues (Already Tracked) ### God Object: index.ts Refactoring #### #881 - refactor: Modularize index.ts into plugin architecture - **Status**: OPEN, **in-progress** - **Labels**: enhancement, architecture, technical-debt, needs-triage - **File**: `src/index.ts` (6,028 lines) - **Issue**: Single file contains entire server with 20+ responsibilities - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/881 - **Note**: This is the primary tracking issue for index.ts refactoring #### #512 - Refactor: Clean up root directory clutter and split massive index.ts file - **Status**: OPEN - **Labels**: enhancement, good first issue, priority: medium - **Scope**: Broader cleanup including index.ts splitting - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/512 **Todd's Offer**: Todd Dibble has offered to help with the index.ts refactoring as he specializes in breaking up monolithic structures into modular components. We welcome this contribution! --- ## False Positives (Not Issues) ### Issue #2: Command Injection via Git Clone **Todd's Concern**: "Allows git URLs like `git://evil.com/repo.git` for remote code execution" **Why It's Not An Issue**: - The regex validator at `commandValidator.ts:32` is `/^[a-zA-Z0-9\-_.\/]+$/` - This **blocks all colons**, which means: - ❌ `https://example.com` - blocked (colon in protocol) - ❌ `git://evil.com` - blocked (colon in protocol) - ❌ `file://path` - blocked (colon in protocol) - ✅ Only allows: alphanumeric, dash, underscore, dot, forward slash - **Actual Risk**: Local path traversal like `../../../etc/passwd` IS allowed, but git requires valid repository structure, limiting exploit potential - **Verdict**: Protocol injection already mitigated by design ### Issue #6: Backtick Command Pattern Too Permissive **Todd's Concern**: "Allows many backtick usages with bypass potential" **Why It's Not An Issue**: - Lines 64-68 in `contentValidator.ts` show **highly specific targeting** of dangerous commands only - Patterns explicitly match dangerous operations: `rm -rf`, `cat /etc/`, `chmod 777`, `sudo rm`, `passwd`, `nc -l`, etc. - Pattern example: `/`[^`]*(?:rm\s+-r[f]?|cat\s+\/etc\/|chmod\s+777|...)[^`]*`/gi` - **Intentional Design**: Blocks backticks containing actual shell exploits while allowing educational examples like `` `echo "Hello"` `` - **Verdict**: Working exactly as designed - restrictive, not permissive --- ## Verified Issues (Investigation Complete) All 5 previously unverified issues have been investigated and confirmed. GitHub issues created for all findings: ### Issue #12: Random Sample Algorithm O(n log n) → Issue #1301 ✅ CONFIRMED #### #1301 - Replace inefficient O(n log n) random sample with Fisher-Yates shuffle - **Severity**: MEDIUM (Performance) - **Location**: `src/portfolio/EnhancedIndexManager.ts:2168-2171` - **Used At**: Lines 1908, 1992, 2013 (3 call sites) - **Issue**: Uses `.sort(() => 0.5 - Math.random())` which is O(n log n) instead of O(k) - **Fix Time**: 15 minutes - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1301 **Current Code**: ```typescript private randomSample<T>(array: T[], size: number): T[] { const shuffled = [...array].sort(() => 0.5 - Math.random()); // ❌ O(n log n) return shuffled.slice(0, size); } ``` **Performance Impact**: For n=1000 with k=10 samples, current implementation does ~10,000 operations vs Fisher-Yates ~10 operations (~1000x improvement). --- ### Issue #13: Collection Search O(n) Linear → Issue #1302 ✅ CONFIRMED #### #1302 - Optimize collection search with inverted index for O(1) lookups - **Severity**: MEDIUM (Performance Optimization) - **Location**: `src/collection/CollectionSearch.ts:345-365` - **Method**: `performIndexSearch()` - **Issue**: Uses linear `.filter()` over all entries; no inverted index - **Current Mitigations**: Heavy caching via `CollectionIndexCache` + `LRUCache` makes impact acceptable for now - **Fix Time**: 4-6 hours - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1302 **Finding**: While O(n) linear search is used, current collection size and aggressive caching strategy make this acceptable. Inverted index would improve scalability but isn't critical. --- ### Issue #15: Lock Queue Unbounded → Issue #1303 ⚠️ PARTIALLY CONFIRMED #### #1303 - Add max queue depth limit to FileLockManager for burst traffic protection - **Severity**: MEDIUM (Security Hardening) - **Location**: `src/security/fileLockManager.ts:19` - **Issue**: `Map<string, Promise<any>>` has no maximum depth limit - **Current Protections**: 10-second timeout per lock + automatic cleanup in finally block - **Vulnerability**: Burst of thousands of requests within 10-second window could exhaust memory - **Fix Time**: 1-2 hours - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1303 **Assessment**: Not unbounded in practice due to timeout, but lacks explicit queue depth protection for burst traffic scenarios. Defense-in-depth improvement recommended. --- ### Issue #17: Circular Dependencies → Issue #1300 🔴 CRITICALLY CONFIRMED #### #1300 - CRITICAL: Runtime circular dependency between EnhancedIndexManager and VerbTriggerManager - **Severity**: CRITICAL (Architectural Flaw) - **Location**: - `src/portfolio/EnhancedIndexManager.ts:30, 233` - `src/portfolio/VerbTriggerManager.ts:16-17, 115` - **Issue**: **Runtime circular dependency** via singleton `getInstance()` calls - **Impact**: Can cause initialization deadlocks, partially initialized singletons, unpredictable behavior - **Fix Time**: 4-8 hours - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1300 **Dependency Chain**: ``` EnhancedIndexManager constructor → VerbTriggerManager.getInstance() → VerbTriggerManager constructor → EnhancedIndexManager.getInstance() → CIRCULAR DEPENDENCY ``` **Critical Finding**: This is NOT just import-level circular dependency (which TypeScript handles). This is a runtime initialization circular dependency between two singletons that can cause serious reliability issues. --- ### Issue #18: Missing Observer Pattern → Issue #1304 💡 FEATURE REQUEST #### #1304 - Feature Request: Add element lifecycle events (Observer Pattern) - **Type**: ENHANCEMENT (Feature Request) - **Severity**: LOW (Nice-to-have) - **Issue**: No event system for element lifecycle (create, update, delete, activate, deactivate) - **Current State**: Architecture works well without events; not a bug - **Use Cases**: Audit logging, analytics, reactive UI updates, plugin system - **Fix Time**: 6-10 hours - **Status**: Open - **URL**: https://github.com/DollhouseMCP/mcp-server/issues/1304 **Verdict**: Correctly classified as feature request. Would be valuable for plugin ecosystem but not required for current functionality. --- ## Summary for Todd Dibble Hi Todd, Thank you for the comprehensive security audit! Here's our complete assessment after full verification: ### ✅ **15 Real Issues Confirmed** - All Tracked We've created GitHub issues #1290-1304 for all confirmed problems: **Initial 10 Issues** (#1290-1299): - 4 security issues (1 critical, 2 high, 1 medium) - 6 architecture/code quality issues **Verified 5 Additional Issues** (#1300-1304): - **#1300** - CRITICAL: Runtime circular dependency (architectural flaw) - **#1301** - MEDIUM: Random sample algorithm O(n log n) - **#1302** - MEDIUM: Collection search optimization - **#1303** - MEDIUM: Lock queue depth limits - **#1304** - LOW: Observer pattern (feature request) ### 📋 **1 Already Tracked** The index.ts refactoring (#881, #512) is already in our backlog and marked in-progress. **We would absolutely love your help on this!** Your expertise in breaking up monolithic structures is exactly what this needs. ### ❌ **2 False Positives** - Git clone command injection - already protected by regex blocking all URL protocols - Backtick patterns - working as designed, appropriately restrictive ### ✅ **All 5 Previously Unverified Issues - NOW VERIFIED** Full investigation completed with detailed findings: - ✅ Random sample algorithm - **CONFIRMED** at EnhancedIndexManager.ts:2168 - ✅ Collection search O(n) - **CONFIRMED** with mitigation context - ⚠️ Lock queue unbounded - **PARTIALLY CONFIRMED** (has timeouts but no explicit limit) - 🔴 Circular dependencies - **CRITICALLY CONFIRMED** (runtime initialization circular dependency!) - 💡 Observer pattern - **CONFIRMED** as feature request (not bug) ### 🚀 **Priority Actions** **CRITICAL (This Week)**: - #1300 (circular dependency) - architectural flaw requiring immediate attention - #1290 (symlink path traversal) - security vulnerability - #1301 (random sample) - quick win (~15 min fix) **HIGH (Next Sprint)**: - #1291 (memory leak) - setInterval cleanup - #1292 (cache growth) - size limits needed - #1303 (lock queue depth) - burst traffic protection **MEDIUM (Backlog)**: - #1302 (inverted index) - scalability improvement - #1293-1299 (various architecture/quality issues) - #1304 (observer pattern) - feature enhancement All issues are properly credited to you. Your audit uncovered a critical runtime circular dependency (#1300) that was previously unknown. **Excellent work!** --- ## Files Reference ### Security Files Reviewed - `src/security/pathValidator.ts` - Path validation (symlink issue found) - `src/security/commandValidator.ts` - Command validation (already secured) - `src/security/contentValidator.ts` - Content validation (backtick patterns, YAML bombs) - `src/security/tokenManager.ts` - Token encryption (could be hardened) ### Performance/Architecture Files Reviewed - `src/index.ts` - 6,028 lines (already tracked for refactoring) - `src/portfolio/NLPScoringManager.ts` - Memory leak found - `src/portfolio/EnhancedIndexManager.ts` - 2,338 lines (needs extraction, random sample issue, circular dependency) - `src/portfolio/VerbTriggerManager.ts` - Circular dependency with EnhancedIndexManager - `src/collection/CollectionSearch.ts` - O(n) linear search (mitigated by caching) - `src/cache/CollectionIndexCache.ts` - Heavy caching infrastructure - `src/cache/APICache.ts` - Unbounded growth found - `src/security/fileLockManager.ts` - Unbounded lock queue (timeout-protected) - `src/elements/skills/SkillManager.ts` - Code duplication with TemplateManager - `src/elements/templates/TemplateManager.ts` - Code duplication with SkillManager ### Statistics from Audit - `as any` type assertions: 89 occurrences across 25 files - `Record<string, any>` usages: 53 occurrences - Singleton classes: 35+ with `getInstance()` pattern - YAML bomb amplification threshold: Currently 10:1, recommend 5:1 --- **Document Created**: October 9, 2025 **Last Updated**: October 9, 2025 (Verification Complete - All 18 findings assessed) **Verification Status**: ✅ Complete - 15 confirmed, 2 false positives, 1 already tracked **GitHub Issues**: #1290-1304 **Contact**: DollhouseMCP Team