Skip to content

Conversation

@jrfnl
Copy link
Collaborator

@jrfnl jrfnl commented Jul 26, 2025

Security/EscapingVoidReturnFunctions: extend AbstractFunctionParameterSniff

As things were, the determination of whether or not a T_STRING is a call to any of the global WP esc_*() or wp_kses*() functions, was severely flawed.

By switching the sniff over to be based on the WordPressCS AbstractFunctionParameterSniff class, this flaw is mitigated.

Take note of the $target_functions using wildcards to match all function calls starting with the prefixes indicates (as it was before).

Includes adding a slew of additional tests, some of which (line 8 - 15, line 52, 60, 63) are specific to the flaw being addressed in this commit.

Additionally, the tests have been made more comprehensive and varied by:

  • Testing against false positives for calls to methods or namespaced function calls (= the issue being addressed in this PR).
  • Ensure function import use statements are not flagged. We're not interested in those.
  • Safeguarding that function calls using PHP 5.6+ argument unpacking are not flagged.
  • Safeguarding handling of PHP 8.0+ attribute class using the same name as a target function.
  • Safeguarding that the function is not flagged when used as a PHP 8.1+ first class callable.
  • Adding tests with more variations:
    • Variation in the escaping functions being called.
    • Variation in the printing functions being called.
    • Non-lowercase function call(s), for both the escaping functions as well as the printing functions.
      This was previously handled correctly for the printing functions (via WPCS), but not for the escaping functions.
    • Fully qualified function calls for the escaping functions.
    • Use PHP 7.3+ trailing comma's in a few function calls.
    • Multi-line function call.
    • Comments in unexpected places.

Security/EscapingVoidReturnFunctions: document support for $customPrintingFunctions

As the sniff uses the WPCS WordPressCS\WordPress\Helpers\PrintingFunctionsTrait, a public (user-settable) $customPrintingFunctions property is automatically supported.

This commit adds tests documenting this.

Security/EscapingVoidReturnFunctions: bug fix - false negative with fully qualified printing functions

As things were, the sniff would not recognize nested printing functions if the printing function was a fully qualified function call.

Fixed now.

Safeguarded by updating some of the pre-existing tests.

Security/EscapingVoidReturnFunctions: bug fix - false negative for tag_escape()

The tag_escape() function is also one of the WP native escaping functions, but was not examined by this sniff.

Fixed now.

Includes test.

Ref:

Security/EscapingVoidReturnFunctions: bug fix - false positive for printing, not functions

As things were, the sniff would presume that if the first non-empty token within the escaping function call was a T_STRING, it would be a function call.

As the T_STRING token is used for quite a few things, this would lead to false positives.

Fixed now by making sure there is an open parenthesis after the T_STRING.

Includes test.

Ref:

Security/EscapingVoidReturnFunctions: add support for PHP 8.0+ named parameters

To allow support for named parameters, we need to know the position and name of the parameter to examine within a function call.

Previously, this sniff used "wildcard" matching for esc_* and wp_kses* functions.

While the parameter position is the same for all of these, the parameter name is not, so we can no longer use wildcard matching if we want the sniff to support PHP 8.0+ function calls using named parameters.

To this end:

  1. Change the $target_functions property to be explicit about the functions the sniff is targetting.
    Notes:
    • I've included all WP native esc_* functions with the exception of esc_sql() which doesn't feel like it belongs in this list. Please let me know if you prefer that esc_sql() is still included.
    • I've included all WP native wp_kses_* functions with the exception of wp_kses_allowed_html() which is not an escaping function.
    • Also take note that this also means that custom/user defined esc_*/wp_kses* functions wrapping printing functions will no longer be flagged.
  2. Changed the $target_functions property to contain information about the target parameter name and position.
  3. Adjusted the logic in the sniff to allow for named parameters using the new PHPCSUtils 1.0.0-alpha4 PassedParameters::getParameterFromStack() method.

Includes additional unit tests.

Closes #539

jrfnl added 6 commits July 27, 2025 01:48
…rSniff

