SQL Error on feeds for invalid posts

[robertaccettura]

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.

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

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() ) {

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

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.

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

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.

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

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?

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

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

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.

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

Protect against using unset $this->posts

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

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

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

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

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.

this has been fixed since r6683.