mcp: enforce URL policy after variable resolution

src/vs/platform/configuration/common/configurationVariables.ts

@@ -0,0 +1,66 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+export interface IConfigurationVariable {
+	/** ${name:arg} */
+	id: string;
+	/** The `name:arg` in ${name:arg} */
+	inner: string;
+	/** The `name` in ${name:arg} */
+	name: string;
+	/** The `arg` in ${name:arg} */
+	arg?: string;
+}
+
+/** Parses a configuration variable at the given offset, including nested braces. */
+export function parseConfigurationVariable(value: string, start: number): { replacement: IConfigurationVariable; end: number } | undefined {
+	if (value[start] !== '$' || value[start + 1] !== '{') {
+		return undefined;
+	}
+
+	let end = start + 2;
+	let braceCount = 1;
+	while (end < value.length) {
+		if (value[end] === '{') {
+			braceCount++;
+		} else if (value[end] === '}') {
+			braceCount--;
+			if (braceCount === 0) {
+				break;
+			}
+		}
+		end++;
+	}
+
+	if (braceCount !== 0) {
+		return undefined;
+	}
+
+	const id = value.slice(start, end + 1);
+	const inner = value.substring(start + 2, end);
+	const colon = inner.indexOf(':');
+	return {
+		replacement: colon === -1
+			? { id, name: inner, inner }
+			: { id, inner, name: inner.slice(0, colon), arg: inner.slice(colon + 1) },
+		end
+	};
+}
+
+/** Scans backwards for a variable with a matching closing brace in linear time and constant space. */
+export function hasConfigurationVariable(value: string): boolean {
+	let unmatchedClosingBraces = 0;
+	for (let offset = value.length - 1; offset >= 0; offset--) {
+		if (value[offset] === '}') {
+			unmatchedClosingBraces++;
+		} else if (value[offset] === '{' && unmatchedClosingBraces > 0) {
+			unmatchedClosingBraces--;
+			if (offset > 0 && value[offset - 1] === '$') {
+				return true;
+			}
+		}
+	}
+	return false;
+}

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);
+	}
+});

src/vs/platform/mcp/common/allowedMcpServersService.ts

@@ -7,6 +7,7 @@ import { Disposable } from '../../../base/common/lifecycle.js';
 import * as nls from '../../../nls.js';
 import { createCommandUri, IMarkdownString, MarkdownString } from '../../../base/common/htmlContent.js';
 import { IConfigurationService } from '../../configuration/common/configuration.js';
+import { hasConfigurationVariable } from '../../configuration/common/configurationVariables.js';
 import { Emitter } from '../../../base/common/event.js';
 import { hasKey, isString } from '../../../base/common/types.js';
 import { checkMcpServerAllowed, getMcpServerMatchers, IMcpServerIdentity, IMcpServerMatcher, McpServerAllowResult } from './allowedMcpServers.js';
