diff --git a/copilot/hooks/test_pretool_mcp.py b/copilot/hooks/test_pretool_mcp.py index ec7f2f78..90d3a6d6 100644 --- a/copilot/hooks/test_pretool_mcp.py +++ b/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__` 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__` 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)) diff --git a/copilot/hooks/unbound.py b/copilot/hooks/unbound.py index 8c2469ed..677df81a 100644 --- a/copilot/hooks/unbound.py +++ b/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__` (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__` 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__` 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}"