chore: sync core lib and CLAUDE.md from agent-core#27
Conversation
There was a problem hiding this comment.
Code Review
This pull request attempts to mitigate potential Regular Expression Denial of Service (ReDoS) vulnerabilities across several files by introducing arbitrary length bounds on wildcards, whitespaces, and character classes. However, the feedback highlights that many of the original regular expressions were already safe from ReDoS due to mutually exclusive negated character classes or single lazy quantifiers. The introduced bounds (such as 2,000 characters for arrow function parameters or markdown links, and 50,000 characters for XML blocks) risk causing silent failures or false positives on large, valid files. It is recommended to revert these unnecessary bounds or replace complex regex patterns with safer, more performant string operations.
| // ReDoS fix: bound the unbounded \s* / async runs and the parameter list so the | ||
| // matcher cannot backtrack polynomially on pathological input. Bounds are large | ||
| // enough that all realistic source matches identically to the prior \s*/[^)]* form. | ||
| const arrowPattern = /(?:const|let)\s{1,1000}([a-zA-Z_$][a-zA-Z0-9_$]*)\s{0,1000}=\s{0,1000}(?:async\s{0,1000})?\([^)]{0,2000}\)\s{0,1000}=>/g; |
There was a problem hiding this comment.
The original regular expression /(?:const|let)\s+([a-zA-Z_$][a-zA-Z0-9_$]*)\s*=\s*(?:async\s*)?\([^)]*\)\s*=>/g does not possess a polynomial or exponential backtracking (ReDoS) vulnerability. Negated character classes like [^)]* followed by their delimiter ) match linearly and do not backtrack overlappingly.
By introducing an arbitrary limit of [^)]{0,2000}, this pattern will silently fail to match arrow functions with parameter lists longer than 2000 characters. In TypeScript files (which this collector scans, e.g., .ts and .tsx), parameter lists with complex object destructuring, default values, or type annotations can easily exceed 2000 characters.
We should revert to the original clean regex which is safe and robust.
| const arrowPattern = /(?:const|let)\s{1,1000}([a-zA-Z_$][a-zA-Z0-9_$]*)\s{0,1000}=\s{0,1000}(?:async\s{0,1000})?\([^)]{0,2000}\)\s{0,1000}=>/g; | |
| const arrowPattern = /(?:const|let)\s+([a-zA-Z_$][a-zA-Z0-9_$]*)\s*=\s*(?:async\s*)?\([^)]*\)\s*=>/g; |
| const hasLists = /^[-*]\s{1,1000}[^\n]{1,2000}$/m.test(content); | ||
| // ReDoS fix: bound the unbounded [\s\S]*? so an unterminated <tag> cannot | ||
| // drive polynomial backtracking; 50k chars covers any realistic XML block. | ||
| const hasXML = /<\w+>[\s\S]{0,50000}?<\/\w+>/.test(content); |
There was a problem hiding this comment.
The regex /<\w+>[\s\S]*?<\/\w+>/ matches linearly and does not suffer from polynomial backtracking because [\s\S]*? is a single lazy quantifier with no overlapping or nested quantifiers.
By bounding it to [\s\S]{0,50000}?, any file containing an XML block larger than 50,000 characters will fail to match hasXML. This will cause the analyzer to incorrectly report a no_xml_for_data finding (Complex content without XML tags) for large, valid files.
We should revert to the original unbounded lazy quantifier which is safe and robust.
| const hasXML = /<\w+>[\s\S]{0,50000}?<\/\w+>/.test(content); | |
| const hasXML = /<\w+>[\s\S]*?<\/\w+>/.test(content); |
| const isPatternDoc = | ||
| /pattern.*detect.*usually|example.*vague|fuzzy.*language.*like/i.test(content) || | ||
| /vague.*terms.*like|"usually".*"sometimes"/i.test(content); | ||
| /pattern[^\n]{0,500}detect[^\n]{0,500}usually|example[^\n]{0,500}vague|fuzzy[^\n]{0,500}language[^\n]{0,500}like/i.test(content) || | ||
| /vague[^\n]{0,500}terms[^\n]{0,500}like|"usually"[^\n]{0,500}"sometimes"/i.test(content); |
There was a problem hiding this comment.
The original regexes used .* which can indeed backtrack heavily if there are multiple occurrences of the intermediate words on a very long line. However, bounding them to [^\n]{0,500} introduces a strict limit of 500 characters. In Markdown files, paragraphs are frequently written as single, very long lines (often exceeding 1000+ characters). This limit will cause the detection to fail on valid pattern documentation.
Instead of using complex regular expressions with arbitrary limits, we can split the content into lines and check for the presence of these words in order using simple, highly performant string operations. This completely eliminates any backtracking/ReDoS risk and removes all arbitrary line-length limits.
const lines = content.toLowerCase().split('\n');
const hasWordsInOrder = (str, words) => {
let index = 0;
for (const word of words) {
index = str.indexOf(word, index);
if (index === -1) return false;
index += word.length;
}
return true;
};
const isPatternDoc = lines.some(line =>
hasWordsInOrder(line, ['pattern', 'detect', 'usually']) ||
hasWordsInOrder(line, ['example', 'vague']) ||
hasWordsInOrder(line, ['fuzzy', 'language', 'like']) ||
hasWordsInOrder(line, ['vague', 'terms', 'like']) ||
hasWordsInOrder(line, ['"usually"', '"sometimes"'])
);| // ReDoS fix: bound the unbounded [\s\S]* so a "Task({" with no following | ||
| // subagent_type cannot drive polynomial backtracking; 50k chars covers any | ||
| // realistic Task(...) call body. | ||
| /Task\s{0,100}\(\s{0,100}\{[\s\S]{0,50000}subagent_type/i.test(content); |
There was a problem hiding this comment.
The regex /Task\s*\(\s*\{[\s\S]*subagent_type/i does not have a polynomial backtracking vulnerability because [\s\S]* is a single quantifier. However, we can completely avoid any backtracking and eliminate the arbitrary 50000 character limit by using simple string checks.
We can check if subagent_type is present in the content, and if so, find the index of Task to ensure it is part of a task definition. This is extremely fast and robust.
| /Task\s{0,100}\(\s{0,100}\{[\s\S]{0,50000}subagent_type/i.test(content); | |
| content.toLowerCase().includes('subagent_type') && /Task\s*\(\s*\{/i.test(content); |
| const linkRegex = /\[([^\]]+)\]\(([^)]+)\)/g; | ||
| // ReDoS fix: bound the negated-class captures so the matcher is linear; | ||
| // bounds far exceed any realistic markdown link, so matches are unchanged. | ||
| const linkRegex = /\[([^\]]{1,2000})\]\(([^)]{1,4000})\)/g; |
There was a problem hiding this comment.
The original regex /\[([^\]]+)\]\(([^)]+)\)/g does not have a ReDoS vulnerability. Negated character classes like [^\]]+ and [^)]+ followed by their respective closing delimiters ] and ) are mutually exclusive and do not backtrack overlappingly.
By bounding them to 2000 and 4000 characters, we risk failing to match valid markdown links that contain long URLs, such as data URIs, complex query parameters, or long base64-encoded links. We should revert to the original unbounded version which is safe and correct.
| const linkRegex = /\[([^\]]{1,2000})\]\(([^)]{1,4000})\)/g; | |
| const linkRegex = /\[([^\n\t\r\\\]]+)\]\(([^\n\t\r\\)]+)\)/g; |
|
Superseded by latest sync (post ReDoS + prototype-pollution hardening). |
Automated sync of lib/ and CLAUDE.md from agent-core.