fix(mcp): tool allowlist stored/read as jsonb string, not array (edit-page crash + allowlist not enforced) #172
@@ -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,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -21,32 +21,35 @@ export class AiMcpServerRepo {
|
||||
id: string,
|
||||
workspaceId: string,
|
||||
): Promise<AiMcpServer | undefined> {
|
||||
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<AiMcpServer[]> {
|
||||
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<AiMcpServer[]> {
|
||||
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<string[]>`${JSON.stringify(value)}::jsonb`;
|
||||
return sql<string[]>`${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) };
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user