From 6d7e3bf14cea7f7fcc038ac296c845e4974b1ca9 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 1/3] 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 | 36 +++++++- server/src/index.ts | 8 ++ server/tests/agent-connection-live.test.ts | 72 +++++++++++++++ server/tests/copilot.test.ts | 102 +++++++++++++++++++++ 6 files changed, 309 insertions(+), 5 deletions(-) diff --git a/server/src/agents/connection-test.ts b/server/src/agents/connection-test.ts index 377d6f14..03cbf2a2 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 e3777ed6..c9b86f59 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 919ef3b8..c8f1c6e3 100644 --- a/server/src/copilot.ts +++ b/server/src/copilot.ts @@ -15,7 +15,7 @@ import { PROVENANCE_GUIDANCE, } from "../../shared/bot-prompt"; 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 type { SelectableSkill, Selection } from "./plugins/selection"; import { @@ -296,6 +296,14 @@ export async function buildAgents( loadVendors: () => Promise = async () => [], /** How a run's tools are narrowed to what it is about. Absent means they are not. */ selection?: ToolSelection, + /** + * 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> { const vendors = await loadVendors().catch(() => [] as readonly string[]); return Object.fromEntries( @@ -312,6 +320,7 @@ export async function buildAgents( computerGuidance, vendors, selection, + agentFetch, ), ]), ), @@ -328,6 +337,7 @@ async function buildAgent( computerGuidance?: string, connectedVendors: readonly string[] = [], selection?: ToolSelection, + agentFetch?: AgentFetch, ): Promise { if (agent.type === "unavailable") { return new UnavailableAgent(agent); @@ -384,6 +394,7 @@ async function buildAgent( signRun, connectedVendors, narrowing ? offeredFor : undefined, + agentFetch, ); } @@ -478,6 +489,8 @@ function remoteAgentWithStandingRole( * Absent means no narrowing, which is the behaviour every deployment had before this existed. */ narrow?: (input: RunAgentInput) => Promise, + /** The fetch this agent is dialled with. See {@link buildAgents}. */ + agentFetch?: AgentFetch, ) { const remote = new HttpAgent({ url: agent.endpoint, @@ -485,9 +498,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 } + : {}), }); /* * What this Bot holds, as a second standing message. @@ -711,6 +733,7 @@ export async function resolveRuntimeAgents( computerGuidance?: string, loadVendors?: () => Promise, selection?: ToolSelection, + agentFetch?: AgentFetch, ): Promise> { const registered = await loadAgents(); if (registered.length === 0) { @@ -732,6 +755,7 @@ export async function resolveRuntimeAgents( computerGuidance, loadVendors, selection, + agentFetch, ); } @@ -788,6 +812,8 @@ export function createRequestAgents( * grants, and because the discovery row has to name the person the run belongs to. */ selectionForActor?: (actorId: string) => ToolSelection, + /** The fetch remote agents are dialled with. See {@link buildAgents}. */ + agentFetch?: AgentFetch, ) { return async ({ request }: { request: Request }) => { const actor = await identifyActor(request); @@ -801,6 +827,7 @@ export function createRequestAgents( computerGuidance, loadVendors, selectionForActor?.(actor.id), + agentFetch, ); }; } @@ -830,6 +857,8 @@ export function mountCopilotRuntime( basePath = "/api/copilotkit", loadVendors?: () => Promise, selectionForActor?: (actorId: string) => ToolSelection, + /** The fetch remote agents are dialled with. See {@link buildAgents}. */ + agentFetch?: AgentFetch, ) { const { intelligence } = config.runtime; @@ -870,6 +899,7 @@ export function mountCopilotRuntime( config.computer ? COMPUTER_GUIDANCE : undefined, loadVendors, selectionForActor, + agentFetch, ) as never, }); diff --git a/server/src/index.ts b/server/src/index.ts index 93bf2641..6b673395 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"; @@ -489,6 +490,13 @@ const app = createApp( }); }, }), + // 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 b425c047..3b2e9468 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 7f72ff28..6d439464 100644 --- a/server/tests/copilot.test.ts +++ b/server/tests/copilot.test.ts @@ -209,6 +209,108 @@ 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, + 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, + undefined, + 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, + 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. * From 16e3a6cf361116c5137b8e775285cf4466ceef25 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 21 Aug 2026 22:11:05 -0500 Subject: [PATCH 2/3] Stop a redirect carrying the credentials on to the next host A hop that leaves the host the request was authorised for now arrives with nothing that proves who we are. The customer's key was given to us for their host, and the signed run assertion is this deployment's own capability: whatever holds it can call back as that Bot, for that person, and it rides in the body, so dropping headers alone would leave the more valuable of the two travelling. Only the two protocol headers survive the hop, and the run is taken out of the body. Once dropped they stay dropped, so a chain that wanders off and comes back does not collect them again. A scheme upgrade on the same host is not a different party and keeps both, which is the shape a deployment behind a redirect actually has; the downgrade is treated as one. The stored address is checked before it is dialled, not only the hops after it. A row written before this guard existed is dialled on every run, and that was the one address a check reading only Location headers never looked at. `EndpointRedirectError` is now `EndpointNotAllowedError`, because it answers for the stored address as well as the hops. --- server/src/agents/connection-test.ts | 8 +- server/src/agents/endpoint.ts | 124 ++++++++++++++-- server/tests/agent-endpoint.test.ts | 211 ++++++++++++++++++++++++++- 3 files changed, 325 insertions(+), 18 deletions(-) diff --git a/server/src/agents/connection-test.ts b/server/src/agents/connection-test.ts index 03cbf2a2..c8810f43 100644 --- a/server/src/agents/connection-test.ts +++ b/server/src/agents/connection-test.ts @@ -1,7 +1,7 @@ import { checkAgentEndpoint, createAgentFetch, - EndpointRedirectError, + EndpointNotAllowedError, } from "./endpoint"; /** @@ -122,9 +122,9 @@ 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) { + // An address this deployment will not dial is a specific thing that happened, and the person + // registering can act on it: it names the address, or the hop their address sent us to. + if (error instanceof EndpointNotAllowedError) { return { ok: false, reason: error.message }; } const timedOut = error instanceof Error && error.name === "TimeoutError"; diff --git a/server/src/agents/endpoint.ts b/server/src/agents/endpoint.ts index c9b86f59..015a2f44 100644 --- a/server/src/agents/endpoint.ts +++ b/server/src/agents/endpoint.ts @@ -57,23 +57,95 @@ export function checkAgentEndpoint( } /** - * How many redirects an agent is allowed before we stop believing it has somewhere to be. + * How many redirects an agent is allowedbefore 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 { +/** An address this deployment will not dial, named so the person registering sees which hop. */ +export class EndpointNotAllowedError extends Error { constructor(message: string) { super(message); - this.name = "EndpointRedirectError"; + this.name = "EndpointNotAllowedError"; } } const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); +/** + * The headers an AG-UI POST needs to be an AG-UI POST. Everything else on one of these requests is + * the registered agent's own configuration, which is to say its key. + */ +const PROTOCOL_HEADERS = new Set(["content-type", "accept"]); + +/** + * Whether a hop stays inside the authorisation the request was carrying credentials for. + * + * Host and port must match, because a different one is a different party however similar the name. + * A scheme upgrade is the exception in the permissive direction: `http` to `https` on the same host + * is the ordinary shape of a deployment behind a redirect, and the credential ends up somewhere + * strictly better protected than where it started. The downgrade is not the same trade and is + * treated as a different party. + */ +function sameCredentialScope(from: string, to: string): boolean { + const a = new URL(from); + const b = new URL(to); + if (a.hostname !== b.hostname || a.port !== b.port) return false; + return ( + a.protocol === b.protocol || + (a.protocol === "http:" && b.protocol === "https:") + ); +} + +/** The request with everything that proves who we are taken out of it. */ +function withoutCredentials(init: RequestInit | undefined): RequestInit { + const kept = new Headers(); + for (const [name, value] of new Headers(init?.headers)) { + if (PROTOCOL_HEADERS.has(name.toLowerCase())) kept.set(name, value); + } + return { ...init, headers: kept, ...strippedBody(init?.body) }; +} + +/** + * The body with this deployment's own signed run taken out of it. + * + * The run assertion is a bearer capability: it names the Bot and the person, and whatever holds it + * can call back and spend that person's grants. Stripping the headers and forwarding the body would + * leave the more valuable of the two credentials travelling. + * + * A body this cannot read is not forwarded at all. A stream or a form is not a shape this deployment + * sends here, so the choice is between refusing an unreachable case and forwarding something + * unexamined to a host the request was not authorised for, and only one of those fails safely. + */ +function strippedBody(body: BodyInit | null | undefined): { body?: BodyInit } { + if (body === null || body === undefined) return {}; + if (typeof body !== "string") { + throw new EndpointNotAllowedError( + "That address redirected to another host, and this deployment will not forward the run to it.", + ); + } + + let parsed: unknown; + try { + parsed = JSON.parse(body); + } catch { + // Not ours to sanitise and not ours to leak. The same reasoning as the non-string case. + throw new EndpointNotAllowedError( + "That address redirected to another host, and this deployment will not forward the run to it.", + ); + } + if (parsed === null || typeof parsed !== "object") return { body }; + + const run = parsed as { forwardedProps?: Record }; + if (!run.forwardedProps || typeof run.forwardedProps !== "object") { + return { body }; + } + const { openbotRun: _dropped, ...rest } = run.forwardedProps; + return { body: JSON.stringify({ ...run, forwardedProps: rest }) }; +} + /** * `fetch`, with the endpoint check applied to every hop rather than only the address a person typed. * @@ -83,11 +155,20 @@ const REDIRECT_STATUSES = new Set([301, 302, 303, 307, 308]); * cloud metadata. The address that gets dialled is the one that must be allowed, and a redirect makes * those two different addresses. * + * The stored address is checked too, not only the hops after it. A row written before this guard + * existed, or under a rule that has since changed, is dialled on every run, and that is the one + * address a check that only reads `Location` headers never looks at. + * * 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. * + * A hop that leaves the host the request was authorised for arrives with nothing that proves who we + * are: the customer's key is theirs and was given to us for their host, and the run assertion is + * this deployment's own capability. Once dropped they stay dropped, so a chain that wanders off and + * comes back does not collect them again. + * * 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 @@ -97,14 +178,32 @@ export function createAgentFetch( options: { allowPrivateHosts?: boolean; fetchImpl?: typeof fetch } = {}, ): (url: string, init?: RequestInit) => Promise { const doFetch = options.fetchImpl ?? fetch; + const check = (address: string) => + checkAgentEndpoint(address, { + ...(options.allowPrivateHosts !== undefined + ? { allowPrivateHosts: options.allowPrivateHosts } + : {}), + }); return async function guardedFetch(url: string, init?: RequestInit) { - let target = url; + const stored = check(url); + if (!stored.allowed) { + throw new EndpointNotAllowedError( + `This deployment will not dial ${url}: ${stored.reason.charAt(0).toLowerCase()}${stored.reason.slice(1)}`, + ); + } + + const origin = stored.url; + let target = stored.url; + let carried = init; 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" }); + const response = await doFetch(target, { + ...carried, + redirect: "manual", + }); if (!REDIRECT_STATUSES.has(response.status)) return response; const location = response.headers.get("location"); @@ -113,20 +212,19 @@ export function createAgentFetch( if (!location) return response; const next = new URL(location, target).toString(); - const verdict = checkAgentEndpoint(next, { - ...(options.allowPrivateHosts !== undefined - ? { allowPrivateHosts: options.allowPrivateHosts } - : {}), - }); + const verdict = check(next); if (!verdict.allowed) { - throw new EndpointRedirectError( + throw new EndpointNotAllowedError( `That address redirected to ${next}, and ${verdict.reason.charAt(0).toLowerCase()}${verdict.reason.slice(1)}`, ); } + if (!sameCredentialScope(origin, verdict.url)) { + carried = withoutCredentials(carried); + } target = verdict.url; } - throw new EndpointRedirectError( + throw new EndpointNotAllowedError( `That address redirected more than ${MAX_REDIRECTS} times without arriving anywhere.`, ); }; diff --git a/server/tests/agent-endpoint.test.ts b/server/tests/agent-endpoint.test.ts index 9072dd67..12dd7094 100644 --- a/server/tests/agent-endpoint.test.ts +++ b/server/tests/agent-endpoint.test.ts @@ -1,7 +1,7 @@ import { describe, expect, test } from "bun:test"; import { agentAuthHeaders, storeAgentAuth } from "../src/agents/auth-header"; import { testAgentConnection } from "../src/agents/connection-test"; -import { checkAgentEndpoint } from "../src/agents/endpoint"; +import { checkAgentEndpoint, createAgentFetch } from "../src/agents/endpoint"; import { parseAgentInput } from "../src/agents/routes"; /** A 32-byte key, as the vault expects. */ @@ -306,3 +306,212 @@ describe("the key a customer's agent sits behind", () => { if (parsed.ok) expect(parsed.value.auth).toBeUndefined(); }); }); + +/** + * The fetch a run is dialled with, rather than the check a registration passed. + * + * The address in the database was allowed once, by whatever rules were in force that day, and every + * run afterwards dials it again. So this fetch re-asks the question on the way out: the stored + * address is checked before the first byte, and each redirect is checked before it is followed. + * + * The second half is what a redirect does to the things the request is carrying. A hop that leaves + * the host it was authorised for is a hop to somebody else, and everything on that request that + * proves who we are, the customer's key in the headers and this deployment's own signed run in the + * body, has to stop there. + */ +describe("dialling a stored agent endpoint", () => { + /** A fetch that records what it was asked to do and answers however the test says. */ + function recorder(answers: Array<() => Response>) { + const calls: Array<{ url: string; init: RequestInit }> = []; + let index = 0; + const impl = (async (url: string, init: RequestInit) => { + calls.push({ url, init }); + const answer = answers[Math.min(index, answers.length - 1)]; + index += 1; + return answer(); + }) as unknown as typeof fetch; + return { calls, impl }; + } + + const redirectTo = (location: string) => () => + new Response(null, { status: 307, headers: { location } }); + const arrived = () => new Response("ok"); + + /** What a run carries: the customer's key, and this deployment's statement of whose run it is. */ + const runRequest = { + method: "POST", + // Capitalised the way `@ag-ui/client` actually sends them, so a check keyed on the lower-cased + // name is tested against the shape a run really arrives in rather than a tidier one. + headers: { + "Content-Type": "application/json", + Accept: "text/event-stream", + Authorization: "Bearer customer-key", + "X-Api-Key": "customer-key", + }, + body: JSON.stringify({ + threadId: "t", + forwardedProps: { openbotBotId: "risk", openbotRun: "signed.run.token" }, + }), + }; + + test("the stored address is checked again before it is dialled", async () => { + // The row was written before this guard existed, or under an older rule. Checking only the + // redirects leaves the one address that is dialled on every single run unchecked. + const { calls, impl } = recorder([arrived]); + const dial = createAgentFetch({ fetchImpl: impl }); + + await expect( + dial("http://169.254.169.254/latest/meta-data/", runRequest), + ).rejects.toThrow(/may not live there|refus/i); + expect(calls.length).toBe(0); + }); + + test("a hop to another host does not take the credentials with it", async () => { + // What curl and a browser do, and for the reason they do it: the key was handed to us for one + // host, and a redirect is that host naming a different one. + const { calls, impl } = recorder([ + redirectTo("https://elsewhere.example.com/ag-ui"), + arrived, + ]); + const dial = createAgentFetch({ fetchImpl: impl }); + + const response = await dial("https://agent.example.com/ag-ui", runRequest); + expect(response.status).toBe(200); + expect(calls.length).toBe(2); + + const first = new Headers(calls[0]?.init.headers); + expect(first.get("authorization")).toBe("Bearer customer-key"); + + const second = new Headers(calls[1]?.init.headers); + expect(second.get("authorization")).toBeNull(); + expect(second.get("x-api-key")).toBeNull(); + // The protocol headers are not credentials, and an AG-UI POST without them is not an AG-UI POST. + expect(second.get("content-type")).toBe("application/json"); + expect(second.get("accept")).toBe("text/event-stream"); + }); + + test("a hop to another host does not take the signed run with it", async () => { + // The run assertion is a bearer capability: whoever holds it can call back as this Bot, for this + // person. It rides in the body, so stripping headers alone leaves the leak open. + const { calls, impl } = recorder([ + redirectTo("https://elsewhere.example.com/ag-ui"), + arrived, + ]); + const dial = createAgentFetch({ fetchImpl: impl }); + + await dial("https://agent.example.com/ag-ui", runRequest); + + const forwarded = JSON.parse(String(calls[1]?.init.body)) as { + threadId?: string; + forwardedProps?: Record; + }; + expect(forwarded.forwardedProps?.openbotRun).toBeUndefined(); + // Only the credential is removed. The rest of the run is still the run. + expect(forwarded.threadId).toBe("t"); + expect(forwarded.forwardedProps?.openbotBotId).toBe("risk"); + }); + + test("a hop that stays on the same host keeps them", async () => { + // An agent that redirects `/ag-ui` to `/ag-ui/` is the same agent, and stripping its own key + // there would answer a working registration with a 401. + const { calls, impl } = recorder([ + redirectTo("https://agent.example.com/ag-ui/"), + arrived, + ]); + const dial = createAgentFetch({ fetchImpl: impl }); + + await dial("https://agent.example.com/ag-ui", runRequest); + + const second = new Headers(calls[1]?.init.headers); + expect(second.get("authorization")).toBe("Bearer customer-key"); + const forwarded = JSON.parse(String(calls[1]?.init.body)) as { + forwardedProps?: Record; + }; + expect(forwarded.forwardedProps?.openbotRun).toBe("signed.run.token"); + }); + + test("an upgrade from http to https on the same host keeps them", async () => { + // The ordinary case a deployment behind a redirect actually has. Treating a scheme upgrade as a + // different host would break it while protecting nothing: the credential ends up somewhere + // strictly safer than it started. + const { calls, impl } = recorder([ + redirectTo("https://agent.example.com/ag-ui"), + arrived, + ]); + const dial = createAgentFetch({ fetchImpl: impl }); + + await dial("http://agent.example.com/ag-ui", runRequest); + + const second = new Headers(calls[1]?.init.headers); + expect(second.get("authorization")).toBe("Bearer customer-key"); + }); + + test("a downgrade from https to http does not", async () => { + // Same host, but the key would leave over a connection anybody on the path can read. + const { calls, impl } = recorder([ + redirectTo("http://agent.example.com/ag-ui"), + arrived, + ]); + const dial = createAgentFetch({ + fetchImpl: impl, + allowPrivateHosts: false, + }); + + await dial("https://agent.example.com/ag-ui", runRequest); + + const second = new Headers(calls[1]?.init.headers); + expect(second.get("authorization")).toBeNull(); + }); + + test("a body this cannot read is not forwarded to another host at all", async () => { + // Sanitising a body means understanding it. A stream is not a shape this deployment sends, so + // the only two options are refusing an unreachable case or handing something unexamined to a + // host the request was never authorised for. + const { calls, impl } = recorder([ + redirectTo("https://elsewhere.example.com/ag-ui"), + arrived, + ]); + const dial = createAgentFetch({ fetchImpl: impl }); + + await expect( + dial("https://agent.example.com/ag-ui", { + method: "POST", + body: new ReadableStream(), + }), + ).rejects.toThrow(/will not forward/i); + expect(calls.length).toBe(1); + }); + + test("a body that is not the JSON it should be is not forwarded either", async () => { + const { calls, impl } = recorder([ + redirectTo("https://elsewhere.example.com/ag-ui"), + arrived, + ]); + const dial = createAgentFetch({ fetchImpl: impl }); + + await expect( + dial("https://agent.example.com/ag-ui", { + method: "POST", + body: "not json at all", + }), + ).rejects.toThrow(/will not forward/i); + expect(calls.length).toBe(1); + }); + + test("credentials dropped on one hop do not come back on the next", async () => { + // A chain that leaves the host and returns to it is not a way to get the key back: the request + // has already been shown to somebody else. + const { calls, impl } = recorder([ + redirectTo("https://elsewhere.example.com/one"), + redirectTo("https://agent.example.com/ag-ui"), + arrived, + ]); + const dial = createAgentFetch({ fetchImpl: impl }); + + await dial("https://agent.example.com/ag-ui", runRequest); + + expect(calls.length).toBe(3); + const third = new Headers(calls[2]?.init.headers); + expect(third.get("authorization")).toBeNull(); + }); +}); From 8431e94daf2bd18caecb321595614ffac429dc40 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Sat, 22 Aug 2026 23:38:59 -0500 Subject: [PATCH 3/3] Put a refused agent dial on the audit trail, and say what changed in the changelog A refused hop threw and nothing else happened. The person whose run failed found out immediately and the deployment found out nothing, which is the wrong way round for this particular failure: a registration is one person at one moment, but a stored agent that has quietly begun redirecting somewhere it should not is a fact about an endpoint, happening on every run, with nobody watching. It reads as an agent being flaky until somebody can count it. `createAgentFetch` now reports refusals to its caller and `index.ts` turns that into an `agent.dial_refused` row naming the address and the reason. The callback rather than an audit store keeps `endpoint.ts` deciding and nothing else, which is the same reason it reuses the navigation target check instead of growing a second one. Reporting cannot take a refusal down with it. A reporter that throws is swallowed and a row that cannot be written is logged, because the request is already refused by the time either runs and the alternative is trading a lost record for a dialled request. All four refusal paths report: the stored address, a redirect destination, a body that cannot be stripped for a cross-host hop, and the redirect cap. The last is not a trust decision and is counted anyway, since an endpoint that loops is another thing only the trail can show is happening repeatedly. Three tests, and the negative one earns its place: removing the report turns two red, and reporting on a permitted hop turns the third red, so neither direction is vacuous. --- CHANGELOG.md | 22 +++++++++ server/src/agents/endpoint.ts | 53 ++++++++++++++++++-- server/src/audit.ts | 14 ++++++ server/src/index.ts | 16 ++++++ server/tests/agent-endpoint.test.ts | 77 ++++++++++++++++++++++++++++- 5 files changed, 176 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5e4c841..8d35f4a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,28 @@ host port. Nothing changes for a default deployment. `scripts/start.sh` already reached it on `localhost`. +### An agent's address is checked where it ends up, not only where it starts + +`checkAgentEndpoint` decides whether this deployment will dial an address, and the request was then +handed to a fetch that followed redirects. The checked address and the dialled address were the same +address only while nobody redirected. An agent answering `307 Location: http://169.254.169.254/` put +the server on its own cloud metadata endpoint, on every run rather than once. + +Every hop is now checked before it is followed, capped at three. Redirects are still followed, +because a deployment that puts its agent behind one has done nothing wrong, and each destination has +to be somewhere registering it directly would have been allowed to reach. The stored address is +checked before it is dialled too, which is the one address a check reading only `Location` headers +never looked at. + +A hop that leaves the host the request was authorised for arrives with nothing that proves who we +are. The customer's key was given to us for their host, and this deployment's signed run assertion +names the Bot and the person and can spend their grants, so both stop at that boundary and do not +come back if the chain returns. A scheme upgrade to the same host and port keeps them. + +Refusals are now on the audit trail as `agent.dial_refused`, with the address and the reason. A +refused run already told the person what happened; nothing told the deployment, and an agent that +has quietly started redirecting somewhere it should not is worth being able to count. + ## 0.0.4 ### A click citing a ref this deployment cannot resolve is refused diff --git a/server/src/agents/endpoint.ts b/server/src/agents/endpoint.ts index 015a2f44..646039e3 100644 --- a/server/src/agents/endpoint.ts +++ b/server/src/agents/endpoint.ts @@ -175,9 +175,37 @@ function strippedBody(body: BodyInit | null | undefined): { body?: BodyInit } { * following a link. */ export function createAgentFetch( - options: { allowPrivateHosts?: boolean; fetchImpl?: typeof fetch } = {}, + options: { + allowPrivateHosts?: boolean; + fetchImpl?: typeof fetch; + /** + * Told about every address this refused to dial, and why. + * + * A refusal is the one thing on this path an operator cannot otherwise learn. The person who + * registered the agent finds out immediately, because their run fails and says why; the + * deployment finds out nothing, and a stored agent that has quietly begun redirecting to the + * metadata address is precisely the event worth being able to count. + * + * A callback rather than an audit store, so this module keeps deciding and nothing else. It is + * the same reason `checkAgentEndpoint` reuses the navigation target check instead of growing a + * second one: a file that decides is testable without the machinery that records. + * + * Reporting must never be able to stop a refusal, so a throwing reporter is swallowed. The + * refusal is the security property and the row is the record of it; losing the record is bad and + * turning it into a dialled request would be worse. + */ + onRefusal?: (refusal: { address: string; reason: string }) => void; + } = {}, ): (url: string, init?: RequestInit) => Promise { const doFetch = options.fetchImpl ?? fetch; + const refuse = (address: string, reason: string) => { + try { + options.onRefusal?.({ address, reason }); + } catch { + // See above: a reporter that throws must not become a request that succeeds. + } + return new EndpointNotAllowedError(reason); + }; const check = (address: string) => checkAgentEndpoint(address, { ...(options.allowPrivateHosts !== undefined @@ -188,7 +216,8 @@ export function createAgentFetch( return async function guardedFetch(url: string, init?: RequestInit) { const stored = check(url); if (!stored.allowed) { - throw new EndpointNotAllowedError( + throw refuse( + url, `This deployment will not dial ${url}: ${stored.reason.charAt(0).toLowerCase()}${stored.reason.slice(1)}`, ); } @@ -214,17 +243,31 @@ export function createAgentFetch( const next = new URL(location, target).toString(); const verdict = check(next); if (!verdict.allowed) { - throw new EndpointNotAllowedError( + throw refuse( + next, `That address redirected to ${next}, and ${verdict.reason.charAt(0).toLowerCase()}${verdict.reason.slice(1)}`, ); } if (!sameCredentialScope(origin, verdict.url)) { - carried = withoutCredentials(carried); + // A body this cannot strip refuses the hop rather than forwarding it, and that refusal is + // worth the same row as any other: it means a run was carrying something unreadable to a + // host it was not authorised for. + try { + carried = withoutCredentials(carried); + } catch (error) { + throw refuse( + verdict.url, + error instanceof Error ? error.message : String(error), + ); + } } target = verdict.url; } - throw new EndpointNotAllowedError( + // Counted with the rest. An agent that loops is not a trust decision, but it is an endpoint + // failing in a way only the trail can show is happening repeatedly. + throw refuse( + target, `That address redirected more than ${MAX_REDIRECTS} times without arriving anywhere.`, ); }; diff --git a/server/src/audit.ts b/server/src/audit.ts index 2977cec6..24ff40f8 100644 --- a/server/src/audit.ts +++ b/server/src/audit.ts @@ -51,6 +51,20 @@ export const auditEventTypes = [ */ "channel.routed", "agent.invoked", + /** + * An address this deployment declined to dial for a Bot, and why. + * + * The stored endpoint is re-checked on the way out of every run, and so is each address it + * redirects to. When one of those is refused the run fails and the person sees why, which is the + * whole of what anybody learns without this row. + * + * That is the wrong shape for the thing worth knowing. A registration is one person at one moment; + * a stored agent quietly beginning to redirect somewhere it should not is a fact about an endpoint, + * happening on every run, with nobody watching. It reads as an agent being flaky until somebody can + * count it. The row names the address and the reason, so a reader can tell an agent that moved from + * one aimed at the metadata endpoint. + */ + "agent.dial_refused", /** * A Bot's stream stopped producing anything and the turn was ended for it. * diff --git a/server/src/index.ts b/server/src/index.ts index 6b673395..ae13e393 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -496,6 +496,22 @@ const app = createApp( // reading and the same one `createApp` takes. createAgentFetch({ allowPrivateHosts: config.computer?.allowPrivateHosts === true, + // The refusal is what the run already knows; this is what the deployment knows. Written here + // rather than in `endpoint.ts` so that file keeps deciding and nothing else, the way the + // target check it reuses does. + onRefusal: ({ address, reason }) => { + void recordAuditEvent(bootAuditStore, { + eventType: "agent.dial_refused", + targetType: "agent_endpoint", + targetId: address, + payload: { address, reason }, + }).catch((error) => { + // A trail that cannot be written must not take a refusal down with it: the request is + // already refused by the time this runs, and the alternative to a logged failure here is + // an unhandled rejection. + console.error("Could not record a refused agent dial.", error); + }); + }, }), ), // The only path to an acting call. diff --git a/server/tests/agent-endpoint.test.ts b/server/tests/agent-endpoint.test.ts index 12dd7094..b74c26b1 100644 --- a/server/tests/agent-endpoint.test.ts +++ b/server/tests/agent-endpoint.test.ts @@ -1,7 +1,11 @@ import { describe, expect, test } from "bun:test"; import { agentAuthHeaders, storeAgentAuth } from "../src/agents/auth-header"; import { testAgentConnection } from "../src/agents/connection-test"; -import { checkAgentEndpoint, createAgentFetch } from "../src/agents/endpoint"; +import { + checkAgentEndpoint, + createAgentFetch, + EndpointNotAllowedError, +} from "../src/agents/endpoint"; import { parseAgentInput } from "../src/agents/routes"; /** A 32-byte key, as the vault expects. */ @@ -366,6 +370,77 @@ describe("dialling a stored agent endpoint", () => { expect(calls.length).toBe(0); }); + /** + * A refusal here is the one thing on this path an operator cannot otherwise find out about. + * + * The person who registered the agent learns immediately, because their run fails and says why. The + * deployment learns nothing: a stored agent that quietly began redirecting to the metadata address + * is exactly the event worth counting, and it is invisible in the trail unless the refusal writes a + * row. So the fetch reports refusals rather than only throwing them, and the caller decides what + * that means. The reason travels with it, because "an agent was refused" without which address and + * why is a row nobody can act on. + */ + test("reports a refused hop to its caller, with the address and the reason", async () => { + const refusals: Array<{ address: string; reason: string }> = []; + const { calls, impl } = recorder([ + redirectTo("http://169.254.169.254/latest/meta-data/"), + arrived, + ]); + const dial = createAgentFetch({ + fetchImpl: impl, + onRefusal: (refusal) => refusals.push(refusal), + }); + + await expect( + dial("https://agent.example.com/ag-ui", runRequest), + ).rejects.toThrow(EndpointNotAllowedError); + + expect(refusals).toHaveLength(1); + expect(refusals[0]?.address).toBe( + "http://169.254.169.254/latest/meta-data/", + ); + expect(refusals[0]?.reason).toMatch(/may not live there|refus/i); + // Reported, and still not dialled. A row about a request that went out anyway would be worse + // than no row at all. + expect(calls).toHaveLength(1); + }); + + test("reports a stored address refused before the first byte", async () => { + // The other refusal an operator wants counted, and the one with no person watching: this fires + // on every run of an agent whose stored address stopped being acceptable. + const refusals: Array<{ address: string; reason: string }> = []; + const { calls, impl } = recorder([arrived]); + const dial = createAgentFetch({ + fetchImpl: impl, + onRefusal: (refusal) => refusals.push(refusal), + }); + + await expect( + dial("http://169.254.169.254/latest/meta-data/", runRequest), + ).rejects.toThrow(EndpointNotAllowedError); + + expect(refusals).toHaveLength(1); + expect(calls).toHaveLength(0); + }); + + test("says nothing when nothing was refused", async () => { + // The negative case, because a reporter that fires on a permitted hop would fill the trail with + // rows about agents that are working. + const refusals: unknown[] = []; + const { impl } = recorder([ + redirectTo("https://agent.example.com/moved"), + arrived, + ]); + const dial = createAgentFetch({ + fetchImpl: impl, + onRefusal: (refusal) => refusals.push(refusal), + }); + + await dial("https://agent.example.com/ag-ui", runRequest); + + expect(refusals).toHaveLength(0); + }); + test("a hop to another host does not take the credentials with it", async () => { // What curl and a browser do, and for the reason they do it: the key was handed to us for one // host, and a redirect is that host naming a different one.