WordPress.org

Make WordPress Core

Opened 2 years ago

#43215 new feature request

Allow wp_kses to pass allowed CSS properties

Reported by: mclaurent Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.9.2
Component: Security Keywords:
Focuses: Cc:

Description

wp_kses takes arguments for allowed HTML elements, HTML element attributes, and allowed protocols. It takes all of these directly via the function argument through a nicely formatted assoc array. On the side side, attribute values (i.e. "display: block; visibility: hidden;") are configured via a hook, which means they are changed globally for all future calls.

Here a sample how the system is behaving currently:

<?php
$allowed_output_html = array(
        'script' => array(),
        'noscript' => array(),
        'iframe' => array(
                'src' => array(),
                'width' => array(),
                'height' => array(),
                'style' => array(),
        ),
);

$allowed_output_protocol = array(
        'https',
        'javascript',
);

$google_tag_manager_noscript <<<'ENDSTRING'
<noscript><iframe src="https://www.googletagmanager.com/ns.html?id=123"
        height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript>
ENDSTRING;

echo wp_kses( $google_tag_manager_noscript, $allowed_output_html, $allowed_output_protocol);

// Outputs as it should except for the style="display:none;visibility:hidden" portion
// <noscript><iframe src="https://www.googletagmanager.com/ns.html?id=123" height="0" width="0"></iframe></noscript>

It seems that currently the only way to allow the "style" attribute from showing is to write a filter for "safe_style_css" to add in the "display" and "visibility" attributes, however this means that we are globally altering this behavior, which in other scenarios would allow unexpected HTML to appear (ie unsafe element properties). Even when removing the items immediately after executing the code will not work because the attribute may in the future be consider secure. This then causes an inconsistent execution.
For example, imagine that if we wanted to make sure the "float" attribute was allowed in the attribute value. Currently this value is allowed, so us adding it in won't change the array. However when we're then undoing our action (ie removing the "float" attribute from the list of allowed attributes, we are actually removing one that was set as "safe" by WordPress themselves. It is easy to imaging how this could escalate further, when plugins blanket whitelist element attributes, then remove them from the list, then meaning that no attributes are allowed.
Alternatively, in this same example if we removed our filter (with remove_filter) right after our wp_kses call, it would mean that our attribute whitelist would apply to all HTML attributes (display, visibility,...) and not only the one we would want to whitelist (style).

<?php

// Function to whitelist safe CSS attributes
function my_safe_css_attributes( $array ){
   $array[] = 'display';
   $array[] = 'visibility';
   return $array;
});

// Function to remove our CSS attributes from the whitelist
function my_safe_css_remove_attributes( $array ){
   unset $array['display'];
   unset $array['visibility'];
   return $array;
}

$allowed_output_html = array(
        'script' => array(),
        'noscript' => array(),
        'iframe' => array(
                'src' => array(),
                'width' => array(),
                'height' => array(),
                'style' => array(),
        ),
);

$allowed_output_protocol = array(
        'https',
        'javascript',
);

$google_tag_manager_noscript <<<'ENDSTRING'
<noscript><iframe src="https://www.googletagmanager.com/ns.html?id=123"
        height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript>
ENDSTRING;

// Telling WordPress about safe CSS attributes...
add_filter( 'safe_style_css', 'my_safe_css_attributes' );

echo wp_kses( $google_tag_manager_noscript, $allowed_output_html, $allowed_output_protocol);

add_filter( 'safe_style_css', 'my_safe_css_remove_attributes' );

// Outputs as it should.
// <noscript><iframe src="https://www.googletagmanager.com/ns.html?id=123" height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript>

I initially expected the allowed_output_html to allow overriding allowed attribute values from the "style" element in the array, which I thought would have been a really neat way of overwriting the execution on a one-time basis.

<?php
$allowed_output_html = array(
        'script' => array(),
        'noscript' => array(),
        'iframe' => array(
                'src' => array(),
                'width' => array(),
                'height' => array(),
                'style' => array(
                        'display', 'visibility'
                ),
        ),
);

// ...

I think it would make sense to make the function accept this added level of depth as it allows very flexible configuration of what attributes are allowed without compromising attribute

Change History (0)

Note: See TracTickets for help on using tickets.