WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 5 weeks ago

#51094 accepted defect (bug)

WP_Query.query with invalid post_status will return all

Reported by: carsonreinke Owned by: metalandcoffee
Milestone: 5.7 Priority: normal
Severity: critical Version: 3.9
Component: Query Keywords: dev-feedback has-patch needs-unit-tests
Focuses: Cc:

Description

Hello, I noticed an issue with WooCommerce method wc_get_products and dove into the issue.

The problem was wc_get_products(array('status' => 'published')); would return ALL posts, even trashed ones because of the "published" status is not a valid post status.

I believe that behavior is unexpected that it should return NO posts.

The issue appears to be this portion of WP_Query.get_posts that only known post statuses are added to the query:

<?php
                                foreach ( get_post_stati() as $status ) {
                                        if ( in_array( $status, $q_status, true ) ) {
                                                if ( 'private' === $status ) {
                                                        $p_status[] = "{$wpdb->posts}.post_status = '$status'";
                                                } else {
                                                        $r_status[] = "{$wpdb->posts}.post_status = '$status'";
                                                }
                                        }
                                }

I believe it would best if this method checks for any unrequested post status supplied in the query and either applies them to the query or produces an error. I would imagine this is a pretty major change.

Attachments (1)

51094.diff (801 bytes) - added by metalandcoffee 6 weeks ago.

Download all attachments as: .zip

Change History (8)

#1 @kishanjasani
5 months ago

Hey @carsonreinke, Welcome to WordPress Core Trac! This issue seems like issue of the WooCommerce.
So you have to report it here: https://github.com/woocommerce/woocommerce/issues

#2 @carsonreinke
5 months ago

@kishanjasani Sorry for the confusion, my reference to WooCommerce was simply how I came to this area in WordPress.

The following below seems wrong to me, since instead of returning NOTHING, it will return ALL posts:

<?php
$q = new WP_Query( array( 'post_status' => 'publish' ) );
$q->found_posts; //All published posts

$q = new WP_Query( array( 'post_status' => 'fake status' ) );
$q->found_posts; //Should be zero, instead ALL posts

#3 @xParham
3 months ago

  • Severity changed from minor to critical

I also believe this is a bug in WP_Query and actually one with potentially serious consequences. I came across the bug running a wp post CLI command, in my case passing an unregistered custom post status (trying to clean up posts in a previously registered custom post_status).

But here is another case, if you simply have a typo in your command, passing pendin instead of pending here:
wp post delete $(wp post list --post_type='post' --post_status=pendin --format=ids) --force

You will end up having ALL of your posts deleted.

Last edited 3 months ago by xParham (previous) (diff)

#4 @metalandcoffee
3 months ago

  • Keywords needs-patch added

I've also confirmed this issue. I tested the bug all the way back to WordPress version 3.9 and it was still an issue. Maybe this has always been a thing?

It looks like the logic inside of WP_Query's get_posts method (wp-includes/class-wp-query.php) only accommodates for the following cases:

  1. No post_status argument was provided in the new WP_query object so it generates the SQL statement with the default values ('publish' or 'private'):

Example: new WP_Query( array( 'author' => '1') );

Generated SQL:
SELECT SQL_CALC_FOUND_ROWS wp_posts.ID FROM wp_posts WHERE 1=1 AND wp_posts.post_author IN (1) AND wp_posts.post_type = 'post' AND (wp_posts.post_status = 'publish' OR wp_posts.post_status = 'private') ORDER BY wp_posts.post_date DESC LIMIT 0, 10

  1. A post_status argument is provided in the new WP_query object and matches one of the available post statuses.

Example: new WP_Query( array( 'post_status' => 'publish') );

Generated SQL:
SELECT SQL_CALC_FOUND_ROWS wp_posts.ID FROM wp_posts WHERE 1=1 AND wp_posts.post_type = 'post' AND ((wp_posts.post_status = 'publish')) ORDER BY wp_posts.post_date DESC LIMIT 0, 10

It doesn't accommodate for the case where a post_status argument is provided but it doesn't match any of the available post statuses.

So in that case, because none of the conditionals are met in order to append one of the relevant post_status SQL conditonals, the following SQL is generated:
SELECT SQL_CALC_FOUND_ROWS wp_posts.ID FROM wp_posts WHERE 1=1 AND wp_posts.post_type = 'post' ORDER BY wp_posts.post_date DESC LIMIT 0, 10

Which obviously results in all posts being returned.

#5 @johnbillion
3 months ago

  • Version changed from trunk to 3.9

#6 @metalandcoffee
6 weeks ago

  • Keywords dev-feedback has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 5.7
  • Owner set to metalandcoffee
  • Status changed from new to accepted

Hi again! I'd like to propose a patch although this would require some major dev feedback since I do not want to risk breaking other aspects of the WP_Query class.

Like @carsonreinke pointed out, this bug does result in posts with any post stati being returned (including trashed and auto-draft posts) so I think it's worth fixing.

In my patch, I changed from looping through the available post stati to looping through the post stati provided in the custom query. This results in the following generated SQL statement if a custom query has invalid post stati:

SELECT SQL_CALC_FOUND_ROWS wp_posts.ID FROM wp_posts WHERE 1=1 AND wp_posts.post_type = 'post' AND ((wp_posts.post_status = 'fakestatus' OR wp_posts.post_status = 'publishe')) ORDER BY wp_posts.post_date DESC LIMIT 0, 10

This results in 0 posts being returned which is what we want if we enter something invalid.

I set the milestone to 5.7 because it'd be nice to get this fix in there :)

@metalandcoffee
6 weeks ago

#7 @SergeyBiryukov
5 weeks ago

  • Keywords needs-unit-tests added
Note: See TracTickets for help on using tickets.