fix(secrets): wrap an image server's own command, not just entrypoint
Some checks failed
CI/CD / lint (pull_request) Successful in 1m25s
CI/CD / test (pull_request) Successful in 1m32s
CI/CD / typecheck (pull_request) Successful in 3m0s
CI/CD / smoke (pull_request) Failing after 2m0s
CI/CD / build (pull_request) Successful in 4m34s
CI/CD / publish (pull_request) Has been skipped
Some checks failed
CI/CD / lint (pull_request) Successful in 1m25s
CI/CD / test (pull_request) Successful in 1m32s
CI/CD / typecheck (pull_request) Successful in 3m0s
CI/CD / smoke (pull_request) Failing after 2m0s
CI/CD / build (pull_request) Successful in 4m34s
CI/CD / publish (pull_request) Has been skipped
Migrating docmost and my-home-assistant, both rendered their secret and neither picked it up. The pod spec showed why: command: (empty) args: ["node","build/index.js"] server.entrypoint: (unset) wrapCommand consulted only `server.entrypoint` for non-package servers, so with `entrypoint` unset it returned undefined, the wrapper was skipped entirely, and the container ran its normal command — which never sourced /vault/secrets/<name>. The failure is silent by construction: the agent init container succeeds, the file is there, and the server simply starts with empty credentials. An explicit `command` on an image server is already a complete command line — mcpd's exec mode would run exactly it — so it should be wrapped verbatim. `entrypoint` is only needed when there is no command at all and the image's own ENTRYPOINT would take over. Now branches on the three real shapes: package server (prepend the runner entrypoint mcpd owns), image + command (use verbatim), image only (require the declared entrypoint). Seven tests, one per shape plus the two undefined cases. Reintroducing the old logic fails two of them — checked before keeping. Both servers were rolled back to secretDelivery: env and are healthy; they can migrate once this ships. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vybEitX4FykeMatKe5Xki
This commit is contained in:
@@ -135,17 +135,29 @@ export class ServerIdentityService {
|
||||
const secretNames = this.secretNamesFor(server);
|
||||
if (secretNames.length === 0) return undefined;
|
||||
|
||||
// mcpd owns the runner images, so their entrypoints are known. A
|
||||
// dockerImage server's is not introspectable — hence `entrypoint` being
|
||||
// required on the row at validation time.
|
||||
const imageEntrypoint = server.packageName
|
||||
? server.runtime === 'python'
|
||||
? ['uvx']
|
||||
: ['npx', '-y']
|
||||
: ((server.entrypoint as string[] | null) ?? undefined);
|
||||
if (imageEntrypoint === undefined || imageEntrypoint.length === 0) return undefined;
|
||||
// Build the COMPLETE argv the container should run. It differs by shape:
|
||||
//
|
||||
// package server — mcpd owns the runner image, whose ENTRYPOINT
|
||||
// (`npx -y` / `uvx`) is lost once we take over
|
||||
// `command`, so it must be prepended here.
|
||||
// image + command — `command` is already a full command line; mcpd would
|
||||
// have run exactly it. Prepending anything breaks it.
|
||||
// image only — the image's own ENTRYPOINT would run and mcpd cannot
|
||||
// introspect it, so the row must declare `entrypoint`.
|
||||
//
|
||||
// Getting this wrong returns undefined and SILENTLY skips the wrapper: the
|
||||
// agent still renders the file, nothing sources it, and the server starts
|
||||
// with empty credentials. Observed on docmost and my-home-assistant, which
|
||||
// carry a `command` but no `entrypoint`.
|
||||
const hasPackage = server.packageName !== null && server.packageName !== undefined && server.packageName !== '';
|
||||
const argv = hasPackage
|
||||
? [...(server.runtime === 'python' ? ['uvx'] : ['npx', '-y']), ...(command ?? [server.packageName as string])]
|
||||
: command !== undefined && command.length > 0
|
||||
? command
|
||||
: ((server.entrypoint as string[] | null) ?? undefined);
|
||||
|
||||
return wrapCommandForInjector([...imageEntrypoint, ...(command ?? [])], secretNames);
|
||||
if (argv === undefined || argv.length === 0) return undefined;
|
||||
return wrapCommandForInjector(argv, secretNames);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
58
src/mcpd/tests/injector-argv.test.ts
Normal file
58
src/mcpd/tests/injector-argv.test.ts
Normal file
@@ -0,0 +1,58 @@
|
||||
/**
|
||||
* Which argv the injector wrapper wraps, by server shape.
|
||||
*
|
||||
* Getting this wrong is SILENT: wrapCommand returns undefined, the wrapper is
|
||||
* skipped, the agent still renders /vault/secrets/<name>, nothing sources it,
|
||||
* and the server starts with empty credentials. Observed live on docmost and
|
||||
* my-home-assistant, which carry a `command` but no `entrypoint`.
|
||||
*/
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { ServerIdentityService } from '../src/services/server-identity.service.js';
|
||||
import type { SecretBackendService } from '../src/services/secret-backend.service.js';
|
||||
|
||||
const svc = new ServerIdentityService(
|
||||
{} as unknown as SecretBackendService,
|
||||
{ namespace: 'mcpctl-servers', ensure: async () => undefined, remove: async () => undefined },
|
||||
);
|
||||
|
||||
const withSecret = { env: [{ name: 'T', valueFrom: { secretRef: { name: 'creds', key: 'K' } } }] };
|
||||
/** The wrapper is `sh -c <script> arg0 arg1...`; argv starts at index 3. */
|
||||
const argvOf = (r: string[] | undefined): string[] | undefined => r?.slice(3);
|
||||
|
||||
describe('wrapCommand argv by server shape', () => {
|
||||
it('prepends the node runner entrypoint for a package server', () => {
|
||||
const r = svc.wrapCommand({ ...withSecret, packageName: '@leval/mcp-grafana', runtime: 'node', entrypoint: null } as never, ['@leval/mcp-grafana']);
|
||||
expect(argvOf(r)).toEqual(['npx', '-y', '@leval/mcp-grafana']);
|
||||
});
|
||||
|
||||
it('prepends uvx for a python package server', () => {
|
||||
const r = svc.wrapCommand({ ...withSecret, packageName: 'mcp-searxng', runtime: 'python', entrypoint: null } as never, ['mcp-searxng']);
|
||||
expect(argvOf(r)).toEqual(['uvx', 'mcp-searxng']);
|
||||
});
|
||||
|
||||
it('uses an image server\'s command verbatim — prepending anything breaks it', () => {
|
||||
// The docmost/home-assistant regression: this used to return undefined.
|
||||
const r = svc.wrapCommand({ ...withSecret, packageName: null, entrypoint: null } as never, ['node', 'build/index.js']);
|
||||
expect(argvOf(r)).toEqual(['node', 'build/index.js']);
|
||||
});
|
||||
|
||||
it('falls back to the declared entrypoint for an image server with no command', () => {
|
||||
const r = svc.wrapCommand({ ...withSecret, packageName: null, entrypoint: ['/usr/local/bin/gitea-mcp'] } as never, undefined);
|
||||
expect(argvOf(r)).toEqual(['/usr/local/bin/gitea-mcp']);
|
||||
});
|
||||
|
||||
it('returns undefined when there is genuinely nothing to run', () => {
|
||||
expect(svc.wrapCommand({ ...withSecret, packageName: null, entrypoint: null } as never, undefined)).toBeUndefined();
|
||||
});
|
||||
|
||||
it('returns undefined for a server with no secret refs', () => {
|
||||
expect(svc.wrapCommand({ env: [], packageName: 'p', runtime: 'node', entrypoint: null } as never, ['p'])).toBeUndefined();
|
||||
});
|
||||
|
||||
it('always sources before exec, whatever the shape', () => {
|
||||
const r = svc.wrapCommand({ ...withSecret, packageName: null, entrypoint: null } as never, ['node', 'x.js']);
|
||||
expect(r?.[0]).toBe('/bin/sh');
|
||||
expect(r?.[2]).toContain('. /vault/secrets/creds');
|
||||
expect(r?.[2]).toContain('exec "$0" "$@"');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user