Skip to content

Commit e0a5627

Browse files
committed
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.
1 parent 7ad96e9 commit e0a5627

3 files changed

Lines changed: 85 additions & 13 deletions

File tree

src/cli/handlers/mcp.tsx

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -201,17 +201,22 @@ export async function mcpRemoveHandler(name: string, options: {
201201
const projectConfig = getCurrentProjectConfig();
202202
const globalConfig = getGlobalConfig();
203203

204-
// Check if server exists in project scope (.mcp.json)
204+
// Check if server exists in project scope (.mcp.json). These maps are plain
205+
// objects from JSON config, so a `!!servers[name]` / `?.[name]` check treats
206+
// inherited Object.prototype members ('constructor', '__proto__', …) as
207+
// present. `mcp remove constructor` would then report the reserved name as
208+
// living in multiple scopes instead of "No MCP server found". Gate on
209+
// own-property.
205210
const {
206211
servers: projectServers
207212
} = getMcpConfigsByScope('project');
208-
const mcpJsonExists = !!projectServers[name];
213+
const mcpJsonExists = Object.hasOwn(projectServers, name);
209214

210215
// Count how many scopes contain this server
211216
const scopes: Array<Exclude<ConfigScope, 'dynamic'>> = [];
212-
if (projectConfig.mcpServers?.[name]) scopes.push('local');
217+
if (Object.hasOwn(projectConfig.mcpServers ?? {}, name)) scopes.push('local');
213218
if (mcpJsonExists) scopes.push('project');
214-
if (globalConfig.mcpServers?.[name]) scopes.push('user');
219+
if (Object.hasOwn(globalConfig.mcpServers ?? {}, name)) scopes.push('user');
215220
if (scopes.length === 0) {
216221
cliError(`No MCP server found with name: "${name}"`);
217222
} else if (scopes.length === 1) {
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
import { afterEach, beforeEach, expect, test } from 'bun:test'
2+
3+
import {
4+
getGlobalConfig,
5+
saveCurrentProjectConfig,
6+
saveGlobalConfig,
7+
} from '../../utils/config.js'
8+
import { getMcpConfigByName } from './config.js'
9+
10+
// The MCP `servers` maps are plain objects built from JSON config, so a bare
11+
// `servers[name]` lookup exposes inherited Object.prototype members. A user
12+
// running `openclaude mcp get constructor` (or `__proto__`, `toString`, …) must
13+
// get "not found", not the Object constructor cast as a server config.
14+
const PROTO_NAMES = [
15+
'constructor',
16+
'__proto__',
17+
'toString',
18+
'hasOwnProperty',
19+
'valueOf',
20+
'isPrototypeOf',
21+
]
22+
23+
let savedGlobalMcp: ReturnType<typeof getGlobalConfig>['mcpServers']
24+
25+
beforeEach(() => {
26+
process.env.NODE_ENV = 'test'
27+
savedGlobalMcp = getGlobalConfig().mcpServers
28+
saveGlobalConfig(config => ({
29+
...config,
30+
mcpServers: { realserver: { command: 'echo', args: [] } },
31+
}))
32+
saveCurrentProjectConfig(config => ({
33+
...config,
34+
mcpServers: { locallyreal: { command: 'echo', args: [] } },
35+
}))
36+
})
37+
38+
afterEach(() => {
39+
saveGlobalConfig(config => ({ ...config, mcpServers: savedGlobalMcp }))
40+
saveCurrentProjectConfig(config => ({ ...config, mcpServers: undefined }))
41+
})
42+
43+
test('resolves a real server by name', () => {
44+
const found = getMcpConfigByName('realserver')
45+
expect(found).not.toBeNull()
46+
expect(found?.scope).toBe('user')
47+
})
48+
49+
test('returns null for a plainly missing name', () => {
50+
expect(getMcpConfigByName('nope-not-here')).toBeNull()
51+
})
52+
53+
test('returns null for Object.prototype member names', () => {
54+
// Before the fix each of these resolved to an inherited function (truthy),
55+
// so the lookup returned a bogus config and callers skipped their not-found
56+
// guard.
57+
for (const name of PROTO_NAMES) {
58+
expect(getMcpConfigByName(name)).toBeNull()
59+
}
60+
})

src/services/mcp/config.ts

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,29 +1031,36 @@ export function getMcpConfigsByScope(
10311031
* @returns The server configuration with scope, or undefined if not found
10321032
*/
10331033
export function getMcpConfigByName(name: string): ScopedMcpServerConfig | null {
1034+
// The `servers` maps are plain object literals populated from JSON config, so
1035+
// a bare `servers[name]` lookup resolves inherited Object.prototype members
1036+
// (`constructor`, `toString`, `__proto__`, …) as truthy values. `mcp get
1037+
// constructor` would then skip the not-found guard and pass the Object
1038+
// constructor on as a server config. Gate every lookup on own-property.
10341039
const { servers: enterpriseServers } = getMcpConfigsByScope('enterprise')
10351040

10361041
// When MCP is locked to plugin-only, only enterprise servers are reachable
10371042
// by name. User/project/local servers are blocked — same as getClaudeCodeMcpConfigs().
10381043
if (isRestrictedToPluginOnly('mcp')) {
1039-
return enterpriseServers[name] ?? null
1044+
return Object.hasOwn(enterpriseServers, name)
1045+
? enterpriseServers[name]!
1046+
: null
10401047
}
10411048

10421049
const { servers: userServers } = getMcpConfigsByScope('user')
10431050
const { servers: projectServers } = getMcpConfigsByScope('project')
10441051
const { servers: localServers } = getMcpConfigsByScope('local')
10451052

1046-
if (enterpriseServers[name]) {
1047-
return enterpriseServers[name]
1053+
if (Object.hasOwn(enterpriseServers, name)) {
1054+
return enterpriseServers[name]!
10481055
}
1049-
if (localServers[name]) {
1050-
return localServers[name]
1056+
if (Object.hasOwn(localServers, name)) {
1057+
return localServers[name]!
10511058
}
1052-
if (projectServers[name]) {
1053-
return projectServers[name]
1059+
if (Object.hasOwn(projectServers, name)) {
1060+
return projectServers[name]!
10541061
}
1055-
if (userServers[name]) {
1056-
return userServers[name]
1062+
if (Object.hasOwn(userServers, name)) {
1063+
return userServers[name]!
10571064
}
10581065

10591066
return null

0 commit comments

Comments
 (0)