Skip to content

fix: 修复了MCP工具对http/sse的兼容性问题 - #7712

Open
hjdhnx wants to merge 39 commits into
AstrBotDevs:masterfrom
hjdhnx:master
Open

fix: 修复了MCP工具对http/sse的兼容性问题#7712
hjdhnx wants to merge 39 commits into
AstrBotDevs:masterfrom
hjdhnx:master

Conversation

@hjdhnx

@hjdhnx hjdhnx commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Modifications / 改动点

修改了astrbot/core/agent/mcp_client.py,去除了SSL验证,防止http/sse 格式的MCP服务不可用

  • This is NOT a breaking change. / 这不是一个破坏性变更。

Screenshots or Test Results / 运行截图或测试结果

image

Checklist / 检查清单

  • 😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
    / 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。

  • 👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
    / 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”

  • 🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in requirements.txt and pyproject.toml.
    / 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到 requirements.txtpyproject.toml 文件相应位置。

  • 😮 My changes do not introduce malicious code.
    / 我的更改没有引入恶意代码。

Summary by Sourcery

Improve MCP HTTP/SSE client compatibility by customizing the underlying HTTP client configuration.

Bug Fixes:

  • Ensure MCP HTTP and SSE transports work with endpoints that fail standard SSL verification by using a non-verifying HTTP client when available.

Enhancements:

  • Unify HTTP client configuration for SSE and streamable HTTP transports via shared client factory options.

Taois and others added 10 commits April 20, 2026 13:44
1. 常见的openai和anthropic协议,如 智谱的codingpan
https://open.bigmodel.cn/api/coding/paas/v4
2. 新出的一些没有模型列表的自定义模型提供商,如科大讯飞
https://maas-coding-api.cn-huabei-1.xf-yun.com/v2
# Conflicts:
#	astrbot/core/utils/network_utils.py
已知:
  根因:系统有代理 127.0.0.1:7890,mcp 库底层用 httpx 会自动走代理,但代理的 SSL 证书不被 Python 信任,导致 SSL 验证失败 → 抛出 CancelledError(BaseException 子类) → 绕过
  except Exception → Quart 返回 HTTP 500。

  修复: 在 mcp_client.py 中新增了 _create_no_verify_httpx_client 工厂函数(verify=False),通过 httpx_client_factory 参数传给 sse_client 和 streamablehttp_client,跳过 SSL
  证书验证。
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. area:core The bug / feature is about astrbot's core, backend labels Apr 21, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • Disabling SSL verification globally via _create_no_verify_httpx_client is a significant security relaxation; consider making this opt-in via config (e.g., verify_ssl: false) or only using it for http:// endpoints while preserving verification for https://.
  • Instead of hardcoding verify=False, it may be better to expose verify (or a CA bundle path) as a parameter in cfg and pass it through to the HTTP client so users can choose between strict verification, custom certs, or no verification.
  • When _create_no_verify_httpx_client is unavailable, the behavior silently falls back to the default client; consider logging or surfacing this difference so users understand that SSL verification is still enforced in that case.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Disabling SSL verification globally via `_create_no_verify_httpx_client` is a significant security relaxation; consider making this opt-in via config (e.g., `verify_ssl: false`) or only using it for `http://` endpoints while preserving verification for `https://`.
- Instead of hardcoding `verify=False`, it may be better to expose `verify` (or a CA bundle path) as a parameter in `cfg` and pass it through to the HTTP client so users can choose between strict verification, custom certs, or no verification.
- When `_create_no_verify_httpx_client` is unavailable, the behavior silently falls back to the default client; consider logging or surfacing this difference so users understand that SSL verification is still enforced in that case.

## Individual Comments

### Comment 1
<location path="astrbot/core/agent/mcp_client.py" line_range="122" />
<code_context>
+    ) -> _httpx.AsyncClient:
+        kwargs: dict[str, Any] = {
+            "follow_redirects": True,
+            "verify": False,
+        }
+        if timeout is None:
</code_context>
<issue_to_address>
**🚨 issue (security):** Reconsider disabling TLS verification by default in the shared HTTPX client factory.

Setting `verify=False` here forces all callers to skip TLS certificate verification, which is unsafe as a default. If you need to support self‑signed certs, consider instead:

- Exposing `verify` as a parameter that defaults to `True`, so call sites explicitly opt into `False`, or
- Accepting a custom CA bundle while keeping verification enabled.

