vvzvlad/gitmost
1
0
Fork 0
Code Issues 14 Pull Requests 12 Packages Projects Releases Activity

fix(db): jsonb double-encoding follow-ups from PR #172 review (#173)

Browse Source 
PR #172 fixed the jsonb double-encoding for `tool_allowlist` but the same
class of bug, and the same re-derived workaround, remained elsewhere.

1. model_config (agent roles): jsonbObject still used the buggy `::jsonb`
   bind, so `ai_agent_roles.model_config` round-tripped as a jsonb STRING
   SCALAR. The read-path `typeof === 'object'` check then failed and the
   model override was SILENTLY dropped (role fell back to the default model).
   Fixed to `::text::jsonb` and added `parseModelConfig` + `normalizeRow` so
   every read self-heals already-corrupted rows (no migration).

2. Centralized the write workaround as `jsonbBind()` in database/utils.ts —
   one implementation with one explanation of the quirk — replacing the
   per-repo `jsonbArray` (mcp) and `jsonbObject` (roles).

3. Integration coverage (the fix is a DB round-trip a unit test cannot see;
   the read-side parser MASKS a write regression): new
   ai-mcp-server-repo.int-spec asserts `jsonb_typeof(tool_allowlist)='array'`
   after insert + heals a seeded string-scalar row; ai-agent-roles-repo
   int-spec gains the same for `model_config` (`'object'` + heal).

4. Updated the stale `ai-mcp-servers.types.ts` comment (the driver returns a
   JSON string for legacy rows; the repo normalizes every read).

5. Fail-open logging: a corrupt tool_allowlist degrades to "no restriction"
   (agent gets ALL tools) — normalizeRow now warns (server id only, never
   contents) so the silent widening leaves a trace.

6. Simplified parseToolAllowlist (normalize the string once, then a single
   array-of-strings check) — identical behaviour, all 12 cases still pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-06-25 04:19:12 +03:00
parent a766672574
commit 1cfad1f6fb
10 changed files with 402 additions and 104 deletions
Download Patch File Download Diff File Expand all files Collapse all files

38
apps/server/src/database/jsonb-bind.spec.ts Normal file
View File

@@ -0,0 +1,38 @@
import { jsonbBind } from './utils';
/**
* Unit tests for jsonbBind: THE shared helper that encodes a JS array/object as
* a jsonb bind (or null when there is nothing to persist). It is the last line
* of defence before a jsonb column write, so the null-vs-bind decision is what
* matters here. We assert only null vs non-null because the non-null value is a
* kysely `sql` template fragment whose internal shape is an implementation
* detail of the SQL tag (the `::text::jsonb` double-encoding fix is verified
* end-to-end by the repo integration specs, where a real DB round-trip can
* actually observe `jsonb_typeof`).
*/
describe('jsonbBind', () => {
it('returns null for null / undefined', () => {
expect(jsonbBind(null)).toBeNull();
expect(jsonbBind(undefined)).toBeNull();
});
it('returns null for an empty array (nothing to persist)', () => {
expect(jsonbBind([])).toBeNull();
});
it('returns null for an empty object (nothing to persist)', () => {
expect(jsonbBind({})).toBeNull();
});
it('returns a (non-null) bind for a non-empty array', () => {
const out = jsonbBind(['search', 'crawl']);
expect(out).not.toBeNull();
expect(out).toBeDefined();
});
it('returns a (non-null) bind for a non-empty object', () => {
const out = jsonbBind({ driver: 'gemini', chatModel: 'gemini-2.0-flash' });
expect(out).not.toBeNull();
expect(out).toBeDefined();
});
});

8
apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts
View File

