fix: harden _is_github_server host validation to require hostname match

[sergio-sisternes-epam]

Description

Addresses Item 2 of #816: _is_github_server currently returns True (triggering GitHub token injection) on name match alone. A poisoned registry entry named github-mcp-server pointing at a non-GitHub URL would receive the user's GitHub token.

Fix: Require BOTH name match AND is_github_hostname() returning True before injecting tokens.

Item 1 (smart per-runtime sourcing) is tracked separately — it has status/needs-design and is a larger scope.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Security fix

Testing

• Unit test: poisoned-name + non-GitHub URL does not get a token header
• Unit test: legitimate GitHub server still gets the token
• All existing tests pass

Closes #816 (Item 2)

[Copilot]

Pull request overview

Hardens GitHub MCP server detection in the Copilot client adapter to reduce the risk of GitHub token injection when registry data is poisoned (e.g., a GitHub-looking name pointing at a non-GitHub host).

Changes:

• Updates CopilotClientAdapter._is_github_server() to parse URLs and validate hostnames against a GitHub/Copilot hostname allowlist.
• Adds unit tests covering _is_github_server() decision behavior for poisoned-name, valid GitHub, and enterprise/Copilot variants.
• Documents MCP server token-injection threat model and hostname/name requirements in the enterprise security docs.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
src/apm_cli/adapters/client/copilot.py	Adjusts GitHub MCP server detection logic used to decide whether to inject GitHub auth headers for remote MCP servers.
tests/unit/test_copilot_adapter.py	Adds a new unit test class for _is_github_server() behavior under malicious and legitimate inputs.
docs/src/content/docs/enterprise/security.md	Adds documentation describing MCP token injection and the intended protections against poisoned registry entries.

[github-actions]

APM Review Panel: needs_rework

Security fix closes real PAT exfiltration vector but ships dead code that silently voids the stated BOTH-name-AND-host policy; doc has a factual accuracy error on the security boundary; HTTPS scheme gap is a new exposure. Rework before merge.

cc @sergio-sisternes-epam @danielmeppiel -- a fresh advisory pass is ready for your review.

Four panelists independently confirmed the same structural defect: both branches of if name_matches: return host_matches and the trailing return host_matches are identical, making the name allowlist vestigial. The PR description commits to requiring BOTH name match AND verified hostname, but the shipped code implements host-only policy. This is not a theoretical gap -- it means a server named totally-not-github pointing at https://api.github.com receives the PAT. The security improvement is real (a poisoned-name server with a non-GitHub URL no longer gets a token), but the stated policy is not implemented and the code comment is actively misleading. The CEO ruling on policy: host-only is architecturally cleaner and more defensible than name-gating for token injection -- names are attacker-controlled labels, hostnames are network-layer facts -- but that is a deliberate policy choice that must be documented explicitly, not an accidental tautology. The dead code must be removed and the docstring corrected to match whichever policy the author intends. The doc-writer finding is blocking on its own terms: security.md omits *.github.com and bare githubcopilot.com from the recognised-hostname list, giving enterprise evaluators an incorrect picture of the security boundary at the exact moment they are deciding whether to trust APM with their PATs. The supply-chain-security finding on HTTPS scheme is a net-new exposure: (api.github.com/redacted) passes the hostname check and the token travels in cleartext. This is not addressed by this PR and must be before merge. The test-coverage-expert finding carries evidence outcome=missing on a secure-by-default surface: no test verifies that configure_mcp_server` omits the Authorization header for a poisoned-name remote entry. Per panel weighting rules, a missing regression-trap on a secure-by-default surface ranks above opinion-tier recommended findings. This gap means a future refactor can reintroduce the exfiltration vector with all 7 existing unit tests still green.

Dissent. Auth-expert frames the URL-only token-grant path as a policy ambiguity requiring documentation or closure. Supply-chain-security agrees the name allowlist is vestigial and recommends simplification. Python-architect says pick one policy. These three are aligned in diagnosis; the framing difference is tone, not substance. CEO ruling: host-only is the correct policy if the dead conditional is removed and the docstring is rewritten to say so. Auth-expert's concern about 'malicious-tool' with a GitHub URL receiving a token is valid as a documentation gap, not a security regression relative to pre-PR behavior, because pre-PR the same server would also have received the token via name match alone. The fix strictly narrows the injection surface.

