Skip to content

Initial revision of enterprise MCP allowlist support,#309356

Draft
dmitrivMS wants to merge 17 commits intomainfrom
dev/dmitriv/mcp-allow-list
Draft

Initial revision of enterprise MCP allowlist support,#309356
dmitrivMS wants to merge 17 commits intomainfrom
dev/dmitriv/mcp-allow-list

Conversation

@dmitrivMS
Copy link
Copy Markdown
Contributor

@dmitrivMS dmitrivMS commented Apr 13, 2026

Co-authored-by: Copilot <copilot@github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a first pass of enterprise MCP allowlist enforcement by flowing enterprise registry metadata from the default account policy into MCP startup/connection logic, and by introducing a workbench service that fetches and evaluates an enterprise allowlist.

Changes:

  • Extend default account policy data to include enterprise MCP allowlist entries derived from copilot/mcp_registry.
  • Introduce IMcpAllowListService + implementation to fetch/cache allowlisted server fingerprints and gate MCP server launches.
  • Gate MCP autostart and connection resolution on allowlist readiness / evaluation, and add @github/mcp-registry for fingerprint computation.
Show a summary per file
File Description
src/vs/workbench/services/accounts/browser/defaultAccount.ts Extracts enterprise allowlist entries from MCP registry response into policy data.
src/vs/workbench/contrib/mcp/common/mcpService.ts Waits for allowlist readiness before autostarting MCP servers.
src/vs/workbench/contrib/mcp/common/mcpRegistry.ts Gates resolveConnection() on allowlist evaluation; computes server fingerprint via SDK.
src/vs/workbench/contrib/mcp/common/mcpAllowListService.ts New service to fetch/merge/cache enterprise allowlist fingerprints and evaluate server permission.
src/vs/workbench/contrib/mcp/browser/mcp.contribution.ts Registers the allowlist service singleton.
src/vs/workbench/contrib/git/common/utils.ts Exports parseRemoteUrl for allowlist repo-scoping logic.
src/vs/platform/mcp/common/mcpAllowListService.ts New platform-level allowlist service interface and state enum.
src/vs/base/common/defaultAccount.ts Adds IMcpAllowlistEntry and stores allowlist entries on policy data.
package.json Adds @github/mcp-registry dependency.
package-lock.json Locks @github/mcp-registry dependency.

Copilot's findings

Comments suppressed due to low confidence (2)

