Skip to content

Commit ff75da7

Browse files
GaryJonesGaryJones
authored
Merge pull request #850 from Automattic/feature/variables-servervariables-sniff-review
2 parents ecdc10c + 67bba57 commit ff75da7

File tree

3 files changed

+165
-18
lines changed

3 files changed

+165
-18
lines changed

‎WordPressVIPMinimum/Sniffs/Variables/ServerVariablesSniff.php

Lines changed: 83 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99

1010
namespace WordPressVIPMinimum\Sniffs\Variables;
1111

12+
use PHP_CodeSniffer\Util\Tokens;
13+
use PHPCSUtils\Utils\TextStrings;
1214
use WordPressVIPMinimum\Sniffs\Sniff;
1315

1416
/**
@@ -17,7 +19,7 @@
1719
class ServerVariablesSniff extends Sniff {
1820

1921
/**
20-
* List of restricted constant names.
22+
* List of restricted indices.
2123
*
2224
* @var array<string, array<string, bool>>
2325
*/
@@ -53,21 +55,94 @@ public function register() {
5355
*/
5456
public function process_token( $stackPtr ) {
5557

56-
if ( $this->tokens[ $stackPtr ]['content'] !== '$_SERVER' ) {
57-
// Not the variable we are looking for.
58+
if ( $this->tokens[ $stackPtr ]['content'] !== '$_SERVER'
59+
&& $this->tokens[ $stackPtr ]['content'] !== '$GLOBALS'
60+
) {
61+
// Not a variable we are looking for.
5862
return;
5963
}
6064

61-
$variableNamePtr = $this->phpcsFile->findNext( [ T_CONSTANT_ENCAPSED_STRING ], $stackPtr + 1, null, false, null, true );
62-
$variableName = str_replace( [ "'", '"' ], '', $this->tokens[ $variableNamePtr ]['content'] );
65+
$searchStart = $stackPtr;
66+
if ( $this->tokens[ $stackPtr ]['content'] === '$GLOBALS' ) {
67+
$globalsIndexPtr = $this->get_array_access_key( $stackPtr );
68+
if ( $globalsIndexPtr === false ) {
69+
// Couldn't find an array index token usable for the purposes of this sniff. Bow out.
70+
return;
71+
}
6372

64-
if ( isset( $this->restrictedVariables['authVariables'][ $variableName ] ) ) {
73+
$globalsIndexName = TextStrings::stripQuotes( $this->tokens[ $globalsIndexPtr ]['content'] );
74+
if ( $globalsIndexName !== '_SERVER' ) {
75+
// Not access to `$GLOBALS['_SERVER']`.
76+
return;
77+
}
78+
79+
// Set the start point for the next array access key search to the close bracket of this array index.
80+
// No need for defensive coding as we already know there will be a valid close bracket next.
81+
$searchStart = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $globalsIndexPtr + 1 ), null, true );
82+
}
83+
84+
$prevNonEmpty = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true );
85+
if ( $this->tokens[ $prevNonEmpty ]['code'] === T_DOUBLE_COLON ) {
86+
// Access to OO property mirroring the name of the superglobal. Not our concern.
87+
return;
88+
}
89+
90+
$indexPtr = $this->get_array_access_key( $searchStart );
91+
if ( $indexPtr === false ) {
92+
// Couldn't find an array index token usable for the purposes of this sniff. Bow out as undetermined.
93+
return;
94+
}
95+
96+
$indexName = TextStrings::stripQuotes( $this->tokens[ $indexPtr ]['content'] );
97+
98+
if ( isset( $this->restrictedVariables['authVariables'][ $indexName ] ) ) {
6599
$message = 'Basic authentication should not be handled via PHP code.';
66100
$this->phpcsFile->addError( $message, $stackPtr, 'BasicAuthentication' );
67-
} elseif ( isset( $this->restrictedVariables['userControlledVariables'][ $variableName ] ) ) {
101+
} elseif ( isset( $this->restrictedVariables['userControlledVariables'][ $indexName ] ) ) {
68102
$message = 'Header "%s" is user-controlled and should be properly validated before use.';
69-
$data = [ $variableName ];
103+
$data = [ $indexName ];
70104
$this->phpcsFile->addError( $message, $stackPtr, 'UserControlledHeaders', $data );
71105
}
72106
}
107+
108+
/**
109+
* Get the array access key.
110+
*
111+
* Find the array access key and check if it is:
112+
* - comprised of a single functional token.
113+
* - that token is a T_CONSTANT_ENCAPSED_STRING.
114+
*
115+
* @param int $stackPtr The position of either a variable or the close bracket of a previous array access.
116+
*
117+
* @return int|false Stack pointer to the index token; or FALSE for
118+
* live coding, non-indexed array assignment, or non plain text array keys.
119+
*/
120+
private function get_array_access_key( $stackPtr ) {
121+
$openBracket = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
122+
if ( $openBracket === false
123+
|| $this->tokens[ $openBracket ]['code'] !== T_OPEN_SQUARE_BRACKET
124+
|| isset( $this->tokens[ $openBracket ]['bracket_closer'] ) === false
125+
) {
126+
// If it isn't an open bracket, this isn't array access. And without closer, it is a parse error/live coding.
127+
return false;
128+
}
129+
130+
$closeBracket = $this->tokens[ $openBracket ]['bracket_closer'];
131+
132+
$indexPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $openBracket + 1 ), $closeBracket, true );
133+
if ( $indexPtr === false
134+
|| $this->tokens[ $indexPtr ]['code'] !== T_CONSTANT_ENCAPSED_STRING
135+
) {
136+
// No array access (like for array assignment without key) or key is not plain text.
137+
return false;
138+
}
139+
140+
$hasOtherTokens = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $indexPtr + 1 ), $closeBracket, true );
141+
if ( $hasOtherTokens !== false ) {
142+
// The array index is comprised of multiple tokens. Bow out as undetermined.
143+
return false;
144+
}
145+
146+
return $indexPtr;
147+
}
73148
}