Aligned with: Secure by default -- partially satisfied: the poisoned-name attack vector is closed but the HTTPS scheme gap and dead name-allowlist mean the security boundary is narrower than stated; rework closes both. Pragmatic as npm -- needs work: silent rejection of a local proxy named github-mcp-server with a localhost URL is real operator friction; a single debug/warning log line resolves this. OSS community driven -- the PR author is an external contributor closing a real CVE-class finding; community trust requires landing this cleanly with a CHANGELOG entry and correct security.md so the fix is attributable and verifiable.

Growth signal. 'APM now requires verified GitHub hostname before injecting your PAT' is a one-liner enterprise security teams will forward internally. Recommend a dedicated ### Security subsection in CHANGELOG under [Unreleased] and a one-sentence callout in README or why-apm.md. The security.md addition is a strong enterprise proof-point but only once the hostname list is accurate -- do not surface it in the docs landing until the doc-writer blocking fix is applied.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	1	The security intent is correct and the poisoned-name attack vector is closed. One silent tautology makes the name allowlist dead code, which also introduces a quiet behavioral regression for URL-less servers. The inner _is_github_mcp_hostname helper diverges from the canonical is_github_hostname utility without extending it, creating a maintenance drift risk.
CLI Logging Expert	0	2	1	The security fix is correct and tight. From a logging/UX lens, the main gap is that silent rejection on URL-parse failure and silent rejection when name matches but hostname does not are both invisible to operators and AI agents running --verbose. No blocking regressions; two recommended additions.
DevX UX Expert	0	1	1	The security hardening is sound and the docs update is helpful. From a developer UX lens there is one silent-failure risk worth calling out: a local-proxy setup named github-mcp-server pointing at localhost silently stops receiving the auth token with no diagnostic.
Supply Chain Security Expert	0	2	1	The fix correctly closes the poisoned-name token-injection vector. Two issues deserve attention: URL scheme is never validated so HTTP URLs to GitHub hostnames pass; and name_matches is dead code making the method unconditionally grant tokens to any GitHub hostname regardless of server name.
OSS Growth Hacker	0	2	1	This is a genuine trust-signal PR. One growth gap: the CHANGELOG should be written for users. One positioning opportunity: the security.md addition is a strong enterprise proof-point that should be surfaced higher in the marketing funnel.
Auth Expert	0	3	1	The fix correctly closes the name-only token injection bypass. The core logic is sound. Key concerns: URL-only path injects token for any GitHub hostname regardless of server name (policy ambiguity); githubcopilot.com hostnames bypass the centralized is_github_hostname; url=None plus name-match silently returns False.
Doc Writer	1	1	1	The new 'MCP server token injection' section is well-placed, matches the page voice, and adds useful security context. One blocking accuracy error: the recognised-hostname list is incomplete relative to the code. One recommended gap: CHANGELOG has no security entry for this fix.
Test Coverage Expert	0	1	1	7 new unit tests correctly defend the _is_github_server gate logic; one recommended gap: no test asserts the Authorization header is absent from configure_mcp_server output when the gate returns False (the user-visible token-leak promise).

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Test Coverage Expert] Add regression-trap test: configure_mcp_server with name=github-mcp-server, url=(evil.example.com/redacted) must assert Authorization` not in result config headers -- Missing evidence on secure-by-default surface. Existing 7 unit tests are white-box gate tests; this is the user-visible promise. A future refactor can reopen the exfiltration vector with all current tests green.
2. [Supply Chain Security Expert] Validate URL scheme is HTTPS before injecting token; return False for (redacted) URLs even when hostname matches -- (api.github.com/redacted) passes the current hostname check and sends the PAT in cleartext over any shared network. One-line fix: check parsed_url.scheme.lower() == 'https'.
3. [Doc Writer] Fix security.md hostname list to include *.github.com (e.g. api.github.com) and bare githubcopilot.com -- Blocking accuracy error. Enterprise evaluators reading this section to understand the token injection security boundary get an incorrect picture. Fix is one sentence.
4. [Python Architect] Remove the dead name_matches conditional and rewrite docstring to state the actual policy (host-only, or BOTH if name-gating is intentionally restored) -- Four panelists independently confirmed the tautology. The code comment asserts BOTH-name-AND-host policy but the implementation is host-only. Leaving dead code ships a misleading docstring into a security-critical function.
5. [Doc Writer] Add ### Security subsection to CHANGELOG [Unreleased] citing PR fix: harden _is_github_server host validation to require hostname match #1239 and the closed exfiltration vector -- Matches CHANGELOG convention and is enterprise-marketable. OSS-growth-hacker independently flagged the same gap. One-line entry.

Architecture

classDiagram
    direction LR

    class CopilotAdapter {
        <<Facade>>
        +_is_github_server(server_name, url) bool
        +_inject_env_vars_into_docker_args(args, env_vars)
    }

    class _is_github_mcp_hostname {
        <<PureFunction>>
        +__call__(hostname: str) bool
    }

    class is_github_hostname {
        <<PureFunction>>
        +__call__(hostname: str | None) bool
    }

    class GitHubTokenManager {
        <<Strategy>>
        +get_token_for_purpose(purpose: str) str | None
    }

    class github_host {
        <<Module>>
    }

    class _is_github_mcp_hostname:::touched
    class CopilotAdapter:::touched

    CopilotAdapter ..> _is_github_mcp_hostname : defines inline
    CopilotAdapter ..> GitHubTokenManager : instantiates on True
    _is_github_mcp_hostname ..> is_github_hostname : delegates (partial)
    is_github_hostname --* github_host : defined in

    note for _is_github_mcp_hostname "Extends is_github_hostname with\n.github.com subdomains +\ngithubcopilot.com -- diverges\nfrom canonical util"
    note for CopilotAdapter "Guard-clause pattern half-applied:\nname_matches computed but never\ngates output (dead code)"

    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm copilot run\n(user CLI entry)"] --> B["CopilotAdapter\n_build_mcp_server_config()\ncopilot.py:~560"]

    B --> C["server_name = server_info.get('name')\nurl = remote.get('url', '')"]

    C --> D["_is_github_server(server_name, url)\ncopilot.py:1208"]

    D --> E{"name_matches?\n(allowlist check)\n[DEAD -- not used in output]"}

    E -->|"True or False"| F["[NET] urlparse(url)\nextract hostname\ncopilot.py:1250"]
    F -->|"parse error"| G["return False\n(no token)"]

    F --> H["_is_github_mcp_hostname(hostname)\ncopilot.py:1234"]

    H --> I{"is_github_hostname()\n-> github.com / *.ghe.com?"}
    I -->|"Yes"| J["host_matches = True"]
    I -->|"No"| K{"*.github.com\nor githubcopilot.com?"}
    K -->|"Yes"| J
    K -->|"No"| L["host_matches = False"]

    J --> M["return host_matches = True"]
    L --> N["return host_matches = False"]

    M --> O["[NET] GitHubTokenManager\n.get_token_for_purpose('copilot')\ntoken_manager.py:111"]
    O --> P["[NET] config headers =\nAuthorization: Bearer <token>\ncopilot.py:580"]
    N --> Q["no token injection"]

    style E fill:#ffcccc,stroke:#cc0000
    style M fill:#ccffcc,stroke:#006600
    style N fill:#f5f5f5

Loading

sequenceDiagram
    participant CLI as apm CLI
    participant CA as CopilotAdapter
    participant GH as github_host.is_github_hostname
    participant TM as GitHubTokenManager

    CLI->>CA: _build_mcp_server_config(server_info, remote)
    CA->>CA: name_matches = server_name in allowlist
    Note over CA: name_matches is computed but\nnever gates the final return
    CA->>CA: urlparse(url) -- hostname
    CA->>GH: is_github_hostname(hostname)
    GH-->>CA: True / False
    alt hostname is GitHub
        CA->>CA: host_matches = True
        CA-->>CLI: return True
        CLI->>TM: get_token_for_purpose("copilot")
        TM-->>CLI: token
        CLI->>CLI: config["headers"] = Bearer token
    else hostname is not GitHub
        CA->>CA: host_matches = False
        CA-->>CLI: return False
        Note over CLI: no token injected
    end

Loading

Recommendation

The security improvement is real and the contributor deserves a fast turnaround. Four items must be resolved before merge: (1) remove the dead name_matches conditional and correct the docstring to name the actual policy; (2) add HTTPS scheme validation before token injection; (3) fix the security.md hostname list accuracy error; (4) add the configure_mcp_server end-to-end regression-trap test asserting Authorization is absent for a poisoned-name remote entry. None of these require architectural changes -- all four are localised edits. A single follow-up commit from the author closes all blocking gaps. Recommended nits (warning log on name-match-but-host-fail, url=None parametrize entry, CHANGELOG entry, centralising githubcopilot.com into is_github_hostname) can land in this PR or in a fast follow. Do not merge in the current state: the dead code ships a misleading docstring into a security-critical function, the doc accuracy error mis-documents the security boundary for enterprise evaluators, and the missing regression-trap test leaves the exfiltration vector unguarded against future refactors.

---

Full per-persona findings

Python Architect

• [recommended] name_matches is dead code -- name allowlist has zero effect on the result at src/apm_cli/adapters/client/copilot.py:1258
  Both branches of if name_matches: return host_matches and the trailing return host_matches are identical. The name allowlist (github-mcp-server, github, ...) is computed but never gates the output. The effective logic is simply return host_matches. This is almost certainly unintentional: the PR description says 'Require BOTH name match AND is_github_hostname() returning True', but the code returns host_matches unconditionally. Design pattern note: this is a guard-clause / policy pattern that was half-applied.
  Suggested: If the intent is 'name OR host': return name_matches or host_matches. If 'name AND host': return name_matches and host_matches. If 'host always required, name is informational only': drop name_matches entirely and return host_matches with a clarifying comment.

• [recommended] Silent behavioral regression: URL-less / local MCP servers named github-mcp-server no longer receive tokens at src/apm_cli/adapters/client/copilot.py:1248
  Before this PR, a server named github-mcp-server with an empty or absent URL returned True (token injected). After this PR, hostname=None -> host_matches=False -> return False (no token). Any locally-run or stdin-transport GitHub MCP server registered without an HTTP URL is silently broken.
  Suggested: Add an explicit test for (server_name='github-mcp-server', url=None) and decide: should this case return True (preserve compat) or False (strict)? If True, add if name_matches and not url: return True before the URL-parse block.

• [nit] _is_github_mcp_hostname extends is_github_hostname inline rather than at the source at src/apm_cli/adapters/client/copilot.py:1234
  The inner helper adds .github.com subdomains and githubcopilot.com/.githubcopilot.com patterns that is_github_hostname in github_host.py does not recognise. This creates two diverging hostname classifiers in the same repo.

CLI Logging Expert

• [recommended] URL-parse exception swallows diagnostic context silently at src/apm_cli/adapters/client/copilot.py
  The bare except Exception: return False discards the exception and emits nothing. In --verbose mode an operator or agent has no way to know that a server was rejected because its URL was malformed rather than because it pointed at a non-GitHub host.
  Suggested: In the except block, emit a verbose-only warning via CommandLogger: e.g. [!] Skipping token injection for '<server_name>' -- URL parse failed: <exc>. Gate it behind --verbose so normal runs stay clean.

• [recommended] Name-matches-but-host-fails path is completely silent at src/apm_cli/adapters/client/copilot.py
  When name_matches=True and host_matches=False the method returns False with zero output. This is the exact attack-surface path the fix is hardening against, but it is also the path a legitimate user hits when they have a custom registry entry whose URL has not been updated.
  Suggested: Before the final return host_matches inside the if name_matches: branch, log at verbose level: [!] Token injection denied for '<server_name>' -- name matched allowlist but hostname '<hostname>' is not a verified GitHub domain.

• [nit] Docstring uses Markdown bold (not) which renders oddly in CLI --help or pydoc at src/apm_cli/adapters/client/copilot.py
  Suggested: Replace **not** with plain NOT (all-caps is the conventional emphasis style in APM docstrings).

DevX UX Expert

• [recommended] Silent rejection when name matches but URL is non-GitHub -- no diagnostic emitted
  A developer running a local proxy named github-mcp-server pointing at (localhost/redacted) will hit a silent auth failure. APM returns False with no log line, no warning, and no hint about why the token was withheld. The failure will surface as a downstream 401 or missing-auth error far from the policy that caused it. *Suggested:* Emit a _rich_warningor at minimum alogger.debugline in thename_matches and not host_matches` branch: e.g. 'GitHub token not injected for server "%s": URL hostname "%s" does not match a recognized GitHub domain.'

