From 6fa409d9b6b3ce5dbd3fdde10f760944a2d69826 Mon Sep 17 00:00:00 2001 From: Rodaddy Date: Sun, 5 Jul 2026 01:22:49 -0400 Subject: [PATCH 1/2] fix(connection): resolve ${secret:...} refs in stdio env before spawn (closes #65) For stdio services, ${secret:...} references in `env` (and `args`) reached the spawned child UNRESOLVED on any connect path that did not resolve upstream. transport.ts spreads `service.env` raw into the child env and only runs ${VAR} expansion on args, never the secret resolver -- so a literal `${secret:...}` (or a blanked `${secret:...}` arg) landed in the child. Observed on `n8n`: n8n-mcp's zod `.url()` validation of N8N_API_URL rejected the literal ref and all 15 management tools reported the misleading "n8n API not configured". The direct CLI path (resolveDirectServiceConfig) and the daemon pool (pool.ts) already resolve, but the pool's HTTP/WebSocket stdio *fallback* constructors (connectFallback / connectWsFallback) build a stdio config from raw fb.env / fb.args and call connectToService with no resolution -- a proven leak. Rather than patch each caller, resolve at the single universal stdio chokepoint: connectToService, which every stdio spawn (direct, pool, and both fallbacks) passes through. Resolution is idempotent -- resolveServiceSecretRefs short-circuits via hasSecretRefs when no ${secret:} remains -- so callers that already resolve upstream incur no extra Vaultwarden lookups. The resolver is injectable (options.resolver) for testing; default stays VaultwardenSecretResolver. Log labels intentionally keep the UNRESOLVED command/args so resolved secret values never reach logs. Test: new env-echo fixture MCP server + client.test.ts cases assert a ${secret:...} env value and a ${secret:...} arg both reach the child RESOLVED (mocked resolver), and that a ref-free config triggers zero resolver calls. Mutation-verified: reverting the fix makes the child read back the literal `${secret:...}` env and a stripped `--token=` arg, failing both. Verified: bun run typecheck clean; full suite 1080 pass / 0 fail across three repeated runs. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/connection/client.ts | 26 +++++++-- tests/connection/client.test.ts | 81 +++++++++++++++++++++++++++ tests/fixtures/env-echo-mcp-server.ts | 81 +++++++++++++++++++++++++++ 3 files changed, 184 insertions(+), 4 deletions(-) create mode 100644 tests/fixtures/env-echo-mcp-server.ts 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..9fce73c --- /dev/null +++ b/tests/fixtures/env-echo-mcp-server.ts @@ -0,0 +1,81 @@ +#!/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. + */ + +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", + ); +} From f85e7c2a667a8b6d74e2034911ef590ffce26a1e Mon Sep 17 00:00:00 2001 From: Rodaddy Date: Sun, 5 Jul 2026 01:25:31 -0400 Subject: [PATCH 2/2] test(fixtures): make env-echo fixture a module (fix TS1431 under strict TS) Top-level `for await` requires the file be a module. Adds `export {}` so strict-context typecheck accepts the fixture; repo tsc was already lax. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/fixtures/env-echo-mcp-server.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/fixtures/env-echo-mcp-server.ts b/tests/fixtures/env-echo-mcp-server.ts index 9fce73c..650d752 100644 --- a/tests/fixtures/env-echo-mcp-server.ts +++ b/tests/fixtures/env-echo-mcp-server.ts @@ -7,6 +7,8 @@ * 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();