fix(mcp): форма сохраняет deny-all allowlist вместо тихого расширения до allow-all (#477 ревью)

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) <noreply@anthropic.com>
2026-07-11 10:24:03 +03:00
parent 1e7bd1f9d2
commit 5adcd2f08b
4 changed files with 71 additions and 6 deletions
@@ -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 = {
@@ -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"]);
+  });
+});
@@ -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<IAiMcpServer, "toolAllowlist">,
+): string[] | null {
+  if (fieldValue.length > 0) return fieldValue;
+  const wasDenyAll =
+    Array.isArray(server?.toolAllowlist) && server.toolAllowlist.length === 0;
+  return wasDenyAll ? [] : null;
+}