TODO: Code Review Findings

Review Date: January 13, 2026 Overall Assessment: Solid architecture with good security foundations. After detailed review: 0 critical issues (cleanUserId approach is sound), 5 high-severity items, ~60% test coverage

Security Issues

[x] 1. Review Graph Query Authorization Policy ✅ COMPLETED

Severity: MEDIUM (Downgraded from CRITICAL after review) Files: src/tools/primitives/graph-queries.js:80-150, src/server.js:891-932

Resolution: Only org owners can view other users' collaboration data. Regular members can only query their own data.

Implementation:

• Added validatePersonQueryAccess() helper function in graph-queries.js
• Applied authorization check to findCollaborators, getPersonMeetings, and getRecordingsForPerson
• Case-insensitive email comparison for user matching
• Generic "Access denied" error message (no information disclosure)

Tests: __tests__/graph-authorization.test.js - 15 unit tests covering:

• Self-query allowed
• Cross-user query denied for non-org-owners
• Cross-user query allowed for org owners
• Case-insensitive email matching
• Security edge cases (partial matches, plus addressing, dots)

[ ] 2. Prompt Injection via User Context in AI Prompts

Severity: HIGH (Downgraded - email format limits attack surface) Files: src/MCPIntentClassifier.js:108-118, src/services/AIQueryGenerator.js:418-419

Issue: User email embedded in AI prompts without cleaning or escaping.

// MCPIntentClassifier.js:114 - Current
USER_EMAIL: userContext.email,  // Raw email, not cleaned
USER_ID: userContext.userId,    // This IS cleaned via buildSecurityContext

Analysis:

• userId IS already cleaned via buildSecurityContext() → cleanUserId()
• email is NOT cleaned before embedding in prompts
• cleanUserId() removes . and + but NOT ", \n, } (injection chars)
• Real emails with " or \n are rare but possible in display names

Attack Vector: Email display name like "attacker\n}]NEW_INSTRUCTIONS:"@example.com

Fix: Use cleaned userId consistently AND escape for prompt safety:

// src/MCPIntentClassifier.js - _buildSystemPrompt()
import { cleanUserId } from './utils/index.js';

_buildSystemPrompt(userContext) {
  const cleanedEmail = cleanUserId(userContext.email);
  return getPrompt('intent-classifier', {
    USER_EMAIL: cleanedEmail,  // Use cleaned version for consistency
    USER_ID: userContext.userId,  // Already cleaned
    ...
  });
}

Also update: src/services/AIQueryGenerator.js:418-419 to use cleaned email in query context.

High Severity Issues (P1)

[ ] 3. Team Data Access Without Pre-Validation

Files: src/server.js:937-1003

Issue: The includeTeam parameter is passed directly from user input to graph tools. Validation occurs in tool layer but should be at server handler level.

Fix: Add server-level validation:

if (input.includeTeam && !securityContext.isOrgOwner) {
  return { success: false, error: 'Only org owners can access team data' };
}

[ ] 4. Security Bounds Not Guaranteed in AI Query Generator

File: src/services/AIQueryGenerator.js:683-720

Issue: _injectSecurityBounds() lacks a default case. Unknown collections may not get security filters.

Fix: Add validation after security injection:

// Add at end of _injectSecurityBounds()
if (!securityFilter || !securityFilter.apiKey) {
  throw new Error(`Security bounds not applied for collection: ${collection}`);
}

[ ] 5. Information Disclosure via Error Messages

File: src/authorization.js:86

Issue: Error messages reveal resource existence:

// Current
throw new MCPAuthorizationError('You do not have access to this room');
// Reveals room exists

Fix: Use generic messages:

throw new MCPAuthorizationError('Access denied');

[ ] 6. Missing Rate Limiting on Agent Queries

File: src/agent/MCPAgentOrchestrator.js

Issue: No rate limiting on complex queries that can cost $0.50-1.00 per execution.

Fix: Implement per-user rate limiting (e.g., max 10 agent queries per minute).

Medium Severity Issues (P2)

[ ] 7. Weak Email Validation Regex

File: src/tools/primitives/actions.js:86

Issue: Current regex accepts invalid emails like @@example.com:

const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;

Fix: Use stricter validation:

const emailRegex = /^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$/;

[ ] 8. Query Injection in AIQueryGenerator

File: src/services/AIQueryGenerator.js:418-419

Issue: User query embedded without sanitization:

const queryWithContext = `User question: ${naturalQuery}`;

Fix: Apply sanitization before embedding (reuse _sanitizeQuery from MCPIntentClassifier).

[ ] 9. No DateTime Range Limits

File: src/tools/primitives/actions.js:29-76

Issue: Can schedule meetings 100+ years in future.

Fix: Add range validation:

const year = date.getFullYear();
const currentYear = new Date().getFullYear();
if (year > currentYear + 10 || year < currentYear - 1) {
  return { valid: false, error: 'Date out of allowed range' };
}

Low Severity Issues (P3)

[ ] 10. Detailed Errors Leak Schema Information

File: src/services/AIQueryGenerator.js:551

Issue: Error logs may expose MongoDB schema to users in production.

Fix: Return generic errors to users, log details internally only.

[ ] 11. Potential ReDoS in Room Name Matching

File: src/server.js:1127

Issue: Verify escapeRegex() implementation handles all dangerous regex characters.

Fix: Review src/utils/escapeRegex.js for completeness.

Code Quality Issues

[ ] 12. Circular Module Dependencies

File: src/agent/index.js:123

