From 64bc96918adcec1e63e136ebfbc0e13697057ba7 Mon Sep 17 00:00:00 2001 From: oktofeesh1 <287075021+oktofeesh1@users.noreply.github.com> Date: Sat, 27 Jun 2026 07:13:54 -0700 Subject: [PATCH 1/2] feat(enrichment): ReDoS scanner on added/changed regex Adds a REES analyzer that flags regex literals introduced by the PR (added diff lines) vulnerable to catastrophic backtracking: a group quantified by an unbounded quantifier (+, *, {n,}) whose body also contains an unbounded quantifier (the classic (a+)+ / (\w+\.)+ shape that turns attacker-controlled input into a DoS). This is the structural analysis the no-checkout in-prompt reviewer cannot do; the brief block is spliced into the review (additive, fail-safe). Self-contained and pure-CPU: no network and no new runtime dependency. A structural detector (not a bundled recheck/redos-detector binary), so no Dockerfile/runtime-image change. Structural-only keeps precision high: linear (abc)+, bounded (a+){2,4}, and non-quantified (a|b)+ are not flagged. Closes #1503. --- review-enrichment/src/analyzers/redos.ts | 161 ++++++++++++++++++++++ review-enrichment/src/brief.ts | 2 + review-enrichment/src/render.ts | 12 ++ review-enrichment/src/types.ts | 10 ++ review-enrichment/test/enrichment.test.ts | 99 +++++++++++++ 5 files changed, 284 insertions(+) create mode 100644 review-enrichment/src/analyzers/redos.ts diff --git a/review-enrichment/src/analyzers/redos.ts b/review-enrichment/src/analyzers/redos.ts new file mode 100644 index 0000000000..a234ab7118 --- /dev/null +++ b/review-enrichment/src/analyzers/redos.ts @@ -0,0 +1,161 @@ +// ReDoS analyzer (#1503). Flags regex literals INTRODUCED by the PR (added `+` diff lines) that are vulnerable to +// catastrophic backtracking — a group quantified by an unbounded quantifier (`+`, `*`, `{n,}`) whose body ALSO +// contains an unbounded quantifier: the classic `(a+)+` shape that turns attacker-controlled input into a DoS. +// Pure compute, no network, no external detector (structural-only → high precision: linear shapes like `(abc)+` +// are NOT flagged). Line-cited via hunk headers, mirroring the actions-pin analyzer. +import type { EnrichRequest, RedosFinding } from "../types.js"; + +// Every loop runs over an attacker-controlled patch, so each is bounded. +const MAX_FINDINGS = 25; // keep the brief bounded +const MAX_PATTERN_CHARS = 1000; // ignore absurdly long literals (a hand-written regex is never this long) +const MAX_LINE_CHARS = 2000; // skip extraction on pathologically long lines (defensive) +const REPORT_CHARS = 80; // truncate the reported pattern so the brief stays readable + +// A `/.../flags` literal in regex position (line start or an operator/punctuation that cannot begin a division), +// OR a `new RegExp("…")` / `RegExp('…')` constructor argument. The structural check below filters non-ReDoS noise, +// so a slightly-loose extraction here cannot, on its own, produce a false ReDoS finding. Both patterns use only +// non-overlapping alternations + negated classes, so they are themselves linear-time (no self-ReDoS). +const LITERAL_RE = + /(?:^|[=(,:?&|!{[;\s])\/((?:\\.|\[(?:\\.|[^\]\\])*\]|[^/\\\n])+)\/[a-z]*/g; +const CTOR_RE = /\bRegExp\s*\(\s*(['"`])((?:\\.|(?!\1)[\s\S])*?)\1/g; + +/** Extract candidate regex SOURCES from one line of added code (`/.../` literals + `RegExp(...)` string args). */ +export function extractRegexSources(line: string): string[] { + const sources: string[] = []; + if (line.length > MAX_LINE_CHARS) return sources; + LITERAL_RE.lastIndex = 0; + for (let m: RegExpExecArray | null; (m = LITERAL_RE.exec(line)); ) { + if (m[1] && m[1].length <= MAX_PATTERN_CHARS) sources.push(m[1]); + } + CTOR_RE.lastIndex = 0; + for (let m: RegExpExecArray | null; (m = CTOR_RE.exec(line)); ) { + if (m[2] && m[2].length <= MAX_PATTERN_CHARS) sources.push(m[2]); + } + return sources; +} + +// An unbounded quantifier (`+`, `*`, or `{n,}`) at index `i`? `{n}` and `{n,m}` are bounded and ignored. +function unboundedQuantifierAt(p: string, i: number): boolean { + const c = p[i]; + if (c === "+" || c === "*") return true; + if (c === "{") return /^\{\d*,\}/.test(p.slice(i)); + return false; +} + +// Does the group body p[open+1 .. close-1] contain an unbounded quantifier (ignoring escapes + char classes, +// inside which `+`/`*` are literal)? +function bodyHasUnboundedQuantifier( + p: string, + open: number, + close: number, +): boolean { + let i = open + 1; + let inClass = false; + while (i < close) { + const c = p[i]; + if (c === "\\") { + i += 2; + continue; + } + if (inClass) { + if (c === "]") inClass = false; + i++; + continue; + } + if (c === "[") { + inClass = true; + i++; + continue; + } + if (unboundedQuantifierAt(p, i)) return true; + i++; + } + return false; +} + +/** Catastrophic-backtracking detector: a group `(…)` quantified by an unbounded quantifier whose body ALSO + * contains an unbounded quantifier — `(a+)+`, `(\d+)*`, `(.*)+`, … Returns false for linear shapes like `(abc)+`. */ +export function hasCatastrophicBacktracking(pattern: string): boolean { + const openStack: number[] = []; + let i = 0; + let inClass = false; + while (i < pattern.length) { + const c = pattern[i]; + if (c === "\\") { + i += 2; + continue; + } + if (inClass) { + if (c === "]") inClass = false; + i++; + continue; + } + if (c === "[") { + inClass = true; + i++; + continue; + } + if (c === "(") { + openStack.push(i); + i++; + continue; + } + if (c === ")") { + const open = openStack.pop(); + if ( + open !== undefined && + unboundedQuantifierAt(pattern, i + 1) && + bodyHasUnboundedQuantifier(pattern, open, i) + ) { + return true; + } + i++; + continue; + } + i++; + } + return false; +} + +/** Scan one file patch's added lines for ReDoS-prone regex literals, line-cited via hunk headers. Pure. */ +export function scanPatchForRedos(path: string, patch: string): RedosFinding[] { + const findings: RedosFinding[] = []; + let newLine = 0; + for (const line of patch.split("\n")) { + if (line.startsWith("+++") || line.startsWith("---")) continue; + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (hunk) { + newLine = Number(hunk[1]); + continue; + } + if (line.startsWith("+")) { + for (const source of extractRegexSources(line.slice(1))) { + if (hasCatastrophicBacktracking(source)) { + findings.push({ + file: path, + line: newLine, + kind: "nested-quantifier", + pattern: source.slice(0, REPORT_CHARS), + }); + } + } + newLine++; + } else if (!line.startsWith("-")) { + newLine++; + } + } + return findings; +} + +/** Analyzer entrypoint: scan every changed file's added lines for ReDoS-prone regex literals. */ +export async function scanRedos(req: EnrichRequest): Promise { + const findings: RedosFinding[] = []; + for (const file of req.files ?? []) { + if (!file.patch) continue; + for (const finding of scanPatchForRedos(file.path, file.patch)) { + findings.push(finding); + if (findings.length >= MAX_FINDINGS) return findings; + } + } + return findings; +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts index 2ac176149a..8313780e6e 100644 --- a/review-enrichment/src/brief.ts +++ b/review-enrichment/src/brief.ts @@ -13,6 +13,7 @@ import { scanLicenses } from "./analyzers/license-check.js"; import { scanInstallScripts } from "./analyzers/install-scripts.js"; import { scanActionPins } from "./analyzers/actions-pin.js"; import { scanEol } from "./analyzers/eol-check.js"; +import { scanRedos } from "./analyzers/redos.js"; import { renderBrief } from "./render.js"; type AnalyzerFn = (req: EnrichRequest, signal: AbortSignal) => Promise; @@ -25,6 +26,7 @@ const ANALYZERS: Record = { installScript: (req) => scanInstallScripts(req), actionPin: (req) => scanActionPins(req), eol: (req) => scanEol(req), + redos: (req) => scanRedos(req), }; function runWithTimeout( diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index f026b7d63d..848a2f67de 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -117,6 +117,18 @@ export function renderBrief( } } + const redos = findings.redos ?? []; + if (redos.length) { + lines.push( + "### ReDoS-prone regex (catastrophic backtracking — DoS on attacker-controlled input)", + ); + for (const item of redos) { + lines.push( + `- ${safeCodeSpan(`${item.file}:${item.line}`)} — ${safeCodeSpan(item.pattern)} nests an unbounded quantifier inside an unbounded-quantified group; bound the repetition or rewrite without nesting`, + ); + } + } + if (!lines.length) return { promptSection: "", systemSuffix: "" }; const header = diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 785650f7b0..ae893825ca 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -84,6 +84,15 @@ export interface EolFinding { status: "eol" | "soon"; } +/** A regex literal introduced by the PR that is vulnerable to catastrophic backtracking (ReDoS). Reports the + * location + the (truncated) vulnerable pattern only — never any matched value. */ +export interface RedosFinding { + file: string; + line: number; + kind: "nested-quantifier"; + pattern: string; +} + /** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1477/#1478). */ export interface BriefFindings { dependency?: DependencyFinding[]; @@ -92,6 +101,7 @@ export interface BriefFindings { actionPin?: ActionPinFinding[]; installScript?: InstallScriptFinding[]; eol?: EolFinding[]; + redos?: RedosFinding[]; } export type AnalyzerStatus = "ok" | "degraded" | "skipped"; diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index cd80458637..7eeb3d90b8 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -15,6 +15,11 @@ import { scanActionPins, } from "../dist/analyzers/actions-pin.js"; import { scanEol, extractVersionPins } from "../dist/analyzers/eol-check.js"; +import { + hasCatastrophicBacktracking, + scanPatchForRedos, + scanRedos, +} from "../dist/analyzers/redos.js"; const NOW = new Date("2026-06-26").getTime(); const eolFetch = @@ -597,6 +602,100 @@ test("buildBrief: action-pin analyzer runs (pure, no network)", async () => { } }); +test("hasCatastrophicBacktracking: flags nested unbounded quantifiers, not linear/bounded shapes", () => { + for (const vuln of [ + "(a+)+", + "(a*)*", + "(a+)*", + "(.*)+", + "(\\d+){2,}", + "([a-z]+)+", + "((ab)+)+", + ]) { + assert.equal(hasCatastrophicBacktracking(vuln), true, vuln); + } + for (const safe of [ + "(abc)+", + "[a-z]+", + "(a+)?", + "(a+){2,4}", + "abc", + "(a|b)+", + "\\(a+\\)+", + ]) { + assert.equal(hasCatastrophicBacktracking(safe), false, safe); + } +}); + +test("scanPatchForRedos: flags added ReDoS literals + RegExp(...) ctors, line-cited; ignores context + safe regex", () => { + const patch = [ + "@@ -1,1 +1,4 @@", + " const ok = /(abc)+/;", + "+const bad = /(a+)+$/;", + "+const safe = /[a-z]+/;", + '+const ctor = new RegExp("(\\\\d+)*x");', + ].join("\n"); + const findings = scanPatchForRedos("src/x.ts", patch); + assert.deepEqual( + findings.map(({ file, line, kind }) => ({ file, line, kind })), + [ + { file: "src/x.ts", line: 2, kind: "nested-quantifier" }, + { file: "src/x.ts", line: 4, kind: "nested-quantifier" }, + ], + ); + assert.equal(findings[0].pattern, "(a+)+$"); +}); + +test("scanRedos: scans every changed file's added lines, caps to its budget", async () => { + const findings = await scanRedos({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { path: "src/a.ts", patch: "@@ -1,0 +1,1 @@\n+const r = /(x+)+/;" }, + { path: "src/b.ts", patch: "@@ -1,0 +1,1 @@\n+const r = /[0-9]+/;" }, + { path: "README.md", patch: undefined }, + ], + }); + assert.equal(findings.length, 1); + assert.equal(findings[0].file, "src/a.ts"); +}); + +test("renderBrief: renders the ReDoS block, code-spanning + sanitizing the pattern", () => { + const r = renderBrief({ + redos: [ + { + file: "src/re.ts", + line: 7, + kind: "nested-quantifier", + pattern: "(a+)+", + }, + ], + }); + assert.match(r.promptSection, /ReDoS-prone regex/); + assert.match(r.promptSection, /`src\/re\.ts:7`/); + assert.match(r.promptSection, /`\(a\+\)\+/); + assert.doesNotMatch(r.promptSection, //); // control char in the pattern is neutralized +}); + +test("buildBrief: ReDoS analyzer runs (pure, no network)", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async () => ({ ok: true, json: async () => ({}) }); + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { path: "src/x.ts", patch: "@@ -1,0 +1,1 @@\n+const r = /(a+)+$/;" }, + ], + }); + assert.equal(brief.analyzerStatus.redos, "ok"); + assert.equal(brief.findings.redos.length, 1); + assert.match(brief.promptSection, /ReDoS-prone regex/); + } finally { + globalThis.fetch = realFetch; + } +}); + test("extractDependencyChanges: caps manifest files and patch lines", () => { const changes = extractDependencyChanges( [ From c6681ba8434575edcf7a6c360d8ccf6922b49f92 Mon Sep 17 00:00:00 2001 From: oktofeesh1 <287075021+oktofeesh1@users.noreply.github.com> Date: Sat, 27 Jun 2026 07:41:17 -0700 Subject: [PATCH 2/2] fix(enrichment): make the ReDoS regex extractor a linear scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The LITERAL_RE / CTOR_RE extractors were themselves vulnerable to catastrophic backtracking: their alternations overlap (the char-class branch and the single-char fallback both match '[' / ']'), so adversarial diff input such as many empty '[]' classes with no closing slash forced exponential backtracking — the line-length cap does not bound 2^n. Replace both regexes with a single linear character scan (escapes and '[...]' classes transparent to the closing '/', plus a RegExp(...) string-arg reader), so the extractor visits each char once and can never be the DoS it exists to detect. Extraction semantics are preserved; added a regression test feeding the adversarial char-class input. --- review-enrichment/src/analyzers/redos.ts | 137 +++++++++++++++++++--- review-enrichment/test/enrichment.test.ts | 15 +++ 2 files changed, 137 insertions(+), 15 deletions(-) diff --git a/review-enrichment/src/analyzers/redos.ts b/review-enrichment/src/analyzers/redos.ts index a234ab7118..b919f58e4a 100644 --- a/review-enrichment/src/analyzers/redos.ts +++ b/review-enrichment/src/analyzers/redos.ts @@ -11,25 +11,132 @@ const MAX_PATTERN_CHARS = 1000; // ignore absurdly long literals (a hand-written const MAX_LINE_CHARS = 2000; // skip extraction on pathologically long lines (defensive) const REPORT_CHARS = 80; // truncate the reported pattern so the brief stays readable -// A `/.../flags` literal in regex position (line start or an operator/punctuation that cannot begin a division), -// OR a `new RegExp("…")` / `RegExp('…')` constructor argument. The structural check below filters non-ReDoS noise, -// so a slightly-loose extraction here cannot, on its own, produce a false ReDoS finding. Both patterns use only -// non-overlapping alternations + negated classes, so they are themselves linear-time (no self-ReDoS). -const LITERAL_RE = - /(?:^|[=(,:?&|!{[;\s])\/((?:\\.|\[(?:\\.|[^\]\\])*\]|[^/\\\n])+)\/[a-z]*/g; -const CTOR_RE = /\bRegExp\s*\(\s*(['"`])((?:\\.|(?!\1)[\s\S])*?)\1/g; +// Extraction runs as a single LINEAR left-to-right scan — deliberately NOT a regex. A regex with the alternation +// needed here (escapes | char-classes | other chars, all under `+`) has overlapping branches and would itself +// backtrack catastrophically on adversarial diff input (e.g. many empty `[]` classes with no closing `/`). The +// hand scan visits each char once, so the extractor can never be the DoS it exists to detect. Two shapes: +// - a `/.../flags` literal in regex position (line start, or after a punctuator that cannot end an operand, so +// `a / b` division is not mistaken for a regex); +// - a `new RegExp("…")` / `RegExp('…')` / RegExp(`…`) constructor's string argument. +// A slightly-loose extraction is fine: the structural check below is what decides ReDoS, so over-extraction can +// never produce a false finding on its own. + +// `/` opens a regex literal (not division) when the char just before it is a statement/operator boundary. +const REGEX_POSITION_PREFIX = "=(,:?&|!{[;"; + +function isWordChar(ch: string): boolean { + return ( + (ch >= "a" && ch <= "z") || + (ch >= "A" && ch <= "Z") || + (ch >= "0" && ch <= "9") || + ch === "_" || + ch === "$" + ); +} + +function isRegexPosition(line: string, slash: number): boolean { + if (slash === 0) return true; + const before = line[slash - 1]!; + return ( + before === " " || before === "\t" || REGEX_POSITION_PREFIX.includes(before) + ); +} + +// From the opening `/` at `open`, linearly consume the literal body (escapes and `[...]` classes are transparent +// to the closing `/`) plus any trailing flags. Returns the body + index past the literal, or null if unterminated. +function scanRegexLiteral( + line: string, + open: number, +): { body: string; end: number } | null { + const n = line.length; + const bodyStart = open + 1; + let i = bodyStart; + let inClass = false; + while (i < n) { + const ch = line[i]!; + if (ch === "\\") { + if (i + 1 >= n) return null; + i += 2; + continue; + } + if (ch === "\n") return null; + if (inClass) { + if (ch === "]") inClass = false; + i++; + continue; + } + if (ch === "[") { + inClass = true; + i++; + continue; + } + if (ch === "/") { + if (i === bodyStart) return null; // empty body — `//` is a comment, not a regex literal + let j = i + 1; + while (j < n && line[j]! >= "a" && line[j]! <= "z") j++; + return { body: line.slice(bodyStart, i), end: j }; + } + i++; + } + return null; +} + +// `RegExp` is expected at `i` (word boundary checked by the caller). Linearly read its first string argument. +// Returns the raw string source + index past the closing quote, or null if it is not a string-literal ctor call. +function scanRegExpCtorArg( + line: string, + i: number, +): { body: string; end: number } | null { + const n = line.length; + let j = i + "RegExp".length; + while (j < n && (line[j] === " " || line[j] === "\t")) j++; + if (line[j] !== "(") return null; + j++; + while (j < n && (line[j] === " " || line[j] === "\t")) j++; + const quote = line[j]; + if (quote !== '"' && quote !== "'" && quote !== "`") return null; + const bodyStart = ++j; + while (j < n) { + const ch = line[j]!; + if (ch === "\\") { + if (j + 1 >= n) return null; + j += 2; + continue; + } + if (ch === quote) return { body: line.slice(bodyStart, j), end: j + 1 }; + j++; + } + return null; +} /** Extract candidate regex SOURCES from one line of added code (`/.../` literals + `RegExp(...)` string args). */ export function extractRegexSources(line: string): string[] { const sources: string[] = []; - if (line.length > MAX_LINE_CHARS) return sources; - LITERAL_RE.lastIndex = 0; - for (let m: RegExpExecArray | null; (m = LITERAL_RE.exec(line)); ) { - if (m[1] && m[1].length <= MAX_PATTERN_CHARS) sources.push(m[1]); - } - CTOR_RE.lastIndex = 0; - for (let m: RegExpExecArray | null; (m = CTOR_RE.exec(line)); ) { - if (m[2] && m[2].length <= MAX_PATTERN_CHARS) sources.push(m[2]); + const n = line.length; + if (n > MAX_LINE_CHARS) return sources; + let i = 0; + while (i < n) { + const c = line[i]!; + if (c === "/" && isRegexPosition(line, i)) { + const lit = scanRegexLiteral(line, i); + if (lit) { + if (lit.body.length <= MAX_PATTERN_CHARS) sources.push(lit.body); + i = lit.end; + continue; + } + } else if ( + c === "R" && + (i === 0 || !isWordChar(line[i - 1]!)) && + line.startsWith("RegExp", i) + ) { + const ctor = scanRegExpCtorArg(line, i); + if (ctor) { + if (ctor.body.length <= MAX_PATTERN_CHARS) sources.push(ctor.body); + i = ctor.end; + continue; + } + } + i++; } return sources; } diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts index 7eeb3d90b8..6f189d1143 100644 --- a/review-enrichment/test/enrichment.test.ts +++ b/review-enrichment/test/enrichment.test.ts @@ -16,6 +16,7 @@ import { } from "../dist/analyzers/actions-pin.js"; import { scanEol, extractVersionPins } from "../dist/analyzers/eol-check.js"; import { + extractRegexSources, hasCatastrophicBacktracking, scanPatchForRedos, scanRedos, @@ -627,6 +628,20 @@ test("hasCatastrophicBacktracking: flags nested unbounded quantifiers, not linea } }); +test("extractRegexSources: linear scan, no catastrophic backtracking on adversarial char classes (#1503 regression)", () => { + // The former LITERAL_RE extractor backtracked exponentially on many empty `[]` classes with no closing slash; + // the linear scanner returns immediately (a regression would hang this test). + const adversarial = "x = /" + "[]".repeat(800); + assert.deepEqual(extractRegexSources(adversarial), []); + // Well-formed literals (incl. char classes + flags) and RegExp() ctors still extract correctly: + assert.deepEqual(extractRegexSources("const r = /[a-z][0-9]+/g;"), [ + "[a-z][0-9]+", + ]); + assert.deepEqual(extractRegexSources('new RegExp("(a+)+")'), ["(a+)+"]); + // `a / b` division is not mistaken for a regex literal: + assert.deepEqual(extractRegexSources("const n = a / b;"), []); +}); + test("scanPatchForRedos: flags added ReDoS literals + RegExp(...) ctors, line-cited; ignores context + safe regex", () => { const patch = [ "@@ -1,1 +1,4 @@",