feat(plugins): allow component paths within repository boundary#308776
feat(plugins): allow component paths within repository boundary#308776xAndreiLi wants to merge 2 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR relaxes component-directory path containment for marketplace-discovered agent plugins by allowing manifests to reference component paths outside the plugin folder as long as they remain within a specified boundary (intended to be the marketplace repository root).
Changes:
- Add optional
boundaryUriparameter toresolveComponentDirs(and thread it throughparsePlugin) so containment can be checked against a wider boundary than the plugin root. - Pass the marketplace repository root as the boundary for marketplace plugin discovery to enable shared-component layouts across multiple plugins in one repo.
- Add unit tests validating boundary-based acceptance/rejection of traversing paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/chat/common/plugins/agentPluginServiceImpl.ts | Plumbs a repository-root boundary into marketplace plugin discovery so watchers/components can include shared dirs. |
| src/vs/platform/agentPlugins/common/pluginParsers.ts | Extends component-dir resolution (and parse entrypoint) with an optional boundary for containment checks. |
| src/vs/platform/agentPlugins/test/common/pluginParsers.test.ts | Adds tests covering boundary-allowed traversal and boundary-escape rejection. |
| const repositoryUri = this._pluginRepositoryService.getRepositoryUri(entry.plugin.marketplaceReference, entry.plugin.marketplaceType); | ||
|
|
||
| sources.push({ | ||
| uri: stat.resource, | ||
| fromMarketplace: entry.plugin, | ||
| repositoryUri, |
There was a problem hiding this comment.
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.
| export function resolveComponentDirs(pluginUri: URI, defaultDir: string, config: IComponentPathConfig, boundaryUri?: URI): readonly URI[] { | ||
| const boundary = 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); |
There was a problem hiding this comment.
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.
|
@microsoft-github-policy-service agree |
Add an optional `boundaryUri` parameter to `resolveComponentDirs` and `parsePlugin` that relaxes the path containment check from the plugin directory to a wider boundary (e.g. the repository root). Previously, relative paths like `../../skills/shared-skill` in a plugin manifest were silently dropped because the resolved URI fell outside the plugin directory. For marketplace plugins whose repository contains multiple plugins sharing resources, this was too restrictive. Now `MarketplaceAgentPluginDiscovery` passes the repository URI (from `IAgentPluginRepositoryService.getRepositoryUri`) as the boundary, so paths that stay within the repository are accepted. Non-marketplace plugins continue using the plugin directory as the default boundary, preserving the existing security constraint.
Only use boundaryUri when pluginUri is actually contained within it (isEqualOrParent(pluginUri, boundaryUri)). Otherwise fall back to pluginUri, preserving the default containment check. Previously, if a marketplace plugin was installed outside the repository directory (e.g. via npm/pip), passing that repository URI as boundaryUri would reject all manifest paths — including valid ones inside the plugin itself. Now resolveComponentDirs validates the relationship before widening the boundary, making the API safe for all callers without changes at the call site.
xAndreiLi
force-pushed
the
allow-marketplace-paths
branch
from
886c488 to
d4848d7
Compare
Add an optional
boundaryUriparameter toresolveComponentDirsandparsePluginthat relaxes the path containment check from the plugin directory to a wider boundary (e.g. the repository root).Previously, relative paths like
../../skills/shared-skillin a plugin manifest were silently dropped because the resolved URI fell outside the plugin directory. For marketplace plugins whose repository contains multiple plugins sharing resources, this was too restrictive.Now
MarketplaceAgentPluginDiscoverypasses the repository URI (fromIAgentPluginRepositoryService.getRepositoryUri) as the boundary, so paths that stay within the repository are accepted. Non-marketplace plugins continue using the plugin directory as the default boundary, preserving the existing security constraint.Closes #308775