search

woocommerce / action-scheduler

A scalable, traceable job queue for background processing large queues of tasks in WordPress. Specifically designed for distribution in WordPress plugins (and themes) - no server access required.
https://actionscheduler.org
GNU General Public License v3.0
627 stars 113 forks source link

PHPCS Issues in ActionScheduler_Schedule_Deprecated.php #666

Closed amberhinds closed 2 years ago

amberhinds commented 3 years ago

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/deprecated/ActionScheduler_Schedule_Deprecated.php
------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------
 25 | ERROR | All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks),
    |       | found '$replacement_method'. (WordPress.Security.EscapeOutput.OutputNotEscaped)
------------------------------------------------------------------------------------------------------------------------------------------
barryhughes commented 3 years ago

Hello @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 this case specifically, I think everything is fine: $replacement_method can only be one of two string literals (defined a few lines up) and so it's safe to use without escaping. That said, we could probably add a comment instructing PHPCS to ignore this (and so reduce noice when running the linter):

// phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped

If you're up for submitting a PR please feel free or else we'll do our best to make this change ourselves in due course :-)

ovidiul commented 3 years ago

Adding a proposed PR here https://github.com/woocommerce/action-scheduler/pull/762

barryhughes commented 2 years ago

https://github.com/woocommerce/action-scheduler/pull/762 is merged, closing.