MCP: smart per-runtime sourcing for dual-mode registry entries + GitHub auth host validation

[danielmeppiel]

Summary

Surfaced during PR #810 panel review (rubber-duck pass on a proposed README simplification). Two related items, scoped together because the security fix is a precondition for safely changing default selection.

Item 1: Smart per-runtime sourcing for dual-mode entries

When a registry entry advertises both packages (e.g. docker stdio) and remotes (HTTPS), adapter selection is currently non-deterministic across clients:

• Copilot (src/apm_cli/adapters/client/copilot.py:186-236): prefers remotes first.
• VS Code (src/apm_cli/adapters/client/vscode.py:227-345): prefers packages first.
• Codex (src/apm_cli/adapters/client/codex.py:122-131): falls through to packages if both exist; only skips when remotes and not packages.

This forces the README to use transport: http as a deterministic-selection workaround, which in turn introduced the UX confusion that PR #810 patched with inline disambiguators (# MCP transport name, not URL scheme -- connects over HTTPS).

Naive fix ("strip packages when both exist + no overlay"): blocked because (a) Codex would skip with 'remote not supported' on previously-working installs, (b) VS Code would silently flip from stdio to remote.

Proper fix needs per-runtime sourcing rules -- a small policy table per adapter declaring its preference order and whether it supports remote/stdio at all. The smart-default then becomes 'pick the first variant the runtime supports' rather than a global mutation.

Acceptance criteria

• Each adapter exposes a declared transport-preference order and capability set (e.g. Codex: stdio-only; Copilot: remote-first then stdio; VS Code: stdio-first then remote).
• Pre-fetch normalization step uses each adapter's declared preference to pick a single variant per runtime.
• README example (and docs) can drop transport: http overlay -- bare io.github.github/github-mcp-server works deterministically across all runtimes (Copilot uses remote, VS Code uses docker, Codex uses docker, etc).
• Behavior change CHANGELOG entry under ### Changed.
• No interactive token prompt on first-run with the GitHub MCP server when only Copilot is installed (bare shorthand should pick remote on Copilot without prompting).

Item 2: _is_github_server host validation

src/apm_cli/adapters/client/copilot.py:675-717 returns True (and triggers GitHub token injection at copilot.py:201-212) on either name match OR hostname match. A poisoned/custom registry entry named github-mcp-server pointing at https://evil.example.com/mcp/ would receive a GitHub token at the non-GitHub host.

