From f99585eef830d7701e803be979742e8692e2d1bd Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 25 Dec 2018 15:14:52 +0100 Subject: [PATCH 01/81] WPCS native PHPCS ruleset: minor tweaks The `basepath` and `parallel` options were introduced in PHPCS 3.0 and can now be enabled for the WPCS native PHPCS checking. And as WPCS 2.0.0 drops the `VIP` ruleset, we can now include the `WordPress` ruleset instead of including the `WordPress-Extra` + `WordPress-Docs` rulesets separately. --- .phpcs.xml.dist | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index 0bc9d6c10e..ef779c2dc3 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -7,19 +7,19 @@ + + /bin/class-ruleset-test.php */vendor/* - + - - From 83f445f407b49164011b782379ba74e712ff2489 Mon Sep 17 00:00:00 2001 From: Andrey Savchenko Date: Wed, 16 Jan 2019 15:48:26 +0200 Subject: [PATCH 02/81] Bumped suggested installer version --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 7e6f35545b..f306ca7bdc 100644 --- a/composer.json +++ b/composer.json @@ -24,7 +24,7 @@ "phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0" }, "suggest": { - "dealerdirect/phpcodesniffer-composer-installer": "^0.4.3 || This Composer plugin will sort out the PHPCS 'installed_paths' automatically." + "dealerdirect/phpcodesniffer-composer-installer": "^0.5.0 || This Composer plugin will sort out the PHPCS 'installed_paths' automatically." }, "minimum-stability": "RC", "scripts": { From 97167b228a7e6cb753c369a656ad9c8adf7a9f3b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 22 Jan 2019 17:14:03 +0100 Subject: [PATCH 03/81] PrefixAllGlobals: prevent false negatives for autoloaded user-defined global functions The `PrefixAllGlobals` sniff verifies that functions declared in the global namespace are prefixed with one of the whitelisted prefixes as passed to the sniff in the `prefixes` property in a custom ruleset. The sniff prevents false positives for polyfills for PHP native functions - which should be named exactly as named in PHP without prefix for them to be usable as a polyfill - by checking that the function didn't exist. Generally speaking this works fine in 95% of all cases as PHPCS is normally run stand-alone and doesn't contain any functions defined in the global namespace. Similarly, when PHPCS is installed via Composer, this would normally work fine as the commonly used `autoload` options are `PSR-0`, `PSR-4` and `classmap` which are all based on code being in classes. However, if the Composer `autoload` `files` option is used to load, for instance, a functions file declaring functions in the global namespace, this "polyfill false positive prevention" would incorrectly cause errors _not_ to be thrown for functions declared in the global namespace which were now autoloaded via Composer. I have now fixed this by, instead of using `function_exists()`, using a check against a functions list retrieved via `get_defined_functions()`. This change does not have unit tests as: * Preventing false positives for polyfills is already unit tested. * Checking that autoloaded user defined functions are not recognized as PHP native functions is not something which can be unit tested as such. This would need an integration test including a composer setup to be tested. Based on the test case provided in the issue which originally reported this, I have confirmed that this PR fixes the issue though. Fixes 1632 --- .../PrefixAllGlobalsSniff.php | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php b/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php index 1dd90a3fb8..f40894f08e 100644 --- a/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php +++ b/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php @@ -173,6 +173,18 @@ class PrefixAllGlobalsSniff extends AbstractFunctionParameterSniff { 'WP_DEFAULT_THEME' => true, ); + /** + * List of all PHP native functions. + * + * Using this list rather than a call to `function_exists()` prevents + * false negatives from user-defined functions when those would be + * autoloaded via a Composer autoload files directives. + * + * @var array + */ + private $built_in_functions; + + /** * Returns an array of tokens this test wants to listen for. * @@ -181,6 +193,11 @@ class PrefixAllGlobalsSniff extends AbstractFunctionParameterSniff { * @return array */ public function register() { + // Get a list of all PHP native functions. + $all_functions = get_defined_functions(); + $this->built_in_functions = array_flip( $all_functions['internal'] ); + + // Set the sniff targets. $targets = array( \T_NAMESPACE => \T_NAMESPACE, \T_FUNCTION => \T_FUNCTION, @@ -345,7 +362,7 @@ public function process_token( $stackPtr ) { } $item_name = $this->phpcsFile->getDeclarationName( $stackPtr ); - if ( function_exists( '\\' . $item_name ) ) { + if ( isset( $this->built_in_functions[ $item_name ] ) ) { // Backfill for PHP native function. return; } From b5acadbac6e8c0603d93d1cabad11e0292e42b96 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 23 Jan 2019 06:07:06 +0100 Subject: [PATCH 04/81] ValidatedSanitizedInput: only recognize a variable as validated if the correct variable is examined The `Sniff::is_validated()` method did not check whether the variable being validated was in actual fact the same variable as the one for which validation was needed. This could cause false negatives for the `InputNotValidated` error if an array key in, for instance, `$_GET` was validated, but that same array key was later requested - without validation - from `$_POST`. --- WordPress/Sniff.php | 4 ++++ .../Tests/Security/ValidatedSanitizedInputUnitTest.inc | 7 ++++++- .../Tests/Security/ValidatedSanitizedInputUnitTest.php | 1 + 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 721e85ecba..98830cecc4 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1772,6 +1772,10 @@ protected function is_validated( $stackPtr, $array_key = null, $in_condition_onl continue; } + if ( $this->tokens[ $stackPtr ]['content'] !== $this->tokens[ $i ]['content'] ) { + continue; + } + // If we're checking for a specific array key (ex: 'hello' in // $_POST['hello']), that must match too. Quote-style, however, doesn't matter. if ( isset( $array_key ) diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index 9a8e6c4844..d5f6eafa4f 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -85,7 +85,7 @@ echo array_map( array( $obj, 'sanitize_text_field' ), wp_unslash( $_GET['test'] $foo = (int) $_POST['foo6']; // Bad. // Non-assignment checks are OK. -if ( 'bar' === $_POST['foo'] ) {} // Ok. +if ( isset( $_POST['foo'] ) && 'bar' === $_POST['foo'] ) {} // Ok. if ( $_GET['test'] != 'a' ) {} // Ok. if ( 'bar' === do_something( wp_unslash( $_POST['foo'] ) ) ) {} // Bad. @@ -183,3 +183,8 @@ function barfoo() { if ( isset( $_POST[ 'currentid' ] ) ){ $id = (int) $_POST[ "currentid" ]; // OK. } + +// Only recognize validation if the correct superglobal is examined. +if ( isset ( $_POST['thisisnotget'] ) ) { + $get = (int) $_GET['thisisnotget']; // Bad. +} diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php index 02b7e9d4ae..49a4a09076 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php @@ -54,6 +54,7 @@ public function getErrorList() { 138 => 1, 150 => 2, 160 => 2, + 189 => 1, ); } From 44e5857686de2fedcf7b2912d84e1f33cf7c48dd Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 23 Jan 2019 06:12:42 +0100 Subject: [PATCH 05/81] ValidatedSanitizedInput: add tests validating variables with array_key_exists() --- .../ValidatedSanitizedInputUnitTest.inc | 21 +++++++++++++++++++ .../ValidatedSanitizedInputUnitTest.php | 3 +++ 2 files changed, 24 insertions(+) diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index d5f6eafa4f..da3be11da1 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -188,3 +188,24 @@ if ( isset( $_POST[ 'currentid' ] ) ){ if ( isset ( $_POST['thisisnotget'] ) ) { $get = (int) $_GET['thisisnotget']; // Bad. } + +// Recognize PHP native array_key_exists() as validation function. +if ( array_key_exists( 'my_field1', $_POST ) ) { + $id = (int) $_POST['my_field1']; // OK. +} + +if ( \array_key_exists( 'my_field2', $_POST ) ) { + $id = (int) $_POST['my_field2']; // OK. +} + +if ( \Some\ClassName\array_key_exists( 'my_field3', $_POST ) ) { + $id = (int) $_POST['my_field3']; // Bad. +} + +if ( $obj->array_key_exists( 'my_field4', $_POST ) ) { + $id = (int) $_POST['my_field4']; // Bad. +} + +if ( ClassName::array_key_exists( 'my_field5', $_POST ) ) { + $id = (int) $_POST['my_field5']; // Bad. +} diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php index 49a4a09076..46ab050e97 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php @@ -55,6 +55,9 @@ public function getErrorList() { 150 => 2, 160 => 2, 189 => 1, + 202 => 1, + 206 => 1, + 210 => 1, ); } From 4e8d27f7910d2d0601b3cdfb9f0f593faee98c54 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 23 Jan 2019 06:13:04 +0100 Subject: [PATCH 06/81] Sniff::is_in_isset_or_empty(): allow for array_key_exists() Just like, `isset()` and `empty()`, `array_key_exists()` is a way to validate a variable exists and should therefore be recognized by this function. --- WordPress/Sniff.php | 41 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 98830cecc4..53a8799ff7 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1428,9 +1428,11 @@ protected function has_nonce_check( $stackPtr ) { } /** - * Check if a token is inside of an isset() or empty() statement. + * Check if a token is inside of an isset(), empty() or array_key_exists() statement. * * @since 0.5.0 + * @since 2.0.1 Now checks for the token being used as the array parameter + * in function calls to array_key_exists() as well. * * @param int $stackPtr The index of the token in the stack. * @@ -1448,7 +1450,42 @@ protected function is_in_isset_or_empty( $stackPtr ) { $open_parenthesis = key( $nested_parenthesis ); $previous_non_empty = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $open_parenthesis - 1 ), null, true, null, true ); - return in_array( $this->tokens[ $previous_non_empty ]['code'], array( \T_ISSET, \T_EMPTY ), true ); + if ( false === $previous_non_empty ) { + return false; + } + + $previous_code = $this->tokens[ $previous_non_empty ]['code']; + if ( \T_ISSET === $previous_code || \T_EMPTY === $previous_code ) { + return true; + } + + if ( \T_STRING === $previous_code && 'array_key_exists' === $this->tokens[ $previous_non_empty ]['content'] ) { + $before_function = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $previous_non_empty - 1 ), null, true, null, true ); + + if ( false !== $before_function ) { + if ( \T_OBJECT_OPERATOR === $this->tokens[ $before_function ]['code'] + || \T_DOUBLE_COLON === $this->tokens[ $before_function ]['code'] + ) { + // Method call. + return false; + } + + if ( \T_NS_SEPARATOR === $this->tokens[ $before_function ]['code'] ) { + $before_before = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $before_function - 1 ), null, true, null, true ); + if ( false !== $before_before && \T_STRING === $this->tokens[ $before_before ]['code'] ) { + // Namespaced function call. + return false; + } + } + } + + $second_param = $this->get_function_call_parameter( $previous_non_empty, 2 ); + if ( $stackPtr >= $second_param['start'] && $stackPtr <= $second_param['end'] ) { + return true; + } + } + + return false; } /** From 5f3905a1203b60dd1173c706fef4817c3a40c2ce Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 23 Jan 2019 06:14:54 +0100 Subject: [PATCH 07/81] Sniff::is_validated(): allow for array_key_exists() Just like, `isset()` and `empty()`, `array_key_exists()` is a way to validate a variable exists and should therefore be recognized by this function. --- WordPress/Sniff.php | 97 ++++++++++++++++++++++++++++++++++++--------- 1 file changed, 78 insertions(+), 19 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 53a8799ff7..7f39609418 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1703,7 +1703,7 @@ protected function get_array_access_key( $stackPtr ) { } /** - * Check if the existence of a variable is validated with isset() or empty(). + * Check if the existence of a variable is validated with isset(), empty() or array_key_exists(). * * When $in_condition_only is false, (which is the default), this is considered * valid: @@ -1726,6 +1726,7 @@ protected function get_array_access_key( $stackPtr ) { * ``` * * @since 0.5.0 + * @since 2.0.1 Now recognizes array_key_exists() as a validation function. * * @param int $stackPtr The index of this token in the stack. * @param string $array_key An array key to check for ("bar" in $foo['bar']). @@ -1791,36 +1792,94 @@ protected function is_validated( $stackPtr, $array_key = null, $in_condition_onl } $bare_array_key = $this->strip_quotes( $array_key ); + $targets = array( + \T_ISSET => 'construct', + \T_EMPTY => 'construct', + \T_UNSET => 'construct', + \T_STRING => 'function_call', + ); // phpcs:ignore Generic.CodeAnalysis.JumbledIncrementer.Found -- On purpose, see below. for ( $i = ( $scope_start + 1 ); $i < $scope_end; $i++ ) { - if ( ! \in_array( $this->tokens[ $i ]['code'], array( \T_ISSET, \T_EMPTY, \T_UNSET ), true ) ) { + if ( isset( $targets[ $this->tokens[ $i ]['code'] ] ) === false ) { continue; } - $issetOpener = $this->phpcsFile->findNext( \T_OPEN_PARENTHESIS, $i ); - $issetCloser = $this->tokens[ $issetOpener ]['parenthesis_closer']; + switch ( $targets[ $this->tokens[ $i ]['code'] ] ) { + case 'construct': + $issetOpener = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true, null, true ); + if ( false === $issetOpener || \T_OPEN_PARENTHESIS !== $this->tokens[ $issetOpener ]['code'] ) { + // Parse error or live coding. + continue 2; + } - // Look for this variable. We purposely stomp $i from the parent loop. - for ( $i = ( $issetOpener + 1 ); $i < $issetCloser; $i++ ) { + $issetCloser = $this->tokens[ $issetOpener ]['parenthesis_closer']; - if ( \T_VARIABLE !== $this->tokens[ $i ]['code'] ) { - continue; - } + // Look for this variable. We purposely stomp $i from the parent loop. + for ( $i = ( $issetOpener + 1 ); $i < $issetCloser; $i++ ) { - if ( $this->tokens[ $stackPtr ]['content'] !== $this->tokens[ $i ]['content'] ) { - continue; - } + if ( \T_VARIABLE !== $this->tokens[ $i ]['code'] ) { + continue; + } - // If we're checking for a specific array key (ex: 'hello' in - // $_POST['hello']), that must match too. Quote-style, however, doesn't matter. - if ( isset( $array_key ) - && $this->strip_quotes( $this->get_array_access_key( $i ) ) !== $bare_array_key ) { - continue; - } + if ( $this->tokens[ $stackPtr ]['content'] !== $this->tokens[ $i ]['content'] ) { + continue; + } - return true; + // If we're checking for a specific array key (ex: 'hello' in + // $_POST['hello']), that must match too. Quote-style, however, doesn't matter. + if ( isset( $array_key ) + && $this->strip_quotes( $this->get_array_access_key( $i ) ) !== $bare_array_key ) { + continue; + } + + return true; + } + + break; + + case 'function_call': + // Only check calls to array_key_exists(). + if ( 'array_key_exists' !== $this->tokens[ $i ]['content'] ) { + continue 2; + } + + $next_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true, null, true ); + if ( false === $next_non_empty || \T_OPEN_PARENTHESIS !== $this->tokens[ $next_non_empty ]['code'] ) { + // Not a function call. + continue 2; + } + + $previous_non_empty = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $i - 1 ), null, true, null, true ); + if ( false !== $previous_non_empty ) { + if ( \T_OBJECT_OPERATOR === $this->tokens[ $previous_non_empty ]['code'] + || \T_DOUBLE_COLON === $this->tokens[ $previous_non_empty ]['code'] + ) { + // Method call. + continue 2; + } + + if ( \T_NS_SEPARATOR === $this->tokens[ $previous_non_empty ]['code'] ) { + $pprev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $previous_non_empty - 1 ), null, true, null, true ); + if ( false !== $pprev && \T_STRING === $this->tokens[ $pprev ]['code'] ) { + // Namespaced function call. + continue 2; + } + } + } + + $params = $this->get_function_call_parameters( $i ); + if ( $params[2]['raw'] !== $this->tokens[ $stackPtr ]['content'] ) { + continue 2; + } + + if ( isset( $array_key ) + && $this->strip_quotes( $params[1]['raw'] ) !== $bare_array_key ) { + continue 2; + } + + return true; } } From 71ace5641638b607cfc080ebd312acbf5ace64e7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 28 Jan 2019 16:51:06 +0100 Subject: [PATCH 08/81] Sniff::$test_class_whitelist: add newly added base test class to the list See: https://core.trac.wordpress.org/changeset/44701 --- WordPress/Sniff.php | 1 + 1 file changed, 1 insertion(+) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 98830cecc4..69fa88726a 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -770,6 +770,7 @@ abstract class Sniff implements PHPCS_Sniff { * @var string[] */ protected $test_class_whitelist = array( + 'WP_UnitTestCase_Base' => true, 'WP_UnitTestCase' => true, 'WP_Ajax_UnitTestCase' => true, 'WP_Canonical_UnitTestCase' => true, From ca209e78e1ea40970c1282fd57e7f9cc2ce328f6 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 11 Feb 2019 17:46:35 +0100 Subject: [PATCH 09/81] Travis: test builds against PHP 7.4 Nightly has become PHP 8.0 since PHP 7.4 has been branched, so to continue to also test against PHP 7.4, it needs to be added separately. Refs: * https://twitter.com/nikita_ppv/status/1089839541828112384 * https://twitter.com/nikita_ppv/status/1094897743594770433 --- .travis.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.travis.yml b/.travis.yml index 64fe800947..9fd375f3de 100644 --- a/.travis.yml +++ b/.travis.yml @@ -21,6 +21,7 @@ php: - 7.1 - 7.2 - 7.3 + - "7.4snapshot" - nightly env: @@ -42,6 +43,7 @@ matrix: allow_failures: # Allow failures for unstable builds. + - php: "7.4snapshot" - php: nightly before_install: From 4426a06ce160dbd807a113e9682ec2625995eb71 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 11 Feb 2019 17:53:17 +0100 Subject: [PATCH 10/81] Travis: work around PHPUnit 8.x on PHP >= 7.2 images As of recently, the Travis images for PHP 7.2+ ship with PHPUnit 8.x. The PHPCS native test framework is not compatible with PHPUnit 8.x and won't be able to be made compatible with it until the minimum PHP version would be raised to PHP 7.1. So for the unit tests to be able to run on PHP 7.2+, we need to explicitly require PHPUnit 7.x for those builds. This has been implemented in the same way as a similar requirement was previously implemented fo HHVM. As for nightly: there is no PHPUnit version which is currently compatible with PHP 8. As that either mean that the builds for `nightly` would always fail or - if the unit tests would be skipped -, the only check executed on `nightly` would be linting the files, I've elected to remove build testing against `nightly` for the time being. --- .travis.yml | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 9fd375f3de..a272e7ef9d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -22,7 +22,6 @@ php: - 7.2 - 7.3 - "7.4snapshot" - - nightly env: # `master` is now 3.x. @@ -44,7 +43,6 @@ matrix: allow_failures: # Allow failures for unstable builds. - php: "7.4snapshot" - - php: nightly before_install: # Speed up build time by disabling Xdebug. @@ -62,12 +60,20 @@ before_install: # The above require already does the install. $(pwd)/vendor/bin/phpcs --config-set installed_paths $(pwd) fi + # Download PHPUnit 7.x for builds on PHP >= 7.2 as the PHPCS + # test suite is currently not compatible with PHPUnit 8.x. + - if [[ ${TRAVIS_PHP_VERSION:0:3} > "7.1" ]]; then wget -P $PHPUNIT_DIR https://phar.phpunit.de/phpunit-7.phar && chmod +x $PHPUNIT_DIR/phpunit-7.phar; fi script: # Lint the PHP files against parse errors. - if [[ "$LINT" == "1" ]]; then if find . -path ./vendor -prune -o -path ./bin -prune -o -name "*.php" -exec php -l {} \; | grep "^[Parse error|Fatal error]"; then exit 1; fi; fi # Run the unit tests. - - phpunit --filter WordPress --bootstrap="$(pwd)/vendor/squizlabs/php_codesniffer/tests/bootstrap.php" $(pwd)/vendor/squizlabs/php_codesniffer/tests/AllTests.php + - | + if [[ ${TRAVIS_PHP_VERSION:0:3} > "7.1" ]]; then + php $PHPUNIT_DIR/phpunit-7.phar --filter WordPress --bootstrap="$(pwd)/vendor/squizlabs/php_codesniffer/tests/bootstrap.php" $(pwd)/vendor/squizlabs/php_codesniffer/tests/AllTests.php + else + phpunit --filter WordPress --bootstrap="$(pwd)/vendor/squizlabs/php_codesniffer/tests/bootstrap.php" $(pwd)/vendor/squizlabs/php_codesniffer/tests/AllTests.php + fi # Test for fixer conflicts by running the auto-fixers of the complete WPCS over the test case files. # This is not an exhaustive test, but should give an early indication for typical fixer conflicts. # For the first run, the exit code will be 1 (= all fixable errors fixed). From 7529b4d5bb9fa67f3a3f471db1710baafa0233e7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 13 Feb 2019 04:20:32 +0100 Subject: [PATCH 11/81] WP.EnqueuedResources: bug fix Identified by joyously in the ThemeReviewCS repo: WPTRT/WPThemeReview 202 When inline JQuery is used to reference a stylesheet link tag, the sniff misidentifies this as a stylesheet which needs to be enqueued. This very simple fix should prevent that, at least for the reported cases. Includes unit test. --- WordPress/Sniffs/WP/EnqueuedResourcesSniff.php | 2 +- WordPress/Tests/WP/EnqueuedResourcesUnitTest.inc | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniffs/WP/EnqueuedResourcesSniff.php b/WordPress/Sniffs/WP/EnqueuedResourcesSniff.php index 1473eac208..e8d0333ad4 100644 --- a/WordPress/Sniffs/WP/EnqueuedResourcesSniff.php +++ b/WordPress/Sniffs/WP/EnqueuedResourcesSniff.php @@ -44,7 +44,7 @@ public function register() { public function process_token( $stackPtr ) { $token = $this->tokens[ $stackPtr ]; - if ( preg_match( '#rel=\\\\?[\'"]?stylesheet\\\\?[\'"]?#', $token['content'] ) > 0 ) { + if ( preg_match( '# rel=\\\\?[\'"]?stylesheet\\\\?[\'"]?#', $token['content'] ) > 0 ) { $this->phpcsFile->addError( 'Stylesheets must be registered/enqueued via wp_enqueue_style', $stackPtr, diff --git a/WordPress/Tests/WP/EnqueuedResourcesUnitTest.inc b/WordPress/Tests/WP/EnqueuedResourcesUnitTest.inc index 941eacd779..3b5ec4f191 100644 --- a/WordPress/Tests/WP/EnqueuedResourcesUnitTest.inc +++ b/WordPress/Tests/WP/EnqueuedResourcesUnitTest.inc @@ -30,3 +30,9 @@ $head = <<<'EOD' EOD; + +?> + +jQuery( document ).ready( function() { + $('link[rel="stylesheet"]:not([data-inprogress])').forEach(StyleFix.link); +}); From 0f425fbeb303893358516d2855b342e4aac58dbf Mon Sep 17 00:00:00 2001 From: Stephen Edgar Date: Fri, 15 Feb 2019 14:02:36 +1100 Subject: [PATCH 12/81] Remove `sudo: false` --- .travis.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index a272e7ef9d..bb60dd86aa 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,3 @@ -sudo: false - dist: trusty cache: From 76011f6d214ccd439b93ea2ec4de248bf33263be Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 18 Feb 2019 22:06:30 +0100 Subject: [PATCH 13/81] PrefixAllGlobals: improve error message clarity When people run the sniffs without the `-s` option, the messages for some of the errors in this sniff were not specific enough as the "Prefix**AllGlobals**" part would not be seen. I considered adjusting the error message template `ERROR_MSG`, however, this would cause more confusion as, for instance, namespace declarations will **always** be in the global namespace. So, I've opted to adjust select error messages instead. --- .../Sniffs/NamingConventions/PrefixAllGlobalsSniff.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php b/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php index f40894f08e..de46e0bdae 100644 --- a/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php +++ b/WordPress/Sniffs/NamingConventions/PrefixAllGlobalsSniff.php @@ -367,7 +367,7 @@ public function process_token( $stackPtr ) { return; } - $error_text = 'Functions declared'; + $error_text = 'Functions declared in the global namespace'; $error_code = 'NonPrefixedFunctionFound'; break; @@ -555,7 +555,7 @@ protected function process_variable_variable( $stackPtr ) { $stackPtr, 'NonPrefixedVariableFound', array( - 'Variables defined', + 'Global variables defined', $variable_name, ) ); @@ -705,7 +705,7 @@ protected function process_variable_assignment( $stackPtr ) { $is_error, 'NonPrefixedVariableFound', array( - 'Variables defined', + 'Global variables defined', '$' . $variable_name, ) ); From a392d28c878f1be470e082c0cdb9d20c6298daa3 Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Wed, 20 Feb 2019 13:08:16 +0100 Subject: [PATCH 14/81] Fixes #1447, adds new sniff for blacklisted ini_set directives. --- .../PHP/DiscouragedPHPFunctionsSniff.php | 1 - WordPress/Sniffs/PHP/IniSetSniff.php | 173 ++++++++++++++++++ .../PHP/DiscouragedPHPFunctionsUnitTest.inc | 1 - WordPress/Tests/PHP/IniSetUnitTest.inc | 28 +++ WordPress/Tests/PHP/IniSetUnitTest.php | 61 ++++++ 5 files changed, 262 insertions(+), 2 deletions(-) create mode 100644 WordPress/Sniffs/PHP/IniSetSniff.php create mode 100644 WordPress/Tests/PHP/IniSetUnitTest.inc create mode 100644 WordPress/Tests/PHP/IniSetUnitTest.php diff --git a/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php b/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php index f7d613df83..62102f3c39 100644 --- a/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php +++ b/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php @@ -61,7 +61,6 @@ public function getGroups() { 'error_reporting', 'ini_alter', 'ini_restore', - 'ini_set', 'apache_setenv', 'putenv', 'set_include_path', diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php new file mode 100644 index 0000000000..40188062bb --- /dev/null +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -0,0 +1,173 @@ + array(), + ); + + /** + * Array of options that are allowed to be manipulated + * + * @since x.x.x + * + * @var array Multidimensional array with parameter details. + * $whitelisted_options = array( + * (string) option name. = array( + * (string[]) 'valid_values' = array() + * ) + * ); + */ + protected $whitelisted_options = array( + 'auto_detect_line_endings'=>array(), + 'highlight.bg'=>array(), + 'highlight.comment'=>array(), + 'highlight.default'=>array(), + 'highlight.html'=>array(), + 'highlight.keyword'=>array(), + 'highlight.string'=>array(), + 'short_open_tag'=>array( + 'valid_values'=>array('true', '1', 'On') + ), + ); + + /** + * Array of options that are not allowed to be manipulated + * + * @since x.x.x + * + * @var array Multidimensional array with parameter details. + * $blacklisted_options = array( + * (string) option name. = array( + * (string[]) 'invalid_values' = array() + * (string) 'message' + * ) + * ); + */ + protected $blacklisted_options = array( + 'max_execution_time'=>array( + 'message'=>'Use `set_time_limit()` instead.' + ), + 'short_open_tag'=>array( + 'invalid_values'=>array('false', '0', 'Off'), + 'message'=>'Turning off short_open_tag is prohibited as it might possibily break other plugins.', + ), + 'bcmath.scale'=>array( + 'message'=>'Use `bcscale()` instead.' + ), + 'display_errors'=>array( + 'message'=>'Use `WP_DEBUG_DISPLAY` instead.' + ), + 'error_reporting'=>array( + 'message'=>'Use `WP_DEBUG` instead.' + ), + 'filter.default'=>array( + 'message'=>'Use the filter flag constants when calling the functions instead (as you will possibly break other plugins if you change this).' + ), + 'filter.default_flags'=>array( + 'message'=>'Use the filter flag constants when calling the functions instead (as you will possibly break other plugins if you change this).' + ), + 'iconv.input_encoding'=>array( + 'message'=>'PHP < 5.6 only - use `iconv_set_encoding()` instead.' + ), + 'iconv.internal_encoding'=>array( + 'message'=>'PHP < 5.6 only - use `iconv_set_encoding()` instead.' + ), + 'iconv.output_encoding'=>array( + 'message'=>'PHP < 5.6 only - use `iconv_set_encoding()` instead.' + ), + 'ignore_user_abort'=>array( + 'message'=>'Use `ignore_user_abort()` instead.' + ), + 'log_errors'=>array( + 'message'=>'Use `WP_DEBUG_LOG` instead.' + ), + 'memory_limit'=>array( + 'message'=>'Use `wp_raise_memory_limit()` or hook into the filters in that function.' + ), + ); + + /** + * Process the parameter of a matched function. Errors if an option + * is found in the blacklist. Warns as 'risky' when the option is not + * found in the whitelist. + * + * @since x.x.x + * + * @param int $stackPtr The position of the current token in the stack. + * @param array $group_name The name of the group which was matched. + * @param string $matched_content The token content (function name) which was matched. + * @param array $parameters Array with information about the parameters. + * + * @return void + */ + public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ){ + $ini_set_function = $this->tokens[ $stackPtr ]; + $option_name = $this->strip_quotes($parameters[1]['raw']); + $option_value = $this->strip_quotes($parameters[2]['raw']); + if(array_key_exists($option_name, $this->whitelisted_options)){ + $whitelisted_option = $this->whitelisted_options[$option_name]; + if(!isset($whitelisted_option['valid_values']) || in_array($option_value, $whitelisted_option['valid_values'], true) ){ + return; + } + } + + if(array_key_exists($option_name, $this->blacklisted_options)){ + $blacklisted_option = $this->blacklisted_options[$option_name]; + if(!isset($blacklisted_option['invalid_values']) || in_array($option_value, $blacklisted_option['invalid_values'], true) ){ + $this->phpcsFile->addError( + '%s(%s, %s) found. %s', + $stackPtr, + 'Blacklisted', + array( + $ini_set_function['content'], + $parameters[1]['raw'], + $parameters[2]['raw'], + $blacklisted_option['message'] + ) + ); + return; + } + } + $this->phpcsFile->addWarning( + '%s(%s, %s) found. Changing these configuration values at runtime is rarely necessary.', + $stackPtr, + 'Risky', + array( + $ini_set_function['content'], + $parameters[1]['raw'], + $parameters[2]['raw'] + ) + ); + return; + } +} diff --git a/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.inc b/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.inc index 792b0cad31..5a6e9a5577 100644 --- a/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.inc +++ b/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.inc @@ -16,7 +16,6 @@ dl(); // Warning. error_reporting(); // Warning. ini_alter(); // Warning. ini_restore(); // Warning. -ini_set(); // Warning. magic_quotes_runtime(); // Warning. set_magic_quotes_runtime(); // Warning. apache_setenv(); // Warning. diff --git a/WordPress/Tests/PHP/IniSetUnitTest.inc b/WordPress/Tests/PHP/IniSetUnitTest.inc new file mode 100644 index 0000000000..ef198be6a6 --- /dev/null +++ b/WordPress/Tests/PHP/IniSetUnitTest.inc @@ -0,0 +1,28 @@ + => + */ + public function getErrorList() { + return array( + 12 => 1, + 13 => 1, + 14 => 1, + 15 => 1, + 16 => 1, + 17 => 1, + 18 => 1, + 19 => 1, + 20 => 1, + 21 => 1, + 22 => 1, + 23 => 1, + 24 => 1, + 25 => 1, + ); + } + + /** + * Returns the lines where warnings should occur. + * + * @return array => + */ + public function getWarningList() { + return array( + 27 => 1, + 28 => 1, + ); + } + +} From bb93c38c29d9f16bed536050577cad8bf9392cf2 Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Wed, 20 Feb 2019 13:21:27 +0100 Subject: [PATCH 15/81] Fixes unit-test for removed ini_set check --- WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.php b/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.php index db73fe44bd..5470314fb6 100644 --- a/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.php +++ b/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.php @@ -50,18 +50,17 @@ public function getWarningList() { 22 => 1, 23 => 1, 24 => 1, - 25 => 1, + 27 => 1, 28 => 1, 29 => 1, 30 => 1, 31 => 1, - 32 => 1, + 34 => 1, 35 => 1, 36 => 1, 37 => 1, 38 => 1, 39 => 1, - 40 => 1, ); } From 1650b77d0e662dc8daea3e9aaeec5c2eb0ab1eba Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Wed, 20 Feb 2019 14:32:45 +0100 Subject: [PATCH 16/81] Fixes codestyle --- WordPress/Sniffs/PHP/IniSetSniff.php | 94 ++++++++++++++-------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index 40188062bb..f4c5475c9a 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -48,15 +48,15 @@ class IniSetSniff extends AbstractFunctionParameterSniff { * ); */ protected $whitelisted_options = array( - 'auto_detect_line_endings'=>array(), - 'highlight.bg'=>array(), - 'highlight.comment'=>array(), - 'highlight.default'=>array(), - 'highlight.html'=>array(), - 'highlight.keyword'=>array(), - 'highlight.string'=>array(), - 'short_open_tag'=>array( - 'valid_values'=>array('true', '1', 'On') + 'auto_detect_line_endings' => array(), + 'highlight.bg' => array(), + 'highlight.comment' => array(), + 'highlight.default' => array(), + 'highlight.html' => array(), + 'highlight.keyword' => array(), + 'highlight.string' => array(), + 'short_open_tag' => array( + 'valid_values' => array( 'true', '1', 'On' ), ), ); @@ -74,45 +74,45 @@ class IniSetSniff extends AbstractFunctionParameterSniff { * ); */ protected $blacklisted_options = array( - 'max_execution_time'=>array( - 'message'=>'Use `set_time_limit()` instead.' + 'max_execution_time' => array( + 'message' => 'Use `set_time_limit()` instead.', ), - 'short_open_tag'=>array( - 'invalid_values'=>array('false', '0', 'Off'), - 'message'=>'Turning off short_open_tag is prohibited as it might possibily break other plugins.', + 'short_open_tag' => array( + 'invalid_values' => array( 'false', '0', 'Off' ), + 'message' => 'Turning off short_open_tag is prohibited as it might possibily break other plugins.', ), - 'bcmath.scale'=>array( - 'message'=>'Use `bcscale()` instead.' + 'bcmath.scale' => array( + 'message' => 'Use `bcscale()` instead.', ), - 'display_errors'=>array( - 'message'=>'Use `WP_DEBUG_DISPLAY` instead.' + 'display_errors' => array( + 'message' => 'Use `WP_DEBUG_DISPLAY` instead.', ), - 'error_reporting'=>array( - 'message'=>'Use `WP_DEBUG` instead.' + 'error_reporting' => array( + 'message' => 'Use `WP_DEBUG` instead.', ), - 'filter.default'=>array( - 'message'=>'Use the filter flag constants when calling the functions instead (as you will possibly break other plugins if you change this).' + 'filter.default' => array( + 'message' => 'Use the filter flag constants when calling the functions instead (as you will possibly break other plugins if you change this).', ), - 'filter.default_flags'=>array( - 'message'=>'Use the filter flag constants when calling the functions instead (as you will possibly break other plugins if you change this).' + 'filter.default_flags' => array( + 'message' => 'Use the filter flag constants when calling the functions instead (as you will possibly break other plugins if you change this).', ), - 'iconv.input_encoding'=>array( - 'message'=>'PHP < 5.6 only - use `iconv_set_encoding()` instead.' + 'iconv.input_encoding' => array( + 'message' => 'PHP < 5.6 only - use `iconv_set_encoding()` instead.', ), - 'iconv.internal_encoding'=>array( - 'message'=>'PHP < 5.6 only - use `iconv_set_encoding()` instead.' + 'iconv.internal_encoding' => array( + 'message' => 'PHP < 5.6 only - use `iconv_set_encoding()` instead.', ), - 'iconv.output_encoding'=>array( - 'message'=>'PHP < 5.6 only - use `iconv_set_encoding()` instead.' + 'iconv.output_encoding' => array( + 'message' => 'PHP < 5.6 only - use `iconv_set_encoding()` instead.', ), - 'ignore_user_abort'=>array( - 'message'=>'Use `ignore_user_abort()` instead.' + 'ignore_user_abort' => array( + 'message' => 'Use `ignore_user_abort()` instead.', ), - 'log_errors'=>array( - 'message'=>'Use `WP_DEBUG_LOG` instead.' + 'log_errors' => array( + 'message' => 'Use `WP_DEBUG_LOG` instead.', ), - 'memory_limit'=>array( - 'message'=>'Use `wp_raise_memory_limit()` or hook into the filters in that function.' + 'memory_limit' => array( + 'message' => 'Use `wp_raise_memory_limit()` or hook into the filters in that function.', ), ); @@ -130,20 +130,20 @@ class IniSetSniff extends AbstractFunctionParameterSniff { * * @return void */ - public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ){ + public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) { $ini_set_function = $this->tokens[ $stackPtr ]; - $option_name = $this->strip_quotes($parameters[1]['raw']); - $option_value = $this->strip_quotes($parameters[2]['raw']); - if(array_key_exists($option_name, $this->whitelisted_options)){ - $whitelisted_option = $this->whitelisted_options[$option_name]; - if(!isset($whitelisted_option['valid_values']) || in_array($option_value, $whitelisted_option['valid_values'], true) ){ + $option_name = $this->strip_quotes( $parameters[1]['raw'] ); + $option_value = $this->strip_quotes( $parameters[2]['raw'] ); + if ( array_key_exists( $option_name, $this->whitelisted_options ) ) { + $whitelisted_option = $this->whitelisted_options[ $option_name ]; + if ( ! isset( $whitelisted_option['valid_values'] ) || in_array( $option_value, $whitelisted_option['valid_values'], true ) ) { return; } } - if(array_key_exists($option_name, $this->blacklisted_options)){ - $blacklisted_option = $this->blacklisted_options[$option_name]; - if(!isset($blacklisted_option['invalid_values']) || in_array($option_value, $blacklisted_option['invalid_values'], true) ){ + if ( array_key_exists( $option_name, $this->blacklisted_options ) ) { + $blacklisted_option = $this->blacklisted_options[ $option_name ]; + if ( ! isset( $blacklisted_option['invalid_values'] ) || in_array( $option_value, $blacklisted_option['invalid_values'], true ) ) { $this->phpcsFile->addError( '%s(%s, %s) found. %s', $stackPtr, @@ -152,7 +152,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $ini_set_function['content'], $parameters[1]['raw'], $parameters[2]['raw'], - $blacklisted_option['message'] + $blacklisted_option['message'], ) ); return; @@ -165,7 +165,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p array( $ini_set_function['content'], $parameters[1]['raw'], - $parameters[2]['raw'] + $parameters[2]['raw'], ) ); return; From 55c08d7b6ec4be992792f2c7c9361802d8a7d243 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 20 Feb 2019 15:35:34 +0100 Subject: [PATCH 17/81] AlternativeFunctions: allow for php://input used by file_get_contents() Reported by remcotolsma in 49e7700073b7486077c95b37afe55d129b3e4881#commitcomment-32391794 While a little esoteric in a WP context, using the WP File Wrapper functions is not a suitable alternative when `php://input` is being read. Includes unit test. --- WordPress/Sniffs/WP/AlternativeFunctionsSniff.php | 8 +++++++- WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php b/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php index 68092de5d0..675189b9b9 100644 --- a/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php +++ b/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php @@ -202,7 +202,13 @@ public function process_matched_token( $stackPtr, $group_name, $matched_content return; } - unset( $params ); + $raw_stripped = $this->strip_quotes( $params[1]['raw'] ); + if ( 'php://input' === $raw_stripped ) { + // This is not a file, but the read-only raw data stream from the request body. + return; + } + + unset( $params, $raw_stripped ); break; } diff --git a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc index ca2989c033..0f6974dcb6 100644 --- a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc +++ b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc @@ -50,3 +50,4 @@ file_get_contents(MYABSPATH . 'plugin-file.json'); // Warning. file_get_contents( MUPLUGINDIR . 'some-file.xml' ); // OK. file_get_contents( plugin_dir_path( __FILE__ ) . 'subfolder/*.conf' ); // OK. file_get_contents(WP_Upload_Dir()['path'] . 'subdir/file.inc'); // OK. +file_get_contents( 'php://input' ); // OK. From d3536cfbb4273220af59884ab8a5fa909885a654 Mon Sep 17 00:00:00 2001 From: Juliette <663378+jrfnl@users.noreply.github.com> Date: Wed, 20 Feb 2019 21:05:43 +0100 Subject: [PATCH 18/81] Update WordPress/Sniffs/PHP/IniSetSniff.php Co-Authored-By: NielsdeBlaauw --- WordPress/Sniffs/PHP/IniSetSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index f4c5475c9a..b8ccafac63 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -159,7 +159,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p } } $this->phpcsFile->addWarning( - '%s(%s, %s) found. Changing these configuration values at runtime is rarely necessary.', + '%s(%s, %s) found. Changing configuration values at runtime is rarely necessary.', $stackPtr, 'Risky', array( From 18a5cccb909b772ca491cf620df4914251608ce5 Mon Sep 17 00:00:00 2001 From: Juliette <663378+jrfnl@users.noreply.github.com> Date: Wed, 20 Feb 2019 21:05:51 +0100 Subject: [PATCH 19/81] Update WordPress/Tests/PHP/IniSetUnitTest.php Co-Authored-By: NielsdeBlaauw --- WordPress/Tests/PHP/IniSetUnitTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Tests/PHP/IniSetUnitTest.php b/WordPress/Tests/PHP/IniSetUnitTest.php index 09ebff5095..b09e2f81d9 100644 --- a/WordPress/Tests/PHP/IniSetUnitTest.php +++ b/WordPress/Tests/PHP/IniSetUnitTest.php @@ -12,7 +12,7 @@ use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest; /** - * Unit test class for the DontExtract sniff. + * Unit test class for the IniSet sniff. * * @package WPCS\WordPressCodingStandards * From 8d7593e9eb0804896da53f241f224f29215b432c Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Wed, 20 Feb 2019 21:17:12 +0100 Subject: [PATCH 20/81] Adds ini_alter checks --- WordPress/Sniff.php | 118 +++++++++--------- .../PHP/DiscouragedPHPFunctionsSniff.php | 1 - WordPress/Sniffs/PHP/IniSetSniff.php | 4 +- .../PHP/DiscouragedPHPFunctionsUnitTest.inc | 2 +- .../PHP/DiscouragedPHPFunctionsUnitTest.php | 1 - WordPress/Tests/PHP/IniSetUnitTest.inc | 4 + WordPress/Tests/PHP/IniSetUnitTest.php | 2 + 7 files changed, 68 insertions(+), 64 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 7d5de2c044..4d72469145 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -175,65 +175,65 @@ abstract class Sniff implements PHPCS_Sniff { * @var array */ protected $autoEscapedFunctions = array( - 'allowed_tags' => true, - 'bloginfo' => true, - 'body_class' => true, - 'calendar_week_mod' => true, - 'category_description' => true, - 'checked' => true, - 'comment_class' => true, - 'count' => true, - 'disabled' => true, - 'do_shortcode' => true, - 'do_shortcode_tag' => true, - 'get_archives_link' => true, - 'get_attachment_link' => true, - 'get_avatar' => true, - 'get_bookmark_field' => true, - 'get_calendar' => true, - 'get_comment_author_link' => true, - 'get_current_blog_id' => true, - 'get_delete_post_link' => true, - 'get_search_form' => true, - 'get_search_query' => true, - 'get_the_author_link' => true, - 'get_the_author' => true, - 'get_the_date' => true, - 'get_the_ID' => true, - 'get_the_post_thumbnail' => true, - 'get_the_term_list' => true, - 'paginate_comments_links' => true, - 'post_type_archive_title' => true, - 'readonly' => true, - 'selected' => true, - 'single_cat_title' => true, - 'single_month_title' => true, - 'single_post_title' => true, - 'single_tag_title' => true, - 'single_term_title' => true, - 'tag_description' => true, - 'term_description' => true, - 'the_author' => true, - 'the_date' => true, - 'the_title_attribute' => true, - 'walk_nav_menu_tree' => true, - 'wp_dropdown_categories' => true, - 'wp_dropdown_users' => true, - 'wp_generate_tag_cloud' => true, - 'wp_get_archives' => true, - 'wp_get_attachment_image' => true, - 'wp_get_attachment_link' => true, - 'wp_link_pages' => true, - 'wp_list_authors' => true, - 'wp_list_bookmarks' => true, - 'wp_list_categories' => true, - 'wp_list_comments' => true, - 'wp_login_form' => true, - 'wp_loginout' => true, - 'wp_nav_menu' => true, - 'wp_register' => true, - 'wp_tag_cloud' => true, - 'wp_title' => true, + 'allowed_tags' => true, + 'bloginfo' => true, + 'body_class' => true, + 'calendar_week_mod' => true, + 'category_description' => true, + 'checked' => true, + 'comment_class' => true, + 'count' => true, + 'disabled' => true, + 'do_shortcode' => true, + 'do_shortcode_tag' => true, + 'get_archives_link' => true, + 'get_attachment_link' => true, + 'get_avatar' => true, + 'get_bookmark_field' => true, + 'get_calendar' => true, + 'get_comment_author_link' => true, + 'get_current_blog_id' => true, + 'get_delete_post_link' => true, + 'get_search_form' => true, + 'get_search_query' => true, + 'get_the_author_link' => true, + 'get_the_author' => true, + 'get_the_date' => true, + 'get_the_ID' => true, + 'get_the_post_thumbnail' => true, + 'get_the_term_list' => true, + 'paginate_comments_links' => true, + 'post_type_archive_title' => true, + 'readonly' => true, + 'selected' => true, + 'single_cat_title' => true, + 'single_month_title' => true, + 'single_post_title' => true, + 'single_tag_title' => true, + 'single_term_title' => true, + 'tag_description' => true, + 'term_description' => true, + 'the_author' => true, + 'the_date' => true, + 'the_title_attribute' => true, + 'walk_nav_menu_tree' => true, + 'wp_dropdown_categories' => true, + 'wp_dropdown_users' => true, + 'wp_generate_tag_cloud' => true, + 'wp_get_archives' => true, + 'wp_get_attachment_image' => true, + 'wp_get_attachment_link' => true, + 'wp_link_pages' => true, + 'wp_list_authors' => true, + 'wp_list_bookmarks' => true, + 'wp_list_categories' => true, + 'wp_list_comments' => true, + 'wp_login_form' => true, + 'wp_loginout' => true, + 'wp_nav_menu' => true, + 'wp_register' => true, + 'wp_tag_cloud' => true, + 'wp_title' => true, ); /** diff --git a/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php b/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php index 62102f3c39..5fb8f9e7b4 100644 --- a/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php +++ b/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php @@ -59,7 +59,6 @@ public function getGroups() { 'message' => '%s() found. Changing configuration at runtime is rarely necessary.', 'functions' => array( 'error_reporting', - 'ini_alter', 'ini_restore', 'apache_setenv', 'putenv', diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index b8ccafac63..b271a65fe1 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -32,7 +32,8 @@ class IniSetSniff extends AbstractFunctionParameterSniff { * ); */ protected $target_functions = array( - 'ini_set' => array(), + 'ini_set' => array(), + 'ini_alter' => array(), ); /** @@ -168,6 +169,5 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $parameters[2]['raw'], ) ); - return; } } diff --git a/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.inc b/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.inc index 5a6e9a5577..9a17c8a6d8 100644 --- a/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.inc +++ b/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.inc @@ -14,7 +14,7 @@ rawurlencode(); // Ok. dl(); // Warning. error_reporting(); // Warning. -ini_alter(); // Warning. + ini_restore(); // Warning. magic_quotes_runtime(); // Warning. set_magic_quotes_runtime(); // Warning. diff --git a/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.php b/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.php index 5470314fb6..0b43deffe5 100644 --- a/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.php +++ b/WordPress/Tests/PHP/DiscouragedPHPFunctionsUnitTest.php @@ -42,7 +42,6 @@ public function getWarningList() { 12 => 1, 15 => 1, 16 => 1, - 17 => 1, 18 => 1, 19 => 1, 20 => 1, diff --git a/WordPress/Tests/PHP/IniSetUnitTest.inc b/WordPress/Tests/PHP/IniSetUnitTest.inc index ef198be6a6..5f13927979 100644 --- a/WordPress/Tests/PHP/IniSetUnitTest.inc +++ b/WordPress/Tests/PHP/IniSetUnitTest.inc @@ -26,3 +26,7 @@ ini_set('memory_limit', -1); // Error. ini_set('report_memleaks', true); // Warning. ini_set('short_open_tag', 1230); // Warning. + +ini_alter('auto_detect_line_endings', true); // Ok. +ini_alter('display_errors', false); // Error. +ini_set('report_memleaks', 1230); // Warning. diff --git a/WordPress/Tests/PHP/IniSetUnitTest.php b/WordPress/Tests/PHP/IniSetUnitTest.php index b09e2f81d9..3022eed55a 100644 --- a/WordPress/Tests/PHP/IniSetUnitTest.php +++ b/WordPress/Tests/PHP/IniSetUnitTest.php @@ -43,6 +43,7 @@ public function getErrorList() { 23 => 1, 24 => 1, 25 => 1, + 31 => 1, ); } @@ -55,6 +56,7 @@ public function getWarningList() { return array( 27 => 1, 28 => 1, + 32 => 1, ); } From 43acdd26e709dd29d888e0ede6d24c76511fb95f Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Wed, 20 Feb 2019 21:19:52 +0100 Subject: [PATCH 21/81] Adds test marking variable option name as risky --- WordPress/Tests/PHP/IniSetUnitTest.inc | 1 + WordPress/Tests/PHP/IniSetUnitTest.php | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/WordPress/Tests/PHP/IniSetUnitTest.inc b/WordPress/Tests/PHP/IniSetUnitTest.inc index 5f13927979..9d939d8801 100644 --- a/WordPress/Tests/PHP/IniSetUnitTest.inc +++ b/WordPress/Tests/PHP/IniSetUnitTest.inc @@ -26,6 +26,7 @@ ini_set('memory_limit', -1); // Error. ini_set('report_memleaks', true); // Warning. ini_set('short_open_tag', 1230); // Warning. +ini_set($test, 1230); // Warning. ini_alter('auto_detect_line_endings', true); // Ok. ini_alter('display_errors', false); // Error. diff --git a/WordPress/Tests/PHP/IniSetUnitTest.php b/WordPress/Tests/PHP/IniSetUnitTest.php index 3022eed55a..0f96f571a6 100644 --- a/WordPress/Tests/PHP/IniSetUnitTest.php +++ b/WordPress/Tests/PHP/IniSetUnitTest.php @@ -43,7 +43,7 @@ public function getErrorList() { 23 => 1, 24 => 1, 25 => 1, - 31 => 1, + 32 => 1, ); } @@ -56,7 +56,8 @@ public function getWarningList() { return array( 27 => 1, 28 => 1, - 32 => 1, + 29 => 1, + 33 => 1, ); } From f0b223e6157095734f74f0471791433e91f94289 Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Wed, 20 Feb 2019 21:26:00 +0100 Subject: [PATCH 22/81] Isset instead of array_key_exists --- WordPress/Sniffs/PHP/IniSetSniff.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index b271a65fe1..c8bab3218e 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -135,14 +135,14 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $ini_set_function = $this->tokens[ $stackPtr ]; $option_name = $this->strip_quotes( $parameters[1]['raw'] ); $option_value = $this->strip_quotes( $parameters[2]['raw'] ); - if ( array_key_exists( $option_name, $this->whitelisted_options ) ) { + if ( isset( $this->whitelisted_options[ $option_name ] ) ) { $whitelisted_option = $this->whitelisted_options[ $option_name ]; if ( ! isset( $whitelisted_option['valid_values'] ) || in_array( $option_value, $whitelisted_option['valid_values'], true ) ) { return; } } - if ( array_key_exists( $option_name, $this->blacklisted_options ) ) { + if ( isset( $this->blacklisted_options[ $option_name ] ) ) { $blacklisted_option = $this->blacklisted_options[ $option_name ]; if ( ! isset( $blacklisted_option['invalid_values'] ) || in_array( $option_value, $blacklisted_option['invalid_values'], true ) ) { $this->phpcsFile->addError( From a8f3f1032083497caa3d68c7e3f867466d2248af Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Wed, 20 Feb 2019 21:33:20 +0100 Subject: [PATCH 23/81] Adds modular sniff name, removes use --- WordPress/Sniffs/PHP/IniSetSniff.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index c8bab3218e..fdcce52fe9 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -10,7 +10,6 @@ namespace WordPressCS\WordPress\Sniffs\PHP; use WordPressCS\WordPress\AbstractFunctionParameterSniff; -use \PHP_CodeSniffer\Util\Tokens; /** * Checks use of ini_set function with a blacklist for errors. @@ -148,7 +147,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $this->phpcsFile->addError( '%s(%s, %s) found. %s', $stackPtr, - 'Blacklisted', + $this->string_to_errorcode( $option_name . '_Blacklisted' ), array( $ini_set_function['content'], $parameters[1]['raw'], @@ -159,6 +158,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p return; } } + $this->phpcsFile->addWarning( '%s(%s, %s) found. Changing configuration values at runtime is rarely necessary.', $stackPtr, From 1290ca4a8210356f8251357e2bef0ce53e4eaf73 Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Wed, 20 Feb 2019 21:41:00 +0100 Subject: [PATCH 24/81] Adds alternative invalid and valid values --- WordPress/Tests/PHP/IniSetUnitTest.inc | 10 +++++++--- WordPress/Tests/PHP/IniSetUnitTest.php | 14 ++++++++------ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/WordPress/Tests/PHP/IniSetUnitTest.inc b/WordPress/Tests/PHP/IniSetUnitTest.inc index 9d939d8801..b28fb2eee8 100644 --- a/WordPress/Tests/PHP/IniSetUnitTest.inc +++ b/WordPress/Tests/PHP/IniSetUnitTest.inc @@ -8,9 +8,9 @@ ini_set('highlight.html', '#FFFFFF'); // Ok. ini_set('highlight.keyword', '#FFFFFF'); // Ok. ini_set('highlight.string', '#FFFFFF'); // Ok. ini_set('short_open_tag', true); // Ok. +ini_set('short_open_tag', 1); // Ok. +ini_set('short_open_tag', 'On'); // Ok. -ini_set('max_execution_time', 60); // Error. -ini_set('short_open_tag', false); // Error. ini_set('bcmath.scale', 0); // Error. ini_set('display_errors', 0); // Error. ini_set('error_reporting', 0); // Error. @@ -22,7 +22,11 @@ ini_set('iconv.output_encoding', ''); // Error. ini_set('iconv.output_encoding', ''); // Error. ini_set('ignore_user_abort', true); // Error. ini_set('log_errors', true); // Error. +ini_set('max_execution_time', 60); // Error. ini_set('memory_limit', -1); // Error. +ini_set('short_open_tag', false); // Error. +ini_set('short_open_tag', 0); // Error. +ini_set('short_open_tag', 'Off'); // Error. ini_set('report_memleaks', true); // Warning. ini_set('short_open_tag', 1230); // Warning. @@ -30,4 +34,4 @@ ini_set($test, 1230); // Warning. ini_alter('auto_detect_line_endings', true); // Ok. ini_alter('display_errors', false); // Error. -ini_set('report_memleaks', 1230); // Warning. +ini_alter('report_memleaks', 1230); // Warning. diff --git a/WordPress/Tests/PHP/IniSetUnitTest.php b/WordPress/Tests/PHP/IniSetUnitTest.php index 0f96f571a6..e110217212 100644 --- a/WordPress/Tests/PHP/IniSetUnitTest.php +++ b/WordPress/Tests/PHP/IniSetUnitTest.php @@ -29,8 +29,6 @@ class IniSetUnitTest extends AbstractSniffUnitTest { */ public function getErrorList() { return array( - 12 => 1, - 13 => 1, 14 => 1, 15 => 1, 16 => 1, @@ -43,7 +41,11 @@ public function getErrorList() { 23 => 1, 24 => 1, 25 => 1, - 32 => 1, + 26 => 1, + 27 => 1, + 28 => 1, + 29 => 1, + 36 => 1, ); } @@ -54,10 +56,10 @@ public function getErrorList() { */ public function getWarningList() { return array( - 27 => 1, - 28 => 1, - 29 => 1, + 31 => 1, + 32 => 1, 33 => 1, + 37 => 1, ); } From 8da3b6d96f0508bea090106fe1bc51ae4f286196 Mon Sep 17 00:00:00 2001 From: Juliette <663378+jrfnl@users.noreply.github.com> Date: Wed, 20 Feb 2019 21:45:40 +0100 Subject: [PATCH 25/81] Apply suggestions from code review Co-Authored-By: NielsdeBlaauw --- WordPress/Sniffs/PHP/IniSetSniff.php | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index fdcce52fe9..4993d056a7 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -12,7 +12,11 @@ use WordPressCS\WordPress\AbstractFunctionParameterSniff; /** - * Checks use of ini_set function with a blacklist for errors. + * Detect use of the `ini_set()` function. + * + * - Won't throw notices for "safe" ini directives as listed in the whitelist. + * - Throws errors for ini directives listed in the blacklist. + * - A warning will be thrown in all other cases. * * @package WPCS\WordPressCodingStandards * @@ -21,7 +25,7 @@ class IniSetSniff extends AbstractFunctionParameterSniff { /** - * Array of functions that must be checked + * Array of functions that must be checked. * * @since x.x.x * @@ -36,7 +40,7 @@ class IniSetSniff extends AbstractFunctionParameterSniff { ); /** - * Array of options that are allowed to be manipulated + * Array of PHP configuration options that are allowed to be manipulated. * * @since x.x.x * @@ -61,7 +65,7 @@ class IniSetSniff extends AbstractFunctionParameterSniff { ); /** - * Array of options that are not allowed to be manipulated + * Array of PHP configuration options that are not allowed to be manipulated. * * @since x.x.x * @@ -79,7 +83,7 @@ class IniSetSniff extends AbstractFunctionParameterSniff { ), 'short_open_tag' => array( 'invalid_values' => array( 'false', '0', 'Off' ), - 'message' => 'Turning off short_open_tag is prohibited as it might possibily break other plugins.', + 'message' => 'Turning off short_open_tag is prohibited as it can break other plugins.', ), 'bcmath.scale' => array( 'message' => 'Use `bcscale()` instead.', @@ -91,7 +95,7 @@ class IniSetSniff extends AbstractFunctionParameterSniff { 'message' => 'Use `WP_DEBUG` instead.', ), 'filter.default' => array( - 'message' => 'Use the filter flag constants when calling the functions instead (as you will possibly break other plugins if you change this).', + 'message' => 'Changing the option value can break other plugins. Use the filter flag constants when calling the Filter functions instead.', ), 'filter.default_flags' => array( 'message' => 'Use the filter flag constants when calling the functions instead (as you will possibly break other plugins if you change this).', @@ -149,7 +153,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $stackPtr, $this->string_to_errorcode( $option_name . '_Blacklisted' ), array( - $ini_set_function['content'], + $matched_content, $parameters[1]['raw'], $parameters[2]['raw'], $blacklisted_option['message'], @@ -164,7 +168,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $stackPtr, 'Risky', array( - $ini_set_function['content'], + $matched_content, $parameters[1]['raw'], $parameters[2]['raw'], ) From bc8d26a203d46c3f63d0d19091f825e82d0ec36c Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Wed, 20 Feb 2019 21:49:30 +0100 Subject: [PATCH 26/81] Adds @since tags --- WordPress/Sniffs/PHP/IniSetSniff.php | 10 +++++----- WordPress/Tests/PHP/IniSetUnitTest.php | 4 +--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index 4993d056a7..cdf9f6d689 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -20,14 +20,14 @@ * * @package WPCS\WordPressCodingStandards * - * @since x.x.x + * @since 2.1.0 */ class IniSetSniff extends AbstractFunctionParameterSniff { /** * Array of functions that must be checked. * - * @since x.x.x + * @since 2.1.0 * * @var array Multidimensional array with parameter details. * $target_functions = array( @@ -42,7 +42,7 @@ class IniSetSniff extends AbstractFunctionParameterSniff { /** * Array of PHP configuration options that are allowed to be manipulated. * - * @since x.x.x + * @since 2.1.0 * * @var array Multidimensional array with parameter details. * $whitelisted_options = array( @@ -67,7 +67,7 @@ class IniSetSniff extends AbstractFunctionParameterSniff { /** * Array of PHP configuration options that are not allowed to be manipulated. * - * @since x.x.x + * @since 2.1.0 * * @var array Multidimensional array with parameter details. * $blacklisted_options = array( @@ -125,7 +125,7 @@ class IniSetSniff extends AbstractFunctionParameterSniff { * is found in the blacklist. Warns as 'risky' when the option is not * found in the whitelist. * - * @since x.x.x + * @since 2.1.0 * * @param int $stackPtr The position of the current token in the stack. * @param array $group_name The name of the group which was matched. diff --git a/WordPress/Tests/PHP/IniSetUnitTest.php b/WordPress/Tests/PHP/IniSetUnitTest.php index e110217212..938f7cac93 100644 --- a/WordPress/Tests/PHP/IniSetUnitTest.php +++ b/WordPress/Tests/PHP/IniSetUnitTest.php @@ -16,9 +16,7 @@ * * @package WPCS\WordPressCodingStandards * - * @since 0.10.0 - * @since 0.13.0 Class name changed: this class is now namespaced. - * @since 1.0.0 This sniff has been moved from the `Functions` category to the `PHP` category. + * @since 2.1.0 */ class IniSetUnitTest extends AbstractSniffUnitTest { From 6d54ff399533d94a6155061c89c3166b9b5f5015 Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Wed, 20 Feb 2019 22:03:05 +0100 Subject: [PATCH 27/81] Adds sniff to WP-extra, doc fixes and unused statement removed --- WordPress-Extra/ruleset.xml | 4 ++++ WordPress/Sniffs/PHP/IniSetSniff.php | 12 ++++++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/WordPress-Extra/ruleset.xml b/WordPress-Extra/ruleset.xml index b03af3ee86..6ff9b6cf42 100644 --- a/WordPress-Extra/ruleset.xml +++ b/WordPress-Extra/ruleset.xml @@ -113,6 +113,10 @@ https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/242 --> + + + diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index cdf9f6d689..72cacaea32 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -121,9 +121,10 @@ class IniSetSniff extends AbstractFunctionParameterSniff { ); /** - * Process the parameter of a matched function. Errors if an option - * is found in the blacklist. Warns as 'risky' when the option is not - * found in the whitelist. + * Process the parameter of a matched function. + * + * Errors if an option is found in the blacklist. Warns as + * 'risky' when the option is not found in the whitelist. * * @since 2.1.0 * @@ -135,9 +136,8 @@ class IniSetSniff extends AbstractFunctionParameterSniff { * @return void */ public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) { - $ini_set_function = $this->tokens[ $stackPtr ]; - $option_name = $this->strip_quotes( $parameters[1]['raw'] ); - $option_value = $this->strip_quotes( $parameters[2]['raw'] ); + $option_name = $this->strip_quotes( $parameters[1]['raw'] ); + $option_value = $this->strip_quotes( $parameters[2]['raw'] ); if ( isset( $this->whitelisted_options[ $option_name ] ) ) { $whitelisted_option = $this->whitelisted_options[ $option_name ]; if ( ! isset( $whitelisted_option['valid_values'] ) || in_array( $option_value, $whitelisted_option['valid_values'], true ) ) { From 5e5a8710fb106293f723b2bf22f2738038045c30 Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Fri, 22 Feb 2019 20:18:01 +0100 Subject: [PATCH 28/81] Better wording for warning message --- WordPress/Sniffs/PHP/IniSetSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index 72cacaea32..06532461bf 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -164,7 +164,7 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p } $this->phpcsFile->addWarning( - '%s(%s, %s) found. Changing configuration values at runtime is rarely necessary.', + '%s(%s, %s) found. Changing configuration values at runtime is strongly discouraged.', $stackPtr, 'Risky', array( From 51331e11bd24dddc2863d43a4103682c39fa1d75 Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Fri, 22 Feb 2019 20:21:36 +0100 Subject: [PATCH 29/81] Consistent wording for the discouraged PHP functions --- WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php b/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php index 5fb8f9e7b4..e2691c2e60 100644 --- a/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php +++ b/WordPress/Sniffs/PHP/DiscouragedPHPFunctionsSniff.php @@ -56,7 +56,7 @@ public function getGroups() { 'runtime_configuration' => array( 'type' => 'warning', - 'message' => '%s() found. Changing configuration at runtime is rarely necessary.', + 'message' => '%s() found. Changing configuration values at runtime is strongly discouraged.', 'functions' => array( 'error_reporting', 'ini_restore', From 72353264505f698c65c66f543e970736c0eb764a Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Fri, 22 Feb 2019 20:28:00 +0100 Subject: [PATCH 30/81] Array in target_functions had no use, replaced with true --- WordPress/Sniffs/PHP/IniSetSniff.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index 06532461bf..243a4f7ba5 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -35,8 +35,8 @@ class IniSetSniff extends AbstractFunctionParameterSniff { * ); */ protected $target_functions = array( - 'ini_set' => array(), - 'ini_alter' => array(), + 'ini_set' => true, + 'ini_alter' => true, ); /** From d97fba416df7c750ccd3d6da8af423b94351b577 Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Fri, 22 Feb 2019 20:31:27 +0100 Subject: [PATCH 31/81] Alphabetical ordering for blacklisted_options --- WordPress/Sniffs/PHP/IniSetSniff.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index 243a4f7ba5..71dbcbaa8a 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -78,13 +78,6 @@ class IniSetSniff extends AbstractFunctionParameterSniff { * ); */ protected $blacklisted_options = array( - 'max_execution_time' => array( - 'message' => 'Use `set_time_limit()` instead.', - ), - 'short_open_tag' => array( - 'invalid_values' => array( 'false', '0', 'Off' ), - 'message' => 'Turning off short_open_tag is prohibited as it can break other plugins.', - ), 'bcmath.scale' => array( 'message' => 'Use `bcscale()` instead.', ), @@ -115,9 +108,16 @@ class IniSetSniff extends AbstractFunctionParameterSniff { 'log_errors' => array( 'message' => 'Use `WP_DEBUG_LOG` instead.', ), + 'max_execution_time' => array( + 'message' => 'Use `set_time_limit()` instead.', + ), 'memory_limit' => array( 'message' => 'Use `wp_raise_memory_limit()` or hook into the filters in that function.', ), + 'short_open_tag' => array( + 'invalid_values' => array( 'false', '0', 'Off' ), + 'message' => 'Turning off short_open_tag is prohibited as it can break other plugins.', + ), ); /** From 029f02e76af6a6c37562f44d7b887e32f78a79e7 Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Fri, 22 Feb 2019 20:34:17 +0100 Subject: [PATCH 32/81] Better message for filter.default_flags --- WordPress/Sniffs/PHP/IniSetSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index 71dbcbaa8a..2f768a3970 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -91,7 +91,7 @@ class IniSetSniff extends AbstractFunctionParameterSniff { 'message' => 'Changing the option value can break other plugins. Use the filter flag constants when calling the Filter functions instead.', ), 'filter.default_flags' => array( - 'message' => 'Use the filter flag constants when calling the functions instead (as you will possibly break other plugins if you change this).', + 'message' => 'Changing the option value can break other plugins. Use the filter flag constants when calling the Filter functions instead.', ), 'iconv.input_encoding' => array( 'message' => 'PHP < 5.6 only - use `iconv_set_encoding()` instead.', From be87a89e650ef35a962eaa3c0f689488880fdb8a Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Fri, 22 Feb 2019 20:40:20 +0100 Subject: [PATCH 33/81] Resets phpcbf on Sniff.php --- WordPress/Sniff.php | 118 ++++++++++++++++++++++---------------------- 1 file changed, 59 insertions(+), 59 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 4d72469145..7d5de2c044 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -175,65 +175,65 @@ abstract class Sniff implements PHPCS_Sniff { * @var array */ protected $autoEscapedFunctions = array( - 'allowed_tags' => true, - 'bloginfo' => true, - 'body_class' => true, - 'calendar_week_mod' => true, - 'category_description' => true, - 'checked' => true, - 'comment_class' => true, - 'count' => true, - 'disabled' => true, - 'do_shortcode' => true, - 'do_shortcode_tag' => true, - 'get_archives_link' => true, - 'get_attachment_link' => true, - 'get_avatar' => true, - 'get_bookmark_field' => true, - 'get_calendar' => true, - 'get_comment_author_link' => true, - 'get_current_blog_id' => true, - 'get_delete_post_link' => true, - 'get_search_form' => true, - 'get_search_query' => true, - 'get_the_author_link' => true, - 'get_the_author' => true, - 'get_the_date' => true, - 'get_the_ID' => true, - 'get_the_post_thumbnail' => true, - 'get_the_term_list' => true, - 'paginate_comments_links' => true, - 'post_type_archive_title' => true, - 'readonly' => true, - 'selected' => true, - 'single_cat_title' => true, - 'single_month_title' => true, - 'single_post_title' => true, - 'single_tag_title' => true, - 'single_term_title' => true, - 'tag_description' => true, - 'term_description' => true, - 'the_author' => true, - 'the_date' => true, - 'the_title_attribute' => true, - 'walk_nav_menu_tree' => true, - 'wp_dropdown_categories' => true, - 'wp_dropdown_users' => true, - 'wp_generate_tag_cloud' => true, - 'wp_get_archives' => true, - 'wp_get_attachment_image' => true, - 'wp_get_attachment_link' => true, - 'wp_link_pages' => true, - 'wp_list_authors' => true, - 'wp_list_bookmarks' => true, - 'wp_list_categories' => true, - 'wp_list_comments' => true, - 'wp_login_form' => true, - 'wp_loginout' => true, - 'wp_nav_menu' => true, - 'wp_register' => true, - 'wp_tag_cloud' => true, - 'wp_title' => true, + 'allowed_tags' => true, + 'bloginfo' => true, + 'body_class' => true, + 'calendar_week_mod' => true, + 'category_description' => true, + 'checked' => true, + 'comment_class' => true, + 'count' => true, + 'disabled' => true, + 'do_shortcode' => true, + 'do_shortcode_tag' => true, + 'get_archives_link' => true, + 'get_attachment_link' => true, + 'get_avatar' => true, + 'get_bookmark_field' => true, + 'get_calendar' => true, + 'get_comment_author_link' => true, + 'get_current_blog_id' => true, + 'get_delete_post_link' => true, + 'get_search_form' => true, + 'get_search_query' => true, + 'get_the_author_link' => true, + 'get_the_author' => true, + 'get_the_date' => true, + 'get_the_ID' => true, + 'get_the_post_thumbnail' => true, + 'get_the_term_list' => true, + 'paginate_comments_links' => true, + 'post_type_archive_title' => true, + 'readonly' => true, + 'selected' => true, + 'single_cat_title' => true, + 'single_month_title' => true, + 'single_post_title' => true, + 'single_tag_title' => true, + 'single_term_title' => true, + 'tag_description' => true, + 'term_description' => true, + 'the_author' => true, + 'the_date' => true, + 'the_title_attribute' => true, + 'walk_nav_menu_tree' => true, + 'wp_dropdown_categories' => true, + 'wp_dropdown_users' => true, + 'wp_generate_tag_cloud' => true, + 'wp_get_archives' => true, + 'wp_get_attachment_image' => true, + 'wp_get_attachment_link' => true, + 'wp_link_pages' => true, + 'wp_list_authors' => true, + 'wp_list_bookmarks' => true, + 'wp_list_categories' => true, + 'wp_list_comments' => true, + 'wp_login_form' => true, + 'wp_loginout' => true, + 'wp_nav_menu' => true, + 'wp_register' => true, + 'wp_tag_cloud' => true, + 'wp_title' => true, ); /** From 9f6c94baf5b6aa26f007f555ce5ac7b84ae770ae Mon Sep 17 00:00:00 2001 From: Niels de Blaauw Date: Sun, 24 Feb 2019 10:45:36 +0100 Subject: [PATCH 34/81] Adds more testcases. Option value is no longer case sensitive --- WordPress/Sniffs/PHP/IniSetSniff.php | 8 ++++---- WordPress/Tests/PHP/IniSetUnitTest.inc | 6 ++++++ WordPress/Tests/PHP/IniSetUnitTest.php | 16 ++++++++++------ 3 files changed, 20 insertions(+), 10 deletions(-) diff --git a/WordPress/Sniffs/PHP/IniSetSniff.php b/WordPress/Sniffs/PHP/IniSetSniff.php index 2f768a3970..afd58f1bf8 100644 --- a/WordPress/Sniffs/PHP/IniSetSniff.php +++ b/WordPress/Sniffs/PHP/IniSetSniff.php @@ -60,7 +60,7 @@ class IniSetSniff extends AbstractFunctionParameterSniff { 'highlight.keyword' => array(), 'highlight.string' => array(), 'short_open_tag' => array( - 'valid_values' => array( 'true', '1', 'On' ), + 'valid_values' => array( 'true', '1', 'on' ), ), ); @@ -115,7 +115,7 @@ class IniSetSniff extends AbstractFunctionParameterSniff { 'message' => 'Use `wp_raise_memory_limit()` or hook into the filters in that function.', ), 'short_open_tag' => array( - 'invalid_values' => array( 'false', '0', 'Off' ), + 'invalid_values' => array( 'false', '0', 'off' ), 'message' => 'Turning off short_open_tag is prohibited as it can break other plugins.', ), ); @@ -140,14 +140,14 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $option_value = $this->strip_quotes( $parameters[2]['raw'] ); if ( isset( $this->whitelisted_options[ $option_name ] ) ) { $whitelisted_option = $this->whitelisted_options[ $option_name ]; - if ( ! isset( $whitelisted_option['valid_values'] ) || in_array( $option_value, $whitelisted_option['valid_values'], true ) ) { + if ( ! isset( $whitelisted_option['valid_values'] ) || in_array( strtolower( $option_value ), $whitelisted_option['valid_values'], true ) ) { return; } } if ( isset( $this->blacklisted_options[ $option_name ] ) ) { $blacklisted_option = $this->blacklisted_options[ $option_name ]; - if ( ! isset( $blacklisted_option['invalid_values'] ) || in_array( $option_value, $blacklisted_option['invalid_values'], true ) ) { + if ( ! isset( $blacklisted_option['invalid_values'] ) || in_array( strtolower( $option_value ), $blacklisted_option['invalid_values'], true ) ) { $this->phpcsFile->addError( '%s(%s, %s) found. %s', $stackPtr, diff --git a/WordPress/Tests/PHP/IniSetUnitTest.inc b/WordPress/Tests/PHP/IniSetUnitTest.inc index b28fb2eee8..b39d3e1a3b 100644 --- a/WordPress/Tests/PHP/IniSetUnitTest.inc +++ b/WordPress/Tests/PHP/IniSetUnitTest.inc @@ -1,6 +1,7 @@ 1, - 15 => 1, 16 => 1, 17 => 1, 18 => 1, @@ -43,7 +41,12 @@ public function getErrorList() { 27 => 1, 28 => 1, 29 => 1, - 36 => 1, + 30 => 1, + 31 => 1, + 32 => 1, + 33 => 1, + 34 => 1, + 42 => 1, ); } @@ -54,10 +57,11 @@ public function getErrorList() { */ public function getWarningList() { return array( - 31 => 1, - 32 => 1, - 33 => 1, + 36 => 1, 37 => 1, + 38 => 1, + 39 => 1, + 43 => 1, ); } From b417db32694ea08fa88ff5a63d0a2863cb89f6a8 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 6 Mar 2019 18:28:26 +0100 Subject: [PATCH 35/81] WP/AlternativeFunctions: allow for more input streams with file related functions Similar to 1649 which allowed for using `php://input` with `file_get_contents()` and surprisingly inspired by the closing of issue 295. This PR expands on the earlier work done in relation to the PHP native input streams by: * recognizing more PHP native input streams; * recognizing the PHP input stream constants; * allowing for these in a number of the `file_system_read` group functions as well as for the `file_get_contents` function. Refs: * http://php.net/manual/en/wrappers.php.php * http://php.net/manual/en/features.commandline.io-streams.php Includes unit tests. Related 1649 Related 295 Notes: * I have not checked the WP FileSystem to see if it even could handle these input streams. If it can, we may need to discuss what is the preferred option in that case. Personally, this to me seems like something for which the WP FileSystem would be overkill/superfluous. * At a later point in time, the new method + properties could be a candidate for moving to `Sniff` or a separate utility class. As no other sniffs currently need them though, this is not necessary at this moment and could possible be combined with/actioned when 1465 comes into play. --- .../Sniffs/WP/AlternativeFunctionsSniff.php | 101 ++++++++++++++++-- .../Tests/WP/AlternativeFunctionsUnitTest.inc | 15 +++ .../Tests/WP/AlternativeFunctionsUnitTest.php | 8 +- 3 files changed, 111 insertions(+), 13 deletions(-) diff --git a/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php b/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php index 675189b9b9..556f3f90e6 100644 --- a/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php +++ b/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php @@ -25,6 +25,47 @@ */ class AlternativeFunctionsSniff extends AbstractFunctionRestrictionsSniff { + /** + * Local input streams which should not be flagged for the file system function checks. + * + * @link http://php.net/manual/en/wrappers.php.php + * + * @var array + */ + protected $allowed_local_streams = array( + 'php://input' => true, + 'php://output' => true, + 'php://stdin' => true, + 'php://stdout' => true, + 'php://stderr' => true, + ); + + /** + * Local input streams which should not be flagged for the file system function checks if + * the $filename starts with them. + * + * @link http://php.net/manual/en/wrappers.php.php + * + * @var array + */ + protected $allowed_local_stream_partials = array( + 'php://temp/', + 'php://fd/', + ); + + /** + * Local input stream constants which should not be flagged for the file system function checks. + * + * @link http://php.net/manual/en/wrappers.php.php + * + * @var array + */ + protected $allowed_local_stream_constants = array( + 'STDIN' => true, + 'STDOUT' => true, + 'STDERR' => true, + ); + /** * Groups of functions to restrict. * @@ -83,13 +124,13 @@ public function getGroups() { 'since' => '2.5.0', 'functions' => array( 'readfile', - 'fopen', - 'fsockopen', - 'pfsockopen', 'fclose', + 'fopen', 'fread', 'fwrite', 'file_put_contents', + 'fsockopen', + 'pfsockopen', ), ), @@ -202,13 +243,34 @@ public function process_matched_token( $stackPtr, $group_name, $matched_content return; } - $raw_stripped = $this->strip_quotes( $params[1]['raw'] ); - if ( 'php://input' === $raw_stripped ) { - // This is not a file, but the read-only raw data stream from the request body. + if ( $this->is_local_data_stream( $params[1]['raw'] ) === true ) { + // Local data stream. return; } - unset( $params, $raw_stripped ); + unset( $params ); + + break; + + case 'readfile': + case 'fopen': + case 'file_put_contents': + /* + * Allow for handling raw data streams from the request body. + */ + $first_param = $this->get_function_call_parameter( $stackPtr, 1 ); + + if ( false === $first_param ) { + // If the file to work with is not set, local data streams don't come into play. + break; + } + + if ( $this->is_local_data_stream( $first_param['raw'] ) === true ) { + // Local data stream. + return; + } + + unset( $first_param ); break; } @@ -223,4 +285,29 @@ public function process_matched_token( $stackPtr, $group_name, $matched_content } } + /** + * Determine based on the "raw" parameter value, whether a file parameter points to + * a local data stream. + * + * @param string $raw_param_value Raw parameter value. + * + * @return bool True if this is a local data stream. False otherwise. + */ + protected function is_local_data_stream( $raw_param_value ) { + + $raw_stripped = $this->strip_quotes( $raw_param_value ); + if ( isset( $this->allowed_local_streams[ $raw_stripped ] ) + || isset( $this->allowed_local_stream_constants[ $raw_param_value ] ) + ) { + return true; + } + + foreach ( $this->allowed_local_stream_partials as $partial ) { + if ( strpos( $raw_stripped, $partial ) === 0 ) { + return true; + } + } + + return false; + } } diff --git a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc index 0f6974dcb6..7b18dc5e11 100644 --- a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc +++ b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc @@ -51,3 +51,18 @@ file_get_contents( MUPLUGINDIR . 'some-file.xml' ); // OK. file_get_contents( plugin_dir_path( __FILE__ ) . 'subfolder/*.conf' ); // OK. file_get_contents(WP_Upload_Dir()['path'] . 'subdir/file.inc'); // OK. file_get_contents( 'php://input' ); // OK. + +// Loosely related to issue 295. +file_get_contents( 'php://stdin' ); // OK. +$input_stream = fopen( 'php://stdin', 'w' ); // OK. +$csv_ar = fopen(STDIN); // OK. + +$output_stream = fopen( 'php://output', 'w' ); // OK. +$output_stream = fopen( 'php://stdout', 'w' ); // OK. +$output_stream = fopen( 'php://stderr', 'w' ); // OK. +$output_stream = fopen( STDOUT, 'w' ); // OK. +$output_stream = fopen( STDERR, 'w' ); // OK. +$output_stream = fopen( 'php://fd/3', 'w' ); // OK. +$fp = fopen("php://temp/maxmemory:$fiveMBs", 'r+'); // OK. +readfile( 'php://filter/resource=http://www.example.com' ); // Warning. +file_put_contents("php://filter/write=string.rot13/resource=example.txt","Hello World"); // Warning. diff --git a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.php b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.php index 36ea1bcb51..55107f0f6a 100644 --- a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.php +++ b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.php @@ -40,13 +40,9 @@ public function getWarningList() { 5 => 1, 6 => 1, 7 => 1, - 10 => 1, - 12 => 1, - 14 => 1, - 16 => 1, 17 => 1, 18 => 1, @@ -60,13 +56,13 @@ public function getWarningList() { 26 => 1, 27 => 1, 28 => 1, - 40 => 1, - 44 => 1, 46 => 1, 47 => 1, 49 => 1, + 67 => 1, + 68 => 1, ); } From b1f321005613925c2eacc4cef537ce742b1d8a25 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 7 Mar 2019 05:23:18 +0100 Subject: [PATCH 36/81] Docs: exclude the `InlineComment.SpacingAfter` errorcode Fixes 1534 --- WordPress-Docs/ruleset.xml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/WordPress-Docs/ruleset.xml b/WordPress-Docs/ruleset.xml index fb74597d76..f029649646 100644 --- a/WordPress-Docs/ruleset.xml +++ b/WordPress-Docs/ruleset.xml @@ -71,6 +71,8 @@ + + From d30d22fc12493dd5b5a8e1add8a8d1a54b202ee5 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 7 Mar 2019 05:26:59 +0100 Subject: [PATCH 37/81] Docs: make the inclusion of the Generic sniff slightly more specific The `Generic.Commenting` category has three sniffs: * `DocComment` * `Todo` * `Fixme` As WP allows to do's and has no opinion on `FIXME` comments, we may as well just include the one sniff we actually need. --- WordPress-Docs/ruleset.xml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/WordPress-Docs/ruleset.xml b/WordPress-Docs/ruleset.xml index fb74597d76..b11dcb06bc 100644 --- a/WordPress-Docs/ruleset.xml +++ b/WordPress-Docs/ruleset.xml @@ -86,7 +86,7 @@ - + @@ -103,8 +103,5 @@ - - - From a6dbd886d10caedc25ee06f8c68f12a1564393f7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 7 Mar 2019 07:24:19 +0100 Subject: [PATCH 38/81] WP/AlternativeFunctions: allow calling `curl_version()` `curl_version()` doesn't create or need a cURL resource, so is perfectly fine to use. Ref: http://php.net/manual/en/function.curl-version.php --- WordPress/Sniffs/WP/AlternativeFunctionsSniff.php | 4 ++++ WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc | 2 ++ 2 files changed, 6 insertions(+) diff --git a/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php b/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php index 556f3f90e6..52e1356f21 100644 --- a/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php +++ b/WordPress/Sniffs/WP/AlternativeFunctionsSniff.php @@ -273,6 +273,10 @@ public function process_matched_token( $stackPtr, $group_name, $matched_content unset( $first_param ); break; + + case 'curl_version': + // Curl version doesn't actually create a connection. + return; } if ( ! isset( $this->groups[ $group_name ]['since'] ) ) { diff --git a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc index 7b18dc5e11..a9646d98e9 100644 --- a/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc +++ b/WordPress/Tests/WP/AlternativeFunctionsUnitTest.inc @@ -66,3 +66,5 @@ $output_stream = fopen( 'php://fd/3', 'w' ); // OK. $fp = fopen("php://temp/maxmemory:$fiveMBs", 'r+'); // OK. readfile( 'php://filter/resource=http://www.example.com' ); // Warning. file_put_contents("php://filter/write=string.rot13/resource=example.txt","Hello World"); // Warning. + +curl_version(); // OK. From d43381a2a57f2e30b68355daf99f3ac590d9deb6 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 04:41:50 +0100 Subject: [PATCH 39/81] Sniff::is_sanitized(): make the method more code style independent Whether people comply with the WP style rules regarding whitespace and such, is irrelevant for determining whether or not a value is sanitized/unslashed. This fixed a false positive where superglobals which were correctly unslashed/sanitized were not being recognized as such. Includes unit test in the `ValidatedSanitizedInput` sniff. --- WordPress/Sniff.php | 27 ++++++++++--------- .../ValidatedSanitizedInputUnitTest.inc | 2 ++ 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 7d5de2c044..caf12632b2 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1572,46 +1572,47 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { if ( $require_unslash ) { $this->add_unslash_error( $stackPtr ); } + return false; } // Get the function that it's in. $nested_parenthesis = $this->tokens[ $stackPtr ]['nested_parenthesis']; - $function_closer = end( $nested_parenthesis ); - $function_opener = key( $nested_parenthesis ); - $function = $this->tokens[ ( $function_opener - 1 ) ]; + $nested_openers = array_keys( $nested_parenthesis ); + $function_opener = array_pop( $nested_openers ); + $functionPtr = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $function_opener - 1 ), null, true, null, true ); // If it is just being unset, the value isn't used at all, so it's safe. - if ( \T_UNSET === $function['code'] ) { + if ( \T_UNSET === $this->tokens[ $functionPtr ]['code'] ) { return true; } - // If this isn't a call to a function, it sure isn't sanitizing function. - if ( \T_STRING !== $function['code'] ) { + // If this isn't a call to a function, it sure isn't a sanitizing function. + if ( \T_STRING !== $this->tokens[ $functionPtr ]['code'] ) { if ( $require_unslash ) { $this->add_unslash_error( $stackPtr ); } + return false; } - $functionName = $function['content']; + $functionName = $this->tokens[ $functionPtr ]['content']; // Check if wp_unslash() is being used. if ( 'wp_unslash' === $functionName ) { $is_unslashed = true; - $function_closer = prev( $nested_parenthesis ); + $function_opener = array_pop( $nested_openers ); // If there is no other function being used, this value is unsanitized. - if ( ! $function_closer ) { + if ( ! isset( $function_opener ) ) { return false; } - $function_opener = key( $nested_parenthesis ); - $functionName = $this->tokens[ ( $function_opener - 1 ) ]['content']; + $functionPtr = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $function_opener - 1 ), null, true, null, true ); + $functionName = $this->tokens[ $functionPtr ]['content']; } else { - $is_unslashed = false; } @@ -1619,7 +1620,7 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { if ( 'array_map' === $functionName ) { // Get the first parameter. - $callback = $this->get_function_call_parameter( ( $function_opener - 1 ), 1 ); + $callback = $this->get_function_call_parameter( $functionPtr, 1 ); if ( ! empty( $callback ) ) { /* diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index da3be11da1..d9671b4dfb 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -209,3 +209,5 @@ if ( $obj->array_key_exists( 'my_field4', $_POST ) ) { if ( ClassName::array_key_exists( 'my_field5', $_POST ) ) { $id = (int) $_POST['my_field5']; // Bad. } + +echo sanitize_text_field (wp_unslash ($_GET['test'])); // OK. From 2600112fd16d2b2d4a609c94613bb5c50e1921aa Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 06:32:24 +0100 Subject: [PATCH 40/81] GlobalVariablesOverride: implement the Sniff::is_foreach_as() method Removing some code duplication. --- .../Sniffs/WP/GlobalVariablesOverrideSniff.php | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php index 4485c027ad..6a23ab6370 100644 --- a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php +++ b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php @@ -321,17 +321,8 @@ protected function process_global_statement( $stackPtr, $in_function_scope ) { } // Check if this is a variable assignment within a `foreach()` declaration. - if ( isset( $this->tokens[ $ptr ]['nested_parenthesis'] ) ) { - $nested_parenthesis = $this->tokens[ $ptr ]['nested_parenthesis']; - $close_parenthesis = end( $nested_parenthesis ); - if ( isset( $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ) - && \T_FOREACH === $this->tokens[ $this->tokens[ $close_parenthesis ]['parenthesis_owner'] ]['code'] - && ( false !== $previous - && ( \T_DOUBLE_ARROW === $this->tokens[ $previous ]['code'] - || \T_AS === $this->tokens[ $previous ]['code'] ) ) - ) { - $this->maybe_add_error( $ptr ); - } + if ( $this->is_foreach_as( $ptr ) === true ) { + $this->maybe_add_error( $ptr ); } } } From 0a18e6bd5d6d4e37edf6184cd54a29c8fba3f757 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 08:05:34 +0100 Subject: [PATCH 41/81] Clean up: use array_change_key_case() You learn something new every day. Never came across this function before, but simplifies a few code snippets. Ref: https://php.net/manual/en/function.array-change-key-case.php --- WordPress/Sniffs/WP/DeprecatedClassesSniff.php | 6 ++---- WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php | 6 ++---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/WordPress/Sniffs/WP/DeprecatedClassesSniff.php b/WordPress/Sniffs/WP/DeprecatedClassesSniff.php index 69894fb644..cd69ba988b 100644 --- a/WordPress/Sniffs/WP/DeprecatedClassesSniff.php +++ b/WordPress/Sniffs/WP/DeprecatedClassesSniff.php @@ -58,13 +58,11 @@ class DeprecatedClassesSniff extends AbstractClassRestrictionsSniff { */ public function getGroups() { // Make sure all array keys are lowercase. - $keys = array_keys( $this->deprecated_classes ); - $keys = array_map( 'strtolower', $keys ); - $this->deprecated_classes = array_combine( $keys, $this->deprecated_classes ); + $this->deprecated_classes = array_change_key_case( $this->deprecated_classes, CASE_LOWER ); return array( 'deprecated_classes' => array( - 'classes' => $keys, + 'classes' => array_keys( $this->deprecated_classes ), ), ); } diff --git a/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php b/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php index 7270899004..5c2be16c07 100644 --- a/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php +++ b/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php @@ -1339,13 +1339,11 @@ class DeprecatedFunctionsSniff extends AbstractFunctionRestrictionsSniff { */ public function getGroups() { // Make sure all array keys are lowercase. - $keys = array_keys( $this->deprecated_functions ); - $keys = array_map( 'strtolower', $keys ); - $this->deprecated_functions = array_combine( $keys, $this->deprecated_functions ); + $this->deprecated_functions = array_change_key_case( $this->deprecated_functions, CASE_LOWER ); return array( 'deprecated_functions' => array( - 'functions' => $keys, + 'functions' => array_keys( $this->deprecated_functions ), ), ); } From d392c45ddf3c07e76a5336edcae5a70d9745f0c7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 05:38:25 +0100 Subject: [PATCH 42/81] Sniff: add two new utility methods `is_class_object_call()` and `is_token_namespaced()` These should help make the checking of whether or not a function call is a global function, method call or a namespaced function call more consistent. These functions will be unit tested by the implementations in various other places and the fact that the existing unit tests still pass after that. --- WordPress/Sniff.php | 65 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index caf12632b2..05c71d84f5 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1489,6 +1489,71 @@ protected function is_in_isset_or_empty( $stackPtr ) { return false; } + /** + * Check if a particular token is a (static or non-static) call to a class method or property. + * + * @internal Note: this may still mistake a namespaced function imported via a `use` statement for + * a global function! + * + * @since 2.1.0 + * + * @param int $stackPtr The index of the token in the stack. + * + * @return bool + */ + protected function is_class_object_call( $stackPtr ) { + $before = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true, null, true ); + + if ( false === $before ) { + return false; + } + + if ( \T_OBJECT_OPERATOR !== $this->tokens[ $before ]['code'] + && \T_DOUBLE_COLON !== $this->tokens[ $before ]['code'] + ) { + return false; + } + + return true; + } + + /** + * Check if a particular token is prefixed with a namespace. + * + * @internal This will give a false positive if the file is not namespaced and the token is prefixed + * with `namespace\`. + * + * @since 2.1.0 + * + * @param int $stackPtr The index of the token in the stack. + * + * @return bool + */ + protected function is_token_namespaced( $stackPtr ) { + $prev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true, null, true ); + + if ( false === $prev ) { + return false; + } + + if ( \T_NS_SEPARATOR !== $this->tokens[ $prev ]['code'] ) { + return false; + } + + $before_prev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $prev - 1 ), null, true, null, true ); + if ( false === $before_prev ) { + return false; + } + + if ( \T_STRING !== $this->tokens[ $before_prev ]['code'] + && \T_NAMESPACE !== $this->tokens[ $before_prev ]['code'] + ) { + return false; + } + + return true; + } + /** * Check if something is only being sanitized. * From 7c1b6f82de4a7ac93407313a854c1fcfa9059f72 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 05:39:05 +0100 Subject: [PATCH 43/81] Sniff::is_in_isset_or_empty(): implement is_class_object_call() and is_token_namespaced() --- WordPress/Sniff.php | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 05c71d84f5..f53d525b66 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1461,23 +1461,12 @@ protected function is_in_isset_or_empty( $stackPtr ) { } if ( \T_STRING === $previous_code && 'array_key_exists' === $this->tokens[ $previous_non_empty ]['content'] ) { - $before_function = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $previous_non_empty - 1 ), null, true, null, true ); - - if ( false !== $before_function ) { - if ( \T_OBJECT_OPERATOR === $this->tokens[ $before_function ]['code'] - || \T_DOUBLE_COLON === $this->tokens[ $before_function ]['code'] - ) { - // Method call. - return false; - } + if ( $this->is_class_object_call( $previous_non_empty ) === true ) { + return false; + } - if ( \T_NS_SEPARATOR === $this->tokens[ $before_function ]['code'] ) { - $before_before = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $before_function - 1 ), null, true, null, true ); - if ( false !== $before_before && \T_STRING === $this->tokens[ $before_before ]['code'] ) { - // Namespaced function call. - return false; - } - } + if ( $this->is_token_namespaced( $previous_non_empty ) === true ) { + return false; } $second_param = $this->get_function_call_parameter( $previous_non_empty, 2 ); From e5c6ecb9e762f0ab16b9d24b16206e590d189f92 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 05:47:05 +0100 Subject: [PATCH 44/81] Sniff::is_validated(): implement is_class_object_call() and is_token_namespaced() --- WordPress/Sniff.php | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index f53d525b66..02b25928ee 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1907,22 +1907,14 @@ protected function is_validated( $stackPtr, $array_key = null, $in_condition_onl continue 2; } - $previous_non_empty = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $i - 1 ), null, true, null, true ); - if ( false !== $previous_non_empty ) { - if ( \T_OBJECT_OPERATOR === $this->tokens[ $previous_non_empty ]['code'] - || \T_DOUBLE_COLON === $this->tokens[ $previous_non_empty ]['code'] - ) { - // Method call. - continue 2; - } + if ( $this->is_class_object_call( $i ) === true ) { + // Method call. + continue 2; + } - if ( \T_NS_SEPARATOR === $this->tokens[ $previous_non_empty ]['code'] ) { - $pprev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $previous_non_empty - 1 ), null, true, null, true ); - if ( false !== $pprev && \T_STRING === $this->tokens[ $pprev ]['code'] ) { - // Namespaced function call. - continue 2; - } - } + if ( $this->is_token_namespaced( $i ) === true ) { + // Namespaced function call. + continue 2; } $params = $this->get_function_call_parameters( $i ); From a55e3e08ac8dd1eb5ee07591c415caa8960cd898 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 06:13:21 +0100 Subject: [PATCH 45/81] Sniff::is_use_of_global_constant(): implement is_token_namespaced() --- WordPress/Sniff.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 02b25928ee..7359f145df 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -2726,10 +2726,7 @@ public function is_use_of_global_constant( $stackPtr ) { return false; } - if ( false !== $prev - && \T_NS_SEPARATOR === $this->tokens[ $prev ]['code'] - && \T_STRING === $this->tokens[ ( $prev - 1 ) ]['code'] - ) { + if ( $this->is_token_namespaced( $stackPtr ) === true ) { // Namespaced constant of the same name. return false; } From c98ecc258b8ada9889d59436c17117c4fc59dcae Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 05:55:50 +0100 Subject: [PATCH 46/81] AbstractFunctionRestrictions: implement Sniff::is_class_object_call() and Sniff::is_token_namespaced() --- .../AbstractFunctionRestrictionsSniff.php | 20 +++++++++---------- .../PHP/DevelopmentFunctionsUnitTest.inc | 5 +++++ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/WordPress/AbstractFunctionRestrictionsSniff.php b/WordPress/AbstractFunctionRestrictionsSniff.php index 5b11af4dad..4553e3f472 100644 --- a/WordPress/AbstractFunctionRestrictionsSniff.php +++ b/WordPress/AbstractFunctionRestrictionsSniff.php @@ -213,7 +213,15 @@ public function process_token( $stackPtr ) { public function is_targetted_token( $stackPtr ) { // Exclude function definitions, class methods, and namespaced calls. - if ( \T_STRING === $this->tokens[ $stackPtr ]['code'] && isset( $this->tokens[ ( $stackPtr - 1 ) ] ) ) { + if ( \T_STRING === $this->tokens[ $stackPtr ]['code'] ) { + if ( $this->is_class_object_call( $stackPtr ) === true ) { + return false; + } + + if ( $this->is_token_namespaced( $stackPtr ) === true ) { + return false; + } + $prev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true ); if ( false !== $prev ) { @@ -222,21 +230,11 @@ public function is_targetted_token( $stackPtr ) { \T_FUNCTION => \T_FUNCTION, \T_CLASS => \T_CLASS, \T_AS => \T_AS, // Use declaration alias. - \T_DOUBLE_COLON => \T_DOUBLE_COLON, - \T_OBJECT_OPERATOR => \T_OBJECT_OPERATOR, ); if ( isset( $skipped[ $this->tokens[ $prev ]['code'] ] ) ) { return false; } - - // Skip namespaced functions, ie: \foo\bar() not \bar(). - if ( \T_NS_SEPARATOR === $this->tokens[ $prev ]['code'] ) { - $pprev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $prev - 1 ), null, true ); - if ( false !== $pprev && \T_STRING === $this->tokens[ $pprev ]['code'] ) { - return false; - } - } } return true; diff --git a/WordPress/Tests/PHP/DevelopmentFunctionsUnitTest.inc b/WordPress/Tests/PHP/DevelopmentFunctionsUnitTest.inc index 5039be590e..b07e7a8ce6 100644 --- a/WordPress/Tests/PHP/DevelopmentFunctionsUnitTest.inc +++ b/WordPress/Tests/PHP/DevelopmentFunctionsUnitTest.inc @@ -32,3 +32,8 @@ phpinfo(); // Ok - within excluded group. // phpcs:set WordPress.PHP.DevelopmentFunctions exclude[] trigger_error(); // Error. phpinfo(); // Error. + +Wrapper_Class::var_dump(); // OK, not the native PHP function. +$wrapper ->var_dump(); // OK, not the native PHP function. +namespace\var_dump(); // OK as long as the file is namespaced. +MyNamespace\var_dump(); // OK, namespaced function. From 374691c9c39d457f8b3c239d7c4455020d86b9ef Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 05:59:11 +0100 Subject: [PATCH 47/81] GlobalVariablesOverride: implement Sniff::is_class_object_call() --- WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php index 6a23ab6370..57b64d07a5 100644 --- a/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php +++ b/WordPress/Sniffs/WP/GlobalVariablesOverrideSniff.php @@ -310,8 +310,7 @@ protected function process_global_statement( $stackPtr, $in_function_scope ) { } // Don't throw false positives for static class properties. - $previous = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $ptr - 1 ), null, true, null, true ); - if ( false !== $previous && \T_DOUBLE_COLON === $this->tokens[ $previous ]['code'] ) { + if ( $this->is_class_object_call( $ptr ) === true ) { continue; } From 08f4a7ff6abfcc91f90c42d76b4e5630b71b1abf Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 06:09:07 +0100 Subject: [PATCH 48/81] DiscouragedConstants: implement Sniff::is_token_namespaced() --- WordPress/Sniffs/WP/DiscouragedConstantsSniff.php | 5 +---- WordPress/Tests/WP/DiscouragedConstantsUnitTest.inc | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/WordPress/Sniffs/WP/DiscouragedConstantsSniff.php b/WordPress/Sniffs/WP/DiscouragedConstantsSniff.php index 00e1e8efa8..6539b49cc5 100644 --- a/WordPress/Sniffs/WP/DiscouragedConstantsSniff.php +++ b/WordPress/Sniffs/WP/DiscouragedConstantsSniff.php @@ -124,10 +124,7 @@ public function process_arbitrary_tstring( $stackPtr ) { return; } - if ( false !== $prev - && \T_NS_SEPARATOR === $this->tokens[ $prev ]['code'] - && \T_STRING === $this->tokens[ ( $prev - 1 ) ]['code'] - ) { + if ( $this->is_token_namespaced( $stackPtr ) === true ) { // Namespaced constant of the same name. return; } diff --git a/WordPress/Tests/WP/DiscouragedConstantsUnitTest.inc b/WordPress/Tests/WP/DiscouragedConstantsUnitTest.inc index d5e5f3a689..4949a9557a 100644 --- a/WordPress/Tests/WP/DiscouragedConstantsUnitTest.inc +++ b/WordPress/Tests/WP/DiscouragedConstantsUnitTest.inc @@ -42,7 +42,7 @@ define( 'My\STYLESHEETPATH', 'something' ); if ( defined( 'STYLESHEETPATH' ) ) { // Ok. // Do something unrelated. } - +echo namespace\STYLESHEETPATH; // "Magic" namespace operator. /* * These are all bad. From 7d01ff3f06c4b6b92d8e0ced29d9c4b5955df270 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 02:34:30 +0100 Subject: [PATCH 49/81] New utility method `Sniff::is_in_function_call()` Allows for checking whether a token is (part of) a parameter passed to a specific (set of) function(s). Optionally checks whether the function call is to a global function. Optionally only checks the "deepest" function call. Returns the stack pointer to the `T_STRING` token for the function call or `false`. --- WordPress/Sniff.php | 69 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 7359f145df..c1ea6f2577 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1543,6 +1543,75 @@ protected function is_token_namespaced( $stackPtr ) { return true; } + /** + * Check if a token is (part of) a parameter for a function call to a select list of functions. + * + * @since 2.1.0 + * + * @param int $stackPtr The index of the token in the stack. + * @param array $valid_functions List of valid function names. + * Note: The keys to this array should be the function names + * in lowercase. Values are irrelevant. + * @param bool $global Optional. Whether to make sure that the function call is + * to a global function. If `false`, methods and namespaced + * function calls will also be allowed. + * Defaults to `true`. + * @param bool $allow_nested Optional. Whether to allow for nested function calls within the + * call to this function. + * I.e. when checking whether a token is within a function call + * to `strtolower()`, whether to accept `strtolower( trim( $var ) )` + * or only `strtolower( $var )`. + * Defaults to `false`. + * + * @return int|bool Stack pointer to the function call T_STRING token or false otherwise. + */ + protected function is_in_function_call( $stackPtr, $valid_functions, $global = true, $allow_nested = false ) { + if ( ! isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { + return false; + } + + $nested_parenthesis = $this->tokens[ $stackPtr ]['nested_parenthesis']; + if ( false === $allow_nested ) { + $nested_parenthesis = array_reverse( $nested_parenthesis, true ); + } + + foreach ( $nested_parenthesis as $open => $close ) { + + $prev_non_empty = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $open - 1 ), null, true, null, true ); + if ( false === $prev_non_empty || \T_STRING !== $this->tokens[ $prev_non_empty ]['code'] ) { + continue; + } + + if ( isset( $valid_functions[ strtolower( $this->tokens[ $prev_non_empty ]['content'] ) ] ) === false ) { + if ( false === $allow_nested ) { + // Function call encountered, but not to one of the allowed functions. + return false; + } + + continue; + } + + if ( false === $global ) { + return $prev_non_empty; + } + + /* + * Now, make sure it is a global function. + */ + if ( $this->is_class_object_call( $prev_non_empty ) === true ) { + continue; + } + + if ( $this->is_token_namespaced( $prev_non_empty ) === true ) { + continue; + } + + return $prev_non_empty; + } + + return false; + } + /** * Check if something is only being sanitized. * From 95e3a3d634b6c00e391d68813b70f48df9b08541 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 10:14:20 +0100 Subject: [PATCH 50/81] Sniff::is_in_isset_or_empty(): implement new `is_in_function_call()` method This fixes potential false negatives when a method/namespaced function mirroring the name of the PHP array_key_exists() function would be used. --- WordPress/Sniff.php | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index c1ea6f2577..9eb605d789 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1460,16 +1460,9 @@ protected function is_in_isset_or_empty( $stackPtr ) { return true; } - if ( \T_STRING === $previous_code && 'array_key_exists' === $this->tokens[ $previous_non_empty ]['content'] ) { - if ( $this->is_class_object_call( $previous_non_empty ) === true ) { - return false; - } - - if ( $this->is_token_namespaced( $previous_non_empty ) === true ) { - return false; - } - - $second_param = $this->get_function_call_parameter( $previous_non_empty, 2 ); + $functionPtr = $this->is_in_function_call( $stackPtr, array( 'array_key_exists' => true ) ); + if ( false !== $functionPtr ) { + $second_param = $this->get_function_call_parameter( $functionPtr, 2 ); if ( $stackPtr >= $second_param['start'] && $stackPtr <= $second_param['end'] ) { return true; } From 6e470dec3441c038fda3cbf007fbc18732069f56 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 04:46:40 +0100 Subject: [PATCH 51/81] Sniff::is_sanitized(): implement new `is_in_function_call()` method This fixes potential false negatives when a method/namespaced function mirroring the name of the WP unslashing/sanitization functions would be used. Includes unit test. --- WordPress/Sniff.php | 25 +++++++++++++------ .../ValidatedSanitizedInputUnitTest.inc | 5 ++++ .../ValidatedSanitizedInputUnitTest.php | 2 ++ 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 9eb605d789..5fa62c1310 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1703,9 +1703,16 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { return true; } - // If this isn't a call to a function, it sure isn't a sanitizing function. - if ( \T_STRING !== $this->tokens[ $functionPtr ]['code'] ) { - if ( $require_unslash ) { + $valid_functions = $this->sanitizingFunctions; + $valid_functions += $this->unslashingSanitizingFunctions; + $valid_functions['wp_unslash'] = true; + $valid_functions['array_map'] = true; + + $functionPtr = $this->is_in_function_call( $stackPtr, $valid_functions ); + + // If this isn't a call to one of the valid functions, it sure isn't a sanitizing function. + if ( false === $functionPtr ) { + if ( true === $require_unslash ) { $this->add_unslash_error( $stackPtr ); } @@ -1717,15 +1724,17 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { // Check if wp_unslash() is being used. if ( 'wp_unslash' === $functionName ) { - $is_unslashed = true; - $function_opener = array_pop( $nested_openers ); + $is_unslashed = true; + + unset( $valid_functions['wp_unslash'] ); + $higherFunctionPtr = $this->is_in_function_call( $functionPtr, $valid_functions ); - // If there is no other function being used, this value is unsanitized. - if ( ! isset( $function_opener ) ) { + // If there is no other valid function being used, this value is unsanitized. + if ( false === $higherFunctionPtr ) { return false; } - $functionPtr = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $function_opener - 1 ), null, true, null, true ); + $functionPtr = $higherFunctionPtr; $functionName = $this->tokens[ $functionPtr ]['content']; } else { diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index d9671b4dfb..57db08843f 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -211,3 +211,8 @@ if ( ClassName::array_key_exists( 'my_field5', $_POST ) ) { } echo sanitize_text_field (wp_unslash ($_GET['test'])); // OK. + +if ( isset( $_GET['unslash_check'] ) ) { + $clean = sanitize_text_field( WP_Faker::wp_unslash( $_GET['unslash_check'] ) ); // Bad x1 - unslash. + $clean = WP_Faker\sanitize_text_field( wp_unslash( $_GET['unslash_check'] ) ); // Bad x1 - sanitize. +} diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php index 46ab050e97..430b278b81 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php @@ -58,6 +58,8 @@ public function getErrorList() { 202 => 1, 206 => 1, 210 => 1, + 216 => 1, + 217 => 1, ); } From 91b053fd9a50577a272f34ac94601f5278b1bee3 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 03:28:23 +0100 Subject: [PATCH 52/81] CronInterval: implement new `Sniff::is_in_function_call()` method This fixes some potential false positives. Includes unit tests. --- WordPress/Sniffs/WP/CronIntervalSniff.php | 18 ++++++++++++++++-- WordPress/Tests/WP/CronIntervalUnitTest.inc | 7 ++++++- WordPress/Tests/WP/CronIntervalUnitTest.php | 1 - 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/WordPress/Sniffs/WP/CronIntervalSniff.php b/WordPress/Sniffs/WP/CronIntervalSniff.php index d7c81c2bb4..d7e91b8321 100644 --- a/WordPress/Sniffs/WP/CronIntervalSniff.php +++ b/WordPress/Sniffs/WP/CronIntervalSniff.php @@ -55,6 +55,15 @@ class CronIntervalSniff extends Sniff { 'YEAR_IN_SECONDS' => 31536000, ); + /** + * Function within which the hook should be found. + * + * @var array + */ + protected $valid_functions = array( + 'add_filter' => true, + ); + /** * Returns an array of tokens this test wants to listen for. * @@ -82,8 +91,8 @@ public function process_token( $stackPtr ) { } // If within add_filter. - $functionPtr = $this->phpcsFile->findPrevious( \T_STRING, key( $token['nested_parenthesis'] ) ); - if ( false === $functionPtr || 'add_filter' !== $this->tokens[ $functionPtr ]['content'] ) { + $functionPtr = $this->is_in_function_call( $stackPtr, $this->valid_functions ); + if ( false === $functionPtr ) { return; } @@ -92,6 +101,11 @@ public function process_token( $stackPtr ) { return; } + if ( $stackPtr >= $callback['start'] ) { + // "cron_schedules" found in the second parameter, not the first. + return; + } + // Detect callback function name. $callbackArrayPtr = $this->phpcsFile->findNext( Tokens::$emptyTokens, $callback['start'], ( $callback['end'] + 1 ), true ); diff --git a/WordPress/Tests/WP/CronIntervalUnitTest.inc b/WordPress/Tests/WP/CronIntervalUnitTest.inc index 712eb64072..19b061fe34 100644 --- a/WordPress/Tests/WP/CronIntervalUnitTest.inc +++ b/WordPress/Tests/WP/CronIntervalUnitTest.inc @@ -52,7 +52,7 @@ add_filter( 'cron_schedules' ); // Ignore, no callback parameter. add_filter( 'cron_schedules', [ 'Foo', 'my_add_quicklier' ] ); // Error: 5 min. -// Ignore, not our function. Currently gives false positive, will be fixed later when function call detection utility functions are added. +// Ignore, not our function. My_Custom::add_filter( 'cron_schedules', [ 'Foo', 'my_add_quicklier' ] ); // Deal correctly with the WP time constants. @@ -139,3 +139,8 @@ add_filter( 'cron_schedules', function ( $schedules ) { ); return $schedules; } ); // Error: 9 min. + +Custom::add_filter( 'cron_schedules', array( $class, $method ) ); // OK, not the WP function. +add_filter( 'some_hook', array( $place, 'cron_schedules' ) ); // OK, not the hook we're looking for. +add_filter( function() { return get_hook_name('cron_schedules'); }(), array( $class, $method ) ); // OK, nested in another function call. + diff --git a/WordPress/Tests/WP/CronIntervalUnitTest.php b/WordPress/Tests/WP/CronIntervalUnitTest.php index be5cc9f6b8..704923cc5f 100644 --- a/WordPress/Tests/WP/CronIntervalUnitTest.php +++ b/WordPress/Tests/WP/CronIntervalUnitTest.php @@ -45,7 +45,6 @@ public function getWarningList() { 41 => 1, 43 => 1, 53 => 1, - 56 => 1, // False positive. 67 => 1, 76 => 1, 85 => 1, From 4e05eaa766f8077ec26373d352b633908ae03c5a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 01:04:08 +0100 Subject: [PATCH 53/81] ValidatedSanitizedInput: minor code readability improvement --- .../Security/ValidatedSanitizedInputSniff.php | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index 5681de7cea..2f5ca051b1 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -133,8 +133,12 @@ function ( $symbol ) { // Check for validation first. if ( ! $this->is_validated( $stackPtr, $array_key, $this->check_validation_in_scope_only ) ) { - $this->phpcsFile->addError( 'Detected usage of a non-validated input variable: %s', $stackPtr, 'InputNotValidated', $error_data ); - // return; // Should we just return and not look for sanitizing functions ? + $this->phpcsFile->addError( + 'Detected usage of a non-validated input variable: %s', + $stackPtr, + 'InputNotValidated', + $error_data + ); } if ( $this->has_whitelist_comment( 'sanitization', $stackPtr ) ) { @@ -150,7 +154,12 @@ function ( $symbol ) { // Now look for sanitizing functions. if ( ! $this->is_sanitized( $stackPtr, true ) ) { - $this->phpcsFile->addError( 'Detected usage of a non-sanitized input variable: %s', $stackPtr, 'InputNotSanitized', $error_data ); + $this->phpcsFile->addError( + 'Detected usage of a non-sanitized input variable: %s', + $stackPtr, + 'InputNotSanitized', + $error_data + ); } } From 5cb689fcee6dc2fb50ea63746682217e87e6e755 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 05:02:51 +0100 Subject: [PATCH 54/81] Sniff::is_comparison(): minor defensive coding tweak There may not be a next token during live coding. This prevents the method from evaluating `false` to `0` and looking at the first token in the file. --- WordPress/Sniff.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 7359f145df..7939004c4f 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1984,7 +1984,7 @@ protected function is_comparison( $stackPtr ) { ); // This might be an opening square bracket in the case of arrays ($var['a']). - while ( \T_OPEN_SQUARE_BRACKET === $this->tokens[ $next_token ]['code'] ) { + while ( false !== $next_token && \T_OPEN_SQUARE_BRACKET === $this->tokens[ $next_token ]['code'] ) { $next_token = $this->phpcsFile->findNext( Tokens::$emptyTokens, @@ -1994,7 +1994,7 @@ protected function is_comparison( $stackPtr ) { ); } - if ( isset( Tokens::$comparisonTokens[ $this->tokens[ $next_token ]['code'] ] ) ) { + if ( false !== $next_token && isset( Tokens::$comparisonTokens[ $this->tokens[ $next_token ]['code'] ] ) ) { return true; } From 1b3cdfc714c00ba9b35df2993da209cd085c7113 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 13:48:55 +0100 Subject: [PATCH 55/81] Sniff::is_assignment(): minor defensive coding tweak There "bracket_closer" index may not be set during live coding/in case of parse errors. This prevents the method from throwing "_Undefined index: bracket_closer_" notices in that case. --- WordPress/Sniff.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 7359f145df..2a4805fdee 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1321,7 +1321,9 @@ protected function is_assignment( $stackPtr ) { } // Check if this is an array assignment, e.g., `$var['key'] = 'val';` . - if ( \T_OPEN_SQUARE_BRACKET === $this->tokens[ $next_non_empty ]['code'] ) { + if ( \T_OPEN_SQUARE_BRACKET === $this->tokens[ $next_non_empty ]['code'] + && isset( $this->tokens[ $next_non_empty ]['bracket_closer'] ) + ) { return $this->is_assignment( $this->tokens[ $next_non_empty ]['bracket_closer'] ); } From 4e143ec158294a9c44b6d29801c0a3d898ac56aa Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 31 Mar 2019 11:57:00 +0200 Subject: [PATCH 56/81] Sniff::is_in_function_call(): expand the function documentation with some examples --- WordPress/Sniff.php | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 5fa62c1310..171a55fcca 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1539,6 +1539,14 @@ protected function is_token_namespaced( $stackPtr ) { /** * Check if a token is (part of) a parameter for a function call to a select list of functions. * + * This is useful, for instance, when trying to determine the context a variable is used in. + * + * For example: this function could be used to determine if the variable `$foo` is used + * in a global function call to the function `is_foo()`. + * In that case, a call to this function would return the stackPtr to the T_STRING `is_foo` + * for code like: `is_foo( $foo, 'some_other_param' )`, while it would return `false` for + * the following code `is_bar( $foo, 'some_other_param' )`. + * * @since 2.1.0 * * @param int $stackPtr The index of the token in the stack. @@ -1546,8 +1554,10 @@ protected function is_token_namespaced( $stackPtr ) { * Note: The keys to this array should be the function names * in lowercase. Values are irrelevant. * @param bool $global Optional. Whether to make sure that the function call is - * to a global function. If `false`, methods and namespaced - * function calls will also be allowed. + * to a global function. If `false`, calls to methods, be it static + * `Class::method()` or via an object `$obj->method()`, and + * namespaced function calls, like `MyNS\function_name()` will + * also be accepted. * Defaults to `true`. * @param bool $allow_nested Optional. Whether to allow for nested function calls within the * call to this function. From c088b1bcfcb7a97ee7cac89ab59a1675c78f9859 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 01:01:05 +0100 Subject: [PATCH 57/81] Sniff::$unslashingSanitizingFunctions: add doubleval() and count() While `doubleval()` is an alias of `floatval()` and shouldn't be used, for the purposes of the ValidatedSanitizedInput sniff, both functions should be recognized. And as `count()` doesn't actually access the data in the variable, but only counts the number of elements, it is also safe to use without unslashing/sanitizing the variable beforehand. Same goes for the `sizeof()` alias of `count()`. Includes unit tests. Fixes 1659 --- WordPress/Sniff.php | 3 +++ .../Tests/Security/ValidatedSanitizedInputUnitTest.inc | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index b00c25713c..8583d81f8b 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -309,10 +309,13 @@ abstract class Sniff implements PHPCS_Sniff { protected $unslashingSanitizingFunctions = array( 'absint' => true, 'boolval' => true, + 'count' => true, + 'doubleval' => true, 'floatval' => true, 'intval' => true, 'is_array' => true, 'sanitize_key' => true, + 'sizeof' => true, ); /** diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index 57db08843f..4b42017e65 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -216,3 +216,12 @@ if ( isset( $_GET['unslash_check'] ) ) { $clean = sanitize_text_field( WP_Faker::wp_unslash( $_GET['unslash_check'] ) ); // Bad x1 - unslash. $clean = WP_Faker\sanitize_text_field( wp_unslash( $_GET['unslash_check'] ) ); // Bad x1 - sanitize. } + +function test_more_safe_functions() { + if ( ! isset( $_GET['test'] ) ) { + return; + } + + $float = doubleval( $_GET['test'] ); // OK. + $count = count( $_GET['test'] ); // Issue #1659; OK. +} From 34920c722d6b82029ccbaad8d3288aecdf48d6dd Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 31 Mar 2019 13:49:29 +0200 Subject: [PATCH 58/81] Update default minimum_supported_version to WP 4.8 The minimum version should be three versions behind the latest WP release, so what with 5.1 being out, it should now be 4.8. Includes updating the list of deprecated functions. Input for this based on @JDGrimes's WP deprecated code scanner. --- WordPress/Sniff.php | 2 +- .../Sniffs/WP/DeprecatedFunctionsSniff.php | 15 +++++++ .../Tests/WP/DeprecatedFunctionsUnitTest.inc | 12 ++++-- .../Tests/WP/DeprecatedFunctionsUnitTest.php | 39 ++++++++++--------- .../Tests/WP/DeprecatedParametersUnitTest.inc | 4 +- .../Tests/WP/DeprecatedParametersUnitTest.php | 3 +- phpcs.xml.dist.sample | 2 +- 7 files changed, 48 insertions(+), 29 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index b00c25713c..b18fed30d6 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -82,7 +82,7 @@ abstract class Sniff implements PHPCS_Sniff { * * @var string WordPress version. */ - public $minimum_supported_version = '4.7'; + public $minimum_supported_version = '4.8'; /** * Custom list of classes which test classes can extend. diff --git a/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php b/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php index 5c2be16c07..8525dc33bb 100644 --- a/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php +++ b/WordPress/Sniffs/WP/DeprecatedFunctionsSniff.php @@ -579,6 +579,11 @@ class DeprecatedFunctionsSniff extends AbstractFunctionRestrictionsSniff { 'alt' => 'wp_die()', 'version' => '3.0.0', ), + // Verified version & alternative. + 'install_blog_defaults' => array( + 'alt' => 'wp_install_defaults', + 'version' => '3.0.0', + ), 'is_main_blog' => array( 'alt' => 'is_main_site()', 'version' => '3.0.0', @@ -1330,6 +1335,16 @@ class DeprecatedFunctionsSniff extends AbstractFunctionRestrictionsSniff { 'alt' => '', 'version' => '4.9.0', ), + + // WP 5.1.0. + 'insert_blog' => array( + 'alt' => 'wp_insert_site()', + 'version' => '5.1.0', + ), + 'install_blog' => array( + 'alt' => '', + 'version' => '5.1.0', + ), ); /** diff --git a/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.inc b/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.inc index 55b5107e2f..eb0822692e 100644 --- a/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.inc +++ b/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.inc @@ -143,6 +143,7 @@ get_user_details(); get_usermeta(); get_usernumposts(); graceful_fail(); +install_blog_defaults(); is_main_blog(); is_site_admin(); is_taxonomy(); @@ -319,10 +320,6 @@ use function popuplinks as something_else; // Related to issue #1306. post_form_autocomplete_off(); wp_embed_handler_googlevideo(); wp_get_sites(); - -/* - * Warning. - */ /* ============ WP 4.7 ============ */ _sort_nav_menu_items(); _usort_terms_by_ID(); @@ -330,6 +327,10 @@ _usort_terms_by_name(); get_paged_template(); wp_get_network(); wp_kses_js_entities(); + +/* + * Warning. + */ /* ============ WP 4.8 ============ */ wp_dashboard_plugins_output(); /* ============ WP 4.9 ============ */ @@ -337,3 +338,6 @@ get_shortcut_link(); is_user_option_local(); wp_ajax_press_this_add_category(); wp_ajax_press_this_save_post(); +/* ============ WP 5.1 ============ */ +insert_blog(); +install_blog(); diff --git a/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.php b/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.php index 5fbce22ce0..c14a79c9bd 100644 --- a/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.php +++ b/WordPress/Tests/WP/DeprecatedFunctionsUnitTest.php @@ -28,7 +28,7 @@ class DeprecatedFunctionsUnitTest extends AbstractSniffUnitTest { */ public function getErrorList() { - $errors = array_fill( 8, 314, 1 ); + $errors = array_fill( 8, 322, 1 ); // Unset the lines related to version comments. unset( @@ -45,22 +45,23 @@ public function getErrorList() { $errors[80], $errors[118], $errors[125], - $errors[161], - $errors[174], - $errors[178], - $errors[210], - $errors[233], - $errors[251], - $errors[255], - $errors[262], - $errors[274], - $errors[281], - $errors[285], - $errors[290], - $errors[295], - $errors[303], - $errors[310], - $errors[318] + $errors[162], + $errors[175], + $errors[179], + $errors[211], + $errors[234], + $errors[252], + $errors[256], + $errors[263], + $errors[275], + $errors[282], + $errors[286], + $errors[291], + $errors[296], + $errors[304], + $errors[311], + $errors[319], + $errors[323] ); return $errors; @@ -73,10 +74,10 @@ public function getErrorList() { */ public function getWarningList() { - $warnings = array_fill( 326, 14, 1 ); + $warnings = array_fill( 335, 9, 1 ); // Unset the lines related to version comments. - unset( $warnings[326], $warnings[333], $warnings[335] ); + unset( $warnings[336], $warnings[341] ); return $warnings; } diff --git a/WordPress/Tests/WP/DeprecatedParametersUnitTest.inc b/WordPress/Tests/WP/DeprecatedParametersUnitTest.inc index 7e35ad422a..8022d59bc5 100644 --- a/WordPress/Tests/WP/DeprecatedParametersUnitTest.inc +++ b/WordPress/Tests/WP/DeprecatedParametersUnitTest.inc @@ -48,6 +48,7 @@ the_author( 'deprecated', 'deprecated' ); the_author_posts_link( 'deprecated' ); trackback_rdf( 'deprecated' ); trackback_url( 'deprecated' ); +unregister_setting( '', '', '', 'deprecated' ); update_blog_option( '', '', '', 'deprecated' ); update_user_status( '', '', '', 'deprecated' ); wp_get_http_headers( '', 'deprecated' ); @@ -60,7 +61,6 @@ wp_title_rss( 'deprecated' ); wp_upload_bits( '', 'deprecated' ); xfn_check( '', '', 'deprecated' ); -// All will give an WARNING as they have been deprecated after WP 4.7. +// All will give an WARNING as they have been deprecated after WP 4.8. get_category_parents( '', '', '', '', array( 'deprecated') ); -unregister_setting( '', '', '', 'deprecated' ); diff --git a/WordPress/Tests/WP/DeprecatedParametersUnitTest.php b/WordPress/Tests/WP/DeprecatedParametersUnitTest.php index ffbc573474..77d4d87e88 100644 --- a/WordPress/Tests/WP/DeprecatedParametersUnitTest.php +++ b/WordPress/Tests/WP/DeprecatedParametersUnitTest.php @@ -27,7 +27,7 @@ class DeprecatedParametersUnitTest extends AbstractSniffUnitTest { * @return array => */ public function getErrorList() { - $errors = array_fill( 28, 34, 1 ); + $errors = array_fill( 28, 35, 1 ); $errors[22] = 1; $errors[23] = 1; @@ -47,7 +47,6 @@ public function getErrorList() { */ public function getWarningList() { return array( - 65 => 1, 66 => 1, ); } diff --git a/phpcs.xml.dist.sample b/phpcs.xml.dist.sample index ef8dae68b1..ffe70c7f1a 100644 --- a/phpcs.xml.dist.sample +++ b/phpcs.xml.dist.sample @@ -70,7 +70,7 @@ the wiki: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties --> - + From 38944372232d285de12cd9da9292b0cd7e3dae1e Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 13:33:35 +0100 Subject: [PATCH 59/81] ValidatedSanitizedInput: allow for validation using key_exists() This builds onto the previous enhancement made in 1635 which started recognizing `array_key_exists()` as a way to validate a variable. `key_exists()` is an alias for `array_key_exists()` and while alias functions shouldn't be used, for the purposes of the ValidatedSanitizedInput sniff, both functions should be recognized. Includes unit test. Notes: * Removes the `array_key_exists()` method from the list of `$sanitizingFunctions` as it doesn't belong there and is now handled differently anyway (this should have been removed in 1635). * Updates the version numbers for the change in the method documentation. We never released version 2.0.1, so both this change as well as the one from 1635 will now be released in 2.1.0. --- WordPress/Sniff.php | 23 ++++++++++++------- .../ValidatedSanitizedInputUnitTest.inc | 5 ++++ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index da69c52adb..69eaddc60c 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -252,7 +252,6 @@ abstract class Sniff implements PHPCS_Sniff { */ protected $sanitizingFunctions = array( '_wp_handle_upload' => true, - 'array_key_exists' => true, 'esc_url_raw' => true, 'filter_input' => true, 'filter_var' => true, @@ -1437,8 +1436,8 @@ protected function has_nonce_check( $stackPtr ) { * Check if a token is inside of an isset(), empty() or array_key_exists() statement. * * @since 0.5.0 - * @since 2.0.1 Now checks for the token being used as the array parameter - * in function calls to array_key_exists() as well. + * @since 2.1.0 Now checks for the token being used as the array parameter + * in function calls to array_key_exists() and key_exists() as well. * * @param int $stackPtr The index of the token in the stack. * @@ -1465,7 +1464,12 @@ protected function is_in_isset_or_empty( $stackPtr ) { return true; } - $functionPtr = $this->is_in_function_call( $stackPtr, array( 'array_key_exists' => true ) ); + $valid_functions = array( + 'array_key_exists' => true, + 'key_exists' => true, // Alias. + ); + + $functionPtr = $this->is_in_function_call( $stackPtr, $valid_functions ); if ( false !== $functionPtr ) { $second_param = $this->get_function_call_parameter( $functionPtr, 2 ); if ( $stackPtr >= $second_param['start'] && $stackPtr <= $second_param['end'] ) { @@ -1845,7 +1849,8 @@ protected function get_array_access_key( $stackPtr ) { } /** - * Check if the existence of a variable is validated with isset(), empty() or array_key_exists(). + * Check if the existence of a variable is validated with isset(), empty(), array_key_exists() + * or key_exists(). * * When $in_condition_only is false, (which is the default), this is considered * valid: @@ -1868,7 +1873,7 @@ protected function get_array_access_key( $stackPtr ) { * ``` * * @since 0.5.0 - * @since 2.0.1 Now recognizes array_key_exists() as a validation function. + * @since 2.1.0 Now recognizes array_key_exists() and key_exists() as validation functions. * * @param int $stackPtr The index of this token in the stack. * @param string $array_key An array key to check for ("bar" in $foo['bar']). @@ -1982,8 +1987,10 @@ protected function is_validated( $stackPtr, $array_key = null, $in_condition_onl break; case 'function_call': - // Only check calls to array_key_exists(). - if ( 'array_key_exists' !== $this->tokens[ $i ]['content'] ) { + // Only check calls to array_key_exists() and key_exists(). + if ( 'array_key_exists' !== $this->tokens[ $i ]['content'] + && 'key_exists' !== $this->tokens[ $i ]['content'] + ) { continue 2; } diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index 4b42017e65..a4059d5b09 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -225,3 +225,8 @@ function test_more_safe_functions() { $float = doubleval( $_GET['test'] ); // OK. $count = count( $_GET['test'] ); // Issue #1659; OK. } + +function test_allow_array_key_exists_alias() { + if ( key_exists( 'my_field1', $_POST ) ) { + $id = (int) $_POST['my_field1']; // OK. +} From 866436418cec28311be4b3dd0542e8ba63832a14 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 29 Mar 2019 06:42:52 +0100 Subject: [PATCH 60/81] Sniff: add new get_array_access_keys() utility method ... to retrieve _all_ array keys for a multi-level array access. The original `Sniff::get_array_access_key()` method defers to the new method under the hood, however, the behaviour of that method has not changed. --- WordPress/Sniff.php | 74 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 16 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 69eaddc60c..837d772f99 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1814,12 +1814,66 @@ public function add_unslash_error( $stackPtr ) { ); } + /** + * Get the index keys of an array variable. + * + * E.g., "bar" and "baz" in $foo['bar']['baz']. + * + * @since 2.1.0 + * + * @param int $stackPtr The index of the variable token in the stack. + * @param bool $all Whether to get all keys or only the first. + * Defaults to `true`(= all). + * + * @return array An array of index keys whose value is being accessed. + * or an empty array if this is not array access. + */ + protected function get_array_access_keys( $stackPtr, $all = true ) { + + $keys = array(); + + if ( \T_VARIABLE !== $this->tokens[ $stackPtr ]['code'] ) { + return $keys; + } + + $current = $stackPtr; + + do { + // Find the next non-empty token. + $open_bracket = $this->phpcsFile->findNext( + Tokens::$emptyTokens, + ( $current + 1 ), + null, + true + ); + + // If it isn't a bracket, this isn't an array-access. + if ( false === $open_bracket + || \T_OPEN_SQUARE_BRACKET !== $this->tokens[ $open_bracket ]['code'] + || ! isset( $this->tokens[ $open_bracket ]['bracket_closer'] ) + ) { + break; + } + + $key = $this->phpcsFile->getTokensAsString( + ( $open_bracket + 1 ), + ( $this->tokens[ $open_bracket ]['bracket_closer'] - $open_bracket - 1 ) + ); + + $keys[] = trim( $key ); + $current = $this->tokens[ $open_bracket ]['bracket_closer']; + } while ( isset( $this->tokens[ $current ] ) && true === $all ); + + return $keys; + } + /** * Get the index key of an array variable. * * E.g., "bar" in $foo['bar']. * * @since 0.5.0 + * @since 2.1.0 Now uses get_array_access_keys() under the hood. * * @param int $stackPtr The index of the token in the stack. * @@ -1827,25 +1881,13 @@ public function add_unslash_error( $stackPtr ) { */ protected function get_array_access_key( $stackPtr ) { - // Find the next non-empty token. - $open_bracket = $this->phpcsFile->findNext( - Tokens::$emptyTokens, - ( $stackPtr + 1 ), - null, - true - ); + $keys = $this->get_array_access_keys( $stackPtr, false ); - // If it isn't a bracket, this isn't an array-access. - if ( false === $open_bracket || \T_OPEN_SQUARE_BRACKET !== $this->tokens[ $open_bracket ]['code'] ) { - return false; + if ( isset( $keys[0] ) ) { + return $keys[0]; } - $key = $this->phpcsFile->getTokensAsString( - ( $open_bracket + 1 ), - ( $this->tokens[ $open_bracket ]['bracket_closer'] - $open_bracket - 1 ) - ); - - return trim( $key ); + return false; } /** From a598b08f544155d04f35d9e82e058ae233b894a0 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 29 Mar 2019 08:17:18 +0100 Subject: [PATCH 61/81] ValidatedSanitizedInput/Sniff::is_validated(): allow for multi-level array key check This commit builds onto earlier improvements to this sniff via PR 1634 and 1635 This implements the use of the new `get_array_access_keys()` method in the `WordPress.Security.ValidatedSanitizedInput` sniff and the `Sniff::is_validated()` method. This allow for more precise checking of whether the correct variable has been validated properly before use. This should prevent some false positives as well as false negatives. Includes unit tests. --- WordPress/Sniff.php | 75 +++++++++++++++---- .../Security/ValidatedSanitizedInputSniff.php | 6 +- .../ValidatedSanitizedInputUnitTest.inc | 27 +++++++ .../ValidatedSanitizedInputUnitTest.php | 5 ++ 4 files changed, 94 insertions(+), 19 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 837d772f99..0589796ba5 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1916,17 +1916,20 @@ protected function get_array_access_key( $stackPtr ) { * * @since 0.5.0 * @since 2.1.0 Now recognizes array_key_exists() and key_exists() as validation functions. + * @since 2.1.0 Stricter check on whether the correct variable and the correct + * array keys are being validated. * - * @param int $stackPtr The index of this token in the stack. - * @param string $array_key An array key to check for ("bar" in $foo['bar']). - * @param bool $in_condition_only Whether to require that this use of the - * variable occur within the scope of the - * validating condition, or just in the same - * scope as it (default). + * @param int $stackPtr The index of this token in the stack. + * @param array|string $array_keys An array key to check for ("bar" in $foo['bar']) + * or an array of keys for multi-level array access. + * @param bool $in_condition_only Whether to require that this use of the + * variable occur within the scope of the + * validating condition, or just in the same + * scope as it (default). * * @return bool Whether the var is validated. */ - protected function is_validated( $stackPtr, $array_key = null, $in_condition_only = false ) { + protected function is_validated( $stackPtr, $array_keys = array(), $in_condition_only = false ) { if ( $in_condition_only ) { /* @@ -1977,11 +1980,14 @@ protected function is_validated( $stackPtr, $array_key = null, $in_condition_onl } $scope_end = $stackPtr; + } + if ( ! empty( $array_keys ) && ! is_array( $array_keys ) ) { + $array_keys = (array) $array_keys; } - $bare_array_key = $this->strip_quotes( $array_key ); - $targets = array( + $bare_array_keys = array_map( array( $this, 'strip_quotes' ), $array_keys ); + $targets = array( \T_ISSET => 'construct', \T_EMPTY => 'construct', \T_UNSET => 'construct', @@ -2016,11 +2022,15 @@ protected function is_validated( $stackPtr, $array_key = null, $in_condition_onl continue; } - // If we're checking for a specific array key (ex: 'hello' in + // If we're checking for specific array keys (ex: 'hello' in // $_POST['hello']), that must match too. Quote-style, however, doesn't matter. - if ( isset( $array_key ) - && $this->strip_quotes( $this->get_array_access_key( $i ) ) !== $bare_array_key ) { - continue; + if ( ! empty( $bare_array_keys ) ) { + $found_keys = $this->get_array_access_keys( $i ); + $found_keys = array_map( array( $this, 'strip_quotes' ), $found_keys ); + $diff = array_diff_assoc( $bare_array_keys, $found_keys ); + if ( ! empty( $diff ) ) { + continue; + } } return true; @@ -2053,12 +2063,45 @@ protected function is_validated( $stackPtr, $array_key = null, $in_condition_onl } $params = $this->get_function_call_parameters( $i ); - if ( $params[2]['raw'] !== $this->tokens[ $stackPtr ]['content'] ) { + if ( count( $params ) < 2 ) { continue 2; } - if ( isset( $array_key ) - && $this->strip_quotes( $params[1]['raw'] ) !== $bare_array_key ) { + $param2_first_token = $this->phpcsFile->findNext( Tokens::$emptyTokens, $params[2]['start'], ( $params[2]['end'] + 1 ), true ); + if ( false === $param2_first_token + || \T_VARIABLE !== $this->tokens[ $param2_first_token ]['code'] + || $this->tokens[ $param2_first_token ]['content'] !== $this->tokens[ $stackPtr ]['content'] + ) { + continue 2; + } + + if ( ! empty( $bare_array_keys ) ) { + // Prevent the original array from being altered. + $bare_keys = $bare_array_keys; + $last_key = array_pop( $bare_keys ); + + /* + * For multi-level array access, the complete set of keys could be split between + * the first and the second parameter, but could also be completely in the second + * parameter, so we need to check both options. + */ + + $found_keys = $this->get_array_access_keys( $param2_first_token ); + $found_keys = array_map( array( $this, 'strip_quotes' ), $found_keys ); + + // First try matching the complete set against the second parameter. + $diff = array_diff_assoc( $bare_array_keys, $found_keys ); + if ( empty( $diff ) ) { + return true; + } + + // If that failed, try getting an exact match for the subset against the + // second parameter and the last key against the first. + if ( $bare_keys === $found_keys && $this->strip_quotes( $params[1]['raw'] ) === $last_key ) { + return true; + } + + // Didn't find the correct array keys. continue 2; } diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index 2f5ca051b1..6fa904bc80 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -123,16 +123,16 @@ function ( $symbol ) { return; } - $array_key = $this->get_array_access_key( $stackPtr ); + $array_keys = $this->get_array_access_keys( $stackPtr ); - if ( empty( $array_key ) ) { + if ( empty( $array_keys ) ) { return; } $error_data = array( $this->tokens[ $stackPtr ]['content'] ); // Check for validation first. - if ( ! $this->is_validated( $stackPtr, $array_key, $this->check_validation_in_scope_only ) ) { + if ( ! $this->is_validated( $stackPtr, $array_keys, $this->check_validation_in_scope_only ) ) { $this->phpcsFile->addError( 'Detected usage of a non-validated input variable: %s', $stackPtr, diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index a4059d5b09..1cce83dc39 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -230,3 +230,30 @@ function test_allow_array_key_exists_alias() { if ( key_exists( 'my_field1', $_POST ) ) { $id = (int) $_POST['my_field1']; // OK. } + +function test_correct_multi_level_array_validation() { + if ( isset( $_POST['toplevel']['sublevel'] ) ) { + $id = (int) $_POST['toplevel']; // OK, if a subkey exists, the top level key *must* also exist. + $id = (int) $_POST['toplevel']['sublevel']; // OK. + $id = (int) $_POST['toplevel']['sublevel']['subsub']; // Bad x1 - validate, this sub has not been validated. + } + + if ( array_key_exists( 'bar', $_POST['foo'] ) ) { + $id = (int) $_POST['bar']; // Bad x 1 - validate. + $id = (int) $_POST['foo']; // OK. + $id = (int) $_POST['foo']['bar']; // OK. + $id = (int) $_POST['foo']['bar']['baz']; // Bad x 1 - validate. + } +} + +function test_correct_multi_level_array_validation_key_order() { + if ( isset( $_POST['toplevel']['sublevel'] ) ) { + $id = (int) $_POST['sublevel']['toplevel']; // Bad x 1 - validate - key order is wrong. + } +} + +function test_invalid_array_key_exists_call() { + if ( array_key_exists( 'bar' ) ) { + $id = (int) $_POST['bar']; // Bad x 1 - validate. + } +} diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php index 430b278b81..06886006a1 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php @@ -60,6 +60,11 @@ public function getErrorList() { 210 => 1, 216 => 1, 217 => 1, + 238 => 1, + 242 => 1, + 245 => 1, + 251 => 1, + 257 => 1, ); } From b4fb6babb791b0dfcfe2d9294676b34619cdf34d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 16:42:00 +0100 Subject: [PATCH 62/81] ValidatedSanitizedInput: make the error messages more informative This changes two things: 1. For both the `InputNotValidated` as well as the `InputNotSanitized` error, it will now display the array keys for the variable triggering the error. Previously, the message would just say `$_POST`, now it will say `$_POST['foo']['bar']`. 2. For the `InputNotValidated`, the error message text has been expanded to make it more obvious how to fix this issue. Fixes 1541 --- WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index 6fa904bc80..2e12075997 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -129,12 +129,12 @@ function ( $symbol ) { return; } - $error_data = array( $this->tokens[ $stackPtr ]['content'] ); + $error_data = array( $this->tokens[ $stackPtr ]['content'] . '[' . implode( '][', $array_keys ) . ']' ); // Check for validation first. if ( ! $this->is_validated( $stackPtr, $array_keys, $this->check_validation_in_scope_only ) ) { $this->phpcsFile->addError( - 'Detected usage of a non-validated input variable: %s', + 'Detected usage of a possibly undefined superglobal array index: %s. Use isset() or empty() to check the index exists before using it', $stackPtr, 'InputNotValidated', $error_data From 05a71fff27071017fba48ca0f8b25926c76f35d0 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 28 Mar 2019 01:22:30 +0100 Subject: [PATCH 63/81] Security: allow for type testing superglobals When a superglobal variable is being tested with, for instance, `is_numeric()`, unslashing or sanitization are not needed and it's ok for the nonce check to be done after it. This is completely safe. Ref: https://php.net/manual/en/ref.var.php Using these type testing functions should however not be regarded as a way of sanitizing/unslashing the variable and the variable does still need to be validated before being passed to one of these functions. To test whether a variable is used in one of these functions, a new `is_in_type_test()` method has been added to the `WordPressCS\Sniff` class, along with a `$typeTestFunctions` property containing the names of the functions this applies to. Notes: * The `is_array()` function which was previously, incorrectly, listed in the `$unslashingSanitizingFunctions` list has been moved to the new property. `is_array()` does not unslash or sanitize the contents of a variable, it only checks the variable type. * Implemented the use of the new `Sniff::is_in_type_test()` method in both the `ValidatedSanitizedInput` sniff, as well as in the `Sniff:has_nonce_check()` method for the `NonceVerification` sniff. Includes unit tests via the sniffs. --- WordPress/Sniff.php | 68 +++++++++++++++++-- .../Security/ValidatedSanitizedInputSniff.php | 5 ++ .../Security/NonceVerificationUnitTest.inc | 20 +++++- .../Security/NonceVerificationUnitTest.php | 1 + .../ValidatedSanitizedInputUnitTest.inc | 10 +++ .../ValidatedSanitizedInputUnitTest.php | 1 + 6 files changed, 97 insertions(+), 8 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 0589796ba5..f1dfe5a545 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -312,11 +312,42 @@ abstract class Sniff implements PHPCS_Sniff { 'doubleval' => true, 'floatval' => true, 'intval' => true, - 'is_array' => true, 'sanitize_key' => true, 'sizeof' => true, ); + /** + * List of PHP native functions to test the type of a variable. + * + * Using these functions is safe in combination with superglobals without + * unslashing or sanitization. + * + * They should, however, not be regarded as unslashing or sanitization functions. + * + * @since 2.1.0 + * + * @var array + */ + protected $typeTestFunctions = array( + 'is_array' => true, + 'is_bool' => true, + 'is_callable' => true, + 'is_countable' => true, + 'is_double' => true, + 'is_float' => true, + 'is_int' => true, + 'is_integer' => true, + 'is_iterable' => true, + 'is_long' => true, + 'is_null' => true, + 'is_numeric' => true, + 'is_object' => true, + 'is_real' => true, + 'is_resource' => true, + 'is_scalar' => true, + 'is_string' => true, + ); + /** * Token which when they preceed code indicate the value is safely casted. * @@ -1372,12 +1403,17 @@ protected function has_nonce_check( $stackPtr ) { } } - $in_isset = $this->is_in_isset_or_empty( $stackPtr ); + $allow_nonce_after = false; + if ( $this->is_in_isset_or_empty( $stackPtr ) + || $this->is_in_type_test( $stackPtr ) + ) { + $allow_nonce_after = true; + } - // We allow for isset( $_POST['var'] ) checks to come before the nonce check. - // If this is inside an isset(), check after it as well, all the way to the - // end of the scope. - if ( $in_isset ) { + // We allow for certain actions, such as an isset() check to come before the nonce check. + // If this superglobal is inside such a check, look for the nonce after it as well, + // all the way to the end of the scope. + if ( true === $allow_nonce_after ) { $end = ( 0 === $start ) ? $this->phpcsFile->numTokens : $tokens[ $start ]['scope_closer']; } @@ -1393,7 +1429,7 @@ protected function has_nonce_check( $stackPtr ) { // If we have already found an nonce check in this scope, we just // need to check whether it comes before this token. It is OK if the // check is after the token though, if this was only a isset() check. - return ( $in_isset || $last['nonce_check'] < $stackPtr ); + return ( true === $allow_nonce_after || $last['nonce_check'] < $stackPtr ); } elseif ( $end <= $last['end'] ) { // If not, we can still go ahead and return false if we've already // checked to the end of the search area. @@ -1624,6 +1660,24 @@ protected function is_in_function_call( $stackPtr, $valid_functions, $global = t return false; } + /** + * Check if a token is inside of an is_...() statement. + * + * @since 2.1.0 + * + * @param int $stackPtr The index of the token in the stack. + * + * @return bool Whether the token is being type tested. + */ + protected function is_in_type_test( $stackPtr ) { + /* + * Casting the potential integer stack pointer return value to boolean here is fine. + * The return can never be `0` as there will always be a PHP open tag before the + * function call. + */ + return (bool) $this->is_in_function_call( $stackPtr, $this->typeTestFunctions ); + } + /** * Check if something is only being sanitized. * diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index 6fa904bc80..b7ac24d329 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -145,6 +145,11 @@ function ( $symbol ) { return; } + // If this variable is being tested with one of the `is_..()` functions, sanitization isn't needed. + if ( $this->is_in_type_test( $stackPtr ) ) { + return; + } + // If this is a comparison ('a' == $_POST['foo']), sanitization isn't needed. if ( $this->is_comparison( $stackPtr ) ) { return; diff --git a/WordPress/Tests/Security/NonceVerificationUnitTest.inc b/WordPress/Tests/Security/NonceVerificationUnitTest.inc index b5387355de..652eb3afa7 100644 --- a/WordPress/Tests/Security/NonceVerificationUnitTest.inc +++ b/WordPress/Tests/Security/NonceVerificationUnitTest.inc @@ -17,7 +17,7 @@ function ajax_process() { } add_action( 'wp_ajax_process', 'ajax_process' ); -// It's also OK to check with isset() before the the nonce check. +// It's also OK to check with isset() before the nonce check. function foo() { if ( ! isset( $_POST['test'] ) || ! wp_verify_nonce( 'some_action' ) ) { exit; @@ -160,3 +160,21 @@ function foo_8() { sanitize_pc( $_POST['bar'] ); // Bad. my_nonce_check( sanitize_twitter( $_POST['tweet'] ) ); // Bad. } + +/* + * Using a superglobal in a is_...() function is OK as long as a nonce check is done + * before the variable is *really* used. + */ +function test_whitelisting_use_in_type_test_functions() { + if ( ! is_numeric ( $_POST['foo'] ) ) { // OK. + return; + } + + wp_verify_nonce( 'some_action' ); +} + +function test_incorrect_use_in_type_test_functions() { + if ( ! is_numeric ( $_POST['foo'] ) ) { // Bad. + return; + } +} diff --git a/WordPress/Tests/Security/NonceVerificationUnitTest.php b/WordPress/Tests/Security/NonceVerificationUnitTest.php index d937edb1e9..63ecbcdf37 100644 --- a/WordPress/Tests/Security/NonceVerificationUnitTest.php +++ b/WordPress/Tests/Security/NonceVerificationUnitTest.php @@ -46,6 +46,7 @@ public function getErrorList() { 159 => 1, 160 => 1, 161 => 1, + 177 => 1, ); } diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index 1cce83dc39..a607e375d7 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -257,3 +257,13 @@ function test_invalid_array_key_exists_call() { $id = (int) $_POST['bar']; // Bad x 1 - validate. } } + +function test_ignoring_is_type_function_calls() { + // Usage within variable type tests does not need unslashing or sanitization. + if ( isset( $_POST[ $key ] ) && is_numeric( $_POST[ $key ] ) ) {} // OK. + if ( isset($_POST['plugin']) && is_string( $_POST['plugin'] ) ) {} // OK. + + if ( ! is_null( $_GET['not_set'] ) ) {} // Bad x1 - missing validation. + if ( array_key_exists( 'null', $_GET ) && ! is_null( $_GET['null'] ) ) {} // OK. + if ( array_key_exists( 'null', $_POST ) && $_POST['null'] !== null ) {} // OK. +} diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php index 06886006a1..e96a623db0 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php @@ -65,6 +65,7 @@ public function getErrorList() { 245 => 1, 251 => 1, 257 => 1, + 266 => 1, ); } From 7841dab2325e539e1722d752ca35e9f056d7f0c3 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 15:31:28 +0100 Subject: [PATCH 64/81] Sniff::is_sanitized(): allow for map_deep() to sanitize arrays **Important note:** Functions which alter the array by reference, such as the PHP native `array_walk()` and `array_walk_recursive()` are not listed in the new `$arrayWalkingFunctions` property on purpose. * These cannot easily be used for sanitization as they can't be combined with unslashing, so could only be used in combination with the `$unslashingSanitizingFunctions`. Additionally as they alter the array by reference, at this moment, accessing the array after the sanitization through one of these function would _still_ trigger errors. If at some point in the future, the above combination would be accounted for, preventing these additional errors would also need to be addressed. * Similarly, they cannot be used for late escaping as the return value is a boolean, not the altered array. Also see my comment in the original issue where I go in slightly more detail: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/ 1660#issuecomment-478254403 Includes unit test. Fixes 1660 Fixes 1618 Once this has been merged, the [Sanitizing array input data](https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Sanitizing-array-input-data) wiki page should probably be updated (or an issue be opened saying that it should be with a link to this PR and the tickets). --- WordPress/Sniff.php | 29 +++++++++++++++---- .../ValidatedSanitizedInputUnitTest.inc | 9 ++++++ .../ValidatedSanitizedInputUnitTest.php | 1 + 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index f1dfe5a545..7786624c25 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -361,6 +361,25 @@ abstract class Sniff implements PHPCS_Sniff { \T_BOOL_CAST => true, ); + /** + * List of array functions which apply a callback to the array. + * + * These are often used for sanitization/escaping an array variable. + * + * Note: functions which alter the array by reference are not listed here on purpose. + * These cannot easily be used for sanitization as they can't be combined with unslashing. + * Similarly, they cannot be used for late escaping as the return value is a boolean, not + * the altered array. + * + * @since 2.1.0 + * + * @var array => + */ + protected $arrayWalkingFunctions = array( + 'array_map' => 1, + 'map_deep' => 2, + ); + /** * Functions that format strings. * @@ -1778,8 +1797,8 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { $valid_functions = $this->sanitizingFunctions; $valid_functions += $this->unslashingSanitizingFunctions; + $valid_functions += $this->arrayWalkingFunctions; $valid_functions['wp_unslash'] = true; - $valid_functions['array_map'] = true; $functionPtr = $this->is_in_function_call( $stackPtr, $valid_functions ); @@ -1814,11 +1833,11 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { $is_unslashed = false; } - // Arrays might be sanitized via array_map(). - if ( 'array_map' === $functionName ) { + // Arrays might be sanitized via an array walking function using a callback. + if ( isset( $this->arrayWalkingFunctions[ $functionName ] ) ) { - // Get the first parameter. - $callback = $this->get_function_call_parameter( $functionPtr, 1 ); + // Get the callback parameter. + $callback = $this->get_function_call_parameter( $functionPtr, $this->arrayWalkingFunctions[ $functionName ] ); if ( ! empty( $callback ) ) { /* diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index a607e375d7..84b4c16fed 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -267,3 +267,12 @@ function test_ignoring_is_type_function_calls() { if ( array_key_exists( 'null', $_GET ) && ! is_null( $_GET['null'] ) ) {} // OK. if ( array_key_exists( 'null', $_POST ) && $_POST['null'] !== null ) {} // OK. } + +function test_additional_array_walking_functions() { + if ( ! isset( $_GET['test'] ) ) { + return; + } + + $sane = map_deep( wp_unslash( $_GET['test'] ), 'sanitize_text_field' ); // Ok. + $sane = map_deep( wp_unslash( $_GET['test'] ), 'foo' ); // Bad. +} diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php index e96a623db0..74ff09a7d3 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php @@ -66,6 +66,7 @@ public function getErrorList() { 251 => 1, 257 => 1, 266 => 1, + 277 => 1, ); } From d0f7084b4c263ba22f249bf6f0ca02f409ce80f8 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 31 Mar 2019 06:52:46 +0200 Subject: [PATCH 65/81] Sniff::has_nonce_check(): add new `is_class_object_call()` and `is_token_namespaced()` checks This fixes potential false negatives when a method/namespaced function mirroring the name of the WP nonc verification functions would be used. Includes unit tests. --- WordPress/Sniff.php | 11 +++++++++++ .../Tests/Security/NonceVerificationUnitTest.inc | 11 +++++++++++ .../Tests/Security/NonceVerificationUnitTest.php | 2 ++ 3 files changed, 24 insertions(+) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index f1dfe5a545..7b3e29b658 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1457,6 +1457,17 @@ protected function has_nonce_check( $stackPtr ) { // If this is one of the nonce verification functions, we can bail out. if ( isset( $this->nonceVerificationFunctions[ $tokens[ $i ]['content'] ] ) ) { + /* + * Now, make sure it is a call to a global function. + */ + if ( $this->is_class_object_call( $i ) === true ) { + continue; + } + + if ( $this->is_token_namespaced( $i ) === true ) { + continue; + } + $last['nonce_check'] = $i; return true; } diff --git a/WordPress/Tests/Security/NonceVerificationUnitTest.inc b/WordPress/Tests/Security/NonceVerificationUnitTest.inc index 652eb3afa7..d7959cfa95 100644 --- a/WordPress/Tests/Security/NonceVerificationUnitTest.inc +++ b/WordPress/Tests/Security/NonceVerificationUnitTest.inc @@ -178,3 +178,14 @@ function test_incorrect_use_in_type_test_functions() { return; } } + +function fix_false_negatives_userland_method_same_name() { + WP_Faker::check_ajax_referer( 'something' ); + $faker->check_admin_referer( 'something' ); + do_something( $_POST['abc'] ); // Bad. +} + +function fix_false_negatives_namespaced_function_same_name() { + WP_Faker\SecurityBypass\wp_verify_nonce( 'something' ); + do_something( $_POST['abc'] ); // Bad. +} diff --git a/WordPress/Tests/Security/NonceVerificationUnitTest.php b/WordPress/Tests/Security/NonceVerificationUnitTest.php index 63ecbcdf37..0929ffda1c 100644 --- a/WordPress/Tests/Security/NonceVerificationUnitTest.php +++ b/WordPress/Tests/Security/NonceVerificationUnitTest.php @@ -47,6 +47,8 @@ public function getErrorList() { 160 => 1, 161 => 1, 177 => 1, + 185 => 1, + 190 => 1, ); } From e05b727a843699e7ebcfae9f52d0280ff41f7066 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 15:35:47 +0100 Subject: [PATCH 66/81] EscapeOutput: allow for map_deep() to output escape arrays Includes unit test. --- .../Sniffs/Security/EscapeOutputSniff.php | 36 ++++++++++++------- .../Tests/Security/EscapeOutputUnitTest.inc | 3 ++ .../Tests/Security/EscapeOutputUnitTest.php | 1 + 3 files changed, 28 insertions(+), 12 deletions(-) diff --git a/WordPress/Sniffs/Security/EscapeOutputSniff.php b/WordPress/Sniffs/Security/EscapeOutputSniff.php index 9a1feaa857..d1dae3af85 100644 --- a/WordPress/Sniffs/Security/EscapeOutputSniff.php +++ b/WordPress/Sniffs/Security/EscapeOutputSniff.php @@ -382,20 +382,32 @@ public function process_token( $stackPtr ) { if ( false !== $function_opener ) { - if ( 'array_map' === $functionName ) { - - // Get the first parameter (name of function being used on the array). - $mapped_function = $this->phpcsFile->findNext( - Tokens::$emptyTokens, - ( $function_opener + 1 ), - $this->tokens[ $function_opener ]['parenthesis_closer'], - true + if ( isset( $this->arrayWalkingFunctions[ $functionName ] ) ) { + + // Get the callback parameter. + $callback = $this->get_function_call_parameter( + $ptr, + $this->arrayWalkingFunctions[ $functionName ] ); - // If we're able to resolve the function name, do so. - if ( $mapped_function && \T_CONSTANT_ENCAPSED_STRING === $this->tokens[ $mapped_function ]['code'] ) { - $functionName = $this->strip_quotes( $this->tokens[ $mapped_function ]['content'] ); - $ptr = $mapped_function; + if ( ! empty( $callback ) ) { + /* + * If this is a function callback (not a method callback array) and we're able + * to resolve the function name, do so. + */ + $mapped_function = $this->phpcsFile->findNext( + Tokens::$emptyTokens, + $callback['start'], + ( $callback['end'] + 1 ), + true + ); + + if ( false !== $mapped_function + && \T_CONSTANT_ENCAPSED_STRING === $this->tokens[ $mapped_function ]['code'] + ) { + $functionName = $this->strip_quotes( $this->tokens[ $mapped_function ]['content'] ); + $ptr = $mapped_function; + } } } diff --git a/WordPress/Tests/Security/EscapeOutputUnitTest.inc b/WordPress/Tests/Security/EscapeOutputUnitTest.inc index bd7adb3930..875f575f23 100644 --- a/WordPress/Tests/Security/EscapeOutputUnitTest.inc +++ b/WordPress/Tests/Security/EscapeOutputUnitTest.inc @@ -289,3 +289,6 @@ echo esc_html( $something ), echo get_the_title(); // Bad. echo wp_kses_post( get_the_title() ); // Ok. echo esc_html( get_the_title() ); // Ok. + +echo implode( '
', map_deep( $items, 'esc_html' ) ); // Ok. +echo implode( '
', map_deep( $items, 'foo' ) ); // Bad. diff --git a/WordPress/Tests/Security/EscapeOutputUnitTest.php b/WordPress/Tests/Security/EscapeOutputUnitTest.php index 6fc0837e54..52b554be55 100644 --- a/WordPress/Tests/Security/EscapeOutputUnitTest.php +++ b/WordPress/Tests/Security/EscapeOutputUnitTest.php @@ -79,6 +79,7 @@ public function getErrorList() { 264 => 1, 266 => 1, 289 => 1, + 294 => 1, ); } From 0934e18a3c1b02d190c2a84b7e8b991ad6284026 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 17:34:53 +0100 Subject: [PATCH 67/81] ValidatedSanitizedInput: treat array-value comparison functions same as other comparisons Whether a comparison is made via a comparison operator or via one of the array-value comparison functions shouldn't make a difference for this sniff. Both ways of making a comparison should be treated the same. This was, so far, not the case. While `in_array()` was listed in the `$sanitizingFunctions` list, the other array-value comparison functions were not. And for `in_array()` a "missing unslash" error would still be thrown, while this doesn't happen when using straight comparisons. This PR fixes that and adds a new `is_in_array_comparison()` utility function to the `Sniff` class. Includes unit tests via the sniff. --- WordPress/Sniff.php | 45 ++++++++++++++++++- .../Security/ValidatedSanitizedInputSniff.php | 5 +++ .../ValidatedSanitizedInputUnitTest.inc | 13 ++++++ .../ValidatedSanitizedInputUnitTest.php | 1 + 4 files changed, 63 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 10ac07bc01..80b24d54d3 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -256,7 +256,6 @@ abstract class Sniff implements PHPCS_Sniff { 'filter_input' => true, 'filter_var' => true, 'hash_equals' => true, - 'in_array' => true, 'is_email' => true, 'number_format' => true, 'sanitize_bookmark_field' => true, @@ -380,6 +379,22 @@ abstract class Sniff implements PHPCS_Sniff { 'map_deep' => 2, ); + /** + * Array functions to compare a $needle to a predefined set of values. + * + * If the value is set to an integer, the function needs to have at least that + * many parameters for it to be considered as a comparison. + * + * @since 2.1.0 + * + * @var array => + */ + protected $arrayCompareFunctions = array( + 'in_array' => true, + 'array_search' => true, + 'array_keys' => 2, + ); + /** * Functions that format strings. * @@ -2263,6 +2278,34 @@ protected function is_comparison( $stackPtr ) { return false; } + /** + * Check if a token is inside of an array-value comparison function. + * + * @since 2.1.0 + * + * @param int $stackPtr The index of the token in the stack. + * + * @return bool Whether the token is (part of) a parameter to an + * array-value comparison function. + */ + protected function is_in_array_comparison( $stackPtr ) { + $function_ptr = $this->is_in_function_call( $stackPtr, $this->arrayCompareFunctions, true, true ); + if ( false === $function_ptr ) { + return false; + } + + $function_name = $this->tokens[ $function_ptr ]['content']; + if ( true === $this->arrayCompareFunctions[ $function_name ] ) { + return true; + } + + if ( $this->get_function_call_parameter_count( $function_ptr ) >= $this->arrayCompareFunctions[ $function_name ] ) { + return true; + } + + return false; + } + /** * Check what type of 'use' statement a token is part of. * diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index 14b3f26b39..881480e33b 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -155,6 +155,11 @@ function ( $symbol ) { return; } + // If this is a comparison using the array comparison functions, sanitization isn't needed. + if ( $this->is_in_array_comparison( $stackPtr ) ) { + return; + } + $this->mergeFunctionLists(); // Now look for sanitizing functions. diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index 84b4c16fed..495f9342fc 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -276,3 +276,16 @@ function test_additional_array_walking_functions() { $sane = map_deep( wp_unslash( $_GET['test'] ), 'sanitize_text_field' ); // Ok. $sane = map_deep( wp_unslash( $_GET['test'] ), 'foo' ); // Bad. } + +function test_recognize_array_comparison_functions_as_such() { + if ( ! isset( $_POST['form_fields'] ) ) { + return; + } + + if ( isset( $_REQUEST['plugin_status'] ) && in_array( $_REQUEST['plugin_status'], array( 'install', 'update', 'activate' ), true ) ) {} // OK. + if ( isset( $_REQUEST['plugin_state'] ) && in_array( $_REQUEST['plugin_state'], $states, true ) ) {} // OK. + if ( in_array( 'my_form_hidden_field_value', $_POST['form_fields'], true ) ) {} // OK. + if ( array_search( $_POST['form_fields'], 'my_form_hidden_field_value' ) ) {} // OK. + if ( array_keys( $_POST['form_fields'], 'my_form_hidden_field_value', true ) ) {} // OK. + if ( array_keys( $_POST['form_fields'] ) ) {} // Bad x2. +} diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php index 74ff09a7d3..ab2aa92d74 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php @@ -67,6 +67,7 @@ public function getErrorList() { 257 => 1, 266 => 1, 277 => 1, + 290 => 2, ); } From 5f2d6b544042797426c6ef5b24a844b3071e4025 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 31 Mar 2019 07:14:47 +0200 Subject: [PATCH 68/81] Sniff::has_nonce_check(): ignore nonce checks in nested closed scopes A function, closure, anonymous class and other OO constructs are all "closed" scopes. If any of these are nested, they open a nested closed scope and anything within that scope should be disregarded for the purpose of verifying whether or not a nonce check has been executed. This small change implements that. This fixes some potential false negatives. Includes unit tests. Related to 764 --- WordPress/Sniff.php | 10 ++++++++++ .../Security/NonceVerificationUnitTest.inc | 20 +++++++++++++++++++ .../Security/NonceVerificationUnitTest.php | 2 ++ 3 files changed, 32 insertions(+) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 10ac07bc01..8a64bc8689 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1468,6 +1468,16 @@ protected function has_nonce_check( $stackPtr ) { // Loop through the tokens looking for nonce verification functions. for ( $i = $start; $i < $end; $i++ ) { + // Skip over nested closed scope constructs. + if ( \T_FUNCTION === $tokens[ $i ]['code'] + || \T_CLOSURE === $tokens[ $i ]['code'] + || isset( Tokens::$ooScopeTokens[ $tokens[ $i ]['code'] ] ) + ) { + if ( isset( $tokens[ $i ]['scope_closer'] ) ) { + $i = $tokens[ $i ]['scope_closer']; + } + continue; + } // If this isn't a function name, skip it. if ( \T_STRING !== $tokens[ $i ]['code'] ) { diff --git a/WordPress/Tests/Security/NonceVerificationUnitTest.inc b/WordPress/Tests/Security/NonceVerificationUnitTest.inc index d7959cfa95..7c604beb3f 100644 --- a/WordPress/Tests/Security/NonceVerificationUnitTest.inc +++ b/WordPress/Tests/Security/NonceVerificationUnitTest.inc @@ -189,3 +189,23 @@ function fix_false_negatives_namespaced_function_same_name() { WP_Faker\SecurityBypass\wp_verify_nonce( 'something' ); do_something( $_POST['abc'] ); // Bad. } + +function skip_over_nested_constructs_1() { + $b = function () { + check_ajax_referer( 'something' ); // Nonce check is not in the same function scope. + }; + + do_something( $_POST['abc'] ); // Bad. +} + +function skip_over_nested_constructs_2() { + if ( $_POST['abc'] === 'test' ) { // Bad. + return; + } + + $b = new class() { + public function named() { + check_ajax_referer( 'something' ); // Nonce check is not in the same function scope. + } + }; +} diff --git a/WordPress/Tests/Security/NonceVerificationUnitTest.php b/WordPress/Tests/Security/NonceVerificationUnitTest.php index 0929ffda1c..838e897a9b 100644 --- a/WordPress/Tests/Security/NonceVerificationUnitTest.php +++ b/WordPress/Tests/Security/NonceVerificationUnitTest.php @@ -49,6 +49,8 @@ public function getErrorList() { 177 => 1, 185 => 1, 190 => 1, + 198 => 1, + 202 => 1, ); } From 1a79eae7dccbb15b203c332671b20d14548aa1c8 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 04:27:53 +0100 Subject: [PATCH 69/81] ValidatedSanitizedInput: add unit tests for null coalesce (equals) operators --- .../ValidatedSanitizedInputUnitTest.inc | 35 ++++++++++++++++++- .../ValidatedSanitizedInputUnitTest.php | 11 ++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index 495f9342fc..93ceaec2bb 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -161,7 +161,7 @@ some string {$_POST[some_var]} {$_GET['evil']} EOD ); // Bad x2. -if ( ( $_POST['foo'] ?? 'post' ) === 'post' ) {} // OK. +if ( ( $_POST['foo'] ?? 'post' ) === 'post' ) {} // Bad x2 - unslash, sanitize - more complex compares are not handled. if ( ( $_POST['foo'] <=> 'post' ) === 0 ) {} // OK. // Test whitespace independent isset/empty detection. @@ -289,3 +289,36 @@ function test_recognize_array_comparison_functions_as_such() { if ( array_keys( $_POST['form_fields'], 'my_form_hidden_field_value', true ) ) {} // OK. if ( array_keys( $_POST['form_fields'] ) ) {} // Bad x2. } + +/* + * Test recognition of validation via null coalesce, while still checking the var for sanitization. + */ +function test_null_coalesce_1() { + $var = sanitize_text_field( wp_unslash( $_POST['foo'] ?? '' ) ); // OK. + $var = sanitize_text_field( wp_unslash( $_POST['fool'] ?? $_POST['secondary'] ?? '' ) ); // OK. + $var = sanitize_text_field( wp_unslash( $_POST['bar']['sub'] ?? '' ) ); // OK. + $var = sanitize_text_field( $_POST['foobar'] ?? '' ); // Bad x1 - unslash. +} + +// The below two sets should give the same errors. +function test_null_coalesce_2() { + $var = $_POST['foo'] ?? ''; // Bad x2 - sanitize + unslash. + $var = $_POST['bar']['sub'] ?? ''; // Bad x2 - sanitize + unslash. + $var = ( $_POST['foobar']['sub'] ?? '' ); // Bad x2 - sanitize + unslash. + + $var = isset( $_POST['_foo'] ) ? $_POST['_foo'] : ''; // Bad x2 - sanitize + unslash. + $var = isset( $_POST['_bar']['_sub'] ) ? $_POST['_bar']['_sub'] : ''; // Bad x2 - sanitize + unslash. + $var = ( isset( $_POST['_foobar']['_sub'] ) ? $_POST['_foobar']['_sub'] : '' ); // Bad x2 - sanitize + unslash. +} + +function test_null_coalesce_validation() { + $_POST['key'] = $_POST['key'] ?? 'default'; // OK, assignment & Bad x2 - unslash, sanitize. + $key = sanitize_text_field( wp_unslash( $_POST['key'] ) ); // OK, validated via null coalesce. + $another_key = sanitize_text_field( wp_unslash( $_POST['another_key'] ) ); // Bad, not validated, different key. +} + +function test_null_coalesce_equals_validation() { + $_POST['key'] ??= 'default'; // OK, assignment. + $key = sanitize_text_field( wp_unslash( $_POST['key'] ) ); // OK, validated via null coalesce equals. + $another_key = sanitize_text_field( wp_unslash( $_POST['another_key'] ) ); // Bad, not validated, different key. +} diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php index ab2aa92d74..ca0821bd59 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.php @@ -54,6 +54,7 @@ public function getErrorList() { 138 => 1, 150 => 2, 160 => 2, + 164 => 2, 189 => 1, 202 => 1, 206 => 1, @@ -68,6 +69,16 @@ public function getErrorList() { 266 => 1, 277 => 1, 290 => 2, + 300 => 1, + 305 => 2, + 306 => 2, + 307 => 2, + 309 => 2, + 310 => 2, + 311 => 2, + 315 => 2, + 317 => 1, + 323 => 1, ); } From 6c8d0523d289fb7841d5a599200edfecbbc0d202 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 04:58:38 +0100 Subject: [PATCH 70/81] Sniff::is_comparison(): allow to disregard null coalesce The null coalesce operator `??` is a special comparison operator, in the sense that it doesn't compare a variable to whatever is on the other side of the comparison operator. For this reason, it should be possible to disregard it. --- WordPress/Sniff.php | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 9f00241cc4..51562dae5e 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -2229,12 +2229,24 @@ protected function is_validated( $stackPtr, $array_keys = array(), $in_condition * Also recognizes `switch ( $var )`. * * @since 0.5.0 + * @since 2.1.0 Added the $include_coalesce parameter. * - * @param int $stackPtr The index of this token in the stack. + * @param int $stackPtr The index of this token in the stack. + * @param bool $include_coalesce Optional. Whether or not to regard the null + * coalesce operator - ?? - as a comparison operator. + * Defaults to true. + * Null coalesce is a special comparison operator in this + * sense as it doesn't compare a variable to whatever is + * on the other side of the comparison operator. * * @return bool Whether this is a comparison. */ - protected function is_comparison( $stackPtr ) { + protected function is_comparison( $stackPtr, $include_coalesce = true ) { + + $comparisonTokens = Tokens::$comparisonTokens; + if ( false === $include_coalesce ) { + unset( $comparisonTokens[ \T_COALESCE ] ); + } // We first check if this is a switch statement (switch ( $var )). if ( isset( $this->tokens[ $stackPtr ]['nested_parenthesis'] ) ) { @@ -2258,7 +2270,7 @@ protected function is_comparison( $stackPtr ) { true ); - if ( isset( Tokens::$comparisonTokens[ $this->tokens[ $previous_token ]['code'] ] ) ) { + if ( isset( $comparisonTokens[ $this->tokens[ $previous_token ]['code'] ] ) ) { return true; } @@ -2281,7 +2293,7 @@ protected function is_comparison( $stackPtr ) { ); } - if ( false !== $next_token && isset( Tokens::$comparisonTokens[ $this->tokens[ $next_token ]['code'] ] ) ) { + if ( false !== $next_token && isset( $comparisonTokens[ $this->tokens[ $next_token ]['code'] ] ) ) { return true; } From 449ea6472d328ae8ba44d21134803447aa121b1b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 04:29:05 +0100 Subject: [PATCH 71/81] Sniff::is_validated(): recognize null coalesce (equal) operator as a way to validate a variable This adds recognition of the coalesce operator `??` (PHP 7.0) and the coalesce equals operator `??=`, as will be added in PHP 7.4, to the `Sniff::is_validated()` method. This prevents false positives where variables would be seen as "not validated", when the variable has in fact been validated via a coalesce equals assignment in a previous statement. Related to 764, 840 --- WordPress/Sniff.php | 44 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 51562dae5e..f7d0f2ff75 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -2097,10 +2097,12 @@ protected function is_validated( $stackPtr, $array_keys = array(), $in_condition $bare_array_keys = array_map( array( $this, 'strip_quotes' ), $array_keys ); $targets = array( - \T_ISSET => 'construct', - \T_EMPTY => 'construct', - \T_UNSET => 'construct', - \T_STRING => 'function_call', + \T_ISSET => 'construct', + \T_EMPTY => 'construct', + \T_UNSET => 'construct', + \T_STRING => 'function_call', + \T_COALESCE => 'coalesce', + \T_COALESCE_EQUAL => 'coalesce', ); // phpcs:ignore Generic.CodeAnalysis.JumbledIncrementer.Found -- On purpose, see below. @@ -2215,6 +2217,40 @@ protected function is_validated( $stackPtr, $array_keys = array(), $in_condition } return true; + + case 'coalesce': + $prev = $i; + do { + $prev = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $prev - 1 ), null, true, null, true ); + // Skip over array keys, like $_GET['key']['subkey']. + if ( \T_CLOSE_SQUARE_BRACKET === $this->tokens[ $prev ]['code'] ) { + $prev = $this->tokens[ $prev ]['bracket_opener']; + continue; + } + + break; + } while ( $prev >= ( $scope_start + 1 ) ); + + // We should now have reached the variable. + if ( \T_VARIABLE !== $this->tokens[ $prev ]['code'] ) { + continue 2; + } + + if ( $this->tokens[ $prev ]['content'] !== $this->tokens[ $stackPtr ]['content'] ) { + continue 2; + } + + if ( ! empty( $bare_array_keys ) ) { + $found_keys = $this->get_array_access_keys( $prev ); + $found_keys = array_map( array( $this, 'strip_quotes' ), $found_keys ); + $diff = array_diff_assoc( $bare_array_keys, $found_keys ); + if ( ! empty( $diff ) ) { + continue 2; + } + } + + // Right variable, correct key. + return true; } } From 625f221df1c2294c905796afeba2a1584f77da57 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 10:30:06 +0100 Subject: [PATCH 72/81] ValidatedSanitizedInput: throw unslash/sanitization errors for null coalesced variable --- WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index 881480e33b..1a0be175c3 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -151,7 +151,7 @@ function ( $symbol ) { } // If this is a comparison ('a' == $_POST['foo']), sanitization isn't needed. - if ( $this->is_comparison( $stackPtr ) ) { + if ( $this->is_comparison( $stackPtr, false ) ) { return; } From 7c6f0a85abccaf38e89f1f95c985e98e61dd45c4 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 30 Mar 2019 10:32:14 +0100 Subject: [PATCH 73/81] ValidatedSanitizedInput: allow for validation via null coalesce / null coalesce equals PHP 7.0 introduced the null coalesce operator, while PHP 7.4 will introduce the null coalesce equal operator. These operators should be accounted for in the `ValidatedSanitizedInput` sniff as valid ways to validate a variable, but should still allow for the sniff to *also* check for sanitization. Refs: * https://php.net/manual/en/language.operators.comparison.php#language.operators.comparison.coalesce * https://wiki.php.net/rfc/isset_ternary * https://wiki.php.net/rfc/null_coalesce_equal_operator Related to 764 Fixes 837 Closes 840 which is superseded by this PR --- .../Security/ValidatedSanitizedInputSniff.php | 34 +++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php index 1a0be175c3..4b65ac65be 100644 --- a/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php +++ b/WordPress/Sniffs/Security/ValidatedSanitizedInputSniff.php @@ -10,6 +10,7 @@ namespace WordPressCS\WordPress\Sniffs\Security; use WordPressCS\WordPress\Sniff; +use PHP_CodeSniffer\Util\Tokens; /** * Flag any non-validated/sanitized input ( _GET / _POST / etc. ). @@ -131,8 +132,37 @@ function ( $symbol ) { $error_data = array( $this->tokens[ $stackPtr ]['content'] . '[' . implode( '][', $array_keys ) . ']' ); - // Check for validation first. - if ( ! $this->is_validated( $stackPtr, $array_keys, $this->check_validation_in_scope_only ) ) { + /* + * Check for validation first. + */ + $validated = false; + + for ( $i = ( $stackPtr + 1 ); $i < $this->phpcsFile->numTokens; $i++ ) { + if ( isset( Tokens::$emptyTokens[ $this->tokens[ $i ]['code'] ] ) ) { + continue; + } + + if ( \T_OPEN_SQUARE_BRACKET === $this->tokens[ $i ]['code'] + && isset( $this->tokens[ $i ]['bracket_closer'] ) + ) { + // Skip over array keys. + $i = $this->tokens[ $i ]['bracket_closer']; + continue; + } + + if ( \T_COALESCE === $this->tokens[ $i ]['code'] ) { + $validated = true; + } + + // Anything else means this is not a validation coalesce. + break; + } + + if ( false === $validated ) { + $validated = $this->is_validated( $stackPtr, $array_keys, $this->check_validation_in_scope_only ); + } + + if ( false === $validated ) { $this->phpcsFile->addError( 'Detected usage of a possibly undefined superglobal array index: %s. Use isset() or empty() to check the index exists before using it', $stackPtr, From 7b187492e9556e9567d5baf52746b3ef63b2de15 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 31 Mar 2019 07:00:36 +0200 Subject: [PATCH 74/81] Sniff::has_nonce_check(): allow for comparing a variable before nonce check This builds onto the similar changes made for the `ValidatedSanitizedInput` sniff in ... This fixes false positives as reported in 1114 and 1506. Note: it is not currently checked that the nonce check is done within the same conditional scope as the comparison. Just that it is done within the same _function_ scope. Includes unit tests. Fixes 1114 Fixes 1506 --- WordPress/Sniff.php | 2 + .../Security/NonceVerificationUnitTest.inc | 37 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 9f00241cc4..8e6c6ffef3 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1440,6 +1440,8 @@ protected function has_nonce_check( $stackPtr ) { $allow_nonce_after = false; if ( $this->is_in_isset_or_empty( $stackPtr ) || $this->is_in_type_test( $stackPtr ) + || $this->is_comparison( $stackPtr ) + || $this->is_in_array_comparison( $stackPtr ) ) { $allow_nonce_after = true; } diff --git a/WordPress/Tests/Security/NonceVerificationUnitTest.inc b/WordPress/Tests/Security/NonceVerificationUnitTest.inc index 7c604beb3f..cd5a7d4f92 100644 --- a/WordPress/Tests/Security/NonceVerificationUnitTest.inc +++ b/WordPress/Tests/Security/NonceVerificationUnitTest.inc @@ -209,3 +209,40 @@ function skip_over_nested_constructs_2() { } }; } + +// Issue #1506 +function allow_for_compare_before_noncecheck() { + if ( + 'newsletter_sign_up' === $_POST['action'] && // OK. + wp_verify_nonce( $_POST['newsletter_nonce'] ) + ) {} +} + +// Issue #1114 +function allow_for_nonce_check_within_switch() { + if ( ! isset( $_REQUEST['action'] ) ) { + return; + } + + switch ( $_REQUEST['action'] ) { // OK. + case 'foo': + check_admin_referer( 'foo' ); + break; + case 'bar': + check_admin_referer( 'bar' ); + break; + } +} + +function allow_for_array_compare_before_noncecheck() { + if ( array_search( array( 'subscribe', 'unsubscribe', $_POST['action'], true ) // OK. + && wp_verify_nonce( $_POST['newsletter_nonce'] ) + ) {} +} + +function allow_for_array_comparison_in_condition() { + if ( in_array( $_GET['action'], $valid_actions, true ) ) { // OK. + check_admin_referer( 'foo' ); + foo(); + } +} From 2ac765f2b0ae653efef9f5dda74cd2324f3abe78 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 2 Apr 2019 10:04:51 +0200 Subject: [PATCH 75/81] ValidatedSanitizedInput: allow for more unslashing functions As correctly pointed out in https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/ 869#issuecomment-478329921 and confirmed via a trace-back of the functions used by `wp_unslash()`, WP contains two additional unslashing functions which are perfectly valid to use for unslashing received data. Refs: * https://developer.wordpress.org/reference/functions/wp_unslash/ * https://developer.wordpress.org/reference/functions/stripslashes_deep/ * https://developer.wordpress.org/reference/functions/stripslashes_from_strings_only/ This commit adds allowance for using these. It also updates the error message to reflect this. Includes unit tests. --- WordPress/Sniff.php | 38 +++++++++++++------ .../ValidatedSanitizedInputUnitTest.inc | 9 +++++ 2 files changed, 36 insertions(+), 11 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index c4b066e911..27441c89a3 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -315,6 +315,19 @@ abstract class Sniff implements PHPCS_Sniff { 'sizeof' => true, ); + /** + * Functions which unslash the data passed to them. + * + * @since 2.1.0 + * + * @var array + */ + protected $unslashingFunctions = array( + 'stripslashes_deep' => true, + 'stripslashes_from_strings_only' => true, + 'wp_unslash' => true, + ); + /** * List of PHP native functions to test the type of a variable. * @@ -1801,8 +1814,8 @@ protected function is_safe_casted( $stackPtr ) { * @since 0.5.0 * * @param int $stackPtr The index of the token in the stack. - * @param bool $require_unslash Whether to give an error if wp_unslash() isn't - * used on the variable before sanitization. + * @param bool $require_unslash Whether to give an error if no unslashing function + * is used on the variable before sanitization. * * @return bool Whether the token being sanitized. */ @@ -1833,10 +1846,10 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { return true; } - $valid_functions = $this->sanitizingFunctions; - $valid_functions += $this->unslashingSanitizingFunctions; - $valid_functions += $this->arrayWalkingFunctions; - $valid_functions['wp_unslash'] = true; + $valid_functions = $this->sanitizingFunctions; + $valid_functions += $this->unslashingSanitizingFunctions; + $valid_functions += $this->unslashingFunctions; + $valid_functions += $this->arrayWalkingFunctions; $functionPtr = $this->is_in_function_call( $stackPtr, $valid_functions ); @@ -1851,12 +1864,15 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { $functionName = $this->tokens[ $functionPtr ]['content']; - // Check if wp_unslash() is being used. - if ( 'wp_unslash' === $functionName ) { + // Check if an unslashing function is being used. + if ( isset( $this->unslashingFunctions[ $functionName ] ) ) { $is_unslashed = true; - unset( $valid_functions['wp_unslash'] ); + // Remove the unslashing functions. + $valid_functions = array_diff_key( $valid_functions, $this->unslashingFunctions ); + + // Check is any of the remaining (sanitizing) functions is used. $higherFunctionPtr = $this->is_in_function_call( $functionPtr, $valid_functions ); // If there is no other valid function being used, this value is unsanitized. @@ -1909,7 +1925,7 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { } /** - * Add an error for missing use of wp_unslash(). + * Add an error for missing use of unslashing. * * @since 0.5.0 * @@ -1918,7 +1934,7 @@ protected function is_sanitized( $stackPtr, $require_unslash = false ) { public function add_unslash_error( $stackPtr ) { $this->phpcsFile->addError( - 'Missing wp_unslash() before sanitization.', + '%s data not unslashed before sanitization. Use wp_unslash() or similar', $stackPtr, 'MissingUnslash', array( $this->tokens[ $stackPtr ]['content'] ) diff --git a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc index 93ceaec2bb..46dad6254c 100644 --- a/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc +++ b/WordPress/Tests/Security/ValidatedSanitizedInputUnitTest.inc @@ -322,3 +322,12 @@ function test_null_coalesce_equals_validation() { $key = sanitize_text_field( wp_unslash( $_POST['key'] ) ); // OK, validated via null coalesce equals. $another_key = sanitize_text_field( wp_unslash( $_POST['another_key'] ) ); // Bad, not validated, different key. } + +function test_using_different_unslashing_functions() { + if ( ! isset( $_GET['test'] ) ) { + return; + } + + $sane = sanitize_text_field(stripslashes_deep($_GET['test'])); // Ok. + $sane = sanitize_text_field( stripslashes_from_strings_only( $_GET['test'] ) ); // OK. +} From bb646b6a30cff20535709f93f9640c2a39c8f739 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 1 Apr 2019 07:19:11 +0200 Subject: [PATCH 76/81] Sniff::has_nonce_check(): allow for unslashing a variable before nonce check This also gets rid of a bug were sanitization was not recognized as such if the call to `wp_unslash()` was nested inside it. Includes unit tests. Fixes 572 --- WordPress/Sniff.php | 1 + .../Security/NonceVerificationUnitTest.inc | 18 ++++++++++++++++++ .../Security/NonceVerificationUnitTest.php | 1 + 3 files changed, 20 insertions(+) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index 27441c89a3..f868537201 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1455,6 +1455,7 @@ protected function has_nonce_check( $stackPtr ) { || $this->is_in_type_test( $stackPtr ) || $this->is_comparison( $stackPtr ) || $this->is_in_array_comparison( $stackPtr ) + || $this->is_in_function_call( $stackPtr, $this->unslashingFunctions ) !== false ) { $allow_nonce_after = true; } diff --git a/WordPress/Tests/Security/NonceVerificationUnitTest.inc b/WordPress/Tests/Security/NonceVerificationUnitTest.inc index cd5a7d4f92..b6e3e9aeef 100644 --- a/WordPress/Tests/Security/NonceVerificationUnitTest.inc +++ b/WordPress/Tests/Security/NonceVerificationUnitTest.inc @@ -246,3 +246,21 @@ function allow_for_array_comparison_in_condition() { foo(); } } + +# Issue #572. +function allow_for_unslash_before_noncecheck_but_demand_noncecheck() { + $var = wp_unslash( $_POST['foo'] ); // Bad. + echo $var; +} + +function allow_for_unslash_before_noncecheck() { + $var = stripslashes_from_strings_only( $_POST['foo'] ); // OK. + wp_verify_nonce( $var ); + echo $var; +} + +function allow_for_unslash_in_sanitization() { + $var = sanitize_text_field( wp_unslash( $_POST['foo'] ) ); // OK. + wp_verify_nonce( $var ); + echo $var; +} diff --git a/WordPress/Tests/Security/NonceVerificationUnitTest.php b/WordPress/Tests/Security/NonceVerificationUnitTest.php index 838e897a9b..5cd8813314 100644 --- a/WordPress/Tests/Security/NonceVerificationUnitTest.php +++ b/WordPress/Tests/Security/NonceVerificationUnitTest.php @@ -51,6 +51,7 @@ public function getErrorList() { 190 => 1, 198 => 1, 202 => 1, + 252 => 1, ); } From a8d8c6ce09f6def51e5de6d7944c37f0037d2bb7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 31 Mar 2019 09:20:07 +0200 Subject: [PATCH 77/81] NonceVerification: bug fix - sanitization is no alternative for nonce check Any usage of superglobals in calls to sanitization functions were up to now ignored. As the output of sanitization is normally assigned to a names variable, this meant that it was possible to bypass the nonce verification sniff that way. I don't believe that was the intended behaviour. Instead IMO, a sanitization call should be allowed prior to the nonce verification, but should not negate the nonce verification check. This fixes that. Includes unit tests. --- WordPress/Sniff.php | 1 + WordPress/Sniffs/Security/NonceVerificationSniff.php | 4 ---- .../Tests/Security/NonceVerificationUnitTest.inc | 11 +++++++++++ .../Tests/Security/NonceVerificationUnitTest.php | 1 + 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/WordPress/Sniff.php b/WordPress/Sniff.php index f868537201..07d88fbe81 100644 --- a/WordPress/Sniff.php +++ b/WordPress/Sniff.php @@ -1456,6 +1456,7 @@ protected function has_nonce_check( $stackPtr ) { || $this->is_comparison( $stackPtr ) || $this->is_in_array_comparison( $stackPtr ) || $this->is_in_function_call( $stackPtr, $this->unslashingFunctions ) !== false + || $this->is_only_sanitized( $stackPtr ) ) { $allow_nonce_after = true; } diff --git a/WordPress/Sniffs/Security/NonceVerificationSniff.php b/WordPress/Sniffs/Security/NonceVerificationSniff.php index b29dcb1fa0..e9b2cf6603 100644 --- a/WordPress/Sniffs/Security/NonceVerificationSniff.php +++ b/WordPress/Sniffs/Security/NonceVerificationSniff.php @@ -121,10 +121,6 @@ public function process_token( $stackPtr ) { $this->mergeFunctionLists(); - if ( $this->is_only_sanitized( $stackPtr ) ) { - return; - } - if ( $this->has_nonce_check( $stackPtr ) ) { return; } diff --git a/WordPress/Tests/Security/NonceVerificationUnitTest.inc b/WordPress/Tests/Security/NonceVerificationUnitTest.inc index b6e3e9aeef..d02e6a7cb0 100644 --- a/WordPress/Tests/Security/NonceVerificationUnitTest.inc +++ b/WordPress/Tests/Security/NonceVerificationUnitTest.inc @@ -264,3 +264,14 @@ function allow_for_unslash_in_sanitization() { wp_verify_nonce( $var ); echo $var; } + +function dont_allow_bypass_nonce_via_sanitization() { + $var = sanitize_text_field( $_POST['foo'] ); // Bad. + echo $var; +} + +function dont_allow_bypass_nonce_via_sanitization() { + $var = sanitize_text_field( $_POST['foo'] ); // OK. + wp_verify_nonce( $var ); + echo $var; +} diff --git a/WordPress/Tests/Security/NonceVerificationUnitTest.php b/WordPress/Tests/Security/NonceVerificationUnitTest.php index 5cd8813314..964ab9501a 100644 --- a/WordPress/Tests/Security/NonceVerificationUnitTest.php +++ b/WordPress/Tests/Security/NonceVerificationUnitTest.php @@ -52,6 +52,7 @@ public function getErrorList() { 198 => 1, 202 => 1, 252 => 1, + 269 => 1, ); } From cce11ae5f02832367b2a0852ef864fc0223e6ecc Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 27 Mar 2019 01:17:08 +0100 Subject: [PATCH 78/81] Core: forbid assignments in conditions As per the proposal in https://make.wordpress.org/core/2019/03/26/coding-standards-updates-for-php-5-6/ Open actions: * [x] Adjust Core PHP handbook to mention this rule in the `Clever code` section --- WordPress-Core/ruleset.xml | 7 +++++++ WordPress-Extra/ruleset.xml | 6 ------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/WordPress-Core/ruleset.xml b/WordPress-Core/ruleset.xml index 16edf427a6..e14d49b15f 100644 --- a/WordPress-Core/ruleset.xml +++ b/WordPress-Core/ruleset.xml @@ -392,6 +392,13 @@ + + + diff --git a/WordPress-Extra/ruleset.xml b/WordPress-Extra/ruleset.xml index 6ff9b6cf42..f62542455e 100644 --- a/WordPress-Extra/ruleset.xml +++ b/WordPress-Extra/ruleset.xml @@ -24,12 +24,6 @@ - - - From d6f9dc0bc74202f5f7bc901edaf15abcba82218e Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 27 Mar 2019 01:44:11 +0100 Subject: [PATCH 79/81] Core: warn against the use of loose comparisons As per the proposal in 1624, this moves the sniffs which `warn` against the use of loose comparisons to the Core ruleset. Open actions: * [x] Adjust Core PHP handbook to mention this rule in the `Clever code` section (or wherever else it should go). --- WordPress-Core/ruleset.xml | 5 +++++ WordPress-Extra/ruleset.xml | 9 --------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/WordPress-Core/ruleset.xml b/WordPress-Core/ruleset.xml index e14d49b15f..9a3485b353 100644 --- a/WordPress-Core/ruleset.xml +++ b/WordPress-Core/ruleset.xml @@ -392,6 +392,11 @@ + + + + - - - - - - From 4d814b73729f78adef1e306c990b6e9f2e8f789b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 3 Apr 2019 07:03:07 +0200 Subject: [PATCH 80/81] Changelog for WPCS 2.1.0 * Release date set at this Monday April 8th. * Includes all currently merged changes. * Includes a minor update to the Readme file for the DealerDirect Composer plugin version number. Related to 1628 (which is included in this release) --- CHANGELOG.md | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++ README.md | 2 +- 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96b8b726e9..fdfb7c265f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,81 @@ This projects adheres to [Semantic Versioning](https://semver.org/) and [Keep a _No documentation available about unreleased changes as of yet._ +## [2.1.0] - 2019-04-08 + +### Added +- New `WordPress.PHP.IniSet` sniff to the `WordPress-Extra` ruleset. + This sniff will detect calls to `ini_set()` and `ini_alter()` and warn against their use as changing configuration values at runtime leads to an unpredictable runtime environment, which can result in conflicts between core/plugins/themes. + - The sniff will not throw notices about a very limited set of "safe" ini directives. + - For a number of ini directives for which there are alternative, non-conflicting ways to achieve the same available, the sniff will throw an `error` and advise using the alternative. +- `doubleval()`, `count()` and `sizeof()` to the list of functions which unslash and sanitize `Sniff::$unslashingSanitizingFunctions`. + This affects the `WordPress.Security.ValidatedSanitizedInput` and the `WordPress.Security.NonceVerification` sniffs. +- The new WP 5.1 `WP_UnitTestCase_Base` class to the `Sniff::$test_class_whitelist` property. +- New `Sniff::get_array_access_keys()` utility method to retrieve all array keys for a variable using multi-level array access. +- New `Sniff::is_class_object_call()`, `Sniff::is_token_namespaced()` utility methods. + These should help make the checking of whether or not a function call is a global function, method call or a namespaced function call more consistent. + This also implements allowing for the [namespace keyword being used as an operator](https://www.php.net/manual/en/language.namespaces.nsconstants.php#example-258). +- New `Sniff::is_in_function_call()` utility method to facilitate checking whether a token is (part of) a parameter passed to a specific (set of) function(s). +- New `Sniff::is_in_type_test()` utility method to determine if a variable is being type tested, along with a `Sniff::$typeTestFunctions` property containing the names of the functions this applies to. +- New `Sniff::is_in_array_comparison()` utility method to determine if a variable is (part of) a parameter in an array-value comparison, along with a `Sniff::$arrayCompareFunctions` property containing the names of the relevant functions. +- New `Sniff::$arrayWalkingFunctions` property containing the names of array functions which apply a callback to the array, but don't change the array by reference. +- New `Sniff::$unslashingFunctions` property containing the names of functions which unslash data passed to them and return the unslashed result. + +### Changed +- Moved the `WordPress.PHP.StrictComparisons`, `WordPress.PHP.StrictInArray` and the `WordPress.CodeAnalysis.AssignmentInCondition` sniff from the `WordPress-Extra` to the `WordPress-Core` ruleset. +- The `Squiz.Commenting.InlineComment.SpacingAfter` error is no longer included in the `WordPress-Docs` ruleset. +- The default value for `minimum_supported_wp_version`, as used by a [number of sniffs detecting usage of deprecated WP features](https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#minimum-wp-version-to-check-for-usage-of-deprecated-functions-classes-and-function-parameters), has been updated to `4.8`. +- The `WordPress.WP.DeprecatedFunctions` sniff will now detect functions deprecated in WP 5.1. +- The `WordPress.Security.NonceVerification` sniff now allows for variable type testing, comparisons, unslashing and sanitization before the nonce check. A nonce check within the same scope, however, is still required. +- The `WordPress.Security.ValidatedSanitizedInput` sniff now allows for using a superglobal in an array-value comparison without sanitization, same as when the superglobal is used in a scalar value comparison. +- `WordPress.NamingConventions.PrefixAllGlobals`: some of the error messages have been made more explicit. +- The error messages for the `WordPress.Security.ValidatedSanitizedInput` sniff will now contain information on the index keys accessed. +- The error message for the `WordPress.Security.ValidatedSanitizedInput.InputNotValidated` has been reworded to make it more obvious what the actual issue being reported is. +- The error message for the `WordPress.Security.ValidatedSanitizedInput.MissingUnslash` has been reworded. +- The `Sniff::is_comparison()` method now has a new `$include_coalesce` parameter to allow for toggling whether the null coalesce operator should be seen as a comparison operator. Defaults to `true`. +- All sniffs are now also being tested against PHP 7.4 (unstable) for consistent sniff results. +- The recommended version of the suggested DealerDirect PHPCS Composer plugin is now `^0.5.0`. +- Various minor code tweaks and clean up. + +### Removed +- `ini_set` and `ini_alter` from the list of functions detected by the `WordPress.PHP.DiscouragedFunctions` sniff. + These are now covered via the new `WordPress.PHP.IniSet` sniff. +- `in_array()` and `array_key_exists()` from the list of `Sniff::$sanitizingFunctions`. These are now handled differently. + +### Fixed +- The `WordPress.NamingConventions.PrefixAllGlobals` sniff would underreport when global functions would be autoloaded via a Composer autoload `files` configuration. +- The `WordPress.Security.EscapeOutput` sniff will now recognize `map_deep()` for escaping the values in an array via a callback to an output escaping function. This should prevent false positives. +- The `WordPress.Security.NonceVerification` sniff will no longer inadvertently allow for a variable to be sanitized without a nonce check within the same scope. +- The `WordPress.Security.ValidatedSanitizedInput` sniff will no longer throw errors when a variable is only being type tested. +- The `WordPress.Security.ValidatedSanitizedInput` sniff will now correctly recognize the null coalesce (PHP 7.0) and null coalesce equal (PHP 7.4) operators and will now throw errors for missing unslashing and sanitization where relevant. +- The `WordPress.WP.AlternativeFunctions` sniff will no longer recommend using the WP_FileSystem when PHP native input streams, like `php://input`, or the PHP input stream constants are being read or written to. +- The `WordPress.WP.AlternativeFunctions` sniff will no longer report on usage of the `curl_version()` function. +- The `WordPress.WP.CronInterval` sniff now has improved function recognition which should lower the chance of false positives. +- The `WordPress.WP.EnqueuedResources` sniff will no longer throw false positives for inline jQuery code trying to access a stylesheet link tag. +- Various bugfixes for the `Sniff::has_nonce_check()` method: + - The method will no longer incorrectly identify methods/namespaced functions mirroring the name of WP native nonce verification functions as if they were the global functions. + This will prevent some false negatives. + - The method will now skip over nested closed scopes, such as closures and anonymous classes. This should prevent some false negatives for nonce verification being done while not in the correct scope. + + These fixes affect the `WordPress.Security.NonceVerification` sniff. +- The `Sniff::is_in_isset_or_empty()` method now also checks for usage of `array_key_exist()` and `key_exists()` and will regard these as correct ways to validate a variable. + This should prevent false positives for the `WordPress.Security.ValidatedSanitizedInput` and the `WordPress.Security.NonceVerification` sniffs. +- Various bugfixes for the `Sniff::is_sanitized()` method: + - The method presumed the WordPress coding style regarding code layout, which could lead to false positives. + - The method will no longer incorrectly identify methods/namespaced functions mirroring the name of WP/PHP native unslashing/sanitization functions as if they were the global functions. + This will prevent some false negatives. + - The method will now recognize `map_deep()` for sanitizing an array via a callback to a sanitization function. This should prevent false positives. + - The method will now recognize `stripslashes_deep()` and `stripslashes_from_strings_only()` as valid unslashing functions. This should prevent false positives. + All these fixes affect both the `WordPress.Security.ValidatedSanitizedInput` and the `WordPress.Security.NonceVerification` sniff. +- Various bugfixes for the `Sniff::is_validated()` method: + - The method did not verify correctly whether a variable being validated was the same variable as later used which could lead to false negatives. + - The method did not verify correctly whether a variable being validated had the same array index keys as the variable as later used which could lead to both false negatives as well as false positives. + - The method now also checks for usage of `array_key_exist()` and `key_exists()` and will regard these as correct ways to validate a variable. This should prevent some false positives. + - The methods will now recognize the null coalesce and the null coalesce equal operators as ways to validate a variable. This prevents some false positives. + The results from the `WordPress.Security.ValidatedSanitizedInput` sniff should be more accurate because of these fixes. +- A potential "Undefined index" notice from the `Sniff::is_assignment()` method. + + ## [2.0.0] - 2019-01-16 ### Important information about this release: @@ -994,6 +1069,7 @@ See the comparison for full list. Initial tagged release. [Unreleased]: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/compare/master...HEAD +[2.1.0]: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/compare/2.0.0...2.1.0 [2.0.0]: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/compare/2.0.0-RC1...2.0.0 [2.0.0-RC1]: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/compare/1.2.1...2.0.0-RC1 [1.2.1]: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/compare/1.2.0...1.2.1 diff --git a/README.md b/README.md index b2662a15e1..17c4a52dc4 100644 --- a/README.md +++ b/README.md @@ -82,7 +82,7 @@ When installing the WordPress Coding Standards as a dependency in a larger proje There are two actively maintained Composer plugins which can handle the registration of standards with PHP_CodeSniffer for you: * [composer-phpcodesniffer-standards-plugin](https://github.com/higidi/composer-phpcodesniffer-standards-plugin) -* [phpcodesniffer-composer-installer](https://github.com/DealerDirect/phpcodesniffer-composer-installer):"^0.4.3" +* [phpcodesniffer-composer-installer](https://github.com/DealerDirect/phpcodesniffer-composer-installer):"^0.5.0" It is strongly suggested to `require` one of these plugins in your project to handle the registration of external standards with PHPCS for you. From f4b82aea6c3ac845fb1f437e569a2ed9fb096d23 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 3 Apr 2019 21:48:49 +0200 Subject: [PATCH 81/81] Changelog: clarified the addition of the `count()` and `sizeof()` functions --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fdfb7c265f..6a5e83627a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,8 @@ _No documentation available about unreleased changes as of yet._ This sniff will detect calls to `ini_set()` and `ini_alter()` and warn against their use as changing configuration values at runtime leads to an unpredictable runtime environment, which can result in conflicts between core/plugins/themes. - The sniff will not throw notices about a very limited set of "safe" ini directives. - For a number of ini directives for which there are alternative, non-conflicting ways to achieve the same available, the sniff will throw an `error` and advise using the alternative. -- `doubleval()`, `count()` and `sizeof()` to the list of functions which unslash and sanitize `Sniff::$unslashingSanitizingFunctions`. +- `doubleval()`, `count()` and `sizeof()` to `Sniff::$unslashingSanitizingFunctions` property. + While `count()` and its alias `sizeof()`, don't actually unslash or sanitize, the output of these functions is safe to use without unslashing or sanitizing. This affects the `WordPress.Security.ValidatedSanitizedInput` and the `WordPress.Security.NonceVerification` sniffs. - The new WP 5.1 `WP_UnitTestCase_Base` class to the `Sniff::$test_class_whitelist` property. - New `Sniff::get_array_access_keys()` utility method to retrieve all array keys for a variable using multi-level array access.