Refuse a header value that cannot be sent, instead of reporting an unreachable agent

CHANGELOG.md

@@ -8,6 +8,18 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged.
 
 ## Unreleased
 
+### A key pasted with a line break in it is now refused, instead of reported as an unreachable agent
+
+The box that holds an agent's key takes whatever is pasted into it, and what comes off a clipboard is
+not always what was on the screen: a long key copied out of a wrapped terminal line brings the wrap
+with it, and a hyphen copied out of a document has often been turned into an en dash on the way.
+Neither can be sent as an HTTP header — the runtime refuses the value outright — and neither was
+being looked at. On Test connection that refusal surfaced as "This server could not reach that
+address", with a suggestion about tunnels and firewalls, about an agent that was running perfectly
+well and had never been dialled. Stored on the Bot it was quieter and worse: the form said saved, and
+every turn that Bot took afterwards failed on a value nothing on screen said anything about. Both
+places now check the value before accepting it and say which kind of character is in the way. The
+character is named; the key never is.
 ### A deployment directory pasted with a stray space goes where it says
 
 The desktop setup screen asks where OpenBot should live, enables Start once that box is not blank

server/src/agents/routes.ts

@@ -106,6 +106,16 @@ export function parseAgentInput(
       if (!/^[A-Za-z0-9-]+$/.test(header)) {
         return { ok: false, error: "That is not a valid header name." };
       }
+      // Refused here rather than discovered on the first run. This value is encrypted and stored,
+      // and then sent as a header on every turn the Bot takes; one that cannot be a header value
+      // throws inside `fetch` every one of those times, long after the form said it was saved.
+      const unsendable = unsendableHeaderValue(value);
+      if (unsendable) {
+        return {
+          ok: false,
+          error: `That key contains ${unsendable}, so it cannot be sent as a header.`,
+        };
+      }
       auth = { header, value };
     }
   }
@@ -120,6 +130,31 @@ function isAgentInputObject(input: unknown): input is AgentInputObject {
   return typeof input === "object" && input !== null && !Array.isArray(input);
 }
 
