Skip to content

Commit 19a2131

Browse files
committed
BadFunctions/EasyRFI: bug fix - fix detecting of start/end of the statement [1]
Prevent false negatives when an `include`/`require` statement combines parentheses with concatentation outside parentheses. Checking whether the next non-empty token is an open parenthesis could cause false negatives as there may be additional paths of the path _after_ the parenthesized part of the statement. The only reason why this wasn't a problem up to now is because of a bug in the sniff determining `$s`. The `findNext()` function searches in the token stack and treats the `$start` parameter as _inclusive_. That means that when searching for the next non-empty token, passing the `$stackPtr` to an include/require statement would _always_ return the `$stackPtr` and not the next non-empty token _after_ the `$stackPtr` which could have been an open parenthesis (or not). Taking the logic related to the parentheses out of the sniff prevents this issue.
1 parent de13916 commit 19a2131

File tree

4 files changed

+7
-11
lines changed

4 files changed

+7
-11
lines changed

Security/Sniffs/BadFunctions/EasyRFISniff.php

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -38,16 +38,12 @@ public function register() {
3838
* @return void
3939
*/
4040
public function process(File $phpcsFile, $stackPtr) {
41-
$utils = \PHPCS_SecurityAudit\Security\Sniffs\UtilsFactory::getInstance();
41+
$closer = $phpcsFile->findNext(T_SEMICOLON, ($stackPtr + 1));
42+
43+
$utils = \PHPCS_SecurityAudit\Security\Sniffs\UtilsFactory::getInstance();
4244
$tokens = $phpcsFile->getTokens();
43-
$s = $phpcsFile->findNext(\PHP_CodeSniffer\Util\Tokens::$emptyTokens, $stackPtr, null, true, null, true);
45+
$s = $stackPtr;
4446

45-
if ($tokens[$s]['code'] == T_OPEN_PARENTHESIS) {
46-
$closer = $tokens[$s]['parenthesis_closer'];
47-
} else {
48-
$closer = $phpcsFile->findNext(T_SEMICOLON, $stackPtr);
49-
$s = $stackPtr;
50-
}
5147
while ($s) {
5248
$s = $phpcsFile->findNext($this->search, $s + 1, $closer, true);
5349

Security/Tests/BadFunctions/EasyRFIUnitTest.0.inc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
// Base.
88
include ( 'path/to/' . $_GET['filename'] ); // Error.
9-
include_once 'path/to/' . "$filename" . '.' . $extension;
9+
include_once ('path/to/' . "$filename") . '.' . $extension;
1010
require getenv('PATHTOFILE'); // Error.
1111

1212
// Drupal 7.

Security/Tests/BadFunctions/EasyRFIUnitTest.1.inc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
// Base.
88
include ( 'path/to/' . $_GET['filename'] ); // Error.
9-
include 'path/to/' . "$filename" . '.' . $extension; // Warning x 2.
9+
include ('path/to/' . "$filename") . '.' . $extension; // Warning x 2.
1010
include getenv('PATHTOFILE'); // Error.
1111

1212
// Drupal 7.

Security/Tests/BadFunctions/EasyRFIUnitTest.Drupal7.1.inc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
// Base.
88
include ( 'path/to/' . $_GET['filename'] ); // Error.
9-
include 'path/to/' . "$filename" . '.' . $extension; // Warning x 2.
9+
include ('path/to/' . "$filename") . '.' . $extension; // Warning x 2.
1010
include getenv('PATHTOFILE'); // Error.
1111

1212
// Drupal 7.

0 commit comments

Comments
 (0)