Skip to content

Commit f48a181

Browse files
dingo-ddingo-d
authored
Merge pull request WordPress#2618 from rodrigoprimo/escape-output-namespaced-names-tests
Security/EscapeOutput: fix false positive and add tests for namespaced names
2 parents f0dc6ab + 0638f0b commit f48a181

File tree

3 files changed

+171
-3
lines changed

3 files changed

+171
-3
lines changed

‎WordPress/Sniffs/Security/EscapeOutputSniff.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -741,7 +741,7 @@ protected function check_code_is_escaped( $start, $end, $code = 'OutputNotEscape
741741
// Special case get_search_query() which is unsafe if $escaped = false.
742742
if ( 'get_search_query' === strtolower( $functionName ) ) {
743743
$escaped_param = PassedParameters::getParameter( $this->phpcsFile, $ptr, 1, 'escaped' );
744-
if ( false !== $escaped_param && 'true' !== $escaped_param['clean'] ) {
744+
if ( false !== $escaped_param && 'true' !== strtolower( ltrim( $escaped_param['clean'], '\\' ) ) ) {
745745
$this->phpcsFile->addError(
746746
'Output from get_search_query() is unsafe due to $escaped parameter being set to "false".',
747747
$ptr,

‎WordPress/Tests/Security/EscapeOutputUnitTest.1.inc‎

Lines changed: 126 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ echo esc_html_x( $some_nasty_var, 'context' ); // Ok.
258258
<input type="hidden" name="some-action" value="<?php echo esc_attr_x( 'none', 'context' ); ?>" /><!-- OK. -->
259259
<?php
260260

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

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

663663
exit( status: $foo ); // Bad.
664-
die( status: $foo ); // Bad.
664+
\Die( status: $foo ); // Bad.
665665

666666
/*
667667
* Issue https://github.com/WordPress/WordPress-Coding-Standards/issues/2552
@@ -687,3 +687,127 @@ _deprecated_function( __METHOD__, 'x.x.x', \ClassName::class ); // OK.
687687
die( \MyNamespace\ClassName::class . ' has been abandoned' ); // OK.
688688
echo 'Do not use ' . MyNamespace\ClassName::class; // OK.
689689
_deprecated_function( __METHOD__, 'x.x.x', namespace\ClassName::class ); // OK.
690+
691+
/*
692+
* Safeguard correct handling of all types of namespaced escaping and printing function calls.
693+
*/
694+
\printf( 'Hello %s', $foo ); // Bad.
695+
MyNamespace\wp_die( $message ); // Ok.
696+
\MyNamespace\vprintf( 'Hello %s', array( $foo ) ); // Ok.
697+
namespace\wp_dropdown_pages( $args ); // Ok. The sniff should start flagging this once it can resolve relative namespaces.
698+
namespace\Sub\_deprecated_function( __FUNCTION__, '1.3.0', $another_func ); // Ok.
699+
\printf( 'Hello %s', \esc_html( $foo ) ); // Ok.
700+
\wp_die( MyNamespace\number_format( $foo ) ); // Bad.
701+
\vprintf( 'Hello %s', array( \MyNamespace\sanitize_user_field( $foo ) ) ); // Bad.
702+
\wp_dropdown_pages( namespace\sanitize_key( $foo ) ); // Bad. The sniff should stop flagging this once it can resolve relative namespaces.
703+
\_deprecated_function( __FUNCTION__, '1.3.0', namespace\Sub\wp_kses( $another_func ) ); // Bad.
704+
705+
/*
706+
* Safeguard correct handling of namespaced auto-escaped functions.
707+
*/
708+
echo \bloginfo( $var ); // Ok.
709+
echo MyNamespace\count( $var ); // Bad.
710+
echo \MyNamespace\get_archives_link( $url, 'link' ); // Bad.
711+
echo namespace\get_search_form(); // Bad. The sniff should stop flagging this once it can resolve relative namespaces.
712+
echo namespace\Sub\the_author(); // Bad.
713+
714+
/*
715+
* Safeguard correct handling of namespaced unsafe printing functions.
716+
*/
717+
\_e( $text, 'my-domain' ); // Bad.
718+
MyNamespace\_ex( $text, 'context' ); // Ok.
719+
\MyNamespace\_e( $text, 'my-domain' ); // Ok.
720+
namespace\_ex( $text, 'context' ); // Ok. The sniff should start flagging this once it can resolve relative namespaces.
721+
namespace\Sub\_e( $text, 'my-domain' ); // Ok.
722+
723+
/*
724+
* Safeguard correct handling of namespaced formatting functions.
725+
*/
726+
echo \sprintf( '%s', $var ); // Bad.
727+
echo \sprintf( '%s', esc_html( $var ) ); // Ok.
728+
echo MyNamespace\antispambot( esc_html( $email ) ); // Bad.
729+
echo \MyNamespace\ent2ncr( esc_html( $_data ) ); // Bad.
730+
echo namespace\vsprintf( 'Hello %s', array( esc_html( $foo ) ) ); // Bad. The sniff should stop flagging this once it can resolve relative namespaces.
731+
echo namespace\Sub\wp_sprintf( 'Hello %s', array( esc_html( $foo ) ) ); // Bad.
732+
733+
/*
734+
* Safeguard correct handling of get_search_query() as the sniff has special logic to check the $escaped parameter.
735+
*/
736+
echo \get_search_query( true ); // Ok.
737+
echo \get_search_query( false ); // Bad.
738+
echo MyNamespace\get_search_query( true ); // Bad.
739+
echo \MyNamespace\get_search_query( true ); // Bad.
740+
echo namespace\get_search_query( true ); // Bad. The sniff should stop flagging this once it can resolve relative namespaces.
741+
echo namespace\Sub\get_search_query( true ); // Bad.
742+
743+
/*
744+
* Safeguard correct handling of fully qualified and namespace relative functions with special parameter handling.
745+
*/
746+
\trigger_error( esc_html( $message ), $second_param_should_be_ignored ); // Ok.
747+
\User_Error( $message ); // Bad.
748+
namespace\trigger_error( $message ); // Ok. The sniff should start flagging this once it can resolve relative namespaces.
749+
namespace\Sub\user_error( $message ); // Ok.
750+
\_deprecated_file( basename( __FILE__ ), '1.3.0' ); // Ok.
751+
\_deprecated_file( $file, '1.3.0' ); // Error.
752+
namespace\_deprecated_file( basename( __FILE__ ), '1.3.0' ); // Ok.
753+
namespace\_DEPRECATED_FILE( $file, '1.3.0' ); // Ok. The sniff should start flagging this once it can resolve relative namespaces.
754+
namespace\Sub\_deprecated_file( $file, '1.3.0' ); // Ok.
755+
756+
/*
757+
* Safeguard that the basename( __FILE__ ) pattern recognition in _deprecated_file() only applies to
758+
* the global basename() function and not to other constructs.
759+
*/
760+
_deprecated_file( $obj->basename( __FILE__ ), '1.3.0' ); // Bad.
761+
_deprecated_file( $obj?->basename( __FILE__ ), '1.3.0' ); // Bad.
762+
_deprecated_file( MyClass::basename( __FILE__ ), '1.3.0' ); // Bad.
763+
_deprecated_file( BASENAME, __FILE__ ); // Bad.
764+
_deprecated_file( MyNamespace\basename( __FILE__ ), '1.3.0' ); // Bad.
765+
_deprecated_file( \MyNamespace\basename( __FILE__ ), '1.3.0' ); // Bad.
766+
_deprecated_file( namespace\basename( __FILE__ ), '1.3.0' ); // Bad. We might want to update the regex so that the sniff stops flagging this once it can resolve relative namespaces.
767+
_deprecated_file( namespace\Sub\basename( __FILE__ ), '1.3.0' ); // Bad.
768+
_deprecated_file( basename(...), '1.3.0' ); // Bad.
769+
770+
/*
771+
* Safeguard correct handling of FQN true/false/null constants.
772+
*/
773+
echo \true, \False, \NULL; // Ok.
774+
775+
/*
776+
* Safeguard correct handling of namespaced constants that mirror the name of "safe" global PHP constants.
777+
*/
778+
echo MyNamespace\PHP_EOL; // Bad.
779+
echo \MyNamespace\PHP_VERSION_ID; // Bad.
780+
echo namespace\PHP_EXTRA_VERSION; // Bad. The sniff should stop flagging this once it can resolve relative namespaces.
781+
echo namespace\Sub\PHP_VERSION; // Bad.
782+
783+
/*
784+
* Safeguard correct handling of FQN functions with multiple PHP short echo tags.
785+
*/
786+
?>
787+
<a href="<?= \get_permalink(); // Bad. ?>" title="<?= \the_title_attribute( array( 'echo' => false ) ); ?>">
788+
<?= \get_the_title(); // Bad. ?>
789+
</a>
790+
<?php
791+
792+
/*
793+
* Safeguard correct handling of array walking functions with namespaced name variations.
794+
*
795+
* Note: the number of errors triggered by the sniff in these tests is inconsistent due to a bug
796+
* in how the sniff handles namespaced function calls. The calls to MyNamespace\array_map() and \MyNamespace\map_deep()
797+
* should result in the same number of errors. See https://github.com/WordPress/WordPress-Coding-Standards/issues/2671.
798+
*/
799+
echo implode( '<br>', \array_map( 'esc_html', $items ) ); // Ok.
800+
echo implode( '<br>', MyNamespace\array_map( 'esc_html', $items ) ); // Bad x 2.
801+
echo implode( '<br>', \MyNamespace\map_deep( $items, 'esc_html' ) ); // Bad.
802+
echo implode( '<br>', namespace\array_map( 'esc_html', $items ) ); // Bad x 2. The sniff should stop flagging this once it can resolve relative namespaces.
803+
echo implode( '<br>', namespace\Sub\map_deep( $items, 'esc_html' ) ); // Bad.
804+
805+
/*
806+
* Safeguard correct handling of get_search_query() with FQN and non-standard case booleans for the $escaped parameter.
807+
*/
808+
echo \get_search_query( TRUE ); // Ok.
809+
echo \get_search_query( \true ); // Ok.
810+
echo \get_search_query( \True ); // Ok.
811+
echo \get_search_query( FaLsE ); // Bad.
812+
echo \get_search_query( \false ); // Bad.
813+
echo \get_search_query( \FALSE ); // Bad.

‎WordPress/Tests/Security/EscapeOutputUnitTest.php‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,50 @@ public function getErrorList( $testFile = '' ) {
164164
672 => 1,
165165
673 => 1,
166166
678 => 1,
167+
694 => 1,
168+
700 => 1,
169+
701 => 1,
170+
702 => 1,
171+
703 => 1,
172+
709 => 1,
173+
710 => 1,
174+
711 => 1,
175+
712 => 1,
176+
717 => 1,
177+
726 => 1,
178+
728 => 1,
179+
729 => 1,
180+
730 => 1,
181+
731 => 1,
182+
737 => 1,
183+
738 => 1,
184+
739 => 1,
185+
740 => 1,
186+
741 => 1,
187+
747 => 1,
188+
751 => 1,
189+
760 => 1,
190+
761 => 1,
191+
762 => 1,
192+
763 => 1,
193+
764 => 1,
194+
765 => 1,
195+
766 => 1,
196+
767 => 1,
197+
768 => 1,
198+
778 => 1,
199+
779 => 1,
200+
780 => 1,
201+
781 => 1,
202+
787 => 1,
203+
788 => 1,
204+
800 => 2,
205+
801 => 1,
206+
802 => 2,
207+
803 => 1,
208+
811 => 1,
209+
812 => 1,
210+
813 => 1,
167211
);
168212

169213
case 'EscapeOutputUnitTest.6.inc':

0 commit comments

Comments
 (0)