SimplePie: DOMDocument can be disabled

[nacin]

SimplePie uses DOMDocument in a few places. But, it can be disabled, either by a distro messing with PHP (see Redhat) or with --disable-dom. When we use DOMDocument in core, we always check class_exists(). SimplePie does not.

We'll need to get this fixed to avoid fatal errors when SimplePie is used.

SimplePie 1.2 did not use DOMDocument. I wonder what else was added/used in 1.3 that could potentially be disabled.

[nacin]

Thanks dd32 for the report.

[nacin]

Upstream: ​https://github.com/simplepie/simplepie/issues/241

[rmccue]

Regarding changes: this occurred because the autodiscovery was rewritten to use DOMDocument instead of parsing HTML with regex. I don't want to switch back to regex, so I'm willing to just disable autodiscovery on installations without DOMDocument.

Thanks for the upstream, I'll get on that later today if I can.

[nacin]

Replying to rmccue:

I don't want to switch back to regex, so I'm willing to just disable autodiscovery on installations without DOMDocument.

Perfectly fine with me.

[rmccue]

Fixed for one-dot-three: ​https://github.com/simplepie/simplepie/commit/f0e144e1952e4ca083f11941d482f65f71f46397

[rmccue]

Marking as has-patch for nacin to port across to WP.

[rmccue]

Duplicate of #22321.

[ocean90]

Seems like 1.3.1 missed a placed, where new DOMDocument() is called. See source:/trunk/wp-includes/SimplePie/Sanitize.php@22639#L250​

Still ends with a fatal error, reported in the forum: ​http://wordpress.org/support/topic/dashboardplugins-simplepie-error?replies=5#post-3399249

[markjaquith]

First attempt at subclassing SimplePie_Sanitize

[markjaquith]

First attempt at sublassing SimplePie_Sanitize. New file needed because the SP files are loaded conditionally, and I wanted to leave those pristine. Avoided the fancy registration stuff, because we're manually instantiating the sanitization object ourselves. So I just changed that. Only did very preliminary testing: both branches of logic seem to not break the dashboard RSS feed.

Will need PHPDoc, security testing, and unit testing — just wanted to get a first effort up here ASAP.

[markjaquith]

Also, the logic was swiped from rmccue's recommendation on IRC. That could use auditing.

[markjaquith]

Doesn't touch SP files.

[markjaquith]

21990-subclass.2.diff​ doesn't touch class-simplepie.php, and instead modifies fetch_feed().

Note that registering the sanitization class isn't enough, as $feed->sanitize has already been instantiated as the default SP sanitization class. So after registering, I explicitly instantiate the subclass over top of that.

[markjaquith]

with documentation and inline comments

[markjaquith]

Existing unit tests run cleanly. Documentation added in 21990-subclass.3.diff​. We should consider adding unit tests to make sure no untrusted data can seep through. Still needs more thorough testing and feedback.

[rmccue]

Looking again at SimplePie_Sanitize, looks like we'll also need to test SIMPLEPIE_CONSTRUCT_MAYBE_HTML and SIMPLEPIE_CONSTRUCT_BASE64.

[rmccue]

Update patch to also check SIMPLEPIE_CONSTRUCT_MAYBE_HTML and base 64 encoding

[nacin]

In 22811:

Do SimplePie sanitization with wp_kses_post() rather than DOMDocument, which cannot be guaranteed to be available.

Overrides SimplePie_Sanitize with WP_SimplePie_Sanitize_KSES.

props markjaquith, rmccue.
see #21990.

[nacin]

Nice work. In [22811], I tossed WP_SimplePie_Sanitize_KSES into the existing class-feed.php file where we already extend SimplePie_File, SimplePie_Cache, etc.

Lowering priority and severity now that this is no longer critical under most situations.

We still need to update SimplePie core to avoid calling DOMDocument when it is not available. A plugin using SimplePie directly (thus sidestepping fetch_feed()) will not be able to rely on kses, but they should still be given an opportunity to avoid the fatal error. My suggestion would be to pass an error back up the stack (if that makes sense — I'm not that familiar with SP) or just return '', as rmccue and I had discussed.

[iandunn]

This fixed the problem for me. The dashboard is no longer crashing in 3.5-RC1 because DOMDocument isn't loaded.

[rmccue]

Replying to nacin:

We still need to update SimplePie core to avoid calling DOMDocument when it is not available. A plugin using SimplePie directly (thus sidestepping fetch_feed()) will not be able to rely on kses, but they should still be given an opportunity to avoid the fatal error. My suggestion would be to pass an error back up the stack (if that makes sense — I'm not that familiar with SP) or just return '', as rmccue and I had discussed.

Due to the nature of SimplePie::sanitize(), passing up the stack is best done by throwing an exception where needed. To ensure API compatibility, that'll need to be shielded from the end-user, but in the next major version, they'll have the ability to get those exceptions themselves. I know how I'm going to fix this, so I'll get that patched upstream ASAP.

[nacin]

We still need to update SimplePie core to avoid calling DOMDocument when it is not available.

Not seeing anything upstream. Is there a new timeline for this? Happy to look, but if you know how you're going to fix it, I'd rather leave it for you.

[nacin]

21990.diff​ — This brace styling is going to be the death of me.

[rmccue]

​Patched upstream. Fix is similar to nacin's patch​, but with better whitespacing, consistent with the SimplePie style guidelines.

(Marking as needs-patch, since it doesn't have a WP diff for it, but it's simple to port from the ​raw diff.)

[nacin]

In 22970:

SimplePie: Return nothing and throw an error in SimplePie_Sanitize when DOMDocument is disabled.

Note that when SimplePie is used through the WordPress fetch_feed() function, we use kses rather than SimplePie_Sanitize, which removes the dependency on DOMDocument. This change is only for plugins using SimplePie directly.

props rmccue. fixes #21990.

[nacin]

Replying to rmccue:

with better whitespacing

*spits water everywhere*