Context Navigation

← Previous Ticket
Next Ticket →

#65379 closed defect (bug) (fixed)

Elements: Guard against non-string className in render filter

Reported by:	aaronrobertshaw	Owned by:	westonruter
Priority:	normal	Milestone:	7.1
Component:	Editor	Version:	trunk
Severity:	normal	Keywords:	gutenberg-merge has-patch has-unit-tests commit
Cc:		Focuses:

Description

This ticket tracks the backport of PHP files for the following Gutenberg update:

https://github.com/WordPress/gutenberg/pull/78841

Adds a safeguard in elements block support rendering to prevent fatal errors when a block’s className attribute is not a string. The backport updates wp_render_elements_class_name() to bail early and return block content unchanged for invalid className values, and includes a PHPUnit test covering this regression case.

Change History (9)

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

7 weeks ago #1

Keywords has-patch has-unit-tests added

Trac ticket: https://core.trac.wordpress.org/ticket/65379

This PR brings the changes from the following Gutenberg PR to core:

WordPress/gutenberg#78841

## Description
Hardens the elements block support render filter against invalid block attribute data.

wp_render_elements_class_name() currently reads attrs.className and passes it into preg_match(). While className is expected to be a string, malformed or corrupted stored block data can contain other types (for example, an array), which can trigger a fatal TypeError in PHP 8+.

This backport adds a defensive type check so that when className is not a string, the function returns the original block content unchanged instead of attempting regex matching.

A regression test is also added to ensure non-string className values do not cause fatals and continue to fail gracefully.

## Testing

Run the PHPUnit tests for elements block support: wpRenderElementsSupport.php
Verify the new regression test passes: test_elements_block_support_class_with_non_string_class_name
Verify existing tests in the same file continue to pass.

#2 @westonruter
6 weeks ago

Keywords commit added
Owner set to westonruter
Status new → reviewing

@westonruter commented on PR #12028:

6 weeks ago #3

This PR also addresses the following PHPStan rule level 10 errors:

  244    Function wp_render_elements_class_name() has parameter $block with no value type specified in iterable type array.                                                                                                                 
         🪪  missingType.iterableValue                                                                                                                                                                                                      
         💡  See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type                                                                                                                                         
         at src/wp-includes/block-supports/elements.php:244                                                                                                                                                                                 
  245    Cannot access offset 'className' on mixed.                                                                                                                                                                                         
         🪪  offsetAccess.nonOffsetAccessible                                                                                                                                                                                               
         at src/wp-includes/block-supports/elements.php:245                                                                                                                                                                                 
  246    Parameter #2 $subject of function preg_match expects string, mixed given.                                                                                                                                                          
         🪪  argument.type                                                                                                                                                                                                                  
         at src/wp-includes/block-supports/elements.php:246

@westonruter commented on PR #12028:

6 weeks ago #4

