PHPCS Issues in ActionScheduler_Abstract_ListTable.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/abstracts/ActionScheduler_Abstract_ListTable.php
------------------------------------------------------------------------------------------------------------------------------------------
FOUND 48 ERRORS AND 30 WARNINGS AFFECTING 30 LINES
------------------------------------------------------------------------------------------------------------------------------------------
 151 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 151 | ERROR   | Detected usage of a non-sanitized input variable: $_GET
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 152 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 152 | ERROR   | Detected usage of a non-sanitized input variable: $_GET
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 152 | ERROR   | Use placeholders and $wpdb->prepare(); found $ids_sql (WordPress.DB.PreparedSQL.NotPrepared)
 152 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 152 | ERROR   | Detected usage of a non-sanitized input variable: $_GET
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 155 | WARNING | wp_redirect() found. Using wp_safe_redirect(), along with the allowed_redirect_hosts filter if needed, can help avoid
     |         | any chances of malicious redirects within code. It is also important to remember to call exit() after a redirect so that
     |         | no other unwanted code is executed. (WordPress.Security.SafeRedirect.wp_redirect_wp_redirect)
 157 | ERROR   | Detected usage of a non-validated input variable: $_SERVER
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotValidated)
 157 | ERROR   | Detected usage of a non-sanitized input variable: $_SERVER
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 279 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 279 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 279 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 280 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 295 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 295 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 295 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 295 | ERROR   | Detected usage of a non-sanitized input variable: $_GET
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 310 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 310 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 310 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 310 | ERROR   | Detected usage of a non-sanitized input variable: $_GET
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 320 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 320 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 320 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 320 | ERROR   | Detected usage of a non-sanitized input variable: $_GET
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 354 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 360 | ERROR   | Use placeholders and $wpdb->prepare(); found $column (WordPress.DB.PreparedSQL.NotPrepared)
 360 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 360 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 360 | ERROR   | Detected usage of a non-sanitized input variable: $_GET
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 372 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 372 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 379 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 379 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 383 | ERROR   | Use placeholders and $wpdb->prepare(); found interpolated variable $column at "`$column` = %s"
     |         | (WordPress.DB.PreparedSQL.NotPrepared)
 383 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 383 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 383 | ERROR   | Detected usage of a non-sanitized input variable: $_GET
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 406 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 408 | WARNING | wp_redirect() found. Using wp_safe_redirect(), along with the allowed_redirect_hosts filter if needed, can help avoid
     |         | any chances of malicious redirects within code. It is also important to remember to call exit() after a redirect so that
     |         | no other unwanted code is executed. (WordPress.Security.SafeRedirect.wp_redirect_wp_redirect)
 408 | ERROR   | Detected usage of a non-validated input variable: $_SERVER
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotValidated)
 408 | ERROR   | Detected usage of a non-sanitized input variable: $_SERVER
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 431 | ERROR   | Use placeholders and $wpdb->prepare(); found $sql (WordPress.DB.PreparedSQL.NotPrepared)
 434 | ERROR   | Use placeholders and $wpdb->prepare(); found $query_count (WordPress.DB.PreparedSQL.NotPrepared)
 451 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 451 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 451 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 451 | ERROR   | Detected usage of a non-sanitized input variable: $_GET
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 533 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 538 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 538 | ERROR   | Detected usage of a non-validated input variable: $_REQUEST
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotValidated)
 538 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 538 | ERROR   | Detected usage of a non-sanitized input variable: $_REQUEST
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 540 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 540 | ERROR   | Detected usage of a non-validated input variable: $_REQUEST
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotValidated)
 540 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 540 | ERROR   | Detected usage of a non-validated input variable: $_REQUEST
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotValidated)
 540 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 540 | ERROR   | Detected usage of a non-sanitized input variable: $_REQUEST
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 540 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 540 | ERROR   | Detected usage of a non-validated input variable: $_REQUEST
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotValidated)
 540 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 540 | ERROR   | Detected usage of a non-sanitized input variable: $_REQUEST
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 541 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 541 | ERROR   | Detected usage of a non-validated input variable: $_REQUEST
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotValidated)
 541 | ERROR   | Missing wp_unslash() before sanitization. (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
 541 | ERROR   | Detected usage of a non-sanitized input variable: $_REQUEST
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 544 | WARNING | wp_redirect() found. Using wp_safe_redirect(), along with the allowed_redirect_hosts filter if needed, can help avoid
     |         | any chances of malicious redirects within code. It is also important to remember to call exit() after a redirect so that
     |         | no other unwanted code is executed. (WordPress.Security.SafeRedirect.wp_redirect_wp_redirect)
 546 | ERROR   | Detected usage of a non-validated input variable: $_SERVER
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotValidated)
 546 | ERROR   | Detected usage of a non-sanitized input variable: $_SERVER
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
 577 | ERROR   | All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks),
     |         | found '$notice'. (WordPress.Security.EscapeOutput.OutputNotEscaped)
 615 | ERROR   | All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks),
     |         | found '$status_list_items'. (WordPress.Security.EscapeOutput.OutputNotEscaped)
 627 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 647 | WARNING | Processing form data without nonce verification. (WordPress.Security.NonceVerification.NoNonceVerification)
 649 | WARNING | wp_redirect() found. Using wp_safe_redirect(), along with the allowed_redirect_hosts filter if needed, can help avoid
     |         | any chances of malicious redirects within code. It is also important to remember to call exit() after a redirect so that
     |         | no other unwanted code is executed. (WordPress.Security.SafeRedirect.wp_redirect_wp_redirect)
 649 | ERROR   | Detected usage of a non-validated input variable: $_SERVER
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotValidated)
 649 | ERROR   | Detected usage of a non-sanitized input variable: $_SERVER
     |         | (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
------------------------------------------------------------------------------------------------------------------------------------------

[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).

So there a lot of warnings in this file and they fall into a few categories.

• Missing wp_unslash() → These appear to be valid findings.
• Use placeholders and $wpdb->prepare() → These appear to be false positives, but we could nonetheless make a change and add a // phpcs:ignore <rule> comment as needed to suppress the warnings.
• wp_redirect() found. Using wp_safe_redirect() → Agreed! This suggestion makes sense.
• Detected usage of a non-sanitized input variable → Needs review; possibly if we switch to using wp_safe_redirect() then we'll be covered from a safety and security perspective (if so, we should still add a // phpcs:ignore <rule> comment and, if not, we ought to pass through a suitable sanitizing function).
• Processing form data without nonce verification → These also appear to me to be false positives however, if so, we would still need to suppress the warnings.

There's a lot in this set of warnings, and if you feel confident addressing some or all we'll certainly review any PR you submit (or, alternatively, we'll circle back and tidy things up from our side).

Thanks again! :smile_cat:

[ovidiul]

Opened an extensive PR here https://github.com/woocommerce/action-scheduler/pull/763 to address these