-
Notifications
You must be signed in to change notification settings - Fork 43
Security/EscapingVoidReturnFunctions: various improvements #861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
GaryJones
merged 6 commits into
develop
from
feature/security-escapingvoidreturnfunctions-sniff-review
Jul 28, 2025
Merged
Security/EscapingVoidReturnFunctions: various improvements #861
GaryJones
merged 6 commits into
develop
from
feature/security-escapingvoidreturnfunctions-sniff-review
Jul 28, 2025
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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.
GaryJones
approved these changes
Jul 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
PHPCSUtils
The addition and utilisation of PHPCSUtils package
Type: Bug
Type: Enhancement
Type: False positive
Type: Maintenance
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Security/EscapingVoidReturnFunctions: extend AbstractFunctionParameterSniff
As things were, the determination of whether or not a
T_STRINGis a call to any of the global WPesc_*()orwp_kses*()functions, was severely flawed.By switching the sniff over to be based on the WordPressCS
AbstractFunctionParameterSniffclass, this flaw is mitigated.Take note of the
$target_functionsusing 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:
usestatements are not flagged. We're not interested in those.This was previously handled correctly for the printing functions (via WPCS), but not for the escaping functions.
Security/EscapingVoidReturnFunctions: document support for
$customPrintingFunctionsAs the sniff uses the WPCS
WordPressCS\WordPress\Helpers\PrintingFunctionsTrait, apublic(user-settable)$customPrintingFunctionsproperty 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_STRINGtoken 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_*andwp_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:
$target_functionsproperty to be explicit about the functions the sniff is targetting.Notes:
esc_*functions with the exception ofesc_sql()which doesn't feel like it belongs in this list. Please let me know if you prefer thatesc_sql()is still included.wp_kses_*functions with the exception ofwp_kses_allowed_html()which is not an escaping function.esc_*/wp_kses*functions wrapping printing functions will no longer be flagged.$target_functionsproperty to contain information about the target parameter name and position.PassedParameters::getParameterFromStack()method.Includes additional unit tests.
Closes #539