Issue: Dynamic imports inside conditionals can cause runtime failures.

Fix: Move imports to top-level with proper error handling.

[ ] 13. Scattered Singleton State

Files: data-queries.js, index.js, ai-helpers.js

Issue: Multiple singletons (_aiServiceInstance, _deps, aiClient) without clear lifecycle.

Fix: Consider explicit service registry or factory pattern.

[ ] 14. Memory Leak in Agent Loop

File: src/agent/MCPAgentOrchestrator.js:173

Issue: Messages array grows unbounded (70+ messages possible).

Fix: Implement message history truncation after 3 iterations, keeping only summary.

[ ] 15. N+1 Query Patterns

File: src/tools/primitives/data-queries.js

Issue: queryRooms() with participants fetches all rooms, then filters in memory.

Fix: Use MongoDB $in operator for participant filtering.

[ ] 16. Service Instance Per Call

File: src/tools/primitives/search.js:78

Issue: Creates new AISearchService instance per call (200-500ms overhead).

Fix: Implement service caching/pooling.

[ ] 17. Large File Needs Splitting

File: src/tools/primitives/data-queries.js (2000+ lines)

Recommendation: Split into focused modules:

• rooms.js - queryRooms, related functions
• schedules.js - querySchedules, related functions
• transcripts.js - getTranscript, summarizeTranscript
• context.js - queryMeetingContext

[ ] 18. Inconsistent Return Formats

Issue: Different tool categories use different patterns:

• Data queries: { success: true, data: [] }
• Actions: throw Error on failure
• Graph queries: throw Error on failure

Fix: Standardize all to return { success, error?, data? }.

[ ] 19. Timeout Promise Not Cleaned Up

File: src/MCPClient.js:171-177

Issue: Promise.race timeout not cancelled on success, causing memory accumulation.

Fix: Use AbortController pattern for proper cleanup.

Testing Gaps

[ ] 20. Create __tests__/authorization.test.js

Priority: CRITICAL

File: src/authorization.js (317 lines, 0 tests)

Test coverage needed for:

• [ ] validateRoomAccess() - room access control
• [ ] validateScheduleAccess() - schedule access control
• [ ] validateScheduleModifyAccess() - modification rights
• [ ] validateRoomOwnership() - ownership validation
• [ ] applyAuthorizationFilters() - query filter injection
• [ ] Cross-tenant isolation scenarios
• [ ] Admin bypass validation
• [ ] Error scenarios (room/schedule not found)

[ ] 21. Create server/__tests__/http-transport.test.js

Priority: HIGH

File: server/http-transport.js (250+ lines, 0 tests)

Test coverage needed for:

• [ ] SSE connection establishment
• [ ] Authentication header validation (X-API-Key, x-user-email)
• [ ] resolveUserAuth() function
• [ ] Error responses for missing auth
• [ ] Connection timeout handling

[ ] 22. Create Server Tool Handler Tests

Priority: HIGH

File: src/server.js (3344 lines, minimal tests)

Test coverage needed for:

• [ ] query_data tool
• [ ] create_room tool
• [ ] start_meeting tool
• [ ] create_schedule tool
• [ ] cancel_schedule tool
• [ ] get_transcription tool
• [ ] open_file tool
• [ ] Parameter validation for each tool
• [ ] Error handling for each tool

[ ] 23. Create __tests__/search.test.js

Priority: MEDIUM

File: src/tools/primitives/search.js (335+ lines, 0 tests)

Test coverage needed for:

• [ ] searchContent() function
• [ ] Content type filtering
• [ ] Date range filtering
• [ ] Security context validation

[ ] 24. Create __tests__/agent-index.test.js

Priority: MEDIUM

File: src/agent/index.js (338 lines, 0 tests)

Test coverage needed for:

• [ ] handleAgentQuery() main flow
• [ ] Route type handling (DIRECT, SIMPLE, CLASSIFY, AGENT)
• [ ] Security context transformation
• [ ] Error propagation

[ ] 25. Add Coverage Reporting

File: vitest.config.js

Add:

coverage: {
  provider: 'v8',
  reporter: ['text', 'html'],
  all: true,
  include: ['src/**/*.js'],
  exclude: ['src/**/*.test.js', 'node_modules'],
  lines: 80,
  functions: 80,
  branches: 75,
  statements: 80
}

Security Strengths (No Action Needed)

• [x] Prompt injection defense in MCPIntentClassifier._sanitizeQuery() - blocks templates, script tags
• [x] MongoDB dangerous operators blocked ($where, $function, $lookup, $out, $merge)
• [x] Multi-tenant isolation via apiKey consistently applied
• [x] Query results capped at 100 documents
• [x] Regex escaping with escapeRegex() for database queries

Verification Commands

After implementing fixes:

# Security tests
npm test -- -t "authorization"
npm test -- -t "security"

# Full test suite
npm test

# Coverage report (after adding config)
npm test -- --coverage

Manual Testing:

• [ ] Try querying another user's collaborators (should fail for non-org-owners)
• [ ] Test with malicious email containing injection characters
• [ ] Verify error messages don't leak room/schedule existence

Summary

Notes after detailed review:

• Item #1 (Graph Query Auth) downgraded CRITICAL → MEDIUM: Multi-tenant isolation via apiKey IS enforced, cleanUserId() ensures consistent identity matching
• Item #2 (Prompt Injection) downgraded CRITICAL → HIGH: userId IS already cleaned, only email in prompts needs fixing, attack surface limited by email format validation