From c264c9e24f5a2ae0cbdf8e9aa01cb132c891f495 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Wed, 19 Aug 2026 22:59:51 -0500 Subject: [PATCH] Check where an agent address redirects to, not just where it starts `checkAgentEndpoint` decides whether this deployment is willing to talk to an address, and then the request was handed to a fetch that follows redirects. The address that was checked and the address that was dialled were therefore only the same address while nobody redirected. A registrable agent at `https://agent.example.com/ag-ui` answering `307 Location: http://169.254.169.254/latest/meta-data/` put the server on its own cloud metadata endpoint, which the check refuses under every configuration. Both places that dial an agent are affected, and the second is the worse one. The connection test runs once at registration; the runtime dials the stored endpoint on every single run, carrying whatever auth header the registration supplied, so a redirect added after approval is an ongoing exposure rather than a one-off. `createAgentFetch` applies the check to each hop. 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 ordinary case; each destination goes through `checkAgentEndpoint` first, so following one can only reach somewhere registering it directly would have reached. Three hops, then it gives up. Method and body are carried across hops. A browser turns a redirected POST into a GET, and doing that here would only ever produce a confusing "that is not an AG-UI endpoint" from an agent that is one. The stall guard already accepted an inner fetch, so the two compose: a deployment with a timeout configured gets the watch and the redirect check rather than whichever was wired last. --- server/src/agents/connection-test.ts | 20 +++++- server/src/agents/endpoint.ts | 76 +++++++++++++++++++++ server/src/copilot.ts | 37 +++++++++-- server/src/index.ts | 10 ++- server/tests/agent-connection-live.test.ts | 72 ++++++++++++++++++++ server/tests/copilot.test.ts | 77 ++++++++++++++++++++++ 6 files changed, 283 insertions(+), 9 deletions(-) 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. *