WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 3 months ago

#43312 closed defect (bug) (fixed)

PHP 7.2 warning in wp_kses_attr()

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

Description

The function: wp_kses_attr() outpus a warning on newer PHP 7.2.

this function:

<?php
function wp_kses_attr($element, $attr, $allowed_html, $allowed_protocols) {
        if ( ! is_array( $allowed_html ) )
                $allowed_html = wp_kses_allowed_html( $allowed_html );

        // Is there a closing XHTML slash at the end of the attributes?
        $xhtml_slash = '';
        if (preg_match('%\s*/\s*$%', $attr))
                $xhtml_slash = ' /';

        // Are any attributes allowed at all for this element?
        if ( ! isset( $allowed_html[ strtolower( $element ) ] ) || true === $allowed_html[ strtolower( $element ) ] || count( $allowed_html[ strtolower( $element ) ] ) == 0 ) {
                return "<$element$xhtml_slash>";
        }

        // Split it
        $attrarr = wp_kses_hair($attr, $allowed_protocols);

        // Go through $attrarr, and save the allowed attributes for this element
        // in $attr2
        $attr2 = '';
        foreach ( $attrarr as $arreach ) {
                if ( wp_kses_attr_check( $arreach['name'], $arreach['value'], $arreach['whole'], $arreach['vless'], $element, $allowed_html ) ) {
                        $attr2 .= ' '.$arreach['whole'];
                }
        }

        // Remove any "<" or ">" characters
        $attr2 = preg_replace('/[<>]/', '', $attr2);

        return "<$element$attr2$xhtml_slash>";
}

can be changed to this:

this function:

<?php
function wp_kses_attr($element, $attr, $allowed_html, $allowed_protocols) {
        if ( ! is_array( $allowed_html ) )
                $allowed_html = wp_kses_allowed_html( $allowed_html );

        // Is there a closing XHTML slash at the end of the attributes?
        $xhtml_slash = '';
        if (preg_match('%\s*/\s*$%', $attr))
                $xhtml_slash = ' /';

        // Are any attributes allowed at all for this element?
        if ( ! isset( $allowed_html[ strtolower( $element ) ] ) || true === $allowed_html[ strtolower( $element ) ] || empty( $allowed_html[ strtolower( $element ) ] ) ) {
                return "<$element$xhtml_slash>";
        }

        // Split it
        $attrarr = wp_kses_hair($attr, $allowed_protocols);

        // Go through $attrarr, and save the allowed attributes for this element
        // in $attr2
        $attr2 = '';
        foreach ( $attrarr as $arreach ) {
                if ( wp_kses_attr_check( $arreach['name'], $arreach['value'], $arreach['whole'], $arreach['vless'], $element, $allowed_html ) ) {
                        $attr2 .= ' '.$arreach['whole'];
                }
        }

        // Remove any "<" or ">" characters
        $attr2 = preg_replace('/[<>]/', '', $attr2);

        return "<$element$attr2$xhtml_slash>";
}

I have also forked today your git repo and write a change the file here is the link: https://github.com/andrei0x309/WordPress/commit/6d6d784481c3875f4da08f8655e6d0f5dfa78fe1

Thanks in advance.

Attachments (2)

43312.patch (626 bytes) - added by SergeyBiryukov 4 months ago.
43312.tests.patch (619 bytes) - added by soulseekah 4 months ago.
Errors (false does not implement Countable) on PHP 7.2 without 43312.patch, passes with patch

Download all attachments as: .zip

Change History (28)

#1 @SergeyBiryukov
4 months ago

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

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

To clarify, the warning is Warning: count(): Parameter must be an array or an object that implements Countable, and the suggested change is to replace count( $allowed_html[ strtolower( $element ) ] ) == 0 with empty( $allowed_html[ strtolower( $element ) ] ),

Last edited 4 months ago by SergeyBiryukov (previous) (diff)

#2 follow-up: @SergeyBiryukov
4 months ago

  • Keywords has-patch added

I think we can combine the empty() check with ! isset(), see 43312.patch.

#3 follow-up: @SergeyBiryukov
4 months ago

  • Summary changed from wp-includes\kses.php to PHP 7.2 warning in wp_kses_attr()

#4 @ocean90
4 months ago

  • Keywords needs-unit-tests added

#5 in reply to: ↑ 3 @andrei0x309
4 months ago

  • Resolution set to worksforme
  • Status changed from new to closed

Replying to SergeyBiryukov: Thanks for replying, I've seen your patch and is perfect, it removes the warning.

#6 @SergeyBiryukov
4 months ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Tickets are closed when a commit is made to WordPress core :)

@soulseekah
4 months ago

Errors (false does not implement Countable) on PHP 7.2 without 43312.patch, passes with patch

#7 @soulseekah
4 months ago

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

#8 in reply to: ↑ 2 @ocean90
3 months ago

  • Keywords reporter-feedback added
  • Version 4.9.4 deleted

@andrei0x309 Is it correct that the value which had triggered the warning was false? If so, doesn't that mean that the element is actually not allowed? While 43312.patch makes sense, false isn't really a supported value.

#9 @soulseekah
3 months ago

Hmm, agreed, false should not really be supported, the default $allowedtags structure lists elements with no supported attributes with an empty array (array()). What's the use case here @andrei0x309, how did you find out about this warning? Is core code calling this incorrectly?

