diff --git a/src/mcpd/src/services/mcp-proxy-service.ts b/src/mcpd/src/services/mcp-proxy-service.ts index 8e92079..df85367 100644 --- a/src/mcpd/src/services/mcp-proxy-service.ts +++ b/src/mcpd/src/services/mcp-proxy-service.ts @@ -62,6 +62,59 @@ function parseStreamableResponse(body: string): McpProxyResponse { return JSON.parse(body) as McpProxyResponse; } +/** + * Decide how mcpd opens a STDIO session against a running container. + * + * attach → connect to PID 1's stdin/stdout + * exec → spawn a NEW process inside the container + * + * Pure and exported so the choice is testable: it is subtle, and getting it + * wrong fails silently rather than loudly (see the injector case below). + */ +export function chooseStdioMode(server: { + name: string; + id: string; + secretDelivery?: string | null; + command?: string[] | null; + packageName?: string | null; + dockerImage?: string | null; + runtime?: string | null; +}): StdioMode { + // Injected delivery MUST attach, whatever the server type. + // + // The secrets exist only in PID 1's environment: the container command is a + // shell that sources /vault/secrets/ and execs the real server, so the + // values live in that process and nowhere else — not in the pod spec, which + // is the entire point of the feature. + // + // `exec` spawns a NEW process, which never sourced the file and so starts + // with empty credentials. The server comes up, answers tools/list, and fails + // every authenticated call — the exact silent-empty-token failure this + // feature exists to prevent. Observed as + // `Readiness check (list_datasources) failed: process exited 1` on a pod + // whose PID 1 demonstrably held the token. + // + // Safe because wrapCommandForInjector execs rather than forks, so PID 1 IS + // the server process. + if (server.secretDelivery === 'injector') return { kind: 'attach' }; + + if (server.command !== null && server.command !== undefined && server.command.length > 0) { + return { kind: 'exec', command: server.command }; + } + if (server.packageName !== null && server.packageName !== undefined && server.packageName !== '') { + return { kind: 'exec', command: buildRuntimeSpawnCmd(server.runtime ?? 'node', server.packageName) }; + } + // Image entrypoint IS the MCP server. + if (server.dockerImage !== null && server.dockerImage !== undefined && server.dockerImage !== '') { + return { kind: 'attach' }; + } + + throw new InvalidStateError( + `Server '${server.name}' (${server.id}) uses STDIO transport but has no ` + + `packageName, command, or dockerImage. Configure one of these.`, + ); +} + export class McpProxyService { /** Session IDs per server for streamable-http protocol */ private sessions = new Map(); @@ -159,20 +212,15 @@ export class McpProxyService { // - command set → exec the given command in the container. // - dockerImage only → attach to PID 1 (image entrypoint IS the MCP server). // - nothing → unreachable, reject. - const runtime = (server.runtime as string | null) ?? 'node'; - let mode: StdioMode; - if (command && command.length > 0) { - mode = { kind: 'exec', command }; - } else if (packageName) { - mode = { kind: 'exec', command: buildRuntimeSpawnCmd(runtime, packageName) }; - } else if (dockerImage) { - mode = { kind: 'attach' }; - } else { - throw new InvalidStateError( - `Server '${server.name}' (${server.id}) uses STDIO transport but has no ` + - `packageName, command, or dockerImage. Configure one of these.`, - ); - } + const mode = chooseStdioMode({ + name: server.name as string, + id: server.id as string, + secretDelivery: server.secretDelivery as string | null, + command, + packageName, + dockerImage, + runtime: server.runtime as string | null, + }); // Try persistent connection first try { @@ -181,7 +229,7 @@ export class McpProxyService { this.removeClient(instance.containerId); // Fall back to one-shot exec when we have a command to run. if (mode.kind === 'exec') { - return sendViaStdio(this.orchestrator, instance.containerId, packageName, method, params, 120_000, command, runtime); + return sendViaStdio(this.orchestrator, instance.containerId, packageName, method, params, 120_000, command, (server.runtime as string | null) ?? 'node'); } // Attach mode has no one-shot equivalent, but the failure is usually // a stale pipe from an in-place container restart — retry once diff --git a/src/mcpd/tests/stdio-mode.test.ts b/src/mcpd/tests/stdio-mode.test.ts new file mode 100644 index 0000000..a5342d9 --- /dev/null +++ b/src/mcpd/tests/stdio-mode.test.ts @@ -0,0 +1,44 @@ +/** + * How mcpd opens a STDIO session: attach to PID 1, or exec a new process. + * + * Subtle and silent when wrong. With injected secret delivery the credentials + * exist ONLY in PID 1's environment (a shell sourced /vault/secrets/ and + * exec'd the server), so an `exec` starts a process with empty credentials — + * the server comes up, answers tools/list, and fails every authenticated call. + */ +import { describe, it, expect } from 'vitest'; +import { chooseStdioMode } from '../src/services/mcp-proxy-service.js'; + +const base = { name: 's', id: 'id1' }; + +describe('chooseStdioMode', () => { + it('attaches for an injector server even though it has a packageName', () => { + // The regression: packageName would otherwise select exec, and exec loses + // the secrets entirely. + expect(chooseStdioMode({ ...base, secretDelivery: 'injector', packageName: '@leval/mcp-grafana' })) + .toEqual({ kind: 'attach' }); + }); + + it('attaches for an injector server even though it has an explicit command', () => { + expect(chooseStdioMode({ ...base, secretDelivery: 'injector', command: ['node', 'x.js'] })) + .toEqual({ kind: 'attach' }); + }); + + it('still execs a package server on the default env delivery', () => { + const m = chooseStdioMode({ ...base, secretDelivery: 'env', packageName: '@leval/mcp-grafana', runtime: 'node' }); + expect(m.kind).toBe('exec'); + }); + + it('still prefers an explicit command over packageName on env delivery', () => { + expect(chooseStdioMode({ ...base, secretDelivery: 'env', command: ['node', 'x.js'], packageName: 'p' })) + .toEqual({ kind: 'exec', command: ['node', 'x.js'] }); + }); + + it('attaches for an image-entrypoint server, as before', () => { + expect(chooseStdioMode({ ...base, dockerImage: 'gitea/mcp:latest' })).toEqual({ kind: 'attach' }); + }); + + it('rejects a server with no way to start', () => { + expect(() => chooseStdioMode({ ...base })).toThrow(/packageName, command, or dockerImage/); + }); +});