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 2425823..950ccde 100644 --- a/server/src/copilot.ts +++ b/server/src/copilot.ts @@ -7,7 +7,7 @@ import { } from "@copilotkit/runtime/v2"; import { createCopilotHonoHandler } from "@copilotkit/runtime/v2/hono"; 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"; /** @@ -196,11 +196,19 @@ export function buildAgents( apiKey: string | null, /** Absent leaves every stream unwatched, which is what an unconfigured timeout means. */ stallGuard?: StallGuard, + /** + * 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, ): Record { return Object.fromEntries( agents.map((agent) => [ agent.id, - buildAgent(agent, model, apiKey, stallGuard), + buildAgent(agent, model, apiKey, stallGuard, agentFetch), ]), ); } @@ -210,6 +218,7 @@ function buildAgent( model: RuntimeModel, apiKey: string | null, stallGuard?: StallGuard, + agentFetch?: AgentFetch, ): AbstractAgent { if (agent.type === "built_in") { return new BuiltInAgent(builtInAgentConfiguration(agent, model, apiKey)); @@ -217,7 +226,7 @@ function buildAgent( if (agent.type === "unavailable") { return new UnavailableAgent(agent); } - return remoteAgentWithStandingRole(agent, stallGuard); + return remoteAgentWithStandingRole(agent, stallGuard, agentFetch); } /** @@ -235,6 +244,7 @@ function buildAgent( function remoteAgentWithStandingRole( agent: RegisteredRemoteAgent, stallGuard?: StallGuard, + agentFetch?: AgentFetch, ) { const remote = new HttpAgent({ url: agent.endpoint, @@ -242,9 +252,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({ @@ -280,6 +299,7 @@ export async function resolveRuntimeAgents( model: RuntimeModel, resolveModelApiKey: () => Promise, stallGuard?: StallGuard, + agentFetch?: AgentFetch, ): Promise> { const registered = await loadAgents(); if (registered.length === 0) { @@ -291,7 +311,7 @@ export async function resolveRuntimeAgents( const apiKey = registered.some((agent) => agent.type === "built_in") ? await resolveModelApiKey() : null; - return buildAgents(registered, model, apiKey, stallGuard); + return buildAgents(registered, model, apiKey, stallGuard, agentFetch); } /** Who is asking. Agent visibility is decided per person, so a run has to know this first. */ @@ -320,6 +340,7 @@ export function createRequestAgents( * that opened it has been answered. */ stallGuard?: StallGuard, + agentFetch?: AgentFetch, ) { return async ({ request }: { request: Request }) => { const actor = await identifyActor(request); @@ -328,6 +349,7 @@ export function createRequestAgents( model, resolveModelApiKey, stallGuard, + agentFetch, ); }; } @@ -352,6 +374,8 @@ export function mountCopilotRuntime( * there is no reason for a caller to have to say `undefined` here to reach `basePath`. */ stallGuard: StallGuard, + /** The fetch remote agents are dialled with. See {@link buildAgents}. */ + agentFetch?: AgentFetch, basePath = "/api/copilotkit", ) { const { intelligence } = config.runtime; @@ -377,6 +401,7 @@ export function mountCopilotRuntime( model, resolveModelApiKey, stallGuard, + agentFetch, ) as never, }); diff --git a/server/src/index.ts b/server/src/index.ts index 3fc067a..a25abce 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -1,4 +1,5 @@ import { serve } from "bun"; +import { createAgentFetch } from "./agents/endpoint"; import { createAgentProfileStore } from "./agents/profile-store"; import { createRuntimeAgentLoader } from "./agents/runtime-agents"; import { createApp } from "./app"; @@ -12,9 +13,9 @@ import { startChannelActivityListener, } from "./channels/events"; import { createChannelStore } from "./channels/routes"; +import { websocket as channelSocket } from "./channels/socket"; import { createStallGuard } from "./channels/stall-guard"; import { createThreadIdentity } from "./channels/thread-identity"; -import { websocket as channelSocket } from "./channels/socket"; import { createSandboxedStore } from "./components/sandboxed"; import { createComponentStore } from "./components/store"; import { createComputerClient } from "./computer/client"; @@ -333,6 +334,13 @@ const app = createApp( identifyUser, identifyActor, stallGuard, + // 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, + }), ), computerClient, // The only path to an acting call. diff --git a/server/tests/agent-connection-live.test.ts b/server/tests/agent-connection-live.test.ts index 8427a2e..99ea11c 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 a2951ba..574d33a 100644 --- a/server/tests/copilot.test.ts +++ b/server/tests/copilot.test.ts @@ -205,6 +205,83 @@ 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", () => { + 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 = buildAgents(registered, model, null, 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 = buildAgents( + registered, + model, + null, + { + watch: (_bot: { id: string; name: string }, inner?: unknown) => { + handed = inner; + return dialler; + }, + stop: () => undefined, + } as never, + 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, + 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. *