-
Notifications
You must be signed in to change notification settings - Fork 92
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
avoid backtracking when parsing #89
Conversation
It also looks like modern browsers are more lax, for example, they don’t distinguish between rgb and rgba or hsl and hsla. I’ve done that, but I think we’re still a little too lax in the sense of allowing percents and non-percents to be mixed. |
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.
We stop at level 3, right? no rad, deg, turn units? Modern browsers would implement level 4
https://developer.mozilla.org/en-US/docs/Web/CSS/color_value shows an awful mix of possibilities ;-)
The attack now fails, with a string of 5 million chars passing in < 1s
attack_str.length: 5000005: 613 ms
whereas in the current master we die early:
attack_str.length: 100005: 8704 ms
attack_str.length: 200005: 35656 ms
the code is a bit slower in my tests, from 3800ms to perform 20 million conversions, to 5200ms.
I’m worried that if our parser is too lax, someone will parse a color with d3-color and assume that it’s valid, and then try to use the same string with the browser natively where it will fail. I think it’s okay if the inverse is not true, i.e. if a browser fully supports CSS Color Level 4 but d3-color only supports Level 3 (plus perhaps some subset of Level 4). |
For reference, there is a parser here https://github.com/deanm/css-color-parser-js/blob/master/csscolorparser.js (but it doesn't make exactly the same choices). |
I probably need to bite the proverbial bullet and implement a proper tokenizer and parser here. |
Is there any update on that? It would be great to have this merged as it's affecting a lot of packages: https://security.snyk.io/vuln/SNYK-JS-D3COLOR-1076592 |
Superseded by #99. |
This seems to address the issue, but is slightly too lax, and I would like to avoid the tiny amount of code duplication. And I haven’t tested performance in the non-degenerate case.