‎WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.inc

Lines changed: 72 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,75 @@
11
<?php
22

3-
// Bad variables. Should never happen.
4-
$_SERVER['PHP_AUTH_PW'];
5-
$_SERVER['HTTP_X_IP_TRAIL'];
6-
$_SERVER['HTTP_X_FORWARDED_FOR'];
7-
$_SERVER["REMOTE_ADDR"]; // let's test one with double quotes too
3+
/*
4+
* Not the sniff target.
5+
*/
6+
$otherVar = 'ignored';
7+
$notSERVER['PHP_AUTH_USER'];
8+
$_server['PHP_AUTH_USER']; // Variable names are case-sensitive.
89

9-
$_SERVER['SOME_OTHER_VARIABLE']; // We don't care.
10+
11+
/*
12+
* This is okay.
13+
*/
14+
var_dump($_SERVER); // No key access.
15+
$_SERVER[] = 10;
16+
$_SERVER['SOME_OTHER_INDEX'];
17+
$_SERVER['php_auth_user']; // Array indices are case-sensitive.
18+
19+
// PHP 7.4+ spread in array.
20+
$a = [...$_SERVER]; // No key access.
21+
22+
// Ignore as undetermined.
23+
$_SERVER[ $name ];
24+
$_SERVER["HTTP_X_{$a}"];
25+
$_SERVER['HTTP_X_' . $a];
26+
27+
28+
/*
29+
* Bad variables. Should never happen.
30+
*/
31+
$_SERVER['PHP_AUTH_USER'];
32+
$_SERVER[ 'PHP_AUTH_PW' ];
33+
$_SERVER [
34+
'HTTP_X_IP_TRAIL'
35+
];
36+
$_SERVER /*comment*/ [ /*comment*/ 'HTTP_X_FORWARDED_FOR' /*comment*/ ];
37+
$_SERVER["REMOTE_ADDR"]; // Let's test one with double quotes too.
38+
39+
40+
/*
41+
* Prevent various false positives.
42+
*/
43+
// These look like "forbidden" indices, but aren't.
44+
$_SERVER['PHP_"AUTH"_PW'];
45+
$_SERVER["PHP_'AUTH'_PW"];
46+
47+
// Sniff should not get confused over static OO property access.
48+
class PeopleDoSillyThings extends AllowedByPHP {
49+
public static $_SERVER = [];
50+
51+
public function show() {
52+
var_export(self::$_SERVER['PHP_AUTH_USER']);
53+
var_export(parent :: $_SERVER['PHP_AUTH_PW']);
54+
var_export(static::$_SERVER['HTTP_X_FORWARDED_FOR']);
55+
var_export(OtherClass::$_SERVER['HTTP_X_IP_TRAIL']);
56+
}
57+
}
58+
59+
// Safeguard the sniff looks at the access key for the $_SERVER variable only and doesn't walk too far.
60+
echo $_SERVER[$other_key] . $NOT_SERVER['PHP_AUTH_PW'];
61+
62+
// Safeguard the sniff doesn't get confused over partially dynamic keys.
63+
echo $_SERVER[$other_key . 'PHP_AUTH_PW'];
64+
65+
// Access of the array indices via the $GLOBALS superglobal should also be recognized.
66+
$GLOBALS['NOT_SERVER']['PHP_AUTH_USER']; // OK.
67+
$GLOBALS[$a]['PHP_AUTH_USER']; // OK.
68+
$GLOBALS['_SERVER']['SOME_OTHER_KEY']; // OK.
69+
$GLOBALS['_SERVER'][$a]; // OK.
70+
71+
$GLOBALS['_SERVER']['PHP_AUTH_USER'];
72+
$GLOBALS['_SERVER']['PHP_AUTH_PW'];
73+
$GLOBALS["_SERVER"]['HTTP_X_IP_TRAIL'];
74+
$GLOBALS['_SERVER']['HTTP_X_FORWARDED_FOR'];
75+
$GLOBALS['_SERVER']["REMOTE_ADDR"];

‎WordPressVIPMinimum/Tests/Variables/ServerVariablesUnitTest.php

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,16 @@ class ServerVariablesUnitTest extends AbstractSniffUnitTest {
2323
*/
2424
public function getErrorList() {
2525
return [
26-
4 => 1,
27-
5 => 1,
28-
6 => 1,
29-
7 => 1,
26+
31 => 1,
27+
32 => 1,
28+
33 => 1,
29+
36 => 1,
30+
37 => 1,
31+
71 => 1,
32+
72 => 1,
33+
73 => 1,
34+
74 => 1,
35+
75 => 1,
3036
];
3137
}
3238

0 commit comments

Comments
 (0)