Resolve Copilot CLI & VS Code built-in/hosted MCP identity (sanction/policy coverage)

copilot/hooks/test_pretool_mcp.py

@@ -36,10 +36,83 @@ def _read_fixture_servers():
     return out
 
 
+CLI_BUILTIN_READONLY = "https://api.individual.githubcopilot.com/mcp/readonly"
+VSCODE_BUILTIN_URL = "https://api.githubcopilot.com/mcp"
+
+
+class TestDetectMcpCallBuiltin(unittest.TestCase):
+    """CLI bare-name resolution of the runtime-provisioned `github-mcp-server`
+    built-in (never in any config file). The built-in is folded into the candidate
+    set so its full name (len 17) out-ranks a coincidental `github` config (len 6)."""
+
+    def test_builtin_outranks_coincidental_github(self):
+        # A `github` server is configured, but the call is to the built-in
+        # github-mcp-server. The longer built-in name must win (no garbled tool,
+        # correct readonly url) instead of being mis-attributed to `github`.
+        srv, tool, cfg = unbound.detect_mcp_call(
+            "github-mcp-server-list_issues", {"github": {"url": "https://api.githubcopilot.com/mcp/"}})
+        self.assertEqual(srv, "github-mcp-server")
+        self.assertEqual(tool, "list_issues")
+        self.assertEqual(cfg.get("url"), CLI_BUILTIN_READONLY)
+
+    def test_builtin_resolves_with_no_config(self):
+        # No on-disk servers at all -> built-in still resolves (not None).
+        srv, tool, cfg = unbound.detect_mcp_call("github-mcp-server-list_issues", {})
+        self.assertEqual(srv, "github-mcp-server")
+        self.assertEqual(tool, "list_issues")
+        self.assertEqual(cfg.get("url"), CLI_BUILTIN_READONLY)
+
+    def test_configured_server_wins_over_builtin(self):
+        # A user-configured github-mcp-server (custom url) takes precedence over the
+        # built-in registry entry (config last in the merge).
+        custom = "https://internal.example/mcp"
+        srv, tool, cfg = unbound.detect_mcp_call(
+            "github-mcp-server-list_issues", {"github-mcp-server": {"url": custom}})
+        self.assertEqual(srv, "github-mcp-server")
+        self.assertEqual(tool, "list_issues")
+        self.assertEqual(cfg.get("url"), custom)
+
+    def test_unrelated_bare_tool_still_unresolved(self):
+        # The built-in must not broaden matching: an unrelated bare tool name with
+        # no configured server stays (None, None, None).
+        self.assertEqual(
+            unbound.detect_mcp_call("some_native_tool", {}),
+            (None, None, None))
+
+
 class TestResolveVscodeMcp(unittest.TestCase):
     def setUp(self):
         self.servers = _read_fixture_servers()
 
+    def test_builtin_fallback_resolves_when_unconfigured(self):
+        # No `github`/github-mcp-server configured at all. A built-in hosted call
+        # must fall back to the VS Code built-in registry url.
+        servers = {"playwright": {"command": "npx", "args": ["@playwright/mcp@latest"]}}
+        srv, tool, cfg = unbound._resolve_vscode_mcp(
+            "mcp_github_mcp_se_list_commits", servers)
+        self.assertEqual(srv, "github-mcp-server")
+        self.assertEqual(tool, "list_commits")
+        self.assertEqual(cfg.get("url"), VSCODE_BUILTIN_URL)
+
+    def test_configured_server_wins_over_builtin_fallback(self):
+        # setup#168 preserved: a configured server that matches resolves to ITS
+        # config; the built-in fallback never fires (early candidates non-empty).
+        srv, tool, cfg = unbound._resolve_vscode_mcp(
+            "mcp_github_mcp_se_search_repositories", self.servers)
+        self.assertEqual(srv, "io.github.github/github-mcp-server")
+        self.assertEqual(tool, "search_repositories")
+        self.assertEqual(cfg.get("url"), GH_GROUP)
+
+    def test_builtin_fallback_does_not_override_configured_ambiguity(self):
+        # Two configured servers overlap and are genuinely ambiguous -> unresolved.
+        # The built-in fallback must NOT rescue this: it only fires when the
+        # configured candidate set is empty, not when it's ambiguous.
+        servers = {"linear": {"url": "https://danger/mcp"},
+                   "linear_create_safe": {"url": "https://safe/mcp"}}
+        self.assertEqual(
+            unbound._resolve_vscode_mcp("mcp_linear_create_issue", servers),
+            (None, None, None))
+
     def test_truncated_server_name_resolves(self):
         # io.github.github/github-mcp-server surfaces as the truncated
         # `github_mcp_se`; must still map back to the full server key.
