Restricted functions used in callbacks

[jrfnl]

While debugging the AbstractFunctionRestrictionsSniff class, I realized that any restricted functions used as callbacks, i.e. within add_action(), call_user_func(), array_map() are completely ignored.

That makes this kind of sniff extremely easy to bypass and while this is not so much an issue for people who elect to use the WPCS, this will be an issue for the Theme Review Theme Check to sniffs project as in that case, theme authors won't have a choice and bypassing checks that way is something we'll need to guard against.

I'm investigating how we can solve this. /cc @grappler

[westonruter]

Humm. Good catch! So probably the sniff needs to look for any instances of add_action, array_map, etc. and then check to see if the callback arg is among the restricted functions list.

[jrfnl]

Yup, I've made a small start with this, but need to concentrate on some other things first now.

If someone else wants to take it on, feel free. I did already create a list of all the PHP and WP functions I could think of which take callbacks with the position of the callback in the argument list, so can share that as a starting point. Any takers ?

[grappler]

@jrfnl Could you share the list of PHP and WP functions?

[jrfnl]

    /**
     * List of known PHP and WP function which take a callback as an argument.
     *
     * @var array <string function name> => <int callback argument position>
     */
    protected $callback_functions = array(
        'add_filter' => 2,
        'add_action' => 2,
        'call_user_func' => 1,
        'call_user_func_array' => 1,
        'forward_static_call' => 1,
        'forward_static_call_array' => 1,
        'array_diff_uassoc' => -1, // = last argument passed
        'array_diff_ukey' => -1, // = last argument passed
        'array_filter' => 2,
        'array_intersect_uassoc' => -1, // = last argument passed
        'array_intersect_ukey' => -1, // = last argument passed
        'array_map' => 1,
        'array_reduce' => 2,
        'array_udiff_assoc' => -1, // = last argument passed
        'array_udiff_uassoc' => array( -1, -2 ), // = last argument passed
        'array_udiff' => -1, // = last argument passed
        'array_uintersect_assoc' => -1, // = last argument passed
        'array_uintersect_uassoc' => array( -1, -2 ), // = last argument passed
        'array_uintersect' => -1, // = last argument passed
        'array_walk' => 2,
        'array_walk_recursive' => 2,
        'iterator_apply' => 2,
        'usort' => 2,
        'uasort' => 2,
        'uksort' => 2,
        'preg_replace_callback' => 2,
        'preg_replace_callback_array' => 1, // = array of patterns and callbacks...
        'mb_ereg_replace_callback' => 2,
        'header_register_callback' => 1,
        'ob_start' => 1,
        'set_error_handler' => 1,
        'set_exception_handler' => 1,
        'register_shutdown_function' => 1,
        'register_tick_function' => 1,
    );

Also - don't forget eval() and create_function()

[grappler]

I was able to solve this with three lines of code

        if ( isset( $this->callback_functions[ $token['content'] ] ) ) {
            $parameter_arg = $this->getFunctionCallParameter( $phpcsFile, $stackPtr, $this->callback_functions[ $token['content'] ] );
            $token['content'] = trim( $parameter_arg['raw'], '\'"' );
        }

Just need to wait for the utility functions like getFunctionCallParameter to become available.

[grappler]

Now that the utility functions have been merged in I could have another go at this.

https://github.com/grappler/WordPress-Coding-Standards/tree/feature/611-restricted-callbacks

There are a couple of functions where a callback can be defined in two positions e.g.
array_udiff_uassoc I am not sure how to check both. Any ideas?

As we need to support negative positions I was wondering if the support should be ported to teh utility function get_function_call_parameters().

[jrfnl]

@grappler The initial array I provided was from before the utility functions were created. If you'd change the array format to actual param positions instead of relative positions, you could bypass that issue. Changing the values to all be arrays with positions would possibly also make things easier as you wouldn't have to check for number vs array, but could just foreach through it.

[grappler]

If you'd change the array format to actual param positions instead of relative positions, you could bypass that issue.

The positions need to stay relative as for array_udiff_uassoc you can have a different number of arrays. So the last and second last position number changes.

Changing the values to all be arrays with positions would possibly also make things easier as you wouldn't have to check for number vs array, but could just foreach through it.

Ok, I got it work in the end. Had to change a few things around.

[jrfnl]

@grappler Great to hear. I look forward to seeing what you've created!

[jrfnl]

I'm marking this issue as something which should probably be solved via utility functions to be created in PHPCSUtils.
Analyzing callbacks and the functions which expect them can get pretty complicated, so I think having this more complicated logic in PHPCSUtils make sense (if it can be solved at all).