PHP Warning with invalid JSON input

[dd32]

A request such as the following will generate a PHP Warning:

curl https://example.org/wp-json/wp/v2/users/1 --data '"+response.write(document.domain)+"' -H 'Content-Type: application/json'

The warning:

E_WARNING: Invalid argument supplied for foreach() in wp-includes/rest-api/class-wp-rest-request.php:816

The relevant part of the backtrace:

[24-Apr-2024 04:11:35 UTC] PHP  10. WP_REST_Server->serve_request($path = '/wp/v2/users/1') wp-includes/rest-api.php:428
[24-Apr-2024 04:11:35 UTC] PHP  11. WP_REST_Server->dispatch($request = class WP_REST_Request { protected $method = 'POST'; protected $params = ['URL' => ['id' => '1'], 'GET' => [], 'POST' => [], 'FILES' => [], 'JSON' => '+response.write(document.domain)+', 'defaults' => []]; protected $headers = ['content_type' => [0 => 'application/json'] ]; protected $body = '"+response.write(document.domain)+"'; protected $route = '/wp/v2/users/1'; protected $attributes = ['methods' => ['POST' => TRUE, 'PUT' => TRUE, 'PATCH' => TRUE], 'accept_json' => FALSE, 'accept_raw' => FALSE, 'show_in_index' => TRUE,  [.......]; protected $parsed_json = TRUE; protected $parsed_body = FALSE }) wp-includes/rest-api/class-wp-rest-server.php:439
[24-Apr-2024 04:11:35 UTC] PHP  12. WP_REST_Request->sanitize_params() wp-includes/rest-api/class-wp-rest-server.php:1056

[dd32]

Of course, this is actually a valid JSON input.

These are also valid, and produce the same result.

curl https://example.org/wp-json/wp/v2/users/1 --data '"foobar"' -H 'Content-Type: application/json'
curl https://example.org/wp-json/wp/v2/users/1 --data '1' -H 'Content-Type: application/json'

It seems that the expected outcome here is to simply skip sanitisation of the param, ultimately a missing field should be picked up by the schema of the API endpoint, and the endpoint may expect/handle a string/numeric input in the field.

See attached.

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

[antonvlasenko]

Test Report

This report validates that the indicated patch addresses the issue.

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/6491

Environment

• WordPress: 6.6-beta3-58440-src
• PHP: 7.3.33
• Server: Apache/2.4.57 (Unix) PHP/7.3.33
• Database: mysqli (Server: 5.7.43 / Client: mysqlnd 5.0.12-dev)
• Browser: Safari 17.5 (macOS)
• Theme: Twenty Twenty-Four 1.1
• MU-Plugins: None activated
• Plugins:
  • JSON Basic Authentication 0.1

Actual Results

• ✅ No error in the debug.log file after applying the patch.

Additional Notes

The curl request used for testing:

curl --user admin:password http://wordpress.test/wp-json/wp/v2/users/1 --data '1' -H 'Content-Type: application/json'

Supplemental Artifacts

Before applying the patch, the following error was recorded in the debug.log file:

[03-Jul-2024 14:32:18 UTC] PHP Warning:  Invalid argument supplied for foreach() in /src/wp-includes/rest-api/class-wp-rest-request.php on line 816

@dd32 Would you be able to add a test for this?

@peterwilsoncc I don't think I'll have time until next week, but I'll see what I can do! I don't actually fully know how to trigger this from within PHP, I just repro'd it with curl and filed 🤷

Thanks Dion,

I figured out how to test some things but not sure if we'll need more. I added this to tests/phpunit/tests/rest-api/rest-request.php

The one thing I am unsure of is whether strings are intended to be valid. @TimothyBJacobs, @kadamwhite Do either of you know?

/**
 * @ticket 61061
 */
public function test_sanitize_params_with_string_parameters() {
        $this->request->set_url_params( 'foobar' );

        $this->request->set_attributes(
                array(
                        'args' => array(
                                'someinteger' => array(
                                        'sanitize_callback' => 'absint',
                                ),
                                'somestring'  => array(
                                        'sanitize_callback' => 'absint',
                                ),
                        ),
                )
        );

        $this->request->sanitize_params();

        $this->assertSame( 'foobar', $this->request->get_url_params() );
}

/**
 * @ticket 61061
 */
public function test_has_valid_params_with_string_parameter_and_required_attribute() {
        $this->request->set_url_params( 'foobar' );

        $this->request->set_attributes(
                array(
                        'args' => array(
                                'someinteger' => array(
                                        'sanitize_callback' => 'absint',
                                        'required'          => true,
                                ),
                        ),
                )
        );

        $this->assertWPError( $this->request->has_valid_params() );
}

/**
 * @ticket 61061
 */
public function test_has_valid_params_with_string_parameter_without_required_attribute() {
        $this->request->set_url_params( 'foobar' );

        $this->request->set_attributes(
                array(
                        'args' => array(
                                'someinteger' => array(
                                        'sanitize_callback' => 'absint',
                                ),
                        ),
                )
        );

        $this->assertTrue( $this->request->has_valid_params() );
}

I'm currently working on adding more tests to this PR (unless @dd32 has any objections).

[peterwilsoncc]

I've removed the commit keyword and moved this off the release as @antonvlasenko picked up some potentially related issues while working on some tests.

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