From b238baddbf916f772a852064850906660bd2a387 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 4 Jun 2026 08:54:15 -0400 Subject: [PATCH] fix(#663): resolve open CodeQL/Dependabot security alerts (ReDoS, prototype pollution, workflow perms, qs DoS) (#665) * fix(#663): resolve open CodeQL/Dependabot security alerts - ReDoS: collapse ambiguous nested quantifiers in phase-heading regexes (verify/validate/commands) and the plan-filename lookahead (phase) to provably-equivalent non-backtracking forms - prototype pollution: guard __proto__/constructor/prototype in setConfigValue - remove dead no-op .replace(/-/g,'-') in phase.cts - escape all regex metachars in bug-2839 test - add contents:read permissions to security-scan + install-smoke workflows - pin qs >= 6.15.2 via overrides (DoS GHSA) - broaden prompt-injection allowlist to translated security-model docs Closes #663 Co-Authored-By: Claude Opus 4.8 * test(#663): regression tests for prototype-pollution guard and roadmap-phase ReDoS Behavioral test that config-set rejects __proto__/constructor/prototype keys without polluting Object.prototype, plus a ReDoS guard (timing-bound) and behavior-preservation assertions for the collapsed phase-heading regexes. Co-Authored-By: Claude Opus 4.8 * test(#663): make ReDoS regression assert structured result, not elapsed time Replace elapsed-time assertions (which tripped local/no-elapsed-assertion ESLint rule and were unsound for synchronous ReDoS) with structured-result assertions on adversarial inputs: assert that malformed phase headings/ unchecked-item lines without a terminating colon/space yield an empty Set, which is both the correct behavior and an exercise of the fixed linear regex on the catastrophic-backtracking input shape. Co-Authored-By: Claude Opus 4.8 * chore(#663): add Security changeset fragment for #665 Co-Authored-By: Claude Opus 4.8 * test(#663): fold prototype-pollution regression into config.test.cjs The standalone bug-663-config-prototype-pollution.test.cjs was a 9th config-module test file, tripping lint-test-file-count (the allowlist is ratcheted and must not grow). Consolidated into config.test.cjs instead. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .changeset/humble-badgers-dart.md | 5 + .github/workflows/install-smoke.yml | 3 + .github/workflows/security-scan.yml | 3 + package-lock.json | 16 -- package.json | 3 + scripts/prompt-injection-scan.sh | 2 +- src/commands.cts | 2 +- src/config.cts | 4 + src/phase.cts | 6 +- src/validate.cts | 4 +- src/verify.cts | 4 +- ...-review-fix-transactional-cleanup.test.cjs | 2 +- ...g-663-redos-roadmap-phase-parsing.test.cjs | 155 ++++++++++++++++++ tests/config.test.cjs | 57 +++++++ 14 files changed, 240 insertions(+), 26 deletions(-) create mode 100644 .changeset/humble-badgers-dart.md create mode 100644 tests/bug-663-redos-roadmap-phase-parsing.test.cjs diff --git a/.changeset/humble-badgers-dart.md b/.changeset/humble-badgers-dart.md new file mode 100644 index 000000000..ad1f67751 --- /dev/null +++ b/.changeset/humble-badgers-dart.md @@ -0,0 +1,5 @@ +--- +type: Security +pr: 665 +--- +**Hardened roadmap-phase parsing and config writes** — resolved ReDoS in phase-heading/plan-filename regexes (validate/verify/commands/phase), blocked prototype-pollution through dotted config keys in `config-set`, and pinned `qs >= 6.15.2` (DoS advisory). diff --git a/.github/workflows/install-smoke.yml b/.github/workflows/install-smoke.yml index 4368a0ef5..279923a8a 100644 --- a/.github/workflows/install-smoke.yml +++ b/.github/workflows/install-smoke.yml @@ -49,6 +49,9 @@ concurrency: group: install-smoke-${{ github.workflow }}-${{ github.head_ref || github.run_id }} cancel-in-progress: true +permissions: + contents: read + jobs: # --------------------------------------------------------------------------- # Job 1: tarball install (existing canonical path) diff --git a/.github/workflows/security-scan.yml b/.github/workflows/security-scan.yml index b687079c5..d25c3c9e4 100644 --- a/.github/workflows/security-scan.yml +++ b/.github/workflows/security-scan.yml @@ -22,6 +22,9 @@ concurrency: group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} cancel-in-progress: true +permissions: + contents: read + jobs: security: runs-on: ubuntu-latest diff --git a/package-lock.json b/package-lock.json index 1faa2c4b8..4bf7ce5eb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5010,22 +5010,6 @@ "node": ">= 16.0.0" } }, - "node_modules/typed-rest-client/node_modules/qs": { - "version": "6.15.1", - "resolved": "https://registry.npmjs.org/qs/-/qs-6.15.1.tgz", - "integrity": "sha512-6YHEFRL9mfgcAvql/XhwTvf5jKcOiiupt2FiJxHkiX1z4j7WL8J/jRHYLluORvc1XxB5rV20KoeK00gVJamspg==", - "dev": true, - "license": "BSD-3-Clause", - "dependencies": { - "side-channel": "^1.1.0" - }, - "engines": { - "node": ">=0.6" - }, - "funding": { - "url": "https://github.com/sponsors/ljharb" - } - }, "node_modules/typescript": { "version": "6.0.3", "resolved": "https://registry.npmjs.org/typescript/-/typescript-6.0.3.tgz", diff --git a/package.json b/package.json index a5cd186c6..b5a26e698 100644 --- a/package.json +++ b/package.json @@ -62,6 +62,9 @@ "typescript": "^6.0.3", "typescript-eslint": "^8.60.0" }, + "overrides": { + "qs": ">=6.15.2" + }, "optionalDependencies": { "fallow": "^2.70.0" }, diff --git a/scripts/prompt-injection-scan.sh b/scripts/prompt-injection-scan.sh index c05d5bb5c..78231ef12 100755 --- a/scripts/prompt-injection-scan.sh +++ b/scripts/prompt-injection-scan.sh @@ -83,7 +83,7 @@ ALLOWLIST=( # These files contain intentional injection examples / security-model prose # and are not attack vectors — they explain/demonstrate injection patterns. 'TEST-EXAMPLES.md' - 'docs/explanation/security-model.md' + 'explanation/security-model.md' ) is_allowlisted() { diff --git a/src/commands.cts b/src/commands.cts index a269e0a5a..a5cc33820 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -1215,7 +1215,7 @@ function cmdStats(cwd: string, format: string | undefined, raw: boolean): void { const roadmapContent = extractCurrentMilestone(roadmapRaw, cwd); // Matches both plain numeric (Phase 1:) and milestone-prefixed (Phase 2-01:) headings. // Also tolerates optional [bracket-token] scope prefix on phase headings. - const headingPattern = /#{2,4}\s*(?:\[[^\]]+\]\s*)?Phase\s+([\w][\w.-]*(?:-[\w.-]+)*)\s*:\s*([^\n]+)/gi; + const headingPattern = /#{2,4}\s*(?:\[[^\]]+\]\s*)?Phase\s+([\w][\w.-]*)\s*:\s*([^\n]+)/gi; let match: RegExpExecArray | null; while ((match = headingPattern.exec(roadmapContent)) !== null) { const key = normalizePhaseName(match[1]); diff --git a/src/config.cts b/src/config.cts index a1627268b..6c6b6635b 100644 --- a/src/config.cts +++ b/src/config.cts @@ -419,6 +419,10 @@ function setConfigValue(cwd: string, keyPath: string, parsedValue: unknown): Set // Set nested value using dot notation (e.g., "workflow.research") 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]; diff --git a/src/phase.cts b/src/phase.cts index 5b5b87fb1..408f08ae4 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -694,7 +694,7 @@ function cmdPhaseAdd(cwd: string, description: string, raw: boolean, customId?: let _dirName: string; if (customId || config.phase_naming === 'custom') { - _newPhaseId = customId || slug.toUpperCase().replace(/-/g, '-'); + _newPhaseId = customId || slug.toUpperCase(); if (!_newPhaseId) error('--id required when phase_naming is "custom"'); _dirName = `${prefix}${_newPhaseId}-${slug}`; } else { @@ -805,7 +805,7 @@ function cmdPhaseAddBatch(cwd: string, descriptions: string[], raw: boolean): vo let newPhaseId: number | string; let dirName: string; if (config.phase_naming === 'custom') { - newPhaseId = slug.toUpperCase().replace(/-/g, '-'); + newPhaseId = slug.toUpperCase(); dirName = `${prefix}${newPhaseId}-${slug}`; } else { maxPhase += 1; @@ -1176,7 +1176,7 @@ function updateRoadmapAfterPhaseRemoval( `${prefix}${decrementRoadmapPhaseNumber(num, removedInt)}${suffix}`, ); content = content.replace( - /(? `${decrementRoadmapPaddedPhaseNumber(phaseNum, removedInt)}-${planNum}`, ); diff --git a/src/validate.cts b/src/validate.cts index 4a4eb0fe0..0a6c30ece 100644 --- a/src/validate.cts +++ b/src/validate.cts @@ -94,7 +94,7 @@ export function buildRoadmapPhaseVariants(roadmapContent: string): RoadmapPhaseV const roadmapPhaseVariants = new Set(); // Matches both legacy numeric (Phase 1:), decimal (Phase 2.1:), milestone-prefixed (Phase 2-01:), // and bracket-prefixed (### [GSD] Phase 2-01:) headings. - const phasePattern = /#{2,4}\s*(?:\[[^\]]+\]\s*)?Phase\s+([\w][\w.-]*(?:-[\w.-]+)*)\s*:/gi; + const phasePattern = /#{2,4}\s*(?:\[[^\]]+\]\s*)?Phase\s+([\w][\w.-]*)\s*:/gi; let m: RegExpExecArray | null; while ((m = phasePattern.exec(roadmapContent)) !== null) { roadmapPhases.add(m[1]); @@ -106,7 +106,7 @@ export function buildRoadmapPhaseVariants(roadmapContent: string): RoadmapPhaseV export function buildNotStartedPhaseVariants(roadmapContent: string): Set { const notStartedPhases = new Set(); // Also matches milestone-prefixed and bracket-prefixed checklist items. - const uncheckedPattern = /-\s*\[\s\]\s*\*{0,2}Phase\s+([\w][\w.-]*(?:-[\w.-]+)*)[:\s*]/gi; + const uncheckedPattern = /-\s*\[\s\]\s*\*{0,2}Phase\s+([\w][\w.-]*)[:\s*]/gi; let um: RegExpExecArray | null; while ((um = uncheckedPattern.exec(roadmapContent)) !== null) { for (const variant of phaseVariants(um[1])) notStartedPhases.add(variant); diff --git a/src/verify.cts b/src/verify.cts index 9268d4e0b..8aa3d6016 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -643,7 +643,7 @@ function cmdValidateConsistency(cwd: string, raw: boolean): void { const roadmapPhases = new Set(); const phasePattern = - /#{2,4}\s*(?:\[[^\]]+\]\s*)?Phase\s+([\w][\w.-]*(?:-[\w.-]+)*)\s*:/gi; + /#{2,4}\s*(?:\[[^\]]+\]\s*)?Phase\s+([\w][\w.-]*)\s*:/gi; let m: RegExpExecArray | null; while ((m = phasePattern.exec(roadmapContent)) !== null) { roadmapPhases.add(m[1]); @@ -651,7 +651,7 @@ function cmdValidateConsistency(cwd: string, raw: boolean): void { const fullRoadmapPhases = new Set(); const fullPhasePattern = - /#{2,4}\s*(?:\[[^\]]+\]\s*)?Phase\s+([\w][\w.-]*(?:-[\w.-]+)*)\s*:/gi; + /#{2,4}\s*(?:\[[^\]]+\]\s*)?Phase\s+([\w][\w.-]*)\s*:/gi; let fm: RegExpExecArray | null; while ((fm = fullPhasePattern.exec(roadmapContentRaw)) !== null) { fullRoadmapPhases.add(fm[1]); diff --git a/tests/bug-2839-review-fix-transactional-cleanup.test.cjs b/tests/bug-2839-review-fix-transactional-cleanup.test.cjs index 1bd2c5f05..87cfdc66d 100644 --- a/tests/bug-2839-review-fix-transactional-cleanup.test.cjs +++ b/tests/bug-2839-review-fix-transactional-cleanup.test.cjs @@ -125,7 +125,7 @@ describe('bug-2839: /gsd-code-review-fix cleanup is transactional', () => { // (`rm -f .../.review-fix-recovery-pending.json`) or a shell-variable form // referring to the previously-declared `sentinel` variable // (`rm -f "$sentinel"` / `rm -f "${sentinel}"`). - const escapedName = SENTINEL_NAME.replace(/\./g, '\\.'); + const escapedName = SENTINEL_NAME.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); const sentinelRemovalRe = new RegExp( `(rm\\s+(?:-f\\s+)?[^\\n]*(?:${escapedName}|\\$\\{?sentinel\\}?)|unlink[^\\n]*(?:${escapedName}|\\$\\{?sentinel\\}?))` ); diff --git a/tests/bug-663-redos-roadmap-phase-parsing.test.cjs b/tests/bug-663-redos-roadmap-phase-parsing.test.cjs new file mode 100644 index 000000000..f4048c468 --- /dev/null +++ b/tests/bug-663-redos-roadmap-phase-parsing.test.cjs @@ -0,0 +1,155 @@ +/** + * Regression test for the ReDoS fixes in buildRoadmapPhaseVariants() and + * buildNotStartedPhaseVariants() (src/validate.cts, fix #663). + * + * The old patterns used nested quantifiers that caused catastrophic + * backtracking on crafted input: + * old: [\w][\w.-]*(?:-[\w.-]+)* ← ambiguous alternation → exponential + * new: [\w][\w.-]* ← single quantifier → linear + * + * The same nested-quantifier shape was fixed identically in src/verify.cts, + * src/commands.cts, and src/phase.cts. + * + * Part A: behavior preservation — the collapsed regex still matches the same + * phase identifiers as before on normal roadmap content. + * Part B: ReDoS adversarial fixtures — calls buildRoadmapPhaseVariants / + * buildNotStartedPhaseVariants with pathological input (a malformed heading + * or checklist line that has NO terminating colon). The adversarial input + * would cause catastrophic backtracking under the OLD nested-quantifier + * pattern; the fix makes backtracking linear. We assert on the STRUCTURED + * RESULT (the returned Set is empty — no match — because the colon is + * absent) rather than on elapsed time, in compliance with the + * local/no-elapsed-assertion ESLint rule. A { timeout: 5000 } backstop is + * retained so the test fails fast if a future regression reintroduces a + * slow pattern. + * + * Requirements: TEST-663-B + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const { + buildRoadmapPhaseVariants, + buildNotStartedPhaseVariants, +} = require('../gsd-core/bin/lib/validate.cjs'); + +// ─── Part A: behavior preservation ─────────────────────────────────────────── + +describe('buildRoadmapPhaseVariants — behavior preservation (#663)', () => { + test('matches plain numeric heading (Phase 1:)', () => { + const content = [ + '# Roadmap', + '## Phase 1: Foo', + ].join('\n'); + + const { roadmapPhases } = buildRoadmapPhaseVariants(content); + assert.ok(roadmapPhases.has('1'), 'roadmapPhases should contain "1"'); + }); + + test('matches milestone-prefixed heading (Phase 2-01:)', () => { + const content = [ + '# Roadmap', + '### Phase 2-01: Bar', + ].join('\n'); + + const { roadmapPhases } = buildRoadmapPhaseVariants(content); + assert.ok(roadmapPhases.has('2-01'), 'roadmapPhases should contain "2-01"'); + }); + + test('matches bracket-prefixed heading ([GSD] Phase 3.2:)', () => { + const content = [ + '# Roadmap', + '### [GSD] Phase 3.2: Baz', + ].join('\n'); + + const { roadmapPhases } = buildRoadmapPhaseVariants(content); + assert.ok(roadmapPhases.has('3.2'), 'roadmapPhases should contain "3.2"'); + }); + + test('collects all phase identifiers from mixed-format roadmap', () => { + const content = [ + '# Roadmap', + '## Phase 1: Alpha', + '### Phase 2-01: Beta', + '### [GSD] Phase 3.2: Gamma', + ].join('\n'); + + const { roadmapPhases } = buildRoadmapPhaseVariants(content); + assert.ok(roadmapPhases.has('1'), 'should have phase 1'); + assert.ok(roadmapPhases.has('2-01'), 'should have phase 2-01'); + assert.ok(roadmapPhases.has('3.2'), 'should have phase 3.2'); + }); + + test('populates roadmapPhaseVariants with padding-normalized forms', () => { + const content = [ + '# Roadmap', + '### Phase 2-01: Beta', + ].join('\n'); + + const { roadmapPhaseVariants } = buildRoadmapPhaseVariants(content); + // phaseVariants() adds both padded and unpadded forms + assert.ok(roadmapPhaseVariants.has('2-01') || roadmapPhaseVariants.has('02-01'), + 'roadmapPhaseVariants should contain at least one padding form of 2-01'); + }); +}); + +describe('buildNotStartedPhaseVariants — behavior preservation (#663)', () => { + test('matches unchecked checklist item (- [ ] Phase 4-01:)', () => { + const content = [ + '# Roadmap', + '- [ ] Phase 4-01: Qux', + ].join('\n'); + + const notStarted = buildNotStartedPhaseVariants(content); + // phaseVariants() expands 4-01 into multiple forms; at minimum the raw form is present. + assert.ok(notStarted.has('4-01') || notStarted.has('04-01'), + 'notStarted should contain a variant of 4-01'); + }); + + test('does not pick up checked items', () => { + const content = [ + '# Roadmap', + '- [x] Phase 5: Done', + ].join('\n'); + + const notStarted = buildNotStartedPhaseVariants(content); + assert.strictEqual(notStarted.has('5'), false, 'completed phase should not be in notStarted'); + }); +}); + +// ─── Part B: ReDoS adversarial fixtures ────────────────────────────────────── + +describe('buildRoadmapPhaseVariants — ReDoS adversarial fixture (#663)', () => { + // Pathological input: a heading line where the phase-id segment consists of + // many consecutive "-a" chunks with NO terminating colon. Under the old + // nested-quantifier pattern ([\w.-]*(?:-[\w.-]+)*\s*:) the engine must + // explore exponentially many ways to partition the "-a" repetitions before + // concluding there is no match. The fixed single-quantifier pattern + // ([\w.-]*\s*:) backtracks linearly. We assert that the malformed heading + // yields NO match (empty roadmapPhases Set) — the correct behavior when the + // terminating colon is absent. The { timeout: 5000 } backstop catches any + // regression that re-introduces a slow pattern. + test('malformed heading without colon yields no phase match (adversarial input)', { timeout: 5000 }, () => { + const pathological = '## Phase a' + '-a'.repeat(32) + ' '; + const { roadmapPhases } = buildRoadmapPhaseVariants(pathological); + assert.strictEqual(roadmapPhases.size, 0, + 'a heading with no terminating colon should not match any phase'); + }); +}); + +describe('buildNotStartedPhaseVariants — ReDoS adversarial fixture (#663)', () => { + // Same analysis: the old uncheckedPattern used the same nested quantifier. + // A checklist-style line with many "-a" segments and no colon triggers the + // same catastrophic backtracking. Assert the correct structured result: + // the malformed line yields an empty notStarted Set. + test('malformed unchecked-item without terminator yields no phase match (adversarial input)', { timeout: 5000 }, () => { + // No trailing colon or whitespace: the regex terminator [:\s*] cannot match, + // so the engine must backtrack through all '-a' repetitions and conclude no + // match. Under the old nested-quantifier pattern this was exponential; + // under the fixed linear pattern it returns immediately with an empty Set. + const pathological = '- [ ] Phase a' + '-a'.repeat(32); + const notStarted = buildNotStartedPhaseVariants(pathological); + assert.strictEqual(notStarted.size, 0, + 'an unchecked-item line with no terminating colon or space should not match any phase'); + }); +}); diff --git a/tests/config.test.cjs b/tests/config.test.cjs index 3680c9210..93af892ad 100644 --- a/tests/config.test.cjs +++ b/tests/config.test.cjs @@ -993,6 +993,63 @@ describe('config-path command (#2282)', () => { }); }); +// ─── config-set prototype-pollution guard (#663) ───────────────────────────── + +describe('config-set prototype-pollution guard (#663)', () => { + 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('rejects __proto__ key segment and does not pollute Object.prototype', () => { + const result = runGsdTools('config-set __proto__.polluted true', tmpDir); + + assert.strictEqual(result.success, false, `Expected failure but got: ${result.output}`); + + // No prototype pollution. + assert.strictEqual(({}).polluted, undefined, '__proto__ pollution: {}.polluted should be undefined'); + assert.strictEqual(Object.prototype.polluted, undefined, '__proto__ pollution: Object.prototype.polluted should be undefined'); + + // Confirm .planning/config.json does not have a 'polluted' property at any level. + const config = readConfig(tmpDir); + assert.strictEqual(Object.prototype.hasOwnProperty.call(config, 'polluted'), false, + 'config.json root must not gain a "polluted" key'); + }); + + test('rejects constructor.prototype key and does not pollute Object.prototype', () => { + const result = runGsdTools('config-set constructor.prototype.polluted2 true', tmpDir); + + assert.strictEqual(result.success, false, `Expected failure but got: ${result.output}`); + + assert.strictEqual(({}).polluted2, undefined, 'constructor chain pollution: {}.polluted2 should be undefined'); + assert.strictEqual(Object.prototype.polluted2, undefined, + 'constructor chain pollution: Object.prototype.polluted2 should be undefined'); + }); + + test('rejects bare prototype key segment', () => { + const result = runGsdTools('config-set prototype.x true', tmpDir); + + assert.strictEqual(result.success, false, `Expected failure but got: ${result.output}`); + assert.strictEqual(Object.prototype.x, undefined, 'prototype.x should not leak onto Object.prototype'); + }); + + test('positive control: legitimate nested key workflow.research succeeds', () => { + const result = runGsdTools('config-set workflow.research true', tmpDir); + + assert.ok(result.success, `Legitimate key rejected unexpectedly: ${result.error}`); + + const config = readConfig(tmpDir); + assert.strictEqual(config.workflow.research, true, 'workflow.research should be written to config.json'); + }); +}); + // ─── plan_review.source_grounding + _authority (#22) ───────────────────────── describe('plan_review.source_grounding and plan_review.source_grounding_authority (#22)', () => {