Security/EscapeOutput: fix false positive and add tests for namespaced names#2618
Conversation
|
Fixed and force-pushed. The problem was related to a change in how PHPCS tokenizes FQN exit/die. I added more details in a code comment. |
rodrigoprimo
force-pushed
the
escape-output-namespaced-names-tests
branch
from
bcab6ad to
490c471
Compare
|
Moved to draft pending the review of #2620 |
rodrigoprimo
force-pushed
the
escape-output-namespaced-names-tests
branch
from
490c471 to
0d6763d
Compare
…attern Add tests to ensure the `basename( __FILE__ )` pattern recognition in `_deprecated_file()` only applies to global `basename()` function calls, not to other constructs that might look similar.
rodrigoprimo
force-pushed
the
escape-output-namespaced-names-tests
branch
from
0d6763d to
322829b
Compare
|
@jrfnl, I'm moving this PR back to ready for review. |
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
@rodrigoprimo Very good effort, needs a little more work, but these tests will help a lot!
| echo \get_search_query( true ); // Ok. | ||
| echo \get_search_query( false ); // Bad. | ||
| echo MyNamespace\get_search_query( true ); // Bad. | ||
| echo \MyNamespace\get_search_query( true ); // Bad. | ||
| echo namespace\get_search_query( true ); // Bad. The sniff should stop flagging this once it can resolve relative namespaces. | ||
| echo namespace\Sub\get_search_query( true ); // Bad. |
There was a problem hiding this comment.
Should there be tests here with FQN true/false and case-variations ? I.e. \TRUE and \False ?
There was a problem hiding this comment.
This uncovered a bug in how the sniff was handling FQN and non-standard case true. I added the tests and the fix in a separate commit from the rest of the changes. I also updated the PR description and title to mention this fix and added a changelog entry.
rodrigoprimo
force-pushed
the
escape-output-namespaced-names-tests
branch
from
7190fe2 to
d56798e
Compare
Co-authored-by: Juliette <663378+jrfnl@users.noreply.github.com>
… FQN/mixed-case true The sniff was incorrectly flagging `get_search_query()` calls as unsafe when the `$escaped` parameter was passed as fully qualified true or as true using a non-standard case. The comparison `'true' !== $escaped_param['clean']` failed because the parameter value wasn't normalized. This commit fixes this by stripping any leading backslash and converting to lowercase before comparison.
rodrigoprimo
force-pushed
the
escape-output-namespaced-names-tests
branch
from
fb7cbdd to
0638f0b
Compare
rodrigoprimo
changed the title
|
Thanks for your review, @jrfnl! I responded to all of your comments and implemented your suggestions. There is just one item pending for us to discuss regarding the fully qualified tests for functions like |
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
Thanks for all your work on this @rodrigoprimo !!!!
3 participants
Description
In preparation for PHPCS 4.0, which changes the tokenization of namespaced names, this PR adds tests with all variants of namespace names (unqualified, partially qualified, fully qualified, and namespace relative) to the
WordPress.Security.EscapeOutputsniff. This is a continuation of #2581, where tests were added for all sniffs extendingAbstractFunctionRestrictionsexceptEscapeOutput.This PR also includes tests to ensure the
basename( __FILE__ )pattern recognition in_deprecated_file()only applies to globalbasename()function calls, not to other constructs that might look similar (such as namespaced variants or class methods).Besides that, it fixes in a separate commit a false positive bug that was discovered during the PR review, where the sniff would incorrectly handle FQN and/or non-standard case
truepassed toget_search_query().Suggested changelog entry
Fixed:
WordPress.Security.EscapeOutput: false positive forget_search_query()when the$escapedparameter was passed as fully qualifiedtrueor astrueusing a non-standard case.