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. |
Fil
left a comment
There was a problem hiding this comment.
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.