Fix: regex vulnerable to super-linear runtime due to excessive backtracking #1851
Conversation
WalkthroughThe pull request introduces a modification to the regular expression validation in the Changes
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Code Climate has analyzed commit f191315 and detected 0 issues on this pull request. View more on Code Climate. |
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/src/utils/commify.ts (2)
4-6: Consider adding input length validation.While the regex is now safe from catastrophic backtracking, it might be good to add a maximum length check for the input to prevent unnecessarily processing very long strings.
export function commify(value: string | number): string { const comps = String(value).split("."); + // Reject unreasonably long inputs (e.g., > 256 chars) + if (String(value).length > 256) { + return "0"; + } + if (!String(value).match(/^-?\d+(\.\d+)?$/)) { return "0"; }
Line range hint
1-52: Consider using built-in number formatting.For better maintainability and localization support, consider using
Intl.NumberFormatinstead of custom string manipulation.export function commify(value: string | number): string { + // Validate input first const comps = String(value).split("."); if (!String(value).match(/^-?\d+(\.\d+)?$/)) { return "0"; } - // Make sure we have at least one whole digit (0 if none) - let whole = comps[0]; - // ... rest of the current implementation - - return negative + formatted.join(",") + suffix; + // Parse and format the number + const num = parseFloat(String(value)); + return new Intl.NumberFormat('en-US', { + minimumFractionDigits: comps[1]?.length || 0, + maximumFractionDigits: comps[1]?.length || 0, + }).format(num); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/src/utils/commify.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: dependency-review
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (1)
web/src/utils/commify.ts (1)
4-4: Approve: Regex pattern fixed to prevent catastrophic backtracking.The new regex pattern
/^-?\d+(\.\d+)?$/effectively addresses the security vulnerability by:
- Using
+quantifier instead of*to prevent empty matches- Properly grouping the decimal part
- Anchoring at both ends with
^and$Let's verify the fix prevents catastrophic backtracking with various inputs:
✅ Verification successful
✅ Verified: New regex pattern is secure and performs efficiently
The verification confirms the pattern
/^-?\d+(\.\d+)?$/successfully prevents catastrophic backtracking, processing both valid and invalid inputs (up to 1000 characters) in under 10ms total.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Create a test file to verify the regex performance cat << 'EOF' > test.js const regex = /^-?\d+(\.\d+)?$/; const inputs = [ "1234.56", // Valid number "000000000", // Many leading zeros "............", // Many dots "1".repeat(1000), // Long valid input ".".repeat(1000), // Long invalid input ]; console.time('regex'); inputs.forEach(input => { console.log(`Testing: ${input.slice(0, 20)}... (length: ${input.length})`); console.log(`Result: ${regex.test(input)}`); }); console.timeEnd('regex'); EOF # Run the test node test.jsLength of output: 302
Flagged here: https://sonarcloud.io/project/security_hotspots?id=kleros_kleros-v2&branch=dev&issueStatuses=OPEN%2CCONFIRMED&sinceLeakPeriod=true&tab=code
PR-Codex overview
This PR updates the regular expression used to validate numeric values in the
commifyfunction within theweb/src/utils/commify.tsfile to ensure it correctly matches integers and decimals.Detailed summary
/^-?[0-9]*\.?[0-9]*$/to/^-?\d+(\.\d+)?$/for improved validation of numeric formats.Summary by CodeRabbit