Make WordPress Core

Opened 9 years ago

Closed 4 years ago

Last modified 8 days ago

#34659 closed enhancement (worksforme)

Whitelist for validation functions which only accept one argument

Reported by: danielbachhuber's profile danielbachhuber Owned by: rmccue's profile rmccue
Milestone: Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch needs-testing
Focuses: Cc:

Description

WP_REST_Request supports a validate_callback for arguments. When it calls the validate_callback, it passes additional contextual arguments:

$valid_check = call_user_func( $arg['validate_callback'], $param, $this, $key );

However, some validation functions like is_numeric only support one argument:

1) Tests_REST_Request::test_validate_callback_only_pass_value
is_numeric() expects exactly 1 parameter, 3 given

I propose we add a filterable whitelist of validation functions that should only be passed one argument.

We may want to do the same for sanitization callbacks.

Originally:

Attachments (4)

34659.1.diff (2.6 KB) - added by danielbachhuber 9 years ago.
34659.2.diff (4.4 KB) - added by danielbachhuber 9 years ago.
Whitelist specific validation and sanitization functions to only receive one argument
34659.3.diff (3.2 KB) - added by brianhogg 8 years ago.
Adds validate_callback_single_arg and sanitize_callback_single_arg boolean parameters
34659.4.diff (3.1 KB) - added by joshlevinson 8 years ago.
Adding an alternative way of doing this more akin to the plugins API, via {sanitize/validate}_callback_args parameter. Allows you to specify the number of args to pass into your callback. Based on the work of @brianhogg and the idea/pattern of @rmccue.

Download all attachments as: .zip

Change History (34)

#1 follow-up: @TimothyBlynJacobs
9 years ago

The idea of whitelisting functions seems quite a bit kludgy. Most of my sanitization functions, for example, also only accept one parameter. And whitelisting all of those doesn't seem ideal. Plus, what about functions that only need access to the first two?

Why not specify the number of arguments to be passed to the sanitization function the same way we do for hooks?

#2 in reply to: ↑ 1 @danielbachhuber
9 years ago

Replying to TimothyBlynJacobs:

The idea of whitelisting functions seems quite a bit kludgy. Most of my sanitization functions, for example, also only accept one parameter. And whitelisting all of those doesn't seem ideal. Plus, what about functions that only need access to the first two?

In each of these cases, do the functions error or exhibit unexpected behavior when additional arguments are passed? If not, and they silently discard the extra arguments, then you wouldn't need to whitelist them.

Why not specify the number of arguments to be passed to the sanitization function the same way we do for hooks?

Could, although that's a different type of complexity. What would the syntax look like?

#3 follow-ups: @TimothyBlynJacobs
9 years ago

In most cases it would be fine, but it'd be a problem with functions that use a variable number of arguments.

I'd imagine something like this. Though you're right that'd get clunky as well. Especially for a object function name pair.

$arg['validate_callback'] = array( 'myfunction', 1 );

#4 in reply to: ↑ 3 @danielbachhuber
9 years ago

Replying to TimothyBlynJacobs:

I'd imagine something like this. Though you're right that'd get clunky as well. Especially for a object function name pair.

$arg['validate_callback'] = array( 'myfunction', 1 );

Yeah, that's pretty clunky. I can't say that I'm a fan.

@rmccue could I get your thoughts on the patch provided? I'm happy to update it to handle the sanitization callback as well.

#5 in reply to: ↑ 3 @joehoyle
9 years ago

Replying to TimothyBlynJacobs:

In most cases it would be fine, but it'd be a problem with functions that use a variable number of arguments.

I'd imagine something like this. Though you're right that'd get clunky as well. Especially for a object function name pair.

$arg['validate_callback'] = array( 'myfunction', 1 );

I'm not sure if you were pointing it out here - but this would conflict with the array( 'MyClass', 'myMethod' ) syntax for the callbacks.

I'm not a big fan of a whitelist, I'm +1 for specific rest validate functions for those tricky ones line is_numeric()

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


9 years ago

@danielbachhuber
9 years ago

Whitelist specific validation and sanitization functions to only receive one argument

#7 @danielbachhuber
9 years ago

  • Keywords has-unit-tests added

34659.2.diff supports whitelisting specific functions to only receive one argument when used as sanitization or validation callbacks

