Conversation
完整实现 MCP (Model Context Protocol) 透传功能，支持 minimax 图片识别和联网搜索 功能特性: - 支持 4 种透传类型: none, minimax, glm (预留), custom (预留) - 实现 MinimaxMcpClient: webSearch() 和 understandImage() 方法 - 完整的 UI 配置表单和国际化支持 (中英文) - 数据库迁移文件和数据访问层集成 - MCP 透传处理器实现 技术实现: - 数据模型: McpPassthroughType 类型定义 - 数据库: providers 表新增 mcp_passthrough_type 字段 - Repository: 6 处字段映射添加 - Actions: addProvider/editProvider 支持 MCP 配置 - UI: Collapsible 配置区域，支持类型选择 - MCP Client: 类型安全的 API 调用封装 - Handler: 工具调用检测和透传处理 Author: flintttan <flintttan@gmail.com> Related: WFS-mcp-passthrough-feature (IMPL-001 ~ IMPL-008)
完整实现 GLM MCP 透传功能，支持图片分析和视频分析，并补充完整的多语言 i18n 配置 ## 主要功能 - 新增 GLM MCP 客户端：支持 analyze_image、analyze_video 工具 - 更新 MCP 透传处理器：集成 GLM 工具调用处理 - 扩展类型定义：添加 GLM 相关的请求/响应类型 - 完善 i18n 配置：5 种语言（EN、ZH-CN、ZH-TW、JA、RU）共 100 个翻译条目 ## 技术实现 - 架构：统一端点设计（/api/chat/completions） - 客户端：GlmMcpClient 类，完整错误处理和日志 - 处理器：handleGlmToolCall() 方法，参数验证 - 类型安全：TypeScript 严格类型检查 ## 支持的工具 MiniMax (2个): • web_search - 联网搜索 • understand_image - 图片理解 GLM (2个): • analyze_image - 图片分析 ⭐ 新增 • analyze_video - 视频分析 ⭐ 新增 ## 文件变更 新增文件: • src/lib/mcp/glm-client.ts (169 行) • src/lib/mcp/types.ts (扩展 +30 行) • src/lib/mcp/minimax-client.ts (从 commit 37d8665 恢复) • src/app/v1/_lib/proxy/mcp-passthrough-handler.ts (扩展 +70 行) 修改文件: • messages/*/settings.json (5 个语言文件，各 20 个 MCP 配置项) ## 验证结果 ✓ TypeScript 语法检查通过 ✓ JSON 格式验证通过 ✓ i18n 配置验证通过（5 种语言） ✓ GLM 描述已从 "reserved" 更新为实际功能 Related: WFS-mcp-passthrough-feature (IMPL-001 ~ IMPL-009) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Resolve merge conflicts and integrate GLM MCP functionality - Maintain MiniMax MCP support (web_search, understand_image) - Add GLM MCP support (analyze_image, analyze_video) - Update i18n configurations for 5 languages Files updated: • src/lib/mcp/types.ts - Added GLM type definitions • src/app/v1/_lib/proxy/mcp-passthrough-handler.ts - Added GLM handler • messages/*/settings.json - Updated i18n for GLM support 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…-code-hub into feature/mcp-passthrough # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
…-code-hub into feature/mcp-passthrough
- Update GLM MCP status in settings to 'implemented' - Add runtime type validation for MCP tool inputs - Implement timeouts (30s) for Minimax and GLM clients - Add SSRF protection for MCP passthrough URLs - Improve error handling for network issues and URL extraction - Refactor standard endpoints constant
feat(mcp): 实现 MCP 透传功能
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant new capability by integrating external AI services for specialized tool calls. It enables the system to leverage third-party providers for advanced functionalities like web search and image analysis, enriching the overall AI interaction experience. The changes span database schema, user interface, and core proxy logic, ensuring a robust and configurable solution for extending AI provider capabilities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the MCP (Model Context Protocol) passthrough feature, allowing requests to be routed to alternative AI providers like Minimax and GLM for capabilities like image recognition and web search. The changes include database schema updates, new API endpoints and handlers on the backend, and a new configuration section in the provider settings UI.
The implementation is comprehensive, but I've identified a few areas for improvement:
- There appears to be a significant amount of dead code related to MCP handling that isn't integrated.
- SSRF protection logic is inconsistent across the codebase.
- There are opportunities to reduce code duplication by using newly created utility functions.
- Some new client implementations have placeholder
TODOs for error handling that should be addressed.
Overall, this is a great new feature, and addressing these points will improve its security, maintainability, and robustness.
| const [, a, b] = ipv4Match.map(Number); | ||
| // 私有 IP 范围 | ||
| if (a === 127) return true; // 127.0.0.0/8 (loopback range) | ||
| if (a === 10) return true; // 10.0.0.0/8 |
There was a problem hiding this comment.
The SSRF protection logic for IPv4 addresses is incomplete. It correctly checks for 127.0.0.0/8 and 10.0.0.0/8, but it's missing checks for other private IP ranges like 192.168.0.0/16 and 172.16.0.0/12. A more comprehensive implementation of this check has been added in src/lib/validation/schemas.ts for the mcp_passthrough_url. To ensure consistent and robust security, I recommend completing this logic.
| const [, a, b] = ipv4Match.map(Number); | |
| // 私有 IP 范围 | |
| if (a === 127) return true; // 127.0.0.0/8 (loopback range) | |
| if (a === 10) return true; // 10.0.0.0/8 | |
| const [, a, b] = ipv4Match.map(Number); | |
| // 私有 IP 范围 | |
| if (a === 127) return true; // 127.0.0.0/8 (loopback range) | |
| if (a === 10) return true; // 10.0.0.0/8 | |
| if (a === 192 && b === 168) return true; // 192.168.0.0/16 | |
| if (a === 172 && b >= 16 && b <= 31) return true; // 172.16.0.0/12 |
| @@ -0,0 +1,319 @@ | |||
| /** | |||
There was a problem hiding this comment.
This file, along with glm-client.ts and minimax-client.ts, appears to be unused in the current pull request. The logic in forwarder.ts only seems to passthrough requests to a different base URL for MCP, but doesn't use this handler to intercept and process tool calls. This introduces a significant amount of dead code. Was this intended for a future change? If so, it might be better to introduce it in a separate PR when it's actually integrated. If it's part of this feature, the integration seems to be missing.
| const baseUrlObj = new URL(provider.url); | ||
| effectiveBaseUrl = `${baseUrlObj.protocol}//${baseUrlObj.host}`; |
There was a problem hiding this comment.
This can be simplified by using the origin property of the URL object, which is a standard and more concise way to get the protocol, hostname, and port. This is also consistent with the implementation of the new extractBaseUrl utility function.
| const baseUrlObj = new URL(provider.url); | |
| effectiveBaseUrl = `${baseUrlObj.protocol}//${baseUrlObj.host}`; | |
| const baseUrlObj = new URL(provider.url); | |
| effectiveBaseUrl = baseUrlObj.origin; |
|
|
||
| const data = (await response.json()) as T; | ||
|
|
||
| // TODO: Implement GLM-specific error handling based on API docs |
There was a problem hiding this comment.
This TODO notes that GLM-specific error handling is missing. This is important for production robustness. While this might be planned for later, it's worth highlighting that without it, errors from the GLM API might not be handled gracefully, potentially leading to generic error messages that are hard to debug. It would be great to add this before the feature is considered complete.
📊 PR Size AnalysisThis PR is XL with ~1,345 lines changed across 25 files (excluding generated files). Large PRs are harder to review and more likely to introduce bugs. 🔀 Suggested Split:Based on the changes, this PR could be split into 3 smaller PRs: PR 1: Database Schema & Core Types (~100 lines)Establish the foundation for MCP passthrough feature. Files:
PR 2: MCP Client Implementations (~500 lines)Implement MCP clients after foundation is merged. Files:
PR 3: UI Integration & Translations (~700 lines)Add UI and translations after backend is ready. Files:
Why Split?✅ Easier to review - Each PR has a clear, focused purpose Dependencies:PR1 → PR2 → PR3 (sequential merge order) 🤖 Automated analysis by Claude AI |
| if (hostname === "localhost" || hostname === "127.0.0.1" || hostname === "::1") | ||
| return false; | ||
| // Block private IP ranges | ||
| // 10.0.0.0/8 |
There was a problem hiding this comment.
🟠 SSRF Bypass Vulnerability: SSRF validation can be bypassed using DNS rebinding, URL obfuscation, or IPv6 representations
Security Impact: An attacker could bypass the SSRF protection to access internal services, cloud metadata APIs (e.g., 169.254.169.254), or other protected resources.
Attack Scenarios:
- DNS Rebinding: Configure a malicious domain that first resolves to an external IP (to pass validation) then rebinds to an internal IP (during actual request)
- Octal/Hex IP encoding: Use
http://0177.0.0.1(octal) orhttp://0x7f.0.0.1(hex) for 127.0.0.1 - IPv6 bypass: Use IPv6 addresses like
::ffff:127.0.0.1or[::] - Cloud metadata bypass:
http://169.254.169.254for AWS/GCP metadata access is not fully blocked
Remediation:
.refine(
(url) => {
try {
const parsed = new URL(url);
const hostname = parsed.hostname;
// Block localhost variations
if (hostname === "localhost" ||
hostname === "127.0.0.1" ||
hostname === "::1" ||
hostname === "0.0.0.0" ||
hostname.match(/^0+\\./) || // octal 0 prefix
hostname.includes("0x")) // hex notation
return false;
// Block IPv6 loopback and link-local
if (hostname.startsWith("[") ||
hostname.includes("::") ||
hostname.includes("ffff:"))
return false;
// Block cloud metadata endpoints
if (hostname === "169.254.169.254" ||
hostname.startsWith("metadata.") ||
hostname.endsWith(".internal"))
return false;
// Block ALL private IP ranges
if (hostname.startsWith("10.")) return false;
if (hostname.startsWith("192.168.")) return false;
if (hostname.match(/^172\\.(1[6-9]|2[0-9]|3[0-1])\\./)) return false;
if (hostname.startsWith("169.254.")) return false;
// Additional: Resolve hostname at validation time
// to prevent DNS rebinding (requires async validation)
return true;
} catch {
return false;
}
},
{ message: "不允许使用内部网络地址 (SSRF Protection)" }
)References:
| effectiveBaseUrl = `${baseUrlObj.protocol}//${baseUrlObj.host}`; | ||
| logger.debug("ProxyForwarder: Extracted base domain for MCP passthrough", { | ||
| providerId: provider.id, | ||
| providerName: provider.name, |
There was a problem hiding this comment.
🟠 SSRF via Fallback Path: When mcpPassthroughUrl is not configured, the system extracts base URL from provider.url without applying SSRF protections
Security Impact: If a provider URL is set to an internal network address (which may pass different validation or be configured through other means), the MCP passthrough feature could be exploited to access internal services.
Attack Scenario:
- Attacker sets provider URL to a whitelisted API endpoint that happens to be on an internal network (e.g., via admin access or data manipulation)
- MCP passthrough extracts base domain from provider.url
- Attacker sends MCP requests that are forwarded to internal services
Remediation:
} else {
// 自动从 provider.url 提取基础域名（去掉路径部分）
try {
const baseUrlObj = new URL(provider.url);
effectiveBaseUrl = `${baseUrlObj.protocol}//${baseUrlObj.host}`;
// ⚠️ Apply same SSRF validation to extracted base URL
if (\!isExternalUrl(effectiveBaseUrl)) {
throw new ProxyError("Internal network URLs are not allowed for MCP passthrough", 400);
}
logger.debug("ProxyForwarder: Extracted base domain for MCP passthrough", {
...
});
} catch (error) {
...
}
}
// Add helper function:
function isExternalUrl(url: string): boolean {
const parsed = new URL(url);
const hostname = parsed.hostname;
// Apply same blocklist from validation schema
return \!(
hostname === "localhost" ||
hostname === "127.0.0.1" ||
hostname.startsWith("10.") ||
hostname.startsWith("192.168.") ||
hostname.match(/^172\\.(1[6-9]|2[0-9]|3[0-1])\\./)
);
}References:
| // 使用配置的 MCP URL | ||
| effectiveBaseUrl = provider.mcpPassthroughUrl; | ||
| logger.debug("ProxyForwarder: Using configured MCP passthrough URL", { | ||
| providerId: provider.id, |
There was a problem hiding this comment.
🟡 Information Disclosure: Sensitive URLs logged in debug mode
Security Impact: When debug logging is enabled, API keys embedded in URLs or sensitive endpoint information could be leaked to log files.
Current Issue:
logger.debug("ProxyForwarder: Using configured MCP passthrough URL", {
...
configuredUrl: provider.mcpPassthroughUrl, // May contain sensitive info
...
});Remediation:
logger.debug("ProxyForwarder: Using configured MCP passthrough URL", {
providerId: provider.id,
providerName: provider.name,
mcpType: provider.mcpPassthroughType,
configuredUrlHost: new URL(provider.mcpPassthroughUrl).hostname, // Log only hostname
requestPath,
});References:
| baseUrl: provider.url, | ||
| apiKey: provider.key, | ||
| }; | ||
| const client = new MinimaxMcpClient(config); |
There was a problem hiding this comment.
🟡 Missing Input Validation on MCP Tool Inputs: Tool call inputs from toolCall.input are used directly without sanitization
Security Impact: While the code validates that query, imageUrl, etc. are non-empty strings, there is no validation of the content itself. Malicious inputs could potentially:
- Cause unexpected behavior in the MCP service
- Be used for prompt injection attacks against the AI service
- Contain excessively long strings causing DoS
Current Code:
const query = toolCall.input.query;
if (\!query || typeof query \!== "string") {
throw new McpError("Invalid parameter: query must be a non-empty string");
}
const response = await client.webSearch(query); // No length/content validationRemediation:
const query = toolCall.input.query;
if (\!query || typeof query \!== "string") {
throw new McpError("Invalid parameter: query must be a non-empty string");
}
// Add input validation
const MAX_QUERY_LENGTH = 2000;
if (query.length > MAX_QUERY_LENGTH) {
throw new McpError(`Query too long: max ${MAX_QUERY_LENGTH} characters allowed`);
}
// Sanitize to prevent potential injection
const sanitizedQuery = query.trim().substring(0, MAX_QUERY_LENGTH);
const response = await client.webSearch(sanitizedQuery);References:
| throw new McpRequestError("Image source is required"); | ||
| } | ||
| if (!prompt) { | ||
| throw new McpRequestError("Prompt is required"); |
There was a problem hiding this comment.
🟡 Missing URL Validation for Image/Video Sources: URLs passed to analyzeImage and analyzeVideo are not validated for SSRF
Security Impact: The image_source and video_source parameters accept URLs that could point to internal resources, enabling SSRF attacks against the GLM service.
Attack Scenario:
- Attacker sends a tool call with
image_source: "http://169.254.169.254/latest/meta-data/" - The GLM service fetches this URL, potentially exposing cloud metadata
- Even if GLM does not return the content, it confirms the resource exists (timing attack)
Remediation:
async analyzeImage(imageSource: string, prompt: string): Promise<McpGlmImageAnalyzeResponse> {
if (\!imageSource) {
throw new McpRequestError("Image source is required");
}
// Validate URL if it looks like a URL
if (imageSource.startsWith("http://") || imageSource.startsWith("https://")) {
const url = new URL(imageSource);
// Block internal networks
if (this.isInternalUrl(url.hostname)) {
throw new McpRequestError("Internal network URLs are not allowed");
}
}
// ... rest of the code
}
private isInternalUrl(hostname: string): boolean {
return (
hostname === "localhost" ||
hostname === "127.0.0.1" ||
hostname.startsWith("10.") ||
hostname.startsWith("192.168.") ||
hostname.match(/^172\\.(1[6-9]|2[0-9]|3[0-1])\\./) \!== null ||
hostname === "169.254.169.254"
);
}References:
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📋 Code Review Summary
This PR adds MCP (Model Context Protocol) passthrough configuration to the provider management system, allowing requests to be forwarded to third-party AI services like Minimax and GLM. However, the implementation is incomplete - the UI and database schema are added, but the core functionality to intercept and transform tool calls is not integrated into the request/response pipeline.
🔍 Issues Found
- Critical (🔴): 1 issue
- High (🟠): 3 issues
- Medium (🟡): 4 issues
- Low (🟢): 1 issue
🎯 Priority Actions
-
🔴 CRITICAL: The MCP feature is incomplete. Users can configure MCP settings in the UI, but nothing will happen when they try to use it. Three new files (
mcp-passthrough-handler.ts,glm-client.ts,minimax-client.ts) are created but never imported or called anywhere in the codebase.Recommendation: Either remove the UI/schema changes until the feature is fully implemented, or add a clear warning in the UI: "MCP 透传功能正在开发中，当前版本暂不可用".
-
🟠 HIGH: The URL switching logic in
forwarder.ts:2142doesn't actually achieve MCP passthrough. It only changes the base URL for non-standard endpoints, but MCP passthrough requires intercepting Claude'stool_usemessages and transforming them to third-party API formats.Evidence:
McpPassthroughHandleris designed to intercept tool calls, but the code inforwarder.tsjust switches URLs without calling the handler. -
🟠 HIGH: SSRF validation code is duplicated between
CreateProviderSchemaandUpdateProviderSchema(lines 3142-3208). This creates a maintenance burden and increases the risk of security inconsistencies.Recommendation: Extract the validation to a reusable
isInternalUrl()function (which already exists insrc/actions/notifications.ts:25with slightly different logic - consider consolidating). -
🟠 HIGH: Hardcoded 30-second timeout in MCP clients (
glm-client.ts:2723,minimax-client.ts:2892) may cause false timeouts for slow AI services (especially image/video analysis).Recommendation: Use provider's configured
requestTimeoutNonStreamingMsinstead of hardcoded values. -
🟡 MEDIUM: Missing validation for
mcp_passthrough_urlwhenmcp_passthrough_typeis not 'none'. The schema allows MCP to be "enabled" but without a configured URL, relying on implicit fallback behavior.
💡 General Observations
Dead Code Pattern: The PR introduces three new implementation files that are never used:
src/app/v1/_lib/proxy/mcp-passthrough-handler.ts(319 lines)src/lib/mcp/glm-client.ts(176 lines)src/lib/mcp/minimax-client.ts(187 lines)src/lib/mcp/types.ts(112 lines)
Total: 794 lines of unused code that will confuse future maintainers and may trigger lint warnings.
Architecture Issue: The MCP passthrough design appears to be incomplete. Based on the McpPassthroughHandler implementation, the intended flow should be:
- Intercept Claude API responses containing
tool_useblocks - Extract tool name and parameters
- Call third-party API using their native format
- Transform response back to Claude's
tool_resultformat
However, the current implementation in forwarder.ts only switches base URLs, which won't work for most third-party APIs that don't share Claude's endpoint structure.
GLM API Concerns: The GLM client uses /api/chat/completions for both image and video analysis, which appears to be a placeholder (comment says "模拟 GLM MCP 工具的调用方式"). This will likely fail when actually called.
Minor Issues:
- Missing newline at end of SQL migration file (POSIX standard violation)
- Unsafe error object access in multiple places (
error instanceof Error ? error.message : String(error)) - Unused import cleanup (
loggerinv1beta/[...route]/route.ts,CardDescriptionin several components)
🤖 Automated review by Claude AI - focused on identifying issues for improvement
| */ | ||
| async understandImage(imageUrl: string, prompt: string): Promise<McpImageUnderstandResponse> { | ||
| if (!imageUrl) { | ||
| throw new McpRequestError("Image URL is required"); |
There was a problem hiding this comment.
🟡 Missing URL Validation for Image URL: The image_url parameter passed to understandImage is not validated for SSRF
Security Impact: The image_url parameter accepts any URL that could point to internal resources, enabling SSRF attacks through the Minimax service.
Attack Scenario:
- Attacker sends a tool call with
image_url: "http://192.168.1.1/admin" - Minimax service attempts to fetch this internal resource
- Response may leak internal service information or cause the internal service to perform actions
Remediation:
async understandImage(imageUrl: string, prompt: string): Promise<McpImageUnderstandResponse> {
if (!imageUrl) {
throw new McpRequestError("Image URL is required");
}
// Validate URL to prevent SSRF
try {
const url = new URL(imageUrl);
if (this.isInternalUrl(url.hostname)) {
throw new McpRequestError("Internal network URLs are not allowed for image analysis");
}
// Only allow http/https protocols
if (!["http:", "https:"].includes(url.protocol)) {
throw new McpRequestError("Only HTTP/HTTPS URLs are allowed");
}
} catch (e) {
if (e instanceof McpRequestError) throw e;
throw new McpRequestError("Invalid image URL format");
}
// ... rest of the code
}References:
🔒 Security Scan Results🚨 Vulnerabilities Found
⚡ Immediate Actions Required
📋 OWASP Top 10 Coverage
📝 Detailed Findings1. SSRF Protection Gaps (🟠 High)Files affected:
Issue: While SSRF protection exists for
Impact: Attackers could access internal services, cloud metadata APIs, or other protected resources. 2. Missing Input Length Validation (🟡 Medium)File: Issue: MCP tool call inputs ( 3. Information Disclosure in Logs (🟡 Medium)File: Issue: Full URLs including potentially sensitive information are logged in debug mode. 🛡️ Security PostureAssessment: Needs Improvement The PR introduces new external service integration (MCP passthrough) which expands the attack surface. While basic SSRF protection is implemented at the configuration level, there are multiple bypass vectors and the protection is not consistently applied across all URL-consuming code paths. ✅ What's Done Well
📌 Recommendations
🤖 Automated security scan by Claude AI - OWASP Top 10 & CWE coverage |
📊 PR Size AnalysisThis PR is XL with 1,341 effective lines changed across 24 files (excluding generated snapshot files). Large PRs are harder to review and more likely to introduce bugs. 🔀 Suggested Split:Based on the changes, this PR could be split into smaller, focused PRs: PR 1: Database Schema & Repository Layer (~50 lines, 4 files)Foundation changes that other PRs depend on:
PR 2: MCP Core Infrastructure (~475 lines, 4 files)Core MCP implementation without vendor-specific clients:
PR 3: MiniMax Client (~250 lines, 3 files)MiniMax-specific implementation:
PR 4: GLM Client (~200 lines, 1 file)GLM-specific implementation:
PR 5: UI & Internationalization (~330 lines, 11 files)Frontend changes that depend on backend:
📝 Recommended Merge Order:
✨ Why Split?
🚀 Alternative Quick Split (3 PRs):If time is critical, minimum split could be:
🤖 Automated analysis by Claude AI |
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📋 Code Review Summary
This PR implements MCP (Model Context Protocol) passthrough functionality to enable tool calling capabilities from third-party AI providers like MiniMax and GLM. The implementation is comprehensive but has several critical security and reliability issues that must be addressed before merge.
🔍 Issues Found
- Critical (🔴): 3 issues
- High (🟠): 4 issues
- Medium (🟡): 2 issues
- Low (🟢): 0 issues
🎯 Priority Actions
- Add input validation for object before accessing properties (prevents TypeError crashes)
- Implement circuit breaker integration for MCP client failures to prevent cascading failures
- Add rate limiting for MCP API calls to prevent quota exhaustion
- Fix missing newline in SQL migration file (causes migration failures)
- Remove hardcoded 30s timeout and make it configurable per provider
💡 General Observations
The MCP passthrough architecture is well-designed with clear separation of concerns. However, the implementation lacks defensive programming practices for production reliability:
- No circuit breaker protection for third-party API calls
- Missing rate limiting for MCP endpoints
- Insufficient input validation could lead to runtime crashes
- Hardcoded timeouts reduce flexibility
🤖 Automated review by Claude AI - focused on identifying issues for improvement
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📋 Code Review Summary
This PR implements MCP (Model Context Protocol) passthrough functionality to enable tool calling capabilities from third-party AI providers like MiniMax and GLM. The implementation is comprehensive but has several critical security and reliability issues that must be addressed before merge.
🔍 Issues Found
- Critical (🔴): 3 issues
- High (🟠): 4 issues
- Medium (🟡): 2 issues
- Low (🟢): 0 issues
🎯 Priority Actions
- Add input validation for toolCall.input object before accessing properties (prevents TypeError crashes)
- Implement circuit breaker integration for MCP client failures to prevent cascading failures
- Add rate limiting for MCP API calls to prevent quota exhaustion
- Fix missing newline in SQL migration file (causes migration failures)
- Remove hardcoded 30s timeout and make it configurable per provider
💡 General Observations
The MCP passthrough architecture is well-designed with clear separation of concerns. However, the implementation lacks defensive programming practices for production reliability:
- No circuit breaker protection for third-party API calls
- Missing rate limiting for MCP endpoints
- Insufficient input validation could lead to runtime crashes
- Hardcoded timeouts reduce flexibility
🤖 Automated review by Claude AI - focused on identifying issues for improvement
|
File: 🔴 Critical: Missing Input Validation for Tool Parameters Why this is a problem: The code extracts query, image_url, prompt etc. from toolCall.input without validating the object structure first. If toolCall.input is null/undefined or not an object, accessing properties will throw a TypeError and crash the handler. Suggested fix: case "web_search": {
// Add input validation first
if (!toolCall.input || typeof toolCall.input !== 'object') {
throw new McpError("Invalid tool call: input must be an object");
}
const query = toolCall.input.query;
if (!query || typeof query !== "string") {
throw new McpError("Invalid parameter: query must be a non-empty string");
}
// ... rest of the code
} |
|
File: 🔴 Critical: Missing Newline at End of SQL Migration File Why this is a problem: The SQL migration file lacks a newline at the end (line 2), which violates POSIX standards and can cause issues with:
Suggested fix: ALTER TABLE "providers" ADD COLUMN "mcp_passthrough_type" varchar(20) DEFAULT 'none' NOT NULL;
ALTER TABLE "providers" ADD COLUMN "mcp_passthrough_url" varchar(512);
(Add a blank line at the end of the file) |
|
File: 🟠 High: No Circuit Breaker Protection for MCP API Calls Why this is a problem: The MCP clients make direct HTTP calls to third-party APIs without circuit breaker protection. If a third-party service becomes unavailable or slow, it can cause cascading failures affecting all requests, not just MCP tool calls. The existing system has circuit breaker protection for provider API calls (see src/lib/circuit-breaker.ts), but MCP calls bypass this protection. Suggested fix: // In mcp-passthrough-handler.ts
private static async handleMinimaxToolCall(
provider: Provider,
toolCall: McpToolCall
): Promise<McpToolResponse> {
// Check circuit breaker before making MCP call
const { isCircuitOpen } = await import("@/lib/circuit-breaker");
const mcpCircuitKey = `mcp-${provider.mcpPassthroughType}-${provider.id}`;
if (await isCircuitOpen(mcpCircuitKey)) {
throw new McpError("MCP service circuit breaker is open (too many failures)");
}
try {
// Create client and make call
const client = new MinimaxMcpClient(config);
const result = await client.webSearch(query);
// Record success
const { recordSuccess } = await import("@/lib/circuit-breaker");
await recordSuccess(mcpCircuitKey);
return result;
} catch (error) {
// Record failure
const { recordFailure } = await import("@/lib/circuit-breaker");
await recordFailure(mcpCircuitKey, error as Error);
throw error;
}
} |
|
File: 🟠 High: Hardcoded 30-Second Timeout Reduces Flexibility Why this is a problem: Both MCP clients use a hardcoded 30-second timeout for all API calls. Different tools may require different timeouts:
The main proxy system uses configurable timeouts per provider (first_byte_timeout_streaming_ms, etc.), but MCP calls ignore these settings. Suggested fix: export class MinimaxMcpClient {
private baseUrl: string;
private apiKey: string;
private timeout: number; // Add timeout field
constructor(config: McpClientConfig, timeout: number = 30000) {
this.baseUrl = config.baseUrl;
this.apiKey = config.apiKey;
this.timeout = timeout;
}
private async makeRequest<T>(endpoint: string, payload: unknown): Promise<T> {
const url = `${this.baseUrl}${endpoint}`;
const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), this.timeout); // Use configurable timeout
// ... rest of the code
}
}
// In mcp-passthrough-handler.ts
const client = new MinimaxMcpClient(
config,
provider.request_timeout_non_streaming_ms || 30000 // Use provider's timeout setting
); |
|
File: 🟠 High: Missing Rate Limiting for MCP API Calls Why this is a problem: MCP API calls to third-party services (MiniMax, GLM) are not rate-limited. This can lead to:
The main proxy system has comprehensive rate limiting (see src/lib/rate-limit/service.ts), but MCP calls bypass all limits. Suggested fix: // In mcp-passthrough-handler.ts
static async handleToolCall(provider: Provider, toolCall: McpToolCall): Promise<McpToolResponse> {
logger.info("[McpPassthroughHandler] Handling tool call", {
providerId: provider.id,
toolName: toolCall.name,
});
// Add rate limiting check for MCP calls
const { RateLimitService } = await import("@/lib/rate-limit");
const rateLimitKey = `mcp-${provider.mcpPassthroughType}-${provider.id}`;
// Check RPM limit for MCP endpoints
const rateLimitResult = await RateLimitService.checkRateLimit(
rateLimitKey,
provider.rpm || 60 // Use provider's RPM or default to 60
);
if (!rateLimitResult.allowed) {
throw new McpError(
`MCP rate limit exceeded: ${rateLimitResult.remaining}/${rateLimitResult.limit} requests remaining`
);
}
try {
// ... existing tool call logic
} catch (error) {
// ... existing error handling
}
}Alternative: Track MCP costs separately using |
|
File: 🟠 High: MCP Tool Call Extraction Happens Too Late in Request Pipeline Why this is a problem: Based on the PR description, MCP tool call detection happens during response stream processing. This means:
This is actually correct behavior for the stated use case (intercepting Claude's tool_use responses), but the implementation should explicitly document this flow to avoid confusion. Recommendation: Add detailed documentation explaining the request/response flow: /**
* MCP Passthrough Flow:
*
* 1. Client sends request to proxy
* 2. Proxy forwards request to upstream Claude API
* 3. Claude responds with tool_use events in the stream
* 4. McpPassthroughHandler detects tool_use events
* 5. MCP client calls third-party API (MiniMax/GLM)
* 6. tool_result events are injected back into response stream
* 7. Client receives combined response
*
* Note: This adds latency since MCP calls are made serially during streaming.
* Consider implementing parallel tool execution if multiple tools are called.
*/
export class McpPassthroughHandler {
// ... existing implementation
}This is not a bug, but clarity is critical for maintainability. |
|
File: 🟡 Medium: TODO Comment Indicates Incomplete Implementation Why this is a problem: The GLM client has a TODO comment stating "Implement GLM-specific error handling based on API docs". This means:
Recommendation: Either:
// TODO: Implement GLM-specific error handling based on API docs
// Known error codes to handle:
// - 1004: Authentication error (invalid API key)
// - 2001: Rate limit exceeded
// - 2002: Quota exhausted
// - 4001: Invalid request format
// - 5001: Internal server error
// See: https://open.bigmodel.cn/dev/api/error-codes (replace with actual docs URL)
// Issue: #XXX (if tracking in GitHub issues)
logger.debug("[GlmMcpClient] API response", { endpoint, data });This at least gives future developers a starting point for implementation. |
|
File: 🔴 Critical: HTTP Status Code Not Checked Before JSON Parsing Why this is a problem: The code throws an error if response.ok is false, but then continues to parse JSON from the response body. For non-OK responses (4xx, 5xx), the response body might not be valid JSON, causing JSON.parse() to throw a SyntaxError that masks the actual HTTP error. Suggested fix: if (!response.ok) {
// Try to extract error message from response body if available
let errorBody = "";
try {
errorBody = await response.text();
} catch {
// Ignore text extraction errors
}
const errorMsg = errorBody ? ` - ${errorBody}` : "";
throw new McpRequestError(
`HTTP ${response.status}: ${response.statusText}${errorMsg}`,
response.status,
response.headers.get("Trace-Id") ?? undefined
);
}
const data = (await response.json()) as T;This ensures the HTTP error is thrown BEFORE attempting JSON parsing, providing clearer error messages for debugging. |
|
File: src/app/v1/_lib/proxy/mcp-passthrough-handler.ts (Lines ~145-180 in web_search and understand_image handlers) 🟡 Medium: Error Responses Lose Type Information Why this is a problem: When an MCP tool call fails, the error is converted to a string for the tool_result content. This loses valuable debugging information like stack traces, error codes, and structured error data. Suggested fix: Return structured error information instead of a plain string. Replace the catch block error response with structured JSON containing error details, statusCode, traceId, tool name, and timestamp. This provides information that Claude can potentially use to retry or handle errors more intelligently. |
🔒 Security Scan Results🚨 Vulnerabilities Found
⚡ Immediate Actions RequiredThe following critical and high severity issues MUST be fixed before merge: 🔴 Critical: SSRF Protection Bypass (src/lib/validation/schemas.ts:201-223)Vulnerability: Incomplete IPv6 and special hostname blocking in MCP passthrough URL validation Security Impact: Attackers can bypass SSRF protection using:
Attack Scenario:
Proof of Concept: # These bypass current validation:
http://[0:0:0:0:0:0:0:1]:6379 # IPv6 localhost (expanded)
http://[::ffff:127.0.0.1]:5432 # IPv4-mapped IPv6 to PostgreSQL
http://0x7f000001:27017 # Hex-encoded to MongoDB
http://127.0.0.2:22 # Alternative localhost to SSH
http://[fc00::1]:8080 # IPv6 ULA (private range)Remediation: // src/lib/validation/schemas.ts - Replace existing refine() at lines 199-223
.refine(
(url) => {
try {
const parsed = new URL(url);
const hostname = parsed.hostname.toLowerCase();
// Block localhost variations
if (
hostname === 'localhost' ||
hostname === '0.0.0.0' ||
/^127\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(hostname) || // 127.x.x.x
hostname === '::1' ||
hostname === '::ffff:127.0.0.1' ||
hostname === '0:0:0:0:0:0:0:1' ||
/^0x7f[0-9a-f]{6}$/.test(hostname) // Hex-encoded 127.x.x.x
) {
return false;
}
// Block private IPv4 ranges
if (/^10\./.test(hostname)) return false; // 10.0.0.0/8
if (/^192\.168\./.test(hostname)) return false; // 192.168.0.0/16
if (/^172\.(1[6-9]|2[0-9]|3[0-1])\./.test(hostname)) return false; // 172.16.0.0/12
if (/^169\.254\./.test(hostname)) return false; // 169.254.0.0/16
// Block IPv6 private ranges
if (hostname.includes(':')) {
// fc00::/7 and fd00::/8 (ULA - Unique Local Address)
if (hostname.startsWith('fc') || hostname.startsWith('fd')) return false;
// fe80::/10 (link-local)
if (/^fe[89ab]/.test(hostname)) return false;
}
return true;
} catch {
return false;
}
},
{ message: "不允许使用内部网络地址 (SSRF Protection)" }
)Additional Recommendations:
References: 🟠 High: Sensitive Data Exposure in Error Messages (src/lib/mcp/*.ts)Vulnerability: API keys and internal URLs exposed in error logs and responses Security Impact: API keys could be leaked through:
Affected Code:
Attack Scenario:
Remediation: // src/lib/mcp/minimax-client.ts
import { maskSensitiveUrl } from '@/lib/utils/security'; // Create this utility
async webSearch(query: string): Promise<McpWebSearchResponse> {
// Sanitize query before logging
const sanitizedQuery = query.length > 100 ? query.substring(0, 100) + '...' : query;
logger.info('[MinimaxMcpClient] webSearch', {
query: sanitizedQuery,
queryLength: query.length
});
// ... rest of code
}
async understandImage(imageUrl: string, prompt: string): Promise<McpImageUnderstandResponse> {
// Mask URL parameters that may contain tokens
const maskedUrl = maskSensitiveUrl(imageUrl);
logger.info('[MinimaxMcpClient] understandImage', {
imageUrl: maskedUrl,
promptLength: prompt.length
});
// ... rest of code
}
// Utility function
export function maskSensitiveUrl(url: string): string {
try {
const parsed = new URL(url);
// Remove all query parameters (may contain tokens)
parsed.search = parsed.search ? '?[REDACTED]' : '';
return parsed.toString();
} catch {
return '[INVALID_URL]';
}
}// src/app/v1/_lib/proxy/mcp-passthrough-handler.ts:118
// Don't expose raw error messages to clients
return {
type: 'tool_result',
tool_use_id: toolCall.id,
content: 'MCP tool call failed due to an internal error. Please contact support.', // Generic message
is_error: true,
};References: 🟡 Medium: Insufficient Input Validation (src/app/v1/_lib/proxy/mcp-passthrough-handler.ts)Vulnerability: Type coercion vulnerability - input validation checks type but doesn't validate content structure Security Impact: Malformed inputs could cause:
Affected Code: // Lines 145-147, 167-172, 219-224, 245-250
if (\!query || typeof query \!== 'string') {
throw new McpError('Invalid parameter: query must be a non-empty string');
}
// No validation of query content (length, special characters, etc.)Attack Scenario:
Remediation: // src/app/v1/_lib/proxy/mcp-passthrough-handler.ts
private static validateString(
value: unknown,
fieldName: string,
maxLength: number = 10000
): string {
if (\!value || typeof value \!== 'string') {
throw new McpError(`Invalid parameter: ${fieldName} must be a non-empty string`);
}
const trimmed = value.trim();
if (trimmed.length === 0) {
throw new McpError(`Invalid parameter: ${fieldName} cannot be empty`);
}
if (trimmed.length > maxLength) {
throw new McpError(`Invalid parameter: ${fieldName} exceeds maximum length of ${maxLength}`);
}
return trimmed;
}
// Usage:
case 'web_search': {
const query = this.validateString(toolCall.input.query, 'query', 500);
const response = await client.webSearch(query);
// ...
}
case 'understand_image': {
const imageUrl = this.validateString(toolCall.input.image_url, 'image_url', 2048);
const prompt = this.validateString(toolCall.input.prompt, 'prompt', 1000);
// Validate URL format
try {
new URL(imageUrl);
} catch {
throw new McpError('Invalid parameter: image_url must be a valid URL');
}
const response = await client.understandImage(imageUrl, prompt);
// ...
}References: 📋 OWASP Top 10 Coverage
🛡️ Security PostureOverall Assessment: Critical Concerns This PR introduces MCP passthrough functionality with 1 critical SSRF vulnerability that could allow attackers to access internal services. The SSRF protection is incomplete and can be bypassed using IPv6 addresses and alternative IP representations. Additionally, sensitive data exposure through error messages and insufficient input validation present high and medium severity risks. Recommendation: DO NOT MERGE until critical SSRF bypass is fixed and high severity issues are addressed. ✅ Positive Security Practices Observed
🤖 Automated security scan by Claude AI - OWASP Top 10 & CWE coverage |
2 participants
Summary
添加 MCP (Model Context Protocol) 透传功能，允许将 Claude Code 的工具调用请求自动转发到第三方 AI 服务商（如 MiniMax、智谱 GLM），实现工具调用能力的扩展和补充。
Problem
Claude Code 本身支持多种工具调用（tool_use），但某些特定功能（如 web_search、understand_image 等）可能需要依赖第三方 AI 服务商提供的能力。当前系统缺乏统一的机制来：
Solution
本 PR 实现了一套完整的 MCP 透传机制：
核心架构
供应商级别配置
mcp_passthrough_type（透传类型）和mcp_passthrough_url（自定义 URL）none（禁用）、minimax（MiniMax）、glm（智谱 GLM）、custom（自定义）MCP 透传处理器 (
src/app/v1/_lib/proxy/mcp-passthrough-handler.ts)tool_use事件）tool_result事件注入响应流第三方服务客户端
src/lib/mcp/minimax-client.ts)web_search（网页搜索）、understand_image（图片理解）src/lib/mcp/glm-client.ts)analyze_image（图片分析）、analyze_video（视频分析）UI 增强
custom类型时显示 URL 输入框工作流程
支持的工具
MiniMax:
web_search- 网页搜索（返回搜索结果和网页内容）understand_image- 图片理解（分析图片内容）智谱 GLM:
analyze_image- 图片分析analyze_video- 视频分析Changes
数据库层
0024_glossy_silvermane.sqlproviders.mcp_passthrough_type和providers.mcp_passthrough_url字段核心逻辑
McpPassthroughHandler主处理器MinimaxMcpClient客户端GlmMcpClient客户端src/lib/mcp/types.ts)response-handler.ts)UI 和国际化
usage-doc/page.tsx）错误处理和日志
McpError错误类型Testing
web_search参数验证和响应格式化understand_image图片 URL 验证analyze_image多图片支持analyze_video视频 URL 处理none，向后兼容）Screenshots
暂无 UI 截图（建议管理员在供应商管理页面查看新增的"MCP 透传配置"部分）
Related Issues
Closes #（暂无关联 issue）
Notes
mcp_passthrough_type = 'none'）custom）预留用于未来扩展