From 773a4768519fc121e117dd16fd6f6a8aa9aec8ff Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Fri, 18 Sep 2026 14:29:17 -0700 Subject: [PATCH 1/5] mcp: enforce URL policy after variable resolution Distinguish preliminary definition checks from authoritative resolved launch checks. Validate the local resolved connection before starting its transport, and re-evaluate active connections whenever policy changes. Add regression coverage for literal variable markers, resolved URL allow/deny rules, permitted unresolved definitions, and policy changes affecting running servers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../mcp/common/allowedMcpServersService.ts | 13 +- src/vs/platform/mcp/common/mcpManagement.ts | 6 +- .../common/allowedMcpServersService.test.ts | 82 ++++++++++ .../workbench/contrib/mcp/common/mcpServer.ts | 21 +-- .../mcp/test/common/mcpService.test.ts | 153 +++++++++++++++++- 5 files changed, 249 insertions(+), 26 deletions(-) diff --git a/src/vs/platform/mcp/common/allowedMcpServersService.ts b/src/vs/platform/mcp/common/allowedMcpServersService.ts index 7dac7121c32c8..c00061f29b4b6 100644 --- a/src/vs/platform/mcp/common/allowedMcpServersService.ts +++ b/src/vs/platform/mcp/common/allowedMcpServersService.ts @@ -33,10 +33,14 @@ export class AllowedMcpServersService extends Disposable implements IAllowedMcpS } isAllowed(mcpServer: IGalleryMcpServer | ILocalMcpServer | IInstallableMcpServer): true | IMarkdownString { - return this.isServerAllowed(this.toIdentity(mcpServer)); + return this.checkServerAllowed(this.toIdentity(mcpServer), '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 +53,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,7 +66,7 @@ export class AllowedMcpServersService extends Disposable implements IAllowedMcpS return true; } - private checkServerAllowedAtCurrentResolution(allowlist: readonly IMcpServerMatcher[] | undefined, denylist: readonly IMcpServerMatcher[] | undefined, identity: IMcpServerIdentity): McpServerAllowResult { + private checkServerAllowedBeforeResolution(allowlist: readonly IMcpServerMatcher[] | undefined, denylist: readonly IMcpServerMatcher[] | undefined, identity: IMcpServerIdentity): McpServerAllowResult { if (!identity.url?.includes('${')) { return checkMcpServerAllowed(allowlist, denylist, identity); } diff --git a/src/vs/platform/mcp/common/mcpManagement.ts b/src/vs/platform/mcp/common/mcpManagement.ts index 50bb5f1cf75a4..3ff65c2ac3632 100644 --- a/src/vs/platform/mcp/common/mcpManagement.ts +++ b/src/vs/platform/mcp/common/mcpManagement.ts @@ -283,12 +283,12 @@ export interface IAllowedMcpServersService { readonly _serviceBrand: undefined; readonly onDidChangeAllowedMcpServers: Event; + /** Checks a server definition before resolution, deferring URL rules when its URL may contain variables. */ isAllowed(mcpServer: IGalleryMcpServer | ILocalMcpServer | IInstallableMcpServer): 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..d74f31ab70b00 100644 --- a/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts +++ b/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts @@ -65,6 +65,88 @@ suite('AllowedMcpServersService', () => { assert.strictEqual(service.isServerAllowed({ name: 's', url: 'https://api.trusted.example.com/mcp' }), true); }); + suite('URL policy resolution', () => { + 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'] }] }); diff --git a/src/vs/workbench/contrib/mcp/common/mcpServer.ts b/src/vs/workbench/contrib/mcp/common/mcpServer.ts index 427a09b083ceb..b5f498766b1c9 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpServer.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpServer.ts @@ -12,7 +12,7 @@ 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'; @@ -539,7 +539,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); @@ -745,15 +745,9 @@ export class McpServer extends Disposable implements IMcpServer { 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 !Iterable.isEmpty(ConfigurationResolverExpression.parse({ url: identity.url, command: identity.command }).unresolved()); } public start({ interaction, autoTrustChanges, promptType, debug, errorOnUserInteraction }: IMcpServerStartOpts = {}): Promise { @@ -821,11 +815,8 @@ export class McpServer extends Disposable implements IMcpServer { } } - // 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; 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..4d042d33732f0 100644 --- a/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts +++ b/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts @@ -11,20 +11,24 @@ import { toDisposable } from '../../../../../base/common/lifecycle.js'; import { autorun, observableValue, waitForState } from '../../../../../base/common/observable.js'; import { URI } from '../../../../../base/common/uri.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, 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 { 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 { 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 { MCP } from '../../common/modelContextProtocol.js'; @@ -34,7 +38,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 +48,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, isServerAllowed: () => true }], ); const parentInstantiationService = store.add(new TestInstantiationService(services)); @@ -52,7 +56,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 +67,145 @@ suite('Workbench - MCP - McpService', () => { }], undefined); }; + suite('URL policy resolution', () => { + const createPolicyServer = (definitionUrl: string, resolvedUrl = definitionUrl) => { + 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 resolution = sinon.stub(registry, 'resolveConnection').callsFake(async options => store.add(instantiationService.createInstance( + McpServerConnection, + registry.collections.get()[0], + definition, + registry.delegates.get()[0], + { type: McpServerTransportType.HTTP, uri: URI.parse(resolvedUrl), 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, transports }; + }; + + 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]; From 0052d689f910e056718b44fdecc7a5e1ad2c3b66 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Fri, 18 Sep 2026 16:43:13 -0700 Subject: [PATCH 2/5] mcp: preserve policy blocks and settle cancelled startup Keep resolved policy blocks effective after disposing a connection, including suppression of cached tools and prompts. Settle startup waits on explicit stop or disposal so policy restoration permits a retry without regressing normal transport restarts. Share configuration-variable parsing with preliminary MCP checks so literal incomplete markers do not defer allow/deny enforcement. Add regression coverage for retained blocks, definition changes, preliminary checks, and cancelled startup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../common/configurationVariables.ts | 59 +++++++++ .../mcp/common/allowedMcpServersService.ts | 3 +- .../common/allowedMcpServersService.test.ts | 30 +++++ .../workbench/contrib/mcp/common/mcpServer.ts | 34 ++--- .../contrib/mcp/common/mcpServerConnection.ts | 27 +++- .../test/common/mcpServerConnection.test.ts | 40 +++++- .../mcp/test/common/mcpService.test.ts | 125 +++++++++++++++++- .../common/configurationResolverExpression.ts | 55 +------- 8 files changed, 294 insertions(+), 79 deletions(-) create mode 100644 src/vs/platform/configuration/common/configurationVariables.ts diff --git a/src/vs/platform/configuration/common/configurationVariables.ts b/src/vs/platform/configuration/common/configurationVariables.ts new file mode 100644 index 0000000000000..f0e94d12c9acc --- /dev/null +++ b/src/vs/platform/configuration/common/configurationVariables.ts @@ -0,0 +1,59 @@ +/*--------------------------------------------------------------------------------------------- + * 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 + }; +} + +export function hasConfigurationVariable(value: string): boolean { + for (let offset = value.indexOf('${'); offset !== -1; offset = value.indexOf('${', offset + 2)) { + if (parseConfigurationVariable(value, offset)) { + return true; + } + } + return false; +} diff --git a/src/vs/platform/mcp/common/allowedMcpServersService.ts b/src/vs/platform/mcp/common/allowedMcpServersService.ts index c00061f29b4b6..4bf8277948743 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'; @@ -67,7 +68,7 @@ export class AllowedMcpServersService extends Disposable implements IAllowedMcpS } private checkServerAllowedBeforeResolution(allowlist: readonly IMcpServerMatcher[] | undefined, denylist: readonly IMcpServerMatcher[] | undefined, identity: IMcpServerIdentity): McpServerAllowResult { - if (!identity.url?.includes('${')) { + if (!identity.url || !hasConfigurationVariable(identity.url)) { return checkMcpServerAllowed(allowlist, denylist, identity); } diff --git a/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts b/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts index d74f31ab70b00..b1381a6cd7fdc 100644 --- a/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts +++ b/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts @@ -66,6 +66,36 @@ suite('AllowedMcpServersService', () => { }); 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({ diff --git a/src/vs/workbench/contrib/mcp/common/mcpServer.ts b/src/vs/workbench/contrib/mcp/common/mcpServer.ts index b5f498766b1c9..54f2867ad6a0e 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpServer.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpServer.ts @@ -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 }); @@ -547,6 +537,11 @@ export class McpServer extends Disposable implements IMcpServer { // Authoritative: the connection carries the fully resolved launch. return this._evaluatePolicy(this._identityFromLaunch(connection.launchDefinition)); } + 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); + } // 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 @@ -554,7 +549,7 @@ export class McpServer extends Disposable implements IMcpServer { // — 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; + const launch = definition?.launch; if (!launch) { return undefined; } @@ -754,8 +749,6 @@ export class McpServer extends Disposable implements IMcpServer { 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; @@ -808,7 +801,11 @@ 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(); @@ -846,6 +843,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/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 4d042d33732f0..070bd4862f8b3 100644 --- a/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts +++ b/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts @@ -5,7 +5,7 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; -import { DeferredPromise, timeout } from '../../../../../base/common/async.js'; +import { DeferredPromise, raceTimeout, timeout } from '../../../../../base/common/async.js'; import { Event } from '../../../../../base/common/event.js'; import { toDisposable } from '../../../../../base/common/lifecycle.js'; import { autorun, observableValue, waitForState } from '../../../../../base/common/observable.js'; @@ -107,9 +107,130 @@ suite('Workbench - MCP - McpService', () => { return transport; }; mcpService.updateCollectedServers(); - return { server: mcpService.servers.get()[0], configurationService, resolution, transports }; + return { server: mcpService.servers.get()[0], configurationService, resolution, transports, registry, mcpService, definition }; }; + 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, + }); + }; + + 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'); + 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; + }; + + 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 }, + }); + }); + + 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('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'], 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)) { From e635d79b2f4072b0bedca7f8b871003ef3096edc Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Fri, 18 Sep 2026 22:07:25 -0700 Subject: [PATCH 3/5] mcp: invalidate retained policy identity on definition changes Discard the retained resolved launch identity on the first non-equivalent definition change, including while a connection is live. Returning from definition B to A must not revive A's stale policy result or prevent its variables from being resolved again. Reuse the existing policy-enforcement observer and preserve the equivalent-definition and resolved-launch checks. Add failing-first regressions for stopped and live connection histories. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workbench/contrib/mcp/common/mcpServer.ts | 10 ++- .../mcp/test/common/mcpService.test.ts | 67 ++++++++++++++++++- 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/src/vs/workbench/contrib/mcp/common/mcpServer.ts b/src/vs/workbench/contrib/mcp/common/mcpServer.ts index 54f2867ad6a0e..3c5999c2c01a8 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpServer.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpServer.ts @@ -560,10 +560,14 @@ export class McpServer extends Disposable implements IMcpServer { return this._evaluatePolicy(identity); }); - // 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 } 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 070bd4862f8b3..e732bae632266 100644 --- a/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts +++ b/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts @@ -83,12 +83,13 @@ suite('Workbench - MCP - McpService', () => { }; setServerDefinition(registry, definition); + const resolutionResult = { url: resolvedUrl }; const resolution = sinon.stub(registry, 'resolveConnection').callsFake(async options => store.add(instantiationService.createInstance( McpServerConnection, registry.collections.get()[0], definition, registry.delegates.get()[0], - { type: McpServerTransportType.HTTP, uri: URI.parse(resolvedUrl), headers: [] }, + { type: McpServerTransportType.HTTP, uri: URI.parse(resolutionResult.url), headers: [] }, new NullLogger(), true, options.taskManager, @@ -107,7 +108,7 @@ suite('Workbench - MCP - McpService', () => { return transport; }; mcpService.updateCollectedServers(); - return { server: mcpService.servers.get()[0], configurationService, resolution, transports, registry, mcpService, definition }; + return { server: mcpService.servers.get()[0], configurationService, resolution, resolutionResult, transports, registry, mcpService, definition }; }; const setDeniedUrls = async (configurationService: TestConfigurationService, urls: string[]) => { @@ -194,6 +195,68 @@ suite('Workbench - MCP - McpService', () => { }); }); + 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(); From da77578fcfa68bec03bdb3d72dddbc8f8ddd6579 Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sun, 20 Sep 2026 09:03:26 -0700 Subject: [PATCH 4/5] mcp: recover input retries and scan variables in linear time Allow interactive retries to resolve current inputs after a retained policy denial without clearing the block that hides cached metadata. Quiet starts remain blocked, and cancellation or repeated denial keeps the cache hidden until a resolved destination is permitted. Use one shared linear-time, constant-space balanced-variable detector for preliminary checks and workbench discovery instead of repeatedly parsing unmatched suffixes. Preserve the existing nested-variable syntax and avoid constructing resolver expressions just to detect variables. Add failing-first edit/reset, cancellation, repeated-denial and 64 KiB discovery regressions, plus differential grammar coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../common/configurationVariables.ts | 13 +- .../common/configurationVariables.test.ts | 123 +++++++++++ .../workbench/contrib/mcp/common/mcpServer.ts | 13 +- .../mcp/test/common/mcpService.test.ts | 193 ++++++++++++++++-- 4 files changed, 319 insertions(+), 23 deletions(-) create mode 100644 src/vs/platform/configuration/test/common/configurationVariables.test.ts diff --git a/src/vs/platform/configuration/common/configurationVariables.ts b/src/vs/platform/configuration/common/configurationVariables.ts index f0e94d12c9acc..ababc715fef53 100644 --- a/src/vs/platform/configuration/common/configurationVariables.ts +++ b/src/vs/platform/configuration/common/configurationVariables.ts @@ -49,10 +49,17 @@ export function parseConfigurationVariable(value: string, start: number): { repl }; } +/** Scans backwards for a variable with a matching closing brace in linear time and constant space. */ export function hasConfigurationVariable(value: string): boolean { - for (let offset = value.indexOf('${'); offset !== -1; offset = value.indexOf('${', offset + 2)) { - if (parseConfigurationVariable(value, offset)) { - return true; + 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/workbench/contrib/mcp/common/mcpServer.ts b/src/vs/workbench/contrib/mcp/common/mcpServer.ts index 3c5999c2c01a8..060884d045418 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpServer.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpServer.ts @@ -19,6 +19,7 @@ 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'; @@ -746,7 +746,8 @@ export class McpServer extends Disposable implements IMcpServer { /** Whether policy URL/command fields contain unresolved configuration variables; the server name is literal. */ private static _hasUnresolvedVariables(identity: IMcpServerIdentity): boolean { - return !Iterable.isEmpty(ConfigurationResolverExpression.parse({ url: identity.url, command: identity.command }).unresolved()); + return (identity.url !== undefined && hasConfigurationVariable(identity.url)) + || (identity.command?.some(hasConfigurationVariable) ?? false); } public start({ interaction, autoTrustChanges, promptType, debug, errorOnUserInteraction }: IMcpServerStartOpts = {}): Promise { @@ -755,7 +756,13 @@ export class McpServer extends Disposable implements IMcpServer { return this._connectionSequencer.queue(async () => { const preStartBlock = this._policyBlock.get(); if (preStartBlock) { - return preStartBlock; + const definition = this._fullDefinitions.get().server; + const canResolveAgain = !errorOnUserInteraction && this._resolvedPolicyIdentity.get() + && definition && McpServer._hasUnresolvedVariables(this._identityFromLaunch(definition.launch)); + // 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)); 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 e732bae632266..15d069ecf2579 100644 --- a/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts +++ b/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts @@ -6,9 +6,10 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import { DeferredPromise, raceTimeout, timeout } from '../../../../../base/common/async.js'; -import { Event } from '../../../../../base/common/event.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 { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { ConfigurationTarget } from '../../../../../platform/configuration/common/configuration.js'; @@ -20,7 +21,7 @@ import { ILoggerService, NullLogger, NullLogService } from '../../../../../platf import { AllowedMcpServersService } from '../../../../../platform/mcp/common/allowedMcpServersService.js'; import { IAllowedMcpServersService, 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'; @@ -30,7 +31,7 @@ import { TestContextService, TestLoggerService, TestProductService, TestStorageS 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'; @@ -83,17 +84,25 @@ suite('Workbench - MCP - McpService', () => { }; setServerDefinition(registry, definition); - const resolutionResult = { url: resolvedUrl }; - const resolution = sinon.stub(registry, 'resolveConnection').callsFake(async options => 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, - ))); + 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[] = []; @@ -108,7 +117,7 @@ suite('Workbench - MCP - McpService', () => { return transport; }; mcpService.updateCollectedServers(); - return { server: mcpService.servers.get()[0], configurationService, resolution, resolutionResult, transports, registry, mcpService, definition }; + return { server: mcpService.servers.get()[0], configurationService, resolution, resolutionResult, transports, registry, mcpService, definition, inputChanges }; }; const setDeniedUrls = async (configurationService: TestConfigurationService, urls: string[]) => { @@ -121,8 +130,7 @@ suite('Workbench - MCP - McpService', () => { }); }; - 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'); + const provideCachedMetadata = (registry: TestMcpRegistry) => { const createTransport = registry.makeTestTransport; registry.makeTestTransport = () => { const transport = createTransport(); @@ -145,7 +153,34 @@ suite('Workbench - MCP - McpService', () => { })); 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), @@ -173,6 +208,130 @@ suite('Workbench - MCP - McpService', () => { }); }); + 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 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 }); From 8606c58260ea8a2f60f7bd640dcc5fd4ffc965aa Mon Sep 17 00:00:00 2001 From: Dmitriy Vasyura Date: Sun, 20 Sep 2026 11:11:39 -0700 Subject: [PATCH 5/5] mcp: enforce static policy before server resolution Reject access and name restrictions before provider activation or variable resolution, including interactive retries that retain a previous URL denial. Expose the shared preliminary identity check and apply it at rest and before allowing re-resolution. Defer only rules for unresolved URL/command fields; keep the resolved runtime check authoritative. Update test doubles for the internal service contract. Add failing-first coverage for initial starts and retained-identity retries with input/command variables, plus command-policy deferral controls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../mcp/common/allowedMcpServersService.ts | 30 +++++---- src/vs/platform/mcp/common/mcpManagement.ts | 5 +- .../common/allowedMcpServersService.test.ts | 35 ++++++++++ .../test/common/mcpManagementService.test.ts | 12 ++-- .../workbench/contrib/mcp/common/mcpServer.ts | 22 ++----- .../test/browser/mcpWorkbenchService.test.ts | 2 +- .../test/common/mcpResourceFilesystem.test.ts | 2 +- .../mcp/test/common/mcpService.test.ts | 64 +++++++++++++++++-- .../mcpWorkbenchManagementService.test.ts | 1 + 9 files changed, 132 insertions(+), 41 deletions(-) diff --git a/src/vs/platform/mcp/common/allowedMcpServersService.ts b/src/vs/platform/mcp/common/allowedMcpServersService.ts index 4bf8277948743..7733f56523d80 100644 --- a/src/vs/platform/mcp/common/allowedMcpServersService.ts +++ b/src/vs/platform/mcp/common/allowedMcpServersService.ts @@ -34,7 +34,11 @@ export class AllowedMcpServersService extends Disposable implements IAllowedMcpS } isAllowed(mcpServer: IGalleryMcpServer | ILocalMcpServer | IInstallableMcpServer): true | IMarkdownString { - return this.checkServerAllowed(this.toIdentity(mcpServer), 'definition'); + return this.isServerAllowedBeforeResolution(this.toIdentity(mcpServer)); + } + + isServerAllowedBeforeResolution(identity: IMcpServerIdentity): true | IMarkdownString { + return this.checkServerAllowed(identity, 'definition'); } isServerAllowed(identity: IMcpServerIdentity): true | IMarkdownString { @@ -68,26 +72,26 @@ export class AllowedMcpServersService extends Disposable implements IAllowedMcpS } private checkServerAllowedBeforeResolution(allowlist: readonly IMcpServerMatcher[] | undefined, denylist: readonly IMcpServerMatcher[] | undefined, identity: IMcpServerIdentity): McpServerAllowResult { - if (!identity.url || !hasConfigurationVariable(identity.url)) { + 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 3ff65c2ac3632..2978d43addd9b 100644 --- a/src/vs/platform/mcp/common/mcpManagement.ts +++ b/src/vs/platform/mcp/common/mcpManagement.ts @@ -283,9 +283,12 @@ export interface IAllowedMcpServersService { readonly _serviceBrand: undefined; readonly onDidChangeAllowedMcpServers: Event; - /** Checks a server definition before resolution, deferring URL rules when its URL may contain variables. */ + /** 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 access and allow/deny rules against a resolved runtime identity without deferring URL rules. * Runtime callers must use this after resolving a server definition. diff --git a/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts b/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts index b1381a6cd7fdc..9483e2ee51cb8 100644 --- a/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts +++ b/src/vs/platform/mcp/test/common/allowedMcpServersService.test.ts @@ -187,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 060884d045418..63c6c0b8de7b3 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpServer.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpServer.ts @@ -542,22 +542,11 @@ export class McpServer extends Disposable implements IMcpServer { if (resolved && definition && McpServerDefinition.equals(resolved.definition, definition)) { return this._evaluatePolicy(resolved.identity); } - // 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 = definition?.launch; if (!launch) { return undefined; } - const identity = this._identityFromLaunch(launch); - if (McpServer._hasUnresolvedVariables(identity)) { - return undefined; - } - return this._evaluatePolicy(identity); + return this._evaluatePolicy(this._identityFromLaunch(launch), 'definition'); }); this._register(autorun(reader => { @@ -739,8 +728,10 @@ 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 }; } @@ -757,8 +748,9 @@ export class McpServer extends Disposable implements IMcpServer { const preStartBlock = this._policyBlock.get(); if (preStartBlock) { const definition = this._fullDefinitions.get().server; + const identity = definition && this._identityFromLaunch(definition.launch); const canResolveAgain = !errorOnUserInteraction && this._resolvedPolicyIdentity.get() - && definition && McpServer._hasUnresolvedVariables(this._identityFromLaunch(definition.launch)); + && identity && McpServer._hasUnresolvedVariables(identity) && !this._evaluatePolicy(identity, 'definition'); // Keep cached metadata blocked while an interactive retry resolves the current inputs. if (!canResolveAgain) { return preStartBlock; 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/mcpService.test.ts b/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts index 15d069ecf2579..e0dc6f1f0dca0 100644 --- a/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts +++ b/src/vs/workbench/contrib/mcp/test/common/mcpService.test.ts @@ -11,6 +11,7 @@ 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'; @@ -19,7 +20,7 @@ import { ServiceCollection } from '../../../../../platform/instantiation/common/ import { TestInstantiationService } from '../../../../../platform/instantiation/test/common/instantiationServiceMock.js'; import { ILoggerService, NullLogger, NullLogService } from '../../../../../platform/log/common/log.js'; import { AllowedMcpServersService } from '../../../../../platform/mcp/common/allowedMcpServersService.js'; -import { IAllowedMcpServersService, mcpAllowedServersConfig, mcpAutoStartConfig, McpAutoStartValue, mcpDeniedServersConfig } from '../../../../../platform/mcp/common/mcpManagement.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, StorageScope } from '../../../../../platform/storage/common/storage.js'; import { ITelemetryService } from '../../../../../platform/telemetry/common/telemetry.js'; @@ -27,6 +28,7 @@ import { NullTelemetryService } from '../../../../../platform/telemetry/common/t 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'; @@ -49,7 +51,7 @@ suite('Workbench - MCP - McpService', () => { [IWorkbenchEnvironmentService, {}], [ITelemetryService, NullTelemetryService], [IProductService, TestProductService], - [IAllowedMcpServersService, allowedMcpServersService ?? { _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)); @@ -69,7 +71,7 @@ suite('Workbench - MCP - McpService', () => { }; suite('URL policy resolution', () => { - const createPolicyServer = (definitionUrl: string, resolvedUrl = definitionUrl) => { + const createPolicyServer = (definitionUrl: string, resolvedUrl = definitionUrl, requiresActivation = false) => { const configurationService = new TestConfigurationService({ [mcpAllowedServersConfig]: [{ serverUrl: 'https://trusted.example/mcp' }], [mcpDeniedServersConfig]: [{ serverUrl: 'https://blocked.example/*' }], @@ -83,6 +85,15 @@ suite('Workbench - MCP - McpService', () => { 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; @@ -117,7 +128,7 @@ suite('Workbench - MCP - McpService', () => { return transport; }; mcpService.updateCollectedServers(); - return { server: mcpService.servers.get()[0], configurationService, resolution, resolutionResult, transports, registry, mcpService, definition, inputChanges }; + return { server: mcpService.servers.get()[0], configurationService, resolution, resolutionResult, transports, registry, mcpService, definition, inputChanges, activation }; }; const setDeniedUrls = async (configurationService: TestConfigurationService, urls: string[]) => { @@ -314,6 +325,51 @@ suite('Workbench - MCP - McpService', () => { }); }); + 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(); 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],