Make WordPress Core

Opened 7 months ago

Closed 6 months ago

Last modified 4 months 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 (13)

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


7 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
7 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 7 months ago by antonvlasenko (previous) (diff)

#3 @antonvlasenko
7 months ago

  • Keywords reporter-feedback removed

#4 @kadamwhite
6 months 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 NOT the correct response! 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.

Last edited 6 months ago by kadamwhite (previous) (diff)

#5 @kadamwhite
6 months 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
6 months 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.


6 months 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.


6 months ago

#9 @kadamwhite
6 months 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:


6 months ago
#10

Merged in 15b7d2a86885a6c83b520277f97b1debe31048fb

#11 @kadamwhite
6 months ago

  • Milestone changed from Awaiting Review to 6.7

@desrosj commented on PR #7172:


4 months ago
#12

@devansh016 While compiling a list of contributors that helped with the upcoming WordPress 6.7 release using the project's "Prop" script, your account came up as not having linked their WordPress.org and GitHub accounts.

In the WordPress project, all credit given out must be attached to a WordPress.org account. After creating a W.org account (or logging in to your preexisting one), you can link your GitHub account following these steps in the Core Handbook. If you could kindly link your .org account and just share your .org username, we can ensure you receive proper attribution! cc/@jeffpaul

@desrosj commented on PR #7172:


4 months ago
#13

Ah, actually found your .org! https://profiles.wordpress.org/devansh2002/ Thanks for your contributions!

Note: See TracTickets for help on using tickets.