Conversation
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
Bot
added
size:M
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Disabling SSL verification globally via
_create_no_verify_httpx_clientis a significant security relaxation; consider making this opt-in via config (e.g.,verify_ssl: false) or only using it forhttp://endpoints while preserving verification forhttps://. - Instead of hardcoding
verify=False, it may be better to exposeverify(or a CA bundle path) as a parameter incfgand pass it through to the HTTP client so users can choose between strict verification, custom certs, or no verification. - When
_create_no_verify_httpx_clientis 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 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, |
There was a problem hiding this comment.
强制禁用 SSL 验证（verify=False）会使连接面临中间人攻击（MITM）的风险。建议通过配置项（例如从 cfg 中读取）来动态控制是否启用验证，而不是全局硬编码为 False。
| if _create_no_verify_httpx_client is not None: | ||
| _http_client_kwargs["httpx_client_factory"] = ( | ||
| _create_no_verify_httpx_client | ||
| ) |
There was a problem hiding this comment.
此处的改动旨在通过自定义 httpx_client_factory 来跳过 SSL 验证，但 _quick_test_mcp_connection 仍然使用默认配置，这会导致连接测试失败。根据通用规则，建议将 SSL 配置逻辑重构为共享的辅助函数，以避免代码重复并确保逻辑一致性。
References
- 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.
| 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) |
There was a problem hiding this comment.
根据通用规则，新功能应包含单元测试。建议添加测试用例来验证 MCP 客户端在不同传输协议下对 SSL 验证配置的处理逻辑。
References
- New functionality, such as handling attachments, should be accompanied by corresponding unit tests.
|
@zouyonghe 大佬来合并一下这个关键修复 |
|
修改前配置 modelscope部署的 mcp服务必定报错如图所示: |
|
#7603 本提交修复此问题 |
|
#5977 应该是同样问题 |
dosubot
Bot
added
size:L
zouyonghe
left a comment
zouyonghe
left a comment
There was a problem hiding this comment.
这轮更新后我重新看了一遍，仍有两个阻塞问题：
-
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。 -
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 的私有形态。
zouyonghe
left a comment
zouyonghe
left a comment
There was a problem hiding this comment.
这轮重新请求 review 后我又按当前 head 复查了一遍，结论不变：当前 PR 仍然停留在 fb23cb6d941248127c4b56309376ef65d5212228，没有看到针对上次两个阻塞点的修正。
-
astrbot/core/agent/mcp_client.py:525仍然会先走_quick_test_mcp_connection()，而这个预检测在astrbot/core/agent/mcp_client.py:354还是使用aiohttp.ClientSession()的默认 TLS 校验，所以代理证书/非标准证书场景依旧会先在这里失败，后面的httpx_client_factory(... verify=False)到不了。 -
astrbot/dashboard/routes/config.py:899仍然直接调用inst.client.embeddings.create(...)。但现有GeminiEmbeddingProvider在astrbot/core/provider/sources/gemini_embedding_source.py:38/astrbot/core/provider/sources/gemini_embedding_source.py:48走的是client.models.embed_content(...)，所以/config/provider/get_embedding_dim对非 OpenAI embedding provider 的回归还在。
因此当前代码状态下，我仍然认为这两个问题需要先修掉。
dosubot
Bot
added
size:XL
2 participants
Modifications / 改动点
修改了astrbot/core/agent/mcp_client.py，去除了SSL验证，防止http/sse 格式的MCP服务不可用
Screenshots or Test Results / 运行截图或测试结果
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.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.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:
Enhancements: