From fa06a594b08983805df0114b58126804e648fafa Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 17 Aug 2023 15:24:08 +0200 Subject: [PATCH] Sniff: move "missing unslashing" handling to callback function As things were, the "missing unslashing" error was being thrown from the `Sniff::is_sanitized()` method, based on whether the `$require_unslash` parameter was set to `true` or `false`. In my opinion, it should not be the responsibility of utility functions to throw errors/warning, but always of the originating sniff. This commit replaces the boolean `$require_unslash` parameter for the `Sniff::is_sanitized()` method to a nullable callable `$unslash_callback` parameter. Instead of letting the `Sniff::is_sanitized()` method throw the error/warning, the callback will now be called and as the originating sniff passes the callback, the originating sniff can now handle the throwing of the error/warning. To remove any presumptions of the callback being passed, being in the same context as the utility method/originating sniff and having access to the `$phpcsFile` via a property, the callback will receive both the `$phpcsFile` as well as the `$stackPtr` as parameters. The "missing unslash" check can still be skipped by not passing the parameter or by passing anything non-callable. This maintains the original behaviour for the method for the default value/`false` case. The `Sniff::add_unslash_error()` method can with this change now be moved to the `ValidatedSanitizedInput` sniff and that sniff will now have full control over the error message and error code. Note: the callback is called using `call_user_func()` instead of `$callable` to allow some tolerance for the _partially supported callables_, which were deprecated in PHP 8.2. While WPCS itself does not use these, the `ValidatedSanitizedInputSniff` is one of the exceptions which has not been made `final`, so an extending class _could_ use one of the _partially supported callables_ and the `Sniff::is_sanitized()` method should not be the reason that doesn't work. --- WordPress/Sniff.php | 41 ++++++++----------- .../Security/ValidatedSanitizedInputSniff.php | 27 +++++++++++- 2 files changed, 43 insertions(+), 25 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index fcca9a5b28..7bd96029c7 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -145,14 +145,24 @@ protected function is_only_sanitized( $stackPtr ) { * Check if something is being sanitized. * * @since 0.5.0 + * @since 3.0.0 The second ($require_unslash) parameter has been changed from + * a boolean toggle to a ?callable $unslash_callback parameter to + * allow a sniff calling this method to handle their "unslashing" + * related messaging itself. * - * @param int $stackPtr The index of the token in the stack. - * @param bool $require_unslash Whether to give an error if no unslashing function - * is used on the variable before sanitization. + * @param int $stackPtr The index of the token in the stack. + * @param callable|null $unslash_callback Optional. When passed, this method will check if an unslashing + * function is used on the variable before sanitization and if not, + * the callback will be called to handle the missing unslashing. + * The callback will receive the $phpcsFile object and the $stackPtr. + * When not passed or `null`, this method will **not** check + * for unslashing issues. + * Defaults to `null` (skip unslashing checks). * * @return bool Whether the token being sanitized. */ - protected function is_sanitized( $stackPtr, $require_unslash = false ) { + protected function is_sanitized( $stackPtr, $unslash_callback = null ) { + $require_unslash = is_callable( $unslash_callback ); // First we check if it is being casted to a safe value. if ( ContextHelper::is_safe_casted( $this->phpcsFile, $stackPtr ) ) { @@ -162,7 +172,7 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { // If this isn't within a function call, we know already that it's not safe. if ( ! isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { if ( $require_unslash ) { - $this->add_unslash_error( $stackPtr ); + call_user_func( $unslash_callback, $this->phpcsFile, $stackPtr ); } return false; @@ -189,7 +199,7 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { // If this isn't a call to one of the valid functions, it sure isn't a sanitizing function. if ( false === $functionPtr ) { if ( true === $require_unslash ) { - $this->add_unslash_error( $stackPtr ); + call_user_func( $unslash_callback, $this->phpcsFile, $stackPtr ); } return false; @@ -246,7 +256,7 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { // If slashing is required, give an error. if ( ! $is_unslashed && $require_unslash && ! $this->is_sanitizing_and_unslashing_function( $functionName ) ) { - $this->add_unslash_error( $stackPtr ); + call_user_func( $unslash_callback, $this->phpcsFile, $stackPtr ); } // Check if this is a sanitizing function. @@ -256,21 +266,4 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { return false; } - - /** - * Add an error for missing use of unslashing. - * - * @since 0.5.0 - * - * @param int $stackPtr The index of the token in the stack. - */ - public function add_unslash_error( $stackPtr ) { - - $this->phpcsFile->addError( - '%s data not unslashed before sanitization. Use wp_unslash() or similar', - $stackPtr, - 'MissingUnslash', - array( $this->tokens[ $stackPtr ]['content'] ) - ); - } } diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index 683abad29d..1bcb5060c2 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -9,6 +9,7 @@ namespace WordPressCS\WordPress\Sniffs\Security; +use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\TextStrings; use WordPressCS\WordPress\Helpers\ContextHelper; @@ -160,7 +161,7 @@ function ( $embed ) { } // Now look for sanitizing functions. - if ( ! $this->is_sanitized( $stackPtr, true ) ) { + if ( ! $this->is_sanitized( $stackPtr, array( $this, 'add_unslash_error' ) ) ) { $this->phpcsFile->addError( 'Detected usage of a non-sanitized input variable: %s', $stackPtr, @@ -169,4 +170,28 @@ function ( $embed ) { ); } } + + /** + * Add an error for missing use of unslashing. + * + * @since 0.5.0 + * @since 3.0.0 - Moved from the `Sniff` class to this class. + * - The `$phpcsFile` parameter was added. + * + * @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned. + * @param int $stackPtr The index of the token in the stack + * which is missing unslashing. + * + * @return void + */ + public function add_unslash_error( File $phpcsFile, $stackPtr ) { + $tokens = $phpcsFile->getTokens(); + + $phpcsFile->addError( + '%s data not unslashed before sanitization. Use wp_unslash() or similar', + $stackPtr, + 'MissingUnslash', + array( $tokens[ $stackPtr ]['content'] ) + ); + } }