XSS.EscapeOutput sniff: Update the earlier work done by @david-binda

* Document the setting of the PHP ini value in the travis build script. * Set the branch this is tested with to `2.9` as PHPCS 3.x has come out since the original PR was pulled and is now `master`. * Remove the `short_open_tag_enabled` property as the value of this is only used once, so the check can just as efficiently be done in the appropriate place. * Adjust the regex delimiter to be backticks. * Pass `$data` as an array as that's expected by PHPCS. * Remove the throwing of a warning when short open echo tags _"might"_ be found. This warning was also added to the upstream `DisallowShortOpenTag` sniff in PHPCS 2.9.0 and would therefore cause duplicate warnings. See squizlabs/PHP_CodeSniffer/pull/1400. * Adjust the unit test line numbers (merge conflict artefact) Also: adds one missing `@since` tag which is unrelated to this PR, but in the same file and make one very long line a little more readable.
WordPress · Jun 30, 2017 · f930afb · f930afb
1 parent 04d997c
commit f930afb
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 41 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -37,7 +37,7 @@ matrix:
       env: PHPCS_BRANCH=2.9
     # Test PHP 5.3 with short_open_tags set to On (is Off by default)
     - php: 5.3
-      env: PHPCS_BRANCH=master SHORT_OPEN_TAGS=true
+      env: PHPCS_BRANCH=2.9 SHORT_OPEN_TAGS=true
   allow_failures:
     # Allow failures for unstable builds.
     - php: nightly
@@ -55,6 +55,7 @@ before_install:
     # test suite is currently not compatible with PHPUnit 6.x.
     # Fixed at a very specific PHPUnit version which is also compatible with HHVM.
     - if [[ ${TRAVIS_PHP_VERSION:0:2} != "5." ]]; then wget -P $PHPUNIT_DIR https://phar.phpunit.de/phpunit-5.7.17.phar && chmod +x $PHPUNIT_DIR/phpunit-5.7.17.phar; fi
+    # Selectively adjust the ini values for the build image to test ini value dependent sniff features.
     - if [[ "$SHORT_OPEN_TAGS" == "true" ]]; then echo "short_open_tag = On" >> ~/.phpenv/versions/$(phpenv version-name)/etc/php.ini; fi
 
 script:

diff --git a/WordPress/Sniffs/XSS/EscapeOutputSniff.php b/WordPress/Sniffs/XSS/EscapeOutputSniff.php
@@ -15,9 +15,11 @@
  * @package WPCS\WordPressCodingStandards
  *
  * @since   2013-06-11
