diff --git a/.changeset/751-config-prototype-pollution-codeql-alert-26.md b/.changeset/751-config-prototype-pollution-codeql-alert-26.md new file mode 100644 index 000000000..2e638c05e --- /dev/null +++ b/.changeset/751-config-prototype-pollution-codeql-alert-26.md @@ -0,0 +1,5 @@ +--- +type: Security +pr: 752 +--- +**`gsd-tools config-set` prototype-pollution guard hardened and regression-tested.** The guard that blocks `__proto__`, `prototype`, and `constructor` segments in dotted config keys now uses inline literal comparisons at each property-write site (instead of a pre-loop `Set` check), so CodeQL's `js/prototype-pollution-utility` analysis recognises it as a sanitising barrier and code-scanning alert #26 clears. Runtime behaviour is unchanged from #663. Added regression tests that drive schema-valid dynamic-prefix keys (`agent_skills.__proto__`, `agent_skills.constructor`, `features.__proto__`, `review.models.constructor`) all the way to the guard — these reach `setConfigValue` past the schema gate and were previously the guard's only untested attack surface. (#751) diff --git a/src/config.cts b/src/config.cts index a0eb45f7c..4fa4d9efc 100644 --- a/src/config.cts +++ b/src/config.cts @@ -429,22 +429,31 @@ function setConfigValue(cwd: string, keyPath: string, parsedValue: unknown): Set error('Failed to read config.json: ' + (err as Error).message, ERROR_REASON.CONFIG_PARSE_FAILED); } - // Set nested value using dot notation (e.g., "workflow.research") + // Set nested value using dot notation (e.g., "workflow.research"). + // Prototype-pollution guard: reject dangerous segments via inline literal + // comparisons on the exact key used to index `current`, immediately before + // each write. The inline comparison is the barrier CodeQL's + // js/prototype-pollution-utility query recognises — the previous Set-based + // pre-loop check was functionally correct but not traced through, so + // code-scanning alert #26 kept firing. Behaviour is unchanged from #663. const keys = keyPath.split('.'); - const FORBIDDEN_KEYS = new Set(['__proto__', 'prototype', 'constructor']); - if (keys.some((k) => FORBIDDEN_KEYS.has(k))) { - error('Invalid config key (prototype pollution guard): ' + keyPath, ERROR_REASON.CONFIG_PARSE_FAILED); - } let current: Record = config; for (let i = 0; i < keys.length - 1; i++) { const key = keys[i]; + if (key === '__proto__' || key === 'prototype' || key === 'constructor') { + error('Invalid config key (prototype pollution guard): ' + keyPath, ERROR_REASON.CONFIG_PARSE_FAILED); + } if (current[key] === undefined || typeof current[key] !== 'object') { current[key] = {}; } current = current[key] as Record; } - const previousValue = current[keys[keys.length - 1]]; // Capture previous value before overwriting - current[keys[keys.length - 1]] = parsedValue; + const lastKey = keys[keys.length - 1]; + if (lastKey === '__proto__' || lastKey === 'prototype' || lastKey === 'constructor') { + error('Invalid config key (prototype pollution guard): ' + keyPath, ERROR_REASON.CONFIG_PARSE_FAILED); + } + const previousValue = current[lastKey]; // Capture previous value before overwriting + current[lastKey] = parsedValue; // Write back try { diff --git a/tests/config.test.cjs b/tests/config.test.cjs index d369d29fe..1180f5222 100644 --- a/tests/config.test.cjs +++ b/tests/config.test.cjs @@ -1158,6 +1158,127 @@ describe('config-set prototype-pollution guard (#663)', () => { }); }); +// ─── config-set prototype-pollution guard via dynamic-key prefixes (alert #26) ─ + +describe('config-set prototype-pollution guard via dynamic-key prefixes (alert #26)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + // Initialise config so there is a config.json to write to. + runGsdTools('config-ensure-section', tmpDir); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('agent_skills.__proto__ is blocked by setConfigValue guard (not schema gate)', () => { + const result = runGsdTools('config-set agent_skills.__proto__ somevalue', tmpDir); + + assert.strictEqual(result.success, false, `Expected failure but got: ${result.output}`); + + // Must be the pollution guard, not the schema gate. + assert.ok( + result.error.includes('prototype pollution guard'), + `Expected "prototype pollution guard" in error, got: ${result.error}`, + ); + // No schema-gate message. + assert.ok( + !result.error.includes('Unknown config key'), + `Should not hit schema gate, got: ${result.error}`, + ); + + // No prototype pollution occurred. + assert.strictEqual(({}).somevalue, undefined, 'agent_skills.__proto__: {}.somevalue should be undefined'); + assert.strictEqual(Object.prototype.hasOwnProperty.call(Object.prototype, 'somevalue'), false, + 'agent_skills.__proto__: Object.prototype should not gain "somevalue"'); + }); + + test('agent_skills.constructor is blocked by setConfigValue guard (not schema gate)', () => { + const result = runGsdTools('config-set agent_skills.constructor somevalue', tmpDir); + + assert.strictEqual(result.success, false, `Expected failure but got: ${result.output}`); + + assert.ok( + result.error.includes('prototype pollution guard'), + `Expected "prototype pollution guard" in error, got: ${result.error}`, + ); + assert.ok( + !result.error.includes('Unknown config key'), + `Should not hit schema gate, got: ${result.error}`, + ); + + assert.strictEqual(Object.prototype.hasOwnProperty.call(Object.prototype, 'somevalue'), false, + 'agent_skills.constructor: Object.prototype should not gain "somevalue"'); + }); + + test('agent_skills.prototype is blocked by setConfigValue guard (not schema gate)', () => { + const result = runGsdTools('config-set agent_skills.prototype somevalue', tmpDir); + + assert.strictEqual(result.success, false, `Expected failure but got: ${result.output}`); + + assert.ok( + result.error.includes('prototype pollution guard'), + `Expected "prototype pollution guard" in error, got: ${result.error}`, + ); + assert.ok( + !result.error.includes('Unknown config key'), + `Should not hit schema gate, got: ${result.error}`, + ); + + assert.strictEqual(Object.prototype.hasOwnProperty.call(Object.prototype, 'somevalue'), false, + 'agent_skills.prototype: Object.prototype should not gain "somevalue"'); + }); + + test('features.__proto__ is blocked by setConfigValue guard (not schema gate)', () => { + const result = runGsdTools('config-set features.__proto__ somevalue', tmpDir); + + assert.strictEqual(result.success, false, `Expected failure but got: ${result.output}`); + + assert.ok( + result.error.includes('prototype pollution guard'), + `Expected "prototype pollution guard" in error, got: ${result.error}`, + ); + assert.ok( + !result.error.includes('Unknown config key'), + `Should not hit schema gate, got: ${result.error}`, + ); + + assert.strictEqual(({}).somevalue, undefined, 'features.__proto__: {}.somevalue should be undefined'); + assert.strictEqual(Object.prototype.hasOwnProperty.call(Object.prototype, 'somevalue'), false, + 'features.__proto__: Object.prototype should not gain "somevalue"'); + }); + + test('review.models.constructor is blocked by setConfigValue guard (not schema gate)', () => { + const result = runGsdTools('config-set review.models.constructor somevalue', tmpDir); + + assert.strictEqual(result.success, false, `Expected failure but got: ${result.output}`); + + assert.ok( + result.error.includes('prototype pollution guard'), + `Expected "prototype pollution guard" in error, got: ${result.error}`, + ); + assert.ok( + !result.error.includes('Unknown config key'), + `Should not hit schema gate, got: ${result.error}`, + ); + + assert.strictEqual(Object.prototype.hasOwnProperty.call(Object.prototype, 'somevalue'), false, + 'review.models.constructor: Object.prototype should not gain "somevalue"'); + }); + + test('positive control: agent_skills.sonnet-coder with valid value succeeds', () => { + const result = runGsdTools('config-set agent_skills.sonnet-coder true', tmpDir); + + assert.ok(result.success, `Legitimate agent_skills key rejected unexpectedly: ${result.error}`); + + const config = readConfig(tmpDir); + assert.strictEqual(config.agent_skills['sonnet-coder'], true, + 'agent_skills.sonnet-coder should be written to config.json'); + }); +}); + // ─── plan_review.source_grounding + _authority (#22) ───────────────────────── describe('plan_review.source_grounding and plan_review.source_grounding_authority (#22)', () => {