From fcea43c8c933a94fc87542e2deaacc8f14e57a00 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 11 Jan 2023 15:47:58 +0100 Subject: [PATCH 01/21] DB/PreparedSQLPlaceholders: add extra tests ... to covered previously uncovered code. Note: some of the messages generated for these (mostly invalid) code samples are not always that clear, but the fact that the queries are being flagged is still correct. --- .../DB/PreparedSQLPlaceholdersUnitTest.inc | 52 +++++++++++++++++++ .../DB/PreparedSQLPlaceholdersUnitTest.php | 8 +++ 2 files changed, 60 insertions(+) diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index 5d8ac2aa62..c4ebf915ad 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -280,3 +280,55 @@ $wpdb->prepare( "WHERE '%10i' IS NULL", $field ); // UnsupportedIdentifierPlaceh $wpdb->prepare( 'xxx IN ( ' . implode( ',', array_fill( 0, count( $post_types ), '%i' ) ) . ' )', $fields ); // UnsupportedIdentifierPlaceholder + IdentifierWithinIN. // phpcs:set WordPress.DB.PreparedSQLPlaceholders minimum_wp_version + +$wpdb->prepare(); // Ignore. + +$where = $wpdb->prepare( + "{$wpdb->posts}.post_type IN (\"" + . implode() + . "\") AND {$wpdb->posts}.post_id = %s", + $post_id +); // Bad, dynamic placeholder generation quotes with invalid implode call (no params). + +$where = $wpdb->prepare( + "{$wpdb->posts}.post_status IN ('" + . implode( ',' ) + . '\') AND {$wpdb->posts}.post_id = %s', + $post_id +); // Bad, dynamic placeholder generation quotes with invalid implode call (missing second param). + +$where = $wpdb->prepare( + "{$wpdb->posts}.post_status IN ('" + . implode( '|', array_fill( 0, count($post_stati), '%s' ) ) + . '\') AND {$wpdb->posts}.post_id = %s', + $post_id +); // Bad x2, dynamic placeholder generation quotes with invalid implode call (separator parameter does not contain expected ',' value). + +$where = $wpdb->prepare( + "{$wpdb->posts}.post_type IN (\"" + . implode( ',', $array_fill ) + . "\") AND {$wpdb->posts}.post_id = %s", + $post_id +); // Bad, dynamic placeholder generation quotes with invalid implode call (array param is not call to array_fill). + +$where = $wpdb->prepare( + "{$wpdb->posts}.post_type IN (\"" + . implode( ',', array_fill() ) + . "\") AND {$wpdb->posts}.post_id = %s", + $post_id +); // Bad, dynamic placeholder generation quotes with invalid implode call (array param is call to array_fill() without params). + +$where = $wpdb->prepare( + "{$wpdb->posts}.post_type IN (\"" + . implode( ',', array_fill( 0, count($post_types) ) ) + . "\") AND {$wpdb->posts}.post_id = %s", + $post_id +); // Bad, dynamic placeholder generation quotes with invalid implode call (array param is call to array_fill(), but missing third param). + +// Safeguard that short array as $args is handled correctly. +$wpdb->get_col( + $wpdb->prepare( + "SELECT term_taxonomy_id FROM {$wpdb->term_taxonomy} WHERE taxonomy = %s LIMIT %d", + [ 'taxonomy_name', $limit ] + ) // Ok. +); diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php index 059af40509..252f15ccab 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php @@ -95,6 +95,13 @@ public function getErrorList() { 278 => 1, // UnsupportedIdentifierPlaceholder. 279 => 2, // UnsupportedIdentifierPlaceholder + QuotedIdentifierPlaceholder. 280 => 2, // UnsupportedIdentifierPlaceholder + IdentifierWithinIN. + + 288 => 1, + 295 => 1, + 302 => 1, + 309 => 1, + 316 => 1, + 323 => 1, ); } @@ -138,6 +145,7 @@ public function getWarningList() { 234 => 1, // UnfinishedPrepare. 246 => 1, // UnfinishedPrepare. 258 => 1, // ReplacementsWrongNumber. + 300 => 1, ); } } From 671289db4861e2564d7af3deb4dd34e4fa802a2f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 02:27:42 +0200 Subject: [PATCH 02/21] DB/PreparedSQLPlaceholders: make variables used explicit These variables are conditionally set within the `for()` loop. Just declaring them here to be more explicit/complete with the variables in use within the loop. --- WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index e1c3a0abb8..a81615bd96 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -201,6 +201,8 @@ public function process_token( $stackPtr ) { 'implode_fill' => 0, 'adjustment_count' => 0, ); + $skip_from = null; + $skip_to = null; for ( $i = $query['start']; $i <= $query['end']; $i++ ) { // Skip over groups of tokens if they are part of an inline function call. From 27ca93514cc505a17c934a60150fce94840630ec Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 11 Jan 2023 15:57:50 +0100 Subject: [PATCH 03/21] DB/PreparedSQLPlaceholders: implement PHPCSUtils This change is already covered by existing tests (test case file line 58 and 259). --- WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index a81615bd96..8060ea62ee 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -10,6 +10,8 @@ namespace WordPressCS\WordPress\Sniffs\DB; use PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Tokens\Collections; +use PHPCSUtils\Utils\Arrays; use PHPCSUtils\Utils\PassedParameters; use PHPCSUtils\Utils\TextStrings; use WordPressCS\WordPress\Helpers\MinimumWPVersionTrait; @@ -552,7 +554,8 @@ function ( $symbol ) { if ( false !== $next && ( \T_ARRAY === $this->tokens[ $next ]['code'] - || \T_OPEN_SHORT_ARRAY === $this->tokens[ $next ]['code'] ) + || ( isset( Collections::shortArrayListOpenTokensBC()[ $this->tokens[ $next ]['code'] ] ) + && Arrays::isShortArray( $this->phpcsFile, $next ) === true ) ) ) { $replacements = PassedParameters::getParameters( $this->phpcsFile, $next ); } From 0f7b3ad3f1d0df0cc2f04359b13761b348ec7faf Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 22:51:00 +0200 Subject: [PATCH 04/21] DB/PreparedSQLPlaceholders: minor simplification In both these cases, a "quick check" for the target function call was done prior to walking the tokens, but the token walking with a negative search for `Tokens::$emptyToken` doesn't contain the risk of walking very far, so this quick check isn't really needed and removing it shouldn't impact performance. --- WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index 8060ea62ee..a51c870dcb 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -635,10 +635,6 @@ protected function analyse_sprintf( $sprintf_params ) { unset( $sprintf_params[1] ); foreach ( $sprintf_params as $sprintf_param ) { - if ( strpos( strtolower( $sprintf_param['raw'] ), 'implode' ) === false ) { - continue; - } - $implode = $this->phpcsFile->findNext( Tokens::$emptyTokens, $sprintf_param['start'], @@ -689,10 +685,6 @@ protected function analyse_implode( $implode_token ) { return false; } - if ( strpos( strtolower( $implode_params[2]['raw'] ), 'array_fill' ) === false ) { - return false; - } - $array_fill = $this->phpcsFile->findNext( Tokens::$emptyTokens, $implode_params[2]['start'], From 7004173efdfdc816db4ddb75cad20b64d8c0a4ca Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 22:54:27 +0200 Subject: [PATCH 05/21] DB/PreparedSQLPlaceholders: prevent false positive due to comments in params [1] The `'raw'` key in the parameter arrays returned from the `PassedParameters` class contains - as per the name - the _raw_ contents of the parameter. Since PHPCSUtils 1.0.0-alpha4, the return array also contain a `'clean'` index, which contains the contents of the parameter cleaned of comments. By switching to using that key, a potential false positive gets fixed. Includes unit test demonstrating the issue and safeguarding the fix. --- WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php | 2 +- WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index a51c870dcb..7f63f2a9c6 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -681,7 +681,7 @@ protected function analyse_implode( $implode_token ) { return false; } - if ( preg_match( '`^(["\']), ?\1$`', $implode_params[1]['raw'] ) !== 1 ) { + if ( preg_match( '`^(["\']), ?\1$`', $implode_params[1]['clean'] ) !== 1 ) { return false; } diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index c4ebf915ad..f5b7597460 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -332,3 +332,12 @@ $wpdb->get_col( [ 'taxonomy_name', $limit ] ) // Ok. ); + +// Disregard comments in the implode() $separator param. +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( ',' /*comment*/, array_fill( 0, count($post_types), '%s' ) ) + ), + $post_types +); // OK. From abcba3d391af286cd609cbdf1153107668a279cb Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 22:56:50 +0200 Subject: [PATCH 06/21] DB/PreparedSQLPlaceholders: prevent false negative due to comments in params [2] The `'raw'` key in the parameter arrays returned from the `PassedParameters` class contains - as per the name - the _raw_ contents of the parameter. Since PHPCSUtils 1.0.0-alpha4, the return array also contain a `'clean'` index, which contains the contents of the parameter cleaned of comments. By switching to using that key, a potential false negative gets fixed. Includes unit test demonstrating the issue and safeguarding the fix. --- WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php | 4 ++-- WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc | 6 ++++++ WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php | 2 ++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index 7f63f2a9c6..451729f10f 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -704,8 +704,8 @@ protected function analyse_implode( $implode_token ) { return false; } - if ( "'%i'" === $array_fill_params[3]['raw'] - || '"%i"' === $array_fill_params[3]['raw'] + if ( "'%i'" === $array_fill_params[3]['clean'] + || '"%i"' === $array_fill_params[3]['clean'] ) { $this->phpcsFile->addError( 'The %i placeholder cannot be used within SQL `IN()` clauses.', diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index f5b7597460..8be4b9924a 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -341,3 +341,9 @@ $where = $wpdb->prepare( ), $post_types ); // OK. + +// Disregard comments in the array_fill() $value param. +$wpdb->prepare( ' + xxx IN ( ' . implode( ',', array_fill( 0, count( $post_types ), '%i' /*comment*/ ) ) . ' )', + $fields +); // IdentifierWithinIN. diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php index 252f15ccab..ef575ddf74 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php @@ -102,6 +102,8 @@ public function getErrorList() { 309 => 1, 316 => 1, 323 => 1, + + 347 => 2, // UnsupportedIdentifierPlaceholder + IdentifierWithinIN. ); } From 4fc71c3529dfda718894c3d8e91afdf9f47a06b5 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 23:20:36 +0200 Subject: [PATCH 07/21] DB/PreparedSQLPlaceholders: prevent false positive due to comments in params [3] The `'raw'` key in the parameter arrays returned from the `PassedParameters` class contains - as per the name - the _raw_ contents of the parameter. Since PHPCSUtils 1.0.0-alpha4, the return array also contain a `'clean'` index, which contains the contents of the parameter cleaned of comments. By switching to using that key, a potential false positive gets fixed. Includes unit test demonstrating the issue and safeguarding the fix. Also includes unit tests with an unsupported placeholder in the `array_fill()`, which looks like it was so far untested. --- .../DB/PreparedSQLPlaceholdersSniff.php | 2 +- .../DB/PreparedSQLPlaceholdersUnitTest.inc | 30 +++++++++++++++++++ .../DB/PreparedSQLPlaceholdersUnitTest.php | 3 ++ 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index 451729f10f..73895e6309 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -715,6 +715,6 @@ protected function analyse_implode( $implode_token ) { return false; } - return (bool) preg_match( '`^(["\'])%[dfFs]\1$`', $array_fill_params[3]['raw'] ); + return (bool) preg_match( '`^(["\'])%[dfFs]\1$`', $array_fill_params[3]['clean'] ); } } diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index 8be4b9924a..d61c146b89 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -347,3 +347,33 @@ $wpdb->prepare( ' xxx IN ( ' . implode( ',', array_fill( 0, count( $post_types ), '%i' /*comment*/ ) ) . ' )', $fields ); // IdentifierWithinIN. + +$where = $wpdb->prepare( + "{$wpdb->posts}.post_type IN (" + . implode( ',', array_fill( 0, count($post_types), '%C' ) ) + . ')', + array_merge( $post_types ) +); // Bad x 2, UnsupportedPlaceholder + UnfinishedPrepare. + +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( ',', array_fill( 0, count($post_types), '%C' ) ) + ), + $post_types +); // Bad, ReplacementsWrongNumber due to unrecognized placeholder in array_fill(). + +$where = $wpdb->prepare( + "{$wpdb->posts}.post_type IN (" + . implode( ',', array_fill( 0, count($post_types), /*comment*/ '%s' ) ) + . ')', + array_merge( $post_types ) +); // OK. + +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( ',', array_fill( 0, count($post_types), "%s" /*comment*/ ) ) + ), + $post_types +); // OK. diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php index ef575ddf74..2c4f5d55ec 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php @@ -104,6 +104,7 @@ public function getErrorList() { 323 => 1, 347 => 2, // UnsupportedIdentifierPlaceholder + IdentifierWithinIN. + 353 => 1, ); } @@ -148,6 +149,8 @@ public function getWarningList() { 246 => 1, // UnfinishedPrepare. 258 => 1, // ReplacementsWrongNumber. 300 => 1, + 354 => 1, + 358 => 1, ); } } From eee85c05e8d692841892ccf156d2b1a26023b5d7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 22:44:29 +0200 Subject: [PATCH 08/21] DB/PreparedSQLPlaceholders: bug fix - allow for FQN function calls When looking for a function calls to `implode()` or `array_fill()`, the sniff did not allow for fully qualified function calls, which would lead to false positives. Fixed now. Includes unit tests demonstrating the issue and safeguarding the fix. --- .../DB/PreparedSQLPlaceholdersSniff.php | 4 +-- .../DB/PreparedSQLPlaceholdersUnitTest.inc | 26 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index 73895e6309..554f703cf2 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -636,7 +636,7 @@ protected function analyse_sprintf( $sprintf_params ) { foreach ( $sprintf_params as $sprintf_param ) { $implode = $this->phpcsFile->findNext( - Tokens::$emptyTokens, + Tokens::$emptyTokens + array( \T_NS_SEPARATOR => \T_NS_SEPARATOR ), $sprintf_param['start'], $sprintf_param['end'], true @@ -686,7 +686,7 @@ protected function analyse_implode( $implode_token ) { } $array_fill = $this->phpcsFile->findNext( - Tokens::$emptyTokens, + Tokens::$emptyTokens + array( \T_NS_SEPARATOR => \T_NS_SEPARATOR ), $implode_params[2]['start'], $implode_params[2]['end'], true diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index d61c146b89..7d223720bd 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -377,3 +377,29 @@ $where = $wpdb->prepare( ), $post_types ); // OK. + +// Safeguard that FQN function calls to implode() and array_fill() are handled correctly. +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + \implode( ',', array_fill( 0, count($post_types), '%s' ) ) + ), + $post_types +); // OK. + +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( ',', \array_fill( 0, count($post_types), '%s' ) ) + ), + $post_types +); // OK. + +$where = $wpdb->prepare( + "{$wpdb->posts}.post_type IN (" + . implode( ',', \array_fill( 0, count($post_types), '%s' ) ) + . ") AND {$wpdb->posts}.post_status IN (" + . implode( ',', \array_fill( 0, count($post_statusses), '%s' ) ) + . ')', + array_merge( $post_types, $post_statusses ) +); // OK. From 7a4259fc9f5df3c03042c5b47cb3d698a90ddd25 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 23:25:37 +0200 Subject: [PATCH 09/21] DB/PreparedSQLPlaceholders: bug fix - sniff could skip too much As things were, the `$skip_to` param would lead to the first token _after_ the parenthesis closer also being skipped. This was not the intention and while - for valid code - this won't lead to false positives/negatives, it should still be fixed. --- WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index 554f703cf2..b6a8d005c8 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -208,7 +208,7 @@ public function process_token( $stackPtr ) { for ( $i = $query['start']; $i <= $query['end']; $i++ ) { // Skip over groups of tokens if they are part of an inline function call. - if ( isset( $skip_from, $skip_to ) && $i >= $skip_from && $i < $skip_to ) { + if ( isset( $skip_from, $skip_to ) && $i >= $skip_from && $i <= $skip_to ) { $i = $skip_to; continue; } @@ -267,7 +267,7 @@ public function process_token( $stackPtr ) { && isset( $this->tokens[ $next ]['parenthesis_closer'] ) ) { $skip_from = ( $i + 1 ); - $skip_to = ( $this->tokens[ $next ]['parenthesis_closer'] + 1 ); + $skip_to = $this->tokens[ $next ]['parenthesis_closer']; } } } From 7d06fbb066dc9bc3bcbb1568cd290bf1402a6909 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 23:41:32 +0200 Subject: [PATCH 10/21] DB/PreparedSQLPlaceholders: improve error message precision for IdentifierWithinIN Throw the error for `IdentifierWithinIN` error on the actual text string token in the parameter instead of on the function call to `implode()` to give a better indication where the actual error occurred. Safeguarded by adjusting one of the pre-existing tests. --- WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php | 4 +++- WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index b6a8d005c8..c26efc8a1e 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -707,9 +707,11 @@ protected function analyse_implode( $implode_token ) { if ( "'%i'" === $array_fill_params[3]['clean'] || '"%i"' === $array_fill_params[3]['clean'] ) { + $firstNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $array_fill_params[3]['start'], $array_fill_params[3]['end'], true ); + $this->phpcsFile->addError( 'The %i placeholder cannot be used within SQL `IN()` clauses.', - $implode_token, + $firstNonEmpty, 'IdentifierWithinIN' ); return false; diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index 7d223720bd..c50bde4908 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -263,8 +263,8 @@ $wpdb->prepare( $fields ); // ReplacementsWrongNumber + IdentifierWithinIN. -$wpdb->prepare( ' - xxx IN ( ' . implode( ',', array_fill( 0, count( $post_types ), '%i' ) ) . ' )', +$wpdb->prepare( 'xxx IN ( ' . implode( ',', array_fill( 0, count( $post_types ), + '%i' ) ) . ' )', $fields ); // IdentifierWithinIN. From bc75ffe6475fdf8b967679b9e9075c0c78d1c6a8 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 23 Jul 2023 00:02:25 +0200 Subject: [PATCH 11/21] DB/PreparedSQLPlaceholders: improve error message precision for QuotedDynamicPlaceholderGeneration Throw the error for `QuotedDynamicPlaceholderGeneration` error on the actual text string token which contains the open quote instead of on the function call to `implode()` to give a better indication where the actual error occurred. Safeguarded via the existing tests. --- .../Sniffs/DB/PreparedSQLPlaceholdersSniff.php | 2 +- .../Tests/DB/PreparedSQLPlaceholdersUnitTest.php | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index c26efc8a1e..9a409561b1 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -253,7 +253,7 @@ public function process_token( $stackPtr ) { if ( isset( $match[1] ) && $regex_quote !== $this->regex_quote ) { $this->phpcsFile->addError( 'Dynamic placeholder generation should not have surrounding quotes.', - $i, + $prev, 'QuotedDynamicPlaceholderGeneration' ); } diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php index 2c4f5d55ec..1ba40fea81 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php @@ -39,8 +39,8 @@ public function getErrorList() { 41 => 1, 54 => 1, 141 => 1, - 149 => 1, - 151 => 1, + 148 => 1, + 150 => 1, 162 => 1, 163 => 1, 164 => 1, @@ -96,12 +96,12 @@ public function getErrorList() { 279 => 2, // UnsupportedIdentifierPlaceholder + QuotedIdentifierPlaceholder. 280 => 2, // UnsupportedIdentifierPlaceholder + IdentifierWithinIN. - 288 => 1, - 295 => 1, - 302 => 1, - 309 => 1, - 316 => 1, - 323 => 1, + 287 => 1, + 294 => 1, + 301 => 1, + 308 => 1, + 315 => 1, + 322 => 1, 347 => 2, // UnsupportedIdentifierPlaceholder + IdentifierWithinIN. 353 => 1, From 01c5bf4e6e3f245d7508c09277c0a8f670cf05a8 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 23 Jul 2023 00:12:16 +0200 Subject: [PATCH 12/21] DB/PreparedSQLPlaceholders: prevent token walking too far When an `implode()` function call is found in a `$query`, the sniff tries to find the preceding text string token to check for quotes around the placeholders. While it is unlikely that an `implode()` wouldn't be preceded by a text string token + concatenation token, it could still be possible. This updates the logic to do a negative search for the previous non-empty, non-concat token instead to prevent the sniff from walking too far. I've not included a test as I couldn't come up with a valid code snippet which would hit a false positive, but it should still be fixed IMO. :point_right: This commit will be easier to review while ignoring whitespace changes. --- .../DB/PreparedSQLPlaceholdersSniff.php | 52 ++++++++++--------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index 9a409561b1..47c9924a12 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -239,39 +239,43 @@ public function process_token( $stackPtr ) { } elseif ( 'implode' === strtolower( $this->tokens[ $i ]['content'] ) ) { $prev = $this->phpcsFile->findPrevious( - Tokens::$textStringTokens, + Tokens::$emptyTokens + array( \T_STRING_CONCAT => \T_STRING_CONCAT ), ( $i - 1 ), - $query['start'] + $query['start'], + true ); - $prev_content = TextStrings::stripQuotes( $this->tokens[ $prev ]['content'] ); - $regex_quote = $this->get_regex_quote_snippet( $prev_content, $this->tokens[ $prev ]['content'] ); + if ( isset( Tokens::$textStringTokens[ $this->tokens[ $prev ]['code'] ] ) ) { + $prev_content = TextStrings::stripQuotes( $this->tokens[ $prev ]['content'] ); + $regex_quote = $this->get_regex_quote_snippet( $prev_content, $this->tokens[ $prev ]['content'] ); - // Only examine the implode if preceded by an ` IN (`. - if ( preg_match( '`\s+IN\s*\(\s*(' . $regex_quote . ')?$`i', $prev_content, $match ) > 0 ) { + // Only examine the implode if preceded by an ` IN (`. + if ( preg_match( '`\s+IN\s*\(\s*(' . $regex_quote . ')?$`i', $prev_content, $match ) > 0 ) { - if ( isset( $match[1] ) && $regex_quote !== $this->regex_quote ) { - $this->phpcsFile->addError( - 'Dynamic placeholder generation should not have surrounding quotes.', - $prev, - 'QuotedDynamicPlaceholderGeneration' - ); - } - - if ( $this->analyse_implode( $i ) === true ) { - ++$valid_in_clauses['uses_in']; - ++$valid_in_clauses['implode_fill']; + if ( isset( $match[1] ) && $regex_quote !== $this->regex_quote ) { + $this->phpcsFile->addError( + 'Dynamic placeholder generation should not have surrounding quotes.', + $prev, + 'QuotedDynamicPlaceholderGeneration' + ); + } - $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true ); - if ( \T_OPEN_PARENTHESIS === $this->tokens[ $next ]['code'] - && isset( $this->tokens[ $next ]['parenthesis_closer'] ) - ) { - $skip_from = ( $i + 1 ); - $skip_to = $this->tokens[ $next ]['parenthesis_closer']; + if ( $this->analyse_implode( $i ) === true ) { + ++$valid_in_clauses['uses_in']; + ++$valid_in_clauses['implode_fill']; + + $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true ); + if ( \T_OPEN_PARENTHESIS === $this->tokens[ $next ]['code'] + && isset( $this->tokens[ $next ]['parenthesis_closer'] ) + ) { + $skip_from = ( $i + 1 ); + $skip_to = $this->tokens[ $next ]['parenthesis_closer']; + } } } + unset( $next, $prev_content, $regex_quote, $match ); } - unset( $prev, $next, $prev_content, $regex_quote, $match ); + unset( $prev ); } } From e2f237fa49a9b18534e03e7271e02379f772d352 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 22:04:22 +0200 Subject: [PATCH 13/21] DB/PreparedSQLPlaceholders: add tests with PHP 7.3+ trailing commas in function calls This is already handled correctly by the PHPCSUtils `PassedParameters` class. This commit just adjusts some pre-existing tests to safeguard this for this sniff. --- WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index c50bde4908..2392c9d987 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -83,10 +83,10 @@ $where = $wpdb->prepare( sprintf( "{$wpdb->posts}.post_type IN (%s) AND {$wpdb->posts}.post_status IN (%s)", - implode( ',', array_fill( 0, count($post_types), '%s' ) ), - IMPLODE( ',', Array_Fill( 0, count($post_statusses), '%s' ) ) + implode( ',', array_fill( 0, count($post_types), '%s' ), ), + IMPLODE( ',', Array_Fill( 0, count($post_statusses), '%s' ), ), ), - array_merge( $post_types, $post_statusses ) + array_merge( $post_types, $post_statusses, ), ); // OK. $where = $wpdb->prepare( From d61a6167b080bd02260f15c2291a3d0889828b55 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 21:56:44 +0200 Subject: [PATCH 14/21] DB/PreparedSQLPlaceholders: add tests with PHP 8.0+ nullsafe object operator [1] The nullsafe object operator when calling `$wpdb->prepare()` is handled correctly in the `WPDBTrait::is_wpdb_method_call()` method. Adjusted some pre-existing tests to safeguard this. --- WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index 2392c9d987..5617791b63 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -2,7 +2,7 @@ $sql = $wpdb->prepare( $sql, $replacements ); // OK - no query available to examine - this will be handled by the PreparedSQL sniff. $sql = $wpdb->prepare( "SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s", 1, "admin" ); // OK. -$sql = $wpdb->prepare( "SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s", array( 1, "admin" ) ); // OK. +$sql = $wpdb?->prepare( "SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s", array( 1, "admin" ) ); // OK. $sql = $wpdb->prepare( 'SELECT * FROM `table` WHERE `column` = %s AND `field` = %d', 'foo', 1337 ); // OK. $sql = $wpdb->prepare( 'SELECT DATE_FORMAT(`field`, "%%c") FROM `table` WHERE `column` = %s', 'foo' ); // OK. @@ -13,7 +13,7 @@ $sql = $wpdb->prepare( 'SELECT * FROM `table`' ); // Warning. $sql = $wpdb->prepare( 'SELECT * FROM `table` WHERE id = ' . $id ); // OK - this will be handled by the PreparedSQL sniff. $sql = $wpdb->prepare( "SELECT * FROM `table` WHERE id = $id" ); // OK - this will be handled by the PreparedSQL sniff. $sql = $wpdb->prepare( "SELECT * FROM `table` WHERE id = {$id['some%sing']}" ); // OK - this will be handled by the PreparedSQL sniff. -$sql = $wpdb->prepare( 'SELECT * FROM ' . $wpdb->users ); // Warning. +$sql = $wpdb?->prepare( 'SELECT * FROM ' . $wpdb->users ); // Warning. $sql = $wpdb->prepare( "SELECT * FROM `{$wpdb->users}`" ); // Warning. $sql = $wpdb->prepare( "SELECT * FROM `{$wpdb->users}` WHERE id = $id" ); // OK - this will be handled by the PreparedSQL sniff. From b1c9095a7879d7e4e5b522b2019f0f151e82b43f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 22:02:30 +0200 Subject: [PATCH 15/21] DB/PreparedSQLPlaceholders: add tests with PHP 8.0+ nullsafe object operator [2] The sniff looks for variables in a text string, other than `$wpdb`. It doesn't actually look at whether object access is used after the variable. This just adjusts some pre-existing tests to use the nullsafe object operator on variables within the query to safeguard this behaviour against potential future regressions. --- WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index 5617791b63..65996b75a0 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -27,11 +27,11 @@ $sql = $wpdb->prepare( 'SELECT * FROM `table`', $something ); // Warning. */ $sql = $wpdb->prepare( '%d %1$e %%% % %A %h', 1 ); // Bad x 5. $sql = $wpdb->prepare( '%%%s', 1 ); // OK. -$sql = $wpdb->prepare( "SELECT * FROM $wpdb->users WHERE id = %1\$d AND user_login = %2\$s", 1, "admin" ); // OK. 2 x warning for unquoted complex placeholders. -$sql = $wpdb->prepare( "SELECT * FROM $wpdb->users WHERE id = %01.2f AND user_login = %10.10X", 1, "admin" ); // Bad x 1 + 1 warning unquoted complex placeholders + 1 warning nr of replacements. +$sql = $wpdb->prepare( "SELECT * FROM $wpdb?->users WHERE id = %1\$d AND user_login = %2\$s", 1, "admin" ); // OK. 2 x warning for unquoted complex placeholders. +$sql = $wpdb->prepare( "SELECT * FROM $wpdb?->users WHERE id = %01.2f AND user_login = %10.10X", 1, "admin" ); // Bad x 1 + 1 warning unquoted complex placeholders + 1 warning nr of replacements. $sql = $wpdb->prepare( "SELECT * FROM $wpdb->users WHERE id = %'.09F AND user_login = %1\$04x", 1, "admin" ); // Bad x 1 + 1 warning unquoted complex placeholders + 1 warning nr of replacements. $sql = $wpdb->prepare( "SELECT * FROM $wpdb->users WHERE id = \"%1\$c\" AND user_login = '%2\$e'", 1, "admin" ); // Bad x 2 + 1 warning. -$sql = $wpdb->prepare( 'SELECT * FROM ' . $wpdb->users . ' WHERE id = \'%1\$b\' AND user_login = "%2\$o"', 1, "admin" ); // Bad x 2 + 1 warning. +$sql = $wpdb->prepare( 'SELECT * FROM ' . $wpdb?->users . ' WHERE id = \'%1\$b\' AND user_login = "%2\$o"', 1, "admin" ); // Bad x 2 + 1 warning. /* * Test passing quoted simple replacement placeholder and unquoted complex placeholder. @@ -114,7 +114,7 @@ $results = $wpdb->get_results( $wpdb->prepare( ' SELECT ID FROM ' . $wpdb->posts . ' - WHERE ID NOT IN( SELECT post_id FROM ' . $wpdb->postmeta . ' WHERE meta_key = %s AND meta_value = %s ) + WHERE ID NOT IN( SELECT post_id FROM ' . $wpdb?->postmeta . ' WHERE meta_key = %s AND meta_value = %s ) AND post_status in( "future", "draft", "pending", "private", "publish" ) AND post_type in( ' . implode( ',', array_fill( 0, count( $post_types ), '%s' ) ) . ' ) LIMIT %d', From 8f745ff191205d36836695a40a7fd92a9eee2e22 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 11 Jan 2023 17:00:45 +0100 Subject: [PATCH 16/21] DB/PreparedSQLPlaceholders: add support for PHP 8.0+ named parameters [1] 1. Adjusted the way the correct parameters are retrieved from a call to `wpdb::prepare()` to use the new PHPCSUtils 1.0.0-alpha4 `PassedParameters::getParameterFromStack()` method. 2. Verified the parameter names used are in line with the name as per the WP 6.3 release. WP has been renaming parameters and is probably not done yet, but it doesn't look like those changes (so far) made it into changelog entries.... For the purposes of this exercise, I've taken the _current_ parameter name as the "truth" as support for named parameters hasn't officially been announced yet, so any renames _after_ this moment are the only ones relevant. Also note that `wpdb::prepare()` is a variadic function, which by its nature doesn't support named arguments, but that's not the concern of this sniff. Includes additional unit tests. --- .../DB/PreparedSQLPlaceholdersSniff.php | 17 ++++++++----- .../DB/PreparedSQLPlaceholdersUnitTest.inc | 24 +++++++++++++++++++ .../DB/PreparedSQLPlaceholdersUnitTest.php | 3 +++ 3 files changed, 38 insertions(+), 6 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index 47c9924a12..a899f13fe2 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -192,7 +192,11 @@ public function process_token( $stackPtr ) { return; } - $query = $parameters[1]; + $query = PassedParameters::getParameterFromStack( $parameters, 1, 'query' ); + if ( false === $query ) { + return; + } + $text_string_tokens_found = false; $variable_found = false; $sql_wildcard_found = false; @@ -545,14 +549,15 @@ function ( $symbol ) { } $replacements = $parameters; - array_shift( $replacements ); // Remove the query. + unset( $replacements['query'], $replacements[1] ); // Remove the query param, whether passed positionally or named. - // The parameters may have been passed as an array in parameter 2. - if ( isset( $parameters[2] ) && 2 === $total_parameters ) { + // The parameters may have been passed as an array in the variadic $args parameter. + $args_param = PassedParameters::getParameterFromStack( $parameters, 2, 'args' ); + if ( false !== $args_param && 2 === $total_parameters ) { $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, - $parameters[2]['start'], - ( $parameters[2]['end'] + 1 ), + $args_param['start'], + ( $args_param['end'] + 1 ), true ); diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index 65996b75a0..39abea1752 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -403,3 +403,27 @@ $where = $wpdb->prepare( . ')', array_merge( $post_types, $post_statusses ) ); // OK. + +/* + * Safeguard support for PHP 8.0+ named parameters. + */ +// WPDB::prepare() with named params. Named args not supported with ...$args, but that's not the concern of this sniff. +$query = $wpdb->prepare( + args : $replacements, + query : 'SELECT ID + FROM ' . $wpdb->posts . ' + WHERE post_type = %s', +); // OK, named args not supported with ...$args, but that's not the concern of this sniff. + +$query = $wpdb->prepare( + query : 'SELECT ID + FROM ' . $wpdb->posts . ' + WHERE post_type = %s', +); // Bad, missing replacements. + +$query = $wpdb->prepare( + args : $replacements, + queri : 'SELECT ID + FROM ' . $wpdb->posts . ' + WHERE post_type = %s', +); // Ignore, incorrect name used for named param `query`, so param not recognized. diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php index 1ba40fea81..9444208b58 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php @@ -105,6 +105,9 @@ public function getErrorList() { 347 => 2, // UnsupportedIdentifierPlaceholder + IdentifierWithinIN. 353 => 1, + + // Named parameter support. + 418 => 1, ); } From 7e13937d109ba0b560a020d15b2f4fad9172b781 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 11 Jan 2023 19:15:56 +0100 Subject: [PATCH 17/21] DB/PreparedSQLPlaceholders: add support for PHP 8.0+ named parameters [2] 1. Adjusted the way the correct parameters are retrieved from a call to `implode()` to use the new PHPCSUtils 1.0.0-alpha4 `PassedParameters::getParameterFromStack()` method. 2. The parameter names used for `implode()` are in line with the names as per the PHP 8.0 release. PHP itself renamed a lot of parameters in PHP 8.0. As named parameters did not exist before PHP 8.0, the parameter name as per PHP 8.0 (or above) are the only relevant names. Includes additional unit tests. --- .../DB/PreparedSQLPlaceholdersSniff.php | 15 ++++++--- .../DB/PreparedSQLPlaceholdersUnitTest.inc | 33 +++++++++++++++++++ .../DB/PreparedSQLPlaceholdersUnitTest.php | 5 +++ 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index a899f13fe2..3a1d1fe4a3 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -685,19 +685,26 @@ protected function analyse_sprintf( $sprintf_params ) { */ protected function analyse_implode( $implode_token ) { $implode_params = PassedParameters::getParameters( $this->phpcsFile, $implode_token ); - if ( empty( $implode_params ) || \count( $implode_params ) !== 2 ) { return false; } - if ( preg_match( '`^(["\']), ?\1$`', $implode_params[1]['clean'] ) !== 1 ) { + $implode_separator_param = PassedParameters::getParameterFromStack( $implode_params, 1, 'separator' ); + if ( false === $implode_separator_param + || preg_match( '`^(["\']), ?\1$`', $implode_separator_param['clean'] ) !== 1 + ) { + return false; + } + + $implode_array_param = PassedParameters::getParameterFromStack( $implode_params, 2, 'array' ); + if ( false === $implode_array_param ) { return false; } $array_fill = $this->phpcsFile->findNext( Tokens::$emptyTokens + array( \T_NS_SEPARATOR => \T_NS_SEPARATOR ), - $implode_params[2]['start'], - $implode_params[2]['end'], + $implode_array_param['start'], + $implode_array_param['end'], true ); diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index 39abea1752..04b83d40b5 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -427,3 +427,36 @@ $query = $wpdb->prepare( FROM ' . $wpdb->posts . ' WHERE post_type = %s', ); // Ignore, incorrect name used for named param `query`, so param not recognized. + +// Implode() with named params. +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( array: array_fill( 0, count( $post_types ), '%s' ), separator: ',', ), + ), + array_merge( $post_types, $post_statusses ) +); // Okay. + +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( array: $something, separator: ',', ), + ), + array_merge( $post_types, $post_statusses ) +); // Bad - will throw incorrect nr of replacements error - `array` param is not an array_fill() function call. + +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( arrays: array_fill( 0, count( $post_types ), '%s' ), separator: ',', ), + ), + array_merge( $post_types, $post_statusses ) +); // Bad - will throw incorrect nr of replacements error - incorrect name used for named param `array`, so param not recognized. + +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( separator: ',', ), + ), + array_merge( $post_types, $post_statusses ) +); // Bad - will throw incorrect nr of replacements error - missing `array` param. diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php index 9444208b58..a795aee26d 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php @@ -154,6 +154,11 @@ public function getWarningList() { 300 => 1, 354 => 1, 358 => 1, + + // Named parameter support. + 440 => 1, + 448 => 1, + 456 => 1, ); } } From 995ff4364697cd14518d90da8bf5ebd3e9e0a40b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 8 Apr 2023 20:07:13 +0200 Subject: [PATCH 18/21] DB/PreparedSQLPlaceholders: add support for PHP 8.0+ named parameters [3] 1. Adjusted the way the correct parameters are retrieved from a call to `array_fill()` to use the PHPCSUtils `PassedParameters::getParameter()` method with named parameter support. 2. The parameter names used for `array_fill()` are in line with the names as per the PHP 8.0 release. PHP itself renamed a lot of parameters in PHP 8.0. As named parameters did not exist before PHP 8.0, the parameter name as per PHP 8.0 (or above) are the only relevant names. Includes additional unit tests. --- .../DB/PreparedSQLPlaceholdersSniff.php | 13 +++--- .../DB/PreparedSQLPlaceholdersUnitTest.inc | 43 +++++++++++++++++++ .../DB/PreparedSQLPlaceholdersUnitTest.php | 4 ++ 3 files changed, 53 insertions(+), 7 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index 3a1d1fe4a3..d69e59f0ef 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -714,16 +714,15 @@ protected function analyse_implode( $implode_token ) { return false; } - $array_fill_params = PassedParameters::getParameters( $this->phpcsFile, $array_fill ); - - if ( empty( $array_fill_params ) || \count( $array_fill_params ) !== 3 ) { + $array_fill_value_param = PassedParameters::getParameter( $this->phpcsFile, $array_fill, 3, 'value' ); + if ( false === $array_fill_value_param ) { return false; } - if ( "'%i'" === $array_fill_params[3]['clean'] - || '"%i"' === $array_fill_params[3]['clean'] + if ( "'%i'" === $array_fill_value_param['clean'] + || '"%i"' === $array_fill_value_param['clean'] ) { - $firstNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $array_fill_params[3]['start'], $array_fill_params[3]['end'], true ); + $firstNonEmpty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $array_fill_value_param['start'], $array_fill_value_param['end'], true ); $this->phpcsFile->addError( 'The %i placeholder cannot be used within SQL `IN()` clauses.', @@ -733,6 +732,6 @@ protected function analyse_implode( $implode_token ) { return false; } - return (bool) preg_match( '`^(["\'])%[dfFs]\1$`', $array_fill_params[3]['clean'] ); + return (bool) preg_match( '`^(["\'])%[dfFs]\1$`', $array_fill_value_param['clean'] ); } } diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index 04b83d40b5..78e4061cab 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -460,3 +460,46 @@ $where = $wpdb->prepare( ), array_merge( $post_types, $post_statusses ) ); // Bad - will throw incorrect nr of replacements error - missing `array` param. + +// Array_fill() with named params. +$where = $wpdb->prepare( + "{$wpdb->posts}.post_type IN (" + . implode( ',', array_fill( start_index: 0, count: count($post_types), value: '%s' ) ) // Expected order. + . ") AND {$wpdb->posts}.post_status IN (" + . implode( ',', array_fill( value: '%s', start_index: 0, count: count($post_statusses), ) ) // Unconventional order. + . ')', + array_merge( $post_types, $post_statusses ) +); // OK. + +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( ',', array_fill( start_index: 0, count: count($post_types) ) ), + ), + array_merge( $post_types, $post_statusses ) +); // Bad - will throw incorrect nr of replacements error - missing $value param in array_fill(). + +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( ',', array_fill( start_index: 0, values: '%s', count: count($post_types) ) ), + ), + array_merge( $post_types, $post_statusses ) +); // Bad - will throw incorrect nr of replacements error - incorrect $values param name in array_fill(). + +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( ',', array_fill( value: 's', start_index: 0, count: count($post_types) ) ), + ), + array_merge( $post_types, $post_statusses ) +); // Bad - will throw incorrect nr of replacements error - $value param name in array_fill() does not contain placeholder. + +$where = $wpdb->prepare( + sprintf( + "{$wpdb->posts}.post_type IN (%s)", + implode( ',', array_fill( value: $s, start_index: 0, count: count($post_types) ) ), + ), + array_merge( $post_types, $post_statusses ) +); // Bad - will throw incorrect nr of replacements error - $value param name in array_fill() does not contain plain text placeholder. + diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php index a795aee26d..fc491c5080 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.php @@ -159,6 +159,10 @@ public function getWarningList() { 440 => 1, 448 => 1, 456 => 1, + 474 => 1, + 482 => 1, + 490 => 1, + 498 => 1, ); } } From 6063a6afc8994eb39871228dfeac5e6a7c6f5856 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 21:50:25 +0200 Subject: [PATCH 19/21] DB/PreparedSQLPlaceholders: add support for PHP 8.0+ named parameters [4] `sprintf()` does not support named parameters due to its variadic nature and analyzing code which would use named parameters with `sprintf()` is too prone to incorrect sniff results. So instead of allowing for an incorrect call with named parameters, skip the whole `sprintf()` call if named parameters would be used. Includes additional unit test. --- .../DB/PreparedSQLPlaceholdersSniff.php | 30 +++++++++++++++++-- .../DB/PreparedSQLPlaceholdersUnitTest.inc | 9 ++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index d69e59f0ef..9785809be8 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -232,6 +232,32 @@ public function process_token( $stackPtr ) { $sprintf_parameters = PassedParameters::getParameters( $this->phpcsFile, $i ); if ( ! empty( $sprintf_parameters ) ) { + /* + * Check for named params. sprintf() does not support this due to its variadic nature, + * and we cannot analyse the code correctly if it is used, so skip the whole sprintf() + * in that case. + */ + $valid_sprintf = true; + foreach ( $sprintf_parameters as $param ) { + if ( isset( $param['name'] ) ) { + $valid_sprintf = false; + break; + } + } + + if ( false === $valid_sprintf ) { + $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $i + 1 ), null, true ); + if ( \T_OPEN_PARENTHESIS === $this->tokens[ $next ]['code'] + && isset( $this->tokens[ $next ]['parenthesis_closer'] ) + ) { + $skip_from = ( $i + 1 ); + $skip_to = $this->tokens[ $next ]['parenthesis_closer']; + } + + continue; + } + + // We know for sure this sprintf() uses positional parameters, so this will be fine. $skip_from = ( $sprintf_parameters[1]['end'] + 1 ); $last_param = end( $sprintf_parameters ); $skip_to = ( $last_param['end'] + 1 ); @@ -239,7 +265,7 @@ public function process_token( $stackPtr ) { $valid_in_clauses['implode_fill'] += $this->analyse_sprintf( $sprintf_parameters ); $valid_in_clauses['adjustment_count'] += ( \count( $sprintf_parameters ) - 1 ); } - unset( $sprintf_parameters, $last_param ); + unset( $sprintf_parameters, $valid_sprintf, $last_param ); } elseif ( 'implode' === strtolower( $this->tokens[ $i ]['content'] ) ) { $prev = $this->phpcsFile->findPrevious( @@ -641,7 +667,7 @@ protected function get_regex_quote_snippet( $stripped_content, $original_content protected function analyse_sprintf( $sprintf_params ) { $found = 0; - unset( $sprintf_params[1] ); + unset( $sprintf_params[1] ); // Remove the positionally passed $format param. foreach ( $sprintf_params as $sprintf_param ) { $implode = $this->phpcsFile->findNext( diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index 78e4061cab..e748b03291 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -503,3 +503,12 @@ $where = $wpdb->prepare( array_merge( $post_types, $post_statusses ) ); // Bad - will throw incorrect nr of replacements error - $value param name in array_fill() does not contain plain text placeholder. +// Sprintf() with named params. This is invalid as variadic functions do not support named params. +// The sniff will ignore the sprintf() as it cannot be analyzed correctly. +$where = $wpdb->prepare( + sprintf( + values: implode( ',', array_fill( 0, count($post_types), '%s' ) ), + format: "{$wpdb->posts}.post_type IN ('%s')", + ), + array_merge( $post_types, $post_statusses ) +); // OK, well not really, but not something we can reliably analyze. From 57df20c25b7b1e8877501a046e3c1bb3b6458c5a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 22 Jul 2023 22:09:03 +0200 Subject: [PATCH 20/21] DB/PreparedSQLPlaceholders: add tests with PHP 8.1+ first class callable Just a test to safeguard this won't cause any false positives. --- WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc index e748b03291..bbceb7ce5b 100644 --- a/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc +++ b/WordPress/Tests/DB/PreparedSQLPlaceholdersUnitTest.inc @@ -512,3 +512,9 @@ $where = $wpdb->prepare( ), array_merge( $post_types, $post_statusses ) ); // OK, well not really, but not something we can reliably analyze. + +/* + * Safeguard handling of $wpdb->prepare as PHP 8.1+ first class callable. + */ +$callback = $wpdb->prepare(...); // OK. + From 9045552484f71ca16bb22521623079c2416a7d02 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 12 Apr 2023 03:45:45 +0200 Subject: [PATCH 21/21] DB/PreparedSQLPlaceholders: minor documentation fix --- WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php index 9785809be8..e3aa609895 100644 --- a/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php +++ b/WordPress/Sniffs/DB/PreparedSQLPlaceholdersSniff.php @@ -19,9 +19,9 @@ use WordPressCS\WordPress\Sniff; /** - * Check for incorrect use of the $wpdb->prepare method. + * Checks for incorrect use of the $wpdb->prepare method. * - * Check the following issues: + * Checks the following issues: * - The only placeholders supported are: %d, %f (%F), %s, %i, and their variations. * - Literal % signs need to be properly escaped as `%%`. * - Simple placeholders (%d, %f, %F, %s, %i) should be left unquoted in the query string.