@@ -250,6 +323,54 @@ def capturing_gw(request_body, api_key):
         self.assertFalse(self.is_block(ret))
 
 
+class TestProcessPreToolUseVscodeBuiltin(unittest.TestCase):
+    """E2E for the VS Code built-in fallback: no `github` is configured, so the
+    hosted github-mcp-server resolves via the built-in registry url and the org
+    sanction list applies to it."""
+
+    BUILTIN_URL = "https://api.githubcopilot.com/mcp"
+    # Config that deliberately omits any github server so the fallback fires.
+    NO_GITHUB_CONFIG = {"servers": {"playwright": {"command": "npx", "args": ["@playwright/mcp@latest"]}}}
+
+    def setUp(self):
+        self._tmp = tempfile.TemporaryDirectory()
+        cfg_path = Path(self._tmp.name) / ".vscode" / "mcp.json"
+        cfg_path.parent.mkdir(parents=True, exist_ok=True)
+        cfg_path.write_text(json.dumps(self.NO_GITHUB_CONFIG))
+        self._patchers = [
+            patch.object(unbound, "_copilot_mcp_config_paths", lambda cwd=None: [cfg_path]),
+            patch.object(unbound, "load_policy_cache", lambda: None),
+            patch.object(unbound, "get_recent_user_prompts_for_session", lambda *a, **k: []),
+            patch.object(unbound, "get_session_start_model", lambda *a, **k: "auto"),
+            patch.object(unbound, "_is_approval_retry", lambda *a, **k: False),
+            patch.object(unbound, "report_error_to_gateway", lambda *a, **k: None),
+        ]
+        for p in self._patchers:
+            p.start()
+        self.cwd = self._tmp.name
+
+    def tearDown(self):
+        for p in self._patchers:
+            p.stop()
+        self._tmp.cleanup()
+
+    def _run(self, raw_tool, sanctioned_groups):
+        event = {
+            "hook_event_name": "PreToolUse",
+            "tool_name": raw_tool,
+            "tool_input": {"q": "x"}, "cwd": self.cwd, "session_id": "s",
+        }
+        with patch.object(unbound, "send_to_hook_api", _gateway(sanctioned_groups)), \
+             patch.object(unbound, "_read_policy_cache_raw",
+                          lambda: {"policy_check_failure_action": "allow"}):
+            return unbound.process_pre_tool_use(event, "K")
+
+    def test_sanctioned_builtin_hosted_github_is_allowed(self):
+        # Built-in hosted github call, its url sanctioned -> allowed.
+        ret = self._run("mcp_github_mcp_se_list_commits", {self.BUILTIN_URL})
+        self.assertFalse(ProcessPreToolUseBase.is_block(ret))
+
+
 class TestUnresolvedForwarding(ProcessPreToolUseBase):
     def test_unresolved_mcp_is_forwarded_to_gateway_not_short_circuited(self):
         # An unmappable mcp_ call must still reach the gateway, not return {} early.
@@ -268,11 +389,52 @@ def capturing_gw(request_body, api_key):
             unbound.process_pre_tool_use(event, "K")
         self.assertIn("body", called)  # gateway WAS called (not short-circuited)
         md = called["body"]["pre_tool_use_data"]["metadata"]
-        self.assertNotIn("mcp_server", md)  # nothing resolved to forward
-
-    def test_unresolved_mcp_not_sanction_blocked(self):
-        # No resolved server -> allow-list not evaluated (fail-open) for unresolved.
+        # CHANGED (VS Code Fix B): an unresolved `mcp_<server>_<tool>` now forwards a
+        # best-effort server token (mcp_server present, no config) so deny-by-default
+        # can apply, instead of leaving mcp_server unset and slipping the allow-list.
+        self.assertEqual(md.get("mcp_server"), "unknownserver")
+        self.assertEqual(md.get("mcp_tool"), "do_thing")
+        self.assertNotIn("mcp_server_config", md)  # nothing resolved -> no config forwarded
+
+    def test_unresolved_mcp_is_sanction_blocked_fail_secure(self):
+        # CHANGED behavior (VS Code Fix B): an unresolved `mcp_<server>_<tool>` no
+        # longer fails open. The `mcp_` prefix is a reliable MCP signal, so a
+        # best-effort server token is forwarded (mcp_server present) and the active
+        # sanction list now BLOCKS the unrecognized server (deny-by-default).
         ret = self.run_tool("mcp_unknownserver_do_thing", {GH_GROUP})
