From 612e3e60e88a7d83dde3c4f72ec0383f9570dc66 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 7 Jun 2026 00:18:00 -0400 Subject: [PATCH] fix(#751): recognise config-set prototype-pollution guard in CodeQL + test dynamic-key vectors (#752) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL alert #26 (js/prototype-pollution-utility) kept firing on setConfigValue because its dataflow does not trace the #663 Set-based, pre-loop keys.some(...) forbidden-key check as a sanitising barrier on the write site. - src/config.cts: replace the Set + pre-loop check with inline literal comparisons (key === '__proto__' || 'prototype' || 'constructor') on the exact key used to index `current`, immediately before each write (intermediate keys in the descent loop, plus the final key). Same forbidden set, same error message and ERROR_REASON.CONFIG_PARSE_FAILED — behaviour unchanged from #663, but the barrier is now CodeQL-recognised. - tests/config.test.cjs: add regression tests for schema-valid dynamic-prefix keys (agent_skills.__proto__, agent_skills.constructor, agent_skills.prototype, features.__proto__, review.models.constructor) that pass the isValidConfigKey schema gate and reach the guard. Each asserts the guard's own message fires (not the schema gate's "Unknown config key") and Object.prototype is not polluted. The prior #663 tests never reached the guard — their keys are rejected by the schema gate first — so the guard's real attack surface was untested. Co-authored-by: Claude Opus 4.8 --- ...fig-prototype-pollution-codeql-alert-26.md | 5 + src/config.cts | 23 +++- tests/config.test.cjs | 121 ++++++++++++++++++ 3 files changed, 142 insertions(+), 7 deletions(-) create mode 100644 .changeset/751-config-prototype-pollution-codeql-alert-26.md 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)', () => {