diff --git a/src/vs/platform/configuration/common/configurationVariables.ts b/src/vs/platform/configuration/common/configurationVariables.ts new file mode 100644 index 0000000000000..ababc715fef53 --- /dev/null +++ b/src/vs/platform/configuration/common/configurationVariables.ts @@ -0,0 +1,66 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +export interface IConfigurationVariable { + /** ${name:arg} */ + id: string; + /** The `name:arg` in ${name:arg} */ + inner: string; + /** The `name` in ${name:arg} */ + name: string; + /** The `arg` in ${name:arg} */ + arg?: string; +} + +/** Parses a configuration variable at the given offset, including nested braces. */ +export function parseConfigurationVariable(value: string, start: number): { replacement: IConfigurationVariable; end: number } | undefined { + if (value[start] !== '$' || value[start + 1] !== '{') { + return undefined; + } + + let end = start + 2; + let braceCount = 1; + while (end < value.length) { + if (value[end] === '{') { + braceCount++; + } else if (value[end] === '}') { + braceCount--; + if (braceCount === 0) { + break; + } + } + end++; + } + + if (braceCount !== 0) { + return undefined; + } + + const id = value.slice(start, end + 1); + const inner = value.substring(start + 2, end); + const colon = inner.indexOf(':'); + return { + replacement: colon === -1 + ? { id, name: inner, inner } + : { id, inner, name: inner.slice(0, colon), arg: inner.slice(colon + 1) }, + end + }; +} + +/** Scans backwards for a variable with a matching closing brace in linear time and constant space. */ +export function hasConfigurationVariable(value: string): boolean { + let unmatchedClosingBraces = 0; + for (let offset = value.length - 1; offset >= 0; offset--) { + if (value[offset] === '}') { + unmatchedClosingBraces++; + } else if (value[offset] === '{' && unmatchedClosingBraces > 0) { + unmatchedClosingBraces--; + if (offset > 0 && value[offset - 1] === '$') { + return true; + } + } + } + return false; +} diff --git a/src/vs/platform/configuration/test/common/configurationVariables.test.ts b/src/vs/platform/configuration/test/common/configurationVariables.test.ts new file mode 100644 index 0000000000000..b01175114f297 --- /dev/null +++ b/src/vs/platform/configuration/test/common/configurationVariables.test.ts @@ -0,0 +1,123 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; +import { hasConfigurationVariable, parseConfigurationVariable } from '../../common/configurationVariables.js'; + +suite('hasConfigurationVariable', () => { + ensureNoDisposablesAreLeakedInTestSuite(); + + test('recognizes complete variables, including empty expressions', () => { + const values = [ + '${name}', + '${env:PATH}', + '${}', + '${:}', + '${input:}', + '${command:name:argument}', + 'before ${name} after', + '${one}${two}', + '$${name}', + '\\${name}', + ]; + + assert.deepStrictEqual(values.map(hasConfigurationVariable), values.map(() => true)); + }); + + test('recognizes nested ordinary braces and variables', () => { + const values = [ + '${{}}', + '${{{name}}}', + '${command:name{argument}tail}', + '${outer:${inner}}', + '${outer:{${inner:{argument}}}}', + ]; + + assert.deepStrictEqual(values.map(hasConfigurationVariable), values.map(() => true)); + }); + + test('leaves incomplete markers and ordinary braces literal', () => { + const values = [ + '', + 'literal', + '$', + '{name}', + '$ {name}', + '${', + '${name', + '${name{}', + '${{name}', + '${{{}}', + '${outer:{inner}{tail}', + '}${', + '}${{name}', + ]; + + assert.deepStrictEqual(values.map(hasConfigurationVariable), values.map(() => false)); + }); + + test('finds complete inner variables after incomplete outer expressions', () => { + const values = [ + '${outer ${inner}', + '${outer:{${inner}', + '${${}', + '${${name{}}', + '${outer ${incomplete ${env:PATH} trailing ${', + ]; + + assert.deepStrictEqual(values.map(hasConfigurationVariable), values.map(() => true)); + }); + + test('ignores unmatched braces outside complete variables', () => { + const values = [ + '}${name}', + '${name}{', + '${name}}', + '{${name}{', + '${name} trailing ${', + ]; + + assert.deepStrictEqual(values.map(hasConfigurationVariable), values.map(() => true)); + }); + + test('agrees with the parser on all short brace expressions', () => { + const mismatches: string[] = []; + const check = (value: string, remaining: number): void => { + let expected = false; + for (let offset = 0; offset < value.length; offset++) { + if (parseConfigurationVariable(value, offset)) { + expected = true; + break; + } + } + + if (hasConfigurationVariable(value) !== expected) { + mismatches.push(value); + } + + if (remaining > 0) { + for (const character of '${}a:') { + check(value + character, remaining - 1); + } + } + }; + + check('', 6); + assert.deepStrictEqual(mismatches, []); + }); + + for (const suffix of ['', '${env:PATH}']) { + test(`scans 64 KiB of incomplete markers${suffix ? ' followed by a complete variable' : ''} within the performance budget`, () => { + const value = '${'.repeat(32 * 1024) + suffix; + const start = performance.now(); + const actual = hasConfigurationVariable(value); + const elapsed = performance.now() - start; + + assert.strictEqual(actual, suffix.length > 0); + assert.ok(elapsed < 1000, `Expected the scan to take less than 1000 ms, took ${elapsed.toFixed(1)} ms`); + }).timeout(60000); + } +}); diff --git a/src/vs/platform/mcp/common/allowedMcpServersService.ts b/src/vs/platform/mcp/common/allowedMcpServersService.ts index 7dac7121c32c8..7733f56523d80 100644 --- a/src/vs/platform/mcp/common/allowedMcpServersService.ts +++ b/src/vs/platform/mcp/common/allowedMcpServersService.ts @@ -7,6 +7,7 @@ import { Disposable } from '../../../base/common/lifecycle.js'; import * as nls from '../../../nls.js'; import { createCommandUri, IMarkdownString, MarkdownString } from '../../../base/common/htmlContent.js'; import { IConfigurationService } from '../../configuration/common/configuration.js'; +import { hasConfigurationVariable } from '../../configuration/common/configurationVariables.js'; import { Emitter } from '../../../base/common/event.js'; import { hasKey, isString } from '../../../base/common/types.js'; import { checkMcpServerAllowed, getMcpServerMatchers, IMcpServerIdentity, IMcpServerMatcher, McpServerAllowResult } from './allowedMcpServers.js'; @@ -33,10 +34,18 @@ export class AllowedMcpServersService extends Disposable implements IAllowedMcpS } isAllowed(mcpServer: IGalleryMcpServer | ILocalMcpServer | IInstallableMcpServer): true | IMarkdownString { - return this.isServerAllowed(this.toIdentity(mcpServer)); + return this.isServerAllowedBeforeResolution(this.toIdentity(mcpServer)); + } + + isServerAllowedBeforeResolution(identity: IMcpServerIdentity): true | IMarkdownString { + return this.checkServerAllowed(identity, 'definition'); } isServerAllowed(identity: IMcpServerIdentity): true | IMarkdownString { + return this.checkServerAllowed(identity, 'resolved'); + } + + private checkServerAllowed(identity: IMcpServerIdentity, phase: 'definition' | 'resolved'): true | IMarkdownString { if (this.configurationService.getValue(mcpAccessConfig) === McpAccessValue.None) { const settingsCommandLink = createCommandUri('workbench.action.openSettings', { query: `@id:${mcpAccessConfig}` }).toString(); return new MarkdownString(nls.localize('mcp servers are not allowed', "Model Context Protocol servers are disabled in the Editor. Please check your [settings]({0}).", settingsCommandLink)); @@ -49,7 +58,10 @@ export class AllowedMcpServersService extends Disposable implements IAllowedMcpS const denylist = managedOnly ? this.getAllConfiguredMatchers(mcpDeniedServersConfig) : getMcpServerMatchers(this.configurationService.getValue(mcpDeniedServersConfig)); - switch (this.checkServerAllowedAtCurrentResolution(allowlist, denylist, identity)) { + const result = phase === 'definition' + ? this.checkServerAllowedBeforeResolution(allowlist, denylist, identity) + : checkMcpServerAllowed(allowlist, denylist, identity); + switch (result) { case McpServerAllowResult.Denied: return new MarkdownString(nls.localize('mcp server is denied', "This Model Context Protocol server is blocked by your organization's policy. Please contact your administrator for more information.")); case McpServerAllowResult.NotAllowed: @@ -59,27 +71,27 @@ export class AllowedMcpServersService extends Disposable implements IAllowedMcpS return true; } - private checkServerAllowedAtCurrentResolution(allowlist: readonly IMcpServerMatcher[] | undefined, denylist: readonly IMcpServerMatcher[] | undefined, identity: IMcpServerIdentity): McpServerAllowResult { - if (!identity.url?.includes('${')) { + private checkServerAllowedBeforeResolution(allowlist: readonly IMcpServerMatcher[] | undefined, denylist: readonly IMcpServerMatcher[] | undefined, identity: IMcpServerIdentity): McpServerAllowResult { + const unresolvedUrl = identity.url !== undefined && hasConfigurationVariable(identity.url); + const unresolvedCommand = identity.command?.some(hasConfigurationVariable) ?? false; + if (!unresolvedUrl && !unresolvedCommand) { return checkMcpServerAllowed(allowlist, denylist, identity); } - const nonUrlIdentity = { name: identity.name }; - const nonUrlDenylist = denylist?.filter(matcher => !isString(matcher.serverUrl)); - if (checkMcpServerAllowed(undefined, nonUrlDenylist, nonUrlIdentity) === McpServerAllowResult.Denied) { + const knownIdentity: IMcpServerIdentity = { + name: identity.name, + url: unresolvedUrl ? undefined : identity.url, + command: unresolvedCommand ? undefined : identity.command, + }; + if (checkMcpServerAllowed(undefined, denylist, knownIdentity) === McpServerAllowResult.Denied) { return McpServerAllowResult.Denied; } - if (allowlist === undefined) { - return McpServerAllowResult.Allowed; - } - - const nonUrlAllowlist = allowlist.filter(matcher => !isString(matcher.serverUrl)); - if (checkMcpServerAllowed(nonUrlAllowlist, undefined, nonUrlIdentity) === McpServerAllowResult.Allowed) { + if (allowlist === undefined || checkMcpServerAllowed(allowlist, undefined, knownIdentity) === McpServerAllowResult.Allowed) { return McpServerAllowResult.Allowed; } - // URL matchers are authoritative only after runtime variable resolution. - return allowlist.some(matcher => isString(matcher.serverUrl)) + return allowlist.some(matcher => + (unresolvedUrl && isString(matcher.serverUrl)) || (unresolvedCommand && Array.isArray(matcher.serverCommand))) ? McpServerAllowResult.Allowed : McpServerAllowResult.NotAllowed; } diff --git a/src/vs/platform/mcp/common/mcpManagement.ts b/src/vs/platform/mcp/common/mcpManagement.ts index 50bb5f1cf75a4..2978d43addd9b 100644 --- a/src/vs/platform/mcp/common/mcpManagement.ts +++ b/src/vs/platform/mcp/common/mcpManagement.ts @@ -283,12 +283,15 @@ export interface IAllowedMcpServersService { readonly _serviceBrand: undefined; readonly onDidChangeAllowedMcpServers: Event; + /** Checks a server definition before resolution, deferring rules for variable-dependent URL/command fields. */ isAllowed(mcpServer: IGalleryMcpServer | ILocalMcpServer | IInstallableMcpServer): true | IMarkdownString; + /** Checks a definition identity before resolution; access and name restrictions are never deferred. */ + isServerAllowedBeforeResolution(identity: IMcpServerIdentity): true | IMarkdownString; + /** - * Checks whether an MCP server identified by name / remote URL / local command is permitted by - * the `chat.mcp.allowedServers` allowlist (in addition to the `chat.mcp.access` gate). Used by - * the runtime enforcement path, which does not have a gallery/local/installable representation. + * Checks access and allow/deny rules against a resolved runtime identity without deferring URL rules. + * Runtime callers must use this after resolving a server definition. */ isServerAllowed(identity: IMcpServerIdentity): true | IMarkdownString; } diff --git a/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts b/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts index a28c8056cf8c3..9483e2ee51cb8 100644 --- a/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts +++ b/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts @@ -65,6 +65,118 @@ suite('AllowedMcpServersService', () => { assert.strictEqual(service.isServerAllowed({ name: 's', url: 'https://api.trusted.example.com/mcp' }), true); }); + suite('URL policy resolution', () => { + test('preliminary checks do not defer incomplete variable markers', () => { + const service = createService({ + [mcpAllowedServersConfig]: [{ serverUrl: 'https://trusted.example/mcp' }], + [mcpDeniedServersConfig]: [{ serverUrl: 'https://blocked.example/*' }], + }); + const fragments = ['${', '${input:host', '${outer{inner}']; + assert.deepStrictEqual( + fragments.map(fragment => ['trusted.example', 'blocked.example', 'other.example'].map(host => service.isAllowed({ + name: 'server', + config: { type: McpServerType.REMOTE, url: `https://${host}/mcp#${fragment}` } + }) === true)), + fragments.map(() => [true, false, false]) + ); + }); + + test('preliminary checks preserve balanced and nested variable deferral', () => { + const service = createService({ + [mcpAllowedServersConfig]: [{ serverUrl: 'https://trusted.example/mcp' }], + [mcpDeniedServersConfig]: [{ serverUrl: 'https://blocked.example/*' }], + }); + const variables = ['${input:host}', '${input:${env:HOST}}', '${incomplete${env:HOST}']; + assert.deepStrictEqual( + variables.map(variable => service.isAllowed({ + name: 'server', + config: { type: McpServerType.REMOTE, url: `https://blocked.example/${variable}` } + }) === true), + variables.map(() => true) + ); + }); + + for (const fragment of ['${', '${input:literal}']) { + test(`enforces the resolved URL allowlist with fragment ${fragment}`, () => { + const service = createService({ + [mcpAllowedServersConfig]: [{ serverUrl: 'https://trusted.example/mcp' }], + [mcpDeniedServersConfig]: [{ serverUrl: 'https://blocked.example/*' }], + }); + + const result = service.isServerAllowed({ name: 'server', url: `https://attacker.example/mcp#${fragment}` }); + assert.strictEqual(result !== true && result.value.includes('not in the list of servers allowed by your organization'), true); + }); + + test(`enforces the resolved URL denylist with fragment ${fragment}`, () => { + const service = createService({ + [mcpAllowedServersConfig]: [{ serverUrl: 'https://trusted.example/mcp' }], + [mcpDeniedServersConfig]: [{ serverUrl: 'https://blocked.example/*' }], + }); + + const result = service.isServerAllowed({ name: 'server', url: `https://blocked.example/mcp#${fragment}` }); + assert.strictEqual(result !== true && result.value.includes('blocked by your organization'), true); + }); + } + + test('enforces resolved URL denies without an allowlist', () => { + const service = createService({ [mcpDeniedServersConfig]: [{ serverUrl: 'https://blocked.example/*' }] }); + const result = service.isServerAllowed({ name: 'server', url: 'https://blocked.example/mcp#${' }); + + assert.strictEqual(result !== true && result.value.includes('blocked by your organization'), true); + }); + + test('preserves allowed URLs, name rules, and disabled access', () => { + const identity = { name: 'server', url: 'https://trusted.example/mcp#${' }; + const allowedByUrl = createService({ [mcpAllowedServersConfig]: [{ serverUrl: 'https://trusted.example/mcp' }] }); + const allowedByName = createService({ [mcpAllowedServersConfig]: [{ serverName: identity.name }] }); + const deniedByName = createService({ [mcpDeniedServersConfig]: [{ serverName: identity.name }] }); + const disabled = createService({ [mcpAccessConfig]: McpAccessValue.None }); + + assert.deepStrictEqual({ + ordinaryUrl: allowedByUrl.isServerAllowed({ ...identity, url: 'https://trusted.example/mcp' }) === true, + literalFragment: allowedByUrl.isServerAllowed(identity) === true, + allowedByName: allowedByName.isServerAllowed(identity) === true, + deniedByName: deniedByName.isServerAllowed(identity) === true, + disabled: disabled.isServerAllowed(identity) === true, + }, { + ordinaryUrl: true, + literalFragment: true, + allowedByName: true, + deniedByName: false, + disabled: false, + }); + }); + + test('preserves preliminary URL deferral without deferring names or disabled access', () => { + const server: IInstallableMcpServer = { + name: 'server', + config: { type: McpServerType.REMOTE, url: 'https://${input:host}/mcp' }, + }; + const allowedByUrl = createService({ [mcpAllowedServersConfig]: [{ serverUrl: 'https://trusted.example/mcp' }] }); + const deniedByUrl = createService({ [mcpDeniedServersConfig]: [{ serverUrl: 'https://blocked.example/*' }] }); + const allowedByName = createService({ [mcpAllowedServersConfig]: [{ serverName: server.name }] }); + const allowedByOtherName = createService({ [mcpAllowedServersConfig]: [{ serverName: 'other' }] }); + const deniedByName = createService({ [mcpDeniedServersConfig]: [{ serverName: server.name }] }); + const disabled = createService({ [mcpAccessConfig]: McpAccessValue.None }); + + assert.deepStrictEqual({ + allowedByUrl: allowedByUrl.isAllowed(server) === true, + deniedByUrl: deniedByUrl.isAllowed(server) === true, + allowedByName: allowedByName.isAllowed(server) === true, + allowedByOtherName: allowedByOtherName.isAllowed(server) === true, + deniedByName: deniedByName.isAllowed(server) === true, + disabled: disabled.isAllowed(server) === true, + }, { + allowedByUrl: true, + deniedByUrl: true, + allowedByName: true, + allowedByOtherName: false, + deniedByName: false, + disabled: false, + }); + }); + }); + test('isAllowed matches an installable stdio server by its command', () => { const service = createService({ [mcpAllowedServersConfig]: [{ serverCommand: ['npx', '-y', 'server'] }] }); @@ -75,6 +187,41 @@ suite('AllowedMcpServersService', () => { assert.notStrictEqual(service.isAllowed(blocked), true); }); + test('preliminary command checks defer variable-dependent rules but not access or names', () => { + const server: IInstallableMcpServer = { + name: 'server', + config: { type: McpServerType.LOCAL, command: 'node', args: ['${input:script}'] }, + }; + const allowedByCommand = createService({ [mcpAllowedServersConfig]: [{ serverCommand: ['node', 'allowed.js'] }] }); + const deniedByCommand = createService({ [mcpDeniedServersConfig]: [{ serverCommand: ['node', 'blocked.js'] }] }); + const deniedByName = createService({ + [mcpAllowedServersConfig]: [{ serverCommand: ['node', 'allowed.js'] }], + [mcpDeniedServersConfig]: [{ serverName: server.name }], + }); + const otherName = createService({ [mcpAllowedServersConfig]: [{ serverName: 'other' }] }); + const onlyUrl = createService({ [mcpAllowedServersConfig]: [{ serverUrl: 'https://trusted.example/mcp' }] }); + const disabled = createService({ [mcpAccessConfig]: McpAccessValue.None }); + assert.deepStrictEqual({ + allowedByCommand: allowedByCommand.isAllowed(server) === true, + deniedByCommand: deniedByCommand.isAllowed(server) === true, + deniedByName: deniedByName.isAllowed(server) === true, + otherName: otherName.isAllowed(server) === true, + onlyUrl: onlyUrl.isAllowed(server) === true, + disabled: disabled.isAllowed(server) === true, + incompleteLiteral: allowedByCommand.isAllowed({ name: 'server', config: { type: McpServerType.LOCAL, command: 'node', args: ['${'] } }) === true, + resolvedDenied: deniedByCommand.isServerAllowed({ name: 'server', command: ['node', 'blocked.js'] }) === true, + }, { + allowedByCommand: true, + deniedByCommand: true, + deniedByName: false, + otherName: false, + onlyUrl: false, + disabled: false, + incompleteLiteral: false, + resolvedDenied: false, + }); + }); + test('isAllowed matches an installable remote server by its URL', () => { const service = createService({ [mcpAllowedServersConfig]: [{ serverUrl: 'https://mcp.example.com/*' }] }); diff --git a/src/vs/platform/mcp/test/common/mcpManagementService.test.ts b/src/vs/platform/mcp/test/common/mcpManagementService.test.ts index 8058b9ce10da4..5f1d1cb0c87a5 100644 --- a/src/vs/platform/mcp/test/common/mcpManagementService.test.ts +++ b/src/vs/platform/mcp/test/common/mcpManagementService.test.ts @@ -62,7 +62,7 @@ class TestMcpManagementService extends AbstractCommonMcpManagementService { } class TestMcpResourceManagementService extends AbstractMcpResourceManagementService { - constructor(mcpResource: URI, fileService: FileService, uriIdentityService: UriIdentityService, mcpResourceScannerService: McpResourceScannerService, allowedMcpServersService: IAllowedMcpServersService = { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowed: () => true }) { + constructor(mcpResource: URI, fileService: FileService, uriIdentityService: UriIdentityService, mcpResourceScannerService: McpResourceScannerService, allowedMcpServersService: IAllowedMcpServersService = { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowedBeforeResolution: () => true, isServerAllowed: () => true }) { super( mcpResource, ConfigurationTarget.USER, @@ -1529,7 +1529,7 @@ suite('McpResourceManagementService', () => { uriIdentityService, new NullLogService(), scannerService, - { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowed: () => true }, + { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowedBeforeResolution: () => true, isServerAllowed: () => true }, upcastPartial({ userRoamingDataHome: URI.from({ scheme: Schemas.inMemory, path: '/user' }) }), )); const [local] = await galleryService.getInstalled(); @@ -1549,7 +1549,7 @@ suite('McpResourceManagementService', () => { uriIdentityService, new NullLogService(), scannerService, - { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowed: () => true }, + { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowedBeforeResolution: () => true, isServerAllowed: () => true }, upcastPartial({ userRoamingDataHome: URI.from({ scheme: Schemas.inMemory, path: '/user' }) }), )); @@ -1567,7 +1567,7 @@ suite('McpResourceManagementService', () => { uriIdentityService, new NullLogService(), scannerService, - { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowed: () => true }, + { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowedBeforeResolution: () => true, isServerAllowed: () => true }, upcastPartial({ userRoamingDataHome: URI.from({ scheme: Schemas.inMemory, path: '/user' }) }), )); const gallery = { @@ -1610,7 +1610,7 @@ suite('McpResourceManagementService', () => { uriIdentityService, logService, scannerService, - { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowed: () => true }, + { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowedBeforeResolution: () => true, isServerAllowed: () => true }, upcastPartial({ userRoamingDataHome: URI.from({ scheme: Schemas.inMemory, path: '/user' }) }), )); @@ -1636,7 +1636,7 @@ suite('McpResourceManagementService - install policy enforcement', () => { const server: IInstallableMcpServer = { name: 'my-server', config: { type: McpServerType.LOCAL, command: 'node', args: [] } }; function createService(isAllowed: IAllowedMcpServersService['isAllowed']): TestMcpResourceManagementService { - const allowedMcpServersService: IAllowedMcpServersService = { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed, isServerAllowed: () => true }; + const allowedMcpServersService: IAllowedMcpServersService = { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed, isServerAllowedBeforeResolution: () => true, isServerAllowed: () => true }; return disposables.add(new TestMcpResourceManagementService(mcpResource, fileService, uriIdentityService, scannerService, allowedMcpServersService)); } diff --git a/src/vs/workbench/contrib/mcp/common/mcpServer.ts b/src/vs/workbench/contrib/mcp/common/mcpServer.ts index 427a09b083ceb..63c6c0b8de7b3 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpServer.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpServer.ts @@ -12,13 +12,14 @@ import { Disposable, DisposableStore, IDisposable, IReference, MutableDisposable import { LRUCache } from '../../../../base/common/map.js'; import { Schemas } from '../../../../base/common/network.js'; import { mapValues } from '../../../../base/common/objects.js'; -import { autorun, autorunSelfDisposable, derived, derivedDisposable, disposableObservableValue, IDerivedReader, IObservable, IReader, ITransaction, observableFromEvent, ObservablePromise, observableValue, transaction } from '../../../../base/common/observable.js'; +import { autorun, autorunSelfDisposable, derived, derivedDisposable, disposableObservableValue, IDerivedReader, IObservable, IReader, ITransaction, observableFromEvent, ObservablePromise, observableSignalFromEvent, observableValue, transaction } from '../../../../base/common/observable.js'; import { basename } from '../../../../base/common/resources.js'; import { URI } from '../../../../base/common/uri.js'; import { createURITransformer } from '../../../../base/common/uriTransformer.js'; import { generateUuid } from '../../../../base/common/uuid.js'; import { localize } from '../../../../nls.js'; import { ICommandService } from '../../../../platform/commands/common/commands.js'; +import { hasConfigurationVariable } from '../../../../platform/configuration/common/configurationVariables.js'; import { IDialogService } from '../../../../platform/dialogs/common/dialogs.js'; import { IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js'; import { IMcpServerIdentity } from '../../../../platform/mcp/common/allowedMcpServers.js'; @@ -29,7 +30,6 @@ import { IOpenerService } from '../../../../platform/opener/common/opener.js'; import { IStorageService, StorageScope, StorageTarget } from '../../../../platform/storage/common/storage.js'; import { ITelemetryService } from '../../../../platform/telemetry/common/telemetry.js'; import { IWorkspaceContextService } from '../../../../platform/workspace/common/workspace.js'; -import { ConfigurationResolverExpression } from '../../../services/configurationResolver/common/configurationResolverExpression.js'; import { IEditorService } from '../../../services/editor/common/editorService.js'; import { IWorkbenchEnvironmentService } from '../../../services/environment/common/environmentService.js'; import { IExtensionService } from '../../../services/extensions/common/extensions.js'; @@ -402,22 +402,12 @@ export class McpServer extends Disposable implements IMcpServer { public readonly collection: McpCollectionReference; private readonly _connectionSequencer = new Sequencer(); private readonly _connection = this._register(disposableObservableValue(this, undefined)); + private readonly _resolvedPolicyIdentity = observableValue<{ definition: McpServerDefinition; identity: IMcpServerIdentity } | undefined>(this, undefined); public readonly connection = this._connection; - /** - * Reactively evaluates the `chat.mcp.allowedServers` / `chat.mcp.deniedServers` policy against - * this server's identity. Holds an error state while blocked, `undefined` while allowed. - * - * Being a derived, it recomputes whenever the policy changes (via {@link _policyEpoch}), the - * server definition changes, or a connection resolves — so it always evaluates the *resolved* - * launch of a live connection and falls back to the definition otherwise. This also means a - * blocked server surfaces the block at rest (before any start), which hides its cached tools - * and prompts and lets the UI show the reason. - * - * Initialized in the constructor because it depends on the injected allowed-servers service. - */ private readonly _policyEpoch: IObservable; + /** Retains policy enforcement for the last resolved launch while its definition is unchanged. */ private readonly _policyBlock: IObservable; public readonly connectionState: IObservable = derived(reader => this._policyBlock.read(reader) ?? this._connection.read(reader)?.state.read(reader) ?? { state: McpConnectionState.Kind.Stopped }); @@ -539,7 +529,7 @@ export class McpServer extends Disposable implements IMcpServer { this._fullDefinitions = this._mcpRegistry.getServerDefinition(this.collection, this.definition); this.enablement = derived(r => enablementModel.readEnabled(definition.id, r)); - this._policyEpoch = observableFromEvent(this, this._allowedMcpServersService.onDidChangeAllowedMcpServers, () => undefined); + this._policyEpoch = observableSignalFromEvent(this, this._allowedMcpServersService.onDidChangeAllowedMcpServers); this._policyBlock = derived(this, reader => { this._policyEpoch.read(reader); const connection = this._connection.read(reader); @@ -547,28 +537,26 @@ export class McpServer extends Disposable implements IMcpServer { // Authoritative: the connection carries the fully resolved launch. return this._evaluatePolicy(this._identityFromLaunch(connection.launchDefinition)); } - // At rest, only decide when we have a concrete, fully-resolved launch. If the definition - // has not been provided yet (e.g. a lazy/extension server before activation) or the launch - // still contains unresolved `${...}` variables (inputs, workspace or env vars), a - // URL/command allow/deny rule cannot be matched reliably, so defer the decision to start() - // — which re-checks the fully resolved launch — to avoid over-eagerly blocking (and hiding - // the cached tools of) a server that will actually be allowed once resolved. `chat.mcp.access` - // and deny-by-name are still enforced at start(), and access also by the enablement layer. - const launch = this._fullDefinitions.read(reader).server?.launch; - if (!launch) { - return undefined; + const definition = this._fullDefinitions.read(reader).server; + const resolved = this._resolvedPolicyIdentity.read(reader); + if (resolved && definition && McpServerDefinition.equals(resolved.definition, definition)) { + return this._evaluatePolicy(resolved.identity); } - const identity = this._identityFromLaunch(launch); - if (McpServer._hasUnresolvedVariables(identity)) { + const launch = definition?.launch; + if (!launch) { return undefined; } - return this._evaluatePolicy(identity); + return this._evaluatePolicy(this._identityFromLaunch(launch), 'definition'); }); - // Stop a live connection when the policy blocks it (e.g. the policy was tightened while the - // server was running). The block itself is evaluated reactively by `_policyBlock`, which also - // hides cached tools/prompts and surfaces the reason in the UI. this._register(autorun(reader => { + const resolved = this._resolvedPolicyIdentity.read(reader); + const definition = this._fullDefinitions.read(reader).server; + // A reverted definition must not revive an identity from before its last change. + if (resolved && (!definition || !McpServerDefinition.equals(resolved.definition, definition))) { + this._resolvedPolicyIdentity.set(undefined, undefined); + } + if (this._policyBlock.read(reader) && this._connection.read(undefined)) { this._connection.set(undefined, undefined); // disposes and stops the connection } @@ -740,31 +728,33 @@ export class McpServer extends Disposable implements IMcpServer { return { name: this.definition.label }; } - private _evaluatePolicy(identity: IMcpServerIdentity): McpConnectionState.Error | undefined { - const allowed = this._allowedMcpServersService.isServerAllowed(identity); + private _evaluatePolicy(identity: IMcpServerIdentity, phase: 'definition' | 'resolved' = 'resolved'): McpConnectionState.Error | undefined { + const allowed = phase === 'definition' + ? this._allowedMcpServersService.isServerAllowedBeforeResolution(identity) + : this._allowedMcpServersService.isServerAllowed(identity); return allowed === true ? undefined : { state: McpConnectionState.Kind.Error, message: allowed.value }; } - /** - * Whether the URL/command fields matched by the policy still contain unresolved `${...}` - * configuration variables. When they do, matching against allow/deny URL or command rules is - * unreliable, so the block is deferred until the launch is resolved. The server name is used - * verbatim and is not considered here. - */ + /** Whether policy URL/command fields contain unresolved configuration variables; the server name is literal. */ private static _hasUnresolvedVariables(identity: IMcpServerIdentity): boolean { - const variableMarker = ConfigurationResolverExpression.VARIABLE_LHS; - return !!identity.url?.includes(variableMarker) || !!identity.command?.some(arg => arg.includes(variableMarker)); + return (identity.url !== undefined && hasConfigurationVariable(identity.url)) + || (identity.command?.some(hasConfigurationVariable) ?? false); } public start({ interaction, autoTrustChanges, promptType, debug, errorOnUserInteraction }: IMcpServerStartOpts = {}): Promise { interaction?.participants.set(this.definition.id, { s: 'unknown' }); return this._connectionSequencer.queue(async () => { - // Evaluated against the definition here (no connection yet). `_policyBlock` re-evaluates - // against the resolved launch once the connection exists (checked again below). const preStartBlock = this._policyBlock.get(); if (preStartBlock) { - return preStartBlock; + const definition = this._fullDefinitions.get().server; + const identity = definition && this._identityFromLaunch(definition.launch); + const canResolveAgain = !errorOnUserInteraction && this._resolvedPolicyIdentity.get() + && identity && McpServer._hasUnresolvedVariables(identity) && !this._evaluatePolicy(identity, 'definition'); + // Keep cached metadata blocked while an interactive retry resolves the current inputs. + if (!canResolveAgain) { + return preStartBlock; + } } const activationEvent = mcpActivationEvent(this.collection.id.slice(extensionMcpCollectionPrefix.length)); @@ -814,18 +804,19 @@ export class McpServer extends Disposable implements IMcpServer { return { state: McpConnectionState.Kind.Stopped }; } - this._connection.set(connection, undefined); + const resolvedPolicyIdentity = { definition: connection.definition, identity: this._identityFromLaunch(connection.launchDefinition) }; + transaction(tx => { + this._resolvedPolicyIdentity.set(resolvedPolicyIdentity, tx); + this._connection.set(connection, tx); + }); if (connection.definition.devMode) { this.showOutput(); } } - // Re-evaluate the policy against the *resolved* launch definition. Extension activation and - // variable/input substitution during resolution can change the URL or command, so the - // identity that actually launches may differ from the one checked before resolution. - // `_policyBlock` now sees the live connection and uses its resolved launch. - const resolvedBlock = this._policyBlock.get(); + // Check the local resolved connection: reactive policy enforcement may already have cleared `_connection`. + const resolvedBlock = this._evaluatePolicy(this._identityFromLaunch(connection.launchDefinition)); if (resolvedBlock) { this._connection.set(undefined, undefined); // dispose the just-resolved connection return resolvedBlock; @@ -855,6 +846,11 @@ export class McpServer extends Disposable implements IMcpServer { } }); + const policyBlock = this._policyBlock.get(); + if (policyBlock) { + return policyBlock; + } + this._telemetryService.publicLog2('mcp/serverBootState', { state: McpConnectionState.toKindString(state.state), time: Date.now() - start, diff --git a/src/vs/workbench/contrib/mcp/common/mcpServerConnection.ts b/src/vs/workbench/contrib/mcp/common/mcpServerConnection.ts index 5ab7cdc20d88f..2931760e06f47 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpServerConnection.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpServerConnection.ts @@ -7,7 +7,7 @@ import { CancellationTokenSource } from '../../../../base/common/cancellation.js import { CancellationError } from '../../../../base/common/errors.js'; import { Emitter } from '../../../../base/common/event.js'; import { Disposable, DisposableStore, IReference, MutableDisposable, toDisposable } from '../../../../base/common/lifecycle.js'; -import { autorun, IObservable, observableValue } from '../../../../base/common/observable.js'; +import { autorun, IObservable, observableValue, transaction } from '../../../../base/common/observable.js'; import { localize } from '../../../../nls.js'; import { IInstantiationService } from '../../../../platform/instantiation/common/instantiation.js'; import { ILogger, log, LogLevel } from '../../../../platform/log/common/log.js'; @@ -19,6 +19,7 @@ import { IMcpClientMethods, IMcpPotentialSandboxBlock, IMcpServerConnection, Mcp export class McpServerConnection extends Disposable implements IMcpServerConnection { private readonly _launch = this._register(new MutableDisposable>()); private readonly _state = observableValue('mcpServerState', { state: McpConnectionState.Kind.Stopped }); + private readonly _stopRequested = observableValue(this, false); private readonly _requestHandler = observableValue('mcpServerRequestHandler', undefined); private readonly _onPotentialSandboxBlock = this._register(new Emitter()); @@ -41,13 +42,19 @@ export class McpServerConnection extends Disposable implements IMcpServerConnect /** @inheritdoc */ public async start(methods: IMcpClientMethods): Promise { + if (this._store.isDisposed) { + return this._state.get(); + } const currentState = this._state.get(); if (!McpConnectionState.canBeStarted(currentState.state)) { return this._waitForState(McpConnectionState.Kind.Running, McpConnectionState.Kind.Error); } this._launch.value = undefined; - this._state.set({ state: McpConnectionState.Kind.Starting }, undefined); + transaction(tx => { + this._stopRequested.set(false, tx); + this._state.set({ state: McpConnectionState.Kind.Starting }, tx); + }); this._logger.info(localize('mcpServer.starting', 'Starting server {0}', this.definition.label)); try { @@ -121,27 +128,33 @@ export class McpServerConnection extends Disposable implements IMcpServerConnect } public async stop(): Promise { + this._stopRequested.set(true, undefined); this._logger.info(localize('mcpServer.stopping', 'Stopping server {0}', this.definition.label)); this._launch.value?.object.stop(); await this._waitForState(McpConnectionState.Kind.Stopped, McpConnectionState.Kind.Error); } public override dispose(): void { - this._requestHandler.get()?.dispose(); - super.dispose(); - this._state.set({ state: McpConnectionState.Kind.Stopped }, undefined); + transaction(tx => { + this._stopRequested.set(true, tx); + this._requestHandler.get()?.dispose(); + super.dispose(); + this._state.set({ state: McpConnectionState.Kind.Stopped }, tx); + }); } private _waitForState(...kinds: McpConnectionState.Kind[]): Promise { const current = this._state.get(); - if (kinds.includes(current.state)) { + if (kinds.includes(current.state) || (current.state === McpConnectionState.Kind.Stopped && this._stopRequested.get())) { return Promise.resolve(current); } return new Promise(resolve => { const disposable = autorun(reader => { const state = this._state.read(reader); - if (kinds.includes(state.state)) { + // A transport may still expose its previous Stopped state while a restart begins. + const stopped = this._stopRequested.read(reader) && state.state === McpConnectionState.Kind.Stopped; + if (kinds.includes(state.state) || stopped) { disposable.dispose(); resolve(state); } diff --git a/src/vs/workbench/contrib/mcp/test/browser/mcpWorkbenchService.test.ts b/src/vs/workbench/contrib/mcp/test/browser/mcpWorkbenchService.test.ts index 97083aa680baf..e7b0e64ec97e9 100644 --- a/src/vs/workbench/contrib/mcp/test/browser/mcpWorkbenchService.test.ts +++ b/src/vs/workbench/contrib/mcp/test/browser/mcpWorkbenchService.test.ts @@ -899,7 +899,7 @@ suite('McpWorkbenchService', () => { [IWorkbenchEnvironmentService, upcastPartial({})], [ITelemetryService, NullTelemetryService], [IProductService, TestProductService], - [IAllowedMcpServersService, upcastPartial({ onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowed: () => true })], + [IAllowedMcpServersService, upcastPartial({ onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowedBeforeResolution: () => true, isServerAllowed: () => true })], ); const instantiationService = store.add(new TestInstantiationService(services)); const registry = store.add(instantiationService.createInstance(McpRegistry)); diff --git a/src/vs/workbench/contrib/mcp/test/common/mcpResourceFilesystem.test.ts b/src/vs/workbench/contrib/mcp/test/common/mcpResourceFilesystem.test.ts index efed51356dbba..b15ff0163793f 100644 --- a/src/vs/workbench/contrib/mcp/test/common/mcpResourceFilesystem.test.ts +++ b/src/vs/workbench/contrib/mcp/test/common/mcpResourceFilesystem.test.ts @@ -46,7 +46,7 @@ suite('Workbench - MCP - ResourceFilesystem', () => { [IWorkbenchEnvironmentService, {}], [ITelemetryService, NullTelemetryService], [IProductService, TestProductService], - [IAllowedMcpServersService, { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowed: () => true }], + [IAllowedMcpServersService, { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowedBeforeResolution: () => true, isServerAllowed: () => true }], ); const parentInsta1 = ds.add(new TestInstantiationService(services)); diff --git a/src/vs/workbench/contrib/mcp/test/common/mcpServerConnection.test.ts b/src/vs/workbench/contrib/mcp/test/common/mcpServerConnection.test.ts index 8e1c2c2d8c615..cc5e5b40ca3bd 100644 --- a/src/vs/workbench/contrib/mcp/test/common/mcpServerConnection.test.ts +++ b/src/vs/workbench/contrib/mcp/test/common/mcpServerConnection.test.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import * as assert from 'assert'; -import { timeout } from '../../../../../base/common/async.js'; +import { raceTimeout, timeout } from '../../../../../base/common/async.js'; import { Disposable } from '../../../../../base/common/lifecycle.js'; import { autorun, observableValue } from '../../../../../base/common/observable.js'; import { upcast } from '../../../../../base/common/types.js'; @@ -183,6 +183,44 @@ suite('Workbench - MCP - ServerConnection', () => { assert.ok(state.message); }); + for (const action of ['dispose', 'stop'] as const) { + test(`${action} settles every pending start while the transport is Starting`, async () => { + const connection = store.add(instantiationService.createInstance( + McpServerConnection, collection, serverDefinition, delegate, serverDefinition.launch, + new NullLogger(), false, store.add(new McpTaskManager()), + )); + const pending = [connection.start({}), connection.start({})]; + if (action === 'dispose') { + connection.dispose(); + } else { + await connection.stop(); + } + const states = await raceTimeout(Promise.all(pending), 1000); + assert.deepStrictEqual(states?.map(state => state.state), [ + McpConnectionState.Kind.Stopped, + McpConnectionState.Kind.Stopped, + ]); + }); + } + + test('a disposed connection does not start a transport', async () => { + const connection = store.add(instantiationService.createInstance( + McpServerConnection, collection, serverDefinition, delegate, serverDefinition.launch, + new NullLogger(), false, store.add(new McpTaskManager()), + )); + let launches = 0; + delegate.start = () => { + launches++; + return transport; + }; + connection.dispose(); + const state = await raceTimeout(connection.start({}), 1000); + assert.deepStrictEqual({ state: state?.state, launches }, { + state: McpConnectionState.Kind.Stopped, + launches: 0, + }); + }); + test('should handle transport errors', async () => { // Create server connection const connection = instantiationService.createInstance( diff --git a/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts b/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts index 998ed810bb832..e0dc6f1f0dca0 100644 --- a/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts +++ b/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts @@ -5,28 +5,35 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; -import { DeferredPromise, timeout } from '../../../../../base/common/async.js'; -import { Event } from '../../../../../base/common/event.js'; +import { DeferredPromise, raceTimeout, timeout } from '../../../../../base/common/async.js'; +import { Emitter, Event } from '../../../../../base/common/event.js'; import { toDisposable } from '../../../../../base/common/lifecycle.js'; import { autorun, observableValue, waitForState } from '../../../../../base/common/observable.js'; +import { StopWatch } from '../../../../../base/common/stopwatch.js'; import { URI } from '../../../../../base/common/uri.js'; +import { mock } from '../../../../../base/test/common/mock.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; +import { ConfigurationTarget } from '../../../../../platform/configuration/common/configuration.js'; import { TestConfigurationService } from '../../../../../platform/configuration/test/common/testConfigurationService.js'; import { IFileService } from '../../../../../platform/files/common/files.js'; import { ServiceCollection } from '../../../../../platform/instantiation/common/serviceCollection.js'; import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; -import { ILoggerService, NullLogService } from '../../../../../platform/log/common/log.js'; -import { IAllowedMcpServersService, mcpAutoStartConfig, McpAutoStartValue } from '../../../../../platform/mcp/common/mcpManagement.js'; +import { ILoggerService, NullLogger, NullLogService } from '../../../../../platform/log/common/log.js'; +import { AllowedMcpServersService } from '../../../../../platform/mcp/common/allowedMcpServersService.js'; +import { IAllowedMcpServersService, mcpAccessConfig, McpAccessValue, mcpAllowedServersConfig, mcpAutoStartConfig, McpAutoStartValue, mcpDeniedServersConfig } from '../../../../../platform/mcp/common/mcpManagement.js'; import { IProductService } from '../../../../../platform/product/common/productService.js'; -import { IStorageService } from '../../../../../platform/storage/common/storage.js'; +import { IStorageService, StorageScope } from '../../../../../platform/storage/common/storage.js'; import { ITelemetryService } from '../../../../../platform/telemetry/common/telemetry.js'; import { NullTelemetryService } from '../../../../../platform/telemetry/common/telemetryUtils.js'; import { IWorkspaceContextService } from '../../../../../platform/workspace/common/workspace.js'; +import { ConfigurationResolverExpression } from '../../../../services/configurationResolver/common/configurationResolverExpression.js'; import { IWorkbenchEnvironmentService } from '../../../../services/environment/common/environmentService.js'; +import { IExtensionService } from '../../../../services/extensions/common/extensions.js'; import { TestContextService, TestLoggerService, TestProductService, TestStorageService } from '../../../../test/common/workbenchTestServices.js'; import { IMcpRegistry } from '../../common/mcpRegistryTypes.js'; +import { McpServerConnection } from '../../common/mcpServerConnection.js'; import { McpService } from '../../common/mcpService.js'; -import { McpConnectionState, McpServerDefinition, McpServerTransportType } from '../../common/mcpTypes.js'; +import { McpConnectionState, McpServerDefinition, McpServerLaunch, McpServerTransportType } from '../../common/mcpTypes.js'; import { MCP } from '../../common/modelContextProtocol.js'; import { TestMcpMessageTransport, TestMcpRegistry } from './mcpRegistryTypes.js'; @@ -34,7 +41,7 @@ suite('Workbench - MCP - McpService', () => { const store = ensureNoDisposablesAreLeakedInTestSuite(); - const createMcpService = () => { + const createMcpService = (allowedMcpServersService?: IAllowedMcpServersService) => { const storageService = store.add(new TestStorageService()); const services = new ServiceCollection( [IFileService, { registerProvider: () => { } }], @@ -44,7 +51,7 @@ suite('Workbench - MCP - McpService', () => { [IWorkbenchEnvironmentService, {}], [ITelemetryService, NullTelemetryService], [IProductService, TestProductService], - [IAllowedMcpServersService, { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowed: () => true }], + [IAllowedMcpServersService, allowedMcpServersService ?? { _serviceBrand: undefined, onDidChangeAllowedMcpServers: Event.None, isAllowed: () => true, isServerAllowedBeforeResolution: () => true, isServerAllowed: () => true }], ); const parentInstantiationService = store.add(new TestInstantiationService(services)); @@ -52,7 +59,7 @@ suite('Workbench - MCP - McpService', () => { const instantiationService = store.add(parentInstantiationService.createChild(new ServiceCollection([IMcpRegistry, registry]))); const configurationService = new TestConfigurationService({ [mcpAutoStartConfig]: McpAutoStartValue.NewAndOutdated }); const mcpService = store.add(new McpService(instantiationService, registry, new NullLogService(), configurationService, storageService)); - return { mcpService, registry }; + return { mcpService, registry, instantiationService }; }; const setServerDefinition = (registry: TestMcpRegistry, definition: McpServerDefinition) => { @@ -63,6 +70,541 @@ suite('Workbench - MCP - McpService', () => { }], undefined); }; + suite('URL policy resolution', () => { + const createPolicyServer = (definitionUrl: string, resolvedUrl = definitionUrl, requiresActivation = false) => { + const configurationService = new TestConfigurationService({ + [mcpAllowedServersConfig]: [{ serverUrl: 'https://trusted.example/mcp' }], + [mcpDeniedServersConfig]: [{ serverUrl: 'https://blocked.example/*' }], + }); + const allowedMcpServersService = store.add(new AllowedMcpServersService(configurationService)); + const { mcpService, registry, instantiationService } = createMcpService(allowedMcpServersService); + const definition: McpServerDefinition = { + id: 'test-server', + label: 'Test Server', + launch: { type: McpServerTransportType.HTTP, uri: URI.parse(definitionUrl), headers: [] }, + cacheNonce: 'a', + }; + setServerDefinition(registry, definition); + const activation = { done: true, calls: 0 }; + if (requiresActivation) { + const collection = registry.collections.get()[0]; + registry.collections.set([{ ...collection, lazy: { isCached: true, load: async () => { } } }], undefined); + instantiationService.stub(IExtensionService, new class extends mock() { + override activationEventIsDone(): boolean { return activation.done; } + override async activateByEvent(): Promise { activation.calls++; activation.done = true; } + }); + } + + const inputChanges = store.add(new Emitter()); + registry.onDidChangeInputs = inputChanges.event; + const resolutionResult: { url: string; beforeResolve?: () => Promise; cancelled?: boolean } = { url: resolvedUrl }; + const resolution = sinon.stub(registry, 'resolveConnection').callsFake(async options => { + await resolutionResult.beforeResolve?.(); + if (resolutionResult.cancelled) { + return undefined; + } + return store.add(instantiationService.createInstance( + McpServerConnection, + registry.collections.get()[0], + definition, + registry.delegates.get()[0], + { type: McpServerTransportType.HTTP, uri: URI.parse(resolutionResult.url), headers: [] }, + new NullLogger(), + true, + options.taskManager, + )); + }); + store.add(toDisposable(() => resolution.restore())); + + const transports: TestMcpMessageTransport[] = []; + registry.makeTestTransport = () => { + const transport = store.add(new TestMcpMessageTransport()); + transports.push(transport); + transport.setResponder('tools/list', message => ({ + jsonrpc: MCP.JSONRPC_VERSION, + id: (message as MCP.JSONRPCRequest).id, + result: { tools: [] }, + })); + return transport; + }; + mcpService.updateCollectedServers(); + return { server: mcpService.servers.get()[0], configurationService, resolution, resolutionResult, transports, registry, mcpService, definition, inputChanges, activation }; + }; + + const setDeniedUrls = async (configurationService: TestConfigurationService, urls: string[]) => { + await configurationService.setUserConfiguration(mcpDeniedServersConfig, urls.map(serverUrl => ({ serverUrl }))); + configurationService.onDidChangeConfigurationEmitter.fire({ + source: ConfigurationTarget.USER, + affectedKeys: new Set([mcpDeniedServersConfig]), + change: { keys: [mcpDeniedServersConfig], overrides: [] }, + affectsConfiguration: key => key === mcpDeniedServersConfig, + }); + }; + + const provideCachedMetadata = (registry: TestMcpRegistry) => { + const createTransport = registry.makeTestTransport; + registry.makeTestTransport = () => { + const transport = createTransport(); + transport.setResponder('initialize', message => ({ + jsonrpc: MCP.JSONRPC_VERSION, + id: (message as MCP.JSONRPCRequest).id, + result: { + protocolVersion: MCP.LATEST_PROTOCOL_VERSION, + serverInfo: { name: 'Policy Fixture', version: '1.0.0' }, + capabilities: { tools: {}, prompts: {} }, + } + })); + transport.setResponder('tools/list', message => ({ + jsonrpc: MCP.JSONRPC_VERSION, id: (message as MCP.JSONRPCRequest).id, + result: { tools: [{ name: 'cached_tool', inputSchema: { type: 'object', properties: {} } }] }, + })); + transport.setResponder('prompts/list', message => ({ + jsonrpc: MCP.JSONRPC_VERSION, id: (message as MCP.JSONRPCRequest).id, + result: { prompts: [{ name: 'cached_prompt' }] }, + })); + return transport; + }; + }; + + const createBlockedInputServer = async () => { + const context = createPolicyServer('https://${input:host}/mcp', 'https://trusted.example/mcp'); + const { server, registry, configurationService } = context; + provideCachedMetadata(registry); + await configurationService.setUserConfiguration(mcpAllowedServersConfig, [ + { serverUrl: 'https://trusted.example/mcp' }, + { serverUrl: 'https://changed.example/mcp' }, + ]); + await server.start({ promptType: 'never', errorOnUserInteraction: true }); + await Promise.all([ + waitForState(server.tools, tools => tools.length === 1), + waitForState(server.prompts, prompts => prompts.length === 1), + ]); + await setDeniedUrls(configurationService, ['https://trusted.example/*']); + const snapshot = () => ({ + state: server.connectionState.get().state, + connected: !!server.connection.get(), + tools: server.tools.get().length, + prompts: server.prompts.get().length, + }); + return { ...context, snapshot }; + }; + + test('retains a resolved policy block and suppresses cached metadata after disposal', async () => { + const { server, configurationService, registry, definition } = createPolicyServer('https://${input:host}/mcp', 'https://trusted.example/mcp'); + provideCachedMetadata(registry); + await server.start({ promptType: 'never', errorOnUserInteraction: true }); + await Promise.all([ + waitForState(server.tools, tools => tools.length === 1), + waitForState(server.prompts, prompts => prompts.length === 1), + ]); + const snapshot = () => ({ + state: server.connectionState.get().state, + connected: !!server.connection.get(), + tools: server.tools.get().length, + prompts: server.prompts.get().length, + }); + const before = snapshot(); + await setDeniedUrls(configurationService, ['https://trusted.example/*']); + const blocked = snapshot(); + setServerDefinition(registry, { ...definition, launch: { ...definition.launch } }); + const equivalentDefinition = snapshot(); + await setDeniedUrls(configurationService, []); + const allowedAgain = snapshot(); + + assert.deepStrictEqual({ before, blocked, equivalentDefinition, allowedAgain }, { + before: { state: McpConnectionState.Kind.Running, connected: true, tools: 1, prompts: 1 }, + blocked: { state: McpConnectionState.Kind.Error, connected: false, tools: 0, prompts: 0 }, + equivalentDefinition: { state: McpConnectionState.Kind.Error, connected: false, tools: 0, prompts: 0 }, + allowedAgain: { state: McpConnectionState.Kind.Stopped, connected: false, tools: 1, prompts: 1 }, + }); + }); + + for (const inputAction of ['edit', 'reset'] as const) { + test(`explicit retry re-resolves a blocked server after saved input ${inputAction}`, async () => { + const { server, registry, definition, resolution, resolutionResult, inputChanges, transports, snapshot } = await createBlockedInputServer(); + const resolving = new DeferredPromise(); + const resume = new DeferredPromise(); + resolutionResult.beforeResolve = async () => { + void resolving.complete(); + await resume.p; + }; + const changedInput = async () => { + resolutionResult.url = 'https://changed.example/mcp'; + inputChanges.fire(); + }; + const edit = sinon.stub(registry, 'editSavedInput').callsFake(changedInput); + store.add(toDisposable(() => edit.restore())); + const clear = sinon.stub(registry, 'clearSavedInputs').callsFake(changedInput); + store.add(toDisposable(() => clear.restore())); + if (inputAction === 'edit') { + await registry.editSavedInput('${input:host}', undefined, 'mcp', ConfigurationTarget.USER); + } else { + await registry.clearSavedInputs(StorageScope.PROFILE, '${input:host}'); + } + const afterInputChange = snapshot(); + const quiet = await server.start({ promptType: 'never', errorOnUserInteraction: true }); + const quietResolutions = resolution.callCount; + const pending = server.start({ promptType: 'all-untrusted' }); + const enteredResolution = await raceTimeout(resolving.p.then(() => true), 1000) ?? false; + const duringResolution = snapshot(); + await resume.complete(); + const retried = await pending; + const connection = server.connection.get(); + if (connection) { + await waitForState(connection.handler, Boolean); + } + + const blocked = { state: McpConnectionState.Kind.Error, connected: false, tools: 0, prompts: 0 }; + assert.deepStrictEqual({ + definitionUnchanged: registry.collections.get()[0].serverDefinitions.get()[0] === definition, + afterInputChange, + quiet: quiet.state, + quietResolutions, + enteredResolution, + duringResolution, + retried: retried.state, + afterRetry: snapshot(), + resolutions: resolution.callCount, + transports: transports.length, + }, { + definitionUnchanged: true, + afterInputChange: blocked, + quiet: McpConnectionState.Kind.Error, + quietResolutions: 1, + enteredResolution: true, + duringResolution: blocked, + retried: McpConnectionState.Kind.Running, + afterRetry: { state: McpConnectionState.Kind.Running, connected: true, tools: 1, prompts: 1 }, + resolutions: 2, + transports: 2, + }); + }); + } + + test('cancelled and repeatedly denied input retries keep cached metadata blocked', async () => { + const { server, registry, resolution, resolutionResult, inputChanges, transports, snapshot } = await createBlockedInputServer(); + const resolving = new DeferredPromise(); + const resume = new DeferredPromise(); + const clear = sinon.stub(registry, 'clearSavedInputs').callsFake(async () => inputChanges.fire()); + store.add(toDisposable(() => clear.restore())); + await registry.clearSavedInputs(StorageScope.PROFILE, '${input:host}'); + resolutionResult.beforeResolve = async () => { + void resolving.complete(); + await resume.p; + }; + resolutionResult.cancelled = true; + const pending = server.start({ promptType: 'all-untrusted' }); + const enteredResolution = await raceTimeout(resolving.p.then(() => true), 1000) ?? false; + const duringResolution = snapshot(); + await resume.complete(); + const cancelled = await pending; + const afterCancellation = snapshot(); + resolutionResult.beforeResolve = undefined; + resolutionResult.cancelled = false; + const denied = await server.start({ promptType: 'all-untrusted' }); + + const blocked = { state: McpConnectionState.Kind.Error, connected: false, tools: 0, prompts: 0 }; + assert.deepStrictEqual({ + enteredResolution, + duringResolution, + cancelled: cancelled.state, + afterCancellation, + denied: denied.state, + afterDenial: snapshot(), + resolutions: resolution.callCount, + transports: transports.length, + }, { + enteredResolution: true, + duringResolution: blocked, + cancelled: McpConnectionState.Kind.Stopped, + afterCancellation: blocked, + denied: McpConnectionState.Kind.Error, + afterDenial: blocked, + resolutions: 3, + transports: 1, + }); + }); + + for (const variable of ['${input:host}', '${command:resolveHost}']) { + for (const retained of [false, true]) { + for (const [reason, key, value] of [ + ['disabled access', mcpAccessConfig, McpAccessValue.None], + ['denied name', mcpDeniedServersConfig, [{ serverName: 'Test Server' }]], + ['unlisted name', mcpAllowedServersConfig, [{ serverName: 'Another Server' }]], + ] as const) { + test(`${reason} prevents startup side effects for ${variable} with retained identity ${retained}`, async () => { + const { server, configurationService, resolution, resolutionResult, transports, activation } = createPolicyServer(`https://${variable}/mcp`, 'https://blocked.example/mcp', true); + if (retained) { + await server.start({ promptType: 'never', errorOnUserInteraction: true }); + } + await configurationService.setUserConfiguration(key, value); + configurationService.onDidChangeConfigurationEmitter.fire({ + source: ConfigurationTarget.USER, + affectedKeys: new Set([key]), + change: { keys: [key], overrides: [] }, + affectsConfiguration: candidate => candidate === key, + }); + activation.done = false; + const resolveInputs = sinon.stub().resolves(); + resolutionResult.beforeResolve = resolveInputs; + const result = await server.start({ promptType: 'all-untrusted' }); + assert.deepStrictEqual({ + state: result.state, + activations: activation.calls, + resolutions: resolution.callCount, + inputResolutions: resolveInputs.callCount, + transports: transports.length, + tools: server.tools.get().length, + prompts: server.prompts.get().length, + }, { + state: McpConnectionState.Kind.Error, + activations: 0, + resolutions: retained ? 1 : 0, + inputResolutions: 0, + transports: 0, + tools: 0, + prompts: 0, + }); + }); + } + } + } + + for (const transportType of [McpServerTransportType.HTTP, McpServerTransportType.Stdio]) { + test(`discovery handles repeated incomplete markers without URL rules for transport ${transportType}`, () => { + const configurationService = new TestConfigurationService(); + const allowedMcpServersService = store.add(new AllowedMcpServersService(configurationService)); + const { mcpService, registry } = createMcpService(allowedMcpServersService); + const markers = '${'.repeat(32 * 1024); + const launch: McpServerLaunch = transportType === McpServerTransportType.HTTP + ? { type: McpServerTransportType.HTTP, uri: URI.parse(`https://trusted.example/mcp#${markers}`), headers: [] } + : { type: McpServerTransportType.Stdio, command: 'echo', args: [markers], env: {}, envFile: undefined, cwd: undefined, sandbox: undefined }; + setServerDefinition(registry, { id: 'test-server', label: 'Test Server', cacheNonce: 'a', launch }); + const stopwatch = StopWatch.create(); + mcpService.updateCollectedServers(); + const elapsed = stopwatch.elapsed(); + assert.ok(elapsed < 1000, `Discovery took ${elapsed}ms for 64 KiB of incomplete markers`); + assert.strictEqual(mcpService.servers.get()[0].connectionState.get().state, McpConnectionState.Kind.Stopped); + }); + } + + test('a changed definition releases the retained resolved policy block', async () => { + const { server, registry, definition, transports } = createPolicyServer('https://${input:host}/mcp', 'https://blocked.example/mcp'); + const result = await server.start({ promptType: 'never', errorOnUserInteraction: true }); + const blocked = server.connectionState.get().state; + setServerDefinition(registry, { + ...definition, + cacheNonce: 'b', + launch: { type: McpServerTransportType.HTTP, uri: URI.parse('https://trusted.example/mcp'), headers: [] } + }); + assert.deepStrictEqual({ + result: result.state, + blocked, + changed: server.connectionState.get().state, + transports: transports.length, + }, { + result: McpConnectionState.Kind.Error, + blocked: McpConnectionState.Kind.Error, + changed: McpConnectionState.Kind.Stopped, + transports: 0, + }); + }); + + test('reverting a changed definition does not restore a stale resolved policy block', async () => { + const { server, registry, definition, resolution, resolutionResult, transports } = createPolicyServer('https://${input:host}/mcp', 'https://blocked.example/mcp'); + const initial = await server.start({ promptType: 'never', errorOnUserInteraction: true }); + setServerDefinition(registry, { + ...definition, + cacheNonce: 'b', + launch: { type: McpServerTransportType.HTTP, uri: URI.parse('https://${input:otherHost}/mcp'), headers: [] } + }); + const changed = server.connectionState.get().state; + resolutionResult.url = 'https://trusted.example/mcp'; + setServerDefinition(registry, { ...definition, launch: { ...definition.launch } }); + const reverted = server.connectionState.get().state; + const retried = await server.start({ promptType: 'never', errorOnUserInteraction: true }); + + assert.deepStrictEqual({ + initial: initial.state, + changed, + reverted, + retried: retried.state, + resolutions: resolution.callCount, + transports: transports.length, + }, { + initial: McpConnectionState.Kind.Error, + changed: McpConnectionState.Kind.Stopped, + reverted: McpConnectionState.Kind.Stopped, + retried: McpConnectionState.Kind.Running, + resolutions: 2, + transports: 1, + }); + }); + + test('definition changes during a live connection invalidate its retained identity', async () => { + const { server, configurationService, registry, definition, resolution, transports } = createPolicyServer('https://${input:host}/mcp', 'https://trusted.example/mcp'); + const initial = await server.start({ promptType: 'never', errorOnUserInteraction: true }); + setServerDefinition(registry, { + ...definition, + cacheNonce: 'b', + launch: { type: McpServerTransportType.HTTP, uri: URI.parse('https://${input:otherHost}/mcp'), headers: [] } + }); + setServerDefinition(registry, { ...definition, launch: { ...definition.launch } }); + const reverted = server.connectionState.get().state; + await setDeniedUrls(configurationService, ['https://trusted.example/*']); + const revoked = { state: server.connectionState.get().state, connected: !!server.connection.get() }; + const retried = await server.start({ promptType: 'never', errorOnUserInteraction: true }); + + assert.deepStrictEqual({ + initial: initial.state, + reverted, + revoked, + retried: retried.state, + resolutions: resolution.callCount, + transports: transports.length, + }, { + initial: McpConnectionState.Kind.Running, + reverted: McpConnectionState.Kind.Running, + revoked: { state: McpConnectionState.Kind.Stopped, connected: false }, + retried: McpConnectionState.Kind.Error, + resolutions: 2, + transports: 1, + }); + }); + + test('policy revocation during Starting settles startup and permits a later retry', async () => { + const { server, configurationService, registry, transports } = createPolicyServer('https://${input:host}/mcp', 'https://trusted.example/mcp'); + const starting = new DeferredPromise(); + registry.delegates.set([{ + ...registry.delegates.get()[0], + start: () => { + const transport = registry.makeTestTransport(); + if (transports.length === 1) { + void starting.complete(); + } else { + transport.setConnectionState({ state: McpConnectionState.Kind.Running }); + } + return transport; + }, + }], undefined); + + const pending = server.start({ promptType: 'never', errorOnUserInteraction: true }); + await starting.p; + await setDeniedUrls(configurationService, ['https://trusted.example/*']); + const revoked = await raceTimeout(pending, 1000); + assert.ok(revoked, 'Startup must settle when policy disposes a Starting connection'); + const blocked = server.connectionState.get().state; + await setDeniedUrls(configurationService, []); + const retried = await raceTimeout(server.start({ promptType: 'never', errorOnUserInteraction: true }), 1000); + assert.deepStrictEqual({ + revoked: revoked.state, + blocked, + retried: retried?.state, + transports: transports.length, + }, { + revoked: McpConnectionState.Kind.Error, + blocked: McpConnectionState.Kind.Error, + retried: McpConnectionState.Kind.Running, + transports: 2, + }); + }); + + for (const [host, reason] of [ + ['attacker.example', 'not in the list of servers allowed by your organization'], + ['blocked.example', 'blocked by your organization'], + ]) { + test(`blocks a non-variable fragment at rest for ${host}`, async () => { + const url = `https://${host}/mcp#\${`; + const { server, resolution, transports } = createPolicyServer(url); + const beforeStart = server.connectionState.get().state; + const result = await server.start({ promptType: 'never', errorOnUserInteraction: true }); + + assert.deepStrictEqual({ + unresolvedVariables: [...ConfigurationResolverExpression.parse(url).unresolved()].length, + beforeStart, + blocked: result.state === McpConnectionState.Kind.Error && result.message.includes(reason), + resolutions: resolution.callCount, + transports: transports.length, + }, { + unresolvedVariables: 0, + beforeStart: McpConnectionState.Kind.Error, + blocked: true, + resolutions: 0, + transports: 0, + }); + }); + + for (const fragment of ['${', '${input:literal}']) { + test(`blocks the resolved launch for ${host} with fragment ${fragment}`, async () => { + const { server, resolution, transports } = createPolicyServer('https://${input:host}/mcp', `https://${host}/mcp#${fragment}`); + const beforeStart = server.connectionState.get().state; + const result = await server.start({ promptType: 'never', errorOnUserInteraction: true }); + + assert.deepStrictEqual({ + beforeStart, + blocked: result.state === McpConnectionState.Kind.Error && result.message.includes(reason), + resolutions: resolution.callCount, + transports: transports.length, + connected: server.connection.get() !== undefined, + }, { + beforeStart: McpConnectionState.Kind.Stopped, + blocked: true, + resolutions: 1, + transports: 0, + connected: false, + }); + }); + } + } + + for (const url of ['https://trusted.example/mcp', 'https://trusted.example/mcp#${']) { + test(`preserves unresolved definitions that resolve to ${url}`, async () => { + const { server, resolution, transports } = createPolicyServer('https://${input:host}/mcp', url); + const beforeStart = server.connectionState.get().state; + const result = await server.start({ promptType: 'never', errorOnUserInteraction: true }); + const launch = server.connection.get()?.launchDefinition; + + assert.deepStrictEqual({ + beforeStart, + afterStart: result.state, + resolutions: resolution.callCount, + transports: transports.length, + resolvedUrl: launch?.type === McpServerTransportType.HTTP ? launch.uri.toString(true) : undefined, + }, { + beforeStart: McpConnectionState.Kind.Stopped, + afterStart: McpConnectionState.Kind.Running, + resolutions: 1, + transports: 1, + resolvedUrl: url, + }); + }); + } + + test('stops a resolved URL with a literal variable marker when policy denies it', async () => { + const { server, configurationService } = createPolicyServer('https://${input:host}/mcp', 'https://trusted.example/mcp#${input:literal}'); + const result = await server.start({ promptType: 'never', errorOnUserInteraction: true }); + const connection = server.connection.get(); + + await configurationService.setUserConfiguration(mcpDeniedServersConfig, [{ serverUrl: 'https://trusted.example/*' }]); + configurationService.onDidChangeConfigurationEmitter.fire({ + source: ConfigurationTarget.USER, + affectedKeys: new Set([mcpDeniedServersConfig]), + change: { keys: [mcpDeniedServersConfig], overrides: [] }, + affectsConfiguration: key => key === mcpDeniedServersConfig, + }); + + assert.deepStrictEqual({ + started: result.state, + connected: server.connection.get() !== undefined, + connectionState: connection?.state.get().state, + }, { + started: McpConnectionState.Kind.Running, + connected: false, + connectionState: McpConnectionState.Kind.Stopped, + }); + }); + }); + test('first autostart waits for discovery and loads the newly discovered server tools', async () => { const { mcpService, registry } = createMcpService(); const collection = registry.collections.get()[0]; diff --git a/src/vs/workbench/services/configurationResolver/common/configurationResolverExpression.ts b/src/vs/workbench/services/configurationResolver/common/configurationResolverExpression.ts index 02b8ecb8ab937..2b86b5c17f28b 100644 --- a/src/vs/workbench/services/configurationResolver/common/configurationResolverExpression.ts +++ b/src/vs/workbench/services/configurationResolver/common/configurationResolverExpression.ts @@ -5,19 +5,11 @@ import { Iterable } from '../../../../base/common/iterator.js'; import { isLinux, isMacintosh, isWindows } from '../../../../base/common/platform.js'; +import { IConfigurationVariable, parseConfigurationVariable } from '../../../../platform/configuration/common/configurationVariables.js'; import { ConfiguredInput } from './configurationResolver.js'; /** A replacement found in the object, as ${name} or ${name:arg} */ -export type Replacement = { - /** ${name:arg} */ - id: string; - /** The `name:arg` in ${name:arg} */ - inner: string; - /** The `name` in ${name:arg} */ - name: string; - /** The `arg` in ${name:arg} */ - arg?: string; -}; +export type Replacement = IConfigurationVariable; interface IConfigurationResolverExpression { /** @@ -116,47 +108,6 @@ export class ConfigurationResolverExpression implements IConfigurationResolve delete config.linux; } - private parseVariable(str: string, start: number): { replacement: Replacement; end: number } | undefined { - if (str[start] !== '$' || str[start + 1] !== '{') { - return undefined; - } - - let end = start + 2; - let braceCount = 1; - while (end < str.length) { - if (str[end] === '{') { - braceCount++; - } else if (str[end] === '}') { - braceCount--; - if (braceCount === 0) { - break; - } - } - end++; - } - - if (braceCount !== 0) { - return undefined; - } - - const id = str.slice(start, end + 1); - const inner = str.substring(start + 2, end); - const colonIdx = inner.indexOf(':'); - if (colonIdx === -1) { - return { replacement: { id, name: inner, inner }, end }; - } - - return { - replacement: { - id, - inner, - name: inner.slice(0, colonIdx), - arg: inner.slice(colonIdx + 1) - }, - end - }; - } - private parseObject(obj: any): void { if (typeof obj !== 'object' || obj === null) { return; @@ -192,7 +143,7 @@ export class ConfigurationResolverExpression implements IConfigurationResolve if (match === -1) { break; } - const parsed = this.parseVariable(value, match); + const parsed = parseConfigurationVariable(value, match); if (parsed) { pos = parsed.end + 1; if (replacementPath?.includes(parsed.replacement.id)) { diff --git a/src/vs/workbench/services/mcp/test/common/mcpWorkbenchManagementService.test.ts b/src/vs/workbench/services/mcp/test/common/mcpWorkbenchManagementService.test.ts index 7f6e609475a2e..8d834f8f99984 100644 --- a/src/vs/workbench/services/mcp/test/common/mcpWorkbenchManagementService.test.ts +++ b/src/vs/workbench/services/mcp/test/common/mcpWorkbenchManagementService.test.ts @@ -74,6 +74,7 @@ suite('WorkbenchMcpManagementService - workspace configurations', () => { [IAllowedMcpServersService, upcastPartial({ onDidChangeAllowedMcpServers: Event.None, isAllowed: () => options.allowed === false ? new MarkdownString('Blocked by policy') : true, + isServerAllowedBeforeResolution: () => true, isServerAllowed: () => true, })], [ILogService, logService],