diff --git a/includes/Checker/Checks/Plugin_Repo/Plugin_Review_PHPCS_Check.php b/includes/Checker/Checks/Plugin_Repo/Plugin_Review_PHPCS_Check.php index c2af96eea..2ae61f2af 100644 --- a/includes/Checker/Checks/Plugin_Repo/Plugin_Review_PHPCS_Check.php +++ b/includes/Checker/Checks/Plugin_Repo/Plugin_Review_PHPCS_Check.php @@ -96,6 +96,12 @@ protected function add_result_message_for_file( Check_Result $result, $error, $m $docs = 'https://make.wordpress.org/core/2016/07/06/i18n-improvements-in-4-6/'; } + if ( 'WordPress.DB.PreparedSQL.NotPrepared' === $code && $error ) { + $error = false; + $message .= ' ' . esc_html__( 'If the query variable is already being passed through $wpdb->prepare(), this warning may be a false positive. You can suppress it by adding a phpcs:ignore comment.', 'plugin-check' ); + $docs = 'https://developer.wordpress.org/reference/classes/wpdb/prepare/'; + } + parent::add_result_message_for_file( $result, $error, $message, $code, $file, $line, $column, $docs, $severity ); } } diff --git a/includes/Checker/Checks/Security/Late_Escaping_Check.php b/includes/Checker/Checks/Security/Late_Escaping_Check.php index 61eab021e..1dcdf1c36 100644 --- a/includes/Checker/Checks/Security/Late_Escaping_Check.php +++ b/includes/Checker/Checks/Security/Late_Escaping_Check.php @@ -97,7 +97,14 @@ public function get_documentation_url(): string { protected function add_result_message_for_file( Check_Result $result, $error, $message, $code, $file, $line = 0, $column = 0, string $docs = '', $severity = 5 ) { switch ( $code ) { case 'WordPress.Security.EscapeOutput.OutputNotEscaped': - $docs = __( 'https://developer.wordpress.org/apis/security/escaping/#escaping-functions', 'plugin-check' ); + if ( $this->line_calls_save_dom_method( $file, $line ) ) { + // DOMDocument::saveHtml() and saveXML() return safe HTML/XML + // constructed through the DOM API — additional escaping is not needed. + $error = false; + $docs = 'https://www.php.net/manual/en/domdocument.savehtml.php'; + } else { + $docs = __( 'https://developer.wordpress.org/apis/security/escaping/#escaping-functions', 'plugin-check' ); + } break; case 'WordPress.Security.EscapeOutput.UnsafePrintingFunction': @@ -115,4 +122,39 @@ protected function add_result_message_for_file( Check_Result $result, $error, $m parent::add_result_message_for_file( $result, $error, $message, $code, $file, $line, $column, $docs, $severity ); } + + /** + * Checks whether the given file/line contains a call to a safe DOM output method. + * + * DOMDocument::saveHtml() and DOMDocument::saveXML() return HTML/XML + * constructed through the DOM API. The output is already structured and + * does not need to be passed through an escaping function, so the + * WordPress.Security.EscapeOutput sniff reports a false positive here. + * + * @since 2.0.1 + * + * @param string $file Absolute path to the file. + * @param int $line Line number to inspect. + * @return bool True if the line contains a call to saveHtml() or saveXML(). + */ + private function line_calls_save_dom_method( $file, $line ) { + if ( $line <= 0 || ! is_string( $file ) || '' === $file || ! file_exists( $file ) ) { + return false; + } + + $contents = file_get_contents( $file ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents + if ( false === $contents ) { + return false; + } + + $lines = explode( "\n", $contents ); + if ( ! isset( $lines[ $line - 1 ] ) ) { + return false; + } + + $source = $lines[ $line - 1 ]; + + return ( false !== stripos( $source, 'saveHtml(' ) ) + || ( false !== stripos( $source, 'saveXML(' ) ); + } } diff --git a/tests/phpunit/testdata/plugins/test-plugin-late-escaping-errors/load.php b/tests/phpunit/testdata/plugins/test-plugin-late-escaping-errors/load.php index 6b2976c99..d5cbb1b4a 100644 --- a/tests/phpunit/testdata/plugins/test-plugin-late-escaping-errors/load.php +++ b/tests/phpunit/testdata/plugins/test-plugin-late-escaping-errors/load.php @@ -22,3 +22,7 @@ $test = '
Hello World!
'; echo $test; + +$dom = new DOMDocument(); +$dom->loadHTML( 'Hello World!
' ); +echo $dom->saveHtml(); diff --git a/tests/phpunit/testdata/plugins/test-plugin-prepared-sql-with-errors/load.php b/tests/phpunit/testdata/plugins/test-plugin-prepared-sql-with-errors/load.php new file mode 100644 index 000000000..a3c8e909c --- /dev/null +++ b/tests/phpunit/testdata/plugins/test-plugin-prepared-sql-with-errors/load.php @@ -0,0 +1,24 @@ +get_results( + $wpdb->prepare( $query, ...$args ) +); diff --git a/tests/phpunit/tests/Checker/Checks/Late_Escaping_Check_Tests.php b/tests/phpunit/tests/Checker/Checks/Late_Escaping_Check_Tests.php index 0d4ff07c0..648ea5e2c 100644 --- a/tests/phpunit/tests/Checker/Checks/Late_Escaping_Check_Tests.php +++ b/tests/phpunit/tests/Checker/Checks/Late_Escaping_Check_Tests.php @@ -18,8 +18,10 @@ public function test_run_with_errors() { $late_escape_check->run( $check_result ); - $errors = $check_result->get_errors(); + $errors = $check_result->get_errors(); + $warnings = $check_result->get_warnings(); + // The original echo $test should still be an error. $this->assertNotEmpty( $errors ); $this->assertArrayHasKey( 'load.php', $errors ); $this->assertEquals( 1, $check_result->get_error_count() ); @@ -29,6 +31,24 @@ public function test_run_with_errors() { $this->assertArrayHasKey( 6, $errors['load.php'][24] ); $this->assertArrayHasKey( 'code', $errors['load.php'][24][6][0] ); $this->assertEquals( 'WordPress.Security.EscapeOutput.OutputNotEscaped', $errors['load.php'][24][6][0]['code'] ); + + // Verify saveHtml() false positive is downgraded to a warning. + $this->assertNotEmpty( $warnings ); + $this->assertArrayHasKey( 'load.php', $warnings ); + + $found_save_html = false; + foreach ( $warnings['load.php'] as $line => $columns ) { + foreach ( $columns as $column => $messages ) { + foreach ( $messages as $message ) { + if ( 'WordPress.Security.EscapeOutput.OutputNotEscaped' === $message['code'] ) { + $found_save_html = true; + break 3; + } + } + } + } + + $this->assertTrue( $found_save_html, 'saveHtml() should be a warning, not an error.' ); } public function test_run_without_errors() { diff --git a/tests/phpunit/tests/Checker/Checks/Plugin_Review_PHPCS_Check_Tests.php b/tests/phpunit/tests/Checker/Checks/Plugin_Review_PHPCS_Check_Tests.php index 0e0779dba..d8f7950d9 100644 --- a/tests/phpunit/tests/Checker/Checks/Plugin_Review_PHPCS_Check_Tests.php +++ b/tests/phpunit/tests/Checker/Checks/Plugin_Review_PHPCS_Check_Tests.php @@ -161,4 +161,48 @@ public function test_run_without_errors() { $this->assertEquals( 0, $check_result->get_error_count() ); $this->assertEquals( 0, $check_result->get_warning_count() ); } + + /** + * Test for prepared SQL NotPrepared downgrade. + * + * Verifies WordPress.DB.PreparedSQL.NotPrepared is downgraded + * from error to warning, reducing false positives. + */ + public function test_run_with_prepared_sql_errors() { + $plugin_review_phpcs_check = new Plugin_Review_PHPCS_Check(); + $check_context = new Check_Context( UNIT_TESTS_PLUGIN_DIR . 'test-plugin-prepared-sql-with-errors/load.php' ); + $check_result = new Check_Result( $check_context ); + + $plugin_review_phpcs_check->run( $check_result ); + + $errors = $check_result->get_errors(); + $warnings = $check_result->get_warnings(); + + $this->assertNotEmpty( $warnings ); + $this->assertArrayHasKey( 'load.php', $warnings ); + + $found_not_prepared = false; + foreach ( $warnings['load.php'] as $line => $columns ) { + foreach ( $columns as $column => $messages ) { + foreach ( $messages as $message ) { + if ( 'WordPress.DB.PreparedSQL.NotPrepared' === $message['code'] ) { + $found_not_prepared = true; + break 3; + } + } + } + } + + $this->assertTrue( $found_not_prepared, 'WordPress.DB.PreparedSQL.NotPrepared should be a warning.' ); + + if ( isset( $errors['load.php'] ) ) { + foreach ( $errors['load.php'] as $line => $columns ) { + foreach ( $columns as $column => $messages ) { + foreach ( $messages as $message ) { + $this->assertNotEquals( 'WordPress.DB.PreparedSQL.NotPrepared', $message['code'] ); + } + } + } + } + } }