Make WordPress Core

Opened 2 years ago

Last modified 6 months ago

#55962 assigned enhancement

Upgrade `sanitize_hex_color()` to CSS Color Level 4

Reported by: anrghg's profile anrghg Owned by: pbearne's profile pbearne
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Formatting Keywords: dev-feedback has-patch has-unit-tests
Focuses: css Cc:

Description

I’ve noticed that the sanitize_hex_color() function unsupports the CSS Color Level 4 with the alpha channel and can therefore not be used. As users are given the ability to provide settings by configuration filters in a mini-plugin or in their theme’s functions.php, they may wish to configure opacity alongside.

The fix is to extend the validation condition of sanitize_hex_color() from:

if ( preg_match( '|^#([A-Fa-f0-9]{3}){1,2}$|', $color ) )

to

if ( preg_match( '|^#([A-Fa-f0-9]{3,4}){1,2}$|', $color ) )

Change History (11)

#1 @SergeyBiryukov
2 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 6.1

#2 @ocean90
2 years ago

  • Milestone changed from 6.1 to Future Release
  • Type changed from defect (bug) to enhancement

I don't think sanitize_hex_color() can be extend as suggested as it's used for various color settings which do not allow more then 3 or 6 digit yet. Also, this would be unexpected for any plugin/theme which have some custom storage and simply cannot store more than 6 digits.

Sounds more like a new function sanitize_hex_with_alpha_color() would be a option here.

#3 @anrghg
2 years ago

Thank you all for looking into this.

I’m fine with the outcome as long as WordPress does not make this specific color code sanitization a requirement.

The existing function works well on output from browsers’ built-in color pickers not supporting alpha either.

If storage is limited to 3 hex digits, a more appropriate function must be used instead. If storage is limited to 6 hex digits, without sanitization it could be used for a limited 4‑digit hex with alpha workaround.

I see an issue with configuration filters, that users may wish to use for opacity as well.

However, that issue goes beyond adding support for alpha, as users may as well wish to set colors in decimal RGB format, using rgb() or rgba() functions interchangeably, or in HSL format.

As for naming the new function, I’d strongly recommend to opt for sanitize_any_hex_color(), regardless of alpha provided or not, or it would not be backward compatible.

#4 follow-up: @costdev
2 years ago

  • Keywords dev-feedback added

@ocean90 What if we changed the signature of sanitize_hex_color() to include an optional $maybe_alpha argument that defaults to false? That way, $maybe_alpha is only enabled by intent, and this should protect BC.

Instead of working with complicated regex patterns, we could also simplify the function by:

  1. Performing a length check, dependent on the value of $maybe_alpha.
  2. Strip invalid characters and compare the result to the original value.

For example:

function sanitize_hex_color( $color, $maybe_alpha = false ) {
    if ( '' === $color ) {
        return '';
    }
    
    $allowed_lengths = $maybe_alpha ? array( 4, 5, 7, 9 ) : array( 4, 7 );
    $correct_length  = in_array( strlen( $color ), $allowed_lengths, true );
    
    if ( $correct_length && preg_replace( '|^#([^A-Fa-f0-9]+)$|', '', $color ) === $color ) {
        return $color;
    }
}

Tests in 3val

This would also mean that we don't have to struggle against "naming is hard", because sanitize_hex_with_alpha_color() suggests it must have alpha, which means checking a valid hex colour code would require a call to both functions instead of just calling the revised sanitize_hex_color() above.

FYI: My reason for suggesting the simplification is because the example regex in the ticket description will incorrectly allow a six digit hex code with a single alpha value, i.e. #123456f. Things get more complicated regex-wise from there.

Last edited 2 years ago by costdev (previous) (diff)

#5 in reply to: ↑ 4 @SergeyBiryukov
2 years ago

Replying to costdev:

What if we changed the signature of sanitize_hex_color() to include an optional $maybe_alpha argument that defaults to false? That way, $maybe_alpha is only enabled by intent, and this should protect BC.

I think that makes sense.

#6 @ocean90
2 years ago

One of the benefits of all the sanitize_* functions is that you can easily use them as a sanitize callback when registering options, metas, or REST API fields. As soon as there's an argument you have to work with something like a closure to set the argument and ensure no other value is passed to the function as the second argument.

So maybe both?

Also note that the REST API supports hex-color as a format for type string, see rest_sanitize_value_from_schema().

#7 @peterwilsoncc
2 years ago

I'm of the view the purpose of the sanitize functions is to validate against the spec, given a hex colour can now be 3,4,6 or 8 characters the core function should account for that.

Accepting @ocean90's point though, it might be worth introducing sanitize_color() or sanitize_css_color() to validate the many valid color formats that WordPress has no way of validating.

This new function could be documented to indicate that the length of the value may change as new color functions are introduced to the CSS spec.

#8 @crstauf
13 months ago

#58690 was marked as a duplicate.

#9 @pentatonicfunk
7 months ago

there is also rest_parse_hex_color

This ticket was mentioned in PR #5883 on WordPress/wordpress-develop by @pbearne.


6 months ago
#10

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

#11 @pbearne
6 months ago

  • Owner set to pbearne
  • Status changed from new to assigned

I Have created a patch for this with tests

Note: See TracTickets for help on using tickets.