-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
Fix a ReDoS in 'style' format #180
Conversation
As there are no `^` or `$` anchors in the regex, this should be equivalent. Patch deliberately does not change the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the changes looks good from what I can tell.
I don't understand what the uses case for this is though 😂
I think that we should deprecated style
and remove it in the next major. As it currently behaves, it seems to open, it basically accepts anything with a colon in it, and we cannot change it without a breaking change...
I will merge and release this right away since it seems like a change that would be nice to get out to users as fast a possible. Thank you for finding and fixing this! |
Released as 2.20.4 |
btw.
I'm guessing that you talked to Mathias, is there anywhere where I should add my email address as well so that we both can get notified next time? |
Being open is not a problem of the I fixed that in a rewrite (along with a number of other issues). Docs on technical decisions and the reasoning behind the changes are still in progress, but switching to not failing open everywhere was the most significant reason. That induced a mostly complete rewrite of the lib and it can't be fixed with minor changes, I believe. I'll document those things soonish.
That went via https://github.com/nodejs/security-wg and https://hackerone.com/nodejs-ecosystem. |
Nice 👍 My HackerOne username is: |
Filing a public PR as requested.
As there are no
^
or$
anchors in the regex, this should be equivalent.Patch deliberately does not change the behavior.
If the intent was to include
^
and$
and those were missed due to an unrelated mistake, I can probably propose another safe regex for that (but that would change the behavior).Please recheck that this regex is correct.