Skip to content

Commit f477eb9

Browse files
rebeccahumrebeccahum
committed
Re-factor logic to be less redundant and account for comma as a string concatenation operator
1 parent dad0b93 commit f477eb9

File tree

3 files changed

+41
-31
lines changed

3 files changed

+41
-31
lines changed

‎WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php

Lines changed: 34 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,15 @@ class ProperEscapingFunctionSniff extends Sniff {
2424
* @var array
2525
*/
2626
public $escaping_functions = [
27-
'esc_url',
28-
'esc_attr',
29-
'esc_attr__',
30-
'esc_html',
27+
'esc_url' => 'url',
28+
'esc_attr' => 'attr',
29+
'esc_attr__' => 'attr',
30+
'esc_attr_x' => 'attr',
31+
'esc_attr_e' => 'attr',
32+
'esc_html' => 'html',
33+
'esc_html__' => 'html',
34+
'esc_html_x' => 'html',
35+
'esc_html_e' => 'html',
3136
];
3237

3338
/**
@@ -48,51 +53,38 @@ public function register() {
4853
*/
4954
public function process_token( $stackPtr ) {
5055

51-
if ( in_array( $this->tokens[ $stackPtr ]['content'], $this->escaping_functions, true ) === false ) {
56+
if ( isset( $this->escaping_functions[ $this->tokens[ $stackPtr ]['content'] ] ) === false ) {
5257
return;
5358
}
5459

5560
$function_name = $this->tokens[ $stackPtr ]['content'];
5661

5762
$data = [ $function_name ];
5863

59-
if ( $function_name === 'esc_attr' || $function_name === 'esc_attr__' ) {
60-
// Find esc_attr being used between HTML tags.
61-
$tokens = array_merge(
62-
Tokens::$emptyTokens,
63-
[
64-
'T_ECHO' => T_ECHO,
65-
'T_OPEN_TAG' => T_OPEN_TAG,
66-
]
67-
);
68-
$html_tag = $this->phpcsFile->findPrevious( $tokens, $stackPtr - 1, null, true );
69-
if ( $this->tokens[ $html_tag ]['type'] === 'T_INLINE_HTML' && false !== strpos( $this->tokens[ $html_tag ]['content'], '>' ) ) {
70-
$message = 'Wrong escaping function, please do not use `%s()` in a context outside of HTML attributes.';
71-
$this->phpcsFile->addError( $message, $html_tag, 'notAttrEscAttr', $data );
72-
return;
73-
}
74-
}
75-
76-
$echo_or_string_concat = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $stackPtr - 1, null, true );
77-
78-
if ( $this->tokens[ $echo_or_string_concat ]['code'] === T_ECHO ) {
64+
$echo_or_concat_or_html = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $stackPtr - 1, null, true );
65+
66+
if ( $this->tokens[ $echo_or_concat_or_html ]['code'] === T_ECHO ) {
7967
// Very likely inline HTML with <?php tag.
80-
$php_open = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $echo_or_string_concat - 1, null, true );
68+
$php_open = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $echo_or_concat_or_html - 1, null, true );
8169

8270
if ( $this->tokens[ $php_open ]['code'] !== T_OPEN_TAG ) {
8371
return;
8472
}
8573

8674
$html = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $php_open - 1, null, true );
8775

88-
if ( $this->tokens[ $html ]['code'] !== T_INLINE_HTML ) {
76+
if ( $this->tokens[ $html ]['code'] === T_INLINE_HTML && $this->is_outside_html_attr_context( $function_name, $this->tokens[ $html ]['content'] ) ) {
77+
$message = 'Wrong escaping function, please do not use `%s()` in a context outside of HTML attributes.';
78+
$this->phpcsFile->addError( $message, $html, 'notAttrEscAttr', $data );
8979
return;
9080
}
91-
} elseif ( $this->tokens[ $echo_or_string_concat ]['code'] === T_STRING_CONCAT ) {
81+
} elseif ( $this->tokens[ $echo_or_concat_or_html ]['code'] === T_STRING_CONCAT || $this->tokens[ $echo_or_concat_or_html ]['code'] === T_COMMA ) {
9282
// Very likely string concatenation mixing strings and functions/variables.
93-
$html = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $echo_or_string_concat - 1, null, true );
83+
$html = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $echo_or_concat_or_html - 1, null, true );
9484

95-
if ( $this->tokens[ $html ]['code'] !== T_CONSTANT_ENCAPSED_STRING ) {
85+
if ( $this->tokens[ $html ]['code'] === T_CONSTANT_ENCAPSED_STRING && $this->is_outside_html_attr_context( $function_name, trim( $this->tokens[ $html ]['content'], '"\'' ) ) ) {
86+
$message = 'Wrong escaping function, please do not use `%s()` in a context outside of HTML attributes.';
87+
$this->phpcsFile->addError( $message, $html, 'notAttrEscAttr', $data );
9688
return;
9789
}
9890
} else {
@@ -171,4 +163,16 @@ public function is_html_attr( $content ) {
171163
public function endswith( $haystack, $needle ) {
172164
return substr( $haystack, -strlen( $needle ) ) === $needle;
173165
}
166+
167+
/**
168+
* Tests whether provided string ends with closing HTML tag in an attribute context.
169+
*
170+
* @param string $function_name Escaping attribute function name.
171+
* @param string $content Haystack in which we look for open HTML tag.
172+
*
173+
* @return bool True if string ends with open HTML tag.
174+
*/
175+
public function is_outside_html_attr_context( $function_name, $content ) {
176+
return $this->escaping_functions[ $function_name ] === 'attr' && substr( $content, -1 ) === '>';
177+
}
174178
}

‎WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,4 +59,7 @@ h1 {
5959
Test
6060
</article>
6161

62-
<h1><?php echo esc_attr__( $title, 'domain' ); ?></h1> <!-- Error -->
62+
<h1><?php echo esc_attr__( $title, 'domain' ); ?></h1> <!-- Error --> ?>
63+
<?php echo '<h1>' . esc_attr__( $some_var, 'domain' ) . '</h1>'; // Error.
64+
echo '<h1>', esc_attr__( $title, 'domain' ), '</h1>'; // Error.
65+
echo '<h1>', esc_attr__( $title, 'domain' ) . '</h1>'; // Error.

‎WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@ public function getErrorList() {
3636
41 => 1,
3737
45 => 1,
3838
62 => 1,
39+
63 => 1,
40+
64 => 1,
41+
65 => 1,
3942
];
4043
}
4144

0 commit comments

Comments
 (0)