fix(connection): resolve ${secret:...} refs in stdio env before spawn (closes #65)

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<McpConnection> {
   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,
   };
 

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<string> {
+        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<string, string>;
+    };
+
+    // 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<string> {
+        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<string> {
+        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

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",
+  );
+}