[MCP-Sandboxing]Changes for variable substitution and code refactor (#299124)

changes to enable variable substitution for sandboxing
This commit is contained in:
dileepyavan
2026-03-04 11:18:03 -08:00
committed by GitHub
parent 54780aa577
commit 02c0cbabce
16 changed files with 162 additions and 57 deletions

View File

@@ -10,13 +10,14 @@ import { IIterativePager } from '../../../base/common/paging.js';
import { URI } from '../../../base/common/uri.js';
import { SortBy, SortOrder } from '../../extensionManagement/common/extensionManagement.js';
import { createDecorator } from '../../instantiation/common/instantiation.js';
import { IMcpServerConfiguration, IMcpServerVariable } from './mcpPlatformTypes.js';
import { IMcpSandboxConfiguration, IMcpServerConfiguration, IMcpServerVariable } from './mcpPlatformTypes.js';
export type InstallSource = 'gallery' | 'local';
export interface ILocalMcpServer {
readonly name: string;
readonly config: IMcpServerConfiguration;
readonly rootSandbox?: IMcpSandboxConfiguration;
readonly version?: string;
readonly mcpResource: URI;
readonly location?: URI;

View File

@@ -22,7 +22,7 @@ import { ILogService } from '../../log/common/log.js';
import { IUriIdentityService } from '../../uriIdentity/common/uriIdentity.js';
import { IUserDataProfilesService } from '../../userDataProfile/common/userDataProfile.js';
import { DidUninstallMcpServerEvent, IGalleryMcpServer, ILocalMcpServer, IMcpGalleryService, IMcpManagementService, IMcpServerInput, IGalleryMcpServerConfiguration, InstallMcpServerEvent, InstallMcpServerResult, RegistryType, UninstallMcpServerEvent, InstallOptions, UninstallOptions, IInstallableMcpServer, IAllowedMcpServersService, IMcpServerArgument, IMcpServerKeyValueInput, McpServerConfigurationParseResult } from './mcpManagement.js';
import { IMcpServerVariable, McpServerVariableType, IMcpServerConfiguration, McpServerType } from './mcpPlatformTypes.js';
import { IMcpSandboxConfiguration, IMcpServerVariable, McpServerVariableType, IMcpServerConfiguration, McpServerType } from './mcpPlatformTypes.js';
import { IMcpResourceScannerService, McpResourceTarget } from './mcpResourceScannerService.js';
export interface ILocalMcpServerInfo {
@@ -358,7 +358,7 @@ export abstract class AbstractMcpResourceManagementService extends AbstractCommo
const scannedMcpServers = await this.mcpResourceScannerService.scanMcpServers(this.mcpResource, this.target);
if (scannedMcpServers.servers) {
await Promise.allSettled(Object.entries(scannedMcpServers.servers).map(async ([name, scannedServer]) => {
const server = await this.scanLocalServer(name, scannedServer);
const server = await this.scanLocalServer(name, scannedServer, scannedMcpServers.sandbox);
local.set(name, server);
}));
}
@@ -426,7 +426,7 @@ export abstract class AbstractMcpResourceManagementService extends AbstractCommo
return Array.from(this.local.values());
}
protected async scanLocalServer(name: string, config: IMcpServerConfiguration): Promise<ILocalMcpServer> {
protected async scanLocalServer(name: string, config: IMcpServerConfiguration, rootSandbox?: IMcpSandboxConfiguration): Promise<ILocalMcpServer> {
let mcpServerInfo = await this.getLocalServerInfo(name, config);
if (!mcpServerInfo) {
mcpServerInfo = { name, version: config.version, galleryUrl: isString(config.gallery) ? config.gallery : undefined };
@@ -435,6 +435,7 @@ export abstract class AbstractMcpResourceManagementService extends AbstractCommo
return {
name,
config,
rootSandbox,
mcpResource: this.mcpResource,
version: mcpServerInfo.version,
location: mcpServerInfo.location,

View File

@@ -58,7 +58,6 @@ export interface IMcpStdioServerConfiguration extends ICommonMcpServerConfigurat
readonly envFile?: string;
readonly cwd?: string;
readonly sandboxEnabled?: boolean;
readonly sandbox?: IMcpSandboxConfiguration;
readonly dev?: IMcpDevModeConfig;
}

View File

@@ -188,7 +188,7 @@ export class McpResourceScannerService extends Disposable implements IMcpResourc
if (servers.length > 0) {
userMcpServers.servers = {};
for (const [serverName, server] of servers) {
userMcpServers.servers[serverName] = this.sanitizeServer(server, scannedMcpServers.sandbox);
userMcpServers.servers[serverName] = this.sanitizeServer(server);
}
}
return userMcpServers;
@@ -203,14 +203,14 @@ export class McpResourceScannerService extends Disposable implements IMcpResourc
if (servers.length > 0) {
scannedMcpServers.servers = {};
for (const [serverName, config] of servers) {
const serverConfig = this.sanitizeServer(config, scannedMcpServers.sandbox);
const serverConfig = this.sanitizeServer(config);
scannedMcpServers.servers[serverName] = serverConfig;
}
}
return scannedMcpServers;
}
private sanitizeServer(serverOrConfig: IOldScannedMcpServer | Mutable<IMcpServerConfiguration>, sandbox?: IMcpSandboxConfiguration): IMcpServerConfiguration {
private sanitizeServer(serverOrConfig: IOldScannedMcpServer | Mutable<IMcpServerConfiguration>): IMcpServerConfiguration {
let server: IMcpServerConfiguration;
if ((<IOldScannedMcpServer>serverOrConfig).config) {
const oldScannedMcpServer = <IOldScannedMcpServer>serverOrConfig;
@@ -226,11 +226,6 @@ export class McpResourceScannerService extends Disposable implements IMcpResourc
if (server.type === undefined || (server.type !== McpServerType.REMOTE && server.type !== McpServerType.LOCAL)) {
(<Mutable<ICommonMcpServerConfiguration>>server).type = (<IMcpStdioServerConfiguration>server).command ? McpServerType.LOCAL : McpServerType.REMOTE;
}
if (sandbox && server.type === McpServerType.LOCAL) {
(<Mutable<IMcpStdioServerConfiguration>>server).sandbox = sandbox;
}
return server;
}

View File

@@ -4,14 +4,22 @@
*--------------------------------------------------------------------------------------------*/
import assert from 'assert';
import { VSBuffer } from '../../../../base/common/buffer.js';
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js';
import { AbstractCommonMcpManagementService } from '../../common/mcpManagementService.js';
import { IGalleryMcpServer, IGalleryMcpServerConfiguration, IInstallableMcpServer, ILocalMcpServer, InstallOptions, RegistryType, TransportType, UninstallOptions } from '../../common/mcpManagement.js';
import { McpServerType, McpServerVariableType, IMcpServerVariable } from '../../common/mcpPlatformTypes.js';
import { DisposableStore } from '../../../../base/common/lifecycle.js';
import { Schemas } from '../../../../base/common/network.js';
import { AbstractCommonMcpManagementService, AbstractMcpResourceManagementService } from '../../common/mcpManagementService.js';
import { IGalleryMcpServer, IGalleryMcpServerConfiguration, IInstallableMcpServer, ILocalMcpServer, IMcpGalleryService, InstallOptions, RegistryType, TransportType, UninstallOptions } from '../../common/mcpManagement.js';
import { IMcpSandboxConfiguration, McpServerType, McpServerVariableType, IMcpServerConfiguration, IMcpServerVariable } from '../../common/mcpPlatformTypes.js';
import { IMarkdownString } from '../../../../base/common/htmlContent.js';
import { Event } from '../../../../base/common/event.js';
import { URI } from '../../../../base/common/uri.js';
import { ConfigurationTarget } from '../../../configuration/common/configuration.js';
import { FileService } from '../../../files/common/fileService.js';
import { InMemoryFileSystemProvider } from '../../../files/common/inMemoryFilesystemProvider.js';
import { NullLogService } from '../../../log/common/log.js';
import { McpResourceScannerService } from '../../common/mcpResourceScannerService.js';
import { UriIdentityService } from '../../../uriIdentity/common/uriIdentityService.js';
class TestMcpManagementService extends AbstractCommonMcpManagementService {
@@ -42,6 +50,44 @@ class TestMcpManagementService extends AbstractCommonMcpManagementService {
}
}
class TestMcpResourceManagementService extends AbstractMcpResourceManagementService {
constructor(mcpResource: URI, fileService: FileService, uriIdentityService: UriIdentityService, mcpResourceScannerService: McpResourceScannerService) {
super(
mcpResource,
ConfigurationTarget.USER,
{} as IMcpGalleryService,
fileService,
uriIdentityService,
new NullLogService(),
mcpResourceScannerService,
);
}
public reload(): Promise<void> {
return this.updateLocal();
}
override canInstall(_server: IGalleryMcpServer | IInstallableMcpServer): true | IMarkdownString {
throw new Error('Not supported');
}
protected override getLocalServerInfo(_name: string, _mcpServerConfig: IMcpServerConfiguration) {
return Promise.resolve(undefined);
}
protected override installFromUri(_uri: URI): Promise<ILocalMcpServer> {
throw new Error('Not supported');
}
override installFromGallery(_server: IGalleryMcpServer, _options?: InstallOptions): Promise<ILocalMcpServer> {
throw new Error('Not supported');
}
override updateMetadata(_local: ILocalMcpServer, _server: IGalleryMcpServer): Promise<ILocalMcpServer> {
throw new Error('Not supported');
}
}
suite('McpManagementService - getMcpServerConfigurationFromManifest', () => {
let service: TestMcpManagementService;
@@ -1073,3 +1119,74 @@ suite('McpManagementService - getMcpServerConfigurationFromManifest', () => {
});
});
});
suite('McpResourceManagementService', () => {
const mcpResource = URI.from({ scheme: Schemas.inMemory, path: '/mcp.json' });
let disposables: DisposableStore;
let fileService: FileService;
let service: TestMcpResourceManagementService;
setup(async () => {
disposables = new DisposableStore();
fileService = disposables.add(new FileService(new NullLogService()));
disposables.add(fileService.registerProvider(Schemas.inMemory, disposables.add(new InMemoryFileSystemProvider())));
const uriIdentityService = disposables.add(new UriIdentityService(fileService));
const scannerService = disposables.add(new McpResourceScannerService(fileService, uriIdentityService));
service = disposables.add(new TestMcpResourceManagementService(mcpResource, fileService, uriIdentityService, scannerService));
await fileService.writeFile(mcpResource, VSBuffer.fromString(JSON.stringify({
sandbox: {
network: { allowedDomains: ['example.com'] }
},
servers: {
test: {
type: 'stdio',
command: 'node',
sandboxEnabled: true
}
}
}, null, '\t')));
});
teardown(() => {
disposables.dispose();
});
ensureNoDisposablesAreLeakedInTestSuite();
test('fires update when root sandbox changes', async () => {
const initial = await service.getInstalled();
assert.strictEqual(initial.length, 1);
assert.deepStrictEqual(initial[0].rootSandbox, {
network: { allowedDomains: ['example.com'] }
});
let updateCount = 0;
const updatePromise = new Promise<void>(resolve => disposables.add(service.onDidUpdateMcpServers(e => {
assert.strictEqual(e.length, 1);
updateCount++;
resolve();
})));
const updatedSandbox: IMcpSandboxConfiguration = {
network: { allowedDomains: ['changed.example.com'] }
};
await fileService.writeFile(mcpResource, VSBuffer.fromString(JSON.stringify({
sandbox: updatedSandbox,
servers: {
test: {
type: 'stdio',
command: 'node',
sandboxEnabled: true
}
}
}, null, '\t')));
await service.reload();
await updatePromise;
const updated = await service.getInstalled();
assert.strictEqual(updateCount, 1);
assert.deepStrictEqual(updated[0].rootSandbox, updatedSandbox);
});
});

View File

@@ -4052,6 +4052,7 @@ export namespace McpServerDefinition {
command: item.command,
env: item.env,
envFile: undefined,
sandbox: undefined
}
);
}

View File

@@ -96,6 +96,7 @@ export class InstalledMcpServersDiscovery extends Disposable implements IMcpDisc
env: config.env || {},
envFile: config.envFile,
cwd: config.cwd,
sandbox: server.rootSandbox
};
definitions[1].push({
@@ -103,7 +104,6 @@ export class InstalledMcpServersDiscovery extends Disposable implements IMcpDisc
label: server.name,
launch,
sandboxEnabled: config.type === 'http' ? undefined : config.sandboxEnabled,
sandbox: config.type === 'http' ? undefined : config.sandbox,
cacheNonce: await McpServerLaunch.hash(launch),
roots: mcpConfigPath?.workspaceFolder ? [mcpConfigPath.workspaceFolder.uri] : undefined,
variableReplacement: {

View File

@@ -49,6 +49,7 @@ export async function claudeConfigToServerDefinition(idPrefix: string, contents:
env: server.env || {},
envFile: undefined,
cwd: cwd?.fsPath,
sandbox: undefined
};
return {

View File

@@ -100,6 +100,7 @@ export class PluginMcpDiscovery extends Disposable implements IMcpDiscovery {
env: config.env ? { ...config.env } : {},
envFile: config.envFile,
cwd: config.cwd,
sandbox: undefined,
};
}

View File

@@ -19,9 +19,8 @@ import { ILogService } from '../../../../platform/log/common/log.js';
import { IMcpResourceScannerService, McpResourceTarget } from '../../../../platform/mcp/common/mcpResourceScannerService.js';
import { IRemoteAgentEnvironment } from '../../../../platform/remote/common/remoteAgentEnvironment.js';
import { IRemoteAgentService } from '../../../services/remote/common/remoteAgentService.js';
import { IMcpSandboxConfiguration, IMcpStdioServerConfiguration, McpServerType } from '../../../../platform/mcp/common/mcpPlatformTypes.js';
import { IMcpSandboxConfiguration } from '../../../../platform/mcp/common/mcpPlatformTypes.js';
import { IMcpPotentialSandboxBlock, McpServerDefinition, McpServerLaunch, McpServerTransportType } from './mcpTypes.js';
import { Mutable } from '../../../../base/common/types.js';
export const IMcpSandboxService = createDecorator<IMcpSandboxService>('mcpSandboxService');
@@ -85,7 +84,7 @@ export class McpSandboxService extends Disposable implements IMcpSandboxService
}
if (await this.isEnabled(serverDef, remoteAuthority)) {
this._logService.trace(`McpSandboxService: Launching with config target ${configTarget}`);
const launchDetails = await this._resolveSandboxLaunchDetails(configTarget, remoteAuthority, serverDef.sandbox, launch.cwd);
const launchDetails = await this._resolveSandboxLaunchDetails(configTarget, remoteAuthority, launch.sandbox, launch.cwd);
const sandboxArgs = this._getSandboxCommandArgs(launch.command, launch.args, launchDetails.sandboxConfigPath);
const sandboxEnv = this._getSandboxEnvVariables(launchDetails.tempDir, remoteAuthority);
if (launchDetails.srtPath) {
@@ -160,7 +159,7 @@ export class McpSandboxService extends Disposable implements IMcpSandboxService
let didChange = false;
await this._mcpResourceScannerService.updateSandboxConfig(data => {
const existingSandbox = data.sandbox ?? serverDef.sandbox;
const existingSandbox = data.sandbox;
const suggestedAllowedDomains = suggestedSandboxConfig?.network?.allowedDomains ?? [];
const suggestedAllowWrite = suggestedSandboxConfig?.filesystem?.allowWrite ?? [];
@@ -178,41 +177,24 @@ export class McpSandboxService extends Disposable implements IMcpSandboxService
}
}
didChange = currentAllowedDomains.size !== (existingSandbox?.network?.allowedDomains?.length ?? 0)
|| currentAllowWrite.size !== (existingSandbox?.filesystem?.allowWrite?.length ?? 0);
if (!didChange) {
if (suggestedAllowedDomains.length === 0 && suggestedAllowWrite.length === 0) {
return data;
}
const nextSandboxConfig: IMcpSandboxConfiguration = {
...existingSandbox,
};
if (currentAllowedDomains.size > 0 || existingSandbox?.network?.deniedDomains?.length) {
didChange = true;
const nextSandboxConfig: IMcpSandboxConfiguration = {};
if (currentAllowedDomains.size > 0) {
nextSandboxConfig.network = {
...existingSandbox?.network,
allowedDomains: [...currentAllowedDomains],
allowedDomains: [...currentAllowedDomains]
};
}
if (currentAllowWrite.size > 0 || existingSandbox?.filesystem?.denyRead?.length || existingSandbox?.filesystem?.denyWrite?.length) {
if (currentAllowWrite.size > 0) {
nextSandboxConfig.filesystem = {
...existingSandbox?.filesystem,
allowWrite: [...currentAllowWrite],
};
}
//always remove sandbox at server level when writing back, it should only exist at the top level. This is to sanitize any old or malformed configs that may have sandbox defined at the server level.
if (data.servers) {
for (const serverName in data.servers) {
const serverConfig = data.servers[serverName];
if (serverConfig.type === McpServerType.LOCAL) {
delete (serverConfig as Mutable<IMcpStdioServerConfiguration>).sandbox;
}
}
}
return {
...data,
sandbox: nextSandboxConfig,

View File

@@ -138,8 +138,6 @@ export interface McpServerDefinition {
readonly staticMetadata?: McpServerStaticMetadata;
/** Indicates if the sandbox is enabled for this server. */
readonly sandboxEnabled?: boolean;
/** Sandbox configuration to apply for this server. */
readonly sandbox?: IMcpSandboxConfiguration;
readonly presentation?: {
@@ -173,7 +171,6 @@ export namespace McpServerDefinition {
readonly variableReplacement?: McpServerDefinitionVariableReplacement.Serialized;
readonly staticMetadata?: McpServerStaticMetadata;
readonly sandboxEnabled?: boolean;
readonly sandbox?: IMcpSandboxConfiguration;
}
export function toSerialized(def: McpServerDefinition): McpServerDefinition.Serialized {
@@ -188,7 +185,6 @@ export namespace McpServerDefinition {
staticMetadata: def.staticMetadata,
launch: McpServerLaunch.fromSerialized(def.launch),
sandboxEnabled: def.sandboxEnabled,
sandbox: def.sandboxEnabled ? def.sandbox : undefined,
variableReplacement: def.variableReplacement ? McpServerDefinitionVariableReplacement.fromSerialized(def.variableReplacement) : undefined,
};
}
@@ -202,8 +198,8 @@ export namespace McpServerDefinition {
&& objectsEqual(a.presentation, b.presentation)
&& objectsEqual(a.variableReplacement, b.variableReplacement)
&& objectsEqual(a.devMode, b.devMode)
&& a.sandboxEnabled === b.sandboxEnabled
&& objectsEqual(a.sandbox, b.sandbox);
&& a.sandboxEnabled === b.sandboxEnabled;
}
}
@@ -519,6 +515,7 @@ export interface McpServerTransportStdio {
readonly args: readonly string[];
readonly env: Record<string, string | number | null>;
readonly envFile: string | undefined;
readonly sandbox: IMcpSandboxConfiguration | undefined;
}
export interface McpServerTransportHTTPAuthentication {
@@ -551,7 +548,7 @@ export type McpServerLaunch =
export namespace McpServerLaunch {
export type Serialized =
| { type: McpServerTransportType.HTTP; uri: UriComponents; headers: [string, string][]; authentication?: McpServerTransportHTTPAuthentication }
| { type: McpServerTransportType.Stdio; cwd: string | undefined; command: string; args: readonly string[]; env: Record<string, string | number | null>; envFile: string | undefined };
| { type: McpServerTransportType.Stdio; cwd: string | undefined; command: string; args: readonly string[]; env: Record<string, string | number | null>; envFile: string | undefined; sandbox: IMcpSandboxConfiguration | undefined };
export function toSerialized(launch: McpServerLaunch): McpServerLaunch.Serialized {
return launch;
@@ -569,6 +566,7 @@ export namespace McpServerLaunch {
args: launch.args,
env: launch.env,
envFile: launch.envFile,
sandbox: launch.sandbox
};
}
}

View File

@@ -22,7 +22,8 @@ const createStdioLaunch = (): McpServerTransportStdio => ({
command: 'cmd',
args: [],
env: {},
envFile: undefined
envFile: undefined,
sandbox: undefined
});
suite('MCP Icons', () => {

View File

@@ -239,6 +239,7 @@ suite('Workbench - MCP - Registry', () => {
env: {},
envFile: undefined,
cwd: '/test',
sandbox: undefined
}
};
});
@@ -301,6 +302,7 @@ suite('Workbench - MCP - Registry', () => {
},
envFile: undefined,
cwd: '/test',
sandbox: undefined
},
variableReplacement: {
section: 'mcp',
@@ -402,6 +404,7 @@ suite('Workbench - MCP - Registry', () => {
env: {},
envFile: undefined,
cwd: '/test',
sandbox: undefined
},
};
@@ -726,6 +729,7 @@ suite('Workbench - MCP - Registry', () => {
env: {},
envFile: undefined,
cwd: '/test',
sandbox: undefined
}
};
}

View File

@@ -171,7 +171,7 @@ export class TestMcpRegistry implements IMcpRegistry {
serverDefinitions: observableValue(this, [{
id: 'test-server',
label: 'Test Server',
launch: { type: McpServerTransportType.Stdio, command: 'echo', args: ['Hello MCP'], env: {}, envFile: undefined, cwd: undefined },
launch: { type: McpServerTransportType.Stdio, command: 'echo', args: ['Hello MCP'], env: {}, envFile: undefined, cwd: undefined, sandbox: undefined },
cacheNonce: 'a',
} satisfies McpServerDefinition]),
trustBehavior: McpServerTrust.Kind.Trusted,

View File

@@ -108,7 +108,8 @@ suite('Workbench - MCP - ServerConnection', () => {
args: [],
env: {},
envFile: undefined,
cwd: '/test'
cwd: '/test',
sandbox: undefined
}
};
});

View File

@@ -40,7 +40,8 @@ suite('MCP Types', () => {
command: 'test-command',
args: [],
env: {},
envFile: undefined
envFile: undefined,
sandbox: undefined
},
...overrides
});
@@ -89,7 +90,8 @@ suite('MCP Types', () => {
command: 'command1',
args: [],
env: {},
envFile: undefined
envFile: undefined,
sandbox: undefined
}
});
const def2 = createBasicDefinition({
@@ -99,7 +101,8 @@ suite('MCP Types', () => {
command: 'command2',
args: [],
env: {},
envFile: undefined
envFile: undefined,
sandbox: undefined
}
});
assert.strictEqual(McpServerDefinition.equals(def1, def2), false);