From 0d5e1eca02ef564f63ee29d2f57733a400019230 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 19 Jul 2025 20:42:23 +0200 Subject: [PATCH 1/6] Variables/ServerVariables: improve the tests * Expand the tests safeguarding against false positives. * Ensure all five indices are covered by at least one test. * Add some more variations to the pre-existing tests: - Unconventional spacing. - Comments in unexpected places. --- .../Variables/ServerVariablesUnitTest.inc | 40 ++++++++++++++++--- .../Variables/ServerVariablesUnitTest.php | 9 +++-- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc index a22bae0c..2fe595d3 100644 --- a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc @@ -1,9 +1,37 @@ 1, - 5 => 1, - 6 => 1, - 7 => 1, + 31 => 1, + 32 => 1, + 33 => 1, + 36 => 1, + 37 => 1, ]; } From 6f8ebb6369e7a35160032a78c479c57ea45f1c60 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 19 Jul 2025 21:30:42 +0200 Subject: [PATCH 2/6] Variables/ServerVariables: use the correct terminology The sniff looks for specific indexes/keys for array access. This commit fixes some documentation and variable names used to use this terminology correctly. --- .../Sniffs/Variables/ServerVariablesSniff.php | 12 ++++++------ .../Tests/Variables/ServerVariablesUnitTest.inc | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php b/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php index ffd039e2..d47fff6e 100644 --- a/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php +++ b/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php @@ -17,7 +17,7 @@ class ServerVariablesSniff extends Sniff { /** - * List of restricted constant names. + * List of restricted indices. * * @var array> */ @@ -58,15 +58,15 @@ public function process_token( $stackPtr ) { return; } - $variableNamePtr = $this->phpcsFile->findNext( [ T_CONSTANT_ENCAPSED_STRING ], $stackPtr + 1, null, false, null, true ); - $variableName = str_replace( [ "'", '"' ], '', $this->tokens[ $variableNamePtr ]['content'] ); + $indexPtr = $this->phpcsFile->findNext( [ T_CONSTANT_ENCAPSED_STRING ], $stackPtr + 1, null, false, null, true ); + $indexName = str_replace( [ "'", '"' ], '', $this->tokens[ $indexPtr ]['content'] ); - if ( isset( $this->restrictedVariables['authVariables'][ $variableName ] ) ) { + if ( isset( $this->restrictedVariables['authVariables'][ $indexName ] ) ) { $message = 'Basic authentication should not be handled via PHP code.'; $this->phpcsFile->addError( $message, $stackPtr, 'BasicAuthentication' ); - } elseif ( isset( $this->restrictedVariables['userControlledVariables'][ $variableName ] ) ) { + } elseif ( isset( $this->restrictedVariables['userControlledVariables'][ $indexName ] ) ) { $message = 'Header "%s" is user-controlled and should be properly validated before use.'; - $data = [ $variableName ]; + $data = [ $indexName ]; $this->phpcsFile->addError( $message, $stackPtr, 'UserControlledHeaders', $data ); } } diff --git a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc index 2fe595d3..e8e2128b 100644 --- a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc @@ -13,7 +13,7 @@ $_server['PHP_AUTH_USER']; // Variable names are case-sensitive. */ var_dump($_SERVER); // No key access. $_SERVER[] = 10; -$_SERVER['SOME_OTHER_VARIABLE']; +$_SERVER['SOME_OTHER_INDEX']; $_SERVER['php_auth_user']; // Array indices are case-sensitive. // PHP 7.4+ spread in array. From 97f62dd329bb0e1bf41eeb4e6ff7849225d7a11f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 19 Jul 2025 20:45:39 +0200 Subject: [PATCH 3/6] Variables/ServerVariables: bug fix - incorrect quote stripping As things were, quotes _within_ a text string would also be stripped. Not that it's very likely for any of the `$_SERVER` keys to ever have these, but that's beside the point. --- .../Sniffs/Variables/ServerVariablesSniff.php | 3 ++- .../Tests/Variables/ServerVariablesUnitTest.inc | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php b/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php index d47fff6e..4bcd1fca 100644 --- a/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php +++ b/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php @@ -9,6 +9,7 @@ namespace WordPressVIPMinimum\Sniffs\Variables; +use PHPCSUtils\Utils\TextStrings; use WordPressVIPMinimum\Sniffs\Sniff; /** @@ -59,7 +60,7 @@ public function process_token( $stackPtr ) { } $indexPtr = $this->phpcsFile->findNext( [ T_CONSTANT_ENCAPSED_STRING ], $stackPtr + 1, null, false, null, true ); - $indexName = str_replace( [ "'", '"' ], '', $this->tokens[ $indexPtr ]['content'] ); + $indexName = TextStrings::stripQuotes( $this->tokens[ $indexPtr ]['content'] ); if ( isset( $this->restrictedVariables['authVariables'][ $indexName ] ) ) { $message = 'Basic authentication should not be handled via PHP code.'; diff --git a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc index e8e2128b..628654ca 100644 --- a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc @@ -35,3 +35,11 @@ $_SERVER [ ]; $_SERVER /*comment*/ [ /*comment*/ 'HTTP_X_FORWARDED_FOR' /*comment*/ ]; $_SERVER["REMOTE_ADDR"]; // Let's test one with double quotes too. + + +/* + * Prevent various false positives. + */ +// These look like "forbidden" indices, but aren't. +$_SERVER['PHP_"AUTH"_PW']; +$_SERVER["PHP_'AUTH'_PW"]; From d0362edde233b0871a215972f88bf191304d9e78 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 19 Jul 2025 20:53:00 +0200 Subject: [PATCH 4/6] Variables/ServerVariables: bug fix - false positives on OO property access While it should probably be considered bad practice, it is allowed to declare an OO property called `$_SERVER` and this sniff should not get confused by that. Includes tests. --- .../Sniffs/Variables/ServerVariablesSniff.php | 7 +++++++ .../Tests/Variables/ServerVariablesUnitTest.inc | 12 ++++++++++++ 2 files changed, 19 insertions(+) diff --git a/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php b/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php index 4bcd1fca..5cfa1ced 100644 --- a/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php +++ b/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php @@ -9,6 +9,7 @@ namespace WordPressVIPMinimum\Sniffs\Variables; +use PHP_CodeSniffer\Util\Tokens; use PHPCSUtils\Utils\TextStrings; use WordPressVIPMinimum\Sniffs\Sniff; @@ -59,6 +60,12 @@ public function process_token( $stackPtr ) { return; } + $prevNonEmpty = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true ); + if ( $this->tokens[ $prevNonEmpty ]['code'] === T_DOUBLE_COLON ) { + // Access to OO property mirroring the name of the superglobal. Not our concern. + return; + } + $indexPtr = $this->phpcsFile->findNext( [ T_CONSTANT_ENCAPSED_STRING ], $stackPtr + 1, null, false, null, true ); $indexName = TextStrings::stripQuotes( $this->tokens[ $indexPtr ]['content'] ); diff --git a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc index 628654ca..6b2d098a 100644 --- a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc @@ -43,3 +43,15 @@ $_SERVER["REMOTE_ADDR"]; // Let's test one with double quotes too. // These look like "forbidden" indices, but aren't. $_SERVER['PHP_"AUTH"_PW']; $_SERVER["PHP_'AUTH'_PW"]; + +// Sniff should not get confused over static OO property access. +class PeopleDoSillyThings extends AllowedByPHP { + public static $_SERVER = []; + + public function show() { + var_export(self::$_SERVER['PHP_AUTH_USER']); + var_export(parent :: $_SERVER['PHP_AUTH_PW']); + var_export(static::$_SERVER['HTTP_X_FORWARDED_FOR']); + var_export(OtherClass::$_SERVER['HTTP_X_IP_TRAIL']); + } +} From 5bbd1b9eef49137a5240847613238e257703b3eb Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 19 Jul 2025 21:27:07 +0200 Subject: [PATCH 5/6] Variables/ServerVariables: bug fix - faulty array key determination The code to find the array index was flawed and could walk beyond the brackets of this array access. Additionally, array access keys comprised of multiple tokens were not handled correctly. Includes tests. Note: WordPressCS has helper functions to retrieve the array access name, but those are marked as internal, which is the reason to introduce a custom function. --- .../Sniffs/Variables/ServerVariablesSniff.php | 48 ++++++++++++++++++- .../Variables/ServerVariablesUnitTest.inc | 6 +++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php b/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php index 5cfa1ced..fc213d52 100644 --- a/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php +++ b/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php @@ -66,7 +66,12 @@ public function process_token( $stackPtr ) { return; } - $indexPtr = $this->phpcsFile->findNext( [ T_CONSTANT_ENCAPSED_STRING ], $stackPtr + 1, null, false, null, true ); + $indexPtr = $this->get_array_access_key( $stackPtr ); + if ( $indexPtr === false ) { + // Couldn't find an array index token usable for the purposes of this sniff. Bow out as undetermined. + return; + } + $indexName = TextStrings::stripQuotes( $this->tokens[ $indexPtr ]['content'] ); if ( isset( $this->restrictedVariables['authVariables'][ $indexName ] ) ) { @@ -78,4 +83,45 @@ public function process_token( $stackPtr ) { $this->phpcsFile->addError( $message, $stackPtr, 'UserControlledHeaders', $data ); } } + + /** + * Get the array access key. + * + * Find the array access key and check if it is: + * - comprised of a single functional token. + * - that token is a T_CONSTANT_ENCAPSED_STRING. + * + * @param int $stackPtr The position of either a variable or the close bracket of a previous array access. + * + * @return int|false Stack pointer to the index token; or FALSE for + * live coding, non-indexed array assignment, or non plain text array keys. + */ + private function get_array_access_key( $stackPtr ) { + $openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + if ( $openBracket === false + || $this->tokens[ $openBracket ]['code'] !== T_OPEN_SQUARE_BRACKET + || isset( $this->tokens[ $openBracket ]['bracket_closer'] ) === false + ) { + // If it isn't an open bracket, this isn't array access. And without closer, it is a parse error/live coding. + return false; + } + + $closeBracket = $this->tokens[ $openBracket ]['bracket_closer']; + + $indexPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $openBracket + 1 ), $closeBracket, true ); + if ( $indexPtr === false + || $this->tokens[ $indexPtr ]['code'] !== T_CONSTANT_ENCAPSED_STRING + ) { + // No array access (like for array assignment without key) or key is not plain text. + return false; + } + + $hasOtherTokens = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $indexPtr + 1 ), $closeBracket, true ); + if ( $hasOtherTokens !== false ) { + // The array index is comprised of multiple tokens. Bow out as undetermined. + return false; + } + + return $indexPtr; + } } diff --git a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc index 6b2d098a..029d1fbd 100644 --- a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc @@ -55,3 +55,9 @@ class PeopleDoSillyThings extends AllowedByPHP { var_export(OtherClass::$_SERVER['HTTP_X_IP_TRAIL']); } } + +// Safeguard the sniff looks at the access key for the $_SERVER variable only and doesn't walk too far. +echo $_SERVER[$other_key] . $NOT_SERVER['PHP_AUTH_PW']; + +// Safeguard the sniff doesn't get confused over partially dynamic keys. +echo $_SERVER[$other_key . 'PHP_AUTH_PW']; From 67bba5713ac09a29cec19fc52e55b7918e2c3def Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 19 Jul 2025 22:14:44 +0200 Subject: [PATCH 6/6] Variables/ServerVariables: bug fix - false negatives for `$GLOBALS['_SERVER']` The `$GLOBALS['_SERVER']` superglobals access is equivalent to using `$_SERVER`, so should be examined too. Includes tests. --- .../Sniffs/Variables/ServerVariablesSniff.php | 27 ++++++++++++++++--- .../Variables/ServerVariablesUnitTest.inc | 12 +++++++++ .../Variables/ServerVariablesUnitTest.php | 5 ++++ 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php b/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php index fc213d52..f3769380 100644 --- a/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php +++ b/WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php @@ -55,18 +55,39 @@ public function register() { */ public function process_token( $stackPtr ) { - if ( $this->tokens[ $stackPtr ]['content'] !== '$_SERVER' ) { - // Not the variable we are looking for. + if ( $this->tokens[ $stackPtr ]['content'] !== '$_SERVER' + && $this->tokens[ $stackPtr ]['content'] !== '$GLOBALS' + ) { + // Not a variable we are looking for. return; } + $searchStart = $stackPtr; + if ( $this->tokens[ $stackPtr ]['content'] === '$GLOBALS' ) { + $globalsIndexPtr = $this->get_array_access_key( $stackPtr ); + if ( $globalsIndexPtr === false ) { + // Couldn't find an array index token usable for the purposes of this sniff. Bow out. + return; + } + + $globalsIndexName = TextStrings::stripQuotes( $this->tokens[ $globalsIndexPtr ]['content'] ); + if ( $globalsIndexName !== '_SERVER' ) { + // Not access to `$GLOBALS['_SERVER']`. + return; + } + + // Set the start point for the next array access key search to the close bracket of this array index. + // No need for defensive coding as we already know there will be a valid close bracket next. + $searchStart = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $globalsIndexPtr + 1 ), null, true ); + } + $prevNonEmpty = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true ); if ( $this->tokens[ $prevNonEmpty ]['code'] === T_DOUBLE_COLON ) { // Access to OO property mirroring the name of the superglobal. Not our concern. return; } - $indexPtr = $this->get_array_access_key( $stackPtr ); + $indexPtr = $this->get_array_access_key( $searchStart ); if ( $indexPtr === false ) { // Couldn't find an array index token usable for the purposes of this sniff. Bow out as undetermined. return; diff --git a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc index 029d1fbd..28de0071 100644 --- a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc @@ -61,3 +61,15 @@ echo $_SERVER[$other_key] . $NOT_SERVER['PHP_AUTH_PW']; // Safeguard the sniff doesn't get confused over partially dynamic keys. echo $_SERVER[$other_key . 'PHP_AUTH_PW']; + +// Access of the array indices via the $GLOBALS superglobal should also be recognized. +$GLOBALS['NOT_SERVER']['PHP_AUTH_USER']; // OK. +$GLOBALS[$a]['PHP_AUTH_USER']; // OK. +$GLOBALS['_SERVER']['SOME_OTHER_KEY']; // OK. +$GLOBALS['_SERVER'][$a]; // OK. + +$GLOBALS['_SERVER']['PHP_AUTH_USER']; +$GLOBALS['_SERVER']['PHP_AUTH_PW']; +$GLOBALS["_SERVER"]['HTTP_X_IP_TRAIL']; +$GLOBALS['_SERVER']['HTTP_X_FORWARDED_FOR']; +$GLOBALS['_SERVER']["REMOTE_ADDR"]; diff --git a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.php b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.php index 5222e8ce..610dabd0 100644 --- a/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.php +++ b/WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.php @@ -28,6 +28,11 @@ public function getErrorList() { 33 => 1, 36 => 1, 37 => 1, + 71 => 1, + 72 => 1, + 73 => 1, + 74 => 1, + 75 => 1, ]; }