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

Hello @Sukhendu2002 thanks for the PR. It has some conflicts, would you mind rebasing it so it can be tested? Thanks!

Hi @audrasjb, I've rebased the PR and resolved the conflicts. It should be ready for testing now. Let me know if you need any further changes. Thanks!

[tusharaddweb]

Test Environment:

WordPress Version: 6.8
PHP Version: 8.1
Debug Mode: Enabled (WP_DEBUG_LOG set to true)

Test Scenario

Execute the following cURL request:

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

Monitor the debug.log file (wp-content/debug.log) for any warnings or errors.
Apply the patch: GitHub Pull Request #6491.
Re-run the same cURL request.
Verify that no warnings or errors are logged.

Expected Result

No PHP warnings or errors should be logged in debug.log when processing the request.

Actual Result (Before Patch)

PHP Warning appears in debug.log.
The issue occurs due to improper input handling in the REST API request.

Actual Result (After Patch Applied)

✅ No PHP warnings or errors in debug.log.
The request is handled securely without generating unnecessary logs.

Attachments

[Attach relevant screenshots of debug.log before and after the patch]

Test Conclusion

Status: ✅ Issue Fixed After Patch
Impact: Medium (Affects API request handling and debug logs)
Recommendation:

Ensure the patch is merged into future WordPress core updates.
Perform further testing with other unexpected input payloads to confirm robustness.
Verify compatibility with different PHP versions and REST API authentication methods.

screenshots of debug.log before and after the patch :
Before Patch: ​https://prnt.sc/Tbtg4znZfbNi
After Patch: ​https://prnt.sc/FHGcYJu9AorY

[SirLouen]

Combined Bug and Test Reproduction Report

Description

This report validates that the patch works as expected.

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

Environment

• WordPress: 6.8-beta3-60042-src
• PHP: 8.2.28
• Server: nginx/1.27.4
• Database: mysqli (Server: 8.4.4 / Client: mysqlnd 8.2.28)
• Browser: Chrome 134.0.0.0
• OS: Windows 10/11
• Theme: Twenty Twenty-Five 1.1
• MU Plugins: None activated
• Plugins:
  • Test Reports 1.2.0

Steps to Reproduce

1. Using wordpress-develop, executing command

   $ curl http://localhost:8889/wp-json/wp/v2/users/1 --data '"+response.write(document.domain)+"' -H 'Content-Type: application/json'
   {"code":"rest_cannot_edit","message":"Sorry, you are not allowed to edit this user.","data":{"status":401}}
   

2. 🐞 Returns warning:

   Warning:  foreach() argument must be of type array|object, string given in /var/www/src/wp-includes/rest-api/class-wp-rest-request.php on line 828
   

Actual Results after the PR 7961 patch

1. ✅ No warning after curl has been executed

Additional Notes

• ✅ Unit tests passing
• Two unit tests are passing both previously and now, so we can assume that further checks are being implemented
• 🟠 Some tests could be unified into one single test. More info in the PR.

[jorbin]

I tagged @TimothyBlynJacobs on github. One thing that stands out to me is that while the majority of the code change happens inside set_param, none of the tests are directly calling that function.

[devsahadat]

Thank you for reporting the bug and for providing a solution. I’ve thoroughly reviewed the issue and tested the proposed patch.

Bug Summary:
The problem occurs when a malformed JSON input (e.g., a string or numeric value) is sent in a REST API request, causing a PHP warning due to the foreach() function attempting to iterate over a non-array input.

Solution Evaluation:
The proposed patch effectively skips the sanitization step for scalar inputs, such as strings or numbers, ensuring that only arrays are processed with the foreach() function. This solution resolves the PHP warning as intended.

Test Results:
I have tested the patch by replicating the original issue with the cURL command and verifying the debug logs. After applying the patch, no warnings or errors were logged, confirming that the issue has been resolved.

Additional Insights:
While the patch addresses the specific warning, I suggest further testing with different types of invalid inputs (e.g., arrays with invalid types) to ensure robustness. Additionally, it would be good to verify whether this change affects any existing validation or sanitization rules that may be in place for specific API endpoints.

[nazmul111]

Test Report

This report validates that the patch works as expected.

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

Environment
WordPress: 6.7
PHP: 8.2.13
Server: nginx/1.27.4
Database: mysqli (Server: 8.4.4 / Client: mysqlnd 8.2.28)
Browser: Chrome 134.0.0.0
OS: Windows 10/11
Theme: Twenty Twenty-Five 1.1
MU Plugins: None activated

[audrasjb]

We are a couple hours before 6.8 RC2, so let's move this ticket to 6.9.