+        self.assertTrue(self.is_block(ret))
+
+    def test_degenerate_empty_tool_token_is_not_forwarded(self):
+        # Fix 1: a degenerate `mcp_`-prefixed token with NO tool segment (e.g.
+        # `mcp_x` -> body `x` -> partition yields empty tool) must NOT forward a
+        # malformed `mcp__x__` identity. Forwarding mcp_server with an empty tool
+        # over-promises fail-secure: the gateway's `if srv and tool` guard treats
+        # the empty tool as "not MCP" and fails OPEN. So we leave it as-is —
+        # mcp_server/mcp_tool stay unset (the pre-existing behavior, no regression).
+        called = {}
+
+        def capturing_gw(request_body, api_key):
+            called["body"] = request_body
+            return {"decision": "allow"}
+
+        event = {
+            "hook_event_name": "PreToolUse",
+            "tool_name": "mcp_x",
+            "tool_input": {"q": "x"}, "cwd": self.cwd, "session_id": "s",
+        }
+        with patch.object(unbound, "send_to_hook_api", capturing_gw):
+            unbound.process_pre_tool_use(event, "K")
+        md = called["body"]["pre_tool_use_data"]["metadata"]
+        self.assertNotIn("mcp_server", md)  # no malformed identity forwarded
+        self.assertNotIn("mcp_tool", md)
+        self.assertNotIn("mcp_server_config", md)
+
+    def test_degenerate_empty_tool_token_is_not_falsely_blocked(self):
+        # Fix 1 (the fail-secure correction): because no mcp_server is forwarded for
+        # a degenerate token, an active sanction list must NOT block it. The old
+        # behavior would have claimed fail-secure while actually failing open at the
+        # gateway; here it is honestly left as a non-MCP no-op (allow), not blocked.
+        ret = self.run_tool("mcp_x", {GH_GROUP})
         self.assertFalse(self.is_block(ret))
 
 

copilot/hooks/unbound.py

@@ -616,6 +616,32 @@ def _sanitize_copilot_server_name(name):
 _MIN_MCP_SERVER_NAME = 2
 
 
+# Built-in MCP servers provisioned at runtime by the host (never in on-disk config).
+# CLI url != VS Code url (per-surface). gateway-data must classify these urls into
+# the GitHub canonical group for the CLI built-in to match GitHub sanctions.
+COPILOT_CLI_BUILTIN_MCP = {
+    "github-mcp-server": {
+        "url": "https://api.individual.githubcopilot.com/mcp/readonly",
+        "type": "http",
+    }
+}
+
+COPILOT_VSCODE_BUILTIN_MCP = {
+    "github-mcp-server": {
+        "url": "https://api.githubcopilot.com/mcp",
+        "type": "http",
+    }
+}
+
+
+# Copy built-in registry literals so callers can't mutate the shared dicts.
+def _copy_if_builtin(cfg):
+    if cfg is COPILOT_CLI_BUILTIN_MCP.get('github-mcp-server') or \
+            cfg is COPILOT_VSCODE_BUILTIN_MCP.get('github-mcp-server'):
+        return dict(cfg)
+    return cfg
+
+
 # Resolve (server, tool, config) from a Copilot tool name. The mcp__ form is
 # self-delimiting; the bare form is matched against configured server names.
 # Matching is case-insensitive (Copilot lowercases nothing, configs vary case)
@@ -624,15 +650,18 @@ def detect_mcp_call(raw_tool, mcp_servers):
     if not raw_tool:
         return (None, None, None)
 
+    # Fold built-ins into the candidate set; a configured server of the same name wins.
+    merged = {**COPILOT_CLI_BUILTIN_MCP, **mcp_servers}
+
     if raw_tool.startswith('mcp__'):
         parts = raw_tool[len('mcp__'):].split('__', 1)
         server = parts[0]
         mcp_tool = parts[1] if len(parts) >= 2 else ''
-        return (server, mcp_tool, mcp_servers.get(server))
+        return (server, mcp_tool, _copy_if_builtin(merged.get(server)))
 
     raw_lower = raw_tool.lower()
     best = None  # (matched_len, server_name, mcp_tool)
-    for server_name in mcp_servers:
+    for server_name in merged:
         for candidate in {server_name, _sanitize_copilot_server_name(server_name)}:
             if len(candidate) < _MIN_MCP_SERVER_NAME:
                 continue
@@ -652,7 +681,7 @@ def detect_mcp_call(raw_tool, mcp_servers):
 
     if best is None:
         return (None, None, None)
-    return (best[1], best[2], mcp_servers.get(best[1]))
+    return (best[1], best[2], _copy_if_builtin(merged.get(best[1])))
 
 
 # VS Code Copilot names MCP tools `mcp_<server>_<tool>` (single underscore,
