diff --git a/desktop/src/features/agents/ui/McpServersEditor.test.mjs b/desktop/src/features/agents/ui/McpServersEditor.test.mjs index 446111003..e79a73eb4 100644 --- a/desktop/src/features/agents/ui/McpServersEditor.test.mjs +++ b/desktop/src/features/agents/ui/McpServersEditor.test.mjs @@ -23,12 +23,17 @@ import assert from "node:assert/strict"; import { validateMcpServerName, validateMcpServerRow, + validateMcpServerEnvEntry, toRows, toServers, serversEqual, mergeMcpServersByName, + effectiveEnabledCount, + MAX_USER_MCP_SERVERS, + MAX_ENV_VALUE_BYTES, MCP_SERVER_NAME_MAX_LEN, MCP_RESERVED_SERVER_NAME, + RESERVED_ENV_KEYS, } from "./McpServersEditor.tsx"; // ── Invariant 1: name grammar mirrors validate_mcp_servers ───────────────── @@ -301,3 +306,161 @@ test("mergeMcpServersByName_distinct_names_from_all_layers_are_kept", () => { test("mergeMcpServersByName_empty_layers_produce_empty_result", () => { assert.deepEqual(mergeMcpServersByName([], [], []), []); }); + +test("mergeMcpServersByName_sorts_by_ascii_byte_order_not_locale", () => { + // Rust BTreeMap orders by byte value: '_' (0x5F) > 'Z' (0x5A) + // > 'A' (0x41), and uppercase < lowercase. localeCompare would put + // these in a different order (e.g. case-insensitive or locale-dependent). + const servers = [ + { name: "beta", command: "", args: [], env: [], enabled: true }, + { name: "Alpha", command: "", args: [], env: [], enabled: true }, + { name: "_underscore", command: "", args: [], env: [], enabled: true }, + { name: "alpha", command: "", args: [], env: [], enabled: true }, + ]; + const merged = mergeMcpServersByName(servers); + // ASCII: 'A'(65) < '_'(95) < 'a'(97) < 'b'(98) + assert.deepEqual( + merged.map((s) => s.name), + ["Alpha", "_underscore", "alpha", "beta"], + ); +}); + +// ── Invariant 5: effectiveEnabledCount — cap across layers ────────────── + +function makeServer(name, enabled = true) { + return { name, command: "npx", args: [], env: [], enabled }; +} + +test("effectiveEnabledCount_local_only", () => { + const local = [makeServer("a"), makeServer("b", false)]; + assert.equal(effectiveEnabledCount(local, []), 1); +}); + +test("effectiveEnabledCount_inherited_not_overridden", () => { + const local = [makeServer("a")]; + const inherited = [makeServer("b"), makeServer("c")]; + assert.equal(effectiveEnabledCount(local, inherited), 3); +}); + +test("effectiveEnabledCount_inherited_overridden_by_local_not_counted", () => { + // Local row overrides inherited — only the local row's enabled state + // counts, not the inherited one. + const local = [makeServer("a", false)]; + const inherited = [makeServer("a"), makeServer("b")]; + // "a" overridden (local disabled → 0), "b" inherited enabled → 1 + assert.equal(effectiveEnabledCount(local, inherited), 1); +}); + +test("effectiveEnabledCount_disabled_inherited_not_counted", () => { + const local = []; + const inherited = [makeServer("a", false), makeServer("b")]; + assert.equal(effectiveEnabledCount(local, inherited), 1); +}); + +test("effectiveEnabledCount_15_inherited_plus_1_local_hits_cap", () => { + const inherited = Array.from({ length: MAX_USER_MCP_SERVERS }, (_, i) => + makeServer(`inh-${i}`), + ); + const local = [makeServer("local-1")]; + assert.equal( + effectiveEnabledCount(local, inherited), + MAX_USER_MCP_SERVERS + 1, + ); +}); + +test("effectiveEnabledCount_toggling_disabled_to_16th_hits_cap", () => { + // Simulates toggling a disabled local row to enabled when 14 inherited + + // 1 other local are already enabled = 15. The 16th would exceed the cap. + const inherited = Array.from({ length: 14 }, (_, i) => + makeServer(`inh-${i}`), + ); + const local = [makeServer("local-1"), makeServer("local-2", false)]; + // Before toggle: 1 local enabled + 14 inherited = 15 (at cap) + assert.equal(effectiveEnabledCount(local, inherited), MAX_USER_MCP_SERVERS); + // After toggle: 2 local enabled + 14 inherited = 16 (exceeds cap) + const toggled = local.map((s) => + s.name === "local-2" ? { ...s, enabled: true } : s, + ); + assert.equal( + effectiveEnabledCount(toggled, inherited), + MAX_USER_MCP_SERVERS + 1, + ); +}); + +// ── Invariant 6: validateMcpServerEnvEntry — per-server env boundary ──── + +test("validateMcpServerEnvEntry_valid_entry_returns_null", () => { + assert.equal( + validateMcpServerEnvEntry({ name: "MY_TOKEN", value: "abc" }), + null, + ); +}); + +test("validateMcpServerEnvEntry_empty_name_returns_null_skipped", () => { + // Blank rows are skipped by toServers, so no error shown. + assert.equal( + validateMcpServerEnvEntry({ name: "", value: "anything" }), + null, + ); +}); + +test("validateMcpServerEnvEntry_malformed_key_rejected", () => { + const err = validateMcpServerEnvEntry({ name: "1BAD", value: "" }); + assert.match(err, /must match/); +}); + +test("validateMcpServerEnvEntry_key_with_equals_rejected", () => { + const err = validateMcpServerEnvEntry({ name: "KEY=val", value: "" }); + assert.match(err, /must match/); +}); + +test("validateMcpServerEnvEntry_key_with_spaces_rejected", () => { + const err = validateMcpServerEnvEntry({ name: "MY KEY", value: "" }); + assert.match(err, /must match/); +}); + +test("validateMcpServerEnvEntry_reserved_key_rejected", () => { + const err = validateMcpServerEnvEntry({ + name: "BUZZ_ACP_MCP_SERVERS", + value: "", + }); + assert.match(err, /reserved/); +}); + +test("validateMcpServerEnvEntry_reserved_key_case_insensitive", () => { + const err = validateMcpServerEnvEntry({ + name: "buzz_private_key", + value: "", + }); + assert.match(err, /reserved/); +}); + +test("validateMcpServerEnvEntry_nul_in_value_rejected", () => { + const err = validateMcpServerEnvEntry({ name: "TOKEN", value: "abc\0def" }); + assert.match(err, /NUL/); +}); + +test("validateMcpServerEnvEntry_oversized_value_rejected", () => { + const err = validateMcpServerEnvEntry({ + name: "TOKEN", + value: "x".repeat(MAX_ENV_VALUE_BYTES + 1), + }); + assert.match(err, /exceeds/); +}); + +test("validateMcpServerEnvEntry_at_limit_value_accepted", () => { + assert.equal( + validateMcpServerEnvEntry({ + name: "TOKEN", + value: "x".repeat(MAX_ENV_VALUE_BYTES), + }), + null, + ); +}); + +test("validateMcpServerEnvEntry_all_reserved_keys_are_rejected", () => { + for (const key of RESERVED_ENV_KEYS) { + const err = validateMcpServerEnvEntry({ name: key, value: "" }); + assert.ok(err, `Expected "${key}" to be rejected as reserved`); + } +}); diff --git a/desktop/src/features/agents/ui/McpServersEditor.tsx b/desktop/src/features/agents/ui/McpServersEditor.tsx index 70e55c640..9aafbbe6e 100644 --- a/desktop/src/features/agents/ui/McpServersEditor.tsx +++ b/desktop/src/features/agents/ui/McpServersEditor.tsx @@ -39,7 +39,39 @@ export const MCP_RESERVED_SERVER_NAME = "buzz-dev-mcp"; /** Effective/per-layer enabled-server cap. Mirrors `MAX_USER_MCP_SERVERS`. */ export const MAX_USER_MCP_SERVERS = 15; +/** Per-value byte cap. Mirrors `MAX_ENV_VALUE_BYTES` in `env_vars.rs`. */ +export const MAX_ENV_VALUE_BYTES = 32 * 1024; + +/** + * Reserved env keys (case-insensitive). Mirrors `RESERVED_ENV_KEYS` in + * `env_vars.rs`. Keys that override agent identity, code-execution surface, + * security gates, or structured transport must never be user-settable. + */ +export const RESERVED_ENV_KEYS: readonly string[] = [ + "BUZZ_PRIVATE_KEY", + "NOSTR_PRIVATE_KEY", + "BUZZ_AUTH_TAG", + "BUZZ_API_TOKEN", + "BUZZ_ACP_PRIVATE_KEY", + "BUZZ_ACP_API_TOKEN", + "BUZZ_RELAY_URL", + "BUZZ_ACP_AGENT_COMMAND", + "BUZZ_ACP_AGENT_ARGS", + "BUZZ_ACP_MCP_COMMAND", + "BUZZ_ACP_MCP_SERVERS", + "BUZZ_ACP_RESPOND_TO", + "BUZZ_ACP_RESPOND_TO_ALLOWLIST", + "BUZZ_ACP_AGENT_OWNER", + "BUZZ_ACP_SETUP_PAYLOAD", +]; + +const RESERVED_ENV_KEYS_UPPER = new Set( + RESERVED_ENV_KEYS.map((k) => k.toUpperCase()), +); + const NAME_CHARS_RE = /^[A-Za-z0-9_-]*$/; +/** POSIX env key: `[A-Za-z_][A-Za-z0-9_]*`. Mirrors `is_well_formed_env_key`. */ +const ENV_KEY_RE = /^[A-Za-z_][A-Za-z0-9_]*$/; /** * Client-side mirror of the Rust `validate_mcp_servers` name grammar, for @@ -80,6 +112,32 @@ export function validateMcpServerRow( return null; } +/** + * Validate a single per-server env var entry. Mirrors the Rust + * `validate_user_env_keys` boundary (`env_vars.rs`): POSIX key format, + * reserved-key check (case-insensitive), NUL-free values, and per-value + * byte cap. Returns `null` when valid. Exported for unit tests. + */ +export function validateMcpServerEnvEntry(entry: { + name: string; + value: string; +}): string | null { + if (entry.name.length === 0) return null; // blank rows are skipped by toServers + if (!ENV_KEY_RE.test(entry.name)) { + return "Key must match [A-Za-z_][A-Za-z0-9_]*."; + } + if (RESERVED_ENV_KEYS_UPPER.has(entry.name.toUpperCase())) { + return `"${entry.name}" is reserved by Buzz.`; + } + if (entry.value.includes("\0")) { + return "Value cannot contain NUL bytes."; + } + if (new TextEncoder().encode(entry.value).length > MAX_ENV_VALUE_BYTES) { + return `Value exceeds the ${MAX_ENV_VALUE_BYTES}-byte limit.`; + } + return null; +} + type ArgRow = { id: string; value: string }; type EnvRow = { id: string; name: string; value: string }; type ServerRow = { @@ -177,8 +235,8 @@ export function serversEqual(a: McpServersValue, b: McpServersValue): boolean { * effective-server cap: it is a DISPLAY merge for the editor's read-only * "inherited" rows, not the runtime WYSIWYG surface (that's * `RuntimeConfigSurface.buzzAgentMcpServers`, computed server-side). Sorted - * by name for the same deterministic order the backend's `BTreeMap`-backed - * merge produces. Exported for unit tests and dialog callers. + * by ASCII byte order (`<` / `>`) to match Rust's `BTreeMap` + * ordering. Exported for unit tests and dialog callers. */ export function mergeMcpServersByName( ...layers: readonly McpServersValue[] @@ -190,10 +248,28 @@ export function mergeMcpServersByName( } } return Array.from(merged.values()).sort((a, b) => - a.name.localeCompare(b.name), + a.name < b.name ? -1 : a.name > b.name ? 1 : 0, ); } +/** + * Compute the effective enabled count across local and inherited layers. + * Local enabled rows plus inherited enabled rows NOT overridden/masked by + * a local row with the same name. Mirrors the Rust effective-merge cap + * check (`mcp_servers.rs:225-229`). Exported for unit tests. + */ +export function effectiveEnabledCount( + local: McpServersValue, + inherited: McpServersValue, +): number { + const localNames = new Set(local.map((s) => s.name)); + const localEnabled = local.filter((s) => s.enabled).length; + const inheritedEnabled = inherited.filter( + (s) => s.enabled && !localNames.has(s.name), + ).length; + return localEnabled + inheritedEnabled; +} + /** True when a row has any content beyond a bare name — used to decide * whether an empty-name row's "Name is required" hint should show (a * pristine just-added row stays quiet; a row with typed content but no name @@ -245,18 +321,24 @@ export function McpServersEditor({ helperText, disabled = false, }: McpServersEditorProps) { - const [rows, setRows] = React.useState(() => toRows(value)); - const lastEmitted = React.useRef(value); + // Defense-in-depth: coerce at the exported boundary so a caller passing + // `undefined` (e.g. a partial test bridge seed) degrades to empty rather + // than crashing inside `toRows`. Production never produces `undefined` + // (Rust `#[serde(default)]` + `EMPTY_CONFIG`), but a single coercion + // here is cheaper than scattering `?? []` across every mount site. + const safeValue = value ?? []; + const [rows, setRows] = React.useState(() => toRows(safeValue)); + const lastEmitted = React.useRef(safeValue); // Resync from `value` when the parent supplies a value we did not just // emit (e.g. dialog reopened against a different persona/agent). Mirrors // EnvVarsEditor's `recordsEqual` resync guard. React.useEffect(() => { - if (!serversEqual(lastEmitted.current, value)) { - lastEmitted.current = value; - setRows(toRows(value)); + if (!serversEqual(lastEmitted.current, safeValue)) { + lastEmitted.current = safeValue; + setRows(toRows(safeValue)); } - }, [value]); + }, [safeValue]); function emit(nextRows: ServerRow[]) { setRows(nextRows); @@ -337,11 +419,16 @@ export function McpServersEditor({ updateRow(rowId, { env: row.env.filter((e) => e.id !== envId) }); } - const enabledCount = rows.filter((r) => r.enabled).length; - const atCap = enabledCount >= MAX_USER_MCP_SERVERS; + const localNames = new Set(rows.map((r) => r.name)); const visibleInherited = inheritedServers.filter( - (server) => !rows.some((row) => row.name === server.name), + (server) => !localNames.has(server.name), ); + const localEnabledCount = rows.filter((r) => r.enabled).length; + const inheritedEnabledCount = visibleInherited.filter( + (s) => s.enabled, + ).length; + const atCap = + localEnabledCount + inheritedEnabledCount >= MAX_USER_MCP_SERVERS; return (
@@ -433,7 +520,7 @@ export function McpServersEditor({ aria-label={row.enabled ? "Disable server" : "Enable server"} checked={row.enabled} data-testid="mcp-servers-enabled" - disabled={disabled} + disabled={disabled || (!row.enabled && atCap)} onCheckedChange={(checked) => updateRow(row.id, { enabled: checked }) } @@ -530,67 +617,84 @@ export function McpServersEditor({

Environment

- {row.env.map((entry) => ( -
-
- - updateEnvRow(row.id, entry.id, { - name: event.target.value, - }) - } - placeholder="VARIABLE_NAME" - value={entry.name} - /> + {row.env.map((entry) => { + const envError = validateMcpServerEnvEntry(entry); + return ( +
+
+
+ + updateEnvRow(row.id, entry.id, { + name: event.target.value, + }) + } + placeholder="VARIABLE_NAME" + value={entry.name} + /> +
+
+ + updateEnvRow(row.id, entry.id, { + value: event.target.value, + }) + } + placeholder="value" + value={entry.value} + /> +
+ +
+ {envError ? ( +

+ + {envError} +

+ ) : null}
-
- - updateEnvRow(row.id, entry.id, { - value: event.target.value, - }) - } - placeholder="value" - value={entry.value} - /> -
- -
- ))} + ); + })}
diff --git a/desktop/src/testing/e2eBridge.ts b/desktop/src/testing/e2eBridge.ts index 40155247b..f544597cf 100644 --- a/desktop/src/testing/e2eBridge.ts +++ b/desktop/src/testing/e2eBridge.ts @@ -1713,14 +1713,29 @@ function buildMockConfigSurface(pubkey: string): { }, }; - const buzzAgentSurface = { + const buzzAgentBase = { ...gooseSurface, runtimeId: "buzz-agent", runtimeLabel: "Buzz Agent", advanced: [], extensions: [], - // Effective merged (global < definition < agent, enabled-only) servers — - // "what runs." Exercises the WYSIWYG read-only display by default. + sources: { + ...gooseSurface.sources, + configFilePath: null, + mcpConfigFilePath: null, + }, + }; + + // Empty WYSIWYG surface — "No custom servers configured" path. + const buzzAgentEmptySurface = { + ...buzzAgentBase, + buzzAgentMcpServers: [] satisfies McpServerConfig[], + }; + + // Populated WYSIWYG surface — exercises the read-only BuzzAgentMcpServerRow + // rendering with an effective-merged server list. + const buzzAgentPopulatedSurface = { + ...buzzAgentBase, buzzAgentMcpServers: [ { name: "filesystem", @@ -1730,11 +1745,6 @@ function buildMockConfigSurface(pubkey: string): { enabled: true, }, ] satisfies McpServerConfig[], - sources: { - ...gooseSurface.sources, - configFilePath: null, - mcpConfigFilePath: null, - }, }; // Map well-known test pubkeys to specific fixtures. @@ -1743,6 +1753,8 @@ function buildMockConfigSurface(pubkey: string): { "abc1230000000000000000000000000000000000000000000000000000000def"; const PUBKEY_BUZZ_AGENT = "b0220000000000000000000000000000000000000000000000000000000000a9"; + const PUBKEY_BUZZ_AGENT_POPULATED = + "b0220000000000000000000000000000000000000000000000000000000000b1"; switch (pubkey) { case ALICE_PUBKEY: @@ -1756,7 +1768,9 @@ function buildMockConfigSurface(pubkey: string): { case PUBKEY_MULTI_ORIGIN: return multiOriginSurface; case PUBKEY_BUZZ_AGENT: - return buzzAgentSurface; + return buzzAgentEmptySurface; + case PUBKEY_BUZZ_AGENT_POPULATED: + return buzzAgentPopulatedSurface; default: return gooseSurface; } @@ -9307,16 +9321,16 @@ export function maybeInstallE2eTauriMocks() { return null; } case "get_global_agent_config": { - // Return the mock global agent config if provided; otherwise return - // an empty config (no global provider, model, env vars, or MCP servers). - return ( - config?.mock?.globalAgentConfig ?? { - env_vars: {}, - mcp_servers: [], - provider: null, - model: null, - } - ); + // Return the mock global agent config, defaulting each field + // individually so a partial seed (e.g. {env_vars, provider, model} + // without mcp_servers) still produces a valid shape. + return { + env_vars: {}, + mcp_servers: [], + provider: null, + model: null, + ...config?.mock?.globalAgentConfig, + }; } case "set_global_agent_config": { // Echo back the submitted config as the saved value (mirrors the diff --git a/desktop/tests/e2e/config-bridge-screenshots.spec.ts b/desktop/tests/e2e/config-bridge-screenshots.spec.ts index 11d4a73fa..94477e229 100644 --- a/desktop/tests/e2e/config-bridge-screenshots.spec.ts +++ b/desktop/tests/e2e/config-bridge-screenshots.spec.ts @@ -14,6 +14,8 @@ const MULTI_ORIGIN_PUBKEY = "abc1230000000000000000000000000000000000000000000000000000000def"; const BUZZ_AGENT_PUBKEY = "b0220000000000000000000000000000000000000000000000000000000000a9"; +const BUZZ_AGENT_POPULATED_PUBKEY = + "b0220000000000000000000000000000000000000000000000000000000000b1"; const MANAGED_AGENTS = [ { @@ -281,6 +283,30 @@ test.describe("config bridge screenshots", () => { ); }); + test("06b — buzz-agent populated MCP servers", async ({ page }) => { + await installMockBridge(page, { + managedAgents: [ + { + pubkey: BUZZ_AGENT_POPULATED_PUBKEY, + name: "Buzz Populated", + status: "running" as const, + channelNames: ["agents"], + }, + ], + }); + + const panel = await openAgentProfileFromChannel(page, "Buzz Populated"); + + // The populated WYSIWYG surface includes a "filesystem" server row. + await expect(panel.getByText("filesystem", { exact: true })).toBeVisible(); + await expect( + panel.getByText("npx -y @modelcontextprotocol/server-filesystem /tmp"), + ).toBeVisible(); + await expect(panel.getByText("MCP Servers", { exact: true })).toHaveCount( + 1, + ); + }); + test("07 — profile side panel — Configuration section", async ({ page }) => { // charlie (554cef…) is the well-known test pubkey that the mock bridge // seeds as a bot owned by the test viewer, so isBot + isOwner + managedAgent