Potential assignment to empty string

[jugglinmike]

Some of WordPress's built-in shortcodes assume that the provided $attr parameter
is an associative array. For instance, img_caption_shortcode is currently
defined with the following code:

    1489	/**
    1490	 * Builds the Caption shortcode output.
    [...]
    1503	 * @param array  $attr {
    1504	 *     Attributes of the caption shortcode.
    [...]
    1516	 */
    1517	function img_caption_shortcode( $attr, $content = null ) {
    1518	        // New-style shortcode with the caption inside the shortcode with the link and image tags.
    1519	        if ( ! isset( $attr['caption'] ) ) {
    1520	                if ( preg_match( '#((?:<a [^>]+>\s*)?<img [^>]+>(?:\s*</a>)?)(.*)#is', $content, $matches ) ) {
    1521	                        $content         = $matches[1];
    1522	                        $attr['caption'] = trim( $matches[2] );
    1523	                }
    1524	        } elseif ( strpos( $attr['caption'], '<' ) !== false ) {
    1525	                $attr['caption'] = wp_kses( $attr['caption'], 'post' );
    1526	        }

However, the shortcode parser can potentially return the empty string:

    483	/**
    484	 * Retrieve all attributes from the shortcodes tag.
    [...]
    493	 * @return array|string List of attribute values.
    494	 *                      Returns empty array if trim( $text ) == '""'.
    495	 *                      Returns empty string if trim( $text ) == ''.
    496	 *                      All other matches are checked for not empty().
    497	 */
    498	function shortcode_parse_atts( $text ) {

In PHP7, this behavior can cause problems for shortcodes which do not specify
any attributes. (Prior releases of PHP exposed ​surprising behavior for this case which averted the issue.)

To migrate my deployment to PHP7, I've preserved the behavior by overriding shortcodes on an as-needed basis:

    function img_caption_shortcode_supporting_empty( $attr, $content = null ) {
      if ( $attr == '' ) {
        $attr = array();
      }

      return img_caption_shortcode($attr, $content);
    }

    remove_shortcode('caption');
    add_shortcode('caption', 'img_caption_shortcode_supporting_empty');

But it seems like a few things should change in core:

• updating the documented type of the $attr parameter of shortcode functions
• tolerating the empty string

[jugglinmike]

Correction: ​the relevant change to PHP was in 7.1, not 7.0

In versions of PHP 7.0 and prior, if anything other than an array was passed into this function, it would be cast to an array at $attr['caption'] = trim( $matches[2] );

To maintain that behavior in newer versions of PHP, I added a type check that would explicitly perform this behavior.

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

[oglekler]

I added comment to PR. From my point of view, it still needs some work.

[oglekler]

This is a very simple patch but still needs a bit of a work. I am marking this as a good first bug.

[marybaum]

@@brianhenryie will look at the PR and add some unit tests.

Refresh of #4939

In versions of PHP 7.0 and prior, if anything other than an array was passed into this function, it would be cast to an array at $attrcaption? = trim( $matches[2] );

To maintain that behavior in newer versions of PHP, I added a type check that would explicitly perform this behavior.

• Added unit test
• Formatted existing patch (after @mukeshpanchal27 review)

[brianhenryie]

The patch certainly solves the problem as described.

There is a related issue – https://core.trac.wordpress.org/ticket/59249 – which will actually address this same ticket by only returning an empty array and never the empty string that was the cause here.

The first post of this thread says:

But it seems like a few things should change in core:

• updating the documented type of the $attr parameter of shortcode functions
• tolerating the empty string

The first will be incorrect after 59249, so has not been done.
The second has been done with the patch (now PR 5375).

[oglekler]

#59206 already fixed the issue in question, and #59249 is addressing a related issue.

So, I am suggesting to close this tickets as duplicate.

[brianhenryie]

#59206 is definitely a fix for this same issue.

The patch here is more robust and has a unit test, so it's worth considering still.

I.e. 59206 handles only falsy values, this patch handles any non-array values passed to the function.

[joedolson]

Since @swissspidy committed the existing fix to #59206, it would be helpful if he'd take a look here and see if this is a better approach.

[swissspidy]

Do we really need to handke more cases than empty string? I don‘t see how.

In any case, fixing this holistically for all shortcides would be great. So I‘m in favor of closing this as a dupe of #59249

[oglekler]

@swissspidy no, it is not a duplicate, the function in question is publicly accessible and this means that it can be called without appropriate arguments. And @brianhenryie has a point, these case can be covered with unit test, but the patch needs rebase, and it don't need isset(), because $attr is required by function and without it there will be a fatal error and this will be impossible to miss.

[nicolefurlan]

@brianhenryie @joedolson @swissspidy could you review the recent comments and provide feedback? RC1 is quickly approaching so we want to make sure we're continuing to push tickets forward.

Thank you :)

[swissspidy]

I don't think we should cater for cases where plugins call shortcode functions directly with invalid or missing arguments. Let's focus on the case where the parser currently returns an empty string, and make sure it returns an empty array instead. That's what we have #59249 for.

So again suggesting to close this in favor of #59249

[oglekler]

Let's check what others are thinking.

[oglekler]

We are out of time before RC1, so, I am moving this ticket to the next milestone.