Skip to content

feat(plugins): allow component paths within repository boundary #308776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xAndreiLi wants to merge 2 commits into
base: main
Choose a base branch
from
+38 −10
Open

feat(plugins): allow component paths within repository boundary #308776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8eaeca2
feat(plugins): allow component paths within repository boundary
xAndreiLi Apr 9, 2026
d4848d7
fix(plugins): guard boundaryUri against non-ancestor directories
xAndreiLi Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions src/vs/platform/agentPlugins/common/pluginParsers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,16 +187,18 @@ export function parseComponentPathConfig(raw: unknown): IComponentPathConfig {
/**
* Resolves the directories to scan for a given component type, combining
* the default directory with any custom paths from the manifest config.
* Paths that resolve outside the plugin root are silently ignored.
* Paths that resolve outside the boundary are silently ignored.
* @param boundaryUri The outermost directory that resolved paths must stay within. Defaults to {@link pluginUri}.
*/
export function resolveComponentDirs(pluginUri: URI, defaultDir: string, config: IComponentPathConfig): readonly URI[] {
export function resolveComponentDirs(pluginUri: URI, defaultDir: string, config: IComponentPathConfig, boundaryUri?: URI): readonly URI[] {
const boundary = (boundaryUri && isEqualOrParent(pluginUri, boundaryUri)) ? boundaryUri : pluginUri;
const dirs: URI[] = [];
if (!config.exclusive) {
dirs.push(joinPath(pluginUri, defaultDir));
}
for (const p of config.paths) {
const resolved = normalizePath(joinPath(pluginUri, p));
if (isEqualOrParent(resolved, pluginUri)) {
if (isEqualOrParent(resolved, boundary)) {
dirs.push(resolved);
Comment on lines +193 to 202
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boundaryUri currently replaces the containment check target unconditionally (boundary = boundaryUri ?? pluginUri). If a caller accidentally passes a boundary that is not an ancestor of pluginUri (different cache dir, different authority/scheme, etc.), then isEqualOrParent(resolved, boundary) will reject all manifest paths — including ones that are valid within the plugin itself (notably when exclusive: true).

To make this API safer and avoid surprising regressions, consider only using boundaryUri when pluginUri is equal to or contained by it (e.g. boundaryUri && isEqualOrParent(pluginUri, boundaryUri)), otherwise fall back to pluginUri. Adding a small unit test for the non-ancestor boundary case would help lock this in.

Copilot uses AI. Check for mistakes.
}
}
Expand Down Expand Up @@ -811,6 +813,7 @@ export async function parsePlugin(
fileService: IFileService,
workspaceRoot: URI | undefined,
userHome: string,
boundaryUri?: URI,
): Promise<IParsedPlugin> {
const formatConfig = await detectPluginFormat(pluginUri, fileService);

Expand All @@ -819,10 +822,10 @@ export async function parsePlugin(
const manifest = (manifestJson && typeof manifestJson === 'object') ? manifestJson as Record<string, unknown> : undefined;

// Resolve component directories from manifest
const hookDirs = resolveComponentDirs(pluginUri, formatConfig.hookConfigPath, parseComponentPathConfig(manifest?.['hooks']));
const mcpDirs = resolveComponentDirs(pluginUri, '.mcp.json', parseComponentPathConfig(manifest?.['mcpServers']));
const skillDirs = resolveComponentDirs(pluginUri, 'skills', parseComponentPathConfig(manifest?.['skills']));
const agentDirs = resolveComponentDirs(pluginUri, 'agents', parseComponentPathConfig(manifest?.['agents']));
const hookDirs = resolveComponentDirs(pluginUri, formatConfig.hookConfigPath, parseComponentPathConfig(manifest?.['hooks']), boundaryUri);
const mcpDirs = resolveComponentDirs(pluginUri, '.mcp.json', parseComponentPathConfig(manifest?.['mcpServers']), boundaryUri);
const skillDirs = resolveComponentDirs(pluginUri, 'skills', parseComponentPathConfig(manifest?.['skills']), boundaryUri);
const agentDirs = resolveComponentDirs(pluginUri, 'agents', parseComponentPathConfig(manifest?.['agents']), boundaryUri);

// Handle embedded MCP servers in manifest
let embeddedMcp: IMcpServerDefinition[] = [];
Expand Down
20 changes: 20 additions & 0 deletions src/vs/platform/agentPlugins/test/common/pluginParsers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,26 @@ suite('pluginParsers', () => {
// Should only have the default dir, the traversal path is rejected
assert.strictEqual(dirs.length, 1);
});

test('allows paths that escape plugin root but stay within boundaryUri', () => {
const boundaryUri = URI.file('/workspace');
const dirs = resolveComponentDirs(pluginUri, 'skills', { paths: ['../shared-skills'], exclusive: false }, boundaryUri);
assert.strictEqual(dirs.length, 2);
assert.ok(dirs[1].path.endsWith('/shared-skills'));
});

test('rejects paths that escape boundaryUri', () => {
const boundaryUri = URI.file('/workspace');
const dirs = resolveComponentDirs(pluginUri, 'skills', { paths: ['../../outside'], exclusive: false }, boundaryUri);
assert.strictEqual(dirs.length, 1);
});

test('falls back to pluginUri when boundaryUri is not an ancestor of pluginUri', () => {
const boundaryUri = URI.file('/unrelated/directory');
const dirs = resolveComponentDirs(pluginUri, 'skills', { paths: ['custom'], exclusive: false }, boundaryUri);
assert.strictEqual(dirs.length, 2);
assert.ok(dirs[1].path.endsWith('/custom'));
});
});

// ---- normalizeMcpServerConfiguration --------------------------------
Expand Down
11 changes: 8 additions & 3 deletions src/vs/workbench/contrib/chat/common/plugins/agentPluginServiceImpl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ type PluginEntry = IAgentPlugin;
interface IPluginSource {
readonly uri: URI;
readonly fromMarketplace: IMarketplacePlugin | undefined;
/** Repository root that serves as the boundary for component path resolution. */
readonly repositoryUri?: URI;
/** Called when remove is invoked on the plugin */
remove(): void;
}
Expand Down Expand Up @@ -203,7 +205,7 @@ export abstract class AbstractAgentPluginDiscovery extends Disposable implements
if (!seenPluginUris.has(key)) {
seenPluginUris.add(key);
const format = await detectPluginFormat(source.uri, this._fileService);
plugins.push(this._toPlugin(source.uri, format, source.fromMarketplace, () => source.remove()));
plugins.push(this._toPlugin(source.uri, format, source.fromMarketplace, source.repositoryUri, () => source.remove()));
}
}

Expand All @@ -222,7 +224,7 @@ export abstract class AbstractAgentPluginDiscovery extends Disposable implements
}
}

private _toPlugin(uri: URI, format: IPluginFormatConfig, fromMarketplace: IMarketplacePlugin | undefined, removeCallback: () => void): IAgentPlugin {
private _toPlugin(uri: URI, format: IPluginFormatConfig, fromMarketplace: IMarketplacePlugin | undefined, repositoryUri: URI | undefined, removeCallback: () => void): IAgentPlugin {
const key = uri.toString();
const existing = this._pluginEntries.get(key);
if (existing) {
Expand Down Expand Up @@ -258,7 +260,7 @@ export abstract class AbstractAgentPluginDiscovery extends Disposable implements
}

const paths = parseComponentPathConfig(section);
const dirs = resolveComponentDirs(uri, defaultPath, paths);
const dirs = resolveComponentDirs(uri, defaultPath, paths, repositoryUri);
for (const d of dirs) {
const watcher = this._fileService.createWatcher(d, { recursive: false, excludes: [] });
reader.store.add(watcher);
Expand Down Expand Up @@ -632,9 +634,12 @@ export class MarketplaceAgentPluginDiscovery extends AbstractAgentPluginDiscover
continue;
}

const repositoryUri = this._pluginRepositoryService.getRepositoryUri(entry.plugin.marketplaceReference, entry.plugin.marketplaceType);

sources.push({
uri: stat.resource,
fromMarketplace: entry.plugin,
repositoryUri,
Comment on lines +637 to +642
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MarketplaceAgentPluginDiscovery passes repositoryUri as the boundary for all marketplace plugins, but some installed marketplace plugins may not actually live under that repository directory (e.g. npm/pip installs or plugins sourced from an independent clone). In that case resolveComponentDirs(..., boundaryUri) will reject even paths that are inside the plugin itself (especially when exclusive: true), effectively breaking manifest path configuration.

Consider only widening the boundary when the plugin directory is actually contained by the repository (e.g. isEqualOrParent(pluginUri, repositoryUri)), or when the plugin’s source kind is the relative-path-in-repo kind; otherwise pass undefined so the boundary falls back to pluginUri.

Copilot uses AI. Check for mistakes.
remove: () => {
this._enablementModel.remove(stat.resource.toString());
this._pluginMarketplaceService.removeInstalledPlugin(entry.pluginUri);
Expand Down