Add a sniff for "Restricted Filters"

[rebeccahum]

As per #87, I think we should add a sniff that will detect a list of restricted filters...with upload_mimes being the first one on it!

[GaryJones]

Since this is a public property, it could be completely overriden by adding something like <property name="restrictedFilters" type="array"><element value="gettext"/></property> within a PHPCS config file. In this case, gettext would be the only filter that would then get marked as a violation, instead of in addition to upload_mimes. Is this the desired behaviour?

If not, then you have two options:

1. Make the property private, so it could be adjusted from the PHPCS config file.
2. Make the property private, and introduce a public $customRestrictedFunctions` property, and then merge the arrays at runtime.

For the second option, see here for an example of this setup, which aren't merged (since there's an extra public flag), but are both checked against.

Alternatively, see here which has a load of public $custom*Functions properties, which are then merged as needed.

[GaryJones]

I'd like to see more tests please:

• combinations of whitespace / no-whitespace before and after the filter name arg i.e. add_filter('upload_mimes' , 'foo' );
• string with upper case characters ('UPLoad_MimEs')
• string with double quotes ("upload_mimes")
• strings with concatenation ('upload_' . 'mimes')
• strings with compounded strings/vars
  • $x = 'd_m'; add_filter( 'uploa' . $x . 'imes', 'foo')
  • $x = 'd_m'; add_filter( "uploa{$x}imes", 'foo')
• add_filter() calls with anonymous function callbacks or other callables.
• add_filter() called dynamically (may be overkill?)
  • $x = 'add_filter'; $x( 'upload_mimes', 'foo' );
• etc.

See https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress/Tests/NamingConventions/ValidHookNameUnitTest.1.inc and sibling files for other ideas of ways folks could try and bypass the check by mangling the filter name string.

Your code may already cope with some of those (like double-quotes), but adding them as tests just proves that for the future.

Also, keep a clear linespace on the last line (adjust your editor to do this, since the repo doesn't yet have an .editorconfig file).

[GaryJones]

Also, whilst the PR creates the sniff, the test file, and the test inc file, it doesn't actually add the sniff to any ruleset, along with a link reference to the relevant documentation page(s).

[GaryJones]

Capital needed for the start of comments, per WordPress-Docs.

[GaryJones]

Capital needed for the start of comments, per WordPress-Docs.

Code base is PHP 5.6+, so short array syntax can be used.

[GaryJones]

Capital needed for the start of comments, per WordPress-Docs.

[GaryJones]

Add a short comment after each one, explaining what it is testing:

add_filter( 'upload_mimes', 'bad_example_function' ); // Simple string.
add_filter('upload_mimes' ,'bad_example_function'); // Incorrect spacing.
add_filter(  'upload_mimes','bad_example_function'); // Incorrect spacing.
add_filter( "upload_mimes" ,'bad_example_function'); // Double quotes.
add_filter( 'upLoad_mimeS' ,'bad_example_function'); // Uppercase characters.
add_filter( 'upload_' . 'mimes' ,'bad_example_function'); // Single concatenation.
add_filter( 'upl' . 'oad_' . 'mimes' ,'bad_example_function'); // Multiple concatenation.
add_filter( 'upload_mimes', function() { // Anonymous callback.
	// Do stuff.	
});

[GaryJones]

I've spoken to @jrfnl to see if PHPCS or WPCS has some existing battle-tested code for getting the first/second/third/etc argument from a function call, that might already exist and cater for the possibility of dynamic string arguments.

She's not got back to me yet, but get_function_call_parameters() looks to be what I'm after, and this is used in a AbstractFunctionParameterSniff which is what some sniffs extend. This allows simply checking for $parameters[1], for instance.

WPCS has this Sniff, which apart from containing a bunch of properties, also contains some re-usable methods. VIPCS doesn't yet have anything like this. Since we can use PHP 5.6, we could separate this into Traits that sniffs like the one in this PR could then use.

This would be a separate Issue, but I think one that would be worthwhile to do first, and then make use of it for a PR like this one. What do you think @rebeccahum ?

In the meantime, since WPCS is a require dependency, you could have your new sniff class extend AbstractFunctionParametersSniff, adjust the method names, and be able to immediately target the correct parameter, without worrying exactly how to be confident that you've caught all possibilities.

[GaryJones]

@rebeccahum So, give this a try, as a straight replacement for the Sniff:

<?php
/**
 * WordPressVIPMinimum Coding Standard.
 *
 * @package VIPCS\WordPressVIPMinimum
 */

namespace WordPressVIPMinimum\Sniffs\Filters;

use WordPress\AbstractFunctionParameterSniff;

/**
 * This sniff restricts usage of some filters.
 *
 * @package VIPCS\WordPressVIPMinimum
 *
 * @since 0.4.0
 */
class RestrictedFilterSniff extends AbstractFunctionParameterSniff {

	/**
	 * The group name for this group of functions.
	 *
	 * @var string
	 */
	protected $group_name = 'restricted_filters';

	/**
	 * Functions this sniff is looking for.
	 *
	 * @var array The only requirement for this array is that the top level
	 *            array keys are the names of the functions you're looking for.
	 *            Other than that, the array can have arbitrary content
	 *            depending on your needs.
	 */
	protected $target_functions = [
		'add_filter' => true,
		'add_action' => true,
	];

	/**
	 * List of restricted filter names.
	 *
	 * @var array
	 */
	private $restricted_filters = [
		'upload_mimes' => [
			'error'     => 'Please ensure that the mimes being filtered do not include insecure types (e.g. SVG). Manual inspection required.',
			'errorcode' => 'UploadMimes',
		],
	];

	/**
	 * Process the parameters of a matched function.
	 *
	 * @param int    $stackPtr        The position of the current token in the stack.
	 * @param array  $group_name      The name of the group which was matched.
	 * @param string $matched_content The token content (function name) which was matched.
	 * @param array  $parameters      Array with information about the parameters.
	 * @return int|void Integer stack pointer to skip forward or void to continue
	 *                  normal file processing.
	 */
	public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
		foreach ( $this->restricted_filters as $restricted_filter => $filter_args ) {
			if ( $this->normalize_filter_name( $parameters[1] ) === $restricted_filter ) {
				$this->phpcsFile->addWarning( $filter_args['error'], $stackPtr, $filter_args['errorcode'] );
			}
		}
	}

	/**
	 * Transform string.
	 *
	 * @param array $parameter Array with information about a parameter.
	 * @return string
	 */
	private function normalize_filter_name( $parameter ) {
		// If concatenation is found, build filter name.
		$concat_ptr = $this->phpcsFile->findNext(
			T_STRING_CONCAT,
			$parameter['start'],
			$parameter['end'],
			false,
			null,
			true
		);

		if ( $concat_ptr ) {
			$filter_name = '';
			for ( $i = $parameter['start'] + 1; $i < $parameter['end']; $i++ ) {
				if ( T_CONSTANT_ENCAPSED_STRING === $this->tokens[ $i ]['code'] ) {
					$filter_name .= str_replace( [ "'", '"' ], '', $this->tokens[ $i ]['content'] );
				}
			}
		} else {
			$filter_name = $parameter['raw'];
		}

		// Remove quotes (double and single), and use lowercase.
		return trim( strtolower( str_replace( [ "'", '"' ], '', $filter_name ) ) );
	}
}

All of the existing unit tests pass, and there's a couple of others as well:

add_filter( "upload_" . 'mimes' ,'bad_example_function'); // Single concatenation with double and single quotes.
add_filter( 'upl' . "oad_" . "mimes" ,'bad_example_function'); // Multiple concatenation with double and single quotes.
add_action( 'upload_mimes', 'bad_example_function' ); // Check `add_action()`, which is an alias for `add_filter()`.

The normalize_filter_name() could be moved to a base VIPCS sniff class if it was needed to be re-used, and the concat logic could be extracted into its own concatenate_parameter() method or something.

We've used the logic from WPCS, which makes it easier to generalise things and make this sniff more re-usable. Your existing warning message referred directly to the upload_mimes filter, for instance, which means we couldn't add different warning messages for other filters. By moving having the config array of error and errorcode, this is solved.

I've got a local branch with this on, which cherry-picked the commits from the branch on your repo, so I can wrap this up with a fresh PR if you wish - you'd still get author credits for your commits, even after squashing :-)

[GaryJones]

I went ahead and pushed it to a branch: rebecca-gary/restricted-hook-sniff

You'll notice that I also renamed it to RestrictedHook, rather than RestrictedFilter, since add_action() is really just a wrapper for add_filter() anyway.

@rebeccahum If this looks good to you, then go ahead and do the PR for this, or let me know and I will and you can approve :-)

[rebeccahum]

@GaryJones Thank you so much, this looks great. This makes sense to me since we can re-use the logic from WPCS. However, are there any downsides on having WPCS dependencies?

[GaryJones]

However, are there any downsides on having WPCS dependencies?

We already had a require dependency for WPCS, since right now, a few sniffs depend on sniffs from WordPress-VIP. See #187.

I think that #187 could still go ahead, since the code that this PR relies on is not a concrete sniff, but an abstract one that provides re-usable logic, not specific implementation for the end problem.

However, of more interest is squizlabs/PHP_CodeSniffer#2189 which, if implemented, gives a definite path to not have to rely on WPCS for the function parameter logic.

[GaryJones]

Approved, but since I've not officially started at A8C yet, it may be worth getting another pair of eyes. cc @sboisvert @tomjn @gudmdharalds.