\XF\Util\Color::isValidColor() produces a positive match for RGB values without commas, can break CSS

[apathy]

Affected version

2.2.12

The regex used in this function is capable of producing a positive match when an RGB value without commas is entered - regex101 example

This can cause problems when creating Reactions for instance, if a comma-less RGB value is specified for the reaction text color, the CSS can break like in the attached screenshot.

Usually this wouldn't be a huge problem since I imagine most admins are using the color picker (which correctly produces commas), however some of my addons and I imagine many others have color fields in public controllers and use isValidColor() to verify them, so if a user who's not too familiar with RGB syntax entered a value with no commas the whole sites CSS can break.

 

[unproperlypropagated]

same on 2.3.6

 

[Yenxji]

The regex used in this function is capable of producing a positive match when an RGB value without commas is entered - regex101 example

This can cause problems when creating Reactions for instance, if a comma-less RGB value is specified for the reaction text color, the CSS can break like in the attached screenshot.

Usually this wouldn't be a huge problem since I imagine most admins are using the color picker (which correctly produces commas), however some of my addons and I imagine many others have color fields in public controllers and use isValidColor() to verify them, so if a user who's not too familiar with RGB syntax entered a value with no commas the whole sites CSS can break.

This is not a bug issue, your CSS are incorrect.

Before:

.reaction-7 & {
    color: rgb(240 60 100);
}

After:

.reaction-7 & {
    color: rgb(240, 60, 100);
}

Change this to your core.less or extra.less

This should be fixed issue and error may away

 

[apathy]

This is not a bug issue, your CSS are incorrect.

Before:


Change this to your core.less or extra.less

This should be fixed issue and error may away

Thats the point though, rgb(240 60 100) is not a valid color, so isValidColor() should not allow it to be used. I'm not entering it into a template directly

 

[unproperlypropagated]

I made a better isValidColor() if anyone's interested.
Tested and does reject invalid colors and accept valid ones.

This enforces 3 values and 2 commas for rgb/hsl and
also enforces 4 values and 3 commas for rgba/hsla

    public static function isValidColor($color)
{
    if (array_key_exists(Str::strtolower($color), static::$namedColors)) {
        return true;
    }

    if ($color && $color[0] === "#") {
        $color = substr($color, 1);
        return in_array(strlen($color), [3, 4, 6, 8]) && ctype_xdigit($color);
    } elseif (preg_match("/^(rgba|hsla)\(\s*(\d+%?(deg|rad|grad|turn)?\s*,\s*){3}\s*[\d\.]+%?\s*\)$/i", $color)) {
        // Check for rgba or hsla with exactly four values and three commas
        return true;
    } else {
        // Check for rgb or hsl with exactly three values and two commas
        return preg_match("/^(rgb|hsl)\(\s*(\d+%?(deg|rad|grad|turn)?\s*,\s*){2}\s*[\d\.]+%?\s*\)$/i", $color);
    }
}

 

[unproperlypropagated]

@apathy even after fixing isValidColor() with the version above (verifiably in reactions setting for example)
Style Suite doesn't seem to use it? It would save whatever is in the input without checking
it seems.