Make WordPress Core

Opened 5 years ago

Closed 5 months ago

Last modified 5 months ago

#45929 closed defect (bug) (duplicate)

Potential assignment to empty string

Reported by: jugglinmike's profile jugglinmike Owned by: swissspidy's profile swissspidy
Milestone: Priority: normal
Severity: normal Version: 5.0.2
Component: Shortcodes Keywords: has-patch changes-requested good-first-bug has-unit-tests close 2nd-opinion
Focuses: Cc:

Description

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

Change History (23)

This ticket was mentioned in PR #4939 on WordPress/wordpress-develop by mausmalone.


11 months ago
#2

  • Keywords has-patch added

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

#3 @joedolson
11 months ago

  • Milestone changed from Awaiting Review to 6.4
  • Owner set to joedolson
  • Status changed from new to accepted

#4 @oglekler
10 months ago

  • Keywords changes-requested added

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

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


9 months ago

#6 @oglekler
9 months ago

  • Keywords good-first-bug added

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

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


9 months ago

#8 @marybaum
9 months ago

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

This ticket was mentioned in PR #5375 on WordPress/wordpress-develop by @brianhenryie.


9 months ago
#9

  • Keywords has-unit-tests added

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)

#10 @brianhenryie
9 months ago

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 incorrect after 59249, so has not been done.
The second has been done with the patch (now PR 5375).

Version 0, edited 9 months ago by brianhenryie (next)

#11 @oglekler
9 months ago

  • Keywords close added

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

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

#12 @brianhenryie
9 months ago

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

#13 @joedolson
9 months ago

  • Owner changed from joedolson to swissspidy
  • Status changed from accepted to assigned

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.

#14 @swissspidy
9 months ago

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

#15 @oglekler
9 months ago

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

#16 @nicolefurlan
9 months ago

@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 :)

#17 @swissspidy
9 months ago

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

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


9 months ago

#19 @oglekler
9 months ago

  • Keywords 2nd-opinion added

Let's check what others are thinking.

#20 @oglekler
8 months ago

  • Milestone changed from 6.4 to 6.5

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

#21 @swissspidy
5 months ago

  • Milestone 6.5 deleted
  • Resolution set to duplicate
  • Status changed from assigned to closed

Duplicate of #59249.

Note: See TracTickets for help on using tickets.