WordPress.org

Make WordPress Core

Opened 13 months ago

Last modified 6 months ago

#51094 accepted defect (bug)

WP_Query.query with invalid post_status will return all

Reported by: carsonreinke Owned by: metalandcoffee
Milestone: Future Release 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 10 months ago.

Download all attachments as: .zip

Change History (14)

#1 @kishanjasani
13 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
13 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
11 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 11 months ago by xParham (previous) (diff)

#4 @metalandcoffee
11 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
11 months ago

  • Version changed from trunk to 3.9

#6 @metalandcoffee
10 months 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 :)

#7 @SergeyBiryukov
10 months ago

  • Keywords needs-unit-tests added

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


8 months ago

#9 @leogermani
8 months ago

Hi, I just want to bring this other ticket to attention here:

https://core.trac.wordpress.org/ticket/48556

It's changing the behavior in the same place. It was merged into trunk but reverted due to a regression already addressed.

If you agree, I could add some unit tests there and address this issue in the same patch so we don't conflict.

Otherwise, it's better if we decide which patch goes first so the other can adapt.

#10 @peterwilsoncc
7 months ago

@leogermani I think it's a good idea to work on this and #48556 in the same patch. The code is intertwined and the bugs are similar enough.

cc @boonebgorges as you've been involved in the other ticket.

#11 @leogermani
7 months ago

Actually, I made a small confusion when I first looked at this.

This is not directly related to #48556, but rather with its counterpart #52094.

In #52094 I surface a series of inconsistencies in the status query, and I think I include this bug here as well. I will try to work on that this week.

Since it's not a small change, I'm not sure we will have it ready and tested by the next release, so I'm not sure if you guys want to go with the simple fix proposed here that addresses at least part of the problem. But as I show in the other ticket, this is just one of the several issues.

#12 @peterwilsoncc
7 months ago

  • Milestone changed from 5.7 to Future Release

With 5.7's RC1 due next week, I'm going to move this off the milestone for a later release.

#13 @leogermani
6 months ago

I've worked on a fix for this in #52094

Note: See TracTickets for help on using tickets.