WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#12089 closed defect (bug) (fixed)

Unintended blank lines in plugins break XML for feeds

Reported by: bchecketts Owned by: MarkJaquith
Milestone: 3.0 Priority: normal
Severity: normal Version:
Component: Plugins Keywords: has-patch tested dev-feedback
Focuses: Cc:

Description

A plugin or theme file may unintentionally output blank lines. When displaying an RSS feed, this breaks XML validation of the page.

To recreate, simply create and activate a plugin that has an extra blank line after the closing ?> characters in the file.

Google Chrome displays this error. Other browsers display something similar:

error on line 4 at column 6: XML declaration allowed only at the start of the document

Although technically a problem with the plugin, Wordpress can easily be modified to discard the undesired output by calling ob_clean() prior to outputting the XML. It seems that ob_start() is already called earlier, so ob_clean() discards any output since that call.

For example, in the default configuration, you can add this line in wp-includes/feed-rss2.php just above the header() function call (in about line 7)

ob_clean();

The top of wp-includes/feed-rss2.php now looks like this:

<?php
/**
 * RSS2 Feed Template for displaying RSS2 Posts feed.
 *
 * @package WordPress
 */

ob_clean();
header('Content-Type: ' . feed_content_type('rss-http') . '; charset=' . get_option('blog_charset'), true);
$more = 1;

Attachments (7)

testplug.php (391 bytes) - added by miqrogroove 4 years ago.
For anyone who's curious, this plugin caused a deactivation failure.
plugin-deactivation.patch (758 bytes) - added by miqrogroove 4 years ago.
Hack for wp_redirect errors
plugin-activation.patch (798 bytes) - added by miqrogroove 4 years ago.
12089.diff (3.3 KB) - added by miqrogroove 4 years ago.
Full solution with new "sandbox" error for unexpected output.
12089.2.diff (4.0 KB) - added by miqrogroove 4 years ago.
Don't override WP_DEBUG, props sivel. Also discard the error control operator.
12089-extra.diff (598 bytes) - added by miqrogroove 4 years ago.
Fix error window scope
12089.deactivation-header-variant.patch (1.3 KB) - added by hakre 4 years ago.
Just a slight variant of play

Download all attachments as: .zip

Change History (27)

comment:1 filosofo4 years ago

