Add tool definition validation and improve MCP caching

Introduces McpToolDefinitionValidator to validate and filter tool definitions before caching. Updates McpHttpClientService to support configurable MCP server URLs and sanitizes error messages. Enhances McpToolsCacheService to use validated tools, adds restrictive file permissions for cache, and improves error handling and logging. Updates CliConsts with new constants for MCP server configuration.

framework/src/Volo.Abp.Cli.Core/Volo/Abp/Cli/CliConsts.cs

@@ -20,9 +20,13 @@ public static class CliConsts
 
     public static string AppSettingsSecretJsonFileName = "appsettings.secrets.json";
 
+    public const string McpServerUrlEnvironmentVariable = "ABP_MCP_SERVER_URL";
+    public const string DefaultMcpServerUrl = "https://mcp.abp.io";
+
     public static class MemoryKeys
     {
         public const string LatestCliVersionCheckDate = "LatestCliVersionCheckDate";
         public const string McpToolsLastFetchDate = "McpToolsLastFetchDate";
+        public const string McpServerUrl = "McpServerUrl";
     }
 }

framework/src/Volo.Abp.Cli.Core/Volo/Abp/Cli/Commands/Services/McpHttpClientService.cs

@@ -7,6 +7,7 @@ using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
 using Volo.Abp.Cli.Commands.Models;
 using Volo.Abp.Cli.Http;
+using Volo.Abp.Cli.Memory;
 using Volo.Abp.DependencyInjection;
 
 namespace Volo.Abp.Cli.Commands.Services;
@@ -15,21 +16,51 @@ public class McpHttpClientService : ITransientDependency
 {
     private readonly CliHttpClientFactory _httpClientFactory;
     private readonly ILogger<McpHttpClientService> _logger;
-    
-    private const string DefaultMcpServerUrl = "https://mcp.abp.io";
-    private const string LocalMcpServerUrl = "http://localhost:5100";
+    private readonly MemoryService _memoryService;
+    private string _cachedServerUrl;
 
     public McpHttpClientService(
         CliHttpClientFactory httpClientFactory,
-        ILogger<McpHttpClientService> logger)
+        ILogger<McpHttpClientService> logger,
+        MemoryService memoryService)
     {
         _httpClientFactory = httpClientFactory;
         _logger = logger;
+        _memoryService = memoryService;
     }
 
-    public async Task<string> CallToolAsync(string toolName, JsonElement arguments, bool useLocalServer = false)
+    private async Task<string> GetMcpServerUrlAsync()
     {
-        var baseUrl = LocalMcpServerUrl;//useLocalServer ? LocalMcpServerUrl : DefaultMcpServerUrl;
+        // Return cached URL if already resolved
+        if (_cachedServerUrl != null)
+        {
+            return _cachedServerUrl;
+        }
+
+        // 1. Check environment variable (highest priority)
+        var envUrl = Environment.GetEnvironmentVariable(CliConsts.McpServerUrlEnvironmentVariable);
+        if (!string.IsNullOrWhiteSpace(envUrl))
+        {
+            _cachedServerUrl = envUrl.TrimEnd('/');
+            return _cachedServerUrl;
+        }
+
+        // 2. Check persisted setting
+        var persistedUrl = await _memoryService.GetAsync(CliConsts.MemoryKeys.McpServerUrl);
+        if (!string.IsNullOrWhiteSpace(persistedUrl))
+        {
+            _cachedServerUrl = persistedUrl.TrimEnd('/');
+            return _cachedServerUrl;
+        }
+
+        // 3. Return default
+        _cachedServerUrl = CliConsts.DefaultMcpServerUrl;
+        return _cachedServerUrl;
+    }
+
+    public async Task<string> CallToolAsync(string toolName, JsonElement arguments)
+    {
+        var baseUrl = "http://localhost:5100";//await GetMcpServerUrlAsync();
         var url = $"{baseUrl}/tools/call";
 
         try
@@ -53,9 +84,8 @@ public class McpHttpClientService : ITransientDependency
 
             if (!response.IsSuccessStatusCode)
             {
-                var errorContent = await response.Content.ReadAsStringAsync();
-                // Log to stderr to avoid corrupting stdout
-                await Console.Error.WriteLineAsync($"[MCP] API call failed: {response.StatusCode} - {errorContent}");
+                // Log to stderr to avoid corrupting stdout - sanitize error message
+                await Console.Error.WriteLineAsync($"[MCP] API call failed with status: {response.StatusCode}");
                 
                 return JsonSerializer.Serialize(new
                 {
@@ -64,7 +94,7 @@ public class McpHttpClientService : ITransientDependency
                         new
                         {
                             type = "text",
-                            text = $"Error: {response.StatusCode} - {errorContent}"
+                            text = $"Error: API call failed with status {response.StatusCode}"
                         }
                     }
                 });
@@ -91,9 +121,9 @@ public class McpHttpClientService : ITransientDependency
         }
     }
 
