chore: update recheck script to check rules.other#3908
chore: update recheck script to check rules.other#3908UziTech merged 2 commits intomarkedjs:masterfrom
Conversation
|
@UziTech is attempting to deploy a commit to the MarkedJS Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Code Review
This pull request updates the ReDoS check script by converting it to TypeScript and extending it to check the rules.other object. The conversion to TypeScript is a good improvement, and the new script has better logging. However, I've found a critical issue where the script will fail to run because it's a TypeScript file being executed by node directly. I've also pointed out that the script currently skips checking regexes generated by functions, which leaves a significant part of rules.other unchecked.
| } else if (typeof item === 'function') { | ||
| // TODO: skip functions for now | ||
| return; |
There was a problem hiding this comment.
The script currently skips checking any regexes that are generated by functions, as noted by the TODO. While this is a reasonable starting point, a significant portion of the regexes in rules.other (which this PR aims to add checks for) are generated by functions. This means the script doesn't fully check for ReDoS vulnerabilities in rules.other.
To make this check more comprehensive, you should consider implementing logic to call these functions with sample/default arguments to get the RegExp object and check it. For example, for a function that takes an indent level, you could call it with a typical value like 4.
This is not a blocking issue, but it's a significant gap in the functionality this PR introduces.
Description
Update recheck script to check rules.other regexps for backtracking
Also convert to typescript.
Contributor
Committer
In most cases, this should be a different person than the contributor.