Query for multiple post types not considering user permission to retrieve private posts

[leogermani]

When you query for posts not informing a specific post_status, WordPress will return posts that the current user can read (if there is a user logged in).

However, if you query for multiple post types, passing an array, or if you query for any post type, WordPress will ignore this behavior and won't return any private posts at all.

Expected behavior is that it would return posts with private status if they belong to a post type for which the user has the read_private_posts capability.

An existing, and rather undocumented, workaround is to grant the user the read_multiple_post_types capability. But this, again, will not check the permission current user have in each queried post type and will simply return all private posts for all queried post types.

Proposal

The proposed solution for this is to change the SQL query when querying for multiple post types without informing a post status, and combining the post_status and post_type WHERE clauses, checking user capability for each post type and returning the appropriate query in the very same way WordPress already does when you query for only one post type.

Sample Query when querying for posts and pages, for a user that HAS read_private_posts cap but DOES NOT HAVE read_private_pages:

SELECT SQL_CALC_FOUND_ROWS  wptests_posts.ID FROM wptests_posts  WHERE 1=1  AND 
(
  (wptests_posts.post_type = 'post' AND 
    (wptests_posts.post_status = 'publish' OR wptests_posts.post_status = 'private')
  ) 
  OR 
  (wptests_posts.post_type = 'page' AND 
    (wptests_posts.post_status = 'publish' 
     OR wptests_posts.post_author = 4 
     AND wptests_posts.post_status = 'private'
    )
  )
)  ORDER BY wptests_posts.post_date DESC LIMIT 0, 10 }}}

[leogermani]

Attached a patch with the change and some tests.

I made an additional elseif statement so it do not touch all other cases and acts only in this very specific case.

Would love to have some feedback.

git branch: ​https://github.com/leogermani/wordpress-develop/tree/48556

[leogermani]

This is a related ticket: #44737

If either of these tickets are merged, the other have to be updated

[leogermani]

Few observations on this ticket:

1) It changes the query results, so it needs Dev Notes

2) It also changes the SQL query, so it might break things for people filtering the query with "posts_where" filter, for example. Also needs to be in Dev Notes

3) I made it in a way it only changes the query in the very specific case of this ticket. The code could be cleaner if we changed the way the query is built in every case, but I thought this would bring less problems for developers

[leogermani]

There is another case where the same situation described in this ticket happens.

If you query for multiple post_types, informing a private post_status, WP_Query will not check permissions for each post type and again rely on the read_multiple_post_types capability.

All of this only if perm=readable.

The expected behavior is for the query to check read_private_posts permission for each post type consider the informed post_status only when current user has permission.

I don't think it is the case of opening another ticket because both things should be done together and the ticket title still applies. I'll edit the description.

[leogermani]

As a last note for today, I will have a look at all the tests we have for WP_Query. I think we should write more tests to cover as many situations as possible before doing such changes.

[audrasjb]

Hi,

Looks like this ticket is still awaiting review.

With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[boonebgorges]

Hi @leogermani - Thanks for working on this ticket. Your description of the problem, and your general approach to fixing it, seem correct to me.

3) I made it in a way it only changes the query in the very specific case of this ticket. The code could be cleaner if we changed the way the query is built in every case, but I thought this would bring less problems for developers

This decision means that there are two places where the post_status logic is built: one for single post types, and one for multiple post types and/or 'any'. This will create a maintenance burden in the future. Could you please rework the patch so that the multiple-post-type logic is part of this block https://core.trac.wordpress.org/browser/tags/5.5/src/wp-includes/class-wp-query.php#L2514? This also means that we should have tests covering the existing behavior - there are some in tests/phpunit/tests/query/postStatus.php, but this should be reviewed for completeness.

[leogermani]

Thanks for the feedback @boonebgorges

This decision means that there are two places where the post_status logic is built: one for single post types, and one for multiple post types and/or 'any'.

Yes, just clarify that with "less problems for developers" I meant backward compatibility for anyone doing fancy transformations in the query using the posts_where filter.

If we change the query in all cases we will basically break things for anyone using this filter, since they usually rely on the query structure they expect from core using a regex or similar approach.