-    public async Task<bool> CheckServerHealthAsync(bool useLocalServer = false)
+    public async Task<bool> CheckServerHealthAsync()
     {
-        var baseUrl = useLocalServer ? LocalMcpServerUrl : DefaultMcpServerUrl;
+        var baseUrl = "http://localhost:5100";//await GetMcpServerUrlAsync();
 
         try
         {
@@ -108,9 +138,9 @@ public class McpHttpClientService : ITransientDependency
         }
     }
 
-    public async Task<List<McpToolDefinition>> GetToolDefinitionsAsync(bool useLocalServer = false)
+    public async Task<List<McpToolDefinition>> GetToolDefinitionsAsync()
     {
-        var baseUrl = LocalMcpServerUrl; //useLocalServer ? LocalMcpServerUrl : DefaultMcpServerUrl;
+        var baseUrl = "http://localhost:5100";//await GetMcpServerUrlAsync();
         var url = $"{baseUrl}/tools";
 
         try
@@ -120,9 +150,9 @@ public class McpHttpClientService : ITransientDependency
 
             if (!response.IsSuccessStatusCode)
             {
-                var errorContent = await response.Content.ReadAsStringAsync();
-                await Console.Error.WriteLineAsync($"[MCP] Failed to fetch tool definitions: {response.StatusCode} - {errorContent}");
-                throw new Exception($"Failed to fetch tool definitions: {response.StatusCode}");
+                // Sanitize error message - don't expose server details
+                await Console.Error.WriteLineAsync($"[MCP] Failed to fetch tool definitions with status: {response.StatusCode}");
+                throw new Exception($"Failed to fetch tool definitions with status: {response.StatusCode}");
             }
 
             var responseContent = await response.Content.ReadAsStringAsync();

framework/src/Volo.Abp.Cli.Core/Volo/Abp/Cli/Commands/Services/McpToolDefinitionValidator.cs

@@ -0,0 +1,181 @@
+using System;
+using System.Collections.Generic;
+using System.Text.RegularExpressions;
+using Microsoft.Extensions.Logging;
+using Volo.Abp.Cli.Commands.Models;
+using Volo.Abp.DependencyInjection;
+
+namespace Volo.Abp.Cli.Commands.Services;
+
+public class McpToolDefinitionValidator : ITransientDependency
+{
+    private const int MaxToolNameLength = 100;
+    private const int MaxDescriptionLength = 2000;
+    private const string UnknownToolName = "<unknown>";
+    
+    private static readonly Regex ToolNameRegex = new Regex("^[a-zA-Z0-9_]+$", RegexOptions.Compiled);
+    private static readonly HashSet<string> ValidTypeValues = new HashSet<string> 
+        { "string", "number", "boolean", "object", "array" };
+
+    private readonly ILogger<McpToolDefinitionValidator> _logger;
+
+    public McpToolDefinitionValidator(ILogger<McpToolDefinitionValidator> logger)
+    {
+        _logger = logger;
+    }
+
+    public List<McpToolDefinition> ValidateAndFilter(List<McpToolDefinition> tools)
+    {
+        if (tools == null || tools.Count == 0)
+        {
+            return new List<McpToolDefinition>();
+        }
+
+        var validTools = new List<McpToolDefinition>();
+
+        foreach (var tool in tools)
+        {
+            try
+            {
+                if (!IsValidTool(tool))
+                {
+                    continue;
+                }
+
+                validTools.Add(tool);
+            }
+            catch (Exception ex)
+            {
+                _logger.LogWarning($"Error validating tool '{tool?.Name ?? UnknownToolName}': {ex.Message}");
+            }
+        }
+
+        if (validTools.Count < tools.Count)
+        {
+            _logger.LogWarning($"Filtered out {tools.Count - validTools.Count} invalid tool(s). {validTools.Count} valid tool(s) remaining.");
+        }
+
+        return validTools;
+    }
+
+    private bool IsValidTool(McpToolDefinition tool)
+    {
+        if (!IsValidToolName(tool))
+        {
+            return false;
+        }
+
+        if (!IsValidDescription(tool))
+        {
+            return false;
+        }
+
+        if (tool.InputSchema != null && !IsValidInputSchema(tool))
+        {
+            return false;
+        }
+
+        return true;
+    }
+
+    private bool IsValidToolName(McpToolDefinition tool)
+    {
+        if (string.IsNullOrWhiteSpace(tool.Name))
+        {
+            _logger.LogWarning($"Skipping tool with empty name");
+            return false;
+        }
+
+        if (tool.Name.Length > MaxToolNameLength)
+        {
+            _logger.LogWarning($"Skipping tool '{tool.Name}' with name exceeding {MaxToolNameLength} characters");
+            return false;
+        }
+
+        if (!ToolNameRegex.IsMatch(tool.Name))
+        {
+            _logger.LogWarning($"Skipping tool with invalid name format: {tool.Name}");
+            return false;
+        }
+
+        return true;
+    }
+
+    private bool IsValidDescription(McpToolDefinition tool)
+    {
+        if (string.IsNullOrWhiteSpace(tool.Description))
+        {
+            _logger.LogWarning($"Skipping tool '{tool.Name}' with empty description");
+            return false;
+        }
+
+        if (tool.Description.Length > MaxDescriptionLength)
+        {
+            _logger.LogWarning($"Skipping tool '{tool.Name}' with description exceeding {MaxDescriptionLength} characters");
+            return false;
+        }
+
+        return true;
+    }
+
+    private bool IsValidInputSchema(McpToolDefinition tool)
+    {
+        if (!ArePropertiesValid(tool))
+        {
+            return false;
+        }
+
+        if (!AreRequiredFieldsValid(tool))
+        {
+            return false;
+        }
+
+        return true;
+    }
+
+    private bool ArePropertiesValid(McpToolDefinition tool)
+    {
+        if (tool.InputSchema.Properties == null)
+        {
+            return true;
+        }
+
+        foreach (var property in tool.InputSchema.Properties)
+        {
+            if (string.IsNullOrWhiteSpace(property.Value?.Type) ||
+                !ValidTypeValues.Contains(property.Value.Type))
+            {
+                _logger.LogWarning($"Skipping tool '{tool.Name}' with invalid property type: {property.Value?.Type ?? UnknownToolName}");
+                return false;
+            }
+
+            if (property.Value.Description != null && property.Value.Description.Length > MaxDescriptionLength)
+            {
+                _logger.LogWarning($"Skipping tool '{tool.Name}' with property description exceeding {MaxDescriptionLength} characters");
+                return false;
+            }
+        }
+
+        return true;
+    }
+
+    private bool AreRequiredFieldsValid(McpToolDefinition tool)
+    {
+        if (tool.InputSchema.Required == null || tool.InputSchema.Properties == null)
+        {
+            return true;
+        }
+
+        foreach (var required in tool.InputSchema.Required)
+        {
+            if (!tool.InputSchema.Properties.ContainsKey(required))
+            {
+                _logger.LogWarning($"Skipping tool '{tool.Name}' with required field '{required}' not in properties");
+                return false;
+            }
+        }
+
+        return true;
+    }
+}
+

framework/src/Volo.Abp.Cli.Core/Volo/Abp/Cli/Commands/Services/McpToolsCacheService.cs

@@ -16,15 +16,18 @@ public class McpToolsCacheService : ITransientDependency
 {
     private readonly McpHttpClientService _mcpHttpClient;
     private readonly MemoryService _memoryService;
+    private readonly McpToolDefinitionValidator _validator;
     private readonly ILogger<McpToolsCacheService> _logger;
 
     public McpToolsCacheService(
         McpHttpClientService mcpHttpClient,
         MemoryService memoryService,
+        McpToolDefinitionValidator validator,
         ILogger<McpToolsCacheService> logger)
     {
         _mcpHttpClient = mcpHttpClient;
         _memoryService = memoryService;
+        _validator = validator;
         _logger = logger;
     }
 
@@ -46,16 +49,27 @@ public class McpToolsCacheService : ITransientDependency
             await Console.Error.WriteLineAsync("[MCP] Fetching tool definitions from server...");
             var tools = await _mcpHttpClient.GetToolDefinitionsAsync();
             
-            // Save to cache
-            await SaveToCacheAsync(tools);
+            // Validate and filter tool definitions
+            var validTools = _validator.ValidateAndFilter(tools);
+            
+            if (validTools.Count == 0)
+            {
+                _logger.LogWarning("No valid tool definitions received from server");
+                await Console.Error.WriteLineAsync("[MCP] Warning: No valid tool definitions received from server");
+                return new List<McpToolDefinition>();
+            }
+            
+            // Save validated tools to cache
+            await SaveToCacheAsync(validTools);
             await _memoryService.SetAsync(CliConsts.MemoryKeys.McpToolsLastFetchDate, DateTime.Now.ToString(CultureInfo.InvariantCulture));
             
-            await Console.Error.WriteLineAsync($"[MCP] Successfully fetched and cached {tools.Count} tool definitions");
-            return tools;
+            await Console.Error.WriteLineAsync($"[MCP] Successfully fetched and cached {validTools.Count} tool definitions");
+            return validTools;
         }
         catch (Exception ex)
         {
-            _logger.LogWarning($"Failed to fetch tool definitions from server: {ex.Message}");
+            // Sanitize error message - use generic message for logger
+            _logger.LogWarning("Failed to fetch tool definitions from server");
             await Console.Error.WriteLineAsync($"[MCP] Failed to fetch from server, attempting to use cached data...");
             
             // Fall back to cache even if expired
@@ -148,6 +162,9 @@ public class McpToolsCacheService : ITransientDependency
             });
 
             File.WriteAllText(CliPaths.McpToolsCache, json);
+            
+            // Set restrictive file permissions (user read/write only)
+            SetRestrictiveFilePermissions(CliPaths.McpToolsCache);
         }
         catch (Exception ex)
         {
@@ -156,5 +173,23 @@ public class McpToolsCacheService : ITransientDependency
 
         return Task.CompletedTask;
     }
+
+    private void SetRestrictiveFilePermissions(string filePath)
+    {
+        try
+        {
+            // On Unix systems, set permissions to 600 (user read/write only)
+            if (!OperatingSystem.IsWindows())
+            {
+                File.SetUnixFileMode(filePath, UnixFileMode.UserRead | UnixFileMode.UserWrite);
+            }
+            // On Windows, the file inherits permissions from the user profile directory,
+            // which is already restrictive to the current user
+        }
+        catch (Exception ex)
+        {
+            _logger.LogWarning($"Error setting file permissions: {ex.Message}");
+        }
+    }
 }