fix(#2124): harden parsePhaseFromProse per orthogonal review (ReDoS + coercion)
Orthogonal security review of the Phase 1 surface found two issues; both fixed
and regression-tested:
- MEDIUM ReDoS: the name-extraction regexes /\(([^)]+)\)/ and
/—\s*([^(\n]+?).../ backtrack O(n^2) on a crafted STATE.md field value with a
long unterminated "(" / "—" run (reviewer measured ~38s at 320k chars).
Length-bound both quantifiers to {1,200} -> linear (320k now ~100ms). A real
phase name is far shorter than the cap.
- LOW: parsePhaseFromProse threw on non-string truthy input, unlike its three
sibling #2121 functions. Coerce via String(value) up front.
The identical ReDoS regexes are copied verbatim from the pre-existing
state.cts:parseProsePhaseField; per the no-defer rule that surfaced defect is
fixed inline there too (Phase 2 / #2125 later supersedes that function by
delegating to the bounded phase-id.cts parser).
Adds a behavioral bound-guard regression test (a >200-char parenthetical is not
extracted) and a non-string-coercion test.
Refs #2124, #2121
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -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()
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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) ───────────
|
||||
|
||||
Reference in New Issue
Block a user