This complaint appears every so often (see #9610 for example), and the consensus seems to be that we shouldn't add core bloat to paste over plugins' problems.

It seems that ob_start() is already called earlier,

Where is that?

comment:2 follow-ups: miqrogroove4 years ago

  • Milestone changed from Unassigned to 3.0

It would be so easy to fix this. function activate_plugin() calls ob_end_clean() without checking the buffer. With an if statement a few lines above that, WordPress could assert wp_error is returned if the buffer isn't empty. In effect, anyone who tries to activate a plugin that dirties the output stream will see a syntax error and the plugin will not activate.

comment:3 in reply to: ↑ 2 nacin4 years ago

According to the phpdoc, it looks like that is what was intended:

 333   * If any errors are found or text is outputted, then it will be captured to
 334   * ensure that the success redirection will update the error redirection.

comment:4 in reply to: ↑ 2 filosofo4 years ago

Replying to miqrogroove:

It would be so easy to fix this. function activate_plugin() calls ob_end_clean() without checking the buffer. With an if statement a few lines above that, WordPress could assert wp_error is returned if the buffer isn't empty. In effect, anyone who tries to activate a plugin that dirties the output stream will see a syntax error and the plugin will not activate.

The problem is that often these errors creep in attached to action hooks that won't fire during activation.

comment:5 follow-up: miqrogroove4 years ago

I was under the impression this ticket was about syntax errors such as whitespace after the closing PHP tag. If a plugin is randomly generating "blank lines" as a result of actions, then it's an awful, horrible plugin. :P

Still, wouldn't it be nice to catch those closing tag errors using just a few extra lines of code?

comment:6 in reply to: ↑ 5 filosofo4 years ago

Replying to miqrogroove:

Still, wouldn't it be nice to catch those closing tag errors using just a few extra lines of code?

In my opinion it would be better to give plugins the necessary feedback they need to correct the problem (e.g., "I activated your plugin and my feed broke.").

If we're going to go the route of flushing all output printed before X event, we need to do it in a thorough, controlled way, perhaps as part of a templating system, not just tacked on to a few arbitrary parts of WP (such as feeds).

Besides, how would you propose to implement this? If you start output buffering say before plugins are loaded, when are you going to flush it in the non-feed situations?

comment:7 follow-up: miqrogroove4 years ago

Maybe we're not on the same page yet. I'm thinking of heres-my-bad-plugin.php:

<?php
/** Plugin-Author: Me. **/
add_action('blah', 'phpinfo');
?>
oops there's stuff here

In activate_plugin, there's a buffer that's used to block non-fatal errors. If the buffer survives, it gets deleted and the plugin is activated. I'm proposing a fix so that the buffer is examined, and if it's not empty for any reason, it should get reported the same way as the fatal errors. The buffer could say "oops there's stuff here", or it could just as easily say, "Warning: blah not defined, assuming 'blah'". Prefix that with something informative like, "This plugin generated the following unexpected output:"

comment:8 in reply to: ↑ 7 filosofo4 years ago

Replying to miqrogroove:

Maybe we're not on the same page yet. I'm thinking of heres-my-bad-plugin.php:

Ok, I thought you were talking about fixes like the OP's, not just during activation.

I'm proposing a fix so that the buffer is examined, and if it's not empty for any reason, it should get reported the same way as the fatal errors.

That sounds like a good idea.

miqrogroove4 years ago

For anyone who's curious, this plugin caused a deactivation failure.

comment:9 miqrogroove4 years ago

I just did a baseline test and found that the admin screen failed to load when I deactivated the plugin. I'll see if I can pin that down while I'm hacking on the activation parts.

miqrogroove4 years ago

Hack for wp_redirect errors

comment:10 miqrogroove4 years ago

This version gets us to a wp_die() screen when activating testplug.php. It's simple and it works. I'm looking now at how crazy it would get if we try to keep it in the "sandbox" rather than using wp_die().

miqrogroove4 years ago

Full solution with new "sandbox" error for unexpected output.

comment:11 miqrogroove4 years ago

  • Component changed from Feeds to Plugins
  • Keywords has-patch needs-testing added
  • Owner set to westi
  • Priority changed from low to normal

And now it's ready :)

miqrogroove4 years ago

Don't override WP_DEBUG, props sivel. Also discard the error control operator.

comment:12 nacin4 years ago

  • Owner changed from westi to MarkJaquith
  • Status changed from new to assigned

Mark wrote the original sandbox. Assigning to him.

comment:13 miqrogroove4 years ago

  • Keywords tested added; needs-testing removed
  • Resolution set to fixed
  • Status changed from assigned to closed

Changeset [13167] by markjaquith

Detect plugin output on activation, as well as fatal errors. props miqrogroove. fixes #12089

comment:14 hakre4 years ago

That would not be for feeds only but for proerply parsed "XHTML as in XML" pages as well, right?


What I strongly dislike with this patch is that it is using ob_ output functions. The usage of those has not been clearly defined until today. That can really mess things up and this patch might not work as intended.

the ob-length need to be measured before and after the include. the delta between the two values should be 0, not the overall length. that should make it more robust.

I would love to see a comment on this one prior to open (another) new ticket.

comment:15 hakre4 years ago

  • Keywords dev-feedback added

comment:16 nacin4 years ago

ob_start() is right above the include. On activation a plugin should not be echoing anything, whether it is a warning or blank lines. So your delta is equal to ob_get_length().

comment:17 miqrogroove4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Viper007Bond reported an error activating Akismet in debug mode, where the error was reproducible in the sandbox, but would never display in the error window.

Reopen. Expect patch to put the error window in the sandbox so as to generate identical output during the error_scrape.

miqrogroove4 years ago

Fix error window scope

comment:18 markjaquith4 years ago

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

(In [13452]) Plugin activation attempt goes through a function to show the same errors as the test. props miqrogroove. fixes #12089

hakre4 years ago

Just a slight variant of play

comment:19 hakre4 years ago

Plugins that are reporting output on activation should be considered to be deactived completely. I think about adding a button if that happens on activation so a user can directly undo the activation.

Could be a more user-friendly step in between preventing the activation completely.

comment:20 hakre3 years ago

Related: #16485

Note: See TracTickets for help on using tickets.