Variables/ServerVariables: various sniff improvements

WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php

@@ -9,6 +9,8 @@
 
 namespace WordPressVIPMinimum\Sniffs\Variables;
 
+use PHP_CodeSniffer\Util\Tokens;
+use PHPCSUtils\Utils\TextStrings;
 use WordPressVIPMinimum\Sniffs\Sniff;
 
 /**
@@ -17,7 +19,7 @@
 class ServerVariablesSniff extends Sniff {
 
 	/**
-	 * List of restricted constant names.
+	 * List of restricted indices.
 	 *
 	 * @var array<string, array<string, bool>>
 	 */
@@ -53,21 +55,94 @@ 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;
 		}
 
-		$variableNamePtr = $this->phpcsFile->findNext( [ T_CONSTANT_ENCAPSED_STRING ], $stackPtr + 1, null, false, null, true );
-		$variableName    = str_replace( [ "'", '"' ], '', $this->tokens[ $variableNamePtr ]['content'] );
+		$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;
+			}
 
-		if ( isset( $this->restrictedVariables['authVariables'][ $variableName ] ) ) {
+			$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( $searchStart );
+		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 ] ) ) {
 			$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 );
 		}
 	}
+
+	/**
+	 * 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;
+	}
 }

WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc

@@ -1,9 +1,75 @@
 <?php
 
-// Bad variables. Should never happen.
-$_SERVER['PHP_AUTH_PW']; 
-$_SERVER['HTTP_X_IP_TRAIL'];
-$_SERVER['HTTP_X_FORWARDED_FOR'];  
-$_SERVER["REMOTE_ADDR"]; 	// let's test one with double quotes too
+/*
+ * Not the sniff target.
+ */
+$otherVar = 'ignored';
+$notSERVER['PHP_AUTH_USER'];
+$_server['PHP_AUTH_USER']; // Variable names are case-sensitive.
 
-$_SERVER['SOME_OTHER_VARIABLE']; // We don't care.
+
+/*
+ * This is okay.
+ */
+var_dump($_SERVER); // No key access.
+$_SERVER[] = 10;
+$_SERVER['SOME_OTHER_INDEX'];
+$_SERVER['php_auth_user']; // Array indices are case-sensitive.
+
+// PHP 7.4+ spread in array.
+$a = [...$_SERVER]; // No key access.
+
+// Ignore as undetermined.
+$_SERVER[ $name ];
+$_SERVER["HTTP_X_{$a}"];
+$_SERVER['HTTP_X_' . $a];
+
+
+/*
+ * Bad variables. Should never happen.
+ */
+$_SERVER['PHP_AUTH_USER'];
+$_SERVER[  'PHP_AUTH_PW'  ];
+$_SERVER  [
+    'HTTP_X_IP_TRAIL'
+];
+$_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"];
+
+// 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']);
+	}
+}
+
+// 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'];
+
+// 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"];

WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.php

@@ -23,10 +23,16 @@ class ServerVariablesUnitTest extends AbstractSniffUnitTest {
 	 */
 	public function getErrorList() {
 		return [
-			4 => 1,
-			5 => 1,
-			6 => 1,
-			7 => 1,
+			31 => 1,
+			32 => 1,
+			33 => 1,
+			36 => 1,
+			37 => 1,
+			71 => 1,
+			72 => 1,
+			73 => 1,
+			74 => 1,
+			75 => 1,
 		];
 	}