diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index b969343553..546de8c400 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -446,6 +446,218 @@ protected function is_sanitized( $stackPtr ) { // Check if this is a sanitizing function. return in_array( $functionName, WordPress_Sniffs_XSS_EscapeOutputSniff::$sanitizingFunctions ); } + + /** + * Get the index key of an array variable. + * + * E.g., "bar" in $foo['bar']. + * + * @since 0.5.0 + * + * @param int $stackPtr The index of the token in the stack. + * + * @return string|false The array index key whose value is being accessed. + */ + protected function get_array_access_key( $stackPtr ) { + + // Find the next non-empty token. + $open_bracket = $this->phpcsFile->findNext( + PHP_CodeSniffer_Tokens::$emptyTokens, + $stackPtr + 1, + null, + true + ); + + // If it isn't a bracket, this isn't an array-access. + if ( T_OPEN_SQUARE_BRACKET !== $this->tokens[ $open_bracket ]['code'] ) { + return false; + } + + $key = $this->phpcsFile->getTokensAsString( + $open_bracket + 1 + , $this->tokens[ $open_bracket ]['bracket_closer'] - $open_bracket - 1 + ); + + return trim( $key ); + } + + /** + * Check if the existence of a variable is validated with isset() or empty(). + * + * When $in_condition_only is false, (which is the default), this is considered + * valid: + * + * ```php + * if ( isset( $var ) ) { + * // Do stuff, like maybe return or exit (but could be anything) + * } + * + * foo( $var ); + * ``` + * + * When it is true, that would be invalid, the use of the variable must be within + * the scope of the validating condition, like this: + * + * ```php + * if ( isset( $var ) ) { + * foo( $var ); + * } + * ``` + * + * @since 0.5.0 + * + * @param int $stackPtr The index of this token in the stack. + * @param string $array_key An array key to check for ("bar" in $foo['bar']). + * @param bool $in_condition_only Whether to require that this use of the + * variable occur within the scope of the + * validating condition, or just in the same + * scope as it (default). + * + * @return bool Whether the var is validated. + */ + protected function is_validated( $stackPtr, $array_key = null, $in_condition_only = false ) { + + if ( $in_condition_only ) { + + // This is a stricter check, requiring the variable to be used only + // within the validation condition. + + // If there are no conditions, there's no validation. + if ( empty( $this->tokens[ $stackPtr ]['conditions'] ) ) { + return false; + } + + $conditions = $this->tokens[ $stackPtr ]['conditions']; + end( $conditions ); // Get closest condition + $conditionPtr = key( $conditions ); + $condition = $this->tokens[ $conditionPtr ]; + + if ( ! isset( $condition['parenthesis_opener'] ) ) { + + $this->phpcsFile->addError( + 'Possible parse error, condition missing open parenthesis.', + $conditionPtr, + 'IsValidatedMissingConditionOpener' + ); + + return false; + } + + $scope_start = $condition['parenthesis_opener']; + $scope_end = $condition['parenthesis_closer']; + + } else { + + // We are are more loose, requiring only that the variable be validated + // in the same function/file scope as it is used. + + // Check if we are in a function. + $function = $this->phpcsFile->findPrevious( T_FUNCTION, $stackPtr ); + + // If so, we check only within the function, otherwise the whole file. + if ( false !== $function && $stackPtr < $this->tokens[ $function ]['scope_closer'] ) { + $scope_start = $this->tokens[ $function ]['scope_opener']; + } else { + $scope_start = 0; + } + + $scope_end = $stackPtr; + } + + for ( $i = $scope_start + 1; $i < $scope_end; $i++ ) { + + if ( ! in_array( $this->tokens[ $i ]['code'], array( T_ISSET, T_EMPTY, T_UNSET ) ) ) { + continue; + } + + $issetOpener = $this->phpcsFile->findNext( T_OPEN_PARENTHESIS, $i ); + $issetCloser = $this->tokens[ $issetOpener ]['parenthesis_closer']; + + // Look for this variable. We purposely stomp $i from the parent loop. + for ( $i = $issetOpener + 1; $i < $issetCloser; $i++ ) { + + if ( T_VARIABLE !== $this->tokens[ $i ]['code'] ) { + continue; + } + + // If we're checking for a specific array key (ex: 'hello' in + // $_POST['hello']), that mush match too. + if ( $array_key && $this->get_array_access_key( $i ) !== $array_key ) { + continue; + } + + return true; + } + } + + return false; + } + + /** + * Check whether a variable is being compared to another value. + * + * E.g., $var === 'foo', 1 <= $var, etc. + * + * Also recognizes `switch ( $var )`. + * + * @since 0.5.0 + * + * @param int $stackPtr The index of this token in the stack. + * + * @return bool Whether this is a comparison. + */ + protected function is_comparison( $stackPtr ) { + + // We first check if this is a switch statement (switch ( $var )). + if ( isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { + $close_parenthesis = end( $this->tokens[ $stackPtr ]['nested_parenthesis'] ); + + if ( + isset( $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ) + && T_SWITCH === $this->tokens[ $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ]['code'] + ) { + return true; + } + } + + // Find the previous non-empty token. We check before the var first because + // yoda conditions are usually expected. + $previous_token = $this->phpcsFile->findPrevious( + PHP_CodeSniffer_Tokens::$emptyTokens, + $stackPtr - 1, + null, + true + ); + + if ( in_array( $this->tokens[ $previous_token ]['code'], PHP_CodeSniffer_Tokens::$comparisonTokens ) ) { + return true; + } + + // Maybe the comparison operator is after this. + $next_token = $this->phpcsFile->findNext( + PHP_CodeSniffer_Tokens::$emptyTokens, + $stackPtr + 1, + null, + true + ); + + // This might be an opening square bracket in the case of arrays ($var['a']). + while ( T_OPEN_SQUARE_BRACKET === $this->tokens[ $next_token ]['code'] ) { + + $next_token = $this->phpcsFile->findNext( + PHP_CodeSniffer_Tokens::$emptyTokens, + $this->tokens[ $next_token ]['bracket_closer'] + 1, + null, + true + ); + } + + if ( in_array( $this->tokens[ $next_token ]['code'], PHP_CodeSniffer_Tokens::$comparisonTokens ) ) { + return true; + } + + return false; + } } // EOF diff --git a/WordPress/Sniffs/VIP/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/VIP/ValidatedSanitizedInputSniff.php index 94a223fb36..719551b57c 100644 --- a/WordPress/Sniffs/VIP/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/VIP/ValidatedSanitizedInputSniff.php @@ -1,4 +1,5 @@ * @link https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/69 */ -class WordPress_Sniffs_VIP_ValidatedSanitizedInputSniff extends WordPress_Sniff -{ +class WordPress_Sniffs_VIP_ValidatedSanitizedInputSniff extends WordPress_Sniff { /** * Check for validation functions for a variable within its own parenthesis only @@ -23,15 +23,12 @@ class WordPress_Sniffs_VIP_ValidatedSanitizedInputSniff extends WordPress_Sniff * * @return array */ - public function register() - { + public function register() { return array( - T_VARIABLE, - T_DOUBLE_QUOTED_STRING, - ); - - }//end register() - + T_VARIABLE, + T_DOUBLE_QUOTED_STRING, + ); + } /** * Processes this test, when one of its tokens is encountered. @@ -42,17 +39,17 @@ public function register() * * @return void */ - public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) - { + public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) { + $this->init( $phpcsFile ); $tokens = $phpcsFile->getTokens(); $superglobals = WordPress_Sniff::$input_superglobals; // Handling string interpolation - if ( $tokens[ $stackPtr ]['code'] === T_DOUBLE_QUOTED_STRING ) { + if ( T_DOUBLE_QUOTED_STRING === $tokens[ $stackPtr ]['code'] ) { foreach ( $superglobals as $superglobal ) { if ( false !== strpos( $tokens[ $stackPtr ]['content'], $superglobal ) ) { - $phpcsFile->addError( 'Detected usage of a non-sanitized, non-validated input variable: %s', $stackPtr, null, array( $tokens[$stackPtr]['content'] ) ); + $phpcsFile->addError( 'Detected usage of a non-sanitized, non-validated input variable: %s', $stackPtr, null, array( $tokens[ $stackPtr ]['content'] ) ); return; } } @@ -61,106 +58,29 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) } // Check if this is a superglobal. - if ( ! in_array( $tokens[$stackPtr]['content'], $superglobals ) ) + if ( ! in_array( $tokens[ $stackPtr ]['content'], $superglobals ) ) { return; - - $instance = $tokens[$stackPtr]; - $varName = $instance['content']; + } // If we're overriding a superglobal with an assignment, no need to test if ( $this->is_assignment( $stackPtr ) ) { return; } - // Search for casting - $prev = $phpcsFile->findPrevious( array( T_WHITESPACE ), $stackPtr - 1, null, true, null, true ); - $is_casted = in_array( $tokens[ $prev ]['code'], array( T_INT_CAST, T_DOUBLE_CAST, T_BOOL_CAST ) ); - - if ( isset( $instance['nested_parenthesis'] ) ) { - $nested = $instance['nested_parenthesis']; - // Ignore if wrapped inside ISSET - end( $nested ); // Get closest parenthesis - if ( in_array( $tokens[ key( $nested ) - 1 ]['code'], array( T_ISSET, T_EMPTY, T_UNSET ) ) ) - return; - } else { - if ( $this->has_whitelist_comment( 'sanitization', $stackPtr ) ) { - return; - } - - // Search for casting - $prev = $phpcsFile->findPrevious( array( T_WHITESPACE ), $stackPtr -1, null, true, null, true ); - $is_casted = in_array( $tokens[ $prev ]['code'], array( T_INT_CAST, T_DOUBLE_CAST, T_BOOL_CAST ) ); - if ( ! $is_casted ) { - $phpcsFile->addError( 'Detected usage of a non-sanitized input variable: %s', $stackPtr, 'InputNotSanitized', array( $tokens[$stackPtr]['content'] ) ); - return; - } - } - - - $varKey = $this->getArrayIndexKey( $phpcsFile, $tokens, $stackPtr ); - - if ( empty( $varKey ) ) { + // This superglobal is being validated. + if ( $this->is_in_isset_or_empty( $stackPtr ) ) { return; } - // Check for validation first - $is_validated = false; - - // Validation check in inner scope ? - if ( $this->check_validation_in_scope_only ) { - // Wrapped in a condition? check existence of isset with the variable as an argument - if ( ! empty( $tokens[$stackPtr]['conditions'] ) ) { - $conditions = $tokens[$stackPtr]['conditions']; - end( $conditions ); // Get closest condition - $conditionPtr = key( $conditions ); - $condition = $tokens[$conditionPtr]; - - if ( isset( $condition['parenthesis_opener'] ) ) { - $scope_start = $condition['parenthesis_opener']; - $scope_end = $condition['parenthesis_closer']; - } - } - } else { - // Get outer scope - $function = $phpcsFile->findPrevious( T_FUNCTION, $stackPtr ); - if ( $function !== false && $stackPtr < $tokens[$function]['scope_closer'] ) { - $scope_start = $tokens[$function]['scope_opener']; - $scope_end = $stackPtr; - } else { // In the open air, check whole file - $scope_start = 0; - $scope_end = $stackPtr; - } - } + $array_key = $this->get_array_access_key( $stackPtr ); - for ( $i = $scope_start + 1; $i < $scope_end; $i++ ) { - if ( ! in_array( $tokens[$i]['code'], array( T_ISSET, T_EMPTY, T_UNSET ) ) ) { - continue; - } - $issetPtr = $i; - if ( ! empty( $issetPtr ) ) { - $isset = $tokens[$issetPtr]; - $issetOpener = $phpcsFile->findNext( T_OPEN_PARENTHESIS, $issetPtr ); - $issetCloser = $tokens[$issetOpener]['parenthesis_closer']; - - // Check that it is the same variable name - for ( $i = $issetOpener + 1; $i < $issetCloser; $i++ ) { - if ( $tokens[ $i ]['code'] != T_VARIABLE ) { - continue; - } - - // Double check the $varKey inside the variable, ex: 'hello' in $_POST['hello'] - $varKeyValidated = $this->getArrayIndexKey( $phpcsFile, $tokens, $i ); - - if ( $varKeyValidated == $varKey ) { - // everything matches, variable IS validated afterall .. - $is_validated = true; - } - } - } + if ( empty( $array_key ) ) { + return; } - if ( ! $is_validated ) { - $phpcsFile->addError( 'Detected usage of a non-validated input variable: %s', $stackPtr, 'InputNotValidated', array( $tokens[$stackPtr]['content'] ) ); + // Check for validation first. + if ( ! $this->is_validated( $stackPtr, $array_key, $this->check_validation_in_scope_only ) ) { + $phpcsFile->addError( 'Detected usage of a non-validated input variable: %s', $stackPtr, 'InputNotValidated', array( $tokens[ $stackPtr ]['content'] ) ); // return; // Should we just return and not look for sanitizing functions ? } @@ -168,69 +88,18 @@ public function process( PHP_CodeSniffer_File $phpcsFile, $stackPtr ) return; } - // Now look for sanitizing functions - $is_sanitized = false; - - if ( isset( $is_casted ) && $is_casted ) { - $is_sanitized = true; - } elseif ( isset( $nested ) ) { - $functionPtr = key( $nested ) - 1; - $function = $tokens[$functionPtr]; - if ( T_STRING === $function['code'] ) { - $functionName = $function['content']; - - if ( 'array_map' === $functionName ) { - $function_opener = $phpcsFile->findNext( T_OPEN_PARENTHESIS, $functionPtr + 1 ); - $mapped_function = $phpcsFile->findNext( PHP_CodeSniffer_Tokens::$emptyTokens, $function_opener + 1, $function_opener['parenthesis_closer'], true ); - - if ( $mapped_function && T_CONSTANT_ENCAPSED_STRING === $tokens[ $mapped_function ]['code'] ) { - $functionName = trim( $tokens[ $mapped_function ]['content'], '\'' ); - } - } - - if ( - in_array( $functionName, WordPress_Sniffs_XSS_EscapeOutputSniff::$autoEscapedFunctions ) - || - in_array( $functionName, WordPress_Sniffs_XSS_EscapeOutputSniff::$sanitizingFunctions ) - ) { - $is_sanitized = true; - } - } elseif ( T_UNSET === $function['code'] ) { - $is_sanitized = true; - } elseif ( $is_casted ) { - $is_sanitized = true; - } + // If this is a comparison ('a' == $_POST['foo']), sanitization isn't needed. + if ( $this->is_comparison( $stackPtr ) ) { + return; } - if ( ! $is_sanitized ) { - $phpcsFile->addError( 'Detected usage of a non-sanitized input variable: %s', $stackPtr, 'InputNotSanitized', array( $tokens[$stackPtr]['content'] ) ); + // Now look for sanitizing functions + if ( ! $this->is_sanitized( $stackPtr ) ) { + $phpcsFile->addError( 'Detected usage of a non-sanitized input variable: %s', $stackPtr, 'InputNotSanitized', array( $tokens[ $stackPtr ]['content'] ) ); } - return; - }//end process() - /** - * Get array index key of the variable requested - * @param [type] $phpcsFile [description] - * @param [type] $tokens [description] - * @param [type] $stackPtr [description] - * @return [type] [description] - */ - public function getArrayIndexKey( $phpcsFile, $tokens, $stackPtr ) { - // Find next bracket - $bracketOpener = $phpcsFile->findNext( array( T_OPEN_SQUARE_BRACKET ), $stackPtr, $stackPtr + 3 ); - - // If no brackets, exit with a warning, this is a non-typical usage of super globals - if ( empty ( $bracketOpener ) ) { - return false; - } - - $bracketCloser = $tokens[$bracketOpener]['bracket_closer']; - - $varKey = trim( $phpcsFile->getTokensAsString( $bracketOpener + 1, $bracketCloser - $bracketOpener - 1 ) ); // aka 'hello' in $_POST['hello'] - - return $varKey; - } + } // end process() -}//end class +} // end class diff --git a/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.inc index 29c775c7e2..76c22735f9 100644 --- a/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.inc @@ -30,7 +30,7 @@ $empty = ( empty( $_POST['foo4'] ) ); -$foo = $_POST['bar']; // Bad +$foo = $_POST['bar']; // Bad x 2 function foo() { // Ok, if WordPress_Sniffs_VIP_ValidatedSanitizedInputSniff::$check_validation_in_scope_only == false @@ -80,3 +80,14 @@ echo array_map( 'foo', $_GET['test'] ); // Bad echo array_map( $something, $_GET['test'] ); // Bad echo array_map( array( $obj, 'func' ), $_GET['test'] ); // Bad echo array_map( array( $obj, 'sanitize_text_field' ), $_GET['test'] ); // Bad + +// Sanitized but not validated. +$foo = (int) $_POST['foo6']; // Bad + +// Non-assignment checks are OK. +if ( 'bar' === $_POST['foo'] ) {} // OK +if ( $_GET['test'] != 'a' ) {} // OK +if ( 'bar' === do_something( $_POST['foo'] ) ) {} // Bad + +switch ( $_POST['foo'] ) {} // OK +switch ( do_something( $_POST['foo'] ) ) {} // Bad diff --git a/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.php index 95ae6095c3..60b27db862 100644 --- a/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/VIP/ValidatedSanitizedInputUnitTest.php @@ -29,12 +29,15 @@ public function getErrorList() 7 => 1, 10 => 1, 20 => 1, - 33 => 1, + 33 => 2, 65 => 1, 79 => 1, 80 => 1, 81 => 1, 82 => 1, + 85 => 1, + 90 => 1, + 93 => 1, ); }//end getErrorList()