From c8b4324583123d39f2cc6ca72884cbcbeded909d Mon Sep 17 00:00:00 2001 From: Mahmoud Mabrouk Date: Thu, 25 Jun 2026 21:53:50 +0200 Subject: [PATCH] =?UTF-8?q?fix(agent):=20harden=20MCP=20delivery=20?= =?UTF-8?q?=E2=80=94=20Daytona=20loopback=20guard,=20Pi=20user-MCP=20fail-?= =?UTF-8?q?loud,=20http=20SSRF=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three runner-only MCP fixes (services/agent): 1. Daytona loopback guard (Finding 1). The internal gateway-tool MCP channel advertised a 127.0.0.1 loopback URL to the harness even on Daytona, where the harness runs INSIDE the sandbox and that URL is unreachable — so Claude + gateway tools + Daytona could call no tool. buildSessionMcpServers now takes isDaytona and skips the internal loopback channel on Daytona; the existing file relay delivers the gateway tools there instead (HTTP advertisement for local, file relay for Daytona). A user http MCP is a remote URL and stays delivered on Daytona. 2. Pi user-MCP fail-loud (Finding 2, F-032). A user-declared MCP server (stdio AND http) on a Pi harness was dropped silently (no log, HTTP 200) because Pi delivers tools through its bundled extension, not MCP. run-plan.ts now refuses any Pi run carrying mcpServers up front with PI_USER_MCP_UNSUPPORTED_MESSAGE, the way the stdio-MCP and code-tool gates fail loud. 3. User http MCP SSRF guard (Finding 3). The runner attaches Agenta-resolved named secrets as headers to the author-supplied url with no checks. validateUserMcpUrl now requires https and rejects internal/metadata hosts (loopback, link-local incl. 169.254.169.254, private literals), with an AGENTA_AGENT_MCP_HOST_ALLOWLIST opt-out. Tests: +11 unit tests (Daytona-skips-loopback + user-http-still-delivered; Pi-stdio/http refusal; SSRF scheme/host/allowlist). Full runner suite green except sandbox-agent-orchestration.test.ts, which fails only on a concurrent lane's uncommitted otel.recordError WIP (not this change). Docs synced: runner-to-mcp-server and mcp-models-and-resolution interface pages. Engine wiring (isDaytona: plan.isDaytona in sandbox_agent.ts) is carried by the first-committer of that contended file (dominated by a concurrent F-029/F-030 lane). Claude-Session: https://claude.ai/code/session_01GYo3UEfvsZpncagqb28Mbc --- .../cross-service/runner-to-mcp-server.md | 35 ++++-- .../in-service/mcp-models-and-resolution.md | 18 ++- .../agent/src/engines/sandbox_agent/mcp.ts | 110 +++++++++++++++++- .../src/engines/sandbox_agent/run-plan.ts | 15 ++- services/agent/src/tools/mcp-bridge.ts | 14 +++ services/agent/tests/unit/mcp-servers.test.ts | 80 ++++++++++++- .../tests/unit/sandbox-agent-run-plan.test.ts | 79 ++++++++++++- .../tests/unit/session-mcp-layering.test.ts | 58 +++++++++ 8 files changed, 386 insertions(+), 23 deletions(-) diff --git a/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md b/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md index 5fa968ce22..5be1e2e58e 100644 --- a/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md +++ b/docs/design/agent-workflows/interfaces/cross-service/runner-to-mcp-server.md @@ -20,12 +20,16 @@ ONLY "user MCP servers are unsupported"; the internal channel never borrows it. **Gating.** The runner builds MCP servers only when the harness is not Pi and the capability probe reports `mcpTools: true`. Pi always returns an empty MCP set because it gets tools the -native way. +native way. Because Pi cannot consume MCP, a USER MCP server (stdio OR http) attached to a Pi run +would be dropped silently — so `run-plan.ts` refuses any Pi run carrying `mcpServers` up front +with `PI_USER_MCP_UNSUPPORTED_MESSAGE` (fail loud, the way the stdio-MCP and code-tool gates do), +rather than returning a "successful" empty run. A user http MCP is a Claude-only capability. -**The internal gateway-tool channel (delivered, HTTP on loopback).** For a non-Pi harness with -executable tool specs, `buildToolMcpServers` starts a tiny MCP server on `127.0.0.1:` -and returns one ACP `type: "http"` entry (`{name: "agenta-tools", url, headers: []}`). The -server speaks JSON-RPC 2.0 over Streamable-HTTP (stateless JSON mode) and answers three methods: +**The internal gateway-tool channel (delivered, HTTP on loopback — LOCAL only).** For a non-Pi +harness with executable tool specs on the LOCAL sandbox, `buildToolMcpServers` starts a tiny MCP +server on `127.0.0.1:` and returns one ACP `type: "http"` entry +(`{name: "agenta-tools", url, headers: []}`). The server speaks JSON-RPC 2.0 over Streamable-HTTP +(stateless JSON mode) and answers three methods: - `initialize`: returns protocol version and `capabilities.tools`. - `tools/list`: returns the resolved tool specs as MCP tools. Client-kind tools are filtered @@ -38,6 +42,15 @@ the relay dir, and it is bound to loopback. It launches no child process — it already-running runner — so it does not reintroduce the runner-host execution hole that #4831 closed for user stdio MCP. The run end closes it (releases the port). +**On Daytona the internal channel is NOT advertised — the file relay delivers the tools.** The +loopback URL is a runner-host address; on Daytona the harness runs IN the sandbox, where +`127.0.0.1` is the sandbox's own loopback, not the runner's, so the URL is unreachable. +`buildSessionMcpServers` therefore skips the internal channel when `isDaytona` is true and the +already-running file relay (below) delivers the gateway tools instead — the runner's relay loop +polls the sandbox filesystem. This honors the design decision "HTTP advertisement for local, file +relay for Daytona." A user http MCP server (a remote URL the harness dials directly) is NOT +loopback-bound and stays delivered on Daytona unchanged. + **The file relay.** A resolved tool may need to run privately rather than inside the harness process. The relay moves the call across that boundary: the child writes a `.req.json` request into the relay directory, the runner polls (every ~300ms), executes the tool through @@ -55,7 +68,12 @@ allowlist, and permission. Two transports, opposite states: transport, and the wire has no separate `headers` field), so `toAcpMcpServers` emits each `env` entry as an HTTP header (`Authorization: `, etc.). The author names the header via the secret-map key — `secrets: {"Authorization": "linear-mcp-token"}` lands as a header. - The runner builds the ACP `McpServer` `type: "http"` variant (`{name, url, headers}`). + The runner builds the ACP `McpServer` `type: "http"` variant (`{name, url, headers}`). Before + attaching the credential, `validateUserMcpUrl` applies an SSRF guard: the `url` must be `https` + and must not target an internal/metadata host (loopback, link-local incl. `169.254.169.254`, + or private literals), else the run fails loud. A host listed in the optional comma-separated + `AGENTA_AGENT_MCP_HOST_ALLOWLIST` env var opts out of both checks (e.g. a known-safe internal + endpoint). - **Stdio (`transport: "stdio"` + `command`) is disabled.** A stdio server launches an arbitrary process on the runner host, outside the sandbox boundary, so the implementation is disabled (parity with the removed code execution) until its security is fixed. `run-plan.ts` @@ -66,8 +84,9 @@ allowlist, and permission. Two transports, opposite states: - `sdks/python/agenta/sdk/agents/mcp/`: the Python models and resolver. - `services/agent/src/engines/sandbox_agent/mcp.ts`: builds the session's MCP servers (the two - layers; `USER_MCP_UNSUPPORTED_MESSAGE`). -- `services/agent/src/tools/mcp-bridge.ts`: the internal gateway-tool channel builder. + layers; the `isDaytona` guard on the internal channel; `validateUserMcpUrl` SSRF guard). +- `services/agent/src/tools/mcp-bridge.ts`: the internal gateway-tool channel builder; the + `USER_MCP_UNSUPPORTED_MESSAGE` and `PI_USER_MCP_UNSUPPORTED_MESSAGE` refusal constants. - `services/agent/src/tools/tool-mcp-http.ts`: the internal loopback HTTP MCP server. - `services/agent/src/tools/mcp-server.ts`: the removed stdio JSON-RPC server (refusing stub). - `services/agent/src/tools/relay.ts`: the file relay loop and hosts. diff --git a/docs/design/agent-workflows/interfaces/in-service/mcp-models-and-resolution.md b/docs/design/agent-workflows/interfaces/in-service/mcp-models-and-resolution.md index b2d95a711b..daaa38102e 100644 --- a/docs/design/agent-workflows/interfaces/in-service/mcp-models-and-resolution.md +++ b/docs/design/agent-workflows/interfaces/in-service/mcp-models-and-resolution.md @@ -34,12 +34,18 @@ separate `headers` wire field). A missing secret raises under the error policy. **Transport delivery (runner side).** HTTP (`transport: "http"` + `url`) servers are delivered: the runner (`toAcpMcpServers`) reads each resolved `env` entry and emits it as an HTTP request header (so `secrets: {"Authorization": "vault-name"}` becomes an `Authorization` header on the -remote call). Stdio (`transport: "stdio"` + `command`) servers are disabled in the sidecar — a -stdio server runs an arbitrary process on the runner host, outside the sandbox boundary — so a -run carrying one is refused (`USER_MCP_UNSUPPORTED_MESSAGE`). This is the USER MCP capability and -is distinct from the runner's internal gateway-tool MCP channel (delivered over loopback HTTP; -see `runner-to-mcp-server.md`). The SDK models, resolver, and wire are transport-agnostic; the -enable/disable split lives entirely in the runner. +remote call). Before attaching the credential, an SSRF guard (`validateUserMcpUrl`) requires the +`url` to be `https` and to not target an internal/metadata host (loopback, `169.254.169.254`, +private literals); `AGENTA_AGENT_MCP_HOST_ALLOWLIST` opts a host out. Stdio (`transport: "stdio"` ++ `command`) servers are disabled in the sidecar — a stdio server runs an arbitrary process on +the runner host, outside the sandbox boundary — so a run carrying one is refused +(`USER_MCP_UNSUPPORTED_MESSAGE`). On a Pi harness, ANY user MCP server (stdio or http) is refused +up front (`PI_USER_MCP_UNSUPPORTED_MESSAGE`) because Pi delivers tools through its bundled +extension, not MCP — refusing it loudly avoids the silent drop. This is the USER MCP capability and +is distinct from the runner's internal gateway-tool MCP channel (delivered over loopback HTTP on +the local sandbox, and via the file relay on Daytona; see `runner-to-mcp-server.md`). The SDK +models, resolver, and wire are transport-agnostic; the enable/disable split lives entirely in the +runner. ## Owned by diff --git a/services/agent/src/engines/sandbox_agent/mcp.ts b/services/agent/src/engines/sandbox_agent/mcp.ts index bc69ca55e9..cb60a289ec 100644 --- a/services/agent/src/engines/sandbox_agent/mcp.ts +++ b/services/agent/src/engines/sandbox_agent/mcp.ts @@ -29,6 +29,70 @@ export interface McpServerHttp { /** One delivered MCP server: a (disabled) stdio entry or an enabled HTTP entry. */ export type McpServerEntry = McpServerStdio | McpServerHttp; +/** + * SSRF guard for a user HTTP MCP `url`. The runner emits the run's Agenta-resolved named secrets + * as request headers to this author-supplied URL, so an attacker-controlled config could point it + * at an internal/metadata endpoint and exfiltrate a credential (a classic server-side request + * forgery). The capability is flag-gated (`AGENTA_AGENT_ENABLE_MCP`, off by default) and + * config-trust, so a scheme + host guard is enough rather than full DNS-resolution pinning: + * + * - require `https` (the secret rides in a header; `http` would send it in clear text). Opt out + * for a known-safe non-https endpoint by listing its host in `AGENTA_AGENT_MCP_HOST_ALLOWLIST`. + * - reject loopback, link-local (incl. the `169.254.169.254` cloud metadata host), and private + * address literals unless the host is in `AGENTA_AGENT_MCP_HOST_ALLOWLIST` (comma-separated). + * + * Returns an error message string when the URL is rejected, or `undefined` when it is allowed. + */ +function mcpHostAllowlist(): Set { + return new Set( + (process.env.AGENTA_AGENT_MCP_HOST_ALLOWLIST ?? "") + .split(",") + .map((h) => h.trim().toLowerCase()) + .filter(Boolean), + ); +} + +/** True for a hostname/IP literal that must not receive a credentialed request (SSRF sinks). */ +function isInternalHost(host: string): boolean { + const h = host.toLowerCase(); + if (h === "localhost" || h.endsWith(".localhost")) return true; + // IPv6 loopback / unspecified (URL host keeps the brackets, e.g. "[::1]"). + if (h === "[::1]" || h === "[::]" || h === "::1" || h === "::") return true; + // Link-local IPv6 (fe80::/10) and unique-local IPv6 (fc00::/7) literals. + if (/^\[?f[cde]/.test(h)) return true; + // IPv4 dotted-quad literals: loopback, link-local (incl. 169.254.169.254 metadata), private. + const m = h.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); + if (m) { + const [a, b] = [Number(m[1]), Number(m[2])]; + if (a === 127) return true; // loopback + if (a === 169 && b === 254) return true; // link-local + cloud metadata (169.254.169.254) + if (a === 10) return true; // private + if (a === 0) return true; // "this host" + if (a === 172 && b >= 16 && b <= 31) return true; // private + if (a === 192 && b === 168) return true; // private + } + return false; +} + +export function validateUserMcpUrl(rawUrl: string): string | undefined { + let parsed: URL; + try { + parsed = new URL(rawUrl); + } catch { + return `http MCP server url is not a valid URL: ${rawUrl}`; + } + const allowlist = mcpHostAllowlist(); + const host = parsed.hostname.toLowerCase(); + const allowed = allowlist.has(host); + if (parsed.protocol !== "https:" && !allowed) { + return `http MCP server url must use https (got ${parsed.protocol.replace(":", "")}): ${rawUrl}`; + } + if (isInternalHost(host) && !allowed) { + return `http MCP server url targets an internal/metadata host (${host}); not allowed: ${rawUrl}`; + } + return undefined; +} + /** * Convert USER-declared MCP servers into ACP entries. (This is the USER MCP capability layer — * distinct from the INTERNAL gateway-tool channel below; see `buildSessionMcpServers`.) @@ -47,6 +111,8 @@ export type McpServerEntry = McpServerStdio | McpServerHttp; * than being delivered. * - A server that is neither a valid http (no `url`) nor a valid stdio (no `command`) is skipped * with a log — it was never deliverable. + * - An http `url` that fails the SSRF guard (`validateUserMcpUrl`: non-https, or an + * internal/metadata host) throws, so a credentialed request is never sent to an internal sink. */ export function toAcpMcpServers( servers: McpServerConfig[] | undefined, @@ -61,6 +127,10 @@ export function toAcpMcpServers( log(`skipping http MCP server '${s?.name ?? "?"}' (no url)`); continue; } + // SSRF guard: the resolved named secret rides as a header on this author-supplied URL, so + // reject a non-https / internal / metadata target before any credential is attached. + const urlError = validateUserMcpUrl(s.url); + if (urlError) throw new Error(urlError); out.push({ type: "http", name: s.name, @@ -88,6 +158,16 @@ export interface BuildSessionMcpServersInput { isPi: boolean; capabilities: HarnessCapabilities; harness: string; + /** + * True when the run executes in a REMOTE Daytona sandbox (the harness runs IN the sandbox, + * not on the runner host). Gates the internal gateway-tool channel: the channel's loopback + * (`127.0.0.1`) HTTP MCP URL resolves to the SANDBOX's loopback there, not the runner's, so + * advertising it would hand the in-sandbox harness an unreachable URL. On Daytona the channel + * is skipped and gateway tools are delivered through the file relay instead (the relay loop + * already polls the sandbox filesystem on Daytona — see `engines/sandbox_agent.ts`). See the + * Daytona guard in `buildSessionMcpServers`. + */ + isDaytona: boolean; toolSpecs: ResolvedToolSpec[]; userMcpServers?: McpServerConfig[]; relayDir: string; @@ -108,17 +188,24 @@ export interface SessionMcpServers { * them into one switch; project gateway-tool-mcp): * 1. INTERNAL gateway-tool channel (`buildToolMcpServers`): the runner-synthesized loopback HTTP * MCP server that delivers the run's resolved gateway/callback tools to the harness. Carries - * only public metadata; execution relays server-side. RESTORED. + * only public metadata; execution relays server-side. RESTORED — but advertised over a + * loopback (`127.0.0.1`) URL, so it is LOCAL-ONLY. On Daytona the harness runs IN the sandbox, + * where `127.0.0.1` is the sandbox's loopback, not the runner's, so the channel is SKIPPED and + * the tools reach the harness through the file relay instead (the relay loop already works on + * Daytona; gateway-tool-mcp open question 3). This honors the #4844 decision: HTTP advertisement + * for local, file relay for Daytona. * 2. USER MCP capability (`toAcpMcpServers`): the user's own declared servers — stdio DISABLED, - * http delivered (#4834). UNCHANGED. + * http delivered (#4834). UNCHANGED on every sandbox: a user http MCP is a REMOTE url the + * harness dials directly (not a runner loopback), so it stays reachable from a Daytona sandbox. * * Returns a `close()` the caller MUST run when the session ends, to release the internal server's - * loopback port. + * loopback port (a no-op on Daytona, where no internal server starts). */ export async function buildSessionMcpServers({ isPi, capabilities, harness, + isDaytona, toolSpecs, userMcpServers, relayDir, @@ -135,9 +222,22 @@ export async function buildSessionMcpServers({ return { servers: [], close: async () => {} }; } - // Layer 1: INTERNAL gateway-tool channel (do not merge with the user gate below). - const internal = await buildToolMcpServers(toolSpecs, relayDir, log); + // Layer 1: INTERNAL gateway-tool channel (do not merge with the user gate below). LOCAL ONLY: + // its advertised URL is a runner loopback (`127.0.0.1`), unreachable from a remote Daytona + // sandbox where the harness runs. On Daytona, skip the loopback HTTP advertisement and let the + // file relay deliver the tools (the relay loop polls the sandbox filesystem; see the Daytona + // tool relay in `engines/sandbox_agent.ts`). + const internal = isDaytona + ? { servers: [], close: async () => {} } + : await buildToolMcpServers(toolSpecs, relayDir, log); + if (isDaytona && toolSpecs.length > 0) { + log( + `daytona: ${toolSpecs.length} gateway tool(s) delivered via the file relay, not a ` + + `loopback MCP URL (unreachable from the sandbox)`, + ); + } // Layer 2: USER MCP capability (stdio disabled, http delivered; do not merge with Layer 1). + // A user http MCP is a remote url the harness dials directly, so it is delivered on Daytona too. const user = toAcpMcpServers(userMcpServers, log); return { diff --git a/services/agent/src/engines/sandbox_agent/run-plan.ts b/services/agent/src/engines/sandbox_agent/run-plan.ts index 8973eb2c78..4f7c7c55c3 100644 --- a/services/agent/src/engines/sandbox_agent/run-plan.ts +++ b/services/agent/src/engines/sandbox_agent/run-plan.ts @@ -12,7 +12,10 @@ import { } from "../../protocol.ts"; import { executableToolSpecs } from "../../tools/public-spec.ts"; import { CODE_TOOL_UNSUPPORTED_MESSAGE } from "../../tools/code.ts"; -import { USER_MCP_UNSUPPORTED_MESSAGE } from "../../tools/mcp-bridge.ts"; +import { + PI_USER_MCP_UNSUPPORTED_MESSAGE, + USER_MCP_UNSUPPORTED_MESSAGE, +} from "../../tools/mcp-bridge.ts"; import { type MaterializedSkill, resolveSkillDirs as defaultResolveSkillDirs, @@ -212,6 +215,16 @@ export function buildRunPlan( return { ok: false, error: CODE_TOOL_UNSUPPORTED_MESSAGE }; } + // Pi delivers tools through its bundled extension, not over ACP MCP, so a user MCP server on + // a Pi run is DROPPED by `buildSessionMcpServers` (it returns [] for Pi). Dropping it silently + // (no log, HTTP 200) is the F-032 silent-drop bug. Refuse ANY user MCP server (stdio AND http) + // on Pi up front with a Pi-specific message, the way the stdio-MCP and code-tool gates fail + // loud. This MUST precede the harness-agnostic stdio gate so Pi gets the clearer reason for + // both transports (http MCP is otherwise a Claude-only capability, #4834). + if (isPi && (request.mcpServers?.length ?? 0) > 0) { + return { ok: false, error: PI_USER_MCP_UNSUPPORTED_MESSAGE }; + } + // stdio MCP servers run as arbitrary processes on the RUNNER HOST, outside the sandbox // boundary, and the sidecar's stdio MCP implementation is disabled (parity with the removed // code execution) until its security is fixed. Refuse any run carrying one, the way code diff --git a/services/agent/src/tools/mcp-bridge.ts b/services/agent/src/tools/mcp-bridge.ts index 5abed02323..d4338b0e0c 100644 --- a/services/agent/src/tools/mcp-bridge.ts +++ b/services/agent/src/tools/mcp-bridge.ts @@ -35,6 +35,20 @@ export type { ResolvedToolSpec, ToolCallbackContext } from "../protocol.ts"; export const USER_MCP_UNSUPPORTED_MESSAGE = "MCP servers are not supported by the sidecar."; +/** + * Pi-family refusal for ANY user-declared MCP server (stdio AND http). The Pi runtime delivers + * tools through the bundled Agenta extension, not over ACP MCP (`buildSessionMcpServers` returns + * `[]` for Pi), so a user MCP server attached to a Pi run would be DROPPED — silently, with no + * log and an HTTP 200. That is exactly the silent-drop F-032 forbids. The `run-plan.ts` gate + * refuses it up front (the way the stdio-MCP and code-tool gates do) so the failure is loud + * instead of a "successful" empty run. Http MCP is a Claude-only capability (#4834); pick a + * non-Pi harness to use one. + */ +export const PI_USER_MCP_UNSUPPORTED_MESSAGE = + "User MCP servers are not supported on the Pi harness (Pi delivers tools through its bundled " + + "extension, not MCP). Use a non-Pi harness (e.g. claude) for a user MCP server, or remove " + + "mcpServers."; + /** ACP McpServerStdio entry: env is a list of {name, value}. Kept for the disabled user path. */ interface EnvVariable { name: string; diff --git a/services/agent/tests/unit/mcp-servers.test.ts b/services/agent/tests/unit/mcp-servers.test.ts index f39f6694f2..643ea158df 100644 --- a/services/agent/tests/unit/mcp-servers.test.ts +++ b/services/agent/tests/unit/mcp-servers.test.ts @@ -15,7 +15,7 @@ * * Run: pnpm test (or: pnpm exec vitest run tests/unit/mcp-servers.test.ts) */ -import { describe, it } from "vitest"; +import { afterEach, describe, it } from "vitest"; import assert from "node:assert/strict"; import { toAcpMcpServers } from "../../src/engines/sandbox_agent.ts"; @@ -94,3 +94,81 @@ describe("toAcpMcpServers (http enabled, stdio disabled)", () => { assert.equal((out[0] as McpServerHttp).name, "remote"); }); }); + +describe("toAcpMcpServers SSRF guard (http url scheme/host)", () => { + const previousAllowlist = process.env.AGENTA_AGENT_MCP_HOST_ALLOWLIST; + afterEach(() => { + if (previousAllowlist === undefined) + delete process.env.AGENTA_AGENT_MCP_HOST_ALLOWLIST; + else process.env.AGENTA_AGENT_MCP_HOST_ALLOWLIST = previousAllowlist; + }); + + const http = (url: string): McpServerConfig[] => [ + { + name: "s", + transport: "http", + url, + env: { Authorization: "Bearer secret" }, + }, + ]; + + it("rejects a non-https url (the secret would ride in clear text)", () => { + assert.throws( + () => toAcpMcpServers(http("http://mcp.example.com/sse")), + /must use https/, + ); + }); + + it("rejects the cloud metadata host (169.254.169.254)", () => { + assert.throws( + () => toAcpMcpServers(http("https://169.254.169.254/latest/meta-data/")), + /internal\/metadata host/, + ); + }); + + it("rejects localhost and loopback / private literals", () => { + for (const url of [ + "https://localhost/mcp", + "https://127.0.0.1/mcp", + "https://10.0.0.5/mcp", + "https://192.168.1.10/mcp", + "https://172.16.4.4/mcp", + "https://[::1]/mcp", + ]) { + assert.throws( + () => toAcpMcpServers(http(url)), + /internal\/metadata host/, + `should reject ${url}`, + ); + } + }); + + it("rejects a malformed url", () => { + assert.throws(() => toAcpMcpServers(http("not a url")), /not a valid URL/); + }); + + it("allows a public https url unchanged", () => { + const out = toAcpMcpServers(http("https://mcp.linear.app/sse")); + assert.equal(out.length, 1); + assert.equal((out[0] as McpServerHttp).url, "https://mcp.linear.app/sse"); + }); + + it("allowlist opts a host out of the https + internal-host checks", () => { + process.env.AGENTA_AGENT_MCP_HOST_ALLOWLIST = "localhost,10.0.0.5"; + // http://localhost is normally rejected twice over (non-https + internal); allowlisted -> ok. + const out = toAcpMcpServers(http("http://localhost:9000/mcp")); + assert.equal(out.length, 1, "allowlisted host is delivered"); + assert.equal((out[0] as McpServerHttp).url, "http://localhost:9000/mcp"); + // A private IP literal in the allowlist is also permitted. + assert.equal( + toAcpMcpServers(http("https://10.0.0.5/mcp")).length, + 1, + "allowlisted private literal delivered", + ); + // A host NOT in the allowlist is still rejected. + assert.throws( + () => toAcpMcpServers(http("https://127.0.0.1/mcp")), + /internal\/metadata host/, + ); + }); +}); diff --git a/services/agent/tests/unit/sandbox-agent-run-plan.test.ts b/services/agent/tests/unit/sandbox-agent-run-plan.test.ts index 6ddabeaf00..7a901adc25 100644 --- a/services/agent/tests/unit/sandbox-agent-run-plan.test.ts +++ b/services/agent/tests/unit/sandbox-agent-run-plan.test.ts @@ -252,10 +252,10 @@ describe("buildRunPlan", () => { assert.match(result.error, /MCP servers are not supported by the sidecar/); }); - it("errors on a stdio MCP server on the local sandbox too", () => { + it("errors on a stdio MCP server on the local sandbox too (non-Pi harness)", () => { const result = buildRunPlan( { - harness: "pi_core", + harness: "claude", sandbox: "local", prompt: "hello", mcpServers: [{ name: "fs", transport: "stdio", command: "mcp-fs" }], @@ -268,6 +268,81 @@ describe("buildRunPlan", () => { assert.match(result.error, /MCP servers are not supported by the sidecar/); }); + it("errors LOUD on a Pi run carrying a user STDIO MCP server (F-032, Pi-specific)", () => { + // Pi delivers tools through its bundled extension, not MCP, so a user MCP server would be + // dropped silently (the F-032 bug). The Pi gate refuses it up front with a Pi-specific + // message (precedes the harness-agnostic stdio gate). + const result = buildRunPlan( + { + harness: "pi_core", + sandbox: "local", + prompt: "hello", + mcpServers: [{ name: "fs", transport: "stdio", command: "mcp-fs" }], + } as AgentRunRequest, + { createLocalCwd: () => "/tmp/local-cwd" }, + ); + + assert.equal(result.ok, false); + if (result.ok) return; + assert.match(result.error, /not supported on the Pi harness/); + }); + + it("errors LOUD on a Pi run carrying a user HTTP MCP server (F-032 silent-drop fix)", () => { + // The core of F-032: an http user MCP on Pi previously passed the stdio gate, then + // buildSessionMcpServers returned [] for Pi -> the server was dropped with NO log and an + // HTTP 200. It must now fail loud, not silently succeed. Fails before any cwd is created. + let created = false; + const result = buildRunPlan( + { + harness: "pi_agenta", + sandbox: "daytona", + prompt: "hello", + mcpServers: [ + { + name: "linear", + transport: "http", + url: "https://mcp.linear.app/sse", + }, + ], + } as AgentRunRequest, + { + createDaytonaCwd: () => { + created = true; + return "/home/sandbox/agenta-fixed"; + }, + }, + ); + + assert.equal(result.ok, false); + if (result.ok) return; + assert.match(result.error, /not supported on the Pi harness/); + assert.equal( + created, + false, + "fails before any cwd is created (up-front gate)", + ); + }); + + it("allows a non-Pi (claude) run with a user HTTP MCP server (Pi gate is Pi-only)", () => { + const result = buildRunPlan( + { + harness: "claude", + sandbox: "local", + prompt: "hello", + mcpServers: [ + { + name: "linear", + transport: "http", + url: "https://mcp.linear.app/sse", + }, + ], + } as AgentRunRequest, + { createLocalCwd: () => "/tmp/local-cwd" }, + ); + + assert.equal(result.ok, true); + }); + it("errors on any run carrying a code tool (code execution removed, fail loud)", () => { // Code tools were removed for security (F-010). The run is refused up-front so the failure // surfaces as a non-success result (ok:false) rather than being laundered into a 200 reply diff --git a/services/agent/tests/unit/session-mcp-layering.test.ts b/services/agent/tests/unit/session-mcp-layering.test.ts index abbe5b3ac8..48c7f9b9d6 100644 --- a/services/agent/tests/unit/session-mcp-layering.test.ts +++ b/services/agent/tests/unit/session-mcp-layering.test.ts @@ -57,6 +57,7 @@ describe("buildSessionMcpServers layering (do-not-merge regression guard)", () = it("(a) gateway tools + no user MCP -> internal channel present, no throw", async () => { const { servers } = await build({ isPi: false, + isDaytona: false, capabilities: mcpCapable, harness: "claude", toolSpecs: [gatewayTool], @@ -83,6 +84,7 @@ describe("buildSessionMcpServers layering (do-not-merge regression guard)", () = () => buildSessionMcpServers({ isPi: false, + isDaytona: false, capabilities: mcpCapable, harness: "claude", toolSpecs: [], @@ -97,6 +99,7 @@ describe("buildSessionMcpServers layering (do-not-merge regression guard)", () = it("(c) gateway tools + user http MCP -> BOTH delivered; user stdio still refused", async () => { const { servers } = await build({ isPi: false, + isDaytona: false, capabilities: mcpCapable, harness: "claude", toolSpecs: [gatewayTool], @@ -123,6 +126,7 @@ describe("buildSessionMcpServers layering (do-not-merge regression guard)", () = () => buildSessionMcpServers({ isPi: false, + isDaytona: false, capabilities: mcpCapable, harness: "claude", toolSpecs: [gatewayTool], @@ -136,6 +140,7 @@ describe("buildSessionMcpServers layering (do-not-merge regression guard)", () = it("Pi gets [] (native delivery, no MCP channel) even with gateway tools", async () => { const { servers } = await build({ isPi: true, + isDaytona: false, capabilities: mcpCapable, harness: "pi_agenta", toolSpecs: [gatewayTool], @@ -151,6 +156,7 @@ describe("buildSessionMcpServers layering (do-not-merge regression guard)", () = it("a non-MCP harness gets [] (capability gate), no internal server started", async () => { const { servers } = await build({ isPi: false, + isDaytona: false, capabilities: { mcpTools: false, toolCalls: false }, harness: "no-mcp", toolSpecs: [gatewayTool], @@ -162,6 +168,7 @@ describe("buildSessionMcpServers layering (do-not-merge regression guard)", () = it("the internal channel advertisement carries no credential (server-side invariant)", async () => { const { servers } = await build({ isPi: false, + isDaytona: false, capabilities: mcpCapable, harness: "claude", toolSpecs: [gatewayTool], @@ -179,4 +186,55 @@ describe("buildSessionMcpServers layering (do-not-merge regression guard)", () = "the private callRef never reaches the advertisement", ); }); + + it("(Daytona) gateway tools -> NO internal loopback advertisement (file relay delivers them)", async () => { + // On Daytona the harness runs IN the sandbox, so the runner-loopback URL (127.0.0.1) is + // unreachable. The internal channel must NOT be advertised; the file relay (already running + // on Daytona) delivers the tools instead. Asserting the loopback URL is absent is the + // Finding-1 regression guard. + const { servers } = await build({ + isPi: false, + isDaytona: true, + capabilities: mcpCapable, + harness: "claude", + toolSpecs: [gatewayTool], + relayDir, + }); + assert.equal( + servers.find((s) => s.name === "agenta-tools"), + undefined, + "no internal agenta-tools server is advertised on Daytona", + ); + assert.ok( + !JSON.stringify(servers).includes("127.0.0.1"), + "the Daytona session never carries an unreachable loopback MCP url", + ); + }); + + it("(Daytona) a user http MCP is STILL delivered (remote url, not a runner loopback)", async () => { + // The Daytona guard is scoped to the INTERNAL loopback channel only. A user http MCP is a + // remote url the harness dials directly, so it stays reachable from the sandbox and must be + // delivered on Daytona unchanged. + const { servers } = await build({ + isPi: false, + isDaytona: true, + capabilities: mcpCapable, + harness: "claude", + toolSpecs: [gatewayTool], + userMcpServers: [ + { + name: "linear", + transport: "http", + url: "https://mcp.linear.app/sse", + env: { Authorization: "Bearer x" }, + }, + ], + relayDir, + }); + assert.deepEqual( + servers.map((s) => s.name), + ["linear"], + "only the user http server is delivered on Daytona (no internal loopback)", + ); + }); });