diff --git a/server/src/app.ts b/server/src/app.ts index fb3c333a0..9598f67da 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -1005,10 +1005,11 @@ export function createApp( return context.json({ error: "This endpoint is the worker's." }, 401); } const body = await context.req.json().catch(() => null); - if ( - typeof (body as { routineRunId?: unknown } | null)?.routineRunId !== - "string" - ) { + const routineRunId = (body as { routineRunId?: unknown } | null) + ?.routineRunId; + // An empty id is a string and used to answer 202 Accepted while the worker swallows the + // failure. Only a non-empty id is accepted for dispatch. + if (typeof routineRunId !== "string" || !routineRunId.trim()) { return context.json({ error: "A routineRunId is required." }, 400); } /* @@ -1018,9 +1019,7 @@ export function createApp( * the fatigue rule owns it. `run()` never throws by contract; this swallow only guards against * that contract being wrong without turning a bug there into an unhandled rejection here. */ - void routineRunner - .run((body as { routineRunId: string }).routineRunId) - .catch(() => {}); + void routineRunner.run(routineRunId).catch(() => {}); return context.json({ accepted: true }, 202); }); } diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index a7ad2a4c8..bad163f9f 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -549,10 +549,17 @@ export function createComputerRoutes( */ routes.get("/:botId/page-frame/:toolCallId", async (context) => { if (!pageFrames) return context.json({ frame: null }); - const stored = await pageFrames.load( - context.req.param("botId"), - context.req.param("toolCallId"), - ); + // Unvalidated params reach the frame table as-is. Blank or overlong ids can never name a + // stored frame, so they are refused here instead of becoming junk reads. + const botId = context.req.param("botId"); + const toolCallId = context.req.param("toolCallId"); + if (!botId.trim() || !toolCallId.trim()) { + return context.json({ error: "A Bot and a turn are required." }, 400); + } + if (botId.length > 200 || toolCallId.length > 200) { + return context.json({ error: "A Bot and a turn are required." }, 400); + } + const stored = await pageFrames.load(botId, toolCallId); return context.json({ frame: stored }); }); diff --git a/server/src/routing/routes.ts b/server/src/routing/routes.ts index 9f1462890..1948f61d9 100644 --- a/server/src/routing/routes.ts +++ b/server/src/routing/routes.ts @@ -111,6 +111,14 @@ export function createRoutingRoutes( } | null; const text = typeof body?.text === "string" ? body.text.trim() : ""; if (!text) return context.json({ error: "A message is required." }, 400); + // Unbounded text becomes the model prompt. Cap it so a multi-megabyte body cannot be used + // to force a timeout or OOM in the router. + if (text.length > 10000) { + return context.json( + { error: "A message of at most 10000 characters is required." }, + 400, + ); + } const named = typeof body?.agentId === "string" && body.agentId.trim() ? body.agentId.trim() diff --git a/server/tests/routing-limits-validation.test.ts b/server/tests/routing-limits-validation.test.ts new file mode 100644 index 000000000..0fd8e6bc4 --- /dev/null +++ b/server/tests/routing-limits-validation.test.ts @@ -0,0 +1,182 @@ +import { describe, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import { Hono } from "hono"; +import type { AppVariables } from "../src/auth/guards"; +import { createApp } from "../src/app"; +import { loadConfig } from "../src/config"; +import { createRoutingRoutes } from "../src/routing/routes"; +import { createComputerRoutes } from "../src/computer/routes"; +import type { RoutineRunner } from "../src/routines/runner"; +import { testEnvironment } from "./support/environment"; + +const requireUser: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, +) => { + context.set("actor", { + id: "user-1", + email: "user@openbot.test", + role: "admin", + }); + await next(); +}; + +/** + * Unbounded `text` becomes the model prompt in the router. A multi-megabyte body would force a + * timeout or OOM; over 10000 characters is now a 400 before the roster is read or the model is + * asked. + */ +describe("POST /api/route text cap", () => { + function app(calls: unknown[]) { + const store = { list: async () => [] }; + const router = { + route: async (...a: unknown[]) => { + calls.push(a); + return { chosen: "bot-1" }; + }, + }; + const app = new Hono<{ Variables: AppVariables }>(); + app.route( + "/", + createRoutingRoutes(store as never, router as never, requireUser), + ); + return app; + } + + test("refuses oversized text with 400 and never routes", async () => { + const calls: unknown[] = []; + const response = await app(calls).request("http://openbot.test/", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ text: "x".repeat(10001) }), + }); + expect(response.status).toBe(400); + expect(await response.json()).toEqual({ + error: "A message of at most 10000 characters is required.", + }); + expect(calls).toEqual([]); + }); + + test("accepts a message at the cap boundary", async () => { + const calls: unknown[] = []; + // Empty roster -> 409 "No coworker is available.", which still proves the text passed the + // cap and reached the router path. + const response = await app(calls).request("http://openbot.test/", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ text: "x".repeat(10000) }), + }); + expect(response.status).toBe(409); + }); +}); + +const SECRET = "worker-shared-secret"; + +function internalApp(runner: RoutineRunner | undefined) { + const args: Parameters = [ + loadConfig({ ...testEnvironment(), WORKER_SHARED_SECRET: SECRET }), + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + runner, + ]; + return createApp(...args); +} + +/** + * `""` is a string and used to answer 202 Accepted while the worker swallowed the failure. + * Only a non-empty run id is accepted for dispatch now. + */ +describe("POST /internal/routines/run id", () => { + test.each([ + ["empty", ""], + ["whitespace", " "], + ])( + "refuses a %s routineRunId with 400 and never runs", + async (_n, routineRunId) => { + const calls: string[] = []; + const app = internalApp({ + run: (id: string) => { + calls.push(id); + return Promise.resolve(); + }, + }); + const response = await app.request( + "http://openbot.local/internal/routines/run", + { + method: "POST", + headers: { + "content-type": "application/json", + authorization: `Bearer ${SECRET}`, + }, + body: JSON.stringify({ routineRunId }), + }, + ); + expect(response.status).toBe(400); + expect(calls).toEqual([]); + }, + ); +}); + +/** + * Blank or overlong frame params can never name a stored frame. Refused here instead of + * becoming junk reads against the frame table. + */ +describe("GET /api/computers/:botId/page-frame/:toolCallId", () => { + function app(calls: unknown[]) { + const pageFrames = { + load: async (...a: unknown[]) => { + calls.push(a); + return null; + }, + }; + const app = new Hono<{ Variables: AppVariables }>(); + app.route( + "/", + createComputerRoutes( + {} as never, + {} as never, + requireUser, + async () => true, + pageFrames as never, + ), + ); + return app; + } + + test("returns null frame on the happy path", async () => { + const calls: unknown[] = []; + const response = await app(calls).request( + "http://openbot.test/bot-1/page-frame/turn-1", + ); + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ frame: null }); + expect(calls).toEqual([["bot-1", "turn-1"]]); + }); + + test("refuses an overlong toolCallId with 400 and never reads", async () => { + const calls: unknown[] = []; + const response = await app(calls).request( + `http://openbot.test/bot-1/page-frame/${"t".repeat(201)}`, + ); + expect(response.status).toBe(400); + expect(calls).toEqual([]); + }); +});