Refuse mistyped server fields on POST /api/plugins/servers* with 400

server/src/plugins/routes.ts

@@ -284,19 +284,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 });
@@ -342,24 +370,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 });

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([]);
+  });
+});