From 08c73796a66db7679acc757d6c59cbaa483c959b Mon Sep 17 00:00:00 2001 From: pbean Date: Sat, 20 Jun 2026 15:57:38 -0700 Subject: [PATCH] fix(bmad-module): prevent atomicSwapDir data loss + harden ide-sync/wds cleanup paths - atomicSwapDir: back the existing target up to a sibling before swapping the staged dir in, and roll back on a failed rename so an interrupted update can no longer leave neither the old nor the new install. - ide-sync cleanup: only remove _bmad/ skill source dirs when EVERY IDE synced successfully (so failed targets remain retryable), and containment-check each CSV-derived path before fs.remove so a malformed row can't escape _bmad/. - module-dirs: validate untrusted wds_folders entries for traversal/absolute escape before mkdir. - Regenerate the vendored ide-sync bundle (vendor:check clean). Co-Authored-By: Claude Opus 4.8 --- .../bmad-module/scripts/lib/fs-safe.mjs | 21 ++++++++++++++++--- .../bmad-module/scripts/lib/module-dirs.mjs | 9 +++++++- .../scripts/lib/vendor/ide-sync.mjs | 9 ++++++-- tools/installer/core/ide-sync.js | 15 ++++++++++--- 4 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/core-skills/bmad-module/scripts/lib/fs-safe.mjs b/src/core-skills/bmad-module/scripts/lib/fs-safe.mjs index fe8380844..2cd846d69 100644 --- a/src/core-skills/bmad-module/scripts/lib/fs-safe.mjs +++ b/src/core-skills/bmad-module/scripts/lib/fs-safe.mjs @@ -99,7 +99,9 @@ export async function stageCopyPlan(srcRoot, destDir, plan, extras = {}) { export async function atomicSwapDir(stagedDir, targetDir) { const parent = path.dirname(targetDir); await fsp.mkdir(parent, { recursive: true }); - const sibling = path.join(parent, `.${path.basename(targetDir)}.bmad-tmp-${crypto.randomBytes(6).toString('hex')}`); + const suffix = crypto.randomBytes(6).toString('hex'); + const sibling = path.join(parent, `.${path.basename(targetDir)}.bmad-tmp-${suffix}`); + const backup = path.join(parent, `.${path.basename(targetDir)}.bmad-old-${suffix}`); try { try { await fsp.rename(stagedDir, sibling); @@ -108,10 +110,23 @@ export async function atomicSwapDir(stagedDir, targetDir) { await copyDir(stagedDir, sibling); await fsp.rm(stagedDir, { recursive: true, force: true }); } - await fsp.rm(targetDir, { recursive: true, force: true }); - await fsp.rename(sibling, targetDir); + // Move any existing target aside as a backup rather than deleting it + // up front, so a failed swap can be rolled back to the old install. + const hadTarget = await fsp + .stat(targetDir) + .then(() => true) + .catch(() => false); + if (hadTarget) await fsp.rename(targetDir, backup); + try { + await fsp.rename(sibling, targetDir); + } catch (e) { + if (hadTarget) await fsp.rename(backup, targetDir).catch(() => {}); + throw e; + } + if (hadTarget) await fsp.rm(backup, { recursive: true, force: true }); } catch (e) { await fsp.rm(sibling, { recursive: true, force: true }); + await fsp.rm(backup, { recursive: true, force: true }); throw e; } } diff --git a/src/core-skills/bmad-module/scripts/lib/module-dirs.mjs b/src/core-skills/bmad-module/scripts/lib/module-dirs.mjs index 47e42c918..dcd41a2a4 100644 --- a/src/core-skills/bmad-module/scripts/lib/module-dirs.mjs +++ b/src/core-skills/bmad-module/scripts/lib/module-dirs.mjs @@ -108,7 +108,14 @@ export async function createModuleDirectories(bmadDir, code, moduleConfig = {}, // WDS subfolders under design_artifacts. if (configKey === 'design_artifacts' && wdsFolders.length) { for (const sub of wdsFolders) { - const subPath = path.join(fullPath, sub); + if (typeof sub !== 'string' || sub === '') continue; + const subPath = path.normalize(path.join(fullPath, sub)); + // `sub` is untrusted module content; reject traversal/absolute escapes + // out of the design_artifacts directory. + if (subPath !== normalizedNewAbs && !subPath.startsWith(normalizedNewAbs + path.sep)) { + warn(`wds_folders entry escapes design_artifacts, skipping: ${sub}`); + continue; + } if (!(await pathExists(subPath))) { await fs.mkdir(subPath, { recursive: true }); createdWdsFolders.push(sub); diff --git a/src/core-skills/bmad-module/scripts/lib/vendor/ide-sync.mjs b/src/core-skills/bmad-module/scripts/lib/vendor/ide-sync.mjs index 8dae69d46..5df8ea9a5 100644 --- a/src/core-skills/bmad-module/scripts/lib/vendor/ide-sync.mjs +++ b/src/core-skills/bmad-module/scripts/lib/vendor/ide-sync.mjs @@ -10233,7 +10233,8 @@ var require_ide_sync = __commonJS({ verbose, silent }); - if (cleanup) await cleanupBmadSkillDirs(bmadDir); + const allSucceeded = results.every((r) => r && r.success); + if (cleanup && allSucceeded) await cleanupBmadSkillDirs(bmadDir); return { skipped: false, results }; } async function cleanupBmadSkillDirs(bmadDir) { @@ -10244,10 +10245,14 @@ var require_ide_sync = __commonJS({ const records = csv.parse(csvContent, { columns: true, skip_empty_lines: true }); const bmadFolderName = path.basename(bmadDir); const bmadPrefix = bmadFolderName + "/"; + const bmadRoot = path.resolve(bmadDir); for (const record of records) { if (!record.path) continue; const relativePath = record.path.startsWith(bmadPrefix) ? record.path.slice(bmadPrefix.length) : record.path; - const sourceDir = path.dirname(path.join(bmadDir, relativePath)); + const skillFilePath = path.resolve(bmadDir, relativePath); + if (skillFilePath !== bmadRoot && !skillFilePath.startsWith(bmadRoot + path.sep)) continue; + const sourceDir = path.dirname(skillFilePath); + if (sourceDir === bmadRoot) continue; if (await fs.pathExists(sourceDir)) { await fs.remove(sourceDir); await removeEmptyParents(path.dirname(sourceDir), bmadDir); diff --git a/tools/installer/core/ide-sync.js b/tools/installer/core/ide-sync.js index e311355bb..6f6de5d29 100644 --- a/tools/installer/core/ide-sync.js +++ b/tools/installer/core/ide-sync.js @@ -54,8 +54,11 @@ async function syncIdes({ projectRoot, bmadDir, ides, previousSkillIds = [], ver }); // Mirror Installer._cleanupSkillDirs: skills are self-contained in IDE dirs, - // so _bmad/ only needs module-level files. - if (cleanup) await cleanupBmadSkillDirs(bmadDir); + // so _bmad/ only needs module-level files. Only clean up when every IDE + // synced successfully — otherwise the source skill dirs are still needed to + // retry the failed targets. + const allSucceeded = results.every((r) => r && r.success); + if (cleanup && allSucceeded) await cleanupBmadSkillDirs(bmadDir); return { skipped: false, results }; } @@ -77,10 +80,16 @@ async function cleanupBmadSkillDirs(bmadDir) { const bmadFolderName = path.basename(bmadDir); const bmadPrefix = bmadFolderName + '/'; + const bmadRoot = path.resolve(bmadDir); for (const record of records) { if (!record.path) continue; const relativePath = record.path.startsWith(bmadPrefix) ? record.path.slice(bmadPrefix.length) : record.path; - const sourceDir = path.dirname(path.join(bmadDir, relativePath)); + const skillFilePath = path.resolve(bmadDir, relativePath); + // Containment guard: a malformed CSV row (absolute path or `../`) must not + // let cleanup escape _bmad/ and remove arbitrary directories. + if (skillFilePath !== bmadRoot && !skillFilePath.startsWith(bmadRoot + path.sep)) continue; + const sourceDir = path.dirname(skillFilePath); + if (sourceDir === bmadRoot) continue; if (await fs.pathExists(sourceDir)) { await fs.remove(sourceDir); await removeEmptyParents(path.dirname(sourceDir), bmadDir);