From 5adcd2f08b045f83097a8529a399bbff4ff7427a Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 10:24:03 +0300 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20=D1=84=D0=BE=D1=80=D0=BC=D0=B0=20?= =?UTF-8?q?=D1=81=D0=BE=D1=85=D1=80=D0=B0=D0=BD=D1=8F=D0=B5=D1=82=20deny-a?= =?UTF-8?q?ll=20allowlist=20=D0=B2=D0=BC=D0=B5=D1=81=D1=82=D0=BE=20=D1=82?= =?UTF-8?q?=D0=B8=D1=85=D0=BE=D0=B3=D0=BE=20=D1=80=D0=B0=D1=81=D1=88=D0=B8?= =?UTF-8?q?=D1=80=D0=B5=D0=BD=D0=B8=D1=8F=20=D0=B4=D0=BE=20allow-all=20(#4?= =?UTF-8?q?77=20=D1=80=D0=B5=D0=B2=D1=8C=D1=8E)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1: сервер с tool_allowlist=[] (deny-all) грузился в форму пустым TagsInput; submit слал null (allow-all) → админ, открывший deny-all сервер сменить имя/URL, молча отдавал агенту ВСЕ тулы (тот же silent-widen класс #476, что PR закрывает на read-стороне). Вынесен pure-хелпер resolveToolAllowlist: пустое поле + сервер был [] → [] (deny-all сохранён); пустое + был null → null (реально неограниченный остаётся). +тест (5 кейсов, различие []vs null). F2: CHANGELOG/Security — флип семантики allowlist ([]=deny-all, было NULL/allow-all; corrupt→fail-closed deny-all; форма не расширяет deny-all). Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 13 +++++++++ .../components/ai-mcp-server-form.tsx | 13 +++++---- .../ai-mcp-server-form.utils.test.ts | 29 +++++++++++++++++++ .../components/ai-mcp-server-form.utils.ts | 22 ++++++++++++++ 4 files changed, 71 insertions(+), 6 deletions(-) create mode 100644 apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.utils.test.ts create mode 100644 apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.utils.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 795c7502..fe5b5cc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -385,6 +385,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 through that exact share (its own share or an ancestor `includeSubPages` share); any other value now returns the generic "not found" instead of serving the page. (#218) +- **MCP tool-allowlist semantics flipped: an empty `[]` now means deny-all + (previously it was coerced to "no restrictions").** For an external MCP server, + a stored `tool_allowlist` of `[]` now denies **every** tool of that server + (zero tools reach the agent) instead of being treated as an empty/unset filter + that allowed all of them. A corrupt or non-array stored value now **fails + closed** to deny-all rather than silently allowing everything. The admin form + no longer silently widens an existing deny-all server: leaving its tag field + empty preserves `[]` (deny-all) on save instead of NULL-ing the column to + allow-all, so a routine rename/toggle can no longer grant the agent every tool. + "No restrictions" is still expressible — a genuinely unrestricted server stores + NULL, and clearing the field on such a server keeps it NULL. Operationally + significant: audit any server that was created or left with a literal `[]`, as + it now exposes no tools until an explicit allowlist (or NULL) is set. (#476) ## [0.94.0] - 2026-06-26 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 1bdfe10e..aca7e406 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 @@ -28,6 +28,7 @@ import { IAiMcpServerCreate, IAiMcpServerUpdate, } from "@/features/workspace/services/ai-mcp-server-service.ts"; +import { resolveToolAllowlist } from "./ai-mcp-server-form.utils.ts"; const formSchema = z.object({ name: z.string().min(1), @@ -121,12 +122,12 @@ 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; + // An empty tag field means "no restriction" (sent as null) — since #476 the + // server persists a literal `[]` as deny-all (zero tools). But a server that + // was ALREADY deny-all loads into an empty field too; sending null there + // would silently widen it to allow-all on a routine edit, so preserve `[]`. + // See resolveToolAllowlist for the full rationale. + const toolAllowlist = resolveToolAllowlist(values.toolAllowlist, server); if (isEdit && server) { const payload: IAiMcpServerUpdate = { diff --git a/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.utils.test.ts b/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.utils.test.ts new file mode 100644 index 00000000..cb3e124d --- /dev/null +++ b/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.utils.test.ts @@ -0,0 +1,29 @@ +import { describe, it, expect } from "vitest"; +import { resolveToolAllowlist } from "./ai-mcp-server-form.utils.ts"; + +describe("resolveToolAllowlist", () => { + it("sends the typed tools when the field is non-empty", () => { + expect(resolveToolAllowlist(["a", "b"], { toolAllowlist: null })).toEqual([ + "a", + "b", + ]); + }); + + it("creates as null (unrestricted) when empty and there is no server", () => { + expect(resolveToolAllowlist([], undefined)).toBeNull(); + }); + + it("sends null for an empty field on a previously-unrestricted server", () => { + expect(resolveToolAllowlist([], { toolAllowlist: null })).toBeNull(); + }); + + it("preserves deny-all: an empty field on a `[]` server stays `[]`, not null", () => { + // The core #476/#477 guard: editing a deny-all server (rename/toggle) with + // an empty tag field must NOT silently widen it to allow-all. + expect(resolveToolAllowlist([], { toolAllowlist: [] })).toEqual([]); + }); + + it("still sends explicit tools even if the server was deny-all", () => { + expect(resolveToolAllowlist(["x"], { toolAllowlist: [] })).toEqual(["x"]); + }); +}); diff --git a/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.utils.ts b/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.utils.ts new file mode 100644 index 00000000..e40bf820 --- /dev/null +++ b/apps/client/src/features/workspace/components/settings/components/ai-mcp-server-form.utils.ts @@ -0,0 +1,22 @@ +import { IAiMcpServer } from "@/features/workspace/services/ai-mcp-server-service.ts"; + +// Resolve the tool allowlist value to persist from the form field. +// +// An empty tag field normally means "no restriction" and is sent as null so +// the server drops the column (all tools allowed). But a server that was +// ALREADY deny-all (a stored literal `[]`, meaning zero tools — creatable via +// the API) loads into the form as an empty field too. Coercing that empty +// field to null on submit would SILENTLY widen a deny-all server to allow-all +// on any routine edit (rename, toggle) — the exact silent-widen class #476 +// closed on the read side. So when the edited server was deny-all, preserve +// `[]` (deny-all); only a genuinely-unrestricted server (stored null/absent) +// stays null. +export function resolveToolAllowlist( + fieldValue: string[], + server?: Pick, +): string[] | null { + if (fieldValue.length > 0) return fieldValue; + const wasDenyAll = + Array.isArray(server?.toolAllowlist) && server.toolAllowlist.length === 0; + return wasDenyAll ? [] : null; +}