fix(mcp): resolve server names by own-property, not the prototype chain (#1983)

* 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 <server>' (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.
This commit is contained in:
0xfandom
2026-09-21 09:04:37 +08:00
committed by GitHub
parent d16318a47f
commit c3930ca5ad
11 changed files with 1044 additions and 64 deletions
+16 -13
View File
@@ -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<Exclude<ConfigScope, 'dynamic'>> = [];
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) {
+13 -2
View File
@@ -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<CommanderCommand> {
// 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<string, ScopedMcpServerConfig>
servers: {} as Record<string, ScopedMcpServerConfig>,
errors: [] as PluginError[]
}) : getClaudeCodeMcpConfigs(dynamicMcpConfig)).then(result => {
mcpConfigResolvedMs = Date.now() - mcpConfigStart;
return result;
@@ -2293,8 +2296,16 @@ async function run(): Promise<CommanderCommand> {
}
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,
+377
View File
@@ -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<typeof getGlobalConfig>['mcpServers']
let savedProjectMcp: ReturnType<typeof getCurrentProjectConfig>['mcpServers']
let savedNodeEnv: string | undefined
let savedSettingSources: ReturnType<typeof getAllowedSettingSources>
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')
})
+219 -41
View File
@@ -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<string, McpServerConfig> | 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<string, McpServerConfig> = {}
for (const [serverName, serverConfig] of Object.entries(
@@ -772,9 +820,13 @@ export async function removeMcpConfig(
): Promise<void> {
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<string, ScopedMcpServerConfig>
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 <server>, 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<string, ScopedMcpServerConfig>
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<string, McpServerConfig> = {}
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())
})
/**
+95
View File
@@ -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')
+40 -6
View File
@@ -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)
}
@@ -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<typeof getGlobalConfig>['mcpServers']
let savedProjectMcp: ReturnType<typeof getCurrentProjectConfig>['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 <server>` → 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')
})
@@ -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')
})
+25
View File
@@ -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([])
})
+21
View File
@@ -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)}`)
}
+4 -2
View File
@@ -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,