Is that ok? Something we let everyone know with a Dev note?

[boonebgorges]

If we change the query in all cases we will basically break things for anyone using this filter, since they usually rely on the query structure they expect from core using a regex or similar approach.

I'm not sure this has to happen. Your logic could be adapted in such a way that the "default" case generates the very same SQL that we currently generate. I guess there could be differences in whitespace or in parentheses, due to the way you're assembling $typewheres? Is that what you have in mind?

In the past, we've found it acceptable to make these kinds of changes to underlying SQL. (a) We can write a dev note that explains the change, as you note. And (b) it may be that developers have written their filters in such a way that these changes will break their customizations. On the latter point, we might be able to run some sort of search on the public wordpress.org plugin repository - perhaps, first matching plugins that filter 'posts_where' and then narrowing it down to those that do something with post_status? - that can give us some concrete examples of what may or may not break. If I had to bet, I'd say that most plugins are not, in fact, doing regex/string manipulation on the query, but instead are rebuilding the query based on the parameters passed to WP_Query. But we can only know if we check :-D

[leogermani]

I guess there could be differences in whitespace or in parentheses, due to the way you're assembling $typewheres? Is that what you have in mind?

Exactly!

In the past, we've found it acceptable to make these kinds of changes to underlying SQL. (a) We can write a dev note that explains the change, as you note. And (b) it may be that developers have written their filters in such a way that these changes will break their customizations. On the latter point, we might be able to run some sort of search on the public wordpress.org plugin repository - perhaps, first matching plugins that filter 'posts_where' and then narrowing it down to those that do something with post_status? - that can give us some concrete examples of what may or may not break.

Cool! Let's do it. I can work in the patch and do a search in the plugin repo. Let's see what we find.

Query for multiple post types not considering user permission to retrieve private posts

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

[leogermani]

hi @boonebgorges ,

I've linked a new PR to this ticket with the changes we spoke about. Please check it out whenever you can.

There's still another case to take care, which is when we also query for multiple post statuses. I'm wondering if that will be too much for a single diff and maybe it's better to wait for this to move forward and do this in a following one. Anyway, I already started looking into it and the solution will be similar.

[hellofromTonya]

Per scrub today, too deep into 5.6 beta for this ticket with RC1 next week. Moving it to 5.7.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[boonebgorges]

@leogermani Thanks for your patience as I reviewed your PR.

I stumbled upon this problem separately, when a client reported a problem with private posts in certain directories. I tracked it down to a check for the (non-existent) 'read_private_anys' capability, which led me to #13509 and #46968. I believe that these bugs have the same root cause as the current one, and the proposed approach will fix them all.

I've attached a new patch 48556.2.diff​ which makes a few changes:

• Fixes a bug where certain combinations of query parameters could cause the list of 'where' clauses to be empty, resulting in invalid SQL syntax of the form AND ().
• Updates code formatting and variable names. I know you were matching existing variable names in other clauses, but they're really difficult to understand, so I've tried to make them clearer.
• Rewritten and relocated the tests, to make them a bit more independent of built-in post types and to isolate the capability issue more clearly.

Have you had a chance to think about backward compatibility concerns? How can we summarize the changes to SQL syntax?

There's still another case to take care, which is when we also query for multiple post statuses. I'm wondering if that will be too much for a single diff and maybe it's better to wait for this to move forward and do this in a following one. Anyway, I already started looking into it and the solution will be similar.

Could you start by writing tests that describe how this is currently broken?

[hellofromTonya]

In talking with @boonebgorges, moving this ticket as a 5.7 early candidate. Why?

The change has some potential for breakage of various sorts.

Early gets it early attention, plenty of eyes on it, early triage, and hopefully a commit early enough in the 5.7 cycle for plenty of testing.

[leogermani]

Replying to boonebgorges:

@leogermani Thanks for your patience as I reviewed your PR.

No problem!

I've attached a new patch 48556.2.diff​ which makes a few changes:

