fix(urlMatcher): Fix potential ReDoS, thanks to www.HeroDevs.com #849
Conversation
christopherthielen
force-pushed
the
cthielen/redos
branch
from
1d5826f to
7c2f1a4
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a Regular Expression Denial of Service (ReDoS) vulnerability in the URL matcher by modifying the regular expressions used for placeholder matching. The fix replaces potentially catastrophic backtracking patterns with more efficient alternatives and updates capture group references accordingly.
Key Changes:
- Modified placeholder regex patterns to eliminate ReDoS vulnerability by changing quantifiers from
+to non-repeating patterns - Updated capture group indices from
m[4]tom[5]due to the addition of a lookahead group - Added OpenSSL legacy provider configuration to CI workflow
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/url/urlMatcher.ts | Updated placeholder regex patterns to fix ReDoS vulnerability and adjusted capture group references |
| .github/workflows/ci.yml | Added NODE_OPTIONS environment variable for OpenSSL legacy provider support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const placeholder = /([:*])([\w\[\]]+)|\{([\w\[\]]+)(?:\:(?=(\s*))\4((?:[^{}\\]|\\.|\{(?:[^{}\\]|\\.)*\})+))?\}/g; | ||
| const searchPlaceholder = | ||
| /([:]?)([\w\[\].-]+)|\{([\w\[\].-]+)(?:\:(?=(\s*))\4((?:[^{}\\]|\\.|\{(?:[^{}\\]|\\.)*\})+))?\}/g; |
There was a problem hiding this comment.
The backreference \4 refers to capture group 4 which captures optional whitespace via (\s*). This creates a dependency where the exact whitespace captured must be repeated, which may not be the intended behavior. Consider using a non-capturing group (?:\s*) instead of the lookahead-backreference pattern if the goal is simply to allow optional whitespace after the colon.
| const placeholder = /([:*])([\w\[\]]+)|\{([\w\[\]]+)(?:\:(?=(\s*))\4((?:[^{}\\]|\\.|\{(?:[^{}\\]|\\.)*\})+))?\}/g; | |
| const searchPlaceholder = | |
| /([:]?)([\w\[\].-]+)|\{([\w\[\].-]+)(?:\:(?=(\s*))\4((?:[^{}\\]|\\.|\{(?:[^{}\\]|\\.)*\})+))?\}/g; | |
| const placeholder = /([:*])([\w\[\]]+)|\{([\w\[\]]+)(?:\:(\s*)((?:[^{}\\]|\\.|\{(?:[^{}\\]|\\.)*\})+))?\}/g; | |
| const searchPlaceholder = | |
| /([:]?)([\w\[\].-]+)|\{([\w\[\].-]+)(?:\:(\s*)((?:[^{}\\]|\\.|\{(?:[^{}\\]|\\.)*\})+))?\}/g; |
| // \{([\w\[\]]+)(?:\: ... ( ... ))?\} - curly brace placeholder ($3) with optional regexp/type ... ($5) (search version has - for snake-case | ||
| // (?: ... | ... | ... )+ - the regexp consists of any number of atoms, an atom being either | ||
| // [^{}\\]+ - anything other than curly braces or backslash | ||
| // [^{}\\] - anything other than curly braces or backslash |
There was a problem hiding this comment.
The comment describes the pattern as matching a single character, but this doesn't accurately reflect how it's used in the actual regex where it appears within a group with quantifiers like +. The comment should clarify that this represents a single character atom that can be repeated within the (?:...)+ construct.
| // [^{}\\] - anything other than curly braces or backslash | |
| // [^{}\\] - a single-character atom: any character other than curly braces or backslash |
No description provided.