Make WordPress Core

Opened 2 months ago

Closed 3 weeks ago

Last modified 3 weeks ago

#61837 closed defect (bug) (fixed)

REST API: Uncaught TypeError when post password is provided as integer

Reported by: mlf20's profile mlf20 Owned by: kadamwhite's profile kadamwhite
Milestone: 6.7 Priority: normal
Severity: normal Version: 6.2.2
Component: REST API Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

Upon creating a fresh instance of WordPress and setting up the REST API a malformed post request REST API endpoint /wp-json/wp/v2/posts/{id} results in an exception in the endpoint.

Request body:
'{"date": "2024-08-07T02:04:37", "date_gmt": "2024-08-07T02:04:37", "slug": "hello-world", "status": "publish", "password": 99999999, "author": 1, "featured_media": 0, "comment_status": "open", "ping_status": "open", "format": "", "sticky": false, "template": "PE9PCU5W", "categories": [], "tags": [""]}'

Command to reproduce

curl -X POST "[WORDPRESSDOMAIN]/wp-json/wp/v2/posts/1" --data '{"date": "2024-08-07T02:04:37", "date_gmt": "2024-08-07T02:04:37", "slug": "hello-world", "status": "publish", "password": 99999999, "author": 1, "featured_media": 0, "comment_status": "open", "ping_status": "open", "format": "", "sticky": false, "template": "PE9PCU5W", "categories": [], "tags": [""]}' -H 'Authorization: Basic [ACCESS_TOKEN]' -H 'Content-Type: application/json'

Stacktrace

