fix(agent): harden MCP delivery — Daytona loopback guard, Pi user-MCP fail-loud, http SSRF guard

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:<ephemeral>`
-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:<ephemeral>` 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 `<id>.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: <token>`, 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.

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
 

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<string> {
+  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 {

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

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;