• Fixes a bug where certain combinations of query parameters could cause the list of 'where' clauses to be empty, resulting in invalid SQL syntax of the form AND ().
• Updates code formatting and variable names. I know you were matching existing variable names in other clauses, but they're really difficult to understand, so I've tried to make them clearer.
• Rewritten and relocated the tests, to make them a bit more independent of built-in post types and to isolate the capability issue more clearly.

The patch looks good, thanks for it! Maybe it's a good idea to have a test that reproduces this scenario you fixed.

Have you had a chance to think about backward compatibility concerns? How can we summarize the changes to SQL syntax?

I was thinking of something like:

(We can either edit it later and make it smaller for a dev note, or expand it with more examples for a full blog post)

This fix changes the structure of the WHERE clause when querying for posts filtering by multiple post types. Any code that relies on the posts_where filter and expecting a specific pattern should check if it will be affected by this change.

The changes will be noticed whenever the post_type argument of the query is set.

Before this change, the clauses for post types and post statuses were separated:

... wp_posts.post_type IN ('post', 'page') AND (wp_posts.post_status = 'publish' ...

Now, each clause for a post type will hold a clause for the post status as well.

... ((wp_posts.post_type = 'post' AND (wp_posts.post_status = 'publish')) OR (wp_posts.post_type = 'page' AND (wp_posts.post_status = 'publish'))) ...

Example:

Sample query:

$query = new WP_Query( 
        array( 
                'post_type' => array( 
                        'post', 
                        'page',
                ) 
        )
);

SQL Query before this change:

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

SQL Query after this change:

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' wp_posts.post_author = 1 AND wp_posts.post_status = 'private')) OR (wp_posts.post_type = 'page' AND (wp_posts.post_status = 'publish' OR wp_posts.post_author = 1 AND wp_posts.post_status = 'private'))) ORDER BY wp_posts.post_date DESC LIMIT 0, 10

There's still another case to take care, which is when we also query for multiple post statuses. I'm wondering if that will be too much for a single diff and maybe it's better to wait for this to move forward and do this in a following one. Anyway, I already started looking into it and the solution will be similar.

Could you start by writing tests that describe how this is currently broken?

Sure, I'll open a new ticket and start by describing the issue and writing tests that reproduce it... it would be easier to write a patch only after this one is merged though, to avoid conflicts and merge hell. (unless you think it's fine to work on this same ticket)

[leogermani]

@boonebgorges I've just created a new ticket with the bugs in the status queries and attached a file with tests that reproduce it: #52094

[boonebgorges]

In 49830:

Query: Respect post-type specific capabilities when querying for multiple post types.

After this change, the relevant read_private_posts capability is checked for
each queried post type. This ensures that private posts appear in search and
archive queries for users who have the ability to view those posts.

Props leogermani.

Fixes #13509, #48968, #48556.

[boonebgorges]

#46968 was marked as a duplicate.

[boonebgorges]

Thanks for this, @leogermani! I've committed the change in [49830].

I'll mark this ticket as needing a dev note, and we'll use your text as a starting place. If we manage #52094 in 5.7 as well, we'll probably want to rewrite to cover both cases.

[peterwilsoncc]

In 49832:

Coding Standards: Minor fixes following [49830].

See #13509, #48968, #48556.

[peterwilsoncc]

In 49833:

Coding Standards: Minor fixes following [49830].

Fixes the fixes missed in [49832]. They are fixed now.

See #13509, #48968, #48556.

[peterwilsoncc]

Reopening this with the intent to revert [49830]

Passing an invalid post type to WP_Query is causing the post type and post status parameters from being dropped from the resulting SQL query for all users.

Prior to the above commit both of the following tests would pass, generating the SQL query:

SELECT SQL_CALC_FOUND_ROWS  wptests_posts.*
FROM wptests_posts
WHERE 1=1
  AND wptests_posts.post_type = 'unregistered_cpt'
  AND (wptests_posts.post_status = 'publish')
ORDER BY wptests_posts.post_date DESC LIMIT 0, 10

Following the change they generate the query:

SELECT SQL_CALC_FOUND_ROWS  wptests_posts.*
FROM wptests_posts
WHERE 1=1
ORDER BY wptests_posts.post_date DESC LIMIT 0, 10

The result is that all post types and statuses are returned.

