Fix Copilot native tools misclassified as MCP - #268
Conversation
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
4 findings — 2 high-confidence, 2 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
🔴 HIGH — Effectful native tools bypass organization policy
copilot/hooks/unbound.py:2083
- Impact:
UNTRACKED_NATIVE_TOOLSincludes effectful host tools (run_playwright_code,install_extension,run_notebook_cell,install_python_packages,create_new_workspace,web_fetch, browser tools, etc.) that are not covered by the upstream Bash/Write/Edit repository gate; the earlyreturn {}skips gateway allow/deny/approval for all of them. - Fix: Restrict the PreToolUse early return to truly benign/internal tools only; forward effectful natives to the gateway (map to the closest canonical event type) and keep
UNTRACKED_NATIVE_TOOLSfor transcript suppression only. - Flagged by: Cursor, Claude, Greptile
🔴 HIGH — MCP tools shadowed by native name lists escape policy and audit
copilot/hooks/unbound.py:2083 (policy) · copilot/hooks/unbound.py:2867 (transcript)
- Impact: The same early return runs on the raw tool name before
_resolve_vscode_mcp/detect_mcp_call. A configured MCP tool whose bare name collides withUNTRACKED_NATIVE_TOOLSorINTERNAL_TOOLS(e.g.navigate_page,run_playwright_code,web_fetch,task,memory,skill) never reaches the gateway and is dropped bymap_copilot_tool, so org policy cannot block it and analytics lose the call entirely. - Fix: Resolve MCP identity first; apply native/internal exclusion only when the name did not resolve to a configured MCP server and has no
mcp_prefix (mirror this ordering in both_evaluate_pre_tool_use_policiesandmap_copilot_tool). - Flagged by: Cursor, Claude
🟡 TRIAGE — Unknown or future Copilot tools silently vanish from telemetry
copilot/hooks/unbound.py:2928
- Impact:
map_copilot_toolnow returnsNonefor any name outside the hardcoded maps and non-resolvable MCP paths, replacing the previousafterMCPExecutionfall-through; new/renamed Copilot tools (including effectful ones) produce no audit signal until lists are updated, and terminal-output reads moved intoUNTRACKED_NATIVE_TOOLSare no longer emitted. - Fix: Emit unknown tools under a generic/low-signal event (e.g.
unknown_tool) for visibility and rubric scoring; reserveNonefor explicitly known-benign names only. - Flagged by: Claude
🟡 TRIAGE — Case-sensitive mcp_ prefix check can emit unattributed MCP rows
copilot/hooks/unbound.py:2924
- Impact: Resolution uses case-sensitive
name.startswith('mcp_')while the drop guard usesname.lower().startswith('mcp_'), so a tool likeMCP_evil_toolmay skip server resolution yet still be emitted as MCP withoutserver_name, weakening per-server policy and attribution. - Fix: Normalize once (
lowered = name.lower()) and use it consistently for both resolution and drop logic. - Flagged by: Claude
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head 47a9570f · 2026-08-27T19:53Z
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
5 findings — 2 high-confidence, 3 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
🔴 HIGH — Effectful native tools dropped from analytics
copilot/hooks/unbound.py:121, copilot/hooks/unbound.py:2881
Impact: UNTRACKED_NATIVE_TOOLS mixes benign reads with effectful host tools (run_playwright_code, install_extension, run_notebook_cell, web_fetch, runTests, etc.); map_copilot_tool returns None for all of them, so package installs, notebook execution, browser automation, and outbound fetches no longer reach post-hoc analytics/rubric scoring.
Fix: Split the set—keep read-only/UI names suppressed; map effectful tools to a tracked event type (shell-like or generic afterMCPExecution) so they remain visible to the gateway.
Flagged by: Claude, Cursor
🔴 HIGH — Unknown native tools silently untracked (fail-open monitoring)
copilot/hooks/unbound.py:2937
Impact: Unlisted tool names that are not MCP-resolvable now return None instead of falling through to generic afterMCPExecution scoring; any new Copilot builtin (including future shell/write/network tools) is invisible until this file is manually updated—test_unknown_copilot_tool_is_not_reported_as_mcp locks in the regression.
Fix: Emit unknown names as generic entries (e.g. flagged unknown_native, no server_name) rather than dropping them, so new tools surface as low-scored noise instead of a blind spot.
Flagged by: Claude, Cursor
🟡 TRIAGE — Native tool matching is case-sensitive while MCP matching is not
copilot/hooks/unbound.py:809, copilot/hooks/unbound.py:2878
Impact: MCP prefix checks were made case-insensitive, but native set lookups remain exact-case; cased variants like Run_In_Terminal or Create_File miss all buckets, skip policy canonicalization, and are dropped from transcripts.
Fix: Normalize tool names once at entry (name.lower()) and store native set keys lowercased so native and MCP paths share the same casing rule.
Flagged by: Claude
🟡 TRIAGE — Repo-supplied MCP config can relabel suppressed native tools
copilot/hooks/unbound.py:2881
Impact: For UNTRACKED_NATIVE_TOOLS/INTERNAL_TOOLS, a workspace .vscode/mcp.json entry with a colliding server name (e.g. read_bash, run_playwright_code) causes host-tool activity to be emitted as afterMCPExecution with that server_name, laundering native actions into fabricated MCP attribution despite the comment claiming the explicit list prevents MCP config from claiming host tools.
Fix: Make native suppression authoritative (ignore MCP resolution for those names), or emit both server_name and a shadows_native_tool marker when a collision occurs.
Flagged by: Claude
🟡 TRIAGE — Synthetic shell audit strings built from unescaped arguments
copilot/hooks/unbound.py:101
Impact: New rg mapping (and existing grep/glob/file_search helpers) interpolates raw pattern/query into fake shell commands written to afterShellExecution audit records; malicious or misleading patterns can corrupt shell-event analytics/alerting even though nothing is executed.
Fix: Quote interpolated values (shlex.quote) or record raw args in a structured field instead of splicing into a command string.
Flagged by: Claude
Previously acknowledged (not re-flagged)
- Pre-tool policy bypass for effectful
UNTRACKED_NATIVE_TOOLS(copilot/hooks/unbound.py:2084) — @zeus-12: fixed in 82e4aaa; raw-name early return removed so configured MCP resolution runs before internal/untracked names are suppressed, preserving gateway policy evaluation for genuine unsupported host tools.
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head 82e4aaaa · 2026-08-27T20:13Z
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
6 findings — 2 high-confidence, 4 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
🔴 HIGH — Effectful tools in INTERNAL_TOOLS skip policy entirely
copilot/hooks/unbound.py:110-128, :2144-2162, :2907-2924
- Impact:
write_agent,extensions_manage,extensions_reload,copilot_runVscodeCommand, andrun_vscode_commandare treated as non-security-relevant and return early with no PreToolUse gateway call, while closely related tools likeinstall_extensionare policy-checked — leaving extension install, arbitrary VS Code command execution, and persistent agent-definition writes ungoverned and invisible to Stop analytics. - Fix: Move these tools out of
INTERNAL_TOOLSintoUNTRACKED_NATIVE_TOOLSand add them toPOLICY_EFFECTFUL_NATIVE_TOOLS(or introduce a dedicated native-tool policy channel) so they reach the same deny/approve path as other effectful host actions. - Reviewers: Claude, Lead
🔴 HIGH — Effectful-native policy channel sends attacker-controlled args as a synthetic Bash command
copilot/hooks/unbound.py:1900-1903, :2184-2186
- Impact:
_effectful_native_policy_commandbuildsf'{raw_tool} {json.dumps(tool_input)}'and submits it astool_name=Bash/command=…to/v1/hooks/pretool; model-controlled argument text can satisfy allow-rule substrings to bypass denies, fail to match real shell deny patterns, and exfiltrate secrets (signed URLs, bearer tokens, browser-typed credentials intype_in_page/web_fetch) into off-box policy records — contradicting the file's own MCP rule of "Names only — never args/config (those can carry secrets)." - Fix: Send effectful natives under their actual tool name with a whitelisted, redacted field set (strip URL query/userinfo, drop browser
text/valuefields, cap length); do not overload the Bashcommandchannel with free-form serialized input. - Reviewers: Claude, Lead
🟡 TRIAGE — New/renamed Copilot tools are silently dropped (no analytics, no policy)
copilot/hooks/unbound.py:2977-2984
- Impact: Any host tool not on the static allowlists and not resolving as configured MCP now returns
Nonewith no telemetry; future upstream effectful tools would be neither scored nor policy-checked until someone updates the lists — a silent blind spot locked in bytest_unknown_copilot_tool_is_not_reported_as_mcp. - Fix: Suppress only explicitly enumerated
INTERNAL_TOOLS/UNTRACKED_NATIVE_TOOLS; for genuinely unknown names emit a low-signal analytics entry or a distinct log/counter so new tools surface for triage. - Reviewers: Claude, Lead
🟡 TRIAGE — Case normalization is asymmetric (MCP prefix vs native set lookups)
copilot/hooks/unbound.py:836, :1787, :2114, :2144
- Impact: MCP prefixes are matched case-insensitively but
canonical_tool_namereturns mixed-case names unchanged and native membership tests (POLICY_EFFECTFUL_NATIVE_TOOLS,SHELL_TOOLS, etc.) remain exact-case; cased variants likeRun_In_TerminalorMCP__srv__toolmay miss policy/analytics or downstream case-sensitive gateway matchers without the priorUNRESOLVEDlog firing. - Fix: Normalize once at ingress (casefold raw name for all set tests; emit canonical lowercase
mcp__<server>__<tool>); add tests for mixed-case native and MCP names. - Reviewers: Claude, Lead
🟡 TRIAGE — Overly permissive file modes on hook artifacts
copilot/hooks/unbound.py:3303, :3457
- Impact:
0o755/$BITS-style permissions may expose hook-side files to other local users on shared machines. - Fix: Tighten to
0o644(or tighter) unless execute bits are required. - Reviewers: Semgrep
Previously acknowledged (not re-flagged)
- Native tools bypass policy (
run_playwright_code,install_extension,run_notebook_cell) — Fixed in 82e4aaa per @zeus-12: raw-name early return removed; configured MCP resolution runs before native suppression; cited effectful untracked tools now route throughPOLICY_EFFECTFUL_NATIVE_TOOLS.
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head 55f53a0a · 2026-08-28T05:18Z
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
6 findings — 0 high-confidence, 6 to triage. Reviewers: Lead, Claude, Semgrep, Gitleaks.
MEDIUM
Effectful native tools in INTERNAL_TOOLS skip command policy
copilot/hooks/unbound.py:113- Impact:
extensions_manageandwrite_agentreturn early vianative_without_policywith no gateway evaluation, while near-equivalent capabilities (install_extension, file writes) are gated — policy depends on which host alias Copilot invokes. - Fix: Move
extensions_manageintoPOLICY_EFFECTFUL_NATIVE_TOOLS; routewrite_agentthrough the Write/Edit policy path (or the effectful-native Bash path). - Reviewers: Claude, Lead
Model-controlled arguments concatenated into synthetic Bash command for policy matching
copilot/hooks/unbound.py:1900- Impact:
_effectful_native_policy_commandbuildsf'{raw_tool} {json.dumps(tool_input)}'and submits it as a shell command; injected argument text can satisfy substring/regex allow rules, pollute audit logs, and carry sensitive page-typed content (e.g.type_in_page). - Fix: Evaluate policy on structured fields (
tool_name+ allow-listed argument keys) instead of a free-form command string; if the command shape must remain, shell-quote values and cap payload length. - Reviewers: Claude, Lead
Native tool catalog lookups are case-sensitive while MCP matching is case-insensitive
copilot/hooks/unbound.py:2139- Impact: A host alias that differs only in case (e.g.
Install_Python_Packagesvsinstall_python_packages) misses every native set, falls through unmatched, and returns{}— the effectful action runs with no policy check. - Fix: Normalize
raw_toolonce (lowercase) and store/test all native catalogs case-insensitively, consistent with themcp_prefix handling. - Reviewers: Claude, Lead
LOW
Unrecognized native tools silently dropped from Stop analytics
copilot/hooks/unbound.py:2972- Impact: Renamed or newly shipped Copilot tools that miss the hardcoded catalog emit no transcript row and no log on the Stop path, so catalog drift degrades coverage without an alert (PreToolUse at least logs
unmatched tool=). - Fix: On the unrecognized-name branch, emit a rate-limited
log_error(..., 'tool_catalog')while keeping the suppress-analytics behavior. - Reviewers: Claude, Lead
Untrusted workspace MCP config can poison server_name in analytics
copilot/hooks/unbound.py:2980- Impact: Repo-controlled
.vscode/mcp.json/.mcp.jsonkeys are copied intoentry['server_name'], letting a contributor attribute MCP activity to a trusted server name in dashboards (reporting/attribution only; PreToolUse policy path is no longer affected). - Fix: Record provenance alongside the name (e.g.
server_name_source: 'workspace_config') or use gateway-side server fingerprinting for attribution. - Reviewers: Claude, Lead
Overly permissive file permissions (pre-existing, outside this diff)
copilot/hooks/unbound.py:3353,copilot/hooks/unbound.py:3507- Impact:
$BITSand0o755grant broader filesystem access than0o644; unrelated to the classification changes in this PR but flagged on full-file scan. - Fix: Tighten to
0o644(or0o600for sensitive files) unless wider permissions are required. - Reviewers: Semgrep
Previously acknowledged (not re-flagged)
- Native tools bypass policy (
copilot/hooks/unbound.py:2084, Greptile P1) — Fixed in 82e4aaa; raw-name early return removed so MCP resolution runs before suppression. - Workspace MCP config shadowing native tools on the PreToolUse policy path (
copilot/hooks/unbound.py:2169, Cursor) — Fixed in 29e448a; known native tool identity is authoritative over untrusted workspace MCP configuration.
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head 29e448af · 2026-08-28T08:36Z
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
5 findings — 1 high-confidence, 4 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
🔴 HIGH
Backfill executes a user-writable hook module without integrity checks
copilot/hooks/setup.py:611-630,copilot/hooks/mdm/setup.py:794-813- Impact:
_backfill_load_hook_module()exec_modules~/.copilot/hooks/unbound.py, which any process running as the user (including a prompt-injected Copilot agent) can rewrite; the next backfill run executes attacker-controlled code (privilege-boundary risk if the MDM installer ever runs elevated). - Fix: Import MCP resolution from the installer’s trusted, version-pinned artifact (verify SHA-256/signature) instead of executing the live home-directory hook, or refuse to load when the path or any parent is not owned by the current uid or is group/world-writable.
- Flagged by: Claude, Lead
🟡 TRIAGE
Extension-management native tools skip command policy
copilot/hooks/unbound.py:120-124- Impact:
extensions_manageandextensions_reloadare inINTERNAL_TOOLSand return early with no gateway call, while siblinginstall_extensionis inPOLICY_EFFECTFUL_NATIVE_TOOLS; an agent can reach extension install/enable behavior without deny/approval policy or audit coverage. - Fix: Reclassify
extensions_manage(andextensions_reloadif it can load newly written extensions) intoPOLICY_EFFECTFUL_NATIVE_TOOLSso they route through the Bash policy path likeinstall_extension. - Flagged by: Claude
write_bash / write_powershell pretool command may be empty
copilot/hooks/unbound.py:70-73,copilot/hooks/unbound.py:2241-2244- Impact: These tools were added to
SHELL_TOOLSand the transcript mapper readscommand/input/text, but pretool still calls unchangedextract_command_for_pretool; if it only readscommand, shell sent viainputreaches the gateway as an empty Bash command and bypasses terminal deny/approval rules. - Fix: Share the same
command/input/textfallback inextract_command_for_pretool(or a shared helper) and add a pretool regression test forwrite_bash/write_powershellwith{"input": ...}. - Flagged by: Claude
Backfill MCP resolution trusts transcript-supplied cwd
copilot/hooks/setup.py:640-652,copilot/hooks/mdm/setup.py:823-835- Impact: The first
cwdstring from transcript entries is passed directly toread_copilot_mcp_servers, so crafted transcript data can point config resolution at an arbitrary directory and forgeserver_namein uploadedmcp_tool_provenance. - Fix: Validate transcript
cwdagainst enumerated workspace roots (e.g._backfill_vscode_workspace_roots) before reading MCP config; treat clientserver_nameas a hint the server re-verifies. - Flagged by: Claude
Effectful-native policy payloads may exfiltrate secrets to the gateway
copilot/hooks/unbound.py:1951-1955- Impact:
_effectful_native_policy_commandJSON-serializes full tool arguments into the Bashcommandfield sent to the gateway, which can include URL query tokens, notebook/Playwright bodies, and other credentials that previously never left the host. - Fix: Cap serialized length and redact credential-bearing fields (URL userinfo/query, keys matching
token|secret|key|password) before policy submission, consistent with the “names only — never args/config” logging rule nearby. - Flagged by: Claude
Previously acknowledged (not re-flagged)
- Effectful native tools bypassing gateway policy (
copilot/hooks/unbound.py:2084) — fixed in 82e4aaa; maintainer removed the raw-name early return so MCP resolution runs before suppression. - Workspace
mcp.jsonshadowing native tool identity and skipping Bash policy (copilot/hooks/unbound.py:2220) — fixed in 29e448a; maintainer made known native tools authoritative over untrusted workspace MCP config, with collision regressions added.
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head c5533eea · 2026-08-28T11:16Z
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
5 findings — 2 high-confidence, 3 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
MEDIUM · 🔴 HIGH — Silent fail-open when bare MCP tools cannot be resolved
copilot/hooks/unbound.py:2094
Impact: Unresolved bare tool names now return {} with no log_error; if workspace MCP config is unreadable or missing, genuine MCP calls skip the allow-list with no telemetry.
Fix: Restore mcp_config / mcp_match logging on the unresolved path (suppress only for known-native names, not all unmatched tools).
Flagged by: Claude, Lead
MEDIUM · 🔴 HIGH — Backfill installer executes unverified hook from home directory
copilot/hooks/setup.py:611, copilot/hooks/mdm/setup.py:794
Impact: _backfill_load_hook_module() runs ~/.copilot/hooks/unbound.py via exec_module with no hash/signature check; anyone who can write that path gets code execution in the installer (MDM path may run with higher privilege).
Fix: Load only from the trusted install path the installer wrote, verify an expected hash before exec_module, and refuse load if the file or parent dir is writable by untrusted users.
Flagged by: Claude, Lead
LOW · 🟡 TRIAGE — Workspace MCP prefix can still claim native tool names
copilot/hooks/unbound.py:1762
Impact: Exact server-key collisions are blocked, but a configured server whose name is a prefix (e.g. run, install) still resolves native tools like run_vscode_command / install_python_packages as MCP, polluting analytics/backfill provenance.
Fix: Reject detect_mcp_call when the raw tool name is in the known native catalog before prefix matching (mirror the pre-tool TERMINAL_LIKE_TOOLS guard).
Flagged by: Claude
LOW · 🟡 TRIAGE — Backfill trusts transcript-supplied cwd for MCP config reads
copilot/hooks/setup.py:640, copilot/hooks/mdm/setup.py:823
Impact: First cwd in transcript entries drives read_copilot_mcp_servers(cwd), so a crafted transcript can steer config reads and upload attacker-chosen server names as provenance.
Fix: Only accept cwd inside known workspace roots (_backfill_vscode_workspace_roots); otherwise pass None.
Flagged by: Claude
LOW · 🟡 TRIAGE — write_bash / write_powershell policy command extraction unverified
copilot/hooks/unbound.py:68
Impact: Both names were added to SHELL_TOOLS (payload under input, not command); if the pre-tool extractor only reads command/commandLine, policy may allow an empty command while shell text still executes.
Fix: Add a pre-tool regression test and extend extraction to fall back to input for these tools.
Flagged by: Claude
Previously acknowledged (not re-flagged)
- Effectful native tools bypass org command policy (
install_python_packages,run_playwright_code, etc.) — @zeus-12: accepted design choice (“without inventing fake Bash/Write semantics”);test_unmapped_native_tool_is_ignoredmatches. - Native-tool early return skipped gateway policy (Greptile @2084) — @zeus-12: fixed in 82e4aaa; raw-name early return removed.
- Workspace MCP exact-name collision disabled command policy for native tools (Cursor @2163) — @zeus-12: fixed in 29e448a; known native tools now precede workspace MCP config; collision regressions pass.
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head 085e0e4f · 2026-08-28T11:26Z
There was a problem hiding this comment.
Stale comment
Agentic security review of this PR identified one new high-severity issue in MDM backfill: the new hook-module load path executes a user-writable
unbound.pyin a dropped-privilege child, then the root parent unpickles the child's result.Sent by Cursor Security Agent: Security Reviewer
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
3 findings — 0 high-confidence, 3 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks, Lead.
Silent MCP allow-list bypass with removed observability
copilot/hooks/unbound.py:2094 · 🟡 TRIAGE
Impact: Bare/non-mcp__ tools that fail MCP resolution return {} without gateway evaluation and without the prior log_error telemetry, so org MCP allow-list gaps become invisible when config is missing, unreadable, or non-matching.
Fix: Keep the fail-open return if required, but restore log_error for unresolved non-native tools (config empty/unreadable vs. unmatched name).
Flagged by: Claude, Lead
Transcript cwd can steer MCP config reads and forge provenance
copilot/hooks/setup.py:627, copilot/hooks/mdm/setup.py:808 · 🟡 TRIAGE
Impact: Backfill takes the first transcript cwd verbatim and passes it to read_copilot_mcp_servers, so a user-editable JSONL transcript can point at arbitrary .vscode/mcp.json / .mcp.json paths and attach forged server_name values to uploaded mcp_tool_provenance audit data.
Fix: Validate cwd (existing directory, within discovered workspace roots) before use; otherwise pass None and fall back to user-level MCP config only.
Flagged by: Claude, Lead
Backfill MCP classification executes user-writable installed hook
copilot/hooks/setup.py:614, copilot/hooks/mdm/setup.py:797 · 🟡 TRIAGE
Impact: _backfill_load_hook_module exec_modules ~/.copilot/hooks/unbound.py (owner-writable) to resolve MCP provenance, so a user can control how their historical tool calls are labeled before upload (integrity gap; root pickle IPC from earlier reviews is closed by JSON IPC in this diff).
Fix: Import classification helpers from the installer's bundled hook copy (or verify ownership/mode and refuse exec_module when effective uid is 0); keep JSON IPC for _run_as_user.
Flagged by: Claude, Cursor (partial — LPE vector fixed), Lead
Previously acknowledged (not re-flagged)
- Unmapped effectful native tools skip gateway policy (
install_python_packages, etc. return{}withoutsend_to_hook_api) — maintainer: intentional for unsupported host tools; raw-name early return removed;test_unmapped_native_tool_is_ignoredpins the behavior (zeus-12). - Workspace MCP server name colliding with native tool names — maintainer: fixed in 29e448a; known native tools take precedence over workspace MCP config (zeus-12).
- MDM
_run_as_userrootpickle.loadsIPC — maintainer: fixed in this PR by replacing pickle with JSON serialization;test_parent_rejects_pickle_from_the_unprivileged_childadded (diff + zeus-12 thread context). - Semgrep
0o700/0o755file-permission warnings — restrictive hook/installer permissions by design; Semgrep rule misfires on intentionally private executables.
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head b39978fb · 2026-08-28T11:44Z
|
Context for the remaining Greptile note: unsupported native tools already return |
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
4 findings — 0 high-confidence, 4 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
🟡 Unresolved MCP / unmatched tools no longer logged
copilot/hooks/unbound.py:2087
Impact: When MCP resolution fails (mcp_server is None), the hook returns {} with no log_error, so unreadable MCP config and unmatched bare tools are indistinguishable from a normal no-op and cannot be detected in telemetry.
Fix: Restore structured logging on this path (tool name only; no args/config), at minimum when mcp_servers is empty/unreadable.
Flagged by: Claude, Lead
🟡 Case-insensitive MCP detection vs case-preserved gateway forwarding
copilot/hooks/unbound.py:793
Impact: Local classification now lowercases mcp prefixes, but unresolved/explicit MCP names are still forwarded in original case; if gateway allow-list matching is case-sensitive, mixed-case invocations (e.g. MCP__github__delete_repo) may skip MCP sanctioning.
Fix: Normalize to canonical lowercase mcp__<server>__<tool> before forwarding, or send explicit mcp_server / mcp_tool fields the gateway can match case-insensitively.
Flagged by: Claude, Lead
🟡 Transcript cwd drives MCP config lookup during backfill
copilot/hooks/setup.py:617, copilot/hooks/mdm/setup.py:816
Impact: Backfill takes cwd from user-writable transcript JSONL and passes it to read_copilot_mcp_servers(cwd) without validation, so a crafted path can steer which MCP server names are attributed in mcp_tool_provenance (analytics integrity).
Fix: Resolve and validate cwd (existing dir, within known workspace/home roots) before reading MCP config; fall back to safe defaults when invalid.
Flagged by: Claude, Lead
🟡 _run_as_user JSON IPC failures are silent
copilot/hooks/mdm/setup.py:89
Impact: Non-JSON-serializable child return values trigger os._exit(1) with no logged reason; the parent returns None, which callers may treat like a generic permission failure and leave MDM install/backfill steps partially applied.
Fix: Audit all _run_as_user call sites for JSON-safe returns; log serialization errors in the child (stderr/debug) before exit.
Flagged by: Claude, Lead
Previously acknowledged (not re-flagged)
- Effectful unsupported native tools skip gateway policy (
unbound.py:2084/2090) — @zeus-12: preserves existingstagingbehavior; no generic native-tool gateway event exists, and generic native policy coverage is out of scope for this PR. - Workspace
mcp.jsoncould relabel native tools as MCP — fixed in29e448a; known native tools now take precedence over workspace MCP config. - MDM root
pickle.loadson child IPC (local privilege escalation) — fixed inb39978f; parent now JSON-decodes only. - Raw-name early return before configured-MCP resolution — fixed in
82e4aaa.
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head 8fc1cf9b · 2026-08-28T12:53Z
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
4 findings — 2 high-confidence, 2 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
🔴 HIGH — Core Copilot edit/write tools no longer reach org policy
copilot/hooks/unbound.py:69-71
- Impact:
insert_edit_into_file,multi_replace_string_in_file,edit_notebook_file,edit,edit_files, andcreate_directorywere removed fromEDIT_TOOLS/WRITE_TOOLS, socanonical_tool_name()returns'', MCP resolution fails, and PreToolUse returns{}before any gateway call — org file-write/edit and out-of-org-repo deny/approval policies no longer apply to primary VS Code Copilot mutations (regression, not the scoped “unsupported native” case). - Fix: Restore those live VS Code/CLI aliases to
EDIT_TOOLS/WRITE_TOOLSand add pretool regressions asserting each still reaches the gateway. - Flagged by: Claude, Cursor
🔴 HIGH — send_to_terminal shell invocations skip command policy
copilot/hooks/unbound.py:68
- Impact:
send_to_terminalwas dropped fromSHELL_TOOLSwhileextract_command_for_pretoolstill readsinput/textfor Bash — terminal commands via this tool canonicalize to'', return{}, and never hit org terminal/command deny rules. - Fix: Re-add
send_to_terminaltoSHELL_TOOLSand add a pretool test that a deny decision is enforced. - Flagged by: Claude, Lead
🟡 TRIAGE — Prefix MCP config can still mislabel native tools
copilot/hooks/unbound.py:1731
- Impact: Exact-name collisions are fixed, but
detect_mcp_callstill prefix-matches workspace.vscode/mcp.jsonserver keys (e.g. serverrun→ nativerun_playwright_codeasrun/playwright_code), fabricating MCP attribution in pretool and backfill provenance and potentially skewing MCP allow-list/analytics. - Fix: Reject bare-name resolution when the raw tool name matches a known native catalog entry by prefix (not only exactly), or require separator-delimited matches against server-advertised tool names.
- Flagged by: Claude
🟡 TRIAGE — Backfill trusts transcript cwd for MCP config lookup
copilot/hooks/setup.py:625, copilot/hooks/mdm/setup.py:812
- Impact:
_backfill_mcp_tool_provenanceuses the firstcwdstring from transcript JSONL to callread_copilot_mcp_servers(cwd); tampered transcripts can steer config resolution and poison uploadedmcp_tool_provenance(user-context only, no privilege escalation). - Fix: Validate
cwdagainst known workspace roots (e.g._backfill_vscode_workspace_roots) before use; otherwise passNone. - Flagged by: Claude
Previously acknowledged (not re-flagged)
- Unsupported effectful native tools return
{}without gateway evaluation — @zeus-12: preserved existing staging behavior; no generic native-tool gateway event exists yet; separate scope. - Workspace MCP server name exactly matching a native tool — @zeus-12: fixed in 29e448a; known natives now take precedence over workspace MCP config.
- MDM root
pickle.loadson child IPC — @zeus-12: fixed in b39978f; parent now JSON-decodes only. - Raw-name early return before configured-MCP resolution — @zeus-12: fixed in 82e4aaa.
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head 729bccb8 · 2026-08-28T15:43Z
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
6 findings — 2 high-confidence, 4 to triage. Reviewers: Lead, Claude, Cursor, Semgrep, Gitleaks.
🔴 HIGH — Write/edit native aliases removed from policy catalogs
copilot/hooks/unbound.py:69-73, copilot/hooks/unbound.py:2164-2167
Impact: Live VS Code/CLI edit tools (insert_edit_into_file, multi_replace_string_in_file, create_directory, edit, edit_notebook_file, etc.) no longer canonicalize to Write/Edit; they fail MCP resolution and PreToolUse returns {}, so org deny/approval and repo-scope gates never run.
Fix: Restore the removed aliases in WRITE_TOOLS/EDIT_TOOLS (and any read aliases still needed) so they reach the gateway as before; add regression tests per alias.
Flagged by: Lead, Claude, Cursor
🔴 HIGH — Workspace MCP config can exfiltrate arbitrary local files via mcp-scan
copilot/hooks/unbound.py:1055, copilot/hooks/unbound.py:3897
Impact: _read_script_body_b64 reads up to 256 KB from any path resolved from untrusted workspace .mcp.json / .vscode/mcp.json command values (including ~/.ssh/id_rsa via traversal) and embeds it in UNBOUND_MCP_SERVER_JSON when the gateway returns unknown_mcp_server.
Fix: Do not upload script bodies from repo-controlled config; keep scriptHash only, constrain reads to an approved root, reject symlinks, and require executable/script semantics.
Flagged by: Lead, Claude
🟡 TRIAGE — MCP arg forwarding changed from allowlist to incomplete denylist
copilot/hooks/unbound.py:945
Impact: _redact_args now forwards all args except pattern-matched secrets, so forms like --private-key, --pat, bare positional tokens, and non-URL DSNs can reach the gateway on PreToolUse and in backfill mcp_server_config provenance.
Fix: Restore the prior allowlist (URLs + scoped npm packages) for forwarded args, or redact positionally against a known-safe schema; compute fingerprints from full args before stripping.
Flagged by: Lead, Claude
🟡 TRIAGE — Untrusted workspace/plugin MCP config can override user server identity
copilot/hooks/unbound.py:838
Impact: Merge order now makes plugin/workspace configs win over user/global settings, so a repo or plugin bundle can replace the command/url fingerprint the gateway uses for a server name the user configured themselves.
Fix: If mirroring Copilot order is required, attach source provenance and refuse untrusted overrides for policy/fingerprint decisions, or ensure user/trusted configs always win for sanctioning.
Flagged by: Lead, Claude
🟡 TRIAGE — Config-supplied _unbound_fingerprint can steer ambiguous MCP resolution
copilot/hooks/unbound.py:1862
Impact: _vscode_fingerprint_key trusts _unbound_fingerprint from the server config dict (fed by workspace files), which may force ambiguous mcp_<server>_<tool> tokens to resolve instead of failing closed.
Fix: Store hook-computed fingerprints in a separate map keyed by server name; never read fingerprint identity from unsanitized config input.
Flagged by: Claude
🟡 TRIAGE — Discovery installer downloaded without integrity verification
copilot/hooks/unbound.py:3766
Impact: _ensure_discovery_installer curls and executes install.sh with no checksum/signature check; the new _dispatch_mcp_server_scan path makes this reachable from workspace-triggered unknown-MCP scans, widening TOCTOU/tamper windows.
Fix: Pin and verify a signed hash before chmod/exec, and create the file with O_EXCL in a user-owned directory.
Flagged by: Claude
Previously acknowledged (not re-flagged)
- Unsupported native tools skip gateway policy (
unbound.py:2162-2167) — @zeus-12: preserved fromstaging; no generic native-tool gateway event exists, and fabricating Bash/MCP semantics would recreate the classification bug; generic native policy is separate scope. - Workspace MCP config relabeling native host tools as MCP — @zeus-12: fixed in
29e448a; known native tools now take precedence over workspace MCP configuration. - MDM backfill root privilege escalation via pickle IPC — @zeus-12: fixed in
b39978f; child results now cross the pipe as JSON only (exec_moduleat user privilege remains intentional for provenance resolution).
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head 727045f3 · 2026-08-29T06:45Z
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
3 findings — 0 high-confidence, 3 to triage. Reviewers: Lead, Claude, Semgrep, Gitleaks.
Findings
🟡 TRIAGE — MCP arg redaction widened from allowlist to denylist
copilot/hooks/unbound.py:962
- Impact:
_redact_argsnow forwards every string arg (regex-redacted only) instead of keeping URL/@packageallowlist entries; forms like-p <secret>,--pat, and bare positional credentials can reachmcp_server_config, gateway metadata, analytics, andUNBOUND_MCP_SERVER_JSON. - Fix: Restore the allowlist as the outer filter and apply the new flag/assignment redaction inside it; if fingerprinting needs more context, send
_unbound_fingerprintrather than raw args. - Reviewers: Claude, Lead
🟡 TRIAGE — Workspace-controlled paths can upload local file contents during MCP scan
copilot/hooks/unbound.py:3891
- Impact:
_dispatch_mcp_server_scanbase64-encodes up to 256 KB from a path derived from repo/workspace.mcp.jsoncommand/argsand ships it off-box viaUNBOUND_MCP_SERVER_JSON; a malicious config can point at sensitive local files (keys, creds) that previously only produced a hash. - Fix: Keep hash-only for workspace-sourced configs, or upload
script_contentonly when the resolved path is under a trusted base (plugin bundle / non-workspace config) and reject paths outside that root. - Reviewers: Claude, Lead
🟡 TRIAGE — Package-level fingerprint can collapse distinct MCP servers
copilot/hooks/unbound.py:1863
- Impact:
_vscode_fingerprint_keyprefers_unbound_fingerprint(e.g.npm:pg-mcp) over url/command identity, so two differently configured servers built from the same package compare equal and ambiguousmcp_<…>names resolve to whichever candidate wins sorting—steering allow-list evaluation. - Fix: Use fingerprint only as a tiebreaker after url/command+args, or include distinguishing fields (url, env keys, full args) in the identity key.
- Reviewers: Claude
Previously acknowledged (not re-flagged)
- Unsupported native tools skip gateway policy — @zeus-12: preserves existing
stagingbehavior whencanonical_tool_name()and MCP resolution both fail; generic native-tool policy is out of scope for this PR. - Early native-tool return before MCP resolution — fixed in
82e4aaa; configured MCP resolution now runs before suppression. - Workspace MCP name collision relabeling natives to bypass Bash policy — fixed in
29e448a; known native tools take precedence over workspace MCP config. - MDM backfill pickle IPC → root
pickle.loadsLPE — fixed inb39978f; child results are JSON-only across the pipe. - Dropped write/edit native aliases bypassing file policy — fixed in
c124516;create_directory,insert_edit_into_file,multi_replace_string_in_file,edit_notebook_file, and CLIeditrestored. - Workspace/plugin MCP config overrides user config — intentional design to mirror Copilot resolution order (
user < workspace < plugin); covered by PR description and updated tests.
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head c1245162 · 2026-08-29T07:25Z
There was a problem hiding this comment.
Agentic security review of the Copilot native/MCP classification changes found three issues on the current head: unverified envelope MCP identity used for authorization, MCP argv forwarding widened from an allowlist to an incomplete denylist, and local MCP script bodies included in mcp-scan config. Prior findings on write/edit alias coverage, workspace MCP name collisions, and pickle IPC were checked against this head and are no longer outstanding.
Sent by Cursor Security Agent: Security Reviewer
| def resolve_copilot_mcp(raw_tool, mcp_servers, server_name=None, tool_name=None): | ||
| if (isinstance(server_name, str) and server_name | ||
| and isinstance(tool_name, str) and tool_name): | ||
| return (server_name, tool_name, mcp_servers.get(server_name)) | ||
| lowered = (raw_tool or '').lower() | ||
| if lowered.startswith(_GITHUB_BUILTIN_MCP_PREFIX): | ||
| tool = raw_tool[len(_GITHUB_BUILTIN_MCP_PREFIX):] | ||
| if tool: | ||
| server = _GITHUB_BUILTIN_MCP_PREFIX[:-1] | ||
| return (server, tool, mcp_servers.get(server)) | ||
| if lowered.startswith('mcp_') and not lowered.startswith('mcp__'): |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
resolve_copilot_mcp() treats envelope mcpServerName / mcpToolName as authoritative MCP identity without binding them to tool_name or a configured-server parse. _evaluate_pre_tool_use_policies() then rewrites the gateway tool to mcp__{server}__{tool} and attaches that server's config, so an unsanctioned call can be scored as an allowlisted server. Copilot still executes the original tool.
Impact: Organization MCP allowlists and deny/approval policies can be evaluated against a spoofed identity while the real tool runs.
Reviewed by Cursor Security Reviewer for commit c124516. Configure here.
| def _redact_args(args): | ||
| if not isinstance(args, list): | ||
| return args | ||
| kept = [] | ||
| redacted = [] | ||
| redact_next = False | ||
| for arg in args: | ||
| if not isinstance(arg, str): | ||
| continue | ||
| if redact_next: | ||
| redacted.append(_REDACTED) | ||
| redact_next = False | ||
| continue | ||
| if _SENSITIVE_ARG_FLAG_RE.match(arg) or arg in _SENSITIVE_ARG_VALUE_FLAGS: | ||
| redacted.append(arg) | ||
| redact_next = True | ||
| continue | ||
| inline_flag, separator, _value = arg.partition('=') | ||
| if separator and inline_flag in _SENSITIVE_ARG_VALUE_FLAGS: | ||
| redacted.append(f'{inline_flag}={_REDACTED}') | ||
| continue | ||
| assignment = _SENSITIVE_ARG_ASSIGNMENT_RE.match(arg) | ||
| if assignment: | ||
| redacted.append(f'{assignment.group(1)}={_REDACTED}') | ||
| continue | ||
| if '://' in arg: | ||
| kept.append(_redact_url(arg)) | ||
| elif arg.startswith('@'): | ||
| kept.append(arg) | ||
| return kept | ||
| redacted.append(_redact_url(arg)) | ||
| else: | ||
| redacted.append(_TOKEN_RE.sub(_REDACTED, arg)) | ||
| return redacted |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
_redact_args() no longer allowlists fingerprint-only argv (URLs and @ packages). It now forwards every string argument after a denylist of a few flags, KEY=value names, and known token regexes. Residual values — including --db-pass, --api-token, and positional secrets — are attached as mcp_server_config.args on PreToolUse, transcript backfill, and mcp-scan.
Impact: Credentials and other secrets in MCP launch args that never left the host on staging can now be uploaded to Unbound.
Reviewed by Cursor Security Reviewer for commit c124516. Configure here.
| if server_config.get('command') and not server_config.get('script_content'): | ||
| script_body = _read_script_body_b64( | ||
| server_config.get('command'), server_config.get('args'), cwd | ||
| ) | ||
| if script_body: | ||
| server_config = {**server_config, 'script_content': script_body} | ||
| with UNBOUND_CONFIG_PATH.open("r", encoding="utf-8") as f: | ||
| unbound_config = json.load(f) | ||
| api_key = unbound_config.get("api_key") | ||
| backend_url = unbound_config.get("base_url") | ||
| if not api_key or not backend_url: | ||
| return | ||
|
|
||
| if RUNNING_FROZEN: | ||
| if not os.path.isfile(FROZEN_DISCOVERY_BIN): | ||
| return | ||
| scan_cmd = [FROZEN_DISCOVERY_BIN, "mcp-scan", "--name", server_name, | ||
| "--domain", backend_url] | ||
| else: | ||
| if not _ensure_discovery_installer(): | ||
| return | ||
| scan_cmd = [ | ||
| "bash", str(DISCOVERY_INSTALL_SH), "mcp-scan", "--name", server_name, | ||
| "--domain", backend_url, | ||
| ] | ||
|
|
||
| popen_kwargs = { | ||
| "stdout": subprocess.DEVNULL, | ||
| "stderr": subprocess.DEVNULL, | ||
| "stdin": subprocess.DEVNULL, | ||
| "close_fds": True, | ||
| "env": { | ||
| **os.environ, | ||
| "UNBOUND_API_KEY": api_key, | ||
| "UNBOUND_MCP_SERVER_JSON": json.dumps(server_config), |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
New Copilot mcp-scan dispatch reads the local MCP launch script (up to 256KiB, base64) into script_content and serializes the whole object as UNBOUND_MCP_SERVER_JSON. Combined with the broader argv forwarding, unknown-server discovery now ships source and config that previously stayed local.
Impact: User or workspace MCP scripts and remaining launch arguments are sent to the discovery/mcp-scan path without an identity-only projection.
Reviewed by Cursor Security Reviewer for commit c124516. Configure here.




Summary
mcp_/mcp__names or the users configured MCP serversSources:
Verification
python -m pytest -q tests/copilot/hooks— 169 passedGreptile Summary
The PR distinguishes verified Copilot-native aliases from MCP calls, dynamically resolves MCP identity from configured servers, and adds resolved provenance to transcript backfills.
Confidence Score: 4/5
The PR is not yet safe to merge because unsupported native tools without a configured MCP match still bypass organization policy evaluation.
The unresolved-native branch returns an empty response before constructing or sending the gateway request, leaving the previously reported policy bypass outstanding.
Files Needing Attention: copilot/hooks/unbound.py
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Copilot PreToolUse] --> B[Canonical native alias mapping] B --> C{Supported native tool?} C -->|Yes| D[Gateway policy evaluation] C -->|No| E[Resolve against configured MCP servers] E --> F{Resolved MCP identity?} F -->|Yes| D F -->|No| G[Return empty response] H[Transcript backfill] --> I[Resolve MCP provenance] I --> J[Attach provenance by tool call ID] J --> K[Chunk and upload session]Reviews (11): Last reviewed commit: "fix(copilot): harden MCP attribution" | Re-trigger Greptile
Context used: