Make WordPress Core

Opened 8 weeks ago

Closed 6 weeks ago

#60585 closed defect (bug) (invalid)

Recursion in WP_REST_Posts_Controller schema

Reported by: derrickkoo's profile derrickkoo Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: REST API Keywords: needs-testing
Focuses: performance Cc:

Description

In the schema definition for the WP_REST_Posts_Controller class, any attribute which invokes a array-like callable with $this for its validate_callback or sanitize_callback option will result in a recursive instance of the WP_REST_Posts_Controller class to be attached to that schema definition. Here's an example of such a callable that causes this issue (code pasted below; see the value of sanitize_callback): https://github.com/WordPress/WordPress/blame/master/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L2288

'slug'         => array(
        'description' => __( 'An alphanumeric identifier for the post unique to its type.' ),
        'type'        => 'string',
        'context'     => array( 'view', 'edit', 'embed' ),
        'arg_options' => array(
                'sanitize_callback' => array( $this, 'sanitize_slug' ),
        ),
),

For most sites this doesn't seem to cause any issues, but on sites that register a lot of custom meta on the post CPT, because each registered meta key adds its own schema attribute, this can result in a huge WP_REST_Posts_Controller object which can potentially cause an unexpected response to POST requests to the posts endpoint. Curiously, this doesn't seem to cause an OOM error in my testing, but instead returns a 403 response with a generic The link you followed has expired error, which sounds more like an expired nonce error than a memory error.

This can be replicated and seen in a couple of ways:

  1. Using wp shell to instantiate the WP_REST_Posts_Controller class for the posts endpoint and fetching its schema:
wp> $controller = new WP_REST_Posts_Controller( 'post' );
wp> $controller->get_item_schema();

The output of get_item_schema() will return a schema object showing the recursive instances of WP_REST_Posts_Controller where those callables are defined.

  1. By making a POST request to the posts endpoint, hooking into the rest_insert_post action hook (see: https://developer.wordpress.org/reference/hooks/rest_insert_this-post_type/) and logging the $request argument. The ['attributes']['args'] property will contain the full schema and show the recursion for any schema attribute invoking a callable using $this.

Potential (untested) solution: instead of invoking callables using $this, convert the callable methods to static methods on the WP_REST_Posts_Controller class or its parent, and invoke using [ __CLASS__, 'method_name' ] to avoid recursion.

Tested with v6.4.3 and v5.9.9 so far.

Change History (4)

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


6 weeks ago

#2 @johnbillion
6 weeks ago

  • Keywords reporter-feedback added
  • Version 6.4.3 deleted

Thanks for the ticket @derrickkoo and welcome to WordPress Trac.

An object that contains deeply nested or fully recursive references doesn't necessarily consume a lot of memory because each of those objecss will be a reference to an instance rather than a copy (this is what allows a fully recursive property to exist on an object without exhausting all the available memory).

I think we'll need some more info from you in order to investigate if there's an underlying problem and what its impact is.

  • Beyond identifying that recursive properties exist on the request object to a REST API endpoint, is there a concrete issue that can be seen? For example a memory leak.
  • You mentioned "this doesn't seem to cause an OOM error in my testing, but instead returns a 403 response". Are you able and willing to dig into this further to see if there is a cause for concern with regards to memory usage? Were you able to identify whether the 403 was related or caused by something else?

#3 @derrickkoo
6 weeks ago

@johnbillion thanks for the response! On our end we haven't been able to confirm whether the 403 was truly a result of this recursion, and after some more testing we believe the recursion isn't actually consuming memory, as you state. Let's park this for now until I can confirm whether there's a true connection between this issue and the 403 response.

#4 @johnbillion
6 weeks ago

  • Keywords reporter-feedback removed
  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Thanks for the update. I'll close this off. Feel free to continue to comment or to reopen it if necessary.

Note: See TracTickets for help on using tickets.