WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 13 months ago

Last modified 13 months ago

#38984 closed defect (bug) (fixed)

REST API: Fix incorrect calls to `rest_sanitize_value_from_schema`

Reported by: jnylen0 Owned by: rachelbaker
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch commit dev-reviewed
Focuses: Cc:

Description

Two instances in WP_REST_Users_Controller where we are passing $request instead of $args. This allows some minor weirdness:

POST /wp/v2/users?username=abcd&password=efgh&type=array

A user will be created with the username and password set to Array because $request['array'] is interpreted as a type argument.

One more incorrect call in rest_sanitize_request_arg where we are calling the function with a nonexistent third argument.

Attachments (2)

38984.diff (1.8 KB) - added by jnylen0 13 months ago.
38984.2.diff (1.6 KB) - added by rachelbaker 13 months ago.
Changes strval() back to (string) typecasting

Download all attachments as: .zip

Change History (13)

@jnylen0
13 months ago

#1 @joehoyle
13 months ago

@jnylen0 looking at this, I think the intention was to use rest_sanitize_request_arg rather than rest_sanitize_value_from_schema. I think maybe it would be better to use rest_sanitize_request_arg incase these params get other schema / options in the future.

#2 @jnylen0
13 months ago

Yep, I wrote this code in #38739.

If we add additional schema / options here, it will presumably be for a reason that will have unit tests and will probably be addressed in the code for these check_user* functions. Until then I prefer the clarity and explicitness of strval - apparently it's easy to misuse these functions.

#3 follow-up: @rachelbaker
13 months ago

@jnylen0 for the username we can't simply use any string value. There are invalid characters that need to be stripped. I *think* we would want to use the sanitize_user() function for this.

#4 in reply to: ↑ 3 ; follow-up: @jnylen0
13 months ago

Replying to rachelbaker:

@jnylen0 for the username we can't simply use any string value. There are invalid characters that need to be stripped.

This is already handled (see validate_username call in the check_username function, and associated tests)

#5 in reply to: ↑ 4 @rachelbaker
13 months ago

Replying to jnylen0:

Replying to rachelbaker:

@jnylen0 for the username we can't simply use any string value. There are invalid characters that need to be stripped.

This is already handled (see validate_username call in the check_username function, and associated tests)

Ah. I had to scroll down :). Now I remember why I don't review patches on mobile.

Why the change to strval() from the (string) typecasting? Is there a reason to change that? In the RC phase of a release I would prefer avoid non-bug code tweaks.

#6 @jnylen0
13 months ago

Should be fine either way. I only changed it because rest_sanitize_value_from_schema uses strval (ref). (When this code was originally written, that conversion to string wasn't present.)

Last edited 13 months ago by jnylen0 (previous) (diff)

This ticket was mentioned in Slack in #core by helen. View the logs.


13 months ago

@rachelbaker
13 months ago

Changes strval() back to (string) typecasting

#8 @rachelbaker
13 months ago

  • Keywords commit dev-feedback added
  • Owner set to rachelbaker
  • Status changed from new to assigned

+1 on 38984.2.diff which just removes the added strval() function in check_user_password() and check_username().

Still needs another permanent committer to review.

#9 @ocean90
13 months ago

  • Keywords dev-reviewed added; dev-feedback removed

#10 @rachelbaker
13 months ago

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

In 39400:

REST API: Fix incorrect uses of rest_sanitize_value_from_schema().

In the check_username() and check_password() callbacks in the Users controller cast the provided request value to a string. The rest_sanitize_value_from_schema() function was being used incorrectly which was causing unintended request parsing.
In rest_sanitize_request_arg() do not pass nonexistent third parameter for the rest_sanitize_value_from_schema() function.

Props jnylen0, joehoyle, rachelbaker, ocean90.
Fixes #38984.

#11 @rachelbaker
13 months ago

In 39401:

REST API: Fix incorrect uses of rest_sanitize_value_from_schema().

In the check_username() and check_password() callbacks in the Users controller cast the provided request value to a string. The rest_sanitize_value_from_schema() function was being used incorrectly which was causing unintended request parsing.
In rest_sanitize_request_arg() do not pass nonexistent third parameter for the rest_sanitize_value_from_schema() function.

Props jnylen0, joehoyle, rachelbaker, ocean90.

Merges [39400] to the 4.7 branch.
Fixes #38984 for 4.7.

Note: See TracTickets for help on using tickets.