Opened 16 years ago
Closed 15 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)
Change History (24)
#3
@
16 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() ) {
#4
@
16 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)?
#5
@
16 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.
#6
@
16 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).
#7
@
16 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.
#9
@
16 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.)
#10
@
16 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?
#11
@
16 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.
#14
@
16 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.
#15
in reply to:
↑ description
@
16 years ago
- Severity changed from major to normal
#16
@
16 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]
).
#17
@
16 years ago
I also think the feed should return a 404, just like the post. Will look at that later.
#18
@
16 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
On a sidenote, this seems to show up even if display_errors = off.