esc_xml() removes valid XML input ( input that is empty() )

[rumpel2116]

The with #50117 introduced function esc_xml() escapes/deletes some valid input: Input that results true if passed to empty().
esc_xml('0') and similar returns an empty string instead of returning '0' while '0' is perfectly XML-safe.

There are two issues checking for empty regex groups that use PHPs empty():

$safe_text = (string) preg_replace_callback(
        $regex,
        static function( $matches ) {
                if ( ! $matches[0] ) {
                        return '';
                }

                if ( ! empty( $matches['non_cdata'] ) ) {
                        // escape HTML entities in the non-CDATA Section.
                        return _wp_specialchars( $matches['non_cdata'], ENT_XML1 );
                }

                // Return the CDATA Section unchanged, escape HTML entities in the rest.
                return _wp_specialchars( $matches['non_cdata_followed_by_cdata'], ENT_XML1 ) . $matches['cdata'];
        },
        $safe_text
);

The first check is to skip further processing of empty strings I believe. Can easily be replaced by ! isset( $matches[0] ) as the group is not set if empty.

The second check validates if there is no non_cdata (without any cdata), but uses ! empty() explicitly. Same solution, using ! isset( $matches['non_cdata'] ) covers the case, if no non_cdata is captured, the regex-group is not set.

Replaced two empty()-checks with isset() to cover values that are not '' but empty() like '0', 0 or false.

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

[azouamauriac]

Hello there welcome to trac, thanks for the report and patch! I have tested it and it works. No objection!

[pbiron]

@rumpel2116 Thanx for this ticket and the patch/PR. At first glance it looks like the problem is real and the PR fixes. I'll do some more extensive testing this week and hopefully we'll get this committed quickly.

@R2116 are you familiar with creating/modifying unit tests for core?

If yes, could be please amend this PR by adding additional test cases the data providers in ​https://github.com/WordPress/wordpress-develop/blob/trunk/tests/phpunit/tests/formatting/escXml.php?

If not, don't worry about it...I'll work some up.

@R2116 are you familiar with creating/modifying unit tests for core?

Not yet, but it's about time to learn. This one was easy to extend.

In the initial bug description I mentioned '0', 0, false as being wrongly escaped. Writing the tests now, I realized esc_xml( $text ) is defined to take strings as parameter. In the PHP doc only, but therefore passing 0 or false should not be necessary to check in tests?

I initially discovered this bug by passing 0.0 (float) to it and receiving an unexpected empty result but the first mistake was to pass a float instead of a string I guess?
The '0' escaping was still buggy, still a valid fix.

@R2116 are you familiar with creating/modifying unit tests for core?

Not yet, but it's about time to learn. This one was easy to extend.

In the initial bug description I mentioned '0', 0, false as being wrongly escaped. Writing the tests now, I realized esc_xml( $text ) is defined to take strings as parameter. In the PHP doc only, but therefore passing 0 or false should not be necessary to check in tests?

I initially discovered this bug by passing 0.0 (float) to it and receiving an unexpected empty result but the first mistake was to pass a float instead of a string I guess?
The '0' escaping was still buggy, still a valid fix.

[pbiron]

@rumpel2116

Thanx! This looks good, thanx for the additional unit test.

I'm marking this for commit.

@swissspidy can you take a look and and commit before the 6.0 Beta 1 release tomorrow. Technically, since this is a bug, it can go in after Beta 1, but it would be better if it were committed before Beta.

[audrasjb]

Self-assigning for final review and commit.

[audrasjb]

In 53144:

Formatting: Avoid escaping valid XML values in esc_xml().

This change improves the esc_xml() function by replacing two empty() checks with isset() to cover values that are not equal to '' but still returning true when checked with empty(), like '0', 0 or false. It also updates the related unit tests accordingly.

Props rumpel2116, pbiron.
Fixes #55399.

Committed in https://core.trac.wordpress.org/changeset/53144