Make WordPress Core

Opened 5 months ago

Closed 2 months ago

Last modified 2 months ago

#54060 closed defect (bug) (fixed)

kses.php global $allowedxmlentitynames naming

Reported by: ovidiul Owned by: SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.9
Component: Formatting Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Hi all,

upon debugging this Notice on one of our client's site

<?php
NoticedError: in_array() expects parameter 2 to be array, null given
in newrelic_notice_error called at ? (?)
in in_array called at /chroot/var/www/wp-includes/kses.php (1863)
in wp_kses_xml_named_entities called at ? (?)
in preg_replace_callback called at /chroot/var/www/wp-includes/kses.php (1805)
in wp_kses_normalize_entities called at /chroot/var/www/wp-includes/formatting.php (981)
in _wp_specialchars called at /chroot/var/www/wp-includes/formatting.php (4574)
in{closure} called at ? (?)
in preg_replace_callback called at /chroot/var/www/wp-includes/formatting.php (4580)

I've found a small discrepancy in the namings used in core file wp-includes/kses.php , so the global $allowedxmlentitynames variable is actually used further as $allowedxmlnamedentities, I am guessing this might not be intentional?

I am lead to believe this might be the source of the issue, however, I was unable to actively duplicate the issue as it happens during a scheduled cron event.

Happy to submit a patch for this if it's confirmed.

Thank you

Change History (11)

#1 @ovidiul
5 months ago

To clarify this more,

I am referring to these [code lines]https://github.com/WordPress/WordPress/blob/master/wp-includes/kses.php#L715-L724 and [this]https://github.com/WordPress/WordPress/blob/master/wp-includes/kses.php#L50

<?php
/**
         * @var string[] $allowedxmlentitynames Array of KSES allowed XML entitity names.
         * @since 5.5.0
         */
        $allowedxmlnamedentities = array(
                'amp',
                'lt',
                'gt',
                'apos',
                'quot',
        );
<?php
global $allowedposttags, $allowedtags, $allowedentitynames, $allowedxmlentitynames;

so the actual $allowedxmlnamedentities variable name is not being set as global in the file.

This ticket was mentioned in PR #1651 on WordPress/wordpress-develop by ovidiul.


5 months ago

  • Keywords has-patch added

This PR should address a naming mismatch and make the $allowedxmlnamedentities referenced on the kses.php file a global variable

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

#3 @SergeyBiryukov
5 months ago

  • Component changed from General to Formatting
  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 5.9

Hi there, welcome to WordPress Trac! Thanks for the report.

Good catch, introduced in [48072] / #50117. This could use some unit tests.

#4 @ovidiul
5 months ago

Added unit test for the wp_kses_xml_named_entities function inside the patch, however, I fail to find a simple way to test if the global names are setup properly inside kses.php file to prevent similar issues, other than maybe plain syntax check.

I've been trying to do this through spl_autoload_register, but that seems to require a more isolated environment outside WordPress Unit Tests.

Any suggestions?

Thanks.

#5 @prbot
2 months ago

ovidiul commented on PR #1651:

Hi @ovidiul! I've sent you a message on Slack with a diff with suggestions for this PR.

Thank you @costdev but I don't think I've received your message yet? Feel free to post the diff here as well.

#6 @prbot
2 months ago

costdev commented on PR #1651:

@ovidiul No problem!

Here's the diff and the details:

  • [x] Tidies up the docblock a little bit.

For consistency with docblock structure in other tests in the test suite + Clarity about the purpose of each argument.

  • [x] Adds named keys to the datasets.

For readability for other tests (and our future selves).

  • [x] Replaces the html_entity_decode() calls with the decoded characters in the data provider.

When testing a function, we should not use any functionality used directly in the function itself.

  • [x] Replaces html_entity_decode( '&nbsp;' ) with utf8_encode( chr( 160 ) ).

When testing a function, we should not use any functionality used directly in the function itself.
&nbsp; renders ASCII 160 rather than ASCII 32. See here.

  • [x] Adds a test and data provider to check that the globals that kses.php should define, are defined.

If this is needed.

#7 @prbot
2 months ago

ovidiul commented on PR #1651:

Thank you for the changes 🙇 @costdev they look great, I've merged them to this PR.

#8 @SergeyBiryukov
2 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#10 @prbot
2 months ago

SergeyBiryukov commented on PR #1651:

Thanks for the PR! This looks good to me, I only have two minor notes:

  • For consistency with the existing $allowedentitynames variable, I think it would be better to standardize on $allowedxmlentitynames here, instead of $allowedxmlnamedentities.
  • The null array key should probably be empty string, to avoid confusion with an actual null value, which is not being tested here.

I can make these adjustments on commit.

#11 @SergeyBiryukov
2 months ago

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

In 52229:

KSES: Use correct global in wp_kses_xml_named_entities().

This fixes a discrepancy where the the global name used in the function did not match the one declared at the beginning of kses.php, and ensures that the function gets the correct array of allowed XML entity names.

Includes unit tests.

Follow-up to [48072].

Props ovidiul, costdev, peterwilsoncc, SergeyBiryukov.
Fixes #54060.

Note: See TracTickets for help on using tickets.