fix(installer): address third-round PR #2353 review comments

(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 <module>'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 <spec> instead of <module.key=value>
(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.
This commit is contained in:
Brian Madison 2026-04-28 11:42:07 -05:00
parent ce12cc1a7f
commit fb57c81176
6 changed files with 80 additions and 17 deletions

View File

@ -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 npx bmad-method install --directory /path/to/project --modules bmm --tools claude-code --yes
``` ```
Override any module config option with `--set <module>.<key>=<value>` (repeatable). Run `--list-options` to see every available key: Override any module config option with `--set <module>.<key>=<value>` (repeatable). Run `--list-options [module]` to see locally-known official keys (built-in modules plus any external officials cached on this machine):
```bash ```bash
npx bmad-method install --yes \ npx bmad-method install --yes \

View File

@ -3030,6 +3030,25 @@ async function runTests() {
const empty = parseSetEntries(); const empty = parseSetEntries();
assert(empty && Object.keys(empty).length === 0, 'parseSetEntries() returns empty object when called without args'); 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 // discoverOfficialModuleYamls + formatOptionsList read the on-disk
// external-module cache. Point that env at a temp dir so test results // external-module cache. Point that env at a temp dir so test results
// don't depend on whatever the developer / CI runner has cached. // don't depend on whatever the developer / CI runner has cached.

View File

@ -19,8 +19,8 @@ module.exports = {
], ],
['--list-tools', 'Print all supported tool/IDE IDs (with target directories) and exit.'], ['--list-tools', 'Print all supported tool/IDE IDs (with target directories) and exit.'],
[ [
'--set <module.key=value>', '--set <spec>',
'Set a module config option non-interactively (e.g. --set bmm.project_knowledge=research). Repeatable. Run --list-options to see available keys.', 'Set a module config option non-interactively. Spec format: <module>.<key>=<value> (e.g. bmm.project_knowledge=research). Repeatable. Run --list-options to see available keys.',
(value, prev) => [...(prev || []), value], (value, prev) => [...(prev || []), value],
[], [],
], ],

View File

@ -175,6 +175,11 @@ async function formatOptionsList(moduleCode) {
} }
const sections = []; 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 <module>` 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 <module>.<key>=<value> (repeatable)', ''); sections.push('Available --set keys', 'Format: --set <module>.<key>=<value> (repeatable)', '');
for (const { code, yamlPath, source } of filtered) { for (const { code, yamlPath, source } of filtered) {
let parsed; let parsed;
@ -182,9 +187,13 @@ async function formatOptionsList(moduleCode) {
parsed = yaml.parse(await fs.readFile(yamlPath, 'utf8')); parsed = yaml.parse(await fs.readFile(yamlPath, 'utf8'));
} catch { } catch {
sections.push(`${code} (${source}): could not parse module.yaml`, ''); sections.push(`${code} (${source}): could not parse module.yaml`, '');
if (moduleCode) moduleScopedFailure = true;
continue;
}
if (!parsed || typeof parsed !== 'object') {
if (moduleCode) moduleScopedFailure = true;
continue; continue;
} }
if (!parsed || typeof parsed !== 'object') continue;
sections.push(formatModuleOptions(code, parsed, source)); 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 }; module.exports = { formatOptionsList, discoverOfficialModuleYamls };

View File

@ -56,27 +56,29 @@ class OfficialModules {
this.collectedConfig[moduleName][key] = value; this.collectedConfig[moduleName][key] = value;
} }
// Load schema so we can flag unknown keys. If the schema can't be loaded, if (!this.setOverrideKeys) this.setOverrideKeys = {};
// we skip key-existence validation but still apply overrides + carry-forward. 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; let schema = null;
const schemaPath = path.join(getModulePath(moduleName), 'module.yaml'); const schemaPath = path.join(getModulePath(moduleName), 'module.yaml');
if (await fs.pathExists(schemaPath)) { if (await fs.pathExists(schemaPath)) {
try { try {
schema = yaml.parse(await fs.readFile(schemaPath, 'utf8')); schema = yaml.parse(await fs.readFile(schemaPath, 'utf8'));
} catch { } 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(); const declaredKeys = new Set();
for (const [key, decl] of Object.entries(schema)) { if (schema && typeof schema === 'object') {
if (decl && typeof decl === 'object' && 'prompt' in decl) declaredKeys.add(key); 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. // Warn + track unknown keys from this run's --set entries.
for (const key of Object.keys(overrides)) { for (const key of Object.keys(overrides)) {
if (!declaredKeys.has(key)) { if (!declaredKeys.has(key)) {
@ -1634,6 +1636,19 @@ class OfficialModules {
for (const key of seededOverrideKeys) { for (const key of seededOverrideKeys) {
allAnswers[`${moduleName}_${key}`] = moduleOverrides[key]; 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 // Drop pre-seeded questions so the user is not re-prompted and so
// skipPrompts mode doesn't overwrite the override with the default. // skipPrompts mode doesn't overwrite the override with the default.
// In-place mutation keeps the rest of this method's `questions` references // In-place mutation keeps the rest of this method's `questions` references

View File

@ -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 <module>.<key>=<value>` entry. * Parse a single `--set <module>.<key>=<value>` entry.
* @param {string} entry - raw flag value * @param {string} entry - raw flag value
@ -26,21 +35,32 @@ function parseSetEntry(entry) {
if (!moduleCode || !key) { if (!moduleCode || !key) {
throw new Error(`--set "${entry}": empty module or key. Expected <module>.<key>=<value>`); throw new Error(`--set "${entry}": empty module or key. Expected <module>.<key>=<value>`);
} }
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 }; return { module: moduleCode, key, value };
} }
/** /**
* Parse repeated `--set` entries into a `{ module: { key: value } }` map. * Parse repeated `--set` entries into a `{ module: { key: value } }` map.
* Later entries overwrite earlier ones for the same key. * 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 * @param {string[]} entries
* @returns {Object<string, Object<string, string>>} * @returns {Object<string, Object<string, string>>}
*/ */
function parseSetEntries(entries) { function parseSetEntries(entries) {
const overrides = {}; const overrides = Object.create(null);
if (!Array.isArray(entries)) return overrides; if (!Array.isArray(entries)) return overrides;
for (const entry of entries) { for (const entry of entries) {
const { module: moduleCode, key, value } = parseSetEntry(entry); const { module: moduleCode, key, value } = parseSetEntry(entry);
if (!overrides[moduleCode]) overrides[moduleCode] = {}; if (!overrides[moduleCode]) overrides[moduleCode] = Object.create(null);
overrides[moduleCode][key] = value; overrides[moduleCode][key] = value;
} }
return overrides; return overrides;