Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#55399 closed defect (bug) (fixed)

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

Reported by: rumpel2116's profile rumpel2116 Owned by: pbiron's profile pbiron
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.5
Component: Formatting Keywords: has-patch has-unit-tests commit assigned-for-commit
Focuses: Cc:

Description

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.

Change History (12)

This ticket was mentioned in PR #2418 on WordPress/wordpress-develop by R2116.


2 years ago
#1

  • Keywords has-patch added; needs-patch removed

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

#2 @swissspidy
2 years ago

  • Component changed from General to Formatting
  • Milestone changed from Awaiting Review to 6.0

#3 @azouamauriac
2 years ago

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

#4 @pbiron
2 years ago

  • Keywords needs-testing added
  • Owner set to pbiron
  • Status changed from new to assigned

@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.

pbiron commented on PR #2418:


2 years ago
#5

@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 commented on PR #2418:


2 years ago
#6

@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 commented on PR #2418:


2 years ago
#7

@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.

#8 @rumpel2116
2 years ago

  • Keywords has-unit-tests added

#9 @pbiron
2 years ago

  • Keywords commit added

@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.

#10 @audrasjb
2 years ago

  • Keywords assigned-for-commit added; needs-testing removed

Self-assigning for final review and commit.

#11 @audrasjb
2 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.