feat(mcp): 实现 MCP 透传功能#157
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)
Summary of ChangesHello @flintttan, 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! 此拉取请求引入了 MCP (Model Context Protocol) 透传功能，旨在增强系统与第三方 AI 服务商的集成能力。通过在提供商配置中增加 MCP 透传设置，用户可以将特定的 AI 工具调用（例如图片识别和联网搜索）无缝转发给如 Minimax 等外部服务。这不仅扩展了平台的功能，也为未来集成更多 AI 工具提供了可扩展的基础架构。 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}
| mcpPassthroughType: varchar('mcp_passthrough_type', { length: 20 }) | ||
| .default('none') | ||
| .$type<'none' | 'minimax' | 'glm' | 'custom'>(), |
There was a problem hiding this comment.
mcpPassthroughType 字段在 Drizzle schema 中的定义与数据库迁移脚本 (drizzle/0021_glossy_silvermane.sql) 不一致。迁移脚本中此字段为 NOT NULL，但 schema 定义中缺少了 .notNull() 约束。这可能导致应用层代码在处理此字段时出现与数据库实际约束不符的意外行为，甚至引发运行时错误。
| mcpPassthroughType: varchar('mcp_passthrough_type', { length: 20 }) | |
| .default('none') | |
| .$type<'none' | 'minimax' | 'glm' | 'custom'>(), | |
| mcpPassthroughType: varchar('mcp_passthrough_type', { length: 20 }) | |
| .default('none') | |
| .notNull() | |
| .$type<'none' | 'minimax' | 'glm' | 'custom'>(), |
| /** | ||
| * MCP 透传处理器 | ||
| * | ||
| * 检测并处理 MCP 工具调用，将其透传到配置的第三方 AI 服务商 | ||
| * 例如：将 web_search、understand_image 等工具调用透传到 minimax | ||
| */ | ||
|
|
||
| import type { Provider } from "@/types/provider"; | ||
| import { logger } from "@/lib/logger"; | ||
| import { MinimaxMcpClient } from "@/lib/mcp/minimax-client"; | ||
| import type { McpClientConfig } from "@/lib/mcp/types"; | ||
| import { McpError } from "@/lib/mcp/types"; | ||
|
|
||
| /** | ||
| * MCP 工具调用信息 | ||
| */ | ||
| interface McpToolCall { | ||
| type: "tool_use"; | ||
| id: string; | ||
| name: string; | ||
| input: Record<string, unknown>; | ||
| } | ||
|
|
||
| /** | ||
| * MCP 工具响应 | ||
| */ | ||
| interface McpToolResponse { | ||
| type: "tool_result"; | ||
| tool_use_id: string; | ||
| content: string | Array<{ type: string; text?: string }>; | ||
| is_error?: boolean; | ||
| } | ||
|
|
||
| /** | ||
| * MCP 透传处理器 | ||
| */ | ||
| export class McpPassthroughHandler { | ||
| /** | ||
| * 检查是否应该处理该工具调用 | ||
| * | ||
| * @param provider - 供应商配置 | ||
| * @param toolName - 工具名称 | ||
| * @returns 是否应该处理 | ||
| */ | ||
| static shouldHandle(provider: Provider, toolName: string): boolean { | ||
| // 检查供应商是否配置了 MCP 透传 | ||
| if (!provider.mcpPassthroughType || provider.mcpPassthroughType === "none") { | ||
| return false; | ||
| } | ||
|
|
||
| // 检查工具名称是否支持 | ||
| const supportedTools = this.getSupportedTools(provider.mcpPassthroughType); | ||
| return supportedTools.includes(toolName); | ||
| } | ||
|
|
||
| /** | ||
| * 获取支持的工具列表 | ||
| * | ||
| * @param mcpType - MCP 透传类型 | ||
| * @returns 支持的工具名称列表 | ||
| */ | ||
| private static getSupportedTools(mcpType: string): string[] { | ||
| switch (mcpType) { | ||
| case "minimax": | ||
| return ["web_search", "understand_image"]; | ||
| case "glm": | ||
| // 预留：智谱 GLM 支持的工具 | ||
| return []; | ||
| case "custom": | ||
| // 预留：自定义 MCP 服务支持的工具 | ||
| return []; | ||
| default: | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 处理工具调用 | ||
| * | ||
| * @param provider - 供应商配置 | ||
| * @param toolCall - 工具调用信息 | ||
| * @returns 工具响应 | ||
| */ | ||
| static async handleToolCall( | ||
| provider: Provider, | ||
| toolCall: McpToolCall | ||
| ): Promise<McpToolResponse> { | ||
| logger.info("[McpPassthroughHandler] Handling tool call", { | ||
| providerId: provider.id, | ||
| providerName: provider.name, | ||
| mcpType: provider.mcpPassthroughType, | ||
| toolName: toolCall.name, | ||
| toolId: toolCall.id, | ||
| }); | ||
|
|
||
| try { | ||
| // 根据 MCP 类型选择客户端 | ||
| switch (provider.mcpPassthroughType) { | ||
| case "minimax": | ||
| return await this.handleMinimaxToolCall(provider, toolCall); | ||
| case "glm": | ||
| throw new McpError("GLM MCP passthrough is not implemented yet"); | ||
| case "custom": | ||
| throw new McpError("Custom MCP passthrough is not implemented yet"); | ||
| default: | ||
| throw new McpError(`Unsupported MCP type: ${provider.mcpPassthroughType}`); | ||
| } | ||
| } catch (error) { | ||
| logger.error("[McpPassthroughHandler] Tool call failed", { | ||
| providerId: provider.id, | ||
| toolName: toolCall.name, | ||
| toolId: toolCall.id, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
|
|
||
| // 返回错误响应 | ||
| return { | ||
| type: "tool_result", | ||
| tool_use_id: toolCall.id, | ||
| content: `MCP tool call failed: ${error instanceof Error ? error.message : String(error)}`, | ||
| is_error: true, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 处理 Minimax 工具调用 | ||
| * | ||
| * @param provider - 供应商配置 | ||
| * @param toolCall - 工具调用信息 | ||
| * @returns 工具响应 | ||
| */ | ||
| private static async handleMinimaxToolCall( | ||
| provider: Provider, | ||
| toolCall: McpToolCall | ||
| ): Promise<McpToolResponse> { | ||
| // 创建 Minimax 客户端 | ||
| const config: McpClientConfig = { | ||
| baseUrl: provider.url, | ||
| apiKey: provider.key, | ||
| }; | ||
| const client = new MinimaxMcpClient(config); | ||
|
|
||
| // 根据工具名称调用对应方法 | ||
| switch (toolCall.name) { | ||
| case "web_search": { | ||
| const query = toolCall.input.query as string; | ||
| if (!query) { | ||
| throw new McpError("Missing required parameter: query"); | ||
| } | ||
|
|
||
| const response = await client.webSearch(query); | ||
|
|
||
| // 格式化响应 | ||
| return { | ||
| type: "tool_result", | ||
| tool_use_id: toolCall.id, | ||
| content: JSON.stringify(response, null, 2), | ||
| }; | ||
| } | ||
|
|
||
| case "understand_image": { | ||
| const imageUrl = toolCall.input.image_url as string; | ||
| const prompt = toolCall.input.prompt as string; | ||
|
|
||
| if (!imageUrl) { | ||
| throw new McpError("Missing required parameter: image_url"); | ||
| } | ||
| if (!prompt) { | ||
| throw new McpError("Missing required parameter: prompt"); | ||
| } | ||
|
|
||
| const response = await client.understandImage(imageUrl, prompt); | ||
|
|
||
| // 格式化响应 | ||
| return { | ||
| type: "tool_result", | ||
| tool_use_id: toolCall.id, | ||
| content: JSON.stringify(response, null, 2), | ||
| }; | ||
| } | ||
|
|
||
| default: | ||
| throw new McpError(`Unsupported tool: ${toolCall.name}`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 从请求中提取工具调用 | ||
| * | ||
| * @param requestBody - 请求体 | ||
| * @returns 工具调用列表（如果有） | ||
| */ | ||
| static extractToolCalls(requestBody: unknown): McpToolCall[] | null { | ||
| if (!requestBody || typeof requestBody !== "object") { | ||
| return null; | ||
| } | ||
|
|
||
| const body = requestBody as Record<string, unknown>; | ||
|
|
||
| // 检查是否包含 messages 数组 | ||
| if (!Array.isArray(body.messages)) { | ||
| return null; | ||
| } | ||
|
|
||
| // 查找包含 tool_use 的消息 | ||
| const toolCalls: McpToolCall[] = []; | ||
|
|
||
| for (const message of body.messages) { | ||
| if (typeof message !== "object" || !message) { | ||
| continue; | ||
| } | ||
|
|
||
| const msg = message as Record<string, unknown>; | ||
|
|
||
| // 检查 content 数组 | ||
| if (Array.isArray(msg.content)) { | ||
| for (const content of msg.content) { | ||
| if ( | ||
| typeof content === "object" && | ||
| content && | ||
| "type" in content && | ||
| content.type === "tool_use" | ||
| ) { | ||
| toolCalls.push(content as McpToolCall); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return toolCalls.length > 0 ? toolCalls : null; | ||
| } | ||
| } |
There was a problem hiding this comment.
这个新文件以及相关的 src/lib/mcp/minimax-client.ts 和 src/lib/mcp/types.ts 文件引入了 MCP 工具调用的处理器和客户端。然而，这些代码在当前的 PR 中似乎没有被任何地方调用。目前的透传功能是通过在 forwarder.ts 中修改基础 URL 实现的。这些新文件实现的是一种更复杂的逻辑（拦截并处理工具调用），但尚未集成。建议将这些未使用的文件从此 PR 中移除，在未来真正集成该功能时再通过独立的 PR 引入，以保持本次 PR 的专注性并避免引入未使用的代码。
|
请问是否考虑： 另外，鉴于 CCH 项目的定位，接入非官方模型的操作，其实更推荐在 CCH 上游再加一个转换层进行兼容。 |
okok，周末我看看哈 |
完整实现 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>
📊 PR Size AnalysisThis PR is XL with 2,898 lines changed across 31 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:
Why Split?
🤖 Automated analysis by Claude AI |
| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE "providers" ADD COLUMN "mcp_passthrough_type" varchar(20) DEFAULT 'none' NOT NULL; | |||
| ALTER TABLE "providers" ADD COLUMN "mcp_passthrough_url" varchar(512); | |||
There was a problem hiding this comment.
🔴 Critical: SQL Migration Missing Newline
Why this is a problem: The SQL migration file does not end with a newline character, which violates POSIX standards and can cause issues with version control systems and some SQL tools.
Suggested fix: Add a newline at the end of the file.
|
|
||
| // 根据工具名称调用对应方法 | ||
| switch (toolCall.name) { | ||
| case "web_search": { |
There was a problem hiding this comment.
🟠 High: Missing Type Validation for MCP Tool Parameters
Why this is a problem: Type assertions (as string) without runtime validation can cause silent failures or runtime errors if invalid data is provided.
Suggested fix:
case "web_search": {
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);
// ...
}
| } | ||
| } | ||
|
|
||
| return data; |
There was a problem hiding this comment.
🟠 High: Incomplete Network Error Handling
Why this is a problem: The makeRequest method only handles HTTP errors but not network-level failures (DNS, timeout, connection refused), leading to unhandled rejections.
Suggested fix:
} catch (error) {
if (error instanceof TypeError) {
throw new McpRequestError(
`Network error: ${error.message}. Check base URL and network.`
);
}
if (error instanceof McpAuthError || error instanceof McpRequestError) {
throw error;
}
throw new McpRequestError(`Request failed: ${error instanceof Error ? error.message : String(error)}`);
}
src/app/v1/_lib/proxy/forwarder.ts
Outdated
There was a problem hiding this comment.
🟠 High: Potential False Positives in Endpoint Detection
Why this is a problem: Using startsWith(endpoint + "?") can cause false matches. The pathname property does not include query params (they are in .search), so this check is redundant and potentially buggy.
Suggested fix:
const standardEndpoints = [
"/v1/messages",
"/v1/messages/count_tokens",
"/v1/responses",
"/v1/chat/completions",
"/v1/models",
];
const requestPath = session.requestUrl.pathname;
// pathname does not include query params, so exact match is sufficient
const isStandardRequest = standardEndpoints.includes(requestPath);
const isMcpRequest = !isStandardRequest;
| * extractBaseUrl("https://api.minimaxi.com/anthropic/v1/messages") // "https://api.minimaxi.com" | ||
| * extractBaseUrl("http://localhost:3000/api") // "http://localhost:3000" | ||
| */ | ||
| export function extractBaseUrl(url: string): string { |
There was a problem hiding this comment.
🟡 Medium: Silent Failure Mode in extractBaseUrl
Why this is a problem: When URL parsing fails, returning the original URL masks configuration errors and makes debugging harder.
Suggested fix:
export function extractBaseUrl(url: string): string {
try {
const parsedUrl = new URL(url);
return parsedUrl.origin;
} catch (error) {
logger.warn("Failed to parse URL", { url, error });
return url;
}
}Or throw an error for strict validation.
| // - 'minimax': 透传到 minimax MCP 服务（图片识别、联网搜索） | ||
| // - 'glm': 透传到智谱 MCP 服务（预留） | ||
| // - 'custom': 自定义 MCP 服务（预留） | ||
| mcpPassthroughType: varchar('mcp_passthrough_type', { length: 20 }) |
There was a problem hiding this comment.
🟡 Medium: Schema Mismatch Between TypeScript and SQL
Why this is a problem: The Drizzle schema defines mcpPassthroughType without .notNull() but the SQL migration has NOT NULL. This discrepancy affects type inference.
Suggested fix:
mcpPassthroughType: varchar("mcp_passthrough_type", { length: 20 })
.notNull() // Add this to match SQL
.default("none")
.$type<"none" | "minimax" | "glm" | "custom">(),…-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.
🔒 Security Scan Results✅ No security vulnerabilities detected This PR has been scanned against OWASP Top 10, CWE Top 25, and common security anti-patterns. No security issues were identified in the code changes. Scanned Categories
Security HighlightsPositive Security Controls Found:
Code Changes Summary
🤖 Automated security scan by Claude AI - OWASP Top 10 & CWE coverage |
…-code-hub into feature/mcp-passthrough
📊 PR Size AnalysisThis PR is size/XL with 2,760 lines added and 26 lines deleted across 27 files. Large PRs are harder to review and more likely to introduce bugs. This PR implements MCP passthrough functionality but mixes several concerns that could be separated. 🔀 Suggested Split:Based on the changes, this PR could be split into 3 smaller PRs: PR 1: Database Schema & Core Infrastructure (Foundation)
Estimated size: ~1,600 lines (mostly generated schema snapshots) PR 2: MCP Client Libraries (Core Feature)
Estimated size: ~700 lines PR 3: UI Integration & i18n (User-Facing)
Estimated size: ~460 lines 🎯 Why Split?
📝 Migration Strategy:
This approach follows the database-first, logic-second, UI-last pattern which minimizes integration conflicts. Note: If you prefer to keep this as a single PR for atomic deployment, consider at minimum:
🤖 Automated analysis by Claude AI |
There was a problem hiding this comment.
🔴 Critical: Type assertion without runtime validation
Why this is a problem: Lines 162-163 perform unsafe type assertions (as string) on user-controlled input without runtime type checking. If a malicious or buggy client sends non-string values (e.g., {image_url: 123, prompt: null}), these will be silently cast and could cause runtime errors.
Suggested fix:
case "understand_image": {
const imageUrl = toolCall.input.image_url;
const prompt = toolCall.input.prompt;
if (!imageUrl || typeof imageUrl !== "string") {
throw new McpError("Invalid parameter: image_url must be a non-empty string");
}
if (!prompt || typeof prompt !== "string") {
throw new McpError("Invalid parameter: prompt must be a non-empty string");
}
const response = await client.understandImage(imageUrl, prompt);
// ...
}Apply the same pattern to all tool handlers (lines 146, 208-209, 229-230).
| const url = `${this.baseUrl}${endpoint}`; | ||
|
|
||
| try { | ||
| const response = await fetch(url, { |
There was a problem hiding this comment.
🔴 Critical: Missing timeout for external HTTP requests
Why this is a problem: The fetch call has no timeout configured. If the Minimax API becomes unresponsive, this request will hang indefinitely, blocking the proxy and potentially exhausting resources. This creates a denial-of-service vulnerability.
Suggested fix:
private async makeRequest<T>(endpoint: string, payload: unknown): Promise<T> {
const url = `${this.baseUrl}${endpoint}`;
const controller = new AbortController();
const timeoutId = setTimeout(() => controller.abort(), 30000); // 30s timeout
try {
const response = await fetch(url, {
method: "POST",
headers: {
"Content-Type": "application/json",
Authorization: `Bearer ${this.apiKey}`,
"MM-API-Source": "Claude-Code-Hub-MCP",
},
body: JSON.stringify(payload),
signal: controller.signal,
});
clearTimeout(timeoutId);
// ... rest of code
} catch (error) {
clearTimeout(timeoutId);
if (error instanceof Error && error.name === "AbortError") {
throw new McpRequestError("Request timeout after 30 seconds");
}
// ... rest of error handling
}
}| const url = `${this.baseUrl}${endpoint}`; | ||
|
|
||
| try { | ||
| const response = await fetch(url, { |
There was a problem hiding this comment.
🔴 Critical: Missing timeout for external HTTP requests
Why this is a problem: Same as minimax-client.ts - no timeout configured for fetch calls. This creates a denial-of-service vulnerability if the GLM API becomes unresponsive.
Suggested fix: Apply the same timeout pattern as suggested for minimax-client.ts (use AbortController with 30-second timeout).
| // 例如：https://api.minimaxi.com/anthropic -> https://api.minimaxi.com | ||
| try { | ||
| const baseUrlObj = new URL(provider.url); | ||
| effectiveBaseUrl = `${baseUrlObj.protocol}//${baseUrlObj.host}`; |
There was a problem hiding this comment.
🟠 High: Insufficient error recovery for URL extraction
Why this is a problem: The URL extraction logic (lines 766-769) only has a try-catch for logging, but the fallback still uses the original provider.url which might be malformed. If the URL constructor throws and the original URL is also invalid, subsequent requests will fail with cryptic errors.
Suggested fix:
try {
const baseUrlObj = new URL(provider.url);
effectiveBaseUrl = `${baseUrlObj.protocol}//${baseUrlObj.host}`;
logger.debug("ProxyForwarder: Extracted base domain for MCP passthrough", {
providerId: provider.id,
providerName: provider.name,
mcpType: provider.mcpPassthroughType,
originalUrl: provider.url,
extractedBaseDomain: effectiveBaseUrl,
requestPath,
});
} catch (error) {
logger.error("ProxyForwarder: Invalid provider URL for MCP passthrough", {
providerId: provider.id,
providerUrl: provider.url,
error,
});
throw new ProxyError(
`Invalid provider URL configuration: ${provider.url}`,
ErrorCategory.CONFIG_ERROR
);
}|
|
||
| return data; | ||
| } catch (error) { | ||
| if (error instanceof TypeError) { |
There was a problem hiding this comment.
🟠 High: Incomplete error handling for network failures
Why this is a problem: Line 165 catches TypeError and wraps it as McpRequestError, but this loses important debugging information. Network errors (DNS failures, connection refused, SSL errors) all throw TypeError, but the generic message "Network error: ..." doesn't help diagnose the root cause.
Suggested fix:
if (error instanceof TypeError) {
// TypeError indicates network-level failure (DNS, connection, SSL, etc.)
throw new McpRequestError(
`Network error: ${error.message}. Failed to connect to ${this.baseUrl}. Check base URL, network connectivity, and firewall settings.`
);
}| @@ -0,0 +1,2 @@ | |||
| ALTER TABLE "providers" ADD COLUMN "mcp_passthrough_type" varchar(20) DEFAULT 'none' NOT NULL;--> statement-breakpoint | |||
| ALTER TABLE "providers" ADD COLUMN "mcp_passthrough_url" varchar(512); No newline at end of file | |||
There was a problem hiding this comment.
🟡 Medium: Missing newline at end of file
Why this is a problem: SQL files should end with a newline character per POSIX standard. While not critical, this can cause issues with some tools and version control systems.
Suggested fix: Add a newline character at the end of the file.
| if (isMcpRequest && provider.mcpPassthroughType && provider.mcpPassthroughType !== "none") { | ||
| // MCP 透传已启用，且当前是 MCP 请求 | ||
| if (provider.mcpPassthroughUrl) { | ||
| // 使用配置的 MCP URL |
There was a problem hiding this comment.
🟡 Medium: Hardcoded endpoint list may become stale
Why this is a problem: The standardEndpoints array (lines 745-751) is hardcoded and may miss future endpoints. If new standard endpoints are added to the API, MCP passthrough logic will incorrectly treat them as MCP requests.
Recommendation: Consider moving this to a configuration constant that can be easily updated, or add a comment warning developers to update this list when adding new endpoints. Alternatively, use a positive match (known MCP endpoints) instead of negative match (everything except standard endpoints).
| ); | ||
| } | ||
|
|
||
| const data = (await response.json()) as T; |
There was a problem hiding this comment.
🟠 High: Missing GLM-specific error handling
Why this is a problem: The comment at line 148 acknowledges that GLM error handling needs adjustment based on actual API response format, but the current implementation only does basic HTTP status code checking. If GLM returns error details in the response body (similar to Minimax's base_resp.status_code), those errors will be silently ignored.
Recommendation: Either:
- Implement proper GLM error parsing based on their API documentation
- Add a TODO comment linking to a tracking issue
- At minimum, add response body logging for non-2xx responses to aid debugging
const data = (await response.json()) as T;
// TODO: Implement GLM-specific error handling based on API docs
// For now, log response for debugging
logger.debug("[GlmMcpClient] API response", { endpoint, data });
return data;
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 support Minimax image recognition and web search, as well as GLM image/video analysis. The implementation adds new database fields, MCP client libraries, and request routing logic. While the feature is well-structured, there are several critical security and reliability issues that must be addressed before merge.
🔍 Issues Found
- Critical (🔴): 5 issues
- High (🟠): 3 issues
- Medium (🟡): 2 issues
- Low (🟢): 0 issues
🎯 Priority Actions
- Fix type assertion vulnerabilities in
mcp-passthrough-handler.ts- Add runtime type validation for alltoolCall.inputproperties before type assertions - Implement timeout handling for MCP HTTP requests in
minimax-client.tsandglm-client.ts- Add AbortSignal with reasonable timeout (10-30s) - Add URL validation in forwarder.ts line 768 - Validate extracted base domain URL is well-formed before using
- Handle network errors properly in MCP clients - Missing error handling for TypeError from fetch failures
- Add missing database migration - The migration file
0024_glossy_silvermane.sqlshould have a newline at end of file
💡 General Observations
Positive patterns:
- Good separation of concerns with dedicated MCP client classes
- Proper logging throughout the MCP passthrough flow
- Graceful error handling with
is_errorflag in tool responses
Areas for improvement:
- Missing input validation on user-controlled data (type assertions)
- No timeout protection for external HTTP calls
- URL extraction logic needs error recovery
- Some error cases not handled (network failures)
🤖 Automated review by Claude AI - focused on identifying issues for improvement
🔒 Security Scan Results🚨 Vulnerabilities Found
⚡ Immediate Actions RequiredCRITICAL - Must fix before merge:
HIGH - Should fix before merge:
MEDIUM - Address in follow-up:
📋 OWASP Top 10 Coverage
🔍 Additional Security Checks
🛡️ Security Posture: CRITICAL CONCERNSOverall Assessment: This PR introduces critical SSRF vulnerabilities that must be fixed before merging. The MCP passthrough feature allows admins to configure arbitrary URLs that the server will make requests to, with no validation to prevent access to internal network resources. Risk Level: HIGH
Recommended Actions:
📚 Security ReferencesSSRF Protection: Input Validation: Denial of Service: 🤖 Automated security scan by Claude AI - OWASP Top 10 & CWE coverage |
- 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
📊 PR Size AnalysisThis PR is XL with ~1,342 lines changed (excluding generated DB migration files) across 24 files. Large PRs are harder to review and more likely to introduce bugs. This is a significant feature addition that implements MCP passthrough functionality. 🔀 Suggested Split:Based on the changes, this PR could be split into 3 smaller PRs: PR 1: Database Schema & Types Foundation (Size: S)Purpose: Add database columns and TypeScript types for MCP passthrough Files:
Total: ~119 lines, 5 files PR 2: MCP Client Libraries (Size: M)Purpose: Implement GLM and Minimax MCP client libraries Files:
Total: ~496 lines, 4 files PR 3: MCP Passthrough Integration & UI (Size: M)Purpose: Integrate MCP passthrough into proxy handler and add UI Depends on: PR 1, PR 2 Files:
Total: ~699 lines, 10 files 💡 Why Split?
📝 Alternative ApproachIf splitting is too much overhead, consider these review checkpoints:
This allows reviewers to tackle the PR in logical chunks. 🤖 Automated analysis by Claude AI |
This comment was marked as resolved.
This comment was marked as resolved.
2 participants
支持 minimax 图片识别和联网搜索 mcp 请求透传