diff --git a/.changeset/phase-id-redos-hardening.md b/.changeset/phase-id-redos-hardening.md new file mode 100644 index 000000000..816a004a6 --- /dev/null +++ b/.changeset/phase-id-redos-hardening.md @@ -0,0 +1,5 @@ +--- +type: Security +pr: 0 +--- +**Hardened phase/roadmap/plan markdown parsing against quadratic-time (ReDoS) CPU exhaustion** — a crafted `ROADMAP.md`, `STATE.md`, or `PLAN.md` with large runs of unclosed `(`, `[`, ``, `|$)/g, ' '); + // Stop-at-next-open body (ReDoS-safe, #2128); an UNCLOSED `|$)` fallback, which would + // wipe to EOF and fail-close the decision-coverage gate). + const htmlStripped = text.replace(//g, ' '); // Fenced-code stripping: delegate to the canonical CommonMark-correct seam. // replaces the prior independent regex copy (```` ``` ``` ```` + `~~~ ~~~`). return stripFencedCode(htmlStripped).text; diff --git a/src/markdown-sectionizer.cts b/src/markdown-sectionizer.cts index c152f8acc..c9b0727c6 100644 --- a/src/markdown-sectionizer.cts +++ b/src/markdown-sectionizer.cts @@ -520,22 +520,25 @@ export function iterateBullets(sectionText: string): BulletItem[] { * fenced code blocks itself. If a `` block appears inside a fenced code * block and should be excluded, the caller should apply `stripFencedCode` first. * - * **Nested tags are NOT supported.** The underlying regex uses a non-greedy - * `[\s\S]*?` match, which means it closes at the FIRST `` encountered. - * Given `inner`, `extractTaggedBlocks(content, 'x')` returns - * `['inner']` — the inner `` is captured as literal text, and the second - * `` is left unmatched (or matched as a second block with empty inner text - * if another `` follows). Callers that need to handle nested tags must - * pre-process the input or use a proper XML/HTML parser. + * **Nested tags are NOT supported.** The body scan terminates at the NEXT + * opening of the same tag (the ReDoS-safe boundary, #2128). Given + * `inner`, `extractTaggedBlocks(content, 'x')` returns `['inner']` + * — the well-formed inner block; the unterminated outer `` is skipped. + * Callers that need true nesting must use a proper XML/HTML parser. + * + * `allowAttributes` (default `false`): when `true`, the opening tag may carry + * bounded attributes (``) — needed for `` blocks. + * Leave `false` for tags that must match exactly (e.g. ``), and never + * enable it for a tag where an attributed form is semantically distinct. * * Generalises `decisions.cts`'s bespoke `matchAll(/([\s\S]*?)<\/decisions>/g)` * so tier T1 can drop its own copy (tracked duplication until T1 lands). */ -export function extractTaggedBlocks(content: string, tagName: string): string[] { +export function extractTaggedBlocks(content: string, tagName: string, allowAttributes = false): string[] { if (typeof content !== 'string' || content.length === 0) return []; if (typeof tagName !== 'string' || tagName.length === 0) return []; - const pattern = taggedBlockPattern(tagName, 'g'); + const pattern = taggedBlockPattern(tagName, 'g', allowAttributes); const results: string[] = []; let match: RegExpExecArray | null; while ((match = pattern.exec(content)) !== null) { @@ -548,30 +551,37 @@ export function extractTaggedBlocks(content: string, tagName: string): string[] * Build the single, ReDoS-safe `…` block regex shared by * `extractTaggedBlocks` (extract bodies) and `stripTaggedBlocks` (remove blocks). * - * Safety: the body uses a `(?:(?!])[\s\S])*?` negative-lookahead scan - * that terminates at the NEXT opening `` instead of lazily rescanning the - * whole remaining document for a `` that may never appear — so a large - * document full of unclosed `` openings stays LINEAR, not quadratic - * (#2128). The opening tag tolerates optional attributes (``), - * bounded to 1000 chars so the attribute scan cannot itself ReDoS. - * Group 1 is the block body. + * Safety: the body terminates at the NEXT opening of this tag (stop-at-next-open) + * instead of lazily rescanning the whole remaining document for a `` that + * may never appear — so a document full of unclosed `` openings scans + * LINEARLY, not quadratically (#2128). Group 1 is the block body. + * + * `allowAttributes`: when `true`, the opener accepts bounded attributes + * (``) and the body boundary is ``. + * When `false`, the opener is the EXACT `` and the boundary is exact ``, + * so an attributed `` is neither an opener nor a boundary — it is body + * content. That exact form is load-bearing for `
` stripping: `
` marks the ACTIVE milestone and must be preserved, not stripped (#557). */ -function taggedBlockPattern(tagName: string, flags: string): RegExp { +function taggedBlockPattern(tagName: string, flags: string, allowAttributes: boolean): RegExp { const esc = tagName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - return new RegExp(`<${esc}(?:\\s[^>]{0,1000})?>((?:(?!<${esc}[\\s>])[\\s\\S])*?)`, flags); + const open = allowAttributes ? `<${esc}(?:\\s[^>]{0,1000})?>` : `<${esc}>`; + const boundary = allowAttributes ? `<${esc}[\\s>]` : `<${esc}>`; + return new RegExp(`${open}((?:(?!${boundary})[\\s\\S])*?)`, flags); } /** * Remove every `…` block (opening tag, body, and closing tag) * from `content`. The ReDoS-safe counterpart to `extractTaggedBlocks` — same - * hardened pattern, `.replace(…, '')` instead of body extraction. Case-insensitive - * by default (matching the `
` strip call sites); pass `caseSensitive` - * to force exact-case matching. + * hardened pattern, `.replace(…, '')` instead of body extraction. `allowAttributes` + * defaults to `false` so `
` (active milestone) is preserved (#557); + * case-insensitive by default (matching the `
` strip call sites), pass + * `caseSensitive` to force exact-case matching. */ -export function stripTaggedBlocks(content: string, tagName: string, caseSensitive = false): string { +export function stripTaggedBlocks(content: string, tagName: string, allowAttributes = false, caseSensitive = false): string { if (typeof content !== 'string' || content.length === 0) return ''; if (typeof tagName !== 'string' || tagName.length === 0) return content; - return content.replace(taggedBlockPattern(tagName, caseSensitive ? 'g' : 'gi'), ''); + return content.replace(taggedBlockPattern(tagName, caseSensitive ? 'g' : 'gi', allowAttributes), ''); } // ─── replaceSection ─────────────────────────────────────────────────────────── diff --git a/src/verify.cts b/src/verify.cts index 8aa3a4778..4695fd5aa 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -206,7 +206,15 @@ function scanNegativeGrepCommentEcho(content: string): { errors: string[]; warni // while a prose echo on the same line is still caught. const cmdSpanRe = /grep(?:\s+-{1,2}[A-Za-z][A-Za-z-]*)+\s+(?:'[^']*'|"[^"]*"|[^\s'"|>&;]+)[^\n]*?(?:==|-eq|=)\s*0\b/g; - const actionZones = extractTaggedBlocks(text, 'action'); + // Security scan: must see the FULL text up to the first — including a + // malformed inner — so a grep-echo-0 trick cannot hide behind a + // deliberately-unclosed tag. Use a bounded to-first-close scan (ReDoS-safe via + // the {0,20000} cap, #2128), NOT the stop-at-next-open extractTaggedBlocks seam + // (which would drop the span before an unterminated inner ). + const actionZones: string[] = []; + const actionRe = /([\s\S]{0,20000}?)<\/action>/g; + let acm: RegExpExecArray | null; + while ((acm = actionRe.exec(text)) !== null) actionZones.push(acm[1]); const scannableActionText = actionZones.map((zone) => zone.replace(cmdSpanRe, ' ')).join('\n'); // 3. Per shell SEGMENT (split lines on && / ||) extract count-grep literals and @@ -363,7 +371,7 @@ function scanFileWideNegativeGateConflict(content: string): { warnings: string[] reqText: string; // + text (requirement side) } const tasks: TaskInfo[] = []; - for (const tc of extractTaggedBlocks(text, 'task')) { + for (const tc of extractTaggedBlocks(text, 'task', true)) { // Extract task name. const namem = extractTaggedBlocks(tc, 'name'); const name = namem.length ? namem[0].trim() : 'unnamed'; @@ -565,7 +573,7 @@ function cmdVerifyPlanStructure(cwd: string, filePath: string, raw: boolean): vo } const tasks: Record[] = []; - for (const taskContent of extractTaggedBlocks(content, 'task')) { + for (const taskContent of extractTaggedBlocks(content, 'task', true)) { const nameArr = extractTaggedBlocks(taskContent, 'name'); const taskName = nameArr.length ? nameArr[0].trim() : 'unnamed'; const hasFiles = //.test(taskContent); diff --git a/tests/markdown-sectionizer.test.cjs b/tests/markdown-sectionizer.test.cjs index ac934f1f7..655b839b3 100644 --- a/tests/markdown-sectionizer.test.cjs +++ b/tests/markdown-sectionizer.test.cjs @@ -33,6 +33,7 @@ const { collectSection, iterateBullets, extractTaggedBlocks, + stripTaggedBlocks, replaceSection, } = require('../gsd-core/bin/lib/markdown-sectionizer.cjs'); @@ -1015,15 +1016,14 @@ describe('stripFencedCode and tokenizeHeadings: backtick info string with backti // ─── FIX 6: extractTaggedBlocks — nested tag behavior ───────────────────────── -describe('extractTaggedBlocks: nested same-name tag behavior (non-greedy limitation)', () => { - test('nested … closes at first (non-greedy; nested tags not supported)', () => { - // Non-greedy match: ([\s\S]*?) closes at the FIRST . - // So inner → first block captures "inner", second is unmatched. +describe('extractTaggedBlocks: nested same-name tag behavior (#2128 stop-at-next-open)', () => { + test('nested inner extracts the well-formed inner block', () => { + // #2128: the ReDoS-safe body scan terminates at the NEXT opening , so the + // unterminated outer is skipped and the inner block is extracted. const content = 'inner'; const result = extractTaggedBlocks(content, 'x'); - // The first match closes at the first , capturing "inner" - assert.equal(result.length, 1, 'non-greedy match produces exactly one result from nested input'); - assert.equal(result[0], 'inner', 'inner capture is the content up to the first closing tag'); + assert.equal(result.length, 1, 'exactly one result from nested input'); + assert.equal(result[0], 'inner', 'the well-formed inner block is extracted; the unterminated outer is skipped'); }); test('back-to-back blocks (not nested) are both extracted', () => { @@ -1033,6 +1033,24 @@ describe('extractTaggedBlocks: nested same-name tag behavior (non-greedy limitat assert.equal(result[0], 'first'); assert.equal(result[1], 'second'); }); + + test('#2128: a document full of unclosed openings stays linear and yields no match', () => { + const content = 'a\n'.repeat(50) + 'no closing tag'; + assert.deepEqual(extractTaggedBlocks(content, 'x'), [], 'no anywhere -> no blocks'); + }); + + test('#557 / #2128: attr-intolerant by default preserves
; opt-in matches ', () => { + // stripTaggedBlocks(details) must PRESERVE
(the active-milestone + // marker) and strip only bare
; extractTaggedBlocks(task, true) must + // match attributed tasks, and must NOT when allowAttributes is left false. + assert.equal( + stripTaggedBlocks('X
shipped
Y
active
Z', 'details'), + 'XY
active
Z', + '#557:
preserved; bare
stripped', + ); + assert.deepEqual(extractTaggedBlocks('body', 'task', true), ['body'], 'attributed task matched with allowAttributes=true'); + assert.deepEqual(extractTaggedBlocks('body', 'task'), [], 'attributed task NOT matched with allowAttributes=false'); + }); }); // Parity guard removed in T5 (ADR-1372): uat-predicate now imports stripFencedCode diff --git a/tests/roadmap-parser.test.cjs b/tests/roadmap-parser.test.cjs index f86b8fcc4..ef6789899 100644 --- a/tests/roadmap-parser.test.cjs +++ b/tests/roadmap-parser.test.cjs @@ -77,6 +77,19 @@ describe('roadmap-parser: stripShippedMilestones', () => { assert.ok(!result.includes('closed content'), 'content removed'); assert.ok(result.includes('after'), 'after content preserved'); }); + + test('#557: preserves an active
block while stripping shipped bare
', () => { + //
marks the ACTIVE milestone (roadmap.analyze must still see its + // phases); only closed/shipped bare
blocks are stripped. Regression for + // #557, which the #2128 shared-seam migration briefly reintroduced via the seam's + // attribute-tolerance — the details strip is now attr-INTOLERANT to keep #557 fixed. + const input = '
\nshipped phase\n
\n
\n- [ ] **Phase 9: Active**\n
\nafter'; + const result = stripShippedMilestones(input); + assert.ok(!result.includes('shipped phase'), 'shipped bare
stripped'); + assert.ok(result.includes('
'), 'active
tag preserved'); + assert.ok(result.includes('Phase 9: Active'), 'active-milestone phases preserved'); + assert.ok(result.includes('after'), 'trailing content preserved'); + }); }); // ─── extractCurrentMilestone ──────────────────────────────────────────────────