diff --git a/src/cli/handlers/mcp.tsx b/src/cli/handlers/mcp.tsx index 8c34b24148..9a2dc89751 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 a7832be9b6..86267f5463 100644 --- a/src/main.tsx +++ b/src/main.tsx @@ -147,6 +147,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'; @@ -1778,7 +1780,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; @@ -2334,8 +2337,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 0000000000..72e18804fb --- /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 0f27e47f30..50eb36477a 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 - const schemaResult = McpJsonConfigSchema().safeParse(configObject) - if (!schemaResult.success) { - return { - config: null, - errors: schemaResult.error.issues.map(issue => ({ + + // 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: issue.path.join('.'), - message: 'Does not adhere to MCP server configuration schema', + 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: [ + ...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 a998bc2cbc..0cdb7320d4 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 79f3cf4eea..55da30c934 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 0000000000..f5f1ecc3fa --- /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 0000000000..0db4d2450f --- /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 0000000000..0c570cb2c2 --- /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 0000000000..f3ba337ceb --- /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 1162ef1c78..430f2150c1 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,