Perhaps we should scan the plugin directory, @SergeyBiryukov for the incorrect usage/structure of $allowedtags, etc.

#10 @andrei0x309
3 months ago

  • Resolution set to invalid
  • Status changed from reopened to closed

Sorry guys I did a double check, @ocean90 is right, this warning was triggered when wp_kses was used with the unsupported false value.

At the time when I had reported the issue I've disabled all plugins, but now after I took a more deep look I have seen that the problem was because the theme I have used called wp_kess like this:

<?php
wp_kses( __('--SOME HTML HERE--', 'theme' ), array( 'strong' => '' ))

Which should have been:

<?php
wp_kses( __('--SOME HTML HERE--', 'theme' ), array( 'strong' => array() ))

I don't know how many developers pass an unsupported value since before PHP 7.2 it was not an issue.

Thanks, and sorry for the ticket.

#11 @andrei0x309
3 months ago

  • Keywords reporter-feedback removed

#12 @SergeyBiryukov
3 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Even though false is not supported, I don't think it should throw a warning.

Some sites currently display this warning in public.

#13 @soulseekah
3 months ago

Agreed, if there's incorrect usage out there, we should adapt and make the extra check. The patch and the tests are good to go if you guys would like to merge them.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 months ago

#15 @audrasjb
3 months ago

  • Keywords commit added

#16 @SergeyBiryukov
3 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from reopened to closed

In 42860:

Formatting: Avoid a PHP 7.2 warning in wp_kses_attr() when one of $allowedtags elements is an uncountable value.

Props andrei0x309, soulseekah, SergeyBiryukov.
Fixes #43312.

#17 @SergeyBiryukov
3 months ago

In 42861:

Formatting: Avoid a PHP 7.2 warning in wp_kses_attr() when one of $allowedtags elements is an uncountable value.

Props andrei0x309, soulseekah, SergeyBiryukov.
Merges [42860] to the 4.9 branch.
Fixes #43312.

#18 @apokalyptik
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

In r42861 the new conditional logic is not equivalent to the old conditional logic.

  $ php -v
    PHP 7.0.27-0+deb9u1 (cli) (built: Jan  5 2018 13:51:52) ( NTS )

  $ cat test.php
    <?php
    $element = 'foo';
    $allowed_html = array( 'foo' => false );
    var_dump( ( ! isset( $allowed_html[ strtolower( $element ) ] ) || true === $allowed_html[ strtolower( $element ) ] || count( 
    $allowed_html[ strtolower( $element ) ] ) == 0 ) );
    $element_low = strtolower( $element );
    var_dump( ( empty( $allowed_html[ $element_low ] ) || true === $allowed_html[ $element_low ] ) );

  $ php ./test.php
    bool(false)
    bool(true)

as you can see above empty() returns true in places where isset() does not.

ironically this use of empty covers the omission of the empty array case. but once that is fixed you'll want to check if it's an array and if so then count and check to keep the logic consistent.

if the logic needs to be changed for what is and is not supported, then that's fine but kses is pretty important and so it shouldn't be hidden in a ticket about fixing warnings (which to me implies that it's about the warning and not the logic/functionality and therefor tends to get less scrutiny)

#19 @soulseekah
3 months ago

@apokalyptik Oooh, thanks. What's test case here? Why haven't one of the many possible test failures been triggered?

#20 @apokalyptik
3 months ago

@soulseekah I was simply asked to review the patch, and noted the difference on visual inspection of the diff. All I know is that the diff changes the logic of the code being touched: That given the same specific input the output of the code block differs pre and post patch.

#21 @ocean90
3 months ago

@apokalyptik What are you proposing here? Just renaming the ticket to something else? Unless I'm missing something, the output is the same. Mostly because wp_kses_attr_check() previously returned false in this case.

#22 @apokalyptik
3 months ago

I'm proposing that a patch pushed through on this ticked should keep the logic the same, piece for piece, and fix the php 7.2 warnings. If you think kses needs to function differently in some way... you should make another ticket and it should be addressed there... even the possibility of subtly breaking kses is an unacceptable risk when including a patch that ostensibly only silences an error.

#23 @ocean90
3 months ago

I still don't see how it's function differently now? Could you explain this with a real world use case please?

#24 @soulseekah
3 months ago

counting a boolean will always return 1. Be it true or false, doesn't matter. So the old logic is actually flawed. The wp_kses_attr did indeed not short-circuit in the first condition and fell through doing useless, extra work (wp_kses_hair, preg_replace). But if you look at the resulting string and what it is composed of, then you'll see that it is absolutely the same as in the case of short-circuiting.

return "<$element$attr2$xhtml_slash>"; is equal to return "<$element$xhtml_slash>";, $attr2 is an empty string of attributes, composed of $attrarr which is empty when false is passed.

Thus, the logic is absolutely identical, minus the extra work that nobody really cares about. If anything, WordPress became faster :)

#25 @apokalyptik
3 months ago

That is a breakdown I can get behind, the work for the proof needed to be done and it needed to be explicit and explained how this was still correct and functionally equivalent. Thanks very much :)

#26 @SergeyBiryukov
3 months ago

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

Right, while the logic of that particular piece has changed, the logic of the function is still the same. It just bails earlier, avoiding some extra work.

Thank you for raising the issue, it's good to have this documented.

Note: See TracTickets for help on using tickets.