@@ -681,19 +710,12 @@ def _vscode_fingerprint_key(config):
     return None
 
 
-def _resolve_vscode_mcp(raw_tool, mcp_servers):
-    """Resolve (server, tool, config) from a VS Code `mcp_<server>_<tool>` name,
-    tolerating truncation; longest server-prefix wins, exact beats truncated on ties.
-    If a *different* server also matches and can't be proven to be the same server
-    (identical fingerprint config), the token is ambiguous -> unresolved (don't
-    guess); same-config duplicates (e.g. two keys for one server) still resolve."""
-    if not raw_tool.startswith('mcp_') or raw_tool.startswith('mcp__'):
-        return (None, None, None)
-    body = raw_tool[len('mcp_'):]
-    body_lower = body.lower()
-    segments = body.split('_')
-    candidates = []  # (server_portion_len, exact_flag, server_name, tool)
-    for server_name in mcp_servers:
+def _vscode_match_servers(server_names, body, body_lower, segments):
+    """Collect `(server_portion_len, exact_flag, server_name, tool)` candidates by
+    matching each server's sanitized aliases against the tool body, tolerating
+    truncation. Candidates resolving to an empty tool portion are dropped."""
+    candidates = []
+    for server_name in server_names:
         for alias in _vscode_server_aliases(server_name):
             if body_lower.startswith(alias + '_'):
                 cand = (len(alias), 1, server_name, body[len(alias) + 1:])
@@ -706,8 +728,29 @@ def _resolve_vscode_mcp(raw_tool, mcp_servers):
                         break
             if cand is not None and cand[3]:
                 candidates.append(cand)
-    if not candidates:
+    return candidates
+
+
+def _resolve_vscode_mcp(raw_tool, mcp_servers):
+    """Resolve (server, tool, config) from a VS Code `mcp_<server>_<tool>` name,
+    tolerating truncation; longest server-prefix wins, exact beats truncated on ties.
+    If a *different* server also matches and can't be proven to be the same server
+    (identical fingerprint config), the token is ambiguous -> unresolved (don't
+    guess); same-config duplicates (e.g. two keys for one server) still resolve."""
+    if not raw_tool.startswith('mcp_') or raw_tool.startswith('mcp__'):
         return (None, None, None)
+    body = raw_tool[len('mcp_'):]
+    body_lower = body.lower()
+    segments = body.split('_')
+    candidates = _vscode_match_servers(mcp_servers, body, body_lower, segments)
+    if not candidates:
+        # Fall back to built-ins only when no configured server matched (configured wins).
+        builtin_candidates = _vscode_match_servers(
+            COPILOT_VSCODE_BUILTIN_MCP, body, body_lower, segments)
+        if len(builtin_candidates) != 1:
+            return (None, None, None)
+        bc = builtin_candidates[0]
+        return (bc[2], bc[3], _copy_if_builtin(COPILOT_VSCODE_BUILTIN_MCP[bc[2]]))
     best = max(candidates, key=lambda c: c[:2])
     best_key = _vscode_fingerprint_key(mcp_servers.get(best[2]))
     for cand in candidates:
@@ -945,9 +988,14 @@ def process_pre_tool_use(event, api_key):
                 'mcp_match',
             )
         else:
-            # Unmappable server: fall through to the gateway with mcp_server unset
-            # (not a short-circuit) so its other policies/logging/metering still run;
-            # the allow-list isn't evaluated without a resolved server (fail-open).
+            # Unresolved mcp_ tool: forward a best-effort identity so deny-by-default
+            # applies (fail-secure), but only with a non-empty tool segment — degenerate
+            # mcp_ tokens (empty tool) are left as-is.
+            body = raw_tool[len('mcp_'):]
+            parsed_server, _, parsed_tool = body.partition('_')
+            if parsed_server and parsed_tool:
+                mcp_server, mcp_tool = parsed_server, parsed_tool
+                canonical = f"mcp__{mcp_server}__{mcp_tool}"
             log_error(f"copilot vscode mcp UNRESOLVED session={session_id} tool={raw_tool}", 'mcp_match')
 
     if not is_mcp and canonical not in ALLOWED_NON_MCP_HOOK_NAMES:
@@ -968,6 +1016,8 @@ def process_pre_tool_use(event, api_key):
                     )
                 else:
                     log_error(f"copilot pre_tool_use unmatched tool={raw_tool}", 'mcp_match')
+            # Deferred: bare unconfigured CLI MCP servers stay fail-open (no delimiter
+            # to split safely; blind-forwarding would over-block native tools).
             return {}
         is_mcp = True
         canonical = f"mcp__{mcp_server}__{mcp_tool}"