From 1e7bd1f9d274cc60436d73b823ff7cdaccd6b922 Mon Sep 17 00:00:00 2001 From: agent_vscode Date: Sat, 11 Jul 2026 01:02:29 +0300 Subject: [PATCH] =?UTF-8?q?ci(#476):=20=D0=B3=D0=B5=D0=B9=D1=82=D1=8B=20?= =?UTF-8?q?=D0=BD=D0=B0=D0=B1=D0=BB=D1=8E=D0=B4=D0=B0=D0=B5=D0=BC=D1=8B?= =?UTF-8?q?=D1=85=20=D1=81=D0=B2=D0=BE=D0=B9=D1=81=D1=82=D0=B2=20=D0=BF?= =?UTF-8?q?=D0=B5=D1=80=D0=B5=D0=B4=20publish=20=E2=80=94=20image-smoke,?= =?UTF-8?q?=20migration-order=20=D0=BD=D0=B0=20push,=20allowlist=20fail-cl?= =?UTF-8?q?osed,=20property-=D1=82=D0=B5=D1=81=D1=82=D1=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/develop.yml | 63 +++++++ .github/workflows/test.yml | 54 ++++-- .../components/ai-mcp-server-form.tsx | 11 +- .../services/ai-mcp-server-service.ts | 8 +- .../external-mcp/dto/create-mcp-server.dto.ts | 5 +- .../external-mcp/dto/update-mcp-server.dto.ts | 5 +- .../external-mcp/mcp-allowlist-filter.spec.ts | 169 ++++++++++++++++++ .../external-mcp/mcp-clients.service.ts | 8 +- .../external-mcp/mcp-servers.service.ts | 3 +- apps/server/src/database/jsonb-bind.spec.ts | 21 +++ .../repos/ai-chat/ai-mcp-server.repo.ts | 28 ++- apps/server/src/database/utils.ts | 26 ++- .../ai-mcp-server-repo.int-spec.ts | 100 +++++++++-- packages/mcp/test-e2e.mjs | 65 +++++++ .../mcp/test/unit/collab-session.test.mjs | 82 +++++++++ scripts/ci/image-smoke.sh | 79 ++++++++ 16 files changed, 670 insertions(+), 57 deletions(-) create mode 100644 apps/server/src/core/ai-chat/external-mcp/mcp-allowlist-filter.spec.ts create mode 100755 scripts/ci/image-smoke.sh diff --git a/.github/workflows/develop.yml b/.github/workflows/develop.yml index f0996955..224a1e43 100644 --- a/.github/workflows/develop.yml +++ b/.github/workflows/develop.yml @@ -62,6 +62,38 @@ jobs: needs: [test, e2e-server, e2e-mcp, build] runs-on: ubuntu-latest timeout-minutes: 30 + # Image boot-smoke (issue #476): every other job tests code from the working + # tree, but the :develop IMAGE that watchtower pulls was never actually + # started anywhere (incident classes #353/#452/#361-boot: startup-migrator + # crash-loop, runtime module missing from the image, wrong static-asset + # headers). The services below back a smoke boot of the exact image right + # before it is pushed; a smoke failure blocks the push. + services: + postgres: + # via mirror.gcr.io (Docker Hub pull-through cache; avoids Hub anonymous + # pull rate-limit that randomly fails on shared GitHub runner IPs). + image: mirror.gcr.io/pgvector/pgvector:pg18 + env: + POSTGRES_DB: docmost + POSTGRES_USER: docmost + POSTGRES_PASSWORD: docmost + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U docmost" + --health-interval 5s + --health-timeout 5s + --health-retries 20 + redis: + # via mirror.gcr.io (see postgres note above). + image: mirror.gcr.io/library/redis:7 + ports: + - 6379:6379 + options: >- + --health-cmd "redis-cli ping" + --health-interval 5s + --health-timeout 5s + --health-retries 20 steps: - name: Checkout uses: actions/checkout@v4 @@ -82,6 +114,37 @@ jobs: id: version run: echo "value=$(git describe --tags --always)" >> "$GITHUB_OUTPUT" + # Load the image into the local docker daemon so it can be booted (the + # push step below exports straight to the registry and leaves nothing + # runnable locally). CONVENTION: build-args here must stay TEXTUALLY + # IDENTICAL to the push step's build-args — same cache scope + same args + # means the layers are reused and the image we smoke IS the image we push. + - name: Build image for smoke (load, no push) + uses: docker/build-push-action@v6 + with: + context: . + platforms: linux/amd64 + build-args: | + APP_VERSION=${{ steps.version.outputs.value }} + AI_AGENT_ROLES_CATALOG_URL=https://raw.githubusercontent.com/vvzvlad/gitmost/develop/agent-roles-catalog + load: true + push: false + tags: gitmost:smoke + cache-from: type=gha,scope=develop-amd64 + + # Boot-smoke the exact image against the job services (see the comment on + # `services:` above): health (startup migrator), auth/setup, client dist + # served, immutable + brotli asset headers. Fails the job (and therefore + # the push) on any miss. + - name: Smoke the built image + run: bash scripts/ci/image-smoke.sh gitmost:smoke + + # The smoke script leaves the container running on failure precisely so + # the boot error (migration mismatch, stack trace) is diagnosable here. + - name: Dump smoke container log on failure + if: failure() + run: docker logs gitmost-smoke 2>&1 | tail -200 || true + - name: Build and push develop image uses: docker/build-push-action@v6 with: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 11cf4694..509ad6be 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -25,37 +25,65 @@ jobs: # filename sorts BEFORE migrations already applied on the target branch (and # thus in prod). The Kysely startup migrator rejects that as "corrupted # migrations" and crash-loops the app on boot (incident #361). This gate fails - # the PR so the migration is renamed to a current timestamp before merge. Only - # runs for pull_request events (needs a base branch to diff against). + # the PR so the migration is renamed to a current timestamp before merge. + # Runs for pull_request (diff against the base branch) AND for push (#476 + # retrospective: a DIRECT push to develop used to bypass this PR-only gate + # entirely — now the push is diffed against its `before` SHA; workflow_call + # from develop.yml inherits the caller's push event). workflow_dispatch has + # nothing to diff against and still skips the job. migration-order: - if: github.event_name == 'pull_request' + if: github.event_name == 'pull_request' || github.event_name == 'push' runs-on: ubuntu-latest timeout-minutes: 5 steps: - - name: Checkout (full history for the base-branch diff) + - name: Checkout (full history for the base diff) uses: actions/checkout@v4 with: fetch-depth: 0 - - name: Added migrations must sort after the newest on the base branch + - name: Added migrations must sort after the newest on the base env: TARGET_BRANCH: ${{ github.base_ref }} + BEFORE_SHA: ${{ github.event.before }} run: | set -euo pipefail MIG_DIR="apps/server/src/database/migrations" - # checkout above already did fetch-depth:0 (full history). Fetch the base - # WITHOUT --depth (a shallow graft would truncate the base history and - # break the merge-base when the base has moved ahead of the PR merge — - # exactly the long-branch-vs-moving-base case this gate guards, #361). - git fetch --no-tags origin "$TARGET_BRANCH" - newest_on_target=$(git ls-tree -r --name-only "origin/${TARGET_BRANCH}" "$MIG_DIR" | sort | tail -1) + if [ "${GITHUB_EVENT_NAME}" = "pull_request" ]; then + # checkout above already did fetch-depth:0 (full history). Fetch the base + # WITHOUT --depth (a shallow graft would truncate the base history and + # break the merge-base when the base has moved ahead of the PR merge — + # exactly the long-branch-vs-moving-base case this gate guards, #361). + git fetch --no-tags origin "$TARGET_BRANCH" + BASE="origin/${TARGET_BRANCH}" + else + # push event: compare against the pre-push tip of the branch. + if [ "$BEFORE_SHA" = "0000000000000000000000000000000000000000" ]; then + echo "::notice::branch creation push — nothing to compare" + exit 0 + fi + if ! git cat-file -e "${BEFORE_SHA}^{commit}" 2>/dev/null; then + # The before-SHA is not in the clone (a force-push rewrote history). + # One recovery attempt — refresh every remote head (cheap: the + # checkout is already fetch-depth:0); a fetch failure aborts via + # `set -e`, which is fail-closed too. + git fetch --no-tags origin '+refs/heads/*:refs/remotes/origin/*' + fi + if ! git cat-file -e "${BEFORE_SHA}^{commit}" 2>/dev/null; then + # FAIL-CLOSED: without the before-SHA there is no base to prove the + # ordering against, and a gate whose job is to BLOCK must not guess. + echo "::error::force-push detected — verify migration order manually, then re-run via workflow_dispatch" + exit 1 + fi + BASE="$BEFORE_SHA" + fi + newest_on_target=$(git ls-tree -r --name-only "$BASE" "$MIG_DIR" | sort | tail -1) # NO `|| true`: a diff failure (e.g. an unresolved merge-base) must fail # the job CLOSED — a gate whose job is to BLOCK must never pass on error. # `set -e` above already aborts on a non-zero diff exit. - added=$(git diff --diff-filter=A --name-only "origin/${TARGET_BRANCH}...HEAD" -- "$MIG_DIR") + added=$(git diff --diff-filter=A --name-only "${BASE}...HEAD" -- "$MIG_DIR") bad=0 for f in $added; do if [[ "$f" < "$newest_on_target" || "$f" == "$newest_on_target" ]]; then - echo "::error::Migration $f sorts at or before the newest on ${TARGET_BRANCH} ($newest_on_target) — rename it with a CURRENT timestamp before merge (do not change its contents). See incident #361." + echo "::error::Migration $f sorts at or before the newest on the base ($newest_on_target) — rename it with a CURRENT timestamp before merge (do not change its contents). See incident #361." bad=1 fi done 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 f3beb39b..1bdfe10e 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 @@ -121,13 +121,20 @@ export default function AiMcpServerForm({ async function handleSubmit(values: FormValues) { const headers = resolveHeaders(); + // An empty tag field means "no restriction" and must be sent as null — + // since #476 the server persists a literal `[]` as deny-all (zero tools), + // so an empty array from this form would silently disable every tool of + // the server. Deny-all remains expressible via the API, not via this form. + const toolAllowlist = + values.toolAllowlist.length === 0 ? null : values.toolAllowlist; + if (isEdit && server) { const payload: IAiMcpServerUpdate = { id: server.id, name: values.name, transport: values.transport, url: values.url, - toolAllowlist: values.toolAllowlist, + toolAllowlist, // Always sent: a blank value clears the stored guidance (server -> null). instructions: values.instructions, enabled: values.enabled, @@ -140,7 +147,7 @@ export default function AiMcpServerForm({ name: values.name, transport: values.transport, url: values.url, - toolAllowlist: values.toolAllowlist, + toolAllowlist, // Blank => server stores null (no guidance). instructions: values.instructions, enabled: values.enabled, diff --git a/apps/client/src/features/workspace/services/ai-mcp-server-service.ts b/apps/client/src/features/workspace/services/ai-mcp-server-service.ts index 782e1412..84f27d92 100644 --- a/apps/client/src/features/workspace/services/ai-mcp-server-service.ts +++ b/apps/client/src/features/workspace/services/ai-mcp-server-service.ts @@ -27,7 +27,9 @@ export interface IAiMcpServerCreate { // Auth headers map (e.g. { Authorization: 'Bearer ...' }). Encrypted on save; // never returned. headers?: Record; - toolAllowlist?: string[]; + // Omit/null => no restriction; `[]` is persisted verbatim and means + // deny-all (zero tools) since #476. + toolAllowlist?: string[] | null; // Admin-authored prompt guidance (#180). Blank => stored as null. instructions?: string; enabled?: boolean; @@ -43,7 +45,9 @@ export interface IAiMcpServerUpdate { transport?: McpTransport; url?: string; headers?: Record; - toolAllowlist?: string[]; + // Absent => unchanged; null => no restriction; `[]` is persisted verbatim + // and means deny-all (zero tools) since #476. + toolAllowlist?: string[] | null; // Admin-authored prompt guidance (#180). Absent => unchanged; blank => cleared. instructions?: string; enabled?: boolean; diff --git a/apps/server/src/core/ai-chat/external-mcp/dto/create-mcp-server.dto.ts b/apps/server/src/core/ai-chat/external-mcp/dto/create-mcp-server.dto.ts index b422fba8..6c993f11 100644 --- a/apps/server/src/core/ai-chat/external-mcp/dto/create-mcp-server.dto.ts +++ b/apps/server/src/core/ai-chat/external-mcp/dto/create-mcp-server.dto.ts @@ -37,10 +37,13 @@ export class CreateMcpServerDto { @IsObject() headers?: Record; + // 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, diff --git a/apps/server/src/core/ai-chat/external-mcp/dto/update-mcp-server.dto.ts b/apps/server/src/core/ai-chat/external-mcp/dto/update-mcp-server.dto.ts index aa8063c6..3156b17b 100644 --- a/apps/server/src/core/ai-chat/external-mcp/dto/update-mcp-server.dto.ts +++ b/apps/server/src/core/ai-chat/external-mcp/dto/update-mcp-server.dto.ts @@ -38,10 +38,13 @@ export class UpdateMcpServerDto { @IsObject() headers?: Record; + // 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. diff --git a/apps/server/src/core/ai-chat/external-mcp/mcp-allowlist-filter.spec.ts b/apps/server/src/core/ai-chat/external-mcp/mcp-allowlist-filter.spec.ts new file mode 100644 index 00000000..cb4d898e --- /dev/null +++ b/apps/server/src/core/ai-chat/external-mcp/mcp-allowlist-filter.spec.ts @@ -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 & { 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, +): Promise { + 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([]); + }); +}); diff --git a/apps/server/src/core/ai-chat/external-mcp/mcp-clients.service.ts b/apps/server/src/core/ai-chat/external-mcp/mcp-clients.service.ts index 1e688974..5621a049 100644 --- a/apps/server/src/core/ai-chat/external-mcp/mcp-clients.service.ts +++ b/apps/server/src/core/ai-chat/external-mcp/mcp-clients.service.ts @@ -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); diff --git a/apps/server/src/core/ai-chat/external-mcp/mcp-servers.service.ts b/apps/server/src/core/ai-chat/external-mcp/mcp-servers.service.ts index 6d366a2f..afece156 100644 --- a/apps/server/src/core/ai-chat/external-mcp/mcp-servers.service.ts +++ b/apps/server/src/core/ai-chat/external-mcp/mcp-servers.service.ts @@ -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, diff --git a/apps/server/src/database/jsonb-bind.spec.ts b/apps/server/src/database/jsonb-bind.spec.ts index 4e9d3ffa..1bac075b 100644 --- a/apps/server/src/database/jsonb-bind.spec.ts +++ b/apps/server/src/database/jsonb-bind.spec.ts @@ -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(); + }); + }); }); 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 8bcfc661..8ddcbc5b 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 @@ -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 }; } diff --git a/apps/server/src/database/utils.ts b/apps/server/src/database/utils.ts index 84c3693b..bf9fe890 100644 --- a/apps/server/src/database/utils.ts +++ b/apps/server/src/database/utils.ts @@ -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( value: T | null | undefined, + opts?: { preserveEmpty?: boolean }, ): RawBuilder | 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`${JSON.stringify(value)}::text::jsonb`; } diff --git a/apps/server/test/integration/ai-mcp-server-repo.int-spec.ts b/apps/server/test/integration/ai-mcp-server-repo.int-spec.ts index 2e181791..23354b63 100644 --- a/apps/server/test/integration/ai-mcp-server-repo.int-spec.ts +++ b/apps/server/test/integration/ai-mcp-server-repo.int-spec.ts @@ -1,5 +1,6 @@ import { Kysely, sql } from 'kysely'; import { randomUUID } from 'node:crypto'; +import { Logger } from '@nestjs/common'; import { AiMcpServerRepo } from '@docmost/db/repos/ai-chat/ai-mcp-server.repo'; import { getTestDb, destroyTestDb, createWorkspace } from './db'; @@ -54,7 +55,11 @@ describe('AiMcpServerRepo tool_allowlist jsonb round-trip [integration]', () => expect(Array.isArray(found?.toolAllowlist)).toBe(true); }); - it('an empty allowlist is normalized to null (no restriction), not []', async () => { + // #476 (deliberate behaviour change): an empty allowlist used to be + // normalized to SQL NULL, which downstream means "no restriction" — so an + // admin's deny-all `[]` silently became allow-all. It must now round-trip as + // a real jsonb `[]` (deny-all), distinct from NULL. + it('an empty allowlist round-trips as jsonb [] (deny-all), not null (#476)', async () => { const row = await repo.insert({ workspaceId: ws, name: `srv-${randomUUID()}`, @@ -62,7 +67,27 @@ describe('AiMcpServerRepo tool_allowlist jsonb round-trip [integration]', () => url: 'https://example.com/mcp', toolAllowlist: [], }); - // The column is SQL NULL, so jsonb_typeof returns SQL NULL (JS null). + // The column holds a real (empty) jsonb ARRAY, not SQL NULL. + expect(await jsonbTypeof(row.id)).toBe('array'); + expect((await repo.findById(row.id, ws))?.toolAllowlist).toEqual([]); + }); + + it('update to [] persists jsonb [] and update to null clears to SQL NULL (#476)', async () => { + const row = await repo.insert({ + workspaceId: ws, + name: `srv-${randomUUID()}`, + transport: 'http', + url: 'https://example.com/mcp', + toolAllowlist: ['search'], + }); + + // Deny-all via update: [] must survive as a real jsonb array. + await repo.update(row.id, ws, { toolAllowlist: [] }); + expect(await jsonbTypeof(row.id)).toBe('array'); + expect((await repo.findById(row.id, ws))?.toolAllowlist).toEqual([]); + + // Explicit clear (null) still means "no restriction" = SQL NULL. + await repo.update(row.id, ws, { toolAllowlist: null }); expect(await jsonbTypeof(row.id)).toBeNull(); expect((await repo.findById(row.id, ws))?.toolAllowlist).toBeNull(); }); @@ -92,23 +117,60 @@ describe('AiMcpServerRepo tool_allowlist jsonb round-trip [integration]', () => expect(healed?.toolAllowlist).toEqual(['alpha', 'beta']); }); - it('FAIL-OPEN: a present-but-corrupt tool_allowlist reads back as null (no restriction)', async () => { - // #185 re-review pt 8: normalizeRow's fail-open branch — the column is - // PRESENT but does not parse into a string[] (here a jsonb string scalar - // holding non-array JSON). The read must degrade to `null` ("no restriction"), - // not crash. (A warn is logged with the server id; not asserted here.) - const id = randomUUID(); - await sql` - INSERT INTO ai_mcp_servers (id, workspace_id, name, transport, url, tool_allowlist) - VALUES ( - ${id}, ${ws}, ${`srv-${id}`}, 'http', 'https://example.com/mcp', - to_jsonb(${'{"not":"an array"}'}::text) - ) - `.execute(db); - // Sanity: the column is present (a jsonb string scalar), not SQL NULL. - expect(await jsonbTypeof(id)).toBe('string'); - // ...yet the read degrades to null (fail-open). - expect((await repo.findById(id, ws))?.toolAllowlist).toBeNull(); + // #476 (deliberate behaviour change, replaces the old FAIL-OPEN pin): a + // present-but-corrupt tool_allowlist used to degrade to null ("no + // restriction"), silently handing the agent ALL of the server's tools. It + // must now FAIL CLOSED to `[]` (deny-all) and log an error. + it('FAIL-CLOSED: a present-but-corrupt tool_allowlist reads back as [] (deny-all) + error log (#476)', async () => { + const errorSpy = jest + .spyOn(Logger.prototype, 'error') + .mockImplementation(() => undefined); + try { + // The column is PRESENT but does not parse into a string[] — a jsonb + // string scalar holding unparseable text (a truncated legacy write). + const id = randomUUID(); + await sql` + INSERT INTO ai_mcp_servers (id, workspace_id, name, transport, url, tool_allowlist) + VALUES ( + ${id}, ${ws}, ${`srv-${id}`}, 'http', 'https://example.com/mcp', + to_jsonb(${'{oops'}::text) + ) + `.execute(db); + // Sanity: the column is present (a jsonb string scalar), not SQL NULL. + expect(await jsonbTypeof(id)).toBe('string'); + // ...and the read degrades to [] (fail-closed deny-all), not null. + expect((await repo.findById(id, ws))?.toolAllowlist).toEqual([]); + // The narrowing is not silent: an error names the server id (never the + // corrupt contents). + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining(`Corrupt tool_allowlist for MCP server ${id}`), + ); + expect( + errorSpy.mock.calls.some((c) => String(c[0]).includes('{oops')), + ).toBe(false); + } finally { + errorSpy.mockRestore(); + } + }); + + it('FAIL-CLOSED: corrupt non-array JSON (an object) also reads back as [] (#476)', async () => { + const errorSpy = jest + .spyOn(Logger.prototype, 'error') + .mockImplementation(() => undefined); + try { + const id = randomUUID(); + await sql` + INSERT INTO ai_mcp_servers (id, workspace_id, name, transport, url, tool_allowlist) + VALUES ( + ${id}, ${ws}, ${`srv-${id}`}, 'http', 'https://example.com/mcp', + to_jsonb(${'{"not":"an array"}'}::text) + ) + `.execute(db); + expect(await jsonbTypeof(id)).toBe('string'); + expect((await repo.findById(id, ws))?.toolAllowlist).toEqual([]); + } finally { + errorSpy.mockRestore(); + } }); }); diff --git a/packages/mcp/test-e2e.mjs b/packages/mcp/test-e2e.mjs index 2dd6851b..6045ec9f 100644 --- a/packages/mcp/test-e2e.mjs +++ b/packages/mcp/test-e2e.mjs @@ -434,6 +434,71 @@ async function main() { } } + // 6h. markdown converter fixpoint (#476): pins the converter fixpoint + // THROUGH the live server/collab path, not just the package tests. The + // unit corpus (docmost-md-roundtrip) proves the converter alone is a + // fixpoint; this asserts the property survives the real pipeline — export + // (REST read, PM -> MD) -> import (MD -> PM -> collab replace -> server + // persistence) -> export — where the server schema, the Yjs structural + // diff or the collab write path could still mangle the doc while every + // unit test stays green. importPageMarkdown is the designed inverse of + // exportPageMarkdown (the self-contained envelope with meta/comments + // blocks); updatePageMarkdown (client.updatePage) takes plain authoring + // markdown and would re-import the envelope blocks as literal content. + { + const FIXMD = [ + "# Fixpoint heading", + "", + "Paragraph with **bold**, *italic* and a [link](https://example.com).", + "", + "## Second level", + "", + "- bullet one", + "- bullet two", + "", + "1. ordered one", + "2. ordered two", + "", + "```js", + "const answer = 42; // code block must survive byte-identically", + "```", + "", + "| A | B |", + "| --- | --- |", + "| one | two |", + "", + ":::info", + "Callout body.", + ":::", + ].join("\n"); + const fx = await client.createPage("E2E md fixpoint " + Date.now(), FIXMD, spaceId); + const fxid = fx.data.id; + try { + const md1 = await client.exportPageMarkdown(fxid); + await client.importPageMarkdown(fxid, md1); + await new Promise((r) => setTimeout(r, 16000)); // wait for server persistence + const md2 = await client.exportPageMarkdown(fxid); + // On failure, name the first diverging line of the two exports. + const firstDiff = (a, b) => { + const al = a.split("\n"); + const bl = b.split("\n"); + for (let i = 0; i < Math.max(al.length, bl.length); i++) { + if (al[i] !== bl[i]) { + return `first diff at line ${i + 1}: ${JSON.stringify(al[i] ?? "")} -> ${JSON.stringify(bl[i] ?? "")}`; + } + } + return "same lines, different bytes (line endings?)"; + }; + check( + "markdown fixpoint: export -> import -> export is byte-identical", + md1 === md2, + md1 === md2 ? "" : firstDiff(md1, md2), + ); + } finally { + try { await client.deletePage(fxid); } catch {} + } + } + // 7. shares: create (idempotent), public access, list, unshare const share = await client.sharePage(pageId); check("sharePage: returns public URL", share.publicUrl?.startsWith(`${APP}/share/`), share.publicUrl); diff --git a/packages/mcp/test/unit/collab-session.test.mjs b/packages/mcp/test/unit/collab-session.test.mjs index c7009a69..9ecfb8ed 100644 --- a/packages/mcp/test/unit/collab-session.test.mjs +++ b/packages/mcp/test/unit/collab-session.test.mjs @@ -9,6 +9,7 @@ import { __sessionCountForTests, } from "../../build/lib/collab-session.js"; import { withPageLock } from "../../build/lib/page-lock.js"; +import { DocmostClient } from "../../build/client.js"; // A stand-in for HocuspocusProvider: it shares the ydoc (so the real yjs // read/transform/write in CollabSession.mutate runs unchanged), auto-completes @@ -89,6 +90,7 @@ const ENV_KEYS = [ "MCP_COLLAB_SESSION_IDLE_MS", "MCP_COLLAB_SESSION_MAX_AGE_MS", "MCP_COLLAB_SESSION_MAX_ENTRIES", + "MCP_COLLAB_TOKEN_TTL_MS", ]; let savedEnv; @@ -345,6 +347,86 @@ test("replaceImage-shaped flow: acquire under an EXTERNAL page lock does not dea ); }); +// --- #439: the collab-token cache is what makes the session cache ACTUALLY hit --- +// +// WHY these two tests exist (the #435 incident): the session registry keys on +// (wsUrl, pageId, token) for identity isolation, but BOTH production token +// sources mint a FRESH JWT on every call (the in-app provider re-signs a JWT +// whose iat/exp changes every second; the external MCP POSTs /auth/collab-token +// per call). The fresh token per call made the session-registry key unstable, +// so the prod hit-rate was 0% — connect storms, 25s timeouts, zombie sessions — +// while every other test in this file stayed green because they pass a FIXED +// "tok" string. The #439 fix is the per-client collab-token cache +// (DocmostClient.getCollabTokenWithReauth + MCP_COLLAB_TOKEN_TTL_MS); these +// tests drive the token through it with a source that returns a DIFFERENT +// fresh JWT per mint, exactly like prod, so a regression in EITHER the token +// cache or the registry keying turns them red. +// +// getCollabTokenWithReauth is TS-private, but the compiled JS exposes it; the +// tests call it directly because that is exactly the per-op composition of the +// production call sites (updatePage etc.: mint the token, then acquire). + +test("#439 token cache ON: fresh-JWT-per-mint source, two ops => ONE connect (session cache hits)", async () => { + process.env.MCP_COLLAB_TOKEN_TTL_MS = "300000"; // cache ON (explicit, not default-dependent) + let mints = 0; + const client = new DocmostClient({ + apiUrl: "http://h/api", + getToken: async () => "user-jwt", + // Like both prod sources: a DIFFERENT fresh JWT on every mint. + getCollabToken: async () => `fresh-jwt-${++mints}`, + }); + + // Op 1: mint the collab token through the client, then acquire + mutate. + const tok1 = await client.getCollabTokenWithReauth(); + const s1 = await acquireCollabSession("page-1", tok1, "http://h/api"); + await s1.mutate(() => docWith("one")); + + // Op 2: the same identity mints again — the cache must serve the SAME token. + const tok2 = await client.getCollabTokenWithReauth(); + const s2 = await acquireCollabSession("page-1", tok2, "http://h/api"); + await s2.mutate(() => docWith("two")); + + assert.equal(mints, 1, "the second op is served from the token cache"); + assert.equal(tok2, tok1, "stable token => stable session-registry key"); + assert.equal(s2, s1, "the live session is reused"); + assert.equal( + FakeProvider.connectCount, + 1, + "two mutations over one identity must cost exactly ONE real connect", + ); + assert.equal(__sessionCountForTests(), 1); +}); + +test("#439 negative control: token cache OFF (TTL=0) reproduces the #435 churn — two ops => TWO connects", async () => { + process.env.MCP_COLLAB_TOKEN_TTL_MS = "0"; // explicit 0 disables the cache (fetch-per-call legacy) + let mints = 0; + const client = new DocmostClient({ + apiUrl: "http://h/api", + getToken: async () => "user-jwt", + getCollabToken: async () => `fresh-jwt-${++mints}`, + }); + + const tok1 = await client.getCollabTokenWithReauth(); + const s1 = await acquireCollabSession("page-1", tok1, "http://h/api"); + await s1.mutate(() => docWith("one")); + + const tok2 = await client.getCollabTokenWithReauth(); + const s2 = await acquireCollabSession("page-1", tok2, "http://h/api"); + await s2.mutate(() => docWith("two")); + + assert.equal(mints, 2, "without the cache every op mints its own token"); + assert.notEqual(tok2, tok1, "unstable token => unstable session-registry key"); + assert.notEqual(s2, s1, "no session reuse"); + assert.equal( + FakeProvider.connectCount, + 2, + "a full reconnect per op — the #435 storm in miniature", + ); + // The first session lingers under its now-unreachable key until its idle + // TTL — the zombie-session symptom of the incident. + assert.equal(__sessionCountForTests(), 2); +}); + test("destroyAllSessions tears down every cached session", async () => { await acquireCollabSession("page-1", "tok", "http://h/api"); await acquireCollabSession("page-2", "tok", "http://h/api"); diff --git a/scripts/ci/image-smoke.sh b/scripts/ci/image-smoke.sh new file mode 100755 index 00000000..d974e17b --- /dev/null +++ b/scripts/ci/image-smoke.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +# Boot-smoke for the exact Docker image that is about to be pushed (issue #476). +# +# Retrospective class "local logic is right, the integration property was never +# checked" (#353/#452/#361): every other CI job builds and tests code from the +# working tree, but the IMAGE watchtower pulls was never actually started +# anywhere before this gate. This script boots the built image against the +# publish job's postgres/redis services and asserts four integration +# properties end-to-end: +# S1 the app boots and /api/health answers (startup migrator + boot) +# S2 the first-run workspace setup endpoint works (API + DB writes) +# S3 the client dist is inside the image and served +# S4 hashed assets are served immutable (#452) with the precompressed +# brotli copy shipped in the image +set -euo pipefail + +IMAGE="${1:?usage: image-smoke.sh }" + +fail() { echo "FAIL: $*"; exit 1; } + +# Boot the exact image that will be pushed, wired to the job services via host +# network (postgres on localhost:5432, redis on localhost:6379). The container +# is deliberately NOT removed on failure so the workflow's dump-on-failure step +# can read `docker logs gitmost-smoke`. +docker run -d --name gitmost-smoke --network host \ + -e DATABASE_URL=postgresql://docmost:docmost@localhost:5432/docmost \ + -e REDIS_URL=redis://localhost:6379 \ + -e APP_SECRET=ci-smoke-secret-change-me-min-32-characters \ + -e APP_URL=http://localhost:3000 \ + "$IMAGE" + +# S1: wait for /api/health — covers the startup migrator + boot inside the +# shipped image (#361-boot, #353 runtime class): a migration the Kysely startup +# migrator rejects, or a runtime module missing from the image, dies right here. +healthy=0 +for _ in $(seq 1 60); do + if curl -fsS http://localhost:3000/api/health > /dev/null 2>&1; then + healthy=1 + break + fi + sleep 2 +done +[ "$healthy" -eq 1 ] || fail "S1: /api/health did not answer within 120s (boot or startup migration failed)" +echo "OK S1: image booted and /api/health answers" + +# S2: the first-run workspace setup works end-to-end (controller -> service -> +# DB write chain inside the shipped image, not just a static health probe). +curl -fsS -X POST http://localhost:3000/api/auth/setup \ + -H "Content-Type: application/json" \ + -d '{"name":"Smoke","email":"smoke@example.com","password":"SmokePassword123","workspaceName":"Smoke"}' \ + > /dev/null || fail "S2: POST /api/auth/setup failed" +echo "OK S2: workspace setup succeeded" + +# S3: the client dist is actually inside the image and served — the SPA HTML +# must reference hashed /assets/ bundles (a broken client COPY in the +# Dockerfile would serve an empty shell that every other job stays green on). +HTML=$(curl -fsS http://localhost:3000/) || fail "S3: fetching / failed" +grep -q '/assets/' <<<"$HTML" || fail "S3: served HTML references no /assets/ bundle (client dist missing from the image?)" +echo "OK S3: client dist served (HTML references /assets/)" + +# S4: hashed /assets/ files must be served with an immutable cache-control +# (#452 class: static.module.ts resolveStaticAssetHeaders owns the header) AND +# with the precompressed brotli neighbour. Both checks are mandatory — verified +# against the code: resolveStaticAssetHeaders marks every /assets/ path +# immutable, and the client build (vite-plugin-compression2, include covers +# .js) emits a .br copy next to every bundle that the Dockerfile ships and +# @fastify/static serves via preCompressed:true. +ASSET=$(grep -oE '/assets/[A-Za-z0-9._@/-]+\.js' <<<"$HTML" | head -1 || true) +[ -n "$ASSET" ] || fail "S4: no /assets/*.js path found in the served HTML" +HDRS=$(curl -fsSI -H 'Accept-Encoding: br' "http://localhost:3000$ASSET") || fail "S4: HEAD $ASSET failed" +grep -qi '^cache-control:.*immutable' <<<"$HDRS" || fail "S4: $ASSET served without an immutable cache-control (#452)" +echo "OK S4: hashed asset served with immutable cache-control" +grep -qi '^content-encoding:.*br' <<<"$HDRS" || fail "S4: $ASSET not served brotli-precompressed (content-encoding: br missing)" +echo "OK S4: hashed asset served with the precompressed brotli copy" + +# Remove the container ONLY on success, so the failure path keeps it around for +# the workflow's "Dump smoke container log on failure" step. +docker rm -f gitmost-smoke > /dev/null +echo "OK image smoke passed"