diff --git a/WordPressVIPMinimum/Sniffs/Functions/OptionsRaceConditionSniff.php b/WordPressVIPMinimum/Sniffs/Functions/OptionsRaceConditionSniff.php index afc88448..44f4e9e2 100644 --- a/WordPressVIPMinimum/Sniffs/Functions/OptionsRaceConditionSniff.php +++ b/WordPressVIPMinimum/Sniffs/Functions/OptionsRaceConditionSniff.php @@ -54,6 +54,7 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco $tokens = $phpcsFile->getTokens(); if ( $tokens[ $stackPtr ]['content'] !== $this->delete_option ) { + // delete_option is not first function found. $stackPtr = $phpcsFile->findNext( [ T_STRING ], $stackPtr + 1, @@ -78,23 +79,58 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco return; // Not a function call, bail. } - $delete_option_semicolon = $phpcsFile->findNext( - [ T_SEMICOLON ], - $tokens[ $delete_option_scope_start ]['parenthesis_closer'] + 1, + $delete_option_name = $phpcsFile->findNext( + Tokens::$emptyTokens, + $delete_option_scope_start + 1, null, - false + true ); - $delete_option_key = $phpcsFile->findNext( + $delete_option_concat = $phpcsFile->findNext( Tokens::$emptyTokens, - $delete_option_scope_start + 1, + $delete_option_name + 1, null, true ); + $delete_option_name = $this->trim_strip_quotes( $tokens[ $delete_option_name ]['content'] ); + + $is_delete_option_concat = $tokens[ $delete_option_concat ]['code'] === T_STRING_CONCAT; + if ( $is_delete_option_concat ) { + // If option name is concatenated, we need to build it out. + $delete_option_concat = $phpcsFile->findNext( + Tokens::$emptyTokens, + $delete_option_concat + 1, + null, + true + ); + + while ( $delete_option_concat < $tokens[ $delete_option_scope_start ]['parenthesis_closer'] ) { + $delete_option_name .= $this->trim_strip_quotes( $tokens[ $delete_option_concat ]['content'] ); + + $delete_option_concat = $phpcsFile->findNext( + array_merge( Tokens::$emptyTokens, [ T_STRING_CONCAT ] ), + $delete_option_concat + 1, + null, + true + ); + } + } + + $delete_option_scope_end = $phpcsFile->findNext( + [ T_SEMICOLON ], + $tokens[ $delete_option_scope_start ]['parenthesis_closer'] + 1, + null, + false + ); + + if ( ! $delete_option_scope_end ) { + return; // Something went wrong with the syntax. + } + $add_option = $phpcsFile->findNext( Tokens::$emptyTokens, - $delete_option_semicolon + 1, + $delete_option_scope_end + 1, null, true ); @@ -126,15 +162,55 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco return; // Not a function call, bail. } - $add_option_inside_if_option_key = $phpcsFile->findNext( + $add_option_inside_if_option_name = $phpcsFile->findNext( Tokens::$emptyTokens, $add_option_inside_if_scope_start + 1, null, true ); - if ( $add_option_inside_if_option_key && $this->is_same_option_key( $tokens, $add_option_inside_if_option_key, $delete_option_key ) ) { - $phpcsFile->addWarning( $message, $add_option_inside_if_option_key, 'OptionsRaceCondition' ); + $add_option_inside_if_concat = $phpcsFile->findNext( + Tokens::$emptyTokens, + $add_option_inside_if_option_name + 1, + null, + true + ); + + $add_option_inside_if_option_name = $this->trim_strip_quotes( $tokens[ $add_option_inside_if_option_name ]['content'] ); + + if ( $is_delete_option_concat && $tokens[ $add_option_inside_if_concat ]['code'] === T_STRING_CONCAT ) { + $add_option_inside_if_concat = $phpcsFile->findNext( + array_merge( Tokens::$emptyTokens, [ T_STRING_CONCAT ] ), + $add_option_inside_if_concat + 1, + null, + true + ); + + $add_option_inside_if_scope_end = $phpcsFile->findNext( + [ T_COMMA ], + $add_option_inside_if_concat + 1, + null, + false + ); + + if ( ! $add_option_inside_if_scope_end ) { + return; // Something went wrong. + } + + while ( $add_option_inside_if_concat < $add_option_inside_if_scope_end ) { + $add_option_inside_if_option_name .= $this->trim_strip_quotes( $tokens[ $add_option_inside_if_concat ]['content'] ); + + $add_option_inside_if_concat = $phpcsFile->findNext( + array_merge( Tokens::$emptyTokens, [ T_STRING_CONCAT ] ), + $add_option_inside_if_concat + 1, + null, + true + ); + } + } + + if ( $this->is_same_option_key( $delete_option_name, $add_option_inside_if_option_name ) ) { + $phpcsFile->addWarning( $message, $add_option_inside_if_scope_start, 'OptionsRaceCondition' ); } // Walk ahead out of IF control structure. @@ -180,16 +256,54 @@ protected function processTokenWithinScope( File $phpcsFile, $stackPtr, $currSco return; // Not a function call, bail. } - // Check if it's the same option being deleted earlier. - $add_option_key = $phpcsFile->findNext( + $add_option_name = $phpcsFile->findNext( Tokens::$emptyTokens, $add_option_scope_start + 1, null, true ); - if ( $this->is_same_option_key( $tokens, $add_option_key, $delete_option_key ) ) { - $phpcsFile->addWarning( $message, $add_option_key, 'OptionsRaceCondition' ); + $add_option_concat = $phpcsFile->findNext( + Tokens::$emptyTokens, + $add_option_name + 1, + null, + true + ); + + $add_option_name = $this->trim_strip_quotes( $tokens[ $add_option_name ]['content'] ); + if ( $is_delete_option_concat && $tokens[ $add_option_concat ]['code'] === T_STRING_CONCAT ) { + $add_option_concat = $phpcsFile->findNext( + Tokens::$emptyTokens, + $add_option_concat + 1, + null, + true + ); + + $add_option_scope_end = $phpcsFile->findNext( + [ T_COMMA ], + $add_option_concat + 1, + null, + false + ); + + if ( ! $add_option_scope_end ) { + return; // Something went wrong. + } + + while ( $add_option_concat < $add_option_scope_end ) { + $add_option_name .= $this->trim_strip_quotes( $tokens[ $add_option_concat ]['content'] ); + + $add_option_concat = $phpcsFile->findNext( + array_merge( Tokens::$emptyTokens, [ T_STRING_CONCAT ] ), + $add_option_concat + 1, + null, + true + ); + } + } + + if ( $this->is_same_option_key( $delete_option_name, $add_option_name ) ) { + $phpcsFile->addWarning( $message, $add_option_scope_start, 'OptionsRaceCondition' ); return; } } @@ -209,14 +323,22 @@ protected function processTokenOutsideScope( File $phpcsFile, $stackPtr ) { /** * Check if option is the same. * - * @param array $tokens List of PHPCS tokens. - * @param int $first_option Stack position of first option. - * @param int $second_option Stack position of second option to match against. + * @param string $option_name Option name. + * @param string $option_name_to_compare Option name to compare against. * * @return false */ - private function is_same_option_key( $tokens, $first_option, $second_option ) { - return $tokens[ $first_option ]['code'] === $tokens[ $second_option ]['code'] && - $tokens[ $first_option ]['content'] === $tokens[ $second_option ]['content']; + private function is_same_option_key( $option_name, $option_name_to_compare ) { + return $option_name === $option_name_to_compare; + } + + /** + * Trim whitespace and strip quotes surrounding an arbitrary string. + * + * @param string $string The raw string. + * @return string String without whitespace or quotes around it. + */ + public function trim_strip_quotes( $string ) { + return trim( preg_replace( '`^([\'"])(.*)\1$`Ds', '$2', $string ) ); } } diff --git a/WordPressVIPMinimum/Tests/Functions/OptionsRaceConditionUnitTest.inc b/WordPressVIPMinimum/Tests/Functions/OptionsRaceConditionUnitTest.inc index 0dae72fd..21f180bf 100644 --- a/WordPressVIPMinimum/Tests/Functions/OptionsRaceConditionUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Functions/OptionsRaceConditionUnitTest.inc @@ -27,9 +27,9 @@ function test_same_option_key() { add_option( $key, 'data' ); // Warning. } -function test_same_option_key_var_assignment2() { - $delete = delete_option( 'option_key' ); - add_option( 'option_key', [ 'stuff', '123' ] ); // Warning. +function test_same_option_key_double_quoted_string() { + $delete = delete_option( "option_{$id}" ); + add_option( "option_{$id}", [ 'stuff', '123' ] ); // Warning. } function test_same_option_key_string() { @@ -103,10 +103,49 @@ function concurrent_option_writes_with_one_if_different_key() { $test = add_option( 'test_option', [ 1, 2, 3 ] ); // Warning. } -function concurrent_option_writes_with_one_if_same_key() { +function test_concurrent_option_writes_with_if() { $test = delete_option( 'test_option' ); if ( $test ) { add_option( 'test_option', [] ); // Warning. } $test = add_option( 'test_option', [ 1, 2, 3 ] ); // Warning. } + +function test_different_quotes() { + delete_option( 'foobar' ); + add_option( "foobar", 'baz' ); // Warning. +} + +function test_spacing_inside_option_name() { + delete_option( 'foobar' ); + add_option( ' foobar ', 'baz' ); // Warning. +} + +function test_concat_same_string_var() { + delete_option( 'foo_' . $bar ); + add_option( 'foo_' . $bar, 'baz' ); // Warning. +} + +function test_concat_different_string_var() { + delete_option( 'foo_' . $ba ); + add_option( 'foo_' . $bar, 'baz' ); // OK - Not same option name. +} + +function test_inside_if_concat_option_name( $test ) { + $test = delete_option( "foo_" . $bar ); + if ( $test ) { + add_option( 'foo_' . $bar, [ 1, 2, 3 ] ); // Warning. + } +} + +function test_long_concat() { + delete_option( 'foo_' . $bar . '_' . $id ); + add_option( "foo_" . $bar . '_' . $id, 'baz' ); // Warning. +} + +function test_long_concat_inside_if() { + $test = delete_option( 'foo_' . $bar . '_' . $id ); + if ( $test ) { + $option_added = add_option( "foo_" . $bar . '_' . $id, 'baz' ); // Warning. + } +} \ No newline at end of file diff --git a/WordPressVIPMinimum/Tests/Functions/OptionsRaceConditionUnitTest.php b/WordPressVIPMinimum/Tests/Functions/OptionsRaceConditionUnitTest.php index ad8fe179..eabcd21d 100644 --- a/WordPressVIPMinimum/Tests/Functions/OptionsRaceConditionUnitTest.php +++ b/WordPressVIPMinimum/Tests/Functions/OptionsRaceConditionUnitTest.php @@ -47,6 +47,12 @@ public function getWarningList() { 103 => 1, 109 => 1, 111 => 1, + 116 => 1, + 121 => 1, + 126 => 1, + 137 => 1, + 143 => 1, + 149 => 1, ]; }