BadFunctions/EasyRFI: add unit tests, includes various bug fixes #72
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #57, follow up on #70, this PR adds unit tests for the
Security.BadFunctions.EasyRFI
sniff.This includes a review of the sniff and making various fixes to it to:
Commit Summary
BadFunctions/EasyRFI: add unit tests
BadFunctions/EasyRFI: efficiency fix
As the tokens to search for don't change during a PHPCS run, it is inefficient to use the "expensive"
array_merge()
function 1) within awhile
loop and 2) every time the sniff is triggered by an include/require token.The set of tokens to search for can just as easily be set only once before the sniff is ever triggered and doing so will make the sniff faster.
BadFunctions/EasyRFI: use the build-in PHPCS functionality [1]
The PHPCS
addError()
andaddWarning()
functions have a build-in string replacementsprintf()
-like functionality, so let's use it.BadFunctions/EasyRFI: use the build-in PHPCS functionality [2]
The PHPCS native
PHP_CodeSniffer\Util\Tokens
class contains a number of useful token groups.For this particular sniff, the
Tokens::$includeTokens
applies and contains all the relevant tokens.It is generally a good idea to use the build-in token groups, as when something would change in how PHP and/or PHPCS tokenizes certain constructs, those groups will be updated too.
BadFunctions/EasyRFI: bug fix - fix detecting of start/end of the statement [1]
Prevent false negatives when an
include
/require
statement combines parentheses with concatenation outside parentheses.Checking whether the next non-empty token is an open parenthesis could cause false negatives as there may be additional parts 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.
BadFunctions/EasyRFI: bug fix - fix detecting of start/end of the statement [2]
PHPCS can be run from within an IDE during live coding. Similarly PHPCS can be run over files containing parse errors.
With that in mind, it is best practice to bow out in those cases.
Parse error detection should catch those errors. That is not the responsibility of this sniff.
BadFunctions/EasyRFI: bug fix - fix detecting of start/end of the statement [3]
An
include
/require
statement can be used within template files to include another template and doesn't need a closing semi-colon in that case.That situation was so far not being considered by the sniff and the sniff could in that case search way too far and report on completely unrelated statements after the include.
BadFunctions/EasyRFI: minor code simplification [1]
Putting the
findNext()
in thewhile
condition allows to simplify theif
conditions within the loop.BadFunctions/EasyRFI: minor code simplification [2]
The only token which can have a
content
of.
is theT_STRING_CONCAT
token, so we may as well exclude it from being found.