@@ -33,10 +34,18 @@ export class AllowedMcpServersService extends Disposable implements IAllowedMcpS
 	}
 
 	isAllowed(mcpServer: IGalleryMcpServer | ILocalMcpServer | IInstallableMcpServer): true | IMarkdownString {
-		return this.isServerAllowed(this.toIdentity(mcpServer));
+		return this.isServerAllowedBeforeResolution(this.toIdentity(mcpServer));
+	}
+
+	isServerAllowedBeforeResolution(identity: IMcpServerIdentity): true | IMarkdownString {
+		return this.checkServerAllowed(identity, 'definition');
 	}
 
 	isServerAllowed(identity: IMcpServerIdentity): true | IMarkdownString {
+		return this.checkServerAllowed(identity, 'resolved');
+	}
+
+	private checkServerAllowed(identity: IMcpServerIdentity, phase: 'definition' | 'resolved'): true | IMarkdownString {
 		if (this.configurationService.getValue(mcpAccessConfig) === McpAccessValue.None) {
 			const settingsCommandLink = createCommandUri('workbench.action.openSettings', { query: `@id:${mcpAccessConfig}` }).toString();
 			return new MarkdownString(nls.localize('mcp servers are not allowed', "Model Context Protocol servers are disabled in the Editor. Please check your [settings]({0}).", settingsCommandLink));
@@ -49,7 +58,10 @@ export class AllowedMcpServersService extends Disposable implements IAllowedMcpS
 		const denylist = managedOnly
 			? this.getAllConfiguredMatchers(mcpDeniedServersConfig)
 			: getMcpServerMatchers(this.configurationService.getValue(mcpDeniedServersConfig));
-		switch (this.checkServerAllowedAtCurrentResolution(allowlist, denylist, identity)) {
+		const result = phase === 'definition'
+			? this.checkServerAllowedBeforeResolution(allowlist, denylist, identity)
+			: checkMcpServerAllowed(allowlist, denylist, identity);
+		switch (result) {
 			case McpServerAllowResult.Denied:
 				return new MarkdownString(nls.localize('mcp server is denied', "This Model Context Protocol server is blocked by your organization's policy. Please contact your administrator for more information."));
 			case McpServerAllowResult.NotAllowed:
@@ -59,27 +71,27 @@ export class AllowedMcpServersService extends Disposable implements IAllowedMcpS
 		return true;
 	}
 
-	private checkServerAllowedAtCurrentResolution(allowlist: readonly IMcpServerMatcher[] | undefined, denylist: readonly IMcpServerMatcher[] | undefined, identity: IMcpServerIdentity): McpServerAllowResult {
-		if (!identity.url?.includes('${')) {
+	private checkServerAllowedBeforeResolution(allowlist: readonly IMcpServerMatcher[] | undefined, denylist: readonly IMcpServerMatcher[] | undefined, identity: IMcpServerIdentity): McpServerAllowResult {
+		const unresolvedUrl = identity.url !== undefined && hasConfigurationVariable(identity.url);
+		const unresolvedCommand = identity.command?.some(hasConfigurationVariable) ?? false;
+		if (!unresolvedUrl && !unresolvedCommand) {
 			return checkMcpServerAllowed(allowlist, denylist, identity);
 		}
 
-		const nonUrlIdentity = { name: identity.name };
-		const nonUrlDenylist = denylist?.filter(matcher => !isString(matcher.serverUrl));
-		if (checkMcpServerAllowed(undefined, nonUrlDenylist, nonUrlIdentity) === McpServerAllowResult.Denied) {
+		const knownIdentity: IMcpServerIdentity = {
+			name: identity.name,
+			url: unresolvedUrl ? undefined : identity.url,
+			command: unresolvedCommand ? undefined : identity.command,
+		};
+		if (checkMcpServerAllowed(undefined, denylist, knownIdentity) === McpServerAllowResult.Denied) {
 			return McpServerAllowResult.Denied;
 		}
-		if (allowlist === undefined) {
-			return McpServerAllowResult.Allowed;
-		}
-
-		const nonUrlAllowlist = allowlist.filter(matcher => !isString(matcher.serverUrl));
-		if (checkMcpServerAllowed(nonUrlAllowlist, undefined, nonUrlIdentity) === McpServerAllowResult.Allowed) {
+		if (allowlist === undefined || checkMcpServerAllowed(allowlist, undefined, knownIdentity) === McpServerAllowResult.Allowed) {
 			return McpServerAllowResult.Allowed;
 		}
 
-		// URL matchers are authoritative only after runtime variable resolution.
-		return allowlist.some(matcher => isString(matcher.serverUrl))
+		return allowlist.some(matcher =>
+			(unresolvedUrl && isString(matcher.serverUrl)) || (unresolvedCommand && Array.isArray(matcher.serverCommand)))
 			? McpServerAllowResult.Allowed
 			: McpServerAllowResult.NotAllowed;
 	}

src/vs/platform/mcp/common/mcpManagement.ts

@@ -283,12 +283,15 @@ export interface IAllowedMcpServersService {
 	readonly _serviceBrand: undefined;
 
 	readonly onDidChangeAllowedMcpServers: Event<void>;
+	/** Checks a server definition before resolution, deferring rules for variable-dependent URL/command fields. */
 	isAllowed(mcpServer: IGalleryMcpServer | ILocalMcpServer | IInstallableMcpServer): true | IMarkdownString;
 
+	/** Checks a definition identity before resolution; access and name restrictions are never deferred. */
+	isServerAllowedBeforeResolution(identity: IMcpServerIdentity): true | IMarkdownString;
+
 	/**
-	 * Checks whether an MCP server identified by name / remote URL / local command is permitted by
-	 * the `chat.mcp.allowedServers` allowlist (in addition to the `chat.mcp.access` gate). Used by
-	 * the runtime enforcement path, which does not have a gallery/local/installable representation.
+	 * Checks access and allow/deny rules against a resolved runtime identity without deferring URL rules.
+	 * Runtime callers must use this after resolving a server definition.
 	 */
 	isServerAllowed(identity: IMcpServerIdentity): true | IMarkdownString;
 }