@@ -35,7 +35,13 @@ describe('AiAgentRoleRepo.findLiveEnabled', () => {
const result = await repo.findLiveEnabled('r-1', 'ws-1');
expect(result).toBe(role);
// The repo normalizes the row (modelConfig parse), so it returns a COPY, not
// the same reference; assert the row's fields are carried through.
expect(result).toMatchObject({
id: 'r-1',
workspaceId: 'ws-1',
enabled: true,
});
expect(db.selectFrom).toHaveBeenCalledWith('aiAgentRoles');
// Every security filter must be present.
expect(where).toHaveBeenCalledWith('id', '=', 'r-1');

67
apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts
View File

@@ -1,8 +1,7 @@
import { Injectable } from '@nestjs/common';
import { InjectKysely } from 'nestjs-kysely';
import { sql } from 'kysely';
import { KyselyDB, KyselyTransaction } from '../../types/kysely.types';
import { dbOrTx } from '../../utils';
import { dbOrTx, jsonbBind } from '../../utils';
import { AiAgentRole } from '@docmost/db/types/entity.types';
/** The jsonb shape persisted in `model_config` (loosely typed for the column). */
@@ -23,13 +22,14 @@ export class AiAgentRoleRepo {
id: string,
workspaceId: string,
): Promise<AiAgentRole | undefined> {
return this.db
const row = await this.db
.selectFrom('aiAgentRoles')
.selectAll('aiAgentRoles')
.where('id', '=', id)
.where('workspaceId', '=', workspaceId)
.where('deletedAt', 'is', null)
.executeTakeFirst();
return row ? normalizeRow(row) : row;
}
/**
@@ -45,7 +45,7 @@ export class AiAgentRoleRepo {
id: string,
workspaceId: string,
): Promise<AiAgentRole | undefined> {
return this.db
const row = await this.db
.selectFrom('aiAgentRoles')
.selectAll('aiAgentRoles')
.where('id', '=', id)
@@ -53,17 +53,19 @@ export class AiAgentRoleRepo {
.where('deletedAt', 'is', null)
.where('enabled', '=', true)
.executeTakeFirst();
return row ? normalizeRow(row) : row;
}
/** All live roles for the workspace (management list + chat picker). */
async listByWorkspace(workspaceId: string): Promise<AiAgentRole[]> {
return this.db
const rows = await this.db
.selectFrom('aiAgentRoles')
.selectAll('aiAgentRoles')
.where('workspaceId', '=', workspaceId)
.where('deletedAt', 'is', null)
.orderBy('createdAt', 'asc')
.execute();
return rows.map(normalizeRow);
}
async insert(
@@ -83,7 +85,7 @@ export class AiAgentRoleRepo {
trx?: KyselyTransaction,
): Promise<AiAgentRole> {
const db = dbOrTx(this.db, trx);
return db
const row = await db
.insertInto('aiAgentRoles')
.values({
workspaceId: values.workspaceId,
@@ -92,7 +94,11 @@ export class AiAgentRoleRepo {
emoji: values.emoji ?? null,
description: values.description ?? null,
instructions: values.instructions,
modelConfig: jsonbObject(values.modelConfig),
// Cast: the generated `model_config` column type is the broad JsonValue
// union, which the concrete RawBuilder<Record> is not structurally
// assignable to (same reason the old jsonbObject cast to any).
// eslint-disable-next-line @typescript-eslint/no-explicit-any
modelConfig: jsonbBind(values.modelConfig) as any,
enabled: values.enabled ?? true,
autoStart: values.autoStart ?? true,
// Empty string is treated as "no custom text" => null.
@@ -100,6 +106,7 @@ export class AiAgentRoleRepo {
})
.returningAll()
.executeTakeFirst();
return normalizeRow(row);
}
async update(
@@ -127,7 +134,7 @@ export class AiAgentRoleRepo {
if (patch.description !== undefined) set.description = patch.description;
if (patch.instructions !== undefined) set.instructions = patch.instructions;
if (patch.modelConfig !== undefined) {
set.modelConfig = jsonbObject(patch.modelConfig);
set.modelConfig = jsonbBind(patch.modelConfig);
}
if (patch.enabled !== undefined) set.enabled = patch.enabled;
if (patch.autoStart !== undefined) set.autoStart = patch.autoStart;
@@ -163,16 +170,40 @@ export class AiAgentRoleRepo {
}
/**
* Encode an object as a jsonb bind for the `model_config` column. The postgres
* driver would otherwise need an explicit cast; bind the JSON text and cast it.
* Returns null for null/undefined/empty objects. Cast to `any` because the
* generated column type is the broad `JsonValue` union, which a concrete object
* type is not structurally assignable to.
* Parse the `model_config` value read from the DB into the object the entity
* type promises. Rows written by the old double-encoding bind (`::jsonb` instead
* of `::text::jsonb`) round-trip as a JSON STRING, so the driver hands back e.g.
* `'{"driver":"gemini"}'` rather than an object; the read-path check
* `typeof cfg === 'object'` then failed and the model override was SILENTLY
* dropped (the role fell back to the default model). Be tolerant: a JSON string
* is parsed; an already-parsed object passes through; null / a non-object (incl.
* an array) / unparseable value becomes null (= no override). This self-heals
* already-corrupted rows on read, no migration required.
*/
export function jsonbObject(value: ModelConfigValue | undefined) {
if (value === null || value === undefined || Object.keys(value).length === 0) {
return null;
export function parseModelConfig(
value: unknown,
): Record<string, unknown> | null {
let v: unknown = value;
if (typeof v === 'string') {
try {
v = JSON.parse(v); // legacy double-encoded read
} catch {
return null;
}
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
return sql`${JSON.stringify(value)}::jsonb` as any;
return v !== null && typeof v === 'object' && !Array.isArray(v)
? (v as Record<string, unknown>)
: null;
}
/** Normalize a DB row so `modelConfig` is always an object or null. The cast
* bridges parseModelConfig's concrete `Record | null` to the column's broad
* generated `JsonValue` type (an object is a valid JsonValue at runtime). */
function normalizeRow(row: AiAgentRole): AiAgentRole {
return {
...row,
modelConfig: parseModelConfig(
row.modelConfig,
) as AiAgentRole['modelConfig'],
};
}

46
apps/server/src/database/repos/ai-agent-roles/parse-model-config.spec.ts Normal file
View File

@@ -0,0 +1,46 @@
import { parseModelConfig } from './ai-agent-roles.repo';
/**
* Unit tests for parseModelConfig: the read-side normalizer that repairs the
* jsonb double-encoding regression on `model_config`. Rows written by the old
* `::jsonb` bind round-trip as a JSON STRING, which the read path's
* `typeof === 'object'` check rejected — silently dropping the model override.
* parseModelConfig accepts an already-parsed object, parses a legacy JSON
* string, and rejects everything that is not an object (null = no override).
*/
describe('parseModelConfig', () => {
it('passes an already-parsed object through', () => {
expect(parseModelConfig({ driver: 'gemini' })).toEqual({
driver: 'gemini',
});
});
it('parses a legacy double-encoded JSON string into an object', () => {
expect(parseModelConfig('{"driver":"gemini","chatModel":"x"}')).toEqual({
driver: 'gemini',
chatModel: 'x',
});
});
it('returns null for null / undefined', () => {
expect(parseModelConfig(null)).toBeNull();
expect(parseModelConfig(undefined)).toBeNull();
});
it('returns null for a non-object JSON value (string/number/array)', () => {
expect(parseModelConfig('"justastring"')).toBeNull();
expect(parseModelConfig('42')).toBeNull();
// An array is an object in JS but not a valid model_config shape.
expect(parseModelConfig('["a","b"]')).toBeNull();
expect(parseModelConfig(['a', 'b'])).toBeNull();
});
it('returns null for an unparseable string', () => {
expect(parseModelConfig('not json at all')).toBeNull();
});
it('returns null for a raw non-object primitive', () => {
expect(parseModelConfig(42 as unknown)).toBeNull();
expect(parseModelConfig(true as unknown)).toBeNull();
});
});

78
apps/server/src/database/repos/ai-chat/ai-mcp-server.repo.ts
View File

@@ -1,10 +1,11 @@
import { Injectable } from '@nestjs/common';
import { Injectable, Logger } from '@nestjs/common';
import { InjectKysely } from 'nestjs-kysely';
import { sql } from 'kysely';
import { KyselyDB, KyselyTransaction } from '../../types/kysely.types';
import { dbOrTx } from '../../utils';
import { dbOrTx, jsonbBind } from '../../utils';
import { AiMcpServer } from '@docmost/db/types/entity.types';
const logger = new Logger('AiMcpServerRepo');
/**
* Repository for per-workspace external MCP servers the agent may use (§5.4).
*
@@ -75,7 +76,7 @@ export class AiMcpServerRepo {
headersEnc: values.headersEnc ?? null,
// jsonb column: the postgres driver would otherwise encode a JS array as
// a Postgres array literal. Bind the JSON text and cast it to jsonb.
toolAllowlist: jsonbArray(values.toolAllowlist),
toolAllowlist: jsonbBind(values.toolAllowlist),
enabled: values.enabled ?? true,
})
.returningAll()
@@ -104,7 +105,7 @@ export class AiMcpServerRepo {
if (patch.url !== undefined) set.url = patch.url;
if (patch.headersEnc !== undefined) set.headersEnc = patch.headersEnc;
if (patch.toolAllowlist !== undefined) {
set.toolAllowlist = jsonbArray(patch.toolAllowlist);
set.toolAllowlist = jsonbBind(patch.toolAllowlist);
}
if (patch.enabled !== undefined) set.enabled = patch.enabled;
await db
@@ -129,58 +130,43 @@ 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 `[]`).
*/
function jsonbArray(value: string[] | null | undefined) {
if (value === null || value === undefined || value.length === 0) {
return null;
}
// Typed as string[] so it is assignable to the toolAllowlist column.
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).
* STRING (rows written by the old double-encoding bind before the `::text::jsonb`
* fix), so the driver hands back a string like `'["a","b"]'` rather than an
* array. Be tolerant: normalize a JSON string to its value, then accept it only
* if it is an array of strings; null / a non-array / unparseable value / an
* array with a non-string element all become 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') {
let v: unknown = value;
if (typeof v === 'string') {
try {
const parsed = JSON.parse(value);
return Array.isArray(parsed) &&
parsed.every((v) => typeof v === 'string')
? (parsed as string[])
: null;
v = JSON.parse(v); // legacy double-encoded read
} catch {
return null;
}
}
return null;
return Array.isArray(v) && v.every((x) => typeof x === 'string')
? (v as string[])
: null;
}
/** Normalize a DB row so `toolAllowlist` is always `string[] | null`. */
/**
* Normalize a DB row so `toolAllowlist` is always `string[] | null`.
*
* FAIL-OPEN logging: a stored value that is present but cannot be parsed into a
* string[] (corrupt JSON, a non-array, non-string elements) degrades to `null` =
* "no restriction", so the agent silently gets ALL of the server's tools. Log
* one line (server id only, never the contents) so that widening is not silent.
*/
function normalizeRow(row: AiMcpServer): AiMcpServer {
return { ...row, toolAllowlist: parseToolAllowlist(row.toolAllowlist) };
const parsed = parseToolAllowlist(row.toolAllowlist);
if (parsed === null && row.toolAllowlist != null) {
logger.warn(
`Corrupt tool_allowlist for MCP server ${row.id}; ignoring it (no tool restriction applied)`,
);
}
return { ...row, toolAllowlist: parsed };
}

4
apps/server/src/database/types/ai-mcp-servers.types.ts
View File

@@ -20,7 +20,9 @@ export interface AiMcpServers {
// Encrypted JSON of the auth headers. Nullable (a server may need no auth).
headersEnc: string | null;
// Optional allowlist of remote tool names to expose; null = expose all.
// Stored as jsonb; reads come back as a string[] from the postgres driver.
// Stored as jsonb. The postgres driver may return a JSON string for legacy
// double-encoded rows; `AiMcpServerRepo` normalizes every read to
// `string[] | null` via `parseToolAllowlist`.
toolAllowlist: string[] | null;
enabled: Generated<boolean>;
createdAt: Generated<Timestamp>;

33
apps/server/src/database/utils.ts
View File

@@ -1,3 +1,4 @@
import { sql, RawBuilder } from 'kysely';
import { KyselyDB, KyselyTransaction } from './types/kysely.types';
/*
@@ -31,3 +32,35 @@ export function dbOrTx(
return db; // Use normal database instance
}
}
/**
* Bind a JS array/object as a `jsonb` column value, working around a postgres
* driver double-encoding quirk. THE single implementation — repos that persist
* jsonb (`tool_allowlist`, `model_config`, ...) call this instead of re-deriving
* the cast.
*
* THE QUIRK: with the `kysely-postgres-js` / postgres.js driver, casting a bound
* parameter straight to `::jsonb` makes the driver infer the param type as jsonb
* and JSON-stringify the (already-JSON) text a SECOND time, so the column ends
* up holding a jsonb STRING SCALAR (`"[\"a\"]"` / `"{\"k\":1}"`) instead of a
* real jsonb array/object. Read paths then see a string, not the structure, and
* silently fall back (an allowlist becomes "unrestricted", a model override is
* ignored). Forcing the param through `::text` first binds it as text (sent
* verbatim); `::jsonb` then parses it into a real array/object. Read-side
* parsers repair rows written the old buggy way without a migration.
*
* Returns `null` for null/undefined and for "empty" values (an empty array, or
* an object with no own enumerable keys) — callers treat empty as "clear/unset",
* so an empty allowlist/config never round-trips as `[]`/`{}`.
*/
export function jsonbBind<T>(
value: T | null | undefined,
): RawBuilder<T> | null {
if (value === null || value === undefined) return null;
if (Array.isArray(value)) {
if (value.length === 0) return null;
} else if (typeof value === 'object') {
if (Object.keys(value as object).length === 0) return null;
}
return sql<T>`${JSON.stringify(value)}::text::jsonb`;
}