<br />
<b>Fatal error</b>:  Uncaught TypeError: hash_equals(): Argument #2 ($user_string) must be of type string, int given in /var/www/html/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php:496
Stack trace:
#0 /var/www/html/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php(496): hash_equals('', 99999999)
#1 /var/www/html/wp-includes/rest-api.php(821): WP_REST_Posts_Controller-&gt;get_item_permissions_check(Object(WP_REST_Request))
#2 /var/www/html/wp-includes/class-wp-hook.php(308): rest_send_allow_header(Object(WP_REST_Response), Object(WP_REST_Server), Object(WP_REST_Request))
#3 /var/www/html/wp-includes/plugin.php(205): WP_Hook-&gt;apply_filters(Object(WP_REST_Response), Array)
#4 /var/www/html/wp-includes/rest-api/class-wp-rest-server.php(466): apply_filters('rest_post_dispa...', Object(WP_REST_Response), Object(WP_REST_Server), Object(WP_REST_Request))
#5 /var/www/html/wp-includes/rest-api.php(410): WP_REST_Server-&gt;serve_request('/wp/v2/posts/1')
#6 /var/www/html/wp-includes/class-wp-hook.php(308): rest_api_loaded(Object(WP))
#7 /var/www/html/wp-includes/class-wp-hook.php(332): WP_Hook-&gt;apply_filters('', Array)
#8 /var/www/html/wp-includes/plugin.php(565): WP_Hook-&gt;do_action(Array)
#9 /var/www/html/wp-includes/class-wp.php(399): do_action_ref_array('parse_request', Array)
#10 /var/www/html/wp-includes/class-wp.php(780): WP-&gt;parse_request('')
#11 /var/www/html/wp-includes/functions.php(1334): WP-&gt;main('')
#12 /var/www/html/wp-blog-header.php(16): wp()
#13 /var/www/html/index.php(17): require('/var/www/html/w...')
#14 {main}
  thrown in <b>/var/www/html/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php</b> on line <b>496</b><br />
{"code":"internal_server_error","message":"<p>There has been a critical error on this website.<\/p><p><a href=\"https:\/\/wordpress.org\/documentation\/article\/faq-troubleshooting\/\">Learn more about troubleshooting WordPress.<\/a><\/p>

Change History (11)

This ticket was mentioned in PR #7172 on WordPress/wordpress-develop by @devansh2002.


2 months ago
#1

  • Keywords has-patch added

Add type check for post password field it should be a string.
Trac ticket: https://core.trac.wordpress.org/ticket/61837

#2 @antonvlasenko
2 months ago

  • Keywords reporter-feedback added

Reproduction Report

Environment

  • WordPress: 6.7-alpha-58576-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.6 (macOS)
  • Theme: Twenty Twenty-Four 1.2
  • MU-Plugins: None activated
  • Plugins:
    • JSON Basic Authentication 0.1

Actual Results

  • The bug is reproducible.

Additional Notes

[13-Aug-2024 21:07:21 UTC] PHP Warning:  hash_equals(): Expected user_string to be a string, int given in wordpress.host/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php on line 502
[13-Aug-2024 21:07:21 UTC] PHP Stack trace:
[13-Aug-2024 21:07:21 UTC] PHP   1. {main}() wordpress.host/src/index.php:0
[13-Aug-2024 21:07:21 UTC] PHP   2. require_once() wordpress.host/src/index.php:19
[13-Aug-2024 21:07:21 UTC] PHP   3. require() wordpress.host/src/_index.php:17
[13-Aug-2024 21:07:21 UTC] PHP   4. wp($query_vars = *uninitialized*) wordpress.host/src/wp-blog-header.php:16
...
<the stack trace is too long to post it here>

Supplemental Artifacts

Last edited 2 months ago by antonvlasenko (previous) (diff)

#3 @antonvlasenko
2 months ago

  • Keywords reporter-feedback removed

#4 @kadamwhite
4 weeks ago

  • Summary changed from Uncaught TypeError in wp/v2/posts/{id} endpoint to REST API: Uncaught TypeError when post password is provided as integer

The issue being reported is that an error is thrown when a numeric password is provided as an integer instead of as a string, correct?

The schema for the password property of a Post object specifies it is expected to be provided a string, not an integer: see WP_REST_Posts_Controller::get_item_schema. The column for post password itself is a varchar(255), and we use a regular textual input box for the password on the WordPress UI and frontend. So there's an argument that an integer password shouldn't be accepted, because you should supply the password as a string, the way the schema expects.

That said, I agree that a 500 due to a bad type internally is the correct response here! Thanks for the report.

The linked PR adds a specific type check within the password validation function. Before landing that (or adding a type coercion to ensure the value is a string before passing to hash_equals(), so that we don't error out early without a good message), we should check whether there is a missing layer of existing validation -- is this hash check running before schema validation, or is schema validation not kicking in at all when a non-string is provided?

I'd personally prefer we unify this check with the other schema validation, rather than introducing a one-off check into this function.

Version 0, edited 4 weeks ago by kadamwhite (next)

#5 @kadamwhite
4 weeks ago

OK this one's cool. :D The current patch is not ideal, because we DO validate that value against the schema already, and return an error. But then we call the permission_callback within rest_send_allow_header, and since the password is in the unexpected type, things blow up while trying to send the error response back to the client.

We're specifically running into a situation where we expect to be checking the password query parameter used for GET access, when we're actually processing a POST that updates the password property of a post.

cc @TimothyBlynJacobs -- I'd like to add a basic type coercion within the permission callback to fix the immediate issue, but it might be worth looking at whether the rest_send_allow_header method is doing too much.

#6 @kadamwhite
4 weeks ago

  • Owner set to kadamwhite
  • Status changed from new to accepted

Update after in-person discussion with @TimothyBlynJacobs , the core issue we should fix is that the get item check should not be considering post body password, only query parameter password.

This ticket was mentioned in PR #7367 on WordPress/wordpress-develop by @kadamwhite.


4 weeks ago
#7

  • Keywords has-unit-tests added

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

Adding unit tests for failing situation described in ticket, then will update with a merged patch.

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


4 weeks ago

#9 @kadamwhite
3 weeks ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 59036:

REST API: Only check password value in query parameters while checking post permissions.

The password property which gets sent as part of a request POST body while setting a post's password should not be checked when calculating post visibility permissions.

That value in the request body is intended to update the post, not to authenticate, and may be malformed or an invalid non-string type which would cause a fatal when checking against the hashed post password value.

Query parameter ?password= values are the correct interface to check, and are also guaranteed to be strings.

Props mlf20, devansh016, antonvlasenko, TimothyBlynJacobs, kadamwhite.
Fixes #61837.

@kadamwhite commented on PR #7367:


3 weeks ago
#10

Merged in 15b7d2a86885a6c83b520277f97b1debe31048fb

#11 @kadamwhite
3 weeks ago

  • Milestone changed from Awaiting Review to 6.7
Note: See TracTickets for help on using tickets.