From 7f77f90030c8e52b7345d342b03fa062e255b547 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 20 Apr 2021 18:38:45 +0200 Subject: [PATCH 1/3] ProperEscapingFunction: improve "action" match precision In VIPCS 2.2.0, checking for the use of `esc_url()` for "action" HTML attributes was introduced in PR 575 in response to issue 554. As it is not inconceivable that "action" is used as a suffix for custom HTML attributes - like "data-action"- for which the value does not necessarily has to be a URL, the check for the "action" HTML attribute should make sure it is the complete attribute name and not used as a suffix for a custom attribute name. This change contains a minor refactor of the code which examines the content of the previous text string. Instead of looping over the various lists and doing a `substr()` on the same content 25 times, it will now use a regular expression to gather the necessary information to throw the right errors in one go. Includes unit tests. Fixes 669 Note: This PR removes two `private` properties which were introduced in 624/VIPCS 2.3.0 and two `public` methods. The removal of the properties is safe. The removal of the `public` methods could be considered a BC-break as these methods were `public`, though they never should have been. If so preferred, the `public` methods could be deprecated instead and remain in the code base as dead code/emptied out methods until the next major release upon which they could be removed. --- .../Security/ProperEscapingFunctionSniff.php | 78 ++++--------------- .../ProperEscapingFunctionUnitTest.inc | 6 +- 2 files changed, 16 insertions(+), 68 deletions(-) diff --git a/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php b/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php index 7c2db497..ae1d6725 100644 --- a/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php @@ -18,6 +18,13 @@ */ class ProperEscapingFunctionSniff extends Sniff { + /** + * Regular expression to match the end of HTML attributes. + * + * @var string + */ + const ATTR_END_REGEX = '`(?href|src|url|\s+action)?=(?:(?:\\\\)?["\'])?$`i'; + /** * List of escaping functions which are being tested. * @@ -49,31 +56,6 @@ class ProperEscapingFunctionSniff extends Sniff { T_NS_SEPARATOR => T_NS_SEPARATOR, ]; - /** - * List of attributes associated with url outputs. - * - * @var array - */ - private $url_attrs = [ - 'href', - 'src', - 'url', - 'action', - ]; - - /** - * List of syntaxes for inside attribute detection. - * - * @var array - */ - private $attr_endings = [ - '=', - '="', - "='", - "=\\'", - '=\\"', - ]; - /** * Returns an array of tokens this test wants to listen for. * @@ -134,57 +116,23 @@ public function process_token( $stackPtr ) { return; } - if ( $escaping_type !== 'url' && $this->attr_expects_url( $content ) ) { + if ( preg_match( self::ATTR_END_REGEX, $content, $matches ) !== 1 ) { + return; + } + + if ( $escaping_type !== 'url' && empty( $matches['attrname'] ) === false ) { $message = 'Wrong escaping function. href, src, and action attributes should be escaped by `esc_url()`, not by `%s()`.'; $this->phpcsFile->addError( $message, $stackPtr, 'hrefSrcEscUrl', $data ); return; } - if ( $escaping_type === 'html' && $this->is_html_attr( $content ) ) { + if ( $escaping_type === 'html' ) { $message = 'Wrong escaping function. HTML attributes should be escaped by `esc_attr()`, not by `%s()`.'; $this->phpcsFile->addError( $message, $stackPtr, 'htmlAttrNotByEscHTML', $data ); return; } } - /** - * Tests whether provided string ends with open attribute which expects a URL value. - * - * @param string $content Haystack in which we look for an open attribute which exects a URL value. - * - * @return bool True if string ends with open attribute which expects a URL value. - */ - public function attr_expects_url( $content ) { - $attr_expects_url = false; - foreach ( $this->url_attrs as $attr ) { - foreach ( $this->attr_endings as $ending ) { - if ( $this->endswith( $content, $attr . $ending ) === true ) { - $attr_expects_url = true; - break; - } - } - } - return $attr_expects_url; - } - - /** - * Tests whether provided string ends with open HMTL attribute. - * - * @param string $content Haystack in which we look for open HTML attribute. - * - * @return bool True if string ends with open HTML attribute. - */ - public function is_html_attr( $content ) { - $is_html_attr = false; - foreach ( $this->attr_endings as $ending ) { - if ( $this->endswith( $content, $ending ) === true ) { - $is_html_attr = true; - break; - } - } - return $is_html_attr; - } - /** * Tests whether an attribute escaping function is being used outside of an HTML tag. * diff --git a/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc b/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc index bd5523d7..e477c716 100644 --- a/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc @@ -38,9 +38,9 @@ echo 'data-param-url="' . Esc_HTML( $share_url ) . '"'; // Error. ?> -
- - + + +link From 1401543a67f01b078923f323b1aa77917f6289e1 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 22 Apr 2021 07:05:38 +0200 Subject: [PATCH 2/3] ProperEscapingFunction: fine-tune attribute regex This adds test cases with: * No space before "action" as it is at the start of the line in a multi-line text string. * A tab before "action". ... and makes minor adjustments to the regex to safeguard handling these cases correctly. --- .../Sniffs/Security/ProperEscapingFunctionSniff.php | 2 +- .../Tests/Security/ProperEscapingFunctionUnitTest.inc | 10 ++++++++++ .../Tests/Security/ProperEscapingFunctionUnitTest.php | 2 ++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php b/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php index ae1d6725..5a966352 100644 --- a/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php @@ -23,7 +23,7 @@ class ProperEscapingFunctionSniff extends Sniff { * * @var string */ - const ATTR_END_REGEX = '`(?href|src|url|\s+action)?=(?:(?:\\\\)?["\'])?$`i'; + const ATTR_END_REGEX = '`(?href|src|url|(^|\s+)action)?=(?:\\\\)?["\']*$`i'; /** * List of escaping functions which are being tested. diff --git a/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc b/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc index e477c716..35f20d1e 100644 --- a/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc +++ b/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc @@ -85,3 +85,13 @@ echo 'data-param-url="' . Esc_HTML::static_method( $share_url ) . '"'; // OK. // Not a target for this sniff (yet). printf( '', esc_attr( $content ) ); // OK. +?> + +// Making sure tabs and new lines before "action" are handled correctly. + +'; // OK. +echo ''; // Error. diff --git a/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.php b/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.php index 8db41f41..2a0e020b 100644 --- a/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.php +++ b/WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.php @@ -53,6 +53,8 @@ public function getErrorList() { 79 => 1, 80 => 1, 82 => 1, + 92 => 1, + 97 => 1, ]; } From a6151099e5f32592db9ca97459587b87a811480e Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 22 Apr 2021 07:26:00 +0200 Subject: [PATCH 3/3] ProperEscapingFunction: deprecate, don't remove ... the `public` methods and as those use the `private` properties and extending sniffs may rely on the functionality of the `public` methods, we can't removed the `private` properties yet either, so deprecating those too. --- .../Security/ProperEscapingFunctionSniff.php | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php b/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php index 5a966352..6b7a6ce1 100644 --- a/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php +++ b/WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php @@ -56,6 +56,39 @@ class ProperEscapingFunctionSniff extends Sniff { T_NS_SEPARATOR => T_NS_SEPARATOR, ]; + /** + * List of attributes associated with url outputs. + * + * @deprecated 2.3.1 Currently unused by the sniff, but needed for + * for public methods which extending sniffs may be + * relying on. + * + * @var array + */ + private $url_attrs = [ + 'href', + 'src', + 'url', + 'action', + ]; + + /** + * List of syntaxes for inside attribute detection. + * + * @deprecated 2.3.1 Currently unused by the sniff, but needed for + * for public methods which extending sniffs may be + * relying on. + * + * @var array + */ + private $attr_endings = [ + '=', + '="', + "='", + "=\\'", + '=\\"', + ]; + /** * Returns an array of tokens this test wants to listen for. * @@ -133,6 +166,48 @@ public function process_token( $stackPtr ) { } } + /** + * Tests whether provided string ends with open attribute which expects a URL value. + * + * @deprecated 2.3.1 + * + * @param string $content Haystack in which we look for an open attribute which exects a URL value. + * + * @return bool True if string ends with open attribute which expects a URL value. + */ + public function attr_expects_url( $content ) { + $attr_expects_url = false; + foreach ( $this->url_attrs as $attr ) { + foreach ( $this->attr_endings as $ending ) { + if ( $this->endswith( $content, $attr . $ending ) === true ) { + $attr_expects_url = true; + break; + } + } + } + return $attr_expects_url; + } + + /** + * Tests whether provided string ends with open HMTL attribute. + * + * @deprecated 2.3.1 + * + * @param string $content Haystack in which we look for open HTML attribute. + * + * @return bool True if string ends with open HTML attribute. + */ + public function is_html_attr( $content ) { + $is_html_attr = false; + foreach ( $this->attr_endings as $ending ) { + if ( $this->endswith( $content, $ending ) === true ) { + $is_html_attr = true; + break; + } + } + return $is_html_attr; + } + /** * Tests whether an attribute escaping function is being used outside of an HTML tag. *