fix(core): harden MCP client transport setup #1389
+216
−7
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@voltagent/core": patch | ||
| --- | ||
|
|
||
| Harden MCP client transport setup by filtering stdio environment variables and rejecting local, private, link-local, and reserved MCP server URL targets. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { isIP } from "node:net"; | ||
| import { Client } from "@modelcontextprotocol/sdk/client/index.js"; | ||
| import { SSEClientTransport } from "@modelcontextprotocol/sdk/client/sse.js"; | ||
| import { | ||
|
|
@@ -38,6 +39,118 @@ import type { | |
| import type { UserInputHandler } from "./user-input-bridge"; | ||
| import { UserInputBridge } from "./user-input-bridge"; | ||
|
|
||
| const SAFE_STDIO_ENV_KEYS = new Set([ | ||
| "PATH", | ||
| "Path", | ||
| "HOME", | ||
| "USERPROFILE", | ||
| "APPDATA", | ||
| "LOCALAPPDATA", | ||
| "TMP", | ||
| "TEMP", | ||
| "SystemRoot", | ||
| "ComSpec", | ||
| ]); | ||
|
|
||
| function buildStdioEnvironment(explicitEnv?: Record<string, string>): Record<string, string> { | ||
| const safeDefaultEnvironment: Record<string, string> = {}; | ||
| const defaultEnvironment = getDefaultEnvironment(); | ||
|
|
||
| for (const [key, value] of Object.entries(defaultEnvironment)) { | ||
| if (SAFE_STDIO_ENV_KEYS.has(key)) { | ||
| safeDefaultEnvironment[key] = value; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| ...safeDefaultEnvironment, | ||
| ...(explicitEnv || {}), | ||
| }; | ||
| } | ||
|
|
||
| function createMCPServerUrl(rawUrl: string): URL { | ||
| const url = new URL(rawUrl); | ||
| if (url.protocol !== "http:" && url.protocol !== "https:") { | ||
| throw new Error(`Unsupported MCP server URL protocol: ${url.protocol}`); | ||
| } | ||
|
|
||
| if (isLocalOrPrivateHost(url.hostname)) { | ||
| throw new Error(`MCP server URL host is not allowed: ${url.hostname}`); | ||
| } | ||
|
|
||
| return url; | ||
| } | ||
|
|
||
| function isLocalOrPrivateHost(hostname: string): boolean { | ||
| const normalized = hostname | ||
| .toLowerCase() | ||
| .replace(/^\[|\]$/g, "") | ||
| .replace(/\.+$/g, ""); | ||
| if (!normalized || normalized === "localhost" || normalized.endsWith(".localhost")) { | ||
| return true; | ||
| } | ||
|
|
||
| return isPrivateIPv4(normalized) || isPrivateIPv6(normalized); | ||
| } | ||
|
Comment on lines
+84
to
+94
Contributor
There was a problem hiding this comment. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🌐 Web query:
💡 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:
🏁 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.
🤖 Prompt for AI Agents |
||
|
|
||
| function isPrivateIPv4(hostname: string): boolean { | ||
| const octets = hostname.split("."); | ||
| if (octets.length !== 4) { | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| return false; | ||
| } | ||
|
|
||
| const values = octets.map((octet) => Number.parseInt(octet, 10)); | ||
| if (values.some((value, index) => !/^\d+$/.test(octets[index]) || value < 0 || value > 255)) { | ||
| return false; | ||
| } | ||
|
|
||
| const [first, second] = values; | ||
| return ( | ||
| first === 0 || | ||
| first === 10 || | ||
| first === 127 || | ||
| (first === 169 && second === 254) || | ||
| (first === 172 && second >= 16 && second <= 31) || | ||
| (first === 192 && second === 168) || | ||
| (first === 100 && second >= 64 && second <= 127) | ||
| ); | ||
| } | ||
|
|
||
| function getIPv4FromMappedIPv6(hostname: string): string | undefined { | ||
| const mappedMatch = hostname.match(/^::ffff:(?:(0):)?([0-9a-f]{1,4}):([0-9a-f]{1,4})$/i); | ||
| if (!mappedMatch) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const [, , highHex, lowHex] = mappedMatch; | ||
| const high = Number.parseInt(highHex, 16); | ||
| const low = Number.parseInt(lowHex, 16); | ||
| if (high > 0xffff || low > 0xffff) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return `${(high >> 8) & 0xff}.${high & 0xff}.${(low >> 8) & 0xff}.${low & 0xff}`; | ||
| } | ||
|
|
||
| function isPrivateIPv6(hostname: string): boolean { | ||
| if (isIP(hostname) !== 6) { | ||
| return false; | ||
| } | ||
|
|
||
| const mappedIPv4 = getIPv4FromMappedIPv6(hostname); | ||
| if (mappedIPv4) { | ||
| return isPrivateIPv4(mappedIPv4); | ||
| } | ||
|
|
||
| const firstHextet = Number.parseInt(hostname.split(":")[0] || "0", 16); | ||
| return ( | ||
| hostname === "::1" || | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| hostname === "0:0:0:0:0:0:0:1" || | ||
| (firstHextet >= 0xfc00 && firstHextet <= 0xfdff) || | ||
| (firstHextet >= 0xfe80 && firstHextet <= 0xfebf) | ||
| ); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * Client for interacting with Model Context Protocol (MCP) servers. | ||
| * Wraps the official MCP SDK client to provide a higher-level interface. | ||
|
|
@@ -159,19 +272,19 @@ export class MCPClient extends SimpleEventEmitter { | |
|
|
||
| if (this.isHTTPServer(config.server)) { | ||
| // HTTP type: Try streamable HTTP first with SSE fallback | ||
| this.transport = new StreamableHTTPClientTransport(createMCPServerUrl(config.server.url), { | ||
| requestInit: config.server.requestInit, | ||
| }); | ||
| this.shouldAttemptFallback = true; | ||
| } else if (this.isSSEServer(config.server)) { | ||
| // Explicit SSE transport | ||
| this.transport = new SSEClientTransport(createMCPServerUrl(config.server.url), { | ||
| requestInit: config.server.requestInit, | ||
| eventSourceInit: config.server.eventSourceInit, | ||
| }); | ||
| } else if (this.isStreamableHTTPServer(config.server)) { | ||
| // Explicit streamable HTTP transport (no fallback) | ||
| this.transport = new StreamableHTTPClientTransport(createMCPServerUrl(config.server.url), { | ||
| requestInit: config.server.requestInit, | ||
| sessionId: config.server.sessionId, | ||
| }); | ||
|
|
@@ -181,7 +294,7 @@ export class MCPClient extends SimpleEventEmitter { | |
| command: config.server.command, | ||
| args: config.server.args || [], | ||
| cwd: config.server.cwd, | ||
| env: buildStdioEnvironment(config.server.env), | ||
| }); | ||
| } else { | ||
| throw new Error( | ||
|
|
@@ -292,7 +405,7 @@ export class MCPClient extends SimpleEventEmitter { | |
| throw new Error("Invalid server config for SSE fallback"); | ||
| } | ||
|
|
||
| this.transport = new SSEClientTransport(createMCPServerUrl(this.serverConfig.url), { | ||
| requestInit: this.serverConfig.requestInit, | ||
| eventSourceInit: this.serverConfig.eventSourceInit, | ||
| }); | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.