eslint-plugin-optimize-regex icon indicating copy to clipboard operation
eslint-plugin-optimize-regex copied to clipboard

Creating unsafe regex?

Open Uzlopak opened this issue 4 years ago • 1 comments
trafficstars

Maybe check the optimized regex by safe-regex. My Regex skills are kind of limited to understand why one is deemed safe and the other one not. But I think, it is better to be on the safe side, and only suggest regex, which are safe.

Example: /^(?:(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[1-9])\.)(?:(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])\.){2}(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])$/i is optimized to

/^(?:25[0-5]|2[0-4]\d|1\d{2}|[1-9]\d|[1-9])\.(?:(?:25[0-5]|2[0-4]\d|1\d{2}|[1-9]\d|\d)\.){2}(?:25[0-5]|2[0-4]\d|1\d{2}|[1-9]\d|\d)$/i

The first is considered a safe regex by safe-regex, the second one not.

Also opened an issue in regexp-tree. https://github.com/DmitrySoshnikov/regexp-tree/issues/236

Uzlopak avatar Nov 19 '21 21:11 Uzlopak

I took a look at those two regexes, and found the following:

  1. Both versions are “safe” in the natural sense and will not result in the problems that safe-regex tries to guard against
  2. This is one of those cases meant by this from the safe-regex readme:

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

  3. The “unsafety” comes from the nested {2} queries
  4. You can improve your regex (in logic and size) by using the dot as a prefix instead of a suffix (also removed the issue safe-regex hints at): /^(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|[1-9])(?:\.(?:25[0-5]|2[0-4]\d|1\d\d|[1-9]\d|\d)){3}$/i

Unless this plugin introduces any asterisks, it should not create “potentially catastrophic exponential-time regular expressions” [sic]. Those are either pre-existing or the resulting regex should be fine.

It seems like your issue would be better served by a change in safe-regex to not mark fixed-number repetitors as unsafe.

c-vetter avatar Feb 13 '22 15:02 c-vetter