Check where an agent address redirects to, not just where it starts (#38)

* 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.

* 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.

* 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.

Author: beardthelion

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

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,

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<string, unknown> };
+  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<Response> {
+  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.`,
+    );
+  };
+}

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.
    *