As written, this factory silently weakens TLS security for all consumers, which is risky for production use.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread astrbot/core/agent/mcp_client.py

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a helper function to create an httpx client with SSL verification disabled and refactors the MCP client to use this factory for SSE and streamable HTTP connections. Review feedback identifies a security vulnerability in hardcoding the disabled verification and suggests making it configurable. Additionally, it is recommended to refactor the SSL logic into a shared utility to ensure consistency with connection tests and to include unit tests for the new functionality.

) -> _httpx.AsyncClient:
kwargs: dict[str, Any] = {
"follow_redirects": True,
"verify": False,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

强制禁用 SSL 验证(verify=False)会使连接面临中间人攻击(MITM)的风险。建议通过配置项(例如从 cfg 中读取)来动态控制是否启用验证,而不是全局硬编码为 False

Comment on lines +471 to +474
if _create_no_verify_httpx_client is not None:
_http_client_kwargs["httpx_client_factory"] = (
_create_no_verify_httpx_client
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

此处的改动旨在通过自定义 httpx_client_factory 来跳过 SSL 验证,但 _quick_test_mcp_connection 仍然使用默认配置,这会导致连接测试失败。根据通用规则,建议将 SSL 配置逻辑重构为共享的辅助函数,以避免代码重复并确保逻辑一致性。

References
  1. When implementing similar functionality for different cases (e.g., direct vs. quoted attachments), refactor the logic into a shared helper function to avoid code duplication.

Comment on lines +115 to +132
def _create_no_verify_httpx_client(
headers: dict[str, str] | None = None,
timeout: _httpx.Timeout | None = None,
auth: _httpx.Auth | None = None,
) -> _httpx.AsyncClient:
kwargs: dict[str, Any] = {
"follow_redirects": True,
"verify": False,
}
if timeout is None:
kwargs["timeout"] = _httpx.Timeout(30, read=300)
else:
kwargs["timeout"] = timeout
if headers is not None:
kwargs["headers"] = headers
if auth is not None:
kwargs["auth"] = auth
return _httpx.AsyncClient(**kwargs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

根据通用规则,新功能应包含单元测试。建议添加测试用例来验证 MCP 客户端在不同传输协议下对 SSL 验证配置的处理逻辑。

References
  1. New functionality, such as handling attachments, should be accompanied by corresponding unit tests.

@hjdhnx

hjdhnx commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

@zouyonghe 大佬来合并一下这个关键修复

@hjdhnx

hjdhnx commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

修改前配置 modelscope部署的 mcp服务必定报错如图所示:
image

@hjdhnx

hjdhnx commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

#7603 本提交修复此问题

@hjdhnx

hjdhnx commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

#5977 应该是同样问题

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Apr 22, 2026

@zouyonghe zouyonghe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这轮更新后我重新看了一遍,仍有两个阻塞问题:

  1. astrbot/core/agent/mcp_client.py:525 在真正创建 sse_client / streamablehttp_client 之前,仍然先调用 _quick_test_mcp_connection()。这个预检测走的是 aiohttp.ClientSession() 的默认 TLS 校验,所以遇到本 PR 想修的代理证书/非标准证书场景时,会先在这里失败,根本走不到后面的 httpx_client_factory(... verify=False)。也就是说核心兼容性问题并没有真正被修掉。建议让预检测和正式连接共用同一套 verify/SSL 配置,或者直接移除这层 preflight。

  2. astrbot/dashboard/routes/config.py:899 现在直接访问 inst.client.embeddings.create(...),这等于把 EmbeddingProvider 假设成 OpenAI 风格客户端。但仓库里现有的 GeminiEmbeddingProvider 走的是 client.models.embed_content(...),因此 /config/provider/get_embedding_dim 会对 Gemini 这类非 OpenAI embedding provider 直接报错。这里应该继续走 EmbeddingProvider 抽象,而不是依赖底层 client 的私有形态。

@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 23, 2026
@zouyonghe
zouyonghe self-requested a review April 26, 2026 01:30
Taois added 6 commits April 29, 2026 10:24
# Conflicts:
#	astrbot/core/provider/sources/openai_source.py
#	tests/test_openai_source.py
# Conflicts:
#	astrbot/core/provider/sources/anthropic_source.py
#	astrbot/core/utils/network_utils.py
@zouyonghe

Copy link
Copy Markdown
Member

目前这版还不能合并,review 里提到的几个阻塞点仍然存在:

  1. mcp_client.py 里 preflight 仍然用默认 TLS 校验的 aiohttp.ClientSession(),会在真正走到 verify=False 的 httpx 客户端之前失败,所以代理证书/非标准证书场景没有真正修好。建议让 preflight 和正式连接共用同一套 SSL/verify 配置,或者移除这层 preflight。
  2. dashboard/routes/config.py 里获取 embedding dim 改成了直接调用 inst.client.embeddings.create(...),这会绕过 EmbeddingProvider 抽象,导致 Gemini 等非 OpenAI embedding provider 报错。这里需要恢复为走 provider 抽象。
  3. CI 里 pytest 仍失败,apscheduler.executors import 破坏了现有 mock 隔离,需要调整 import 或补测试 patch。
  4. verify=False 作为共享工厂默认值也有安全风险,建议改成显式 opt-in,或支持 CA bundle 配置。

另外 PR 现在落后 master 很多,并且混入了不少与 MCP http/sse 兼容性无关的改动。建议先修复上述阻塞项、rebase 到最新 master,并考虑把无关改动拆分出去。修完后再请求维护者 re-review。

@Soulter
Soulter force-pushed the master branch 3 times, most recently from a4c4a7d to 9bd38ca Compare May 28, 2026 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core The bug / feature is about astrbot's core, backend size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants