fix(security): harden MCP client against SSRF, env leakage, npx package exec, and oversized responses

[rishipandey9399]

Fixes #9304

Modifications / 改动点

astrbot/core/agent/mcp_client.py 的 MCP 客户端存在 4 个相关的安全问题（#9304），本 PR 逐一修复：

• VULN-1 — SSRF (HIGH): New astrbot/core/utils/ssrf_guard.py with validate_mcp_url(), which resolves the configured MCP server hostname and rejects private/loopback/link-local/reserved/multicast addresses (blocks things like 169.254.169.254, 127.0.0.1, 10.x, 192.168.x). Wired into both _quick_test_mcp_connection() implementations (mcp_client.py and the duplicate used by the dashboard's test-connection endpoint in func_tool_manager.py) and into MCPClient._do_connect() as defense in depth, so the real connection path is covered even independently of the quick test. Opt-out via ASTRBOT_MCP_ALLOW_PRIVATE_NETWORK_URLS=1 for deployments that intentionally run MCP servers on an internal network.
• VULN-2 — STDIO env leakage (HIGH): The mcp SDK (>=1.8.0, as pinned) already restricts the default subprocess environment on non-Windows to a safe subset (get_default_environment()), but AstrBot's own _merge_environment_variables() explicitly copied all of os.environ (API keys, tokens, etc.) into the subprocess environment on Windows. Replaced the full-environ copy with a curated _WINDOWS_SAFE_ENV_VARS allowlist (PATH, PATHEXT, SYSTEMROOT, TEMP, …) needed for executable resolution, preserving the intent of the original Windows fix (fix(windows): inherit all system environment variables into MCP runner for Windows #7054) without forwarding secrets to MCP subprocesses.
• VULN-3 — npx arbitrary package execution (MEDIUM): Added an opt-in package allowlist for npx/bunx/uvx via ASTRBOT_MCP_NPX_ALLOWED_PACKAGES (comma-separated package specs), matching the fix suggested in the issue. Disabled by default so existing configurations are unaffected.
• VULN-4 — oversized MCP responses (MEDIUM): Added a configurable cap (ASTRBOT_MCP_MAX_RESPONSE_TEXT_LENGTH, default 200k characters) that truncates oversized text blocks returned from MCPTool.call(), guarding against memory/context exhaustion from a malicious or compromised MCP server.

Tests added in tests/unit/test_mcp_client_security.py cover the SSRF guard (blocking/allowing), the Windows env-merge fix (asserts secrets are not leaked), the npx allowlist (default-open, blocks when configured), and response truncation.

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

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

$ ruff check astrbot/core/agent/mcp_client.py astrbot/core/provider/func_tool_manager.py astrbot/core/utils/ssrf_guard.py tests/unit/test_mcp_client_security.py
All checks passed!

$ pytest tests/unit -q
725 passed, 1 warning in 11.72s

---

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.txt 和 pyproject.toml 文件相应位置。
• 😮 My changes do not introduce malicious code. / 我的更改没有引入恶意代码。

🤖 Generated with Claude Code

Summary by Sourcery

Harden MCP client and related tooling against SSRF, environment-variable leakage, arbitrary package execution, and oversized tool responses.

Bug Fixes:

• Prevent MCP HTTP server configuration from targeting private or otherwise unsafe network addresses using a central SSRF guard.
• Avoid leaking host process secrets into MCP stdio subprocesses by restricting inherited Windows environment variables to a safe allowlist.
• Constrain use of npx/bunx/uvx for MCP stdio servers via an optional package allowlist to reduce arbitrary code execution risk.
• Limit the size of text content returned from MCP tool calls to mitigate memory and context exhaustion from malicious responses.

Tests:

• Add unit tests covering MCP URL validation behavior, Windows environment merging semantics, npx package allowlist enforcement, and response-size truncation.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[sourcery-ai]

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

• The SSRF guard currently resolves hostnames on every call to validate_mcp_url; consider caching resolution/allow/deny decisions per hostname (or URL) to avoid repeated getaddrinfo calls on hot paths.
• In _validate_stdio_args for package runners, the allowlist comparison is a strict string equality on non-flag args; if you expect common npx patterns like pkg@version or scoped commands, it may be worth normalizing or parsing package specs more flexibly so legitimate configurations are not inadvertently blocked.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The SSRF guard currently resolves hostnames on every call to `validate_mcp_url`; consider caching resolution/allow/deny decisions per hostname (or URL) to avoid repeated `getaddrinfo` calls on hot paths.
- In `_validate_stdio_args` for package runners, the allowlist comparison is a strict string equality on non-flag args; if you expect common `npx` patterns like `pkg@version` or scoped commands, it may be worth normalizing or parsing package specs more flexibly so legitimate configurations are not inadvertently blocked.

## Individual Comments

### Comment 1
<location path="tests/unit/test_mcp_client_security.py" line_range="61" />
<code_context>
+        assert "PATH" not in merged
+
+
+class TestNpxPackageAllowlist:
+    def test_unrestricted_by_default(self, monkeypatch):
+        monkeypatch.delenv("ASTRBOT_MCP_NPX_ALLOWED_PACKAGES", raising=False)
</code_context>
<issue_to_address>
**suggestion (testing):** Extend `TestNpxPackageAllowlist` to cover bunx/uvx and the case where only flags (no packages) are passed with an allowlist configured.

The allowlist logic is shared by `npx`, `bunx`, and `uvx`, but the tests only cover `npx`. Please add parallel cases for `bunx` and `uvx` so we verify all runners enforce the same restrictions. In addition, when an allowlist is set and the args consist only of flags (no non-`-`-prefixed entries), `_validate_stdio_args` currently raises; a specific test for this flags‑only scenario will ensure that behavior is preserved and that misconfigured invocations cannot skip package checks.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Extend TestNpxPackageAllowlist to cover bunx/uvx and the case where only flags (no packages) are passed with an allowlist configured.

The allowlist logic is shared by npx, bunx, and uvx, but the tests only cover npx. Please add parallel cases for bunx and uvx so we verify all runners enforce the same restrictions. In addition, when an allowlist is set and the args consist only of flags (no non---prefixed entries), _validate_stdio_args currently raises; a specific test for this flags‑only scenario will ensure that behavior is preserved and that misconfigured invocations cannot skip package checks.