diff --git a/WordPressVIPMinimum/Sniffs/Security/EscapingVoidReturnFunctionsSniff.php b/WordPressVIPMinimum/Sniffs/Security/EscapingVoidReturnFunctionsSniff.php index 02d2ab6d..ddbdef9e 100644 --- a/WordPressVIPMinimum/Sniffs/Security/EscapingVoidReturnFunctionsSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/EscapingVoidReturnFunctionsSniff.php @@ -10,8 +10,9 @@ namespace WordPressVIPMinimum\Sniffs\Security; use PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Utils\PassedParameters; +use WordPressCS\WordPress\AbstractFunctionParameterSniff; use WordPressCS\WordPress\Helpers\PrintingFunctionsTrait; -use WordPressVIPMinimum\Sniffs\Sniff; /** * Flag functions that don't return anything, yet are wrapped in an escaping function call. @@ -20,46 +21,131 @@ * * @uses \WordPressCS\WordPress\Helpers\PrintingFunctionsTrait::$customPrintingFunctions */ -class EscapingVoidReturnFunctionsSniff extends Sniff { +class EscapingVoidReturnFunctionsSniff extends AbstractFunctionParameterSniff { use PrintingFunctionsTrait; /** - * Returns an array of tokens this test wants to listen for. + * The group name for this group of functions. * - * @return array + * @var string */ - public function register() { - return [ - T_STRING, - ]; - } + protected $group_name = 'escaping_void'; + + /** + * Functions this sniff is looking for. + * + * @var array Keys are the target functions, + * value, the name and position of the target parameter. + */ + protected $target_functions = [ + 'esc_attr' => [ + 'param_position' => 1, + 'param_name' => 'text', + ], + 'esc_attr__' => [ + 'param_position' => 1, + 'param_name' => 'text', + ], + 'esc_attr_e' => [ + 'param_position' => 1, + 'param_name' => 'text', + ], + 'esc_attr_x' => [ + 'param_position' => 1, + 'param_name' => 'text', + ], + 'esc_html' => [ + 'param_position' => 1, + 'param_name' => 'text', + ], + 'esc_html__' => [ + 'param_position' => 1, + 'param_name' => 'text', + ], + 'esc_html_e' => [ + 'param_position' => 1, + 'param_name' => 'text', + ], + 'esc_html_x' => [ + 'param_position' => 1, + 'param_name' => 'text', + ], + 'esc_js' => [ + 'param_position' => 1, + 'param_name' => 'text', + ], + 'esc_textarea' => [ + 'param_position' => 1, + 'param_name' => 'text', + ], + 'esc_url' => [ + 'param_position' => 1, + 'param_name' => 'url', + ], + 'esc_url_raw' => [ + 'param_position' => 1, + 'param_name' => 'url', + ], + 'esc_xml' => [ + 'param_position' => 1, + 'param_name' => 'text', + ], + 'tag_escape' => [ + 'param_position' => 1, + 'param_name' => 'tag_name', + ], + 'wp_kses' => [ + 'param_position' => 1, + 'param_name' => 'content', + ], + 'wp_kses_data' => [ + 'param_position' => 1, + 'param_name' => 'data', + ], + 'wp_kses_one_attr' => [ + 'param_position' => 1, + 'param_name' => 'attr', + ], + 'wp_kses_post' => [ + 'param_position' => 1, + 'param_name' => 'data', + ], + ]; /** - * Process this test when one of its tokens is encountered + * Process the parameters of a matched function. * - * @param int $stackPtr The position of the current token in the stack passed in $tokens. + * @param int $stackPtr The position of the current token in the stack. + * @param string $group_name The name of the group which was matched. + * @param string $matched_content The token content (function name) which was matched + * in lowercase. + * @param array $parameters Array with information about the parameters. * * @return void */ - public function process_token( $stackPtr ) { + public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) { + $param_position = $this->target_functions[ $matched_content ]['param_position']; + $param_name = $this->target_functions[ $matched_content ]['param_name']; - if ( strpos( $this->tokens[ $stackPtr ]['content'], 'esc_' ) !== 0 && strpos( $this->tokens[ $stackPtr ]['content'], 'wp_kses' ) !== 0 ) { - // Not what we are looking for. + $target_param = PassedParameters::getParameterFromStack( $parameters, $param_position, $param_name ); + if ( $target_param === false ) { + // Missing (required) target parameter. Probably live coding, nothing to examine (yet). Bow out. return; } - $next_token = $this->phpcsFile->findNext( Tokens::$emptyTokens, $stackPtr + 1, null, true ); + $ignore = Tokens::$emptyTokens; + $ignore[ T_NS_SEPARATOR ] = T_NS_SEPARATOR; - if ( $this->tokens[ $next_token ]['code'] !== T_OPEN_PARENTHESIS ) { - // Not a function call. + $next_token = $this->phpcsFile->findNext( $ignore, $target_param['start'], ( $target_param['end'] + 1 ), true ); + if ( $next_token === false || $this->tokens[ $next_token ]['code'] !== T_STRING ) { + // Not what we are looking for. return; } - $next_token = $this->phpcsFile->findNext( Tokens::$emptyTokens, $next_token + 1, null, true ); - - if ( $this->tokens[ $next_token ]['code'] !== T_STRING ) { - // Not what we are looking for. + $next_after = $this->phpcsFile->findNext( Tokens::$emptyTokens, $next_token + 1, ( $target_param['end'] + 1 ), true ); + if ( $next_after === false || $this->tokens[ $next_after ]['code'] !== T_OPEN_PARENTHESIS ) { + // Not a function call inside the escaping function. return; } diff --git a/WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.inc b/WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.inc index 9a9de966..3ab7ba59 100644 --- a/WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.inc @@ -1,4 +1,100 @@ esc_js( _deprecated_file() ); +$this?->esc_textarea( _e( $something ) ); +MyClass::esc_url( wp_die(), ); +echo WP_KSES; +namespace\wp_kses_post( user_error( $something ) ); + +function esc_js( _E $param ) {} // Class "_E" as type declaration. + +/* + * These should all be okay. + */ +// Incomplete function call, should be ignored by the sniff. +$incorrect_but_ok = esc_html(); +$incorrect_but_ok = wp_kses(); + +// Parameter does not contain a function call. +esc_html_x( $hook_name ); +\wp_kses_data( $obj->_deprecated_function() ); +esc_attr__( CONSTANT_NAME, ); +\ESC_ATTR_X( "do_$something" ); + +esc_attr(...$params); // PHP 5.6 argument unpacking. + +#[Esc_Textarea('text')] // PHP 8.0+ class instantiation via an attribute. Can't contain a nested function call anyway. +function foo() {} + +array_walk($text_strings, \esc_html__(...),); // PHP 8.1 first class callable. + +// Parameter contains a function call but not calling one of the printing functions. +esc_html( __( $something ) ); +Esc_URL_Raw( \get( $url ) ); +\esc_xml( deprecated_argument() ); // Note: missing "_" prefix for printing function. +wp_kses_one_attr ( + /* comment */ + get_attr(), +); + + +/* + * These should all be flagged. + */ +esc_html( _e( $something ) ); +esc_attr__( \_deprecated_argument( $a ) ); +ESC_ATTR_E( _Deprecated_Constructor($a), ); +\esc_attr_x( _deprecated_file(), ); +esc_attr( _deprecated_function() ); +esc_HTML__( _deprecated_hook() ); +esc_html_e( _Doing_It_Wrong( $a ) ); +esc_html_X( \_e( $foo ), ); +\esc_html( /*comment*/ _ex( $foo ) ); +esc_js( printf( $foo ) ); +Esc_textarea( trigger_error( $foo ) ); +\esc_URL_raw( \user_Error( $foo, '' ), ); +esc_url( vprintf( $foo, ) /*comment*/ ); +WP_Kses_Data( WP_DIE( $foo ), ); +wp_kses_one_attr /*comment*/ ( wp_dropdown_pages( $pages ) ); +wp_kses_post( + \_deprecated_function( $fn ) +); +wp_kses( _ex() ); + +// Adding custom printing functions is supported. +// phpcs:set WordPressVIPMinimum.Security.EscapingVoidReturnFunctions customPrintingFunctions[] to_screen,my_print +esc_attr( to_screen( $var1 ) ); +\wp_kses_post( my_print() ); +// phpcs:set WordPressVIPMinimum.Security.EscapingVoidReturnFunctions customPrintingFunctions[] + +tag_escape( $tag ); // OK. +tag_escape( _e() ); // Bad. + +// Bug: these are not function calls inside. +esc_attr__( User_Error::CONSTANT_NAME, ); // OK. +esc_js( _doing_it_wrong::class ); // OK, PHP 5.5 ::class resolution. + +// Safeguard handling of function calls using PHP 8.0+ named parameters. +esc_attr_x( context: get_context(), domain: _e(),); // OK, well, not really, missing required $text param, but that's not the concern of this sniff. +esc_url_raw(protocols: $protocols, url: $url); // OK. +wp_kses_one_attr(element: $element, att: trigger_error( $foo ) ); // OK, well, not really, typo in param name, but that's not the concern of the sniff. +wp_kses( content: \not_deprecated_function( $fn ) ); // OK. +wp_kses( content: /*comment*/ ); // OK, well, not really, invalid function call, but that's not the concern of the sniff. + +esc_html_x(context: $c, text: _doing_it_wrong() ); // Bad. +wp_kses(allowed_html: $allowed_html, content: printf() ); // Bad. +esc_url(protocols: $protocols, url: vprintf()); // Bad. + +// These are no longer flagged as they are not "escaping" functions for the purpose of this sniff. +esc_sql( trigger_error( $foo ) ); // OK. +wp_kses_allowed_html( _deprecated_function() ); // OK. + +// These are no longer flagged as these are not the WP native escaping functions, so we don't know what parameter to check. +esc_something( trigger_error( $foo ) ); // OK. +wp_kses_page( _deprecated_function() ); // OK. diff --git a/WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.php b/WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.php index 52734c3f..954e2bb2 100644 --- a/WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.php +++ b/WordPressVIPMinimum/Tests/Security/EscapingVoidReturnFunctionsUnitTest.php @@ -23,7 +23,29 @@ class EscapingVoidReturnFunctionsUnitTest extends AbstractSniffUnitTest { */ public function getErrorList() { return [ - 3 => 1, + 50 => 1, + 51 => 1, + 52 => 1, + 53 => 1, + 54 => 1, + 55 => 1, + 56 => 1, + 57 => 1, + 58 => 1, + 59 => 1, + 60 => 1, + 61 => 1, + 62 => 1, + 63 => 1, + 64 => 1, + 65 => 1, + 68 => 1, + 72 => 1, + 73 => 1, + 77 => 1, + 90 => 1, + 91 => 1, + 92 => 1, ]; }