From 255bc06883efa59cb8b283b6fd1638a8cda1e02b Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Wed, 24 Jun 2026 17:11:50 +0300 Subject: [PATCH] fix(mcp): tool allowlist stored/read as jsonb string, not array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening the edit form for an MCP server that has a saved tool allowlist crashed the whole settings page (`TypeError: Ke.map is not a function` in Mantine) — and, worse, the allowlist was silently NOT enforced. Both stem from one root cause: the `tool_allowlist` jsonb column round-trips as a JSON STRING, not an array. Root cause: `jsonbArray` bound `JSON.stringify(value)` (already a JSON string) straight to a `::jsonb` cast. node-postgres infers the param type as jsonb and JSON-stringifies it a SECOND time, so the column stored a jsonb STRING SCALAR (`"[\"a\"]"`, jsonb_typeof = string) instead of an array. On read the driver hands back the JS string `'["a"]'`. Then: - the edit form's TagsInput called `.map` on a string -> page crash; - mcp-clients did `Array.isArray(allow)` -> false for a string -> fell through to "no restriction" and exposed ALL of the server's tools. Fix (both verified on the stand): - Write: `jsonbArray` casts `::text::jsonb` so the param is bound as text (sent verbatim) and parsed into a real jsonb array. New rows now store jsonb_typeof=array. - Read: `normalizeRow` runs every fetched row through `parseToolAllowlist`, which returns `string[] | null` for both shapes (already-array passes through; a JSON string is parsed; null/invalid -> null). This REPAIRS existing double-encoded rows on read, so the UI and the allowlist enforcement work without a data migration. Applied in findById / listByWorkspace / listEnabled. - Client: defensive `Array.isArray(...) ? ... : []` guard in the form so a bad shape can never take the settings page down again. Tests: ai-mcp-server.repo.spec (8 cases for parseToolAllowlist — array, the JSON-string read, null, empty, non-array json, unparseable, non-string elements, non-string primitive). mcp-servers-to-view + mcp-namespacing still green. Verified live: an old double-encoded row now reads as an array; a newly created server stores jsonb_typeof=array. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../components/ai-mcp-server-form.tsx | 8 ++- .../repos/ai-chat/ai-mcp-server.repo.spec.ts | 48 +++++++++++++++++ .../repos/ai-chat/ai-mcp-server.repo.ts | 51 +++++++++++++++++-- 3 files changed, 102 insertions(+), 5 deletions(-) create mode 100644 apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.spec.ts 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) }; } -- 2.49.1