Initial revision of enterprise MCP allowlist support,#309356
Initial revision of enterprise MCP allowlist support,#309356
Conversation
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
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-registryfor 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
AllowListis aSet<string>, but it's being persisted viaJSON.stringify({ value }).JSON.stringify(new Set([...]))serializes to{}, so the cached allowlist will be corrupted on disk and later reads will not return aSet. Persist a JSON-friendly representation (e.g. an array of strings plus a marker for "not applicable"), and reconstruct aSetwhen 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
_checkAllowlistawaitswaitForReady()without handling rejection. Since the allowlist service can cancel/reject its readiness promise on load failure, this can throw and makeresolveConnection()fail with an exception rather than cleanly treating the allowlist asUnavailable/"cannot verify". Handle errors fromwaitForReady()(or ensurewaitForReady()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
| // 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; | ||
| } |
There was a problem hiding this comment.
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.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
joshspicer
left a comment
There was a problem hiding this comment.
Looks reasonable to me! Would be great to get @sandy081 's 👀 if possible
Fixes https://github.com/microsoft/vscode-internalbacklog/issues/6959