I'll revert the commit today and subsequently add theses and any other relevant tests I can think of. Once this is looked at again, the tests will need to be updated.

<?php
class Tests_Query_InvalidPostTypes extends WP_UnitTestCase {
        public $last_posts_request;

        public function setUp() {
                parent::setUp();

                // Clean up variable before each test.
                $this->last_posts_request = '';
                // Store last query for tests.
                add_filter( 'posts_request', array( $this, '_set_last_posts_request' ) );
        }

        public function _set_last_posts_request( $request ) {
                $this->last_posts_request = $request;
                return $request;
        }

        function test_unregistered_post_type_wp_query() {
                global $wpdb;

                new WP_Query( array( 'post_type' => 'unregistered_cpt' ) );

                $this->assertContains( "{$wpdb->posts}.post_type = 'unregistered_cpt'", $this->last_posts_request );
                $this->assertContains( "{$wpdb->posts}.post_status = 'publish'", $this->last_posts_request );
        }

        function test_unregistered_post_type_goto() {
                global $wpdb;

                $this->go_to( home_url( '?post_type=unregistered_cpt' ) );

                $this->assertContains( "{$wpdb->posts}.post_type = 'unregistered_cpt'", $this->last_posts_request );
                $this->assertContains( "{$wpdb->posts}.post_status = 'publish'", $this->last_posts_request );
        }
}

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

[peterwilsoncc]

In 49899:

Query: Revert post-type specific capability changes.

The modified checks of the read_private_posts capability could result in unexpected SQL queries when calling WP_Query with invalid parameters.

Reverts [49830], [49832] and [49833].
See #48556.

[peterwilsoncc]

In 49900:

Query: Add bad path tests with invalid WP_Query parameters.

See #48556.

[leogermani]

@peterwilsoncc thanks for finding this and for writing the new tests. I'll try and update the patch as soon as I can.

I'll be AFK for most of January so I might not be able to do it very soon though.

[SergeyBiryukov]

In 49902:

Tests: Correct @ticket references in tests/query/invalidQueries.php.

This ensures that running phpunit --group 48556 works as expected.

Follow-up to [49900].

See #48556.

Query for multiple post types not considering user permission to retrieve private posts

Iteration 3, after a bug was found when querying for invalid post types.

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

[leogermani]

Hi @peterwilsoncc , @boonebgorges

I managed to put a fix together and created a new ​PR (already linked to this ticket).

I created a new test, making a request for a valid and an invalid post type at the same time.

I believe this ​commit will fix the issue while keeping good backward compatibility.

Querying for an invalid post type won't invalidate the whole query. If other valid post types are queried, they will still work. Also, if there are posts saved with an unregistered post type, they will be returned.

[leogermani]

Hi @peterwilsoncc , @boonebgorges

Friendly new year's ping to see if you can have a look at this and we don't lose the momentum. It would be nice to look at it while we still have the issue fresh in our heads.

Thanks!

[peterwilsoncc]

Per discussion in #51094, working on both tickets in a single patch makes sense due to the similarity of errors.

[peterwilsoncc]

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

[leogermani]

Hi @peterwilsoncc ,

Last time we merged this patch we thought it was a good idea to merge it early in the cycle -> see https://core.trac.wordpress.org/ticket/48556#comment:22

So maybe it's a good time to do it again?

Also, please have a look at #52094. There I fixed the same situation with post statues. And there's a proposed unified fix for both tickets - or a separate if we want.

cc @boonebgorges

[peterwilsoncc]

@leogermani You're right, it's probably a good time to try to get this in.

I wanted to acknowledge this but let you know I probably won't have a chance to review this until after 5.7.1 is released as that's my current focus.

[leogermani]

No worries @peterwilsoncc,

When you have a chance, please also look at #52094 where there's a proposal to fix both issues with one unified patch.

[prbot]

​leogermani commented on ​PR #843:

Added a couple more comments.

Do you mind if I push to your branch (you'll need to select the "Allow edits by maintainers " checkbox under the subscribe button)? I think better in unit tests so I may as well push any additional ones that I write.

Ta

Checkbox is already enabled. Feel free to commit changes