fix(agent): address CodeRabbit findings on direct-call dispatch (#4891)

services/agent/src/tools/direct.ts

@@ -95,8 +95,11 @@ export function deepDelete(target: Record<string, unknown>, path: string): void
 /**
  * Recursively merge `overlay` onto `base`. `overlay` WINS on every conflict (so server-fixed
  * fields override the model's args); two plain objects at the same key merge, anything else
- * replaces. Prototype-polluting keys in `overlay` are skipped. Returns a new object; inputs are
- * not mutated.
+ * replaces. Prototype-polluting keys are skipped at EVERY level: a plain-object value is always
+ * merged through this function (onto the existing subtree, or onto a fresh one when the base side
+ * is not an object), so a nested `__proto__`/`constructor`/`prototype` carried inside an untrusted
+ * subtree can never be assigned wholesale. Returns a new object; inputs are not mutated. Passing an
+ * empty `base` (`deepMerge({}, value)`) is the way to deep-sanitize an untrusted object.
  */
 export function deepMerge(
   base: Record<string, unknown>,
@@ -105,8 +108,11 @@ export function deepMerge(
   const out: Record<string, unknown> = isPlainObject(base) ? { ...base } : {};
   for (const [key, value] of Object.entries(overlay)) {
     if (UNSAFE_KEYS.has(key)) continue;
-    if (isPlainObject(value) && isPlainObject(out[key])) {
-      out[key] = deepMerge(out[key] as Record<string, unknown>, value);
+    if (isPlainObject(value)) {
+      const existing = isPlainObject(out[key])
+        ? (out[key] as Record<string, unknown>)
+        : {};
+      out[key] = deepMerge(existing, value);
     } else {
       out[key] = value;
     }
@@ -167,13 +173,17 @@ export function assembleBody(
   params: unknown,
   runContext?: RunContext,
 ): Record<string, unknown> {
-  // 1. Model args, at args_into (deep-set) or the root.
+  // 1. Model args, at args_into (deep-set) or the root. The args are model-generated (untrusted),
+  //    so an object is deep-sanitized first (`deepMerge({}, ...)` strips `__proto__`/`constructor`/
+  //    `prototype` at every level) before it lands at args_into or the root — a bare spread or a
+  //    raw deep-set would carry a nested prototype-polluting key through unchecked.
   let body: Record<string, unknown> = {};
-  const args = params ?? {};
+  const raw = params ?? {};
+  const args = isPlainObject(raw) ? deepMerge({}, raw) : raw;
   if (call.args_into) {
     deepSet(body, call.args_into, args);
   } else if (isPlainObject(args)) {
-    body = { ...args };
+    body = args;
   }
   // 2. Server-fixed fields win over the model's args.
   if (call.body) body = deepMerge(body, call.body);
@@ -209,7 +219,9 @@ export function assembleBody(
  *    `http://host:8000/tools/call`). A non-empty mount must contain the path, so a normalized
  *    escape out of the API surface is rejected; an empty mount (API at the origin root) relies on
  *    the host-lock alone. Deriving the mount instead of hard-coding `/api` is what lets this work
- *    on a self-host where the API is not under `/api`.
+ *    on a self-host where the API is not under `/api`. A callback path that does not end with
+ *    `/tools/call` (after a trailing slash is normalized) is rejected outright — it would otherwise
+ *    derive an empty mount and silently widen the guard, so it FAILS CLOSED instead.
  */
 export function directCallUrl(callbackEndpoint: string, call: DirectCall): string {
   if (!DIRECT_CALL_METHODS.has(call.method)) {
@@ -256,13 +268,20 @@ export function directCallUrl(callbackEndpoint: string, call: DirectCall): strin
       `direct-call path '${path}' resolves outside the run's Agenta origin`,
     );
   }
-  // Confine to the callback's mount (the callback path minus a trailing `/tools/call`). An empty
-  // mount (API at the root) imposes no prefix; a non-empty mount must contain the resolved path,
-  // so a normalized escape like `/api/%2e%2e/admin` -> `/admin` is rejected.
+  // Confine to the callback's mount (the callback path minus a trailing `/tools/call`). The mount
+  // for `http://host:8000/tools/call` is the empty string — the API at the origin root, confined
+  // by the host-lock alone — so an empty mount is legitimate. But a callback path that does NOT end
+  // with `/tools/call` is unexpected (the gateway always posts there), and treating it as an empty
+  // mount would silently widen the guard to any same-origin path. Normalize a trailing slash, then
+  // FAIL CLOSED on any other shape rather than degrade the guard.
   const CALLBACK_PATH_SUFFIX = "/tools/call";
-  const mount = base.pathname.endsWith(CALLBACK_PATH_SUFFIX)
-    ? base.pathname.slice(0, -CALLBACK_PATH_SUFFIX.length)
-    : "";
+  const callbackPath = base.pathname.replace(/\/+$/, "");
+  if (!callbackPath.endsWith(CALLBACK_PATH_SUFFIX)) {
+    throw new Error(
+      `cannot derive Agenta API mount from callback endpoint '${callbackEndpoint}'`,
+    );
+  }
+  const mount = callbackPath.slice(0, -CALLBACK_PATH_SUFFIX.length);
   if (
     mount &&
     resolved.pathname !== mount &&
@@ -292,6 +311,17 @@ export async function callDirect(
   body: Record<string, unknown>,
   signal?: AbortSignal,
 ): Promise<string> {
+  // A GET sends no request body (fetch forbids it), so a non-empty assembled body would be silently
+  // dropped — any model args or `args_into`/`body`/`context` field on a GET descriptor would never
+  // reach the endpoint. Fail fast rather than execute a call that quietly loses its inputs. No
+  // resolver emits a GET `call` with a body today; this guards a future one until a GET descriptor
+  // defines a query-parameter mapping.
+  if (method === "GET" && Object.keys(body).length > 0) {
+    throw new Error(
+      "direct-call GET cannot carry a request body; a GET descriptor must map inputs to query parameters",
+    );
+  }
+
   const headers: Record<string, string> = {
     "content-type": "application/json",
   };
@@ -327,14 +357,16 @@ export async function callDirect(
     throw new Error("direct tool call failed");
   }
 
-  const bodyText = await response.text();
   if (!response.ok) {
-    // Keep the internal URL and the upstream response body server-side; the model gets only the
-    // status code. (`redirect: "manual"` makes a 3xx a non-ok response, so it lands here too.)
+    // The model gets only the status code. The upstream response body can carry user data or tool
+    // outputs, so it is never read or logged on this error path; the URL (the run's own Agenta
+    // endpoint — no credentials, the authorization header is never logged) stays, since it is the
+    // load-bearing detail for diagnosing the SSRF-guarded direct path. (`redirect: "manual"` makes
+    // a 3xx a non-ok response, so it lands here too.)
     console.error(
-      `direct tool call ${method} ${url} returned HTTP ${response.status}: ${bodyText.slice(0, 500)}`,
+      `direct tool call ${method} ${url} returned HTTP ${response.status}`,
     );
     throw new Error(`direct tool call failed: HTTP ${response.status}`);
   }
-  return bodyText;
+  return response.text();
 }

services/agent/tests/unit/tool-direct.test.ts

@@ -1,15 +1,18 @@
 /**
- * Unit tests for direct-call tools (tools/direct.ts) and the two dispatch branches that use it
- * (tools/dispatch.ts `runResolvedTool`, tools/relay.ts `startToolRelay` -> `executeRelayedTool`).
+ * Unit tests for direct-call tools (tools/direct.ts) and the live dispatch branch that uses it
+ * (tools/relay.ts `startToolRelay` -> `executeRelayedTool`). The symmetric `tools/dispatch.ts`
+ * `runResolvedTool` host-direct branch is deferred (see direct.ts), so it is not exercised here.
  *
  * A resolved callback tool can carry a `call` descriptor; when it does the runner calls the
  * Agenta endpoint directly instead of routing through /tools/call. These tests cover:
- *  - assembleBody: args_into deep-set, the fixed-wins overlay, the root merge, and
- *    prototype-pollution-safe assignment.
- *  - directCallUrl (the SSRF guard): method allowlist, the /api-relative path rule, traversal /
- *    protocol-relative / absolute-URL rejection, and origin binding to the run's callback endpoint.
- *  - the dispatch branch (runResolvedTool, host-direct) and the relay branch (startToolRelay,
- *    Daytona host) with FAKE `call` specs and a mocked global fetch.
+ *  - assembleBody: args_into deep-set, the fixed-wins overlay, the root merge, the run-context
+ *    binding, and prototype-pollution-safe assignment (including nested unsafe keys).
+ *  - directCallUrl (the SSRF guard): method allowlist, the mount-agnostic absolute-path rule,
+ *    traversal / protocol-relative / absolute-URL rejection, origin binding (host-lock) to the
+ *    run's callback endpoint, and failing closed when the callback mount cannot be derived.
+ *  - callDirect: the redirect:manual round-trip and the GET-with-body fail-fast guard.
+ *  - the relay branch (startToolRelay -> executeRelayedTool) with FAKE `call` specs and a mocked
+ *    global fetch.
  *
  * No network and no harness: `globalThis.fetch` is stubbed per test and restored after.
  *
@@ -23,6 +26,7 @@ import { join } from "node:path";
 
 import {
   assembleBody,
+  callDirect,
   deepDelete,
   deepMerge,
   deepSet,
@@ -161,6 +165,28 @@ describe("assembleBody", () => {
     assert.equal((body as any).polluted, undefined);
     assert.equal(({} as any).polluted, undefined);
   });
+
+  it("strips a nested unsafe key from the model args at the root", () => {
+    // Untrusted model args with a NESTED "__proto__" must be sanitized at every level, not just
+    // the top. JSON.parse makes the "__proto__" an own key (an object literal would set the proto).
+    const call: DirectCall = { method: "POST", path: "/api/x" };
+    const args = JSON.parse('{"nested": {"__proto__": {"polluted": true}, "ok": 1}}');
+    const body = assembleBody(call, args);
+    assert.deepEqual(body, { nested: { ok: 1 } });
+    assert.equal(({} as any).polluted, undefined);
+  });
+
+  it("strips a nested unsafe key from the model args at args_into", () => {
+    const call: DirectCall = {
+      method: "POST",
+      path: "/api/x",
+      args_into: "data.inputs",
+    };
+    const args = JSON.parse('{"__proto__": {"polluted": true}, "city": "Paris"}');
+    const body = assembleBody(call, args);
+    assert.deepEqual(body, { data: { inputs: { city: "Paris" } } });
+    assert.equal(({} as any).polluted, undefined);
+  });
 });
 
 // ---------------------------------------------------------------------------
@@ -337,6 +363,15 @@ describe("deepSet / deepMerge", () => {
     assert.deepEqual(base, { a: { x: 1 }, keep: 1 }, "base is untouched");
   });
 
+  it("deepMerge strips a nested unsafe key even when the base side is not an object", () => {
+    // The overlay subtree lands on a non-object base key, so it cannot merge onto an existing
+    // subtree — it must still be sanitized through deepMerge rather than assigned wholesale.
+    const overlay = JSON.parse('{"a": {"__proto__": {"polluted": true}, "ok": 1}}');
+    const out = deepMerge({ a: 5 }, overlay);
+    assert.deepEqual(out, { a: { ok: 1 } });
+    assert.equal(({} as any).polluted, undefined);
+  });
+
   it("deepDelete removes a nested leaf and no-ops a missing parent, proto-safely", () => {
     const target: Record<string, unknown> = { a: { b: 1, c: 2 }, keep: 3 };
     deepDelete(target, "a.b");
@@ -396,6 +431,27 @@ describe("directCallUrl", () => {
     assert.equal(url, "http://host:8000/workflows/invoke");
   });
 
+  it("normalizes a trailing slash on the callback endpoint before deriving the mount", () => {
+    const url = directCallUrl("https://agenta.example/api/tools/call/", {
+      method: "POST",
+      path: "/api/workflows/invoke",
+    });
+    assert.equal(url, "https://agenta.example/api/workflows/invoke");
+  });
+
+  it("fails closed when the callback path does not end with /tools/call", () => {
+    // An unexpected callback shape would derive an empty mount and silently widen the guard to any
+    // same-origin path; reject it instead.
+    assert.throws(
+      () =>
+        directCallUrl("https://agenta.example/api/health", {
+          method: "POST",
+          path: "/api/workflows/invoke",
+        }),
+      /cannot derive Agenta API mount from callback endpoint/,
+    );
+  });
+
   it("rejects a same-origin path outside the callback's mount", () => {
     assert.throws(
       () => directCallUrl(ENDPOINT, { method: "POST", path: "/secrets" }),
@@ -445,6 +501,31 @@ describe("directCallUrl", () => {
   });
 });
 
+// ---------------------------------------------------------------------------
+// callDirect (HTTP round-trip)
+// ---------------------------------------------------------------------------
+
+describe("callDirect", () => {
+  it("fails fast on a GET with a non-empty body (inputs would be silently dropped)", async () => {
+    // fetch forbids a GET body, so a GET descriptor with assembled inputs must not run — it would
+    // execute without them. The guard throws before fetch is ever called.
+    const calls = stubFetch("never");
+    await assert.rejects(
+      callDirect("GET", "https://agenta.example/api/x", undefined, { city: "Paris" }),
+      /GET cannot carry a request body/,
+    );
+    assert.equal(calls.length, 0, "fetch is never called");
+  });
+
+  it("allows a GET with an empty body", async () => {
+    const calls = stubFetch("ok");
+    const out = await callDirect("GET", "https://agenta.example/api/x", undefined, {});
+    assert.equal(out, "ok");
+    assert.equal(calls.length, 1);
+    assert.equal(calls[0].init.body, undefined, "no GET body is sent");
+  });
+});
+
 // The reference-tool spec reused by the live dispatch tests below: a stored workflow invoked as
 // a tool (args at data.inputs, the resolved revision baked into the fixed body).
 const refSpec: ResolvedToolSpec = {