diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fa2daca5..49d7fdc46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- `apm install --target copilot` no longer bakes environment variable values into `~/.copilot/mcp-config.json`; placeholders like `${env:VAR}`, `${VAR}`, and legacy `` are translated to Copilot CLI's native runtime substitution syntax (`${VAR}`) so secrets stay in the shell environment instead of on disk. Legacy `` syntax is auto-translated with a deprecation warning; migrate to `${VAR}` in `apm.yml`. (#1152) + ### Changed - **Explicit, auditable target resolution.** `apm install` and `apm compile` now resolve harness targets in a strict priority chain (`--target` flag > `apm.yml` `targets:` > auto-detect from filesystem signals) and print a one-line `[i] Targets: ... (source: ...)` provenance summary so the chosen path is never silently inferred. Empty repositories with no signal now exit 2 with a teaching message instead of silently defaulting to `copilot`. Adds `apm targets` discovery command and `apm compile --all` flag (deprecates `--target all`). (#1165, closes #1154, closes #1122, closes #1130, closes #518, closes #888, closes #891, closes #650, closes #1056) diff --git a/docs/src/content/docs/guides/dependencies.md b/docs/src/content/docs/guides/dependencies.md index 9567b6011..7090e6e7c 100644 --- a/docs/src/content/docs/guides/dependencies.md +++ b/docs/src/content/docs/guides/dependencies.md @@ -489,6 +489,23 @@ mcp: apm install --trust-transitive-mcp ``` +### Environment variable placeholders + +`env`, `headers`, and `args` values may reference environment variables using either of two equivalent forms: + +| Syntax | Meaning | +| ------------- | ----------------------------------------------------------- | +| `${VAR}` | Reference to an environment variable named `VAR` | +| `${env:VAR}` | Same as above (VS Code-style prefix, normalized internally) | + +How APM materializes a placeholder depends on the target harness: + +- **Copilot CLI** (`~/.copilot/mcp-config.json`): the placeholder is preserved as `${VAR}` in the generated config and resolved by Copilot CLI from the host environment at server-start. APM never reads the value, so secrets stay in your shell. Make sure the variable is exported before launching `gh copilot`. +- **VS Code** (`.vscode/mcp.json`): the placeholder is rewritten to VS Code's `${env:VAR}` form and resolved by VS Code at server-start. +- **Other harnesses** (Cursor, Windsurf, OpenCode, Claude Desktop, Gemini, Codex): the placeholder is resolved from the current process environment at install time and the literal value is written into the harness config. + +The legacy `` syntax is still accepted for backward compatibility but emits a deprecation warning; migrate to `${VAR}` in `apm.yml`. + ### Validation Run `apm install --dry-run` to preview MCP dependency configuration without writing any files. Self-defined deps are validated for required fields and transport values; overlay deps are loaded as-is and unknown fields are ignored. diff --git a/docs/src/content/docs/reference/cli-commands.md b/docs/src/content/docs/reference/cli-commands.md index e2b9fea8f..c711b7f07 100644 --- a/docs/src/content/docs/reference/cli-commands.md +++ b/docs/src/content/docs/reference/cli-commands.md @@ -148,6 +148,26 @@ When installing into `.claude/commands/`, prompt files with an `input:` front-ma This transformation only applies to the `claude` target. Other targets receive the prompt content unchanged. +**Copilot CLI: env-var translation in `mcp-config.json`:** + +When installing MCP servers for the `copilot` target, env-var placeholders in `apm.yml` are **translated** to Copilot CLI's native runtime substitution syntax (`${VAR}`) instead of being resolved to literal values at install time. This applies to HTTP `headers`, stdio `env` blocks, and stdio `args`. + +| `apm.yml` syntax | Written to `~/.copilot/mcp-config.json` | +|---------------------|------------------------------------------| +| `${env:VAR}` | `${VAR}` | +| `${VAR}` | `${VAR}` (passthrough) | +| `` | `${VAR}` (with deprecation warning) | +| `${VAR:-default}` | `${VAR:-default}` (passthrough) | +| `${input:foo}` | `${input:foo}` (passthrough) | + +Copilot CLI resolves these at server-start from the host environment, so plaintext secrets are never written to disk. After install, `apm` emits an aggregated summary: + +- A **security improvement** warning when overwriting a config that previously stored literal env values, listing the affected variable names. +- An **unset env var** warning listing every referenced variable not currently exported in your shell, with a copy-pasteable `export KEY=...` hint. +- A one-line **deprecation** warning when any server still uses the legacy `` syntax. + +Other targets (`cursor`, `windsurf`, `opencode`, `claude`, `gemini`) continue to resolve env-var placeholders at install time pending per-adapter audits. + **Local `.apm/` Content Deployment:** After integrating dependencies, `apm install` deploys primitives from the project's own `.apm/` directory (instructions, prompts, agents, skills, hooks, commands) to target directories (`.github/`, `.claude/`, `.cursor/`, etc.). Local content takes priority over dependencies on collision. Deployed files are tracked in the lockfile for cleanup on subsequent installs. This works even with zero dependencies -- just `apm.yml` and `.apm/` content is enough. diff --git a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md index 82e13c375..75fd09a98 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/dependencies.md +++ b/packages/apm-guide/.apm/skills/apm-usage/dependencies.md @@ -167,11 +167,13 @@ dependencies: headers: X-Custom: "value" # Env-var placeholders in headers/env values: - # ${VAR} or ${env:VAR} -> resolved from host env at install time - # by Copilot (VS Code resolves at runtime; - # Codex passes ${...} through unchanged) + # ${VAR} or ${env:VAR} -> Copilot CLI: preserved as ${VAR} and resolved + # from host env at server-start (no plaintext on disk). + # VS Code: rewritten to ${env:VAR} and resolved at runtime. + # Cursor/Windsurf/OpenCode/Claude/Gemini: resolved at install time. + # Codex: passed through unchanged. # ${input:} -> VS Code prompts user at runtime - # -> legacy Copilot syntax (still supported) + # -> deprecated; auto-translated, emits a warning Authorization: "Bearer ${MY_TOKEN}" tools: ["repos", "issues"] diff --git a/src/apm_cli/adapters/client/claude.py b/src/apm_cli/adapters/client/claude.py index 487740299..448b47164 100644 --- a/src/apm_cli/adapters/client/claude.py +++ b/src/apm_cli/adapters/client/claude.py @@ -42,6 +42,12 @@ class ClaudeClientAdapter(CopilotClientAdapter): target_name: str = "claude" mcp_servers_key: str = "mcpServers" + # Claude Desktop / Code's mcp config does NOT support runtime env-var + # substitution -- the value in ``env`` must be a literal string. This + # adapter MUST keep the legacy install-time resolution behaviour. + # See #1152 supply-chain analysis. + _supports_runtime_env_substitution: bool = False + @staticmethod def _normalize_mcp_entry_for_claude_code(entry: dict) -> dict: """Normalize a server entry to Claude Code's on-disk shape. diff --git a/src/apm_cli/adapters/client/copilot.py b/src/apm_cli/adapters/client/copilot.py index 5445b4b60..e7897a9dd 100644 --- a/src/apm_cli/adapters/client/copilot.py +++ b/src/apm_cli/adapters/client/copilot.py @@ -9,11 +9,15 @@ import os import re from pathlib import Path +from typing import ClassVar + +import click from ...core.docker_args import DockerArgsProcessor from ...core.token_manager import GitHubTokenManager from ...registry.client import SimpleRegistryClient from ...registry.integration import RegistryIntegration +from ...utils.console import _rich_warning from ...utils.github_host import is_github_hostname from .base import _ENV_VAR_RE, MCPClientAdapter @@ -27,6 +31,67 @@ # per-call cost. ``${input:...}`` is intentionally not matched here. _COPILOT_ENV_RE = re.compile(r"<([A-Z_][A-Z0-9_]*)>|" + _ENV_VAR_RE.pattern) +# Detects the legacy ```` placeholder syntax. Used both for translation +# and for emitting an aggregated deprecation warning, mirroring the analogous +# pattern in ``vscode.py``. +_LEGACY_ANGLE_VAR_RE = re.compile(r"<([A-Z_][A-Z0-9_]*)>") + + +def _translate_env_placeholder(value): + """Pure-textual translation of env-var placeholders to Copilot CLI's + native runtime substitution syntax (``${VAR}``). + + This is the security-critical helper for issue #1152: it MUST NOT read + ``os.environ`` and MUST NOT resolve placeholders to their literal values. + Copilot CLI resolves ``${VAR}`` from the host environment at server-start + time, so APM emits placeholders verbatim rather than baking secrets into + ``~/.copilot/mcp-config.json``. + + Translations: + ``${env:VAR}`` -> ``${VAR}`` (strip ``env:`` prefix) + ``${VAR}`` -> ``${VAR}`` (no-op) + ```` -> ``${VAR}`` (legacy syntax migration) + ``${VAR:-default}``-> passthrough (regex doesn't match) + ``$VAR`` (bare) -> passthrough (regex doesn't match) + ``${input:foo}`` -> passthrough (regex doesn't match) + non-string -> passthrough + + The translation is idempotent: applying it twice produces the same + result as applying it once. + """ + if not isinstance(value, str): + return value + + def _to_brace(match): + # group(1) = legacy ; group(2) = ${VAR} / ${env:VAR} + var_name = match.group(1) or match.group(2) + return "${" + var_name + "}" + + return _COPILOT_ENV_RE.sub(_to_brace, value) + + +def _extract_legacy_angle_vars(value): + """Return the set of legacy ```` names present in *value*. + + Used to aggregate deprecation warnings across all servers in a single + install run, so authors see one helpful list instead of one warning per + occurrence. + """ + if not isinstance(value, str): + return set() + return set(_LEGACY_ANGLE_VAR_RE.findall(value)) + + +def _has_env_placeholder(value): + """True if *value* is a string containing any recognised env-var + placeholder syntax (``${VAR}``, ``${env:VAR}``, or legacy ````). + Used to distinguish placeholder-sourced env values (which translate) + from hardcoded literal defaults (which stay literal). + """ + if not isinstance(value, str): + return False + return bool(_COPILOT_ENV_RE.search(value)) + class CopilotClientAdapter(MCPClientAdapter): """Copilot CLI implementation of MCP client adapter. @@ -41,6 +106,42 @@ class CopilotClientAdapter(MCPClientAdapter): target_name: str = "copilot" mcp_servers_key: str = "mcpServers" + # When True, env-var placeholders (``${VAR}``, ``${env:VAR}``, legacy + # ````) are translated to Copilot CLI's native runtime-substitution + # syntax (``${VAR}``) and emitted into mcp-config.json verbatim. The + # secret never touches disk. + # + # When False, placeholders are resolved at install time against the host + # environment and the literal value is baked into the config file + # (legacy pre-#1152 behaviour). + # + # Subclasses (Cursor / Windsurf / OpenCode / Claude / Gemini) override + # this to ``False`` until their respective config formats are individually + # audited for runtime-substitution support. Critically, Claude Desktop's + # config format does NOT support runtime substitution -- it MUST keep + # resolving at install time. + _supports_runtime_env_substitution: bool = True + + # Process-wide aggregation of legacy ```` offenders, keyed by + # adapter class so subclasses (Cursor, etc.) maintain their own + # buckets. Populated by ``configure_mcp_server`` and drained by the + # post-install summary helper. Class-level so cross-server warnings + # work even when a fresh adapter instance is created per dep. + _legacy_angle_offenders_by_server: ClassVar[dict] = {} + # Process-wide aggregation of env-var keys whose values were previously + # baked as plaintext literals on disk and have just been rewritten to + # ``${KEY}`` placeholders. Drives the security-improvement notice. + _security_upgraded_keys: ClassVar[set] = set() + # Process-wide aggregation of env-var names referenced by configs that + # are NOT exported in the current shell. Drives the post-install + # actionable warning that lists vars the user must export before + # launching ``gh copilot``. + _unset_env_keys_by_server: ClassVar[dict] = {} + # Guard so the post-install summary is emitted at most once per CLI + # invocation, regardless of how many ``configure_mcp_server`` calls + # contributed to the aggregation buckets. + _install_run_summary_emitted: ClassVar[bool] = False + def __init__( self, registry_url=None, @@ -61,6 +162,14 @@ def __init__( super().__init__(project_root=project_root, user_scope=user_scope) self.registry_client = SimpleRegistryClient(registry_url) self.registry_integration = RegistryIntegration(registry_url) + # Per-server tracking of placeholder-sourced env-var keys, populated + # during ``_format_server_config`` and consumed by the post-install + # summary line. Keys: env-var names; never holds resolved values. + self._last_env_placeholder_keys = set() + # Per-server collection of legacy ```` offenders, populated by + # the resolution helpers and consumed by ``configure_mcp_server`` to + # feed the aggregated deprecation warning. + self._last_legacy_angle_vars = set() def get_config_path(self): """Get the path to the Copilot CLI MCP configuration file. @@ -154,6 +263,26 @@ def configure_mcp_server( print(f"Error: MCP server '{server_url}' not found in registry") return False + # Reset per-server tracking before formatting (so the per-server + # summary line and aggregated diagnostics reflect this server only). + self._last_env_placeholder_keys = set() + self._last_legacy_angle_vars = set() + + # Detect security upgrade: was the previous on-disk config for + # this server holding literal (resolved) values for env keys + # we are about to replace with ${KEY} placeholders? If so, + # remember the affected keys for the post-install notice. We + # snapshot BEFORE writing the new config. + previously_baked_keys = set() + previously_baked_headers = False + if self._supports_runtime_env_substitution: + previously_baked_keys, previously_baked_headers = ( + self._collect_previously_baked_keys(server_url, server_name) + ) + + # Generate server configuration with environment and runtime variable resolution + server_config = self._format_server_config(server_info, env_overrides, runtime_vars) + # Determine the server name for configuration key if server_name: # Use explicitly provided server name @@ -168,19 +297,193 @@ def configure_mcp_server( # Fallback to full server_url if no slash config_key = server_url - # Generate server configuration with environment and runtime variable resolution - server_config = self._format_server_config(server_info, env_overrides, runtime_vars) - # Update configuration using the chosen key self.update_config({config_key: server_config}) - print(f"Successfully configured MCP server '{config_key}' for {self._client_label}") + # Aggregate diagnostics for the post-install summary. + if self._supports_runtime_env_substitution: + if self._last_legacy_angle_vars: + self._legacy_angle_offenders_by_server[config_key] = set( + self._last_legacy_angle_vars + ) + # Only flag a security upgrade when the previously baked keys + # actually overlap with what we are now placeholderizing -- OR + # when the previous on-disk state had baked HTTP header + # literals (which don't expose env-var names directly, so we + # surface every newly-placeholderised key for this server). + upgraded = previously_baked_keys & self._last_env_placeholder_keys + if previously_baked_headers and self._last_env_placeholder_keys: + upgraded = upgraded | self._last_env_placeholder_keys + if upgraded: + self._security_upgraded_keys.update(upgraded) + + # Per-server install line with env-var summary parenthetical. + self._emit_install_summary(config_key, server_config) return True except Exception as e: print(f"Error configuring MCP server: {e}") return False + def _collect_previously_baked_keys(self, server_url, server_name): + """Return ``(env_keys, headers_were_baked)`` for the existing on-disk + entry: the set of env-block keys whose values are literal + (non-placeholder) strings, and a flag indicating whether the headers + block contained any literal values. Together these drive the + security-improvement notice. Headers don't expose env-var names + directly, so the caller unions current-write placeholder keys when + ``headers_were_baked`` is True. + """ + try: + current = self.get_current_config() + except Exception: + return set(), False + servers = current.get("mcpServers") or {} + # Match the same key resolution rule used below. + if server_name: + key = server_name + elif "/" in server_url: + key = server_url.split("/")[-1] + else: + key = server_url + existing = servers.get(key) + if not isinstance(existing, dict): + return set(), False + baked_env_keys = set() + env_block = existing.get("env") or {} + if isinstance(env_block, dict): + for k, v in env_block.items(): + if isinstance(v, str) and v.strip() and not _has_env_placeholder(v): + baked_env_keys.add(k) + headers_were_baked = False + headers_block = existing.get("headers") or {} + if isinstance(headers_block, dict): + for v in headers_block.values(): + if isinstance(v, str) and v.strip() and not _has_env_placeholder(v): + headers_were_baked = True + break + return baked_env_keys, headers_were_baked + + def _emit_install_summary(self, config_key, server_config): + """Record env-var references for the post-install aggregated + summary. No per-server line is emitted here; the integrator's + tree (``| + {name} -> Copilot (configured)``) is the success + signal. The summary references env-var names only -- never their + values. + """ + if not self._supports_runtime_env_substitution: + return + keys = set(self._last_env_placeholder_keys) + if isinstance(server_config, dict): + for block_key in ("env", "headers"): + block = server_config.get(block_key) + if not isinstance(block, dict): + continue + for value in block.values(): + if isinstance(value, str): + for match in _ENV_VAR_RE.finditer(value): + keys.add(match.group(1)) + unset = sorted(name for name in keys if not os.environ.get(name)) + if unset: + self.__class__._unset_env_keys_by_server.setdefault(config_key, []).extend( + u + for u in unset + if u not in self.__class__._unset_env_keys_by_server.get(config_key, []) + ) + + @classmethod + def emit_install_run_summary(cls): + """Emit aggregated cross-server diagnostics at the end of an install + run. Idempotent: subsequent calls within the same process are no-ops. + + Three diagnostics are emitted (when applicable): + + 1. Security improvement notice -- when the install rewrote + previously baked literal env values to runtime placeholders. + Emitted as a warning because it is an action item (the user + must export the affected vars). + 2. Aggregated unset-env warning -- when one or more configured + servers reference env vars that are not currently exported. + Includes a copy-pasteable ``export`` hint. + 3. Aggregated legacy ```` deprecation warning -- one line + naming all affected servers, mirroring the established VS Code + adapter pattern. + + State is drained after emission so a subsequent install run in + the same process (e.g. tests) starts clean. + """ + if cls._install_run_summary_emitted: + return + + # Visual separator from the install tree's closing line so the + # post-tree summary block reads as a distinct section. + emitted_any = False + + def _emit_separator_once(): + nonlocal emitted_any + if not emitted_any: + click.echo("") + emitted_any = True + + if cls._security_upgraded_keys: + visible = sorted(cls._security_upgraded_keys) + count = len(visible) + noun = "variable" if count == 1 else "variables" + affected = ", ".join(visible) + _emit_separator_once() + _rich_warning( + f"Security improvement: {count} environment {noun} previously stored as " + f"plaintext in the Copilot config are now resolved at runtime.\n" + f" Affected: {affected}\n" + f" Ensure these are exported in your shell before running 'gh copilot'", + symbol="warning", + ) + if cls._unset_env_keys_by_server: + all_unset: set[str] = set() + for names in cls._unset_env_keys_by_server.values(): + all_unset.update(names) + sorted_unset = sorted(all_unset) + export_hint = " ".join(f"{name}=..." for name in sorted_unset) + count = len(sorted_unset) + noun = "variable" if count == 1 else "variables" + _emit_separator_once() + _rich_warning( + f"Copilot CLI will resolve {count} environment {noun} at runtime " + f"that {'is' if count == 1 else 'are'} not currently set: " + f"{', '.join(sorted_unset)}.\n" + f" Export {'it' if count == 1 else 'them'} in your shell before " + f"running 'gh copilot', e.g.:\n" + f" export {export_hint}", + symbol="warning", + ) + # Deprecation notice is informational housekeeping (not a runtime + # blocker), but it ships unguarded for now so legacy usage + # remains visible until the v1.0 removal. If --quiet gating is + # added in future, the unset-env and security warnings above must + # remain unsuppressible because they describe action-required state. + if cls._legacy_angle_offenders_by_server: + servers = sorted(cls._legacy_angle_offenders_by_server.keys()) + count = len(servers) + noun = "server" if count == 1 else "servers" + _emit_separator_once() + _rich_warning( + f"Deprecated: placeholder syntax used in {count} {noun} " + f"({', '.join(servers)}). Migrate to ${{VAR}} in apm.yml. " + f" support will be removed in v1.0.", + symbol="warning", + ) + cls._install_run_summary_emitted = True + + @classmethod + def reset_install_run_state(cls): + """Reset the process-wide aggregation buckets. Intended for tests + and for explicitly starting a new install run within the same + process.""" + cls._legacy_angle_offenders_by_server = {} + cls._security_upgraded_keys = set() + cls._unset_env_keys_by_server = {} + cls._install_run_summary_emitted = False + def _format_server_config(self, server_info, env_overrides=None, runtime_vars=None): """Format server information into Copilot CLI MCP configuration format. @@ -202,14 +505,27 @@ def _format_server_config(self, server_info, env_overrides=None, runtime_vars=No "id": server_info.get("id", ""), # Add registry UUID for conflict detection } - # Self-defined stdio deps carry raw command/args -- use directly + # Self-defined stdio deps carry raw command/args -- use directly, + # but route values through the env-var translation/resolution pipeline + # so secrets are not baked into the persisted config when the harness + # supports runtime substitution (Copilot CLI). raw = server_info.get("_raw_stdio") if raw: config["command"] = raw["command"] - config["args"] = raw["args"] + resolved_env_for_args = {} if raw.get("env"): - config["env"] = raw["env"] + resolved_env_for_args = self._resolve_environment_variables( + raw["env"], env_overrides=env_overrides + ) + config["env"] = resolved_env_for_args self._warn_input_variables(raw["env"], server_info.get("name", ""), "Copilot CLI") + args = raw.get("args") or [] + config["args"] = [ + self._resolve_variable_placeholders(arg, resolved_env_for_args, runtime_vars) + if isinstance(arg, str) + else arg + for arg in args + ] # Apply tools override if present tools_override = server_info.get("_apm_tools_override") if tools_override: @@ -385,15 +701,93 @@ def _format_server_config(self, server_info, env_overrides=None, runtime_vars=No return config def _resolve_environment_variables(self, env_vars, env_overrides=None): - """Resolve environment variables to actual values. + """Resolve (or translate) declared environment variables. + + Behaviour depends on ``self._supports_runtime_env_substitution``: + + - True (Copilot CLI default): each declared env var ``NAME`` gets a + ``${NAME}`` placeholder that Copilot CLI resolves at server-start + from the host environment. Hardcoded literal defaults + (``GITHUB_TOOLSETS``, ``GITHUB_DYNAMIC_TOOLSETS``) stay literal + because they are not secrets and provide essential server + configuration. The host environment is NOT read; secrets never + touch disk. See issue #1152 for context. + + - False (legacy / sibling-adapter behaviour): resolve each variable + to its literal value via ``env_overrides`` -> ``os.environ`` -> + optional interactive prompt, baking the result into the config. Args: - env_vars (list): List of environment variable definitions from server info. - env_overrides (dict, optional): Pre-collected environment variable overrides. + env_vars (list): List of environment variable definitions from + server info (each item is ``{name, description, required}``). + env_overrides (dict, optional): Pre-collected environment + variable overrides. Ignored in translate mode. Returns: - dict: Dictionary of resolved environment variables. + dict: ``{name: value}`` -- placeholder string in translate mode, + literal value in legacy mode. """ + # Hardcoded literal defaults that supply essential server behaviour + # rather than secrets. These stay literal in translate mode so that + # tool-selection still works without a user export step. + default_github_env = {"GITHUB_TOOLSETS": "context", "GITHUB_DYNAMIC_TOOLSETS": "1"} + + # Self-defined stdio deps pass ``env`` as a plain dict + # ({NAME: value-or-placeholder}); registry-sourced deps pass a list + # of {name, description, required} dicts. Translate-mode handling + # for the dict shape: each value is either already a placeholder + # (translate it to the canonical ${VAR} form) or a literal (record + # the key as a placeholder reference and emit ${NAME} so the + # value never lands on disk). See issue #1152. + if isinstance(env_vars, dict) and self._supports_runtime_env_substitution: + translated = {} + placeholder_keys = [] + for name, raw_value in env_vars.items(): + if not name: + continue + if not isinstance(raw_value, str): + translated[name] = raw_value + continue + if _has_env_placeholder(raw_value): + self._last_legacy_angle_vars.update(_extract_legacy_angle_vars(raw_value)) + translated[name] = _translate_env_placeholder(raw_value) + # Record every ${VAR} in the translated value (handles + # both ${env:VAR} -> ${VAR} and bare ${VAR} cases). + for match in _ENV_VAR_RE.finditer(translated[name]): + placeholder_keys.append(match.group(1)) + elif name in default_github_env and raw_value == default_github_env[name]: + translated[name] = raw_value + else: + # Literal value present in apm.yml -- replace with a + # runtime placeholder so the secret never touches disk. + translated[name] = "${" + name + "}" + placeholder_keys.append(name) + self._last_env_placeholder_keys = set(placeholder_keys) + return translated + + if self._supports_runtime_env_substitution: + resolved = {} + placeholder_keys = [] + for env_var in env_vars: + if not isinstance(env_var, dict): + continue + name = env_var.get("name", "") + if not name: + continue + if name in default_github_env: + # Non-secret literal default -- preserve as-is. + resolved[name] = default_github_env[name] + else: + # Emit a runtime-substitution placeholder; Copilot CLI + # resolves ``${NAME}`` from the host environment at + # server-start. APM never reads or stores the value. + resolved[name] = "${" + name + "}" + placeholder_keys.append(name) + # Record for the post-install summary line and the + # security-improvement notice. + self._last_env_placeholder_keys = set(placeholder_keys) + return resolved + import os import sys @@ -416,10 +810,6 @@ def _resolve_environment_variables(self, env_vars, env_overrides=None): if not is_interactive: skip_prompting = True - # Add default GitHub MCP server environment variables for essential functionality first - # This ensures variables have defaults when user provides empty values or they're optional - default_github_env = {"GITHUB_TOOLSETS": "context", "GITHUB_DYNAMIC_TOOLSETS": "1"} - # Track which variables were explicitly provided with empty values (user wants defaults) empty_value_vars = set() if env_overrides: @@ -465,16 +855,42 @@ def _resolve_environment_variables(self, env_vars, env_overrides=None): return resolved def _resolve_env_variable(self, name, value, env_overrides=None): - """Resolve a single environment variable value. + """Resolve (or translate) a single environment variable value. + + Behaviour depends on ``self._supports_runtime_env_substitution``: + + - True (Copilot CLI default): translate placeholders to Copilot CLI's + native runtime substitution syntax (``${VAR}``). The host + environment is NOT read; the secret never touches disk. See issue + #1152 for context. Legacy ```` offenders are tracked for the + aggregated deprecation warning emitted by + ``configure_mcp_server``. + + - False (legacy / sibling-adapter behaviour): resolve placeholders + to literal values via ``env_overrides`` -> ``os.environ`` -> + optional interactive prompt, baking the result into the config. Args: name (str): Environment variable name. value (str): Environment variable value or placeholder. - env_overrides (dict, optional): Pre-collected environment variable overrides. + env_overrides (dict, optional): Pre-collected environment + variable overrides. Ignored in translate mode. Returns: - str: Resolved environment variable value. + str: Translated placeholder (translate mode) or resolved + literal value (legacy mode). """ + if self._supports_runtime_env_substitution: + # Track legacy offenders for the aggregated deprecation + # warning. Translation itself is a pure-textual rewrite. + self._last_legacy_angle_vars.update(_extract_legacy_angle_vars(value)) + # Track env-var names referenced via this header/value so the + # security-upgrade detector and per-server summary can see + # them (the env-block path tracks via _resolve_environment_variables). + for match in _ENV_VAR_RE.finditer(value): + self._last_env_placeholder_keys.add(match.group(1)) + return _translate_env_placeholder(value) + import sys from rich.prompt import Prompt @@ -683,15 +1099,31 @@ def _process_arguments(self, arguments, resolved_env=None, runtime_vars=None): return processed def _resolve_variable_placeholders(self, value, resolved_env, runtime_vars): - """Resolve both environment and runtime variable placeholders in values. + """Resolve runtime template variables and translate or resolve env-var + placeholders in argument strings. + + Behaviour depends on ``self._supports_runtime_env_substitution``: + + - True (Copilot CLI default): env-var placeholders (````, + ``${VAR}``, ``${env:VAR}``) are translated to ``${VAR}`` for + runtime substitution by Copilot CLI. APM template variables + (``{runtime_var}``) are still resolved at install time because + they are an APM-internal concept Copilot cannot interpret. + + - False (legacy / sibling-adapter behaviour): legacy ```` + placeholders are resolved against ``resolved_env`` (the dict of + literal env-var values), and ``{runtime_var}`` against + ``runtime_vars``. Newer ``${VAR}`` / ``${env:VAR}`` syntaxes are + left as-is for backward compatibility. Args: - value (str): Value that may contain placeholders like or {ado_org} - resolved_env (dict): Dictionary of resolved environment variables. + value (str): Value that may contain placeholders. + resolved_env (dict): Dictionary of resolved env vars (legacy + mode) or placeholder strings (translate mode). runtime_vars (dict): Dictionary of resolved runtime variables. Returns: - str: Processed value with actual variable values. + str: Processed value with placeholders translated or resolved. """ import re @@ -700,23 +1132,33 @@ def _resolve_variable_placeholders(self, value, resolved_env, runtime_vars): processed = str(value) - # Replace with actual values from resolved_env (for Docker env vars) - env_pattern = r"<([A-Z_][A-Z0-9_]*)>" - - def replace_env_var(match): - env_name = match.group(1) - return resolved_env.get(env_name, match.group(0)) # Return original if not found - - processed = re.sub(env_pattern, replace_env_var, processed) - - # Replace {runtime_var} with actual values from runtime_vars (for NPM args) - runtime_pattern = r"\{([a-zA-Z_][a-zA-Z0-9_]*)\}" - - def replace_runtime_var(match): - var_name = match.group(1) - return runtime_vars.get(var_name, match.group(0)) # Return original if not found - - processed = re.sub(runtime_pattern, replace_runtime_var, processed) + if self._supports_runtime_env_substitution: + # Track legacy offenders before translating them away. + self._last_legacy_angle_vars.update(_extract_legacy_angle_vars(processed)) + # Translate all three env-var placeholder syntaxes to ${VAR}. + processed = _translate_env_placeholder(processed) + else: + # Replace with actual values from resolved_env (for Docker env vars) + env_pattern = r"<([A-Z_][A-Z0-9_]*)>" + + def replace_env_var(match): + env_name = match.group(1) + return resolved_env.get(env_name, match.group(0)) # Return original if not found + + processed = re.sub(env_pattern, replace_env_var, processed) + + # Replace {runtime_var} with actual values from runtime_vars (for NPM args). + # Negative lookbehind on `$` so we never re-substitute inside an already-translated + # ${VAR} env placeholder (the brace is part of a Copilot CLI runtime substitution, + # not an APM template variable). + if runtime_vars: + runtime_pattern = r"(? apm install --target copilot -> ~/.copilot/mcp-config.json + +The unit tests in tests/unit/test_copilot_adapter.py cover translation in +isolation; this test pins the integration boundary so plaintext secrets +cannot regress back onto disk. + +Also includes a Cursor regression trap: Cursor's adapter is pinned to +the legacy install-time resolution behaviour (per the design contract +in copilot.py) until its config format is individually audited. That +adapter MUST keep producing literal values; this test fails loudly if +the Copilot translation accidentally bleeds into Cursor. +""" + +import json +import os +import shutil +import subprocess +from pathlib import Path + +import pytest +import yaml + + +@pytest.fixture +def apm_command(): + apm_on_path = shutil.which("apm") + if apm_on_path: + return apm_on_path + venv_apm = Path(__file__).parent.parent.parent / ".venv" / "bin" / "apm" + if venv_apm.exists(): + return str(venv_apm) + return "apm" + + +def _write_apm_yml(project_dir, mcp_servers): + config = { + "name": "mcp-env-vars-copilot-e2e", + "version": "1.0.0", + "dependencies": {"apm": [], "mcp": mcp_servers}, + } + (project_dir / "apm.yml").write_text( + yaml.dump(config, default_flow_style=False), encoding="utf-8" + ) + + +class TestMcpEnvVarHeadersCopilot: + """#1152 regression: Copilot mcp-config.json must contain ``${VAR}`` + runtime placeholders for env-var references in apm.yml. The literal + values from the installer's environment must NEVER appear on disk. + """ + + def test_self_defined_http_server_translates_env_vars_not_resolves(self, tmp_path, apm_command): + """``${VAR}`` and ``${env:VAR}`` syntaxes in apm.yml headers must + land in mcp-config.json as ``${VAR}`` (Copilot CLI's native runtime + substitution syntax). No host env values may leak into the file. + """ + project_dir = tmp_path / "project" + project_dir.mkdir() + # Copilot target signal: presence of .github/ activates the + # copilot/vscode runtime detection chain. + (project_dir / ".github").mkdir() + + # Isolated HOME so we don't touch the developer's real + # ~/.copilot/mcp-config.json. The Copilot adapter uses + # ``Path.home() / ".copilot"``. + fake_home = tmp_path / "home" + fake_home.mkdir() + + _write_apm_yml( + project_dir, + [ + { + "name": "test-http-server", + "registry": False, + "transport": "http", + "url": "https://example.com/mcp", + "headers": { + "Authorization": "Bearer ${MY_BEARER_TOKEN}", + "X-Api-Key": "${env:MY_API_KEY}", + }, + } + ], + ) + + env = os.environ.copy() + env["HOME"] = str(fake_home) + # Sentinel values: if these strings ever appear in the rendered + # config, the security contract has regressed. + env["MY_BEARER_TOKEN"] = "should-not-appear-in-copilot-json" + env["MY_API_KEY"] = "should-not-appear-in-copilot-json" + env["GIT_TERMINAL_PROMPT"] = "0" + env["APM_NON_INTERACTIVE"] = "1" + + result = subprocess.run( + [apm_command, "install", "--target", "copilot"], + cwd=project_dir, + capture_output=True, + text=True, + timeout=120, + env=env, + ) + + assert result.returncode == 0, ( + f"apm install failed (rc={result.returncode}).\n" + f"STDOUT:\n{result.stdout}\nSTDERR:\n{result.stderr}" + ) + + mcp_config = fake_home / ".copilot" / "mcp-config.json" + assert mcp_config.exists(), ( + f"Expected ~/.copilot/mcp-config.json to exist after install.\n" + f"STDOUT:\n{result.stdout}\nSTDERR:\n{result.stderr}" + ) + + config = json.loads(mcp_config.read_text(encoding="utf-8")) + servers = config.get("mcpServers") or {} + assert len(servers) == 1, ( + f"Expected 1 server in mcp-config.json, got: {list(servers.keys())}" + ) + server = next(iter(servers.values())) + headers = server.get("headers") or {} + + # ``${VAR}`` already Copilot-native: pass through unchanged. + assert headers.get("Authorization") == "Bearer ${MY_BEARER_TOKEN}", ( + f"Bare ${{VAR}} must remain ${{VAR}} for Copilot CLI.\nGot: {headers!r}" + ) + # ``${env:VAR}`` translated to ``${VAR}`` (env: prefix stripped). + assert headers.get("X-Api-Key") == "${MY_API_KEY}", ( + f"${{env:VAR}} must translate to ${{VAR}} for Copilot CLI.\nGot: {headers!r}" + ) + + # CRITICAL: no plaintext secret may appear anywhere in the file. + full_text = mcp_config.read_text(encoding="utf-8") + assert "should-not-appear-in-copilot-json" not in full_text, ( + "Copilot mcp-config.json leaked the literal env value -- " + "the install-time translation regressed and secrets are now " + "baked to disk.\n" + f"File contents:\n{full_text}" + ) + + def test_self_defined_stdio_server_translates_env_vars_in_args(self, tmp_path, apm_command): + """Self-defined stdio server with env-var placeholders in BOTH the + ``env`` block and ``args`` list must land in mcp-config.json with + ``${VAR}`` runtime placeholders. Closes the integration-tier gap + flagged by test-coverage review for the ``_raw_stdio`` branch of + ``_format_server_config`` and the supply-chain regression where + the dict-shaped ``env`` block was silently dropped to ``{}``. + """ + project_dir = tmp_path / "project" + project_dir.mkdir() + (project_dir / ".github").mkdir() + fake_home = tmp_path / "home" + fake_home.mkdir() + + _write_apm_yml( + project_dir, + [ + { + "name": "test-stdio-server", + "registry": False, + "transport": "stdio", + "command": "echo", + "args": [ + "--token=${env:MY_STDIO_TOKEN}", + "--bearer=${MY_STDIO_TOKEN}", + "--legacy=", + ], + "env": { + "PRIMARY_TOKEN": "${MY_STDIO_TOKEN}", + "PREFIXED_TOKEN": "${env:MY_STDIO_TOKEN}", + "LEGACY_TOKEN": "", + }, + } + ], + ) + + env = os.environ.copy() + env["HOME"] = str(fake_home) + env["MY_STDIO_TOKEN"] = "stdio-secret-must-not-leak" + env["MY_LEGACY_VAR"] = "legacy-secret-must-not-leak" + env["GIT_TERMINAL_PROMPT"] = "0" + env["APM_NON_INTERACTIVE"] = "1" + + result = subprocess.run( + [apm_command, "install", "--target", "copilot"], + cwd=project_dir, + capture_output=True, + text=True, + timeout=120, + env=env, + ) + + assert result.returncode == 0, ( + f"apm install failed (rc={result.returncode}).\n" + f"STDOUT:\n{result.stdout}\nSTDERR:\n{result.stderr}" + ) + + mcp_config = fake_home / ".copilot" / "mcp-config.json" + assert mcp_config.exists(), ( + f"Expected ~/.copilot/mcp-config.json to exist after install.\n" + f"STDOUT:\n{result.stdout}\nSTDERR:\n{result.stderr}" + ) + + config = json.loads(mcp_config.read_text(encoding="utf-8")) + servers = config.get("mcpServers") or {} + assert len(servers) == 1, ( + f"Expected 1 server in mcp-config.json, got: {list(servers.keys())}" + ) + server = next(iter(servers.values())) + + # env block: all three syntaxes translate to ${VAR}. + env_block = server.get("env") or {} + assert env_block.get("PRIMARY_TOKEN") == "${MY_STDIO_TOKEN}", ( + f"Bare ${{VAR}} in stdio env must remain ${{VAR}}.\nGot: {env_block!r}" + ) + assert env_block.get("PREFIXED_TOKEN") == "${MY_STDIO_TOKEN}", ( + f"${{env:VAR}} in stdio env must translate to ${{VAR}}.\nGot: {env_block!r}" + ) + assert env_block.get("LEGACY_TOKEN") == "${MY_LEGACY_VAR}", ( + f"Legacy in stdio env must translate to ${{VAR}}.\nGot: {env_block!r}" + ) + + # args list: all three syntaxes translate to ${VAR}. + args = server.get("args") or [] + assert "--token=${MY_STDIO_TOKEN}" in args, ( + f"${{env:VAR}} in stdio args must translate to ${{VAR}}.\nGot: {args!r}" + ) + assert "--bearer=${MY_STDIO_TOKEN}" in args, ( + f"Bare ${{VAR}} in stdio args must remain ${{VAR}}.\nGot: {args!r}" + ) + assert "--legacy=${MY_LEGACY_VAR}" in args, ( + f"Legacy in stdio args must translate to ${{VAR}}.\nGot: {args!r}" + ) + + # CRITICAL: neither secret may appear as a literal anywhere. + full_text = mcp_config.read_text(encoding="utf-8") + assert "stdio-secret-must-not-leak" not in full_text, ( + f"Copilot stdio config leaked MY_STDIO_TOKEN as plaintext.\nFile contents:\n{full_text}" + ) + assert "legacy-secret-must-not-leak" not in full_text, ( + f"Copilot stdio config leaked MY_LEGACY_VAR as plaintext.\nFile contents:\n{full_text}" + ) + + +class TestMcpEnvVarHeadersCursor: + """Sibling-adapter regression trap for #1152. + + Cursor's mcp.json runtime-substitution support has not yet been + individually audited, so its adapter is pinned to the legacy + install-time resolution behaviour. This test fails if that pin + accidentally lifts -- either by removing the + ``_supports_runtime_env_substitution = False`` override on + ``CursorClientAdapter`` or by changing the base class default in + a way that breaks Cursor. + """ + + def test_cursor_still_resolves_env_vars_to_literal(self, tmp_path, apm_command): + project_dir = tmp_path / "project" + project_dir.mkdir() + # Cursor target signal. + (project_dir / ".cursor").mkdir() + + _write_apm_yml( + project_dir, + [ + { + "name": "test-http-server", + "registry": False, + "transport": "http", + "url": "https://example.com/mcp", + "headers": { + "Authorization": "Bearer ${MY_BEARER_TOKEN}", + }, + } + ], + ) + + env = os.environ.copy() + env["MY_BEARER_TOKEN"] = "literal-cursor-value" + env["GIT_TERMINAL_PROMPT"] = "0" + env["APM_NON_INTERACTIVE"] = "1" + + result = subprocess.run( + [apm_command, "install", "--target", "cursor"], + cwd=project_dir, + capture_output=True, + text=True, + timeout=120, + env=env, + ) + + assert result.returncode == 0, ( + f"apm install failed (rc={result.returncode}).\n" + f"STDOUT:\n{result.stdout}\nSTDERR:\n{result.stderr}" + ) + + cursor_config = project_dir / ".cursor" / "mcp.json" + assert cursor_config.exists(), ( + f"Expected .cursor/mcp.json to exist after install.\n" + f"STDOUT:\n{result.stdout}\nSTDERR:\n{result.stderr}" + ) + + config = json.loads(cursor_config.read_text(encoding="utf-8")) + servers = config.get("mcpServers") or {} + assert len(servers) == 1, ( + f"Expected 1 server in cursor mcp.json, got: {list(servers.keys())}" + ) + server = next(iter(servers.values())) + headers = server.get("headers") or {} + + # Cursor MUST keep the legacy resolve-to-literal behaviour + # until a per-adapter audit lifts the pin. This guard fires + # if the Copilot fix accidentally bleeds into Cursor. + assert headers.get("Authorization") == "Bearer literal-cursor-value", ( + f"Cursor adapter unexpectedly stopped resolving env vars at " + f"install time. If this is intentional, update the design " + f"contract in copilot.py and remove this regression trap.\n" + f"Got: {headers!r}" + ) diff --git a/tests/unit/test_copilot_adapter.py b/tests/unit/test_copilot_adapter.py index c67e8767d..2a50d7000 100644 --- a/tests/unit/test_copilot_adapter.py +++ b/tests/unit/test_copilot_adapter.py @@ -149,16 +149,20 @@ def test_remote_skips_entries_without_url(self): self.assertEqual(config["url"], "https://good.example.com/sse") -class TestCopilotEnvVarResolutionInHeaders(unittest.TestCase): - """Issue #944: ``${VAR}`` and ``${env:VAR}`` in headers are install-time resolved. - - Copilot CLI's mcp-config.json has no runtime env interpolation, so APM bakes - the actual value in. The legacy ```` syntax already worked; these tests - cover the new ``${VAR}`` and ``${env:VAR}`` syntaxes added for #944. Together - with the existing ```` path, the three syntaxes share the same - env_overrides -> os.environ -> prompt resolution flow. +class TestCopilotEnvVarTranslationInHeaders(unittest.TestCase): + """Issue #1152: Copilot CLI adapter translates env-var placeholders to + its native runtime substitution syntax (``${VAR}``) instead of resolving + them to plaintext at install time. Per Copilot CLI documentation, + ``${VAR}``, ``$VAR`` and ``${VAR:-default}`` are evaluated at server + start, so install-time resolution unnecessarily bakes secrets to disk. """ + def setUp(self): + CopilotClientAdapter.reset_install_run_state() + + def tearDown(self): + CopilotClientAdapter.reset_install_run_state() + def _adapter(self): with ( patch("apm_cli.adapters.client.copilot.SimpleRegistryClient"), @@ -166,64 +170,77 @@ def _adapter(self): ): return CopilotClientAdapter() - def test_resolves_bare_dollar_brace_var(self): + def test_translate_env_prefix_to_bare(self): + """``${env:VAR}`` becomes ``${VAR}``; the ``env:`` prefix is stripped.""" adapter = self._adapter() with patch.dict(os.environ, {"MY_TOKEN": "secret-xyz"}, clear=False): result = adapter._resolve_env_variable( - "Authorization", "Bearer ${MY_TOKEN}", env_overrides=None + "Authorization", "Bearer ${env:MY_TOKEN}", env_overrides=None ) - self.assertEqual(result, "Bearer secret-xyz") + self.assertEqual(result, "Bearer ${MY_TOKEN}") + self.assertNotIn("secret-xyz", result) - def test_resolves_env_prefixed_var(self): - """``${env:VAR}`` (VS Code-flavored) also resolves to the host env value.""" + def test_translate_bare_brace_is_idempotent(self): + """``${VAR}`` is already Copilot-native; pass through unchanged.""" adapter = self._adapter() with patch.dict(os.environ, {"MY_TOKEN": "secret-xyz"}, clear=False): result = adapter._resolve_env_variable( - "Authorization", "Bearer ${env:MY_TOKEN}", env_overrides=None + "Authorization", "Bearer ${MY_TOKEN}", env_overrides=None ) - self.assertEqual(result, "Bearer secret-xyz") + self.assertEqual(result, "Bearer ${MY_TOKEN}") + self.assertNotIn("secret-xyz", result) - def test_legacy_angle_bracket_still_works(self): - """Regression: ```` legacy syntax must keep functioning.""" + def test_translation_ignores_os_environ(self): + """Canonical regression trap: translation MUST be pure-textual. + Even if the env var is set in the process, the result must be the + ``${VAR}`` reference -- never the literal value. + """ adapter = self._adapter() - with patch.dict(os.environ, {"MY_TOKEN": "secret-xyz"}, clear=False): + with patch.dict(os.environ, {"MY_TOKEN": "PLAINTEXT_DO_NOT_BAKE"}, clear=False): result = adapter._resolve_env_variable( - "Authorization", "Bearer ", env_overrides=None + "Authorization", "Bearer ${env:MY_TOKEN}", env_overrides=None ) - self.assertEqual(result, "Bearer secret-xyz") + self.assertNotIn("PLAINTEXT_DO_NOT_BAKE", result) + self.assertEqual(result, "Bearer ${MY_TOKEN}") - def test_env_overrides_take_precedence(self): - """``env_overrides`` wins over ``os.environ``, identical to legacy behavior.""" + def test_translation_ignores_env_overrides(self): + """``env_overrides`` is irrelevant in translation mode -- the value + is never resolved here, the variable name is preserved as-is. + """ adapter = self._adapter() - with patch.dict(os.environ, {"MY_TOKEN": "from-env"}, clear=False): - result = adapter._resolve_env_variable( - "Authorization", - "Bearer ${MY_TOKEN}", - env_overrides={"MY_TOKEN": "from-overrides"}, - ) - self.assertEqual(result, "Bearer from-overrides") + result = adapter._resolve_env_variable( + "Authorization", + "Bearer ${MY_TOKEN}", + env_overrides={"MY_TOKEN": "from-overrides"}, + ) + self.assertEqual(result, "Bearer ${MY_TOKEN}") + self.assertNotIn("from-overrides", result) - def test_unresolvable_passes_through(self): - """Unset vars survive verbatim in non-interactive (env_overrides supplied) mode.""" + def test_default_syntax_passes_through(self): + """``${VAR:-default}`` is Copilot-native default syntax; passthrough.""" adapter = self._adapter() - # Make sure target var is not in env - with patch.dict(os.environ, {}, clear=True): - result = adapter._resolve_env_variable( - "Authorization", - "Bearer ${MISSING_VAR}", - env_overrides={"OTHER": "x"}, # presence forces non-interactive path - ) - self.assertEqual(result, "Bearer ${MISSING_VAR}") + result = adapter._resolve_env_variable( + "Authorization", "Bearer ${MY_TOKEN:-anon}", env_overrides=None + ) + self.assertEqual(result, "Bearer ${MY_TOKEN:-anon}") + + def test_legacy_angle_translates_with_warning(self): + """```` legacy APM syntax translates to ``${VAR}`` and is + recorded for the post-install deprecation warning.""" + adapter = self._adapter() + adapter._last_legacy_angle_vars = set() + result = adapter._resolve_env_variable( + "Authorization", "Bearer ", env_overrides=None + ) + self.assertEqual(result, "Bearer ${MY_TOKEN}") + self.assertIn("MY_TOKEN", adapter._last_legacy_angle_vars) def test_input_syntax_is_not_resolved(self): - """``${input:...}`` must NOT be resolved here -- it's runtime-prompted by VS Code.""" + """``${input:...}`` is VS Code-only; passthrough untouched.""" adapter = self._adapter() - with patch.dict(os.environ, {"input": "should-not-match"}, clear=False): - result = adapter._resolve_env_variable( - "Authorization", - "Bearer ${input:my-token}", - env_overrides={"OTHER": "x"}, - ) + result = adapter._resolve_env_variable( + "Authorization", "Bearer ${input:my-token}", env_overrides=None + ) self.assertEqual(result, "Bearer ${input:my-token}") def test_github_actions_template_is_not_touched(self): @@ -232,35 +249,15 @@ def test_github_actions_template_is_not_touched(self): result = adapter._resolve_env_variable( "Authorization", "Bearer ${{ secrets.GITHUB_TOKEN }}", - env_overrides={"OTHER": "x"}, + env_overrides=None, ) self.assertEqual(result, "Bearer ${{ secrets.GITHUB_TOKEN }}") - def test_resolved_value_is_not_recursively_expanded(self): - """Regression guard: a resolved value containing placeholder-like text - must NOT be re-scanned for further substitution. - - Mirrors the original ````-only semantics where each placeholder is - resolved exactly once. Important for tokens/values that legitimately - contain ``${...}`` literal text (e.g. regex patterns, templated strings). - """ - adapter = self._adapter() - with patch.dict( - os.environ, - {"OUTER": "literal-${INNER}", "INNER": "should-not-appear"}, - clear=False, - ): - # Test all three placeholder syntaxes for symmetry. - for syntax in ("", "${OUTER}", "${env:OUTER}"): - with self.subTest(syntax=syntax): - result = adapter._resolve_env_variable( - "Authorization", syntax, env_overrides={"OTHER": "x"} - ) - self.assertEqual(result, "literal-${INNER}") - - def test_mixed_syntaxes_in_one_value(self): - """A header may mix legacy and new placeholders; all should resolve.""" + def test_mixed_syntaxes_translated_consistently(self): + """A header may mix legacy ```` and new ``${VAR}``/``${env:VAR}``; + all translate to ``${VAR}``, none resolve to literal values.""" adapter = self._adapter() + adapter._last_legacy_angle_vars = set() with patch.dict( os.environ, {"OLD": "old-val", "NEW": "new-val", "ENV_PREFIXED": "env-val"}, @@ -271,7 +268,325 @@ def test_mixed_syntaxes_in_one_value(self): "old= new=${NEW} env=${env:ENV_PREFIXED}", env_overrides=None, ) - self.assertEqual(result, "old=old-val new=new-val env=env-val") + self.assertEqual(result, "old=${OLD} new=${NEW} env=${ENV_PREFIXED}") + for plaintext in ("old-val", "new-val", "env-val"): + self.assertNotIn(plaintext, result) + + +class TestCopilotEnvTranslationStdioEnv(unittest.TestCase): + """Translation behaviour in stdio ``env`` block values.""" + + def _adapter(self): + with ( + patch("apm_cli.adapters.client.copilot.SimpleRegistryClient"), + patch("apm_cli.adapters.client.copilot.RegistryIntegration"), + ): + return CopilotClientAdapter() + + def test_env_block_value_translates_to_runtime_placeholder(self): + """An env-var declared in registry stdio env_vars with a placeholder + default emits ``${KEY}`` so Copilot CLI resolves it at server start. + """ + adapter = self._adapter() + env_vars = [ + {"name": "MY_TOKEN", "value": "${env:MY_TOKEN}"}, + ] + with patch.dict(os.environ, {"MY_TOKEN": "PLAINTEXT_DO_NOT_BAKE"}, clear=False): + resolved = adapter._resolve_environment_variables(env_vars, env_overrides=None) + self.assertEqual(resolved.get("MY_TOKEN"), "${MY_TOKEN}") + self.assertNotIn("PLAINTEXT_DO_NOT_BAKE", str(resolved)) + self.assertIn("MY_TOKEN", adapter._last_env_placeholder_keys) + + def test_github_toolsets_literal_default_preserved(self): + """Non-secret literal defaults (e.g. ``GITHUB_TOOLSETS=context``) + must remain literal so tools work without the user exporting them.""" + adapter = self._adapter() + env_vars = [ + {"name": "GITHUB_TOOLSETS", "value": "context"}, + ] + resolved = adapter._resolve_environment_variables(env_vars, env_overrides=None) + self.assertEqual(resolved.get("GITHUB_TOOLSETS"), "context") + + +class TestCopilotEnvTranslationStdioArgs(unittest.TestCase): + """Translation behaviour for placeholders in stdio command ``args``.""" + + def _adapter(self): + with ( + patch("apm_cli.adapters.client.copilot.SimpleRegistryClient"), + patch("apm_cli.adapters.client.copilot.RegistryIntegration"), + ): + return CopilotClientAdapter() + + def test_args_env_placeholder_translates(self): + """``--host=${env:H}`` becomes ``--host=${H}`` in the rendered args.""" + adapter = self._adapter() + with patch.dict(os.environ, {"H": "PLAINTEXT_DO_NOT_BAKE"}, clear=False): + result = adapter._resolve_variable_placeholders( + "--host=${env:H}", resolved_env={}, runtime_vars=None + ) + self.assertEqual(result, "--host=${H}") + self.assertNotIn("PLAINTEXT_DO_NOT_BAKE", result) + + def test_args_runtime_template_var_still_resolved(self): + """``{ado_org}`` template vars are APM-specific; resolved at install + time even in translate mode (Copilot can't see them).""" + adapter = self._adapter() + result = adapter._resolve_variable_placeholders( + "--org={ado_org}", resolved_env={}, runtime_vars={"ado_org": "myorg"} + ) + self.assertEqual(result, "--org=myorg") + + +class TestSiblingAdaptersUnchanged(unittest.TestCase): + """Regression trap: sibling adapters that inherit from + ``CopilotClientAdapter`` MUST keep the legacy install-time resolution + behaviour. Their ``_supports_runtime_env_substitution`` is pinned to + ``False`` until each is individually audited (#1152 follow-ups).""" + + def _adapter(self, cls): + with ( + patch("apm_cli.adapters.client.copilot.SimpleRegistryClient"), + patch("apm_cli.adapters.client.copilot.RegistryIntegration"), + ): + return cls() + + def test_cursor_still_resolves_to_literal(self): + from apm_cli.adapters.client.cursor import CursorClientAdapter + + adapter = self._adapter(CursorClientAdapter) + self.assertFalse(adapter._supports_runtime_env_substitution) + with patch.dict(os.environ, {"MY_TOKEN": "literal-value"}, clear=False): + result = adapter._resolve_env_variable( + "Authorization", "Bearer ${MY_TOKEN}", env_overrides=None + ) + self.assertEqual(result, "Bearer literal-value") + + def test_claude_still_resolves_to_literal(self): + """Claude Desktop config does NOT support runtime substitution -- + this adapter MUST keep resolving.""" + from apm_cli.adapters.client.claude import ClaudeClientAdapter + + adapter = self._adapter(ClaudeClientAdapter) + self.assertFalse(adapter._supports_runtime_env_substitution) + with patch.dict(os.environ, {"MY_TOKEN": "literal-value"}, clear=False): + result = adapter._resolve_env_variable( + "Authorization", "Bearer ${env:MY_TOKEN}", env_overrides=None + ) + self.assertEqual(result, "Bearer literal-value") + + def test_windsurf_still_resolves_to_literal(self): + from apm_cli.adapters.client.windsurf import WindsurfClientAdapter + + adapter = self._adapter(WindsurfClientAdapter) + self.assertFalse(adapter._supports_runtime_env_substitution) + with patch.dict(os.environ, {"MY_TOKEN": "literal-value"}, clear=False): + result = adapter._resolve_env_variable( + "Authorization", "Bearer ${MY_TOKEN}", env_overrides=None + ) + self.assertEqual(result, "Bearer literal-value") + + def test_opencode_still_resolves_to_literal(self): + from apm_cli.adapters.client.opencode import OpenCodeClientAdapter + + adapter = self._adapter(OpenCodeClientAdapter) + self.assertFalse(adapter._supports_runtime_env_substitution) + with patch.dict(os.environ, {"MY_TOKEN": "literal-value"}, clear=False): + result = adapter._resolve_env_variable( + "Authorization", "Bearer ${MY_TOKEN}", env_overrides=None + ) + self.assertEqual(result, "Bearer literal-value") + + def test_gemini_still_resolves_to_literal(self): + from apm_cli.adapters.client.gemini import GeminiClientAdapter + + adapter = self._adapter(GeminiClientAdapter) + self.assertFalse(adapter._supports_runtime_env_substitution) + with patch.dict(os.environ, {"MY_TOKEN": "literal-value"}, clear=False): + result = adapter._resolve_env_variable( + "Authorization", "Bearer ${MY_TOKEN}", env_overrides=None + ) + self.assertEqual(result, "Bearer literal-value") + + +class TestCopilotEnvVarTranslationInStdioEnvBlock(unittest.TestCase): + """Issue #1152 supply-chain regression trap: self-defined stdio deps + pass ``env`` as a plain dict ({NAME: value}), not a list of + {name, description, required} dicts. The translate-mode pipeline + must handle this shape without silently dropping the block to ``{}``. + """ + + def setUp(self): + CopilotClientAdapter.reset_install_run_state() + + def tearDown(self): + CopilotClientAdapter.reset_install_run_state() + + def test_dict_shaped_env_block_translates_all_placeholder_syntaxes(self): + adapter = CopilotClientAdapter() + result = adapter._resolve_environment_variables( + { + "PRIMARY_TOKEN": "${MY_STDIO_TOKEN}", + "PREFIXED_TOKEN": "${env:MY_STDIO_TOKEN}", + "LEGACY_TOKEN": "", + }, + env_overrides=None, + ) + self.assertEqual(result["PRIMARY_TOKEN"], "${MY_STDIO_TOKEN}") + self.assertEqual(result["PREFIXED_TOKEN"], "${MY_STDIO_TOKEN}") + self.assertEqual(result["LEGACY_TOKEN"], "${MY_LEGACY_VAR}") + + def test_dict_shaped_env_block_with_literal_replaced_by_runtime_placeholder(self): + adapter = CopilotClientAdapter() + with patch.dict(os.environ, {"MY_TOKEN": "ignored-os-env"}, clear=False): + result = adapter._resolve_environment_variables( + {"MY_TOKEN": "literal-value-from-apm-yml"}, env_overrides=None + ) + self.assertEqual(result["MY_TOKEN"], "${MY_TOKEN}") + for v in result.values(): + self.assertNotIn("literal-value-from-apm-yml", v) + self.assertNotIn("ignored-os-env", v) + + def test_dict_shaped_env_block_does_not_silently_drop(self): + """Regression trap for the bug where dict-input was iterated as a + list-of-dicts, every key failed isinstance(dict), and the result + was an empty {} -- breaking every self-defined stdio MCP server. + """ + adapter = CopilotClientAdapter() + result = adapter._resolve_environment_variables( + {"FOO": "${env:FOO}", "BAR": "${BAR}"}, env_overrides=None + ) + self.assertEqual(set(result.keys()), {"FOO", "BAR"}) + + +class TestCopilotInstallRunSummary(unittest.TestCase): + """Issue #1152: aggregated post-install diagnostics. + + ``emit_install_run_summary`` consolidates security-upgrade, + unset-env-var, and legacy-syntax diagnostics into a single + end-of-run block so the user sees one actionable summary even when + many servers were configured. + """ + + def setUp(self): + CopilotClientAdapter.reset_install_run_state() + + def tearDown(self): + CopilotClientAdapter.reset_install_run_state() + + def _adapter(self): + with ( + patch("apm_cli.adapters.client.copilot.SimpleRegistryClient"), + patch("apm_cli.adapters.client.copilot.RegistryIntegration"), + ): + return CopilotClientAdapter() + + def test_security_upgrade_warning_when_baked_keys_detected(self): + """A server config that previously had literal env values triggers + a single security-improvement warning naming the affected keys. + """ + adapter = self._adapter() + # Simulate a previously-baked config on disk. + with patch.object( + CopilotClientAdapter, + "_collect_previously_baked_keys", + return_value=({"GITHUB_TOKEN", "LINEAR_KEY"}, False), + ): + adapter._collect_previously_baked_keys.__call__ # noqa: B018 - sanity + CopilotClientAdapter._security_upgraded_keys.update({"GITHUB_TOKEN", "LINEAR_KEY"}) + + with patch("apm_cli.adapters.client.copilot._rich_warning") as mock_warn: + CopilotClientAdapter.emit_install_run_summary() + + joined = "\n".join(call.args[0] for call in mock_warn.call_args_list) + self.assertIn("Security improvement", joined) + self.assertIn("GITHUB_TOKEN", joined) + self.assertIn("LINEAR_KEY", joined) + + def test_security_upgrade_detects_baked_http_header_literals(self): + """Regression trap: a previously-baked HTTP header literal (which + does not expose the env-var name) must still trigger the + security-improvement notice for whatever placeholder keys the new + write introduces. The pre-fix path produced an empty intersection + because the headers branch added a sentinel string instead of a + real env-var name. + """ + adapter = self._adapter() + # Simulate previous on-disk state: env block had no baked literals + # (empty set), but headers block did (True). Combined with new + # placeholder keys for this write, the upgrade notice MUST list + # the new keys -- they are the vars the user must export. + with patch.object( + CopilotClientAdapter, + "_collect_previously_baked_keys", + return_value=(set(), True), + ): + adapter._last_env_placeholder_keys = {"GH_TOKEN"} + previously_baked_keys, previously_baked_headers = ( + adapter._collect_previously_baked_keys("github/server", "github-mcp") + ) + self.assertEqual(previously_baked_keys, set()) + self.assertTrue(previously_baked_headers) + # Mirror configure_mcp_server's upgrade-detection logic. + upgraded = previously_baked_keys & adapter._last_env_placeholder_keys + if previously_baked_headers and adapter._last_env_placeholder_keys: + upgraded = upgraded | adapter._last_env_placeholder_keys + self.assertEqual(upgraded, {"GH_TOKEN"}) + CopilotClientAdapter._security_upgraded_keys.update(upgraded) + + with patch("apm_cli.adapters.client.copilot._rich_warning") as mock_warn: + CopilotClientAdapter.emit_install_run_summary() + joined = "\n".join(call.args[0] for call in mock_warn.call_args_list) + self.assertIn("Security improvement", joined) + self.assertIn("GH_TOKEN", joined) + # The legacy "(http header value)" sentinel must NOT appear. + self.assertNotIn("(http header value)", joined) + + def test_unset_env_warning_aggregates_across_servers(self): + """Two servers contributing different unset env vars produce a + single aggregated warning with a copy-pasteable export hint. + """ + CopilotClientAdapter._unset_env_keys_by_server["github-mcp"] = ["GH_TOKEN"] + CopilotClientAdapter._unset_env_keys_by_server["linear"] = ["LINEAR_KEY"] + with patch("apm_cli.adapters.client.copilot._rich_warning") as mock_warn: + CopilotClientAdapter.emit_install_run_summary() + joined = "\n".join(call.args[0] for call in mock_warn.call_args_list) + self.assertIn("GH_TOKEN", joined) + self.assertIn("LINEAR_KEY", joined) + self.assertIn("export GH_TOKEN=... LINEAR_KEY=...", joined) + + def test_summary_emitted_only_once(self): + """Calling ``emit_install_run_summary`` twice in the same process + emits the diagnostics exactly once (idempotent guard).""" + CopilotClientAdapter._unset_env_keys_by_server["s"] = ["X"] + with patch("apm_cli.adapters.client.copilot._rich_warning") as mock_warn: + CopilotClientAdapter.emit_install_run_summary() + CopilotClientAdapter.emit_install_run_summary() + self.assertEqual(mock_warn.call_count, 1) + + def test_unset_env_emit_summary_records_keys(self): + """``_emit_install_summary`` populates the unset-env bucket for + keys not present in ``os.environ``; set keys are not recorded. + """ + adapter = self._adapter() + adapter._last_env_placeholder_keys = {"DEFINITELY_NOT_SET_VAR_XYZ"} + with patch.dict(os.environ, {}, clear=False): + os.environ.pop("DEFINITELY_NOT_SET_VAR_XYZ", None) + adapter._emit_install_summary("svc", {"type": "local"}) + self.assertIn("svc", CopilotClientAdapter._unset_env_keys_by_server) + self.assertIn( + "DEFINITELY_NOT_SET_VAR_XYZ", + CopilotClientAdapter._unset_env_keys_by_server["svc"], + ) + + def test_set_env_var_not_recorded_as_unset(self): + """When the env var IS exported, the unset bucket is not + populated for that server.""" + adapter = self._adapter() + adapter._last_env_placeholder_keys = {"PRESENT_VAR"} + with patch.dict(os.environ, {"PRESENT_VAR": "value"}, clear=False): + adapter._emit_install_summary("svc", {"type": "local"}) + self.assertNotIn("svc", CopilotClientAdapter._unset_env_keys_by_server) class TestCopilotSelectRemoteWithUrl(unittest.TestCase):