diff --git a/src/phase-id.cts b/src/phase-id.cts index 68d6b4015..e08a4af43 100644 --- a/src/phase-id.cts +++ b/src/phase-id.cts @@ -273,9 +273,16 @@ function phaseTokenMatches(dirName: string, normalized: string): boolean { */ function parsePhaseFromProse(value: string | null): { phase: string | null; name: string | null } { if (!value) return { phase: null, name: null }; - const phaseMatch = value.match(/^\s*(?:Phase\s+)?(?:[A-Z][A-Z0-9_]*-)?(\d+[A-Z]?(?:\.\d+)*)\b/i); - const parenName = value.match(/\(([^)]+)\)/); - const dashName = value.match(/—\s*([^(\n]+?)(?:\s*\(|$)/); + // Coerce defensively so a non-string caller cannot throw on this canonical + // surface (mirrors the sibling #2121 functions' String(...) handling). + const str = String(value); + const phaseMatch = str.match(/^\s*(?:Phase\s+)?(?:[A-Z][A-Z0-9_]*-)?(\d+[A-Z]?(?:\.\d+)*)\b/i); + // The name-extraction quantifiers are length-bounded so a crafted long + // unterminated run (many `(` or `—`) in an untrusted STATE.md field value + // cannot drive O(n^2) regex backtracking (CPU-exhaustion DoS). A real phase + // name is far shorter than the cap. + const parenName = str.match(/\(([^)]{1,200})\)/); + const dashName = str.match(/—\s*([^(\n]{1,200}?)(?:\s*\(|$)/); const rawName = parenName?.[1] ?? dashName?.[1] ?? null; const name = rawName && !/^(?:complete|executing|not started)$/i.test(rawName.trim()) ? rawName.trim() diff --git a/src/state.cts b/src/state.cts index 02cfc8b59..0e8199266 100644 --- a/src/state.cts +++ b/src/state.cts @@ -1118,8 +1118,12 @@ function matchSessionSection(body: string): RegExpMatchArray | null { function parseProsePhaseField(value: string | null): { phase: string | null; name: string | null } { if (!value) return { phase: null, name: null }; const phaseMatch = value.match(/\b(\d+[A-Z]?(?:\.\d+)*)\b/i); - const parenName = value.match(/\(([^)]+)\)/); - const dashName = value.match(/—\s*([^(\n]+?)(?:\s*\(|$)/); + // #2124 review: length-bound the name quantifiers so a crafted long + // unterminated `(` / `—` run in an untrusted STATE.md field cannot drive + // O(n^2) backtracking (CPU DoS). (Phase 2 / #2125 supersedes this function + // by delegating to phase-id.cts:parsePhaseFromProse, which is bounded too.) + const parenName = value.match(/\(([^)]{1,200})\)/); + const dashName = value.match(/—\s*([^(\n]{1,200}?)(?:\s*\(|$)/); const rawName = parenName?.[1] ?? dashName?.[1] ?? null; const name = rawName && !/^(?:complete|executing|not started)$/i.test(rawName.trim()) ? rawName.trim() diff --git a/tests/phase-id.test.cjs b/tests/phase-id.test.cjs index e56f1d56c..286cf7a0c 100644 --- a/tests/phase-id.test.cjs +++ b/tests/phase-id.test.cjs @@ -494,6 +494,22 @@ describe('parsePhaseFromProse', () => { assert.deepEqual(phaseId.parsePhaseFromProse('3A — Delta (executing)'), { phase: '3A', name: null }); assert.equal(phaseId.parsePhaseFromProse('3 (complete)').name, null); }); + + test('#2124 review: name quantifiers are length-bounded (ReDoS guard)', () => { + // A parenthetical within the bound extracts; one longer than the bound is + // NOT matched — the cap is what prevents O(n^2) backtracking on a crafted + // untrusted value. Removing the bound would extract the long name → fail. + assert.equal(phaseId.parsePhaseFromProse('3 (Delta)').name, 'Delta'); + assert.equal(phaseId.parsePhaseFromProse(`3 (${'x'.repeat(201)})`).name, null); + // A long unterminated "(" run yields no name and still parses the phase. + assert.deepEqual(phaseId.parsePhaseFromProse(`3 ${'('.repeat(5000)}`), { phase: '3', name: null }); + }); + + test('#2124 review: non-string input is coerced, never throws', () => { + assert.doesNotThrow(() => phaseId.parsePhaseFromProse(3)); + assert.equal(phaseId.parsePhaseFromProse(3).phase, '3'); + assert.deepEqual(phaseId.parsePhaseFromProse(true), { phase: null, name: null }); + }); }); // ─── stripConfiguredProjectCodePrefix (#2121 / #2104, config-aware) ───────────