WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 7 months ago

Last modified 7 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 7 months ago.
38984.2.diff (1.6 KB) - added by rachelbaker 7 months ago.
Changes strval() back to (string) typecasting

Download all attachments as: .zip

Change History (13)

@jnylen0
7 months ago

#1 @joehoyle
7 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
7 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
7 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
7 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
7 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
7 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 7 months ago by jnylen0 (previous) (diff)

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


7 months ago

@rachelbaker
7 months ago

Changes strval() back to (string) typecasting

#8 @rachelbaker
7 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
7 months ago

  • Keywords dev-reviewed added; dev-feedback removed

#10 @rachelbaker
7 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
7 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.