WP_REST_Posts_Controller::prepare_items_query() - Warning $prepared_args can be null

[apermo]

In a project I maintain, we are using sentry, and I've received a warning.

Warning: foreach() argument must be of type array|object, null given

For:
/wp/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php in WP_REST_Posts_Controller::prepare_items_query at line 1124

This happens for GET request on /wp-json/wp/vs/posts with the query parameters order, orderby and per_page

I could not yet figure out, why null was given, but I propose to add a is_array() condition around the loop. This will work as intended, without possibly throwing errors as a type hint in the function header woul.d

I will prepare a PR on github with a proposed solution, that will ensure, this warning will not show up.

As discribed in the track ticket, it is possible that prepare_items_query() can receive a null value as $prepared_args which results in a warning. This fixes this issue.

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

[apermo]

• Posts controller (class-wp-rest-posts-controller.php): The vulnerable foreach ($prepared_args as ...) pattern was introduced in commit 4f685410b2 ("REST API: Remove get_allowed_query_vars() now filter is gone.") — first released in WordPress 4.7.0.
• Revisions controller (class-wp-rest-revisions-controller.php): Introduced in commit b513113c49 ("REST API: Support pagination, order, search and other common query parameters for revisions.") — first released in WordPress 5.0.0.

[dmsnell]

thanks for sharing this report @apermo — I can only guess that some custom code implementing the rest_{$this->post_type}_query is returning NULL, which then gets sent into prepare_items_query()

because of that, and because it’s protected/private, I feel like it could be more reasonable to fix the call sites, defaulting NULL values to array() before sending them to the method. why? this is coherent with the existing types, which state that the argument must be an array, and while it’s optional, that does not permit sending a NULL value — only in sending no values.

open to thoughts here. if this were more exposed to plugin and theme authors as part of a public interface I would probably favor adding more guards internally, but in doing so we create a inconsistency in the code between the types and the expectations.

In order to see the tests pass I have merged trunk, which includes a fix for broken end-to-end tests.

@dmsnell I'm glad to see this. I think your addition is the key one (which I iterated on a bit more in e54f258ea1a23c5be7e89220e8c752a0776bb9e6). The return value of apply_filters() cannot be trusted. If that along addresses the issue, it would seem fine to revert the change to the prepare_items_query methods, which I also amended in 35606cfd2db5ca90e3369007e3a7e7c6abf89f3e to account for other values being passed in, for example an empty string.

With that in place, I'd ideally want to add the type hints to the methods and eliminate the default values, since the params are “always” provided:

src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php

@@ -1208 +1208 @@
          *
          * @since 4.7.0
          *
-         * @param array           $prepared_args Optional. Prepared WP_Query arguments. Default empty array.
-         * @param WP_REST_Request $request       Optional. Full details about the request.
+         * @param array           $prepared_args Prepared WP_Query arguments. Default empty array.
+         * @param WP_REST_Request $request       Full details about the request.
          * @return array Items query arguments.
          */
-        protected function prepare_items_query( $prepared_args = array(), $request = null ) {
+        protected function prepare_items_query( array $prepared_args, WP_REST_Request $request ): array {
                 $query_args = array();
-                if ( is_array( ! $prepared_args ) ) {
-                        $prepared_args = array();
-                }
 
                 foreach ( $prepared_args as $key => $value ) {
                         /**

src/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php

@@ -546 +546 @@
          *
          * @since 5.0.0
          *
-         * @param array           $prepared_args Optional. Prepared WP_Query arguments. Default empty array.
-         * @param WP_REST_Request $request       Optional. Full details about the request.
+         * @param array           $prepared_args Prepared WP_Query arguments. Default empty array.
+         * @param WP_REST_Request $request       Full details about the request.
          * @return array Items query arguments.
          */
-        protected function prepare_items_query( $prepared_args = array(), $request = null ) {
+        protected function prepare_items_query( array $prepared_args, WP_REST_Request $request ): array {
                 $query_args = array();
-                if ( is_array( ! $prepared_args ) ) {
-                        $prepared_args = array();
-                }
 
                 foreach ( $prepared_args as $key => $value ) {
                         /** This filter is documented in wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php */

However, this does not account for subclasses which may be calling this protected method. So if we did that, we should do so early in a release cycle to ensure plugins which extend these classes are passing in the proper values as we've done in this PR with type checking the filter return values. But since a subclass may be also using a filter for the $prepared_args param being passed in to prepare_items_query() then it seems safest to add the type checking inside method as well since we didn't add the type hint in the first place back in 5.0.

[westonruter]

In 61773:

REST API: Guard against passing null as $prepared_args to WP_REST_Posts_Controller::prepare_items_query().

The variable may get set to null by a filter or subclass.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/7625

Follow-up to r38832.

Props apermo, dmsnell, westonruter.
Fixes #62287.

Thanks @westonruter — I had a commit prepared but had been stalled by some flakey PHP Unit Tests and was trying to wait until they passed. Appreciate the teamwork.

@dmsnell oh! Sorry if I stepped on your toes. 🤝