CSS canvas color parsing accepts invalid color identifiers

• Component: CSS
• Status: RESOLVED FIXED

[Javier Fernandez]

Current implementation of the CSSParser::parseSystemColor assumes that if a valid cssValueKeywordID is got from the color string then it has to be a valid color. Such assumption is wrong and lead to many bugs and layout test failures. On example of these failures is the one detected after trying the patch for the bug #134419, which adds a new ValueKey for the "true" string. This string is part of the invalid values testing case of the canvas-color-serialization.html Layout test.

[Javier Fernandez]

Created attachment 234454 [details]
Patch

[Benjamin Poulain]

Comment on attachment 234454 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234454&action=review

Hilarious bug, that's a good catch.

I disagree with your fix though. IMHO, we should never pass an invalid ID to systemColor(). You could check that the input ID is in the range alpha->-webkit-text.

I would also like the same test for CSS style resolution in addition to canvas. I know CSSParser::parseSystemColor() is not used for CSS parsing, but it is better to have the test to be on the safe side if the code changes.

Long term, someone should investigate if it would be better to split CSSValueKeywords into tiny perfect hash tables. That's completely out of scope here though :)

> Source/WebCore/ChangeLog:9
> +        that if a valid cssValueKeywordID is got from the color string

"is got".

> Source/WebCore/css/CSSParser.cpp:1362
> +    color = parsedColor.rgb();

Changing the color when parsedColor.isValid() is false does not seem right.

[Darin Adler]

(In reply to comment #2)
> You could check that the input ID is in the range alpha->-webkit-text.

That doesn’t sound right. I think it’s the job of the theme to decide which CSS identifiers are actually legal colors. What if one of the names of the colors was also an identifier that was used for another purpose elsewhere in CSS?

[Benjamin Poulain]

(In reply to comment #3)
> (In reply to comment #2)
> > You could check that the input ID is in the range alpha->-webkit-text.
> 
> That doesn’t sound right. I think it’s the job of the theme to decide which CSS identifiers are actually legal colors. What if one of the names of the colors was also an identifier that was used for another purpose elsewhere in CSS?

I don't think this would work with colors defined in CSS, they are already filtered by value ID (see parseColorValue()).

[Benjamin Poulain]

> I don't think this would work with colors defined in CSS, they are already filtered by value ID (see parseColorValue()).

Well, obviously one can also add more branches in there :)

[Darin Adler]

(In reply to comment #4)
> I don't think this would work with colors defined in CSS, they are already filtered by value ID (see parseColorValue()).

Oh, maybe we should put that code somewhere we can share it!

[Darin Adler]

Retitling since the bug is in a function only called by the canvas implementation. The title would be misleading if it claimed a broader problem with CSS color parsing.

[Benjamin Poulain]

(In reply to comment #6)
> (In reply to comment #4)
> > I don't think this would work with colors defined in CSS, they are already filtered by value ID (see parseColorValue()).
> 
> Oh, maybe we should put that code somewhere we can share it!

Yep, agreed. When I originally commented I thought that code was a simple check in range. But that's a little more complicated that that...

It would be best to have this in a separate function.

[Javier Fernandez]

Created attachment 234639 [details]
Patch

Applied suggested changes.

[Benjamin Poulain]

Comment on attachment 234639 [details]
Patch

Awesome! Great tests!

[Javier Fernandez]

Committed r170933: <http://trac.webkit.org/changeset/170933>