diff --git a/services/agent/src/tools/direct.ts b/services/agent/src/tools/direct.ts index a1ccbab12e..448363b02d 100644 --- a/services/agent/src/tools/direct.ts +++ b/services/agent/src/tools/direct.ts @@ -95,8 +95,11 @@ export function deepDelete(target: Record, 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, @@ -105,8 +108,11 @@ export function deepMerge( const out: Record = 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, value); + if (isPlainObject(value)) { + const existing = isPlainObject(out[key]) + ? (out[key] as Record) + : {}; + out[key] = deepMerge(existing, value); } else { out[key] = value; } @@ -167,13 +173,17 @@ export function assembleBody( params: unknown, runContext?: RunContext, ): Record { - // 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 = {}; - 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, signal?: AbortSignal, ): Promise { + // 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 = { "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(); } diff --git a/services/agent/tests/unit/tool-direct.test.ts b/services/agent/tests/unit/tool-direct.test.ts index fadce276f7..49101ff3c9 100644 --- a/services/agent/tests/unit/tool-direct.test.ts +++ b/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 = { 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 = {