6f309979de
Relocates the MCP module to a top-level namespace as part of the monorepo separation, removing the intermediate Mod directory layer. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
179 lines
11 KiB
Markdown
179 lines
11 KiB
Markdown
# 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.
|