Skip to content

Commit fde9c00

Browse files
david-bindadavid-binda
committed
Adding VIP.ProperEscapingFunction sniff for catching instances where esc_url is not used for escaping href or src attribute, and when esc_html is used for escaping HTML attriutes.
1 parent 6e16ac4 commit fde9c00

File tree

5 files changed

+227
-1
lines changed

5 files changed

+227
-1
lines changed

‎WordPressVIPMinimum/Sniffs/VIP/ProperEscapingFunctionSniff.php‎

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

‎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)