- * @since   0.4.0 This class now extends WordPress_Sniff.
- * @since   0.5.0 The various function list properties which used to be contained in this class
- *                have been moved to the WordPress_Sniff parent class.
+ * @since   0.4.0  This class now extends WordPress_Sniff.
+ * @since   0.5.0  The various function list properties which used to be contained in this class
+ *                 have been moved to the WordPress_Sniff parent class.
+ * @since   0.12.0 This sniff will now also check for output escaping when using shorthand
+ *                 echo tags `<?=`.
  */
 class WordPress_Sniffs_XSS_EscapeOutputSniff extends WordPress_Sniff {
 
@@ -93,6 +95,8 @@ class WordPress_Sniffs_XSS_EscapeOutputSniff extends WordPress_Sniff {
 	/**
 	 * List of names of the tokens representing PHP magic constants.
 	 *
+	 * @since 0.10.0
+	 *
 	 * @var array
 	 */
 	private $magic_constant_tokens = array(
@@ -144,25 +148,12 @@ class WordPress_Sniffs_XSS_EscapeOutputSniff extends WordPress_Sniff {
 		'T_END_NOWDOC'               => true,
 	);
 
-	/**
-	 * Status of short_open_tag feature
-	 *
-	 * @since 0.11.0
-	 *
-	 * @var bool
-	 */
-	private $short_open_tag_enabled = true;
-
 	/**
 	 * Returns an array of tokens this test wants to listen for.
 	 *
 	 * @return array
 	 */
 	public function register() {
-		// Check whether short_open_tag is disabled on PHP version < 5.4 (it's enabled by default in later versions).
-		if ( PHP_VERSION_ID < 50400 && false === (bool) ini_get( 'short_open_tag' ) ) {
-			$this->short_open_tag_enabled = false;
-		}
 
 		$tokens = array(
 			T_ECHO,
@@ -173,10 +164,15 @@ public function register() {
 		);
 
 		/*
-		 * In case short_open_tag is turned off, we can attempt to regex T_INLINE_HTML
-		 * which is how short open tags are being handled in that case.
+		 * Check whether short open echo tags are disabled and if so, register the
+		 * T_INLINE_HTML token which is how short open tags are being handled in that case.
+		 *
+		 * In PHP < 5.4, support for short open echo tags depended on whether the
+		 * `short_open_tag` ini directive was set to `true`.
+		 * For PHP >= 5.4, the `short_open_tag` no longer affects the short open
+		 * echo tags and these are now always enabled.
 		 */
-		if ( false === $this->short_open_tag_enabled ) {
+		if ( PHP_VERSION_ID < 50400 && false === (bool) ini_get( 'short_open_tag' ) ) {
 			$tokens[] = T_INLINE_HTML;
 		}
 		return $tokens;
@@ -214,23 +210,23 @@ public function process_token( $stackPtr ) {
 			if ( in_array( $function, array( 'trigger_error', 'user_error' ), true ) ) {
 				$end_of_statement = $this->phpcsFile->findEndOfStatement( $open_paren + 1 );
 			}
-		} else if ( false === $this->short_open_tag_enabled && T_INLINE_HTML === $this->tokens[ $stackPtr ]['code'] ) {
-			// Skip if no PHP short_open_tag is in the string.
+		} elseif ( T_INLINE_HTML === $this->tokens[ $stackPtr ]['code'] ) {
+			// Skip if no PHP short_open_tag is found in the string.
 			if ( false === strpos( $this->tokens[ $stackPtr ]['content'], '<?=' ) ) {
 				return;
 			}
 
-			// Report on what very likely is a PHP short open tag outputting variable.
-			if ( preg_match( '/\<\?\=[\s]*(\$[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*(?:(?:->\S+|\[[^\]]+\]))*)[\s]*;?[\s]*\?\>/', $this->tokens[ $stackPtr ]['content'], $matches ) ) {
-				$this->phpcsFile->addError( 'Expected next thing to be an escaping function, not %s.', $stackPtr, 'OutputNotEscaped', $matches[1] );
+			// Report on what is very likely a PHP short open echo tag outputting a variable.
+			if ( preg_match( '`\<\?\=[\s]*(\$[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*(?:(?:->\S+|\[[^\]]+\]))*)[\s]*;?[\s]*\?\>`', $this->tokens[ $stackPtr ]['content'], $matches ) > 0 ) {
+				$this->phpcsFile->addError(
+					'Expected next thing to be an escaping function, not %s.',
+					$stackPtr,
+					'OutputNotEscaped',
+					array( $matches[1] )
+				);
 				return;
 			}
 
-			// Throw warning in case the T_INLINE_HTML looks like a open_short_tag.
-			if ( false !== strpos( $this->tokens[ $stackPtr ]['content'], '<?=' ) ) {
-				$this->phpcsFile->addWarning( 'Possible use of PHP short open tag ( "<?=" ) detected. Needs manual inspection.', $stackPtr, 'PossibleShortOpenTag' );
-				return;
-			}
 			return;
 		}
 
@@ -240,7 +236,12 @@ public function process_token( $stackPtr ) {
 		}
 
 		if ( isset( $end_of_statement, $this->unsafePrintingFunctions[ $function ] ) ) {
-			$error = $this->phpcsFile->addError( "Expected next thing to be an escaping function (like %s), not '%s'", $stackPtr, 'UnsafePrintingFunction', array( $this->unsafePrintingFunctions[ $function ], $function ) );
+			$error = $this->phpcsFile->addError(
+				"Expected next thing to be an escaping function (like %s), not '%s'",
+				$stackPtr,
+				'UnsafePrintingFunction',
+				array( $this->unsafePrintingFunctions[ $function ], $function )
+			);
 
 			// If the error was reported, don't bother checking the function's arguments.
 			if ( $error ) {

diff --git a/WordPress/Tests/XSS/EscapeOutputUnitTest.php b/WordPress/Tests/XSS/EscapeOutputUnitTest.php
@@ -64,9 +64,9 @@ public function getErrorList() {
 			206 => 1,
 			207 => 1,
 			212 => ( PHP_VERSION_ID < 50300 ) ? 1 : 0, // PHPCS on PHP 5.2 does not recognize T_NOWDOC.
-			210 => 1,
-			212 => 1,
-			213 => 1,
+			223 => 1,
+			225 => 1,
+			226 => 1,
 		);
 
 	} // end getErrorList()
@@ -77,14 +77,8 @@ public function getErrorList() {
 	 * @return array <int line number> => <int number of warnings>
 	 */
 	public function getWarningList() {
-		$list = array();
+		return array();
 
-		// Adding Warning which is triggerred in case open_short_tag is set to Off.
-		if ( PHP_VERSION_ID < 50400 && false === (bool) ini_get( 'short_open_tag' ) ) {
-			$list[211] = 1;
-		}
-
-		return $list;
 	}
 
 } // End class.