+/**
+ * What in this value stops it being a header, or null when nothing does.
+ *
+ * `new Headers()` refuses a line break, a NUL, and any code point above U+00FF, and it refuses them
+ * by throwing a TypeError from inside `fetch` — which is not a decision this deployment gets to
+ * take part in. Both surfaces below take a header value from the same box on the same form and
+ * neither looked, so the throw arrived somewhere that reads as something else entirely: on the
+ * connection test it lands in the catch written for a dead host, and the person is told "this server
+ * could not reach that address", which sends them to their tunnel and their firewall over a key
+ * they had just pasted with a wrapped line in it. A hyphen a document turned into an en dash does
+ * the same thing, and looks like nothing at all in a password box.
+ *
+ * The description never contains the value. This is asked of a credential on one of the two paths,
+ * and one character of a secret in an error message is one character too many.
+ */
+function unsendableHeaderValue(value: string): string | null {
+  for (const character of value) {
+    const code = character.codePointAt(0) ?? 0;
+    if (code === 0x0a || code === 0x0d) return "a line break";
+    if (code === 0) return "a null character";
+    if (code > 0xff) return "a character that cannot go in a header value";
+  }
+  return null;
+}
+
 /**
  * Parse and validate the headers a person attaches to a connection test.
  *
@@ -177,6 +212,13 @@ export function parseConnectionHeaders(
         error: `Header "${name}" must be at most 4096 characters.`,
       };
     }
+    const unsendable = unsendableHeaderValue(value);
+    if (unsendable) {
+      return {
+        ok: false,
+        error: `Header "${name}" contains ${unsendable}, so it cannot be sent.`,
+      };
+    }
     headers[name] = value;
   }
   return { ok: true, value: entries.length === 0 ? undefined : headers };

server/tests/agent-test-connection-headers.test.ts

@@ -1,10 +1,11 @@
 import { describe, expect, test } from "bun:test";
 import type { MiddlewareHandler } from "hono";
-import type { AppVariables } from "../src/auth/guards";
 import {
   createAgentRoutes,
+  parseAgentInput,
   parseConnectionHeaders,
 } from "../src/agents/routes";
+import type { AppVariables } from "../src/auth/guards";
 
 describe("parseConnectionHeaders", () => {
   test("absent headers stay absent", () => {
@@ -39,6 +40,75 @@ describe("parseConnectionHeaders", () => {
     const parsed = parseConnectionHeaders(input);
     expect(parsed.ok).toBe(false);
   });
+
+  /*
+   * A value the runtime will not put on the wire.
+   *
+   * `new Headers()` throws a TypeError for a line break, a NUL, and any code point above U+00FF,
+   * measured on Bun 1.3.14. That throw comes out of the probe `fetch`, lands in the catch that is
+   * there for a dead host, and is reported as an address this server could not reach — so a pasted
+   * key with a wrapped line in it, or one carrying the en dash a document turned a hyphen into,
+   * sends somebody looking at their tunnel and their firewall.
+   */
+  test.each([
+    ["a newline in the value", "Bearer abc\ndef"],
+    ["a carriage return in the value", "Bearer abc\rdef"],
+    ["a NUL in the value", `Bearer abc${String.fromCharCode(0)}def`],
+    ["a character above Latin-1", "Bearer — abc"],
+  ])("rejects %s", (_name, value) => {
+    const parsed = parseConnectionHeaders({ Authorization: value });
+    expect(parsed.ok).toBe(false);
+  });
+
+  /*
+   * The guard against over-correcting. Everything here is a value `new Headers()` accepts, so
+   * refusing any of it would be taking away a header somebody's agent really wants.
+   */
+  test.each([
+    ["a tab", "Bearer\tabc"],
+    ["an inner space", "Bearer abc def"],
+    ["a Latin-1 accent", "Bearer café"],
+    ["punctuation", "Bearer abc-_.~+/=:;,@!$%^&*()[]{}"],
+  ])("keeps %s", (_name, value) => {
+    expect(parseConnectionHeaders({ Authorization: value })).toEqual({
+      ok: true,
+      value: { Authorization: value },
+    });
+  });
+});
+
+/*
+ * The other half of the same form.
+ *
+ * The key stored on a Bot is typed into the same box as the one a connection test carries, and it is
+ * sent as a header on every run rather than once. Accepted here, it is encrypted, stored, and then
+ * throws inside `fetch` on every turn that Bot takes.
+ */
+describe("the key stored on a Bot", () => {
+  const agent = (value: string) => ({
+    name: "Helper",
+    title: "Helper",
+    roleDescription: "Helps.",
+    visibility: "private",
+    endpoint: "https://agent.example/ag-ui",
+    auth: { header: "Authorization", value },
+  });
+
+  test("a key that cannot be sent as a header is refused", () => {
+    const parsed = parseAgentInput(agent("Bearer abc\ndef"));
+    expect(parsed.ok).toBe(false);
+  });
+
+  test("an ordinary key is still stored", () => {
+    const parsed = parseAgentInput(agent("Bearer abc"));
+    expect(parsed.ok).toBe(true);
+    if (parsed.ok) {
+      expect(parsed.value.auth).toEqual({
+        header: "Authorization",
+        value: "Bearer abc",
+      });
+    }
+  });
 });
 
 describe("POST /test-connection header validation", () => {
@@ -89,3 +159,62 @@ describe("POST /test-connection header validation", () => {
     expect(response.status).toBe(400);
   });
 });
+
+/*
+ * The failure as the person registering an agent meets it: a real server on the other end, a real
+ * `fetch`, and a header value with a line break in it.
+ */
+describe("a header value that cannot be sent", () => {
+  test("is refused before anything is dialled", async () => {
+    let dialled = 0;
+    const agent = Bun.serve({
+      port: 0,
+      fetch: () => {
+        dialled += 1;
+        return new Response(
+          'event: RUN_STARTED\ndata: {"type":"RUN_STARTED"}\n\n',
+          { headers: { "content-type": "text/event-stream" } },
+        );
+      },
+    });
+
+    try {
+      const requireUser: MiddlewareHandler<{
+        Variables: AppVariables;
+      }> = async (context, next) => {
+        context.set("actor", {
+          id: "user-1",
+          email: "user@openbot.test",
+          role: "user",
+        });
+        await next();
+      };
+      const app = createAgentRoutes({} as never, requireUser, true);
+
+      const response = await app.request(
+        "http://openbot.test/test-connection",
+        {
+          method: "POST",
+          headers: { "content-type": "application/json" },
+          body: JSON.stringify({
+            endpoint: `http://127.0.0.1:${agent.port}/ag-ui`,
+            headers: { Authorization: "Bearer abc\ndef" },
+          }),
+        },
+      );
+
+      expect(response.status).toBe(400);
+      // The agent is fine and was never asked. What is wrong is the value in the box, and that is
+      // what the answer has to be about.
+      expect(dialled).toBe(0);
+      const body = (await response.json()) as {
+        error?: string;
+        reason?: string;
+      };
+      expect(body.reason).toBeUndefined();
+      expect(body.error).toMatch(/line break|cannot be sent/i);
+    } finally {
+      agent.stop(true);
+    }
+  });
+});