WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 6 months ago

Last modified 4 months ago

#38593 closed enhancement (fixed)

Make rest_parse_request_arg the default sanitize_callback for params

Reported by: joehoyle Owned by: joehoyle
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: REST API Keywords:
Focuses: Cc:

Description

As our support for validating and sanitizing has got better, we are making use of it more and more throughout the codebase. Already rest_parse_request_arg (which does all the auto validation / sanitization) is used for the schema in get_item_schema, it makes sense to just make rest_parse_request_arg the default sanitizer for all params if a type is specified in the options.

This could be disabled with sanitize_callback => false etc.

Attachments (1)

38593.diff (938 bytes) - added by joehoyle 6 months ago.

Download all attachments as: .zip

Change History (12)

@joehoyle
6 months ago

#1 @joehoyle
6 months ago

@rmccue would like to get your thoughts on this.

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


6 months ago

#3 @ChopinBach
6 months ago

I like this idea a lot, as it will probably help out a lot of people from inadvertently creating security issues for their own endpoint development, while still offering the flexibility to disable sanitizing entirely.

To nitpick some coding standards, inline comments must start with a capital letter and end with a full stop character usually a period. :)

#4 @jnylen0
6 months ago

I think this needs a test case for 'sanitize_callback' => false.

#5 @rmccue
6 months ago

Seems fine to me, so long as it's clearly documented that this is the case, and that sanitize_callback can be overridden.

This should potentially be pushed to 4.8 instead though, as it's really an enhancement and we're in beta.

#6 @joehoyle
6 months ago

@rmccue main reason for getting this is now, is I'd like to do a final audit of all the params, and having this would allow us to remove a lot of junk that's accumulated in the param registration that _mostly_ isn't needed now we have a lot of good schema validation stuff.

#7 @rmccue
6 months ago

  • Type changed from defect (bug) to enhancement

#8 @joehoyle
6 months ago

  • Owner set to joehoyle
  • Status changed from new to assigned

#9 @rmccue
6 months ago

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

In 39091:

REST API: Set default sanitize callback if type is set.

Props joehoyle, ChopinBach, jnylen0.
Fixes #38593.

#10 @rachelbaker
5 months ago

In 39563:

REST API: Allow schema sanitization_callback to be set to null to bypass fallback sanitization functions.

The logic in WP_REST_Request->sanitize_params() added in [39091] did not account for null or false being the sanitization_callback preventing overriding rest_parse_request_arg(). This fixes that oversight, allowing the built in sanitization function to be bypassed. See #38593.

Props kkoppenhaver, rachelbaker, jnylen0.
Fixes #39042.

#11 @rachelbaker
4 months ago

In 39642:

REST API: Allow schema sanitization_callback to be set to null to bypass fallback sanitization functions.

The logic in WP_REST_Request->sanitize_params() added in [39091] did not account for null or false being the sanitization_callback preventing overriding rest_parse_request_arg(). This fixes that oversight, allowing the built in sanitization function to be bypassed. See #38593.

Merges [39563] to the 4.7 branch.

Props kkoppenhaver, rachelbaker, jnylen0.
Fixes #39042.

Note: See TracTickets for help on using tickets.