From af032fc81a9086442d8d973f974c3acce89b30c2 Mon Sep 17 00:00:00 2001 From: Devvyn <22340871+opticon454@users.noreply.github.com> Date: Fri, 2 Oct 2026 18:24:23 +0800 Subject: [PATCH] fix(mcp): block __proto__ server names, fix lint; add route and registry tests Co-Authored-By: Claude Sonnet 5.5 --- src/mcp-sync.ts | 8 +- test/mcp-sync-registry.test.ts | 44 +++++++++++ test/mcp-sync.test.ts | 22 ++++++ test/routes/mcp-sync-routes.test.ts | 116 ++++++++++++++++++++++++++++ 4 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 test/mcp-sync-registry.test.ts create mode 100644 test/routes/mcp-sync-routes.test.ts diff --git a/src/mcp-sync.ts b/src/mcp-sync.ts index 523fd1ea..b0b9dadb 100644 --- a/src/mcp-sync.ts +++ b/src/mcp-sync.ts @@ -74,6 +74,9 @@ export interface McpSyncResult { const isRecord = (v: unknown): v is Record => typeof v === 'object' && v !== null && !Array.isArray(v); +/** Names that would reach Object.prototype through a plain-object table (`out[name] = ...`). */ +const UNSAFE_NAMES = new Set(['__proto__', 'constructor', 'prototype']); + function strMap(v: unknown): Record | undefined { if (!isRecord(v)) return undefined; const out: Record = {}; @@ -323,7 +326,7 @@ function parseTomlKey(src: string, start: number): [string, number] { /** Split a table header like `mcp_servers."my.srv".env` into dotted key parts. */ function parseTomlHeader(line: string): string[] | null { - const m = /^\[([^\]\[].*)\]\s*(#.*)?$/.exec(line.trim()); + const m = /^\[([^[\]].*)\]\s*(#.*)?$/.exec(line.trim()); if (!m) return null; const body = m[1]; const parts: string[] = []; @@ -357,6 +360,7 @@ function parseCodexTables(text: string): Record structuredClone(STOCK_CLIS.find((e) => (e.id as string) === 'claude')!) as CliEntry; + +function withMcp(mcpConfig: unknown) { + const e = claude(); + (e.capabilities as Record).mcpConfig = mcpConfig; + return CliEntrySchema.safeParse(e); +} + +describe('capabilities.mcpConfig', () => { + it('is declared by exactly the CLIs whose format is verified', () => { + const declared = STOCK_CLIS.filter((e) => e.capabilities.mcpConfig).map((e) => e.id as string); + expect(declared.sort()).toEqual(['antigravity', 'claude', 'codex', 'gemini', 'opencode']); + }); + + it('every stock declaration passes the schema, with a distinct file per CLI', () => { + for (const e of STOCK_CLIS) expect(CliEntrySchema.safeParse(e).success, e.id as string).toBe(true); + const paths = STOCK_CLIS.flatMap((e) => (e.capabilities.mcpConfig ? [e.capabilities.mcpConfig.path] : [])); + expect(new Set(paths).size).toBe(paths.length); + }); + + it('accepts a home-relative path with a known format', () => { + expect(withMcp({ path: '.tool/mcp.json', format: 'claude-json' }).success).toBe(true); + }); + + it.each([ + ['parent traversal', { path: '../evil.json', format: 'claude-json' }], + ['nested traversal', { path: '.a/../../evil.json', format: 'claude-json' }], + ['absolute path', { path: '/etc/cron.d/x', format: 'claude-json' }], + ['shell metacharacters', { path: '.a;rm -rf', format: 'claude-json' }], + ['unknown format', { path: '.a/mcp.json', format: 'yaml' }], + ['extra key', { path: '.a/mcp.json', format: 'claude-json', mode: 'rw' }], + ])('rejects %s', (_label, value) => { + expect(withMcp(value).success).toBe(false); + }); +}); diff --git a/test/mcp-sync.test.ts b/test/mcp-sync.test.ts index dc9d494c..da1059cd 100644 --- a/test/mcp-sync.test.ts +++ b/test/mcp-sync.test.ts @@ -134,6 +134,28 @@ describe('real CLI output (captured from `agy`/`gemini`/`codex mcp add`)', () => }); }); +describe('hostile config files', () => { + it('never lets a server name reach Object.prototype (toml and json)', () => { + const toml = parseServers( + 'codex-toml', + '[mcp_servers.__proto__]\ncommand = "x"\npolluted = "yes"\n[mcp_servers.ok]\ncommand = "y"\n' + ); + expect(Object.keys(toml)).toEqual(['ok']); + const json = parseServers( + 'claude-json', + '{"mcpServers":{"__proto__":{"command":"x"},"constructor":{"command":"x"},"ok":{"command":"y"}}}' + ); + expect(Object.keys(json)).toEqual(['ok']); + expect(({} as Record).polluted).toBeUndefined(); + expect(({} as Record).command).toBeUndefined(); + }); + + it('rejects a non-object server table instead of overwriting it', () => { + expect(() => parseServers('claude-json', '{"mcpServers":[]}')).toThrow(); + expect(() => parseServers('claude-json', '[]')).toThrow(); + }); +}); + describe('addServers', () => { it('preserves other keys and existing servers, appends codex tables without touching the rest', () => { const out = JSON.parse( diff --git a/test/routes/mcp-sync-routes.test.ts b/test/routes/mcp-sync-routes.test.ts new file mode 100644 index 00000000..9845d00f --- /dev/null +++ b/test/routes/mcp-sync-routes.test.ts @@ -0,0 +1,116 @@ +/** + * @fileoverview Route tests for /api/mcp-sync. Only CLIs that are ENABLED in the registry take + * part; enabled agent CLIs with no known MCP config are reported as unsupported. + * + * ⚠️ test/setup.ts gives the whole FILE one temp HOME, so each test wipes the config files it + * creates. Port: N/A (app.inject()). + */ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { homedir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { createRouteTestHarness } from './_route-test-utils.js'; +import { registerMcpSyncRoutes } from '../../src/web/routes/mcp-sync-routes.js'; +import { registryFilePath, reloadCliRegistry } from '../../src/config/cli-registry/registry.js'; + +const home = () => homedir(); +const write = (rel: string, text: string) => { + const f = join(home(), rel); + mkdirSync(dirname(f), { recursive: true }); + writeFileSync(f, text); +}; +const disable = (...ids: string[]) => { + const file = registryFilePath(); + mkdirSync(dirname(file), { recursive: true }); + const clis = Object.fromEntries(ids.map((id) => [id, { enabled: false }])); + writeFileSync(file, JSON.stringify({ schemaVersion: 1, clis }), { mode: 0o600 }); + reloadCliRegistry(); +}; + +const CLAUDE = '.claude.json'; +const CODEX = '.codex/config.toml'; +const GEMINI = '.gemini/settings.json'; + +beforeEach(() => { + rmSync(registryFilePath(), { force: true }); + reloadCliRegistry(); + for (const d of ['.claude.json', '.codex', '.gemini', '.config']) + rmSync(join(home(), d), { recursive: true, force: true }); + write(CLAUDE, JSON.stringify({ mcpServers: { fs: { type: 'stdio', command: 'npx', args: ['-y', 'fs'] } } })); +}); +afterEach(() => { + delete process.env.CODEMAN_MULTIUSER; + rmSync(registryFilePath(), { force: true }); + reloadCliRegistry(); +}); + +describe('/api/mcp-sync', () => { + it('GET previews without writing', async () => { + const { app } = await createRouteTestHarness(registerMcpSyncRoutes); + const res = await app.inject({ method: 'GET', url: '/api/mcp-sync' }); + expect(res.statusCode).toBe(200); + const body = res.json(); + expect(body.success).toBe(true); + expect(body.data.applied).toBe(false); + expect(body.data.targets.find((t: { id: string }) => t.id === 'codex').added).toEqual(['fs']); + expect(existsSync(join(home(), CODEX))).toBe(false); + }); + + it('POST adds the server to every enabled CLI', async () => { + const { app } = await createRouteTestHarness(registerMcpSyncRoutes); + const res = await app.inject({ method: 'POST', url: '/api/mcp-sync' }); + expect(res.json().data.applied).toBe(true); + expect(readFileSync(join(home(), CODEX), 'utf8')).toContain('[mcp_servers.fs]'); + expect(JSON.parse(readFileSync(join(home(), GEMINI), 'utf8')).mcpServers.fs.command).toBe('npx'); + }); + + it('never touches a CLI that is disabled in the registry', async () => { + disable('codex'); + const { app } = await createRouteTestHarness(registerMcpSyncRoutes); + const res = await app.inject({ method: 'POST', url: '/api/mcp-sync' }); + const ids = res.json().data.targets.map((t: { id: string }) => t.id); + expect(ids).not.toContain('codex'); + expect(ids).toContain('gemini'); + expect(existsSync(join(home(), '.codex'))).toBe(false); + expect(existsSync(join(home(), GEMINI))).toBe(true); + }); + + it('lists enabled agent CLIs without MCP support, and omits disabled ones and the shell', async () => { + disable('pi'); + const { app } = await createRouteTestHarness(registerMcpSyncRoutes); + const { unsupported } = (await app.inject({ method: 'GET', url: '/api/mcp-sync' })).json().data; + expect(unsupported).toContain('Grok'); + expect(unsupported).not.toContain('Pi'); + expect(unsupported.some((l: string) => /shell|terminal/i.test(l))).toBe(false); + }); + + it('never returns env values or headers', async () => { + write( + CLAUDE, + JSON.stringify({ mcpServers: { fs: { type: 'stdio', command: 'npx', env: { TOKEN: 'sekrit-value' } } } }) + ); + const { app } = await createRouteTestHarness(registerMcpSyncRoutes); + const res = await app.inject({ method: 'POST', url: '/api/mcp-sync' }); + expect(res.body).not.toContain('sekrit-value'); + }); + + it('multi-user: a non-admin is refused on both verbs and nothing is written', async () => { + process.env.CODEMAN_MULTIUSER = '1'; + const { app } = await createRouteTestHarness(registerMcpSyncRoutes, { + authUser: { username: 'bob', role: 'user' }, + }); + for (const method of ['GET', 'POST'] as const) { + const res = await app.inject({ method, url: '/api/mcp-sync' }); + expect(res.json().success, method).toBe(false); + } + expect(existsSync(join(home(), CODEX))).toBe(false); + }); + + it('multi-user: an admin is allowed', async () => { + process.env.CODEMAN_MULTIUSER = '1'; + const { app } = await createRouteTestHarness(registerMcpSyncRoutes, { + authUser: { username: 'root', role: 'admin' }, + }); + expect((await app.inject({ method: 'GET', url: '/api/mcp-sync' })).json().success).toBe(true); + }); +});