From 21d8cae7344cc0095e69883f6b2ef89a66651b89 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Aug 2023 05:46:04 +0200 Subject: [PATCH 1/8] Rename `SanitizingFunctionsTrait` to `SanitizationHelperTrait` The `Sniff:;is_*sanitized()` utility methods are about to be moved to this trait, but once they have been, the trait name is no longer correct as all other `*Functions[Helper|Trait]` classes/traits _only_ deal with function lists and checking whether something is in those lists, while this trait will now do more than that. With that in mind, I propose to rename the trait to `SanitizationHelperTrait`. --- ...nitizingFunctionsTrait.php => SanitizationHelperTrait.php} | 2 +- WordPress/Sniff.php | 4 ++-- WordPress/Sniffs/Security/NonceVerificationSniff.php | 4 ++-- WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php | 4 ++-- WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) rename WordPress/Helpers/{SanitizingFunctionsTrait.php => SanitizationHelperTrait.php} (99%) diff --git a/WordPress/Helpers/SanitizingFunctionsTrait.php b/WordPress/Helpers/SanitizationHelperTrait.php similarity index 99% rename from WordPress/Helpers/SanitizingFunctionsTrait.php rename to WordPress/Helpers/SanitizationHelperTrait.php index 368a6fc5e1..222f642c6e 100644 --- a/WordPress/Helpers/SanitizingFunctionsTrait.php +++ b/WordPress/Helpers/SanitizationHelperTrait.php @@ -23,7 +23,7 @@ * `WordPressCS\WordPress\Sniff` class and partially in the `NonceVerificationSniff` * and the `ValidatedSanitizedInputSniff` classes and have been moved here. */ -trait SanitizingFunctionsTrait { +trait SanitizationHelperTrait { /** * Custom list of functions that sanitize the values passed to them. diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 7bd96029c7..e25591c794 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -15,7 +15,7 @@ use PHPCSUtils\Utils\TextStrings; use WordPressCS\WordPress\Helpers\ArrayWalkingFunctionsHelper; use WordPressCS\WordPress\Helpers\ContextHelper; -use WordPressCS\WordPress\Helpers\SanitizingFunctionsTrait; +use WordPressCS\WordPress\Helpers\SanitizationHelperTrait; use WordPressCS\WordPress\Helpers\UnslashingFunctionsHelper; /** @@ -27,7 +27,7 @@ */ abstract class Sniff implements PHPCS_Sniff { - use SanitizingFunctionsTrait; + use SanitizationHelperTrait; /** * A list of superglobals that incorporate user input. diff --git a/WordPress/Sniffs/Security/NonceVerificationSniff.php b/WordPress/Sniffs/Security/NonceVerificationSniff.php index 0909fc257b..a80d5d1767 100644 --- a/WordPress/Sniffs/Security/NonceVerificationSniff.php +++ b/WordPress/Sniffs/Security/NonceVerificationSniff.php @@ -31,8 +31,8 @@ * @since 1.0.0 This sniff has been moved from the `CSRF` category to the `Security` category. * @since 3.0.0 This sniff has received significant updates to its logic and structure. * - * @uses \WordPressCS\WordPress\Helpers\SanitizingFunctionsTrait::$customSanitizingFunctions - * @uses \WordPressCS\WordPress\Helpers\SanitizingFunctionsTrait::$customUnslashingSanitizingFunctions + * @uses \WordPressCS\WordPress\Helpers\SanitizationHelperTrait::$customSanitizingFunctions + * @uses \WordPressCS\WordPress\Helpers\SanitizationHelperTrait::$customUnslashingSanitizingFunctions */ class NonceVerificationSniff extends Sniff { diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index 1bcb5060c2..eff761e635 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -28,8 +28,8 @@ * @since 0.13.0 Class name changed: this class is now namespaced. * @since 1.0.0 This sniff has been moved from the `VIP` category to the `Security` category. * - * @uses \WordPressCS\WordPress\Helpers\SanitizingFunctionsTrait::$customSanitizingFunctions - * @uses \WordPressCS\WordPress\Helpers\SanitizingFunctionsTrait::$customUnslashingSanitizingFunctions + * @uses \WordPressCS\WordPress\Helpers\SanitizationHelperTrait::$customSanitizingFunctions + * @uses \WordPressCS\WordPress\Helpers\SanitizationHelperTrait::$customUnslashingSanitizingFunctions */ class ValidatedSanitizedInputSniff extends Sniff { diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php index 87e8776789..dd320c8a3f 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php @@ -19,7 +19,7 @@ * @since 1.0.0 This sniff has been moved from the `VIP` category to the `Security` category. * * @covers \WordPressCS\WordPress\Helpers\ArrayWalkingFunctionsHelper - * @covers \WordPressCS\WordPress\Helpers\SanitizingFunctionsTrait + * @covers \WordPressCS\WordPress\Helpers\SanitizationHelperTrait * @covers \WordPressCS\WordPress\Helpers\UnslashingFunctionsHelper * @covers \WordPressCS\WordPress\Helpers\ValidationHelper * @covers \WordPressCS\WordPress\Helpers\VariableHelper From 50e1fe9cf9404c359b56c26644458879052e370c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 6 Jul 2023 10:08:59 +0200 Subject: [PATCH 2/8] Move sanitization methods to `SanitizationHelperTrait` This moves the last utility functions from the `Sniff` class to a dedicated trait. I am also explicitly marking the trait as internal API to allow us to iterate on this (not so very clean) solution in future 3.x releases without being forced to wait for a 4.0 release. The methods have been made stand-alone, as in, the presumption that the WPCS `Sniff` class is being extended has been removed by adding the `$phpcsFile` parameter. Closes 1465 --- WordPress/Helpers/SanitizationHelperTrait.php | 186 ++++++++++++++++++ WordPress/Sniff.php | 169 +--------------- .../Security/NonceVerificationSniff.php | 5 +- .../Security/ValidatedSanitizedInputSniff.php | 5 +- 4 files changed, 195 insertions(+), 170 deletions(-) diff --git a/WordPress/Helpers/SanitizationHelperTrait.php b/WordPress/Helpers/SanitizationHelperTrait.php index 222f642c6e..ccbf23890a 100644 --- a/WordPress/Helpers/SanitizationHelperTrait.php +++ b/WordPress/Helpers/SanitizationHelperTrait.php @@ -9,7 +9,13 @@ namespace WordPressCS\WordPress\Helpers; +use PHP_CodeSniffer\Files\File; +use PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Utils\TextStrings; +use WordPressCS\WordPress\Helpers\ArrayWalkingFunctionsHelper; +use WordPressCS\WordPress\Helpers\ContextHelper; use WordPressCS\WordPress\Helpers\RulesetPropertyHelper; +use WordPressCS\WordPress\Helpers\UnslashingFunctionsHelper; /** * Helper functions and function lists for checking whether a sanitizing function is being used. @@ -19,9 +25,18 @@ * - `customSanitizingFunctions`. * - `customUnslashingSanitizingFunctions` * + * --------------------------------------------------------------------------------------------- + * This trait is only intended for internal use by WordPressCS and is not part of the public API. + * This also means that it has no promise of backward compatibility. Use at your own risk. + * --------------------------------------------------------------------------------------------- + * + * @internal + * * @since 3.0.0 The properties in this trait were previously contained partially in the * `WordPressCS\WordPress\Sniff` class and partially in the `NonceVerificationSniff` * and the `ValidatedSanitizedInputSniff` classes and have been moved here. + * The pre-existing methods in this trait were previously contained in the + * `WordPressCS\WordPress\Sniff` class and have been moved here. */ trait SanitizationHelperTrait { @@ -236,4 +251,175 @@ public function is_sanitizing_function( $functionName ) { public function is_sanitizing_and_unslashing_function( $functionName ) { return isset( $this->get_sanitizing_and_unslashing_functions()[ strtolower( $functionName ) ] ); } + + /** + * Check if something is only being sanitized. + * + * @since 0.5.0 + * @since 3.0.0 - Moved from the Sniff class to this class. + * - The method visibility was changed from `protected` to `public`. + * - 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. + * + * @return bool Whether the token is only within a sanitization. + */ + public function is_only_sanitized( File $phpcsFile, $stackPtr ) { + $tokens = $phpcsFile->getTokens(); + + // If it isn't being sanitized at all. + if ( ! $this->is_sanitized( $phpcsFile, $stackPtr ) ) { + return false; + } + + // If the token isn't in parentheses, we know the value must have only been casted, because + // is_sanitized() would have returned `false` otherwise. + if ( ! isset( $tokens[ $stackPtr ]['nested_parenthesis'] ) ) { + return true; + } + + // At this point we're expecting the value to have not been casted. If it + // was, it wasn't *only* casted, because it's also in a function. + if ( ContextHelper::is_safe_casted( $phpcsFile, $stackPtr ) ) { + return false; + } + + // The only parentheses should belong to the sanitizing function. If there's + // more than one set, this isn't *only* sanitization. + return ( \count( $tokens[ $stackPtr ]['nested_parenthesis'] ) === 1 ); + } + + /** + * Check if something is being sanitized. + * + * @since 0.5.0 + * @since 3.0.0 - Moved from the Sniff class to this class. + * - The method visibility was changed from `protected` to `public`. + * - The `$phpcsFile` parameter was added. + * - The $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 \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned. + * @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. + */ + public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = null ) { + $tokens = $phpcsFile->getTokens(); + $require_unslash = is_callable( $unslash_callback ); + + // First we check if it is being casted to a safe value. + if ( ContextHelper::is_safe_casted( $phpcsFile, $stackPtr ) ) { + return true; + } + + // If this isn't within a function call, we know already that it's not safe. + if ( ! isset( $tokens[ $stackPtr ]['nested_parenthesis'] ) ) { + if ( $require_unslash ) { + call_user_func( $unslash_callback, $phpcsFile, $stackPtr ); + } + + return false; + } + + // Get the function that it's in. + $nested_parenthesis = $tokens[ $stackPtr ]['nested_parenthesis']; + $nested_openers = array_keys( $nested_parenthesis ); + $function_opener = array_pop( $nested_openers ); + $functionPtr = $phpcsFile->findPrevious( Tokens::$emptyTokens, ( $function_opener - 1 ), null, true, null, true ); + + // If it is just being unset, the value isn't used at all, so it's safe. + if ( \T_UNSET === $tokens[ $functionPtr ]['code'] ) { + return true; + } + + $valid_functions = $this->get_sanitizing_functions(); + $valid_functions += $this->get_sanitizing_and_unslashing_functions(); + $valid_functions += UnslashingFunctionsHelper::get_functions(); + $valid_functions += ArrayWalkingFunctionsHelper::get_functions(); + + $functionPtr = ContextHelper::is_in_function_call( $phpcsFile, $stackPtr, $valid_functions ); + + // 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 ) { + call_user_func( $unslash_callback, $phpcsFile, $stackPtr ); + } + + return false; + } + + $functionName = $tokens[ $functionPtr ]['content']; + + // Check if an unslashing function is being used. + if ( UnslashingFunctionsHelper::is_unslashing_function( $functionName ) ) { + + $is_unslashed = true; + + // Remove the unslashing functions. + $valid_functions = array_diff_key( $valid_functions, UnslashingFunctionsHelper::get_functions() ); + + // Check is any of the remaining (sanitizing) functions is used. + $higherFunctionPtr = ContextHelper::is_in_function_call( $phpcsFile, $functionPtr, $valid_functions ); + + // If there is no other valid function being used, this value is unsanitized. + if ( false === $higherFunctionPtr ) { + return false; + } + + $functionPtr = $higherFunctionPtr; + $functionName = $tokens[ $functionPtr ]['content']; + + } else { + $is_unslashed = false; + } + + // Arrays might be sanitized via an array walking function using a callback. + if ( ArrayWalkingFunctionsHelper::is_array_walking_function( $functionName ) ) { + + // Get the callback parameter. + $callback = ArrayWalkingFunctionsHelper::get_callback_parameter( $phpcsFile, $functionPtr ); + + if ( ! empty( $callback ) ) { + /* + * If this is a function callback (not a method callback array) and we're able + * to resolve the function name, do so. + */ + $first_non_empty = $phpcsFile->findNext( + Tokens::$emptyTokens, + $callback['start'], + ( $callback['end'] + 1 ), + true + ); + + if ( false !== $first_non_empty && \T_CONSTANT_ENCAPSED_STRING === $tokens[ $first_non_empty ]['code'] ) { + $functionName = TextStrings::stripQuotes( $tokens[ $first_non_empty ]['content'] ); + } + } + } + + // If slashing is required, give an error. + if ( ! $is_unslashed && $require_unslash && ! $this->is_sanitizing_and_unslashing_function( $functionName ) ) { + call_user_func( $unslash_callback, $phpcsFile, $stackPtr ); + } + + // Check if this is a sanitizing function. + if ( $this->is_sanitizing_function( $functionName ) || $this->is_sanitizing_and_unslashing_function( $functionName ) ) { + return true; + } + + return false; + } } diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index e25591c794..8b3ad0507d 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -9,14 +9,8 @@ namespace WordPressCS\WordPress; -use PHP_CodeSniffer\Sniffs\Sniff as PHPCS_Sniff; use PHP_CodeSniffer\Files\File; -use PHP_CodeSniffer\Util\Tokens; -use PHPCSUtils\Utils\TextStrings; -use WordPressCS\WordPress\Helpers\ArrayWalkingFunctionsHelper; -use WordPressCS\WordPress\Helpers\ContextHelper; -use WordPressCS\WordPress\Helpers\SanitizationHelperTrait; -use WordPressCS\WordPress\Helpers\UnslashingFunctionsHelper; +use PHP_CodeSniffer\Sniffs\Sniff as PHPCS_Sniff; /** * Represents a PHP_CodeSniffer sniff for sniffing WordPress coding standards. @@ -27,8 +21,6 @@ */ abstract class Sniff implements PHPCS_Sniff { - use SanitizationHelperTrait; - /** * A list of superglobals that incorporate user input. * @@ -107,163 +99,4 @@ protected function init( File $phpcsFile ) { $this->phpcsFile = $phpcsFile; $this->tokens = $phpcsFile->getTokens(); } - - /** - * Check if something is only being sanitized. - * - * @since 0.5.0 - * - * @param int $stackPtr The index of the token in the stack. - * - * @return bool Whether the token is only within a sanitization. - */ - protected function is_only_sanitized( $stackPtr ) { - - // If it isn't being sanitized at all. - if ( ! $this->is_sanitized( $stackPtr ) ) { - return false; - } - - // If this isn't set, we know the value must have only been casted, because - // is_sanitized() would have returned false otherwise. - if ( ! isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { - return true; - } - - // At this point we're expecting the value to have not been casted. If it - // was, it wasn't *only* casted, because it's also in a function. - if ( ContextHelper::is_safe_casted( $this->phpcsFile, $stackPtr ) ) { - return false; - } - - // The only parentheses should belong to the sanitizing function. If there's - // more than one set, this isn't *only* sanitization. - return ( \count( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) === 1 ); - } - - /** - * 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 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, $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 ) ) { - return true; - } - - // 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 ) { - call_user_func( $unslash_callback, $this->phpcsFile, $stackPtr ); - } - - return false; - } - - // Get the function that it's in. - $nested_parenthesis = $this->tokens[ $stackPtr ]['nested_parenthesis']; - $nested_openers = array_keys( $nested_parenthesis ); - $function_opener = array_pop( $nested_openers ); - $functionPtr = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $function_opener - 1 ), null, true, null, true ); - - // If it is just being unset, the value isn't used at all, so it's safe. - if ( \T_UNSET === $this->tokens[ $functionPtr ]['code'] ) { - return true; - } - - $valid_functions = $this->get_sanitizing_functions(); - $valid_functions += $this->get_sanitizing_and_unslashing_functions(); - $valid_functions += UnslashingFunctionsHelper::get_functions(); - $valid_functions += ArrayWalkingFunctionsHelper::get_functions(); - - $functionPtr = ContextHelper::is_in_function_call( $this->phpcsFile, $stackPtr, $valid_functions ); - - // 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 ) { - call_user_func( $unslash_callback, $this->phpcsFile, $stackPtr ); - } - - return false; - } - - $functionName = $this->tokens[ $functionPtr ]['content']; - - // Check if an unslashing function is being used. - if ( UnslashingFunctionsHelper::is_unslashing_function( $functionName ) ) { - - $is_unslashed = true; - - // Remove the unslashing functions. - $valid_functions = array_diff_key( $valid_functions, UnslashingFunctionsHelper::get_functions() ); - - // Check is any of the remaining (sanitizing) functions is used. - $higherFunctionPtr = ContextHelper::is_in_function_call( $this->phpcsFile, $functionPtr, $valid_functions ); - - // If there is no other valid function being used, this value is unsanitized. - if ( false === $higherFunctionPtr ) { - return false; - } - - $functionPtr = $higherFunctionPtr; - $functionName = $this->tokens[ $functionPtr ]['content']; - - } else { - $is_unslashed = false; - } - - // Arrays might be sanitized via an array walking function using a callback. - if ( ArrayWalkingFunctionsHelper::is_array_walking_function( $functionName ) ) { - - // Get the callback parameter. - $callback = ArrayWalkingFunctionsHelper::get_callback_parameter( $this->phpcsFile, $functionPtr ); - - if ( ! empty( $callback ) ) { - /* - * If this is a function callback (not a method callback array) and we're able - * to resolve the function name, do so. - */ - $first_non_empty = $this->phpcsFile->findNext( - Tokens::$emptyTokens, - $callback['start'], - ( $callback['end'] + 1 ), - true - ); - - if ( false !== $first_non_empty && \T_CONSTANT_ENCAPSED_STRING === $this->tokens[ $first_non_empty ]['code'] ) { - $functionName = TextStrings::stripQuotes( $this->tokens[ $first_non_empty ]['content'] ); - } - } - } - - // If slashing is required, give an error. - if ( ! $is_unslashed && $require_unslash && ! $this->is_sanitizing_and_unslashing_function( $functionName ) ) { - call_user_func( $unslash_callback, $this->phpcsFile, $stackPtr ); - } - - // Check if this is a sanitizing function. - if ( $this->is_sanitizing_function( $functionName ) || $this->is_sanitizing_and_unslashing_function( $functionName ) ) { - return true; - } - - return false; - } } diff --git a/WordPress/Sniffs/Security/NonceVerificationSniff.php b/WordPress/Sniffs/Security/NonceVerificationSniff.php index a80d5d1767..0241147327 100644 --- a/WordPress/Sniffs/Security/NonceVerificationSniff.php +++ b/WordPress/Sniffs/Security/NonceVerificationSniff.php @@ -17,6 +17,7 @@ use PHPCSUtils\Utils\Scopes; use WordPressCS\WordPress\Helpers\ContextHelper; use WordPressCS\WordPress\Helpers\RulesetPropertyHelper; +use WordPressCS\WordPress\Helpers\SanitizationHelperTrait; use WordPressCS\WordPress\Helpers\UnslashingFunctionsHelper; use WordPressCS\WordPress\Helpers\VariableHelper; use WordPressCS\WordPress\Sniff; @@ -36,6 +37,8 @@ */ class NonceVerificationSniff extends Sniff { + use SanitizationHelperTrait; + /** * Superglobals to notify about when not accompanied by an nonce check. * @@ -234,7 +237,7 @@ protected function needs_nonce_check( $stackPtr, array $cache_keys ) { || VariableHelper::is_assignment( $this->phpcsFile, $stackPtr, true ) || ContextHelper::is_in_array_comparison( $this->phpcsFile, $stackPtr ) || ContextHelper::is_in_function_call( $this->phpcsFile, $stackPtr, UnslashingFunctionsHelper::get_functions() ) !== false - || $this->is_only_sanitized( $stackPtr ) + || $this->is_only_sanitized( $this->phpcsFile, $stackPtr ) ) { $needs_nonce = 'after'; } diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index eff761e635..cbbcab1059 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -13,6 +13,7 @@ use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\TextStrings; use WordPressCS\WordPress\Helpers\ContextHelper; +use WordPressCS\WordPress\Helpers\SanitizationHelperTrait; use WordPressCS\WordPress\Helpers\ValidationHelper; use WordPressCS\WordPress\Helpers\VariableHelper; use WordPressCS\WordPress\Sniff; @@ -33,6 +34,8 @@ */ class ValidatedSanitizedInputSniff extends Sniff { + use SanitizationHelperTrait; + /** * Check for validation functions for a variable within its own parenthesis only. * @@ -161,7 +164,7 @@ function ( $embed ) { } // Now look for sanitizing functions. - if ( ! $this->is_sanitized( $stackPtr, array( $this, 'add_unslash_error' ) ) ) { + if ( ! $this->is_sanitized( $this->phpcsFile, $stackPtr, array( $this, 'add_unslash_error' ) ) ) { $this->phpcsFile->addError( 'Detected usage of a non-sanitized input variable: %s', $stackPtr, From e72ca2cf86508c9a68afa4ecd5a73687384fc8dc Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Aug 2023 06:26:28 +0200 Subject: [PATCH 3/8] SanitizationHelperTrait::is_sanitized(): add some defensive coding As this method is now stand-alone, we'd better make sure the token being examined exists. Note: no need to do the same for the `is_only_sanitized()` method as the first thing that method calls is this method and if this method returns `false`, the `is_only_sanitized()` method will too. --- WordPress/Helpers/SanitizationHelperTrait.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/WordPress/Helpers/SanitizationHelperTrait.php b/WordPress/Helpers/SanitizationHelperTrait.php index ccbf23890a..14631e3eaa 100644 --- a/WordPress/Helpers/SanitizationHelperTrait.php +++ b/WordPress/Helpers/SanitizationHelperTrait.php @@ -320,6 +320,10 @@ public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = nu $tokens = $phpcsFile->getTokens(); $require_unslash = is_callable( $unslash_callback ); + if ( isset( $tokens[ $stackPtr ] ) === false ) { + return false; + } + // First we check if it is being casted to a safe value. if ( ContextHelper::is_safe_casted( $phpcsFile, $stackPtr ) ) { return true; From b217ebc3b3477d5ac2e99b96ab79502398d7bc22 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 6 Jul 2023 10:26:25 +0200 Subject: [PATCH 4/8] SanitizationHelperTrait::is_sanitized(): implement PHPCSUtils Note: The `$functionPtr` variable is overwritten a few lines later and the other variables being set are effectively unused (other than in these lines), so this code can be safely removed/replaced. --- WordPress/Helpers/SanitizationHelperTrait.php | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/WordPress/Helpers/SanitizationHelperTrait.php b/WordPress/Helpers/SanitizationHelperTrait.php index 14631e3eaa..760e63c7b0 100644 --- a/WordPress/Helpers/SanitizationHelperTrait.php +++ b/WordPress/Helpers/SanitizationHelperTrait.php @@ -11,6 +11,7 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Utils\Context; use PHPCSUtils\Utils\TextStrings; use WordPressCS\WordPress\Helpers\ArrayWalkingFunctionsHelper; use WordPressCS\WordPress\Helpers\ContextHelper; @@ -338,14 +339,8 @@ public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = nu return false; } - // Get the function that it's in. - $nested_parenthesis = $tokens[ $stackPtr ]['nested_parenthesis']; - $nested_openers = array_keys( $nested_parenthesis ); - $function_opener = array_pop( $nested_openers ); - $functionPtr = $phpcsFile->findPrevious( Tokens::$emptyTokens, ( $function_opener - 1 ), null, true, null, true ); - // If it is just being unset, the value isn't used at all, so it's safe. - if ( \T_UNSET === $tokens[ $functionPtr ]['code'] ) { + if ( Context::inUnset( $phpcsFile, $stackPtr ) ) { return true; } From 0834f59e58ef96c0ae5198e3001d1c53c47ccce9 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Aug 2023 06:11:47 +0200 Subject: [PATCH 5/8] SanitizationHelperTrait::is_sanitized(): efficiency tweak - move unset check up A variable which is being unset, doesn't need to be unslashed or sanitized, so bow out early. --- WordPress/Helpers/SanitizationHelperTrait.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/WordPress/Helpers/SanitizationHelperTrait.php b/WordPress/Helpers/SanitizationHelperTrait.php index 760e63c7b0..6aa28b6d9a 100644 --- a/WordPress/Helpers/SanitizationHelperTrait.php +++ b/WordPress/Helpers/SanitizationHelperTrait.php @@ -325,6 +325,11 @@ public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = nu return false; } + // If the variable is just being unset, the value isn't used at all, so it's safe. + if ( Context::inUnset( $phpcsFile, $stackPtr ) ) { + return true; + } + // First we check if it is being casted to a safe value. if ( ContextHelper::is_safe_casted( $phpcsFile, $stackPtr ) ) { return true; @@ -339,11 +344,6 @@ public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = nu return false; } - // If it is just being unset, the value isn't used at all, so it's safe. - if ( Context::inUnset( $phpcsFile, $stackPtr ) ) { - return true; - } - $valid_functions = $this->get_sanitizing_functions(); $valid_functions += $this->get_sanitizing_and_unslashing_functions(); $valid_functions += UnslashingFunctionsHelper::get_functions(); From 1bf115b51f109c60fd331b95b632a31393a968dd Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Aug 2023 06:13:01 +0200 Subject: [PATCH 6/8] SanitizationHelperTrait::is_sanitized(): efficiency fix/improve performance Remove the need for a call to the performance negative `array_diff_key()` function by setting up the original arrays in a better way. --- WordPress/Helpers/SanitizationHelperTrait.php | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/WordPress/Helpers/SanitizationHelperTrait.php b/WordPress/Helpers/SanitizationHelperTrait.php index 6aa28b6d9a..00fa571908 100644 --- a/WordPress/Helpers/SanitizationHelperTrait.php +++ b/WordPress/Helpers/SanitizationHelperTrait.php @@ -344,11 +344,12 @@ public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = nu return false; } - $valid_functions = $this->get_sanitizing_functions(); - $valid_functions += $this->get_sanitizing_and_unslashing_functions(); - $valid_functions += UnslashingFunctionsHelper::get_functions(); - $valid_functions += ArrayWalkingFunctionsHelper::get_functions(); + $sanitizing_functions = $this->get_sanitizing_functions(); + $sanitizing_functions += $this->get_sanitizing_and_unslashing_functions(); + $sanitizing_functions += ArrayWalkingFunctionsHelper::get_functions(); + $valid_functions = $sanitizing_functions + UnslashingFunctionsHelper::get_functions(); + // Get the function that it's in. $functionPtr = ContextHelper::is_in_function_call( $phpcsFile, $stackPtr, $valid_functions ); // If this isn't a call to one of the valid functions, it sure isn't a sanitizing function. @@ -367,11 +368,8 @@ public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = nu $is_unslashed = true; - // Remove the unslashing functions. - $valid_functions = array_diff_key( $valid_functions, UnslashingFunctionsHelper::get_functions() ); - // Check is any of the remaining (sanitizing) functions is used. - $higherFunctionPtr = ContextHelper::is_in_function_call( $phpcsFile, $functionPtr, $valid_functions ); + $higherFunctionPtr = ContextHelper::is_in_function_call( $phpcsFile, $functionPtr, $sanitizing_functions ); // If there is no other valid function being used, this value is unsanitized. if ( false === $higherFunctionPtr ) { From 6440bed1a8fe2b936e5fee64737c750c2a9c674a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Aug 2023 06:56:13 +0200 Subject: [PATCH 7/8] SanitizationHelperTrait::is_sanitized(): minor code tweaks * Get rid of an unnecessary `else`. * Improve readability of long condition. * Get rid of unnecessary true/false condition code. --- WordPress/Helpers/SanitizationHelperTrait.php | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/WordPress/Helpers/SanitizationHelperTrait.php b/WordPress/Helpers/SanitizationHelperTrait.php index 00fa571908..1d63f603c7 100644 --- a/WordPress/Helpers/SanitizationHelperTrait.php +++ b/WordPress/Helpers/SanitizationHelperTrait.php @@ -364,8 +364,8 @@ public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = nu $functionName = $tokens[ $functionPtr ]['content']; // Check if an unslashing function is being used. + $is_unslashed = false; if ( UnslashingFunctionsHelper::is_unslashing_function( $functionName ) ) { - $is_unslashed = true; // Check is any of the remaining (sanitizing) functions is used. @@ -378,14 +378,10 @@ public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = nu $functionPtr = $higherFunctionPtr; $functionName = $tokens[ $functionPtr ]['content']; - - } else { - $is_unslashed = false; } // Arrays might be sanitized via an array walking function using a callback. if ( ArrayWalkingFunctionsHelper::is_array_walking_function( $functionName ) ) { - // Get the callback parameter. $callback = ArrayWalkingFunctionsHelper::get_callback_parameter( $phpcsFile, $functionPtr ); @@ -408,15 +404,14 @@ public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = nu } // If slashing is required, give an error. - if ( ! $is_unslashed && $require_unslash && ! $this->is_sanitizing_and_unslashing_function( $functionName ) ) { + if ( false === $is_unslashed + && true === $require_unslash + && ! $this->is_sanitizing_and_unslashing_function( $functionName ) + ) { call_user_func( $unslash_callback, $phpcsFile, $stackPtr ); } // Check if this is a sanitizing function. - if ( $this->is_sanitizing_function( $functionName ) || $this->is_sanitizing_and_unslashing_function( $functionName ) ) { - return true; - } - - return false; + return ( $this->is_sanitizing_function( $functionName ) || $this->is_sanitizing_and_unslashing_function( $functionName ) ); } } From 2d019df783e077b811f9af506f1ae16a717de6b4 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 18 Aug 2023 06:25:40 +0200 Subject: [PATCH 8/8] SanitizationHelperTrait::is_sanitized(): minor doc tweaks --- WordPress/Helpers/SanitizationHelperTrait.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Helpers/SanitizationHelperTrait.php b/WordPress/Helpers/SanitizationHelperTrait.php index 1d63f603c7..9760eca67c 100644 --- a/WordPress/Helpers/SanitizationHelperTrait.php +++ b/WordPress/Helpers/SanitizationHelperTrait.php @@ -315,7 +315,7 @@ public function is_only_sanitized( File $phpcsFile, $stackPtr ) { * check for unslashing issues. * Defaults to `null` (skip unslashing checks). * - * @return bool Whether the token being sanitized. + * @return bool Whether the token is being sanitized. */ public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = null ) { $tokens = $phpcsFile->getTokens(); @@ -368,7 +368,7 @@ public function is_sanitized( File $phpcsFile, $stackPtr, $unslash_callback = nu if ( UnslashingFunctionsHelper::is_unslashing_function( $functionName ) ) { $is_unslashed = true; - // Check is any of the remaining (sanitizing) functions is used. + // Check whether this function call is wrapped within a sanitizing function. $higherFunctionPtr = ContextHelper::is_in_function_call( $phpcsFile, $functionPtr, $sanitizing_functions ); // If there is no other valid function being used, this value is unsanitized.