• [nit] Final two return host_matches lines are logically identical and mislead readers
  After the if name_matches: block both branches return host_matches, making the conditional dead code from a logic standpoint.
  Suggested: Collapse to a single return host_matches after the if name_matches block, and move the security comment above it.

Supply Chain Security Expert

• [recommended] URL scheme not validated -- HTTP URLs to GitHub hostnames pass hostname check at src/apm_cli/adapters/client/copilot.py:1258
_is_github_mcp_hostname only checks the hostname, not the scheme. A URL like (api.github.com/redacted) passes and the GitHub token is injected into an unencrypted request. A passive observer on any network segment between the APM host and GitHub sees the Authorization header in cleartext. *Suggested:* After extracting parsed_url, check parsed_url.scheme.lower() == 'https'` and return False if not satisfied.

• [recommended] name_matches is dead code -- name allowlist has no effect on the return value at src/apm_cli/adapters/client/copilot.py:1262
  Both branches of if name_matches: return host_matches and the fallthrough return host_matches are identical. The github_server_names allowlist is vestigial -- any server whose URL hostname is a GitHub domain receives the token, even named totally-not-github.
  Suggested: Decide on the intended policy: (A) hostname-only suffices -> delete github_server_names, name_matches, and the if name_matches branch; (B) both required -> return name_matches and host_matches. Option A matches the PR description and is cleaner.

• [nit] githubcopilot.com wildcard subdomain trust is broad at src/apm_cli/adapters/client/copilot.py:1243
*.githubcopilot.com is fully trusted for token injection. Consider documenting the explicit list of expected subdomains if it is static.

OSS Growth Hacker

• [recommended] Mine this fix for a user-facing CHANGELOG entry framed as a trust guarantee, not an internal patch
  Security fixes that close real exfiltration vectors are rare, concrete story material. 'APM now requires verified GitHub hostname before injecting your token -- a poisoned registry entry with a GitHub-sounding name can no longer steal your PAT' is enterprise-forwardable.
  Suggested: Add a CHANGELOG entry in the 'Security' section: 'Token injection now requires a verified GitHub hostname in addition to a name-allowlist match. A registry entry named github-mcp-server pointing at a non-GitHub URL will no longer receive your GitHub token.' Cross-link to security.md.

• [recommended] Surface the new security.md paragraph in the docs landing or README security callout
  The security.md addition lives deep in /enterprise/security.md -- a page that only visitors who already trust APM enough to evaluate enterprise use will reach. The promise is a differentiation claim vs. naive package managers.
  Suggested: Add one sentence to the README security callout: 'Token injection is gated on verified GitHub hostname, not just server name -- closing the poisoned-registry exfiltration vector.' Link to security.md for details.

• [nit] PR body checklist items are unchecked despite 7 unit tests existing in the diff
  Merged PRs with incomplete checklists subtly undermine process credibility when prospective contributors audit the repo's merge hygiene.

Auth Expert

• [recommended] URL-only path injects token for any GitHub hostname regardless of server name -- policy should be explicit at src/apm_cli/adapters/client/copilot.py
  When name_matches is False but host_matches is True, the function returns True and injects the token. This means a server named 'malicious-tool' with url='https://api.github.com/anything' gets a GitHub token. If intentional, document it explicitly.
  Suggested: Add an explicit comment documenting the intentional policy, or restrict token injection to require EITHER (name matches AND host matches) OR (no name provided AND host matches).

• [recommended] githubcopilot.com is not in is_github_hostname() -- bypasses centralized hostname allowlist at src/apm_cli/adapters/client/copilot.py
githubcopilot.com and *.githubcopilot.com are added inline in _is_github_mcp_hostname but not in the canonical is_github_hostname() utility. This creates two diverging lists.
  Suggested: Consider extending is_github_hostname() in github_host.py to include githubcopilot.com and *.githubcopilot.com.

• [recommended] url=None + name match silently returns False with no diagnostic -- callers may not detect misconfiguration at src/apm_cli/adapters/client/copilot.py
  When url is None or empty and server_name matches an allowlisted name, token injection is suppressed silently. A legitimate GitHub MCP server configured without a URL field would silently lose its auth header.
  Suggested: When name_matches is True but hostname is None/empty, emit a _rich_warning() explaining that token injection was suppressed because no URL was provided.

• [nit] Redundant final return host_matches -- both branches of the if/else do the same thing at src/apm_cli/adapters/client/copilot.py
  Suggested: Collapse to return host_matches with a comment: '# Token injected iff URL resolves to a GitHub hostname (name match now requires host match too)'.

Doc Writer

• [blocking] Recognised hostname list omits *.github.com subdomains and the exact githubcopilot.com match at docs/src/content/docs/enterprise/security.md
  The doc states: 'Recognised hostnames include github.com, *.ghe.com (GitHub Enterprise), and *.githubcopilot.com (Copilot API).' Per the code, _is_github_mcp_hostname also matches *.github.com (covering api.github.com) and the bare githubcopilot.com exact hostname. Users reading this section to understand what endpoints receive tokens will have an incomplete and incorrect picture.
  Suggested: Replace the final sentence with: 'Recognised hostnames include github.com, *.github.com (e.g. api.github.com), *.ghe.com (GitHub Enterprise), githubcopilot.com, and *.githubcopilot.com (Copilot API).'

• [recommended] No CHANGELOG entry for this security fix at CHANGELOG.md
  CHANGELOG.md uses a ### Security subsection under [Unreleased] for security-relevant fixes. Hardening token-injection host validation warrants the same treatment.
  Suggested: Add under ## [Unreleased] -> ### Security: '- MCP server token injection now requires both a name-allowlist match and a verified GitHub hostname; a name match alone no longer causes token injection. Recognised hostnames: github.com, *.github.com, *.ghe.com, githubcopilot.com, *.githubcopilot.com. (fix: harden _is_github_server host validation to require hostname match #1239)'

• [nit] Paragraph is slightly dense for a security reference section at docs/src/content/docs/enterprise/security.md
  Splitting into two sentences -- one for the threat model, one for the mitigation -- would improve scannability.

Test Coverage Expert

• [recommended] Missing regression-trap: no test verifies Authorization header is absent from configure_mcp_server output for a poisoned-name remote entry at tests/unit/test_copilot_adapter.py
  The 7 new tests call _is_github_server directly (white-box unit), confirming the gate's boolean logic. The user-visible promise is 'my GitHub token is never sent to a non-GitHub server.' That depends on _is_github_server returning False AND configure_mcp_server not writing config['headers'] when it does. If a future refactor inverts the header-injection conditional, all 7 existing tests pass while the token leaks. Probed with grep: no 'configure_mcp_server' + 'Authorization' + 'not in' match found.
  Suggested: Add a test calling configure_mcp_server with a fixture having name='github-mcp-server', url='(evil.example.com/redacted) and asserting 'Authorization' not in result_config.get('headers', {}). Pair it with an inverse test proving the good path still injects the header. *Proof (missing):* tests/unit/test_copilot_adapter.py::test_poisoned_remote_does_not_inject_authorization_header` -- proves: A poisoned-name registry entry does not receive the user's GitHub token in the HTTP request config [secure-by-default]

• [nit] url=None (not empty string) is untested; code handles it correctly but no explicit assertion at tests/unit/test_copilot_adapter.py
  The 'if url:' guard leaves hostname=None when url=None, returns False correctly. A one-line parametrize entry would seal this edge case formally.
  Suggested: Add: _is_github_server('github-mcp-server', None) -> assertFalse
  Proof (missing): tests/unit/test_copilot_adapter.py::test_name_match_url_none_returns_false -- proves: url=None is handled safely -- no AttributeError, returns False [secure-by-default]

_{This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.}

Generated by PR Review Panel for issue #1239 · ● 3M · ◷