Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape output sniff fix for static method calls #2370

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open
20 changes: 16 additions & 4 deletions WordPress/Helpers/EscapingFunctionsTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,15 @@ final public function is_escaping_function( $functionName ) {
if ( array() === $this->allEscapingFunctions
|| $this->customEscapingFunctions !== $this->addedCustomEscapingFunctions['escape']
) {
/*
* Lowercase all names, both custom as well as "known", as PHP treats namespaced names case-insensitively.
*/
$custom_escaping_functions = array_map( 'strtolower', $this->customEscapingFunctions );
$escaping_functions = array_change_key_case( $this->escapingFunctions, \CASE_LOWER );

$this->allEscapingFunctions = RulesetPropertyHelper::merge_custom_array(
$this->customEscapingFunctions,
$this->escapingFunctions
$custom_escaping_functions,
$escaping_functions
);

$this->addedCustomEscapingFunctions['escape'] = $this->customEscapingFunctions;
Expand All @@ -242,9 +248,15 @@ final public function is_auto_escaped_function( $functionName ) {
if ( array() === $this->allAutoEscapedFunctions
|| $this->customAutoEscapedFunctions !== $this->addedCustomEscapingFunctions['autoescape']
) {
/*
* Lowercase all names, both custom as well as "known", as PHP treats namespaced names case-insensitively.
*/
$custom_auto_escaped_functions = array_map( 'strtolower', $this->customAutoEscapedFunctions );
$auto_escaped_functions = array_change_key_case( $this->autoEscapedFunctions, \CASE_LOWER );

$this->allAutoEscapedFunctions = RulesetPropertyHelper::merge_custom_array(
$this->customAutoEscapedFunctions,
$this->autoEscapedFunctions
$custom_auto_escaped_functions,
$auto_escaped_functions
);

$this->addedCustomEscapingFunctions['autoescape'] = $this->customAutoEscapedFunctions;
Expand Down
128 changes: 128 additions & 0 deletions WordPress/Sniffs/Security/EscapeOutputSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,7 @@ public function process_matched_token( $stackPtr, $group_name, $matched_content
* Check whether each relevant part of an arbitrary group of token is output escaped.
*
* @since 3.0.0 Split off from the process_token() method.
* @since 3.1.0 Add the check for static methods.
*
* @param int $start The position to start checking from.
* @param int $end The position to stop the check at.
Expand Down Expand Up @@ -738,6 +739,64 @@ protected function check_code_is_escaped( $start, $end, $code = 'OutputNotEscape

$content = $functionName;

// Check if it's static method call.
if ( $this->is_static_method_call( $i ) ) {
$fully_qualified_name = '';
$double_colon = $this->phpcsFile->findNext( \T_DOUBLE_COLON, $i, $end );
$keyword = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $double_colon + 1 ), $end, true );

// Add a namespace separator to the class name, if it exists.
if ( \T_NS_SEPARATOR === $this->tokens[ $i - 1 ]['code'] ) {
$fully_qualified_name = '\\';
}

/*
* Check what is the type of the $keyword token:
* T_STRING followed by a parenthesis opener - static method
* T_STRING without a parenthesis - enum/class constant
* T_VARIABLE - static public property
* Based on this, we will construct the content to pass to, and set the $i and the $ptr variables.
*/
if ( \T_STRING === $this->tokens[ $keyword ]['code'] ) {
$static_method_end = $this->phpcsFile->findNext( \T_CLOSE_PARENTHESIS, $i, $end );

// Enum/class constant.
if ( false === $static_method_end ) {
$constant_ptr = $this->phpcsFile->findNext( \T_STRING, $double_colon, $end );

for ( $name_start = $i; $name_start <= $constant_ptr; $name_start++ ) {
$fully_qualified_name .= $this->tokens[ $name_start ]['content'];
}
unset( $name_start );

$i = $constant_ptr;
$ptr = $i;
} else { // Static method.
for ( $name_start = $i; $name_start <= $keyword; $name_start++ ) {
$fully_qualified_name .= $this->tokens[ $name_start ]['content'];
}
unset( $name_start );

$i = $static_method_end;
$ptr = $keyword;
}
} elseif ( \T_VARIABLE === $this->tokens[ $keyword ]['code'] ) {
for ( $name_start = $i; $name_start <= $keyword; $name_start++ ) {
$fully_qualified_name .= $this->tokens[ $name_start ]['content'];
}
unset( $name_start );

$i = $keyword;
$ptr = $i;
}

// Content should be a class and a method/constant.
$content = ! empty( $fully_qualified_name ) ? trim( $fully_qualified_name ) : $content;

if ( $this->is_escaping_function( $content ) ) {
continue;
}
}
} else {
$content = $this->tokens[ $i ]['content'];
$ptr = $i;
Expand Down Expand Up @@ -900,4 +959,73 @@ private function walk_match_expression( $stackPtr, $code ) {

return $end;
}

/**
* Check if the current \T_STRING token is a part of the static method call.
*
* @since 3.1.0 Add the method to check for a static method call.
*
* @param int $stackPtr Current \T_STRING token pointer.
*
* @return bool True if it is, false if it isn't.
*/
private function is_static_method_call( $stackPtr ) {
/*
* Find the previous empty token, then inspect the token next to it (non-empty one).
* If the non-empty one is the $stackPtr, check for the double colon from it.
* If the non-empty one is a \T_NS_SEPARATOR, try see if it's a part of the FQCN, then if it is,
* check if there is a double colon after the FQCN or not.
*/
if ( \T_NS_SEPARATOR === $this->tokens[ $stackPtr - 1 ]['code'] ) { // FQCN.
$name_ptr = $this->get_fully_qualified_name_ptr( $stackPtr - 1 );

return $this->does_double_colon_exists( $name_ptr );
} else { // Imported or non-namespaced.
$name_ptr = $this->get_fully_qualified_name_ptr( $stackPtr );

return $this->does_double_colon_exists( $name_ptr );
}
}

/**
* Get the end pointer of the fully qualified class name (FQCN).
*
* Check if the next token is a \T_STRING, then if the one after one is \T_NS_SEPARATOR.
* If it's not, that's the end of the name, if it is, call the method again
* until you find the end of it and return this value.
*
* @since 3.1.0 Add the method that will get the pointer of the FQCN.
*
* @param int $stackPtr Current token pointer.
*
* @return int Stack pointer to the end of the FQCN.
*/
private function get_fully_qualified_name_ptr( $stackPtr ) {
if ( \T_STRING === $this->tokens[ $stackPtr ]['code'] ) {
if ( \T_NS_SEPARATOR === $this->tokens[ $stackPtr + 1 ]['code'] ) {
$namePtr = $this->get_fully_qualified_name_ptr( $stackPtr + 1 );
} else {
$namePtr = $stackPtr;
}
} elseif ( \T_NS_SEPARATOR === $this->tokens[ $stackPtr ]['code'] ) {
$namePtr = $this->get_fully_qualified_name_ptr( $stackPtr + 1 );
} else {
$namePtr = $stackPtr;
}

return $namePtr;
}

/**
* Check if the current token pointer is a double colon.
*
* @since 3.1.0 Add the method that checks is the token is a double colon one.
*
* @param int $stackPtr Current token pointer.
*
* @return bool True if it is, false if not.
*/
private function does_double_colon_exists( $stackPtr ) {
return false !== $stackPtr && \T_DOUBLE_COLON === $this->tokens[ $stackPtr + 1 ]['code'];
}
}
56 changes: 56 additions & 0 deletions WordPress/Tests/Security/EscapeOutputUnitTest.21.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
<?php

echo ClassName::someMethodName( $attributes, $test ); // Bad.

echo ClassName::someMethodName( $attributes, $test ), $someOtherUnescapedContent; // Bad x2.

echo ClassName::someMethodName( $attributes, $test ), esc_html( $someOtherUnescapedContent ); // Bad, but only for the static method that is not escaped.

echo $someOtherUnescapedContent, ClassName::someMethodName( $attributes, $test ), // Bad x2
esc_html( $someOtherUnescapedContent );

echo $someOtherUnescapedContent, // Bad
ClassName::someMethodName( $attributes, $test ), // Bad
esc_html( $someOtherUnescapedContent );

echo $someOtherUnescapedContent, ClassName::someMethodName( $attributes, $test ), $check_me; // Bad x3.

// These are covered in no 1 test, but should be all ok.
_deprecated_function( __METHOD__, 'x.x.x', ClassName::class ); // OK.
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.
_deprecated_function( __METHOD__, 'x.x.x', ClassName::escapingMethod() ); // Bad.
echo 'Do not use ' . $object::class ); // OK.

echo MyClass\ClassName::render( // Bad.
'testing',
ClassName::props('test-other', $attributes, [ // Should be skipped over, since the wrapping static method is not escaped.
'someName' => $attributeName
])
);

echo Example::statMethod(); // Bad.

echo \FullyQualifiedWithoutImport::statMethod(); // Bad.

echo \Namespaced\ClassName::statMethod(); // Bad.

echo Example\Namespaced\ClassName::statMethod(); // Bad.

echo \QualifiedExample\Namespaced\ClassName::statMethod(); // Bad.

echo Some\Enum\Color::ENUM_CONSTANT; // Bad.
echo Some\ClassName:: CLASS_CONSTANT; // Bad.

echo ClassName::$publicStaticProperty; // Bad

// phpcs:set WordPress.Security.EscapeOutput customEscapingFunctions[] \QualifiedExample\Namespaced\ClassName::statMethod

echo \QualifiedExample\Namespaced\ClassName::statMethod(); // Ok.
echo \QualifiedExample\Namespaced\ClassName::statmethod(); // Ok.
echo \QualifiedExample\namespaced\className::statmethoD(); // Ok.

// phpcs:set WordPress.Security.EscapeOutput customEscapingFunctions[]

echo \QualifiedExample\Namespaced\ClassName::statMethod(); // Bad.
22 changes: 22 additions & 0 deletions WordPress/Tests/Security/EscapeOutputUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,28 @@ public function getErrorList( $testFile = '' ) {
25 => 1,
);

case 'EscapeOutputUnitTest.21.inc':
return array(
3 => 1,
5 => 2,
7 => 1,
9 => 2,
12 => 1,
13 => 1,
16 => 3,
23 => 1,
26 => 1,
33 => 1,
35 => 1,
37 => 1,
39 => 1,
41 => 1,
43 => 1,
44 => 1,
46 => 1,
56 => 1,
);

default:
return array();
}
Expand Down
Loading