search

anandthakker / doiuse

:bomb: Lint CSS for browser support against caniuse database.
MIT License
1.24k stars 51 forks source link

Fixed possible regular expression catastrophic backtracking #105

Closed marekdedic closed 3 years ago

marekdedic commented 5 years ago

Fixes #95

marekdedic commented 5 years ago

I don't think the failing CI has anything to do with the PR - let me know if it does.

marekdedic commented 5 years ago

Hey, any progress on this? This is a blocking issue for me...

Thanks a lot, Marek

pkuczynski commented 3 years ago

@marekdedic, apologies for the late reply. Could you provide some tests and changelog entry for this?

marekdedic commented 3 years ago

Hi, thanks for the reply anyway :slightly_smiling_face:

I've added a changelog entry, however, I'm a bit stuck with the tests, see #126.

pkuczynski commented 3 years ago

@marekdedic I had a look at the example you provided and after adding ? as you suggest, I am only getting a match on .:

https://regex101.com/r/MlbAhW/1 vs https://regex101.com/r/MlbAhW/2

I don't think this is correct?

marekdedic commented 3 years ago

Hmm, it's not correct. I don't really know how to fix it though, as I am not really sure what the mathOutsideOfBrackets function does...

pkuczynski commented 3 years ago

To be honest, I don't know either. I have not written this package and I am only maintaining it. Approving PR and releasing the new versions. The only person who knows is @anandthakker, but he is not responsive :(

marekdedic commented 3 years ago

Hi, I looked over it once more and it seems to me that the function matchOutsideOfBrackets(X) checks whether there is the characterX somewhere not inside [] or (). So the match on . would actually be correct. And expanding the regex constructed by mathcOutsideOfBrackets, the non-greedy approach doesn't change the meaning.

So I do actually think this PR is correct, however there is one more thing... We can actually undo #88 with this change and simplify it!

marekdedic commented 3 years ago

(and undoing #88 would allow for testing for catastrophic regexes...)

marekdedic commented 3 years ago

Hi, the PR is now working and with tests for unsafe regexes (not limited to the one in #95)

marekdedic commented 3 years ago

Hmm, so the tests wouldn't catch #95. I've tried switching safe-regex for vuln-regex-detector but that seems to be abandoned... So at least an imperfect test...

pkuczynski commented 3 years ago

And thank you for putting effort into this! :)

marekdedic commented 3 years ago

Hold your horses. I think I've found some more performance issues with this function. Going to go for a different solution to solve it once and for all.

marekdedic commented 3 years ago

Hi, I've reworked it to not use such a horrendous regex but a simpler function instead. Unit tests pass, tried it on 2 of my projects, same output, no performance issues. I think you can merge this now.

pkuczynski commented 3 years ago

Let's give it a try and release it as next major version :)