Skip to content

Commit

Permalink
Security/EscapeOutput: add support for examining throw statements and…
Browse files Browse the repository at this point in the history
… PHP 8.0+ throw expressions

Any exception which isn't caught runs the risk of being displayed and should therefore be output escaped, along the same lines as is already required for parameters passed to `trigger_error()` function calls.

This commit add the `T_THROW` token to the tokens the sniff listens for. Parameters passed to the exception creation function call/class instantiation will be examined for being correctly escaped.

Notes:
* As custom exceptions may expect different parameters from the PHP native parameters, *all* parameters will be examined, not just the  `$message` parameter.
* When a `throw` statement is wrapped within a `try - catch` control structure, it is presumed that the exception will be caught and the `throw` statement will be ignored.
* The current logic supports a _lot_ of different ways of exception creation, but does not (yet) support variable variables for the exception name and other exotics like that.

Includes tests.

Fixes 884
  • Loading branch information
jrfnl committed Jul 28, 2023
1 parent 4ee9469 commit 90f291c
Show file tree
Hide file tree
Showing 7 changed files with 131 additions and 0 deletions.
55 changes: 55 additions & 0 deletions WordPress/Sniffs/Security/EscapeOutputSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use PHP_CodeSniffer\Util\Tokens;
use PHPCSUtils\BackCompat\BCFile;
use PHPCSUtils\Tokens\Collections;
use PHPCSUtils\Utils\Conditions;
use PHPCSUtils\Utils\Operators;
use PHPCSUtils\Utils\PassedParameters;
use PHPCSUtils\Utils\TextStrings;
Expand Down Expand Up @@ -121,6 +122,7 @@ public function register() {
$targets = parent::register();
$targets[] = \T_ECHO;
$targets[] = \T_PRINT;
$targets[] = \T_THROW;
$targets[] = \T_EXIT;
$targets[] = \T_OPEN_TAG_WITH_ECHO;

Expand Down Expand Up @@ -193,6 +195,59 @@ public function process_token( $stackPtr ) {
$end = ( $this->tokens[ $next_non_empty ]['parenthesis_closer'] + 1 );
break;

case \T_THROW:
// Find the open parentheses, while stepping over the exception creation tokens.
$ignore = Tokens::$emptyTokens;
$ignore += Collections::namespacedNameTokens();
$ignore += Collections::functionCallTokens();
$ignore += Collections::objectOperators();

$next_relevant = $this->phpcsFile->findNext( $ignore, ( $stackPtr + 1 ), null, true );
if ( false === $next_relevant ) {
return;
}

if ( \T_NEW === $this->tokens[ $next_relevant ]['code'] ) {
$next_relevant = $this->phpcsFile->findNext( $ignore, ( $next_relevant + 1 ), null, true );
if ( false === $next_relevant ) {
return;
}
}

if ( \T_OPEN_PARENTHESIS !== $this->tokens[ $next_relevant ]['code']
|| isset( $this->tokens[ $next_relevant ]['parenthesis_closer'] ) === false
) {
// Live codind/parse error or a pre-created exception. Nothing to do for us.
return;
}

$end = $this->tokens[ $next_relevant ]['parenthesis_closer'];

// Check if the throw is within a `try-catch`.
// Doing this here (instead of earlier) to allow skipping to the end of the statement.
$search_for = Collections::closedScopes();
$search_for[ \T_TRY ] = \T_TRY;

$last_condition = Conditions::getLastCondition( $this->phpcsFile, $stackPtr, $search_for );
if ( false !== $last_condition && \T_TRY === $this->tokens[ $last_condition ]['code'] ) {
// This exception will (probably) be caught, so ignore it.
return $end;
}

$call_token = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $next_relevant - 1 ), null, true );
$params = PassedParameters::getParameters( $this->phpcsFile, $call_token );
if ( empty( $params ) ) {
// No parameters passed, nothing to do.
return $end;
}

// Examine each parameter individually.
foreach ( $params as $param ) {
$this->check_code_is_escaped( $param['start'], ( $param['end'] + 1 ) );
}

return $end;

case \T_PRINT:
$end = BCFile::findEndOfStatement( $this->phpcsFile, $stackPtr );
if ( \T_COMMA !== $this->tokens[ $end ]['code']
Expand Down
45 changes: 45 additions & 0 deletions WordPress/Tests/Security/EscapeOutputUnitTest.1.inc
Original file line number Diff line number Diff line change
Expand Up @@ -539,3 +539,48 @@ die( self::CLASS . ' has been abandoned' ); // OK.
_deprecated_function( __METHOD__, 'x.x.x', parent::Class ); // OK.
_deprecated_function( __METHOD__, 'x.x.x', static::class ); // OK.
echo 'Do not use ' . $object::class ); // OK.

/*
* Examine the parameters passed for exception creation via throw.
*/
throw new MyException(); // OK.
throw new Exception( esc_html( $message ), (int) $code ); // OK.
throw new /*comment*/ parent( esc_html( $message ), (int) $code ); // OK.
throw MyException::get( esc_html( $message ), (int) $code ); // OK.
throw $obj->getException( esc_html( $message ), (int) $code ); // OK.

throw new Exception( $message, $code ); // Bad x 2.
throw new self( $message, $code ); // Bad x 2.
throw
MyException :: get /*comment*/ ( $message, $code ); // Bad x 2.
throw $obj?->getException( $message, $code ); // Bad x 2.
throw new $exceptionName( $message, $code ); // Bad x 2.

throw static::get( esc_html( $message ), $code ); // Bad x 1.
throw \Vendor\Name\MyException::get( $message, (int) $code ); // Bad x 1.
throw Name\MyException::get( esc_html( $message ), $code ); // Bad x 1.
throw namespace\MyException::get( $message, (int) $code ); // Bad x 1.
throw new class('text', 0) extends Exception {}; // OK.

// Since PHP 8.0, throw can be used as an expression.
$var = ( ( $fop === 1 ) ? throw new Exception( esc_html( $message ), (int) $code ) : $not_part_of_the_throw ); // OK.
$var = ( ( $fop === 1 ) ? throw /*comment*/ new Exception( $message ) : $not_part_of_the_throw; // Bad x 1.

// The following should be ignored as this is not exception creation, so we don't have access to the parameters.
throw $stored_exceptions['key'];
throw $obj->stored_exception;

// We should also ignore any exception which is being caught straight away.
try {
throw new Exception( $message, $code ); // OK, as exception is being caught.
} catch ( Exception $e ) {
} finally {
}

// .. but only if it is not within a closed scope nested within the try.
try {
$callback = function() {
throw new Exception( $message, 10 ); // Bad. Unclear if the exception will be caught or not.
};
} catch ( Exception $e ) {
}
5 changes: 5 additions & 0 deletions WordPress/Tests/Security/EscapeOutputUnitTest.16.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?php

// Live coding/parse error. The sniff should ignore this.
// This must be the last test in the file.
throw
5 changes: 5 additions & 0 deletions WordPress/Tests/Security/EscapeOutputUnitTest.17.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?php

// Live coding/parse error. The sniff should ignore this.
// This must be the last test in the file.
throw new
5 changes: 5 additions & 0 deletions WordPress/Tests/Security/EscapeOutputUnitTest.18.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?php

// Live coding/parse error. The sniff should ignore this.
// This must be the last test in the file.
throw new MyException(
5 changes: 5 additions & 0 deletions WordPress/Tests/Security/EscapeOutputUnitTest.19.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<?php

// Live coding/parse error. The sniff should ignore this.
// This must be the last test in the file.
throw MyException::get(
11 changes: 11 additions & 0 deletions WordPress/Tests/Security/EscapeOutputUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,17 @@ public function getErrorList( $testFile = '' ) {
527 => 1,
529 => 2,
533 => 1,
552 => 2,
553 => 2,
555 => 2,
556 => 2,
557 => 2,
559 => 1,
560 => 1,
561 => 1,
562 => 1,
567 => 1,
583 => 1,
);

case 'EscapeOutputUnitTest.6.inc':
Expand Down

0 comments on commit 90f291c

Please sign in to comment.