Skip to content

Commit b225a93

Browse files
jrfnljrfnl
committed
Security/StaticStrreplace: handle more "static" tokens
This commit primarily addresses that the sniff did not take PHP 5.3 nowdocs into account as "static text" tokens. However, heredocs can also be "static text" if there is no interpolation and there are some other variants of valid code which would still make the code effectively "static text". Fixed now by checking the code more thoroughly. The actual checking has been moved to a new `is_parameter_static_text()` method as if the passed parameter is an array, the same checks would need to be done again for each array item. By having the check in a separate method, the method can recursively call itself for array items. Includes tests.
1 parent 45cb9d5 commit b225a93

File tree

3 files changed

+97
-19
lines changed

3 files changed

+97
-19
lines changed

‎WordPressVIPMinimum/Sniffs/Security/StaticStrreplaceSniff.php‎

Lines changed: 63 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,10 @@
1010
namespace WordPressVIPMinimum\Sniffs\Security;
1111

1212
use PHP_CodeSniffer\Util\Tokens;
13-
use PHPCSUtils\Exceptions\UnexpectedTokenType;
1413
use PHPCSUtils\Tokens\Collections;
14+
use PHPCSUtils\Utils\Arrays;
1515
use PHPCSUtils\Utils\PassedParameters;
16+
use PHPCSUtils\Utils\TextStrings;
1617
use WordPressCS\WordPress\AbstractFunctionParameterSniff;
1718

