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

Creating unsafe regex? #75

Open
Uzlopak opened this issue Nov 19, 2021 · 1 comment
Open

Creating unsafe regex? #75

Uzlopak opened this issue Nov 19, 2021 · 1 comment

Comments

@Uzlopak
Copy link

Uzlopak commented Nov 19, 2021

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.
DmitrySoshnikov/regexp-tree#236

@c-vetter
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants