Skip to content

Commit 93f9757

Browse files
Sergio SisternesCopilot
andcommitted
fix(security): require hostname validation in _is_github_server (#816)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent cda5104 commit 93f9757

2 files changed

Lines changed: 78 additions & 16 deletions

File tree

src/apm_cli/adapters/client/copilot.py

Lines changed: 30 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1208,8 +1208,12 @@ def _select_best_package(self, packages):
12081208
def _is_github_server(self, server_name, url):
12091209
"""Securely determine if a server is a GitHub MCP server.
12101210
1211-
This method uses proper URL parsing and hostname validation to prevent
1212-
security vulnerabilities from substring-based checks.
1211+
Uses proper URL parsing and hostname validation to prevent token
1212+
injection via poisoned registry entries. A name-allowlist match
1213+
alone is **not** sufficient -- the URL hostname must also belong
1214+
to a verified GitHub domain. A verified hostname on its own
1215+
(regardless of name) is still accepted so that custom-named
1216+
servers pointing at GitHub continue to work.
12131217
12141218
Args:
12151219
server_name (str): Name of the MCP server.
@@ -1220,31 +1224,41 @@ def _is_github_server(self, server_name, url):
12201224
"""
12211225
from urllib.parse import urlparse
12221226

1223-
# Check server name against an allowlist of known GitHub MCP servers
12241227
github_server_names = [
12251228
"github-mcp-server",
12261229
"github",
12271230
"github-mcp",
12281231
"github-copilot-mcp-server",
12291232
]
12301233

1231-
# Exact match check for server names (case-insensitive)
1232-
if server_name and server_name.lower() in [name.lower() for name in github_server_names]:
1233-
return True
1234-
1235-
# If URL is provided, validate the hostname
1234+
def _is_github_mcp_hostname(hostname: str) -> bool:
1235+
"""Check if *hostname* belongs to GitHub (cloud, enterprise, or Copilot API)."""
1236+
if is_github_hostname(hostname):
1237+
return True
1238+
h = hostname.lower()
1239+
# Subdomains of github.com (e.g. api.github.com)
1240+
if h.endswith(".github.com"):
1241+
return True
1242+
# Copilot API hosts (e.g. api.githubcopilot.com, api.business.githubcopilot.com)
1243+
return h == "githubcopilot.com" or h.endswith(".githubcopilot.com")
1244+
1245+
name_matches = bool(
1246+
server_name and server_name.lower() in [n.lower() for n in github_server_names]
1247+
)
1248+
1249+
# Parse and validate hostname from URL
1250+
hostname = None
12361251
if url:
12371252
try:
12381253
parsed_url = urlparse(url)
12391254
hostname = parsed_url.hostname
1240-
1241-
if hostname:
1242-
# Use helper to determine whether hostname is a GitHub host (cloud or enterprise)
1243-
if is_github_hostname(hostname):
1244-
return True
1245-
12461255
except Exception:
1247-
# If URL parsing fails, assume it's not a GitHub server
12481256
return False
12491257

1250-
return False
1258+
host_matches = bool(hostname and _is_github_mcp_hostname(hostname))
1259+
1260+
# SECURITY: require BOTH name AND host when name matches,
1261+
# but allow host-only match (URL points to GitHub regardless of name).
1262+
if name_matches:
1263+
return host_matches # name alone is no longer sufficient
1264+
return host_matches # URL-only path preserved

tests/unit/test_copilot_adapter.py

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -611,5 +611,53 @@ def test_handles_empty_list(self):
611611
self.assertIsNone(CopilotClientAdapter._select_remote_with_url([]))
612612

613613

614+
class TestIsGitHubServer(unittest.TestCase):
615+
"""Tests for ``_is_github_server`` hostname-validation security hardening (#816)."""
616+
617+
def setUp(self):
618+
self.adapter = CopilotClientAdapter.__new__(CopilotClientAdapter)
619+
620+
# -- poisoned registry entry: name matches but URL is malicious ----------
621+
def test_poisoned_name_returns_false(self):
622+
result = self.adapter._is_github_server(
623+
"github-mcp-server", "https://evil.example.com/mcp/"
624+
)
625+
self.assertFalse(result, "Name match alone must NOT be sufficient")
626+
627+
# -- legitimate server: both name and hostname match ---------------------
628+
def test_legitimate_server_returns_true(self):
629+
result = self.adapter._is_github_server(
630+
"github-mcp-server", "https://api.githubcopilot.com/mcp/"
631+
)
632+
self.assertTrue(result)
633+
634+
# -- URL-only match: GitHub hostname with non-matching name --------------
635+
def test_url_only_match_returns_true(self):
636+
result = self.adapter._is_github_server("custom-server", "https://api.github.com/mcp")
637+
self.assertTrue(result)
638+
639+
# -- no match: neither name nor hostname ---------------------------------
640+
def test_no_match_returns_false(self):
641+
result = self.adapter._is_github_server("random-server", "https://evil.example.com/mcp/")
642+
self.assertFalse(result)
643+
644+
# -- name match but empty URL: must reject -------------------------------
645+
def test_name_match_no_url_returns_false(self):
646+
result = self.adapter._is_github_server("github-mcp-server", "")
647+
self.assertFalse(result, "Name match with empty URL must return False")
648+
649+
# -- GHE hostname --------------------------------------------------------
650+
def test_ghe_hostname_returns_true(self):
651+
result = self.adapter._is_github_server("github", "https://mcp.myorg.ghe.com/v1")
652+
self.assertTrue(result)
653+
654+
# -- Copilot enterprise variant ------------------------------------------
655+
def test_copilot_enterprise_variant_returns_true(self):
656+
result = self.adapter._is_github_server(
657+
"github-mcp", "https://api.business.githubcopilot.com/mcp/"
658+
)
659+
self.assertTrue(result)
660+
661+
614662
if __name__ == "__main__":
615663
unittest.main()

0 commit comments

Comments
 (0)