diff --git a/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx b/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx index da823ec6..a3d07a94 100644 --- a/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx +++ b/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.tsx @@ -56,7 +56,13 @@ function buildInitialValues(server?: IAiMcpServer): FormValues { transport: server?.transport ?? "http", url: server?.url ?? "", authHeader: "", - toolAllowlist: server?.toolAllowlist ?? [], + // Defensive: TagsInput calls `.map`, so a non-array here (e.g. an API that + // returns the jsonb column as a JSON string) would crash the whole page. The + // server normalizes this now, but guard anyway so a bad shape can never take + // the settings UI down. + toolAllowlist: Array.isArray(server?.toolAllowlist) + ? server.toolAllowlist + : [], enabled: server?.enabled ?? true, }; } diff --git a/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.spec.ts b/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.spec.ts new file mode 100644 index 00000000..a04b77aa --- /dev/null +++ b/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.spec.ts @@ -0,0 +1,48 @@ +import { parseToolAllowlist } from './ai-mcp-server.repo'; + +/** + * The `tool_allowlist` jsonb column historically round-trips as a JSON STRING + * (rows written by the old double-encoding `jsonbArray`), so the driver hands + * back `'["a","b"]'` instead of an array. `parseToolAllowlist` normalizes both + * shapes to the `string[] | null` the entity type promises — fixing the settings + * UI crash (TagsInput `.map` on a string) and the tool-allowlist enforcement + * (which did `Array.isArray(allow)` and silently allowed ALL tools for a string). + */ +describe('parseToolAllowlist', () => { + it('passes a real string array through unchanged', () => { + expect(parseToolAllowlist(['search', 'crawl'])).toEqual(['search', 'crawl']); + }); + + it('parses a JSON-string array (the double-encoded read) into an array', () => { + // This is exactly what the DB returns for an old row: a jsonb string scalar. + expect(parseToolAllowlist('["alpha","beta"]')).toEqual(['alpha', 'beta']); + }); + + it('returns null for null / undefined (unrestricted)', () => { + expect(parseToolAllowlist(null)).toBeNull(); + expect(parseToolAllowlist(undefined)).toBeNull(); + }); + + it('returns [] for an empty array (no items, but a present allowlist)', () => { + expect(parseToolAllowlist([])).toEqual([]); + }); + + it('returns null for a JSON string that is not an array', () => { + expect(parseToolAllowlist('"justastring"')).toBeNull(); + expect(parseToolAllowlist('{"a":1}')).toBeNull(); + }); + + it('returns null for an unparseable string', () => { + expect(parseToolAllowlist('not json at all')).toBeNull(); + }); + + it('returns null when elements are not all strings (defensive)', () => { + expect(parseToolAllowlist([1, 2, 3] as unknown)).toBeNull(); + expect(parseToolAllowlist('[1,2,3]')).toBeNull(); + }); + + it('returns null for a non-string, non-array primitive', () => { + expect(parseToolAllowlist(42 as unknown)).toBeNull(); + expect(parseToolAllowlist(true as unknown)).toBeNull(); + }); +}); diff --git a/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts b/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts index f91f4af5..a0f2da50 100644 --- a/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts +++ b/apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts @@ -21,32 +21,35 @@ export class AiMcpServerRepo { id: string, workspaceId: string, ): Promise { - return this.db + const row = await this.db .selectFrom('aiMcpServers') .selectAll('aiMcpServers') .where('id', '=', id) .where('workspaceId', '=', workspaceId) .executeTakeFirst(); + return row ? normalizeRow(row) : row; } async listByWorkspace(workspaceId: string): Promise { - return this.db + const rows = await this.db .selectFrom('aiMcpServers') .selectAll('aiMcpServers') .where('workspaceId', '=', workspaceId) .orderBy('createdAt', 'asc') .execute(); + return rows.map(normalizeRow); } /** Enabled servers only — used by the agent loop to build the toolset. */ async listEnabled(workspaceId: string): Promise { - return this.db + const rows = await this.db .selectFrom('aiMcpServers') .selectAll('aiMcpServers') .where('workspaceId', '=', workspaceId) .where('enabled', '=', true) .orderBy('createdAt', 'asc') .execute(); + return rows.map(normalizeRow); } async insert( @@ -130,6 +133,14 @@ export class AiMcpServerRepo { * Encode a string[] as a jsonb bind for the `tool_allowlist` column. Passing a * plain JS array to the postgres driver would serialize it as a Postgres array * literal (incompatible with jsonb), so we bind the JSON text and cast it. + * + * The cast is `::text::jsonb`, NOT `::jsonb`: if the parameter is bound straight + * to a jsonb cast, node-postgres infers its type as jsonb and JSON-stringifies + * the (already-JSON) string a SECOND time, so the column ends up holding a jsonb + * STRING SCALAR (`"[\"a\"]"`) instead of a jsonb ARRAY. Forcing the param through + * `::text` first binds it as text (sent verbatim), and `::jsonb` then parses it + * into a real array. (`normalizeRow` below repairs rows written the old way.) + * * Returns null for null/empty arrays (an empty allowlist means "no restriction" * is not intended — callers pass null to clear; an empty array is normalized to * null here so it never round-trips as `[]`). @@ -139,5 +150,37 @@ function jsonbArray(value: string[] | null | undefined) { return null; } // Typed as string[] so it is assignable to the toolAllowlist column. - return sql`${JSON.stringify(value)}::jsonb`; + return sql`${JSON.stringify(value)}::text::jsonb`; +} + +/** + * Parse the `toolAllowlist` value read from the DB into the `string[] | null` + * the entity type promises. The jsonb column historically round-trips as a JSON + * STRING (rows written by the old double-encoding `jsonbArray`, see above), so + * the driver hands back a string like `'["a","b"]'` rather than an array. Be + * tolerant: an already-parsed array passes through; a JSON string is parsed; null + * / a non-array / unparseable value becomes null (unrestricted). + */ +export function parseToolAllowlist(value: unknown): string[] | null { + if (value == null) return null; + if (Array.isArray(value)) { + return value.every((v) => typeof v === 'string') ? (value as string[]) : null; + } + if (typeof value === 'string') { + try { + const parsed = JSON.parse(value); + return Array.isArray(parsed) && + parsed.every((v) => typeof v === 'string') + ? (parsed as string[]) + : null; + } catch { + return null; + } + } + return null; +} + +/** Normalize a DB row so `toolAllowlist` is always `string[] | null`. */ +function normalizeRow(row: AiMcpServer): AiMcpServer { + return { ...row, toolAllowlist: parseToolAllowlist(row.toolAllowlist) }; }