diff --git a/src/connection/client.ts b/src/connection/client.ts index ecc9b47..5e2ad4b 100644 --- a/src/connection/client.ts +++ b/src/connection/client.ts @@ -3,6 +3,11 @@ import { ConnectionError } from "./errors.ts"; import { createMcpClient } from "./capabilities.ts"; import type { ConnectionOptions, McpConnection } from "./types.ts"; import type { StdioService } from "../config/schema.ts"; +import { + resolveServiceSecretRefs, + VaultwardenSecretResolver, +} from "../secrets/index.ts"; +import type { SecretResolver } from "../secrets/index.ts"; import { createLogger } from "../logger/index.ts"; const log = createLogger("connection"); @@ -11,17 +16,30 @@ const log = createLogger("connection"); * Connect to an MCP server via stdio transport. * Spawns the process, completes the initialization handshake, returns a live connection. * Enforces timeout -- server hang won't block indefinitely. + * + * ${secret:...} references in the service's `env` and `args` are resolved here, + * before the child is spawned, so no stdio spawn path can leak a literal ref + * into the child's environment (#65). Resolution is idempotent: an already + * resolved config has no remaining refs, so callers that resolve upstream + * (the direct CLI path and the daemon pool) incur no extra secret lookups. */ export async function connectToService( service: StdioService, - options?: { timeout?: number }, + options?: { timeout?: number; resolver?: SecretResolver }, ): Promise { const timeout = options?.timeout ?? 30000; + const resolver = options?.resolver ?? new VaultwardenSecretResolver(); + const resolved = (await resolveServiceSecretRefs( + service.command, + service, + resolver, + )) as StdioService; + const connOpts: ConnectionOptions = { - command: service.command, - args: service.args, - env: service.env, + command: resolved.command, + args: resolved.args, + env: resolved.env, timeout, }; diff --git a/tests/connection/client.test.ts b/tests/connection/client.test.ts index 6115f39..93b86f3 100644 --- a/tests/connection/client.test.ts +++ b/tests/connection/client.test.ts @@ -4,10 +4,12 @@ import { ConnectionError } from "../../src/connection/errors.ts"; import { EXIT_CODES } from "../../src/types/index.ts"; import type { McpConnection } from "../../src/connection/types.ts"; import type { StdioService } from "../../src/config/schema.ts"; +import type { SecretResolver } from "../../src/secrets/index.ts"; import { resolve } from "path"; const MOCK_SERVER = resolve(import.meta.dir, "../fixtures/mock-mcp-server.ts"); const NOISY_SERVER = resolve(import.meta.dir, "../fixtures/noisy-mcp-server.ts"); +const ENV_ECHO_SERVER = resolve(import.meta.dir, "../fixtures/env-echo-mcp-server.ts"); function mockService(script: string): StdioService { return { @@ -126,6 +128,85 @@ describe("connectToService", () => { } }, 15_000); + it("resolves ${secret:...} env refs before spawning the child (#65)", async () => { + const calls: string[] = []; + const resolver: SecretResolver = { + async resolve(ref: string): Promise { + calls.push(ref); + return "RESOLVED_SECRET_VALUE"; + }, + }; + + const service: StdioService = { + backend: "stdio" as const, + command: "bun", + args: [ENV_ECHO_SERVER], + env: { N8N_API_URL: "${secret:RTech MCPs - foo#fields.N8N_API_URL}" }, + }; + + connection = await connectToService(service, { resolver }); + + const result = (await connection.client.callTool({ + name: "echo_env", + arguments: {}, + })) as { content: Array<{ type: string; text: string }> }; + + const payload = JSON.parse(result.content[0]!.text) as { + env: Record; + }; + + // The child must see the RESOLVED value, never the literal ref. + expect(payload.env.N8N_API_URL).toBe("RESOLVED_SECRET_VALUE"); + expect(payload.env.N8N_API_URL).not.toContain("${secret:"); + expect(calls).toContain("RTech MCPs - foo#fields.N8N_API_URL"); + }, 15_000); + + it("resolves ${secret:...} refs in args before spawning the child (#65)", async () => { + const resolver: SecretResolver = { + async resolve(): Promise { + return "arg-secret"; + }, + }; + + const service: StdioService = { + backend: "stdio" as const, + command: "bun", + args: [ENV_ECHO_SERVER, "--token=${secret:some#field}"], + env: {}, + }; + + connection = await connectToService(service, { resolver }); + + const result = (await connection.client.callTool({ + name: "echo_env", + arguments: {}, + })) as { content: Array<{ type: string; text: string }> }; + + const payload = JSON.parse(result.content[0]!.text) as { argv: string[] }; + + expect(payload.argv).toContain("--token=arg-secret"); + for (const a of payload.argv) { + expect(a).not.toContain("${secret:"); + } + }, 15_000); + + it("does not invoke the resolver when there are no secret refs", async () => { + let called = false; + const resolver: SecretResolver = { + async resolve(): Promise { + called = true; + return "unused"; + }, + }; + + connection = await connectToService(mockService(MOCK_SERVER), { resolver }); + + expect(connection.client).toBeDefined(); + // Idempotency guard: already-resolved configs (direct path / daemon pool) + // trigger no extra secret lookups here. + expect(called).toBe(false); + }, 15_000); + it("CLI error handler maps ConnectionError to exit code 5", () => { // Unit test: verify ConnectionError is caught by instanceof // and maps to EXIT_CODES.CONNECTION diff --git a/tests/fixtures/env-echo-mcp-server.ts b/tests/fixtures/env-echo-mcp-server.ts new file mode 100644 index 0000000..650d752 --- /dev/null +++ b/tests/fixtures/env-echo-mcp-server.ts @@ -0,0 +1,83 @@ +#!/usr/bin/env bun +/** + * Mock MCP server that echoes back its own process environment and argv. + * Used to prove that ${secret:...} refs in a stdio service's env/args are + * resolved to their real values BEFORE the child is spawned (#65). + * + * tools/call "echo_env" returns { env, argv } as JSON text content. + */ + +export {}; // mark as a module so top-level `for await` is valid under strict TS (TS1431) + +let buffer = ""; +const decoder = new TextDecoder(); + +for await (const chunk of Bun.stdin.stream()) { + buffer += decoder.decode(chunk, { stream: true }); + + let newlineIndex: number; + while ((newlineIndex = buffer.indexOf("\n")) !== -1) { + const line = buffer.slice(0, newlineIndex).trim(); + buffer = buffer.slice(newlineIndex + 1); + if (line.length === 0) continue; + try { + handleMessage(JSON.parse(line)); + } catch { + // Ignore non-JSON lines + } + } +} + +function handleMessage(msg: { jsonrpc: string; id?: number; method?: string }) { + if (msg.id === undefined) return; + + switch (msg.method) { + case "initialize": + respond(msg.id, { + protocolVersion: "2024-11-05", + capabilities: { tools: {} }, + serverInfo: { name: "env-echo-mcp-server", version: "1.0.0" }, + }); + break; + + case "tools/list": + respond(msg.id, { + tools: [ + { + name: "echo_env", + description: "Echoes back the server's env and argv", + inputSchema: { type: "object", properties: {}, required: [] }, + annotations: { readOnlyHint: true }, + }, + ], + }); + break; + + case "tools/call": + respond(msg.id, { + content: [ + { + type: "text", + text: JSON.stringify({ + env: process.env, + argv: process.argv.slice(2), + }), + }, + ], + }); + break; + + default: + respondError(msg.id, -32601, `Method not found: ${msg.method}`); + } +} + +function respond(id: number, result: unknown) { + process.stdout.write(JSON.stringify({ jsonrpc: "2.0", id, result }) + "\n"); +} + +function respondError(id: number, code: number, message: string) { + process.stdout.write( + JSON.stringify({ jsonrpc: "2.0", id, error: { code, message } }) + "\n", + ); +}