diff --git a/src/__tests__/commands/install.test.ts b/src/__tests__/commands/install.test.ts index fdfb4c4..7721bdf 100644 --- a/src/__tests__/commands/install.test.ts +++ b/src/__tests__/commands/install.test.ts @@ -214,7 +214,8 @@ describe("handleInstall — happy path (GREEN trust score)", () => { expect(adapter.addServer).toHaveBeenCalledWith( "/fake/claude-desktop/config.json", "io.github.test/my-server", - expect.objectContaining({ command: "npx" }) + expect.objectContaining({ command: "npx" }), + expect.objectContaining({}) ); }); @@ -506,7 +507,8 @@ describe("handleInstall — env var prompting", () => { expect(adapter.addServer).toHaveBeenCalledWith( expect.any(String), expect.any(String), - expect.objectContaining({ env: { API_KEY: "secret-value" } }) + expect.objectContaining({ env: { API_KEY: "secret-value" } }), + expect.objectContaining({}) ); }); @@ -1343,7 +1345,8 @@ describe("handleInstall — no env vars on server", () => { expect(adapter.addServer).toHaveBeenCalledWith( expect.any(String), expect.any(String), - expect.not.objectContaining({ env: expect.anything() }) + expect.not.objectContaining({ env: expect.anything() }), + expect.objectContaining({}) ); }); }); diff --git a/src/commands/install.ts b/src/commands/install.ts index 8a373d3..5ee0243 100644 --- a/src/commands/install.ts +++ b/src/commands/install.ts @@ -453,7 +453,7 @@ export async function handleInstall( : {}), }; - await adapter.addServer(configPath, name, entry); + await adapter.addServer(configPath, name, entry, { force: options.force }); installedClients.push(clientId); } diff --git a/src/config/adapters/base.ts b/src/config/adapters/base.ts index 857b8bf..c0b9443 100644 --- a/src/config/adapters/base.ts +++ b/src/config/adapters/base.ts @@ -86,7 +86,8 @@ export abstract class BaseAdapter implements ConfigAdapter { async addServer( configPath: string, name: string, - entry: McpServerEntry + entry: McpServerEntry, + options?: { force?: boolean } ): Promise { // readRaw returns {} for ENOENT and re-throws all other errors, // so we can call it directly here. @@ -94,7 +95,7 @@ export abstract class BaseAdapter implements ConfigAdapter { const existing = (raw[this.rootKey] ?? {}) as Record; - if (Object.prototype.hasOwnProperty.call(existing, name)) { + if (Object.prototype.hasOwnProperty.call(existing, name) && !options?.force) { throw new Error( `Server "${name}" already exists in ${this.clientId} config. Use --force to overwrite.` ); diff --git a/src/config/adapters/index.ts b/src/config/adapters/index.ts index 32a237f..360bc08 100644 --- a/src/config/adapters/index.ts +++ b/src/config/adapters/index.ts @@ -36,13 +36,15 @@ export interface ConfigAdapter { read(configPath: string): Promise>; /** - * Add a new server entry. Throws if the server name already exists. + * Add a new server entry. Throws if the server name already exists + * (unless options.force is true, in which case it overwrites). * Creates the file (with correct structure) if it does not exist. */ addServer( configPath: string, name: string, - entry: McpServerEntry + entry: McpServerEntry, + options?: { force?: boolean } ): Promise; /** Remove a server entry. Throws if the server name is not found. */ diff --git a/src/scanner/health-check.ts b/src/scanner/health-check.ts index d586894..c1192f7 100644 --- a/src/scanner/health-check.ts +++ b/src/scanner/health-check.ts @@ -12,6 +12,88 @@ import { spawn } from "node:child_process"; import type { McpServerEntry } from "../config/adapters/index.js"; +// --------------------------------------------------------------------------- +// Environment sanitization for health checks +// --------------------------------------------------------------------------- + +/** + * Environment variables that should NOT be passed to untrusted MCP servers + * during health checks. The health check only needs the server to start and + * respond to tools/list — it does not need access to the user's secrets. + * + * Pattern: known secret variable names from major cloud providers, AI APIs, + * CI systems, and databases. Uses exact matches (case-insensitive compare). + */ +const SENSITIVE_ENV_PREFIXES: readonly string[] = [ + "AWS_SECRET", + "AWS_SESSION", + "AZURE_CLIENT_SECRET", + "AZURE_TENANT", + "GCP_SERVICE_ACCOUNT", + "GOOGLE_APPLICATION_CREDENTIALS", +]; + +const SENSITIVE_ENV_NAMES: ReadonlySet = new Set([ + // Cloud provider secrets + "AWS_SECRET_ACCESS_KEY", + "AWS_SESSION_TOKEN", + // AI API keys + "ANTHROPIC_API_KEY", + "OPENAI_API_KEY", + "GOOGLE_API_KEY", + // VCS tokens + "GITHUB_TOKEN", + "GH_TOKEN", + "GITLAB_TOKEN", + "BITBUCKET_TOKEN", + // Package registry tokens + "NPM_TOKEN", + "NODE_AUTH_TOKEN", + "PYPI_TOKEN", + // Database + "DATABASE_URL", + "DB_PASSWORD", + "PGPASSWORD", + "MYSQL_PWD", + "REDIS_PASSWORD", + // CI/CD + "CI_JOB_TOKEN", + "CIRCLE_TOKEN", + "TRAVIS_TOKEN", + // Generic + "SECRET_KEY", + "PRIVATE_KEY", + "ENCRYPTION_KEY", + "API_SECRET", + "AUTH_TOKEN", +]); + +/** + * Build a sanitized environment for health check subprocesses. + * Strips known sensitive variables from process.env before merging + * the server's declared env vars (which may legitimately need API keys + * the user provided during install). + */ +function buildHealthCheckEnv( + serverEnv?: Record, + extraEnv?: Record +): Record { + const base: Record = {}; + + for (const [key, value] of Object.entries(process.env)) { + if (value === undefined) continue; + const upper = key.toUpperCase(); + if (SENSITIVE_ENV_NAMES.has(upper)) continue; + if (SENSITIVE_ENV_PREFIXES.some((p) => upper.startsWith(p))) continue; + base[key] = value; + } + + // Merge caller-provided env and server-declared env on top. + // Server env vars (e.g. API keys the user typed during install) are + // intentionally NOT filtered — the user explicitly provided them. + return { ...base, ...(extraEnv ?? {}), ...(serverEnv ?? {}) }; +} + export interface HealthCheckResult { tier: 1 | 2 | 3; passed: boolean; @@ -86,7 +168,7 @@ export async function runHealthCheck( const input = `${initRequest}\n${initializedNotification}\n${toolsRequest}\n`; - const mergedEnv = { ...process.env, ...(env ?? {}), ...(entry.env ?? {}) }; + const mergedEnv = buildHealthCheckEnv(entry.env, env); return new Promise((resolve) => { const child = spawn(command, [...args], { diff --git a/src/server/handlers.test.ts b/src/server/handlers.test.ts index 5f9fd68..04b9208 100644 --- a/src/server/handlers.test.ts +++ b/src/server/handlers.test.ts @@ -195,7 +195,7 @@ describe("handleRemove", () => { }; const deps = makeDeps({ getAdapter: vi.fn().mockReturnValue(adapter) }); - const result = await handleRemove({ name: "my-server" }, deps); + const result = await handleRemove({ name: "io.github.test/my-server" }, deps); const r = result as { removed: boolean; clients: string[] }; expect(r.removed).toBe(true); expect(r.clients).toContain("cursor"); @@ -210,7 +210,7 @@ describe("handleRemove", () => { }; const deps = makeDeps({ getAdapter: vi.fn().mockReturnValue(adapter) }); - await expect(handleRemove({ name: "nonexistent" }, deps)).rejects.toThrow(/not found/i); + await expect(handleRemove({ name: "io.github.test/nonexistent" }, deps)).rejects.toThrow(/not found/i); }); }); diff --git a/src/server/handlers.ts b/src/server/handlers.ts index 7a314ac..f51231f 100644 --- a/src/server/handlers.ts +++ b/src/server/handlers.ts @@ -16,6 +16,36 @@ import { extractRegistryMeta } from "../utils/format-trust.js"; import { formatMcpEntryCommand } from "../utils/format-entry.js"; import { resolveInstallEntry } from "../commands/install.js"; +// --------------------------------------------------------------------------- +// Input validation for MCP server tool arguments +// --------------------------------------------------------------------------- + +/** + * Server name pattern for MCP registry names. + * Format: "namespace/server-name" — alphanumeric with dots, hyphens, underscores. + * Max length 256 to prevent abuse. Must not contain shell metacharacters, + * path traversal sequences, or control characters. + */ +const SERVER_NAME_RE = + /^[a-zA-Z0-9][a-zA-Z0-9._-]{0,126}\/[a-zA-Z0-9][a-zA-Z0-9._-]{0,126}$/; + +/** + * Validate a server name received from an MCP tool call. + * This is the trust boundary — AI agents provide these strings, and they + * could be influenced by prompt injection or adversarial inputs. + */ +function validateMcpServerName(name: string): void { + if (typeof name !== "string" || name.length === 0 || name.length > 256) { + throw new Error(`Invalid server name: must be a non-empty string under 256 characters.`); + } + if (!SERVER_NAME_RE.test(name)) { + throw new Error( + `Invalid server name format: "${name}". Expected format: "namespace/server-name" ` + + `(alphanumeric, dots, hyphens, underscores only).` + ); + } +} + const execFileAsync = promisify(execFile); // --------------------------------------------------------------------------- @@ -87,13 +117,32 @@ export async function handleSearch( return { servers }; } +/** Default minimum trust score for MCP server tool installs (no human in the loop). */ +const DEFAULT_MIN_TRUST_SCORE = 50; + export async function handleInstall( - args: { name: string; client?: string }, + args: { name: string; client?: string; minTrustScore?: number }, deps: ServerDeps, preResolved?: { entry: ServerEntry; trust: TrustScore } ): Promise { + if (!preResolved) validateMcpServerName(args.name); const entry = preResolved?.entry ?? await deps.registryGetServer(args.name); const trust = preResolved?.trust ?? computeTrust(entry, deps); + + // Security gate: reject servers below the minimum trust score. + // Unlike the CLI path which has a human confirmation prompt, the MCP server + // path is driven by AI agents with no human in the loop. A malicious prompt + // could trick an agent into installing a dangerous server, so we enforce a + // hard trust floor here. + const minScore = args.minTrustScore ?? DEFAULT_MIN_TRUST_SCORE; + if (trust.score < minScore) { + throw new Error( + `Server "${args.name}" has trust score ${trust.score}/${trust.maxPossible} ` + + `(level: ${trust.level}), which is below the minimum threshold of ${minScore}. ` + + `Install rejected for safety. Use mcpm CLI with --yes to override after manual review.` + ); + } + const clients = await resolveClients(args.client, deps); const installedClients: ClientId[] = []; @@ -125,6 +174,7 @@ export async function handleInfo( args: { name: string }, deps: ServerDeps ): Promise { + validateMcpServerName(args.name); const entry = await deps.registryGetServer(args.name); const trust = computeTrust(entry, deps); return { @@ -164,6 +214,7 @@ export async function handleRemove( args: { name: string; client?: string }, deps: ServerDeps ): Promise { + validateMcpServerName(args.name); const clients = await resolveClients(args.client, deps); const removedClients: ClientId[] = []; diff --git a/src/server/tools.ts b/src/server/tools.ts index a274c49..8f64c24 100644 --- a/src/server/tools.ts +++ b/src/server/tools.ts @@ -22,12 +22,13 @@ export const TOOL_DEFINITIONS = [ }, { name: "mcpm_install", - description: "Install an MCP server from the registry into detected AI client configs. Runs trust assessment automatically.", + description: "Install an MCP server from the registry into detected AI client configs. Runs trust assessment automatically. Rejects servers below the minimum trust score (default 50).", inputSchema: { type: "object" as const, properties: { name: { type: "string", description: "Server name (e.g. io.github.domdomegg/filesystem-mcp)" }, client: { type: "string", description: "Install to specific client only (claude-desktop, cursor, vscode, windsurf)" }, + minTrustScore: { type: "number", description: "Minimum trust score to allow install (default 50, range 0-100)" }, }, required: ["name"], }, @@ -107,6 +108,7 @@ export const SearchInput = z.object({ export const InstallInput = z.object({ name: z.string(), client: z.string().optional(), + minTrustScore: z.number().min(0).max(100).optional().default(50), }); export const InfoInput = z.object({