Checking callbacks for restricted functions #843

grappler · 2017-02-12T19:26:39Z

fixes #611

I removed preg_replace_callback_array() from the list as the callback is not a parameter but part of the value. In the following example where foobar is a restricted function.

$subject = 'Aaaaaa Bbb';
preg_replace_callback_array(
    [
        '~[a]+~i' => 'foobar',
    ],
    $subject
);

We could make a separate issue for this.

I was not sure where to place the tests for the abstract so I ended up using the old FunctionRestrictionsSniff. If there is a better place to add the tests I can move them there.

grappler · 2017-03-08T10:32:23Z

@jrfnl @GaryJones Can this be merged?

jrfnl · 2017-03-08T13:09:09Z

@grappler Reviewing this now.

jrfnl

@grappler Thank you for your efforts on this.

I'm sorry, but I think this PR still needs quite a lot of work and possibly a rethink on the principle of it.

The original issue only mentioned sniffing for functions through the AbstractFunctionRestrictionsSniff, but the principle behind it can - and probably should - also be applied when checking for sanitization functions and such.

For that to be possible, we need to implement this with more abstraction.

First off, I'd like to suggest moving the $callback_functions list to the WordPress_Sniff as this list might also be useful for other sniffs in the future.
We may even want to split the list in two (or three) - there are callback functions which directly execute (the PHP ones) and callback functions which are used in specific circumstances and even functions which take a callback, but don't execute it. For different purposes we may want to use a different subsection of the list. Splitting the list up would allow for that, in combination with a retrieval function which could get them all:

protected function get_all_callback_functions() {
	static $list;
	if ( ! isset( $list ) ) {
		$list = array_merge( $array1, $array2, $array3 );
	}
	return $list;
}

In a similar vain, a is_callback_function() method in the WordPress_Sniff which would return false or an array of the names of the callback functions would allow other sniffs to use the logic as well.

At a later point we could add an is_callback_method() for class methods which take callbacks.

Apart from that I have left quite a lot of comments on the code itself. Main points:

We're missing quite a lot of functions (I know you based it on a list I posted, but I did say that was a start, not complete).
Array callbacks could be misidentified as function callbacks
Another function parameter could be misidentified as a function callback.

Altogether, this PR is IMHO not ready for merge.

From #611 (comment):

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

I imagine this may need to be addressed separately. In that case, please open a new issue for it.

jrfnl · 2017-03-08T12:59:56Z

WordPress/Tests/Functions/FunctionRestrictionsUnitTest.php

+ */
+
+/**
+ * Unit test class for the DontExtract sniff.


Comment seems incorrect.

jrfnl · 2017-03-08T13:04:40Z

WordPress/Sniffs/Functions/FunctionRestrictionsSniff.php

+					'foobar*',
+				),
+			),
+		);


Please remove this as - in the unlikely event that a theme/plugin would have a function prefixed with foobar, it would trigger.

If you need to add a function group for unit testing only, please use the setUp() method in the unit test file. See https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress/Tests/DB/RestrictedClassesUnitTest.php for an example.

jrfnl · 2017-03-08T13:09:51Z