diff --git a/docker/base-image/agent_server/mcp_template.py b/docker/base-image/agent_server/mcp_template.py new file mode 100644 index 000000000..1718fe3b5 --- /dev/null +++ b/docker/base-image/agent_server/mcp_template.py @@ -0,0 +1,343 @@ +"""Render `.mcp.json.template` into `.mcp.json` at container startup (#2007). + +`docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md` tells template authors that Trinity +replaces `${VAR}` in `.mcp.json.template` with values from the credential +store. For a `github:` template nothing did: the backend's renderer +(`template_service.generate_credential_files`) reads `.mcp.json` — not the +`.template` — and is `local:`-only anyway, while a `github:` agent's files only +exist **after** `startup.sh` clones the repo *inside* the container. So the sole +writer of `~/.mcp.json` on a `github:` agent was +`inject_trinity_mcp_if_configured()`, and every declared server was silently +absent. A freshly-seeded Cornelius shipped three servers and ran with none. + +This module is that missing renderer, and it runs in-container because that is +the only place the files exist. + +## What it substitutes, and why only there + +`${VAR}` is expanded **inside `env` blocks only**. That is not a simplification +— it is the only form `mcp_validator` accepts. A `${VAR}` in `args` is rejected +outright (a bare `$` is a shell metacharacter and args are never ref-stripped), +and `command` must be a literal allowlist entry, so substituting there is the +RCE-by-config class #590 closed: a credential value becoming the executed +command. The published guide's own example only shows `env` placeholders. + +## Refuse, never blank + +A `${VAR}` with no value is **not** blanked to `""`. Blanking is what makes a +broken config look healthy — #1929's defect, and the reason the deploy path's +laundering (#2006) hid a rejected config. A server whose placeholders cannot be +resolved is withheld with a named reason on stdout (captured by Vector), and +the rest are installed. + +## Merge, never clobber + +Servers are only **added** when absent from `~/.mcp.json`. An entry already +there — the `trinity` entry this agent's server injects, or one an owner edited +— is left exactly as it is. That makes this idempotent across restarts and +order-independent with respect to `inject_trinity_mcp_if_configured()`, which +may create the file before or after this runs. + +Every candidate is validated **individually** through the vendored +`mcp_validator` (byte-identical to the backend copy, Invariant #5) before it is +allowed in, so one bad server does not cost the agent the good ones. +""" + +from __future__ import annotations + +import json +import os +import re +import sys +from pathlib import Path +from typing import Dict, List, Optional, Tuple + +try: # package import (normal) … + from .mcp_validator import McpValidationError, validate_mcp_config +except ImportError: # pragma: no cover — … or standalone, for a script run + from mcp_validator import McpValidationError, validate_mcp_config # type: ignore + +AGENT_HOME = Path("/home/developer") +TEMPLATE_FILE = AGENT_HOME / ".mcp.json.template" +CONFIG_FILE = AGENT_HOME / ".mcp.json" +ENV_FILE = AGENT_HOME / ".env" + +# `${VAR}` and `${VAR:-default}`. The default form is honoured because template +# authors use it for optional paths; an unresolvable ref is still refused +# rather than blanked. +_REF_RE = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)(?::-([^}]*))?\}") + +_ENV_KEY_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + +# Both files are agent-writable; cap the reads so a runaway file cannot be +# loaded whole at every container start. +_MAX_BYTES = 1024 * 1024 + + +def _log(message: str) -> None: + """stdout, so it lands in the container log Vector captures.""" + print(f"[mcp-template] {message}", flush=True) + + +def parse_env_file(path: Path = ENV_FILE) -> Dict[str, str]: + """Read `.env` into a dict. Never raises. + + Mirrors the writer in `routers/credentials.py` (`KEY="value"`, embedded `"` + backslash-escaped) while tolerating the hand-written shapes: `export KEY=v`, + single quotes, bare values, comments, CRLF. + """ + try: + if not path.is_file() or path.stat().st_size > _MAX_BYTES: + return {} + raw = path.read_text(encoding="utf-8", errors="replace") + except OSError: + return {} + + out: Dict[str, str] = {} + for line in raw.splitlines(): + line = line.strip() + if not line or line.startswith("#") or "=" not in line: + continue + if line.startswith("export "): + line = line[len("export "):].lstrip() + key, _, value = line.partition("=") + key = key.strip() + if not _ENV_KEY_RE.match(key): + continue + value = value.strip() + if len(value) >= 2 and value[0] == value[-1] and value[0] in ('"', "'"): + inner = value[1:-1] + value = inner.replace('\\"', '"') if value[0] == '"' else inner + out[key] = value + return out + + +def _resolve_refs(value: str, values: Dict[str, str]) -> Tuple[Optional[str], Optional[str]]: + """Expand every `${VAR}` in `value`. Returns `(resolved, None)` or `(None, reason)`. + + An empty stored value counts as unset: a credential the operator has not + filled in yet is exactly the case that must not silently produce a server + configured with `""`. + """ + missing: List[str] = [] + + def sub(match: re.Match) -> str: + name, default = match.group(1), match.group(2) + resolved = values.get(name) or "" + if resolved: + return resolved + if default is not None: + return default + missing.append(name) + return "" + + expanded = _REF_RE.sub(sub, value) + if missing: + return None, f"unresolved placeholder(s): {', '.join(sorted(set(missing)))}" + return expanded, None + + +def _probe_copy(config: dict) -> dict: + """A validation probe with refs INTACT but written in the plain form. + + `mcp_validator` is vendored byte-identically (Invariant #5) and knows only + `${NAME}`; Trinity's `${NAME:-default}` extension trips its variable-name + check. Rewriting each ref to `${NAME}` for the probe keeps the value a + REFERENCE — which is the shape the validator's literal-secret and + shell-metachar rules are written against — without teaching the vendored + copy a new syntax or validating a rendered secret. + """ + probe = json.loads(json.dumps(config)) + env = probe.get("env") + if isinstance(env, dict): + for key, value in list(env.items()): + if isinstance(value, str): + env[key] = _REF_RE.sub(lambda m: "${%s}" % m.group(1), value) + return probe + + +def render_server( + config: dict, values: Dict[str, str], name: str = "server" +) -> Tuple[Optional[dict], Optional[str]]: + """Render ONE server entry. Returns `(rendered, None)` or `(None, reason)`. + + Substitution is confined to `env`. A `${...}` anywhere else is reported — + not expanded — because the validator would reject the expansion anyway and + a silent expansion into `command` is the #590 class. + + Validation runs BEFORE substitution (#2007 review). `mcp_validator` was + written for the UNRENDERED config: it strips `${...}` refs and rejects + whatever literal remains against `_LITERAL_SECRET_PATTERNS` and + `_SHELL_METACHARS_RE`. After substitution the literal IS the secret, so + validating the rendered entry rejects every real credential — + `AIzaSy…` (Google's actual key shape), `sk-ant-`, `sk-proj-`, `ghp_`, + `github_pat_`, `xoxb-`, `AKIA`, and anything containing a shell + metacharacter — telling the operator to "store it in .env and reference as + ${VAR}", which is precisely what they did. That made the headline outcome + unreachable in production while CI passed on a fixture value + (`GEMINI_API_KEY="real-key-value"`) that no credential looks like. + + `name` is the DECLARED server name and is used in the failure reason. The + probe key used to be the literal `"_probe"`, which the validator embeds + verbatim, so the operator-facing message read + `withheld MCP server 'aistudio': Server '_probe': ...` — and the named, + actionable reason is this feature's core value claim. + """ + if not isinstance(config, dict): + return None, "entry is not an object" + + # Trinity's own placeholder rules run FIRST, so the actionable reason wins + # over the validator's generic "command not in allowlist" for the same + # entry — an operator reading "Trinity substitutes only inside 'env'" knows + # what to change; one reading "'${SHELL_PATH}' not in allowlist" does not. + for field in ("command", "url"): + value = config.get(field) + if isinstance(value, str) and _REF_RE.search(value): + return None, ( + f"'{field}' contains a ${{VAR}} placeholder; Trinity substitutes " + f"only inside 'env' (a credential value must never become the " + f"executed command)" + ) + declared_args = config.get("args") + if isinstance(declared_args, list): + for arg in declared_args: + if isinstance(arg, str) and _REF_RE.search(arg): + return None, ( + "'args' contains a ${VAR} placeholder; Trinity substitutes " + "only inside 'env', and the validator rejects placeholders " + "in args" + ) + + # Validate the entry with refs INTACT — the shape the validator was + # designed for — then substitute into the copy that gets written. + probe_key = name if isinstance(name, str) and name else "server" + try: + validate_mcp_config(json.dumps({"mcpServers": {probe_key: _probe_copy(config)}})) + except McpValidationError as e: + return None, str(e) + + rendered = json.loads(json.dumps(config)) # deep copy, JSON-only by construction + + env = rendered.get("env") + if env is not None: + if not isinstance(env, dict): + return None, "'env' is not an object" + for key, value in list(env.items()): + if not isinstance(value, str): + continue + resolved, reason = _resolve_refs(value, values) + if reason: + return None, reason + env[key] = resolved + + return rendered, None + + +def render( + template_file: Path = TEMPLATE_FILE, + config_file: Path = CONFIG_FILE, + env_file: Path = ENV_FILE, +) -> Dict[str, object]: + """Render the template and merge the result into `.mcp.json`. + + Never raises: this runs on the container startup path, where an exception + would cost the agent its boot over a malformed optional file. + """ + result: Dict[str, object] = {"added": [], "skipped": {}, "status": "ok"} + + try: + if not template_file.is_file(): + result["status"] = "no_template" + return result + if template_file.stat().st_size > _MAX_BYTES: + _log(f"{template_file.name} is too large; ignoring") + result["status"] = "template_too_large" + return result + template_raw = template_file.read_text(encoding="utf-8", errors="replace") + except OSError as e: + _log(f"could not read {template_file}: {e}") + result["status"] = "unreadable" + return result + + try: + template = json.loads(template_raw) + except ValueError as e: + _log(f"{template_file.name} is not valid JSON ({e}); no servers rendered") + result["status"] = "invalid_json" + return result + + declared = template.get("mcpServers") if isinstance(template, dict) else None + if not isinstance(declared, dict) or not declared: + result["status"] = "no_servers" + return result + + existing: dict = {} + if config_file.is_file(): + try: + loaded = json.loads(config_file.read_text(encoding="utf-8", errors="replace")) + if isinstance(loaded, dict): + existing = loaded + except (OSError, ValueError) as e: + # A .mcp.json we cannot parse is not ours to repair — and merging + # into it blind would destroy whatever is there. + _log(f"existing .mcp.json is unreadable/invalid ({e}); leaving it alone") + result["status"] = "existing_unreadable" + return result + + servers = existing.setdefault("mcpServers", {}) + if not isinstance(servers, dict): + _log("existing .mcp.json has a non-object 'mcpServers'; leaving it alone") + result["status"] = "existing_unreadable" + return result + + values = parse_env_file(env_file) + added: List[str] = [] + skipped: Dict[str, str] = {} + + for name, config in declared.items(): + if name in servers: + continue # never clobber an entry that is already installed + # Guarded (#2007 review): `render()` documents "never raises", but + # `_resolves_to_private_ip` catches only `socket.gaierror` and + # `getaddrinfo` raises `UnicodeError` for an over-long hostname label — + # so one bad entry propagated out of an unguarded call, out of this + # unguarded loop, and cost every VALID sibling server its render. + # `startup.sh`'s `|| echo` saved the boot, not the file. + try: + rendered, reason = render_server(config, values, name) + except Exception as e: # noqa: BLE001 — one entry must never cost the rest + skipped[name] = f"could not be validated ({type(e).__name__}: {e})" + continue + if reason: + skipped[name] = reason + continue + servers[name] = rendered + added.append(name) + + for name, reason in sorted(skipped.items()): + _log(f"withheld MCP server '{name}': {reason}") + + if added: + try: + tmp = config_file.with_name(config_file.name + ".tmp") + tmp.write_text(json.dumps(existing, indent=2) + "\n", encoding="utf-8") + os.chmod(tmp, 0o600) + os.replace(tmp, config_file) # atomic: never a half-written config + except OSError as e: + _log(f"could not write {config_file}: {e}") + result["status"] = "write_failed" + return result + _log(f"rendered MCP server(s) into .mcp.json: {', '.join(added)}") + + result["added"] = added + result["skipped"] = skipped + return result + + +def main() -> int: + render() + return 0 # startup must continue regardless + + +if __name__ == "__main__": # pragma: no cover — exercised via startup.sh + sys.exit(main()) diff --git a/docker/base-image/agent_server/mcp_validator.py b/docker/base-image/agent_server/mcp_validator.py new file mode 100644 index 000000000..c8fdbe86d --- /dev/null +++ b/docker/base-image/agent_server/mcp_validator.py @@ -0,0 +1,628 @@ +""" +MCP server config validator (#598, Layer 2 of AISEC-C2 closure). + +Re-allows `.mcp.json` content through `POST /api/agents/{name}/credentials/inject` +ONLY when the structure passes strict validation. Layer 1 (#590) closed the +RCE-by-config bypass by removing `.mcp.json` from the inject allowlist; this +module restores the legitimate use case (owners adding/editing MCP servers +post-deploy) while keeping the attack surface closed. + +Public API: + validate_mcp_config(content: str) -> None + Raises McpValidationError on any rejection. + + class McpValidationError(ValueError): + Distinct exception for the router to surface as 400 Bad Request. + +Threat model: + - Attacker is an authenticated agent OWNER (already has the JWT). + - Goal: defense in depth against shell-injection patterns and the + AISEC-C2 exact reproduction. Does NOT prevent owners from running + malicious code via approved runtimes (npx ) — that's + Layer 3 (sandbox MCP execution). + +Architecture (SOLID at appropriate scale — single file, internal classes): + validate_mcp_config() + └─ _validate_servers_dict() schema + per-entry dispatch + └─ _ENTRY_VALIDATORS_BY_TRANSPORT[transport].validate(name, server) + ├─ _StdioValidator command + args + env + ├─ _HttpValidator url + headers + env (+ SSRF) + └─ _SseValidator subclass of _HttpValidator (semantically + distinct, same rules) +""" +from __future__ import annotations + +import ipaddress +import json +import os +import re +import socket +from typing import Mapping +from urllib.parse import urlparse + + +# --------------------------------------------------------------------------- +# Public exception +# --------------------------------------------------------------------------- + + +class McpValidationError(ValueError): + """Raised when an MCP server config fails validation. + + Routers translate this to HTTP 400. The message is included verbatim in + the response, so it must be safe to surface to the caller (no internal + paths or stack traces). + """ + + +# --------------------------------------------------------------------------- +# Constants — tunable in one place +# --------------------------------------------------------------------------- + +# Maximum size of the rendered `.mcp.json` content. 64KB is ~10x what a +# realistic config needs and keeps validation O(n) bounded. +MAX_CONTENT_BYTES = 64 * 1024 + +# Maximum number of mcpServers entries. Real configs have <10; cap defends +# against pathological JSON. +MAX_SERVER_COUNT = 32 + +# Server name (the dict key under mcpServers): conservative ASCII rule. Long +# enough for realistic identifiers, short enough to be UI-safe. +_SERVER_NAME_RE = re.compile(r"^[a-zA-Z0-9_-]{1,64}$") + +# Reserved server names — owners cannot overwrite Trinity's auto-injected +# entry with a different SHAPE (which would break agent-to-agent +# collaboration or open an RCE-by-config vector). The trinity entry IS +# allowed through if its shape matches what the agent server's +# inject_trinity_mcp_if_configured() writes — this lets owners legitimately +# update the bearer token (e.g. rotating their MCP API key) without +# tripping the reserved-name reject. See _is_canonical_trinity_entry. +RESERVED_SERVER_NAMES = frozenset({"trinity"}) + +# Canonical Trinity-MCP entry shape (mirrors +# docker/base-image/agent_server/services/trinity_mcp.py:_inject_claude_mcp). +# url is whatever the agent's TRINITY_MCP_URL env points to; we accept both +# the runtime value (so admins can override it cluster-wide) and the +# documented default for fresh installs. +_TRINITY_DEFAULT_URL = "http://mcp-server:8080/mcp" +_TRINITY_BEARER_RE = re.compile(r"^Bearer\s+trinity_mcp_[A-Za-z0-9_-]{1,200}$") + + +def _is_canonical_trinity_entry(server: dict) -> bool: + """Return True iff ``server`` looks exactly like the Trinity-injected + entry: only ``type``, ``url``, ``headers`` keys; ``type=http``; ``url`` + matches the configured Trinity MCP URL; ``headers`` contains only + ``Authorization`` with a ``Bearer trinity_mcp_…`` token of the right + shape. + + Strict allowlist — any extra key, any wrong value, any extra header, + any non-bearer auth scheme is rejected. Owners who want to redefine + trinity with a different shape (e.g. as a stdio server) hit the + reserved-name reject; the canonical shape is the only escape. + """ + if set(server.keys()) != {"type", "url", "headers"}: + return False + if server.get("type") != "http": + return False + + url = server.get("url") + if not isinstance(url, str): + return False + expected_url = os.getenv("TRINITY_MCP_URL", _TRINITY_DEFAULT_URL) + if url not in (expected_url, _TRINITY_DEFAULT_URL): + return False + + headers = server.get("headers") + if not isinstance(headers, dict) or list(headers.keys()) != ["Authorization"]: + return False + auth = headers.get("Authorization") + if not isinstance(auth, str) or not _TRINITY_BEARER_RE.match(auth): + return False + + return True + +# Allowed values for the `transport` field. `stdio` is implicit when only +# `command` is present (we set transport explicitly during validation). +ALLOWED_TRANSPORTS = frozenset({"stdio", "http", "sse"}) + +# Stdio runtime allowlist. Each entry is the EXACT command name the user can +# specify; absolute paths and alternates are rejected. `python3` listed +# separately from `python` because both are real on different distros. +COMMAND_ALLOWLIST = frozenset({ + "npx", "uvx", "python", "python3", "node", "bun", "deno", "docker", +}) + +# Per-runtime "execution flags" that turn the runtime into a shell. We block +# these as the FIRST positional arg (which would replace the script/package +# arg with inline code). Owners with a real need (`-c "import …"`) should +# package as a script and reference it instead. +_INLINE_EXEC_FLAGS_BY_COMMAND: Mapping[str, frozenset[str]] = { + "python": frozenset({"-c", "--command"}), + "python3": frozenset({"-c", "--command"}), + "node": frozenset({"-e", "--eval", "-p", "--print"}), + "bun": frozenset({"-e", "--eval"}), + "deno": frozenset({"eval"}), + # npx / uvx don't have an inline-exec flag; the first positional IS the + # package name. Likewise docker. +} + +# Shell metacharacters that have no business in any arg passed to a runtime +# allowlisted above. Each runtime invokes its target via execve, not a shell, +# so these characters never need to appear unescaped in real configs. +_SHELL_METACHARS_RE = re.compile(r"[;&|<>`$\n\r\x00]") + +# Substring patterns that indicate command substitution. `re.search` finds +# them anywhere in the arg. +_COMMAND_SUBSTITUTION_RE = re.compile(r"\$\([^)]+\)|`[^`]+`") + +# Env var names allowed as `${VAR}` references in args/env values. ASCII +# uppercase + digits + underscore, must start with a letter — standard +# POSIX shape; rejects `${PATH}` at a separate gate. +_ENV_VAR_REF_RE = re.compile(r"^[A-Z][A-Z0-9_]*$") + +# Env vars the user must NOT reference — overriding any of these from an +# attacker-controlled `.mcp.json` could change library/binary loading, +# attach an interpreter, or hijack subsequent process launches. +RESERVED_ENV_REFS = frozenset({ + "PATH", "HOME", "USER", "SHELL", + "LD_PRELOAD", "LD_LIBRARY_PATH", "DYLD_INSERT_LIBRARIES", + "PYTHONPATH", "PYTHONSTARTUP", "PYTHONHOME", + "NODE_OPTIONS", "NODE_PATH", + # Trinity-internal — the agent server reads these on startup + "TRINITY_MCP_API_KEY", "TRINITY_MCP_URL", "ADMIN_PASSWORD", + "SECRET_KEY", "INTERNAL_API_SECRET", "CREDENTIAL_ENCRYPTION_KEY", + "CLAUDE_CODE_OAUTH_TOKEN", "ANTHROPIC_API_KEY", +}) + +# Patterns that look like raw secrets accidentally pasted into a config — +# reject with a clear message rather than silently accepting them. Mirrors +# `credential_patterns` in docker/base-image/hooks/guardrails-baseline.json. +_LITERAL_SECRET_PATTERNS = ( + re.compile(r"sk-ant-[a-zA-Z0-9_-]{20,}"), + re.compile(r"sk-ant-oat01-[a-zA-Z0-9_-]{20,}"), + re.compile(r"sk-(?:proj-)?[a-zA-Z0-9]{32,}"), + re.compile(r"ghp_[a-zA-Z0-9]{30,}"), + re.compile(r"github_pat_[a-zA-Z0-9_]{40,}"), + re.compile(r"AKIA[0-9A-Z]{16}"), + re.compile(r"xox[baprs]-[0-9a-zA-Z-]{20,}"), + re.compile(r"AIza[0-9A-Za-z_-]{35}"), +) + +# Header names allowed in http/sse server entries. Limit to the small set +# real servers actually use to avoid weird header smuggling vectors. +_ALLOWED_HEADER_NAMES = frozenset({ + "authorization", "x-api-key", "user-agent", "accept", "content-type", +}) + + +# --------------------------------------------------------------------------- +# Helpers (private) +# --------------------------------------------------------------------------- + + +def _is_printable_ascii(s: str) -> bool: + """Strict ASCII printable check (defeats Unicode lookalikes + null bytes).""" + return all(0x20 <= ord(c) <= 0x7E for c in s) + + +# `${VAR}` substring used to extract refs from values like `Bearer ${TOKEN}`. +# Any reference found must satisfy `_ENV_VAR_REF_RE` shape AND not be in +# `RESERVED_ENV_REFS`. The remaining literal portion (with refs stripped) +# is then checked for shell metacharacters. +_ENV_VAR_SUBSTRING_RE = re.compile(r"\$\{([^}]*)\}") + + +def _validate_env_value(server_name: str, key: str, value: object) -> None: + """Validate one env value. + + Allowed shapes (covers real-world cases like `Bearer ${API_TOKEN}`, + `${OPENAI_BASE_URL}/v1`, plain literal URLs, plain `${VAR}` refs): + - Any number of `${VAR}` substring references, each with a valid + var name that is NOT in RESERVED_ENV_REFS + - The remaining literal portion (refs stripped) is checked for shell + metacharacters and command substitution + + Reject: + - non-string values, oversize values + - command substitution patterns (`$(…)` or backticks) + - shell metacharacters in the literal portion + - literal secrets (defense against accidental paste) + - malformed or reserved `${VAR}` references + """ + if not isinstance(value, str): + raise McpValidationError( + f"Server '{server_name}': env['{key}'] must be a string" + ) + if len(value) > 4096: + raise McpValidationError( + f"Server '{server_name}': env['{key}'] exceeds 4096 chars" + ) + + # Pull out every `${VAR}` reference; each must be safe. + refs = _ENV_VAR_SUBSTRING_RE.findall(value) + for var_name in refs: + if not _ENV_VAR_REF_RE.match(var_name): + raise McpValidationError( + f"Server '{server_name}': env['{key}'] references malformed " + f"variable name '{var_name}'" + ) + if var_name in RESERVED_ENV_REFS: + raise McpValidationError( + f"Server '{server_name}': env['{key}'] references reserved " + f"variable '{var_name}'" + ) + + # Strip refs to get the literal portion, then apply shell-safety checks + # to it. This way `Bearer ${TOKEN}` validates as `Bearer ` (safe). + literal = _ENV_VAR_SUBSTRING_RE.sub("", value) + + if _COMMAND_SUBSTITUTION_RE.search(literal): + raise McpValidationError( + f"Server '{server_name}': env['{key}'] contains command " + f"substitution" + ) + if _SHELL_METACHARS_RE.search(literal): + raise McpValidationError( + f"Server '{server_name}': env['{key}'] contains shell " + f"metacharacters" + ) + for pat in _LITERAL_SECRET_PATTERNS: + if pat.search(literal): + raise McpValidationError( + f"Server '{server_name}': env['{key}'] looks like a literal " + f"secret — store it in .env and reference as ${{VAR}}" + ) + + +def _resolves_to_private_ip(hostname: str) -> bool: + """Best-effort DNS check (mirrors SEC-179 / #179). + + Returns True if the hostname resolves to ANY private/loopback/link-local/ + multicast IP. On DNS failure: True (fail closed). Used to block IMDS, + localhost, RFC 1918, and similar SSRF targets. + """ + try: + # getaddrinfo returns a list of (family, type, proto, canonname, sockaddr). + # We only need the IP from sockaddr. + infos = socket.getaddrinfo(hostname, None) + except socket.gaierror: + return True # fail closed + for info in infos: + sockaddr = info[4] + try: + ip = ipaddress.ip_address(sockaddr[0]) + except (ValueError, IndexError): + continue + if ( + ip.is_private + or ip.is_loopback + or ip.is_link_local + or ip.is_multicast + or ip.is_reserved + or ip.is_unspecified + ): + return True + return False + + +# --------------------------------------------------------------------------- +# Transport validators +# --------------------------------------------------------------------------- + + +class _StdioValidator: + """Stdio-transport server: command + args + env.""" + + @staticmethod + def validate(server_name: str, server: dict) -> None: + if "command" not in server: + raise McpValidationError( + f"Server '{server_name}' (stdio): missing required field 'command'" + ) + command = server["command"] + if not isinstance(command, str) or not command: + raise McpValidationError( + f"Server '{server_name}': command must be a non-empty string" + ) + if "/" in command or "\\" in command: + raise McpValidationError( + f"Server '{server_name}': command must be a name, not a path " + f"(got '{command}')" + ) + if not _is_printable_ascii(command): + raise McpValidationError( + f"Server '{server_name}': command contains non-ASCII or " + f"control characters" + ) + if command not in COMMAND_ALLOWLIST: + raise McpValidationError( + f"Server '{server_name}': command '{command}' not in allowlist. " + f"Allowed: {sorted(COMMAND_ALLOWLIST)}" + ) + + args = server.get("args", []) + if not isinstance(args, list): + raise McpValidationError( + f"Server '{server_name}': args must be a list" + ) + if len(args) > 64: + raise McpValidationError( + f"Server '{server_name}': args list too long (max 64)" + ) + + # Block the inline-exec flag as the FIRST positional. Later positions + # would be bare strings the runtime treats as data. + inline_flags = _INLINE_EXEC_FLAGS_BY_COMMAND.get(command, frozenset()) + for i, arg in enumerate(args): + if not isinstance(arg, str): + raise McpValidationError( + f"Server '{server_name}': args[{i}] must be a string" + ) + if len(arg) > 1024: + raise McpValidationError( + f"Server '{server_name}': args[{i}] exceeds 1024 chars" + ) + if "\x00" in arg: + raise McpValidationError( + f"Server '{server_name}': args[{i}] contains null byte" + ) + if _COMMAND_SUBSTITUTION_RE.search(arg): + raise McpValidationError( + f"Server '{server_name}': args[{i}] contains command " + f"substitution" + ) + if _SHELL_METACHARS_RE.search(arg): + raise McpValidationError( + f"Server '{server_name}': args[{i}] contains shell " + f"metacharacters" + ) + if i == 0 and arg in inline_flags: + raise McpValidationError( + f"Server '{server_name}': inline-exec flag '{arg}' not " + f"allowed; package the code as a script and reference its path" + ) + + env = server.get("env", {}) + if not isinstance(env, dict): + raise McpValidationError( + f"Server '{server_name}': env must be an object" + ) + if len(env) > 64: + raise McpValidationError( + f"Server '{server_name}': env has too many entries (max 64)" + ) + for key, value in env.items(): + if not isinstance(key, str) or not _ENV_VAR_REF_RE.match(key): + raise McpValidationError( + f"Server '{server_name}': env key '{key}' must match " + f"^[A-Z][A-Z0-9_]*$" + ) + _validate_env_value(server_name, key, value) + + +class _HttpValidator: + """HTTP-transport server: url + headers + env.""" + + transport_label = "http" + + @classmethod + def validate(cls, server_name: str, server: dict) -> None: + if "url" not in server: + raise McpValidationError( + f"Server '{server_name}' ({cls.transport_label}): " + f"missing required field 'url'" + ) + url = server["url"] + if not isinstance(url, str) or len(url) > 2048: + raise McpValidationError( + f"Server '{server_name}': url must be a string < 2048 chars" + ) + + try: + parsed = urlparse(url) + except ValueError as e: + raise McpValidationError( + f"Server '{server_name}': invalid url ({e})" + ) + + if parsed.scheme != "https": + raise McpValidationError( + f"Server '{server_name}': url must use https (got '{parsed.scheme}')" + ) + # Reject userinfo (`https://user:pass@evil.com/...`) which can confuse + # display-only auditors and is never needed for MCP. + if "@" in (parsed.netloc or ""): + raise McpValidationError( + f"Server '{server_name}': url must not contain userinfo (@)" + ) + hostname = (parsed.hostname or "").lower() + if not hostname: + raise McpValidationError( + f"Server '{server_name}': url missing hostname" + ) + if not _is_printable_ascii(hostname): + raise McpValidationError( + f"Server '{server_name}': url hostname contains non-ASCII " + f"(possible homograph)" + ) + if _resolves_to_private_ip(hostname): + raise McpValidationError( + f"Server '{server_name}': url hostname '{hostname}' resolves " + f"to a private/loopback/link-local address (SSRF guard)" + ) + + headers = server.get("headers", {}) + if not isinstance(headers, dict): + raise McpValidationError( + f"Server '{server_name}': headers must be an object" + ) + if len(headers) > 16: + raise McpValidationError( + f"Server '{server_name}': headers has too many entries (max 16)" + ) + for key, value in headers.items(): + if not isinstance(key, str): + raise McpValidationError( + f"Server '{server_name}': header name must be a string" + ) + if key.lower() not in _ALLOWED_HEADER_NAMES: + raise McpValidationError( + f"Server '{server_name}': header '{key}' not in allowlist. " + f"Allowed: {sorted(_ALLOWED_HEADER_NAMES)}" + ) + # Header values reuse the env-value rules: ${VAR} or safe literal. + _validate_env_value(server_name, f"headers.{key}", value) + + # http/sse can also carry env (rare but allowed by the MCP spec). + env = server.get("env", {}) + if not isinstance(env, dict): + raise McpValidationError( + f"Server '{server_name}': env must be an object" + ) + for key, value in env.items(): + if not isinstance(key, str) or not _ENV_VAR_REF_RE.match(key): + raise McpValidationError( + f"Server '{server_name}': env key '{key}' must match " + f"^[A-Z][A-Z0-9_]*$" + ) + _validate_env_value(server_name, key, value) + + +class _SseValidator(_HttpValidator): + """SSE-transport server. Identical rules to HTTP; separate class for + diagnostic clarity in error messages. + """ + transport_label = "sse" + + +# Dispatch table — Open-Closed: add a new transport by adding a class and +# one entry here, no edits elsewhere. +_ENTRY_VALIDATORS_BY_TRANSPORT = { + "stdio": _StdioValidator, + "http": _HttpValidator, + "sse": _SseValidator, +} + + +# --------------------------------------------------------------------------- +# Entry / config orchestration +# --------------------------------------------------------------------------- + + +def _resolve_transport(server_name: str, server: dict) -> str: + """Determine the transport for an entry. + + The MCP config spec lets transport be implicit: + - stdio: presence of `command` + - http/sse: presence of `url` + explicit `type` field + We require the `type` field for http/sse to avoid ambiguity, and accept + `command` as an implicit stdio signal. + """ + explicit = server.get("type") + if explicit is not None: + if not isinstance(explicit, str) or explicit not in ALLOWED_TRANSPORTS: + raise McpValidationError( + f"Server '{server_name}': type must be one of " + f"{sorted(ALLOWED_TRANSPORTS)}" + ) + return explicit + # No explicit type → infer + if "command" in server: + return "stdio" + if "url" in server: + raise McpValidationError( + f"Server '{server_name}': url provided without 'type' field; " + f"set type to 'http' or 'sse'" + ) + raise McpValidationError( + f"Server '{server_name}': cannot determine transport (no command, " + f"no url, no type)" + ) + + +def _validate_entry(server_name: str, server: object) -> None: + """Validate a single MCP server entry.""" + if not isinstance(server_name, str): + raise McpValidationError("MCP server name must be a string") + if not _SERVER_NAME_RE.match(server_name): + raise McpValidationError( + f"MCP server name '{server_name}' invalid; must match " + f"^[a-zA-Z0-9_-]{{1,64}}$" + ) + if not isinstance(server, dict): + raise McpValidationError( + f"MCP server '{server_name}' must be a JSON object" + ) + if server_name in RESERVED_SERVER_NAMES: + # Allow the reserved trinity entry through ONLY if its shape + # matches the agent server's auto-inject — owners legitimately + # need to edit the bearer token to rotate their MCP API key. + # Any other shape (different url, extra headers, redefined as + # stdio, etc.) hits the reserved-name reject. + if server_name == "trinity" and _is_canonical_trinity_entry(server): + return + raise McpValidationError( + f"MCP server name '{server_name}' is reserved by Trinity" + ) + + transport = _resolve_transport(server_name, server) + validator = _ENTRY_VALIDATORS_BY_TRANSPORT[transport] + validator.validate(server_name, server) + + # Reject any unknown top-level fields. Closed schema = no surprise fields + # that future MCP versions might interpret in unexpected ways. + allowed_keys = {"command", "args", "env", "url", "headers", "type"} + extra = set(server.keys()) - allowed_keys + if extra: + raise McpValidationError( + f"Server '{server_name}': unknown field(s) {sorted(extra)}; " + f"allowed: {sorted(allowed_keys)}" + ) + + +def _validate_servers_dict(servers: dict) -> None: + """Validate the top-level mcpServers dict.""" + if len(servers) > MAX_SERVER_COUNT: + raise McpValidationError( + f"Too many MCP servers ({len(servers)}); max {MAX_SERVER_COUNT}" + ) + for name, entry in servers.items(): + _validate_entry(name, entry) + + +def validate_mcp_config(content: str) -> None: + """Validate the rendered `.mcp.json` content string. + + Raises McpValidationError with a single human-readable message on the + first failure. Routers should surface the message in the HTTP 400 body. + """ + if not isinstance(content, str): + raise McpValidationError(".mcp.json content must be a string") + if len(content.encode("utf-8")) > MAX_CONTENT_BYTES: + raise McpValidationError( + f".mcp.json content exceeds {MAX_CONTENT_BYTES} bytes" + ) + + try: + config = json.loads(content) + except json.JSONDecodeError as e: + raise McpValidationError(f".mcp.json is not valid JSON: {e.msg}") + + if not isinstance(config, dict): + raise McpValidationError(".mcp.json root must be a JSON object") + + # Allow only `mcpServers` at the root. Other top-level keys (e.g. legacy + # `inputs`, future spec fields) are rejected to keep the schema closed. + extra_root = set(config.keys()) - {"mcpServers"} + if extra_root: + raise McpValidationError( + f".mcp.json has unknown top-level field(s) {sorted(extra_root)}; " + f"only 'mcpServers' is allowed" + ) + + servers = config.get("mcpServers", {}) + if not isinstance(servers, dict): + raise McpValidationError(".mcp.json mcpServers must be an object") + + _validate_servers_dict(servers) diff --git a/docker/base-image/startup.sh b/docker/base-image/startup.sh index 00b70cd09..eb6946ffb 100644 --- a/docker/base-image/startup.sh +++ b/docker/base-image/startup.sh @@ -551,6 +551,25 @@ if [ -f ".credentials.enc" ] && [ ! -f ".env" ]; then fi fi +# === Render .mcp.json.template (#2007) === +# The agent guide promises Trinity substitutes ${VAR} in `.mcp.json.template`. +# For a `github:` template nothing did it: the backend renderer is `local:`-only +# and reads `.mcp.json`, not the `.template`, and a github: agent's files only +# exist after the clone above — in here. So declared MCP servers were silently +# absent (a freshly-seeded Cornelius shipped three and ran with none). +# +# Runs AFTER both `.env` sources (the /generated-creds copy and the +# decrypt-and-inject fallback above) so credentials are present either way, and +# merges rather than overwrites — the agent server's own `trinity` entry may be +# written before or after this point. Substitution is confined to `env` blocks, +# each rendered server is validated individually, and an unresolvable +# placeholder withholds that one server with a reason on stdout instead of +# blanking it. Never fails startup. +if [ -f "/home/developer/.mcp.json.template" ]; then + (cd /app && python3 -m agent_server.mcp_template) || \ + echo "Warning: .mcp.json.template rendering failed (continuing startup)" +fi + # === Content Folder Convention === # Create content/ directory for large generated assets (videos, audio, images, exports) # These files persist across restarts but are NOT synced to GitHub diff --git a/docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md b/docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md index 451f4f8a6..898fbfa8d 100644 --- a/docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md +++ b/docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md @@ -90,7 +90,7 @@ See [CLAUDE.md Requirements](#claudemd-requirements) for guidelines. ### 3. `.mcp.json.template` (Required if using MCP servers) -MCP server configuration with credential placeholders. Trinity replaces `${VAR}` with actual values from the credential store. +MCP server configuration with credential placeholders. At container startup Trinity renders this file into `.mcp.json`, replacing `${VAR}` with values from the credential store (#2007). ```json { @@ -108,10 +108,38 @@ MCP server configuration with credential placeholders. Trinity replaces `${VAR}` ``` **Important:** -- Use `${VAR_NAME}` syntax for credential placeholders +- Use `${VAR_NAME}` syntax for credential placeholders — **inside `env` blocks only** +- `${VAR:-default}` is also supported (the default is used when the credential is unset) - Never commit actual secrets - Server names must match `credentials.mcp_servers` keys in `template.yaml` +**Where substitution happens, and what happens when it can't** + +Trinity substitutes **only inside `env`**. A `${VAR}` in `args` or `command` is +**not** expanded, because the MCP config validator rejects both: a bare `$` in +`args` reads as a shell metacharacter, and `command` must be a literal entry +from the runtime allowlist (`npx`, `uvx`, `python`, `python3`, `node`, `bun`, +`deno`, `docker`). Letting a credential value become the executed command is the +config-injection class Trinity closed deliberately — so if you need a path, +pass it through `env` and read it in your server, or use `uvx ` rather +than an absolute interpreter path. + +Rendering is **merge-only and refuse-on-doubt**: + +- A server already present in `.mcp.json` is left untouched — including the + `trinity` entry Trinity injects, and anything you edited by hand. Re-running + is a no-op, so a restart never reverts your changes. +- A server whose placeholders cannot be resolved (no such credential, or the + value is empty) is **withheld**, not configured with a blank value. The + reason is logged to the agent's container output, one line per withheld + server. +- A server the validator rejects is withheld the same way, with the validator's + own reason — so `"command": "uv"` tells you `uv` is not in the allowlist + rather than failing later at exec time. + +The rest of the servers install normally: one bad entry never costs you the +good ones. + ### 4. `.env.example` (Recommended) Documents all required environment variables. Helps users understand what credentials are needed. diff --git a/docs/memory/architecture.md b/docs/memory/architecture.md index 53964b50d..76a8c6ca8 100644 --- a/docs/memory/architecture.md +++ b/docs/memory/architecture.md @@ -348,6 +348,8 @@ The agent server also runs two loops: the 15-min git `auto_sync` heartbeat (see **Durable subscription-token override (#1089):** `startup.sh` exports `CLAUDE_CODE_OAUTH_TOKEN` from `/var/lib/trinity/oauth-token` (when present, non-empty) **before** launching the agent server, so a token rotated via hot-reload survives any plain stop+start of the same container (historically that included `routers/ops.py`'s fleet restart, which bypassed `start_agent_internal` entirely; since #1860 a fleet restart routes through `lifecycle.restart_agent_internal` — a no-drift agent keeps its container and the override survives, a drifted agent is recreated and cleanly re-bakes `Config.Env` from the DB). The path is deliberately on the writable layer, **not** under the persisted `/home/developer` volume: it survives `stop`→`start` (same container) but is wiped on recreate (fresh layer), so a DB-driven recreate cleanly re-bakes `Config.Env` from the DB and the stale override is gone — self-reconciling, no marker logic. Dir created+chowned to UID 1000 in the base-image Dockerfile. +**`.mcp.json.template` rendering (#2007):** `startup.sh` runs `agent_server/mcp_template.py` after the credential-import steps — the missing implementation of the contract `docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md` publishes. It renders `~/.mcp.json.template` into `~/.mcp.json`, substituting `${VAR}` / `${VAR:-default}` from `.env` **inside `env` blocks only** (the only form `mcp_validator` accepts — a `${VAR}` in `args` is rejected as a shell metacharacter and `command` must be a literal allowlist entry, so substituting there is the #590 RCE-by-config class). It lives in-container because that is where the files are: a `github:` agent's template is cloned by `startup.sh`, so the backend renderer (`template_service.generate_credential_files`, `local:`-only and reads `.mcp.json` not the `.template`) never saw it and every declared server was silently absent. Each candidate server is validated individually through the **vendored** `mcp_validator` (byte-identical to the backend copy, Invariant #5); a server whose placeholders don't resolve, or that the validator rejects, is **withheld with a named reason on stdout** rather than blanked (the #1929 contract), and the rest still install. Merge-only-missing, so the `trinity` entry and any owner edit survive; idempotent across restarts and order-independent with respect to `inject_trinity_mcp_if_configured()`. Never fails the boot. + **Template-supplied pre-check** (SCHED-COND-001, #454): if the template ships an executable `~/.trinity/pre-check`, the backend's internal endpoint `POST /api/internal/agents/{name}/pre-check` runs it via `docker exec` before a cron-triggered chat. Language-agnostic — interpreter selected by shebang. The hook's stdout becomes the chat message; empty stdout + exit 0 records a skipped execution (Claude never invoked). Uses the same `execute_command_in_container` primitive as `git_service.py`, `ssh_service.py`, and the agent terminal — no agent-server HTTP endpoint. **Persistent chat:** all chat messages auto-saved to SQLite (`chat_sessions`, `chat_messages`) with full observability (costs, context, tool calls, execution time); sessions survive container restarts/deletions; users see only their own messages (admins see all). diff --git a/tests/registry.json b/tests/registry.json index fc59a215e..07b6e131f 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -33,6 +33,19 @@ ], "description": "A2A exposed-skills filter (ent#180): the open-core seam that narrows a card's skills[] to what an agent may advertise. OSS-unchanged by construction (no provider → identity, same list object); None = no opinion = advertise all (the unconfigured default, so an exposed agent's card is byte-identical across upgrade) vs [] = explicit advertise-nothing; stale/unknown stored ids are inert (the selection only subtracts, the template stays the source of truth); fail-open on provider error AND on a malformed return (a str would otherwise iterate to chars and silently empty the card — fail-closed, invisible); both card surfaces filter via one router helper (structural guard against a route rebuilding an unfiltered card). Disclosure control only — message/send dispatches free-form text, so this never narrows what a caller may ask for." }, + { + "file": "unit/test_2007_mcp_template_render.py", + "feature": "#2007", + "added": "2026-08-05", + "categories": [ + "agent-server", + "unit", + "security", + "credentials", + "parity" + ], + "description": "TRINITY_COMPATIBLE_AGENT_GUIDE.md promised Trinity replaces ${VAR} in .mcp.json.template with credential-store values; for a github: template nothing did. The backend renderer (template_service.generate_credential_files) reads .mcp.json - not the .template - and is local:-only, while a github: agent's files only exist AFTER startup.sh clones the repo inside the container, so the only writer of ~/.mcp.json was inject_trinity_mcp_if_configured() and every declared server was silently absent (a freshly-seeded Cornelius shipped three and ran with none). Fix: agent_server/mcp_template.py, invoked by startup.sh after both .env sources (the /generated-creds copy and the decrypt-and-inject fallback). Substitution is confined to env blocks - the only form mcp_validator accepts, since ${VAR} in args reads as a shell metachar and command must be a literal allowlist entry, making substitution there the #590 RCE-by-config class. Unresolvable placeholders WITHHOLD the server with a named reason on stdout instead of blanking to \"\" (the #1929 contract); merge-only-missing keeps the trinity entry and any owner edit across restarts; each candidate is validated individually through the vendored validator so one bad server never costs the good ones. Covers: byte-parity of the vendored mcp_validator (Invariant #5), the Cornelius trio installing two and withholding ebook-mcp with the validator's own 'uv not in allowlist' reason, ${VAR:-default}, empty-value-counts-as-unset, placeholder-in-command asserted against the renderer's OWN named reason (a withheld-only assertion passes with the guard deleted, because the validator refuses the literal too - and would tell the operator the wrong thing to fix) plus an independent 'the credential value never appears in the output' property, idempotence, unparseable-existing-config left alone, every malformed-template shape degrading quietly, main() always returning 0, and two static wiring checks (startup.sh invokes it, and does so after the credential import)." + }, { "file": "unit/test_ent15_import_intents.py", "feature": "trinity-enterprise#15", diff --git a/tests/unit/test_2007_mcp_template_render.py b/tests/unit/test_2007_mcp_template_render.py new file mode 100644 index 000000000..821bde31d --- /dev/null +++ b/tests/unit/test_2007_mcp_template_render.py @@ -0,0 +1,483 @@ +"""#2007 — `github:` templates never rendered `.mcp.json.template`. + +`TRINITY_COMPATIBLE_AGENT_GUIDE.md` promised Trinity replaces `${VAR}` in +`.mcp.json.template` with credential-store values. Nothing did, for a `github:` +template: the backend renderer (`template_service.generate_credential_files`) +reads `.mcp.json` — not the `.template` — and is `local:`-only, while a +`github:` agent's files only exist after `startup.sh` clones the repo *inside* +the container. The only writer of `~/.mcp.json` was +`inject_trinity_mcp_if_configured()`, so every declared server was silently +absent — a freshly-seeded Cornelius shipped three servers and ran with none. + +The renderer therefore lives in the container. These tests pin the four +properties that make it safe to run unattended on every boot: + + 1. substitution is confined to `env` — the only form `mcp_validator` accepts, + and the reason a `${VAR}` must never reach `command` (#590); + 2. an unresolvable placeholder **withholds** the server with a reason, it + never blanks to `""` (#1929's defect, and what hid the #2006 residue); + 3. merge-only-missing, so an entry already installed — `trinity`, or one an + owner edited — survives every restart; + 4. one bad server never costs the agent the good ones. + +Loaded standalone by path: the agent server ships in its own image and cannot +import `src/backend` (the `test_1965_agent_server_safe_yaml.py` idiom). +""" + +from __future__ import annotations + +import importlib.util +import json +import sys +from pathlib import Path + +import pytest + +_ROOT = Path(__file__).resolve().parents[2] +_AGENT_SERVER = _ROOT / "docker" / "base-image" / "agent_server" +_MODULE = _AGENT_SERVER / "mcp_template.py" +_CANON_VALIDATOR = _ROOT / "src" / "backend" / "services" / "mcp_validator.py" +_VENDORED_VALIDATOR = _AGENT_SERVER / "mcp_validator.py" + +pytestmark = pytest.mark.unit + + +@pytest.fixture(scope="module") +def mod(): + """Import the renderer with its vendored validator resolvable. + + The module prefers the package-relative import and falls back to a flat + one; the fallback is what a `python3 -m` run inside the image exercises, so + the vendored validator is put on `sys.path` here rather than stubbed. + """ + sys.path.insert(0, str(_AGENT_SERVER)) + try: + spec = importlib.util.spec_from_file_location("_mcp_template_2007", _MODULE) + m = importlib.util.module_from_spec(spec) + spec.loader.exec_module(m) + return m + finally: + sys.path.remove(str(_AGENT_SERVER)) + + +@pytest.fixture +def home(tmp_path): + return tmp_path + + +def _paths(home): + return home / ".mcp.json.template", home / ".mcp.json", home / ".env" + + +def _render(mod, home, template: dict | str, env: str = "", existing=None): + tpl, cfg, envf = _paths(home) + tpl.write_text(template if isinstance(template, str) else json.dumps(template)) + envf.write_text(env) + if existing is not None: + cfg.write_text(json.dumps(existing)) + result = mod.render(template_file=tpl, config_file=cfg, env_file=envf) + written = json.loads(cfg.read_text()) if cfg.exists() else None + return result, written + + +# --------------------------------------------------------------------------- +# Vendored-mirror parity (Invariant #5) +# --------------------------------------------------------------------------- + +def test_validator_copies_are_byte_identical(): + """The container validates with the SAME rules as the backend, or the two + surfaces accept different configs for the same file.""" + assert _VENDORED_VALIDATOR.exists(), "mcp_validator is not vendored (#2007)" + assert _CANON_VALIDATOR.read_bytes() == _VENDORED_VALIDATOR.read_bytes(), ( + "mcp_validator.py drifted between backend and agent-server — re-copy " + "the canonical file over the vendored one." + ) + + +# --------------------------------------------------------------------------- +# The reported bug +# --------------------------------------------------------------------------- + +class TestDeclaredServersAppear: + + def test_a_declared_server_reaches_mcp_json(self, mod, home): + """AC #1 — the whole point: a github: template's declared server is + actually configured.""" + result, written = _render( + mod, + home, + {"mcpServers": {"mermaid-diagram": { + "command": "npx", "args": ["-y", "@peng-shawn/mermaid-mcp-server"]}}}, + ) + assert result["added"] == ["mermaid-diagram"] + assert written["mcpServers"]["mermaid-diagram"]["command"] == "npx" + + def test_env_placeholder_is_substituted_from_the_credential_store(self, mod, home): + """Cornelius's `aistudio` server, verbatim.""" + result, written = _render( + mod, + home, + {"mcpServers": {"aistudio": { + "command": "npx", "args": ["-y", "aistudio-mcp-server"], + "env": {"GEMINI_API_KEY": "${GEMINI_API_KEY}"}}}}, + env='GEMINI_API_KEY="real-key-value"\n', + ) + assert result["added"] == ["aistudio"] + assert written["mcpServers"]["aistudio"]["env"] == { + "GEMINI_API_KEY": "real-key-value" + } + + def test_default_form_is_honoured(self, mod, home): + result, written = _render( + mod, home, + {"mcpServers": {"x": {"command": "npx", "args": ["-y", "pkg"], + "env": {"P": "${MISSING:-/opt/fallback}"}}}}, + ) + assert result["added"] == ["x"] + assert written["mcpServers"]["x"]["env"]["P"] == "/opt/fallback" + + def test_output_passes_the_real_validator(self, mod, home): + """AC #2 — whatever is written must be a config the platform accepts.""" + from services.mcp_validator import validate_mcp_config + + _, written = _render( + mod, home, + {"mcpServers": {"ok": {"command": "python3", "args": ["-m", "srv"], + "env": {"K": "${K}"}}}}, + env="K=v\n", + ) + validate_mcp_config(json.dumps(written)) # raises on rejection + + +# --------------------------------------------------------------------------- +# Refuse, never blank (AC #3 — the shared contract with #1929) +# --------------------------------------------------------------------------- + +class TestRefuseNeverBlank: + + def test_unresolved_placeholder_withholds_the_server_with_a_reason(self, mod, home): + result, written = _render( + mod, home, + {"mcpServers": {"aistudio": {"command": "npx", "args": ["-y", "p"], + "env": {"GEMINI_API_KEY": "${GEMINI_API_KEY}"}}}}, + env="", + ) + assert result["added"] == [] + assert "GEMINI_API_KEY" in result["skipped"]["aistudio"] + assert written is None, "nothing should be written when nothing rendered" + + def test_an_empty_credential_counts_as_unset(self, mod, home): + """A key present but blank is the operator-hasn't-filled-it-in case — + exactly the one that must not silently produce env={'K': ''}.""" + result, _ = _render( + mod, home, + {"mcpServers": {"x": {"command": "npx", "args": ["-y", "p"], + "env": {"K": "${K}"}}}}, + env='K=""\n', + ) + assert result["added"] == [] + assert "K" in result["skipped"]["x"] + + def test_placeholder_in_command_is_refused_not_substituted(self, mod, home): + """The #590 class: a credential value becoming the executed command. + `/api/credentials/update`'s whole-text `str.replace` did exactly this + (#2008); this renderer must not. + + Asserted against the renderer's OWN named reason, not merely "the + server was withheld": an unsubstituted `${SHELL_PATH}` is also refused + by the validator (not in COMMAND_ALLOWLIST), so a withheld-only + assertion passes with the explicit guard deleted and tells the operator + the wrong thing to fix. To be honest about what protects what: the + boundary is "never substitute outside `env`" plus the validator; this + guard exists to name the actual cause. + """ + result, written = _render( + mod, home, + {"mcpServers": {"evil": {"command": "${SHELL_PATH}", "args": []}}}, + env='SHELL_PATH="/bin/sh -c whatever"\n', + ) + assert result["added"] == [] + assert "Trinity substitutes" in result["skipped"]["evil"], ( + "expected the named placeholder-in-command reason, got: " + f"{result['skipped']['evil']}" + ) + assert written is None + + def test_a_substituted_command_never_appears_in_the_output(self, mod, home): + """The property that actually matters, stated independently of the + error text: whatever happens, the credential value must not end up as + the executed command.""" + _, written = _render( + mod, home, + {"mcpServers": { + "evil": {"command": "${SHELL_PATH}", "args": []}, + "ok": {"command": "npx", "args": ["-y", "p"]}, + }}, + env='SHELL_PATH="/bin/sh -c whatever"\n', + ) + assert "/bin/sh" not in json.dumps(written) + + def test_placeholder_in_args_is_refused(self, mod, home): + """Cornelius's `ebook-mcp` shape. The validator rejects `${VAR}` in args + (bare `$` is a shell metachar), so expanding there could only produce a + config the platform refuses.""" + result, _ = _render( + mod, home, + {"mcpServers": {"ebook-mcp": { + "command": "uvx", + "args": ["--directory", "${EBOOK_MCP_PATH:-/opt/mcp/ebook-mcp}", + "run", "ebook-mcp"]}}}, + env="", + ) + assert result["added"] == [] + assert "args" in result["skipped"]["ebook-mcp"] + + def test_a_command_outside_the_allowlist_is_withheld_with_the_validator_reason( + self, mod, home + ): + """Cornelius's real `ebook-mcp` uses `uv`, which is not in + COMMAND_ALLOWLIST — the withholding reason is the validator's own, so + the operator is told what to change (`uvx`).""" + result, _ = _render( + mod, home, + {"mcpServers": {"ebook-mcp": {"command": "uv", "args": ["run", "x"]}}}, + ) + assert result["added"] == [] + assert "allowlist" in result["skipped"]["ebook-mcp"] + + def test_one_bad_server_does_not_cost_the_good_ones(self, mod, home): + """AC #5's shape: the Cornelius trio — two install, one is withheld + with a documented reason.""" + result, written = _render( + mod, home, + {"mcpServers": { + "mermaid-diagram": {"command": "npx", "args": ["-y", "mermaid"]}, + "aistudio": {"command": "npx", "args": ["-y", "aistudio"], + "env": {"GEMINI_API_KEY": "${GEMINI_API_KEY}"}}, + "ebook-mcp": {"command": "uv", "args": ["run", "ebook-mcp"]}, + }}, + env="GEMINI_API_KEY=k\n", + ) + assert sorted(result["added"]) == ["aistudio", "mermaid-diagram"] + assert list(result["skipped"]) == ["ebook-mcp"] + assert sorted(written["mcpServers"]) == ["aistudio", "mermaid-diagram"] + + +# --------------------------------------------------------------------------- +# Merge semantics +# --------------------------------------------------------------------------- + +class TestMergeNeverClobber: + + def test_the_trinity_entry_survives(self, mod, home): + """`inject_trinity_mcp_if_configured()` may write .mcp.json before OR + after this runs; either way its entry must be intact.""" + trinity = {"type": "http", "url": "http://mcp-server:8080/mcp", + "headers": {"Authorization": "Bearer trinity_mcp_x"}} + _, written = _render( + mod, home, + {"mcpServers": {"new": {"command": "npx", "args": ["-y", "p"]}}}, + existing={"mcpServers": {"trinity": trinity}}, + ) + assert written["mcpServers"]["trinity"] == trinity + assert "new" in written["mcpServers"] + + def test_an_owner_edited_entry_is_not_reverted_on_restart(self, mod, home): + edited = {"command": "npx", "args": ["-y", "pkg@2.0.0"]} + result, written = _render( + mod, home, + {"mcpServers": {"pkg": {"command": "npx", "args": ["-y", "pkg@1.0.0"]}}}, + existing={"mcpServers": {"pkg": edited}}, + ) + assert result["added"] == [] + assert written["mcpServers"]["pkg"] == edited + + def test_rendering_twice_changes_nothing(self, mod, home): + tpl, cfg, envf = _paths(home) + tpl.write_text(json.dumps( + {"mcpServers": {"x": {"command": "npx", "args": ["-y", "p"]}}})) + envf.write_text("") + + first = mod.render(template_file=tpl, config_file=cfg, env_file=envf) + snapshot = cfg.read_text() + second = mod.render(template_file=tpl, config_file=cfg, env_file=envf) + + assert first["added"] == ["x"] + assert second["added"] == [] + assert cfg.read_text() == snapshot + + def test_an_unparseable_existing_config_is_left_alone(self, mod, home): + """Not ours to repair, and merging into it blind would destroy it.""" + tpl, cfg, envf = _paths(home) + tpl.write_text(json.dumps({"mcpServers": {"x": {"command": "npx"}}})) + envf.write_text("") + cfg.write_text("{ not json") + + result = mod.render(template_file=tpl, config_file=cfg, env_file=envf) + + assert result["status"] == "existing_unreadable" + assert cfg.read_text() == "{ not json" + + +# --------------------------------------------------------------------------- +# Never breaks the boot +# --------------------------------------------------------------------------- + +class TestStartupSafety: + + @pytest.mark.parametrize("template,status", [ + ("{ not json", "invalid_json"), + ({"mcpServers": {}}, "no_servers"), + ({"no_servers_key": 1}, "no_servers"), + ('"a string"', "no_servers"), + ]) + def test_malformed_template_degrades_quietly(self, mod, home, template, status): + result, written = _render(mod, home, template) + assert result["status"] == status + assert written is None + + def test_absent_template_is_a_no_op(self, mod, home): + tpl, cfg, envf = _paths(home) + result = mod.render(template_file=tpl, config_file=cfg, env_file=envf) + assert result["status"] == "no_template" + assert not cfg.exists() + + def test_main_always_reports_success(self, mod, monkeypatch): + """startup.sh must continue whatever happens here.""" + monkeypatch.setattr(mod, "TEMPLATE_FILE", Path("/nonexistent/.mcp.json.template")) + assert mod.main() == 0 + + +# --------------------------------------------------------------------------- +# Wiring +# --------------------------------------------------------------------------- + +def test_startup_sh_invokes_the_renderer(): + """The module is only a fix if the boot path runs it.""" + startup = (_ROOT / "docker" / "base-image" / "startup.sh").read_text() + assert "agent_server.mcp_template" in startup, ( + "startup.sh does not run the .mcp.json.template renderer (#2007)" + ) + + +def test_the_renderer_runs_after_credentials_are_available(): + """Ordering: `.env` arrives either from the /generated-creds copy or from + the decrypt-and-inject fallback. Rendering before the latter would resolve + placeholders against a file that is not there yet.""" + startup = (_ROOT / "docker" / "base-image" / "startup.sh").read_text() + assert startup.index("decrypt-and-inject") < startup.index("agent_server.mcp_template"), ( + "the renderer runs before the credential auto-import — placeholders " + "would be unresolvable on that path" + ) + + +# --------------------------------------------------------------------------- +# Review follow-ups (#2013 review) +# --------------------------------------------------------------------------- + +class TestRealCredentialValuesRender: + """AC #5 must hold for values that look like real credentials. + + `mcp_validator` was written for the UNRENDERED config: it strips `${...}` + refs and rejects whatever literal remains against `_LITERAL_SECRET_PATTERNS` + and `_SHELL_METACHARS_RE`. Validating AFTER substitution means the literal + IS the secret, so every real key was withheld with advice ("store it in + .env and reference as ${VAR}") that the operator had already followed. CI + passed only because the fixture used `GEMINI_API_KEY="real-key-value"`, + which no credential looks like. + """ + + # Each is the real vendor shape, not a lookalike. + SHAPES = [ + "AIzaSyA1234567890123456789012345678901234", # Google (AIza[0-9A-Za-z_-]{35}) + "sk-ant-api03-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", # Anthropic + "sk-proj-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", # OpenAI + "ghp_1234567890abcdefghij1234567890abcdefgh", # GitHub PAT + "github_pat_11ABCDEFG0abcdefghijkl_ABCDEFGHI", # GitHub fine-grained + "xoxb-123456789012-1234567890123-abcdefghij", # Slack bot + "AKIAIOSFODNN7EXAMPLE", # AWS + "pa$$w|rd;with&shell`meta", # shell metacharacters + ] + + @pytest.mark.parametrize("secret", SHAPES) + def test_the_declared_server_is_rendered_not_withheld(self, mod, home, secret): + result, written = _render( + mod, home, + {"mcpServers": {"aistudio": { + "command": "npx", "args": ["-y", "@aistudio/mcp"], + "env": {"GEMINI_API_KEY": "${GEMINI_API_KEY}"}, + }}}, + env=f'GEMINI_API_KEY="{secret}"\n', + ) + assert result["added"] == ["aistudio"], ( + f"a real-shaped credential was withheld: {result['skipped']}" + ) + assert written["mcpServers"]["aistudio"]["env"]["GEMINI_API_KEY"] == secret + + def test_the_default_form_still_resolves(self, mod, home): + """`${VAR:-default}` is a Trinity extension the vendored validator does + not know, so the probe normalises it to `${VAR}` rather than teaching + the vendored copy a new syntax (Invariant #5) or validating a rendered + secret.""" + result, written = _render( + mod, home, + {"mcpServers": {"x": {"command": "npx", "args": ["-y", "p"], + "env": {"P": "${MISSING:-/opt/fallback}"}}}}, + ) + assert result["added"] == ["x"], result["skipped"] + assert written["mcpServers"]["x"]["env"]["P"] == "/opt/fallback" + + +class TestOneBadServerNeverCostsTheGoodOnes: + """`render()` documents 'never raises' and the per-server call was unguarded. + + `_resolves_to_private_ip` catches only `socket.gaierror`, but + `getaddrinfo` raises `UnicodeError` for an over-long hostname label — so a + single bad entry propagated out of the loop and NOTHING was written, + losing every valid sibling. `startup.sh`'s `|| echo` saved the boot, not + the render. + """ + + def test_an_over_long_hostname_label_only_withholds_its_own_entry(self, mod, home): + result, written = _render( + mod, home, + {"mcpServers": { + "good": {"command": "npx", "args": ["-y", "ok"], "env": {}}, + "bad": {"type": "http", "url": f"https://{'a' * 80}.example.com/mcp"}, + }}, + ) + assert result["added"] == ["good"], ( + "a valid sibling was lost to another entry's failure" + ) + assert "bad" in result["skipped"] + assert written is not None and "good" in written["mcpServers"] + + def test_an_unexpected_error_is_reported_not_raised(self, mod, home, monkeypatch): + """The guard is on the CALL, so any future validator exception type is + covered — not just the one hostname case that was found.""" + def boom(*a, **k): + raise RuntimeError("validator exploded") + + monkeypatch.setattr(mod, "render_server", boom) + result, _ = _render( + mod, home, + {"mcpServers": {"x": {"command": "npx", "args": ["-y", "p"], "env": {}}}}, + ) + assert result["status"] == "ok" + assert "validator exploded" in result["skipped"]["x"] + + +def test_the_withheld_reason_names_the_server_not_the_probe(mod, home): + """The named, actionable reason is this feature's core value claim. + + The probe key used to be the literal `"_probe"`, which the validator embeds + verbatim — so the operator read `withheld MCP server 'aistudio': Server + '_probe': ...`. + """ + result, _ = _render( + mod, home, + {"mcpServers": {"aistudio": {"command": "/bin/sh", "args": ["-c", "id"]}}}, + ) + reason = result["skipped"]["aistudio"] + assert "_probe" not in reason, f"the probe key leaked into the reason: {reason}" + assert "aistudio" in reason, f"the reason does not name the server: {reason}"