Fatal error in rest api with invalid inputs

[dd32]

A request similar to the following causes a PHP fatal error under PHP8:
https://example.org/?rest_route[pen]=tester

This causes irrelevant error log noise for sites which attempt to process the request as a rest-api request.

This can be duplicated via playground:
​https://playground.wordpress.net/?php=8.1&url=%3Frest_route%5Bpen%5D%3Dtester

PHP Fatal error:  Uncaught TypeError: rtrim(): Argument #1 ($string) must be of type string, array given in /wordpress/wp-includes/formatting.php:2
Stack trace:
#0 /wordpress/wp-includes/formatting.php(2): rtrim(Array, '/\\')
#1 /wordpress/wp-includes/rest-api.php(2): untrailingslashit(Array)
#2 /wordpress/wp-includes/class-wp-hook.php(3): rest_api_loaded(Object(WP))
#3 /wordpress/wp-includes/class-wp-hook.php(3): WP_Hook->apply_filters('', Array)
#4 /wordpress/wp-includes/plugin.php(2): WP_Hook->do_action(Array)
#5 /wordpress/wp-includes/class-wp.php(3): do_action_ref_array('parse_request', Array)
#6 /wordpress/wp-includes/class-wp.php(3): WP->parse_request('')
#7 /wordpress/wp-includes/functions.php(2): WP->main('')
#8 /wordpress/wp-blog-header.php(2): wp()
#9 /wordpress/index.php(2): require('/wordpress/wp-b...')
#10 {main}
  thrown in /wordpress/wp-includes/formatting.php on line 2

A minimal patch is simply to check for stringyness:

src/wp-includes/rest-api.php

@@ -426 +426 @@
  * @global WP $wp Current WordPress environment instance.
  */
 function rest_api_loaded() {
-        if ( empty( $GLOBALS['wp']->query_vars['rest_route'] ) ) {
+        if ( empty( $GLOBALS['wp']->query_vars['rest_route'] ) || ! is_string( $GLOBALS['wp']->query_vars['rest_route'] ) ) {
                 return;
         }

[peterwilsoncc]

Moving this on to 6.8 for consideration as it's a pretty easy fix.

An alternative to the approach above would be to catch the error and die

src/wp-includes/rest-api.php

@@ -441 +441 @@
         // Initialize the server.
         $server = rest_get_server();
 
+        if ( ! is_string( $GLOBALS['wp']->query_vars['rest_route'] ) ) {
+                $rest_type_error = new WP_Error(
+                        'rest_path_invalid_type',
+                        __( 'The rest route parameter must be a string.' ),
+                        array( 'status' => 400 )
+                );
+                wp_die( $rest_type_error );
+        }
+
         // Fire off the request.
         $route = untrailingslashit( $GLOBALS['wp']->query_vars['rest_route'] );
         if ( empty( $route ) ) {

This PR introduces an early return validation in rest_api_loaded( to ensure that the rest_route query variable is always a string. If the rest_route is not a string, the function now returns a WP_Error, allowing for better error handling and maintaining proper REST API response behaviour.

Changes:

• Added an early return if rest_route is not a string.
• Replaced wp_die() with a WP_Error response to improve flexibility.

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

[TimothyBlynJacobs]

Yeah, I agree with @peterwilsoncc. Erroring to the user makes the most sense to me. Helpful to know what you're doing wrong, as opposed to just seeing a random 404 or something.

@peterwilsoncc I have added the unit-test in ​https://github.com/WordPress/wordpress-develop/pull/8287/commits/f14f6581d6f45fca5f27fe4e4dcaaa0b61094e65. Let me know if that looks good or needs some change

[dd32]

I'm totally fine with an error here, I was just following the same behaviour for other invalid rest-api requests such as http://example.org/?rest_route=.

This is such an edge-case that we might as well be helpful to a confused developer.

@geekofshire I think I've figured it out and it's on me, sorry 🫢

Calling rest_api_loaded() in the test suite is hitting the line define( 'REST_REQUEST', true ); which will affect the rest of the test suite. I suspect the failed tests are interacting with the constant in some way (or the functions they call).

Unfortunately that makes rest_api_loaded() largely untestable (@TimothyBJacobs can you confirm) so we might need to rely on manual testing of the code.

@peterwilsoncc Do we have any update on this on testing this change and since we need to manually test this out should I remove the unit test?

@geekofshire I think there are two options:

• remove the test
• move the logic check triggering wp_die() above define( 'REST_REQUEST', true );

I'm happy with either option, which do you prefer?

@peterwilsoncc I think we should move the logic above define( 'REST_REQUEST', true ) because if the rest_route query var isn't a string, there's no need to define REST_REQUEST and initialize the server. The process should stop immediately.

Let me know if this sounds good so I can update the PR.

@geekofshire That sounds good, thanks.

@peterwilsoncc Updated the approach please check once, the 2 tests are failing due to some connection issues/server config issue.

[peterwilsoncc]

The linked PR is good for commit.

[peterwilsoncc]

In 59886:

REST API: Exit gracefully for malformed URLs.

Exit gracefully for requests with a malformed rest_route query string parameter, ie anything that is not a string.

This prevents fatal errors from occurring with URLs such as example.com/?rest_route[]=array as the URL is user input so logging the data provides no benefit to developers as they are unable to resolve the issue.

Props geekofshire, dd32, timothyblynjacobs.
Fixes #62932.