Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The no-unsafe-regex rule is annoying #153

Closed
sindresorhus opened this issue Feb 14, 2018 · 8 comments · Fixed by #2135
Closed

The no-unsafe-regex rule is annoying #153

sindresorhus opened this issue Feb 14, 2018 · 8 comments · Fixed by #2135
Labels
bug 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted

Comments

@sindresorhus
Copy link
Owner

sindresorhus commented Feb 14, 2018

Issuehunt badges

It matches way too many regexes to be useful. And it's unclear what's wrong with the regexes and how to fix it.

It matches simple regex like:

https://github.com/xojs/stylelint-config-xo/blob/9c49d0bbf96a433ec6b2eef83875acadd26ba78f/index.js#L4

const customError = /^(?:[A-Z][a-z\d]*)*Error$/;

const disableRegex = /^eslint-disable(-next-line|-line)?($|(\s+(@[\w-]+\/[\w-]+\/)?[\w-]+)?)/;

And many many more.


IssueHunt Summary

fisker fisker has been rewarded.

Backers (Total: $60.00)

Submitted pull Requests


Tips

@sindresorhus
Copy link
Owner Author

// @bdougherty

@sindresorhus
Copy link
Owner Author

The safe-regex module also seems unmaintained, so not a great start.

For this plugin to be useful it needs to pinpoint exactly where in the regex the problem lies, and suggest how it could be fixed, ideally with --fix support.

It could maybe use regexp-tree.

@bdougherty
Copy link
Contributor

Based on what I was reading about the regexes that it's looking for (primarily http://www.regular-expressions.info/catastrophic.html), I'm not sure that there is an automated way to fix them.

But I agree that it should give some indication of what the issue is, I'm just not sure how to do that exactly. I'll mess around with it and see if I can come up with anything.

voxpelli added a commit to Sydsvenskan/eslint-config-hds that referenced this issue Apr 6, 2018
See sindresorhus/eslint-plugin-unicorn#153 – Unicorn itself will anyways include it if they find a better way to do it
@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label May 31, 2019
@IssueHuntBot
Copy link

@IssueHunt has funded $60.00 to this issue.


@Jessidhia
Copy link

Jessidhia commented Aug 20, 2019

Even a trivial regular expression like /\./, which should have a star height of 0, is enough to trip the rule. It's possible it's just an incompatibility with the u flag, though.

Arguably this regexp does not need u, but there is a good reason for using u on all regexps: it disables Annex B features.

image

@devinrhode2
Copy link

safe-regex readme now says

WARNING: This module has both false positives and false negatives. Use vuln-regex-detector for improved accuracy.

Discussion on using this here:
eslint-community/eslint-plugin-security#28

I wonder if it's possible for one eslint plugin to pull in just one rule from another plugin

@mircowidmer
Copy link

Interestingly,

(( )?[0-9]){2}$ also raises the warning, (( )?[0-9])(( )?[0-9])$ does not. It seems to have to do with the repetition.

@issuehunt-oss
Copy link

issuehunt-oss bot commented May 18, 2023

@sindresorhus has rewarded $54.00 to @fisker. See it on IssueHunt

  • 💰 Total deposit: $60.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $6.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels May 18, 2023
AwfulDarkness0110 added a commit to AwfulDarkness0110/eslint-plugin-unicorn that referenced this issue Jun 28, 2023
mattstern31 added a commit to mattstern31/eslint-plugin-unicorn that referenced this issue Nov 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants