Enforce the enterprise MCP allow/deny policy on the agent-host path - #333291
Draft
joshspicer wants to merge 2 commits into
Draft
Enforce the enterprise MCP allow/deny policy on the agent-host path#333291joshspicer wants to merge 2 commits into
joshspicer wants to merge 2 commits into
Conversation
`ChatAllowedMcpServers` / `ChatDeniedMcpServers` were enforced only in
`McpServer`, which governs servers VS Code launches itself. A delegated
agent-host session never goes through that code: eligible servers are
projected into a synthetic synced plugin and handed to a separate process
that launches them on its own, so a blocked server connected and served
tools there while being correctly blocked for the local agent.
Evaluate the policy in `resolveMcpServerForAgentHostDelivery`, the single
choke point shared by the forwarding path and the support assessment, so
that a blocked server is both withheld from the bundle and reported
honestly in the UI. The check runs against the resolved configuration that
would actually be forwarded, and covers `http` and `stdio` servers alike; a
configuration whose URL or command still carries unresolved `${...}`
variables cannot match an allow entry and so fails closed.
Both scopes now also recompute when the policy changes, so a newly blocked
server is withdrawn instead of lingering for the life of the session.
The identity derivation shared by every enforcement path moves to
`mcpServerIdentityFromConfiguration` so the delegated path cannot drift
from the local one.
Host-discovered workspace `.mcp.json` servers remain out of scope: the
agent host finds those itself and never consults the client.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (5)
| Severity | Finding |
|---|---|
 |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostMcpServerSupport.ts — This gate is bypassed by the earlier AgentPlugin and ProviderBuiltIn returns (lines 239–245). A… |
 |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostMcpServerSupport.ts — This does not provide the documented fail-closed behavior for unresolved URLs or commands.… |
 |
src/vs/workbench/contrib/chat/test/browser/agentSessions/resolveCustomizationRefs.test.ts — The resolver-specific JSDoc now attaches to makeAllowedMcpServersService, where it describes… |
|
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostMcpServerSupport.ts — Condense this method-body comment to one line; repository guidance caps inline comments at one… |
|
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostActiveClientService.ts — Condense this method-body comment to one line; repository guidance caps inline comments at one line. |
What changed in this PR
Enforces enterprise MCP allow/deny policies for MCP servers forwarded to delegated Agent Host sessions.
Changes:
- Applies policy matching to resolved forwarded configurations.
- Refreshes Agent Host customizations when policy changes.
- Adds shared identity derivation and policy tests.
| File | Description |
|---|---|
allowedMcpServers.ts |
Adds shared MCP identity derivation. |
allowedMcpServersService.ts |
Reuses shared identity logic. |
agentHostMcpServerSupport.ts |
Adds policy-based delivery blocking. |
agentHostMcpServerSupportScope.ts |
Refreshes support assessments on policy changes. |
agentHostLocalCustomizations.ts |
Filters blocked forwarded servers. |
agentHostActiveClientService.ts |
Republishes customizations after policy changes. |
resolveCustomizationRefs.test.ts |
Adds enterprise policy tests. |
agentHostMcpServerSupport.test.ts |
Updates support test dependencies. |
agentHostClientTools.test.ts |
Adds policy-service test stubs. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ables
Address review feedback on the MCP policy enforcement.
The policy check ran after the `AgentPlugin` and `ProviderBuiltIn` early
returns, so a denied server contributed by a client-synced plugin, or the
provider's built-in GitHub replacement, stayed available and was reported as
supported even though the local `McpServer` path blocks it. Move the check
ahead of both branches so every handoff path is gated, not just client
forwarding. A plugin syncs as a directory rather than a per-server config, so
a denied child is withheld by marking it disabled in `childEnablement`, which
the host maps to `disabledMcpServers`.
The documented fail-closed behavior was also not implemented: `isServerAllowed`
still returns `true` for a matching `serverName`, or for a `serverUrl` wildcard
that happens to match the literal `${...}` text. Forwarding is the last point
at which the client can refuse -- unlike `McpServer`, which re-evaluates once
the launch resolves -- so an allow verdict that relied on unresolved text is
now re-checked with that field dropped and blocked unless the server is still
allowed on its name alone. An allowlist entry matching by name is honoured,
since the URL never affected that verdict.
Also derive the policy identity from the launch the same way `McpServer` does,
so a definition without a usable command still matches `serverName` rules.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #328241
The bug
ChatAllowedMcpServers/ChatDeniedMcpServersare enforced inMcpServer(_policyBlock/_evaluatePolicy), which governs MCP servers VS Code launches itself. A delegated agent-host session never goes through that code.Instead,
resolveCustomizationRefs->collectNonPluginMcpServersprojects eligible servers into a synthetic synced plugin's.mcp.jsonand hands it to the agent host, which is a separate process that launches the server on its own. Nothing on that path consulted the allow/deny policy, so a server blocked for the local agent connected normally in a delegated session.This matches the customer report exactly: with an allowlist of
["npx","--yes","--registry","..."],deepwiki(type: http) was blocked for the local agent but connected and answeredread_wiki_structurein a delegated session.Developer: Policy Diagnosticsreported the policy as applied because it was applied - just not on this path.The fix
Evaluate the policy in
resolveMcpServerForAgentHostDelivery, ahead of every delivery branch, so client forwarding, plugin-contributed servers and provider built-ins are all gated. A blocked server becomesNotDeliveredwith a newBlockedByPolicyreason, so it is both withheld from the bundle and reported honestly in the UI rather than being shown as forwarded.Details worth reviewing:
httpandstdioidentically and matches the same fields the local path matches.${...}, the server is re-checked with that field dropped and blocked unless it is still allowed on its name alone. So aserverUrlwildcard that merely matched the literal${...}text does not grant access, while an allowlist entry matching byserverNameis honoured (the URL never affected that verdict).McpServercan defer this until launch resolves; here, forwarding is the last decision point.AgentCustomizationScopeandAgentHostMcpServerSupportScopenow recompute ononDidChangeAllowedMcpServers, so a newly blocked server is withdrawn instead of lingering for the life of the session.mcpServerIdentityFromConfiguration, used byAllowedMcpServersServiceand by this path, so the two enforcement points cannot drift.Explicitly out of scope
Servers the agent host discovers itself — workspace-root
.mcp.json(AgentHostMcpServerDelivery.RuntimeDiscovered) — are unaffected. The client never forwards those, and the agent host has no access tochat.mcp.allowedServers(the setting declares noagentHostSync, andallowedMcpServersis not part ofIAgentHostManagedSettingsPermissions). Closing that remaining gap means plumbing the matchers to the host and gating per provider, which is a separate change and partly runtime-side.One further limit worth calling out: for
ProviderBuiltIn(a provider's own bundled GitHub MCP server) the client can now correctly report the block, but it cannot stop the provider from starting its own built-in server — that also needs host-side support.Tests
New
resolveCustomizationRefs - enterprise MCP policysuite, driving the realAllowedMcpServersServiceoff configuration rather than a stub, covering the reported allowlist:httpserver blocked by the allowlist is not forwarded (thedeepwikirepro)stdioserver blocked by the allowlist is not forwardedhttpandstdioservers blocked by the denylist are not forwardedAll three blocking tests were confirmed to fail against the unpatched code.
Validation
npm run typecheck-client- clean for all touched filesnpm run valid-layers-check- no violations./scripts/test.sh --grep "resolveCustomizationRefs"- 33 passing./scripts/test.sh --grep "MCP|Mcp|mcp"- 722 passing./scripts/test.sh --grep "agentHost"- 199 passing