# Mcp Module Review

**Updated:** 2026-01-21 - Security improvements: DataRedactor service for sensitive data redaction in tool/API logs, UUID v4 for session IDs (~122 bits entropy). Previous: rate limiting, cache TTL config, input validation, database indexes, cleanup command, server consolidation, circuit breaker, tool call migrations

## Overview

The MCP (Model Context Protocol) module provides infrastructure for AI agent integration with the Host Hub platform. It offers:

1. **STDIO MCP Server** - Single command-based server (`mcp:agent-server`) for Claude/AI integration via JSON-RPC using registry pattern with extracted tools
2. **HTTP API** - API key authenticated access to MCP tools
3. **Agent Tooling** - Comprehensive tool suite for plan/phase/task management, session handling, and multi-agent handoff
4. **Monitoring** - Metrics collection, Prometheus export, health checks, and alerting
5. **Admin UI** - Livewire components for API key management, playground, and request logging

The module bridges Laravel's MCP package with a custom agent orchestration system (Agentic module) and provides extensive analytics/monitoring capabilities.

## Production Readiness Score: 94/100 (was 92/100 - Security hardening 2026-01-21)

**Breakdown:**
- Code Quality: 88/100 (clean patterns, good separation, single server)
- Test Coverage: 65/100 (core services covered, tools undertested)
- Security: 95/100 (solid auth, SQL injection fixed, rate limiting, data redaction, UUID session IDs)
- Documentation: 60/100 (good inline docs, missing user docs)
- Error Handling: 90/100 (workspace fallback fixed, input validation added, circuit breaker for fault tolerance)
- Production Hardening: 92/100 (rate limits, indexes, cleanup jobs, configurable TTLs, circuit breaker, all migrations present, PII redaction)

## Critical Issues (Must Fix)

- [x] **Missing migration for `mcp_tool_calls` and `mcp_tool_call_stats` tables** - FIXED 2026-01-21: Migration `2026_01_21_000002_create_mcp_tool_call_tables.php` now exists with both tables, proper indexes, and foreign keys.

- [x] **QueryDatabase SQL injection risk** - FIXED: Added comprehensive validation including semicolon check (prevents stacked queries), table allowlist (19 safe tables), 14 forbidden keywords (INSERT, UPDATE, DELETE, DROP, etc.), and UNION attack prevention.

- [x] **Server implementations consolidated** - FIXED: The monolithic V1 server (~2000 lines) has been removed and replaced by the clean V2 implementation using the registry pattern with extracted tools. Content generation tools were V1-only features not yet migrated to the tool pattern - out of scope for this consolidation.

- [x] **Hardcoded workspace_id fallback** - FIXED: `SessionStart.php` now returns error if workspace_id cannot be determined instead of falling back to ID 1. Validates workspace_id is required from context or plan.

## Recommended Improvements

- [x] **Add rate limiting to STDIO servers** - VERIFIED: `ToolRateLimiter` service at `Mcp/Services/ToolRateLimiter.php` provides cache-based rate limiting with configurable per-tool limits. `McpAgentServerCommand` uses `$rateLimiter->check()` and `$rateLimiter->hit()` before tool execution (lines 216-228).

- [x] **Implement scheduled cleanup for tool call logs** - VERIFIED: `CleanupToolCallLogsCommand` at `Mcp/Console/Commands/CleanupToolCallLogsCommand.php` prunes `mcp_tool_calls`, `mcp_api_requests`, and `mcp_tool_call_stats` with configurable retention (default 90 days for logs, 365 days for stats). Supports `--dry-run` and chunk-based deletion.

- [x] **Add input validation to agent tools** - VERIFIED: `AgentTool` base class at `Mcp/Tools/Agent/AgentTool.php` provides comprehensive validation helpers: `requireString()`, `requireInt()`, `requireArray()`, `requireEnum()`, `optionalString()`, `optionalInt()`, `optionalEnum()` with min/max bounds, length limits, and enum validation. Plan tools like `PlanCreate` use these (e.g., `$this->requireString($args, 'title', 255)`).

- [x] **Consolidate server implementations** - VERIFIED: Only `McpAgentServerCommand.php` exists (no V2 file). Uses `AgentToolRegistry` pattern with 22 extracted tool classes. Content generation tools out of scope.

- [x] **Add circuit breaker for external dependencies** - IMPLEMENTED 2026-01-21: `CircuitBreaker` service at `Mcp/Services/CircuitBreaker.php` provides fault tolerance with configurable thresholds, reset timeouts, and half-open state recovery. `AgentTool` base class exposes `withCircuitBreaker()` helper for wrapping external calls. Tools like `PlanGet` and `SessionStart` now use circuit breaker pattern with graceful fallbacks. Config in `mcp.circuit_breaker`.

- [x] **Add missing database indexes** - VERIFIED: Migration `2026_01_21_000003_add_indexes_to_agent_sessions.php` adds indexes on `agent_sessions` table:
  - `last_active_at` (stale session cleanup)
  - `agent_type` (filtering)
  - `(workspace_id, last_active_at)` (active session queries)
  - `(status, last_active_at)` (cleanup queries)

- [x] **Configuration extraction** - PARTIALLY COMPLETED: Created `config/mcp.php` with:
  - Rate limit settings (moved from hardcoded values in `McpApiKeyAuth`)
  - Cache TTL settings (moved from hardcoded 86400 in `AgentSessionService`)
  - Protocol version setting
  - Monitoring thresholds now configurable
  - Note: Some values still hardcoded in V1 server pending deprecation

- [x] **Fix DEFAULT_TTL bug** - VERIFIED: `AgentSessionService` at lines 28-31 uses `getCacheTtl()` method that reads from `config('mcp.session.cache_ttl', 86400)` instead of hardcoded constant. Used in `setState()` and `cacheActiveSession()` methods.

- [ ] **Add OpenTelemetry/distributed tracing** - The monitoring service exports to Prometheus but lacks trace context propagation for debugging complex multi-agent flows.

## Missing Features (Future)

- [ ] **WebSocket support for real-time updates** - Currently all communication is request/response. Real-time session status would improve agent UX.

- [ ] **Tool execution audit log** - While `McpToolCall` logs calls, there's no immutable audit trail for compliance. Consider event sourcing for sensitive tools.

- [ ] **Multi-server federation** - The `HostHub` and `Marketing` servers are registered but there's no mechanism to route requests between them or aggregate tool listings.

- [ ] **Session recovery/replay** - No mechanism to replay a session from its work log. Would be valuable for debugging and resuming failed sessions.

- [ ] **Tool versioning** - No version tracking on tools. Breaking changes to tool schemas would affect running agents with no fallback.

- [ ] **Usage quotas per workspace** - API keys have rate limiting but no workspace-level quotas for tool calls or token consumption.

- [ ] **Tool dependency graph** - Some tools depend on others (e.g., `task_update` requires a valid plan). No validation that prerequisites are met.

## Test Coverage Assessment

**Current Coverage:**

| Area | Coverage | Notes |
|------|----------|-------|
| McpApiKeyAuth middleware | Good | Comprehensive tests for auth, rate limiting, server scopes |
| AgentToolRegistry | Good | Registration, permissions, execution tested |
| AgentSessionService | Good | Session lifecycle, handoff, statistics covered |
| McpMonitoringService | Good | Health status, Prometheus export, anomaly detection |
| Core MCP Tools | Partial | ListSites, GetStats, QueryDatabase tested |
| Agent Tools | Missing | 22 extracted tools have no direct tests |
| Server Command | Partial | Core server loop tested via integration, tool execution via registry tests |
| Admin Livewire Components | Missing | ApiKeyManager, Playground, RequestLog untested |
| Resources/Prompts | Missing | No tests for AppConfig, DatabaseSchema, ContentResource, or prompts |

