Skip to content

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

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.

Sign up for GitHub

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

Open
rodrigoprimo wants to merge 4 commits into
base: develop
Choose a base branch
from
+171 −3
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
27701fd
Security/EscapeOutput: add tests for namespaced names
rodrigoprimo Aug 21, 2025
322829b
Security/EscapeOutput: add edge case tests for basename( __FILE__ ) p…
rodrigoprimo Nov 14, 2025
6b7601d
Apply changes from code review
rodrigoprimo Dec 9, 2025
0638f0b
Security/EscapeOutput: fix false positive for get_search_query() with…
rodrigoprimo Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 67 additions & 2 deletions WordPress/Tests/Security/EscapeOutputUnitTest.1.inc
View file
Open in desktop
Copy link
Member

@jrfnl jrfnl Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jrfnl jrfnl Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

@jrfnl jrfnl Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@rodrigoprimo rodrigoprimo Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

<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><!-- Bad x 2. -->

Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ echo esc_html_x( $some_nasty_var, 'context' ); // Ok.
<input type="hidden" name="some-action" value="<?php echo esc_attr_x( 'none', 'context' ); ?>" /><!-- OK. -->
<?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.
Copy link
Member

@jrfnl jrfnl Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


trigger_error( 'DEBUG INFO - ' . __METHOD__ . '::internal_domains: domain = ' . $domain ); // Bad.
Trigger_ERROR( $domain ); // Bad.
Expand Down Expand Up @@ -661,7 +661,7 @@ exit( status: esc_html( $foo ) ); // Ok.
die( status: esc_html( $foo ) ); // Ok.

exit( status: $foo ); // Bad.
die( status: $foo ); // Bad.
\die( status: $foo ); // Bad.

/*
* Issue https://github.com/WordPress/WordPress-Coding-Standards/issues/2552
Expand All @@ -687,3 +687,68 @@ _deprecated_function( __METHOD__, 'x.x.x', \ClassName::class ); // OK.
die( \MyNamespace\ClassName::class . ' has been abandoned' ); // OK.
echo 'Do not use ' . MyNamespace\ClassName::class; // OK.
_deprecated_function( __METHOD__, 'x.x.x', namespace\ClassName::class ); // OK.

/*
* Safeguard correct handling of all types of namespaced escaping and printing function calls.
*/
\printf( 'Hello %s', $foo ); // Bad.
MyNamespace\wp_die( $message ); // Ok.
\MyNamespace\vprintf( 'Hello %s', array( $foo ) ); // Ok.
namespace\wp_dropdown_pages( $args ); // Ok. The sniff should start flagging this once it can resolve relative namespaces.
namespace\Sub\_deprecated_function( __FUNCTION__, '1.3.0', $another_func ); // Ok.
\printf( 'Hello %s', \esc_html( $foo ) ); // Ok.
\wp_die( MyNamespace\number_format( $foo ) ); // Bad.
\vprintf( 'Hello %s', array( \MyNamespace\sanitize_user_field( $foo ) ) ); // Bad.
\wp_dropdown_pages( namespace\sanitize_key( $foo ) ); // Bad. The sniff should stop flagging this once it can resolve relative namespaces.
\_deprecated_function( __FUNCTION__, '1.3.0', namespace\Sub\wp_kses( $another_func ) ); // Bad.

/*
* Safeguard correct handling of namespaced auto-escaped functions.
*/
echo \bloginfo( $var ); // Ok.
echo MyNamespace\count( $var ); // Bad.
echo \MyNamespace\get_archives_link( $url, 'link' ); // Bad.
echo namespace\get_search_form(); // Bad. The sniff should stop flagging this once it can resolve relative namespaces.
echo namespace\Sub\the_author(); // Bad.

/*
* Safeguard correct handling of namespaced unsafe printing functions.
*/
\_e( $text, 'my-domain' ); // Bad.
MyNamespace\_ex( $text, 'context' ); // Ok.
\MyNamespace\_e( $text, 'my-domain' ); // Ok.
namespace\_ex( $text, 'context' ); // Ok. The sniff should start flagging this once it can resolve relative namespaces.
namespace\Sub\_e( $text, 'my-domain' ); // Ok.

/*
* Safeguard correct handling of namespaced formatting functions.
*/
echo \sprintf( '%s', $var ); // Bad.
echo \sprintf( '%s', esc_html( $var ) ); // Ok.
echo MyNamespace\antispambot( esc_html( $email ) ); // Bad.
echo \MyNamespace\ent2ncr( esc_html( $_data ) ); // Bad.
echo namespace\vsprintf( 'Hello %s', array( esc_html( $foo ) ) ); // Bad. The sniff should stop flagging this once it can resolve relative namespaces.
echo namespace\Sub\wp_sprintf( 'Hello %s', array( esc_html( $foo ) ) ); // Bad.

/*
* Safeguard correct handling of get_search_query() as the sniff has special logic to check the $escaped parameter.
*/
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.
Comment on lines +736 to +741
Copy link
Member

@jrfnl jrfnl Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@rodrigoprimo rodrigoprimo Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/*
* Safeguard correct handling of fully qualified and namespace relative functions with special parameter handling.
*/
\trigger_error( esc_html( $message ), $second_param_should_be_ignored ); // Ok.
\User_Error( $message ); // Bad.
namespace\trigger_error( $message ); // Ok. The sniff should start flagging this once it can resolve relative namespaces.
namespace\Sub\user_error( $message ); // Ok.
\_deprecated_file( basename( __FILE__ ), '1.3.0' ); // Ok.
\_deprecated_file( $file, '1.3.0' ); // Error.
namespace\_deprecated_file( basename( __FILE__ ), '1.3.0' ); // Ok.
namespace\_DEPRECATED_FILE( $file, '1.3.0' ); // Ok. The sniff should start flagging this once it can resolve relative namespaces.
namespace\Sub\_deprecated_file( $file, '1.3.0' ); // Ok.
29 changes: 28 additions & 1 deletion WordPress/Tests/Security/EscapeOutputUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
namespace WordPressCS\WordPress\Tests\Security;

use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
use PHPCSUtils\BackCompat\Helper;

/**
* Unit test class for the EscapeOutput sniff.
Expand Down Expand Up @@ -37,6 +38,8 @@ final class EscapeOutputUnitTest extends AbstractSniffUnitTest {
public function getErrorList( $testFile = '' ) {
switch ( $testFile ) {
case 'EscapeOutputUnitTest.1.inc':
$phpcs_version = Helper::getVersion();

return array(
17 => 1,
19 => 1,
Expand Down Expand Up @@ -160,10 +163,34 @@ public function getErrorList( $testFile = '' ) {
655 => 1,
657 => 1,
663 => 1,
664 => 1,
// 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,
Copy link
Member

@jrfnl jrfnl Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

672 => 1,
673 => 1,
678 => 1,
694 => 1,
700 => 1,
701 => 1,
702 => 1,
703 => 1,
709 => 1,
710 => 1,
711 => 1,
712 => 1,
717 => 1,
726 => 1,
728 => 1,
729 => 1,
730 => 1,
731 => 1,
737 => 1,
738 => 1,
739 => 1,
740 => 1,
741 => 1,
747 => 1,
751 => 1,
);

case 'EscapeOutputUnitTest.6.inc':
Expand Down