This is latent today (only known registry entries with that name are GitHub's own) but tightly coupled to Item 1: if the smart-default expands the remote-path footprint, this risk grows.

Acceptance criteria

• _is_github_server requires BOTH name match AND is_github_hostname(parsed_url.hostname) returning True before injecting a GitHub token. Name match alone is no longer sufficient.
• Unit test: poisoned-name + non-GitHub URL does not get a token header.
• Unit test: legitimate GitHub server (github-mcp-server at api.githubcopilot.com) still gets the token.
• Threat-model sentence in docs/.../mcp-servers.md Security section.

Why these are bundled

• (1) without (2) is a security regression amplification.
• (2) without (1) is fine to ship standalone if (1) takes longer.

Related

• PR feat(install): add --mcp flag for declaratively adding MCP servers to apm.yml #810 -- shipped the docs-only band-aid (transport: http + inline disambiguators).
• Closes feat(cli): extend 'apm install --mcp' for declarative MCP server addition #807, bug: apm mcp search/list/show ignore MCP_REGISTRY_URL env var #813, security: validate MCP_REGISTRY_URL and reject http:// overrides at SimpleRegistryClient #814 already handled in PR feat(install): add --mcp flag for declaratively adding MCP servers to apm.yml #810.

[github-actions]

Triage decision

needs-design

Proposed labels

theme/security
area/mcp-config
area/mcp-trust
type/bug
status/needs-design
priority/high

Milestone

null

Suggested next action

Design the per-adapter transport-preference interface (policy table shape, adapter capability declaration) and confirm that the _is_github_server host-validation fix can land as a standalone security hotfix before the full Item 1 design settles.

Suggested issue comment

Thanks for surfacing both items together -- the security bundling rationale is correct.

**Item 2 (`_is_github_server` host validation)** is a clear security fix: name-match alone as the GitHub token injection gate is insufficient. A poisoned registry entry named `github-mcp-server` pointing at a non-GitHub host would receive a GitHub token. This should be treated as a security hotfix and can land as a standalone PR without waiting for Item 1. Acceptance criteria are exact and testable.

**Item 1 (per-runtime transport-preference)** is a clean design direction, but the interface is cross-cutting -- every adapter must declare a transport-preference order and capability set. The design question to settle before code lands: what is the exact shape of that policy table? Options include a dataclass per adapter, a module-level constant, or a method on the adapter base class.

Recommended path: land Item 2 as a security hotfix PR now. Open a follow-up design discussion for Item 1 linked here.

Setting `status/needs-design` for the combined scope; `priority/high` for the security surface.

Per-lens notes (collapsed)

DevX UX Expert -- User-Need Reviewer

Two bundled items: (1) Smart transport selection to eliminate the transport: http workaround in the README; (2) Security fix for _is_github_server host validation. Item 1 directly addresses UX confusion where non-deterministic adapter behavior forces a workaround in the README. Item 2 is a security fix. Both map to real user surfaces. Issue is well-framed with exact acceptance criteria.

Supply Chain Security Expert -- Risk-Surface Reviewer

Item 2 is an explicit credential-injection vulnerability: _is_github_server at copilot.py:675-717 returns True on name match alone, meaning a poisoned registry entry named github-mcp-server pointing at evil.example.com would receive a GitHub token. theme/security is required. Areas: area/mcp-config, area/mcp-trust. Item 1 (smart sourcing) amplifies this risk if it expands the remote-path footprint without fixing Item 2 first.

OSS Growth Hacker -- Contributor-Tone Reviewer

Not activated -- danielmeppiel is a COLLABORATOR with many prior interactions; standard tone is appropriate.

Python Architect -- Architecture Reviewer

Item 1 requires each adapter to declare a transport-preference order and capability set -- a new interface contract touching every adapter file (Copilot, VS Code, Codex). The design as proposed is sound ("small policy table per adapter"), but the exact interface shape is unspecified (method? dataclass? enum?). status/needs-design is correct for the combined scope. Item 2 is a standalone security fix with no design dependency and should land immediately.

Doc Writer -- Documentation Reviewer

Item 1 affects the README example (dropping transport: http overlay). Item 2 needs a threat-model sentence in docs/.../mcp-servers.md. Both imply documentation updates. The wording in the issue body is clear and technically precise.

APM CEO -- Triage Arbiter

Security issue deserves priority/high. Item 2 is a standalone security fix that should not wait for the Item 1 design. status/needs-design for the combined scope is correct. The design needed for Item 1: define the per-adapter transport-preference interface before cross-adapter behavior changes land. Strategic call: get Item 2 out as a hotfix, then design Item 1 properly.

{
  "decision": "needs-design",
  "decision_detail": "Item 1 (per-adapter transport-preference interface) requires cross-cutting design; Item 2 (host validation security fix) can land as a standalone hotfix immediately.",
  "theme": "theme/security",
  "areas": ["area/mcp-config", "area/mcp-trust"],
  "type": "type/bug",
  "status": "status/needs-design",
  "priority": "priority/high",
  "preserved_labels": [],
  "milestone": null,
  "next_action": "Design the per-adapter transport-preference interface; land _is_github_server host-validation fix as a standalone security hotfix PR without waiting for Item 1 design.",
  "comment_markdown": "Thanks for surfacing both items together -- the security bundling rationale is correct.\n\n**Item 2 (`_is_github_server` host validation)** is a clear security fix: name-match alone as the GitHub token injection gate is insufficient. A poisoned registry entry named `github-mcp-server` pointing at a non-GitHub host would receive a GitHub token. This can land as a standalone PR now.\n\n**Item 1 (per-runtime transport-preference)** needs design: every adapter must declare a transport-preference order and capability set. The interface shape (dataclass? module constant? base-class method?) must be agreed on before the cross-adapter change lands.\n\nRecommended path: land Item 2 as a security hotfix PR; open a follow-up design discussion for Item 1."
}

---

Triage status: agentic proposal pending human ratification.
Silence is approval. Maintainers can:

• Override any label or milestone above by editing it directly --
  human edits are authoritative and will not be reverted on
  subsequent runs.
• Re-trigger triage by applying the status/needs-triage label, or
  by removing status/triaged to enroll the issue in the next
  daily sweep.

Posted by the Triage Panel workflow. See .apm/skills/apm-triage-panel for the panel skill.

Generated by Triage Panel · ● 1.6M · ◷