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/connection-test.ts b/server/src/agents/connection-test.ts index 377d6f14..c8810f43 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, + EndpointNotAllowedError, +} 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) { + // 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"; return { ok: false, diff --git a/server/src/agents/endpoint.ts b/server/src/agents/endpoint.ts index e3777ed6..646039e3 100644 --- a/server/src/agents/endpoint.ts +++ b/server/src/agents/endpoint.ts @@ -55,3 +55,220 @@ export function checkAgentEndpoint( return { allowed: true, url: verdict.url }; } + +/** + * 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; + +/** 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 = "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. + * + * 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. + * + * 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 + * following a link. + */ +export function createAgentFetch( + 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 + ? { allowPrivateHosts: options.allowPrivateHosts } + : {}), + }); + + return async function guardedFetch(url: string, init?: RequestInit) { + const stored = check(url); + if (!stored.allowed) { + throw refuse( + url, + `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, { + ...carried, + 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 = check(next); + if (!verdict.allowed) { + throw refuse( + next, + `That address redirected to ${next}, and ${verdict.reason.charAt(0).toLowerCase()}${verdict.reason.slice(1)}`, + ); + } + if (!sameCredentialScope(origin, verdict.url)) { + // 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; + } + + // 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/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..ae13e393 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,29 @@ 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 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. 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/agent-endpoint.test.ts b/server/tests/agent-endpoint.test.ts index 9072dd67..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 } 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. */ @@ -306,3 +310,283 @@ 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); + }); + + /** + * 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. + 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(); + }); +}); 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. *