src/vs/workbench/contrib/mcp/common/mcpAllowListService.ts:261

  • AllowList is a Set<string>, but it's being persisted via JSON.stringify({ value }). JSON.stringify(new Set([...])) serializes to {}, so the cached allowlist will be corrupted on disk and later reads will not return a Set. Persist a JSON-friendly representation (e.g. an array of strings plus a marker for "not applicable"), and reconstruct a Set when reading from storage.
	private async loadAllowList(query: RegistryQuery, authToken: LazyStatefulPromise<string>, token: CancellationToken): Promise<AllowList> {
		const { entry, repo } = query;
		const storageKey = `${ALLOWLIST_CACHE_KEY}.${entry.ownerId}${repo ? `.${repo}` : ''}`;

		const cached = this.storageService.getObject<{ value: AllowList }>(storageKey, StorageScope.APPLICATION);
		if (cached) {
			this.logService.debug(`[McpAllowlist] Loaded allowlist for ${repo ?? '<none>'} from cache`);
			return cached.value;
		}

		const value = await this.fetchAllowList(query, authToken, token);
		this.storageService.store(storageKey, JSON.stringify({ value }), StorageScope.APPLICATION, StorageTarget.MACHINE);
		return value;

src/vs/workbench/contrib/mcp/common/mcpRegistry.ts:595

  • _checkAllowlist awaits waitForReady() without handling rejection. Since the allowlist service can cancel/reject its readiness promise on load failure, this can throw and make resolveConnection() fail with an exception rather than cleanly treating the allowlist as Unavailable/"cannot verify". Handle errors from waitForReady() (or ensure waitForReady() never rejects) so server resolution behavior remains deterministic.
		if (this._mcpAllowlistService.state === McpAllowListState.Loading) {
			await raceTimeout(this._mcpAllowlistService.waitForReady(), 10_000);
		}

  • Files reviewed: 9/10 changed files
  • Comments generated: 5

Comment thread src/vs/workbench/services/accounts/browser/defaultAccount.ts
Comment thread src/vs/workbench/contrib/mcp/common/mcpAllowListService.ts Outdated
Comment thread src/vs/workbench/contrib/mcp/common/mcpAllowListService.ts Outdated
Comment thread src/vs/workbench/contrib/mcp/common/mcpService.ts
Comment on lines +509 to +520
// Enterprise allowlist gate: wait for allowlist data and check if server is permitted
const allowlistResult = await this._checkAllowlist(definition);
if (allowlistResult !== true) {
if (opts.errorOnUserInteraction) {
throw new UserInteractionRequiredError('allowlist');
}
this._notificationService.notify({
severity: Severity.Warning,
message: allowlistResult.value,
});
return undefined;
}
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enterprise allowlist gating is now part of resolveConnection(), but there are no tests covering allowed/blocked/loading/unavailable outcomes. Since this file already has a dedicated test suite, add tests that stub the allowlist service and verify (1) allowed fingerprints proceed, (2) blocked fingerprints prevent connection and surface a warning / UserInteractionRequiredError('allowlist') when errorOnUserInteraction is set, and (3) unavailable/loading states behave as intended.

This issue also appears on line 592 of the same file.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does a user know if an installed MCP server is allowed or not by looking at it in the extensions view?

I think we do the access validation in the mcp workbench service which is the source for all installed mcp servers and disable them if they are not valid. Please check here -

if (accessValue === McpAccessValue.Registry) {
if (!mcpServer.gallery) {
return {
state: McpServerEnablementState.DisabledByAccess,
message: {
severity: Severity.Warning,
text: new MarkdownString(localize('disabled - some not allowed', "This MCP Server is disabled because it is configured to be disabled in the Editor. Please check your [settings]({0}).", settingsCommandLink))
}
};
}
const remoteUrl = mcpServer.local.config.type === McpServerType.REMOTE && mcpServer.local.config.url;
if (remoteUrl && !mcpServer.gallery.configuration.remotes?.some(remote => remote.url === remoteUrl)) {
return {
state: McpServerEnablementState.DisabledByAccess,
message: {
severity: Severity.Warning,
text: new MarkdownString(localize('disabled - some not allowed', "This MCP Server is disabled because it is configured to be disabled in the Editor. Please check your [settings]({0}).", settingsCommandLink))
}
};
}
}
- IMO we should do the same here

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 13, 2026

Screenshot Changes

Base: 93250509 Current: c25e18ca

Changed (2)

chat/aiCustomizations/aiCustomizationManagementEditor/McpBrowseMode/Light
Before After
before after
editor/inlineCompletions/other/JumpToHint/Dark
Before After
before after

@dmitrivMS dmitrivMS added plg gpo Group policies github-mcp-server Issues related to the GitHub MCP Server integration in the Copilot Chat extension mcp and removed plg github-mcp-server Issues related to the GitHub MCP Server integration in the Copilot Chat extension labels Apr 14, 2026
joshspicer
joshspicer previously approved these changes Apr 14, 2026
Copy link
Copy Markdown
Member

@joshspicer joshspicer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me! Would be great to get @sandy081 's 👀 if possible

@hstaudacher
Copy link
Copy Markdown

A change has been made to the /copilot/mcp_registry endpoint that should be respected here as well. It does support adding a repo param to scope the response to the repo's enterprise/org. Details can be found in this issue (right now the team is fixing a bug with this param. Should work as expected soon).

Copy link
Copy Markdown
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good work on this, I liked how the overall changes look. I left some comments on the implementation. Thanks.

Comment on lines +509 to +520
// Enterprise allowlist gate: wait for allowlist data and check if server is permitted
const allowlistResult = await this._checkAllowlist(definition);
if (allowlistResult !== true) {
if (opts.errorOnUserInteraction) {
throw new UserInteractionRequiredError('allowlist');
}
this._notificationService.notify({
severity: Severity.Warning,
message: allowlistResult.value,
});
return undefined;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does a user know if an installed MCP server is allowed or not by looking at it in the extensions view?

I think we do the access validation in the mcp workbench service which is the source for all installed mcp servers and disable them if they are not valid. Please check here -

if (accessValue === McpAccessValue.Registry) {
if (!mcpServer.gallery) {
return {
state: McpServerEnablementState.DisabledByAccess,
message: {
severity: Severity.Warning,
text: new MarkdownString(localize('disabled - some not allowed', "This MCP Server is disabled because it is configured to be disabled in the Editor. Please check your [settings]({0}).", settingsCommandLink))
}
};
}
const remoteUrl = mcpServer.local.config.type === McpServerType.REMOTE && mcpServer.local.config.url;
if (remoteUrl && !mcpServer.gallery.configuration.remotes?.some(remote => remote.url === remoteUrl)) {
return {
state: McpServerEnablementState.DisabledByAccess,
message: {
severity: Severity.Warning,
text: new MarkdownString(localize('disabled - some not allowed', "This MCP Server is disabled because it is configured to be disabled in the Editor. Please check your [settings]({0}).", settingsCommandLink))
}
};
}
}
- IMO we should do the same here

Comment thread src/vs/workbench/contrib/mcp/common/mcpAllowListService.ts
Comment thread src/vs/workbench/contrib/mcp/common/mcpAllowListService.ts
Comment thread src/vs/workbench/contrib/mcp/common/mcpAllowListService.ts
Comment thread src/vs/workbench/contrib/mcp/common/mcpAllowListService.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gpo Group policies mcp

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants