WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#21990 closed defect (bug) (fixed)

SimplePie: DOMDocument can be disabled

Reported by: nacin Owned by: rmccue
Milestone: 3.5 Priority: low
Severity: normal Version: 3.5
Component: External Libraries Keywords: needs-unit-tests needs-patch
Focuses: Cc:

Description

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.

Attachments (5)

21990-subclass.diff (1.3 KB) - added by markjaquith 2 years ago.
First attempt at subclassing SimplePie_Sanitize
21990-subclass.2.diff (1.1 KB) - added by markjaquith 2 years ago.
Doesn't touch SP files.
21990-subclass.3.diff (1.7 KB) - added by markjaquith 2 years ago.
with documentation and inline comments
21990-subclass.4.diff (2.3 KB) - added by rmccue 2 years ago.
Update patch to also check SIMPLEPIE_CONSTRUCT_MAYBE_HTML and base 64 encoding
21990.diff (516 bytes) - added by nacin 2 years ago.

Download all attachments as: .zip

Change History (33)

comment:1 @nacin2 years ago

Thanks dd32 for the report.

comment:3 follow-up: @rmccue2 years ago

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.

comment:4 in reply to: ↑ 3 @nacin2 years ago

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.

comment:5 @cklosows2 years ago

  • Cc cklosowski@… added

comment:7 @rmccue2 years ago

  • Keywords has-patch added

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

comment:8 @rmccue2 years ago

  • Milestone 3.5 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #22321.

comment:9 @ocean902 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone set to 3.5
  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Version set to trunk

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

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

Version 0, edited 2 years ago by ocean90 (next)

comment:10 @iandunn2 years ago

  • Cc ian_dunn@… added

comment:11 @nacin2 years ago

  • Owner set to rmccue
  • Status changed from reopened to assigned

comment:12 @nacin2 years ago

  • Owner changed from rmccue to markjaquith

@markjaquith2 years ago

First attempt at subclassing SimplePie_Sanitize

comment:13 @markjaquith2 years ago

  • Keywords has-patch needs-docs dev-feedback needs-testing needs-unit-tests added; needs-patch removed

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.

comment:14 @markjaquith2 years ago

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

@markjaquith2 years ago

Doesn't touch SP files.

comment:15 @markjaquith2 years ago

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.

@markjaquith2 years ago

with documentation and inline comments

comment:16 @markjaquith2 years ago

  • Keywords needs-docs removed

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.

comment:17 @rmccue2 years ago

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

@rmccue2 years ago

Update patch to also check SIMPLEPIE_CONSTRUCT_MAYBE_HTML and base 64 encoding

comment:18 @nacin2 years ago

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.

comment:19 follow-up: @nacin2 years ago

  • Keywords dev-feedback removed
  • Priority changed from normal to low
  • Severity changed from blocker to normal

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.

comment:20 @iandunn2 years ago

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

comment:21 in reply to: ↑ 19 @rmccue2 years ago

  • Keywords needs-patch added; has-patch needs-testing removed

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.

comment:22 @nacin2 years ago

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.

comment:23 @nacin2 years ago

  • Owner changed from markjaquith to rmccue

@nacin2 years ago

comment:24 @nacin2 years ago

  • Keywords has-patch added; needs-patch removed

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

comment:25 @nacin2 years ago

  • Keywords commit added

comment:26 follow-up: @rmccue2 years ago

  • Keywords needs-patch added; has-patch commit removed

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

comment:27 @nacin2 years ago

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

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.

comment:28 in reply to: ↑ 26 @nacin2 years ago

Replying to rmccue:

with better whitespacing

*spits water everywhere*

Note: See TracTickets for help on using tickets.