PHPCS Issues in ActionScheduler_wpPostStore.php

[amberhinds]

Hello!

We've included Action Scheduler in one of our plugins (thank you!) and while doing security audits on our plugin, identified problems related to Action Scheduler.

We ran the plugin through WP Engine's linting test which helps identify best practices and potential problems. For this process, we are using PHP Codesniffer with rules derived from both the WordPress Coding Standards and PHPCompatibility rulesets. Below is a detailed line-by-line report of the sniff violation.

Can you please let me know if these are actual errors that require fixes? If so, we may be able to submit a pull request with fixes.

FILE: /includes/action-scheduler/classes/data-stores/ActionScheduler_wpPostStore.php
------------------------------------------------------------------------------------------------------------------------------------------
FOUND 22 ERRORS AFFECTING 19 LINES
------------------------------------------------------------------------------------------------------------------------------------------
 266 | ERROR | Use placeholders and $wpdb->prepare(); found $query (WordPress.DB.PreparedSQL.NotPrepared)
 268 | ERROR | Use placeholders and $wpdb->prepare(); found $query (WordPress.DB.PreparedSQL.NotPrepared)
 405 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 419 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 419 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 529 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 531 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 605 | ERROR | Use placeholders and $wpdb->prepare(); found interpolated variable $update at "{$update} {$where} {$order}"
     |       | (WordPress.DB.PreparedSQL.NotPrepared)
 605 | ERROR | Use placeholders and $wpdb->prepare(); found interpolated variable $where at "{$update} {$where} {$order}"
     |       | (WordPress.DB.PreparedSQL.NotPrepared)
 605 | ERROR | Use placeholders and $wpdb->prepare(); found interpolated variable $order at "{$update} {$where} {$order}"
     |       | (WordPress.DB.PreparedSQL.NotPrepared)
 671 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 672 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 685 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 686 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 700 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 701 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 712 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 713 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 749 | ERROR | Use placeholders and $wpdb->prepare(); found interpolated variable $column_name at "SELECT {$column_name} FROM
     |       | {$wpdb->posts} WHERE ID=%d AND post_type=%s" (WordPress.DB.PreparedSQL.NotPrepared)
 760 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 761 | ERROR | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 841 | ERROR | All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks),
     |       | found '$message'. (WordPress.Security.EscapeOutput.OutputNotEscaped)
------------------------------------------------------------------------------------------------------------------------------------------

[barryhughes]

HI @amberhinds—thanks for reporting this!

:memo: As a friendly note for the future, I'd ask first of all that you report any other security issues via HackerOne. This is our preferred way to handle possible security issues (and I'll make a note to update our readme file to make this clearer, as I don't think we currently cover this).

In most of these cases we are in fact using $wpdb->prepare() but we could still make some changes to reduce noise. For instance, we could add an appropriate // phpcs:ignore <rule> comment in the relevant places, and/or we could possibly restructure the code in some of those spots.

With regards to the error found on line 841 specifically, I agree that we could make use of the esc_html() function to add safety. If you're happy to submit a pull request for this that would be great, or else we can aim to circle back ourselves.

[ovidiul]

PR opened here https://github.com/woocommerce/action-scheduler/pull/780 to address the scan response.

[barryhughes]

Closing: https://github.com/woocommerce/action-scheduler/pull/780 has been merged (and should ship in 3.4.0).