WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 17 months ago

Last modified 17 months 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 17 months ago.
First attempt at subclassing SimplePie_Sanitize
21990-subclass.2.diff (1.1 KB) - added by markjaquith 17 months ago.
Doesn't touch SP files.
21990-subclass.3.diff (1.7 KB) - added by markjaquith 17 months ago.
with documentation and inline comments
21990-subclass.4.diff (2.3 KB) - added by rmccue 17 months ago.
Update patch to also check SIMPLEPIE_CONSTRUCT_MAYBE_HTML and base 64 encoding
21990.diff (516 bytes) - added by nacin 17 months ago.

Download all attachments as: .zip

Change History (33)

comment:1 nacin19 months ago

Thanks dd32 for the report.

comment:3 follow-up: rmccue19 months 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 nacin19 months 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 cklosows19 months ago

  • Cc cklosowski@… added

comment:7 rmccue18 months ago

  • Keywords has-patch added

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

comment:8 rmccue18 months ago

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

Duplicate of #22321.

comment:9 ocean9017 months 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 17 months ago by ocean90 (next)

comment:10 iandunn17 months ago

  • Cc ian_dunn@… added

comment:11 nacin17 months ago

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

comment:12 nacin17 months ago

  • Owner changed from rmccue to markjaquith

markjaquith17 months ago

First attempt at subclassing SimplePie_Sanitize

comment:13 markjaquith17 months 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 markjaquith17 months ago

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

markjaquith17 months ago

Doesn't touch SP files.

comment:15 markjaquith17 months 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.

markjaquith17 months ago

with documentation and inline comments

comment:16 markjaquith17 months 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 rmccue17 months ago

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

rmccue17 months ago

Update patch to also check SIMPLEPIE_CONSTRUCT_MAYBE_HTML and base 64 encoding

comment:18 nacin17 months 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: nacin17 months 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 iandunn17 months 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 rmccue17 months 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 nacin17 months 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 nacin17 months ago

  • Owner changed from markjaquith to rmccue

nacin17 months ago

comment:24 nacin17 months ago

  • Keywords has-patch added; needs-patch removed

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

comment:25 nacin17 months ago

  • Keywords commit added

comment:26 follow-up: rmccue17 months 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 nacin17 months 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 nacin17 months ago

Replying to rmccue:

with better whitespacing

*spits water everywhere*

Note: See TracTickets for help on using tickets.