WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 6 years ago

#5185 closed defect (bug) (fixed)

SQL Error on feeds for invalid posts

Reported by: robertaccettura Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.3
Component: General Keywords: sql error feed query.php needs-patch
Focuses: Cc:

Description

If you append /feed to an invalid post url (the post itself returns a 404), you get a SQL error on top:

WordPress database error: [You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'AND comment_approved = '1' ORDER BY comment_date_gmt DESC LIMIT 10' at line 1]
SELECT wp_comments.* FROM wp_comments WHERE comment_post_ID = AND comment_approved = '1' ORDER BY comment_date_gmt DESC LIMIT 10

Looks like it's coming from get_posts() in wp-includes/query.php

Calling this major since it breaks the feed.

Google webmaster's console is even weirder. When a redirect points to a feed broken like this, it shows up as a 404.

Attachments (1)

query.diff (793 bytes) - added by ruckus 7 years ago.
Protect against using unset $this->posts

Download all attachments as: .zip

Change History (24)

comment:1 @robertaccettura8 years ago

On a sidenote, this seems to show up even if display_errors = off.

comment:2 @Nazgul8 years ago

  • Milestone set to 2.5

comment:3 @markjaquith8 years ago

  • Milestone changed from 2.5 to 2.4

Reproduced.

Can prevent it with a bandaid. It serves a 404, so that's good. What output do we want to send? Is there any standard for serving 404 content to a feed reader? Or is the 404 sufficient? i.e. Can we just die with a human-readable error?

Bandaid:

wp-includes/query.php:1258

		if ( $this->is_comment_feed && $this->is_singular && $this->have_posts() ) {

comment:4 @robertaccettura8 years ago

I think that's better than a SQL error.

But doesn't that still give away that there's a private post (even if one is not privileged to know that)?

comment:5 @markjaquith7 years ago

Are you still able to tell the difference between a private post and a 404 post with this patch? Feed output looks the same, to me.

comment:6 @robertaccettura7 years ago

I presume that's for the trunk. I've only got 2.3 running atm...

-              if ( $this->is_comment_feed && $this->is_singular ) {
+                if ( $this->is_comment_feed && $this->is_singular && $this->have_posts() ) {
 

Not really fixing it.

For example:
/private-post/feed/

Title on feed reads: Comments on: ...

For example
/invalid-post/feed/
Title on feed reads: Blog Name » 2007 » October » 14

I think throwing a 404 is 100% acceptable to be perfectly honest. AFAIK most feed readers do know/understand the header. Most are very http complaint (thanks to 301's and If-Modified-Since). Those that don't will either:

a) do nothing (not so bad considering the situation).
b) find the feed invalid (not so bad considering there is no feed).

comment:7 @robertaccettura7 years ago

Silly me... when in doubt clear your cache. Title's are the same, that fixes the security aspect.

As to best behavior, both I think are Ok, though I personally prefer the 404. Otherwise there are a billion blank feeds available on the site. 404's don't need feeds.

Pardon rushed appearance here... hand injury.

comment:8 @pishmishy7 years ago

  • Keywords invalid removed
  • Owner changed from anonymous to pishmishy

comment:9 @pishmishy7 years ago

  • Status changed from new to assigned

I can't replicate this problem in exactly the same way but the problem does appear to still exist. Requests such as http://www.mywpblog.com/?feed=rss2&p=666 causes an SQL error.

WordPress database error: [You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'AND comment_approved = '1' ORDER BY comment_date_gmt DESC LIMIT 10' at line 1]
SELECT wp_comments.* FROM wp_comments WHERE comment_post_ID = AND comment_approved = '1' ORDER BY comment_date_gmt DESC LIMIT 10

(See http://www.securityfocus.com/archive/1/484608/30/0/threaded, although it's not believed to be the security problem claimed, and the new password hashing makes acquiring the hash less useful.)

comment:10 @pishmishy7 years ago

I think this needs to be done slightly differently, although it's possible I've gone in the wrong direction as I'm not too familiar with query.php. I've added

               if ($this->is_comment_feed && !$this->have_posts) {
                       //Escape if the post doesn't exist
                       $this->set_404();
                       return array();
               }
               if ( $this->is_comment_feed && ( $this->is_archive || ...

just before the invalid SQL is formed (line 1200, query.php). This causes a 404 error to be raised and an empty result returned. The feed templating doesn't appear to allow for them to raise a 404, so something along the lines of

if(is_404()){
        status_header( 404 );
        include(get_404_template());
        return;
}

needs to be added to the start of each template. Perhaps there's a tidier way to handle 404 errors for every feed template together?

comment:11 @lloydbudd7 years ago

Ticket #5471 likely has the same cause, but b/c it uses extrema ID, I would like to leave it open until resolution.

comment:12 @lloydbudd7 years ago

  • Component changed from General to Security

comment:13 @pishmishy7 years ago

Apart from the information disclosure, how is this a security bug? If it is security related, I'd say that classification should be for #5473, leaving this as a bug exposed by #5473. I confuse myself. =)

comment:14 @lloydbudd7 years ago

  • Component changed from Security to General

General is the biggest bucket, so any subclassification is a good thing, but in the context of the work in #5473 this is no longer a security issue.

comment:15 in reply to: ↑ description @lloydbudd7 years ago

  • Severity changed from major to normal

comment:16 @ruckus7 years ago

  • Cc ruckus added
  • Keywords has-patch added

Here is a patch to protect the SQL query for comments from being executed when there are no posts found. I'm using !empty($this->posts) to match the code block just below that fetches the post status (and also refers to $this->posts[0]).

@ruckus7 years ago

Protect against using unset $this->posts

comment:17 @ruckus7 years ago

I also think the feed should return a 404, just like the post. Will look at that later.

comment:18 @Otto427 years ago

Apparently there's other ways to trigger this problem:

http://wordpress.org/support/topic/153126 does it by hitting http://example.com/blog/page/2/feed

comment:19 @ryan7 years ago

(In [6683]) Make sure we have a post when doing post comment feed query. Props ruckus. see #5185

comment:20 @ryan7 years ago

Put the empty check in for now. Leaving open for 404 resolution.

comment:21 @pishmishy7 years ago

  • Owner changed from pishmishy to anonymous
  • Status changed from assigned to new

comment:22 @mrmist6 years ago

  • Keywords needs-patch added; has-patch removed

Feed requests for non-existant post IDs return 404s for me, can this be closed off now? If not, it needs a patch for whatever is outstanding.

comment:23 @Denis-de-Bernardy6 years ago

  • Milestone 2.9 deleted
  • Resolution set to fixed
  • Status changed from new to closed

this has been fixed since r6683.

Note: See TracTickets for help on using tickets.