Skip to content

Commit 288ad35

Browse files
rebeccahumrebeccahum
authored
Merge pull request #624 from Automattic/fix_155
ProperEscapingFunction: Flag when esc_attr is being used outside of HTML attributes
2 parents 35f3305 + 5962dc4 commit 288ad35

File tree

3 files changed

+164
-53
lines changed

3 files changed

+164
-53
lines changed

‎WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php‎

Lines changed: 92 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,56 @@ class ProperEscapingFunctionSniff extends Sniff {
2323
*
2424
* @var array
2525
*/
26-
public $escaping_functions = [
27-
'esc_url',
28-
'esc_attr',
29-
'esc_html',
26+
protected $escaping_functions = [
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',
36+
];
37+
38+
/**
39+
* List of tokens we can skip.
40+
*
41+
* @var array
42+
*/
43+
private $echo_or_concat_tokens =
44+
[
45+
T_ECHO => T_ECHO,
46+
T_OPEN_TAG => T_OPEN_TAG,
47+
T_OPEN_TAG_WITH_ECHO => T_OPEN_TAG_WITH_ECHO,
48+
T_STRING_CONCAT => T_STRING_CONCAT,
49+
T_COMMA => T_COMMA,
50+
T_NS_SEPARATOR => T_NS_SEPARATOR,
51+
];
52+
53+
/**
54+
* List of attributes associated with url outputs.
55+
*
56+
* @var array
57+
*/
58+
private $url_attrs = [
59+
'href',
60+
'src',
61+
'url',
62+
'action',
63+
];
64+
65+
/**
66+
* List of syntaxes for inside attribute detection.
67+
*
68+
* @var array
69+
*/
70+
private $attr_endings = [
71+
'=',
72+
'="',
73+
"='",
74+
"=\\'",
75+
'=\\"',
3076
];
3177

3278
/**
@@ -35,6 +81,8 @@ class ProperEscapingFunctionSniff extends Sniff {
3581
* @return array
3682
*/
3783
public function register() {
84+
$this->echo_or_concat_tokens += Tokens::$emptyTokens;
85+
3886
return [ T_STRING ];
3987
}
4088

@@ -47,47 +95,47 @@ public function register() {
4795
*/
4896
public function process_token( $stackPtr ) {
4997

50-
if ( in_array( $this->tokens[ $stackPtr ]['content'], $this->escaping_functions, true ) === false ) {
98+
$function_name = strtolower( $this->tokens[ $stackPtr ]['content'] );
99+
100+
if ( isset( $this->escaping_functions[ $function_name ] ) === false ) {
51101
return;
52102
}
53103

54-
$function_name = $this->tokens[ $stackPtr ]['content'];
104+
$next_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true );
105+
if ( $next_non_empty === false || $this->tokens[ $next_non_empty ]['code'] !== T_OPEN_PARENTHESIS ) {
106+
// Not a function call.
107+
return;
108+
}
55109

56-
$echo_or_string_concat = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $stackPtr - 1, null, true );
110+
$html = $this->phpcsFile->findPrevious( $this->echo_or_concat_tokens, $stackPtr - 1, null, true );
57111

58-
if ( $this->tokens[ $echo_or_string_concat ]['code'] === T_ECHO ) {
59-
// Very likely inline HTML with <?php tag.
60-
$php_open = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $echo_or_string_concat - 1, null, true );
112+
// Use $textStringTokens b/c heredoc and nowdoc tokens will never be encountered in this context anyways..
113+
if ( $html === false || isset( Tokens::$textStringTokens[ $this->tokens[ $html ]['code'] ] ) === false ) {
114+
return;
115+
}
61116

62-
if ( $this->tokens[ $php_open ]['code'] !== T_OPEN_TAG ) {
63-
return;
64-
}
117+
$data = [ $function_name ];
65118

66-
$html = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $php_open - 1, null, true );
119+
$content = $this->tokens[ $html ]['content'];
120+
if ( isset( Tokens::$stringTokens[ $this->tokens[ $html ]['code'] ] ) === true ) {
121+
$content = Sniff::strip_quotes( $content );
122+
}
67123

68-
if ( $this->tokens[ $html ]['code'] !== T_INLINE_HTML ) {
69-
return;
70-
}
71-
} elseif ( $this->tokens[ $echo_or_string_concat ]['code'] === T_STRING_CONCAT ) {
72-
// Very likely string concatenation mixing strings and functions/variables.
73-
$html = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $echo_or_string_concat - 1, null, true );
124+
$escaping_type = $this->escaping_functions[ $function_name ];
74125

75-
if ( $this->tokens[ $html ]['code'] !== T_CONSTANT_ENCAPSED_STRING ) {
76-
return;
77-
}
78-
} else {
79-
// Neither - bailing.
126+
if ( $escaping_type === 'attr' && $this->is_outside_html_attr_context( $content ) ) {
127+
$message = 'Wrong escaping function, using `%s()` in a context outside of HTML attributes may not escape properly.';
128+
$this->phpcsFile->addError( $message, $html, 'notAttrEscAttr', $data );
80129
return;
81130
}
82131

83-
$data = [ $function_name ];
84-
85-
if ( $function_name !== 'esc_url' && $this->attr_expects_url( $this->tokens[ $html ]['content'] ) ) {
132+
if ( $escaping_type !== 'url' && $this->attr_expects_url( $content ) ) {
86133
$message = 'Wrong escaping function. href, src, and action attributes should be escaped by `esc_url()`, not by `%s()`.';
87134
$this->phpcsFile->addError( $message, $stackPtr, 'hrefSrcEscUrl', $data );
88135
return;
89136
}
90-
if ( $function_name === 'esc_html' && $this->is_html_attr( $this->tokens[ $html ]['content'] ) ) {
137+
138+
if ( $escaping_type === 'html' && $this->is_html_attr( $content ) ) {
91139
$message = 'Wrong escaping function. HTML attributes should be escaped by `esc_attr()`, not by `%s()`.';
92140
$this->phpcsFile->addError( $message, $stackPtr, 'htmlAttrNotByEscHTML', $data );
93141
return;
@@ -99,17 +147,12 @@ public function process_token( $stackPtr ) {
99147
*
100148
* @param string $content Haystack in which we look for an open attribute which exects a URL value.
101149
*
102-
* @return bool True if string ends with open attribute which exects a URL value.
150+
* @return bool True if string ends with open attribute which expects a URL value.
103151
*/
104152
public function attr_expects_url( $content ) {
105153
$attr_expects_url = false;
106-
foreach ( [ 'href', 'src', 'url', 'action' ] as $attr ) {
107-
foreach ( [
108-
'="',
109-
"='",
110-
'=\'"', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
111-
'="\'', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
112-
] as $ending ) {
154+
foreach ( $this->url_attrs as $attr ) {
155+
foreach ( $this->attr_endings as $ending ) {
113156
if ( $this->endswith( $content, $attr . $ending ) === true ) {
114157
$attr_expects_url = true;
115158
break;
@@ -128,12 +171,7 @@ public function attr_expects_url( $content ) {
128171
*/
129172
public function is_html_attr( $content ) {
130173
$is_html_attr = false;
131-
foreach ( [
132-
'="',
133-
"='",
134-
'=\'"', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
135-
'="\'', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
136-
] as $ending ) {
174+
foreach ( $this->attr_endings as $ending ) {
137175
if ( $this->endswith( $content, $ending ) === true ) {
138176
$is_html_attr = true;
139177
break;
@@ -142,6 +180,17 @@ public function is_html_attr( $content ) {
142180
return $is_html_attr;
143181
}
144182

183+
/**
184+
* Tests whether an attribute escaping function is being used outside of an HTML tag.
185+
*
186+
* @param string $content Haystack where we look for the end of a HTML tag.
187+
*
188+
* @return bool True if the passed string ends a HTML tag.
189+
*/
190+
public function is_outside_html_attr_context( $content ) {
191+
return $this->endswith( trim( $content ), '>' );
192+
}
193+
145194
/**
146195
* A helper function which tests whether string ends with some other.
147196
*

‎WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc‎

Lines changed: 53 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,26 @@
11
<?php
22

3-
echo '<a href="' . esc_attr( $some_var ) . '"></a>'; // NOK.
3+
echo '<a href="' . esc_attr( $some_var ) . '"></a>'; // Error.
44

5-
echo "<a href='" . esc_attr( $some_var ) . "'></a>"; // NOK.
5+
echo "<a href='" . \esc_attr( $some_var ) . "'></a>"; // Error.
66

7-
echo '<a href="' . esc_url( $some_var ) . '"></a>'; // OK.
7+
echo '<a href="' . \esc_url( $some_var ) . '"></a>'; // OK.
88

99
echo "<a href='" . esc_url( $some_var ) . "'></a>"; // OK.
1010

1111
echo '<a title="' . esc_attr( $some_var ) . '"></a>'; // OK.
1212

13-
echo "<a title='" . esc_attr( $some_var ) . "'></a>"; // OK.
13+
echo "<a title='" . \esc_attr( $some_var ) . "'></a>"; // OK.
1414

15-
echo '<a title="' . esc_html( $some_var ) . '"></a>'; // NOK.
15+
echo '<a title="' . esc_html_x( $some_var ) . '"></a>'; // Error.
1616

17-
echo "<a title='" . esc_html( $some_var ) . "'></a>"; // NOK.
17+
echo "<a title='" . \esc_html( $some_var ) . "'></a>"; // Error.
1818

1919
?>
2020

21-
<a href="<?php echo esc_attr( $some_var ); ?>">Hello</a> <!-- NOK. -->
21+
<a href="<?php echo esc_attr( $some_var ); ?>">Hello</a> <!-- Error. -->
2222

23-
<a href="" class="<?php echo esc_html( $some_var); ?>">Hey</a> <!-- NOK. -->
23+
<a href="" class="<?php esc_html_e( $some_var); ?>">Hey</a> <!-- Error. -->
2424

2525
<a href="<?php esc_url( $url );?>"></a> <!-- OK. -->
2626

@@ -30,12 +30,55 @@ echo "<a title='" . esc_html( $some_var ) . "'></a>"; // NOK.
3030

3131
echo '<media:content url="' . esc_url( $post_image ) . '" medium="image">'; // OK.
3232

33-
echo '<media:content url="' . esc_attr( $post_image ) . '" medium="image">'; // NOK.
33+
echo '<media:content url="' . esc_attr( $post_image ) . '" medium="image">'; // Error.
3434

3535
echo 'data-param-url="' . esc_url( $share_url ) . '"'; // OK.
3636

37-
echo 'data-param-url="' . esc_html( $share_url ) . '"'; // NOK.
37+
echo 'data-param-url="' . Esc_HTML( $share_url ) . '"'; // Error.
3838

3939
?>
4040

4141
<form method="post" action="<?php echo esc_html(admin_url('admin.php?page='.$base_name.'&amp;mode=logs&amp;id='.$poll_id)); ?>">
42+
43+
44+
45+
<a href="#link"><?php echo esc_attr( 'testing' ); // Error.
46+
?> </a>
47+
48+
<?php echo ' <h' . "2> " , esc_attr( $heading ) . "</h2>"; // Error.
49+
?> <style>
50+
h1 {
51+
color: <?php echo esc_attr( $color ) ?>; /* OK */
52+
font-family: Arial;
53+
font-size: 10px;
54+
}
55+
</style>
56+
57+
<article id="foo-bar" data-property="<?php echo esc_attr( $year ); // OK.
58+
?>">
59+
Test
60+
</article>
61+
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_x( $title, 'domain' ), '</h1>'; // Error.
65+
echo "<$tag> " , esc_attr( $test ) , "</$tag>"; // Error.
66+
?>
67+
<h1> <?php echo esc_attr( $title ) . '</h1>'; ?> // Error.
68+
<div><?= esc_attr($attr) ?></div><!-- Error -->
69+
<div><?php esc_attr_e($attr) ?></div><!-- Error -->
70+
<div data-tag="<?= esc_attr( $attr ); ?>"> <!-- OK -->
71+
<?php echo "<div>" . $test . "</div>"; // OK.
72+
echo "<{$tag}>" . esc_attr( $tag_content ) . "</{$tag}>"; // Error.
73+
echo "<$tag" . ' >' . esc_attr( $tag_content ) . "</$tag>"; // Error.
74+
echo '<div class=\'' . esc_html($class) . '\'>'; // Error.
75+
echo "<div class=\"" . \esc_html__($class) . '">'; // Error.
76+
echo "<div $someAttribute class=\"" . esc_html($class) . '">'; // Error.
77+
echo '<a href=\'' . esc_html($url) . '\'>'; // Error.
78+
echo "<img src=\"" . esc_html($src) . '"/>'; // Error.
79+
echo "<div $someAttributeName-url=\"" . esc_html($url) . '">'; // Error.
80+
echo '<a href="', esc_html($url), '">'; // Error.
81+
82+
echo '<a href=', esc_html($url), '>'; // Error.
83+
84+
echo 'data-param-url="' . Esc_HTML::static_method( $share_url ) . '"'; // OK.

‎WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.php‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,25 @@ public function getErrorList() {
3434
33 => 1,
3535
37 => 1,
3636
41 => 1,
37+
45 => 1,
38+
48 => 1,
39+
62 => 1,
40+
63 => 1,
41+
64 => 1,
42+
65 => 1,
43+
67 => 1,
44+
68 => 1,
45+
69 => 1,
46+
72 => 1,
47+
73 => 1,
48+
74 => 1,
49+
75 => 1,
50+
76 => 1,
51+
77 => 1,
52+
78 => 1,
53+
79 => 1,
54+
80 => 1,
55+
82 => 1,
3756
];
3857
}
3958

0 commit comments

Comments
 (0)