Skip to content

Commit cc1022e

Browse files
committed
ProperEscapingFunction: fix overreach
As part of the changes made in 624, the `T_COMMA` token was added to the list of tokens to skip over to allow for `echo` statements with multiple arguments passed as a coma-delimited list. As a side-effect, this caused the sniff to also examine `[s]printf()`-like function calls where the first parameter is a text string, while the second is often a variable within a call to one of the escaping functions. The current change fixes this by only adding the `T_COMMA` token to the "ignore when looking for the previous token"-list when in an `echo` statement. Includes unit test. Fixes 667 Additional notes: * I've run the sniff over WP Core to verify the fix and have verified that all 23 violations being throw up are correctly detected violations. * If it would be considered a good idea to also examine, `[s]printf()`-like function calls for this sniff for proper escaping, I suggest opening a separate, new feature request as that change would need significantly different and quite complex logic and does not fall within the scope of this bug fix.
1 parent 181b6f7 commit cc1022e

File tree

2 files changed

+10
-2
lines changed

2 files changed

+10
-2
lines changed

WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@ class ProperEscapingFunctionSniff extends Sniff {
4646
T_OPEN_TAG => T_OPEN_TAG,
4747
T_OPEN_TAG_WITH_ECHO => T_OPEN_TAG_WITH_ECHO,
4848
T_STRING_CONCAT => T_STRING_CONCAT,
49-
T_COMMA => T_COMMA,
5049
T_NS_SEPARATOR => T_NS_SEPARATOR,
5150
];
5251

@@ -107,7 +106,13 @@ public function process_token( $stackPtr ) {
107106
return;
108107
}
109108

110-
$html = $this->phpcsFile->findPrevious( $this->echo_or_concat_tokens, $stackPtr - 1, null, true );
109+
$ignore = $this->echo_or_concat_tokens;
110+
$start_of_statement = $this->phpcsFile->findStartOfStatement( $stackPtr, T_COMMA );
111+
if ( $this->tokens[ $start_of_statement ]['code'] === T_ECHO ) {
112+
$ignore[ T_COMMA ] = T_COMMA;
113+
}
114+
115+
$html = $this->phpcsFile->findPrevious( $ignore, $stackPtr - 1, null, true );
111116

112117
// Use $textStringTokens b/c heredoc and nowdoc tokens will never be encountered in this context anyways..
113118
if ( $html === false || isset( Tokens::$textStringTokens[ $this->tokens[ $html ]['code'] ] ) === false ) {

WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,3 +82,6 @@ echo '<a href="', esc_html($url), '">'; // Error.
8282
echo '<a href=', esc_html($url), '>'; // Error.
8383

8484
echo 'data-param-url="' . Esc_HTML::static_method( $share_url ) . '"'; // OK.
85+
86+
// Not a target for this sniff (yet).
87+
printf( '<meta name="generator" content="%s">', esc_attr( $content ) ); // OK.

0 commit comments

Comments
 (0)