fix(installer): address PR review findings for plugin resolver

- Guard against path traversal in plugin-resolver.js: skill paths from
  unverified marketplace.json are now constrained to the repo root using
  path.resolve() + startsWith check
- Skip npm install during browsing phase: cloneRepo() accepts
  skipInstall option, used in ui.js before user confirms selection,
  preventing arbitrary lifecycle script execution from untrusted repos
- Add createModuleDirectories() call to installFromResolution() so
  modules with declarative directory config are fully set up
- Fix ESLint: use replaceAll instead of replace with global regex
This commit is contained in:
Brian Madison 2026-04-09 08:44:17 -05:00
parent d03ba50a60
commit 489067fdda
4 changed files with 18 additions and 7 deletions

View File

@ -104,6 +104,7 @@ class CustomModuleManager {
* @param {string} repoUrl - GitHub repository URL
* @param {Object} [options] - Clone options
* @param {boolean} [options.silent] - Suppress spinner output
* @param {boolean} [options.skipInstall] - Skip npm install (for browsing before user confirms)
* @returns {string} Path to the cloned repository
*/
async cloneRepo(repoUrl, options = {}) {
@ -159,9 +160,9 @@ class CustomModuleManager {
}
}
// Install dependencies if package.json exists
// Install dependencies if package.json exists (skip during browsing/analysis)
const packageJsonPath = path.join(repoCacheDir, 'package.json');
if (await fs.pathExists(packageJsonPath)) {
if (!options.skipInstall && (await fs.pathExists(packageJsonPath))) {
const installSpinner = await createSpinner();
installSpinner.start(`Installing dependencies for ${owner}/${repo}...`);
try {

View File

@ -326,6 +326,11 @@ class OfficialModules {
if (fileTrackingCallback) fileTrackingCallback(helpTarget);
}
// Create directories declared in module.yaml (strategies 1-4 may have these)
if (!options.skipModuleInstaller) {
await this.createModuleDirectories(resolved.code, bmadDir, options);
}
// Update manifest
const { Manifest } = require('../core/manifest');
const manifestObj = new Manifest();

View File

@ -33,11 +33,16 @@ class PluginResolver {
return [];
}
// Resolve skill paths to absolute and filter out non-existent
// Resolve skill paths to absolute, constrain to repo root, filter non-existent
const repoRoot = path.resolve(repoPath);
const skillPaths = [];
for (const rel of skillRelPaths) {
const normalized = rel.replace(/^\.\//, '');
const abs = path.join(repoPath, normalized);
const abs = path.resolve(repoPath, normalized);
// Guard against path traversal (.. segments, absolute paths in marketplace.json)
if (!abs.startsWith(repoRoot + path.sep) && abs !== repoRoot) {
continue;
}
if (await fs.pathExists(abs)) {
skillPaths.push(abs);
}
@ -384,7 +389,7 @@ class PluginResolver {
_escapeCSVField(value) {
if (!value) return '';
if (value.includes(',') || value.includes('"') || value.includes('\n')) {
return `"${value.replace(/"/g, '""')}"`;
return `"${value.replaceAll('"', '""')}"`;
}
return value;
}

View File

@ -863,11 +863,11 @@ class UI {
'UNVERIFIED MODULE: This module has not been reviewed by the BMad team.\n' + ' Only install modules from sources you trust.',
);
// Clone the repo so we can resolve plugin structures
// Clone the repo so we can resolve plugin structures (skip npm install until user confirms)
s.start('Cloning repository...');
let repoPath;
try {
repoPath = await customMgr.cloneRepo(url.trim());
repoPath = await customMgr.cloneRepo(url.trim(), { skipInstall: true });
s.stop('Repository cloned');
} catch (cloneError) {
s.error('Failed to clone repository');