fix(core): harden MCP client transport setup - #1389
Conversation
Validate MCP HTTP/SSE URLs before constructing transports and block direct local/private targets.\n\nLimit stdio server environment inheritance to a small allowlist plus explicitly configured env vars so parent process secrets are not passed by default.\n\nAdds regression tests for the transport-level issues reported in VoltAgent#1382.
|
📝 WalkthroughWalkthroughThe MCP client now filters stdio environment variables and preserves explicit overrides. It validates MCP server URLs to allow HTTP(S) schemes and reject local, private, loopback, link-local, and reserved hosts across supported transports. Tests cover both controls. ChangesMCP transport security
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant URLValidation
participant MCPTransport
MCPClient->>URLValidation: validate configured MCP server URL
URLValidation-->>MCPClient: return approved URL
MCPClient->>MCPTransport: construct transport with approved URL
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/mcp/client/index.ts (1)
92-113: 🚀 Performance & Scalability | 🔵 TrivialIPv4 range list omits several reserved/non-routable blocks.
The allow-list of blocked ranges misses
192.0.0.0/24,192.0.2.0/24(TEST-NET-1),198.18.0.0/15(benchmarking),198.51.100.0/24and203.0.113.0/24(TEST-NET-2/3), and the multicast/reserved ranges224.0.0.0/4and240.0.0.0/4. These are lower-risk than the core private ranges already covered, but they are still not legitimate public MCP server targets. See the consolidated recommendation below to replace this hand-rolled logic with a maintained IP-range library.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/mcp/client/index.ts` around lines 92 - 113, Update isPrivateIPv4 to block the omitted non-routable and reserved IPv4 ranges, including 192.0.0.0/24, 192.0.2.0/24, 198.18.0.0/15, 198.51.100.0/24, 203.0.113.0/24, 224.0.0.0/4, and 240.0.0.0/4; preferably replace the hand-rolled range checks with a maintained IP-range library as recommended, while preserving existing validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/mcp/client/index.ts`:
- Around line 115-123: Update isPrivateIPv6 in
packages/core/src/mcp/client/index.ts (lines 115-123) to reject IPv4-mapped IPv6
addresses, including the ::ffff:0:0/96 range, while preserving existing private
IPv6 checks. Add regression cases in packages/core/src/mcp/client/index.spec.ts
(lines 200-216) for the two IPv4-mapped addresses and the 10.0.0.1, 192.168.1.1,
and 172.16.0.1 private IPv4 URLs, verifying each is rejected.
- Around line 83-90: Harden isLocalOrPrivateHost and the connection path so
DNS-resolved addresses are validated at connection time, preventing rebinding or
short-TTL DNS from bypassing private-host checks; use the existing
resolver/agent policy where available rather than relying only on the initial
URL hostname. Normalize hostnames by removing trailing dots before localhost and
IP comparisons, while preserving bracket handling and private IPv4/IPv6
detection.
---
Nitpick comments:
In `@packages/core/src/mcp/client/index.ts`:
- Around line 92-113: Update isPrivateIPv4 to block the omitted non-routable and
reserved IPv4 ranges, including 192.0.0.0/24, 192.0.2.0/24, 198.18.0.0/15,
198.51.100.0/24, 203.0.113.0/24, 224.0.0.0/4, and 240.0.0.0/4; preferably
replace the hand-rolled range checks with a maintained IP-range library as
recommended, while preserving existing validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: faf06026-6653-416c-b7ab-a2b89c011101
📒 Files selected for processing (2)
packages/core/src/mcp/client/index.spec.tspackages/core/src/mcp/client/index.ts
| function isLocalOrPrivateHost(hostname: string): boolean { | ||
| const normalized = hostname.toLowerCase().replace(/^\[|\]$/g, ""); | ||
| if (!normalized || normalized === "localhost" || normalized.endsWith(".localhost")) { | ||
| return true; | ||
| } | ||
|
|
||
| return isPrivateIPv4(normalized) || isPrivateIPv6(normalized); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does the WHATWG URL parser strip a trailing dot from non-numeric domain hostnames?
💡 Result:
No, the WHATWG URL parser does not strip a trailing dot from a domain hostname [1][2]. The parser preserves the trailing dot if it is present in the input [1][3]. In the WHATWG URL Standard, the hostname is parsed and maintained as a string, and explicit logic exists to handle the trailing dot separately when performing operations such as determining the public suffix or the registrable domain [4][3]. For example, when calculating these properties, the standard explicitly defines a "trailingDot" variable that captures the dot if it exists at the end of the host, allowing algorithms to account for it while treating the rest of the host according to established rules [4][3]. This behavior contrasts with some other specifications or implementations that may normalize hostnames by removing trailing dots [5][2]. Because the WHATWG parser keeps the trailing dot, it is possible for a URL with a trailing dot (e.g., example.com.) to be treated as distinct from one without it (e.g., example.com) in some contexts, such as string-based security or filtering mechanisms [5][2]. The standard itself notes that while certificate comparison requires a host equivalence check that ignores the trailing dot, the URL parser does not enforce such restrictions on the host component [4][3][6].
Citations:
- 1: Empty labels (consecutive dots, and trailing dot for host writing + UTS46) whatwg/url#871
- 2: https://alltools.dev/reference/tech/url-parsing-whatwg-vs-rfc-3986/
- 3: https://github.com/whatwg/url/blob/main/url.bs
- 4: https://url.spec.whatwg.org/
- 5: https://daniel.haxx.se/blog/2022/05/12/a-tale-of-a-trailing-dot/
- 6: https://github.com/whatwg/url/blob/b40eb97221ad039df7f3cf3814c024f6de75c69d/url.bs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the target file and relevant implementation/imports.
if [ -f packages/core/src/mcp/client/index.ts ]; then
echo "== outline =="
ast-grep outline packages/core/src/mcp/client/index.ts --view expanded || true
echo
echo "== target lines =="
sed -n '1,140p' packages/core/src/mcp/client/index.ts | nl -ba
else
echo "target file not found"
fd -a 'index.ts' . | sed -n '1,20p'
fi
echo
echo "== isLocalOrPrivateHost usages =="
rg -n "isLocalOrPrivateHost|privateIPv4|privateIPv6|isPrivateIPv" packages/core/src/mcp packages -S || true
echo
echo "== DNS resolution / connector usage mentions =="
rg -n "lookup|family|createServer|createClient|http\.|https\.|net\.|Agent|RequestInit|initTimeout|timeout|ssrf|mcp.*/client|serverTransport|ClientTransport" packages/core/src packages -S | sed -n '1,160p'Repository: VoltAgent/voltagent
Length of output: 2525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target lines =="
sed -n '1,130p' packages/core/src/mcp/client/index.ts
echo
echo "== connect and HTTP/SSE construction excerpts =="
sed -n '130,420p' packages/core/src/mcp/client/index.ts
echo
echo "== isLocalOrPrivateHost usages =="
rg -n "isLocalOrPrivateHost|privateIPv4|privateIPv6|isPrivateIPv" . -S || true
echo
echo "== DNS resolution / connector usage mentions =="
rg -n "lookup|family|createServer|createClient|http\.|https\.|net\.|Agent|RequestInit|initTimeout|timeout|ssrf|mcp.*/client|serverTransport|ClientTransport|fetch\\(" packages/core/src packages -S 2>/dev/null | sed -n '1,240p'Repository: VoltAgent/voltagent
Length of output: 36592
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node WHATWG URL hostname behavior =="
node - <<'JS'
for (const raw of ["http://example.com", "http://example.com.", "http://localhost", "http://localhost.", "http://[::1]", "http://[::1]."]) {
const url = new URL(raw)
console.log(`${raw} -> protocol=${url.protocol}; host=${url.host}; hostname=${url.hostname}; hostEndsWithDot=${url.host.endsWith(".")}; hostnameEndsWithDot=${url.hostname.endsWith(".")}`)
}
JS
echo
echo "== MCP package version / lockfiles =="
fd -a 'package.json' packages/core packages/sdk | while read -r f; do
echo "-- $f"
sed -n '1,140p' "$f"
done
echo
echo "== lockfile mentions of mcp sdk =="
rg -n "`@modelcontextprotocol/sdk`|modelcontextprotocol" package.json pnpm-lock.yaml yarn.lock packages -S 2>/dev/null | sed -n '1,220p'Repository: VoltAgent/voltagent
Length of output: 1490
Guard DNS and trailing-dot variants before connecting.
isLocalOrPrivateHost() checks the URL hostname string once before transport construction. Add connection-time IP validation or a custom resolver/agent policy to close rebind/short-TTl DNS bypasses. Also normalize/truncate trailing dots on hostnames before comparison, because new URL("...://localhost.") preserves hostname as localhost..
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/mcp/client/index.ts` around lines 83 - 90, Harden
isLocalOrPrivateHost and the connection path so DNS-resolved addresses are
validated at connection time, preventing rebinding or short-TTL DNS from
bypassing private-host checks; use the existing resolver/agent policy where
available rather than relying only on the initial URL hostname. Normalize
hostnames by removing trailing dots before localhost and IP comparisons, while
preserving bracket handling and private IPv4/IPv6 detection.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Addressed the MCP URL hardening review feedback in
Validation run:
|
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Thanks for the review — I pushed Changes made:
Validation:
Note: I did not add DNS rebind / connection-time resolver enforcement in this pass because the current MCP SDK transport construction used here does not expose a resolver/agent hook to pin or filter resolved addresses safely. This patch covers the deterministic literal-host bypasses without changing transport internals. |
2 participants
Summary
Notes
This addresses the transport setup and stdio environment portions of #1382. The response-injection concern is intentionally left separate because it needs an agreed policy for how VoltAgent should transform or annotate MCP tool output without breaking existing tool result semantics.
Test Plan
vitest run packages/core/src/mcp/client/index.spec.ts --config vitest.config.mtspnpm --filter @voltagent/core typecheckpnpm --filter @voltagent/core buildbiome check packages/core/src/mcp/client/index.ts packages/core/src/mcp/client/index.spec.tsRelated to #1382
Summary by cubic
Hardened MCP client transport setup by validating server URLs and restricting stdio env inheritance. Prevents local/private (including IPv4‑mapped IPv6 and CGNAT) connections and avoids leaking parent process secrets (addresses #1382 transport and stdio env).
http/httpsURLs and apply the same checks to SSE and the SSE fallback.PATH,HOME) plus explicit vars; do not forward secrets by default.Written for commit 1ed799f. Summary will update on new commits.
Summary by CodeRabbit