From 009849862cb93cb78788229045494fbccf7dcb57 Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Tue, 15 Sep 2026 14:26:23 +0530 Subject: [PATCH] Refuse mistyped server fields on POST /api/plugins/servers* with 400 POST /servers truthiness-checked key and passed instanceHost and credentialId straight to the store, where input.credentialId?.trim() throws a TypeError on a number or object and escapes the mapped-error catch as a 500. POST /servers/custom did the same for credentialId. Validate key as a trimmed non-empty string and optional instanceHost and credentialId as trimmed non-empty strings when present, before any store call or audit row. Adds route-level regression tests proving numbers, objects, arrays and whitespace-only values answer 400 and never reach the store, plus a trimming happy path. --- server/src/plugins/routes.ts | 75 +++++++-- .../tests/plugin-servers-validation.test.ts | 144 ++++++++++++++++++ 2 files changed, 206 insertions(+), 13 deletions(-) create mode 100644 server/tests/plugin-servers-validation.test.ts diff --git a/server/src/plugins/routes.ts b/server/src/plugins/routes.ts index f02f52279..68a6f54ad 100644 --- a/server/src/plugins/routes.ts +++ b/server/src/plugins/routes.ts @@ -186,19 +186,47 @@ export function createPluginRoutes( if (forbidden) return forbidden; const body = (await context.req.json().catch(() => null)) as { - key?: string; - instanceHost?: string; - credentialId?: string; + key?: unknown; + instanceHost?: unknown; + credentialId?: unknown; } | null; - if (!body?.key) { + const key = typeof body?.key === "string" ? body.key.trim() : ""; + if (!key) { return context.json({ error: "A catalogue key is required." }, 400); } + // Optional fields travel to `input.credentialId?.trim()` in the store, where a number or + // object throws a TypeError that escapes as a 500. A string that is only whitespace would + // silently become `undefined` there, so it is refused here instead of being coerced. + if ( + body?.instanceHost !== undefined && + (typeof body.instanceHost !== "string" || !body.instanceHost.trim()) + ) { + return context.json( + { error: "An instance host must be a non-empty string." }, + 400, + ); + } + if ( + body?.credentialId !== undefined && + (typeof body.credentialId !== "string" || !body.credentialId.trim()) + ) { + return context.json( + { error: "A credential id must be a non-empty string." }, + 400, + ); + } try { const server = await store.addServer({ - key: body.key, - instanceHost: body.instanceHost, - credentialId: body.credentialId, + key, + instanceHost: + typeof body?.instanceHost === "string" + ? body.instanceHost.trim() + : undefined, + credentialId: + typeof body?.credentialId === "string" + ? body.credentialId.trim() + : undefined, by: actorEmail(context), }); return context.json({ server }); @@ -227,24 +255,45 @@ export function createPluginRoutes( if (forbidden) return forbidden; const body = (await context.req.json().catch(() => null)) as { - id?: string; - title?: string; - url?: string; - credentialId?: string; + id?: unknown; + title?: unknown; + url?: unknown; + credentialId?: unknown; } | null; - if (!body?.id?.trim() || !body?.title?.trim() || !body?.url?.trim()) { + if ( + typeof body?.id !== "string" || + !body.id.trim() || + typeof body?.title !== "string" || + !body.title.trim() || + typeof body?.url !== "string" || + !body.url.trim() + ) { return context.json( { error: "A name, a title and a URL are required." }, 400, ); } + // `addCustomServer` dereferences `input.credentialId?.trim()`, so a number or object here + // throws a TypeError that escapes as a 500 instead of a 400. + if ( + body.credentialId !== undefined && + (typeof body.credentialId !== "string" || !body.credentialId.trim()) + ) { + return context.json( + { error: "A credential id must be a non-empty string." }, + 400, + ); + } try { const server = await store.addCustomServer({ id: body.id.trim(), title: body.title.trim(), url: body.url.trim(), - credentialId: body.credentialId, + credentialId: + typeof body.credentialId === "string" + ? body.credentialId.trim() + : undefined, by: actorEmail(context), }); return context.json({ server }); diff --git a/server/tests/plugin-servers-validation.test.ts b/server/tests/plugin-servers-validation.test.ts new file mode 100644 index 000000000..9e53ecf40 --- /dev/null +++ b/server/tests/plugin-servers-validation.test.ts @@ -0,0 +1,144 @@ +import { describe, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import type { AppVariables } from "../src/auth/guards"; +import type { BotAccessCheck } from "../src/plugins/routes"; +import { createPluginRoutes } from "../src/plugins/routes"; +import type { PluginStore } from "../src/plugins/store"; + +function appWith(calls: { addServer: unknown[]; addCustomServer: unknown[] }) { + const store = { + addServer: async (input: unknown) => { + // Mirror the real store: it dereferences `input.credentialId?.trim()`, so a + // non-string credential id throws a TypeError that used to escape as a 500. + const credentialId = (input as { credentialId?: unknown }).credentialId; + if (credentialId !== undefined && typeof credentialId !== "string") { + (credentialId as { trim: () => string }).trim(); + } + calls.addServer.push(input); + return { server: input }; + }, + addCustomServer: async (input: unknown) => { + const credentialId = (input as { credentialId?: unknown }).credentialId; + if (credentialId !== undefined && typeof credentialId !== "string") { + (credentialId as { trim: () => string }).trim(); + } + calls.addCustomServer.push(input); + return { server: input }; + }, + } as unknown as PluginStore; + const requireUser: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, + ) => { + context.set("actor", { + id: "user-1", + email: "user@openbot.test", + role: "admin", + }); + await next(); + }; + const canUseBot: BotAccessCheck = async () => true; + return createPluginRoutes(store, requireUser, canUseBot); +} + +function calls() { + return { addServer: [] as unknown[], addCustomServer: [] as unknown[] }; +} + +/** + * Optional fields reach `input.credentialId?.trim()` in the store, where a number throws a + * TypeError that escapes the mapped-error catch and answers 500. A whitespace-only value would + * silently coerce to `undefined` there. Both are refused at the edge with a 400 before the store + * or audit trail is touched. `key` was truthiness-checked, so `123` passed the edge and only + * failed later as an unknown catalogue entry. + */ +describe("POST /api/plugins/servers", () => { + test.each([ + ["a number key", { key: 123 }], + ["an object key", { key: {} }], + ["a whitespace key", { key: " " }], + ["a number credentialId", { key: "user-oauth", credentialId: 123 }], + ["an object credentialId", { key: "user-oauth", credentialId: {} }], + ["an array credentialId", { key: "user-oauth", credentialId: [] }], + ["a whitespace credentialId", { key: "user-oauth", credentialId: " " }], + ["a number instanceHost", { key: "k", instanceHost: 123 }], + ["a whitespace instanceHost", { key: "k", instanceHost: " " }], + ])("refuses %s with 400 and never reaches the store", async (_n, body) => { + const seen = calls(); + const response = await appWith(seen).request( + "http://openbot.test/servers", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }, + ); + + expect(response.status).toBe(400); + expect(await response.json()).toEqual({ + error: expect.any(String), + }); + expect(seen.addServer).toEqual([]); + expect(seen.addCustomServer).toEqual([]); + }); + + test("trims the key and optional fields on the happy path", async () => { + const seen = calls(); + const response = await appWith(seen).request( + "http://openbot.test/servers", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + key: " user-oauth ", + credentialId: " cred-1 ", + }), + }, + ); + + expect(response.status).toBe(200); + expect(seen.addServer).toHaveLength(1); + expect(seen.addServer[0]).toMatchObject({ + key: "user-oauth", + credentialId: "cred-1", + }); + }); +}); + +/** + * `POST /servers/custom` validated `id/title/url` but passed `credentialId` straight to + * `addCustomServer`, where `input.credentialId?.trim()` throws on a number or object and the + * route answers 500. Non-string ids also crashed `?.trim()` differently per field; all are 400 + * at the edge now. + */ +describe("POST /api/plugins/servers/custom", () => { + test.each([ + [ + "a number credentialId", + { id: "s", title: "T", url: "https://x.test", credentialId: 42 }, + ], + [ + "an object credentialId", + { id: "s", title: "T", url: "https://x.test", credentialId: {} }, + ], + [ + "a whitespace credentialId", + { id: "s", title: "T", url: "https://x.test", credentialId: " " }, + ], + ["a number id", { id: 123, title: "T", url: "https://x.test" }], + ["a whitespace title", { id: "s", title: " ", url: "https://x.test" }], + ])("refuses %s with 400 and never reaches the store", async (_n, body) => { + const seen = calls(); + const response = await appWith(seen).request( + "http://openbot.test/servers/custom", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }, + ); + + expect(response.status).toBe(400); + expect(seen.addCustomServer).toEqual([]); + }); +});