diff --git a/agent-computer/src/authorisation.ts b/agent-computer/src/authorisation.ts index cbeabb108..57ffade8e 100644 --- a/agent-computer/src/authorisation.ts +++ b/agent-computer/src/authorisation.ts @@ -31,11 +31,15 @@ export function matchesToken(expected: string, offered: string): boolean { * parameter. */ export function offeredToken(headers: Headers, url: URL): string { - if (url.pathname === "/stream") return url.searchParams.get("token") ?? ""; + // The header path trims; the query path must too, or `?token=%20SECRET` 401s while the same + // value in a header succeeds and the failure looks stream-specific. + if (url.pathname === "/stream") + return url.searchParams.get("token")?.trim() ?? ""; const header = headers.get("x-openbot-computer-token")?.trim(); if (header) return header; const authorization = headers.get("authorization")?.trim() ?? ""; - return authorization.replace(/^Bearer /i, ""); + // The remainder needs trimming too: `Bearer SECRET ` left leading spaces behind. + return authorization.replace(/^Bearer /i, "").trim(); } /** diff --git a/agent-computer/src/control.ts b/agent-computer/src/control.ts index dc8922869..026f7afa4 100644 --- a/agent-computer/src/control.ts +++ b/agent-computer/src/control.ts @@ -186,9 +186,11 @@ export function createControl( ...state, requested: true, requestedAt: now(), + // Polled ~1Hz by every viewer for HELP_REQUEST_TTL_MS: a model-generated megabyte reason + // would be retained and re-served the whole time. Capped like form fields are. reason: typeof reason === "string" && reason.trim() - ? reason.trim() + ? reason.trim().slice(0, 500) : "The assistant needs a person to continue.", }; return this.get(); @@ -210,7 +212,7 @@ export function createControl( ...state, secretWanted: typeof input.label === "string" && input.label.trim() - ? input.label.trim() + ? input.label.trim().slice(0, 500) : "the value this page is asking for", secretRef: input.ref.trim(), secretSnapshotId: diff --git a/agent-computer/src/index.ts b/agent-computer/src/index.ts index 16b66ddb3..0d32fffdb 100644 --- a/agent-computer/src/index.ts +++ b/agent-computer/src/index.ts @@ -204,7 +204,9 @@ function botIdOf(request: Request, fallback?: string | null): string { * would only add a syscall to every call. Everything about why confinement is harder than it looks * lives in workspace.ts. */ -const workspace = createWorkspace(process.env.WORKSPACE_DIR ?? "/workspace"); +const workspace = createWorkspace( + process.env.WORKSPACE_DIR?.trim() || "/workspace", +); /** * Who has the wheel, as a state machine in its own module. @@ -232,12 +234,12 @@ const workspace = createWorkspace(process.env.WORKSPACE_DIR ?? "/workspace"); * meant to shrink. */ const profiles = createProfiles( - process.env.PROFILES_DIR ?? "/profiles", + process.env.PROFILES_DIR?.trim() || "/profiles", (botId) => sessions.get(botId)?.viewer.releaseAll(COMPUTER_STOPPED), ); // Rooted in the same workspace the file tools use, so a command and a written file see one // directory rather than two. -const shell = createShell(process.env.WORKSPACE_DIR ?? "/workspace"); +const shell = createShell(process.env.WORKSPACE_DIR?.trim() || "/workspace"); /** * The id normally arrives as a header on every request. This is the fallback for a caller that has no @@ -515,6 +517,9 @@ serve({ } message = validated.message; } catch { + // The validated-but-wrong branch above sends an error frame; unparseable input used to + // be dropped silently, so a buggy surface saw input "ignored" with no diagnostic. + ws.send(JSON.stringify({ type: "error", error: "Input is not JSON." })); return; } // A person's input is accepted only while they hold the wheel. The socket being open is not permission: diff --git a/agent-computer/tests/runtime-hardening.test.ts b/agent-computer/tests/runtime-hardening.test.ts new file mode 100644 index 000000000..d83abd70e --- /dev/null +++ b/agent-computer/tests/runtime-hardening.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, test } from "bun:test"; +import { offeredToken } from "../src/authorisation"; +import { createControl } from "../src/control"; + +const SECRET = "a-long-development-secret"; +const url = (path: string) => new URL(`http://computer.test${path}`); + +/** + * The header path trimmed while `/stream` did not, so `?token=%20SECRET` 401d while the same + * value in a header succeeded. `Bearer SECRET ` left leading spaces behind the regex. + */ +describe("offered token trimming", () => { + test("trims a padded stream query token", () => { + const headers = new Headers(); + expect( + offeredToken( + headers, + url(`/stream?token=${encodeURIComponent(` ${SECRET} `)}`), + ), + ).toBe(SECRET); + }); + + test("trims the bearer remainder", () => { + const headers = new Headers({ authorization: `Bearer ${SECRET} ` }); + expect(offeredToken(headers, url("/snapshot"))).toBe(SECRET); + }); +}); + +/** + * `reason` and `label` are stored and polled ~1Hz by every viewer for the request TTL. A + * model-generated megabyte string would be retained and re-served the whole time; capped at 500. + */ +describe("control reason/label caps", () => { + test("caps a long help reason at 500 characters", () => { + const control = createControl(); + const state = control.requestHelp("r".repeat(2000)); + expect(state.reason).toHaveLength(500); + }); + + test("caps a long secret label at 500 characters", () => { + const control = createControl(); + const state = control.requestSecret({ label: "l".repeat(2000), ref: "e1" }); + expect(state.secretWanted).toHaveLength(500); + }); +}); diff --git a/shared/user-content-hardening.test.ts b/shared/user-content-hardening.test.ts new file mode 100644 index 000000000..69ba4e41f --- /dev/null +++ b/shared/user-content-hardening.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, test } from "bun:test"; +import { userContent } from "./user-content"; + +const png = Buffer.from([137, 80, 78, 71]).toString("base64"); + +/** + * The mime type reaches a `data:` URL sent to model providers with no allowlist, and the value + * with no shape check. `text/html`, smuggling whitespace, and empty values now degrade to a + * named part instead of a provider payload. + */ +describe("userContent image hardening", () => { + test("passes an allowlisted png through", () => { + expect( + userContent([ + { + type: "image", + source: { type: "data", value: png, mimeType: "image/png" }, + }, + ]), + ).toEqual([ + { type: "image_url", image_url: { url: `data:image/png;base64,${png}` } }, + ]); + }); + + test.each([["text/html"], ["application/javascript"], ["text/plain"]])( + "names a %s attachment instead of sending it", + (mimeType) => { + expect( + userContent([ + { + type: "image", + source: { type: "data", value: png, mimeType }, + }, + ]), + ).toEqual([{ type: "text", text: "[image]" }]); + }, + ); + + test("names an empty value instead of sending it", () => { + expect( + userContent([ + { + type: "image", + source: { type: "data", value: " ", mimeType: "image/png" }, + }, + ]), + ).toEqual([{ type: "text", text: "[image]" }]); + }); + + test("normalises a padded, upper-case mime type", () => { + expect( + userContent([ + { + type: "image", + source: { type: "data", value: png, mimeType: " IMAGE/JPEG " }, + }, + ]), + ).toEqual([ + { + type: "image_url", + image_url: { url: `data:image/jpeg;base64,${png}` }, + }, + ]); + }); +}); diff --git a/shared/user-content.ts b/shared/user-content.ts index 0859ac9cc..3e72a7def 100644 --- a/shared/user-content.ts +++ b/shared/user-content.ts @@ -33,15 +33,25 @@ export function userContent(content: unknown): string | UserContentPart[] { return { type: "text", text: item.text }; } const source = item.source; + // The mime type reaches a `data:` URL sent to model providers. Allowlisted to images and + // base64-shape-checked, so `text/html` (or whitespace/quotes/CRLF smuggling) and empty values + // degrade to a named part instead of a provider payload. if ( item.type === "image" && source?.type === "data" && typeof source.value === "string" && - typeof source.mimeType === "string" + source.value.trim() && + /^[A-Za-z0-9+/]*={0,2}$/.test(source.value.replace(/\s/g, "")) && + typeof source.mimeType === "string" && + ["image/png", "image/jpeg", "image/gif", "image/webp"].includes( + source.mimeType.trim().toLowerCase(), + ) ) { return { type: "image_url", - image_url: { url: `data:${source.mimeType};base64,${source.value}` }, + image_url: { + url: `data:${source.mimeType.trim().toLowerCase()};base64,${source.value}`, + }, }; } const name = diff --git a/supervisor/src/environment.ts b/supervisor/src/environment.ts index ae3a814aa..be13ec82a 100644 --- a/supervisor/src/environment.ts +++ b/supervisor/src/environment.ts @@ -12,9 +12,22 @@ export function environmentFor( const passthrough = Object.entries(env).filter(([key]) => key.startsWith("EGRESS_PROXY"), ); - const computerToken = env.COMPUTER_TOKEN; + const computerToken = env.COMPUTER_TOKEN?.trim() || undefined; const spireSocketVolume = env.SPIRE_AGENT_SOCKET_VOLUME; - const browserMode = env.COMPUTER_BROWSER_MODE; + // Fail fast here rather than forwarding an invalid mode that crashes the child at + // `browserModeFromEnv`: whitespace-only is falsy after trim, anything else must be headless + // or headed. + const rawBrowserMode = env.COMPUTER_BROWSER_MODE?.trim() || undefined; + if ( + rawBrowserMode !== undefined && + rawBrowserMode !== "headless" && + rawBrowserMode !== "headed" + ) { + throw new Error( + `COMPUTER_BROWSER_MODE must be headless or headed, not ${JSON.stringify(env.COMPUTER_BROWSER_MODE)}.`, + ); + } + const browserMode = rawBrowserMode; return [ `COMPUTER_BOT_ID=${botId}`, ...(computerToken ? [`COMPUTER_TOKEN=${computerToken}`] : []), diff --git a/supervisor/src/index.ts b/supervisor/src/index.ts index 37d4f5296..3ed41df5f 100644 --- a/supervisor/src/index.ts +++ b/supervisor/src/index.ts @@ -59,16 +59,18 @@ if (!token) { ); process.exit(1); } -const image = process.env.COMPUTER_IMAGE ?? "openbot-agent-computer:latest"; -const network = process.env.COMPUTER_NETWORK; -const runtime = process.env.COMPUTER_RUNTIME; +const image = + process.env.COMPUTER_IMAGE?.trim() || "openbot-agent-computer:latest"; +const network = process.env.COMPUTER_NETWORK?.trim() || undefined; +const runtime = process.env.COMPUTER_RUNTIME?.trim() || undefined; const resolvedMemory = computerMemoryBytes(process.env.COMPUTER_MEMORY_BYTES); if (!resolvedMemory.ok) { console.error(resolvedMemory.reason); process.exit(1); } const memoryBytes = resolvedMemory.bytes; -const spireSocketVolume = process.env.SPIRE_AGENT_SOCKET_VOLUME; +const spireSocketVolume = + process.env.SPIRE_AGENT_SOCKET_VOLUME?.trim() || undefined; const app = new Hono(); diff --git a/supervisor/tests/environment-hardening.test.ts b/supervisor/tests/environment-hardening.test.ts new file mode 100644 index 000000000..d49d0bd62 --- /dev/null +++ b/supervisor/tests/environment-hardening.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, test } from "bun:test"; +import { environmentFor } from "../src/environment"; + +/** + * Whitespace-only `COMPUTER_TOKEN` used to be forwarded verbatim while the child trims and then + * exits, a boot crash loop from a value the supervisor accepted. Invalid `COMPUTER_BROWSER_MODE` + * was likewise forwarded to crash the child instead of failing fast here. + */ +describe("supervisor environment hardening", () => { + test("omits a whitespace-only computer token", () => { + expect( + environmentFor("bot-1", { + COMPUTER_TOKEN: " ", + COMPUTER_BROWSER_MODE: "headless", + }), + ).toEqual(["COMPUTER_BOT_ID=bot-1", "COMPUTER_BROWSER_MODE=headless"]); + }); + + test("trims a padded token", () => { + expect(environmentFor("bot-1", { COMPUTER_TOKEN: " secret " })).toContain( + "COMPUTER_TOKEN=secret", + ); + }); + + test("refuses an invalid browser mode instead of forwarding it", () => { + expect(() => + environmentFor("bot-1", { + COMPUTER_TOKEN: "s", + COMPUTER_BROWSER_MODE: "fullscreen", + }), + ).toThrow(/headless or headed/); + }); + + test("accepts both valid modes", () => { + for (const mode of ["headless", "headed"]) { + expect( + environmentFor("bot-1", { + COMPUTER_TOKEN: "s", + COMPUTER_BROWSER_MODE: mode, + }), + ).toContain(`COMPUTER_BROWSER_MODE=${mode}`); + } + }); +});