I just noticed that the changes here are extremely similar to something else we did recently in r62359 (20b5d10910b0d598b99313bc27f16a52cfb8f476, via https://github.com/WordPress/wordpress-develop/pull/11686). Therefore, I've aligned wp_render_elements_class_name() to match the implementation in wp_render_custom_css_class_name(). In particular, it was possible for a class name to have been added like my-wp-elements-foo which would erroneously get matched since a hyphen is a \b word boundary in regex. So now it tokenizes the class name according to the HTML spec.

Now that we have two instances of the same code, it probably makes sense to factor out into a helper for parsing CSS class names. cc @sirreal @dmsnell

#5 @westonruter
6 weeks ago

Resolution → fixed
Status reviewing → closed

In 62475:

Editor: Guard against non-string className in wp_render_elements_class_name() block render filter callback.

The wp_render_elements_class_name() function reads the className block attribute and passes it straight to preg_match(). While className is expected to be a string, malformed or corrupted stored block data can hold another type, such as an array, which triggers a fatal TypeError on PHP 8+. Guard against this by returning the block content unchanged when className is not a string.

While here, align the implementation with wp_render_custom_css_class_name() from r62359: replace the regular expression with a str_contains() short-circuit followed by an HTML-spec-compliant strtok() walk over the class tokens. This also corrects a latent matching bug: the previous \bwp-elements-\S+\b pattern treated the hyphen as a word boundary, so a class such as my-wp-elements-foo was erroneously matched. Tokenizing the attribute first ensures only a standalone wp-elements-* class is applied.

Add regression tests for the non-string and substring-prefix cases, and resolve PHPStan errors at rule level 10 (missingType.iterableValue, offsetAccess.nonOffsetAccessible, argument.type) by adding an array shape to the @phpstan-param tag.

Developed in https://github.com/WordPress/wordpress-develop/pull/12028 and https://github.com/WordPress/gutenberg/pull/78841.
Follow-up to r58074, r62359.

Props aaronrobertshaw, andrewserong, mukesh27, westonruter.
Fixes #65379.

@dmsnell commented on PR #12028:

6 weeks ago #6

thanks @westonruter

it probably makes sense to factor out into a helper for parsing CSS class names

I was thinking that the last place we left this was that it was unclear if the abstraction warranted a new method, or a basic wrapper with the Tag Processor.

$class_processor = new WP_HTML_Tag_Processor( '<div>' );
$class_processor->next_tag();
$class_processor->set_attribute( 'class', $processor->get_attribute( 'class' ) );
foreach ( $class_processor->class_list() as $class_name ) {
        …
}

one of the issues I remember is that this is actually a quite odd task to perform because it requires extracting the class attribute, and semantics in a JSON blob might not match those in HTML. for example, if the JSON contains A is that a literal value or A? we have run into these issues with attempts to fix issues with theme.json ambiguities.

probably okay IMO to let some of these sit duplicated for a while, but always happy to continue to explore what WordPress could provide to make things better.

@westonruter commented on PR #12028:

6 weeks ago #7

@dmsnell right, in the case here, there would not be any HTML entities, so directly we wouldn't need to do that encoding work.

@jonsurrell commented on PR #12028:

6 weeks ago #8

Now that we have two instances of the same code, it probably makes sense to factor out into a helper for parsing CSS class names.

I wonder how common this is and how useful it might be broadly for extenders. Have you thought about further about the utility?

Here's some sketching, if it seems valuable we can create a ticket:

/**
 * Splits a string containing multiple class names yielding individual values.
 *
 * This operates on _plaintext_. DO NOT USE WITH HTML ENCODED VALUES. It will
 * not perform any decoding. Only use on plaintext inputs!
 * 
 * Named for classnames for discoverability as most common use case, but suitable
 * for any HTML whitespace delimited list (DOMTokenList).
 * 
 * For HTML encoded values, use {@see WP_HTML_Tag_Processor}.
 *
 * Splits on HTML ASCII whitespace. This matches the behavior of DOMTokenList
 * and is suitable for class lists (…add more things that use DOMTokenList).
 * 
 * @todo add examples to documentation…
 *
 * @param string $classnames Plaintext space-delimited list.
 * @return Generator<string> Individual items.
 */
function classnames_to_class_list( $classnames ): Generator {
    if ( ! is_string( $classnames ) ) {
        // _doing_it_wrong?
        return;
    }
        $delimiters = " \t\n\f\r";
        $length     = strlen( $classnames );
        $offset     = 0;

        while ( $offset < $length ) {
                $offset += strspn( $classnames, $delimiters, $offset );

                if ( $offset >= $length ) {
                        break;
                }

                $token_length = strcspn( $classnames, $delimiters, $offset );

                yield substr( $classnames, $offset, $token_length );

                $offset += $token_length;
        }
}

$classnames = "\t  important\t\f\n\r\nsuper wp-block-find-me   \x0B-vertical-tab-preceded     ";

// Find a single class name
$class_name = null;
foreach ( classnames_to_class_list( $classnames ) as $class_name_candidate ) {
        if ( str_starts_with( 'wp-block', $class_name_candidate ) ) {
            $class_name = $class_name_candidate;
                break;
        }
}
var_dump( $class_name );

// Get an array. Much better than a simple `explode()`! 
$naive = explode( $classnames, ' ' );
var_export( $naive ); // [ ' ' ]
// \S is close, but still wrong. U+000B LINE TABULATION was ignored!
preg_match_all( '/\S+/', $classnames, $naive_regex );
var_export( $naive_regex ); // [ 'important', 'super', 'wp-block-find-me', '-vertical-tab-preceded' ]
$better = iterator_to_array( classnames_to_class_list( $classnames ), false );
var_export( $better ); // [ 'button', 'is-primary', 'js-open', "\x0B-vertical-tab-preceded" ]

@westonruter commented on PR #12028:

5 weeks ago #9

@sirreal That looks pretty good to me. There seem to be six unique cases which could use this helper. I've field a ticket: Core-65466.

Note: See TracTickets for help on using tickets.

Download in other formats:

Make WordPress Core

Context Navigation

#65379 closed defect (bug) (fixed)

Elements: Guard against non-string className in render filter

Description

Change History (9)

This ticket was mentioned in ​PR #12028 on ​WordPress/wordpress-develop by ​@aaronrobertshaw. 7 weeks ago #1

#2 @westonruter 6 weeks ago

​@westonruter commented on ​PR #12028: 6 weeks ago #3

​@westonruter commented on ​PR #12028: 6 weeks ago #4

#5 @westonruter 6 weeks ago

​@dmsnell commented on ​PR #12028: 6 weeks ago #6

​@westonruter commented on ​PR #12028: 6 weeks ago #7

​@jonsurrell commented on ​PR #12028: 6 weeks ago #8

​@westonruter commented on ​PR #12028: 5 weeks ago #9