Merge pull request #854 from Automattic/feature/functions-striptags-sniff-review

Author: GaryJones

WordPressVIPMinimum/Sniffs/Functions/StripTagsSniff.php

@@ -9,6 +9,7 @@
 
 namespace WordPressVIPMinimum\Sniffs\Functions;
 
+use PHPCSUtils\Utils\PassedParameters;
 use WordPressCS\WordPress\AbstractFunctionParameterSniff;
 
 /**
@@ -43,16 +44,60 @@ class StripTagsSniff extends AbstractFunctionParameterSniff {
 	 *                                in lowercase.
 	 * @param array  $parameters      Array with information about the parameters.
 	 *
-	 * @return int|void Integer stack pointer to skip forward or void to continue
-	 *                  normal file processing.
+	 * @return void
 	 */
 	public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
-		if ( count( $parameters ) === 1 ) {
-			$message = '`strip_tags()` does not strip CSS and JS in between the script and style tags. Use `wp_strip_all_tags()` to strip all tags.';
-			$this->phpcsFile->addWarning( $message, $stackPtr, 'StripTagsOneParameter' );
-		} elseif ( isset( $parameters[2] ) ) {
+		$string_param       = PassedParameters::getParameterFromStack( $parameters, 1, 'string' );
+		$allowed_tags_param = PassedParameters::getParameterFromStack( $parameters, 2, 'allowed_tags' );
+
+		if ( $string_param !== false && $allowed_tags_param === false ) {
+			$this->add_warning( $stackPtr, 'StripTagsOneParameter' );
+		} elseif ( $allowed_tags_param !== false ) {
 			$message = '`strip_tags()` does not strip CSS and JS in between the script and style tags. Use `wp_kses()` instead to allow only the HTML you need.';
 			$this->phpcsFile->addWarning( $message, $stackPtr, 'StripTagsTwoParameters' );
+		} else {
+			$this->add_warning( $stackPtr );
 		}
 	}
+
+	/**
+	 * Process the function if no parameters were found.
+	 *
+	 * @param int    $stackPtr        The position of the current token in the stack.
+	 * @param string $group_name      The name of the group which was matched.
+	 * @param string $matched_content The token content (function name) which was matched
+	 *                                in lowercase.
+	 *
+	 * @return void
+	 */
+	public function process_no_parameters( $stackPtr, $group_name, $matched_content ) {
+		$this->add_warning( $stackPtr );
+	}
+
+	/**
+	 * Process the function if it is used as a first class callable.
+	 *
+	 * @param int    $stackPtr        The position of the current token in the stack.
+	 * @param string $group_name      The name of the group which was matched.
+	 * @param string $matched_content The token content (function name) which was matched
+	 *                                in lowercase.
+	 *
+	 * @return void
+	 */
+	public function process_first_class_callable( $stackPtr, $group_name, $matched_content ) {
+		$this->add_warning( $stackPtr );
+	}
+
+	/**
+	 * Add a warning if the function is used at all.
+	 *
+	 * @param int    $stackPtr   The position of the current token in the stack.
+	 * @param string $error_code Error code to use for the warning.
+	 *
+	 * @return void
+	 */
+	private function add_warning( $stackPtr, $error_code = 'Used' ) {
+		$message = '`strip_tags()` does not strip CSS and JS in between the script and style tags. Use `wp_strip_all_tags()` to strip all tags.';
+		$this->phpcsFile->addWarning( $message, $stackPtr, $error_code );
+	}
 }

WordPressVIPMinimum/Tests/Functions/StripTagsUnitTest.inc

@@ -1,13 +1,48 @@
 <?php
 
-$string = '<script>haxx0red</script>';
-$html = '<br><a><b><i>';
+/*
+ * Not the sniff target.
+ */
+use strip_tags;
+use MyNs\{
+	function strip_tags,
+};
+
+my\ns\strip_tags($a, $b);
+$this->strip_tags($a, $b);
+$this?->strip_tags($a, $b);
+MyClass::strip_tags($a, $b);
+echo STRIP_TAGS;
+namespace\strip_tags($a, $b);
+
+// Looks like a function call, but is a PHP 8.0+ class instantiation via an attribute.
+#[Strip_tags('text')]
+function foo() {}
 
 strip_tag( 'Test', $html ); // Ok - similarly-named function.
 wp_strip_all_tags( $string ); // Ok.
 
+
+/*
+ * These should all be flagged with a warning.
+ */
 strip_tags( 'Testing' ); // Warning.
-strip_tags( 'Test', $html ); // Warning.
-strip_tags( 'Test' . ', ' . 'HTML' ); // Warning - concatenation on first parameter.
-strip_tags( 'Test, String', $html ); // Warning - comma in first parameter.
-strip_tags( $string ); // Warning.
+STRIP_TAGS( 'Test', $html ); // Warning.
+\strip_tags( 'Test' . ', ' . 'HTML', ); // Warning.
+strip_tags( 'Test, String', $html, ); // Warning.
+Strip_Tags( $string ); // Warning.
+
+// The function should always be flagged, even during live coding (missing required parameter).
+strip_tags();
+
+strip_tags(...$params); // PHP 5.6 argument unpacking.
+
+// Safeguard correct handling of function calls using PHP 8.0+ named parameters.
+strip_tags(allowed_tags: $allowed, string: $html ); // Warning.
+strip_tags(string: $html); // Warning.
+strip_tags(allowed_tags: $allowed); // Warning. Invalid function call, but that's not the concern of this sniff.
+
+\strip_tags(string: $html, allowed_tag: $allowed); // Warning (mind: deliberate typo in param name).
+strip_tags(html: $html); // Warning (mind: deliberately using incorrect param name).
+
+add_action('my_action', strip_tags(...)); // PHP 8.1 first class callable.

WordPressVIPMinimum/Tests/Functions/StripTagsUnitTest.php

@@ -32,11 +32,19 @@ public function getErrorList() {
 	 */
 	public function getWarningList() {
 		return [
-			9  => 1,
-			10 => 1,
-			11 => 1,
-			12 => 1,
-			13 => 1,
+			29 => 1,
+			30 => 1,
+			31 => 1,
+			32 => 1,
+			33 => 1,
+			36 => 1,
+			38 => 1,
+			41 => 1,
+			42 => 1,
+			43 => 1,
+			45 => 1,
+			46 => 1,
+			48 => 1,
 		];
 	}
 }