As things were, the determination of whether or not a `T_STRING` is a call to any of the global WP `esc_*()` or `wp_kses*()`  functions, was severely flawed.

By switching the sniff over to be based on the WordPressCS `AbstractFunctionParameterSniff` class, this flaw is mitigated.

Take note of the `$target_functions` using wildcards to match all function calls starting with the prefixes indicates (as it was before).

Includes adding a slew of additional tests, some of which (line 8 - 15, line 52, 60, 63) are specific to the flaw being addressed in this commit.

Additionally, the tests have been made more comprehensive and varied by:
* Testing against false positives for calls to methods or namespaced function calls (= the issue being addressed in this PR).
* Ensure function import `use` statements are not flagged. We're not interested in those.
* Safeguarding that function calls using PHP 5.6+ argument unpacking are not flagged.
* Safeguarding handling of PHP 8.0+ attribute class using the same name as a target function.
* Safeguarding that the function is not flagged when used as a PHP 8.1+ first class callable.
* Adding tests with more variations:
    - Variation in the escaping functions being called.
    - Variation in the printing functions being called.
    - Non-lowercase function call(s), for both the escaping functions as well as the printing functions.
        This was previously handled correctly for the printing functions (via WPCS), but not for the escaping functions.
    - Fully qualified function calls for the escaping functions.
    - Use PHP 7.3+ trailing comma's in a few function calls.
    - Multi-line function call.
    - Comments in unexpected places.
…intingFunctions`

As the sniff uses the WPCS `WordPressCS\WordPress\Helpers\PrintingFunctionsTrait`, a `public` (user-settable) `$customPrintingFunctions` property is automatically supported.

This commit adds tests documenting this.
…ully qualified printing functions

As things were, the sniff would not recognize nested printing functions if the printing function was a fully qualified function call.

Fixed now.

Safeguarded by updating some of the pre-existing tests.
…ag_escape()`

The `tag_escape()` function is also one of the WP native escaping functions, but was not examined by this sniff.

Fixed now.

Includes test.

Ref:
* https://developer.wordpress.org/reference/functions/tag_escape/
…inting, not functions

As things were, the sniff would presume that if the first non-empty token within the escaping function call was a `T_STRING`, it would be a function call.

As the `T_STRING` token is used for quite a few things, this would lead to false positives.

Fixed now by making sure there is an open parenthesis after the `T_STRING`.

Includes test.

Ref:
* https://developer.wordpress.org/reference/functions/tag_escape/
…parameters

To allow support for named parameters, we need to know the position and name of the parameter to examine within a function call.

Previously, this sniff used "wildcard" matching for `esc_*` and `wp_kses*` functions.

While the parameter position is the same for all of these, the parameter name is not, so we can no longer use wildcard matching if we want the sniff to support PHP 8.0+ function calls using named parameters.

To this end:

1. Change the `$target_functions` property to be explicit about the functions the sniff is targetting.
    Notes:
    - I've included all WP native `esc_*` functions with the exception of `esc_sql()` which doesn't feel like it belongs in this list. Please let me know if you prefer that `esc_sql()` is still included.
    - I've included all WP native `wp_kses_*` functions with the exception of `wp_kses_allowed_html()` which is not an escaping function.
    - Also take note that this also means that custom/user defined `esc_*`/`wp_kses*` functions wrapping printing functions will no longer be flagged.
2. Changed the `$target_functions` property to contain information about the target parameter name and position.
3. Adjusted the logic in the sniff to allow for named parameters using the new PHPCSUtils 1.0.0-alpha4 `PassedParameters::getParameterFromStack()` method.

Includes additional unit tests.
@jrfnl jrfnl added this to the 3.1.0 milestone Jul 26, 2025
@jrfnl jrfnl requested a review from a team as a code owner July 26, 2025 23:53
@GaryJones GaryJones merged commit 31f6da9 into develop Jul 28, 2025
42 checks passed
@GaryJones GaryJones deleted the feature/security-escapingvoidreturnfunctions-sniff-review branch July 28, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review the WordPressVIPMinimum.Security.EscapingVoidReturnFunctions sniff

3 participants