PHPCS Issues in ActionScheduler_wcSystemStatus.php

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/classes/ActionScheduler_wcSystemStatus.php
------------------------------------------------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
------------------------------------------------------------------------------------------------------------------------------------------
 101 | ERROR | All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks),
     |       | found 'wc_help_tip'. (WordPress.Security.EscapeOutput.OutputNotEscaped)
 122 | ERROR | All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks),
     |       | found 'number_format_i18n'. (WordPress.Security.EscapeOutput.OutputNotEscaped)
 123 | ERROR | All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks),
     |       | found '$oldest_and_newest'. (WordPress.Security.EscapeOutput.OutputNotEscaped)
 124 | ERROR | All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks),
     |       | found '$oldest_and_newest'. (WordPress.Security.EscapeOutput.OutputNotEscaped)
------------------------------------------------------------------------------------------------------------------------------------------

barryhughes commented 3 years ago

Hello again @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).

Running through those specific issues:

Line 101 → We can't actually wrap wc_help_tip() within an escaping function, because the function itself returns HTML. Similar to a previous issue, though, we could certainly add a // phpcs:ignore <rule> style comment to reassure PHPCS.
Line 122 → Agreed! We could wrap this in a suitable escaping function such as esc_html().
Line 123 → Agreed! We could wrap this in a suitable escaping function such as esc_html().
Line 124 → Agreed! We could wrap this in a suitable escaping function such as esc_html().

If you are interested in submitting a pull request we'd definitely appreciate it, or else we'll aim to circle back on our side and fix these up.

ovidiul commented 3 years ago

Opened a PR here #761 to address these issues.

barryhughes commented 2 years ago

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

woocommerce / action-scheduler

PHPCS Issues in ActionScheduler_wcSystemStatus.php #665