Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
}
}
44 changes: 43 additions & 1 deletion includes/Checker/Checks/Security/Late_Escaping_Check.php
Original file line number Diff line number Diff line change
Expand Up @@ -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':
Expand All @@ -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(' ) );
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,7 @@
$test = '<p><strong>Hello World!</strong></p>';

echo $test;

$dom = new DOMDocument();
$dom->loadHTML( '<p><strong>Hello World!</strong></p>' );
echo $dom->saveHtml();
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php
/**
* Plugin Name: Test Plugin DB Prepared Query with Errors
* Plugin URI: https://github.com/WordPress/plugin-check
* Requires at least: 6.0
* Requires PHP: 5.6
* Version: 1.0.0 Beta
* Author: WordPress Performance Team
* Author URI: https://make.wordpress.org/performance/
* License: GPLv2 or later
* License URI: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
* Text Domain: test-plugin-check-prepared-sql
*
* @package test-plugin-check-prepared-sql
*/

global $wpdb;

$query = 'SELECT * FROM wp_posts WHERE post_status = %s';
$args = array( 'publish' );

$results = $wpdb->get_results(
$wpdb->prepare( $query, ...$args )
);
22 changes: 21 additions & 1 deletion tests/phpunit/tests/Checker/Checks/Late_Escaping_Check_Tests.php
Original file line number Diff line number Diff line change
Expand Up @@ -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() );
Expand All @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'] );
}
}
}
}
}
}
Loading