Skip to content

feat(enrichment): ReDoS scanner on added/changed regex - #1601

Merged
JSONbored merged 2 commits into
JSONbored:mainfrom
oktofeesh1:feat/enrichment-redos-scanner
Jun 27, 2026
Merged

feat(enrichment): ReDoS scanner on added/changed regex#1601
JSONbored merged 2 commits into
JSONbored:mainfrom
oktofeesh1:feat/enrichment-redos-scanner

Conversation

@oktofeesh1

Copy link
Copy Markdown
Contributor

Closes #1503.

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 exactly the heavy/structural analysis the no-checkout in-prompt reviewer cannot do; the brief block is spliced into the review (additive + fail-safe).

Approach

Self-contained, pure-CPU — no network and no new runtime dependency. A structural detector rather than a bundled recheck/redos-detector binary, so there is no Dockerfile/runtime-image change. Structural-only keeps precision high: linear shapes like (abc)+, bounded (a+){2,4}, and non-quantified alternation (a|b)+ are not flagged.

  • Extracts regex sources from added lines: /.../flags literals in regex position + new RegExp("…") / RegExp('…') constructor args. The extractors use only non-overlapping alternations + negated classes, so they are themselves linear-time (no self-ReDoS).
  • Detects nested unbounded quantifiers by tracking group spans, escape- and char-class-aware.
  • Line-cited via hunk headers, mirroring the actions-pin analyzer. Findings are bounded (MAX_FINDINGS, per-line length guard); the reported pattern is truncated and rendered through safeCodeSpan (public-safe — no matched value echoed).

Files (all inside review-enrichment/, per the established analyzer pattern)

  • src/types.tsRedosFinding type + redos BriefFindings key
  • src/analyzers/redos.ts — the analyzer (pure helpers + scanRedos entrypoint)
  • src/brief.ts — registered in the ANALYZERS registry
  • src/render.ts — public-safe brief block
  • test/enrichment.test.ts — detector unit tests, line-cited scan, entrypoint cap, render sanitization, and a buildBrief integration test

