ci(#476): гейты наблюдаемых свойств перед publish — image-smoke, migration-order на push, allowlist fail-closed, property-тесты

Retrospective of 22.06-10.07 merges showed one recurring miss class: local logic verified, integration property never checked (#361, #353, #452, #172, #435). This lands four gates so each of those classes fails BEFORE the :develop image is pushed: 1. Image boot-smoke in the publish job (develop.yml + scripts/ci/image-smoke.sh): the exact image watchtower pulls is booted against postgres/redis services before the push — /api/health (startup migrator, #361-boot/#353), auth/setup, client dist served, hashed assets immutable + brotli (#452). 2. migration-order gate now also runs on push (test.yml): direct pushes used to bypass the PR-only gate; base = event.before, zero-SHA skips, force-push fails closed. 3. External-MCP tool allowlist fails closed (#172 class): corrupt stored value now reads as [] (deny-all) with an error log instead of null (allow-all); [] round-trips as jsonb [] via jsonbBind({preserveEmpty}) and means deny-all in the toolset filter. The settings form sends null for an empty tag field so existing "unrestricted" servers are not silently narrowed. 4. Property tests for the silent-degradation classes: converter fixpoint through the live server path (mcp e2e), and CollabSession cache-key stability under per-call fresh tokens (#435/#439 lesson) incl. a negative control with the token cache disabled. Closes #476 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-11 01:02:29 +03:00
parent 3411bda2d1
commit 5f3f720d9a
16 changed files with 670 additions and 57 deletions
@@ -37,10 +37,13 @@ export class CreateMcpServerDto {
  @IsObject()
  headers?: Record<string, string>;

+  // Omit/null => no restriction; `[]` is persisted verbatim and means deny-all
+  // (zero tools) since #476. @IsOptional() skips validation for null as well,
+  // so an explicit null is accepted.
  @IsOptional()
  @IsArray()
  @IsString({ each: true })
-  toolAllowlist?: string[];
+  toolAllowlist?: string[] | null;

  // Admin-authored guidance ("how/when to use this server's tools") injected
  // into the agent system prompt next to the tool descriptions (#180). Trusted,
@@ -38,10 +38,13 @@ export class UpdateMcpServerDto {
  @IsObject()
  headers?: Record<string, string>;

+  // Absent => unchanged; null => no restriction; `[]` is persisted verbatim
+  // and means deny-all (zero tools) since #476. @IsOptional() skips validation
+  // for null as well, so an explicit null is accepted.
  @IsOptional()
  @IsArray()
  @IsString({ each: true })
-  toolAllowlist?: string[];
+  toolAllowlist?: string[] | null;

  // Admin-authored prompt guidance (#180). Absent => unchanged; blank => cleared
  // (stored as null by the repo). Capped to bound prompt/token size.
@@ -0,0 +1,169 @@
+import { type Tool } from 'ai';
+import { McpClientsService } from './mcp-clients.service';
+
+/**
+ * Tool-allowlist filtering semantics on the merged external toolset (#476).
+ *
+ * COVERAGE CHOICE (documented per issue #476): the full corrupt-row chain
+ * (DB value -> repo normalizeRow -> toolsFor filter) is covered on TWO levels
+ * instead of one live-stub-MCP-server integration test:
+ *   (a) apps/server/test/integration/ai-mcp-server-repo.int-spec.ts pins the
+ *       repo read/write semantics against a real Postgres — `[]` round-trips
+ *       as jsonb `[]`, a present-but-corrupt value fails CLOSED to `[]` with
+ *       an error log;
+ *   (b) THIS spec pins what the toolset builder does with the repo's output —
+ *       null = unrestricted, `['alpha']` = only alpha, `[]` (including the
+ *       corrupt-row fallback) = ZERO tools.
+ * Together they prove the end-to-end property "corrupt/empty allowlist can
+ * never widen to all tools" without a live stub HTTP MCP server.
+ *
+ * The drive path mirrors mcp-namespacing.spec.ts: stub the repo's listEnabled,
+ * spy the private `connect` to return a fake client, inspect the merged keys.
+ */
+
+function fakeTool(): Tool {
+  return { description: 'x', inputSchema: undefined } as unknown as Tool;
+}
+
+interface FakeServer {
+  id: string;
+  name: string;
+  transport: string;
+  url: string;
+  headersEnc: string | null;
+  toolAllowlist: string[] | null;
+}
+
+function server(
+  over: Partial<FakeServer> & { id: string; name: string },
+): FakeServer {
+  return {
+    transport: 'http',
+    url: 'https://example.com/mcp',
+    headersEnc: null,
+    toolAllowlist: null,
+    ...over,
+  };
+}
+
+/**
+ * Build a service whose repo returns `servers` and whose fake clients expose
+ * `rawTools` from tools(). Returns the merged tool keys produced by toolsFor.
+ */
+async function mergedKeysFor(
+  servers: FakeServer[],
+  rawTools: Record<string, Tool>,
+): Promise<string[]> {
+  const repoStub = {
+    listEnabled: jest.fn().mockResolvedValue(servers),
+  };
+  const service = new McpClientsService(repoStub as never, {} as never);
+
+  jest
+    .spyOn(
+      service as unknown as { connect: (s: FakeServer) => unknown },
+      'connect',
+    )
+    .mockImplementation(() =>
+      Promise.resolve({
+        tools: () => Promise.resolve(rawTools),
+        close: () => Promise.resolve(),
+      }),
+    );
+
+  const toolset = await service.toolsFor('ws-1');
+  // Release the lease so the service does not hold the fake clients open.
+  await Promise.all(toolset.clients.map((c) => c.close()));
+  return Object.keys(toolset.tools);
+}
+
+describe('external MCP tool-allowlist filtering (via toolsFor, #476)', () => {
+  afterEach(() => jest.restoreAllMocks());
+
+  const RAW = () => ({
+    alpha: fakeTool(),
+    beta: fakeTool(),
+    gamma: fakeTool(),
+  });
+
+  it("['alpha'] lets ONLY alpha through", async () => {
+    const keys = await mergedKeysFor(
+      [server({ id: 'id-1', name: 'srv', toolAllowlist: ['alpha'] })],
+      RAW(),
+    );
+    expect(keys).toEqual(['srv_alpha']);
+  });
+
+  it('null (no restriction) lets every tool through', async () => {
+    const keys = await mergedKeysFor(
+      [server({ id: 'id-1', name: 'srv', toolAllowlist: null })],
+      RAW(),
+    );
+    expect(keys.sort()).toEqual(['srv_alpha', 'srv_beta', 'srv_gamma']);
+  });
+
+  it('[] (deny-all) yields ZERO tools — an empty array is authoritative, not falsy (#476)', async () => {
+    // This is the regression the #476 change guards: `[]` used to fall through
+    // the old `allow.length > 0` check and expose ALL tools. It must expose NONE.
+    const keys = await mergedKeysFor(
+      [server({ id: 'id-1', name: 'srv', toolAllowlist: [] })],
+      RAW(),
+    );
+    expect(keys).toEqual([]);
+  });
+
+  it('the corrupt-row fallback ([] from the repo) also yields ZERO tools (#476)', async () => {
+    // The repo turns a present-but-corrupt tool_allowlist into `[]` (fail-closed,
+    // see normalizeRow in ai-mcp-server.repo.ts + the int-spec); this pins that
+    // the toolset builder honours that fallback as deny-all rather than allow-all.
+    const corruptFallback: string[] = [];
+    const keys = await mergedKeysFor(
+      [server({ id: 'id-1', name: 'srv', toolAllowlist: corruptFallback })],
+      RAW(),
+    );
+    expect(keys).toEqual([]);
+  });
+
+  it('allowlisted names not exposed by the server are ignored (no phantom tools)', async () => {
+    const keys = await mergedKeysFor(
+      [
+        server({
+          id: 'id-1',
+          name: 'srv',
+          toolAllowlist: ['alpha', 'does-not-exist'],
+        }),
+      ],
+      RAW(),
+    );
+    expect(keys).toEqual(['srv_alpha']);
+  });
+
+  it('a deny-all server contributes no prompt instructions (0 tools merged)', async () => {
+    const repoStub = {
+      listEnabled: jest.fn().mockResolvedValue([
+        {
+          ...server({ id: 'id-1', name: 'srv', toolAllowlist: [] }),
+          instructions: 'use the tools wisely',
+        },
+      ]),
+    };
+    const service = new McpClientsService(repoStub as never, {} as never);
+    jest
+      .spyOn(
+        service as unknown as { connect: (s: FakeServer) => unknown },
+        'connect',
+      )
+      .mockImplementation(() =>
+        Promise.resolve({
+          tools: () => Promise.resolve(RAW()),
+          close: () => Promise.resolve(),
+        }),
+      );
+
+    const toolset = await service.toolsFor('ws-1');
+    await Promise.all(toolset.clients.map((c) => c.close()));
+    expect(Object.keys(toolset.tools)).toEqual([]);
+    // mergeNamespaced reported 0 contributed tools, so no guidance is attached.
+    expect(toolset.instructions).toEqual([]);
+  });
+});
@@ -285,9 +285,13 @@ export class McpClientsService {
      try {
        client = await this.connectWithTimeout(server, CONNECT_TIMEOUT_MS);
        const raw = await withTimeout(client.tools(), CONNECT_TIMEOUT_MS);
+        // Allowlist semantics (#476): null/absent = no restriction (all tools);
+        // ANY array — including `[]` — is authoritative, so an EMPTY allowlist
+        // yields ZERO tools (deny-all). Do NOT add a `.length > 0` escape here:
+        // that read `[]` as falsy and silently widened deny-all to allow-all
+        // (the repo also fails corrupt rows closed to `[]` for the same reason).
        const allow = server.toolAllowlist;
-        const picked =
-          Array.isArray(allow) && allow.length > 0 ? pick(raw, allow) : raw;
+        const picked = Array.isArray(allow) ? pick(raw, allow) : raw;
        // Bound each tool's execute with a per-call total-timeout guard before
        // merging, so a single chatty-but-stuck call is aborted after the cap.
        const guarded = wrapToolsWithCallTimeout(picked, callTimeoutMs);
@@ -100,7 +100,8 @@ export class McpServersService {
      transport: dto.transport,
      url: dto.url,
      headersEnc,
-      // undefined => unchanged; [] / value handled by repo (empty => null).
+      // undefined => unchanged; null => no restriction; `[]` is persisted
+      // verbatim and means deny-all (#476).
      toolAllowlist: dto.toolAllowlist,
      // undefined => unchanged; blank => cleared (null) by the repo.
      instructions: dto.instructions,
@@ -35,4 +35,25 @@ describe('jsonbBind', () => {
    expect(out).not.toBeNull();
    expect(out).toBeDefined();
  });
+
+  // preserveEmpty (#476): opts a column OUT of the empty-to-null collapse so an
+  // empty container is persisted verbatim (e.g. `[]` = deny-all for
+  // tool_allowlist). null stays null regardless of the flag.
+  describe('preserveEmpty', () => {
+    it('returns a (non-null) bind for an empty array', () => {
+      const out = jsonbBind([], { preserveEmpty: true });
+      expect(out).not.toBeNull();
+      expect(out).toBeDefined();
+    });
+
+    it('returns a (non-null) bind for an empty object', () => {
+      const out = jsonbBind({}, { preserveEmpty: true });
+      expect(out).not.toBeNull();
+      expect(out).toBeDefined();
+    });
+
+    it('still returns null for null (null means null, flag or not)', () => {
+      expect(jsonbBind(null, { preserveEmpty: true })).toBeNull();
+    });
+  });
 });
@@ -78,7 +78,9 @@ 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: jsonbBind(values.toolAllowlist),
+        // preserveEmpty (#476): `[]` is a real value here (deny-all), distinct
+        // from null ("no restriction") — it must round-trip as `[]`, not null.
+        toolAllowlist: jsonbBind(values.toolAllowlist, { preserveEmpty: true }),
        // Plain text column: blank/whitespace-only guidance is stored as null.
        instructions: blankToNull(values.instructions),
        enabled: values.enabled ?? true,
@@ -111,7 +113,10 @@ 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 = jsonbBind(patch.toolAllowlist);
+      // preserveEmpty (#476): see insert — `[]` (deny-all) must not become null.
+      set.toolAllowlist = jsonbBind(patch.toolAllowlist, {
+        preserveEmpty: true,
+      });
    }
    if (patch.instructions !== undefined) {
      // Blank/whitespace-only guidance clears the column (stored as null).
@@ -158,7 +163,9 @@ export function blankToNull(value: string | null | undefined): string | null {
 * 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).
+ * array with a non-string element all become null. NOTE: null here only means
+ * "could not parse" — the null-vs-deny-all policy decision lives in
+ * normalizeRow (#476: present-but-corrupt fails CLOSED to `[]`).
 */
 export function parseToolAllowlist(value: unknown): string[] | null {
  // Shape guard only; the legacy double-encoding self-heal lives in
@@ -173,17 +180,20 @@ export function parseToolAllowlist(value: unknown): 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.
+ * FAIL-CLOSED (#476): a stored value that is PRESENT but cannot be parsed into
+ * a string[] (corrupt JSON, a non-array, non-string elements) degrades to `[]`
+ * = deny-all, so a corrupted allowlist can never silently widen to "the agent
+ * gets ALL of the server's tools" (the old fail-open null). An error line is
+ * logged (server id only, never the contents) so the admin can repair the row.
+ * A column that is truly NULL/absent stays `null` = "no restriction".
 */
 function normalizeRow(row: AiMcpServer): AiMcpServer {
  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)`,
+    logger.error(
+      `Corrupt tool_allowlist for MCP server ${row.id}; failing closed (NO tools allowed) — re-save the server's allowlist to repair it`,
    );
+    return { ...row, toolAllowlist: [] };
  }
  return { ...row, toolAllowlist: parsed };
 }
@@ -78,18 +78,30 @@ export function violatedConstraint(err: unknown): string | undefined {
 * 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 `[]`/`{}`.
+ * Returns `null` for null/undefined. By default it ALSO returns `null` for
+ * "empty" values (an empty array, or an object with no own enumerable keys) —
+ * most callers treat empty as "clear/unset", so an empty config never
+ * round-trips as `[]`/`{}`.
+ *
+ * `preserveEmpty` (issue #476) opts a column OUT of that empty-to-null
+ * normalization so `[]`/`{}` are persisted as real jsonb values. Needed where
+ * empty and null mean DIFFERENT things: an empty `tool_allowlist` is
+ * deny-all ("zero tools allowed"), while null is "no restriction" — collapsing
+ * `[]` to null silently widened deny-all to allow-all. Deliberately an opt-in
+ * flag, NOT a global change: the other jsonb callers (model_config, source)
+ * keep the empty-means-unset contract.
 */
 export function jsonbBind<T>(
  value: T | null | undefined,
+  opts?: { preserveEmpty?: boolean },
 ): 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;
+  if (!opts?.preserveEmpty) {
+    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`;
 }