From fb57c81176d753db175fe9db077eae7d354fd29e Mon Sep 17 00:00:00 2001 From: Brian Madison Date: Tue, 28 Apr 2026 11:42:07 -0500 Subject: [PATCH] fix(installer): address third-round PR #2353 review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (J) Prototype pollution guard (CodeRabbit major). `--set __proto__.x=1` previously mutated Object.prototype because `overrides.__proto__` returned Object.prototype on a plain object, and assigning `[key]=value` polluted every plain object in the process. Verified the attack reproduces on f1c9e12 and is now blocked: parser rejects __proto__/prototype/constructor segments, and the maps are Object.create(null) for defense-in-depth. (I) Non-zero exit when --list-options 's yaml is unparseable (CodeRabbit major). formatOptionsList tracks moduleScopedFailure and returns ok:false in that case; install.js exits 1. (F) Dynamic defaults can now see --set sibling values (Augment medium). buildQuestion's function default falls back to `this.collectedConfig[mod][otherKey]`, but overrides were only in `allAnswers` (local) at default-evaluation time. Pre-write override raw values to collectedConfig before the prompt batch so the fallback resolves. Post-prompt template processing overwrites with the rendered version. (E) applyOverridesAfterSeeding no longer bypasses carry-forward when the schema can't be loaded (Augment low). Restructured: schema-load is now best-effort; without schema, declaredKeys is an empty Set, so all overrides are flagged as "unknown" and carry-forward runs against every prior key. Comment now matches behavior. (G) Flag placeholder --set instead of (Augment low) — angle brackets in the placeholder were misleading; the description spells out the spec format. (H) README wording: "every available key" → "locally-known official keys (built-in modules plus any external officials cached on this machine)" (CodeRabbit minor) — accurately reflects scope. Tests: +2 cases for prototype-pollution rejection. Total 343 passing. --- README.md | 2 +- test/test-installation-components.js | 19 +++++++++++ tools/installer/commands/install.js | 4 +-- tools/installer/list-options.js | 13 ++++++-- tools/installer/modules/official-modules.js | 35 +++++++++++++++------ tools/installer/set-overrides.js | 24 ++++++++++++-- 6 files changed, 80 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index f64b7186c..ea7ba5254 100644 --- a/README.md +++ b/README.md @@ -52,7 +52,7 @@ Follow the installer prompts, then open your AI IDE (Claude Code, Cursor, etc.) npx bmad-method install --directory /path/to/project --modules bmm --tools claude-code --yes ``` -Override any module config option with `--set .=` (repeatable). Run `--list-options` to see every available key: +Override any module config option with `--set .=` (repeatable). Run `--list-options [module]` to see locally-known official keys (built-in modules plus any external officials cached on this machine): ```bash npx bmad-method install --yes \ diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 77eca1aaa..0a2dad0c1 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -3030,6 +3030,25 @@ async function runTests() { const empty = parseSetEntries(); assert(empty && Object.keys(empty).length === 0, 'parseSetEntries() returns empty object when called without args'); + // parseSetEntries — prototype-pollution guard. `--set __proto__.x=1` would + // otherwise reach `overrides.__proto__[x] = 1` and pollute Object.prototype. + const polluteProbe = {}; + let pollutionThrown = false; + try { + parseSetEntries(['__proto__.polluted=1']); + } catch { + pollutionThrown = true; + } + assert(pollutionThrown, 'parseSetEntries rejects __proto__ as a module name'); + assert(polluteProbe.polluted === undefined, 'Object.prototype is not polluted by __proto__ in --set entries'); + let constructorThrown = false; + try { + parseSetEntries(['bmm.constructor=evil']); + } catch { + constructorThrown = true; + } + assert(constructorThrown, 'parseSetEntries rejects "constructor" as a key name'); + // discoverOfficialModuleYamls + formatOptionsList read the on-disk // external-module cache. Point that env at a temp dir so test results // don't depend on whatever the developer / CI runner has cached. diff --git a/tools/installer/commands/install.js b/tools/installer/commands/install.js index 962d0e4bd..e3dc3e48c 100644 --- a/tools/installer/commands/install.js +++ b/tools/installer/commands/install.js @@ -19,8 +19,8 @@ module.exports = { ], ['--list-tools', 'Print all supported tool/IDE IDs (with target directories) and exit.'], [ - '--set ', - 'Set a module config option non-interactively (e.g. --set bmm.project_knowledge=research). Repeatable. Run --list-options to see available keys.', + '--set ', + 'Set a module config option non-interactively. Spec format: .= (e.g. bmm.project_knowledge=research). Repeatable. Run --list-options to see available keys.', (value, prev) => [...(prev || []), value], [], ], diff --git a/tools/installer/list-options.js b/tools/installer/list-options.js index 0f3ae4517..84e091340 100644 --- a/tools/installer/list-options.js +++ b/tools/installer/list-options.js @@ -175,6 +175,11 @@ async function formatOptionsList(moduleCode) { } const sections = []; + // Track when a module-scoped lookup couldn't actually be rendered (yaml + // unparseable or empty after parse). The full `--list-options` output is + // tolerant of one bad entry, but `--list-options ` against a single + // unreadable module should still fail tooling so a CI script catches it. + let moduleScopedFailure = false; sections.push('Available --set keys', 'Format: --set .= (repeatable)', ''); for (const { code, yamlPath, source } of filtered) { let parsed; @@ -182,9 +187,13 @@ async function formatOptionsList(moduleCode) { parsed = yaml.parse(await fs.readFile(yamlPath, 'utf8')); } catch { sections.push(`${code} (${source}): could not parse module.yaml`, ''); + if (moduleCode) moduleScopedFailure = true; + continue; + } + if (!parsed || typeof parsed !== 'object') { + if (moduleCode) moduleScopedFailure = true; continue; } - if (!parsed || typeof parsed !== 'object') continue; sections.push(formatModuleOptions(code, parsed, source)); } @@ -194,7 +203,7 @@ async function formatOptionsList(moduleCode) { ); } - return { text: sections.join('\n'), ok: true }; + return { text: sections.join('\n'), ok: !moduleScopedFailure }; } module.exports = { formatOptionsList, discoverOfficialModuleYamls }; diff --git a/tools/installer/modules/official-modules.js b/tools/installer/modules/official-modules.js index be5976084..444cb6587 100644 --- a/tools/installer/modules/official-modules.js +++ b/tools/installer/modules/official-modules.js @@ -56,27 +56,29 @@ class OfficialModules { this.collectedConfig[moduleName][key] = value; } - // Load schema so we can flag unknown keys. If the schema can't be loaded, - // we skip key-existence validation but still apply overrides + carry-forward. + if (!this.setOverrideKeys) this.setOverrideKeys = {}; + if (!this.setOverrideKeys[moduleName]) this.setOverrideKeys[moduleName] = new Set(); + + // Try to load the module's schema. When available we can distinguish + // declared keys from unknown ones; when not (built-in is missing or + // unparseable — rare for `core`), we treat every prior + override key as + // unknown so carry-forward still runs and writeCentralConfig keeps them. let schema = null; const schemaPath = path.join(getModulePath(moduleName), 'module.yaml'); if (await fs.pathExists(schemaPath)) { try { schema = yaml.parse(await fs.readFile(schemaPath, 'utf8')); } catch { - // schema unparseable — skip key-existence validation + // schema unparseable — fall through to no-schema behavior } } - if (!schema || typeof schema !== 'object') return; - const declaredKeys = new Set(); - for (const [key, decl] of Object.entries(schema)) { - if (decl && typeof decl === 'object' && 'prompt' in decl) declaredKeys.add(key); + if (schema && typeof schema === 'object') { + for (const [key, decl] of Object.entries(schema)) { + if (decl && typeof decl === 'object' && 'prompt' in decl) declaredKeys.add(key); + } } - if (!this.setOverrideKeys) this.setOverrideKeys = {}; - if (!this.setOverrideKeys[moduleName]) this.setOverrideKeys[moduleName] = new Set(); - // Warn + track unknown keys from this run's --set entries. for (const key of Object.keys(overrides)) { if (!declaredKeys.has(key)) { @@ -1634,6 +1636,19 @@ class OfficialModules { for (const key of seededOverrideKeys) { allAnswers[`${moduleName}_${key}`] = moduleOverrides[key]; } + // Pre-write raw override values into collectedConfig so dynamic-default + // resolvers (`buildQuestion`'s function default) can see them when a + // sibling key uses a `{other_key}` placeholder. The fallback chain in + // that closure is: current prompt batch → `this.collectedConfig[mod]`, + // and overridden keys are removed from the prompt batch — without this + // pre-write the placeholder lookup would miss them. The raw value is + // overwritten with the template-rendered version after prompts complete. + if (seededOverrideKeys.size > 0) { + if (!this.collectedConfig[moduleName]) this.collectedConfig[moduleName] = {}; + for (const key of seededOverrideKeys) { + this.collectedConfig[moduleName][key] = moduleOverrides[key]; + } + } // Drop pre-seeded questions so the user is not re-prompted and so // skipPrompts mode doesn't overwrite the override with the default. // In-place mutation keeps the rest of this method's `questions` references diff --git a/tools/installer/set-overrides.js b/tools/installer/set-overrides.js index f47ae553a..27a7476eb 100644 --- a/tools/installer/set-overrides.js +++ b/tools/installer/set-overrides.js @@ -1,3 +1,12 @@ +// Names that, when used as object keys, can mutate `Object.prototype` and +// cascade into every plain-object lookup in the process. The whole `--set` +// pipeline assigns into plain `{}` maps keyed by user input, so a flag like +// `--set __proto__.polluted=1` would otherwise reach +// `overrides.__proto__[key] = value`, which assigns onto Object.prototype. +// We reject both segments at parse time and harden the maps in +// `parseSetEntries` with `Object.create(null)`. +const PROTOTYPE_POLLUTING_NAMES = new Set(['__proto__', 'prototype', 'constructor']); + /** * Parse a single `--set .=` entry. * @param {string} entry - raw flag value @@ -26,21 +35,32 @@ function parseSetEntry(entry) { if (!moduleCode || !key) { throw new Error(`--set "${entry}": empty module or key. Expected .=`); } + if (PROTOTYPE_POLLUTING_NAMES.has(moduleCode) || PROTOTYPE_POLLUTING_NAMES.has(key)) { + throw new Error( + `--set "${entry}": '__proto__', 'prototype', and 'constructor' are reserved and cannot be used as a module or key name.`, + ); + } return { module: moduleCode, key, value }; } /** * Parse repeated `--set` entries into a `{ module: { key: value } }` map. * Later entries overwrite earlier ones for the same key. + * + * Both the outer map and the per-module inner maps are `Object.create(null)` + * so that even if a future caller bypasses `parseSetEntry`'s reserved-name + * check, lookups can't traverse `Object.prototype` and pollution-style writes + * land on the map's own properties (not the global prototype). + * * @param {string[]} entries * @returns {Object>} */ function parseSetEntries(entries) { - const overrides = {}; + const overrides = Object.create(null); if (!Array.isArray(entries)) return overrides; for (const entry of entries) { const { module: moduleCode, key, value } = parseSetEntry(entry); - if (!overrides[moduleCode]) overrides[moduleCode] = {}; + if (!overrides[moduleCode]) overrides[moduleCode] = Object.create(null); overrides[moduleCode][key] = value; } return overrides;