diff --git a/server/src/agents/connection-test.ts b/server/src/agents/connection-test.ts index 377d6f1..03cbf2a 100644 --- a/server/src/agents/connection-test.ts +++ b/server/src/agents/connection-test.ts @@ -1,4 +1,8 @@ -import { checkAgentEndpoint } from "./endpoint"; +import { + checkAgentEndpoint, + createAgentFetch, + EndpointRedirectError, +} from "./endpoint"; /** * Ask an endpoint whether it is really an agent before it is stored. @@ -97,7 +101,14 @@ export async function testAgentConnection( }); if (!verdict.allowed) return { ok: false, reason: verdict.reason }; - const doFetch = options.fetchImpl ?? fetch; + // Wrapped rather than called directly, so the address the request finally lands on is checked too. + // Checking only what the person typed leaves the redirect as the way around it. + const doFetch = createAgentFetch({ + ...(options.allowPrivateHosts !== undefined + ? { allowPrivateHosts: options.allowPrivateHosts } + : {}), + ...(options.fetchImpl ? { fetchImpl: options.fetchImpl } : {}), + }); let response: Response; try { response = await doFetch(verdict.url, { @@ -111,6 +122,11 @@ export async function testAgentConnection( signal: AbortSignal.timeout(options.timeoutMs ?? TEST_TIMEOUT_MS), }); } catch (error) { + // A refused redirect is a specific thing that happened, and the person registering can act on it: + // it names where their address sent us. + if (error instanceof EndpointRedirectError) { + return { ok: false, reason: error.message }; + } const timedOut = error instanceof Error && error.name === "TimeoutError"; return { ok: false, diff --git a/server/src/agents/endpoint.ts b/server/src/agents/endpoint.ts index e3777ed..c9b86f5 100644 --- a/server/src/agents/endpoint.ts +++ b/server/src/agents/endpoint.ts @@ -55,3 +55,79 @@ export function checkAgentEndpoint( return { allowed: true, url: verdict.url }; } + +/** + * How many redirects an agent is allowed before we stop believing it has somewhere to be. + * + * Three, which covers the ordinary shapes (`http` to `https`, a host rename, a trailing-slash + * canonicalisation) and stops a chain that has no end. + */ +const MAX_REDIRECTS = 3; + +/** A redirect this deployment will not follow, named so the person registering sees which hop. */ +export class EndpointRedirectError extends Error { + constructor(message: string) { + super(message); + this.name = "EndpointRedirectError"; + } +} + +const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); + +/** + * `fetch`, with the endpoint check applied to every hop rather than only the address a person typed. + * + * Checking the URL once and then handing it to a fetch that follows redirects is a check with a hole + * in it: `https://agent.example.com/ag-ui` passes, answers `307`, and the request lands wherever the + * `Location` header says, which is how a registrable agent becomes a way to read the deployment's own + * cloud metadata. The address that gets dialled is the one that must be allowed, and a redirect makes + * those two different addresses. + * + * Redirects are followed rather than refused, because a deployment that puts its agent behind one has + * done nothing wrong and `http` to `https` is the common case. Each destination goes through + * {@link checkAgentEndpoint} first, so following one can only ever reach somewhere registering it + * directly would have been allowed to reach. + * + * The method and body are carried across every hop. A browser turns a redirected `POST` into a `GET`; + * doing that here would only ever produce a confusing "that is not an AG-UI endpoint" from an agent + * that is one, because AG-UI is a POST protocol and this is a server talking to an API, not a person + * following a link. + */ +export function createAgentFetch( + options: { allowPrivateHosts?: boolean; fetchImpl?: typeof fetch } = {}, +): (url: string, init?: RequestInit) => Promise { + const doFetch = options.fetchImpl ?? fetch; + + return async function guardedFetch(url: string, init?: RequestInit) { + let target = url; + + for (let hop = 0; hop <= MAX_REDIRECTS; hop += 1) { + // `manual` is what makes this a check rather than a comment: the caller sees the redirect, and + // the underlying fetch cannot quietly follow one on its own. + const response = await doFetch(target, { ...init, redirect: "manual" }); + if (!REDIRECT_STATUSES.has(response.status)) return response; + + const location = response.headers.get("location"); + // A redirect status with nowhere to go is just an answer. Whatever it means, it is the + // agent's own reply and not a hop. + if (!location) return response; + + const next = new URL(location, target).toString(); + const verdict = checkAgentEndpoint(next, { + ...(options.allowPrivateHosts !== undefined + ? { allowPrivateHosts: options.allowPrivateHosts } + : {}), + }); + if (!verdict.allowed) { + throw new EndpointRedirectError( + `That address redirected to ${next}, and ${verdict.reason.charAt(0).toLowerCase()}${verdict.reason.slice(1)}`, + ); + } + target = verdict.url; + } + + throw new EndpointRedirectError( + `That address redirected more than ${MAX_REDIRECTS} times without arriving anywhere.`, + ); + }; +} diff --git a/server/src/copilot.ts b/server/src/copilot.ts index 0229768..1b020f6 100644 --- a/server/src/copilot.ts +++ b/server/src/copilot.ts @@ -6,10 +6,10 @@ import { CopilotRuntime, } from "@copilotkit/runtime/v2"; import { createCopilotHonoHandler } from "@copilotkit/runtime/v2/hono"; +import { z } from "zod"; import type { AgentActor } from "./agents/profile-types"; -import type { StallGuard } from "./channels/stall-guard"; +import type { AgentFetch, StallGuard } from "./channels/stall-guard"; import type { DeploymentConfig } from "./config"; -import { z } from "zod"; import type { GrantedTool } from "./plugins/tools"; /** @@ -228,12 +228,28 @@ export async function buildAgents( /** Absent leaves every Bot with no tools, which is the correct answer when nothing is granted. */ loadTools: LoadToolsForBot = async () => [], signRun?: SignRun, + /** + * The fetch a remote agent is dialled with. + * + * Absent uses the runtime's own, which follows redirects wherever they point. A deployment passes + * one that re-checks each hop, because the address a registration was validated against and the + * address a run finally reaches are only the same address while nobody redirects. + */ + agentFetch?: AgentFetch, ): Promise> { return Object.fromEntries( await Promise.all( agents.map(async (agent) => [ agent.id, - await buildAgent(agent, model, apiKey, stallGuard, loadTools, signRun), + await buildAgent( + agent, + model, + apiKey, + stallGuard, + loadTools, + signRun, + agentFetch, + ), ]), ), ); @@ -246,6 +262,7 @@ async function buildAgent( stallGuard: StallGuard | undefined, loadTools: LoadToolsForBot, signRun?: SignRun, + agentFetch?: AgentFetch, ): Promise { if (agent.type === "built_in") { return new BuiltInAgent( @@ -265,6 +282,7 @@ async function buildAgent( stallGuard, await loadTools(agent.id), signRun, + agentFetch, ); } @@ -292,6 +310,7 @@ function remoteAgentWithStandingRole( */ tools: GrantedTool[] = [], signRun?: SignRun, + agentFetch?: AgentFetch, ) { const remote = new HttpAgent({ url: agent.endpoint, @@ -299,9 +318,18 @@ function remoteAgentWithStandingRole( // The customer's own key, if their agent sits behind one. `HttpAgentConfig` is // `{ url, headers?, fetch? }`, verified against @ag-ui/client 0.0.57. ...(agent.headers ? { headers: agent.headers } : {}), + // The watch wraps whichever fetch is underneath, so a deployment gets both the stall timeout and + // the redirect check rather than having to choose. ...(stallGuard - ? { fetch: stallGuard.watch({ id: agent.id, name: agent.name }) } - : {}), + ? { + fetch: stallGuard.watch( + { id: agent.id, name: agent.name }, + agentFetch, + ), + } + : agentFetch + ? { fetch: agentFetch } + : {}), }); remote.use((input, next) => next.run({ @@ -389,6 +417,7 @@ export async function resolveRuntimeAgents( stallGuard?: StallGuard, loadTools?: LoadToolsForBot, signRun?: SignRun, + agentFetch?: AgentFetch, ): Promise> { const registered = await loadAgents(); if (registered.length === 0) { @@ -400,7 +429,15 @@ export async function resolveRuntimeAgents( const apiKey = registered.some((agent) => agent.type === "built_in") ? await resolveModelApiKey() : null; - return buildAgents(registered, model, apiKey, stallGuard, loadTools, signRun); + return buildAgents( + registered, + model, + apiKey, + stallGuard, + loadTools, + signRun, + agentFetch, + ); } /** What one Bot may call, for the person whose request this is. */ @@ -445,6 +482,7 @@ export function createRequestAgents( loadToolsForActor?: (actorId: string) => LoadToolsForBot, /** Resolved per request, because what it signs is who this request turned out to be. */ signRunForActor?: (actorId: string) => SignRun, + agentFetch?: AgentFetch, ) { return async ({ request }: { request: Request }) => { const actor = await identifyActor(request); @@ -455,6 +493,7 @@ export function createRequestAgents( stallGuard, loadToolsForActor?.(actor.id), signRunForActor?.(actor.id), + agentFetch, ); }; } @@ -481,6 +520,8 @@ export function mountCopilotRuntime( stallGuard: StallGuard, loadToolsForActor?: (actorId: string) => LoadToolsForBot, signRunForActor?: (actorId: string) => SignRun, + /** The fetch remote agents are dialled with. See {@link buildAgents}. */ + agentFetch?: AgentFetch, basePath = "/api/copilotkit", ) { const { intelligence } = config.runtime; @@ -513,6 +554,7 @@ export function mountCopilotRuntime( stallGuard, loadToolsForActor, signRunForActor, + agentFetch, ) as never, }); diff --git a/server/src/index.ts b/server/src/index.ts index 4cdac0b..6402131 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -1,5 +1,6 @@ import { serve } from "bun"; import { mintRunAssertion } from "./agents/callback-token"; +import { createAgentFetch } from "./agents/endpoint"; import { createAgentProfileStore } from "./agents/profile-store"; import { createRuntimeAgentLoader } from "./agents/runtime-agents"; import { createApp } from "./app"; @@ -400,6 +401,13 @@ const app = createApp( */ (actorId) => (botId, runId) => mintRunAssertion({ botId, actorId, runId }, config.keyEncryptionKey), + // Every run dials the stored endpoint again, so the check that was applied when it was + // registered has to be applied to wherever it redirects now. + // Absent computer configuration means nothing opted into private hosts, which is the safe + // reading and the same one `createApp` takes. + createAgentFetch({ + allowPrivateHosts: config.computer?.allowPrivateHosts === true, + }), ), // The only path to an acting call. computerGateway, diff --git a/server/tests/agent-connection-live.test.ts b/server/tests/agent-connection-live.test.ts index b425c04..3b2e946 100644 --- a/server/tests/agent-connection-live.test.ts +++ b/server/tests/agent-connection-live.test.ts @@ -60,6 +60,78 @@ describe("registering an agent that really answers", () => { expect(result.ok).toBe(false); }); + test("a redirect cannot carry the request somewhere the check refuses", async () => { + // The check only ever sees the URL a person typed. Following a redirect blindly makes that check + // decorative: anything registrable can bounce the server at the metadata endpoint. + const bounced: string[] = []; + const redirector = Bun.serve({ + port: 0, + fetch: (request) => { + bounced.push(request.url); + return new Response(null, { + status: 307, + headers: { location: "http://169.254.169.254/latest/meta-data/" }, + }); + }, + }); + + try { + const result = await testAgentConnection( + `http://127.0.0.1:${redirector.port}/ag-ui`, + { allowPrivateHosts: true, timeoutMs: 4_000 }, + ); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.reason).toMatch(/redirect/i); + // The first hop is the registered address and is expected. Nothing beyond it should have been + // dialled, which is what the refusal is for. + expect(bounced.length).toBe(1); + } finally { + redirector.stop(true); + } + }); + + test("a redirect to an address the check permits is still followed", async () => { + // A deployment that puts its agent behind a redirect, http to https being the ordinary case, has + // done nothing wrong. Refusing every redirect would break it, so the destination is checked + // rather than the hop count. + const redirector = Bun.serve({ + port: 0, + fetch: () => + new Response(null, { status: 307, headers: { location: url } }), + }); + + try { + const result = await testAgentConnection( + `http://127.0.0.1:${redirector.port}/ag-ui`, + { allowPrivateHosts: true, timeoutMs: 8_000 }, + ); + + expect(result.ok).toBe(true); + if (result.ok) expect(result.events).toContain("RUN_STARTED"); + } finally { + redirector.stop(true); + } + }); + + test("a redirect that never arrives anywhere gives up rather than looping", async () => { + const looper = Bun.serve({ + port: 0, + fetch: (request) => + new Response(null, { status: 307, headers: { location: request.url } }), + }); + + try { + const result = await testAgentConnection( + `http://127.0.0.1:${looper.port}/ag-ui`, + { allowPrivateHosts: true, timeoutMs: 8_000 }, + ); + expect(result.ok).toBe(false); + } finally { + looper.stop(true); + } + }); + test("a port with nothing on it reports the direction of the connection", async () => { // The server dials the agent, so localhost must be tested from the server side. const dead = await testAgentConnection("http://127.0.0.1:9/", { diff --git a/server/tests/copilot.test.ts b/server/tests/copilot.test.ts index 5bd14d4..adc5aa6 100644 --- a/server/tests/copilot.test.ts +++ b/server/tests/copilot.test.ts @@ -205,6 +205,99 @@ describe("registered Copilot agents", () => { expect(agents.risk).toBeInstanceOf(HttpAgent); }); + /* + * The dialling fetch reaches a remote Bot, through the guard and without one. + * + * Same sentinel trick as below, and for the same reason. This is the wiring that keeps the endpoint + * check applied at run time: a registration is validated once, and every run afterwards dials that + * address again, so the fetch that follows a redirect has to be the one that re-checks where it + * goes. + */ + test("dials a remote Bot with the fetch it was given, guarded or not", async () => { + const dialler = async () => new Response(null); + const registered = [ + { + id: "risk", + name: "Risk", + type: "remote_ag_ui" as const, + endpoint: "http://risk.internal/ag-ui", + }, + ]; + const model = { provider: "openai" as const, defaultModel: "gpt-4.1" }; + + const plain = ( + await buildAgents( + registered, + model, + null, + undefined, + undefined, + undefined, + dialler, + ) + ).risk; + if (!(plain instanceof HttpAgent)) + throw new Error("Expected the remote agent"); + expect(plain.fetch).toBe(dialler); + + // With a timeout configured the watch wraps it, so the guard is handed the dialling fetch rather + // than replacing it. A deployment gets both, not whichever was wired last. + let handed: unknown; + const watched = ( + await buildAgents( + registered, + model, + null, + { + watch: (_bot: { id: string; name: string }, inner?: unknown) => { + handed = inner; + return dialler; + }, + stop: () => undefined, + } as never, + undefined, + undefined, + dialler, + ) + ).risk; + if (!(watched instanceof HttpAgent)) + throw new Error("Expected the remote agent"); + expect(handed).toBe(dialler); + }); + + /* + * The same fetch, but arriving the way the server actually builds agents. + * + * `buildAgents` is not what the runtime calls; `resolveRuntimeAgents` is, and it takes the fetch as + * its own parameter. A parameter accepted and not forwarded looks identical from the outside to one + * that works, and the run would quietly go back to the runtime's own fetch, which follows a + * redirect anywhere. + */ + test("carries the dialling fetch through resolveRuntimeAgents", async () => { + const dialler = async () => new Response(null); + const agents = await resolveRuntimeAgents( + async () => [ + { + id: "risk", + name: "Risk", + type: "remote_ag_ui" as const, + endpoint: "http://risk.internal/ag-ui", + }, + ], + { provider: "openai" as const, defaultModel: "gpt-4.1" }, + async () => null, + undefined, + undefined, + undefined, + dialler, + ); + + const risk = agents.risk; + if (!(risk instanceof HttpAgent)) + throw new Error("Expected the remote agent"); + expect(risk.fetch).toBe(dialler); + }); + /* * Told apart by a sentinel, because nothing else tells them apart. *