diff --git a/.github/skills/policy-and-managed-settings/github-managed-settings.md b/.github/skills/policy-and-managed-settings/github-managed-settings.md index 55f732f83434f5..625ad588e3ffd6 100644 --- a/.github/skills/policy-and-managed-settings/github-managed-settings.md +++ b/.github/skills/policy-and-managed-settings/github-managed-settings.md @@ -104,7 +104,7 @@ the schema's nested > (`managedSettings.ts` `normalizeExtraKnownMarketplaces`; `IExtraKnownMarketplaceEntry` > in `base/common/managedSettings.ts` only types `github`/`git`). > -> Note every **structured** key — `enabledPlugins`, `extraKnownMarketplaces`, +> Note every purely **structured** key — `enabledPlugins`, `extraKnownMarketplaces`, > `strictKnownMarketplaces` — is declared on its policy as **`{ type: 'string' }`**: the > object/array value is carried as a JSON string in the bag and parsed back on read (see > [Structured settings](#structured-objectarray-settings)). The *setting's* own `type` is @@ -114,7 +114,10 @@ the schema's nested > `'string' | 'number' | 'boolean'`, so omitting it or declaring `'object'` / `'array'` is a > *compile* error; declaring `'number'` / `'boolean'` compiles but then fails projection > validation at *runtime* (the JSON-string bag value flunks `typeof value === type`), so the key -> is dropped and silently never applies. +> is dropped and silently never applies. A hybrid schema field that accepts both a scalar and a +> structured value declares the transported scalar union instead, for example +> `{ type: ['boolean', 'string'] }`; its normalizer preserves booleans and carries the structured +> form as canonical JSON. Note the schema's `x-composition` describes the **server/runtime** layering across enterprise/org/user. Inside VS Code the bag has already been collapsed to a single diff --git a/package-lock.json b/package-lock.json index d622b46bd0dcdf..77f956ed3c64c7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32,7 +32,7 @@ "@vscode/iconv-lite-umd": "0.7.1", "@vscode/native-watchdog": "^1.4.6", "@vscode/os-proxy-resolver": "^0.4.0", - "@vscode/policy-watcher": "^1.4.0", + "@vscode/policy-watcher": "^1.5.0", "@vscode/proxy-agent": "^0.45.0", "@vscode/ripgrep-universal": "^1.18.0", "@vscode/sandbox-runtime": "0.0.1", @@ -4944,9 +4944,9 @@ ] }, "node_modules/@vscode/policy-watcher": { - "version": "1.4.0", - "resolved": "https://registry.npmjs.org/@vscode/policy-watcher/-/policy-watcher-1.4.0.tgz", - "integrity": "sha512-QKTLV/UtV0HH5AJELfN5D3Jcxj2hB9CYT9GtG334I8bU5TdusaSGYSpDjumZBGWO2YxwVYpBNytA7mq/I3ZtzA==", + "version": "1.5.0", + "resolved": "https://registry.npmjs.org/@vscode/policy-watcher/-/policy-watcher-1.5.0.tgz", + "integrity": "sha512-mn81Gbr9v4VF52KhTj3Cr5Bl+ZncCzq3rFwSP1FMhQpCauQCatKa6MjL7tutGxLDDOIT13Zk1MrLfrn6cxp9Vw==", "hasInstallScript": true, "license": "MIT", "dependencies": { diff --git a/package.json b/package.json index d9260d55953d3b..2572f2cd0b79d8 100644 --- a/package.json +++ b/package.json @@ -122,7 +122,7 @@ "@vscode/iconv-lite-umd": "0.7.1", "@vscode/native-watchdog": "^1.4.6", "@vscode/os-proxy-resolver": "^0.4.0", - "@vscode/policy-watcher": "^1.4.0", + "@vscode/policy-watcher": "^1.5.0", "@vscode/proxy-agent": "^0.45.0", "@vscode/ripgrep-universal": "^1.18.0", "@vscode/sandbox-runtime": "0.0.1", @@ -314,7 +314,7 @@ "@vscode/native-watchdog@1.4.6": true, "@vscode/ripgrep@1.17.1": true, "@vscode/deviceid@0.1.5": true, - "@vscode/policy-watcher@1.4.0": true, + "@vscode/policy-watcher@1.5.0": true, "@vscode/spdlog@0.15.8": true, "@vscode/sqlite3@5.1.12-vscode": true, "@vscode/windows-registry@1.2.0": true, diff --git a/src/vs/base/common/policy.ts b/src/vs/base/common/policy.ts index ffe87224d421bd..022e18dc9fc31d 100644 --- a/src/vs/base/common/policy.ts +++ b/src/vs/base/common/policy.ts @@ -21,8 +21,10 @@ export type PolicyValue = string | number | boolean; export type ManagedSettingValue = PolicyValue; export type ManagedSettingsData = Readonly>; +export type ManagedSettingType = 'string' | 'number' | 'boolean'; + export interface IManagedSettingPolicyDefinition { - readonly type: 'string' | 'number' | 'boolean'; + readonly type: ManagedSettingType | readonly [ManagedSettingType, ...ManagedSettingType[]]; } export type IManagedSettingsPolicyDefinitions = Readonly>; diff --git a/src/vs/platform/configuration/test/common/policyConfiguration.test.ts b/src/vs/platform/configuration/test/common/policyConfiguration.test.ts index 39947f5eeae3c3..4d8b0f77aa7c8d 100644 --- a/src/vs/platform/configuration/test/common/policyConfiguration.test.ts +++ b/src/vs/platform/configuration/test/common/policyConfiguration.test.ts @@ -252,6 +252,14 @@ suite('PolicyConfiguration', () => { assert.deepStrictEqual(acutal.getValue('policy.booleanSetting'), true); }); + test('initialize: parses a structured internal value for a boolean policy-backed setting', async () => { + await fileService.writeFile(policyFile, VSBuffer.fromString(JSON.stringify({ 'PolicyBooleanSetting': '["mcp"]' }))); + + await testObject.initialize(); + + assert.deepStrictEqual(testObject.configurationModel.getValue('policy.booleanSetting'), ['mcp']); + }); + test('initialize: with object type policy ignores policy if value is not valid', async () => { await fileService.writeFile(policyFile, VSBuffer.fromString(JSON.stringify({ 'PolicyObjectSetting': '{"a": "b", "hello": }' }))); diff --git a/src/vs/platform/policy/common/copilotManagedSettings.ts b/src/vs/platform/policy/common/copilotManagedSettings.ts index f928defff092f0..785f37fc2fd413 100644 --- a/src/vs/platform/policy/common/copilotManagedSettings.ts +++ b/src/vs/platform/policy/common/copilotManagedSettings.ts @@ -51,6 +51,14 @@ export const COPILOT_DENIED_MCP_SERVERS_KEY = 'deniedMcpServers'; /** Managed-settings key that blocks standalone user/workspace customizations. */ export const COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY = 'strictPluginOnlyCustomization'; +export const STRICT_PLUGIN_ONLY_CUSTOMIZATION_SELECTORS = ['skills', 'agents', 'hooks', 'mcp'] as const; + +export type StrictPluginOnlyCustomizationSelector = typeof STRICT_PLUGIN_ONLY_CUSTOMIZATION_SELECTORS[number]; + +export function isStrictPluginOnlyCustomizationSelectorArray(value: unknown): value is readonly StrictPluginOnlyCustomizationSelector[] { + return Array.isArray(value) && value.every(selector => isString(selector) && STRICT_PLUGIN_ONLY_CUSTOMIZATION_SELECTORS.includes(selector as StrictPluginOnlyCustomizationSelector)); +} + /** Managed-settings key that makes the enterprise MCP allowlist authoritative. */ export const COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_KEY = 'allowManagedMcpServersOnly'; @@ -163,6 +171,22 @@ export type IForceRemoteSettingsRefreshResolution = | { readonly effective: true; readonly source: ManagedSettingsChannel } | { readonly effective: false }; +export function strictPluginOnlyCustomizationValue(policyData: IPolicyData): ManagedSettingValue | undefined { + const value = policyData.managedSettings?.[COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]; + if (typeof value !== 'string') { + return value; + } + try { + const parsed: unknown = JSON.parse(value); + if (typeof parsed === 'boolean') { + return parsed; + } + return isStrictPluginOnlyCustomizationSelectorArray(parsed) ? value : true; + } catch { + return true; + } +} + /** * Resolve the fail-closed startup refresh control across every delivery channel, reusing * {@link pickManagedSettings} precedence rather than re-implementing it. A non-boolean value is @@ -337,10 +361,11 @@ export function projectManagedSettings(values: ManagedSettingsData, definitions: if (value === undefined) { continue; } - if (typeof value === definitions[key].type) { + const expectedTypes = definitions[key].type; + if (Array.isArray(expectedTypes) ? expectedTypes.includes(typeof value as 'string' | 'number' | 'boolean') : typeof value === expectedTypes) { projected[key] = value; } else { - onWarn?.(`Ignoring managed setting "${key}": expected ${definitions[key].type}, got ${typeof value}`); + onWarn?.(`Ignoring managed setting "${key}": expected ${Array.isArray(expectedTypes) ? expectedTypes.join(' or ') : expectedTypes}, got ${typeof value}`); } } return projected; @@ -639,11 +664,28 @@ export function normalizeManagedSettings(parsed: Record, onWarn // `__proto__` key, matching a destructuring rest. Structured keys may be nested (e.g. // `telemetry.resourceAttributes`), so removal clones only the touched path. let scalarRest: Record = { ...parsed }; + scalarRest = withNestedManagedKeyDeleted(scalarRest, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY); for (const setting of STRUCTURED_MANAGED_SETTINGS) { scalarRest = withNestedManagedKeyDeleted(scalarRest, setting.key); } const result: Record = { ...flattenManagedSettings(scalarRest) }; + const strictPluginOnlyCustomization = parsed[COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]; + if (strictPluginOnlyCustomization !== undefined) { + if (typeof strictPluginOnlyCustomization === 'boolean') { + result[COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY] = strictPluginOnlyCustomization; + } else if (Array.isArray(strictPluginOnlyCustomization)) { + if (isStrictPluginOnlyCustomizationSelectorArray(strictPluginOnlyCustomization)) { + result[COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY] = JSON.stringify(strictPluginOnlyCustomization); + } else { + onWarn?.(`Managed setting "${COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY}" contains an invalid selector and will fail closed`); + result[COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY] = true; + } + } else { + onWarn?.(`Managed setting "${COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY}" has an invalid value and will fail closed`); + result[COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY] = true; + } + } for (const setting of STRUCTURED_MANAGED_SETTINGS) { const encoded = setting.encode(readNestedManagedKey(parsed, setting.key), onWarn); diff --git a/src/vs/platform/policy/node/nativeManagedSettingsService.ts b/src/vs/platform/policy/node/nativeManagedSettingsService.ts index 305cc0bd6288f7..0b5156cab7a2e9 100644 --- a/src/vs/platform/policy/node/nativeManagedSettingsService.ts +++ b/src/vs/platform/policy/node/nativeManagedSettingsService.ts @@ -8,7 +8,7 @@ import { IStringDictionary } from '../../../base/common/collections.js'; import { Emitter } from '../../../base/common/event.js'; import { Disposable, MutableDisposable } from '../../../base/common/lifecycle.js'; import { equals } from '../../../base/common/objects.js'; -import { IManagedSettingsPolicyDefinitions, ManagedSettingsData } from '../../../base/common/policy.js'; +import { IManagedSettingPolicyDefinition, IManagedSettingsPolicyDefinitions, ManagedSettingsData } from '../../../base/common/policy.js'; import { ILogService } from '../../log/common/log.js'; import { collectManagedSettingsDefinitions, INativeManagedSettingsService, MANAGED_SETTINGS_CONTROL_DEFINITIONS } from '../common/copilotManagedSettings.js'; import { PolicyDefinition, PolicyValue } from '../common/policy.js'; @@ -20,7 +20,7 @@ export interface INativePolicyWatcherOptions { export type NativePolicyWatcherFactory = ( productName: string, - policies: Record, + policies: Record, onDidChange: (update: Record) => void, options?: INativePolicyWatcherOptions, ) => Watcher; @@ -35,6 +35,7 @@ export class NativeManagedSettingsService extends Disposable implements INativeM private watchedSettings: IManagedSettingsPolicyDefinitions = MANAGED_SETTINGS_CONTROL_DEFINITIONS; private initializationPromise: Promise | undefined; private initializationVersion = 0; + private activeWatcherVersion = 0; private readonly _onDidChangeManagedSettings = this._register(new Emitter()); readonly onDidChangeManagedSettings = this._onDidChangeManagedSettings.event; @@ -67,9 +68,15 @@ export class NativeManagedSettingsService extends Disposable implements INativeM return this.initialize(); } + const previousWatchedSettings = this.watchedSettings; this.watchedSettings = managedSettings; + try { + await this.ensureWatcher(true); + } catch (error) { + this.watchedSettings = previousWatchedSettings; + throw error; + } const changed = this.pruneManagedSettingsValues(); - await this.ensureWatcher(true); if (changed) { this._onDidChangeManagedSettings.fire(this.managedSettings); } @@ -87,7 +94,7 @@ export class NativeManagedSettingsService extends Disposable implements INativeM private async updateWatcherAndTrack(version: number): Promise { try { - await this.updateWatcher(); + await this.updateWatcher(version); } catch (error) { if (this.initializationVersion === version) { this.initializationPromise = undefined; @@ -107,7 +114,7 @@ export class NativeManagedSettingsService extends Disposable implements INativeM return changed; } - private async updateWatcher(): Promise { + private async updateWatcher(version: number): Promise { const managedSettingDefinitions = this.getManagedSettingDefinitions(); this.logService.trace(`NativeManagedSettingsService#updateWatcher - Found ${Object.keys(managedSettingDefinitions).length} managed-settings definitions`); if (Object.keys(managedSettingDefinitions).length === 0) { @@ -124,10 +131,38 @@ export class NativeManagedSettingsService extends Disposable implements INativeM await this.throttler.queue(() => new Promise((c, e) => { try { this.logService.trace(`Creating native managed-settings watcher for productName ${this.productName}`); - this.watcher.value = createWatcher(this.productName, managedSettingDefinitions, update => { - this._onDidManagedSettingsChange(update as Record); + let ready = false; + const pendingUpdates: Array> = []; + const onDidChange = (update: Record) => { + if (!ready) { + pendingUpdates.push(update); + } else if (this.activeWatcherVersion === version) { + this._onDidManagedSettingsChange(update); + } c(); - }, this.watcherOptions); + }; + let watcher; + try { + watcher = createWatcher(this.productName, managedSettingDefinitions, onDidChange, this.watcherOptions); + } catch (error) { + const hasScalarUnion = Object.values(managedSettingDefinitions).some(definition => Array.isArray(definition.type)); + if (!hasScalarUnion || !(error instanceof TypeError) || error.message !== 'Expected policy type to be string') { + throw error; + } + this.logService.warn('Native managed-settings watcher does not support scalar unions; using the first declared type until the native module is updated'); + const legacyDefinitions: Record = {}; + for (const key in managedSettingDefinitions) { + const type = managedSettingDefinitions[key].type; + legacyDefinitions[key] = { type: Array.isArray(type) ? type[0] : type }; + } + watcher = createWatcher(this.productName, legacyDefinitions, onDidChange, this.watcherOptions); + } + this.activeWatcherVersion = version; + this.watcher.value = watcher; + ready = true; + for (const update of pendingUpdates) { + this._onDidManagedSettingsChange(update); + } } catch (err) { this.logService.error(`NativeManagedSettingsService#updateWatcher - Error creating watcher:`, err); e(err); @@ -142,8 +177,8 @@ export class NativeManagedSettingsService extends Disposable implements INativeM * watcher our internal state: it decouples the two shapes so a future field on * `IManagedSettingPolicyDefinition` cannot silently leak across the native boundary. */ - private getManagedSettingDefinitions(): Record { - const definitions: Record = {}; + private getManagedSettingDefinitions(): Record { + const definitions: Record = {}; for (const key in this.watchedSettings) { definitions[key] = { type: this.watchedSettings[key].type }; } diff --git a/src/vs/platform/policy/test/common/copilotManagedSettings.test.ts b/src/vs/platform/policy/test/common/copilotManagedSettings.test.ts index beb7ee54419059..1cf17d4840ff33 100644 --- a/src/vs/platform/policy/test/common/copilotManagedSettings.test.ts +++ b/src/vs/platform/policy/test/common/copilotManagedSettings.test.ts @@ -7,7 +7,7 @@ import assert from 'assert'; import { IStringDictionary } from '../../../../base/common/collections.js'; import { IPolicyData } from '../../../../base/common/defaultAccount.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; -import { collectManagedSettingsDefinitions, COPILOT_FORCE_REMOTE_SETTINGS_REFRESH_KEY, COPILOT_MODEL_KEY, COPILOT_SANDBOX_ENABLED_KEY, COPILOT_TOP_LEVEL_MODEL_KEY, hasManagedSettingsDefinitions, managedModelValue, managedSettingsDisabledValue, managedSettingValue, projectManagedSettings, pickManagedSettings, resolveForceRemoteSettingsRefresh } from '../../common/copilotManagedSettings.js'; +import { collectManagedSettingsDefinitions, COPILOT_FORCE_REMOTE_SETTINGS_REFRESH_KEY, COPILOT_MODEL_KEY, COPILOT_SANDBOX_ENABLED_KEY, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY, COPILOT_TOP_LEVEL_MODEL_KEY, hasManagedSettingsDefinitions, managedModelValue, managedSettingsDisabledValue, managedSettingValue, projectManagedSettings, pickManagedSettings, resolveForceRemoteSettingsRefresh, strictPluginOnlyCustomizationValue } from '../../common/copilotManagedSettings.js'; import { PolicyDefinition } from '../../common/policy.js'; suite('Copilot managed settings projection', () => { @@ -167,6 +167,45 @@ suite('Copilot managed settings projection', () => { ); }); + test('projectManagedSettings accepts any declared scalar type', () => { + assert.deepStrictEqual({ + boolean: projectManagedSettings( + { [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: true }, + { [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: { type: ['boolean', 'string'] } }, + ), + string: projectManagedSettings( + { [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: '["mcp"]' }, + { [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: { type: ['boolean', 'string'] } }, + ), + }, { + boolean: { [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: true }, + string: { [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: '["mcp"]' }, + }); + }); + + test('strictPluginOnlyCustomizationValue validates structured policy values', () => { + const read = (value: string | boolean | undefined) => strictPluginOnlyCustomizationValue({ + managedSettings: value === undefined ? {} : { [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: value }, + } as IPolicyData); + assert.deepStrictEqual([ + read(undefined), + read(false), + read(true), + read('false'), + read('["skills","mcp"]'), + read('["skills","unknown"]'), + read('{'), + ], [ + undefined, + false, + true, + false, + '["skills","mcp"]', + true, + true, + ]); + }); + test('projectManagedSettings warns once per type mismatch', () => { const warnings: string[] = []; projectManagedSettings( diff --git a/src/vs/platform/policy/test/common/fileManagedSettingsService.test.ts b/src/vs/platform/policy/test/common/fileManagedSettingsService.test.ts index 66e80eeb02913b..f7457becf21582 100644 --- a/src/vs/platform/policy/test/common/fileManagedSettingsService.test.ts +++ b/src/vs/platform/policy/test/common/fileManagedSettingsService.test.ts @@ -56,10 +56,18 @@ suite('normalizeManagedSettings', () => { }); }); - test('drops a non-boolean strictPluginOnlyCustomization value', () => { - assert.deepStrictEqual(normalizeManagedSettings({ - [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: ['skills', 'unknown'], - }), {}); + test('normalizes selective and malformed strictPluginOnlyCustomization values', () => { + const warnings: string[] = []; + assert.deepStrictEqual([ + normalizeManagedSettings({ [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: ['skills'] }, warning => warnings.push(warning)), + normalizeManagedSettings({ [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: ['skills', 'unknown'] }, warning => warnings.push(warning)), + normalizeManagedSettings({ [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: null }, warning => warnings.push(warning)), + ], [ + { [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: '["skills"]' }, + { [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: true }, + { [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: true }, + ]); + assert.strictEqual(warnings.length, 2); }); test('normalizes extraKnownMarketplaces from schema format to config dict', () => { diff --git a/src/vs/platform/policy/test/node/nativeManagedSettingsService.test.ts b/src/vs/platform/policy/test/node/nativeManagedSettingsService.test.ts index 9e88d1c884768a..b12711641e6305 100644 --- a/src/vs/platform/policy/test/node/nativeManagedSettingsService.test.ts +++ b/src/vs/platform/policy/test/node/nativeManagedSettingsService.test.ts @@ -6,7 +6,7 @@ import assert from 'assert'; import { Emitter, Event } from '../../../../base/common/event.js'; import { Disposable } from '../../../../base/common/lifecycle.js'; -import { ManagedSettingsData } from '../../../../base/common/policy.js'; +import { IManagedSettingsPolicyDefinitions, ManagedSettingsData } from '../../../../base/common/policy.js'; import { IChannel } from '../../../../base/parts/ipc/common/ipc.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; import { NullLogService } from '../../../log/common/log.js'; @@ -29,6 +29,7 @@ suite('NativeManagedSettingsService', () => { [COPILOT_SANDBOX_ENABLED_KEY]: { type: 'boolean' }, [COPILOT_SANDBOX_ALLOW_BYPASS_KEY]: { type: 'boolean' }, }); + onDidChange = callback; callback({}); return Disposable.None; @@ -51,8 +52,117 @@ suite('NativeManagedSettingsService', () => { assert.deepStrictEqual(service.managedSettings, { [COPILOT_DISABLE_BYPASS_PERMISSIONS_MODE_KEY]: 'enable' }); }); + test('passes scalar union definitions to the native watcher', async () => { + let watchedSettings: IManagedSettingsPolicyDefinitions = {}; + const watcherFactory: NativePolicyWatcherFactory = (_productName, policies, callback) => { + watchedSettings = policies; + callback({}); + return Disposable.None; + }; + + const service = disposables.add(new NativeManagedSettingsService(new NullLogService(), 'com.github.copilot', undefined, watcherFactory)); + await service.updatePolicyDefinitions({ + [policyName]: { + type: 'boolean', + managedSettings: { + strictPluginOnlyCustomization: { type: ['boolean', 'string'] }, + }, + }, + }); + + assert.deepStrictEqual(watchedSettings.strictPluginOnlyCustomization, { type: ['boolean', 'string'] }); + }); + + test('falls back to the first scalar type for a legacy native watcher', async () => { + const watchedSettings: IManagedSettingsPolicyDefinitions[] = []; + const watcherFactory: NativePolicyWatcherFactory = (_productName, policies, callback) => { + watchedSettings.push(policies); + if (Object.values(policies).some(definition => Array.isArray(definition.type))) { + throw new TypeError('Expected policy type to be string'); + } + callback({ strictPluginOnlyCustomization: true }); + return Disposable.None; + }; + + const service = disposables.add(new NativeManagedSettingsService(new NullLogService(), 'com.github.copilot', undefined, watcherFactory)); + await service.updatePolicyDefinitions({ + [policyName]: { + type: 'boolean', + managedSettings: { + strictPluginOnlyCustomization: { type: ['boolean', 'string'] }, + }, + }, + }); + + assert.deepStrictEqual({ + definitions: watchedSettings.map(settings => settings.strictPluginOnlyCustomization), + managedSettings: service.managedSettings, + }, { + definitions: [{ type: ['boolean', 'string'] }, { type: 'boolean' }], + managedSettings: { strictPluginOnlyCustomization: true }, + }); + }); + + test('ignores callbacks from a replaced watcher', async () => { + const callbacks: Array<(update: Record) => void> = []; + const watcherFactory: NativePolicyWatcherFactory = (_productName, _policies, callback) => { + callbacks.push(callback); + callback({}); + return Disposable.None; + }; + + const service = disposables.add(new NativeManagedSettingsService(new NullLogService(), 'com.github.copilot', undefined, watcherFactory)); + await service.updatePolicyDefinitions({ + [policyName]: { + type: 'boolean', + managedSettings: { first: { type: 'boolean' } }, + }, + }); + await service.updatePolicyDefinitions({ + [policyName]: { + type: 'boolean', + managedSettings: { second: { type: 'boolean' } }, + }, + }); + + callbacks[0]({ first: true }); + callbacks[1]({ second: true }); + assert.deepStrictEqual(service.managedSettings, { second: true }); + }); + + test('keeps callbacks from the active watcher when replacement fails', async () => { + let callback: ((update: Record) => void) | undefined; + let failCreation = false; + const watcherFactory: NativePolicyWatcherFactory = (_productName, _policies, onDidChange) => { + if (failCreation) { + throw new Error('failed to replace watcher'); + } + callback = onDidChange; + onDidChange({}); + return Disposable.None; + }; + + const service = disposables.add(new NativeManagedSettingsService(new NullLogService(), 'com.github.copilot', undefined, watcherFactory)); + await service.updatePolicyDefinitions({ + [policyName]: { + type: 'boolean', + managedSettings: { first: { type: 'boolean' } }, + }, + }); + failCreation = true; + await assert.rejects(service.updatePolicyDefinitions({ + [policyName]: { + type: 'boolean', + managedSettings: { second: { type: 'boolean' } }, + }, + }), /failed to replace watcher/); + + callback?.({ first: true }); + assert.deepStrictEqual(service.managedSettings, { first: true }); + }); + test('watches transport controls without a managed-settings policy definition', async () => { - let watchedSettings: Record = {}; + let watchedSettings: IManagedSettingsPolicyDefinitions = {}; const watcherFactory: NativePolicyWatcherFactory = (_productName, policies, callback) => { watchedSettings = policies; callback({ [COPILOT_FORCE_REMOTE_SETTINGS_REFRESH_KEY]: true }); diff --git a/src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts b/src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts index dbf8497019703f..f6d18ca3ab4b10 100644 --- a/src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts +++ b/src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts @@ -38,7 +38,7 @@ import { DEFAULT_LOCAL_TRANSCRIPTION_MODEL } from '../../../../platform/localTra import { McpAccessValue, McpAutoStartValue, mcpAccessConfig, mcpAllowedServersConfig, mcpAppsEnabledConfig, mcpAutoStartConfig, mcpDeniedServersConfig, mcpGalleryServiceEnablementConfig, mcpGalleryServiceUrlConfig } from '../../../../platform/mcp/common/mcpManagement.js'; import { AgentNetworkFilterService, IAgentNetworkFilterService } from '../../../../platform/networkFilter/common/networkFilterService.js'; import { AgentNetworkDomainSettingId } from '../../../../platform/networkFilter/common/settings.js'; -import { COPILOT_ALLOWED_MCP_SERVERS_KEY, COPILOT_ALLOW_MANAGED_HOOKS_ONLY_CONFIG, COPILOT_ALLOW_MANAGED_HOOKS_ONLY_KEY, COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_CONFIG, COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_KEY, COPILOT_DENIED_MCP_SERVERS_KEY, COPILOT_DISABLE_BYPASS_PERMISSIONS_MODE_KEY, COPILOT_ENABLED_PLUGINS_KEY, COPILOT_EXTRA_MARKETPLACES_KEY, COPILOT_MODEL_KEY, COPILOT_STRICT_MARKETPLACES_KEY, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY, COPILOT_TOP_LEVEL_MODEL_KEY, managedModelValue, managedSettingsDisabledValue, managedSettingValue } from '../../../../platform/policy/common/copilotManagedSettings.js'; +import { COPILOT_ALLOWED_MCP_SERVERS_KEY, COPILOT_ALLOW_MANAGED_HOOKS_ONLY_CONFIG, COPILOT_ALLOW_MANAGED_HOOKS_ONLY_KEY, COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_CONFIG, COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_KEY, COPILOT_DENIED_MCP_SERVERS_KEY, COPILOT_DISABLE_BYPASS_PERMISSIONS_MODE_KEY, COPILOT_ENABLED_PLUGINS_KEY, COPILOT_EXTRA_MARKETPLACES_KEY, COPILOT_MODEL_KEY, COPILOT_STRICT_MARKETPLACES_KEY, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY, COPILOT_TOP_LEVEL_MODEL_KEY, managedModelValue, managedSettingsDisabledValue, managedSettingValue, strictPluginOnlyCustomizationValue } from '../../../../platform/policy/common/copilotManagedSettings.js'; import product from '../../../../platform/product/common/product.js'; import { Registry } from '../../../../platform/registry/common/platform.js'; import { AgentSandboxEnabledValue, AgentSandboxSettingId } from '../../../../platform/sandbox/common/settings.js'; @@ -1467,9 +1467,9 @@ configurationRegistry.registerConfiguration({ name: 'ChatStrictPluginOnlyCustomization', category: PolicyCategory.InteractiveSession, minimumVersion: '1.132', - value: managedSettingValue(COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY), + value: strictPluginOnlyCustomizationValue, managedSettings: { - [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: { type: 'boolean' }, + [COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY]: { type: ['boolean', 'string'] }, }, localization: { description: { diff --git a/src/vs/workbench/contrib/chat/common/customizationLockdown.ts b/src/vs/workbench/contrib/chat/common/customizationLockdown.ts index 7c258d27df1871..799bb328e88ab2 100644 --- a/src/vs/workbench/contrib/chat/common/customizationLockdown.ts +++ b/src/vs/workbench/contrib/chat/common/customizationLockdown.ts @@ -4,20 +4,37 @@ *--------------------------------------------------------------------------------------------*/ import { PromptsType } from './promptSyntax/promptTypes.js'; +import { isStrictPluginOnlyCustomizationSelectorArray, StrictPluginOnlyCustomizationSelector } from '../../../../platform/policy/common/copilotManagedSettings.js'; -export type StrictPluginOnlyCustomization = boolean | null | undefined; +export type StrictPluginOnlyCustomization = boolean | readonly unknown[] | null | undefined; export function isStrictPluginOnlyCustomizationEnabled(value: StrictPluginOnlyCustomization): boolean { return value === true; } +export function isStrictPluginOnlyCustomizationBlocked(value: StrictPluginOnlyCustomization, surface: StrictPluginOnlyCustomizationSelector | 'instructions'): boolean { + if (value === true) { + return true; + } + if (value === false || value === undefined) { + return false; + } + if (!isStrictPluginOnlyCustomizationSelectorArray(value)) { + return true; + } + return surface !== 'instructions' && value.includes(surface); +} + export function isPromptTypeBlocked(value: StrictPluginOnlyCustomization, type: PromptsType): boolean { switch (type) { case PromptsType.skill: + return isStrictPluginOnlyCustomizationBlocked(value, 'skills'); case PromptsType.agent: + return isStrictPluginOnlyCustomizationBlocked(value, 'agents'); case PromptsType.hook: + return isStrictPluginOnlyCustomizationBlocked(value, 'hooks'); case PromptsType.instructions: - return isStrictPluginOnlyCustomizationEnabled(value); + return isStrictPluginOnlyCustomizationBlocked(value, 'instructions'); default: return false; } diff --git a/src/vs/workbench/contrib/chat/common/plugins/AGENTS_PLUGINS.md b/src/vs/workbench/contrib/chat/common/plugins/AGENTS_PLUGINS.md index 399c2095ee2268..124e9209ab82eb 100644 --- a/src/vs/workbench/contrib/chat/common/plugins/AGENTS_PLUGINS.md +++ b/src/vs/workbench/contrib/chat/common/plugins/AGENTS_PLUGINS.md @@ -185,11 +185,32 @@ Each `PluginSourceKind` has a strategy that knows how to compute cache paths, pr The managed customization controls are complementary: - `strictKnownMarketplaces` restricts which marketplace sources may provide plugins. -- `strictPluginOnlyCustomization` blocks standalone user and workspace skills, agents, hooks, instructions, and MCP servers. Eligible plugin contributions remain available. +- `strictPluginOnlyCustomization: true` blocks standalone user and workspace skills, agents, hooks, instructions, and MCP servers. Eligible plugin contributions remain available. - `allowManagedMcpServersOnly` makes the managed MCP allowlist authoritative; lower-layer allow entries cannot broaden it and deny entries remain restrictive. - `allowManagedHooksOnly` permits plugin hooks only when managed `enabledPlugins` force-enables the plugin. User/workspace hooks and hooks from otherwise user-enabled plugins do not load. -`strictPluginOnlyCustomization` does not replace strict marketplace enforcement. Hardened deployments apply both controls when plugin source and standalone customization provenance must both be constrained. +`strictPluginOnlyCustomization` also accepts an array that blocks only the named standalone surfaces: + +```json +{ + "strictPluginOnlyCustomization": ["skills", "agents", "hooks", "mcp"] +} +``` + +| Selector | Standalone sources blocked | +|----------|----------------------------| +| `skills` | Skills | +| `agents` | Custom agents | +| `hooks` | Hook files and hooks embedded in standalone agents | +| `mcp` | MCP servers | + +An empty array blocks nothing. This is blocklist behavior, unlike allowlist controls such as `strictKnownMarketplaces`, where an empty array blocks every source. Arrays containing an unknown or malformed selector fail closed as a full lockdown rather than applying only their valid entries. + +Instructions remain blocked by boolean `true` only; a selective rules/instructions mapping is not currently defined. Therefore `["skills", "agents", "hooks", "mcp"]` is not equivalent to `true`. + +The controls compose independently. `strictPluginOnlyCustomization` does not replace `strictKnownMarketplaces`, and selecting `hooks` or `mcp` does not make plugin hooks or MCP allowlists enterprise-managed. Hardened deployments combine it with `strictKnownMarketplaces`, `allowManagedHooksOnly`, or `allowManagedMcpServersOnly` according to the provenance they need to constrain. + +Selective values require a client version that supports the array contract. The managed-settings service must continue sending boolean `true`, or return `client_update_required`, to older clients; administrators should not replace a native boolean policy with the JSON-array string until their fleet supports it. Future selectors likewise require a managed-schema minimum-client-version update because older clients intentionally treat unknown selectors as a full lockdown. ### Storage (ApplicationScope, MachineTarget) | Key | Description | diff --git a/src/vs/workbench/contrib/chat/test/common/customizationLockdown.test.ts b/src/vs/workbench/contrib/chat/test/common/customizationLockdown.test.ts index 8265647a2f4f46..a008d3fe922bad 100644 --- a/src/vs/workbench/contrib/chat/test/common/customizationLockdown.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/customizationLockdown.test.ts @@ -5,7 +5,7 @@ import assert from 'assert'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; -import { isPromptTypeBlocked, isStrictPluginOnlyCustomizationEnabled } from '../../common/customizationLockdown.js'; +import { isPromptTypeBlocked, isStrictPluginOnlyCustomizationBlocked, isStrictPluginOnlyCustomizationEnabled } from '../../common/customizationLockdown.js'; import { PromptsType } from '../../common/promptSyntax/promptTypes.js'; suite('Customization lockdown', () => { @@ -24,4 +24,36 @@ suite('Customization lockdown', () => { assert.strictEqual(isPromptTypeBlocked(true, PromptsType.instructions), true); assert.strictEqual(isPromptTypeBlocked(true, PromptsType.prompt), false); }); + + test('selective values block only named surfaces', () => { + const values = [undefined, false, [], ['skills'], ['agents'], ['hooks'], ['mcp'], ['skills', 'hooks'], true]; + assert.deepStrictEqual(values.map(value => ({ + skills: isStrictPluginOnlyCustomizationBlocked(value, 'skills'), + agents: isStrictPluginOnlyCustomizationBlocked(value, 'agents'), + hooks: isStrictPluginOnlyCustomizationBlocked(value, 'hooks'), + mcp: isStrictPluginOnlyCustomizationBlocked(value, 'mcp'), + instructions: isStrictPluginOnlyCustomizationBlocked(value, 'instructions'), + })), [ + { skills: false, agents: false, hooks: false, mcp: false, instructions: false }, + { skills: false, agents: false, hooks: false, mcp: false, instructions: false }, + { skills: false, agents: false, hooks: false, mcp: false, instructions: false }, + { skills: true, agents: false, hooks: false, mcp: false, instructions: false }, + { skills: false, agents: true, hooks: false, mcp: false, instructions: false }, + { skills: false, agents: false, hooks: true, mcp: false, instructions: false }, + { skills: false, agents: false, hooks: false, mcp: true, instructions: false }, + { skills: true, agents: false, hooks: true, mcp: false, instructions: false }, + { skills: true, agents: true, hooks: true, mcp: true, instructions: true }, + ]); + }); + + test('malformed values fail closed without partially applying selectors', () => { + const malformed = [null, ['skills', 'unknown'], ['skills', 1], 'skills' as never]; + assert.deepStrictEqual(malformed.map(value => ({ + skills: isStrictPluginOnlyCustomizationBlocked(value, 'skills'), + agents: isStrictPluginOnlyCustomizationBlocked(value, 'agents'), + hooks: isStrictPluginOnlyCustomizationBlocked(value, 'hooks'), + mcp: isStrictPluginOnlyCustomizationBlocked(value, 'mcp'), + instructions: isStrictPluginOnlyCustomizationBlocked(value, 'instructions'), + })), malformed.map(() => ({ skills: true, agents: true, hooks: true, mcp: true, instructions: true }))); + }); }); diff --git a/src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts b/src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts index bb8a065010bd02..86fb3945a467eb 100644 --- a/src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts +++ b/src/vs/workbench/contrib/chat/test/common/promptSyntax/service/promptsService.test.ts @@ -5011,6 +5011,29 @@ suite('PromptsService', () => { assert.strictEqual((await service.listPromptFiles(PromptsType.prompt, CancellationToken.None)).length, 1); }); + test('selective agent lockdown filters agents without affecting skills', async () => { + testConfigService.setUserConfiguration(PromptsConfig.USE_AGENT_SKILLS, true); + testConfigService.setUserConfiguration(PromptsConfig.SKILLS_LOCATION_KEY, {}); + testConfigService.setUserConfiguration(COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, ['agents']); + const rootFolderUri = URI.file('/selective-agent-lockdown'); + workspaceContextService.setWorkspace(testWorkspace(rootFolderUri)); + await mockFiles(fileService, [{ + path: '/selective-agent-lockdown/.github/agents/reviewer.agent.md', + contents: ['---', 'description: "Review code"', '---'], + }, { + path: '/selective-agent-lockdown/.github/skills/review/SKILL.md', + contents: ['---', 'name: "review"', 'description: "Review skill"', '---'], + }]); + + assert.deepStrictEqual({ + agents: await service.getCustomAgents(CancellationToken.None), + skills: (await service.listPromptFiles(PromptsType.skill, CancellationToken.None)).map(skill => skill.uri.path), + }, { + agents: [], + skills: ['/selective-agent-lockdown/.github/skills/review/SKILL.md'], + }); + }); + test('skill lockdown filters standalone skills before discovery and preserves plugin skills', async () => { testConfigService.setUserConfiguration(PromptsConfig.USE_AGENT_SKILLS, true); testConfigService.setUserConfiguration(COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, true); @@ -5121,6 +5144,31 @@ suite('PromptsService', () => { assert.deepStrictEqual(agents, []); }); + test('selective hook lockdown removes standalone agent hooks without removing the agent', async () => { + testConfigService.setUserConfiguration(PromptsConfig.USE_CHAT_HOOKS, true); + testConfigService.setUserConfiguration(COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, ['hooks']); + const rootFolderUri = URI.file('/selective-hook-lockdown'); + workspaceContextService.setWorkspace(testWorkspace(rootFolderUri)); + await mockFiles(fileService, [{ + path: '/selective-hook-lockdown/.github/agents/reviewer.agent.md', + contents: [ + '---', + 'description: "Review code"', + 'hooks:', + ' PreToolUse:', + ' - type: command', + ' command: "echo blocked"', + '---', + ], + }]); + + const agents = await service.getCustomAgents(CancellationToken.None); + assert.deepStrictEqual(agents.map(agent => ({ name: agent.name, hooks: agent.hooks })), [{ + name: 'reviewer', + hooks: undefined, + }]); + }); + test('managed-only hooks preserve frontmatter hooks from force-enabled plugin agents', async () => { testConfigService.setUserConfiguration(PromptsConfig.USE_CHAT_HOOKS, true); testConfigService.setUserConfiguration(COPILOT_ALLOW_MANAGED_HOOKS_ONLY_CONFIG, true); diff --git a/src/vs/workbench/contrib/mcp/common/mcpRegistry.ts b/src/vs/workbench/contrib/mcp/common/mcpRegistry.ts index fef5ad9ec09941..52aab7de0905fc 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpRegistry.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpRegistry.ts @@ -37,7 +37,7 @@ import { IMcpSandboxService } from './mcpSandboxService.js'; import { McpServerConnection } from './mcpServerConnection.js'; import { IMcpServerConnection, LazyCollectionState, McpCollectionDefinition, McpCollectionProvenance, McpDefinitionReference, McpServerDefinition, McpServerLaunch, McpServerTrust, McpStartServerInteraction, UserInteractionRequiredError } from './mcpTypes.js'; import { COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG } from '../../../../platform/policy/common/copilotManagedSettings.js'; -import { isStrictPluginOnlyCustomizationEnabled, StrictPluginOnlyCustomization } from '../../chat/common/customizationLockdown.js'; +import { isStrictPluginOnlyCustomizationBlocked, StrictPluginOnlyCustomization } from '../../chat/common/customizationLockdown.js'; const notTrustedNonce = '__vscode_not_trusted'; @@ -496,7 +496,7 @@ export class McpRegistry extends Disposable implements IMcpRegistry { } private isCollectionAllowed(collection: McpCollectionDefinition, strictPluginOnly: StrictPluginOnlyCustomization): boolean { - return !isStrictPluginOnlyCustomizationEnabled(strictPluginOnly) + return !isStrictPluginOnlyCustomizationBlocked(strictPluginOnly, 'mcp') || collection.provenance === McpCollectionProvenance.Plugin; } @@ -518,21 +518,22 @@ export class McpRegistry extends Disposable implements IMcpRegistry { if (!collection || !definition) { throw new Error(`Collection or definition not found for ${collectionRef.id} and ${definitionRef.id}`); } + const resolvedCollection = collection; - const delegate = this._delegates.get().find(d => d.canStart(collection, definition)); + const delegate = this._delegates.get().find(d => d.canStart(resolvedCollection, definition)); if (!delegate) { throw new Error('No delegate found that can handle the connection'); } - const trusted = await this._checkTrust(collection, definition, opts); + const trusted = await this._checkTrust(resolvedCollection, definition, opts); interaction?.participants.set(definition.id, { s: 'resolved' }); if (!trusted) { return undefined; } let launch: McpServerLaunch | undefined = definition.launch; - if (collection.resolveServerLanch) { - launch = await collection.resolveServerLanch(definition); + if (resolvedCollection.resolveServerLanch) { + launch = await resolvedCollection.resolveServerLanch(definition); if (!launch) { return undefined; // interaction cancelled by user } @@ -545,7 +546,7 @@ export class McpRegistry extends Disposable implements IMcpRegistry { launch = await this._instantiationService.invokeFunction(accessor => accessor.get(IMcpDevModeDebugging).transform(definition, launch!)); } // If sandbox is enabled for this server, attempt to launch in sandbox - launch = await this._mcpSandboxService.launchInSandboxIfEnabled(definition, launch, collection.remoteAuthority ?? undefined, collection.configTarget); + launch = await this._mcpSandboxService.launchInSandboxIfEnabled(definition, launch, resolvedCollection.remoteAuthority ?? undefined, resolvedCollection.configTarget); } catch (e) { if (e instanceof UserInteractionRequiredError) { throw e; @@ -555,7 +556,7 @@ export class McpRegistry extends Disposable implements IMcpRegistry { severity: Severity.Error, message: localize('mcp.launchError', 'Error starting {0}: {1}', definition.label, String(e)), actions: { - primary: collection.presentation?.origin && [ + primary: resolvedCollection.presentation?.origin && [ { id: 'mcp.launchError.openConfig', class: undefined, @@ -563,7 +564,7 @@ export class McpRegistry extends Disposable implements IMcpRegistry { tooltip: '', label: localize('mcp.launchError.openConfig', 'Open Configuration'), run: () => this._editorService.openEditor({ - resource: collection.presentation!.origin, + resource: resolvedCollection.presentation!.origin, options: { selection: definition.presentation?.origin?.range } }), } @@ -573,9 +574,17 @@ export class McpRegistry extends Disposable implements IMcpRegistry { return; } + const currentCollection = this._collections.get().find(candidate => candidate.id === collectionRef.id); + if (currentCollection !== resolvedCollection || !currentCollection.serverDefinitions.get().includes(definition)) { + throw new Error(`MCP collection ${collectionRef.id} changed while resolving the connection`); + } + if (!this.isCollectionAllowed(currentCollection, this._strictPluginOnlyCustomization.get())) { + throw new Error(`MCP collection ${collectionRef.id} is blocked by enterprise customization policy`); + } + return this._instantiationService.createInstance( McpServerConnection, - collection, + currentCollection, definition, delegate, launch, diff --git a/src/vs/workbench/contrib/mcp/common/mcpServer.ts b/src/vs/workbench/contrib/mcp/common/mcpServer.ts index 427a09b083ceba..a25ac9af2e6e5c 100644 --- a/src/vs/workbench/contrib/mcp/common/mcpServer.ts +++ b/src/vs/workbench/contrib/mcp/common/mcpServer.ts @@ -542,6 +542,10 @@ export class McpServer extends Disposable implements IMcpServer { this._policyEpoch = observableFromEvent(this, this._allowedMcpServersService.onDidChangeAllowedMcpServers, () => undefined); this._policyBlock = derived(this, reader => { this._policyEpoch.read(reader); + const fullDefinitions = this._fullDefinitions.read(reader); + if (!fullDefinitions.collection) { + return { state: McpConnectionState.Kind.Error, message: localize('mcp.customizationPolicyBlocked', "Blocked by enterprise customization policy") }; + } const connection = this._connection.read(reader); if (connection) { // Authoritative: the connection carries the fully resolved launch. @@ -554,7 +558,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 = fullDefinitions.server?.launch; if (!launch) { return undefined; } diff --git a/src/vs/workbench/contrib/mcp/test/common/mcpRegistry.test.ts b/src/vs/workbench/contrib/mcp/test/common/mcpRegistry.test.ts index 7d5ea1f8f5ba23..22aa827607d253 100644 --- a/src/vs/workbench/contrib/mcp/test/common/mcpRegistry.test.ts +++ b/src/vs/workbench/contrib/mcp/test/common/mcpRegistry.test.ts @@ -5,7 +5,7 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; -import { timeout } from '../../../../../base/common/async.js'; +import { DeferredPromise, timeout } from '../../../../../base/common/async.js'; import { Disposable } from '../../../../../base/common/lifecycle.js'; import { ISettableObservable, observableValue } from '../../../../../base/common/observable.js'; import { URI } from '../../../../../base/common/uri.js'; @@ -288,6 +288,29 @@ suite('Workbench - MCP - Registry', () => { assert.strictEqual(registry.getServerDefinition(pluginCollection, baseDefinition).get().server, baseDefinition); }); + test('selective plugin-only customization applies only to MCP', () => { + store.add(registry.registerCollection(testCollection)); + const pluginCollection = { + ...testCollection, + id: `${MCP_PLUGIN_COLLECTION_ID_PREFIX}selective`, + provenance: McpCollectionProvenance.Plugin, + serverDefinitions: observableValue('selectivePluginDefinitions', [baseDefinition]), + }; + store.add(registry.registerCollection(pluginCollection)); + + configurationService.setUserConfiguration(COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, ['skills']); + configurationService.onDidChangeConfigurationEmitter.fire({ + affectsConfiguration: (key: string) => key === COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, + } as unknown as IConfigurationChangeEvent); + assert.deepStrictEqual(registry.collections.get().map(collection => collection.id), [testCollection.id, pluginCollection.id]); + + configurationService.setUserConfiguration(COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, ['mcp']); + configurationService.onDidChangeConfigurationEmitter.fire({ + affectsConfiguration: (key: string) => key === COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, + } as unknown as IConfigurationChangeEvent); + assert.deepStrictEqual(registry.collections.get().map(collection => collection.id), [pluginCollection.id]); + }); + test('collections are not visible when not enabled', () => { const disposable = registry.registerCollection(testCollection); store.add(disposable); @@ -446,6 +469,61 @@ suite('Workbench - MCP - Registry', () => { connection.dispose(); }); + test('resolveConnection rejects a collection blocked while launch resolution is pending', async () => { + const deferredLaunch = new DeferredPromise(); + const customCollection: McpCollectionDefinition = { + ...testCollection, + id: 'pending-launch-collection', + serverDefinitions: observableValue('pendingLaunchDefinitions', [baseDefinition]), + resolveServerLanch: () => deferredLaunch.p, + }; + store.add(registry.registerDelegate(new TestMcpHostDelegate())); + store.add(registry.registerCollection(customCollection)); + + const connection = registry.resolveConnection({ + collectionRef: customCollection, + definitionRef: baseDefinition, + logger, + trustNonceBearer, + taskManager, + }); + configurationService.setUserConfiguration(COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, ['mcp']); + configurationService.onDidChangeConfigurationEmitter.fire({ + affectsConfiguration: (key: string) => key === COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, + } as unknown as IConfigurationChangeEvent); + deferredLaunch.complete(baseDefinition.launch); + + await assert.rejects(connection, /blocked by enterprise customization policy/); + }); + + test('resolveConnection rejects a same-ID collection replacement while launch resolution is pending', async () => { + const deferredLaunch = new DeferredPromise(); + const originalCollection: McpCollectionDefinition = { + ...testCollection, + id: 'replaced-launch-collection', + serverDefinitions: observableValue('replacedLaunchDefinitions', [baseDefinition]), + resolveServerLanch: () => deferredLaunch.p, + }; + store.add(registry.registerDelegate(new TestMcpHostDelegate())); + const registration = registry.registerCollection(originalCollection); + + const connection = registry.resolveConnection({ + collectionRef: originalCollection, + definitionRef: baseDefinition, + logger, + trustNonceBearer, + taskManager, + }); + registration.dispose(); + store.add(registry.registerCollection({ + ...originalCollection, + provenance: McpCollectionProvenance.Plugin, + })); + deferredLaunch.complete(baseDefinition.launch); + + await assert.rejects(connection, /changed while resolving the connection/); + }); + test('resolveConnection calls launchInSandboxIfEnabled with expected arguments when sandboxing is enabled', async () => { testMcpSandboxService.enabled = true; const mcpResource = URI.file('/test/mcp.json'); @@ -584,7 +662,7 @@ suite('Workbench - MCP - Registry', () => { }, }; store.add(registry.registerCollection(lazyCollection)); - configurationService.setUserConfiguration(COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, true); + configurationService.setUserConfiguration(COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, ['mcp']); configurationService.onDidChangeConfigurationEmitter.fire({ affectsConfiguration: (key: string) => key === COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, } as unknown as IConfigurationChangeEvent); diff --git a/src/vs/workbench/services/accounts/browser/managedSettings.ts b/src/vs/workbench/services/accounts/browser/managedSettings.ts index b764f929f499db..2d133233c9cb3c 100644 --- a/src/vs/workbench/services/accounts/browser/managedSettings.ts +++ b/src/vs/workbench/services/accounts/browser/managedSettings.ts @@ -7,7 +7,7 @@ import { IPolicyData } from '../../../../base/common/defaultAccount.js'; import { IProductConfiguration } from '../../../../base/common/product.js'; import { isString } from '../../../../base/common/types.js'; import { IManagedSettingsCompatibilityError, MANAGED_SETTINGS_UPDATE_REQUIRED_ERROR_CODE } from '../../../../platform/defaultAccount/common/defaultAccount.js'; -import { hasRawManagedSettings, normalizeManagedSettings } from '../../../../platform/policy/common/copilotManagedSettings.js'; +import { hasRawManagedSettings, normalizeManagedSettings, StrictPluginOnlyCustomizationSelector } from '../../../../platform/policy/common/copilotManagedSettings.js'; /** * Client identity VS Code reports to the managed settings service. It names this codebase's own @@ -64,7 +64,7 @@ export interface IManagedSettingsResponse { readonly strictKnownMarketplaces?: readonly unknown[]; readonly allowedMcpServers?: ReadonlyArray; readonly deniedMcpServers?: ReadonlyArray; - readonly strictPluginOnlyCustomization?: boolean; + readonly strictPluginOnlyCustomization?: boolean | readonly StrictPluginOnlyCustomizationSelector[]; readonly allowManagedMcpServersOnly?: boolean; readonly allowManagedHooksOnly?: boolean; readonly forceRemoteSettingsRefresh?: boolean; diff --git a/src/vs/workbench/services/accounts/test/browser/managedSettings.test.ts b/src/vs/workbench/services/accounts/test/browser/managedSettings.test.ts index 477b4f9426d303..f9dc32d73e95a1 100644 --- a/src/vs/workbench/services/accounts/test/browser/managedSettings.test.ts +++ b/src/vs/workbench/services/accounts/test/browser/managedSettings.test.ts @@ -151,6 +151,16 @@ suite('adaptManagedSettings', () => { }); }); + test('carries selective customization lockdown controls', () => { + assert.deepStrictEqual(adaptManagedSettings({ + strictPluginOnlyCustomization: ['skills', 'mcp'], + }), { + managedSettings: { + strictPluginOnlyCustomization: '["skills","mcp"]', + }, + }); + }); + test('flattens scalar telemetry leaves and carries resourceAttributes and headers as single JSON keys', () => { assert.deepStrictEqual(adaptManagedSettings({ telemetry: { diff --git a/src/vs/workbench/services/policies/test/browser/accountPolicyService.test.ts b/src/vs/workbench/services/policies/test/browser/accountPolicyService.test.ts index c779c755943a30..3981a6ac837c91 100644 --- a/src/vs/workbench/services/policies/test/browser/accountPolicyService.test.ts +++ b/src/vs/workbench/services/policies/test/browser/accountPolicyService.test.ts @@ -7,7 +7,7 @@ import assert from 'assert'; import { IDefaultAccount, IDefaultAccountAuthenticationProvider, IPolicyData } from '../../../../../base/common/defaultAccount.js'; import { Emitter, Event } from '../../../../../base/common/event.js'; import { Disposable } from '../../../../../base/common/lifecycle.js'; -import { ManagedSettingsData, PolicyCategory } from '../../../../../base/common/policy.js'; +import { IManagedSettingPolicyDefinition, ManagedSettingsData, PolicyCategory } from '../../../../../base/common/policy.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../base/test/common/utils.js'; import { AgentHostEnablementService } from '../../../../../platform/agentHost/browser/agentHostEnablementService.js'; import { Extensions, IConfigurationNode, IConfigurationRegistry } from '../../../../../platform/configuration/common/configurationRegistry.js'; @@ -749,7 +749,7 @@ suite('AccountPolicyService', () => { readonly _serviceBrand: undefined; private readonly _onDidChangeManagedSettings = new Emitter(); readonly onDidChangeManagedSettings = this._onDidChangeManagedSettings.event; - registeredManagedSettings: Record = {}; + registeredManagedSettings: Record = {}; constructor(public managedSettings: ManagedSettingsData = {}) { }