On Slack, we also explored a couple other options:

  • Only ever passing one argument to the validation / sanitization callback. But, the extra context can be helpful, as we're using in the plugin itself.
  • Introducing an ancillary validate_callback_single argument, which would only pass one argument to the callback. However, this doesn't seem very glamorous.

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


9 years ago

#9 @joehoyle
9 years ago

This looks good to me - I think in the case that we decided to change to :

Only ever passing one argument to the validation / sanitization callback. But, the extra context can be helpful, as we're using in the plugin itself.

At some point, the user-land code would still be compatible, so I think the whitelist is the best way to go.

#10 @danielbachhuber
9 years ago

  • Keywords commit added
  • Owner set to johnbillion
  • Status changed from new to assigned

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


9 years ago

#12 @danielbachhuber
9 years ago

  • Owner changed from johnbillion to rmccue

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


9 years ago

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


9 years ago

#15 @rmccue
9 years ago

  • Keywords 4.5-early added; commit removed
  • Milestone changed from 4.4 to Future Release

I'm unconvinced on the whitelist approach, and I'd rather not lock us into using it for future versions. As painful as using this stuff in the meantime may be (with wrapper functions)... bumping to next release.

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


9 years ago

#17 @brianhogg
9 years ago

Since we can't go back on having validate_callback and sanitize_callback functions being passed 3 parameters without breaking things for those that are using them, whitelisting core WP and PHP functions that either return a different value then when passed just the argument value or throws a warning seems like the only reasonable approach. Functions like strip_tags also return NULL when passed 3 parameters.

The only alternative I see is doing some kind of reflection to only pass in the number of parameters a function accepts, which would likely cause performance issues and be unnecessary (and undesired) for functions that dynamically fetch arguments.

#18 @rmccue
9 years ago

  • Keywords 4.5-early removed
  • Type changed from defect (bug) to enhancement

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


9 years ago

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


8 years ago

#21 @rmccue
8 years ago

  • Keywords needs-patch added; has-patch has-unit-tests removed

I want to solve this with sanitize_callback_args and validate_callback_args, which are used as a number in array_slice. This matches the $accepted_args parameter to add_filter, and is completely backwards compatible, plus doesn't involve some weird special whitelist for specific functions.

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


8 years ago

@brianhogg
8 years ago

Adds validate_callback_single_arg and sanitize_callback_single_arg boolean parameters

#23 @brianhogg
8 years ago

After discussing with @rachelbaker added a validate_callback_single_arg and sanitize_callback_single_arg. When set to true will only pass the value and nothing else to the callback function.

#24 @brianhogg
8 years ago

  • Keywords has-patch added; needs-patch removed

@joshlevinson
8 years ago

Adding an alternative way of doing this more akin to the plugins API, via {sanitize/validate}_callback_args parameter. Allows you to specify the number of args to pass into your callback. Based on the work of @brianhogg and the idea/pattern of @rmccue.

#25 @brianhogg
8 years ago

@joshlevinson the decision on _single_arg was that a) people would likely either want just the value or the three params and b) its not really clear what passing "2" would do or passing sanitize/validate_callback_args with a value greater than 3 (error/warning? no effect?)

If there's a case we missed where one would just want 2 args passed without using an anonymous function for the callback (ie. value and the request object, but without the key) could go with the args count approach?

#26 @rmccue
8 years ago

  • Keywords needs-testing added
  • Version set to 4.7

Patch here looks good to me, but requires some testing. Also need to decide whether this is for 4.7.2 or 4.8.

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


8 years ago

#28 @TimothyBlynJacobs
4 years ago

  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from assigned to closed

Now that our minimum PHP version is 5.6, it seems like this is far better served by using an anonymous function. I don't think it's worth introducing the complexity to WP_REST_Request anymore.

If someone feels strongly about this and wants to own it, feel free to reopen.

#29 @simongcc
3 years ago

(just reading from REST API Handbook)
Is it a good idea to use func_get_arg() to get the number of arguments passed?
This functions seems available since PHP 4, so compatibility might not be a problem.
With this, less explicit work to do by specifying the number of arguments. While development teams could dynamically adjust inside the core.

Not sure if there is other concerns that I did not aware of.

#30 @khoipro
8 days ago

Just bump up this topic because I think we should have some changes for that.

Note: See TracTickets for help on using tickets.