DollhouseMCP

# Session Notes - October 1, 2025 (Evening) **Date**: October 1, 2025 **Time**: 5:30 PM - 6:30 PM (1 hour) **Focus**: Fix Security Issue #1228 - Zero-width Unicode bypass vulnerability **Outcome**: ✅ Security fix implemented, documented, and merged to develop ## Session Summary Fixed a HIGH severity security vulnerability where zero-width Unicode characters (U+200B-U+200F, U+FEFF) were bypassing the security validator in metadata parsing. The issue was caused by `validateContent: false` in DefaultElementProvider, which disabled ALL content validation including Unicode attack detection. **Result**: PR #1229 merged to develop (commit dc8a538e) with all CI checks passing and Claude reviewer approval. ## Security Vulnerability Analysis ### Issue Reported **Issue #1228**: [SECURITY] Zero-width Unicode characters bypassing security validator Evidence: Failing test in `metadata-edge-cases.test.ts:495-498` ```typescript // Test expected metadata=null for zero-width chars expect(metadata).toBeNull(); // FAILED - got valid metadata instead ``` ### Attack Vectors Enabled Zero-width characters enable: 1. **Steganography** - Hiding malicious content in seemingly innocent text 2. **Homograph attacks** - Creating visually identical but different identifiers 3. **Display manipulation** - Breaking UI layouts or hiding text 4. **Attack vector obfuscation** - Bypassing pattern matching and filters ### Zero-Width Characters Affected - U+200B (Zero Width Space) - U+200C (Zero Width Non-Joiner) - U+200D (Zero Width Joiner) - U+FEFF (Zero Width No-Break Space / BOM) - U+2028-U+202F (Line/paragraph separators) ## Root Cause Investigation ### The Problem File: `src/portfolio/DefaultElementProvider.ts:496-498` ```typescript // BEFORE - VULNERABLE CODE: const parseResult = SecureYamlParser.parse(fullYaml, { validateContent: false, // ❌ Disables ALL content validation validateFields: false }); ``` **Why this was wrong:** - `validateContent: false` was meant to skip field-specific validation - BUT it also disabled security validation in ContentValidator.validateYamlContent() - ContentValidator calls UnicodeValidator.normalize() when validateContent=true - With validateContent=false, Unicode security checks never ran ### Security Architecture Revealed The security validation chain: 1. `SecureYamlParser.parse()` - Entry point with validateContent flag 2. `ContentValidator.validateYamlContent()` - Only runs if validateContent=true 3. `UnicodeValidator.normalize()` - Detects and blocks zero-width chars **Bypass**: Setting validateContent=false broke the entire chain. ## The Fix ### Code Change File: `src/portfolio/DefaultElementProvider.ts:497-504` ```typescript // AFTER - SECURE CODE: const parseResult = SecureYamlParser.parse(fullYaml, { // REQUIRED: validateContent must be true to enable Unicode security validation including // zero-width character detection (U+200B-U+200F, U+FEFF, etc.). Setting this to false // would disable ALL content validation, creating a security bypass for Unicode-based attacks. // See UnicodeValidator.normalize() for detection logic. validateContent: true, validateFields: false // Field-specific validation is optional for general metadata parsing }); ``` **Impact**: Single line change from `false` → `true` restores security validation. ### Documentation Improvements Per Claude reviewer feedback, added: 1. **Enhanced inline comment** (lines 498-501): - Specific Unicode ranges documented - Security bypass risk explained - Reference to UnicodeValidator.normalize() 2. **JSDoc documentation** (lines 455-466): - Security note linking to Issue #1228 - Explains why validateContent: true is REQUIRED - Documents security implications ## CI Failures Encountered (Unrelated to Fix) ### Problem: package-lock.json Out of Sync All CI checks failed with: ``` npm error Invalid: lock file's react@19.1.1 does not satisfy react@19.2.0 ``` ### Root Cause: Node.js Package Management **Key Learning for Non-Node Developers:** **package.json** - "The Recipe" - Lists dependencies with version **ranges** - Example: `^19.0.0` = "any 19.x version is compatible" - Human-edited **package-lock.json** - "The Exact Receipt" - Records **exact versions** actually installed - Auto-generated by `npm install` - Ensures reproducible builds **Transitive Dependencies** - "Hidden Ingredients" - Dependencies of your dependencies - React isn't in our package.json, but `@modelcontextprotocol/inspector` needs it **Why They Got Out of Sync:** ``` Timeline: Dec 15: Someone runs npm install → React 19.1.1 locks in Jan 5: React 19.2.0 releases Jan 6: CI runs npm ci → "Lock file says 19.1.1 but 19.2.0 exists!" ``` **npm ci** is "strict mode" - requires lock file to match what npm would install fresh. ### The Fix ```bash npm install # Re-resolves dependencies to latest compatible versions ``` **Result**: - React: 19.1.1 → 19.2.0 ✅ - react-dom: 19.1.1 → 19.2.0 ✅ - scheduler: 0.26.0 → 0.27.0 ✅ ## Validation & Testing ### Local Testing ```bash npm test -- test/__tests__/unit/portfolio/metadata-edge-cases.test.ts \ --testNamePattern="should handle various Unicode edge cases" --no-coverage ``` ✅ Test passed (was failing before fix) ### Security Test Suite ```bash npm test -- test/__tests__/unit/security/ --no-coverage ``` ✅ All 236 security unit tests passed - unicode-normalization.test.ts ✅ - unicodeValidator.test.ts ✅ - yamlBombDetection.test.ts ✅ ### CI Results All 14 checks passed: - ✅ Test (ubuntu, macos, windows) - ✅ Docker Build & Test (amd64, arm64) - ✅ Docker Compose Test - ✅ Security Audit - ✅ QA Automated Tests - ✅ SonarCloud Code Analysis - ✅ Claude Review: "LGTM - Valid Security Fix" ## Commits Made ### Commit 1: Security Fix ``` fix(security): Block zero-width Unicode characters in metadata validation - Changed DefaultElementProvider.ts:498 validateContent: false → true - Restores Unicode security validation for all metadata parsing ``` ### Commit 2: Documentation ``` docs(security): Enhance documentation for Unicode validation requirement - Enhanced inline comment with specific Unicode ranges - Added comprehensive JSDoc to readMetadataOnly() method - Security implications clearly documented ``` ### Commit 3: Dependency Sync ``` chore(deps): Sync package-lock.json with latest transitive dependencies - Update React 19.1.1 → 19.2.0 - Update react-dom 19.1.1 → 19.2.0 - Update scheduler 0.26.0 → 0.27.0 - Fixes CI failures from package-lock drift ``` ## Pull Request & Merge **PR #1229**: fix(security): Block zero-width Unicode characters in metadata validation [HIGH] **GitHub**: https://github.com/DollhouseMCP/mcp-server/pull/1229 **Review Status**: - ✅ Claude reviewer approved: "LGTM - Valid Security Fix" - ✅ All CI checks passed - ✅ Security analysis confirmed **Merged**: October 1, 2025, 9:55 PM UTC - Target: `develop` branch - Method: Squash merge - Commit: `dc8a538e` - Branch deleted: `fix/issue-1228-zero-width-unicode-bypass` **Issue #1228**: ✅ Closed with resolution summary ## Security Impact Summary ### Before Fix - ❌ Zero-width characters passed through validator undetected - ❌ Metadata with U+200B-U+200F accepted as valid - ❌ Potential for steganography and homograph attacks - ❌ Security control bypassed ### After Fix - ✅ Zero-width characters properly detected and blocked - ✅ Metadata returns `null` for files with zero-width chars (expected behavior) - ✅ UnicodeValidator.normalize() runs on ALL metadata - ✅ Security control restored ## Key Learnings ### Technical 1. **Defense in Depth**: Boolean flags like `validateContent` can inadvertently disable entire security layers 2. **Test-Driven Security**: Failing tests are excellent bug reports for security issues 3. **Documentation Matters**: Clear security notes prevent future regressions ### Process 1. **Package Management**: Understanding transitive dependencies and lock files critical for Node.js 2. **CI Failures**: Always investigate root cause - may be environmental, not code-related 3. **Code Review Value**: Claude reviewer caught documentation improvement opportunities ### Communication 1. **ELI5 Explanations**: Non-technical users benefit from clear analogies (recipe/receipt for package.json/lock) 2. **Commit Messages**: Detailed explanations help future developers understand security decisions 3. **PR Documentation**: Comprehensive descriptions ensure reviewers understand security implications ## Files Modified 1. `src/portfolio/DefaultElementProvider.ts` - Line 498: validateContent: false → true (security fix) - Lines 455-466: Added JSDoc documentation - Lines 498-501: Enhanced security comments 2. `package-lock.json` - Updated React 19.1.1 → 19.2.0 - Updated react-dom 19.1.1 → 19.2.0 - Updated scheduler 0.26.0 → 0.27.0 ## Related Issues & PRs - **Closes**: #1228 - Zero-width Unicode bypass - **Related**: #259 - Security metrics for Unicode events - **Related**: #170 - Additional security gaps - **Related**: #164 - YAML security patterns ## Next Steps None required - security fix is complete and merged. The vulnerability is closed. ## Statistics - **Time**: 1 hour - **Lines Changed**: 27 insertions, 18 deletions - **Files Modified**: 2 - **Commits**: 3 - **Tests Fixed**: 1 (was failing) - **Security Tests Passing**: 236 - **CI Checks**: 14/14 passed - **Review**: Approved by Claude reviewer --- **Session Type**: Security Fix **Priority**: HIGH **Status**: ✅ Complete and Merged **Branch**: develop (commit dc8a538e)