1819
/**
@@ -61,40 +62,83 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p
6162
return;
6263
}
6364

64-
$static_text_tokens = Tokens::$emptyTokens;
65-
$static_text_tokens[ T_CONSTANT_ENCAPSED_STRING ] = T_CONSTANT_ENCAPSED_STRING;
66-
6765
foreach ( [ $search_param, $replace_param, $subject_param ] as $param ) {
68-
$has_non_static_text = $this->phpcsFile->findNext( $static_text_tokens, $param['start'], ( $param['end'] + 1 ), true );
69-
if ( $has_non_static_text === false ) {
66+
if ( $this->is_parameter_static_text( $param ) === false ) {
67+
// Non-static text token found. Not what we're looking for.
68+
return;
69+
}
70+
}
71+
72+
$message = 'This code pattern is often used to run a very dangerous shell programs on your server. The code in these files needs to be reviewed, and possibly cleaned.';
73+
$this->phpcsFile->addError( $message, $stackPtr, 'StaticStrreplace' );
74+
}
75+
76+
/**
77+
* Check whether the current parameter, or array item, only contains tokens which should be regarded
78+
* as a valid part of a static text string.
79+
*
80+
* @param array<string, int|string> $param_info Array with information about a single parameter or array item.
81+
* Must be an array as returned via the PassedParameters class.
82+
*
83+
* @return bool
84+
*/
85+
private function is_parameter_static_text( $param_info ) {
86+
// List of tokens which can be skipped over without further examination.
87+
$static_tokens = [
88+
T_CONSTANT_ENCAPSED_STRING => T_CONSTANT_ENCAPSED_STRING,
89+
T_PLUS => T_PLUS,
90+
T_STRING_CONCAT => T_STRING_CONCAT,
91+
];
92+
$static_tokens += Tokens::$emptyTokens;
93+
94+
for ( $i = $param_info['start']; $i <= $param_info['end']; $i++ ) {
95+
$next_to_examine = $this->phpcsFile->findNext( $static_tokens, $i, ( $param_info['end'] + 1 ), true );
96+
if ( $next_to_examine === false ) {
7097
// The parameter contained only tokens which could be considered static text.
71-
continue;
98+
return true;
7299
}
73100

74-
if ( isset( Collections::arrayOpenTokensBC()[ $this->tokens[ $has_non_static_text ]['code'] ] ) ) {
75-
try {
76-
$array_items = PassedParameters::getParameters( $this->phpcsFile, $has_non_static_text );
77-
} catch ( UnexpectedTokenType $e ) {
101+
if ( isset( Collections::arrayOpenTokensBC()[ $this->tokens[ $next_to_examine ]['code'] ] ) ) {
102+
$arrayOpenClose = Arrays::getOpenClose( $this->phpcsFile, $next_to_examine );
103+
if ( $arrayOpenClose === false ) {
78104
// Short list, parse error or live coding, bow out.
79-
return;
105+
return false;
80106
}
81107

108+
$array_items = PassedParameters::getParameters( $this->phpcsFile, $next_to_examine );
82109
foreach ( $array_items as $array_item ) {
83-
$has_non_static_text = $this->phpcsFile->findNext( $static_text_tokens, $array_item['start'], $array_item['end'], true );
84-
if ( $has_non_static_text !== false ) {
85-
return;
110+
if ( $this->is_parameter_static_text( $array_item ) === false ) {
111+
return false;
86112
}
87113
}
88114

89115
// The array only contained items with tokens which could be considered static text.
116+
$i = $arrayOpenClose['closer'];
90117
continue;
91118
}
92119

93-
// Non-static text token found. Not what we're looking for.
94-
return;
120+
if ( $this->tokens[ $next_to_examine ]['code'] === T_START_HEREDOC ) {
121+
$heredoc_text = TextStrings::getCompleteTextString( $this->phpcsFile, $next_to_examine );
122+
$stripped_text = TextStrings::stripEmbeds( $heredoc_text );
123+
if ( $heredoc_text !== $stripped_text ) {
124+
// Heredoc with interpolated expression(s). Not a static text.
125+
return false;
126+
}
127+
}
128+
129+
if ( ( $this->tokens[ $next_to_examine ]['code'] === T_START_HEREDOC
130+
|| $this->tokens[ $next_to_examine ]['code'] === T_START_NOWDOC )
131+
&& isset( $this->tokens[ $next_to_examine ]['scope_closer'] )
132+
) {
133+
// No interpolation. Skip to end of a heredoc/nowdoc.
134+
$i = $this->tokens[ $next_to_examine ]['scope_closer'];
135+
continue;
136+
}
137+
138+
// Any other token means this parameter should be regarded as non-static text. Not what we're looking for.
139+
return false;
95140
}
96141

97-
$message = 'This code pattern is often used to run a very dangerous shell programs on your server. The code in these files needs to be reviewed, and possibly cleaned.';
98-
$this->phpcsFile->addError( $message, $stackPtr, 'StaticStrreplace' );
142+
return true;
99143
}
100144
}

‎WordPressVIPMinimum/Tests/Security/StaticStrreplaceUnitTest.inc‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,3 +69,33 @@ str_replace( subject: $subject, count: $count, replace: 'baz', search: 'foo', );
6969

7070
str_replace( count: $count, subject: [ 'foobar', 'foobar' ], search: 'foo', replace: 'baz', ); // Bad with different parameter order.
7171
str_replace( [ 'foo', 'bar' ], 'baz', count: $count, subject: 'foobar' ); // Bad, with mixed named and unnamed params.
72+
73+
// Make sure some variations of how plain strings can be passed are handled.
74+
str_replace( 'fo' . 'o', 'bar', 'foobar' ); // Bad.
75+
str_replace( array( 'foo', 'bar' ) + array( 'baz', 'fop' ), array( 'bar', 'foo' ), 'foobar' ); // Bad.
76+
77+
// Safeguard correct handling of PHP 5.3+ nowdocs.
78+
str_replace(
79+
'foo',
80+
'bar',
81+
<<<'EOD'
82+
foobar
83+
EOD
84+
); // Bad.
85+
86+
// Safeguard correct handling of heredocs without interpolation.
87+
str_replace( array(<<<EOD
88+
foo
89+
EOD
90+
),
91+
'bar',
92+
<<<"EOD"
93+
foobar
94+
EOD
95+
); // Bad.
96+
97+
// Ensure heredocs with interpolation are ignored.
98+
str_replace( 'foo', 'bar', <<<EOD
99+
Some text and $foobar
100+
EOD
101+
); // OK.

‎WordPressVIPMinimum/Tests/Security/StaticStrreplaceUnitTest.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,10 @@ public function getErrorList() {
3030
60 => 1,
3131
70 => 1,
3232
71 => 1,
33+
74 => 1,
34+
75 => 1,
35+
78 => 1,
36+
87 => 1,
3337
];
3438
}
3539

0 commit comments

Comments
 (0)