Validation

  • npm test (build + node --test) inside review-enrichment/: 43/43 pass (5 new).
  • git diff --check clean; engine src/** untouched (outside the engine tsc/vitest/codecov scope — zero conflict).

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 JSONbored#1503.
@oktofeesh1
oktofeesh1 requested a review from JSONbored as a code owner June 27, 2026 14:14
@dosubot dosubot Bot added the size:L label Jun 27, 2026
@JSONbored JSONbored added gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. labels Jun 27, 2026
Comment thread review-enrichment/src/analyzers/redos.ts Outdated
@superagent-security superagent-security Bot added the pr:flagged PR flagged for review by security analysis. label Jun 27, 2026
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.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@superagent-security superagent-security Bot removed the pr:flagged PR flagged for review by security analysis. label Jun 27, 2026
@loopover-orb

loopover-orb Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review — safe to merge

5 files · 1 AI reviewers · no blockers · readiness 55/100 · CI green · clean

✅ Approved — safe to merge

Review summary
Clean, well-scoped analyzer addition. The linear hand-scanner in `extractRegexSources` correctly sidesteps self-ReDoS (the central design constraint), the structural `hasCatastrophicBacktracking` detector correctly handles escapes, char classes, bounded `{n,m}` quantifiers, and nested groups, and wiring through `brief.ts`/`render.ts`/`types.ts` follows the established analyzer pattern exactly. Two issues — one a meaningful false-negative class, one a subtle false-positive edge case in the `{,}` quantifier path — are worth tightening before ship.

Signal Result Evidence
Code review ✅ No blockers 1 reviewers, synthesized
Linked issue ✅ Linked #1503
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Review load ❌ 8/20 Readiness component derived from cached public PR metadata and labels; size label size:L.
Validation evidence ❌ 5/25 Cached preflight status is hold.
Open PR queue ❌ 3/10 24 open PR(s), 10 likely reviewable, 14 unlinked.
Contributor context ✅ Confirmed Gittensor contributor oktofeesh1; Gittensor profile; 891 PR(s), 8 issue(s).
Gate result ✅ Passing No configured blocker found.
Nits — 5 non-blocking
  • redos.ts:36 (`REGEX_POSITION_PREFIX`) — `isRegexPosition` returns false whenever a keyword's terminal word-char immediately precedes `/`, so `return /(a+)+/g`, `case /(\w+\.)+/`, `typeof /pat/`, and `throw /pat/` are silently skipped; those are valid regex positions and represent a meaningful false-negative class. Fix: add a backwards word-scan — if the run of word-chars ending at `slash-1` forms a JS regex-position keyword (`return`, `case`, `typeof`, `in`, `instanceof`, `void`, `delete`, `throw`, `new`, `of`), return true.
  • redos.ts:148 (`unboundedQuantifierAt`) — the inline `/^\{\d*,\}/.test(p.slice(i))` matches `{,}` (zero digits before the comma) because `\d*` allows an empty match; in non-Unicode JS `{,}` is treated as literal characters, not a quantifier, so `(a{,})+` would be falsely flagged. Change `\d*` to `\d+` (or hand-scan: consume at least one digit, then `,`, then `}`) to require a numeric lower bound.
  • test/enrichment.test.ts — no test drives `extractRegexSources` or `scanPatchForRedos` against a line of the form `return /(a+)+/g`, so the keyword false-negative above is invisible to the suite; add one assertion to surface the gap, even if it is left as a known limitation with a comment.
  • redos.ts:148 — replace the regex + slice with a hand-scan consistent with the file's own design rationale and fixing the `{,}` false positive: ```ts if (c === '{') { let k = i + 1; while (k < p.length && p[k] >= '0' && p[k] <= '9') k++; return k > i + 1 && p[k] === ',' && p[k + 1] === '}'; } ```
  • redos.ts:36 — extend `isRegexPosition` to cover keyword-ending word-chars with a bounded backwards scan: ```ts if (isWordChar(before)) { let k = slash - 1; while (k > 0 && isWordChar(line[k - 1]!)) k--; const kw = line.slice(k, slash); const REGEX_KWS = new Set(['return','case','typeof','in','instanceof','void','delete','throw','new','of']); return REGEX_KWS.has(kw); } ```
Review context
Contributor next steps
  • Review top overlaps.
  • Add scope summary.
  • Fix blocker.
  • Expect slower review.
  • Refresh registry data or choose a registered active repo.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

Clean, well-scoped analyzer addition. The linear hand-scanner in `extractRegexSources` correctly sidesteps self-ReDoS (the central design constraint), the structural `hasCatastrophicBacktracking` detector correctly handles escapes, char classes, bounded `{n,m}` quantifiers, and nested groups, and wiring through `brief.ts`/`render.ts`/`types.ts` follows the established analyzer pattern exactly. Two issues — one a meaningful false-negative class, one a subtle false-positive edge case in the `{,}` quantifier path — are worth tightening before ship.

Nits (5)

  • redos.ts:36 (`REGEX_POSITION_PREFIX`) — `isRegexPosition` returns false whenever a keyword's terminal word-char immediately precedes `/`, so `return /(a+)+/g`, `case /(\w+\.)+/`, `typeof /pat/`, and `throw /pat/` are silently skipped; those are valid regex positions and represent a meaningful false-negative class. Fix: add a backwards word-scan — if the run of word-chars ending at `slash-1` forms a JS regex-position keyword (`return`, `case`, `typeof`, `in`, `instanceof`, `void`, `delete`, `throw`, `new`, `of`), return true.
  • redos.ts:148 (`unboundedQuantifierAt`) — the inline `/^\{\d*,\}/.test(p.slice(i))` matches `{,}` (zero digits before the comma) because `\d*` allows an empty match; in non-Unicode JS `{,}` is treated as literal characters, not a quantifier, so `(a{,})+` would be falsely flagged. Change `\d*` to `\d+` (or hand-scan: consume at least one digit, then `,`, then `}`) to require a numeric lower bound.
  • test/enrichment.test.ts — no test drives `extractRegexSources` or `scanPatchForRedos` against a line of the form `return /(a+)+/g`, so the keyword false-negative above is invisible to the suite; add one assertion to surface the gap, even if it is left as a known limitation with a comment.
  • redos.ts:148 — replace the regex + slice with a hand-scan consistent with the file's own design rationale and fixing the `{,}` false positive: ```ts if (c === '{') { let k = i + 1; while (k < p.length && p[k] >= '0' && p[k] <= '9') k++; return k > i + 1 && p[k] === ',' && p[k + 1] === '}'; } ```
  • redos.ts:36 — extend `isRegexPosition` to cover keyword-ending word-chars with a bounded backwards scan: ```ts if (isWordChar(before)) { let k = slash - 1; while (k > 0 && isWordChar(line[k - 1]!)) k--; const kw = line.slice(k, slash); const REGEX_KWS = new Set(['return','case','typeof','in','instanceof','void','delete','throw','new','of']); return REGEX_KWS.has(kw); } ```

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor and removed gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. labels Jun 27, 2026
@dosubot dosubot Bot added the lgtm label Jun 27, 2026
@JSONbored JSONbored added the gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier. label Jun 27, 2026
@JSONbored
JSONbored merged commit 8d268a8 into JSONbored:main Jun 27, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. gittensor:priority Maintainer-selected Gittensor priority — scores a 1.5x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(enrichment): ReDoS scanner on added/changed regex

2 participants