Security/EscapeOutput: fix false positive and add tests for namespaced names #2618

rodrigoprimo · 2025-09-18T15:35:05Z

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.EscapeOutput sniff. This is a continuation of #2581, where tests were added for all sniffs extending AbstractFunctionRestrictions except EscapeOutput.

This PR also includes 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 (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 true passed to get_search_query().

Suggested changelog entry

Fixed:

WordPress.Security.EscapeOutput: false positive for get_search_query() when the $escaped parameter was passed as fully qualified true or as true using a non-standard case.

rodrigoprimo · 2025-09-18T15:38:33Z

~~I'm investigating the test that is failing in the PHPCS lowest build.~~

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 · 2025-11-11T16:50:17Z

Moved to draft pending the review of #2620

…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 · 2025-11-24T16:04:52Z

@jrfnl, I'm moving this PR back to ready for review.

jrfnl

@rodrigoprimo Very good effort, needs a little more work, but these tests will help a lot!

WordPress/Tests/Security/EscapeOutputUnitTest.1.inc

jrfnl · 2025-12-09T15:55:35Z

WordPress/Tests/Security/EscapeOutputUnitTest.1.inc

+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.


Should there be tests here with FQN true/false and case-variations ? I.e. \TRUE and \False ?

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.

jrfnl · 2025-12-09T15:58:31Z

WordPress/Tests/Security/EscapeOutputUnitTest.php

+					// PHPCS 3.13.3 changed the tokenization of FQN exit/die it impacts directly how this test case
+					// behaves (see https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/1201).
+					664 => version_compare( $phpcs_version, '3.13.3', '>=' ) ? 1 : 0,


I seem to remember we dropped support for PHPCS < 3.13.4 ? In which case, this toggle should no longer be needed.

jrfnl · 2025-12-09T16:01:05Z

WordPress/Tests/Security/EscapeOutputUnitTest.1.inc

 <?php

-echo PHP_VERSION_ID, PHP_VERSION, PHP_EOL, PHP_EXTRA_VERSION; // OK.
+echo PHP_VERSION_ID, PHP_VERSION, \PHP_EOL, PHP_EXTRA_VERSION; // OK.


Should there be tests with these "safe constants" in namespaced variants ?

WordPress/Tests/Security/EscapeOutputUnitTest.1.inc

jrfnl · 2025-12-09T16:07:43Z

WordPress/Tests/Security/EscapeOutputUnitTest.1.inc

Would it make sense to also have some tests like the below ? (mind: I've not checked whether the function names in the second line (short echo) are the correct WP function names, this will need verification)

?> <a href="<?php \the_permalink(); ?>" title="<?php \the_title_attribute(); ?>"><?php \the_title(); ?></a> <a href="<?= \get_permalink(); ?>" title="<?= \get_title_attribute(); ?>"><?= \get_the_title(); ?></a> <?php

And how about this ?

echo \true, \False, \NULL;

I know, it's kind of silly code, but the tokens are in the "safe tokens" list and with the fully qualified bit, the behaviour may change ?

I also see a separate category of array walking functions being handled. Do these need tests with the namespaced name variations too ?

<a href="<?php \the_permalink(); ?>" title="<?php \the_title_attribute(); ?>"><?php \the_title(); ?></a>

@jrfnl, correct me if I'm missing something, but I'm not sure what the value of adding the test above would be. The sniff bails early when it encounters any of those three functions, as they are not in the list of functions that this sniff looks for. the_title_attribute() is an auto-escaping function, and the_permalink() and the_title() used to be (removed in #1547). But the code above does not trigger the check for auto-escaping functions, and there are already tests for this group of functions. Maybe the original test on line 9 needs to be removed or updated? Or am I missing its purpose?

Regarding the second line you suggested, I made a minor modification, and I believe it makes sense to add it to test multiple short open tags and fully qualified functions.

<a href="<?= \get_permalink(); ?>" title="<?= \the_title_attribute( array( 'echo' => false ) ); ?>"><?= \get_the_title(); ?></a>

Co-authored-by: Juliette <[email protected]>

… 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 · 2025-12-10T14:18:12Z

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 \the_permalink(). Please take a look at my remark above (#2618 (comment)).

rodrigoprimo force-pushed the escape-output-namespaced-names-tests branch from bcab6ad to 490c471 Compare September 18, 2025 15:59

rodrigoprimo added Component: Extra Type: Chores/Cleanup Status: Review ready labels Sep 18, 2025

rodrigoprimo removed the Status: Review ready label Nov 11, 2025

rodrigoprimo marked this pull request as draft November 11, 2025 16:50

Security/EscapeOutput: add tests for namespaced names

27701fd

rodrigoprimo force-pushed the escape-output-namespaced-names-tests branch from 490c471 to 0d6763d Compare November 14, 2025 14:32

Security/EscapeOutput: add edge case tests for basename( __FILE__ ) p…

322829b

…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 November 14, 2025 14:43

rodrigoprimo marked this pull request as ready for review November 24, 2025 16:04

rodrigoprimo added the Status: Review ready label Nov 24, 2025

rodrigoprimo mentioned this pull request Nov 29, 2025

AbstractFunctionRestrictionsSniff: improve namespaced names tests #2663

Open

jrfnl reviewed Dec 9, 2025

View reviewed changes

jrfnl added Status: Awaiting feedback and removed Status: Review ready labels Dec 9, 2025

rodrigoprimo force-pushed the escape-output-namespaced-names-tests branch from 7190fe2 to d56798e Compare December 9, 2025 17:45

rodrigoprimo and others added 2 commits December 10, 2025 10:57

Apply changes from code review

6b7601d

Co-authored-by: Juliette <[email protected]>

rodrigoprimo force-pushed the escape-output-namespaced-names-tests branch from fb7cbdd to 0638f0b Compare December 10, 2025 14:07

rodrigoprimo changed the title ~~Security/EscapeOutput: add tests for namespaced names~~ Security/EscapeOutput: fix false positive and add tests for namespaced names Dec 10, 2025

rodrigoprimo added Status: Review ready and removed Status: Awaiting feedback labels Dec 10, 2025

Uh oh!

Security/EscapeOutput: fix false positive and add tests for namespaced names #2618

Are you sure you want to change the base?

Security/EscapeOutput: fix false positive and add tests for namespaced names #2618

Conversation

rodrigoprimo commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suggested changelog entry

Uh oh!

rodrigoprimo commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodrigoprimo commented Nov 11, 2025

Uh oh!

rodrigoprimo commented Nov 24, 2025

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigoprimo commented Dec 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rodrigoprimo commented Sep 18, 2025 •

edited

Loading

rodrigoprimo commented Sep 18, 2025 •

edited

Loading