Skip to content

🔒 Fix polynomial ReDoS in validation regexes - #8

Merged
stoe merged 1 commit into
mainfrom
stoe/fix-redos-patterns
Aug 31, 2026
Merged

🔒 Fix polynomial ReDoS in validation regexes#8
stoe merged 1 commit into
mainfrom
stoe/fix-redos-patterns

Conversation

@stoe

@stoe stoe commented Aug 31, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes four high-severity js/polynomial-redos code scanning alerts by removing the ambiguous repetitions that let a crafted skill description consume seconds of CPU per validation.

Closes the alerts reported in 5, 6, 7, and 8. Alerts 1–4 are the same defects at the pre-restructure scripts/ paths, so these are pre-existing issues re-detected after #1, not regressions from that stack.

Impact

SKILL.md frontmatter is untrusted input read from disk. validateMicroTemplate reports desc.toolong above 1024 characters but does not stop, so an oversized description still reaches every pattern below it. Measured on a 40,000-character adversarial input:

Alert Pattern Before After
6 \[[^\]]+\]\([^\)]+\) 2444 ms 0.44 ms
7 <[a-zA-Z][^>]*> 1265 ms 0.22 ms
8 [ \t]+$ 3162 ms 0.05 ms

Alert 5 is not reachable through the current call path, because extractFrontmatter splits on \n before applying the regex, so the subject can never contain the newline the quadratic path requires. It is fixed as a latent hazard.

Changes

  • src/core/frontmatter.js: parse key: value with indexOf(':') and a linear /^\w+$/ key check instead of /^(\w+):\s*(.*)$/
  • src/validate/micro-templates.js: narrow [^\]]+ to [^\][]+ and [^\)]+ to [^()]+ so a scan starting at [ stops at the next [
  • src/validate/micro-templates.js: narrow [^>]* to [^<>]*; an XML tag cannot contain <, and <a<b> is still reported because the scan restarts at the inner <
  • src/validate/micro-templates.js: replace /[ \t]+$/ with endsWith(' ') || endsWith('\t'), which is exactly equivalent without the m flag and runs in constant time
  • src/validate/micro-templates.js: coerce non-string descriptions in normalizeText and share a raw value with the NBSP and trailing checks, so a non-string argument no longer throws
  • src/core/frontmatter.js and src/validate/index.js: accept \r?\n in the frontmatter delimiter, fixing CRLF SKILL.md files whose keys were silently dropped
  • src/validate/index.js: harden the two heading patterns with [ \t]+ instead of \s+ for consistency, though CodeQL did not flag them

Affected area

Core helpers and validation rules.

Checklist

  • npm run format passes with no unstaged diffs.
  • npm test passes with no new errors.
  • Tests cover new or changed behavior. Regression tests land in ✅ Add ReDoS regression tests #9.
  • README.md is updated when CLI options, validation rules, or package exports change. Not applicable; validation rules are unchanged.
  • package.json exports and files are updated when modules are added, moved, or removed. Not applicable; no module changes.

Security checklist

  • No secrets, tokens, credentials, or personal local file paths were added to source, tests, or docs.
  • Source uses only Node.js built-in modules or already-declared dependencies.
  • New filesystem writes stay inside the caller-approved base directory.
  • Skill content read by the CLI is treated as untrusted data, never as instructions.
  • npm audit --audit-level=high reports no new high or critical findings.

Verification

  • A differential harness compared the old and new modules across 40 representative inputs (URLs, Markdown links, unbalanced brackets, XML tags, NBSP, control characters, quoted and multi-line frontmatter values, colons inside values) and found 0 behavior differences
  • The only intentional difference is CRLF frontmatter, which returned null before and now parses
  • npm test passes
  • npx prettier --config-precedence prefer-file --check . reports no drift

Additional context

The CRLF fix turned out to be required rather than incidental. The original blocker was the opening delimiter on line 6, not the key pattern on line 17, so the key-parsing change alone would not have fixed it.

Stack

Part of a stacked PR series fixing the polynomial ReDoS code scanning alerts (bottom to top):

  1. 🔒 Fix polynomial ReDoS in validation regexes #8 — 🔒 Fix polynomial ReDoS in validation regexes (stoe/fix-redos-patterns)
  2. ✅ Add ReDoS regression tests #9 — ✅ Add ReDoS regression tests (stoe/add-redos-tests)

- Replace the frontmatter key regex with an index split
- Stop link and XML tag patterns from rescanning on each offset
- Compare trailing whitespace with endsWith instead of a regex
- Accept CRLF frontmatter, which the opening regex rejected before
- Coerce non-string descriptions instead of throwing

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@stoe stoe mentioned this pull request Aug 31, 2026
10 tasks
@stoe
stoe marked this pull request as ready for review August 31, 2026 18:50
@stoe
stoe merged commit 710ed5c into main Aug 31, 2026
7 checks passed
@stoe
stoe deleted the stoe/fix-redos-patterns branch August 31, 2026 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant