Skip to content

Commit 45f8e0d

Browse files
david-bindadavid-binda
authored
Merge pull request #106 from Automattic/add-proper-escaping-function-sniff
Adding VIP.ProperEscapingFunction sniff for catching instances where …
2 parents 6e16ac4 + 9dee12b commit 45f8e0d

File tree

5 files changed

+231
-1
lines changed

5 files changed

+231
-1
lines changed

‎WordPressVIPMinimum/Sniffs/VIP/ProperEscapingFunctionSniff.php‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
<?php
2+
/**
3+
* WordPress-VIP-Minimum Coding Standard.
4+
*
5+
* @package VIPCS\WordPressVIPMinimum
6+
* @link https://github.com/Automattic/VIP-Coding-Standards
7+
*/
8+
9+
namespace WordPressVIPMinimum\Sniffs\VIP;
10+
11+
use PHP_CodeSniffer_File as File;
12+
use PHP_CodeSniffer_Tokens as Tokens;
13+
14+
/**
15+
* Checks whether proper escaping function is used.
16+
*
17+
* @package VIPCS\WordPressVIPMinimum
18+
*/
19+
class ProperEscapingFunctionSniff implements \PHP_CodeSniffer_Sniff {
20+
21+
/**
22+
* List of escaping functions which are being tested.
23+
*
24+
* @var array
25+
*/
26+
public $escaping_functions = array(
27+
'esc_url',
28+
'esc_attr',
29+
'esc_html',
30+
);
31+
32+
/**
33+
* Returns an array of tokens this test wants to listen for.
34+
*
35+
* @return array
36+
*/
37+
public function register() {
38+
return Tokens::$functionNameTokens;
39+
}
40+
41+
/**
42+
* Process this test when one of its tokens is encountered
43+
*
44+
* @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned.
45+
* @param int $stackPtr The position of the current token in the stack passed in $tokens.
46+
*
47+
* @return void
48+
*/
49+
public function process( File $phpcsFile, $stackPtr ) {
50+
51+
$tokens = $phpcsFile->getTokens();
52+
53+
if ( false === in_array( $tokens[ $stackPtr ]['content'], $this->escaping_functions, true ) ) {
54+
return;
55+
}
56+
57+
$function_name = $tokens[ $stackPtr ]['content'];
58+
59+
$echo_or_string_concat = $phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true );
60+
61+
if ( T_ECHO === $tokens[ $echo_or_string_concat ]['code'] ) {
62+
// Very likely inline HTML with <?php tag.
63+
$php_open = $phpcsFile->findPrevious( Tokens::$emptyTokens, ( $echo_or_string_concat - 1 ), null, true );
64+
65+
if ( T_OPEN_TAG !== $tokens[ $php_open ]['code'] ) {
66+
return;
67+
}
68+
69+
$html = $phpcsFile->findPrevious( Tokens::$emptyTokens, ( $php_open - 1 ), null, true );
70+
71+
if ( T_INLINE_HTML !== $tokens[ $html ]['code'] ) {
72+
return;
73+
}
74+
} elseif ( T_STRING_CONCAT === $tokens[ $echo_or_string_concat ]['code'] ) {
75+
// Very likely string concatenation mixing strings and functions/variables.
76+
$html = $phpcsFile->findPrevious( Tokens::$emptyTokens, ( $echo_or_string_concat - 1 ), null, true );
77+
78+
if ( T_CONSTANT_ENCAPSED_STRING !== $tokens[ $html ]['code'] ) {
79+
return;
80+
}
81+
} else {
82+
// Neither - bailing.
83+
return;
84+
}
85+
86+
if ( 'esc_url' !== $function_name && $this->is_href_or_src( $tokens[ $html ]['content'] ) ) {
87+
$phpcsFile->addError( sprintf( 'Wrong escaping function. href and src attributes should be escaped by esc_url, not by %s', $function_name ), $stackPtr, 'hrefSrcEscUrl' );
88+
return;
89+
}
90+
if ( 'esc_html' === $function_name && $this->is_html_attr( $tokens[ $html ]['content'] ) ) {
91+
$phpcsFile->addError( sprintf( 'Wrong escaping function. HTML attributes should be escaped by esc_attr, not by %s', $function_name ), $stackPtr, 'htmlAttrNotByEscHTML' );
92+
return;
93+
}
94+
95+
}//end process()
96+
97+
/**
98+
* Tests whether provided string ends with open src or href attribute.
99+
*
100+
* @param string $content Haystack in which we look for an open src or href attribute.
101+
*
102+
* @return bool True if string ends with open src or href attribute.
103+
*/
104+
public function is_href_or_src( $content ) {
105+
$is_href_or_src = false;
106+
foreach ( array( 'href', 'src' ) as $attr ) {
107+
foreach ( array(
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 ) {
113+
if ( true === $this->endswith( $content, $attr . $ending ) ) {
114+
$is_href_or_src = true;
115+
break;
116+
}
117+
}
118+
}
119+
return $is_href_or_src;
120+
}
121+
122+
/**
123+
* Tests, whether provided string ends with open HMTL attribute.
124+
*
125+
* @param string $content Haystack in which we look for open HTML attribute.
126+
*
127+
* @return bool True if string ends with open HTML attribute.
128+
*/
129+
public function is_html_attr( $content ) {
130+
$is_html_attr = false;
131+
foreach ( array(
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 ) {
137+
if ( true === $this->endswith( $content, $ending ) ) {
138+
$is_html_attr = true;
139+
break;
140+
}
141+
}
142+
return $is_html_attr;
143+
}
144+
145+
/**
146+
* A helper function which tests whether string ends with some other.
147+
*
148+
* @param string $haystack String which is being tested.
149+
* @param string $needle The substring, which we try to locate on the end of the $haystack.
150+
*
151+
* @return bool True if haystack ends with needle.
152+
*/
153+
public function endswith( $haystack, $needle ) {
154+
return substr( $haystack, -strlen( $needle ) ) === $needle;
155+
}
156+
}

‎WordPressVIPMinimum/Tests/VIP/ProperEscapingFunctionUnitTest.inc‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
3+
echo '<a href="' . esc_attr( $some_var ) . '"></a>'; // NOK.
4+
5+
echo "<a href='" . esc_attr( $some_var ) . "'></a>"; // NOK.
6+
7+
echo '<a href="' . esc_url( $some_var ) . '"></a>'; // OK.
8+
9+
echo "<a href='" . esc_url( $some_var ) . "'></a>"; // OK.
10+
11+
echo '<a title="' . esc_attr( $some_var ) . '"></a>'; // OK.
12+
13+
echo "<a title='" . esc_attr( $some_var ) . "'></a>"; // OK.
14+
15+
echo '<a title="' . esc_html( $some_var ) . '"></a>'; // NOK.
16+
17+
echo "<a title='" . esc_html( $some_var ) . "'></a>"; // NOK.
18+
19+
?>
20+
21+
<a href="<?php echo esc_attr( $some_var ); ?>">Hello</a> <!-- NOK. -->
22+
23+
<a href="" class="<?php echo esc_html( $some_var); ?>">Hey</a> <!-- NOK. -->
24+
25+
<a href="<?php esc_url( $url );?>"></a> <!-- OK. -->
26+
27+
<a title="<?php esc_attr( $url );?>"></a> <!-- OK. -->

‎WordPressVIPMinimum/Tests/VIP/ProperEscapingFunctionUnitTest.php‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<?php
2+
/**
3+
* Unit test class for WordPressVIPMinimum Coding Standard.
4+
*
5+
* @package VIPCS\WordPressVIPMinimum
6+
*/
7+
8+
namespace WordPressVIPMinimum\Tests\VIP;
9+
10+
use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
11+
12+
/**
13+
* Unit test class for the ProperEscapingFunction sniff.
14+
*
15+
* @package VIPCS\WordPressVIPMinimum
16+
*/
17+
class ProperEscapingFunctionUnitTest extends AbstractSniffUnitTest {
18+
19+
/**
20+
* Returns the lines where errors should occur.
21+
*
22+
* @return array <int line number> => <int number of errors>
23+
*/
24+
public function getErrorList() {
25+
return array(
26+
3 => 1,
27+
5 => 1,
28+
15 => 1,
29+
17 => 1,
30+
21 => 1,
31+
23 => 1,
32+
);
33+
}
34+
35+
/**
36+
* Returns the lines where warnings should occur.
37+
*
38+
* @return array <int line number> => <int number of warnings>
39+
*/
40+
public function getWarningList() {
41+
return array();
42+
}
43+
44+
} // End class.

‎ruleset_test.inc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,5 +163,7 @@ add_option( 'taxonomy_rating_' . $obj->term_id ); // Bad. Warning.
163163

164164
//wpcom_vip_load_plugin( 'disqus' ); // Bad. Warning.
165165

166+
echo "<a href='" . esc_attr( $some_var ) . "'></a>"; // NOK. Error.
167+
166168
?>
167169

‎ruleset_test.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,8 @@
4848
131 => 1,
4949
133 => 1,
5050
157 => 1,
51-
166 => 1, // Error on the end of the file. When any code is added, bounce this.
51+
166 => 1,
52+
168 => 1, // Error on the end of the file. When any code is added, bounce this.
5253
),
5354
'warnings' => array(
5455
9 => 1,

0 commit comments

Comments
 (0)