From c3930ca5ad7182057b3bc0bc729d0b56172a9dcd Mon Sep 17 00:00:00 2001 From: 0xfandom <50949929+0xfandom@users.noreply.github.com> Date: Mon, 21 Sep 2026 06:34:37 +0530 Subject: [PATCH] fix(mcp): resolve server names by own-property, not the prototype chain (#1983) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): resolve server names by own-property, not the prototype chain getMcpConfigByName and the mcp remove handler look names up in plain object maps built from JSON config with a bare servers[name] truthiness check. Reserved names ('constructor', '__proto__', 'toString', …) resolve to inherited Object.prototype members, which are truthy, so: - `mcp get constructor` skips the not-found guard, prints a fabricated record, and hands the Object constructor to the health check; - `mcp remove constructor` reports the reserved name as present in multiple scopes and prompts to pick one instead of "No MCP server found". The same leak reaches runAgent and the print handlers, which all route through getMcpConfigByName. Gate every lookup on Object.hasOwn so only real, own-property server names resolve. * test(mcp): restore project mcpServers in afterEach for isolation Save and restore getCurrentProjectConfig().mcpServers the same way the global config is handled, rather than hardcoding undefined, so the test never leaks state into suites that share the in-memory test config. * fix(mcp): cover the remaining prototype-name mutation and doctor paths The own-property gate only covered unscoped removal and getMcpConfigByName. The scoped paths still used bare lookups: - removeMcpConfig's project/user/local existence checks accepted inherited members, so 'mcp remove constructor -s user' reported a successful removal while leaving the configuration untouched; - addMcpConfig's already-exists checks rejected valid 'mcp add constructor'; - doctor's servers[name] and activeServers[name] fabricated definitions for prototype names. Gate all of them on Object.hasOwn. Also reject the name '__proto__' at add time: it passes the character check but assigning it on a plain object hits the prototype setter instead of creating an own property, so the server would be reported as added and silently vanish. Restore NODE_ENV in the test teardown alongside the config state. * fix(mcp): report a reserved __proto__ entry in file config A hand-authored .mcp.json can contain a server named "__proto__": JSON.parse gives it a real own key, but it cannot be copied onto a plain object, so the schema's rebuild dropped it before validation ever ran. The entry simply did not exist and nothing said why, and with own-property lookups the scoped removal path now correctly reports it as absent -- so there was no way to learn the name was the problem. Detect it on the raw parsed config and surface a fatal validation error naming the entry, matching the rejection addMcpConfig already performs for the same name. The rest of the file still parses. * test(mcp): serialize the proto-name suite on the shared mutation lock The suite swaps NODE_ENV and the process-wide global and project MCP configurations without holding sharedMutationLock, so bun can run it alongside another state-mutating file: that file observes the injected realserver/locallyreal fixtures, or its own updates are overwritten when this teardown restores its snapshots. Acquire the lock in setup and release it from a finally in teardown, the same shape officialRegistry.test.ts uses. Also cover the reserved __proto__ entry arriving from parsed file config. * fix(mcp): reserve constructor alongside __proto__ With own-property lookups in place, `mcp add constructor` persists an entry the config schema then rejects -- and it rejects the whole mcpServers object, so the newly added server is unusable and every other server in that scope stops loading with it. Refuse the name at add, and name it at file ingress: the only diagnostic before was a generic "does not adhere to schema" against `mcpServers`, which does not say which entry is at fault. The scan runs ahead of the schema parse because that path returns early. `__proto__` and `constructor` are the only two names that fail a write/read round trip -- toString, hasOwnProperty, valueOf and the rest all persist and read back correctly. * fix(mcp): reject a config with a reserved server name outright parseMcpConfig flagged a fatal reserved name but, for __proto__, still returned a usable partial config because zod's rebuild silently drops the key and the schema parse succeeds. Callers that branch on the config -- the --mcp-config ingress in main.tsx -- then took the partial config and discarded the error, starting with the bad entry quietly gone. Return config: null whenever a fatal reserved-name error is present, matching the constructor path where the schema already fails, so the invalid input is rejected at every consumer. * fix(mcp): keep reserved-name findings visible in all-server doctor output doctorAllServers builds server reports only for names that survived parsing or are active, and attaches server-keyed findings to those. A fatal reserved-name error is keyed by a name that never survives parsing, so its finding was built into no report and dropped -- mcp doctor --config-only read clean while the invalid config was present. Surface any finding whose server has no report as a global finding. * fix(mcp): do not mutate a fatally poisoned project .mcp.json A reserved server name (`__proto__`/`constructor`) now fails the whole parse and returns config: null, so getProjectMcpConfigsFromCwd reports an empty map even though valid siblings remain on disk. Project-scope add rebuilt the file from that empty snapshot and wrote only the new server, silently dropping every sibling; project-scope remove reported a valid sibling as "not found". Refuse to mutate the file when it fails to parse, naming the offending entry, instead of treating the empty map as truth. * fix(mcp): surface reserved-name errors in single-server doctor output doctorServer set report.findings to only the global findings and never applied the orphan fold doctorAllServers uses, so a fatal reserved-name error in a poisoned scope was dropped when the requested target lived in another scope -- exit 0 with the real fault hidden. Promote findings for names other than the reported one, and skip the state.not_found branch when a validation finding already explains the requested name so the report no longer carries two blocking findings with contradictory text. * fix(mcp): honor the fatal-parse contract across every scope The config:null contract was only enforced on project-scope .mcp.json. Extend it so a fatally poisoned scope is never silently mutated: - Re-check the project snapshot immediately before the write rebuild, not only at the existence check -- the file can go fatal in between, and rebuilding from the resulting empty map would drop valid siblings. - Refuse user/local add and remove when that scope parses to config:null, so the CLI no longer reports success for a scope that then loads nothing (or mutates a sibling in a raw map that stays unloadable). - Gate enterprise exclusive control on the managed file's presence, not on a successful parse, so a poisoned managed-mcp.json can't silently drop the policy lock and let user/project servers take over. Generalize the guard to assertMcpScopeWritable(errors, scopeLabel). * test(mcp): pin allowedSettingSources so the local-scope guard is hermetic getMcpConfigsByScope() returns an empty writability-error list when a scope's setting source is disabled, and allowedSettingSources is a process-wide global other suites mutate. In bun's file order a suite that leaves localSettings disabled (e.g. one exercising --setting-sources) would make the 'fatally poisoned local scope' case skip its guard, so addMcpConfig(...'local') resolved instead of rejecting -- the intermittent CI failure at config.protoName.test.ts. Pin the full source set in beforeEach and restore it in afterEach, matching the NODE_ENV and MCP-config snapshotting this suite already does. * fix(mcp): guard scope mutations on raw parse errors, not the source filter assertMcpScopeWritable read errors from getMcpConfigsByScope()/ getProjectMcpConfigsFromCwd(), which suppress errors to an empty list when a scope's setting source is disabled. addMcpConfig/removeMcpConfig still mutate the raw mcpServers maps, so under a narrowed --setting-sources set a fatally poisoned scope could be written or deleted (clobbering valid siblings) while the CLI reported success. Add getScopeMutationErrors(), which parses each scope's raw source directly, and route every write guard through it. * fix(mcp): detect unscoped-remove membership from parsed scope views Unscoped `mcp remove` read local/user membership from the raw getCurrentProjectConfig()/getGlobalConfig() maps while project used the parsed view. A fatally poisoned local/user scope has no loadable servers yet still holds the raw entry, so the handler listed the server as living there and then removeMcpConfig refused with 'Cannot modify … config'. Read all three scopes through getMcpConfigsByScope() so detection matches what is actually removable. * fix(mcp): read the raw project scope for mutations, not the source view getScopeMutationErrors only covered the write guard's errors; the project existence check and .mcp.json rebuild still read servers from the source-gated getProjectMcpConfigsFromCwd(). With projectSettings excluded from --setting-sources that returned an empty map, so an add rebuilt the file without the valid siblings and a remove mis-reported the server as absent. Drop the source gate from getProjectMcpConfigsFromCwd() (a mutation-only read) so it always sees the real file, and fold the project write guards back onto its returned errors. Regression covers add/remove with projectSettings off. * test(mcp): re-pin the poisoned local fixture atomically per mutation The local-scope poison regressions flaked under the full parallel suite: sharedMutationLock serializes other lock holders but not every writer of the process-wide test project config or NODE_ENV, so a stray async task from another suite could clobber the fixture during an await gap between the setup and the assertion. addMcpConfig/removeMcpConfig reach their scope guard with no awaited work in between, so re-establishing NODE_ENV, the enabled sources, and the poisoned config synchronously immediately before each mutation (pinPoisonedLocalScope) keeps the guard's read atomic regardless of what ran during earlier awaits. * fix(mcp): surface managed-mcp.json parse errors in enterprise mode Enterprise exclusive mode engages on the managed file's presence, so a malformed or reserved-name managed-mcp.json fail-closes every other MCP source. getClaudeCodeMcpConfigs returned errors: [] for that branch, and getSettingsWithAllErrors aggregated only user/project/local, so startup and the MCP UI saw an empty server list with no diagnosable reason. Propagate the enterprise scope's parse errors as generic-error PluginErrors from the exclusive branch and include the enterprise scope in the settings error aggregation. * fix(mcp): detect unscoped-remove membership source-filter-independent Reading membership from getMcpConfigsByScope applied the --setting-sources load filter, so an unscoped `mcp remove foo` reported "No MCP server found" for a clean user/local/project entry that removeMcpConfig(foo, scope) still mutates when that source is narrowed out. Read each scope the same way removeMcpConfig resolves existence -- the raw getCurrentProjectConfig()/getGlobalConfig() maps and getProjectMcpConfigsFromCwd() -- still own-property gated so inherited members are not treated as present. * fix(mcp): surface managed-mcp.json errors in headless startup The headless (-p) MCP consumer destructured only servers from the config promise and discarded errors. A fatal managed-mcp.json fail-closes every file-based MCP source, so scripted users saw an empty server list with no diagnostic — indistinguishable from an intentionally empty configuration. Emit the config errors on stderr in non-interactive sessions (interactive already surfaces them via the MCP error UI). * fix(mcp): enforce enterprise-exclusive boundary in named lookups getMcpConfigByName() gated plugin-only mode but not the enterprise- exclusive policy. A present managed-mcp.json takes exclusive, fail-closed control of MCP (its mere presence engages the policy, even when the file is malformed), and getClaudeCodeMcpConfigs() drops every non-enterprise source accordingly — but the named lookup fell through to the user/ project/local maps. A user with a locally configured server plus an invalid managed file could therefore still resolve that server by name via 'mcp get ' (checkMcpServerHealth) or an agent definition's named-server reference (runAgent), bypassing the intended policy. Return only an enterprise-owned entry (or null) whenever doesEnterpriseMcpConfigExist() is true, before consulting other scopes, mirroring the boundary getClaudeCodeMcpConfigs() already enforces. --- src/cli/handlers/mcp.tsx | 29 +- src/main.tsx | 15 +- src/services/mcp/config.protoName.test.ts | 377 ++++++++++++++++++ src/services/mcp/config.ts | 260 ++++++++++-- src/services/mcp/doctor.test.ts | 95 +++++ src/services/mcp/doctor.ts | 46 ++- .../mcp/enterpriseMcpBoundary.test.ts | 138 +++++++ src/services/mcp/enterpriseMcpErrors.test.ts | 96 +++++ src/services/mcp/headlessErrors.test.ts | 25 ++ src/services/mcp/headlessErrors.ts | 21 + src/utils/settings/allErrors.ts | 6 +- 11 files changed, 1044 insertions(+), 64 deletions(-) create mode 100644 src/services/mcp/config.protoName.test.ts create mode 100644 src/services/mcp/enterpriseMcpBoundary.test.ts create mode 100644 src/services/mcp/enterpriseMcpErrors.test.ts create mode 100644 src/services/mcp/headlessErrors.test.ts create mode 100644 src/services/mcp/headlessErrors.ts diff --git a/src/cli/handlers/mcp.tsx b/src/cli/handlers/mcp.tsx index 8c34b2414..9a2dc8975 100644 --- a/src/cli/handlers/mcp.tsx +++ b/src/cli/handlers/mcp.tsx @@ -19,7 +19,7 @@ import { } from '../../services/mcp/auth.js' import { doctorAllServers, doctorServer, type McpDoctorReport, type McpDoctorScopeFilter } from '../../services/mcp/doctor.js'; import { connectToServer, getMcpServerConnectionBatchSize } from '../../services/mcp/client.js'; -import { addMcpConfig, getAllMcpConfigs, getMcpConfigByName, getMcpConfigsByScope, removeMcpConfig } from '../../services/mcp/config.js'; +import { addMcpConfig, getAllMcpConfigs, getMcpConfigByName, getProjectMcpConfigsFromCwd, removeMcpConfig } from '../../services/mcp/config.js'; import type { ConfigScope, ScopedMcpServerConfig } from '../../services/mcp/types.js'; import { describeMcpConfigFilePath, ensureConfigScope, getScopeLabel } from '../../services/mcp/utils.js'; import { AppStateProvider } from '../../state/AppState.js'; @@ -197,21 +197,24 @@ export async function mcpRemoveHandler(name: string, options: { cliOk(`File modified: ${describeMcpConfigFilePath(scope)}`); } - // If no scope specified, check where the server exists - const projectConfig = getCurrentProjectConfig(); - const globalConfig = getGlobalConfig(); - - // Check if server exists in project scope (.mcp.json) - const { - servers: projectServers - } = getMcpConfigsByScope('project'); - const mcpJsonExists = !!projectServers[name]; + // If no scope specified, check where the server exists. Membership must be + // read the same way removeMcpConfig(name, scope) resolves existence: from + // the raw, source-filter-independent config for each scope. Using + // getMcpConfigsByScope() here would apply the --setting-sources load filter + // and report "No MCP server found" for a clean user/local/project entry that + // removeMcpConfig still deliberately mutates. These maps are plain objects + // from JSON config, so gate every lookup on own-property — a `!!servers[name]` + // / `?.[name]` check treats inherited members ('constructor', '__proto__', …) + // as present. + const localServers = getCurrentProjectConfig().mcpServers ?? {}; + const userServers = getGlobalConfig().mcpServers ?? {}; + const { servers: projectServers } = getProjectMcpConfigsFromCwd(); // Count how many scopes contain this server const scopes: Array> = []; - if (projectConfig.mcpServers?.[name]) scopes.push('local'); - if (mcpJsonExists) scopes.push('project'); - if (globalConfig.mcpServers?.[name]) scopes.push('user'); + if (Object.hasOwn(localServers, name)) scopes.push('local'); + if (Object.hasOwn(projectServers, name)) scopes.push('project'); + if (Object.hasOwn(userServers, name)) scopes.push('user'); if (scopes.length === 0) { cliError(`No MCP server found with name: "${name}"`); } else if (scopes.length === 1) { diff --git a/src/main.tsx b/src/main.tsx index 15aa8e15c..dc9d1ceac 100644 --- a/src/main.tsx +++ b/src/main.tsx @@ -154,6 +154,8 @@ import { registerMcpXaaIdpCommand } from 'src/commands/mcp/xaaIdpCommand.js'; import { fetchClaudeAIMcpConfigsIfEligible } from 'src/services/mcp/claudeai.js'; import { clearServerCache } from 'src/services/mcp/client.js'; import { areMcpConfigsAllowedWithEnterpriseMcpConfig, dedupClaudeAiMcpServers, doesEnterpriseMcpConfigExist, filterMcpServersByPolicy, getClaudeCodeMcpConfigs, getMcpServerSignature, parseMcpConfig, parseMcpConfigFromFilePath } from 'src/services/mcp/config.js'; +import { getHeadlessMcpConfigWarnings } from 'src/services/mcp/headlessErrors.js'; +import type { PluginError } from 'src/types/plugin.js'; import { excludeCommandsByServer, excludeResourcesByServer } from 'src/services/mcp/utils.js'; import { isXaaEnabled } from 'src/services/mcp/xaaIdpLogin.js'; import { getRelevantTips } from 'src/services/tips/tipRegistry.js'; @@ -1737,7 +1739,8 @@ async function run(): Promise { // only explicit --mcp-config works. dynamicMcpConfig is spread onto // allMcpConfigs downstream so it survives this skip. const mcpConfigPromise = (strictMcpConfig || isBareMode() ? Promise.resolve({ - servers: {} as Record + servers: {} as Record, + errors: [] as PluginError[] }) : getClaudeCodeMcpConfigs(dynamicMcpConfig)).then(result => { mcpConfigResolvedMs = Date.now() - mcpConfigStart; return result; @@ -2293,8 +2296,16 @@ async function run(): Promise { } const { - servers: existingMcpConfigs + servers: existingMcpConfigs, + errors: mcpConfigErrors = [] } = await mcpConfigPromise; + // Headless (-p) has no MCP-error UI, so a fatal managed-mcp.json fail-closes + // every file-based source with no diagnostic — indistinguishable from an + // intentionally empty config. Surface those errors on stderr so scripted + // users see why nothing loaded. Interactive surfaces them via the MCP UI. + for (const line of getHeadlessMcpConfigWarnings(isNonInteractiveSession, mcpConfigErrors)) { + process.stderr.write(`${line}\n`); + } // CLI flag (--mcp-config) should override file-based configs, matching settings precedence const allMcpConfigs = { ...existingMcpConfigs, diff --git a/src/services/mcp/config.protoName.test.ts b/src/services/mcp/config.protoName.test.ts new file mode 100644 index 000000000..72e18804f --- /dev/null +++ b/src/services/mcp/config.protoName.test.ts @@ -0,0 +1,377 @@ +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { afterEach, beforeEach, expect, test } from 'bun:test' + +import { + getAllowedSettingSources, + setAllowedSettingSources, +} from '../../bootstrap/state.js' +import { + SETTING_SOURCES, + type SettingSource, +} from '../../utils/settings/constants.js' +import { + acquireSharedMutationLock, + releaseSharedMutationLock, +} from '../../test/sharedMutationLock.js' +import { + getCurrentProjectConfig, + getGlobalConfig, + saveCurrentProjectConfig, + saveGlobalConfig, +} from '../../utils/config.js' +import { runWithCwdOverride } from '../../utils/cwd.js' +import { + addMcpConfig, + getMcpConfigByName, + parseMcpConfig, + removeMcpConfig, +} from './config.js' + +// A hand-authored .mcp.json that fails to parse as a whole (a reserved name +// plus a valid sibling). JSON.parse creates a true own "__proto__" key. +const POISONED_MCP_JSON = + '{"mcpServers":{"__proto__":{"command":"echo","args":[]},' + + '"realone":{"command":"echo","args":[]}}}' + +// The MCP `servers` maps are plain objects built from JSON config, so a bare +// `servers[name]` lookup exposes inherited Object.prototype members. A user +// running `openclaude mcp get constructor` (or `__proto__`, `toString`, …) must +// get "not found", not the Object constructor cast as a server config. +const PROTO_NAMES = [ + 'constructor', + '__proto__', + 'toString', + 'hasOwnProperty', + 'valueOf', + 'isPrototypeOf', +] + +let savedGlobalMcp: ReturnType['mcpServers'] +let savedProjectMcp: ReturnType['mcpServers'] + +let savedNodeEnv: string | undefined +let savedSettingSources: ReturnType + +beforeEach(async () => { + // This suite swaps NODE_ENV and the process-wide global/project MCP configs, + // so it has to be serialized against the other state-mutating suites bun may + // run alongside it -- otherwise they observe the injected fixtures, or their + // updates are clobbered when this teardown restores a stale snapshot. + await acquireSharedMutationLock('services/mcp/config.protoName.test.ts') + savedNodeEnv = process.env.NODE_ENV + process.env.NODE_ENV = 'test' + // getMcpConfigsByScope() short-circuits a scope's writability errors to an + // empty list when that scope's setting source is disabled, and + // allowedSettingSources is a process-wide global other suites mutate (and may + // leave without localSettings if they crash before their own teardown). If + // localSettings were disabled here the "fatally poisoned local scope" guard + // would never fire and addMcpConfig would resolve instead of rejecting, so + // pin the full source set for the duration of this suite. + savedSettingSources = getAllowedSettingSources() + setAllowedSettingSources([...SETTING_SOURCES]) + savedGlobalMcp = getGlobalConfig().mcpServers + savedProjectMcp = getCurrentProjectConfig().mcpServers + saveGlobalConfig(config => ({ + ...config, + mcpServers: { realserver: { command: 'echo', args: [] } }, + })) + saveCurrentProjectConfig(config => ({ + ...config, + mcpServers: { locallyreal: { command: 'echo', args: [] } }, + })) +}) + +afterEach(() => { + try { + saveGlobalConfig(config => ({ ...config, mcpServers: savedGlobalMcp })) + saveCurrentProjectConfig(config => ({ + ...config, + mcpServers: savedProjectMcp, + })) + if (savedNodeEnv === undefined) { + delete process.env.NODE_ENV + } else { + process.env.NODE_ENV = savedNodeEnv + } + setAllowedSettingSources(savedSettingSources) + } finally { + releaseSharedMutationLock() + } +}) + +test('resolves a real server by name', () => { + const found = getMcpConfigByName('realserver') + expect(found).not.toBeNull() + expect(found?.scope).toBe('user') +}) + +test('returns null for a plainly missing name', () => { + expect(getMcpConfigByName('nope-not-here')).toBeNull() +}) + +test('returns null for Object.prototype member names', () => { + // Before the fix each of these resolved to an inherited function (truthy), + // so the lookup returned a bogus config and callers skipped their not-found + // guard. + for (const name of PROTO_NAMES) { + expect(getMcpConfigByName(name)).toBeNull() + } +}) + +test('rejects adding a server under a reserved name', async () => { + // "constructor" persists, but the schema then rejects the whole mcpServers + // object on the next read -- so adding it takes down every other server in + // that scope too. + await expect( + addMcpConfig('constructor', { command: 'echo', args: [] }, 'user'), + ).rejects.toThrow('reserved') +}) + +test('rejects adding a server named __proto__', async () => { + // "__proto__" passes the character check but assigning it on a plain object + // hits the prototype setter, so the server would be reported as added and + // silently vanish rather than becoming an own property. + await expect( + addMcpConfig('__proto__', { command: 'echo', args: [] }, 'user'), + ).rejects.toThrow('reserved') +}) + +test('reports proto-name removal as not found instead of succeeding', async () => { + // Before the fix the scoped-removal existence checks accepted inherited + // members, so `mcp remove constructor -s user` claimed success while leaving + // the real configuration untouched. + for (const name of PROTO_NAMES) { + await expect(removeMcpConfig(name, 'user')).rejects.toThrow( + 'No user-scoped MCP server found', + ) + } +}) + +test('surfaces a __proto__ entry and rejects the whole file', () => { + // A hand-authored .mcp.json can carry this name: the schema accepts it, but + // copying it into a plain object hits the prototype setter, so the entry + // vanished with no diagnostic and the user could not tell why their server + // did not exist. It must be reported, not silently discarded. + // Parsed from text, exactly as the real file is: JSON.parse creates a true + // own "__proto__" key, which an object literal would not. + const { config, errors } = parseMcpConfig({ + configObject: JSON.parse( + '{"mcpServers":{"__proto__":{"command":"echo","args":[]},' + + '"realone":{"command":"echo","args":[]}}}', + ), + expandVars: false, + scope: 'project', + filePath: '/tmp/.mcp.json', + }) + + const protoError = errors.find(e => e.path === 'mcpServers.__proto__') + expect(protoError).toBeDefined() + expect(protoError?.message).toContain('reserved') + expect(protoError?.mcpErrorMetadata?.severity).toBe('fatal') + // A fatal reserved name rejects the whole input (config: null), the same as + // "constructor" does when the schema fails -- so a caller that branches on + // config cannot start with the bad entry quietly dropped and the error lost. + expect(config).toBeNull() +}) + +test('refuses to add a project server when .mcp.json is fatally poisoned', async () => { + // The fatal parse returns config: null, so getProjectMcpConfigsFromCwd reports + // an empty map even though `realone` is on disk. Rebuilding the file from that + // empty snapshot and writing only the new server would drop the valid sibling, + // so the add must refuse and leave the file untouched. + const dir = mkdtempSync(join(tmpdir(), 'mcp-add-poison-')) + const mcpPath = join(dir, '.mcp.json') + writeFileSync(mcpPath, POISONED_MCP_JSON) + try { + await runWithCwdOverride(dir, async () => { + await expect( + addMcpConfig('newsrv', { command: 'echo', args: [] }, 'project'), + ).rejects.toThrow('Cannot modify .mcp.json') + }) + // The valid sibling survives: the file was not rewritten. + expect(readFileSync(mcpPath, 'utf8')).toBe(POISONED_MCP_JSON) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +test('refuses to remove a project server when .mcp.json is fatally poisoned', async () => { + // An empty parsed map is not proof the server is absent, so removal must not + // report "not found" (nor clobber the siblings) -- it refuses with the fatal + // parse error for both a valid sibling and the reserved key itself. + const dir = mkdtempSync(join(tmpdir(), 'mcp-remove-poison-')) + const mcpPath = join(dir, '.mcp.json') + writeFileSync(mcpPath, POISONED_MCP_JSON) + try { + await runWithCwdOverride(dir, async () => { + await expect(removeMcpConfig('realone', 'project')).rejects.toThrow( + 'Cannot modify .mcp.json', + ) + await expect(removeMcpConfig('__proto__', 'project')).rejects.toThrow( + 'Cannot modify .mcp.json', + ) + }) + expect(readFileSync(mcpPath, 'utf8')).toBe(POISONED_MCP_JSON) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +test('still adds and removes a project server when .mcp.json is clean', async () => { + const dir = mkdtempSync(join(tmpdir(), 'mcp-clean-')) + const mcpPath = join(dir, '.mcp.json') + writeFileSync( + mcpPath, + '{"mcpServers":{"realone":{"command":"echo","args":[]}}}', + ) + try { + await runWithCwdOverride(dir, async () => { + await addMcpConfig('newsrv', { command: 'echo', args: [] }, 'project') + const afterAdd = JSON.parse(readFileSync(mcpPath, 'utf8')) + expect(Object.keys(afterAdd.mcpServers).sort()).toEqual([ + 'newsrv', + 'realone', + ]) + await removeMcpConfig('realone', 'project') + const afterRemove = JSON.parse(readFileSync(mcpPath, 'utf8')) + expect(Object.keys(afterRemove.mcpServers)).toEqual(['newsrv']) + }) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +test('preserves .mcp.json siblings on mutation when projectSettings is disabled', async () => { + // A mutation must read the real .mcp.json, not the load-time source view. + // With projectSettings excluded from --setting-sources the load helper + // reports no project servers, so rebuilding the file from that empty view + // would drop the valid sibling on add and mis-report the server as absent on + // remove. The mutation path reads the raw file, so both stay correct. + const dir = mkdtempSync(join(tmpdir(), 'mcp-project-disabled-')) + const mcpPath = join(dir, '.mcp.json') + writeFileSync( + mcpPath, + '{"mcpServers":{"realone":{"command":"echo","args":[]}}}', + ) + setAllowedSettingSources( + SETTING_SOURCES.filter(source => source !== 'projectSettings'), + ) + try { + await runWithCwdOverride(dir, async () => { + await addMcpConfig('newsrv', { command: 'echo', args: [] }, 'project') + const afterAdd = JSON.parse(readFileSync(mcpPath, 'utf8')) + // The valid sibling survives the rebuild instead of being dropped. + expect(Object.keys(afterAdd.mcpServers).sort()).toEqual([ + 'newsrv', + 'realone', + ]) + await removeMcpConfig('realone', 'project') + const afterRemove = JSON.parse(readFileSync(mcpPath, 'utf8')) + expect(Object.keys(afterRemove.mcpServers)).toEqual(['newsrv']) + }) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +test('refuses user add/remove when the user scope is fatally poisoned', async () => { + // A hand-authored ~/.openclaude.json can carry an own "__proto__" server. The + // scope then parses to config: null, so an add would claim success while the + // scope loads nothing, and a remove would mutate siblings in a raw map that + // stays unloadable. Both must refuse. + saveGlobalConfig(config => ({ + ...config, + mcpServers: JSON.parse( + '{"__proto__":{"command":"echo","args":[]},' + + '"realuser":{"command":"echo","args":[]}}', + ), + })) + await expect( + addMcpConfig('newsrv', { command: 'echo', args: [] }, 'user'), + ).rejects.toThrow('Cannot modify user config') + await expect(removeMcpConfig('realuser', 'user')).rejects.toThrow( + 'Cannot modify user config', + ) +}) + +// Re-establish the poisoned local-scope fixture synchronously, immediately +// before the mutation under test. addMcpConfig/removeMcpConfig reach their scope +// write guard with no awaited work in between, so the guard's read of NODE_ENV, +// the enabled setting sources, and the process-wide test project config all run +// in the same tick as this call. Pinning all three here -- with no await +// separating it from the mutation -- keeps the read atomic even if another +// suite's stray async task clobbered NODE_ENV or testProjectConfigForTesting +// during an await gap earlier in this test. sharedMutationLock only serializes +// other lock holders, not every writer of the shared test config, so this is +// the isolation that makes the regression stable under the full parallel suite. +function pinPoisonedLocalScope(sources: SettingSource[]): void { + process.env.NODE_ENV = 'test' + setAllowedSettingSources(sources) + saveCurrentProjectConfig(config => ({ + ...config, + mcpServers: JSON.parse( + '{"__proto__":{"command":"echo","args":[]},' + + '"reallocal":{"command":"echo","args":[]}}', + ), + })) +} + +test('refuses local add/remove when the local scope is fatally poisoned', async () => { + pinPoisonedLocalScope([...SETTING_SOURCES]) + await expect( + addMcpConfig('newsrv', { command: 'echo', args: [] }, 'local'), + ).rejects.toThrow('Cannot modify local config') + pinPoisonedLocalScope([...SETTING_SOURCES]) + await expect(removeMcpConfig('reallocal', 'local')).rejects.toThrow( + 'Cannot modify local config', + ) +}) + +test('still refuses a poisoned scope whose setting source is disabled', async () => { + // The mutation guards must not defer to the load-time source filter: + // getMcpConfigsByScope() reports no errors for a disabled source, but + // add/remove still write the raw map, so a fatally poisoned scope has to be + // refused even when the user narrowed --setting-sources to exclude it. + const withoutLocal = SETTING_SOURCES.filter( + source => source !== 'localSettings', + ) + pinPoisonedLocalScope(withoutLocal) + await expect( + addMcpConfig('newsrv', { command: 'echo', args: [] }, 'local'), + ).rejects.toThrow('Cannot modify local config') + pinPoisonedLocalScope(withoutLocal) + await expect(removeMcpConfig('reallocal', 'local')).rejects.toThrow( + 'Cannot modify local config', + ) +}) + +test('still allows adding and removing a real server name', async () => { + await addMcpConfig('addedserver', { command: 'echo', args: [] }, 'user') + expect(getMcpConfigByName('addedserver')).not.toBeNull() + await removeMcpConfig('addedserver', 'user') + expect(getMcpConfigByName('addedserver')).toBeNull() +}) + +test('names a reserved constructor entry instead of failing the whole file', () => { + // The schema rejects the entire mcpServers object for this name, so every + // other server in the scope stops loading. Before, the only diagnostic was a + // generic "does not adhere to schema" against `mcpServers`, which does not + // say which entry is at fault. + const { errors } = parseMcpConfig({ + configObject: JSON.parse( + '{"mcpServers":{"constructor":{"command":"echo","args":[]},' + + '"realone":{"command":"echo","args":[]}}}', + ), + expandVars: false, + scope: 'project', + filePath: '/tmp/.mcp.json', + }) + + const named = errors.find(e => e.path === 'mcpServers.constructor') + expect(named).toBeDefined() + expect(named?.message).toContain('reserved') + expect(named?.mcpErrorMetadata?.severity).toBe('fatal') +}) diff --git a/src/services/mcp/config.ts b/src/services/mcp/config.ts index 0f27e47f3..50eb36477 100644 --- a/src/services/mcp/config.ts +++ b/src/services/mcp/config.ts @@ -66,6 +66,13 @@ export function getEnterpriseMcpFilePath(): string { /** * Internal utility: Add scope to server configs */ +/** + * Server names that cannot survive a write/read round trip, so they are refused + * at every ingress rather than accepted and lost. See addMcpConfig and + * parseMcpConfig for what each one does. + */ +const RESERVED_MCP_SERVER_NAMES: readonly string[] = ['__proto__', 'constructor'] + function addScopeToServers( servers: Record | undefined, scope: ConfigScope, @@ -622,6 +629,28 @@ function expandEnvVars(config: McpServerConfig): { * @param scope The configuration scope * @throws Error if name is invalid or server already exists, or if the config is invalid */ +/** + * A fatal parse of a scope's MCP config (a reserved server name such as + * `__proto__`, or a schema violation) returns `config: null`, so the scope + * reports an empty server map even though the entries are still on disk. + * Rebuilding from that empty snapshot would drop every valid sibling, and an + * add would report success for a scope that then loads nothing -- so refuse to + * mutate a fatally poisoned scope and point the user at the entry to fix. + */ +function assertMcpScopeWritable( + errors: ValidationError[], + scopeLabel: string, +): void { + const fatal = errors.filter(e => e.mcpErrorMetadata?.severity === 'fatal') + if (fatal.length === 0) { + return + } + const detail = fatal.map(e => e.message).join(' ') + throw new Error( + `Cannot modify ${scopeLabel}: ${detail} Fix or remove the offending entry, then retry.`, + ) +} + export async function addMcpConfig( name: string, config: unknown, @@ -633,6 +662,16 @@ export async function addMcpConfig( ) } + // These pass the character check but cannot be stored and read back. + // "__proto__" assigns through the prototype setter instead of creating an own + // property, so the server would be reported as added and silently vanish. + // "constructor" persists, but the config schema then rejects the entire + // mcpServers object on the next read -- so adding it takes down every other + // server in that scope as well. + if (RESERVED_MCP_SERVER_NAMES.includes(name)) { + throw new Error(`Cannot add MCP server "${name}": this name is reserved.`) + } + // Block reserved server name "claude-in-chrome" if (isClaudeInChromeMCPServer(name)) { throw new Error(`Cannot add MCP server "${name}": this name is reserved.`) @@ -681,22 +720,27 @@ export async function addMcpConfig( // Check if server already exists in the target scope switch (scope) { case 'project': { - const { servers } = getProjectMcpConfigsFromCwd() - if (servers[name]) { + const { servers, errors } = getProjectMcpConfigsFromCwd() + assertMcpScopeWritable(errors, '.mcp.json') + if (Object.hasOwn(servers, name)) { throw new Error(`MCP server ${name} already exists in .mcp.json`) } break } case 'user': { + // A fatally poisoned user scope parses to an empty map, so the add would + // silently write into a config that then loads no servers at all. + assertMcpScopeWritable(getScopeMutationErrors('user'), 'user config') const globalConfig = getGlobalConfig() - if (globalConfig.mcpServers?.[name]) { + if (Object.hasOwn(globalConfig.mcpServers ?? {}, name)) { throw new Error(`MCP server ${name} already exists in user config`) } break } case 'local': { + assertMcpScopeWritable(getScopeMutationErrors('local'), 'local config') const projectConfig = getCurrentProjectConfig() - if (projectConfig.mcpServers?.[name]) { + if (Object.hasOwn(projectConfig.mcpServers ?? {}, name)) { throw new Error(`MCP server ${name} already exists in local config`) } break @@ -712,7 +756,11 @@ export async function addMcpConfig( // Add based on scope switch (scope) { case 'project': { - const { servers: existingServers } = getProjectMcpConfigsFromCwd() + const { servers: existingServers, errors } = getProjectMcpConfigsFromCwd() + // Re-check the exact snapshot we are about to rebuild from: the file may + // have become fatally invalid since the existence check above, and writing + // from the resulting empty map would drop every valid sibling. + assertMcpScopeWritable(errors, '.mcp.json') const mcpServers: Record = {} for (const [serverName, serverConfig] of Object.entries( @@ -772,9 +820,13 @@ export async function removeMcpConfig( ): Promise { switch (scope) { case 'project': { - const { servers: existingServers } = getProjectMcpConfigsFromCwd() + const { servers: existingServers, errors } = getProjectMcpConfigsFromCwd() + // A fatally poisoned file parses to an empty map, so an empty result is + // not proof the server is absent -- refuse rather than treat it as such + // (which would also clobber every valid sibling on write-back). + assertMcpScopeWritable(errors, '.mcp.json') - if (!existingServers[name]) { + if (!Object.hasOwn(existingServers, name)) { throw new Error(`No MCP server found with name: ${name} in .mcp.json`) } @@ -798,8 +850,12 @@ export async function removeMcpConfig( } case 'user': { + // An empty parsed map from a fatally poisoned scope is not proof of + // absence; refuse rather than mutate a sibling in the raw map while the + // scope stays unloadable. + assertMcpScopeWritable(getScopeMutationErrors('user'), 'user config') const config = getGlobalConfig() - if (!config.mcpServers?.[name]) { + if (!Object.hasOwn(config.mcpServers ?? {}, name)) { throw new Error(`No user-scoped MCP server found with name: ${name}`) } saveGlobalConfig(current => { @@ -813,9 +869,10 @@ export async function removeMcpConfig( } case 'local': { + assertMcpScopeWritable(getScopeMutationErrors('local'), 'local config') // Check if server exists before updating const config = getCurrentProjectConfig() - if (!config.mcpServers?.[name]) { + if (!Object.hasOwn(config.mcpServers ?? {}, name)) { throw new Error(`No project-local MCP server found with name: ${name}`) } saveCurrentProjectConfig(current => { @@ -838,17 +895,19 @@ export async function removeMcpConfig( * Used by addMcpConfig and removeMcpConfig to modify the local .mcp.json file. * Exported for testing purposes. * + * This is a *mutation-path* read, so it deliberately does NOT gate on + * isSettingSourceEnabled('projectSettings'): add/remove rebuild .mcp.json from + * the servers it returns, and suppressing them to an empty map when the source + * is disabled would drop every valid sibling on write-back (and hide a fatally + * poisoned file from the write guard). The load-time source filter is applied + * by getMcpConfigsByScope('project') instead, which this never feeds. + * * @returns Servers with scope information and any validation errors from current directory's .mcp.json */ export function getProjectMcpConfigsFromCwd(): { servers: Record errors: ValidationError[] } { - // Check if project source is enabled - if (!isSettingSourceEnabled('projectSettings')) { - return { servers: {}, errors: [] } - } - const mcpJsonPath = join(getCwd(), '.mcp.json') const { config, errors } = parseMcpConfigFromFilePath({ @@ -1025,35 +1084,99 @@ export function getMcpConfigsByScope( } } +/** + * Fatal parse errors for the user/local scope's *raw* config, independent of + * whether that setting source is currently enabled. + * + * getMcpConfigsByScope() suppresses errors to an empty list when + * isSettingSourceEnabled() is false for the scope. That is correct for loading + * -- a disabled source contributes no servers -- but addMcpConfig()/ + * removeMcpConfig() still mutate the raw mcpServers maps via getGlobalConfig()/ + * getCurrentProjectConfig(). Reusing the load-time helper for the write guard + * means a fatally poisoned scope can be written or deleted (clobbering valid + * siblings on write-back) while the CLI reports success whenever the user runs + * with a narrowed --setting-sources set. Parse the raw source directly so the + * mutation guards fire regardless of the load-time source filter. + * + * The project scope is not handled here: getProjectMcpConfigsFromCwd() is + * already an ungated mutation-path read and returns both the raw servers and + * their errors together, so the project mutations use it directly. + */ +function getScopeMutationErrors(scope: 'user' | 'local'): ValidationError[] { + switch (scope) { + case 'user': { + const mcpServers = getGlobalConfig().mcpServers + if (!mcpServers) { + return [] + } + return parseMcpConfig({ + configObject: { mcpServers }, + expandVars: true, + scope: 'user', + }).errors + } + case 'local': { + const mcpServers = getCurrentProjectConfig().mcpServers + if (!mcpServers) { + return [] + } + return parseMcpConfig({ + configObject: { mcpServers }, + expandVars: true, + scope: 'local', + }).errors + } + } +} + /** * Get an MCP server configuration by name * @param name The name of the server * @returns The server configuration with scope, or undefined if not found */ export function getMcpConfigByName(name: string): ScopedMcpServerConfig | null { + // The `servers` maps are plain object literals populated from JSON config, so + // a bare `servers[name]` lookup resolves inherited Object.prototype members + // (`constructor`, `toString`, `__proto__`, …) as truthy values. `mcp get + // constructor` would then skip the not-found guard and pass the Object + // constructor on as a server config. Gate every lookup on own-property. const { servers: enterpriseServers } = getMcpConfigsByScope('enterprise') + // A present managed-mcp.json takes exclusive, fail-closed control of MCP + // (even when malformed — its mere presence engages the policy). Named lookups + // must honor the same boundary getClaudeCodeMcpConfigs() enforces, or a + // user/local server could still be resolved by name (mcp get , an + // agent's named-server reference) and connected, bypassing the policy. Return + // only an enterprise-owned entry (or null). + if (doesEnterpriseMcpConfigExist()) { + return Object.hasOwn(enterpriseServers, name) + ? enterpriseServers[name]! + : null + } + // When MCP is locked to plugin-only, only enterprise servers are reachable // by name. User/project/local servers are blocked — same as getClaudeCodeMcpConfigs(). if (isRestrictedToPluginOnly('mcp')) { - return enterpriseServers[name] ?? null + return Object.hasOwn(enterpriseServers, name) + ? enterpriseServers[name]! + : null } const { servers: userServers } = getMcpConfigsByScope('user') const { servers: projectServers } = getMcpConfigsByScope('project') const { servers: localServers } = getMcpConfigsByScope('local') - if (enterpriseServers[name]) { - return enterpriseServers[name] + if (Object.hasOwn(enterpriseServers, name)) { + return enterpriseServers[name]! } - if (localServers[name]) { - return localServers[name] + if (Object.hasOwn(localServers, name)) { + return localServers[name]! } - if (projectServers[name]) { - return projectServers[name] + if (Object.hasOwn(projectServers, name)) { + return projectServers[name]! } - if (userServers[name]) { - return userServers[name] + if (Object.hasOwn(userServers, name)) { + return userServers[name]! } return null @@ -1077,7 +1200,8 @@ export async function getClaudeCodeMcpConfigs( servers: Record errors: PluginError[] }> { - const { servers: enterpriseServers } = getMcpConfigsByScope('enterprise') + const { servers: enterpriseServers, errors: enterpriseErrors } = + getMcpConfigsByScope('enterprise') // If an enterprise mcp config exists, do not use any others; this has exclusive control over all MCP servers // (enterprise customers often do not want their users to be able to add their own MCP servers). @@ -1092,7 +1216,19 @@ export async function getClaudeCodeMcpConfigs( filtered[name] = serverConfig } - return { servers: filtered, errors: [] } + // Enterprise mode is fail-closed: a malformed or reserved-name + // managed-mcp.json disables every other MCP source and blocks dynamic + // configuration. Propagate its parse errors instead of a silent empty list + // so startup and the MCP UI can diagnose why no servers loaded. + const errors: PluginError[] = enterpriseErrors.map(error => ({ + type: 'generic-error', + source: getEnterpriseMcpFilePath(), + error: `Managed MCP config is invalid${ + error.path ? ` (${error.path})` : '' + }: ${error.message}`, + })) + + return { servers: filtered, errors } } // Load other scopes — unless the managed policy locks MCP to plugin-only. @@ -1304,24 +1440,57 @@ export function parseMcpConfig(params: { errors: ValidationError[] } { const { configObject, expandVars, scope, filePath } = params + + // A hand-authored .mcp.json can contain one of these names, and neither + // survives being read back. "__proto__" cannot be copied onto a plain object + // -- the assignment invokes the prototype setter instead of creating an own + // property -- so the schema's rebuild drops it silently. "constructor" is + // worse: the schema rejects the whole mcpServers object, so every other + // server in that scope stops loading too. Either way nothing said which + // entry was at fault. Name it. + const reservedNameErrors: ValidationError[] = [] + const rawServers = (configObject as { mcpServers?: unknown } | null) + ?.mcpServers + if (rawServers !== null && typeof rawServers === 'object') { + for (const reserved of RESERVED_MCP_SERVER_NAMES) { + if (!Object.hasOwn(rawServers, reserved)) continue + reservedNameErrors.push({ + ...(filePath && { file: filePath }), + path: `mcpServers.${reserved}`, + message: `Invalid MCP server name "${reserved}": this name is reserved.`, + suggestion: `Rename the "${reserved}" entry under mcpServers.`, + mcpErrorMetadata: { + scope, + serverName: reserved, + severity: 'fatal', + }, + }) + } + } + const schemaResult = McpJsonConfigSchema().safeParse(configObject) if (!schemaResult.success) { return { config: null, - errors: schemaResult.error.issues.map(issue => ({ - ...(filePath && { file: filePath }), - path: issue.path.join('.'), - message: 'Does not adhere to MCP server configuration schema', - mcpErrorMetadata: { - scope, - severity: 'fatal', - }, - })), + errors: [ + ...reservedNameErrors, + ...schemaResult.error.issues.map( + (issue): ValidationError => ({ + ...(filePath && { file: filePath }), + path: issue.path.join('.'), + message: 'Does not adhere to MCP server configuration schema', + mcpErrorMetadata: { + scope, + severity: 'fatal', + }, + }), + ), + ], } } // Validate each server and expand variables if requested - const errors: ValidationError[] = [] + const errors: ValidationError[] = [...reservedNameErrors] const validatedServers: Record = {} for (const [name, config] of Object.entries(schemaResult.data.mcpServers)) { @@ -1370,6 +1539,15 @@ export function parseMcpConfig(params: { validatedServers[name] = configToCheck } + // A reserved name is fatal. For "constructor" the schema already failed and + // returned config: null above, but "__proto__" passes the schema (zod's + // rebuild silently drops the key), so without this the parse would succeed + // with a partial config and callers that branch on config -- e.g. the + // --mcp-config ingress in main.tsx -- would start with the bad entry quietly + // dropped and the error discarded. Reject the whole input either way. + if (reservedNameErrors.length > 0) { + return { config: null, errors } + } return { config: { mcpServers: validatedServers }, errors, @@ -1468,12 +1646,12 @@ export function parseMcpConfigFromFilePath(params: { } export const doesEnterpriseMcpConfigExist = memoize((): boolean => { - const { config } = parseMcpConfigFromFilePath({ - filePath: getEnterpriseMcpFilePath(), - expandVars: true, - scope: 'enterprise', - }) - return config !== null + // Engage enterprise exclusive control on the managed file's PRESENCE, not on a + // successful parse. A fatally poisoned managed-mcp.json (e.g. a reserved + // `__proto__` entry now rejects the whole file) must not silently disable the + // policy lock and let user/project/local servers take over -- fail safe by + // keeping enterprise mode engaged while the file exists on disk. + return getFsImplementation().existsSync(getEnterpriseMcpFilePath()) }) /** diff --git a/src/services/mcp/doctor.test.ts b/src/services/mcp/doctor.test.ts index a998bc2cb..0cdb7320d 100644 --- a/src/services/mcp/doctor.test.ts +++ b/src/services/mcp/doctor.test.ts @@ -240,6 +240,101 @@ test('doctorAllServers reports global validation findings once without duplicati assert.deepEqual(report.servers[0]?.findings, []) }) +test('doctorAllServers surfaces a finding for a name that produced no server report', async () => { + // A fatal reserved-name error ("__proto__") is keyed by a server name that + // never survives parsing, so it is absent from the active/parsed name set and + // gets no server report. Its finding must still be emitted -- otherwise + // `mcp doctor --config-only` reads clean while the invalid config is present. + const deps = makeDependencies({ + getAllMcpConfigs: async () => allConfig(), + getMcpConfigsByScope: scope => + scope === 'project' + ? scopeConfig({}, [ + { + file: '.mcp.json', + path: 'mcpServers.__proto__', + message: 'Invalid MCP server name "__proto__": this name is reserved.', + suggestion: 'Rename the "__proto__" entry under mcpServers.', + mcpErrorMetadata: { + scope: 'project', + serverName: '__proto__', + severity: 'fatal', + }, + }, + ]) + : scopeConfig(), + }) + + const report = await doctorAllServers({ configOnly: true }, deps) + + const reserved = report.findings.find(f => f.serverName === '__proto__') + assert.ok(reserved, 'reserved-name finding should surface globally') + assert.equal(reserved?.blocking, true) + assert.equal(report.summary.blocking, 1) +}) + +const PROTO_RESERVED_ERROR: ValidationError = { + file: '.mcp.json', + path: 'mcpServers.__proto__', + message: 'Invalid MCP server name "__proto__": this name is reserved.', + suggestion: 'Rename the "__proto__" entry under mcpServers.', + mcpErrorMetadata: { + scope: 'project', + serverName: '__proto__', + severity: 'fatal', + }, +} + +test('doctorServer surfaces a poisoned sibling scope while reporting another target', async () => { + // `mcp doctor realserver` with realserver in user settings but a poisoned + // project .mcp.json must not exit 0 with the fatal project error hidden. The + // reserved-name finding is keyed by a name that never survives parsing, so + // only the orphan fold surfaces it. + const deps = makeDependencies({ + getAllMcpConfigs: async () => + allConfig({ realserver: stdioConfig('user', 'node') }), + getMcpConfigsByScope: scope => + scope === 'user' + ? scopeConfig({ realserver: stdioConfig('user', 'node') }) + : scope === 'project' + ? scopeConfig({}, [PROTO_RESERVED_ERROR]) + : scopeConfig(), + }) + + const report = await doctorServer('realserver', { configOnly: true }, deps) + + const reserved = report.findings.find(f => f.serverName === '__proto__') + assert.ok(reserved, 'poisoned sibling finding must surface') + assert.equal(reserved?.blocking, true) + assert.ok(report.summary.blocking >= 1) +}) + +test('doctorServer does not add not_found when a reserved-name finding already explains the target', async () => { + // `mcp doctor __proto__` against a file that still carries that entry attaches + // the fatal reserved-name finding; pushing state.not_found on top would report + // two blocking findings with contradictory messages. + const deps = makeDependencies({ + getMcpConfigsByScope: scope => + scope === 'project' + ? scopeConfig({}, [PROTO_RESERVED_ERROR]) + : scopeConfig(), + }) + + const report = await doctorServer('__proto__', { configOnly: true }, deps) + + const server = report.servers[0] + assert.ok( + server?.findings.some(f => f.serverName === '__proto__' && f.blocking), + 'reserved-name finding should explain the target', + ) + assert.equal( + server?.findings.some(f => f.code === 'state.not_found'), + false, + 'not_found must not double up on the reserved-name finding', + ) + assert.equal(report.summary.blocking, 1) +}) + test('doctorServer explains same-name shadowing across scopes', async () => { const localConfig = stdioConfig('local', 'node-local') const userConfig = stdioConfig('user', 'node-user') diff --git a/src/services/mcp/doctor.ts b/src/services/mcp/doctor.ts index 79f3cf4ee..55da30c93 100644 --- a/src/services/mcp/doctor.ts +++ b/src/services/mcp/doctor.ts @@ -240,7 +240,10 @@ function buildScopeDefinitions( activeConfig: ScopedMcpServerConfig | undefined, deps: McpDoctorDependencies, ): McpDoctorDefinition[] { - const config = servers[name] + // Own-property lookup: these maps are plain objects from JSON config, so a + // bare servers[name] resolves inherited Object.prototype members and would + // fabricate a definition for a name like 'constructor'. + const config = Object.hasOwn(servers, name) ? servers[name] : undefined if (!config) { return [] } @@ -540,7 +543,9 @@ async function buildServerReport( } const { servers: activeServers } = await deps.getAllMcpConfigs() const serverDisabled = deps.isMcpServerDisabled(name) - const runtimeConfig = activeServers[name] ?? undefined + const runtimeConfig = Object.hasOwn(activeServers, name) + ? activeServers[name] + : undefined const activeConfig = serverDisabled ? undefined : runtimeConfig const definitions = [ @@ -579,13 +584,23 @@ async function buildServerReport( ? activeConfig : undefined + const nameValidationFindings = validationFindingsByName.get(name) ?? [] const findings: McpDoctorFinding[] = [ - ...(validationFindingsByName.get(name) ?? []), + ...nameValidationFindings, ...buildShadowingFindings(definitions), ...buildStateFindings(definitions), ] - if (definitions.length === 0 && !shouldAddObservedDefinition) { + // A reserved-name target (`__proto__`/`constructor`) never survives parsing, + // so it has no definition -- but its fatal validation finding already names + // the problem. Adding `state.not_found` on top would report two blocking + // findings with contradictory messages, so skip it when validation already + // explains this name. + if ( + definitions.length === 0 && + !shouldAddObservedDefinition && + nameValidationFindings.length === 0 + ) { findings.push({ blocking: true, code: 'state.not_found', @@ -666,8 +681,19 @@ export async function doctorAllServers( ), ) + // Validation findings are keyed by server name, but a fatal reserved-name + // error ("__proto__"/"constructor") is keyed by a name that never survives + // parsing, so it is absent from `names` and its finding would otherwise be + // built into no server report and silently dropped -- `mcp doctor + // --config-only` would read clean while the invalid config is present. + // Surface any finding whose server has no report as a global finding. + const reportedNames = new Set(names) + const orphanedFindings = Array.from(serverFindingsByName.entries()) + .filter(([name]) => !reportedNames.has(name)) + .flatMap(([, findings]) => findings) + report.servers = servers - report.findings = globalFindings + report.findings = [...globalFindings, ...orphanedFindings] return summarizeReport(report) } @@ -698,6 +724,14 @@ export async function doctorServer( deps, ) report.servers = [server] - report.findings = globalFindings + // Mirror the orphan fold in doctorAllServers: a fatal reserved-name finding is + // keyed by a name that never survives parsing, so unless it happens to be the + // requested target it is built into no report. Promote any finding for a name + // other than the one reported so a poisoned sibling scope is not hidden -- + // e.g. `mcp doctor realserver` while `.mcp.json` still carries `__proto__`. + const orphanedFindings = Array.from(serverFindingsByName.entries()) + .filter(([findingName]) => findingName !== name) + .flatMap(([, findings]) => findings) + report.findings = [...globalFindings, ...orphanedFindings] return summarizeReport(report) } diff --git a/src/services/mcp/enterpriseMcpBoundary.test.ts b/src/services/mcp/enterpriseMcpBoundary.test.ts new file mode 100644 index 000000000..f5f1ecc3f --- /dev/null +++ b/src/services/mcp/enterpriseMcpBoundary.test.ts @@ -0,0 +1,138 @@ +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { afterEach, beforeEach, expect, test } from 'bun:test' + +import { + getAllowedSettingSources, + setAllowedSettingSources, +} from '../../bootstrap/state.js' +import { + acquireSharedMutationLock, + releaseSharedMutationLock, +} from '../../test/sharedMutationLock.js' +import { + getCurrentProjectConfig, + getGlobalConfig, + saveCurrentProjectConfig, + saveGlobalConfig, +} from '../../utils/config.js' +import { + SETTING_SOURCES, + type SettingSource, +} from '../../utils/settings/constants.js' +import { getManagedFilePath } from '../../utils/settings/managedPath.js' +import { doesEnterpriseMcpConfigExist, getMcpConfigByName } from './config.js' + +// getManagedFilePath / doesEnterpriseMcpConfigExist are lodash-memoized; clear +// their caches so the env override below is observed and does not leak. +function clearManagedPathCaches(): void { + ;(getManagedFilePath as unknown as { cache: { clear(): void } }).cache.clear() + ;( + doesEnterpriseMcpConfigExist as unknown as { cache: { clear(): void } } + ).cache.clear() +} + +let dir: string +let savedUserType: string | undefined +let savedManagedPath: string | undefined +let savedNodeEnv: string | undefined +let savedSettingSources: SettingSource[] +let savedGlobalMcp: ReturnType['mcpServers'] +let savedProjectMcp: ReturnType['mcpServers'] + +beforeEach(async () => { + await acquireSharedMutationLock('services/mcp/enterpriseMcpBoundary.test.ts') + savedUserType = process.env.USER_TYPE + savedManagedPath = process.env.CLAUDE_CODE_MANAGED_SETTINGS_PATH + savedNodeEnv = process.env.NODE_ENV + dir = mkdtempSync(join(tmpdir(), 'managed-mcp-boundary-')) + process.env.USER_TYPE = 'ant' + process.env.CLAUDE_CODE_MANAGED_SETTINGS_PATH = dir + process.env.NODE_ENV = 'test' + // A disabled scope's servers would be dropped for an unrelated reason; pin the + // full source set so the assertions isolate the enterprise-exclusive boundary. + savedSettingSources = getAllowedSettingSources() + setAllowedSettingSources([...SETTING_SOURCES]) + // A user- and a local-scoped server that would resolve by name in normal mode. + savedGlobalMcp = getGlobalConfig().mcpServers + savedProjectMcp = getCurrentProjectConfig().mcpServers + saveGlobalConfig(config => ({ + ...config, + mcpServers: { usersrv: { command: 'echo', args: [] } }, + })) + saveCurrentProjectConfig(config => ({ + ...config, + mcpServers: { localsrv: { command: 'echo', args: [] } }, + })) + clearManagedPathCaches() +}) + +afterEach(() => { + try { + saveGlobalConfig(config => ({ ...config, mcpServers: savedGlobalMcp })) + saveCurrentProjectConfig(config => ({ + ...config, + mcpServers: savedProjectMcp, + })) + setAllowedSettingSources(savedSettingSources) + if (savedNodeEnv === undefined) delete process.env.NODE_ENV + else process.env.NODE_ENV = savedNodeEnv + if (savedUserType === undefined) delete process.env.USER_TYPE + else process.env.USER_TYPE = savedUserType + if (savedManagedPath === undefined) + delete process.env.CLAUDE_CODE_MANAGED_SETTINGS_PATH + else process.env.CLAUDE_CODE_MANAGED_SETTINGS_PATH = savedManagedPath + rmSync(dir, { recursive: true, force: true }) + clearManagedPathCaches() + } finally { + releaseSharedMutationLock() + } +}) + +// getMcpConfigByName is the shared chokepoint for both the CLI named lookup +// (`mcp get ` → checkMcpServerHealth) and an agent definition's +// named-server reference (via runAgent). A present managed-mcp.json engages +// enterprise-exclusive, fail-closed mode, so neither route may resolve a +// user/local server while it exists. + +test('a valid managed file blocks user/local names and resolves only enterprise ones', () => { + writeFileSync( + join(dir, 'managed-mcp.json'), + '{"mcpServers":{"entsrv":{"command":"echo","args":[]}}}', + ) + clearManagedPathCaches() + + expect(doesEnterpriseMcpConfigExist()).toBe(true) + // The non-enterprise servers are unreachable by name under the policy. + expect(getMcpConfigByName('usersrv')).toBeNull() + expect(getMcpConfigByName('localsrv')).toBeNull() + // The enterprise-owned server still resolves. + const ent = getMcpConfigByName('entsrv') + expect(ent).not.toBeNull() + expect(ent?.scope).toBe('enterprise') +}) + +test('a malformed managed file still fails closed (no fall-through to user/local)', () => { + // The reserved __proto__ entry makes the whole managed file fatal, so the + // enterprise scope parses to zero servers — but its presence still engages the + // policy, so a named lookup must not fall back to the user/local scopes. + writeFileSync( + join(dir, 'managed-mcp.json'), + '{"mcpServers":{"__proto__":{"command":"echo","args":[]},' + + '"entsrv":{"command":"echo","args":[]}}}', + ) + clearManagedPathCaches() + + expect(doesEnterpriseMcpConfigExist()).toBe(true) + expect(getMcpConfigByName('usersrv')).toBeNull() + expect(getMcpConfigByName('localsrv')).toBeNull() +}) + +test('without a managed file the user/local names resolve as normal', () => { + // Control: the boundary only applies while the managed file is present. + expect(doesEnterpriseMcpConfigExist()).toBe(false) + expect(getMcpConfigByName('usersrv')?.scope).toBe('user') + expect(getMcpConfigByName('localsrv')?.scope).toBe('local') +}) diff --git a/src/services/mcp/enterpriseMcpErrors.test.ts b/src/services/mcp/enterpriseMcpErrors.test.ts new file mode 100644 index 000000000..0db4d2450 --- /dev/null +++ b/src/services/mcp/enterpriseMcpErrors.test.ts @@ -0,0 +1,96 @@ +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +import { afterEach, beforeEach, expect, test } from 'bun:test' + +import { + acquireSharedMutationLock, + releaseSharedMutationLock, +} from '../../test/sharedMutationLock.js' +import { getManagedFilePath } from '../../utils/settings/managedPath.js' +import { doesEnterpriseMcpConfigExist, getClaudeCodeMcpConfigs } from './config.js' + +// getManagedFilePath / doesEnterpriseMcpConfigExist are memoized; clear their +// lodash caches so the env override below is observed and does not leak. +function clearManagedPathCaches(): void { + ;(getManagedFilePath as unknown as { cache: { clear(): void } }).cache.clear() + ;( + doesEnterpriseMcpConfigExist as unknown as { cache: { clear(): void } } + ).cache.clear() +} + +let dir: string +let savedUserType: string | undefined +let savedManagedPath: string | undefined + +beforeEach(async () => { + await acquireSharedMutationLock('services/mcp/enterpriseMcpErrors.test.ts') + savedUserType = process.env.USER_TYPE + savedManagedPath = process.env.CLAUDE_CODE_MANAGED_SETTINGS_PATH + dir = mkdtempSync(join(tmpdir(), 'managed-mcp-')) + process.env.USER_TYPE = 'ant' + process.env.CLAUDE_CODE_MANAGED_SETTINGS_PATH = dir + clearManagedPathCaches() +}) + +afterEach(() => { + try { + rmSync(dir, { recursive: true, force: true }) + if (savedUserType === undefined) { + delete process.env.USER_TYPE + } else { + process.env.USER_TYPE = savedUserType + } + if (savedManagedPath === undefined) { + delete process.env.CLAUDE_CODE_MANAGED_SETTINGS_PATH + } else { + process.env.CLAUDE_CODE_MANAGED_SETTINGS_PATH = savedManagedPath + } + clearManagedPathCaches() + } finally { + releaseSharedMutationLock() + } +}) + +test('surfaces managed-mcp.json parse errors in enterprise exclusive mode', async () => { + // A reserved __proto__ entry makes the whole managed file fatal, so the + // enterprise scope parses to zero servers. Presence still engages the policy + // lock (fail-closed), so without propagating the parse error the caller would + // see an empty server list and no reason why. + writeFileSync( + join(dir, 'managed-mcp.json'), + '{"mcpServers":{"__proto__":{"command":"echo","args":[]},' + + '"real":{"command":"echo","args":[]}}}', + ) + + const { servers, errors } = await getClaudeCodeMcpConfigs() + + expect( + errors.some( + e => + e.type === 'generic-error' && + e.error.includes('Managed MCP config is invalid'), + ), + ).toBe(true) + // Fail-closed: the valid sibling is not loaded either while the file is fatal. + expect(Object.keys(servers)).not.toContain('real') +}) + +test('reports no managed error for a clean managed-mcp.json', async () => { + writeFileSync( + join(dir, 'managed-mcp.json'), + '{"mcpServers":{"real":{"command":"echo","args":[]}}}', + ) + + const { servers, errors } = await getClaudeCodeMcpConfigs() + + expect( + errors.some( + e => + e.type === 'generic-error' && + e.error.includes('Managed MCP config is invalid'), + ), + ).toBe(false) + expect(Object.keys(servers)).toContain('real') +}) diff --git a/src/services/mcp/headlessErrors.test.ts b/src/services/mcp/headlessErrors.test.ts new file mode 100644 index 000000000..0c570cb2c --- /dev/null +++ b/src/services/mcp/headlessErrors.test.ts @@ -0,0 +1,25 @@ +import { expect, test } from 'bun:test' + +import type { PluginError } from '../../types/plugin.js' +import { getHeadlessMcpConfigWarnings } from './headlessErrors.js' + +const managedError: PluginError = { + type: 'generic-error', + source: '/managed/managed-mcp.json', + error: 'Managed MCP config is invalid (mcpServers.__proto__): reserved name', +} + +test('emits a warning line per error in a headless session', () => { + const lines = getHeadlessMcpConfigWarnings(true, [managedError]) + expect(lines).toEqual([ + 'Warning: Managed MCP config is invalid (mcpServers.__proto__): reserved name', + ]) +}) + +test('stays silent in an interactive session (MCP UI surfaces errors there)', () => { + expect(getHeadlessMcpConfigWarnings(false, [managedError])).toEqual([]) +}) + +test('emits nothing when there are no errors', () => { + expect(getHeadlessMcpConfigWarnings(true, [])).toEqual([]) +}) diff --git a/src/services/mcp/headlessErrors.ts b/src/services/mcp/headlessErrors.ts new file mode 100644 index 000000000..f3ba337ce --- /dev/null +++ b/src/services/mcp/headlessErrors.ts @@ -0,0 +1,21 @@ +import { getPluginErrorMessage, type PluginError } from '../../types/plugin.js' + +/** + * Build the stderr warning lines a headless (`-p`) session should emit for MCP + * config errors. + * + * Interactive sessions surface these through the MCP error UI, but headless has + * no such surface: a fatal managed-mcp.json fail-closes every file-based source + * and, without this, the caller sees an empty server list with no reason why — + * indistinguishable from an intentionally empty config. Returns [] when not + * headless or when there is nothing to report, so the caller writes nothing. + */ +export function getHeadlessMcpConfigWarnings( + isNonInteractiveSession: boolean, + errors: PluginError[], +): string[] { + if (!isNonInteractiveSession || errors.length === 0) { + return [] + } + return errors.map(error => `Warning: ${getPluginErrorMessage(error)}`) +} diff --git a/src/utils/settings/allErrors.ts b/src/utils/settings/allErrors.ts index 1162ef1c7..430f2150c 100644 --- a/src/utils/settings/allErrors.ts +++ b/src/utils/settings/allErrors.ts @@ -22,8 +22,10 @@ import type { SettingsWithErrors } from './validation.js' */ export function getSettingsWithAllErrors(): SettingsWithErrors { const result = getSettingsWithErrors() - // 'dynamic' scope does not have errors returned; it throws and is set on cli startup - const scopes = ['user', 'project', 'local'] as const + // 'dynamic' scope does not have errors returned; it throws and is set on cli + // startup. 'enterprise' is included so a malformed managed-mcp.json — which + // fail-closes MCP entirely — surfaces here instead of silently blanking it. + const scopes = ['user', 'project', 'local', 'enterprise'] as const const mcpErrors = scopes.flatMap(scope => getMcpConfigsByScope(scope).errors) return { settings: result.settings,