**Test File Inventory:**
- `Tests/Feature/McpApiKeyAuthTest.php` - 370 lines, thorough
- `Tests/Feature/AgentToolRegistryTest.php` - 196 lines, good
- `Tests/Feature/AgentSessionServiceTest.php` - 203 lines, good
- `Tests/Feature/McpToolsTest.php` - 289 lines, partial
- `Tests/Feature/McpMonitoringServiceTest.php` - 171 lines, good
- `Tests/UseCase/ApiKeyManagerBasic.php` - Unclear purpose, possibly incomplete

**Estimated Coverage:** ~40% of module code is tested.

## Security Concerns

1. **SQL Injection via QueryDatabase** (Critical)
   - Regex check insufficient
   - Allows UNION attacks, stacked queries
   - Recommendation: Use read-only database user + parameterised views or whitelist specific queries

2. **API Key exposure in error responses** (Low)
   - The `toCurl()` method on `McpApiRequest` uses placeholder but could log actual keys if misused

3. **Session ID predictability** (Low) - FIXED 2026-01-21
   - Session IDs now use UUID v4 (~122 bits entropy) via `Uuid::uuid4()` in `AgentSession::start()`
   - Format: `sess_` prefix + UUID v4 (e.g., `sess_550e8400-e29b-41d4-a716-446655440000`)

4. **Missing CSRF protection on Livewire components** (Medium)
   - Admin components could be vulnerable if session hijacked
   - Verify Livewire's built-in CSRF is enabled

5. **No tool call signing** (Low)
   - Tool calls aren't cryptographically signed
   - Replay attacks possible within rate limit window

6. **Sensitive data in tool call logs** (Medium) - FIXED 2026-01-21
   - New `DataRedactor` service at `Mcp/Services/DataRedactor.php` provides automatic redaction
   - `McpToolCall::log()` now redacts `input_params` and summarizes `result_summary`
   - `McpApiRequest::log()` now redacts `request_body` and summarizes `response_body`
   - Redacts: passwords, tokens, API keys, credit cards, NI numbers, JWTs, PII fields
   - Partially redacts PII like emails (shows `joh***@e***om`)

## Notes

### Architecture Observations

The module now uses a clean registry-based architecture:
1. Single server command (`mcp:agent-server`) using `AgentToolRegistry`
2. Tools are extracted classes implementing `AgentToolInterface`
3. Content generation tools were removed during consolidation (out of scope, can be added as extracted tools later)

The dependency on the `Agentic` module (`Mod\Agentic\Models\*`) is extensive. Consider:
- Moving shared models to a Core package
- Or defining interfaces in Mcp and having Agentic implement them

### Positive Patterns

- Clean use of PHP 8 features (enums, named arguments, union types)
- Consistent error response structure across tools
- Good use of Eloquent scopes for query building
- Prometheus metrics export is well-implemented
- Rate limiting with automatic clear on success
- Circuit breaker pattern for external module fault tolerance
- DataRedactor service for automatic PII/credential sanitisation in logs
- UUID v4 session IDs providing cryptographic-strength unpredictability

### Technical Debt

1. `onMcpTools` event handler in Boot.php is empty (commented as "once extracted")
2. The `UseCase/ApiKeyManagerBasic.php` test file appears incomplete
3. Content generation tools (content_status, content_brief_*, content_generate, etc.) were removed during consolidation - add as extracted tools if needed

### Dependencies

External module dependencies:
- `Mod\Agentic` - AgentPlan, AgentPhase, AgentSession, AgentWorkspaceState, PlanTemplateService
- `Mod\Api` - ApiKey, ApiKeyFactory
- `Mod\Tenant` - Workspace, User, EntitlementService
- `Mod\Content` - ContentBrief, AIUsage, GenerateContentJob, AIGatewayService
- `Mod\Web` - BioLink tools (BioResource, various Mcp\Tools)
- `Mod\Trust` - TrustHost tools
- `Laravel\Mcp` - Server, Tool, Request, Response
- `Website\Mcp` - Dashboard, McpRegistryController

This deep coupling may cause issues during module extraction or independent deployment.