Make WordPress Core

Opened 16 months ago

Closed 3 weeks ago

Last modified 3 weeks ago

#58798 closed defect (bug) (fixed)

Fix possible PHP warning in /wp-includes/feed.php

Reported by: zahardoc's profile zahardoc Owned by: johnjamesjacoby's profile johnjamesjacoby
Milestone: 6.7 Priority: normal
Severity: normal Version: 6.2.2
Component: Feeds Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

If some plugins or themes use "enclosure" meta field as a one-line string, it results in PHP warnings:

PHP Warning: Undefined array key 2 in /wp-includes/feed.php on line 484
PHP Warning: Undefined array key 1 in /wp-includes/feed.php on line 494

It happens because there is no check of string content there:

foreach ( (array) get_post_custom() as $key => $val ) {
	if ( 'enclosure' === $key && is_array( $val ) ) {
		foreach ( (array) $val as $enc ) {
			$enclosure = explode( "\n", $enc );


			/ Only get the first element, e.g. 'audio/mpeg' from 'audio/mpeg mpga mp2 mp3'.
			$t    = preg_split( '/[ \t]/', trim( $enclosure[2] ) );
			$type = $t[0];

				/**
				 * Filters the RSS enclosure HTML link tag for the current post.
				 *
				 * @since 2.2.0
				 *
				 * @param string $html_link_tag The HTML link tag with a URI and other attributes.
				 */
		echo apply_filters( 'rss_enclosure', '<enclosure url="' . esc_url( trim( $enclosure[0] ) ) . '" length="' . absint( trim( $enclosure[1] ) ) . '" type="' . esc_attr( $type ) . '" />' . "\n" );
			}
		}
	}

Proposal:

Add checks if the enclosure is an array, and each item can be exploded into exactly 3 items:

if ( 'enclosure' === $key && is_array( $val ) ) {}

And this:

$enclosure = explode( "\n", $enc );

if ( 3 !== count( $enclosure ) ) {
	continue;
}

Attachments (1)

58798.patch (1.3 KB) - added by nihar007 16 months ago.
Patch added

Download all attachments as: .zip

Change History (16)

@nihar007
16 months ago

Patch added

#1 @rghedin
15 months ago

Any workaround while this isn't fixed on WP?

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


12 months ago

#3 @arypneta
6 months ago

Any update on a fix for this issue? It's really cluttering up our debug logs and making it hard to see actual issues.

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


5 months ago
#4

  • Keywords has-patch added; needs-patch removed

#5 follow-up: @johnjamesjacoby
2 months ago

  • Milestone changed from Awaiting Review to 6.7
  • Owner set to johnjamesjacoby
  • Status changed from new to assigned

Hi @zahardoc, thanks for making this ticket here.

I've personally confirmed your findings, and would like to get this fixed in the next major release.

@nihar007, your patch appears to include some unintended changes, so my plan is to base an updated patch on the Pull Request from @narenin on GitHub (though you'll still both get props for contributing to the fix).


For added context, I was seeing this on a site that was running the Seriously Simple Podcasting plugin, because is uses the enclosure post-meta key with only a single-line entry, rather than the multi-line format that WordPress is coded to expect (see: do_enclose()).


Additionally, I think the same fix should be applied to both the rss_enclosure() and atom_enclosure() functions.


Lastly, these functions are ooooold, so I'll be resisting the urge to do any other improvements to the surrounding code, hugging closely to the suggested changes already here.

#6 @peterwilsoncc
5 weeks ago

@johnjamesjacoby Do you think you will have a chance to follow this up prior to the release candidate in a couple of weeks?

#7 @johnjamesjacoby
5 weeks ago

@peterwilsoncc yes, I do!

#8 in reply to: ↑ 5 @Rahmohn
4 weeks ago

Replying to johnjamesjacoby:

Hi @zahardoc, thanks for making this ticket here.

I've personally confirmed your findings, and would like to get this fixed in the next major release.

@nihar007, your patch appears to include some unintended changes, so my plan is to base an updated patch on the Pull Request from @narenin on GitHub (though you'll still both get props for contributing to the fix).


For added context, I was seeing this on a site that was running the Seriously Simple Podcasting plugin, because is uses the enclosure post-meta key with only a single-line entry, rather than the multi-line format that WordPress is coded to expect (see: do_enclose()).


Additionally, I think the same fix should be applied to both the rss_enclosure() and atom_enclosure() functions.


Lastly, these functions are ooooold, so I'll be resisting the urge to do any other improvements to the surrounding code, hugging closely to the suggested changes already here.

Hi @johnjamesjacoby

I found these other functions that this fix could be applied:

  • wp_xmlrpc_server::_prepare_post
  • wp_xmlrpc_server::mw_getPost

Let me know if you need some help. I think I can write some unit tests for these functions.

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


4 weeks ago
#9

  • Keywords has-unit-tests added

This PR checks the size of enclosure meta field in rss_enclosure() function and add some tests to this function.

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

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


4 weeks ago
#10

This PR checks the size of enclosure meta field in rss_enclosure() function and add some tests to this function.

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

#11 @Rahmohn
4 weeks ago

I've created a PR to fix this possible PHP warning. Also, I created some tests for the function rss_enclosure().

I noticed that the function atom_enclosure() doesn't need to be applied the same fix because this function sets default values when the enclosure meta field is not stored in a multiline string i.e. this function checks if the keys exist before accessing them:

<?php
// Parse URL.
if ( isset( $enclosure[0] ) && is_string( $enclosure[0] ) ) {
        $url = trim( $enclosure[0] );
}

// Parse length and type.
for ( $i = 1; $i <= 2; $i++ ) {
        if ( isset( $enclosure[ $i ] ) ) {
                if ( is_numeric( $enclosure[ $i ] ) ) {
                        $length = trim( $enclosure[ $i ] );
                } elseif ( in_array( $enclosure[ $i ], $mimes, true ) ) {
                        $type = trim( $enclosure[ $i ] );
                }
        }
}

@Rahmohn commented on PR #7557:


3 weeks ago
#12

Thanks for your review @peterwilsoncc 🙏

@Rahmohn commented on PR #7557:


3 weeks ago
#13

Added a couple more notes re naming things.

Are you able to run git rm tests/phpunit/tests/feed/feed.php tests/phpunit/tests/feed/rssEnclosure.php too (please validate command, untested).

Sure.

✔️

#14 @peterwilsoncc
3 weeks ago

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

In 59241:

Feeds: Validate enclosures prior to generating tags.

Prevent possible PHP warnings caused by malformed enclosure meta data. This change ensures the enclosure meta data has at least three lines of text before generating the tag in rss_enclosure().

Props arypneta, johnjamesjacoby, nihar007, rahmohn, rghedin, zahardoc.
Fixes #58798.

Note: See TracTickets for help on using tickets.